Git pre-commit/push hook

[infinisil]

trafficstars

We should have this in Nixpkgs for https://github.com/NixOS/nixpkgs/pull/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 https://github.com/NixOS/nixpkgs/pull/322537#issuecomment-2214466166
  • 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 #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"

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)

[toastal]

You can make hooks suggested but ultimately optional by providing basic examples. An example would be creating a .githooks directory, & docs that say:

To use our suggested Git VCS hooks, run from the project root

$ git config --local core.hooksPath .githooks

This allows the user to easily get & use said hooks while trusting the makers/maintainers to not push something shady in these hooks, or they are free to modify .git/hooks manually to their needs (or paranoia) rather than using some third-party script that injects non-consensual-but-forced code execution (ala pre-commit or Husky). This is also a simpler solution that doesn’t require adding more third-party dependencies to the project since it is an interactive shell command & plain Git config.

[jfly]

Closing this in favor of deferring to treefmt on how to handle this. See https://github.com/numtide/treefmt/issues/164

[MattSturgeon]

Closing this in favor of deferring to treefmt on how to handle this. See https://github.com/numtide/treefmt/issues/164

I might be misinterpreting, but wasn't the discussion here about which hooks to use, (pre-commit, pre-push, etc) and whether they should be automatically installed (shellHook, etc)?

Deferring the hook's implementation to treefmt seems reasonable, but it will still be down to nixpkgs to enable it; and decide how automatic it should be.[^1]

That said having this issue in the nixfmt repo does feel off, as that is something that must be done in nixpkgs. Was this closed here because it is a nixpkgs+treefmt issue, rather than a nixfmt issue?

Perhaps a new issue should be opened in nixpkgs, to track the enabling of treefmt git hooks? And to discuss the finer details of how end-users should install such hooks.[^2]

[^1]: I suspect auto-installing hooks will remain controversial. My suggestion would be to find a way to make installation opt-in, then consider if it can transition to being opt-out. Either way it should always be optional.

[^2]: I believe org-owners can transfer issues from one repo to another?

[jfly]

We discussed this during the meeting. This issue seemed to be tracking 2 related, but IMO different things:

1. Building nixfmt itself (or tooling in nixfmt) to support pre-commit hooks.
2. Enabling precommit hooks in nixpkgs itself to automatically format code for contributors.

We agreed that 1. should be handled by treefmt.

Nobody at the meeting particularly cared about 2*. If anyone does, I think it would be better to open a new issue over there that references this one.

*My 2 cents: we should be striving to push autoformatting further left, into people's editors/IDEs. That's a better experience, and is VCS agnostic.