{"thread":{"id":"44035","subject":"[PATCH] Move format-patch base commit and prerequisites before email signature","startedAt":"2016-09-08T01:12:12Z","lastAt":"2016-09-15T17:06:28Z","messageCount":14,"participants":["Josh Triplett","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"301352","messageId":"20160908011200.qzvbdt4wjwiji4h5@x","threadId":"44035","inReplyTo":null,"subject":"[PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-09-08T01:12:01Z","receivedAt":"2016-09-08T01:12:12Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"Any text below the \"-- \" for the email signature gets treated as part of\nthe signature, and many mail clients will trim it from the quoted text\nfor a reply.  Move it above the signature, so people can reply to it\nmore easily.\n\nAdd tests for the exact format of the email signature, and add tests to\nensure the email signature appears last.\n\n(Patch by Junio Hamano; tests by Josh Triplett.)\nSigned-off-by: Josh Triplett <josh@joshtriplett.org>\n---\n\nDoes the above seem reasonable, for a patch that incorporates the\nproposed patch from Message-Id\nxmqqh99rpud4.fsf@gitster.mtv.corp.google.com and adds tests?\nAlternatively, feel free to split this patch into two, the first with\nyou as the author.  I can confirm that the code change doesn't break any\nexisting tests; only the new tests added here check for it.  So a\ntwo-patch series wouldn't result in any breakage after the first patch.\n\n builtin/log.c           |  4 ++--\n t/t4014-format-patch.sh | 22 +++++++++++++++++-----\n 2 files changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 92dc34d..d69d5e6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1042,7 +1042,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tdiff_flush(&opts);\n \n \tfprintf(rev->diffopt.file, \"\\n\");\n-\tprint_signature(rev->diffopt.file);\n }\n \n static const char *clean_message_id(const char *msg_id)\n@@ -1720,6 +1719,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tmake_cover_letter(&rev, use_stdout,\n \t\t\t\t  origin, nr, list, branch_name, quiet);\n \t\tprint_bases(&bases, rev.diffopt.file);\n+\t\tprint_signature(rev.diffopt.file);\n \t\ttotal++;\n \t\tstart_number--;\n \t}\n@@ -1779,13 +1779,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (!use_stdout)\n \t\t\trev.shown_one = 0;\n \t\tif (shown) {\n+\t\t\tprint_bases(&bases, rev.diffopt.file);\n \t\t\tif (rev.mime_boundary)\n \t\t\t\tfprintf(rev.diffopt.file, \"\\n--%s%s--\\n\\n\\n\",\n \t\t\t\t       mime_boundary_leader,\n \t\t\t\t       rev.mime_boundary);\n \t\t\telse\n \t\t\t\tprint_signature(rev.diffopt.file);\n-\t\t\tprint_bases(&bases, rev.diffopt.file);\n \t\t}\n \t\tif (!use_stdout)\n \t\t\tfclose(rev.diffopt.file);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex b0579dd..a4af275 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '\n \tgit format-patch --ignore-if-in-upstream HEAD\n '\n \n+git_version=\"$(git --version | sed \"s/.* //\")\"\n+\n+signature() {\n+\tprintf \"%s\\n%s\\n\\n\" \"-- \" \"${1:-$git_version}\"\n+}\n+\n+test_expect_success 'format-patch default signature' '\n+\tgit format-patch --stdout -1 | tail -n 3 >output &&\n+\tsignature >expect &&\n+\ttest_cmp expect output\n+'\n+\n test_expect_success 'format-patch --signature' '\n-\tgit format-patch --stdout --signature=\"my sig\" -1 >output &&\n-\tgrep \"my sig\" output\n+\tgit format-patch --stdout --signature=\"my sig\" -1 | tail -n 3 >output &&\n+\tsignature \"my sig\" >expect &&\n+\ttest_cmp expect output\n '\n \n test_expect_success 'format-patch with format.signature config' '\n@@ -1502,12 +1515,11 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n \n test_expect_success 'format-patch --base' '\n \tgit checkout side &&\n-\tgit format-patch --stdout --base=HEAD~3 -1 >patch &&\n-\tgrep \"^base-commit:\" patch >actual &&\n-\tgrep \"^prerequisite-patch-id:\" patch >>actual &&\n+\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 6 >actual &&\n \techo \"base-commit: $(git rev-parse HEAD~3)\" >expected &&\n \techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n \techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n+\tsignature >> expected &&\n \ttest_cmp expected actual\n '\n \nbase-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n-- \ngit-series 0.8.10\n"},{"id":"301428","messageId":"xmqqshtags0o.fsf@gitster.mtv.corp.google.com","threadId":"44035","inReplyTo":"20160908011200.qzvbdt4wjwiji4h5@x","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T18:34:15Z","receivedAt":"2016-09-08T18:34:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> Any text below the \"-- \" for the email signature gets treated as part of\n> the signature, and many mail clients will trim it from the quoted text\n> for a reply.  Move it above the signature, so people can reply to it\n> more easily.\n>\n> Add tests for the exact format of the email signature, and add tests to\n> ensure the email signature appears last.\n>\n> (Patch by Junio Hamano; tests by Josh Triplett.)\n> Signed-off-by: Josh Triplett <josh@joshtriplett.org>\n> ---\n>\n> Does the above seem reasonable, for a patch that incorporates the\n> proposed patch from Message-Id\n> xmqqh99rpud4.fsf@gitster.mtv.corp.google.com and adds tests?\n\nOther than that I'd probably retitle it, your problem description\nlooks perfect.  I am still not sure if the code does a reasonable\nthing in MIME case, though.\n\nThanks for tying the loose ends anyway.\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index b0579dd..a4af275 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '\n>  \tgit format-patch --ignore-if-in-upstream HEAD\n>  '\n>  \n> +git_version=\"$(git --version | sed \"s/.* //\")\"\n> +\n> +signature() {\n> +\tprintf \"%s\\n%s\\n\\n\" \"-- \" \"${1:-$git_version}\"\n> +}\n\nHmph.  I would actually have expected that you would force a fixed\nand an easily noticeable string via format.signature for the purpose\nof the test, but I guess this test covers a lot more than what the\npurpose of the main part of the patch does (i.e. enforces that the\ndefault signature must be made from the version string of Git).  It\nis not a bad thing to test, but it probably does not belong to this\nchange.  If you _were_ to split the patch in two, that is where I\nprobably would split, i.e. \"we didn't test what the default signature\nlooks like, or we didn't make sure --signature option overrides the\ndefault signature, so let's test it\" as the preliminary preparation,\nfollowed by \"having base info after sig is inconvenient, let's move\nit and make sure base info stays before sig with additional test\" as\nthe second (and primary) patch.\n\nBut a single patch is fine.\n\nThanks.\n"},{"id":"301431","messageId":"20160908185408.5qtfnztjbastlrtw@x","threadId":"44035","inReplyTo":"xmqqshtags0o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-09-08T18:54:08Z","receivedAt":"2016-09-08T18:54:19Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Thu, Sep 08, 2016 at 11:34:15AM -0700, Junio C Hamano wrote:\n> Josh Triplett <josh@joshtriplett.org> writes:\n> \n> > Any text below the \"-- \" for the email signature gets treated as part of\n> > the signature, and many mail clients will trim it from the quoted text\n> > for a reply.  Move it above the signature, so people can reply to it\n> > more easily.\n> >\n> > Add tests for the exact format of the email signature, and add tests to\n> > ensure the email signature appears last.\n> >\n> > (Patch by Junio Hamano; tests by Josh Triplett.)\n> > Signed-off-by: Josh Triplett <josh@joshtriplett.org>\n> > ---\n> >\n> > Does the above seem reasonable, for a patch that incorporates the\n> > proposed patch from Message-Id\n> > xmqqh99rpud4.fsf@gitster.mtv.corp.google.com and adds tests?\n> \n> Other than that I'd probably retitle it,\n\nAh, true, I should have titled it \"format-patch: move base commit ...\".\n\n> your problem description\n> looks perfect.  I am still not sure if the code does a reasonable\n> thing in MIME case, though.\n\nIt *looks* correct to me.\n\n> Thanks for tying the loose ends anyway.\n> \n> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> > index b0579dd..a4af275 100755\n> > --- a/t/t4014-format-patch.sh\n> > +++ b/t/t4014-format-patch.sh\n> > @@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '\n> >  \tgit format-patch --ignore-if-in-upstream HEAD\n> >  '\n> >  \n> > +git_version=\"$(git --version | sed \"s/.* //\")\"\n> > +\n> > +signature() {\n> > +\tprintf \"%s\\n%s\\n\\n\" \"-- \" \"${1:-$git_version}\"\n> > +}\n> \n> Hmph.  I would actually have expected that you would force a fixed\n> and an easily noticeable string via format.signature for the purpose\n> of the test,\n\nOne of the git tests already did that.  I just modified that test to\ntest the exact signature format and that it appears at the end, rather\nthan just grepping to check that the signature string appears somewhere.\nThen when doing so, I realized that I should check the default case too\n(at which point that test change probably should have gone in a separate\npatch).\n\n> but I guess this test covers a lot more than what the\n> purpose of the main part of the patch does (i.e. enforces that the\n> default signature must be made from the version string of Git).  It\n> is not a bad thing to test, but it probably does not belong to this\n> change.  If you _were_ to split the patch in two, that is where I\n> probably would split, i.e. \"we didn't test what the default signature\n> looks like, or we didn't make sure --signature option overrides the\n> default signature, so let's test it\" as the preliminary preparation,\n> followed by \"having base info after sig is inconvenient, let's move\n> it and make sure base info stays before sig with additional test\" as\n> the second (and primary) patch.\n>\n> But a single patch is fine.\n> \n> Thanks.\n\nIf any other change ends up being necessary, I'll split the patch in v2.\n\n- Josh Triplett\n"},{"id":"301433","messageId":"xmqqoa3ygq9s.fsf@gitster.mtv.corp.google.com","threadId":"44035","inReplyTo":"20160908185408.5qtfnztjbastlrtw@x","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T19:11:59Z","receivedAt":"2016-09-08T19:12:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> If any other change ends up being necessary, I'll split the patch in v2.\n\nThanks. I do not see anything else offhand myself, but other people\nwatching the topic from the sideline may spot something we missed.\n\n"},{"id":"301442","messageId":"20160908200819.pkg7jqcvxjpdqr3a@sigill.intra.peff.net","threadId":"44035","inReplyTo":"20160908185408.5qtfnztjbastlrtw@x","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T20:08:20Z","receivedAt":"2016-09-08T20:08:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 11:54:08AM -0700, Josh Triplett wrote:\n\n> > your problem description\n> > looks perfect.  I am still not sure if the code does a reasonable\n> > thing in MIME case, though.\n> \n> It *looks* correct to me.\n\nHmm. It looks correct to me, too; we stick it just after the patch, so\nwith \"--attach\" it is part of the text/x-patch, which is reasonable.\n\nBut looking at the results of \"--attach\" from _before_ your patch, it\nlooks totally broken. The \"base\" information comes _after the final\ndelimiter of the multipart/mixed. Most mailers would just throw it away\nwhen decoding the multipart, I think.\n\nSo this is actually fixing a bug, and you could probably add a test\n(though I am not sure we have anything in git that actually parses\nmultipart messages _or_ that carefully consumes the base-commit info, so\nit might be hard to test in practice).\n\n-Peff\n"},{"id":"301452","messageId":"xmqqd1kef5k5.fsf@gitster.mtv.corp.google.com","threadId":"44035","inReplyTo":"20160908200819.pkg7jqcvxjpdqr3a@sigill.intra.peff.net","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T21:24:42Z","receivedAt":"2016-09-08T21:24:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Sep 08, 2016 at 11:54:08AM -0700, Josh Triplett wrote:\n>\n>> > your problem description\n>> > looks perfect.  I am still not sure if the code does a reasonable\n>> > thing in MIME case, though.\n>> \n>> It *looks* correct to me.\n> \n> Hmm. It looks correct to me, too; ...\n> ...\n> So this is actually fixing a bug,...\n\nYes, I actually wanted to hear that from Josh and have that in the\nproposed log message ;-).\n"},{"id":"301549","messageId":"xmqq7fakc12z.fsf@gitster.mtv.corp.google.com","threadId":"44035","inReplyTo":"xmqqd1kef5k5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T19:41:56Z","receivedAt":"2016-09-09T19:42:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> On Thu, Sep 08, 2016 at 11:54:08AM -0700, Josh Triplett wrote:\n>>\n>>> > your problem description\n>>> > looks perfect.  I am still not sure if the code does a reasonable\n>>> > thing in MIME case, though.\n>>> \n>>> It *looks* correct to me.\n>> \n>> Hmm. It looks correct to me, too; ...\n>> ...\n>> So this is actually fixing a bug,...\n>\n> Yes, I actually wanted to hear that from Josh and have that in the\n> proposed log message ;-).\n\nSo here is a suggested replacement.  I notice that in the MIME case,\nwe do not leave any blank line between the last line of the patch\nand the baseinfo, which makes it look a bit strange, e.g. output of\n\"format-patch --attach=mimemime -1\" may end like this:\n\n    +       test_write_lines 1 2 >expect &&\n    +       test_cmp expect actual\n    +'\n    +\n     test_expect_success 'format-patch --pretty=mboxrd' '\n            sp=\" \" &&\n            cat >msg <<-INPUT_END &&\n    base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n\n    --------------mimemime--\n\nWe may want to tweak it a bit further.\n\n-- >8 --\nFrom: Josh Triplett <josh@joshtriplett.org>\nDate: Wed, 7 Sep 2016 18:12:01 -0700\nSubject: [PATCH] format-patch: show base info before email signature\n\nAny text below the \"-- \" for the email signature gets treated as part of\nthe signature, and many mail clients will trim it from the quoted text\nfor a reply.  Move it above the signature, so people can reply to it\nmore easily.\n\nSimilarly, when producing the patch as a MIME attachment, the\noriginal code placed the base info after the attached part, which\nwould be discarded.  Move the base info to the end of the part,\nstill inside the part boundary.\n\nAdd tests for the exact format of the email signature, and add tests\nto ensure that the base info appears before the email signature when\nproducing a plain-text output, and that it appears before the part\nboundary when producing a MIME attachment.\n\nSigned-off-by: Josh Triplett <josh@joshtriplett.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/log.c           |  4 ++--\n t/t4014-format-patch.sh | 30 +++++++++++++++++++++++++-----\n 2 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 92dc34d..d69d5e6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1042,7 +1042,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tdiff_flush(&opts);\n \n \tfprintf(rev->diffopt.file, \"\\n\");\n-\tprint_signature(rev->diffopt.file);\n }\n \n static const char *clean_message_id(const char *msg_id)\n@@ -1720,6 +1719,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tmake_cover_letter(&rev, use_stdout,\n \t\t\t\t  origin, nr, list, branch_name, quiet);\n \t\tprint_bases(&bases, rev.diffopt.file);\n+\t\tprint_signature(rev.diffopt.file);\n \t\ttotal++;\n \t\tstart_number--;\n \t}\n@@ -1779,13 +1779,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (!use_stdout)\n \t\t\trev.shown_one = 0;\n \t\tif (shown) {\n+\t\t\tprint_bases(&bases, rev.diffopt.file);\n \t\t\tif (rev.mime_boundary)\n \t\t\t\tfprintf(rev.diffopt.file, \"\\n--%s%s--\\n\\n\\n\",\n \t\t\t\t       mime_boundary_leader,\n \t\t\t\t       rev.mime_boundary);\n \t\t\telse\n \t\t\t\tprint_signature(rev.diffopt.file);\n-\t\t\tprint_bases(&bases, rev.diffopt.file);\n \t\t}\n \t\tif (!use_stdout)\n \t\t\tfclose(rev.diffopt.file);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex b0579dd..535857e 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '\n \tgit format-patch --ignore-if-in-upstream HEAD\n '\n \n+git_version=\"$(git --version | sed \"s/.* //\")\"\n+\n+signature() {\n+\tprintf \"%s\\n%s\\n\\n\" \"-- \" \"${1:-$git_version}\"\n+}\n+\n+test_expect_success 'format-patch default signature' '\n+\tgit format-patch --stdout -1 | tail -n 3 >output &&\n+\tsignature >expect &&\n+\ttest_cmp expect output\n+'\n+\n test_expect_success 'format-patch --signature' '\n-\tgit format-patch --stdout --signature=\"my sig\" -1 >output &&\n-\tgrep \"my sig\" output\n+\tgit format-patch --stdout --signature=\"my sig\" -1 | tail -n 3 >output &&\n+\tsignature \"my sig\" >expect &&\n+\ttest_cmp expect output\n '\n \n test_expect_success 'format-patch with format.signature config' '\n@@ -1502,12 +1515,11 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n \n test_expect_success 'format-patch --base' '\n \tgit checkout side &&\n-\tgit format-patch --stdout --base=HEAD~3 -1 >patch &&\n-\tgrep \"^base-commit:\" patch >actual &&\n-\tgrep \"^prerequisite-patch-id:\" patch >>actual &&\n+\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 6 >actual &&\n \techo \"base-commit: $(git rev-parse HEAD~3)\" >expected &&\n \techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n \techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n+\tsignature >> expected &&\n \ttest_cmp expected actual\n '\n \n@@ -1605,6 +1617,14 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'format-patch --base with --attach' '\n+\tgit format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&\n+\tsed -n -e \"/^base-commit:/s/.*/1/p\" -e \"/^---*mimemime--$/s/.*/2/p\" \\\n+\t\tpatch >actual &&\n+\ttest_write_lines 1 2 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'format-patch --pretty=mboxrd' '\n \tsp=\" \" &&\n \tcat >msg <<-INPUT_END &&\n-- \n2.10.0-339-gc0c747f\n\n"},{"id":"301550","messageId":"20160909200721.xfkbud377ja4wkrt@x","threadId":"44035","inReplyTo":"xmqq7fakc12z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-09-09T20:07:21Z","receivedAt":"2016-09-09T20:07:35Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Fri, Sep 09, 2016 at 12:41:56PM -0700, Junio C Hamano wrote:\n> So here is a suggested replacement.  I notice that in the MIME case,\n> we do not leave any blank line between the last line of the patch\n> and the baseinfo, which makes it look a bit strange, e.g. output of\n> \"format-patch --attach=mimemime -1\" may end like this:\n> \n>     +       test_write_lines 1 2 >expect &&\n>     +       test_cmp expect actual\n>     +'\n>     +\n>      test_expect_success 'format-patch --pretty=mboxrd' '\n>             sp=\" \" &&\n>             cat >msg <<-INPUT_END &&\n>     base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n> \n>     --------------mimemime--\n> \n> We may want to tweak it a bit further.\n> \n> -- >8 --\n> From: Josh Triplett <josh@joshtriplett.org>\n> Date: Wed, 7 Sep 2016 18:12:01 -0700\n> Subject: [PATCH] format-patch: show base info before email signature\n> \n> Any text below the \"-- \" for the email signature gets treated as part of\n> the signature, and many mail clients will trim it from the quoted text\n> for a reply.  Move it above the signature, so people can reply to it\n> more easily.\n> \n> Similarly, when producing the patch as a MIME attachment, the\n> original code placed the base info after the attached part, which\n> would be discarded.  Move the base info to the end of the part,\n> still inside the part boundary.\n> \n> Add tests for the exact format of the email signature, and add tests\n> to ensure that the base info appears before the email signature when\n> producing a plain-text output, and that it appears before the part\n> boundary when producing a MIME attachment.\n> \n> Signed-off-by: Josh Triplett <josh@joshtriplett.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nLooks good to me.\n\n>  builtin/log.c           |  4 ++--\n>  t/t4014-format-patch.sh | 30 +++++++++++++++++++++++++-----\n>  2 files changed, 27 insertions(+), 7 deletions(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index 92dc34d..d69d5e6 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1042,7 +1042,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n>  \tdiff_flush(&opts);\n>  \n>  \tfprintf(rev->diffopt.file, \"\\n\");\n> -\tprint_signature(rev->diffopt.file);\n>  }\n>  \n>  static const char *clean_message_id(const char *msg_id)\n> @@ -1720,6 +1719,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \t\tmake_cover_letter(&rev, use_stdout,\n>  \t\t\t\t  origin, nr, list, branch_name, quiet);\n>  \t\tprint_bases(&bases, rev.diffopt.file);\n> +\t\tprint_signature(rev.diffopt.file);\n>  \t\ttotal++;\n>  \t\tstart_number--;\n>  \t}\n> @@ -1779,13 +1779,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \t\tif (!use_stdout)\n>  \t\t\trev.shown_one = 0;\n>  \t\tif (shown) {\n> +\t\t\tprint_bases(&bases, rev.diffopt.file);\n>  \t\t\tif (rev.mime_boundary)\n>  \t\t\t\tfprintf(rev.diffopt.file, \"\\n--%s%s--\\n\\n\\n\",\n>  \t\t\t\t       mime_boundary_leader,\n>  \t\t\t\t       rev.mime_boundary);\n>  \t\t\telse\n>  \t\t\t\tprint_signature(rev.diffopt.file);\n> -\t\t\tprint_bases(&bases, rev.diffopt.file);\n>  \t\t}\n>  \t\tif (!use_stdout)\n>  \t\t\tfclose(rev.diffopt.file);\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index b0579dd..535857e 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '\n>  \tgit format-patch --ignore-if-in-upstream HEAD\n>  '\n>  \n> +git_version=\"$(git --version | sed \"s/.* //\")\"\n> +\n> +signature() {\n> +\tprintf \"%s\\n%s\\n\\n\" \"-- \" \"${1:-$git_version}\"\n> +}\n> +\n> +test_expect_success 'format-patch default signature' '\n> +\tgit format-patch --stdout -1 | tail -n 3 >output &&\n> +\tsignature >expect &&\n> +\ttest_cmp expect output\n> +'\n> +\n>  test_expect_success 'format-patch --signature' '\n> -\tgit format-patch --stdout --signature=\"my sig\" -1 >output &&\n> -\tgrep \"my sig\" output\n> +\tgit format-patch --stdout --signature=\"my sig\" -1 | tail -n 3 >output &&\n> +\tsignature \"my sig\" >expect &&\n> +\ttest_cmp expect output\n>  '\n>  \n>  test_expect_success 'format-patch with format.signature config' '\n> @@ -1502,12 +1515,11 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n>  \n>  test_expect_success 'format-patch --base' '\n>  \tgit checkout side &&\n> -\tgit format-patch --stdout --base=HEAD~3 -1 >patch &&\n> -\tgrep \"^base-commit:\" patch >actual &&\n> -\tgrep \"^prerequisite-patch-id:\" patch >>actual &&\n> +\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 6 >actual &&\n>  \techo \"base-commit: $(git rev-parse HEAD~3)\" >expected &&\n>  \techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n>  \techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n> +\tsignature >> expected &&\n>  \ttest_cmp expected actual\n>  '\n>  \n> @@ -1605,6 +1617,14 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'format-patch --base with --attach' '\n> +\tgit format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&\n> +\tsed -n -e \"/^base-commit:/s/.*/1/p\" -e \"/^---*mimemime--$/s/.*/2/p\" \\\n> +\t\tpatch >actual &&\n> +\ttest_write_lines 1 2 >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'format-patch --pretty=mboxrd' '\n>  \tsp=\" \" &&\n>  \tcat >msg <<-INPUT_END &&\n> -- \n> 2.10.0-339-gc0c747f\n> \n"},{"id":"301563","messageId":"xmqqpoocajbb.fsf@gitster.mtv.corp.google.com","threadId":"44035","inReplyTo":"20160909200721.xfkbud377ja4wkrt@x","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T20:51:04Z","receivedAt":"2016-09-09T20:51:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> On Fri, Sep 09, 2016 at 12:41:56PM -0700, Junio C Hamano wrote:\n>> So here is a suggested replacement.  I notice that in the MIME case,\n>> we do not leave any blank line between the last line of the patch\n>> and the baseinfo, which makes it look a bit strange, e.g. output of\n>> \"format-patch --attach=mimemime -1\" may end like this:\n>> \n>>     +       test_write_lines 1 2 >expect &&\n>>     +       test_cmp expect actual\n>>     +'\n>>     +\n>>      test_expect_success 'format-patch --pretty=mboxrd' '\n>>             sp=\" \" &&\n>>             cat >msg <<-INPUT_END &&\n>>     base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n>> \n>>     --------------mimemime--\n>> \n>> We may want to tweak it a bit further.\n>>  ...\n>\n> Looks good to me.\n\nThanks.\n\nDo you mean that the base information that appears immediately after\nthe patch text (either for MIME case or plain-text) does not bother\nyou, though?\n"},{"id":"301565","messageId":"20160909210040.zlsczhcotrxnu4e4@x","threadId":"44035","inReplyTo":"xmqqpoocajbb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-09-09T21:00:41Z","receivedAt":"2016-09-09T21:00:53Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Fri, Sep 09, 2016 at 01:51:04PM -0700, Junio C Hamano wrote:\n> Josh Triplett <josh@joshtriplett.org> writes:\n> \n> > On Fri, Sep 09, 2016 at 12:41:56PM -0700, Junio C Hamano wrote:\n> >> So here is a suggested replacement.  I notice that in the MIME case,\n> >> we do not leave any blank line between the last line of the patch\n> >> and the baseinfo, which makes it look a bit strange, e.g. output of\n> >> \"format-patch --attach=mimemime -1\" may end like this:\n> >> \n> >>     +       test_write_lines 1 2 >expect &&\n> >>     +       test_cmp expect actual\n> >>     +'\n> >>     +\n> >>      test_expect_success 'format-patch --pretty=mboxrd' '\n> >>             sp=\" \" &&\n> >>             cat >msg <<-INPUT_END &&\n> >>     base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n> >> \n> >>     --------------mimemime--\n> >> \n> >> We may want to tweak it a bit further.\n> >>  ...\n> >\n> > Looks good to me.\n> \n> Thanks.\n> \n> Do you mean that the base information that appears immediately after\n> the patch text (either for MIME case or plain-text) does not bother\n> you, though?\n\nSorry, I should have clarified that further.  I meant that the\nadditional tests looked good to me.\n\nAs it turns out, the patch I used to test this on happened to have a\nblank line as the last line of context before the base-commit line, so\nI'd overlooked this in the non-MIME case.  The issue you mentioned does\napply to both the MIME and non-MIME cases, and I agree that it needs\nfixing.  It doesn't seem like a functional issue, but aesthetically it\ndoesn't look good.\n\nDo you plan to make that change to print an additional blank line\n(likely inside print_bases), or should I?\n"},{"id":"301569","messageId":"xmqq7fakai5k.fsf@gitster.mtv.corp.google.com","threadId":"44035","inReplyTo":"20160909210040.zlsczhcotrxnu4e4@x","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T21:16:07Z","receivedAt":"2016-09-09T21:16:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> It doesn't seem like a functional issue, but aesthetically it\n> doesn't look good.\n>\n> Do you plan to make that change to print an additional blank line\n> (likely inside print_bases), or should I?\n\nI do not mind doing it myself, but I am already in today's\nintegration cycle (which will merge a handful of topics to\n'master'), so I won't get around to it for some time.  If you are\ninclined to, please be my guest ;-)\n\nThanks.\n"},{"id":"301938","messageId":"xmqqmvjaozrz.fsf@gitster.mtv.corp.google.com","threadId":"44035","inReplyTo":"xmqq7fakai5k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T22:57:36Z","receivedAt":"2016-09-14T22:57:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I do not mind doing it myself, but I am already in today's\n> integration cycle (which will merge a handful of topics to\n> 'master'), so I won't get around to it for some time.  If you are\n> inclined to, please be my guest ;-)\n\nI queued this on top for now; I think it can be just squashed into\nyour patch.  Please say \"I agree\" and I'll make it happen, or say\n\"that's wrong\" followed by a replacement patch ;-).\n\nThanks.\n\n builtin/log.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d69d5e6..cd9c4a4 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1360,7 +1360,7 @@ static void print_bases(struct base_tree_info *bases, FILE *file)\n \t\treturn;\n \n \t/* Show the base commit */\n-\tfprintf(file, \"base-commit: %s\\n\", oid_to_hex(&bases->base_commit));\n+\tfprintf(file, \"\\nbase-commit: %s\\n\", oid_to_hex(&bases->base_commit));\n \n \t/* Show the prerequisite patches */\n \tfor (i = bases->nr_patch_id - 1; i >= 0; i--)\n-- \n2.10.0-458-g8cce42d\n\n"},{"id":"301941","messageId":"20160914235231.GA12672@cloud","threadId":"44035","inReplyTo":"xmqqmvjaozrz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-09-14T23:52:31Z","receivedAt":"2016-09-14T23:52:42Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Wed, Sep 14, 2016 at 03:57:36PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I do not mind doing it myself, but I am already in today's\n> > integration cycle (which will merge a handful of topics to\n> > 'master'), so I won't get around to it for some time.  If you are\n> > inclined to, please be my guest ;-)\n> \n> I queued this on top for now; I think it can be just squashed into\n> your patch.  Please say \"I agree\" and I'll make it happen, or say\n> \"that's wrong\" followed by a replacement patch ;-).\n\n\"I agree\". :)\n\nI'd suggest squashing in an *additional* patch to the testsuite to\nensure the presence of the blank line:\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 535857e..8d90a6e 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1515,8 +1515,9 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n \n test_expect_success 'format-patch --base' '\n \tgit checkout side &&\n-\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 6 >actual &&\n-\techo \"base-commit: $(git rev-parse HEAD~3)\" >expected &&\n+\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual &&\n+\techo >expected &&\n+\techo \"base-commit: $(git rev-parse HEAD~3)\" >>expected &&\n \techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n \techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n \tsignature >> expected &&\n"},{"id":"301981","messageId":"xmqqeg4lozxw.fsf@gitster.mtv.corp.google.com","threadId":"44035","inReplyTo":"20160914235231.GA12672@cloud","subject":"Re: [PATCH] Move format-patch base commit and prerequisites before email signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T17:06:19Z","receivedAt":"2016-09-15T17:06:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> I'd suggest squashing in an *additional* patch to the testsuite to\n> ensure the presence of the blank line:\n\nThanks, will do.\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index 535857e..8d90a6e 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -1515,8 +1515,9 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n>  \n>  test_expect_success 'format-patch --base' '\n>  \tgit checkout side &&\n> -\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 6 >actual &&\n> -\techo \"base-commit: $(git rev-parse HEAD~3)\" >expected &&\n> +\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual &&\n> +\techo >expected &&\n> +\techo \"base-commit: $(git rev-parse HEAD~3)\" >>expected &&\n>  \techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n>  \techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n>  \tsignature >> expected &&\n"}]}