Re: [PATCH] hook: use correct logical variable

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2017, 18:19 UTC
Message-ID: <xmqqwp66ui5h.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170814084646.30781-1-kaarticsivaraam91196@gmail.com>

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Show 22 quoted lines

> 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 <kaarticsivaraam91196@gmail.com>
> ---
>  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.

Show 16 quoted lines

>  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