Re: [pbs-devel] [RFC v2 proxmox-backup] git-hooks: pre-commit runs cargo fmt --check

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 15/01/2024 um 10:28 schrieb Stefan Lendl:
> Thomas Lamprecht <t.lamprecht@proxmox.com> writes:
>> Am 12/01/2024 um 15:43 schrieb Stefan Lendl:
>>> * add a pre-commit hook that declines commiting if cargo fmt would make
>>
>> as said off-list I do not want anything that blocks committing,
>> that is a PITA in development especially as this doesn't gains us
>> much... so NAK!
> 
> It's easy to overwrite the checks when committing with `git commit --no-verify`

Yeah I know, we even wrote about that in the same chat..
But what about rebasing et al.? And even if, one then just aliases
git commit to include that anyway, so making this both a nuisance
and not helpful at all at the same time. And like I said, I explicitly
wrote about only warning in our chat, not sure why you just go for a
hard fail without any acknowledge of that at all...

>> Let's rather add a fmt --check test to the buildbot, then we get
>> pinged on issues and a maintainer can just commit and push a cargo
>> fmt run without that much fuzz.. 
>>
> 
> I agree adding a fmt check to a CI make absolute sense. As of now the
> buildbot is not triggered when sending a patch series. So the user will
> not get feedback.

This is not triggered either when sending a patch series though?!
And it wouldn't be triggered in any way if one never uses the
`make build` target too..

> Guarding locally against not-formatted commits, prevents user from
> sending these patches in the first place.

No it does not, besides that it doesn't hook into git send-email or
format-patch, it will be overridden due to interfering with 

> The intent was to ensure style compliance BEFORE sending a patch series
> and not relying on a manual feedback process to identify this.

buildbot is not a manual feedback process, and having that sometimes
slightly bad formatted code go into the repo just does not matter AT
ALL compared to pissing of people developing, rebasing, ...
So this is not solving any real problem our development process has,
at least none that would actually advance our projects, so it won't
go in, and even if it would just warn, the current form is way to
invasive to be applied for that either – would be great if one thinks
about existing hook users when doing such changes.
 
> Once a CI is ready, this could and should be done in the CI.
> 

buildbot is already running on every commit and sends out mails on
failures of actual feature commits, for those odd cases where one
forgot to  run cargo fmt  (or setup their editor to do so), doing it
there would be more than enough...