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

91–100 of 178 posts

Re: Pre-commit hooks are broken

#91

Post by tomjakubowski »

PunchyHamster wrote: post #46
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 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. you will save your org a lot of pain if you do force it, same as when you do force a formatting style rather than letting anyone do what they please. You can discuss to change it if some…

It's a good thing you can't force it, because `git commit -n` exists. (And besides, management of the `.git/hooks` directory is done locally. You can always just wipe that directory of any noxious hooks.)

I can accept (but still often skip, with `git push -n`) a time-consuming pre-push hook, but a time-consuming and flaky pre-commit hook is totally unacceptable to my workflows and I will always find a way to work around it. Like everyone else is saying, if you want to enforce some rule on the codebase then do it in CI and block merges on it.

Re: Pre-commit hooks are broken

#92

Post by benrutter »

jghn wrote: post #80

On any project where pre-commit hooks are used, the first thing I do is disable them. What I do when the code is on my side of the line isn't your business.

I agree on the other side of the fence! I quite like precommit when I use it, but I've never imposed it on any of my projects. Some people use commits sporadically then squash down- I really don't feel comfortable breaking someone's personal workflow to that degree.

I almost always have a "this cicd must pass to merge" job, that includes linting etc, and then use squash commits exclusively when merging.

Re: Pre-commit hooks are broken

#93

Post by ltbarcly3 »

nirvdrum wrote: post #86
ltbarcly3 wrote: post #85

Earlier quoted context omitted.

pre-commit is just a bad way to do this. 99.9% of my commits won't pass CI. I don't care. I run `git wip` which is an alias for `git commit -am "WIP"` about every 15 minutes during the day. Whenever things are in a running state. I often go back through this history on my branch to undo changes or revisit decisions, especially during refactors, especially when leveraging AI. When the most work you can lose is about 1…

If you’re just committing for your own sake, that workflow sounds productive. I’ve been asked to review PRs with 20+ commits with a “wip” or “.” commit message with the argument: “it’ll be squash merged, so who cares!”. I’m sure that works well for the author, but it’s not great for the reviewer. Breaking change sets up into smaller logical chunks really helps with comprehension. I’m not generally a fan of people bei…

Why do you care about the history of a branch? Just look at the diff. Caring about the history of a branch is weird, I think your approach is just not compatible with how people work.

Re: Pre-commit hooks are broken

#94

Post by yearolinuxdsktp »

ltbarcly3 wrote: post #85
Spivak wrote: post #83

Earlier quoted context omitted.

pre-commit is a convenience for the developer to gain confidence that pre-flight checks in CI will pass. I've found trying to make them automatic just leads to pain when they interact with any non-trivial git feature and don't handle edge cases. I've been much much happier just having a little project specific script I run when I want to do formatting/linting.

pre-commit is just a bad way to do this. 99.9% of my commits won't pass CI. I don't care. I run `git wip` which is an alias for `git commit -am "WIP"` about every 15 minutes during the day. Whenever things are in a running state. I often go back through this history on my branch to undo changes or revisit decisions, especially during refactors, especially when leveraging AI. When the most work you can lose is about 1…

This is why I appreciate JetBrains IDEs having a local history tracked automatically. It helps go back instead of relying on frequent commits.

Re: Pre-commit hooks are broken

#95

Post by koolba »

ltbarcly3 wrote: post #93
nirvdrum wrote: post #86

Earlier quoted context omitted.

If you’re just committing for your own sake, that workflow sounds productive. I’ve been asked to review PRs with 20+ commits with a “wip” or “.” commit message with the argument: “it’ll be squash merged, so who cares!”. I’m sure that works well for the author, but it’s not great for the reviewer. Breaking change sets up into smaller logical chunks really helps with comprehension. I’m not generally a fan of people bei…

Why do you care about the history of a branch? Just look at the diff. Caring about the history of a branch is weird, I think your approach is just not compatible with how people work.

A well laid out history of logical changes makes reviewing complicated change sets easier. Rather than one giant wall of changes, you see a series of independent, self contained, changes that can be reviewed on their own.

Having 25 meaningless “wip” commits does not help with that. It’s fine when something is indeed a work in progress. But once it’s ready for review it should be presented as a series of cleaned up changes.

If it is indeed one giant ball of mud, then it should be presented as such. But more often than not, that just shows a lack of discipline on the part of the creator. Variable renames, whitespace changes, and other cosmetic things can be skipped over to focus on the meat of the PR.

From my own experience, people who work in open source and have been on the review side of large PRs understand this the best.

Really the goal is to make things as easy as possible for the reviewer. The simpler the reviews process, the less reviewer time you’re wasting.

Re: Pre-commit hooks are broken

#96

Post by ltbarcly3 »

koolba wrote: post #95
ltbarcly3 wrote: post #93

Earlier quoted context omitted.

Why do you care about the history of a branch? Just look at the diff. Caring about the history of a branch is weird, I think your approach is just not compatible with how people work.

A well laid out history of logical changes makes reviewing complicated change sets easier. Rather than one giant wall of changes, you see a series of independent, self contained, changes that can be reviewed on their own. Having 25 meaningless “wip” commits does not help with that. It’s fine when something is indeed a work in progress. But once it’s ready for review it should be presented as a series of cleaned up ch…

> A well laid out history of logical changes makes reviewing complicated change sets easier. Rather than one giant wall of changes, you see a series of independent, self contained, changes that can be reviewed on their own.

But this would require hand curation? No development proceeds that way, or if it does then I would question whether the person is spending 80% of their day curating PRs unnecessarily.

I think you must be kind of senior and you can get away with just insisting that other people be less efficient and work in a weird way so you can feel more comfortable?

Re: Pre-commit hooks are broken

#97

Post by Izkata »

koolba wrote: post #95
ltbarcly3 wrote: post #93

Earlier quoted context omitted.

Why do you care about the history of a branch? Just look at the diff. Caring about the history of a branch is weird, I think your approach is just not compatible with how people work.

A well laid out history of logical changes makes reviewing complicated change sets easier. Rather than one giant wall of changes, you see a series of independent, self contained, changes that can be reviewed on their own. Having 25 meaningless “wip” commits does not help with that. It’s fine when something is indeed a work in progress. But once it’s ready for review it should be presented as a series of cleaned up ch…

> A well laid out history of logical changes makes reviewing complicated change sets easier.

I've been on a maintenance team for years and it's also been a massive help here, in our svn repos where squashing isn't possible. Those intermediate commits with good messages are the only context you get years down the line when the original developers are gone or don't remember reasons for something, and have been a massive help so many times.

I'm fine with manual squashing to clean up those WIP commits, but a blind squash-merge should never be done. It throws away too much for no good reason.

For one quick example, code linting/formatting should always be a separate commit. A couple times I've seen those introduce bugs, and since it wasn't squashed it was trivial to see what should have happened.

Re: Pre-commit hooks are broken

#98

Post by OJFord »

rkomorn wrote: post #42
OJFord wrote: post #37

Earlier quoted context omitted.

Any subsequent commits and the branch are inherently rebased on the squashed commit. Rebasing is kind of a short hand for cherry-picking, fixing up, rewording, squashing, dropping, etc. because these things don't make sense in isolation.

I guess my point is that I disagree that rebasing should be shorthand for all these things that aren't rebasing.

Well rebasing is exactly equivalent to moving the branch and then cherry-picking, and the others are among the commands available in rebase --interactive.

Re: Pre-commit hooks are broken

#99

Post by ltbarcly3 »

Izkata wrote: post #97
koolba wrote: post #95

Earlier quoted context omitted.

A well laid out history of logical changes makes reviewing complicated change sets easier. Rather than one giant wall of changes, you see a series of independent, self contained, changes that can be reviewed on their own. Having 25 meaningless “wip” commits does not help with that. It’s fine when something is indeed a work in progress. But once it’s ready for review it should be presented as a series of cleaned up ch…

> A well laid out history of logical changes makes reviewing complicated change sets easier. I've been on a maintenance team for years and it's also been a massive help here, in our svn repos where squashing isn't possible. Those intermediate commits with good messages are the only context you get years down the line when the original developers are gone or don't remember reasons for something, and have been a massiv…

I agree, in a job where you have no documentation and no CI, and are working on something almost as old or older than you with ancient abandoned tools like svn that stopped being relevant 20 years ago, and in a fundamentally dysfunctional company/organization that hasn't bothered to move off of dead/dying tools in the last 20 years, then you just desperately grab at anything you can possibly find to try to avoid breaking things. But there are far better solutions to all of the problems you are mentioning than trying to make people create little mini feature commits on their way to a feature.

Re: Pre-commit hooks are broken

#100

Post by bigstrat2003 »

ltbarcly3 wrote: post #93
nirvdrum wrote: post #86

Earlier quoted context omitted.

If you’re just committing for your own sake, that workflow sounds productive. I’ve been asked to review PRs with 20+ commits with a “wip” or “.” commit message with the argument: “it’ll be squash merged, so who cares!”. I’m sure that works well for the author, but it’s not great for the reviewer. Breaking change sets up into smaller logical chunks really helps with comprehension. I’m not generally a fan of people bei…

Why do you care about the history of a branch? Just look at the diff. Caring about the history of a branch is weird, I think your approach is just not compatible with how people work.

On the contrary, it seems to me that it is your approach which is incompatible with others. I'm not the same person you were replying to but I want the history of a branch to be coherent, not a hot mess of meaningless commits. I do my best to maintain my branches such that they can be merged without squashing, that way it reflects the actual history of how the code was written.
Post reply on HN