am: match --signoff to the original scripted versionjc/builtin-am-signoff-regression-fix

Linus noticed that the recently reimplemented "git am -s" defines
the trailer block too rigidly, resulting in an unnecessary blank
line between the existing sign-offs and his new sign-off.  An e-mail
submission sent to Linus in real life ends with mixture of sign-offs
and commentaries, e.g.

	title here

	message here

	Signed-off-by: Original Author <original@auth.or>
	[rv: tweaked frotz and nitfol]
	Signed-off-by: Re Viewer <rv@ew.er>
	Signed-off-by: Other Reviewer <other@rev.ewer>
	---
	patch here

Because the reimplementation reused append_signoff() helper that is
used by other codepaths, which is unaware that people intermix such
comments with their sign-offs in the trailer block, such a message
was judged to end with a non-trailer, resulting in an extra blank
line before adding a new sign-off.

The original scripted version of "git am" used a lot looser
definition, i.e. "if and only if there is no line that begins with
Signed-off-by:, add a blank line before adding a new sign-off".  For
the upcoming release, stop using the append_signoff() in "git am"
and reimplement the looser definition used by the scripted version
to use only in "git am" to fix this regression in "am" while
avoiding new regressions to other users of append_signoff().

In the longer term, we should look into loosening append_signoff()
so that other codepaths that add a new sign-off behave the same way
as "git am -s", but that is a task for post-release.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

2 files changed, 77 insertions, 2 deletions

diff --git a/builtin/am.c b/builtin/am.c
index 634f7a7aa7..e7828e5d9b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1191,6 +1191,33 @@ static void NORETURN die_user_resolve(const struct am_state *state)
 	exit(128);
 }
 
+static void am_signoff(struct strbuf *sb)
+{
+	char *cp;
+	struct strbuf mine = STRBUF_INIT;
+
+	/* Does it end with our own sign-off? */
+	strbuf_addf(&mine, "\n%s%s\n",
+		    sign_off_header,
+		    fmt_name(getenv("GIT_COMMITTER_NAME"),
+			     getenv("GIT_COMMITTER_EMAIL")));
+	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(sb, mine.buf + !!cp);
+exit:
+	strbuf_release(&mine);
+}
+
 /**
  * Appends signoff to the "msg" field of the am_state.
  */
@@ -1199,7 +1226,7 @@ static void am_append_signoff(struct am_state *state)
 	struct strbuf sb = STRBUF_INIT;
 
 	strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
-	append_signoff(&sb, 0, 0);
+	am_signoff(&sb);
 	state->msg = strbuf_detach(&sb, &state->msg_len);
 }
 
@@ -1303,7 +1330,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 	stripspace(&msg, 0);
 
 	if (state->signoff)
-		append_signoff(&msg, 0, 0);
+		am_signoff(&msg);
 
 	assert(!state->author_name);
 	state->author_name = strbuf_detach(&author_name, NULL);
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index dd627c42d3..5e4855595b 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -873,4 +873,52 @@ test_expect_success 'am --message-id -s signs off after the message id' '
 	test_cmp expected actual
 '
 
+test_expect_success 'am -s unexpected trailer block' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	echo signed >file &&
+	git add file &&
+	cat >msg <<-EOF &&
+	subject here
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	[jc: tweaked log message]
+	Signed-off-by: J C H <j@c.h>
+	EOF
+	git commit -F msg &&
+	git cat-file commit HEAD | sed -e '1,/^$/d' >original &&
+	git format-patch --stdout -1 >patch &&
+
+	git reset --hard HEAD^ &&
+	git am -s patch &&
+	(
+		cat original &&
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	) >expect &&
+	git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+	test_cmp expect actual &&
+
+	cat >msg <<-\EOF &&
+	subject here
+
+	We make sure that there is a blank line between the log
+	message proper and Signed-off-by: line added.
+	EOF
+	git reset HEAD^ &&
+	git commit -F msg file &&
+	git cat-file commit HEAD | sed -e '1,/^$/d' >original &&
+	git format-patch --stdout -1 >patch &&
+
+	git reset --hard HEAD^ &&
+	git am -s patch &&
+
+	(
+		cat original &&
+		echo &&
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	) >expect &&
+	git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+	test_cmp expect actual
+'
+
 test_done