Re: [PATCH] Move format-patch base commit and prerequisites before email signature

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> On Thu, Sep 08, 2016 at 11:54:08AM -0700, Josh Triplett wrote:
>>
>>> > your problem description
>>> > looks perfect.  I am still not sure if the code does a reasonable
>>> > thing in MIME case, though.
>>> 
>>> It *looks* correct to me.
>> 
>> Hmm. It looks correct to me, too; ...
>> ...
>> So this is actually fixing a bug,...
>
> Yes, I actually wanted to hear that from Josh and have that in the
> proposed log message ;-).

So here is a suggested replacement.  I notice that in the MIME case,
we do not leave any blank line between the last line of the patch
and the baseinfo, which makes it look a bit strange, e.g. output of
"format-patch --attach=mimemime -1" may end like this:

    +       test_write_lines 1 2 >expect &&
    +       test_cmp expect actual
    +'
    +
     test_expect_success 'format-patch --pretty=mboxrd' '
            sp=" " &&
            cat >msg <<-INPUT_END &&
    base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b

    --------------mimemime--

We may want to tweak it a bit further.

-- >8 --
From: Josh Triplett <josh@joshtriplett.org>
Date: Wed, 7 Sep 2016 18:12:01 -0700
Subject: [PATCH] format-patch: show base info before email signature

Any text below the "-- " for the email signature gets treated as part of
the signature, and many mail clients will trim it from the quoted text
for a reply.  Move it above the signature, so people can reply to it
more easily.

Similarly, when producing the patch as a MIME attachment, the
original code placed the base info after the attached part, which
would be discarded.  Move the base info to the end of the part,
still inside the part boundary.

Add tests for the exact format of the email signature, and add tests
to ensure that the base info appears before the email signature when
producing a plain-text output, and that it appears before the part
boundary when producing a MIME attachment.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c           |  4 ++--
 t/t4014-format-patch.sh | 30 +++++++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 92dc34d..d69d5e6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1042,7 +1042,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	diff_flush(&opts);
 
 	fprintf(rev->diffopt.file, "\n");
-	print_signature(rev->diffopt.file);
 }
 
 static const char *clean_message_id(const char *msg_id)
@@ -1720,6 +1719,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, branch_name, quiet);
 		print_bases(&bases, rev.diffopt.file);
+		print_signature(rev.diffopt.file);
 		total++;
 		start_number--;
 	}
@@ -1779,13 +1779,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (!use_stdout)
 			rev.shown_one = 0;
 		if (shown) {
+			print_bases(&bases, rev.diffopt.file);
 			if (rev.mime_boundary)
 				fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
 				       mime_boundary_leader,
 				       rev.mime_boundary);
 			else
 				print_signature(rev.diffopt.file);
-			print_bases(&bases, rev.diffopt.file);
 		}
 		if (!use_stdout)
 			fclose(rev.diffopt.file);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b0579dd..535857e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
 	git format-patch --ignore-if-in-upstream HEAD
 '
 
+git_version="$(git --version | sed "s/.* //")"
+
+signature() {
+	printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
+}
+
+test_expect_success 'format-patch default signature' '
+	git format-patch --stdout -1 | tail -n 3 >output &&
+	signature >expect &&
+	test_cmp expect output
+'
+
 test_expect_success 'format-patch --signature' '
-	git format-patch --stdout --signature="my sig" -1 >output &&
-	grep "my sig" output
+	git format-patch --stdout --signature="my sig" -1 | tail -n 3 >output &&
+	signature "my sig" >expect &&
+	test_cmp expect output
 '
 
 test_expect_success 'format-patch with format.signature config' '
@@ -1502,12 +1515,11 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
 
 test_expect_success 'format-patch --base' '
 	git checkout side &&
-	git format-patch --stdout --base=HEAD~3 -1 >patch &&
-	grep "^base-commit:" patch >actual &&
-	grep "^prerequisite-patch-id:" patch >>actual &&
+	git format-patch --stdout --base=HEAD~3 -1 | tail -n 6 >actual &&
 	echo "base-commit: $(git rev-parse HEAD~3)" >expected &&
 	echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected &&
 	echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected &&
+	signature >> expected &&
 	test_cmp expected actual
 '
 
@@ -1605,6 +1617,14 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
 	test_cmp expected actual
 '
 
+test_expect_success 'format-patch --base with --attach' '
+	git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
+	sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
+		patch >actual &&
+	test_write_lines 1 2 >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'format-patch --pretty=mboxrd' '
 	sp=" " &&
 	cat >msg <<-INPUT_END &&
-- 
2.10.0-339-gc0c747f