git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] hook: use correct logical variable
Date: Mon, 14 Aug 2017 11:19:06 -0700	[thread overview]
Message-ID: <xmqqwp66ui5h.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170814084646.30781-1-kaarticsivaraam91196@gmail.com> (Kaartic Sivaraam's message of "Mon, 14 Aug 2017 14:16:46 +0530")

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

>  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

  parent reply	other threads:[~2017-08-14 18:19 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-30 15:43 [PATCH] hooks: add signature to the top of the commit message Kaartic Sivaraam
2017-06-30 16:44 ` Junio C Hamano
2017-07-01 14:15   ` Kaartic Sivaraam
2017-07-01 16:16     ` "git intepret-trailers" vs. "sed script" to add the signature Kaartic Sivaraam
2017-07-01 17:32       ` [PATCH/RFC] hooks: add signature using "interpret-trailers" Kaartic Sivaraam
2017-07-03 16:58       ` "git intepret-trailers" vs. "sed script" to add the signature Junio C Hamano
2017-07-04 19:16         ` Kaartic Sivaraam
2017-07-05  1:48           ` Junio C Hamano
2017-07-05 17:00           ` [PATCH] hooks: add signature using "interpret-trailers" Kaartic Sivaraam
2017-07-05 17:35             ` Kaartic Sivaraam
2017-07-05 19:37               ` Junio C Hamano
2017-07-05 20:14               ` Ramsay Jones
2017-07-06 14:30                 ` Kaartic Sivaraam
2017-07-01 17:36     ` [PATCH] hooks: add signature to the top of the commit message Junio C Hamano
2017-07-01 18:40       ` Philip Oakley
2017-07-01 20:28         ` Junio C Hamano
2017-07-01 21:00           ` Philip Oakley
2017-07-01 18:52       ` Kaartic Sivaraam
2017-07-01 20:31         ` Junio C Hamano
2017-07-02 11:19           ` Kaartic Sivaraam
2017-07-02 11:27             ` [PATCH/RFC] hooks: replace irrelevant hook sample Kaartic Sivaraam
2017-07-05 16:51               ` [PATCH] " Kaartic Sivaraam
2017-07-05 19:50                 ` Junio C Hamano
2017-07-07 11:53                   ` Kaartic Sivaraam
2017-07-07 15:05                     ` Junio C Hamano
2017-07-07 15:24                       ` Kaartic Sivaraam
2017-07-07 16:07                         ` [PATCH 1/2] " Kaartic Sivaraam
2017-07-07 16:07                           ` [PATCH 2/2] hooks: add signature using "interpret-trailers" Kaartic Sivaraam
2017-07-07 18:27                           ` [PATCH 1/2] hooks: replace irrelevant hook sample Junio C Hamano
2017-07-10 14:17                             ` [PATCH 1/4] hook: cleanup script Kaartic Sivaraam
2017-07-10 14:17                               ` [PATCH 2/4] hook: name the positional variables Kaartic Sivaraam
2017-07-10 19:51                                 ` Junio C Hamano
2017-07-10 14:17                               ` [PATCH 3/4] hook: add signature using "interpret-trailers" Kaartic Sivaraam
2017-07-10 15:13                                 ` Ramsay Jones
2017-07-10 19:53                                   ` Junio C Hamano
2017-07-11 14:11                                     ` [PATCH 1/4] hook: cleanup script Kaartic Sivaraam
2017-07-11 14:11                                       ` [PATCH 2/4] hook: name the positional variables Kaartic Sivaraam
2017-07-11 14:11                                       ` [PATCH 3/4] hook: add sign-off using "interpret-trailers" Kaartic Sivaraam
2017-08-14  8:46                                         ` [PATCH] hook: use correct logical variable Kaartic Sivaraam
2017-08-14 17:54                                           ` Stefan Beller
2017-08-14 18:19                                           ` Junio C Hamano [this message]
2017-08-15  9:31                                             ` Kaartic Sivaraam
2017-08-15 17:28                                               ` Junio C Hamano
2017-08-17  2:47                                                 ` Kaartic Sivaraam
2017-08-17  2:50                                                   ` [PATCH v2/RFC] " Kaartic Sivaraam
2017-08-15  9:32                                             ` [PATCH] " Kaartic Sivaraam
2017-07-11 14:11                                       ` [PATCH 4/4] hook: add a simple first example Kaartic Sivaraam
2017-07-11 14:30                                         ` Kaartic Sivaraam
2017-07-11 13:10                                   ` [PATCH 3/4] hook: add signature using "interpret-trailers" Kaartic Sivaraam
2017-07-11 13:18                                   ` Kaartic Sivaraam
2017-07-10 14:17                               ` [PATCH 4/4] hook: add a simple first example Kaartic Sivaraam
2017-07-10 20:02                                 ` Junio C Hamano
2017-07-11 13:29                                   ` Kaartic Sivaraam
2017-07-11 16:03                                     ` Junio C Hamano
2017-07-11 18:04                                       ` Kaartic Sivaraam
2017-07-11 18:06                                       ` Kaartic Sivaraam
2017-07-10 19:50                               ` [PATCH 1/4] hook: cleanup script Junio C Hamano
2017-07-02 11:29             ` [PATCH] hooks: add script to HOOKS that allows adding notes from commit message Kaartic Sivaraam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqwp66ui5h.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=kaarticsivaraam91196@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).