Re: [PATCH v1] am: fix signoff when other trailers are present

[Phillip Wood]

On 07/08/17 18:49, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> From: Phillip Wood 
>>
>> If there was no 'Signed-off-by:' trailer but another trailer such as
>> 'Reported-by:' then 'git am --signoff' would add a blank line between
>> the existing trailers and the added 'Signed-off-by:' line. e.g.
>>
>> Rebase accepts '--rerere-autoupdate' as an option but only honors
>> it if '-m' is also given. Fix it for a non-interactive rebase by
>> passing on the option to 'git am' and 'git cherry-pick'.
>>
>> Reported-by: Junio C Hamano 
>>
>> Signed-off-by: Phillip Wood 
>>
>> Fix by using the code provided for this purpose in sequencer.c.
>> Change the tests so that they check the formatting of the
>> 'Signed-off-by:' lines rather than just grepping for them.
>>
>> Signed-off-by: Phillip Wood 
>> ---
>> I'm not sure if this should be calling ignore_non_trailer() or not -
>> git commit does but git cherry-pick does not. This follows commit and
>> cherry-pick in ignoring the value of trailer.ifExists for the signoff.
>> I'm a bit surprised they do that - is it correct?
> 
> These built-in "sign-off" machinery long predates the "trailer"
> thing, so I am not surprised if they do not behave the same.  I
> vaguely recall having discussions on this earlier this year, but
> details escape me.  

Ah, that explains it. I did a quick search on public-inbox but didn't
turn up any obvious discussion about --signoff and trailer config. If
that's the behavior people are used to then lets leave it as it is
unless there is a call for it to be changed.

> Asking Jonathan, who did a series that ends at 44dc738a ("sequencer:
> add newline before adding footers", 2017-04-26), and Christian, who
> is the original contirbutor to the "trailer" machinery, for input.
> 

Re: [PATCH v1] am: fix signoff when other trailers are present

[Phillip Wood]

On 07/08/17 19:08, Jonathan Tan wrote:
> On Mon, 07 Aug 2017 10:49:28 -0700
> Junio C Hamano  wrote:
> 
>> Phillip Wood  writes:
>>
>>> From: Phillip Wood 
>>>
>>> If there was no 'Signed-off-by:' trailer but another trailer such as
>>> 'Reported-by:' then 'git am --signoff' would add a blank line between
>>> the existing trailers and the added 'Signed-off-by:' line. e.g.
>>>
>>> Rebase accepts '--rerere-autoupdate' as an option but only honors
>>> it if '-m' is also given. Fix it for a non-interactive rebase by
>>> passing on the option to 'git am' and 'git cherry-pick'.
>>>
>>> Reported-by: Junio C Hamano 
>>>
>>> Signed-off-by: Phillip Wood 
>>>
>>> Fix by using the code provided for this purpose in sequencer.c.
>>> Change the tests so that they check the formatting of the
>>> 'Signed-off-by:' lines rather than just grepping for them.
>>>
>>> Signed-off-by: Phillip Wood 
>>> ---
>>> I'm not sure if this should be calling ignore_non_trailer() or not -
>>> git commit does but git cherry-pick does not. This follows commit and
>>> cherry-pick in ignoring the value of trailer.ifExists for the signoff.
>>> I'm a bit surprised they do that - is it correct?
>>
>> These built-in "sign-off" machinery long predates the "trailer"
>> thing, so I am not surprised if they do not behave the same.  I
>> vaguely recall having discussions on this earlier this year, but
>> details escape me.  
>>
>> Asking Jonathan, who did a series that ends at 44dc738a ("sequencer:
>> add newline before adding footers", 2017-04-26), and Christian, who
>> is the original contirbutor to the "trailer" machinery, for input.
> 
> Regarding ignore_non_trailer(), I believe that's because "git commit"
> wants to tolerate blank lines and comments after the "real" commit
> message, whereas "git cherry-pick" doesn't need to. As far as I can
> tell, this "git am" case is similar to "git cherry-pick".
> 
> Regarding trailer.ifExists, the then existing behavior was to refrain
> from writing a new sign-off line only if it would be a duplicate of the
> last one, regardless of trailer.ifExists (as Junio says, back then, the
> sign-off mechanism and the trailer mechanism were independent). I
> preserved that behavior.
> 
Hi Jonathan

Thanks for the background. I'll remove the call to ignore_non_trailer()

Re: [PATCH v1] am: fix signoff when other trailers are present

[Jonathan Tan]

On Mon, 07 Aug 2017 10:49:28 -0700
Junio C Hamano  wrote:

> Phillip Wood  writes:
> 
> > From: Phillip Wood 
> >
> > If there was no 'Signed-off-by:' trailer but another trailer such as
> > 'Reported-by:' then 'git am --signoff' would add a blank line between
> > the existing trailers and the added 'Signed-off-by:' line. e.g.
> >
> > Rebase accepts '--rerere-autoupdate' as an option but only honors
> > it if '-m' is also given. Fix it for a non-interactive rebase by
> > passing on the option to 'git am' and 'git cherry-pick'.
> >
> > Reported-by: Junio C Hamano 
> >
> > Signed-off-by: Phillip Wood 
> >
> > Fix by using the code provided for this purpose in sequencer.c.
> > Change the tests so that they check the formatting of the
> > 'Signed-off-by:' lines rather than just grepping for them.
> >
> > Signed-off-by: Phillip Wood 
> > ---
> > I'm not sure if this should be calling ignore_non_trailer() or not -
> > git commit does but git cherry-pick does not. This follows commit and
> > cherry-pick in ignoring the value of trailer.ifExists for the signoff.
> > I'm a bit surprised they do that - is it correct?
> 
> These built-in "sign-off" machinery long predates the "trailer"
> thing, so I am not surprised if they do not behave the same.  I
> vaguely recall having discussions on this earlier this year, but
> details escape me.  
> 
> Asking Jonathan, who did a series that ends at 44dc738a ("sequencer:
> add newline before adding footers", 2017-04-26), and Christian, who
> is the original contirbutor to the "trailer" machinery, for input.

Regarding ignore_non_trailer(), I believe that's because "git commit"
wants to tolerate blank lines and comments after the "real" commit
message, whereas "git cherry-pick" doesn't need to. As far as I can
tell, this "git am" case is similar to "git cherry-pick".

Regarding trailer.ifExists, the then existing behavior was to refrain
from writing a new sign-off line only if it would be a duplicate of the
last one, regardless of trailer.ifExists (as Junio says, back then, the
sign-off mechanism and the trailer mechanism were independent). I
preserved that behavior.

Re: [PATCH v1] am: fix signoff when other trailers are present

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> If there was no 'Signed-off-by:' trailer but another trailer such as
> 'Reported-by:' then 'git am --signoff' would add a blank line between
> the existing trailers and the added 'Signed-off-by:' line. e.g.
>
> Rebase accepts '--rerere-autoupdate' as an option but only honors
> it if '-m' is also given. Fix it for a non-interactive rebase by
> passing on the option to 'git am' and 'git cherry-pick'.
>
> Reported-by: Junio C Hamano 
>
> Signed-off-by: Phillip Wood 
>
> Fix by using the code provided for this purpose in sequencer.c.
> Change the tests so that they check the formatting of the
> 'Signed-off-by:' lines rather than just grepping for them.
>
> Signed-off-by: Phillip Wood 
> ---
> I'm not sure if this should be calling ignore_non_trailer() or not -
> git commit does but git cherry-pick does not. This follows commit and
> cherry-pick in ignoring the value of trailer.ifExists for the signoff.
> I'm a bit surprised they do that - is it correct?

These built-in "sign-off" machinery long predates the "trailer"
thing, so I am not surprised if they do not behave the same.  I
vaguely recall having discussions on this earlier this year, but
details escape me.  

Asking Jonathan, who did a series that ends at 44dc738a ("sequencer:
add newline before adding footers", 2017-04-26), and Christian, who
is the original contirbutor to the "trailer" machinery, for input.

[PATCH v1] am: fix signoff when other trailers are present

[Phillip Wood]

From: Phillip Wood 

If there was no 'Signed-off-by:' trailer but another trailer such as
'Reported-by:' then 'git am --signoff' would add a blank line between
the existing trailers and the added 'Signed-off-by:' line. e.g.

Rebase accepts '--rerere-autoupdate' as an option but only honors
it if '-m' is also given. Fix it for a non-interactive rebase by
passing on the option to 'git am' and 'git cherry-pick'.

Reported-by: Junio C Hamano 

Signed-off-by: Phillip Wood 

Fix by using the code provided for this purpose in sequencer.c.
Change the tests so that they check the formatting of the
'Signed-off-by:' lines rather than just grepping for them.

Signed-off-by: Phillip Wood 
---
I'm not sure if this should be calling ignore_non_trailer() or not -
git commit does but git cherry-pick does not. This follows commit and
cherry-pick in ignoring the value of trailer.ifExists for the signoff.
I'm a bit surprised they do that - is it correct?

 builtin/am.c  | 26 +--
 t/t4150-am.sh | 83 +--
 2 files changed, 64 insertions(+), 45 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 
c973bd96dcb5d630d56e935733bfa4530ccd2872..f88d47c9d956f8e9d044cb1a4afa85c80f0c17cb
 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1181,34 +1181,10 @@ static void NORETURN die_user_resolve(const struct 
am_state *state)
  */
 static void am_append_signoff(struct am_state *state)
 {
-   char *cp;
-   struct strbuf mine = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
 
strbuf_attach(, state->msg, state->msg_len, state->msg_len);
-
-   /* our sign-off */
-   strbuf_addf(, "\n%s%s\n",
-   sign_off_header,
-   fmt_name(getenv("GIT_COMMITTER_NAME"),
-getenv("GIT_COMMITTER_EMAIL")));
-
-   /* Does sb end with it already? */
-   if (mine.len < sb.len &&
-   !strcmp(mine.buf, sb.buf + sb.len - mine.len))
-   goto exit; /* no need to duplicate */
-
-   /* Does it have any Signed-off-by: in the text */
-   for (cp = sb.buf;
-cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
-cp = strchr(cp, '\n')) {
-   if (sb.buf == cp || cp[-1] == '\n')
-   break;
-   }
-
-   strbuf_addstr(, mine.buf + !!cp);
-exit:
-   strbuf_release();
+   append_signoff(, ignore_non_trailer(sb.buf, sb.len), 0);
state->msg = strbuf_detach(, >msg_len);
 }
 
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 
44807e218d7016f58bd41b89af71104a37f31a8b..73b67b4280b99e0328e201e6b69c3d88b766ea84
 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -40,6 +40,8 @@ test_expect_success 'setup: messages' '
dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio
dignissim qui blandit praesent luptatum zzril delenit augue duis dolore 
te
feugait nulla facilisi.
+
+   Reported-by: A N Other 
EOF
 
cat >failmail <<-\EOF &&
@@ -93,7 +95,7 @@ test_expect_success setup '
echo world >>file &&
git add file &&
test_tick &&
-   git commit -s -F msg &&
+   git commit -F msg &&
git tag second &&
 
git format-patch --stdout first >patch1 &&
@@ -124,8 +126,6 @@ test_expect_success setup '
echo "Date: $GIT_AUTHOR_DATE" &&
echo &&
sed -e "1,2d" msg &&
-   echo &&
-   echo "Signed-off-by: $GIT_COMMITTER_NAME 
<$GIT_COMMITTER_EMAIL>" &&
echo "---" &&
git diff-tree --no-commit-id --stat -p second
} >patch1-stgit.eml &&
@@ -144,8 +144,6 @@ test_expect_success setup '
echo "# Parent  $_z40" &&
cat msg &&
echo &&
-   echo "Signed-off-by: $GIT_COMMITTER_NAME 
<$GIT_COMMITTER_EMAIL>" &&
-   echo &&
git diff-tree --no-commit-id -p second
} >patch1-hg.eml &&
 
@@ -470,13 +468,15 @@ test_expect_success 'am --signoff adds Signed-off-by: 
line' '
git reset --hard &&
git checkout -b master2 first &&
git am --signoff expected &&
-   echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" 
>>expected &&
-   git cat-file commit HEAD^ | grep "Signed-off-by:" >actual &&
-   test_cmp expected actual &&
-   echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" 
>expected &&
-   git cat-file commit HEAD | grep "Signed-off-by:" >actual &&
-   test_cmp expected actual
+   {
+   printf "third\n\nSigned-off-by: %s <%s>\n\n" \
+   "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" &&
+   cat msg &&
+   printf "Signed-off-by: %s <%s>\n\n" \
+