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

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

[Stefan Lendl]

* add a pre-commit hook that declines commiting if cargo fmt would make
  formatting changes.
* `make install-git-hook` installs the hook
* `make build` installs the hook
* `make uninstall-git-hook` removes the hook
* `make lint` runs cargo fmt --check in addition to cargo clippy

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Suggested-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    Changes from v1:
    
    * Fix indentation style in hook *sigh*

 Makefile             |  9 ++++++++-
 git-hooks/pre-commit | 13 +++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
 create mode 100755 git-hooks/pre-commit

diff --git a/Makefile b/Makefile
index 0317dd5e..fc866872 100644
--- a/Makefile
+++ b/Makefile
@@ -86,7 +86,7 @@ doc:
 
 # always re-create this dir
 .PHONY: build
-build:
+build: install-git-hooks
 	rm -rf build
 	mkdir build
 	git rev-parse HEAD > build/.repoid
@@ -188,6 +188,7 @@ $(COMPILED_BINS) $(COMPILEDIR)/dump-catalog-shell-cli $(COMPILEDIR)/docgen: .do-
 
 .PHONY: lint
 lint:
+	cargo fmt --all -- --check
 	cargo clippy -- -A clippy::all -D clippy::correctness
 
 install: $(COMPILED_BINS)
@@ -221,3 +222,9 @@ upload: $(SERVER_DEB) $(CLIENT_DEB) $(RESTORE_DEB) $(DOC_DEB)
 	  | ssh -X repoman@repo.proxmox.com upload --product pbs --dist $(UPLOAD_DIST)
 	tar cf - $(CLIENT_DEB) $(CLIENT_DBG_DEB) | ssh -X repoman@repo.proxmox.com upload --product "pve,pmg,pbs-client" --dist $(UPLOAD_DIST)
 	tar cf - $(RESTORE_DEB) $(RESTORE_DBG_DEB) | ssh -X repoman@repo.proxmox.com upload --product "pve" --dist $(UPLOAD_DIST)
+
+install-git-hooks:
+	ln -sn ../../git-hooks/pre-commit .git/hooks/pre-commit > /dev/null 2>&1 || true
+
+uninstall-git-hooks:
+	rm -f .git/hooks/pre-commit
diff --git a/git-hooks/pre-commit b/git-hooks/pre-commit
new file mode 100755
index 00000000..6cd0b3bf
--- /dev/null
+++ b/git-hooks/pre-commit
@@ -0,0 +1,13 @@
+#!/usr/bin/env bash
+
+# Run cargo fmt before a commit to ensure the format style
+
+cargo fmt --all -- --check > /dev/null 2>&1
+res=$?
+
+if [[ $res != 0 ]]; then
+    echo "git pre-commit hook: There are some code style issues, run \`cargo fmt\` first."
+    exit 1
+fi
+
+exit 0
-- 
2.42.0

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

[Thomas Lamprecht]

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!

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

>   formatting changes.
> * `make install-git-hook` installs the hook
> * `make build` installs the hook
> * `make uninstall-git-hook` removes the hook
> * `make lint` runs cargo fmt --check in addition to cargo clippy
> 
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> Suggested-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
> Notes:
>     Changes from v1:
>     
>     * Fix indentation style in hook *sigh*
> 
>  Makefile             |  9 ++++++++-
>  git-hooks/pre-commit | 13 +++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>  create mode 100755 git-hooks/pre-commit
> 
> diff --git a/Makefile b/Makefile
> index 0317dd5e..fc866872 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -86,7 +86,7 @@ doc:
>  
>  # always re-create this dir
>  .PHONY: build
> -build:
> +build: install-git-hooks
>  	rm -rf build
>  	mkdir build
>  	git rev-parse HEAD > build/.repoid
> @@ -188,6 +188,7 @@ $(COMPILED_BINS) $(COMPILEDIR)/dump-catalog-shell-cli $(COMPILEDIR)/docgen: .do-
>  
>  .PHONY: lint
>  lint:
> +	cargo fmt --all -- --check
>  	cargo clippy -- -A clippy::all -D clippy::correctness
>  
>  install: $(COMPILED_BINS)
> @@ -221,3 +222,9 @@ upload: $(SERVER_DEB) $(CLIENT_DEB) $(RESTORE_DEB) $(DOC_DEB)
>  	  | ssh -X repoman@repo.proxmox.com upload --product pbs --dist $(UPLOAD_DIST)
>  	tar cf - $(CLIENT_DEB) $(CLIENT_DBG_DEB) | ssh -X repoman@repo.proxmox.com upload --product "pve,pmg,pbs-client" --dist $(UPLOAD_DIST)
>  	tar cf - $(RESTORE_DEB) $(RESTORE_DBG_DEB) | ssh -X repoman@repo.proxmox.com upload --product "pve" --dist $(UPLOAD_DIST)
> +
> +install-git-hooks:
> +	ln -sn ../../git-hooks/pre-commit .git/hooks/pre-commit > /dev/null 2>&1 || true
> +
> +uninstall-git-hooks:
> +	rm -f .git/hooks/pre-commit

this is pretty intrusive and destroys other hooks people have..

> diff --git a/git-hooks/pre-commit b/git-hooks/pre-commit
> new file mode 100755
> index 00000000..6cd0b3bf
> --- /dev/null
> +++ b/git-hooks/pre-commit
> @@ -0,0 +1,13 @@
> +#!/usr/bin/env bash
> +
> +# Run cargo fmt before a commit to ensure the format style
> +
> +cargo fmt --all -- --check > /dev/null 2>&1
> +res=$?
> +
> +if [[ $res != 0 ]]; then
> +    echo "git pre-commit hook: There are some code style issues, run \`cargo fmt\` first."
> +    exit 1
> +fi
> +
> +exit 0

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

[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`

>
> 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.
Guarding locally against not-formatted commits, prevents user from
sending these patches in the first place.
The intent was to ensure style compliance BEFORE sending a patch series
and not relying on a manual feedback process to identify this.

Once a CI is ready, this could and should be done in the CI.

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

[Thomas Lamprecht]

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

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

[Lukas Wagner]

On 1/15/24 10:42, Thomas Lamprecht wrote:
>>> 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...

Just some thoughts from my side, since most of what was discussed can 
directly applied to running unit/integration tests as well:

In the end, automated checks and CI should help us to build better 
software and make our lives easier. Everybody makes mistakes, developers 
and maintainers alike. These tools should help to detect them as early 
as possible. If issues can be detected directly on the dev's workstation 
by hooks/make targets, perfect! Intuitively I'd go with (obvious enough)
warnings instead of a hard failure, because the failures can be annoying 
at times and it's always easy enough to amend the commit if you see a 
warning.

But it'd not good to rely on that alone, so automated systems like 
buildbot (or whatever CI runner we choose, as maintaing buildbot seems 
to be a bit of a chore, from what I've heard) are equally important.

One of the main issues I see with buildbot right now is that is does not 
help regular, non-maintainer developers *at all*, due to the fact that 
it only runs for commits that were already applied by a maintainer.

What I'd like to have is a CI system that also has *my* back as a 
developer submitting changes (posting a patch series/requesting a pull), 
in case I don't notice a mistake (be it styling, clippy, build failures 
in certain feature flag permutations, failing tests etc.), allowing me 
to fix it before a maintainer finds the time to review/accept/apply my 
patch series.

This should be the long-term goal, in my opinion. As an immediate 
improvement, added checks on buildbot and non-obtrusive hooks (could be 
opt-in for the dev as well) are a great addition as well, IMO.

     Lukas