Currently the commit-message-validator requires the Bug header to be specified before the change-id.
The way the current commit hook works (the one downloaded from gerrit, and specified in the docs in gerrit) puts the change-id on top of it, so you have to end up amending the commit every time.
This restriction is not needed afaik, and does not improve anything specific, so better be more flexible and allow the Bug header to be specified anywhere (like any other header).
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Remove Bug header order restriction | integration/commit-message-validator | master | +0 -13 |
Change 863238 had a related patch set uploaded (by David Caro; author: David Caro):
[integration/commit-message-validator@master] Remove Bug header order restriction
The way the current commit hook works (the one downloaded from gerrit, and specified in the docs in gerrit) puts the change-id on top of it, so you have to end up amending the commit every time.
I have never experienced this behavior in 9 years of using the gerrit provided commit hook.
Same. I do think there's value in having the main headers that humans need to read/click on before the one that's just needed by Gerrit.
Thanks for your input.
Yes, I understand that it must be something specific with my environment, as otherwise this would have been removed long time ago.
It does not change the fact that the enforcement might be useless, and thus, good to go away, simplify the code, and help, at least, one user (me), that is willing to help fix bugs and contribute to the project.
Same. I do think there's value in having the main headers that humans need to read/click on before the one that's just needed by Gerrit.
That's not what's enforced, the only enforcement is that bug goes before change-id, here for example, the signed-off-by (something you don't need clicking I guess) goes the first:
And you can make it go the last if you want, same for depends-on (that one I would expect to click on) or any other header.
Example of me playing with the headers (and the tests passing): https://gerrit.wikimedia.org/r/c/operations/puppet/+/739766
The way the current commit hook works (the one downloaded from gerrit, and specified in the docs in gerrit) puts the change-id on top of it, so you have to end up amending the commit every time.
This sounds like a Gerrit related bug or potentially a local git config. The hook relies on git-interpret-trailer, it is given a commit message via stdin and using --trailer Change-Id: xxxx would insert the header. The default is to add the trailer at the end unless somehow the Git config trailer.where is not end or does not default to end. Available settings:
trailer.where
This option tells where a new trailer will be added.
This can be end, which is the default, start, after or before.
If it is end, then each new trailer will appear at the end of the existing trailers.
If it is start, then each new trailer will appear at the start, instead of the end, of the existing trailers.
If it is after, then each new trailer will appear just after the last trailer with the same <token>.
If it is before, then each new trailer will appear just before the first trailer with the same <token>.
Example usage:
$ printf 'Summary of commit\n\nDescription\n\nGreat: T333\n'|git interpret-trailers --trailer 'Change-Id: Ixxxx' Summary of commit Description Great: T333 Change-Id: Ixxxx
$ printf 'Summary of commit\n\nDescription\n\nGreat: T333\n'|git interpret-trailers --where start --trailer 'Change-Id: Ixxxx' Summary of commit Description Change-Id: Ixxxx Great: T333
I have looked at Gerrit source code, at some point the hook had --where start but that got reverted and never made it to any Gerrit release. If you have a way to reproduce (a commit message and the commit-msg script associated with the repo`), I am more than willing to spend time fixing it up.
As for the order of headers, I commented on https://gerrit.wikimedia.org/r/c/integration/commit-message-validator/+/863238/ . I think instead of enforcing Bug, Closes, Fixes and Task to come before Change-Id we should enforce Change-Id to come last.
This only affects the first time you add the header right? Once the header is there, it will just not add it, maybe the way to go could be to change that script to move the header at the bottom? (specially if that's what you want to enforce in the tests)
Can we have our own hook for that or do we have to contribute it upstream? (not everyone using gerrit might want that header at the end)
This only affects the first time you add the header right? Once the header is there, it will just not add it
Not really, it seems to add it anyhow:
dcaro@vulcanus$ printf 'Summary of commit\n\nDescription\n\nChange-Id: Something\nGreat: T333\n'|git interpret-trailers --trailer 'Change-Id: IXXX' Summary of commit Description Change-Id: Something Great: T333 Change-Id: IXXX
hmm, I think that the issue might be the signed-off header, maybe at some point I pull commits without it but with change-id, then I amend the commit, so it adds the signed-off, but it adds it at the end, and then change-id gets added again.
Oh, not really, it seems that something else prevents the header from getting added:
dcaro@vulcanus$ git log -1
commit 32337fb4d893c69ca7dc9bdc63203505c95d3659 (HEAD -> wmcs, origin/wmcs)
Author: David Caro <dcaro@wikimedia.org>
Date: Mon Oct 10 12:25:15 2022 +0200
wmcs.create_instance_with_prefix: Add a sec group default
Also improved the help string. We were mentioning that there was a
default when there was not.
Change-Id: Ica2e4be14b37bc018ad32048ad890cfeb25232f3
Signed-off-by: David Caro <dcaro@wikimedia.org>
10:34 AM ~/Work/wikimedia/operations-cookbooks2 (wmcs|✔)
dcaro@vulcanus$ git ca
[wmcs 2f8d2da] wmcs.create_instance_with_prefix: Add a sec group default
Date: Mon Oct 10 12:25:15 2022 +0200
2 files changed, 6 insertions(+), 6 deletions(-)
10:34 AM ~/Work/wikimedia/operations-cookbooks2 (wmcs|✔)
dcaro@vulcanus$ git log -1
commit 2f8d2da7f4d346d696088b543bf438bd6fb6abc8 (HEAD -> wmcs)
Author: David Caro <dcaro@wikimedia.org>
Date: Mon Oct 10 12:25:15 2022 +0200
wmcs.create_instance_with_prefix: Add a sec group default
Also improved the help string. We were mentioning that there was a
default when there was not.
Change-Id: Ica2e4be14b37bc018ad32048ad890cfeb25232f3
Signed-off-by: David Caro <dcaro@wikimedia.org>So far this is what worked best for me (modifying the script at line 52):
# Avoid the --in-place option which only appeared in Git 2.8
# Avoid the --if-exists option which only appeared in Git 2.15
old_change_id=$(grep -e '^Change-Id:' "${dest}")
if test -n "$old_change_id"; then
change_id_header=$old_change_id
else
change_id_header="Change-Id: I${random}"
fi
if ! grep -v -e "^Change-Id: " "$1" \
| git -c trailer.ifexists=doNothing interpret-trailers \
--trailer "${change_id_header}" > "${dest}" ; then
echo "cannot insert change-id line in $1"
exit 1
fiFor some reason though the signed-off-by keeps getting added every time :/
Yep, now it becomes a battle every time I amend a commit, signed-off will add a duplicated signed-off-by header, and then change-id will put itself at the end, making the next amend commit put a new header...
xd
Nice finding, it is indeed an issue with handling of Signed-off-by:
$ printf 'Summary of commit\n\nSigned-off-by: foo <a@example.org>'|git interpret-trailers --where start --trailer 'Change-Id: Ixxxx' Summary of commit Change-Id: Ixxxx Signed-off-by: foo <a@example.org>
And I have reproduced it in a real world example. At first retrieve the latest Gerrit commit-msg hook just to be sure:
rm .git/hooks/commit-msg git-review --setup
Create an empty commit which will have the Change-Id inserted by the hook:
$ git commit -m 'Hello' --allow-empty [master 345fb363943] Hello $ git show commit 345fb36394332df8cbeb70cb328db80e25c3c7f0 (HEAD -> master) Author: Antoine Musso <hashar@free.fr> Date: Tue Dec 6 11:14:11 2022 +0100 Hello Change-Id: Idcd4d85b8271fff7393d37b9175dff51bbe3a4f3
So far so good. Amend it with signed off by:
$ git amend -s --allow-empty [master bf90e1339f0] Hello Date: Tue Dec 6 11:14:11 2022 +0100 $ git show commit bf90e1339f0a43884cc9e0023b78d68a6f6c3398 (HEAD -> master) Author: Antoine Musso <hashar@free.fr> Date: Tue Dec 6 11:14:11 2022 +0100 Hello Change-Id: Idcd4d85b8271fff7393d37b9175dff51bbe3a4f3 Signed-off-by: Antoine Musso <hashar@free.fr>
The signing header is appended last. Thus I guess our commit-message-validator should recognize Signed-off-by can be the last item and in such case enforce Change-Id to be immediately before.
It should accept:
Bug: T333 Change-Id: Ixxxxx Signed-Off-By: foobar@example.org
Signed-Off-By: foobar@example.org Bug: T333 Change-Id: Ixxxxx
And reject:
Change-Id: Ixxxxx Bug: T333 Signed-Off-By: foobar@example.org
Or we enforce Signed-Off-By to be the last header :) I am not sure how git commit -s recognizes whether one already has put their signature in a commit, maybe it expects them all to be the last ones.
maybe it expects them all to be the last ones
Hmm... looking at git's code it seems that the functionality for not duplicating anywhere in the commit is there, but it's not being used.
Implementation is here.
Used by this.
But guarded by a flag (no_dup_sob).
That is set here.
From the global set to 0 :/ actually 1 here.
And the last parameter, set to 0 also here.
Weird
It's only used for the format-patch --signoff!
12:11 PM ~/Work/wikimedia/operations-cookbooks2 (wmcs|✔)
dcaro@vulcanus$ git log -1
commit ac452c06c6511305de930c47085bf25fa61c764a (HEAD -> wmcs)
Author: David Caro <dcaro@wikimedia.org>
Date: Tue Dec 6 10:35:47 2022 +0100
lololo
Signed-off-by: David Caro <dcaro@wikimedia.org>
Change-Id: I1fd4a00394a17946de8d9423968a43508bafc624
12:11 PM ~/Work/wikimedia/operations-cookbooks2 (wmcs|✔)
dcaro@vulcanus$ git -c format.signoff=true format-patch --signoff --stdout -1
From ac452c06c6511305de930c47085bf25fa61c764a Mon Sep 17 00:00:00 2001
From: David Caro <dcaro@wikimedia.org>
Date: Tue, 6 Dec 2022 10:35:47 +0100
Subject: [PATCH] lololo
Signed-off-by: David Caro <dcaro@wikimedia.org>
Change-Id: I1fd4a00394a17946de8d9423968a43508bafc624
---
tox.ini | 1 +
1 file changed, 1 insertion(+)
diff --git a/tox.ini b/tox.ini
index 847ad67..35cf3fb 100644
--- a/tox.ini
+++ b/tox.ini
@@ -56,3 +56,4 @@ ignore =
W503,
# line break after binary operator
W504,
+
--
2.35.1(No extra Signed-off-by added)
Let's see how it goes (might take a while, so I'm already using the modified git locally): https://lore.kernel.org/git/pull.1438.git.1670423522572.gitgitgadget@gmail.com/
So for the gerrit hook ensuring that Change-Id is the latest header, this works for me:
Let me know how do you want to continue with it.
Change 863238 abandoned by David Caro:
[integration/commit-message-validator@master] Remove Bug header order restriction
Reason:
@dcaro Sorry I have completed missed the reply you made back in December 2022 and only came back here after Bryan abandoned a patch for commit-message-validator.
Nice, I wasn't aware of a Github project being available to act as a frontend to the git mailing list. Unfortunately it seems your patch hasn't been caught upstream :-\
So for the gerrit hook ensuring that Change-Id is the latest header, this works for me:
Let me know how do you want to continue with it.
The fix should be sent upstream and they require a CLA to be signed If your employment contract has a joint copyright agreement, you should be able to sign an individual one at https://cla.developers.google.com/clas . And I think that is a requirement for your fix to be proposed Upstream (I don't think I can propose it on your behalf without you having signed the CLA since I am not the author).
The repository is https://gerrit.googlesource.com/gerrit . Being a bug fix, that should be proposed against the earliest supported branch which currently is stable-3.6 (we run the outdated 3.5).
The hook is in resources/com/google/gerrit/server/tools/root/hooks/commit-msg
The test suite is resources/com/google/gerrit/server/commit-msg_test.sh. That requires a test temporary to be provided via the TEST_TMPDIR environment variable and I had success with:
TEST_TMPDIR=$(mktemp -d gerrit-hook-test-XXXX) resources/com/google/gerrit/server/commit-msg_test.sh
Alternative (but that is probably overkill for this use case) use Bazel, the Google build system by first installing Bazelisk and then you can run the tests using:
bazel test //resources/com/google/gerrit/server:commit-msg_test
To propose a patch, the Google Gerrit instance does not support ssh so you have to generate a HTTP token via https://gerrit-review.googlesource.com/settings/#HTTPCredentials
Happy to pair :-]