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

[Junio C Hamano]

Phillip Wood  writes:

>  test_expect_success 'am --signoff does not add Signed-off-by: line if 
> already there' '
> - git format-patch --stdout HEAD^ >patch3 &&
> - sed -e "/^Subject/ s,\[PATCH,Re: Re: Re: & 1/5 v2] [foo," patch3 
> >patch4 &&
> - rm -fr .git/rebase-apply &&
> - git reset --hard &&
> - git checkout HEAD^ &&
> - git am --signoff patch4 &&
> - git cat-file commit HEAD >actual &&
> - test $(grep -c "^Signed-off-by:" actual) -eq 1
> + git format-patch --stdout first >patch3 &&
> + git reset --hard first &&
> + git am --signoff  + git log --pretty=%B -2 HEAD >actual &&
> + test_cmp expected-log actual
> +'
> +
> +test_expect_success 'am --signoff adds Signed-off-by: if another author is 
> preset' '

Present?

I think the motivation for adding this is to make sure that what the
previous test checks will be true only when the one we are about to
add is already at the end.  So perhaps the previous test needs to be
retitled from "if already there" to something a bit tighter,
e.g. "if already at the end"?

Also, strictly speaking, I think this isn't testing if another
author is present---it is specifying what should happen when the
last one is not us.

> + NAME="A N Other" &&
> + EMAIL="a.n.ot...@example.com" &&
> + {
> + printf "third\n\nSigned-off-by: %s <%s>\nSigned-off-by: %s 
> <%s>\n\n" \
> + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
> + "$NAME" "$EMAIL" &&

A "printf" tip: you can do

printf "third\n\n"
printf "S-o-b: %s <%s>\n" A B C D

to get two lines of the latter.  That would clarify what the next
test does which wants to add three of them.

> + cat msg &&
> + printf "Signed-off-by: %s <%s>\nSigned-off-by: %s <%s>\n\n" \
> + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
> + "$NAME" "$EMAIL"
> + } >expected-log &&
> + git reset --hard first &&
> + GIT_COMMITTER_NAME="$NAME" GIT_COMMITTER_EMAIL="$EMAIL" \
> + git am --signoff  + git log --pretty=%B -2 HEAD >actual &&
> + test_cmp expected-log actual
> +'
> +
> +test_expect_success 'am --signoff duplicates Signed-off-by: if it is not the 
> last one' '
> + NAME="A N Other" &&
> + EMAIL="a.n.ot...@example.com" &&
> + {
> + printf "third\n\nSigned-off-by: %s <%s>\n\
> +Signed-off-by: %s <%s>\nSigned-off-by: %s <%s>\n\n" \
> + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
> + "$NAME" "$EMAIL" \
> + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" &&
> + cat msg &&
> + printf "Signed-off-by: %s <%s>\nSigned-off-by: %s <%s>\n\
> +Signed-off-by: %s <%s>\n\n" \
> + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
> + "$NAME" "$EMAIL" \
> + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL"
> + } >expected-log &&
> + git format-patch --stdout first >patch3 &&
> + git reset --hard first &&
> + git am --signoff  + git log --pretty=%B -2 HEAD >actual &&
> + test_cmp expected-log actual
>  '
>  
>  test_expect_success 'am without --keep removes Re: and [PATCH] stuff' '
> + git format-patch --stdout HEAD^ >tmp &&
> + sed -e "/^Subject/ s,\[PATCH,Re: Re: Re: & 1/5 v2] [foo," tmp >patch4 &&
> + git reset --hard HEAD^ &&
> + git amgit rev-parse HEAD >expected &&
>   git rev-parse master2 >actual &&
>   test_cmp expected actual

[PATCH v2] 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've removed the call to ignore_non_trailer(), otherwise this is
unchanged from v1

 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..3aaef59676452fd2e7c6d4a375dc7c95558744c6
 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(, 0, 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" \
+   "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL"
+   } >expected-log &&
+   git log --pretty=%B -2 HEAD >actual &&
+   test_cmp expected-log actual
 '
 
 test_expect_success 'am stays in branch' '
@@ -486,17