Hacker News Reader

Comment by nrclark | Hacker News Reader

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 an optional pre-push hook and also put it into an early CI/CD step for your pull request checker. You'll get the same end result but your fussiest developers will be happier.

View on HN

> 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 is too late for that, and a pre-push hook is a good idea.)

> with git, you can even make anything that happens on the dev machines mandatory

s/can/can't?

Yes, indeed.

You can an an enterprise environment when following SOPs are mandatory due to cybersecurity and infrastructure requirements.

A good analogy is: git hooks are client-side validation; CI is server-side validation, aka the only validation you can trust.

You can run git commit with a --no-verify flag to skip these hooks

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?

That's a controversy I'm not sure you necessarily realise you've stepped into :).

It's fairly common to consider working and PR branches to be "unpublished" from a mutability point of view: if I base my work on someone else's PR, I'm going to have to rebase when they rebase. Merging to `main` publishes the commit, at which point it's immutable.

Working with JJ, its default behaviour is to consider parents of a branch that's not owned by you to be immutable.

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.

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.

Not to disagree, but this is so GitHub-centric. What is up with "diffs", "patches", and "submissions"? :D

Not to disagree, but calling it Github-centric is a bit over specific :)

I regularly work with Github, Bitbucket, and Gitlab. Everything I said applies except for the fact that I said "PR" instead of "MR". But yes, you're right. I'm highlighting a specific, albeit extremely popular, workflow.

I know, I know, I was going to edit it to "Git{Hub,Lab}" in the beginning but oh well.

In any case, my comment just reflects on the fact that you had a series of patches that you could not squash or rebase. It stuck.

And the fact that I see many people use the abbreviation "PR" for something that is merely a patch or diff. For example you might send a diff to the tech@ mailing list, but you should not refer to it as a PR.

Git{Hu,La}b

GitPub

Strong disagree: until the branch is merged, it's mine.

I'm in a camp that prefers single rebased commits as units of change, "stacked diffs" style.

GitHub in particular was annoying with this style but is definitely getting better. It's still not great at dealing with actual stacks of diffs, but I can (and do) work around that by keeping the stack locally and only pushing commits that apply directly to the main branch.

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.

Your hook really shouldn't be running on the merge commit unless you have conflicts in your merge.

Never had conflicts on a merge? We've got a lot of people on the same codebase. Merge conflicts are a fact of life. And they wouldn't be a problem without the stupid commit hook. It's the commit hook that makes them a problem.

If you have conflicts then you can fix them and run your linter or formatter. If you have a no conflict merge it doesn't matter.

Thanks, but that's not the issue here.

For the sake of argument, let's say you have a check that caps the number of lines per file and that both you and main added lines in the same file. It's not too weird if that check fails only after merge, right?

One benign example of something that can break after merge even if each branch is individually passing pre-merge. In less benign cases it will your branch merged to main and actual bugs in the code.

One reason to not allow "unclean merges" and enforced incoming branches to be rebased up-to-date to be mergable to the main branch.

You probably want to run the checks on each commit to main in CI and not rely on them being consistently run by contributors.

You do you but I find rebasing my branch on main instead of merging makes me scratch mybhead way less.

Why not take the best of both worlds? Use pre-commit hooks for client-side validation, and run the same checks in CI as well. I’ve been using this setup for years without any issues.

One key requirement in my setup is that every hook is hermetic and idempotent. I don’t use Rust in production, so I can’t comment on it in depth, but for most other languages—from clang-format to swift-format—I always download precompiled binaries from trusted sources (for example, the team’s S3 storage). This ensures that the tools run in a controlled environment and consistently produce the same results.

Case in point: https://www.jj-vcs.dev/

> 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 parts don't work but consistency lowers the failures, every time.

Enforcement should live in CI. Into people's dev environments, you put opt-in "enablement" that makes work easier in most cases, and gets out of the way otherwise.

Agreed, my company has some helper hooks they want folks to use which break certain workflows.

We’re a game studio with less technical staff using git (art and design) so we use hooks to break some commands that folks usually mess up.

Surprisingly most developers don’t know git well either and this saves them some pain too.

The few power users who know what they’re doing just disable these hooks.

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.

I'm the type of developer who always have a completely different way of working. I hate pre-commit hooks, and agree that pre-push + early step is CI is the right thing to do.

You dont have to install hooks. Its that simple.

Be prepared to have your PR blocked tho.

Reply on news.ycombinator.com