Re: [PATCH] hook: use correct logical variable

3 messages, 2 authors, 2017-08-15 · open the first message on its own page

Re: [PATCH] hook: use correct logical variable

From: Junio C Hamano <hidden>
Date: 2017-08-14 18:19:21

Kaartic Sivaraam [off-list ref] writes:
Sign-off added should be that of the "committer" not that of the
"commit's author".

Use the correct logical variable that identifies the committer.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 This fixes a small issue when trying to do the following with the script enabled,

    $ git commit --amend -s

 If the commit being amended was signed off by the commit's author
 then the above command would *append* the sign-off of the
 committer followed by that of the commit's author.
 That' because the script is invoked only after the sign-off is
 added by the '-s' option AND the default of 'trailer.ifexists'
 for interpret-trailers currently defaults to the
 'addIfDifferentNeighbor' thus interpret-trailer fails to identify
 the existing sign-off of the commit's author and adds it.

 Anyways, it doesn't make sense for a script to add the sign-off
 of the commit's author. So, fixing it seemed correct to me.
I tend to agree with "Anyways" above ;-) simply because I found the
long paragraph more confusing than enlightening, leaving me wonder
"why is the user using different settings for author and committer
name" at the end, which _is_ an irrelevant point in the issue being
addressed.

The real issue is that this commented-out sample hook uses "author"
ident for the sign-off, while all the rest of Git (i.e. all callers
of append_signoff() in sequencer.c) use committer ident.

If anything to be changed to this patch, I would say its "should be"
in the log message can be clarified with "why", perhaps like:

    Sign-off added should be that of the "committer", not that of
    the "commit's author"; that is how the rest of Git adds sign-off
    using sequencer.c::append_signoff().

Thanks.
quoted hunk
 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

Re: [PATCH] hook: use correct logical variable

From: Kaartic Sivaraam <hidden>
Date: 2017-08-15 09:30:33

On Monday 14 August 2017 11:24 PM, Stefan Beller wrote:
On Mon, Aug 14, 2017 at 1:46 AM, Kaartic Sivaraam
[off-list ref] wrote:
quoted
Sign-off added should be that of the "committer" not that of the
"commit's author".

Use the correct logical variable that identifies the committer.

Signed-off-by: Kaartic Sivaraam <redacted>
---
  This fixes a small issue when trying to do the following with the script enabled,

     $ git commit --amend -s

  If the commit being amended was signed off by the commit's author then the above command
  would *append* the sign-off of the committer followed by that of the commit's author.
  That' because the script is invoked only after the sign-off is added by the '-s' option AND
  the default of 'trailer.ifexists' for interpret-trailers currently defaults to the 'addIfDifferentNeighbor'
  thus interpret-trailer fails to identify the existing sign-off of the commit's author and adds it.
The background knowledge provided up to here seems like
a valuable information that we'd want to preserve in the commit
history, i.e. make it part of the commit message?
I didn't do that previously expecting a few people would get confused by 
this (it did turn out
to be true). I could have made it more clearer but didn't attempt as I 
thought it wasn't worth
the effort. Yeah, it sometimes takes time to *simplify* things.

I guess Junio's suggestion found below seems concise enough although it 
doesn't
capture the reason I did the change.

         Sign-off added should be that of the "committer", not that of
         the "commit's author"; that is how the rest of Git adds sign-off
         using sequencer.c::append_signoff().


---
Kaartic

Re: [PATCH] hook: use correct logical variable

From: Kaartic Sivaraam <hidden>
Date: 2017-08-15 09:31:23

On Monday 14 August 2017 11:49 PM, Junio C Hamano wrote:
I tend to agree with "Anyways" above ;-) simply because I found the
long paragraph more confusing than enlightening, leaving me wonder
"why is the user using different settings for author and committer
name" at the end, which _is_ an irrelevant point in the issue being
addressed.
This is typically the reason I didn't add the big paragraph to the log 
message.

---
Kaartic
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help