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

111–120 of 178 posts

Re: Pre-commit hooks are broken

#111

Post by Phlebsy »

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.

Sure, if the warning levels are poorly tuned I might configure my LSP to ignore everything and loosen the enforcement in the build steps until I'm ready to self review. Something I can't stand with Typescript for example is when the local development server has as strict rules as the production builds. There's no good reason to completely block doing anything useful whatsoever just because of an unused variable, unreachable code, or because a test that is never going to get committed dared to have an 'any' type.

Re: Pre-commit hooks are broken

#112

Post by hxtk »

ltbarcly3 wrote: post #103
bigstrat2003 wrote: post #100

Earlier quoted context omitted.

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.

It's how code is written in Google (including their open-source products like AOSP and Chromium), the ffmpeg project, the Linux Kernel, Git, Docker, the Go compiler, Kubernetes, Bitcoin, etc, and it's how things are done at my workplace.

I'm surprised by how confident you are that things simply aren't done this way considering the number of high-profile users of workflows where the commit history is expected to tell a story of how the software evolved over time.

Re: Pre-commit hooks are broken

#113

Post by jghn »

Phlebsy wrote: post #111
ncgl wrote: post #105

Earlier quoted context omitted.

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

Sure, if the warning levels are poorly tuned I might configure my LSP to ignore everything and loosen the enforcement in the build steps until I'm ready to self review. Something I can't stand with Typescript for example is when the local development server has as strict rules as the production builds. There's no good reason to completely block doing anything useful whatsoever just because of an unused variable, unre…

An example I like to use are groups that put their autofmratter into a pre-commit. Why should I be held to the formatting rules for code before I send my code to anyone?

I'm particular about formatting, and it doesn't always match group norms. So I'll reformat things to my preferred style while working locally, and then reformat before pushing. However I may have several commits locally that then ge curated out of existence prior to pushing.

Re: Pre-commit hooks are broken

#114

Post by Marsymars »

tkzed49 wrote: post #72
eru wrote: post #12

Earlier quoted context omitted.

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 c…

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

I'm a fan of pre-commit/push hooks, but they have to be fast. At our suite of pre-commit hooks are <50ms and pre-push hooks are <5s. They get regularly reviewed for performance, and if anything can't be made faster, slow pre-commit hooks will get ejected to pre-push, and slow pre-push hooks will get ejected to the regular CI suite.

Re: Pre-commit hooks are broken

#115

Post by ardeaver »

conradludgate wrote: post #20

I've worked in several projects where running the tests locally automatically install pre-commit hooks and I've wanted to commit warcrimes because of it. Don't do that, just dont.

At my last job, we ran all of our tests, linting/formatting, etc. through pre-commit hooks. It was apparently a historical relic of a time where five developers wanted to push directly to master without having to configure CI.

I too was about to become a war criminal.

Re: Pre-commit hooks are broken

#116

Post by mcv »

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…

There's a weird thing happening on my current project. Sometimes I merge main into my branch and it fails. What fails is the pre-commit hook on the merge commit. Changes in main fail the linting checks in the pre-commit hook. But they still ended up in main, somehow. So the checks on the PR are apparently not as strict as the checks on the pre-commit hook. As a result, many developers have gotten used to committing with `--no-verify`, at which point, what is even the point of a pre-commit hook?

And sometimes I just want to commit work in progress so I can more easily backtrack my changes. These checks are better on pre-push, and definitely should be on the PR pipeline, otherwise they can and will be skipped.

Anyway, thanks for giving me some ammo to make this case.

Re: Pre-commit hooks are broken

#117

Post by Izkata »

ltbarcly3 wrote: post #99
Izkata wrote: post #97

Earlier quoted context omitted.

> 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 brea…

It is not possible to manually document everything down to individual lines of code. You'll drive yourself crazy trying to do so (and good luck getting anyone to look at that massive mess), and that's not even counting how documentation easily falls out of date. Meanwhile, we have "git blame" designed to do exactly that with almost no effort - just make good commit messages while the context is in your head.

CI also doesn't necessarily help here - you have to have tests for all possible edge cases committed from day one for it to prevent these situations. It may be a month or a year or several years later before you hit one of the weird cases no one thought about.

Calling svn part of the problem is also kind of backwards - it has no bearing on the code quality itself, but I brought it up because it was otherwise forcing good practice because it doesn't allow you to erase context that may be useful later.

Over the time I've been here we've migrated from Bugzilla to Fogbugz to Jira, from an internal wiki to ReadTheDocs to Confluence, and some of these hundreds of repos we manage started in cvs, not svn, and are now slowly being migrated to git. Guess what? The cvs->svn->git migrations are the only ones that didn't lose any data. None of the Bugzilla cases still exist and only a very small number were migrated from FogBugz to Jira. Some of the internal wiki was migrated directly to Confluence (and lost all formatting and internal links in the process), but ReadTheDocs are all gone. Commit messages are really the only thing you can actually rely on.

Re: Pre-commit hooks are broken

#118

Post by jghn »

tharkun__ wrote: post #101
oxryly1 wrote: post #71

Earlier quoted context omitted.

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 kee…

My philosophy is that once a PR is open, that's the point at which people should no longer feel free to treat their branch as their own. Even in groups that squash commits, it should still preserve the aggregate commit messages.

But until that PR is open? Totally with you. There is no obligation to "preserve history" up until that point.

Re: Pre-commit hooks are broken

#119

Post by jghn »

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…

There are two halves here. Up until the PR is open, the author should feel free to have 20+ "wip" commits. Or in my case "checkpoint". However, it is also up to the author to curate their commits before pushing it and opening the PR.

So when I open a Pr, I'll have a branch with a gajillion useless commits, and then curate them down to a logical set of commits with appropriate commit messages. Usually this is a single commit, but if I want to highlight some specific pieces as being separable for a reviewer, it'll be multiple commits.

The key point here is that none of those commits exist until just before I make my final push prior to a PR.

Re: Pre-commit hooks are broken

#120

Post by jghn »

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…

> No development proceeds that way,

I do this. Also I do not spend 80% of my time doing it. It's not hard, nor is it time consuming.

Post reply on HN