Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Pre-commit hooks are broken

jyn.dev

Post reply on HN
Print view View original

71–80 of 178 posts

Re: Pre-commit hooks are broken

#71

Post by oxryly1 »

Mic92 wrote: post #3
nrclark wrote: post #2

This was a really interesting read. I'd highly recommend it for anybody who's setting up (or currently maintains) a pre-commit workflow for their developers. I want to add one other note: in any large organization, some developers will use tools in ways nobody can predict. This includes Git. Don't try to force any particular workflow, including mandatory or automatically-enabled hooks. Instead, put what you want in a…

I can second that. If there are multiple commits: https://github.com/tummychow/git-absorb is handy to add formatting changes into the right commit after commits already happened.

It looks like git absorb rewrites history. Doesn’t that break your previously pushed branch?

Re: Pre-commit hooks are broken

#72

Post by tkzed49 »

eru wrote: post #12
tkzed49 wrote: post #4

Thank you. I don't need to "fix" a commit before it ends up on a remote branch. Sometimes I expect a commit to pass checks and sometimes I don't. Frankly, don't even run pre-push hooks. Just run the checks in CI when I push. You'd better be doing that anyway before I'm allowed to push to a production branch, so stop breaking my git workflows and save me the time of running duplicate checks locally. Also, if most deve…

Pre-commit and pre-push hooks are something developers can voluntarily add (or enable) to shorten the latency until they get feedback: instead of the CI rejecting their PR, they can (optionally!) get a local message about it. Otherwise, I agree, your project can not rely on any checks running on the dev machine with git.

Appreciate the perspective. I've worked on projects where hooks are auto-configured, and pre-commit is just never something that's going to agree with me.

I prefer to be able to push instantly and get feedback async, because by the time I've decided I'm done with a change, I've already run the tests for it. And like I said, my editor is applying formatting and lints, so those fail more rarely.

But, if your pre-push checks are fast (rather than ~minutes), I can see the utility! It sucks to get an async failure for feedback that can be delivered quickly.

Re: Pre-commit hooks are broken

#73

Post by geon »

cluckindan wrote: post #68
geon wrote: post #66

Earlier quoted context omitted.

The master branch never gets merged, so it is linear. Finding a bug is very simple with bisect. All commits are atomic, so the failing commit clearly shows the bug. If you want to keep track of what commits belongs to a certain pr, you can still have an empty merge commit at the end of the rebase. Gitlab will add that for you automatically. The ”hasty conflict resolution ” makes a broken merge waaaay harder to fix th…

Linear history is nice, but it is lacking the conflict resolutions. They are never committed, and neither are the ”fix rebase” instances. Having a ”fix broken merge” commit makes it explicit that there was an issue that was fixed. Rebase sometimes seems like an attempt at saving face.

That’s the whole point. You do it properly, so there IS no conflict.

Re: Pre-commit hooks are broken

#76

Post by eru »

normie3000 wrote: post #15
eru wrote: post #11

Earlier quoted context omitted.

> This includes Git. Don't try to force any particular workflow, including mandatory or automatically-enabled hooks. And with git, you can even make anything that happens on the dev machines mandatory. Anything you want to be mandatory needs to go into your CI. Pre-commit and pre-push hooks are just there to lower CI churn, not to guarantee anything. (With the exception of people accidentally pushing secrets. The CI…

> with git, you can even make anything that happens on the dev machines mandatory s/can/can't?

Yes, indeed.

Re: Pre-commit hooks are broken

#77

Post by p_wood »

Ferret7446 wrote: post #21
nine_k wrote: post #7

A bit less enraged: pre-commit hooks should be pure functions. They must not mutate the files being committed. At best, they should generate a report. At worst, they could reject a commit (e.g. if it contains a private key file included by mistake).

In my experience pre-commit hooks are most often used to generate a starting commit message. To put it more bluntly, pre-commit hooks are pre-commit hooks, exactly what it says on the tin. Not linting hooks or checking hooks or content filters. Depending on what exactly you want to do, they may or may not be the best tool for the job. To put it even more bluntly, if you are trying to enforce proper formatting, pre-co…

> In my experience pre-commit hooks are most often used to generate a starting commit message.

The `prepare-commit-msg` hook is a better place to do that as it gives the hook some context about the commit (is the user amending an existing commit etc.)

> To put it even more bluntly, if you are trying to enforce proper formatting, pre-commit hooks are absolutely the wrong tool for the job, as hooks are trivially bypassable, and not shared when cloning a repo, by design.

They aren't a substitute for server post-receive hooks but they do help avoid having pushes rejected by the server.

Re: Pre-commit hooks are broken

#78

Post by skydhash »

seba_dos1 wrote: post #63
nacozarina wrote: post #27

Earlier quoted context omitted.

really; keep reading about all the problems ppl have “every time I rebase” and I wonder what tomfoolery they’re really up to

Unlike some other common operations that can be easily cargo-culted, rebasing is somewhat hard to do correctly when you don't understand git, so people who don't understand git get antagonistic towards it.

Rebasing is basically working at the meta layer, when you are editing patches instead of the code that is being versionned. And due to that, it requires good understanding of the VCS.

Too often, merges is only understood as bring the changes from there to here, it may be useful especially if you have release candidates branches and hotfixes. And you want to keep a trave of that process. But I much prefer rebasing and/or squashing PR onto the main branch.

Re: Pre-commit hooks are broken

#79

Post by andrewaylett »

conradludgate wrote: post #19
lemonlime227 wrote: post #14

To bring up jujutsu, `jj fix` ( https://docs.jj-vcs.dev/latest/cli-reference/#jj-fix ) is a more refined way of ensuring formatting in commits. It runs a formatting command with the diff in stdin and uses the results printed to stdout. It can simplify merges and rebases history to ensure all your commits remain formatted (so if you enable a new formatting option, it can remove the need for a special format/style fix…

The downside currently (although I've been assured this will be fixed one day) is that it doesn't support running static analysis over each commit you want to fix. My git rebase workflow often involves running `git rebase -x "cargo clippy -- --deny=warnings"`. This needs a full checkout to work and not just a single file input

I keep a couple of jj aliases that apply the `pre-commit` tool to a commit or a tree of commits:

https://github.com/andrewaylett/dotfiles/blob/7a79cf166d1e7b...

What I really want is some way within jj to keep track of which commits have been checked and which are currently failing, so I can template it into log lines.

Post reply on HN