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

101–110 of 178 posts

Re: Pre-commit hooks are broken

#101

Post by tharkun__ »

oxryly1 wrote: post #71
Mic92 wrote: post #3

Earlier quoted context omitted.

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?

My branch is mine. Don't tell me what I can or can't do. I push WIP stuff all the time, to share code with others for discussion, to get the build to run in parallel while I keep working or just at the end of the day. I freely amend and will squashed before merging (we only allow a single commit per branch to go to master).

If I or someone else bases something off anything but master that's on them to rebased and keep up to date.

Re: Pre-commit hooks are broken

#102

Post by Groxx »

Literally the only pre-commit hook I've ever "liked" has been one to look for files over ~1/2MB, and error with a message describing how to bypass the check (env var). It stops the mistake at the easiest-to-identify point, and helped teach a lot of people about how to set gitignore or git-attributes correctly when it is most relevant and understandable. Every single other one has been a massive pain at some point...

... but even that one took several rounds of fiddling and complexifying to get it to behave correctly (e.g. merging commits with already-committed bypassed binaries should be allowed. how do you detect that? did you know to check for that scenario when building the hook? someone's gonna get bitten by it, and there are dozens of these cases).

So yea. Agreed. Do not use pre-commit hooks, they're far more trouble than they seem, and the failure modes / surprises are awful and can be quite hard to figure out to laymen who are experiencing it.

(much of this is true for all git hooks imo)

Re: Pre-commit hooks are broken

#103

Post by ltbarcly3 »

bigstrat2003 wrote: post #100
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.

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.

This is not how code is actually written.

Re: Pre-commit hooks are broken

#104

Post by koolba »

ltbarcly3 wrote: post #96
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. 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…

> 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.

If you’re working on something and a piece of it is clearly self contained, you commit it and move on.

> 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?

You can work however you like. But when it’s time to ask someone else to review your work, the onus is on you to clean it up to simplify review. Otherwise you’re saying your time is more valuable than the reviewer’s.

Re: Pre-commit hooks are broken

#105

Post by ncgl »

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.

Would you add type: ignore to all the files too?

My coworker did that the other day and I'm deciding how to respond.

Re: Pre-commit hooks are broken

#106

Post by temporallobe »

I don’t really like pre-commit hooks, but I do think that git-secrets is a very useful one since once a secret is in the commit history, it’s a hassle (though not impossible) to remove it. All other issues can and should be caught early as an optionally blocking step in a CI/CD pipeline build.

Re: Pre-commit hooks are broken

#107

Post by jghn »

ncgl wrote: post #105
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.

Would you add type: ignore to all the files too? My coworker did that the other day and I'm deciding how to respond.

Not if I push my branch it to origin. But until I do that, it's none of your concern if I do or don't. Once it gets thrown over the wall to my colleagues and/or the general public, that's the point where I should be conforming to repo norms. Not before then.

Re: Pre-commit hooks are broken

#108

Post by jghn »

benrutter wrote: post #92
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.

Yes, big fan of enforcing standards via CI/CD workflows. Any rules a group wishes to be met should be in there. As long as someone meets those rules by the time they open a PR, I don't care how they get there.

Re: Pre-commit hooks are broken

#109

Post by nirvdrum »

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.

> Why do you care about the history of a branch?

Presumably, a branch is a logical segment of work. Otherwise, just push directly master/trunk/HEAD. It's what people did for a long time with CVS and arguably worked to some extent. Using merge commits is pretty common and, as such, that branch will get merged into the trunk. Being able to understand that branch in isolation is something I've found helpful in understanding the software as a whole.

> Caring about the history of a branch is weird, I think your approach is just not compatible with how people work.

I get you disagree with me, but you could be less dismissive about it. Work however you want -- I'm certainly not stopping you. I just don't your productivity to come at the expense of mine. And, I offered up other potential (and IMHO, superior) solutions from both developer and system tools.

I suppose what type of project you're working on matters. The "treat git like a versioned zip file" using squashed merges works reasonably well for SaaS applications because you rarely need to roll anything back. However, I've found a logically structured history has been indispensable when working on long-lived projects, particularly in open source. It's how I'm able to dig into a 25 year old OSS tool and be reasonably productive with.

To the point I think you're making: sure, I care what changed, and I can do that with `diff`. But, more often if I'm looking at SCM history I'm trying to learn why a change was made. Some of that can be inferred by seeing what other changes were made at the same time. That context can be explicitly provided with commit messages that explain why a change was made.

Calling it incompatible with how people work is a pretty bold claim, given the practice of squash merging loads of mini commits is a pretty recent development. Maybe that's how your team works and if it works for you, great. But, having logically separate commits isn't some niche development practice. Optimizing for writes could be useful for a startup. A lot of real world software requires being easy to maintain and a good SCM history shines there.

All of that is rather orthogonal to the point I was trying to add to the discussion. We have better tools at our disposal than running `git commit` every 15 minutes.

Re: Pre-commit hooks are broken

#110

Post by nirvdrum »

ltbarcly3 wrote: post #96
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. 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…

> 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.

It's not really hand curation if you're deliberate about it from the get-go. It's certainly not eating up 80% of anyone's time.

Structuring code and writing useful commits a skill to develop, just like writing meaningful tests. As a first step, use `git add -p` instead of `git add .` or `git commit -a`. As an analog, many junior devs will just test everything, even stuff that doesn't make a lot of sense, and then jumble them all together. It takes practice to learn how to better structure that stuff and it isn't done by writing a ton of tests and then curating them after the fact.

> 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?

Your personal productivity should only be one consideration. The long-term health of the project (i.e., maintenance) and the impact on other people's efficiency also must be considered. And efficiency isn't limited to how quickly features ship. Someone who ships fast but makes it much harder to debug issues isn't a top performer. At least, in my experience. I'd imagine it's team, company, and segment-dependent. For OSS projects with many part-time contributors, that history becomes really important because you may not have the future ability to ask someone why they did something a particular way.

Post reply on HN