{"thread":{"id":"34972","subject":"[PATCH] format-patch: print in-body \"From\" only when needed","startedAt":"2013-09-20T10:16:28Z","lastAt":"2013-09-20T21:06:11Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"227931","messageId":"20130920101626.GA21684@sigill.intra.peff.net","threadId":"34972","inReplyTo":null,"subject":"[PATCH] format-patch: print in-body \"From\" only when needed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-20T10:16:28Z","receivedAt":"2013-09-20T10:16:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit a908047 taught format-patch the \"--from\" option,\nwhich places the author ident into an in-body from header,\nand uses the committer ident in the rfc822 from header.  The\ndocumentation claims that it will omit the in-body header\nwhen it is the same as the rfc822 header, but the code never\nimplemented that behavior.\n\nThis patch completes the feature by comparing the two idents\nand doing nothing when they are the same (this is the same\nas simply omitting the in-body header, as the two are by\ndefinition indistinguishable in this case). This makes it\nreasonable to turn on \"--from\" all the time (if it matches\nyour particular workflow), rather than only using it when\nexporting other people's patches.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI had meant for this to be the behavior all along (as shown in the\ndocumentation), and I can't imagine why I didn't implement it along with\nthe original topic. I never noticed because I didn't turn on \"--from\"\nall the time in my workflow scripts until recently, but instead just\nused it manually when sending other people's patches (which I do only\nrarely).\n\n cache.h                 |  9 +++++++++\n ident.c                 | 29 +++++++++++++++++++++++++++++\n pretty.c                |  2 +-\n t/t4014-format-patch.sh | 10 ++++++++++\n 4 files changed, 49 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex a47b9c0..bfea954 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -953,6 +953,15 @@ struct ident_split {\n  */\n extern int split_ident_line(struct ident_split *, const char *, int);\n \n+/*\n+ * Compare split idents for equality or strict ordering. Note that we\n+ * compare only the ident part of the line, ignoring any timestamp.\n+ *\n+ * Because there are two fields, we must choose one as the primary key; we\n+ * currently arbitrarily pick the email.\n+ */\n+extern int ident_cmp(const struct ident_split *, const struct ident_split *);\n+\n struct checkout {\n \tconst char *base_dir;\n \tint base_dir_len;\ndiff --git a/ident.c b/ident.c\nindex 1c123e6..b29f81f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -402,3 +402,32 @@ int git_ident_config(const char *var, const char *value, void *data)\n \n \treturn 0;\n }\n+\n+static int buf_cmp(const char *a_begin, const char *a_end,\n+\t\t   const char *b_begin, const char *b_end)\n+{\n+\tint a_len = a_end - a_begin;\n+\tint b_len = b_end - b_begin;\n+\tint min = a_len < b_len ? a_len : b_len;\n+\tint cmp;\n+\n+\tcmp = memcmp(a_begin, b_begin, min);\n+\tif (cmp)\n+\t\treturn cmp;\n+\n+\treturn a_len - b_len;\n+}\n+\n+int ident_cmp(const struct ident_split *a,\n+\t      const struct ident_split *b)\n+{\n+\tint cmp;\n+\n+\tcmp = buf_cmp(a->mail_begin, a->mail_end,\n+\t\t      b->mail_begin, b->mail_end);\n+\tif (cmp)\n+\t\treturn cmp;\n+\n+\treturn buf_cmp(a->name_begin, a->name_end,\n+\t\t       b->name_begin, b->name_end);\n+}\ndiff --git a/pretty.c b/pretty.c\nindex 74563c9..b4e32b7 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -432,7 +432,7 @@ void pp_user_info(struct pretty_print_context *pp,\n \t\tmap_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n \n \tif (pp->fmt == CMIT_FMT_EMAIL) {\n-\t\tif (pp->from_ident) {\n+\t\tif (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\t\tstrbuf_addstr(&buf, \"From: \");\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 668933b..8f272bc 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1000,6 +1000,16 @@ test_expect_success '--from uses committer ident' '\n \ttest_cmp expect patch.head\n '\n \n+test_expect_success '--from omits redundant in-body header' '\n+\tgit format-patch -1 --stdout --from=\"A U Thor <author@example.com>\" >patch &&\n+\tcat >expect <<-\\EOF &&\n+\tFrom: A U Thor <author@example.com>\n+\n+\tEOF\n+\tsed -ne \"/^From:/p; /^$/p; /^---$/q\" <patch >patch.head &&\n+\ttest_cmp expect patch.head\n+'\n+\n test_expect_success 'in-body headers trigger content encoding' '\n \tGIT_AUTHOR_NAME=\"éxötìc\" test_commit exotic &&\n \ttest_when_finished \"git reset --hard HEAD^\" &&\n-- \n1.8.3.4.20.geeaee04.dirty\n"},{"id":"227942","messageId":"xmqq61tvqrw6.fsf@gitster.dls.corp.google.com","threadId":"34972","inReplyTo":"20130920101626.GA21684@sigill.intra.peff.net","subject":"Re: [PATCH] format-patch: print in-body \"From\" only when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-20T18:17:45Z","receivedAt":"2013-09-20T18:17:45Z","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> Commit a908047 taught format-patch the \"--from\" option,\n> which places the author ident into an in-body from header,\n> and uses the committer ident in the rfc822 from header.  The\n> documentation claims that it will omit the in-body header\n> when it is the same as the rfc822 header, but the code never\n> implemented that behavior.\n>\n> This patch completes the feature by comparing the two idents\n> and doing nothing when they are the same (this is the same\n> as simply omitting the in-body header, as the two are by\n> definition indistinguishable in this case). This makes it\n> reasonable to turn on \"--from\" all the time (if it matches\n> your particular workflow), rather than only using it when\n> exporting other people's patches.\n\nThis fix makes 100% sense to me under the assumption that the\n\"--from\" option is a good idea, but then it makes me wonder why we\nneed the option in the first place.  What would break if we made\npp->from_ident default to the value of GIT_COMMITTER_IDENT?\n"},{"id":"227947","messageId":"20130920191226.GB31170@sigill.intra.peff.net","threadId":"34972","inReplyTo":"xmqq61tvqrw6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: print in-body \"From\" only when needed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-20T19:12:26Z","receivedAt":"2013-09-20T19:12:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 20, 2013 at 11:17:45AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Commit a908047 taught format-patch the \"--from\" option,\n> > which places the author ident into an in-body from header,\n> > and uses the committer ident in the rfc822 from header.  The\n> > documentation claims that it will omit the in-body header\n> > when it is the same as the rfc822 header, but the code never\n> > implemented that behavior.\n> >\n> > This patch completes the feature by comparing the two idents\n> > and doing nothing when they are the same (this is the same\n> > as simply omitting the in-body header, as the two are by\n> > definition indistinguishable in this case). This makes it\n> > reasonable to turn on \"--from\" all the time (if it matches\n> > your particular workflow), rather than only using it when\n> > exporting other people's patches.\n> \n> This fix makes 100% sense to me under the assumption that the\n> \"--from\" option is a good idea, but then it makes me wonder why we\n> need the option in the first place.  What would break if we made\n> pp->from_ident default to the value of GIT_COMMITTER_IDENT?\n\nAnything consuming format-patch output that does not understand in-body\nheaders would be broken.\n\nI think rebase would be safe, because it uses git-am under the hood, so\nin theory it is a noop.\n\nsend-email would have to learn to parse the in-body header, for two\nreasons:\n\n  1. We cannot get rid of send-email's in-body header writing, because\n     we would want to handle patches generated (or munged) outside of\n     format-patch, and because send-email has its own \"--from\" option\n     and sendemail.from configuration.\n\n     If that config matches the committer, it should be a noop. If it\n     doesn't, we have we have two cases:\n\n       a. There is no in-body header. We promote the rfc822 header to an\n          in-body one, and add our configured \"from\" as the rfc822\n          header.\n\n       b. There is an in-body header. We leave it intact, but throw away\n          the current rfc822 header and replace it with our configured\n          header.\n\n  2. send-email does header magic like auto-adding cc's, and suppressing\n     addresses found in other headers. It would need (at least) to pick\n     out the author from the in-body header to cc.\n\nThe patch below turns the feature on all the time. There are test\nbreakages in in t9001, t3901, t4014, and t4013. The last 3 I think are\njust cosmetic. t9001 has a ton of breakages around the header\nsuppressions, but I didn't analyze which ones were just \"this should now\nread 'committer' instead of 'author'\" and which ones represented real\nbreakage.\n\nSo I think it should be possible to fix our internal consumers of\nformat-patch output to handle this. And certainly there are some\nexternal consumers that would be made more happy (i.e., anything that is\nplanning to actually send the output as an email). But I'm a little wary\nof external consumers that might get confused by it. I think we'd want\nto flip it at a major version boundary, at least.\n\n-Peff\n\n---\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 77d0f5f..77564fd 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1235,6 +1235,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\trev.no_inline = 1;\n \t}\n \n+\tfrom = xstrdup(git_committer_info(IDENT_NO_DATE));\n+\n \t/*\n \t * Parse the arguments before setup_revisions(), or something\n \t * like \"git format-patch -o a123 HEAD^..\" may fail; a123 is\n"},{"id":"227949","messageId":"xmqqr4cjp5j0.fsf@gitster.dls.corp.google.com","threadId":"34972","inReplyTo":"20130920191226.GB31170@sigill.intra.peff.net","subject":"Re: [PATCH] format-patch: print in-body \"From\" only when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-20T21:06:11Z","receivedAt":"2013-09-20T21:06:11Z","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> So I think it should be possible to fix our internal consumers \n> ... I'm a little wary\n> of external consumers that might get confused by it.\n\nYeah, thanks for a good summary of analysis.  I agree that it would\nbe doable, but it is dubious if it is worth it.\n\n>\n> ---\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 77d0f5f..77564fd 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1235,6 +1235,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \t\trev.no_inline = 1;\n>  \t}\n>  \n> +\tfrom = xstrdup(git_committer_info(IDENT_NO_DATE));\n> +\n>  \t/*\n>  \t * Parse the arguments before setup_revisions(), or something\n>  \t * like \"git format-patch -o a123 HEAD^..\" may fail; a123 is\n"}]}