Kaartic Sivaraam [off-list ref] writes:
I guess Junio's suggestion found below seems concise enough
although it doesn't capture the reason I did the change.
I did shoot for conciseness, but what is a lot more important is to
record what is at the core of the issue. "I found it by doing A"
can hint to careful readers why doing A leads to an undesirable
behaviour, but when there are other ways to trigger problems that
come from the same cause, "I found it by doing A" is less useful
unless we also record "Doing A reveals the underlying problem X"
that can be shared by other ways B, C, ... to trigger it. The
careful readers need to guess what the X is.
And once you identify the underlying problem X and record _that_ in
the log message, I and A in "I found it by doing A" becomes much
less interesting and the readers do not have to guess.
Your "A" is 'git commit --amend -s' with the disabled part of hook
enabled. But I think 'git commit' without "--amend" and "-s" would
also show an issue that come from the same root cause. The hook
will add SoB that is based on the author, not the committer. That
resulting commit would be different from 'git commit -s' without the
hook enabled, which would add SoB based on the commiter name (that
would be a "B", that causes a related but different problem that
comes from the same underlying issue "X" which is "we should
consistently use the committer info like other parts of the
system").
In any case, thanks for a fix-up. Let's move this forward quickly,
as it is an update to a topic that is already in 'master'.
Thanks.
On Tue, 2017-08-15 at 10:28 -0700, Junio C Hamano wrote:
I did shoot for conciseness, but what is a lot more important is to
record what is at the core of the issue. "I found it by doing A"
can hint to careful readers why doing A leads to an undesirable
behaviour, but when there are other ways to trigger problems that
come from the same cause, "I found it by doing A" is less useful
unless we also record "Doing A reveals the underlying problem X"
that can be shared by other ways B, C, ... to trigger it. The
careful readers need to guess what the X is.
And once you identify the underlying problem X and record _that_ in
the log message, I and A in "I found it by doing A" becomes much
less interesting and the readers do not have to guess.
Your "A" is 'git commit --amend -s' with the disabled part of hook
enabled. But I think 'git commit' without "--amend" and "-s" would
also show an issue that come from the same root cause. The hook
will add SoB that is based on the author, not the committer. That
resulting commit would be different from 'git commit -s' without the
hook enabled, which would add SoB based on the commiter name (that
would be a "B", that causes a related but different problem that
comes from the same underlying issue "X" which is "we should
consistently use the committer info like other parts of the
system").
In any case, thanks for a fix-up. Let's move this forward quickly,
as it is an update to a topic that is already in 'master'.
Thanks.
Seeing the reply, I changed my opinion that "it isn't worth the effort
to write a better commit message for the change". I've given a try but
it got a little off-hand. Let me know if there's ways in which it could
be simplified and/or improved.
--
Kaartic
In general, a 'Sign-off' added should be that of the *committer* and not
that of the *commit's author*. As the 3rd part of the 'prepare-commit-msg'
hook appended the sign-off of the *commit's author* it worked weirdly in
some cases. For example 'git commit --amend -s' when coupled with that part
of the script woked weirdly as illustrated by the following scenario in which
the last commit's log message has the following trailer,
...
Signed-off-by: Random Developer [off-list ref]
and the commit's author is "Random Developer [off-list ref]". Assume
that the commit is trying to be amended by another developer who's identity is
"Another Developer [off-list ref]". When he tries to do
$ git commit --amend -s
with the 3rd part of the hook enabled then the trailer he would see in his editor
would be,
...
Signed-off-by: Random Developer [off-list ref]
Signed-off-by: Another Developer [off-list ref]
Signed-off-by: Random Developer [off-list ref]
This is because,
* the hook is invoked only after the sign-off is appended by the '-s' option
* the script tries to add the sign-off of the *commit's author* using interpret-trailers
and 'interpret-trailers' in it's default configuration tries to adds the trailer
when the *neighbouring* trailer isn't the same as the one trying to be added.
This is just an example and this kind of issue could repeat if similar conditions are
satisified for other cases.
Moreover the rest of Git adds the sign-off of the *committer* using sequencer.c::append_signoff().
So, use the correct logical variable that identifies the committer to append the sign-off
in the sample hook script.
Bottom line: Being consistent prevents all sorts of weird issues.
Signed-off-by: Kaartic Sivaraam <redacted>
---
Changes in v2:
- updated the commit message
Suggestions regarding ways to improve the message are most welcome.
templates/hooks--prepare-commit-msg.sample | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
index a84c3e5a8..12dd8fd88 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -34,7 +34,7 @@ SHA1=$3
# *) ;;
# esac
-# SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
+# SOB=$(git var GIT_COMMITTER_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
# if test -z "$COMMIT_SOURCE"
# then
--
2.14.0.rc1.434.g6eded367a