{"thread":{"id":"21628","subject":"[RFC] format-patch: Ensure that author and commit time are not lost","startedAt":"2009-11-15T13:25:21Z","lastAt":"2009-11-16T05:20:41Z","messageCount":4,"participants":["Björn Gustavsson","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"127609","messageId":"4B000141.5070503@gmail.com","threadId":"21628","inReplyTo":null,"subject":"[RFC] format-patch: Ensure that author and commit time are not lost","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-11-15T13:25:21Z","receivedAt":"2009-11-15T13:25:21Z","isPatch":false,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"'git format-patch' encodes the author of the commit in the From:\nheader and the time of the commit in the Date: header.\nDepending on how the email is sent, however, those headers can be\nlost before the email reaches its destination.\n\nTherefore, if the sender of the email (i.e. the configuration\noptions user.name and user.email) are different from the author\nof a commit, insert From: and Date: headers at the beginning of\nthe body of the commit message.\n\nSigned-off-by: Björn Gustavsson <bgustavsson@gmail.com>\n---\nThe code works, but is not appropriate for 'pu' because a fair\nnumber of test cases will fail.\n\nSome additional work is needed before this functionality can\nbe considered ready for 'pu'. Since I am rather busy at the\nmoment, I don't want to spend any more time on this unless I'll\nget some indication that this functionality is useful for\nsomeone else.\n\nI would also want some input on whether it would be OK\nto make this functionality unconditionally turned on so that\nthere will always From: and Date: headers in the commit\nmessage.\n\nI suspect that the answer is no, because there might be scripts\nthat parse the output of format-patch (perhaps to interface\nwith other source-code control systems).\n\nAssuming that we need an option, should the default be to\nproduce the extra headers (to make sure that the original\nauthor is not lost) or to not produce any extra headers\n(for backwards compatibility)?\n\nI suspect that the answer is that the default should be not\nto generate any extra headers not to break any existing\nscripts.\n\nTo implement an option is not difficult, but will need\nchanges in a fair number of source files in order to propagate\na boolean down to pretty_print_commit() (either as an\nadditional argument or possibly as an additional field\nin the pretty_print_context struct).\n\n pretty.c |   45 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 45 insertions(+), 0 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex da15cf2..63268a1 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -916,6 +916,36 @@ char *reencode_commit_message(const struct commit *commit, const char **encoding\n \treturn logmsg_reencode(commit, encoding);\n }\n \n+static int sender_is_not_author(const char *message)\n+{\n+\tconst char **msg_p = &message;\n+\n+\tfor (;;) {\n+\t\tconst char *line = *msg_p;\n+\t\tint linelen = get_one_line(*msg_p);\n+\t\t*msg_p += linelen;\n+\n+\t\t/* Get out of here if the commit has no author. */\n+\t\tif (linelen <= 1)\n+\t\t\treturn 0;\n+\n+\t\tif (!memcmp(line, \"author \", 7)) {\n+\t\t\tsize_t name_len = strlen(git_default_name);\n+\t\t\tsize_t email_len;\n+\t\t\tline += 7;\n+\t\t\tif (strncmp(line, git_default_name, name_len))\n+\t\t\t\treturn 1;\n+\t\t\tline += name_len;\n+\t\t\tif (line[0] != ' ' || line[1] != '<')\n+\t\t\t\treturn 1;\n+\t\t\tline += 2;\n+\t\t\temail_len = strlen(git_default_email);\n+\t\t\treturn strncmp(line, git_default_email, email_len) ||\n+\t\t\t\tline[email_len] != '>';\n+\t\t}\n+\t}\n+}\n+\n void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,\n \t\t\t struct strbuf *sb,\n \t\t\t const struct pretty_print_context *context)\n@@ -977,6 +1007,21 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,\n \t\tpp_title_line(fmt, &msg, sb, context->subject,\n \t\t\t      context->after_subject, encoding, need_8bit_cte);\n \n+\t/*\n+\t * If we are formatting an email and if the sender is different\n+\t * from the author of the commit, include the From: and Date:\n+\t * headers in the body of the commit message to make sure they\n+\t * are not lost.\n+\t */\n+\tif (fmt == CMIT_FMT_EMAIL) {\n+\t\tconst char *p = reencoded ? reencoded : commit->buffer;\n+\t\tif (sender_is_not_author(commit->buffer)) {\n+\t\t\tpp_header(fmt, context->abbrev, context->date_mode, encoding,\n+\t\t\t\t  commit, &p, sb);\n+\t\t\tstrbuf_addch(sb, '\\n');\n+\t\t}\n+\t}\n+\n \tbeginning_of_body = sb->len;\n \tif (fmt != CMIT_FMT_ONELINE)\n \t\tpp_remainder(fmt, &msg, sb, indent);\n-- \n1.6.5.2.186.geb718e\n"},{"id":"127634","messageId":"7vzl6nfqkx.fsf@alter.siamese.dyndns.org","threadId":"21628","inReplyTo":"4B000141.5070503@gmail.com","subject":"Re: [RFC] format-patch: Ensure that author and commit time are not lost","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-15T21:03:42Z","receivedAt":"2009-11-15T21:03:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Gustavsson <bgustavsson@gmail.com> writes:\n\n> 'git format-patch' encodes the author of the commit in the From: header\n> and the time of the commit in the Date: header.  Depending on how the\n> email is sent, however, those headers can be lost before the email\n> reaches its destination.\n>\n> Therefore, if the sender of the email (i.e. the configuration options\n> user.name and user.email) are different from the author of a commit,\n> insert From: and Date: headers at the beginning of the body of the\n> commit message.\n\nI think you are addressing a very valid issue, but I suspect that you are\ndoing so at a wrong place in the \"patch mail-out\" workflow.\n\n> +static int sender_is_not_author(const char *message)\n> +{\n> +\tconst char **msg_p = &message;\n> +\n> +\tfor (;;) {\n> ...\n> +\t}\n> +}\n\nThis new function is not about \"Is the _sender_ the same as the author?\",\nbut is about \"Is the person who is running format-patch the same as the\nauthor?\".  There is a big difference.\n\nWhat you want to catch is really \"Does the MUA that sends out the final\nmessage have the name of the author on its 'From: ' header?\", and that \ndepends on how the output from format-patch command is processed in the\ndownstream of the workflow.\n\nYou may read the file into your MUA edit session.  You would typically\nedit the first three lines out and move Subject: to the MUA's subject line.\nYou can choose to keep From:/Date: when you do so.  This happens to be the\nway I work, by the way.\n\nThe output may not even be used by a MUA; you may upload it to web based\nthingy like Bugzilla or FrySpray.  The recipient will download the whole\nthing and there is no need to edit.\n\nI would expect the right solution would be to give send-email an ability\nto either (1) use \"Sender:\" to record the operator of the MUA while\nkeeping \"From: \" taken from the payload, or (2) duplicate \"From: \" as an\nin-body header when it sends out.\n\nIt is a separate issue if that ability should be on by default or\ncontrolled by an option, of course.  But I do not think it should be in\nthe format-patch.\n"},{"id":"127642","messageId":"6672d0160911151416u7e8858f4k1591f1c1482cabb8@mail.gmail.com","threadId":"21628","inReplyTo":"7vzl6nfqkx.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] format-patch: Ensure that author and commit time are not lost","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-11-15T22:16:09Z","receivedAt":"2009-11-15T22:16:09Z","isPatch":false,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"2009/11/15 Junio C Hamano <gitster@pobox.com>:\n\n> I think you are addressing a very valid issue, but I suspect that you are\n> doing so at a wrong place in the \"patch mail-out\" workflow.\n[...]\n> This new function is not about \"Is the _sender_ the same as the author?\",\n> but is about \"Is the person who is running format-patch the same as the\n> author?\".  There is a big difference.\n\nYes, I kind of suspected that my solution would not\nbe general enough to suit all users and workflows.\n\nThanks for the feedback.\n\n/Björn\n\n-- \nBjörn Gustavsson, Erlang/OTP, Ericsson AB\n"},{"id":"127659","messageId":"20091116052041.GD14664@coredump.intra.peff.net","threadId":"21628","inReplyTo":"7vzl6nfqkx.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] format-patch: Ensure that author and commit time are not lost","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-16T05:20:41Z","receivedAt":"2009-11-16T05:20:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 15, 2009 at 01:03:42PM -0800, Junio C Hamano wrote:\n\n> I would expect the right solution would be to give send-email an ability\n> to either (1) use \"Sender:\" to record the operator of the MUA while\n> keeping \"From: \" taken from the payload, or (2) duplicate \"From: \" as an\n> in-body header when it sends out.\n\nI agree that send-email is the \"right\" place for this functionality in\nthe git toolchain. And indeed, it already does (2):\n\n  $ sed -ne '/$author ne $sender/,+1p' git-send-email.perl\n          if (defined $author and $author ne $sender) {\n                          $message = \"From: $author\\n\\n$message\";\n\nThat being said, like you, I usually pull the patches directly from\nformat-patch into my MUA, and I fix up the headers manually. I suspect\nthere are many others who do the same thing. And each of us has to\neither handle this case manually, or write our own munging code\nourselves for our particular setup.\n\nSo in that sense, even though format-patch is not the right place, it\nmay be useful for it to give tool support to people who do not use the\n\"format-patch to send-email\" workflow. In other words, I would be\nhappy if my short glue shell-script became:\n\n  git format-patch --stdout --sender-is-me \"$@\" >mbox &&\n  mutt -f mbox\n\nand this just handled the case properly, without me having to parse the\nFrom header of each message and munge the messages in my script.\n\nThe arguments against it are:\n\n  1. It is polluting format-patch with MUA cruft.\n\n  2. --sender-is-me (besides being a terrible name) may not be\n     expressive enough. You might want --sender=... depending on the\n     setup of the calling script.\n\nHonestly, though, I send few enough patches made by other people that I\nhave never found it to be a huge burden. This would be a minor\nconvenience to have.\n\n-Peff\n"}]}