{"thread":{"id":"47151","subject":"[RFC] cover-at-tip","startedAt":"2017-11-10T10:24:51Z","lastAt":"2017-11-17T01:54:19Z","messageCount":25,"participants":["Nicolas Morey-Chaisemartin","Junio C Hamano","Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"332155","messageId":"357e8afb-4814-c950-1530-530bb6dd5f5a@suse.de","threadId":"47151","inReplyTo":null,"subject":"[RFC] cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-10T10:24:44Z","receivedAt":"2017-11-10T10:24:51Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"Hi,\n\nI'm starting to look into the cover-at-tip topic that I found in the leftover bits (http://www.spinics.net/lists/git/msg259573.html)\n\nHere's a first draft of a patch that adds support for format-patch --cover-at-tip. It compiles and works in my nice and user firnedly test case.\nJust wanted to make sure I was going roughly in the right direction here.\n\n\nI was wondering where is the right place to put a commit_is_cover_at_tip() as the test will be needed in other place as the feature is extended to git am/merge/pull.\n\nFeel free to comment. I know the help is not clear at this point and there's still some work to do on option handling (add a config option, probably have --cover-at-tip imply --cover-letter, etc) and\nsome testing :)\n\n\n---\n Documentation/git-format-patch.txt | 4 ++++\n builtin/log.c | 38 +++++++++++++++++++++++++++++++-------\n 2 files changed, 35 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 6cbe462a7..0ac9d4b71 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -228,6 +228,10 @@ feeding the result to `git send-email`.\n \tcontaining the branch description, shortlog and the overall diffstat. You can\n \tfill in a description in the file before sending it out.\n \n+--[no-]cover-letter-at-tip::\n+\tUse the tip of the series as a cover letter if it is an empty commit.\n+ If no cover-letter is to be sent, the tip is ignored.\n+\n --notes[=]::\n \tAppend the notes (see linkgit:git-notes[1]) for the commit\n \tafter the three-dash line.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6c1fa896a..a0e9e61a3 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -986,11 +986,11 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t struct commit *origin,\n \t\t\t int nr, struct commit **list,\n \t\t\t const char *branch_name,\n-\t\t\t int quiet)\n+\t\t\t int quiet,\n+\t\t\t struct commit *cover_at_tip_commit)\n {\n \tconst char *committer;\n \tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n-\tconst char *msg;\n \tstruct shortlog log;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint i;\n@@ -1021,14 +1021,18 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tif (!branch_name)\n \t\tbranch_name = find_branch_name(rev);\n \n-\tmsg = body;\n \tpp.fmt = CMIT_FMT_EMAIL;\n \tpp.date_mode.type = DATE_RFC2822;\n \tpp.rev = rev;\n \tpp.print_email_subject = 1;\n-\tpp_user_info(&pp, NULL, &sb, committer, encoding);\n-\tpp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);\n-\tpp_remainder(&pp, &msg, &sb, 0);\n+\n+\tif (!cover_at_tip_commit) {\n+\t\tpp_user_info(&pp, NULL, &sb, committer, encoding);\n+\t\tpp_title_line(&pp, &body, &sb, encoding, need_8bit_cte);\n+\t\tpp_remainder(&pp, &body, &sb, 0);\n+\t} else {\n+\t\tpretty_print_commit(&pp, cover_at_tip_commit, &sb);\n+\t}\n \tadd_branch_description(&sb, branch_name);\n \tfprintf(rev->diffopt.file, \"%s\\n\", sb.buf);\n \n@@ -1409,6 +1413,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tint just_numbers = 0;\n \tint ignore_if_in_upstream = 0;\n \tint cover_letter = -1;\n+\tint cover_at_tip = -1;\n+\tstruct commit *cover_at_tip_commit = NULL;\n \tint boundary_count = 0;\n \tint no_binary_diff = 0;\n \tint zero_commit = 0;\n@@ -1437,6 +1443,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"print patches to standard out\")),\n \t\tOPT_BOOL(0, \"cover-letter\", &cover_letter,\n \t\t\t N_(\"generate a cover letter\")),\n+\t\tOPT_BOOL(0, \"cover-at-tip\", &cover_at_tip,\n+\t\t\t N_(\"fill the cover letter with the tip of the branch\")),\n \t\tOPT_BOOL(0, \"numbered-files\", &just_numbers,\n \t\t\t N_(\"use simple number sequence for output file names\")),\n \t\tOPT_STRING(0, \"suffix\", &fmt_patch_suffix, N_(\"sfx\"),\n@@ -1698,6 +1706,21 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (ignore_if_in_upstream && has_commit_patch_id(commit, &ids))\n \t\t\tcontinue;\n \n+\t\tif (!nr && cover_at_tip == 1 && !cover_at_tip_commit) {\n+\t\t\t/* Check that it is a candidate to be a cover at tip\n+\t\t\t * Meaning:\n+\t\t\t * - a single parent (merge commits are not eligible)\n+\t\t\t * - tree oid == parent->tree->oid (no diff to the tree)\n+\t\t\t */\n+\t\t\tif (commit->parents && !commit->parents->next &&\n+\t\t\t !oidcmp(&commit->tree->object.oid,\n+\t\t\t\t &commit->parents->item->tree->object.oid)) {\n+\t\t\t\tcover_at_tip_commit = commit;\n+\t\t\t\tcontinue;\n+\t\t\t} else {\n+\t\t\t\tcover_at_tip = 0;\n+\t\t\t}\n+\t\t}\n \t\tnr++;\n \t\tREALLOC_ARRAY(list, nr);\n \t\tlist[nr - 1] = commit;\n@@ -1748,7 +1771,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (thread)\n \t\t\tgen_message_id(&rev, \"cover\");\n \t\tmake_cover_letter(&rev, use_stdout,\n-\t\t\t\t origin, nr, list, branch_name, quiet);\n+\t\t\t\t origin, nr, list, branch_name, quiet,\n+\t\t\t\t cover_at_tip_commit);\n \t\tprint_bases(&bases, rev.diffopt.file);\n \t\tprint_signature(rev.diffopt.file);\n \t\ttotal++;\n-- \n2.15.0.rc0.1.g69b4f6344.dirty\n\n"},{"id":"332186","messageId":"e1d3ab5b-82e6-8490-8f2e-00c1359c6deb@suse.de","threadId":"47151","inReplyTo":"357e8afb-4814-c950-1530-530bb6dd5f5a@suse.de","subject":"Re: [RFC] cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-10T15:37:49Z","receivedAt":"2017-11-10T15:38:35Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 10/11/2017 à 11:24, Nicolas Morey-Chaisemartin a écrit :\n> Hi,\n>\n> I'm starting to look into the cover-at-tip topic that I found in the leftover bits (http://www.spinics.net/lists/git/msg259573.html)\n>\n> Here's a first draft of a patch that adds support for format-patch --cover-at-tip. It compiles and works in my nice and user firnedly test case.\n> Just wanted to make sure I was going roughly in the right direction here.\n>\n>\n> I was wondering where is the right place to put a commit_is_cover_at_tip() as the test will be needed in other place as the feature is extended to git am/merge/pull.\n>\n> Feel free to comment. I know the help is not clear at this point and there's still some work to do on option handling (add a config option, probably have --cover-at-tip imply --cover-letter, etc) and\n> some testing :)\n>\n>\n> ---\n\nLeaving some more updates and questions before the week end:\n\nI started on git am --cover-at-tip.\n\nThe proposed patch for format-patch does not output any \"---\" to signal the end of the commit log and the begining of the patch in the cover letter.\nThis means that the log summary, the diffstat and the git footer ( --\\n) is seen as part of the commit log. Which is just wrong.\n\nRemoving them would solve the issue but I feel they bring some useful info (or they would not be here).\nAdding a \"---\" between the commit log and those added infos poses another problem: git am does not see an empty patch anymore.\nI would need to add \"some\" level of parsing to am.c to make sure the patch content is just garbage and that there are no actual hunks for that.\n\nI did not find any public API that would allow me to do that, although apply_path/parse_chunk would fit the bill.\nIs that the right way to approach this ?\n\nMy branch is here if anyone want to give a look: https://github.com/nmorey/git/tree/dev/cover-at-tip\n\nNicolas\n\n\n\n\n"},{"id":"332205","messageId":"xmqqbmkaf0yn.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"e1d3ab5b-82e6-8490-8f2e-00c1359c6deb@suse.de","subject":"Re: [RFC] cover-at-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-10T18:22:24Z","receivedAt":"2017-11-10T18:22:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> I would need to add \"some\" level of parsing to am.c to make sure\n> the patch content is just garbage and that there are no actual\n> hunks for that.\n>\n> I did not find any public API that would allow me to do that,\n> although apply_path/parse_chunk would fit the bill. Is that the\n> right way to approach this ?\n\nI do not think you would want this non-patch cruft seen at the apply\nlayer at all. Reading a mailbox, with the help of mailsplit and\nmailinfo, and being the driver to create a series of commits is what\n\"am\" is about, and it would have to notice that the non-patch cruft\nat the beginning is not a patch at all and defer creation of an\nempty commit with that cover material at the end. For each of the\nother messages in the series that has patches, it will need to call\napply to update the index and the working tree so that it can make a\ncommit, but there is NO reason whatsoever to ask help from apply, whose\nsole purpose is to read a patch and make modifications to the index\nand the working tree, to handle the cover material.\n\n\n"},{"id":"332208","messageId":"20171110102825.ae7ccb37a13c5904d252faa1@google.com","threadId":"47151","inReplyTo":"e1d3ab5b-82e6-8490-8f2e-00c1359c6deb@suse.de","subject":"Re: [RFC] cover-at-tip","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-11-10T18:28:25Z","receivedAt":"2017-11-10T18:28:31Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Fri, 10 Nov 2017 16:37:49 +0100\nNicolas Morey-Chaisemartin wrote:\n\n> > Hi,\n> >\n> > I'm starting to look into the cover-at-tip topic that I found in the leftover bits (http://www.spinics.net/lists/git/msg259573.html)\n\nThanks - I personally would find this very useful.\n\n> > Here's a first draft of a patch that adds support for format-patch --cover-at-tip. It compiles and works in my nice and user firnedly test case.\n> > Just wanted to make sure I was going roughly in the right direction here.\n> >\n> >\n> > I was wondering where is the right place to put a commit_is_cover_at_tip() as the test will be needed in other place as the feature is extended to git am/merge/pull.\n\nI think you can put this in (root)/commit.c, especially since that test\noperates on a \"struct commit *\".\n\n> > Feel free to comment. I know the help is not clear at this point and there's still some work to do on option handling (add a config option, probably have --cover-at-tip imply --cover-letter, etc) and\n> > some testing :)\n\nBoth are good ideas. You should probably use a\n--cover-letter={no,auto,yes} instead of the current boolean, so that the\nconfig can use the same options and configuring it to \"auto\" (to use a\ncover letter if the tip is empty and singly-parented, and not to use a\ncover letter otherwise) is meaningful.\n\n> The proposed patch for format-patch does not output any \"---\" to signal the end of the commit log and the begining of the patch in the cover letter.\n> This means that the log summary, the diffstat and the git footer ( --\\n) is seen as part of the commit log. Which is just wrong.\n> \n> Removing them would solve the issue but I feel they bring some useful info (or they would not be here).\n> Adding a \"---\" between the commit log and those added infos poses another problem: git am does not see an empty patch anymore.\n> I would need to add \"some\" level of parsing to am.c to make sure the patch content is just garbage and that there are no actual hunks for that.\n\nCould you just take the message from the commit and put that in the\ncover letter? The summary and diffstat do normally have useful info, but\nif the commit is specifically made to be used only for the cover letter,\nI think that is no longer true.\n"},{"id":"332398","messageId":"bbdeaba0-b757-041d-9649-4150080d4b07@suse.de","threadId":"47151","inReplyTo":"xmqqbmkaf0yn.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-13T07:58:24Z","receivedAt":"2017-11-13T07:58:31Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 10/11/2017 à 19:22, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> I would need to add \"some\" level of parsing to am.c to make sure\n>> the patch content is just garbage and that there are no actual\n>> hunks for that.\n>>\n>> I did not find any public API that would allow me to do that,\n>> although apply_path/parse_chunk would fit the bill. Is that the\n>> right way to approach this ?\n> I do not think you would want this non-patch cruft seen at the apply\n> layer at all. Reading a mailbox, with the help of mailsplit and\n> mailinfo, and being the driver to create a series of commits is what\n> \"am\" is about, and it would have to notice that the non-patch cruft\n> at the beginning is not a patch at all and defer creation of an\n> empty commit with that cover material at the end. For each of the\n> other messages in the series that has patches, it will need to call\n> apply to update the index and the working tree so that it can make a\n> commit, but there is NO reason whatsoever to ask help from apply, whose\n> sole purpose is to read a patch and make modifications to the index\n> and the working tree, to handle the cover material.\n>\n>\n\nI agree this is a \"am\" job. Was just wondering if reusing some of the code from apply (and move it so it makes more sense) wouldnd't make more sense than rewriting a patch detection function.\n\nNicolas\n"},{"id":"332401","messageId":"xmqqh8ty8q5x.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"bbdeaba0-b757-041d-9649-4150080d4b07@suse.de","subject":"Re: [RFC] cover-at-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-13T09:48:58Z","receivedAt":"2017-11-13T09:49:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> I agree this is a \"am\" job. Was just wondering if reusing some of\n> the code from apply (and move it so it makes more sense) wouldnd't\n> make more sense than rewriting a patch detection function.\n\nYes, I understood that and have already given an answer, no?\n\n"},{"id":"332405","messageId":"xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"xmqqh8ty8q5x.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] cover-at-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-13T10:30:06Z","receivedAt":"2017-11-13T10:30:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nicolas Morey-Chaisemartin writes:\n>\n>> I agree this is a \"am\" job. Was just wondering if reusing some of\n>> the code from apply (and move it so it makes more sense) wouldnd't\n>> make more sense than rewriting a patch detection function.\n>\n> Yes, I understood that and have already given an answer, no?\n\nThis was a bit too terse to be useful, so let me try again.\n\nI think the ideal endgame would be to allow people to come up with a\ntopic branch of this shape (illustrated is a three-patch series on\ntop of 'origin'):\n\n ---o---o (origin)\n \\\n 1---2---3\n\nand then add an empty commit C whose log message is used to store\n\"cover letter material\", i.e.\n\n ---o---o (origin)\n \\\n 1---2---3---C (topic)\n\nAnd then you should be able to \n\n (1) merge such branch yourself, coming up with a history like this,\n where merge M uses material from C in the merge log message\n\n ---o---o---x---x---M\n \\ /\n 1---2---3\n\n (2) \"git format-patch origin..topic\" that would create the cover\n letter using material found in C in addition to the usual\n stuff (like shortlog) generated by \"format-patch --cover\",\n followed by these three patches.\n\n (3) \"git format-patch M\" should be able to (a) realize that M\n merges a side branch that is a three-commit series (i.e.\n M^1..M^2), and (b) notice that log message of M has\n human-readable description. Then it grabs the merge log\n message of M and do the same as (2).\n\n (4) \"git am\" the result from (2) or (3) should recreate the\n original history i.e. what we started with with C.\n\n ---o---o (origin)\n \\\n 1---2---3---C (topic)\n\nNow, I _think_ what the machinery needs a lot more is to be able to\ndetect C is an empty commit (when doing (2)), and then you have\nquite a lattitude in designing what exactly such an automated cover\nletter looks like, so that the receiving end (4) can recognize it\nmore easily and (more importantly) more robustly than \"the message\ndoes not have any patch in it\". Not all random messages that do\nnot have a patch in it are cover letters, and that is why I do not\nthink touching any code in the apply layer in an attempt to \"reuse\"\nanything is a bad idea. It will risk butchering the code without\nany real gain, because what we really need to know is *not* absence\nof patch, but presence of cover letter material.\n\nThe simplest would probably be to notice that the subject of one has\n0/N on it, while other messages were labeled with 1/N..(N-1)/N; that\nwould be a lot stronger clue that 0/N has a cover than \"it does not\nhave any patch in it\".\n\nIt may be that we would not just want to identify which message is\ncover and which message is not, but which part of the cover letter\nmessage should go back to the log message of the capping empty\ncommit (and moved to the merge log message). Just like we invented\nthe conventions like scissors, three-dashes, etc., you might want to\ncome up with a way to do so in your format-patch enhancement used to\ndo the (2) and (3) above. Then it will be the matter of teaching\nthat convention to \"am\" used in (4).\n\n"},{"id":"332406","messageId":"72b53257-5525-2622-1233-17cf0e0b4513@suse.de","threadId":"47151","inReplyTo":"xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-13T10:48:11Z","receivedAt":"2017-11-13T10:48:18Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 13/11/2017 à 11:30, Junio C Hamano a écrit :\n> Junio C Hamano writes:\n>\n>> Nicolas Morey-Chaisemartin writes:\n>>\n>>> I agree this is a \"am\" job. Was just wondering if reusing some of\n>>> the code from apply (and move it so it makes more sense) wouldnd't\n>>> make more sense than rewriting a patch detection function.\n>> Yes, I understood that and have already given an answer, no?\n> This was a bit too terse to be useful, so let me try again.\n\nThanks ;)\n\n>\n> I think the ideal endgame would be to allow people to come up with a\n> topic branch of this shape (illustrated is a three-patch series on\n> top of 'origin'):\n>\n> ---o---o (origin)\n> \\\n> 1---2---3\n>\n> and then add an empty commit C whose log message is used to store\n> \"cover letter material\", i.e.\n>\n> ---o---o (origin)\n> \\\n> 1---2---3---C (topic)\n>\n> And then you should be able to \n>\n> (1) merge such branch yourself, coming up with a history like this,\n> where merge M uses material from C in the merge log message\n>\n> ---o---o---x---x---M\n> \\ /\n> 1---2---3\n>\n> (2) \"git format-patch origin..topic\" that would create the cover\n> letter using material found in C in addition to the usual\n> stuff (like shortlog) generated by \"format-patch --cover\",\n> followed by these three patches.\n>\n> (3) \"git format-patch M\" should be able to (a) realize that M\n> merges a side branch that is a three-commit series (i.e.\n> M^1..M^2), and (b) notice that log message of M has\n> human-readable description. Then it grabs the merge log\n> message of M and do the same as (2).\n>\n> (4) \"git am\" the result from (2) or (3) should recreate the\n> original history i.e. what we started with with C.\n>\n> ---o---o (origin)\n> \\\n> 1---2---3---C (topic)\n\nThat what I got from the archive referenced in the leftover bits.\nI'm currently focusing on (2) and (4).\n(3) might come reasonably \"easy\" after (2) but I don't know enough about the internal API yet so I focused on the simplest ;)\n\n>\n> Now, I _think_ what the machinery needs a lot more is to be able to\n> detect C is an empty commit (when doing (2)),\n\nUnless I'm mistaken, this should be covered by the RFC for format-patch:\n\n+\t\t\tif (commit->parents && !commit->parents->next &&\n+\t\t\t !oidcmp(&commit->tree->object.oid,\n+\t\t\t\t &commit->parents->item->tree->object.oid)) {\n+\t\t\t\tcover_at_tip_commit = commit;\n\nAs I said, I'm focusing only for (2) now, so we check there is only one parent and that the commit did not change the tree hash. (meaning an empty commit right ?)\n\n> and then you have\n> quite a lattitude in designing what exactly such an automated cover\n> letter looks like, so that the receiving end (4) can recognize it\n> more easily and (more importantly) more robustly than \"the message\n> does not have any patch in it\". Not all random messages that do\n> not have a patch in it are cover letters, and that is why I do not\n> think touching any code in the apply layer in an attempt to \"reuse\"\n> anything is a bad idea. It will risk butchering the code without\n> any real gain, because what we really need to know is *not* absence\n> of patch, but presence of cover letter material.\n\nAgreed.\n\n> The simplest would probably be to notice that the subject of one has\n> 0/N on it, while other messages were labeled with 1/N..(N-1)/N; that\n> would be a lot stronger clue that 0/N has a cover than \"it does not\n> have any patch in it\".\n\nGood idea, and should be easy enough to put in place.\n\n>\n> It may be that we would not just want to identify which message is\n> cover and which message is not, but which part of the cover letter\n> message should go back to the log message of the capping empty\n> commit (and moved to the merge log message). Just like we invented\n> the conventions like scissors, three-dashes, etc., you might want to\n> come up with a way to do so in your format-patch enhancement used to\n> do the (2) and (3) above. Then it will be the matter of teaching\n> that convention to \"am\" used in (4).\n>\n\nI like that too. It could also allow using (4) on series with a manual cover letter without pulling the shortlog/diffstat stuff into the new topic commit.\n\nNicolas\n\n"},{"id":"332423","messageId":"ab9dde24-bd1f-37b6-5fb4-247937e13432@suse.de","threadId":"47151","inReplyTo":"xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com","subject":"[RFC 0/3] Add support for --cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-13T17:13:27Z","receivedAt":"2017-11-13T17:13:35Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"v2:\n- Enhance mailinfo to parse patch series id from subject\n- Detect cover using mailinfo parsed ids in git am\n- Support multiple patch series in a single run\n\nTODO:\n- Add doc/comments\n- Add tests\n- Add a new \"seperator\" at the end of a cover letter.\n Right now I added a triple dash to all cover letter (manual or cover-at-tip) before shortlog/diff stat\n This allows manually written cover letters to be handle by git am --cover-at-tip without including the shortlog/diffstat but\n breaks compat with older git am as it is seen has a malformed patch. A new separator would solve that.\n\nNote: Cover letter automatically generated with --cover-at-tip ;)\n\nSigned-off-by: Nicolas Morey-Chaisemartin \n---\nNicolas Morey-Chaisemartin (3):\n mailinfo: extract patch series id\n am: semi working --cover-at-tip\n log: add an option to generate cover letter from a branch tip\n\n Documentation/git-format-patch.txt | 4 ++\n builtin/am.c | 143 ++++++++++++++++++++++++++++++++-----\n builtin/log.c | 44 +++++++++---\n mailinfo.c | 35 +++++++++\n mailinfo.h | 2 +\n 5 files changed, 201 insertions(+), 27 deletions(-)\n\n-- \n2.15.0.169.g3d3eebb67.dirty\n\n"},{"id":"332424","messageId":"2252b046-a608-b2aa-d67a-8f7e95fe2dbc@suse.de","threadId":"47151","inReplyTo":"xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com","subject":"[RFC 1/3] mailinfo: extract patch series id","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-13T17:13:32Z","receivedAt":"2017-11-13T17:13:38Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"Extract the patch ID and series length from the [PATCH N/M]\n prefix in the mail header\n\nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n mailinfo.c | 35 +++++++++++++++++++++++++++++++++++\n mailinfo.h | 2 ++\n 2 files changed, 37 insertions(+)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a89db22ab..2ab9d446d 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -308,6 +308,39 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)\n \t\t\tif (!pos)\n \t\t\t\tbreak;\n \t\t\tremove = pos - subject->buf + at + 1;\n+\t\t\tif (7 <= remove &&\n+\t\t\t memmem(subject->buf + at, remove, \"PATCH\", 5)){\n+\t\t\t\t/*\n+\t\t\t\t * Look for a N/M series identifier at\n+\t\t\t\t * the end of the brackets\n+\t\t\t\t */\n+\t\t\t\tint is_series = 1;\n+\t\t\t\tint ret, num, total;\n+\t\t\t\tint pt = at + remove - 2;\n+\n+\t\t\t\tif (!isdigit(subject->buf[pt]))\n+\t\t\t\t\tis_series = 0;\n+\t\t\t\telse {\n+\t\t\t\t\twhile(isdigit(subject->buf[--pt]));\n+\t\t\t\t}\n+\n+\t\t\t\tif(is_series && subject->buf[pt--] != '/')\n+\t\t\t\t\tis_series = 0;\n+\n+\t\t\t\tif (!isdigit(subject->buf[pt]))\n+\t\t\t\t\tis_series = 0;\n+\t\t\t\tif (is_series)\n+\t\t\t\t\twhile(isdigit(subject->buf[--pt]));\n+\n+\t\t\t\tpt++;\n+\n+\t\t\t\tret = sscanf(subject->buf + pt, \"%d/%d]\", &num, &total);\n+\t\t\t\tif (ret == 2){\n+\t\t\t\t\tmi->series_id = num;\n+\t\t\t\t\tmi->series_len = total;\n+\t\t\t\t}\n+\t\t\t}\n+\n \t\t\tif (!mi->keep_non_patch_brackets_in_subject ||\n \t\t\t (7 <= remove &&\n \t\t\t memmem(subject->buf + at, remove, \"PATCH\", 5)))\n@@ -1154,6 +1187,8 @@ void setup_mailinfo(struct mailinfo *mi)\n \tmi->header_stage = 1;\n \tmi->use_inbody_headers = 1;\n \tmi->content_top = mi->content;\n+\tmi->series_id = -1;\n+\tmi->series_len = -1;\n \tgit_config(git_mailinfo_config, mi);\n }\n \ndiff --git a/mailinfo.h b/mailinfo.h\nindex 04a25351d..bd4f7c9e0 100644\n--- a/mailinfo.h\n+++ b/mailinfo.h\n@@ -21,6 +21,8 @@ struct mailinfo {\n \tstruct strbuf **content_top;\n \tstruct strbuf charset;\n \tchar *message_id;\n+\tint series_id; /* Id of the patch within a patch series. -1 if not a patch series */\n+\tint series_len; /* Length of the patch series. -1 if not a patch series */\n \tenum {\n \t\tTE_DONTCARE, TE_QP, TE_BASE64\n \t} transfer_encoding;\n-- \n2.15.0.169.g3d3eebb67.dirty\n\n\n"},{"id":"332425","messageId":"948b19c2-9f2d-de9d-1e0a-6681dc9317a9@suse.de","threadId":"47151","inReplyTo":"xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com","subject":"[RFC 2/3] am: semi working --cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-13T17:13:36Z","receivedAt":"2017-11-13T17:13:42Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"Issue with empty patch detection\n\nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n builtin/am.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 126 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 92c485350..702cbf8e0 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -111,6 +111,11 @@ struct am_state {\n \tchar *msg;\n \tsize_t msg_len;\n \n+\t/* Series metadata */\n+\tint series_id;\n+\tint series_len;\n+\tint cover_id;\n+\n \t/* when --rebasing, records the original commit the patch came from */\n \tstruct object_id orig_commit;\n \n@@ -131,6 +136,8 @@ struct am_state {\n \tint committer_date_is_author_date;\n \tint ignore_date;\n \tint allow_rerere_autoupdate;\n+\tint cover_at_tip;\n+\tint applying_cover;\n \tconst char *sign_commit;\n \tint rebasing;\n };\n@@ -160,6 +167,7 @@ static void am_state_init(struct am_state *state)\n \n \tif (!git_config_get_bool(\"commit.gpgsign\", &gpgsign))\n \t\tstate->sign_commit = gpgsign ? \"\" : NULL;\n+\n }\n \n /**\n@@ -432,6 +440,20 @@ static void am_load(struct am_state *state)\n \tread_state_file(&sb, state, \"utf8\", 1);\n \tstate->utf8 = !strcmp(sb.buf, \"t\");\n \n+\tread_state_file(&sb, state, \"cover-at-tip\", 1);\n+\tstate->cover_at_tip = !strcmp(sb.buf, \"t\");\n+\n+\tif (state->cover_at_tip) {\n+\t\tread_state_file(&sb, state, \"series_id\", 1);\n+\t\tstate->series_id = strtol(sb.buf, NULL, 10);\n+\n+\t\tread_state_file(&sb, state, \"series_len\", 1);\n+\t\tstate->series_len = strtol(sb.buf, NULL, 10);\n+\n+\t\tread_state_file(&sb, state, \"cover_id\", 1);\n+\t\tstate->cover_id = strtol(sb.buf, NULL, 10);\n+\t}\n+\n \tif (file_exists(am_path(state, \"rerere-autoupdate\"))) {\n \t\tread_state_file(&sb, state, \"rerere-autoupdate\", 1);\n \t\tstate->allow_rerere_autoupdate = strcmp(sb.buf, \"t\") ?\n@@ -1020,6 +1042,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \twrite_state_bool(state, \"quiet\", state->quiet);\n \twrite_state_bool(state, \"sign\", state->signoff);\n \twrite_state_bool(state, \"utf8\", state->utf8);\n+\twrite_state_bool(state, \"cover-at-tip\", state->cover_at_tip);\n \n \tif (state->allow_rerere_autoupdate)\n \t\twrite_state_bool(state, \"rerere-autoupdate\",\n@@ -1076,6 +1099,12 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\t\tdelete_ref(NULL, \"ORIG_HEAD\", NULL, 0);\n \t}\n \n+\tif (state->cover_at_tip) {\n+\t\twrite_state_count(state, \"series_id\", state->series_id);\n+\t\twrite_state_count(state, \"series_len\", state->series_len);\n+\t\twrite_state_count(state, \"cover_id\", state->cover_id);\n+\t}\n+\n \t/*\n \t * NOTE: Since the \"next\" and \"last\" files determine if an am_state\n \t * session is in progress, they should be written last.\n@@ -1088,13 +1117,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n }\n \n /**\n- * Increments the patch pointer, and cleans am_state for the application of the\n- * next patch.\n- */\n-static void am_next(struct am_state *state)\n+ * Cleans am_state.\n+ */static void am_clean(struct am_state *state)\n {\n-\tstruct object_id head;\n-\n \tFREE_AND_NULL(state->author_name);\n \tFREE_AND_NULL(state->author_email);\n \tFREE_AND_NULL(state->author_date);\n@@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)\n \n \toidclr(&state->orig_commit);\n \tunlink(am_path(state, \"original-commit\"));\n-\n-\tif (!get_oid(\"HEAD\", &head))\n-\t\twrite_state_text(state, \"abort-safety\", oid_to_hex(&head));\n-\telse\n-\t\twrite_state_text(state, \"abort-safety\", \"\");\n-\n-\tstate->cur++;\n-\twrite_state_count(state, \"next\", state->cur);\n }\n \n /**\n@@ -1274,6 +1291,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tfclose(mi.input);\n \tfclose(mi.output);\n \n+\n \t/* Extract message and author information */\n \tfp = xfopen(am_path(state, \"info\"), \"r\");\n \twhile (!strbuf_getline_lf(&sb, fp)) {\n@@ -1298,9 +1316,30 @@ static int parse_mail(struct am_state *state, const char *mail)\n \t\tgoto finish;\n \t}\n \n-\tif (is_empty_file(am_path(state, \"patch\"))) {\n-\t\tprintf_ln(_(\"Patch is empty.\"));\n-\t\tdie_user_resolve(state);\n+\tif (!state->applying_cover) {\n+\n+\t\tstate->series_id = mi.series_id;\n+\t\tstate->series_len = mi.series_len;\n+\n+\t\tif (state->cover_at_tip) {\n+\t\t\twrite_state_count(state, \"series_id\", state->series_id);\n+\t\t\twrite_state_count(state, \"series_len\", state->series_len);\n+\t\t\twrite_state_count(state, \"cover_id\", state->cover_id);\n+\t\t}\n+\n+\t\tif (mi.series_id == 0){\n+\t\t\tstate->cover_id = state->cur;\n+\t\t\tret = 1;\n+\t\t\tgoto finish;\n+\t\t}\n+\n+\t\tif (is_empty_file(am_path(state, \"patch\"))) {\n+\t\t\t\tprintf_ln(_(\"Patch is empty.\"));\n+\t\t\t\tdie_user_resolve(state);\n+\t\t} else if (state->cur == 1) {\n+\t\t\t/* First mail is not empty. cover-at-tip cannot apply */\n+\t\t\tstate->cover_at_tip = 0;\n+\t\t}\n \t}\n \n \tstrbuf_addstr(&msg, \"\\n\\n\");\n@@ -1776,6 +1815,74 @@ static int do_interactive(struct am_state *state)\n \t}\n }\n \n+\n+/**\n+ * Apply the cover letter of a patch series\n+ */\n+static void do_apply_cover(struct am_state *state)\n+{\n+\tint previous_cur = state->cur;\n+\tconst char *mail;\n+\n+\tam_clean(state);\n+\n+\tstate->cur = state->cover_id;\n+\tstate->applying_cover = 1;\n+\tmail = am_path(state, msgnum(state));\n+\tif (!file_exists(mail))\n+\t\tdie(\"BUG: cover has disapeared\");\n+\n+\tif(parse_mail(state, mail))\n+\t\tdie(\"BUG: first patch is not a cover-letter\");\n+\n+\tif (state->signoff)\n+\t\tam_append_signoff(state);\n+\n+\twrite_author_script(state);\n+\twrite_commit_msg(state);\n+\n+\tif (state->interactive && do_interactive(state))\n+\t\tgoto cancel_cover;\n+\n+\tsay(state, stdout, _(\"Applying: %.*s\"), linelen(state->msg), state->msg);\n+\n+\tdo_commit(state);\n+ cancel_cover:\n+\tstate->cur = previous_cur;\n+\tstate->applying_cover = 0;\n+\n+\t/* Reset series metadata */\n+\tstate->series_len = 0;\n+\tstate->series_id = 0;\n+\tstate->cover_id = 0;\n+}\n+\n+/**\n+ * Increments the patch pointer, and cleans am_state for the application of the\n+ * next patch.\n+ */\n+static void am_next(struct am_state *state)\n+{\n+\tstruct object_id head;\n+\n+\t/* Flush the cover letter if needed */\n+\tif (state->cover_at_tip == 1 &&\n+\t state->series_len > 0 &&\n+\t state->series_id == state->series_len &&\n+\t state->cover_id > 0)\n+\t\tdo_apply_cover(state);\n+\n+\tam_clean(state);\n+\n+\tif (!get_oid(\"HEAD\", &head))\n+\t\twrite_state_text(state, \"abort-safety\", oid_to_hex(&head));\n+\telse\n+\t\twrite_state_text(state, \"abort-safety\", \"\");\n+\n+\tstate->cur++;\n+\twrite_state_count(state, \"next\", state->cur);\n+}\n+\n /**\n * Applies all queued mail.\n *\n@@ -2287,6 +2394,8 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"lie about committer date\")),\n \t\tOPT_BOOL(0, \"ignore-date\", &state.ignore_date,\n \t\t\tN_(\"use current timestamp for author date\")),\n+\t\tOPT_BOOL(0, \"cover-at-tip\", &state.cover_at_tip,\n+\t\t\tN_(\"apply cover letter to the tip of the branch\")),\n \t\tOPT_RERERE_AUTOUPDATE(&state.allow_rerere_autoupdate),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &state.sign_commit, N_(\"key-id\"),\n \t\t N_(\"GPG-sign commits\"),\n-- \n2.15.0.169.g3d3eebb67.dirty\n\n\n"},{"id":"332426","messageId":"936c2b33-3432-f113-d84b-0623246ec673@suse.de","threadId":"47151","inReplyTo":"xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com","subject":"[RFC 3/3] log: add an option to generate cover letter from a branch tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-13T17:13:39Z","receivedAt":"2017-11-13T17:13:46Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"TODO: figure out defaults, add a config option, move tip detection to specific function\n\nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n Documentation/git-format-patch.txt | 4 ++++\n builtin/log.c | 44 +++++++++++++++++++++++++++++---------\n 2 files changed, 38 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 6cbe462a7..0ac9d4b71 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -228,6 +228,10 @@ feeding the result to `git send-email`.\n \tcontaining the branch description, shortlog and the overall diffstat. You can\n \tfill in a description in the file before sending it out.\n \n+--[no-]cover-letter-at-tip::\n+\tUse the tip of the series as a cover letter if it is an empty commit.\n+ If no cover-letter is to be sent, the tip is ignored.\n+\n --notes[=]::\n \tAppend the notes (see linkgit:git-notes[1]) for the commit\n \tafter the three-dash line.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6c1fa896a..292626482 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -986,11 +986,11 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t struct commit *origin,\n \t\t\t int nr, struct commit **list,\n \t\t\t const char *branch_name,\n-\t\t\t int quiet)\n+\t\t\t int quiet,\n+\t\t\t struct commit *cover_at_tip_commit)\n {\n \tconst char *committer;\n-\tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n-\tconst char *msg;\n+\tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\\n\";\n \tstruct shortlog log;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint i;\n@@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tif (!branch_name)\n \t\tbranch_name = find_branch_name(rev);\n \n-\tmsg = body;\n \tpp.fmt = CMIT_FMT_EMAIL;\n \tpp.date_mode.type = DATE_RFC2822;\n \tpp.rev = rev;\n \tpp.print_email_subject = 1;\n-\tpp_user_info(&pp, NULL, &sb, committer, encoding);\n-\tpp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);\n-\tpp_remainder(&pp, &msg, &sb, 0);\n-\tadd_branch_description(&sb, branch_name);\n-\tfprintf(rev->diffopt.file, \"%s\\n\", sb.buf);\n \n+\tif (!cover_at_tip_commit) {\n+\t\tpp_user_info(&pp, NULL, &sb, committer, encoding);\n+\t\tpp_title_line(&pp, &body, &sb, encoding, need_8bit_cte);\n+\t\tpp_remainder(&pp, &body, &sb, 0);\n+\t} else {\n+\t\tpretty_print_commit(&pp, cover_at_tip_commit, &sb);\n+\t}\n+\tadd_branch_description(&sb, branch_name);\n+\tfprintf(rev->diffopt.file, \"%s\", sb.buf);\n+\tfprintf(rev->diffopt.file, \"---\\n\", sb.buf);\n \tstrbuf_release(&sb);\n \n \tshortlog_init(&log);\n@@ -1409,6 +1413,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tint just_numbers = 0;\n \tint ignore_if_in_upstream = 0;\n \tint cover_letter = -1;\n+\tint cover_at_tip = -1;\n+\tstruct commit *cover_at_tip_commit = NULL;\n \tint boundary_count = 0;\n \tint no_binary_diff = 0;\n \tint zero_commit = 0;\n@@ -1437,6 +1443,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"print patches to standard out\")),\n \t\tOPT_BOOL(0, \"cover-letter\", &cover_letter,\n \t\t\t N_(\"generate a cover letter\")),\n+\t\tOPT_BOOL(0, \"cover-at-tip\", &cover_at_tip,\n+\t\t\t N_(\"fill the cover letter with the tip of the branch\")),\n \t\tOPT_BOOL(0, \"numbered-files\", &just_numbers,\n \t\t\t N_(\"use simple number sequence for output file names\")),\n \t\tOPT_STRING(0, \"suffix\", &fmt_patch_suffix, N_(\"sfx\"),\n@@ -1698,6 +1706,21 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (ignore_if_in_upstream && has_commit_patch_id(commit, &ids))\n \t\t\tcontinue;\n \n+\t\tif (!nr && cover_at_tip == 1 && !cover_at_tip_commit) {\n+\t\t\t/* Check that it is a candidate to be a cover at tip\n+\t\t\t * Meaning:\n+\t\t\t * - a single parent (merge commits are not eligible)\n+\t\t\t * - tree oid == parent->tree->oid (no diff to the tree)\n+\t\t\t */\n+\t\t\tif (commit->parents && !commit->parents->next &&\n+\t\t\t !oidcmp(&commit->tree->object.oid,\n+\t\t\t\t &commit->parents->item->tree->object.oid)) {\n+\t\t\t\tcover_at_tip_commit = commit;\n+\t\t\t\tcontinue;\n+\t\t\t} else {\n+\t\t\t\tcover_at_tip = 0;\n+\t\t\t}\n+\t\t}\n \t\tnr++;\n \t\tREALLOC_ARRAY(list, nr);\n \t\tlist[nr - 1] = commit;\n@@ -1748,7 +1771,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (thread)\n \t\t\tgen_message_id(&rev, \"cover\");\n \t\tmake_cover_letter(&rev, use_stdout,\n-\t\t\t\t origin, nr, list, branch_name, quiet);\n+\t\t\t\t origin, nr, list, branch_name, quiet,\n+\t\t\t\t cover_at_tip_commit);\n \t\tprint_bases(&bases, rev.diffopt.file);\n \t\tprint_signature(rev.diffopt.file);\n \t\ttotal++;\n-- \n2.15.0.169.g3d3eebb67.dirty\n\n"},{"id":"332433","messageId":"20171113114010.0d4acb09a7a133f4baee9076@google.com","threadId":"47151","inReplyTo":"ab9dde24-bd1f-37b6-5fb4-247937e13432@suse.de","subject":"Re: [RFC 0/3] Add support for --cover-at-tip","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-11-13T19:40:10Z","receivedAt":"2017-11-13T19:40:20Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Mon, 13 Nov 2017 18:13:27 +0100\nNicolas Morey-Chaisemartin wrote:\n\n> v2:\n> - Enhance mailinfo to parse patch series id from subject\n> - Detect cover using mailinfo parsed ids in git am\n\nI noticed that this was done in the patch set by searching for \"PATCH\" -\nthat is probably quite error-prone as not all patches will have a\nsubject line of that form. It may be better to search for \"0/\" and\nensure that it is immediately followed by an integer.\n\nAlso, it might be worth checking the message IDs to ensure that the\nPATCH M/Ns all indeed are replies to PATCH 0/N.\n\n> - Support multiple patch series in a single run\n\nIs this done? I would have expected that some buffering of messages\nwould be necessary, since you're writing a series of messages of the\nform ... to the commits ....\n\n> TODO:\n> - Add doc/comments\n> - Add tests\n> - Add a new \"seperator\" at the end of a cover letter.\n> Right now I added a triple dash to all cover letter (manual or cover-at-tip) before shortlog/diff stat\n> This allows manually written cover letters to be handle by git am --cover-at-tip without including the shortlog/diffstat but\n> breaks compat with older git am as it is seen has a malformed patch. A new separator would solve that.\n\nI think the triple dash works. I tried \"git am\" with a cover letter with\nno triple dash, and it complains that the commit is empty anyway, so\ncompatibility with older git am might not be such a big issue. (With the\ntriple dash, it indeed complains about a malformed patch, as you\ndescribe.)\n"},{"id":"332437","messageId":"0b916607-4d8a-6574-84e5-6e4f8f484616@suse.de","threadId":"47151","inReplyTo":"20171113114010.0d4acb09a7a133f4baee9076@google.com","subject":"Re: [RFC 0/3] Add support for --cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-13T19:53:51Z","receivedAt":"2017-11-13T19:53:58Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 13/11/2017 à 20:40, Jonathan Tan a écrit :\n> On Mon, 13 Nov 2017 18:13:27 +0100\n> Nicolas Morey-Chaisemartin wrote:\n>\n>> v2:\n>> - Enhance mailinfo to parse patch series id from subject\n>> - Detect cover using mailinfo parsed ids in git am\n> I noticed that this was done in the patch set by searching for \"PATCH\" -\n> that is probably quite error-prone as not all patches will have a\n> subject line of that form. It may be better to search for \"0/\" and\n> ensure that it is immediately followed by an integer.\n\nYes this could be moved. I wasn't sure about the risk of colliding with something else.\n\n\n> Also, it might be worth checking the message IDs to ensure that the\n> PATCH M/Ns all indeed are replies to PATCH 0/N.\n\nDoesn't that only work if mails [1..N] are reply to the cover ? Depending on the mailer and your option, this might not always be the case (I think).\n\n>\n>> - Support multiple patch series in a single run\n> Is this done? I would have expected that some buffering of messages\n> would be necessary, since you're writing a series of messages of the\n> form ... to the commits ... N>.\n>\n\nThis is for git am. It handles the fact that an input may contain multiple series (expect them to be in order).\nWhen reaching patch N/N, it flushed the cover.\n\n\n>> TODO:\n>> - Add doc/comments\n>> - Add tests\n>> - Add a new \"seperator\" at the end of a cover letter.\n>> Right now I added a triple dash to all cover letter (manual or cover-at-tip) before shortlog/diff stat\n>> This allows manually written cover letters to be handle by git am --cover-at-tip without including the shortlog/diffstat but\n>> breaks compat with older git am as it is seen has a malformed patch. A new separator would solve that.\n> I think the triple dash works. I tried \"git am\" with a cover letter with\n> no triple dash, and it complains that the commit is empty anyway, so\n> compatibility with older git am might not be such a big issue. (With the\n> triple dash, it indeed complains about a malformed patch, as you\n> describe.)\n\nIt kinda work but it makes the message difficult to understand for the user.\nAnd change the behaviour from the previous releases.\n\nThanks for the feedback.\n\nNicolas\n"},{"id":"332511","messageId":"xmqqfu9h4div.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"2252b046-a608-b2aa-d67a-8f7e95fe2dbc@suse.de","subject":"Re: [RFC 1/3] mailinfo: extract patch series id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T05:47:52Z","receivedAt":"2017-11-14T05:47:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> Extract the patch ID and series length from the [PATCH N/M]\n> prefix in the mail header\n>\n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> mailinfo.c | 35 +++++++++++++++++++++++++++++++++++\n> mailinfo.h | 2 ++\n> 2 files changed, 37 insertions(+)\n\nAs JTan already mentioned, relying on a substring \"PATCH\" may not be\nvery reliable, and trying to locate \"%d/%d]\" feels like a better\napproach.\n\ncleanup_subject() is called only when keep_subject is false, so this\ncode will not trigger in that case at all. Is this intended?\n\nI would have expected that a new helper function would be written,\nwithout changing existing helpers like cleanup_subject(), and that\nnew helper gets called by handle_info() after output_header_lines()\nhelper is called for the \"Subject\".\n\nWhenever mailinfo learns to glean a new useful piece of information,\nit should be made available to scripts that run \"git mailinfo\", too.\nPerhaps show something like\n\n\tPatchNumber: 1\n\tTotalPatches: 3\n\nat the end of handle_info() to mi->output? I do not think existing\ntools mind too much, even if we added a for-debug output e.g.\n\n\tRawSubject: [RFC 1/3] mailinfo: extract patch series id\n\nto the output.\n"},{"id":"332513","messageId":"xmqqbmk54cy3.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"948b19c2-9f2d-de9d-1e0a-6681dc9317a9@suse.de","subject":"Re: [RFC 2/3] am: semi working --cover-at-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T06:00:20Z","receivedAt":"2017-11-14T06:00:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> \tif (!git_config_get_bool(\"commit.gpgsign\", &gpgsign))\n> \t\tstate->sign_commit = gpgsign ? \"\" : NULL;\n> +\n> }\n\nPlease give at least a cursory proof-reading before sending things\nout.\n\n> @@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)\n> \n> \toidclr(&state->orig_commit);\n> \tunlink(am_path(state, \"original-commit\"));\n> -\n> -\tif (!get_oid(\"HEAD\", &head))\n> -\t\twrite_state_text(state, \"abort-safety\", oid_to_hex(&head));\n> -\telse\n> -\t\twrite_state_text(state, \"abort-safety\", \"\");\n> -\n> -\tstate->cur++;\n> -\twrite_state_count(state, \"next\", state->cur);\n\nMoving these lines to a later part of the source file is fine, but\ncan you do so as a separate preparatory patch that does not change\nanything else? That would unclutter the main patch that adds the\nfeature, allowing better reviews from reviewers.\n\nThe hunk below...\n\n> +/**\n> + * Increments the patch pointer, and cleans am_state for the application of the\n> + * next patch.\n> + */\n> +static void am_next(struct am_state *state)\n> +{\n> +\tstruct object_id head;\n> +\n> +\t/* Flush the cover letter if needed */\n> +\tif (state->cover_at_tip == 1 &&\n> +\t state->series_len > 0 &&\n> +\t state->series_id == state->series_len &&\n> +\t state->cover_id > 0)\n> +\t\tdo_apply_cover(state);\n> +\n> +\tam_clean(state);\n> +\n> +\tif (!get_oid(\"HEAD\", &head))\n> +\t\twrite_state_text(state, \"abort-safety\", oid_to_hex(&head));\n> +\telse\n> +\t\twrite_state_text(state, \"abort-safety\", \"\");\n> +\n> +\tstate->cur++;\n> +\twrite_state_count(state, \"next\", state->cur);\n> +}\n\n... if you followed that \"separate preparatory step\" approach, would\nshow clearly that you added the logic to call do_apply_cover() when\nwe transition after applying the Nth patch of a series with N patches,\nas all the existing lines will show only as unchanged context lines.\n\nBy the way, don't we want to sanity check state->last (which we\nlearn by running \"git mailsplit\" that splits the incoming mbox into\npieces and counts the number of messages) against state->series_len?\nSometimes people send [PATCH 0-6/6], a 6-patch series with a cover\nletter, and then follow-up with [PATCH 7/6]. For somebody like me,\nit would be more convenient if the above code (more-or-less) ignored\nseries_len and called do_apply_cover() after applying the last patch\n(which would be [PATCH 7/6]) based on what state->last says.\n\nThanks.\n\n\n"},{"id":"332514","messageId":"xmqq7eut4cae.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"936c2b33-3432-f113-d84b-0623246ec673@suse.de","subject":"Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T06:14:33Z","receivedAt":"2017-11-14T06:14:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> -\tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n> -\tconst char *msg;\n> +\tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\\n\";\n\nHmmmm.\n\n> @@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \tif (!branch_name)\n> \t\tbranch_name = find_branch_name(rev);\n> \n> -\tmsg = body;\n> \tpp.fmt = CMIT_FMT_EMAIL;\n> \tpp.date_mode.type = DATE_RFC2822;\n> \tpp.rev = rev;\n> \tpp.print_email_subject = 1;\n> -\tpp_user_info(&pp, NULL, &sb, committer, encoding);\n> -\tpp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);\n> -\tpp_remainder(&pp, &msg, &sb, 0);\n> -\tadd_branch_description(&sb, branch_name);\n> -\tfprintf(rev->diffopt.file, \"%s\\n\", sb.buf);\n> \n> +\tif (!cover_at_tip_commit) {\n> +\t\tpp_user_info(&pp, NULL, &sb, committer, encoding);\n> +\t\tpp_title_line(&pp, &body, &sb, encoding, need_8bit_cte);\n> +\t\tpp_remainder(&pp, &body, &sb, 0);\n> +\t} else {\n> +\t\tpretty_print_commit(&pp, cover_at_tip_commit, &sb);\n> +\t}\n> +\tadd_branch_description(&sb, branch_name);\n> +\tfprintf(rev->diffopt.file, \"%s\", sb.buf);\n> +\tfprintf(rev->diffopt.file, \"---\\n\", sb.buf);\n> \tstrbuf_release(&sb);\n\nI would have expected that this feature would not change anything\nother than replacing the constant string *body we unconditionally\nprint with the log message of the empty commit at the tip, so from\nthat expectation, I was hoping that a patch looked nothing more than\nthis:\n\n builtin/log.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6c1fa896ad..0af19d5b36 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -986,6 +986,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t struct commit *origin,\n \t\t\t int nr, struct commit **list,\n \t\t\t const char *branch_name,\n+\t\t\t struct commit *cover,\n \t\t\t int quiet)\n {\n \tconst char *committer;\n@@ -1021,7 +1022,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tif (!branch_name)\n \t\tbranch_name = find_branch_name(rev);\n \n-\tmsg = body;\n+\tif (cover)\n+\t\tmsg = get_cover_from_commit(cover);\n+\telse\n+\t\tmsg = body;\n \tpp.fmt = CMIT_FMT_EMAIL;\n \tpp.date_mode.type = DATE_RFC2822;\n \tpp.rev = rev;\n\n\nplus a newly written function get_cover_from_commit(). Why does\nthis patch need to change a lot more than that, I have to wonder.\n\nThis is totally unrelated, but I wonder if it makes sense to do\nsomething similar for branch.description, too. If the user has a\nmeaningful description prepared with \"git branch --edit-desc\", it is\nsomewhat insulting to the user to still add \"*** BLURB HERE ***\".\n"},{"id":"332523","messageId":"e49e107d-b211-f544-9512-b83eab9dd82a@suse.de","threadId":"47151","inReplyTo":"xmqqfu9h4div.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 1/3] mailinfo: extract patch series id","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-14T09:10:33Z","receivedAt":"2017-11-14T09:10:53Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 14/11/2017 à 06:47, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> Extract the patch ID and series length from the [PATCH N/M]\n>> prefix in the mail header\n>>\n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>> mailinfo.c | 35 +++++++++++++++++++++++++++++++++++\n>> mailinfo.h | 2 ++\n>> 2 files changed, 37 insertions(+)\n> As JTan already mentioned, relying on a substring \"PATCH\" may not be\n> very reliable, and trying to locate \"%d/%d]\" feels like a better\n> approach.\n>\n> cleanup_subject() is called only when keep_subject is false, so this\n> code will not trigger in that case at all. Is this intended?\n>\n> I would have expected that a new helper function would be written,\n> without changing existing helpers like cleanup_subject(), and that\n> new helper gets called by handle_info() after output_header_lines()\n> helper is called for the \"Subject\".\nIsn't that too late ?\nIf keep subject is not set, will cleanup_subject not drop all those info from the strbuf  ?\nBut yes it is not in the right function now.\nBut I would call the function before\n            if (!mi->keep_subject) {\n\n>\n> Whenever mailinfo learns to glean a new useful piece of information,\n> it should be made available to scripts that run \"git mailinfo\", too.\n> Perhaps show something like\n>\n> \tPatchNumber: 1\n> \tTotalPatches: 3\n>\n> at the end of handle_info() to mi->output? I do not think existing\n> tools mind too much, even if we added a for-debug output e.g.\n>\n> \tRawSubject: [RFC 1/3] mailinfo: extract patch series id\n>\n> to the output.\nWill do.\n"},{"id":"332524","messageId":"325a3a6f-9916-29cb-48c0-69aa59e5913d@suse.de","threadId":"47151","inReplyTo":"xmqqbmk54cy3.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 2/3] am: semi working --cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-14T09:17:10Z","receivedAt":"2017-11-14T09:17:19Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 14/11/2017 à 07:00, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> \tif (!git_config_get_bool(\"commit.gpgsign\", &gpgsign))\n>> \t\tstate->sign_commit = gpgsign ? \"\" : NULL;\n>> +\n>> }\n> Please give at least a cursory proof-reading before sending things\n> out.\n>\n>> @@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)\n>> \n>> \toidclr(&state->orig_commit);\n>> \tunlink(am_path(state, \"original-commit\"));\n>> -\n>> -\tif (!get_oid(\"HEAD\", &head))\n>> -\t\twrite_state_text(state, \"abort-safety\", oid_to_hex(&head));\n>> -\telse\n>> -\t\twrite_state_text(state, \"abort-safety\", \"\");\n>> -\n>> -\tstate->cur++;\n>> -\twrite_state_count(state, \"next\", state->cur);\n> Moving these lines to a later part of the source file is fine, but\n> can you do so as a separate preparatory patch that does not change\n> anything else? That would unclutter the main patch that adds the\n> feature, allowing better reviews from reviewers.\n>\n> The hunk below...\n\nSure. I usually do all this later in the process.\n>> +/**\n>> + * Increments the patch pointer, and cleans am_state for the application of the\n>> + * next patch.\n>> + */\n>> +static void am_next(struct am_state *state)\n>> +{\n>> +\tstruct object_id head;\n>> +\n>> +\t/* Flush the cover letter if needed */\n>> +\tif (state->cover_at_tip == 1 &&\n>> +\t state->series_len > 0 &&\n>> +\t state->series_id == state->series_len &&\n>> +\t state->cover_id > 0)\n>> +\t\tdo_apply_cover(state);\n>> +\n>> +\tam_clean(state);\n>> +\n>> +\tif (!get_oid(\"HEAD\", &head))\n>> +\t\twrite_state_text(state, \"abort-safety\", oid_to_hex(&head));\n>> +\telse\n>> +\t\twrite_state_text(state, \"abort-safety\", \"\");\n>> +\n>> +\tstate->cur++;\n>> +\twrite_state_count(state, \"next\", state->cur);\n>> +}\n> ... if you followed that \"separate preparatory step\" approach, would\n> show clearly that you added the logic to call do_apply_cover() when\n> we transition after applying the Nth patch of a series with N patches,\n> as all the existing lines will show only as unchanged context lines.\n\nAgreed. The split of am_clean should probably have its own commit too.\n\n>\n> By the way, don't we want to sanity check state->last (which we\n> learn by running \"git mailsplit\" that splits the incoming mbox into\n> pieces and counts the number of messages) against state->series_len?\n> Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover\n> letter, and then follow-up with [PATCH 7/6]. For somebody like me,\n> it would be more convenient if the above code (more-or-less) ignored\n> series_len and called do_apply_cover() after applying the last patch\n> (which would be [PATCH 7/6]) based on what state->last says.\n\nI thought about that.\nIs there a use case for cover after the last patch works and removes the need to touch am_next (can be done out of the loop in am_run).\n\nIf that multiple series in a mbox is something people do, your concern could be solved by flushing the cover when state->series_id goes back to a lower value.\n\nNicolas\n\n\n"},{"id":"332527","messageId":"92c426bc-5ce9-da7c-5f10-66b5fc46825b@suse.de","threadId":"47151","inReplyTo":"xmqq7eut4cae.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-14T09:28:21Z","receivedAt":"2017-11-14T09:28:29Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 14/11/2017 à 07:14, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> -\tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n>> -\tconst char *msg;\n>> +\tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\\n\";\n> Hmmmm.\n\nThe \\n from fprintf(rev->diffopt.file, \"%s\\n\", sb.buf); added an extra line after the cover which is fine for the default one but changed the commit value (at least at some point while I was playing with scissors)\nso I moved it up there to avoid adding a if() around the fprintf\n\n>\n>> @@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n>> \tif (!branch_name)\n>> \t\tbranch_name = find_branch_name(rev);\n>> \n>> -\tmsg = body;\n>> \tpp.fmt = CMIT_FMT_EMAIL;\n>> \tpp.date_mode.type = DATE_RFC2822;\n>> \tpp.rev = rev;\n>> \tpp.print_email_subject = 1;\n>> -\tpp_user_info(&pp, NULL, &sb, committer, encoding);\n>> -\tpp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);\n>> -\tpp_remainder(&pp, &msg, &sb, 0);\n>> -\tadd_branch_description(&sb, branch_name);\n>> -\tfprintf(rev->diffopt.file, \"%s\\n\", sb.buf);\n>> \n>> +\tif (!cover_at_tip_commit) {\n>> +\t\tpp_user_info(&pp, NULL, &sb, committer, encoding);\n>> +\t\tpp_title_line(&pp, &body, &sb, encoding, need_8bit_cte);\n>> +\t\tpp_remainder(&pp, &body, &sb, 0);\n>> +\t} else {\n>> +\t\tpretty_print_commit(&pp, cover_at_tip_commit, &sb);\n>> +\t}\n>> +\tadd_branch_description(&sb, branch_name);\n>> +\tfprintf(rev->diffopt.file, \"%s\", sb.buf);\n>> +\tfprintf(rev->diffopt.file, \"---\\n\", sb.buf);\n>> \tstrbuf_release(&sb);\n> I would have expected that this feature would not change anything\n> other than replacing the constant string *body we unconditionally\n> print with the log message of the empty commit at the tip, so from\n> that expectation, I was hoping that a patch looked nothing more than\n> this:\n>\n> builtin/log.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 6c1fa896ad..0af19d5b36 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -986,6 +986,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \t\t\t struct commit *origin,\n> \t\t\t int nr, struct commit **list,\n> \t\t\t const char *branch_name,\n> +\t\t\t struct commit *cover,\n> \t\t\t int quiet)\n> {\n> \tconst char *committer;\n> @@ -1021,7 +1022,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \tif (!branch_name)\n> \t\tbranch_name = find_branch_name(rev);\n> \n> -\tmsg = body;\n> +\tif (cover)\n> +\t\tmsg = get_cover_from_commit(cover);\n> +\telse\n> +\t\tmsg = body;\n> \tpp.fmt = CMIT_FMT_EMAIL;\n> \tpp.date_mode.type = DATE_RFC2822;\n> \tpp.rev = rev;\n>\n>\n> plus a newly written function get_cover_from_commit(). Why does\n> this patch need to change a lot more than that, I have to wonder.\n\nThe added code is to avoid the get_cover_from_commit generating a single strbuf that needs to be reparse/resplit by pp_user_info/pp_title_line/pp_remainder.\nThere was a helper that did the right job in my case so I took it ;)\n\nThe triple dash is so that the diffstat/shortlog as not seen as part of the cover letter.\nAs said in the cover letter for this series, it kinda breaks legacy behaviour right now.\nIt should either be printed only for cover-at-tip, or a new separator should be added.\n\n>\n> This is totally unrelated, but I wonder if it makes sense to do\n> something similar for branch.description, too. If the user has a\n> meaningful description prepared with \"git branch --edit-desc\", it is\n> somewhat insulting to the user to still add \"*** BLURB HERE ***\".\n\nI guess so.\nAnd the branch description should probably not be added when using cover-at-tip either.\n\n"},{"id":"332536","messageId":"xmqqo9o52ep0.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"92c426bc-5ce9-da7c-5f10-66b5fc46825b@suse.de","subject":"Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T13:05:31Z","receivedAt":"2017-11-14T13:05:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> The triple dash is so that the diffstat/shortlog as not seen as\n> part of the cover letter. As said in the cover letter for this\n> series, it kinda breaks legacy behaviour right now. It should\n> either be printed only for cover-at-tip, or a new separator should\n> be added.\n\nThis reminds me of a couple of random thoughts I had, so before I\ndisconnect from my terminal and forget about them...\n\n[1] format-patch and am must round-trip.\n\nI mentioned four uses cases around the \"cover letter at the\ntip\" in my earlier message\n\n https://public-inbox.org/git/xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com/\n\nSpecifically, (2) we should be able to run \"format-patch\" and record\nthe log message of the empty commit at the tip to the cover letter,\nand (4) we should be able to accept such output with \"am\" and end up\nwith the same sequence of commits as the original (modulo committer\nidentity and timestamps). So from the output we produce with this\nstep, \"am\" should be able to split the material that came from the\noriginal empty commit from the surrounding cruft like shortlog and\ndiffstat. The output format of this step needs to be designed with\nthat in mind.\n\n[2] reusing cover letter material in merge may not be ideal.\n\nWhen people write a cover letter, they write different things in it.\nWhat they wanted to achieve, why they chose the approach they took,\nhow the series is organized, which part of the series they find iffy\nand/orneeds special attention from the reviewers, where to find the\nprevious iteration, what changed since the previous iterations, etc.\n\nAll of them are to help the reviewers, many of who have already\nlooked at the previous rounds, to understand and judge this round of\nthe submission.\n\nThe message in a merge commit as a part of the final history,\nhowever, cannot refer to anything from \"previous rounds\", as the\nprevious attempts are not part of the final history readers of \"git\nlog\" can refer to whey they are trying to understand the merge.\nWhat exactly goes in a merge commit and how the messages are phrased\nmay be different from project to project, but for this project, I've\nbeen trying to write them in an end-user facing terms, i.e. they are\ndesigned in such a way that \"git log --first-parent --merges\" can be\nread as if they were entries in the release notes, summarizing fixes\nand features by describing their user-visible effects. This is only\none part of what people write in their cover letters (i.e. \"what\nthey wanted to achive\").\n\nSo there probably needs a convention meant to be followed by human\nusers when writing cover letters, so a mechanical process can tell\nwhich part of the text is to be made into the merge commit without\nunderstanding human languages.\n"},{"id":"332537","messageId":"d4fec167-aae2-1990-4e24-faaf286f87f5@suse.de","threadId":"47151","inReplyTo":"xmqqo9o52ep0.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-14T13:40:34Z","receivedAt":"2017-11-14T13:40:41Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 14/11/2017 à 14:05, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> The triple dash is so that the diffstat/shortlog as not seen as\n>> part of the cover letter. As said in the cover letter for this\n>> series, it kinda breaks legacy behaviour right now. It should\n>> either be printed only for cover-at-tip, or a new separator should\n>> be added.\n> This reminds me of a couple of random thoughts I had, so before I\n> disconnect from my terminal and forget about them...\n>\n> [1] format-patch and am must round-trip.\n>\n> I mentioned four uses cases around the \"cover letter at the\n> tip\" in my earlier message\n>\n> https://public-inbox.org/git/xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com/\n>\n> Specifically, (2) we should be able to run \"format-patch\" and record\n> the log message of the empty commit at the tip to the cover letter,\n> and (4) we should be able to accept such output with \"am\" and end up\n> with the same sequence of commits as the original (modulo committer\n> identity and timestamps). So from the output we produce with this\n> step, \"am\" should be able to split the material that came from the\n> original empty commit from the surrounding cruft like shortlog and\n> diffstat. The output format of this step needs to be designed with\n> that in mind.\n\nThis should be the case with the current RFC (apart from the branch description which is kept at the moment).\nThings like git am --signoff will break this of course.\n\n>\n> [2] reusing cover letter material in merge may not be ideal.\n>\n> When people write a cover letter, they write different things in it.\n> What they wanted to achieve, why they chose the approach they took,\n> how the series is organized, which part of the series they find iffy\n> and/orneeds special attention from the reviewers, where to find the\n> previous iteration, what changed since the previous iterations, etc.\n>\n> All of them are to help the reviewers, many of who have already\n> looked at the previous rounds, to understand and judge this round of\n> the submission.\n>\n> The message in a merge commit as a part of the final history,\n> however, cannot refer to anything from \"previous rounds\", as the\n> previous attempts are not part of the final history readers of \"git\n> log\" can refer to whey they are trying to understand the merge.\n> What exactly goes in a merge commit and how the messages are phrased\n> may be different from project to project, but for this project, I've\n> been trying to write them in an end-user facing terms, i.e. they are\n> designed in such a way that \"git log --first-parent --merges\" can be\n> read as if they were entries in the release notes, summarizing fixes\n> and features by describing their user-visible effects. This is only\n> one part of what people write in their cover letters (i.e. \"what\n> they wanted to achive\").\n>\n> So there probably needs a convention meant to be followed by human\n> users when writing cover letters, so a mechanical process can tell\n> which part of the text is to be made into the merge commit without\n> understanding human languages.\n\nIn the long term, I agree this would be nice.\nAs a first step, could we force the --edit option when using --cover-at-tip ?\nThe basic merge message would come from the cover letter but can/should be edited to clear the extra stuff out.\n\nAuto-stripping those extra infos, may also conflicts with the point (3) from your previous mail.\nIf git merge is to keep only the relevant infos, git format-patch from this merge will not be able to access those.\n\nThe advantage of a manual edit is that it's the commiter's choice. Keep the info if the branch is to be resubmitted. Drop them once it's merged for good.\n\nNicolas\n\n\n\n"},{"id":"332539","messageId":"xmqqh8tw3obc.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"d4fec167-aae2-1990-4e24-faaf286f87f5@suse.de","subject":"Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T14:52:23Z","receivedAt":"2017-11-14T14:52:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n>> So there probably needs a convention meant to be followed by human\n>> users when writing cover letters, so a mechanical process can tell\n>> which part of the text is to be made into the merge commit without\n>> understanding human languages.\n>\n> In the long term, I agree this would be nice. As a first step,\n> could we force the --edit option when using --cover-at-tip ? The\n> basic merge message would come from the cover letter but\n> can/should be edited to clear the extra stuff out.\n\nAh, \"git merge\" by default opens the editor these days, so there is\nno need to do a special \"force\"-ing only when we are taking the\ninitial log message material from the empty commit at the tip. So\nthat plan would work rather well, I would imagine.\n\nGood thinking. Thanks.\n"},{"id":"332670","messageId":"4bbaaf33-3796-4aa2-6434-ab79182274f5@suse.de","threadId":"47151","inReplyTo":"325a3a6f-9916-29cb-48c0-69aa59e5913d@suse.de","subject":"Re: [RFC 2/3] am: semi working --cover-at-tip","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-11-16T16:21:40Z","receivedAt":"2017-11-16T16:21:47Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 14/11/2017 à 10:17, Nicolas Morey-Chaisemartin a écrit :\n>\n> Le 14/11/2017 à 07:00, Junio C Hamano a écrit :\n>> Nicolas Morey-Chaisemartin writes:\n>>\n>> By the way, don't we want to sanity check state->last (which we\n>> learn by running \"git mailsplit\" that splits the incoming mbox into\n>> pieces and counts the number of messages) against state->series_len?\n>> Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover\n>> letter, and then follow-up with [PATCH 7/6]. For somebody like me,\n>> it would be more convenient if the above code (more-or-less) ignored\n>> series_len and called do_apply_cover() after applying the last patch\n>> (which would be [PATCH 7/6]) based on what state->last says.\n> I thought about that.\n> Is there a use case for cover after the last patch works and removes the need to touch am_next (can be done out of the loop in am_run).\n\nDo you have an opinion on that ? It has quite a big impact on how things are done !\nSingle series only would mean a simple flush at the end.\nMultiple series makes things a whole lot complex.\nWe do not know the series_id of the next patch until it's parsed by parse_mail.\nWhich would mean interrupting parse_mail when detecting a new series to call parse_mail on the cover_id plus an extra detection at the end of the loop.\n\nNicolas\n"},{"id":"332736","messageId":"xmqqlgj5u0u6.fsf@gitster.mtv.corp.google.com","threadId":"47151","inReplyTo":"4bbaaf33-3796-4aa2-6434-ab79182274f5@suse.de","subject":"Re: [RFC 2/3] am: semi working --cover-at-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-17T01:54:09Z","receivedAt":"2017-11-17T01:54:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n>> I thought about that.\n\n>> Is there a use case for cover after the last patch works and\n>> removes the need to touch am_next (can be done out of the loop in\n>> am_run).\n>\n> Do you have an opinion on that ? It has quite a big impact on how things are done !\n> Single series only would mean a simple flush at the end.\n> Multiple series makes things a whole lot complex.\n\nI am not sure what you even mean. Are you wondering what \"am\"\nshould do to a mbox with, say, these messages?\n\n\t1: [PATCH 0/2] Cover for series A\n\t2: [PATCH 1/2] patch 1 of series A\n\t3: [PATCH 2/2] patch 2 of series A\n\t4: [PATCH 0/3] Cover for series B\n\t5: [PATCH 1/3] patch 1 of series B\n\t6: [PATCH 2/3] patch 2 of series B\n\t7: [PATCH 2/3] patch 3 of series B\n\nRunning \"am\" on the whole thing and expecting covers to become the\ncapping empty commit at the tip is crazy, I would think, for such a\nmbox, as there is no way to tell the command (after it processes 1,\n2 and 3, to create commits out of 1, 2 and then an empty one out of\n0 to finish one topic off) that it must create a new branch to store\nthe next series, and building the second series on top of the\ncapping empty commit at the tip of first series would not make any\nsense---the \"tip empty commit\" for the first series will no longer\nbe at the \"tip\".\n\nWhat I was alluding to is a different case, in which additional\npatches are sent as a follow-up later, ending up in a mbox like\nthis:\n\n\t1: [PATCH 0/2] Cover for series A\n\t2: [PATCH 1/2] patch 1 of series A\n\t3: [PATCH 2/2] patch 2 of series A\n\t4: [PATCH 3/2] patch 3 of series A\n\nNaturally, the cover letter may not list 3/2 in its short-log\nsection, but the description for the overall goal and approach\nof the series in it should still be valid even with patch 3/2.\nThe total number of messages mailsplit gives us would be 4, your\nsubject parser would read \"2\" as the number of patches, which would\nmake the number of messages for the series to be expected \"3\"\n(i.e. \"2\" plus cover), but \"am\" would want to create commits for\npatches 1, 2, and 3, and then cap it with the cover material.\n"}]}