Git pre-commit/push hook

[infinisil]

We should have this in Nixpkgs for NixOS/nixpkgs#326407. Here are some notes for that:

• For every push/commit, it should verify that changed and new files are formatted
  • For every commit is more useful because like that no extra commits will be necessary just to format
  • But for every push would also have advantages, see Treewide Nix reformat pass 1 [skip treewide] nixpkgs#322537 (comment)
  • Ideally this could also be together with a rewrite/rebase mode, kind of like https://github.com/grahamc/git-rebase-format/blob/master/git-rebase-format.sh (ref Script to rebase with formatting #159), which would allow users to rebase their PRs, maintaining the same commits, but formatting each of them
    • Probably something with git filter-branch would be much simpler than that script though
• It should use the nixfmt version pinned in the shell.nix
• It should install itself into .git/hooks automatically via shellHook (this is also how https://github.com/cachix/git-hooks.nix works)
• Not a requirement, but ideally it would share as much with the GitHub Action workflow as possible. It's notably fairly easy to get another Git checkout of the base branch.

Ping @emilazy

[emilazy]

Only got five hours of sleep today but I’ll have a go at this soon.

An important design decision here: do we care that every commit has correct formatting, or only that the state of the final HEAD being pushed does? The former matches what a pre-commit hook would try to enforce, but as I understand it the CI check only looks at the latter.

I personally prefer having CI green after every commit when possible for the purpose of bisecting, but for formatting the downside of having incorrectly‐formatted intermediate commits may be minor, it would add additional fuss for users having to potentially amend multiple commits, and it would mean that no intermediate commit could ever have invalid Nix syntax (which, to be fair, might be a good thing).

[infinisil]

@emilazy CI will only care about HEAD being properly formatted (or rather, all files that changed between the base branch and HEAD), but I don't think it should be a pre-push hook, because it would require an extra commit to reformat all files changed in all of the commits, ending up with cases like this:

gitGraph
  commit id:"Change file A"
  commit id:"Change file B"
  commit id:"Reformat file A and B"

Loading

The last commit should then arguably even be added to .git-blame-ignore-revs.

So I think a pre-commit hook would be better, it keeps the history cleaner and as you say would also enforce correct Nix syntax for each commit, which I also think is a good thing :)

[nixos-discourse]

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-07-23/49510/1

[nixos-discourse]

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/satisfaction-survey-from-the-new-rfc-166-formatting/49758/37

[GetPsyched]

So I think a pre-commit hook would be better, it keeps the history cleaner and as you say would also enforce correct Nix syntax for each commit, which I also think is a good thing :)

I generally dislike the use of pre-commit hooks and I like that nixpkgs doesn't have any such hooks until now.

The code not being formatted and being flagged by CI would bring up issues with the author's environment setup. Adding a safety net for it (hooks) would mean that those environments remain mis-configured.

What I'm suggesting would allow for some friction to remain but I think it's worth promoting better configuration of dev environments. What do you think?

[uninsane]

The code not being formatted and being flagged by CI would bring up issues with the author's environment setup. Adding a safety net for it (hooks) would mean that those environments remain mis-configured.

if i understand it right, there's only really a handful of ways to ensure every developer's environment results in nixfmt-conforming formatting:

1. editor integration.
2. git integration.
3. nix/nixpkgs integration (e.g. fail eval for unformatted files, or fail build for unformatted packages).

don't underestimate how varied dev environments are w.r.t. editors. there may be value with a contrib/ folder to share editor-specific nixfmt configs, Home Manager integrations, nixpkgs programs.$EDITOR options, etc. you still won't reach those poor people writing patches with nano over ssh (i see it happen weekly -- again, don't underestimate the variance here).

git as a point of integration is likely to reach almost every nix dev. i think most editors integrating git do so in a way that will trigger the commit hooks (might be worth confirming that)? you still won't reach That Guy who manages his nixpkgs repo with fossil and only exports to git for the PR process but, well, maybe it's worth having both of these things then.

[GetPsyched]

i think most editors integrating git do so in a way that will trigger the commit hooks (might be worth confirming that)?

AFAIK it's Git that executes the pre-commit hooks, not the editor; so, this should be fine.

maybe it's worth having both of these things then.

Given you're referring to the hooks and the CI as "both", the question of having it on the CI is a definite yes, because having a check as a hook and not on the CI is like having front-end validation but not back-end ones.

To clarify, I wasn't recommending a different ways of ensuring that each developer environment is accurate; rather I'm suggesting to not ensure that at all. Mainly have 2 reasons for this:

• Pushes the contributor to fix their environment on their own, bettering their knowledge of contribution guidelines.
• Gives people like guarantee that no miscellaneous code is being run on device. (there's a reason direnv asks to allow each time .envrc changes)

Do let me know why you think this doesn't sound worth it, if at all.

[uninsane]

Given you're referring to the hooks and the CI as "both"

i was saying commit hook + editor integrations are both worth having.

Pushes the contributor to fix their environment on their own

this is exactly what we have to avoid. most people do not care about nix code formatting. i do not mean that in a disparaging way: for any particular nix development (a new CLI option, a new package or service, etc), most users do not benefit and do not care. development progresses only because those who care about a thing go about integrating it in a way that doesn't negatively impact those who don't care about it. formatting is no exception. the ideal integration would be that nobody has to adjust any config.

as you point out, "nobody has to adjust any config" is in tension with "no miscellaneous code is being run on device". oh well, we can at least add an easy-to-toggle and easy-to-discover option and gauge desire for making it a default once it's there. that's what the git hook gets us.

i'm vague on what else you mean by "environment", except editor hooks or shell hooks or something like that. those could be worth having, but they're not the same type of single-point-of-integration as these git hooks. you can't have the CI failure say "add git.enableHooks = true; to get automatic formatting", and if we want formatting to be enforced by CI, we need something like that. not "go read this document and spend an hour fixing your environment."

[GetPsyched]

Hmm. You're right that we probably shouldn't push for something that might scare away contributors, who are the best resource we have. In which case, contradictory to my own point, I think it tips the scale in favour of "nobody has to adjust any config". My concerns were more general and TBH I'd be willing to make an exception for the general good. (not that my personal concerns would've stopped this from going through, but ykwim)