{"thread":{"id":"40178","subject":"[PATCH v2 1/2] trailer: ignore first line of message","startedAt":"2015-08-26T02:51:00Z","lastAt":"2015-08-29T04:00:10Z","messageCount":9,"participants":["Christian Couder","Matthieu Moy","Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"268710","messageId":"1440557461-1078-1-git-send-email-chriscool@tuxfamily.org","threadId":"40178","inReplyTo":null,"subject":"[PATCH v2 1/2] trailer: ignore first line of message","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-08-26T02:51:00Z","receivedAt":"2015-08-26T02:51:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When looking for the start of the trailers in the message\nwe are passed, we should ignore the first line of the message.\n\nThe reason is that if we are passed a patch or commit message\nthen the first line should be the patch title.\nIf we are passed only trailers we can expect that they start\nwith an empty line that can be ignored too.\n\nThis way we can properly process commit messages that have\nonly one line with something that looks like a trailer, for\nexample like \"area of code: change we made\".\n---\n t/t7513-interpret-trailers.sh | 15 ++++++++++++++-\n trailer.c | 4 +++-\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex bd0ab46..9577b4e 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -93,12 +93,25 @@ test_expect_success 'with config option on the command line' '\n \t\tAcked-by: Johan\n \t\tReviewed-by: Peff\n \tEOF\n-\techo \"Acked-by: Johan\" |\n+\t{ echo; echo \"Acked-by: Johan\"; } |\n \tgit -c \"trailer.Acked-by.ifexists=addifdifferent\" interpret-trailers \\\n \t\t--trailer \"Reviewed-by: Peff\" --trailer \"Acked-by: Johan\" >actual &&\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with only a title in the message' '\n+\tcat >expected <<-\\EOF &&\n+\t\tarea: change\n+\n+\t\tReviewed-by: Peff\n+\t\tAcked-by: Johan\n+\tEOF\n+\techo \"area: change\" |\n+\tgit interpret-trailers --trailer \"Reviewed-by: Peff\" \\\n+\t\t--trailer \"Acked-by: Johan\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with config setup' '\n \tgit config trailer.ack.key \"Acked-by: \" &&\n \tcat >expected <<-\\EOF &&\ndiff --git a/trailer.c b/trailer.c\nindex 4b14a56..b808868 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -740,8 +740,10 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t/*\n \t * Get the start of the trailers by looking starting from the end\n \t * for a line with only spaces before lines with one separator.\n+\t * The first line must not be analyzed as the others as it\n+\t * should be either the message title or a blank line.\n \t */\n-\tfor (start = count - 1; start >= 0; start--) {\n+\tfor (start = count - 1; start >= 1; start--) {\n \t\tif (lines[start]->buf[0] == comment_line_char)\n \t\t\tcontinue;\n \t\tif (contains_only_spaces(lines[start]->buf)) {\n-- \n2.5.0.401.g009ef9b.dirty\n"},{"id":"268711","messageId":"1440557461-1078-2-git-send-email-chriscool@tuxfamily.org","threadId":"40178","inReplyTo":"1440557461-1078-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v2 2/2] trailer: support multiline title","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-08-26T02:51:01Z","receivedAt":"2015-08-26T02:51:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"We currently ignore the first line passed to `git interpret-trailers`,\nwhen looking for the beginning of the trailers.\n\nUnfortunately this does not work well when a commit is created with a\nline break in the title, using for example the following command:\n\ngit commit -m 'place of\ncode: change we made'\n\nIn this special case, it is best to look at the first line and if it\ndoes not contain only spaces, consider that the second line is not a\ntrailer.\n---\n t/t7513-interpret-trailers.sh | 14 ++++++++++++++\n trailer.c | 8 +++++++-\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 9577b4e..56efe88 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -112,6 +112,20 @@ test_expect_success 'with only a title in the message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with multiline title in the message' '\n+\tcat >expected <<-\\EOF &&\n+\t\tplace of\n+\t\tcode: change\n+\n+\t\tReviewed-by: Peff\n+\t\tAcked-by: Johan\n+\tEOF\n+\tprintf \"%s\\n%s\\n\" \"place of\" \"code: change\" |\n+\tgit interpret-trailers --trailer \"Reviewed-by: Peff\" \\\n+\t\t--trailer \"Acked-by: Johan\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with config setup' '\n \tgit config trailer.ack.key \"Acked-by: \" &&\n \tcat >expected <<-\\EOF &&\ndiff --git a/trailer.c b/trailer.c\nindex b808868..9a54449 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -759,7 +759,13 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\treturn count;\n \t}\n \n-\treturn only_spaces ? count : 0;\n+\tif (only_spaces)\n+\t\treturn count;\n+\n+\tif (contains_only_spaces(lines[0]->buf))\n+\t\treturn 1;\n+\n+\treturn count;\n }\n \n /* Get the index of the end of the trailers */\n-- \n2.5.0.401.g009ef9b.dirty\n"},{"id":"268714","messageId":"vpqa8tetwfi.fsf@anie.imag.fr","threadId":"40178","inReplyTo":"1440557461-1078-2-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH v2 2/2] trailer: support multiline title","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-26T06:07:13Z","receivedAt":"2015-08-26T06:07:13Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Christian Couder writes:\n\n> Unfortunately this does not work well when a commit is created with a\n> line break in the title, using for example the following command:\n>\n> git commit -m 'place of\n> code: change we made'\n\nI confirm that this patch fixes the behavior for me.\n\nNow, I found another issue: I still have this \"interpret-trailers\" in my\nhooks/commit-msg, and it behaves badly when I use \"git commit -v\". With\n-v, I get a diff in COMMIT_EDITMSG, and interpret-trailers tries to\ninsert my Sign-off within the diff, like this:\n\n # Do not touch the line above.\n # Everything below will be removed.\n diff --git a/git-multimail/README b/git-multimail/README\n index f41906b..93d4751 100644\n \n Signed-off-by: Matthieu Moy \n --- a/git-multimail/README\n +++ b/git-multimail/README\n\nEither commit-msg should be called after stripping the diff from\nCOMMIT_MSG, or interpret-trailers should learn to stop reading when the\npatch starts. I think the first option is better, since it means that\nany commit-msg hook does not have to deal with the patch stuff (my guess\nis that there are many broken commit-msg hooks out there, but people\ndidn't notice because they don't use \"commit -v\").\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268716","messageId":"CA+P7+xqaAKckB0P5oqFS64BWU2H1OsVX8Oq=CLLW8PMJ5fdenQ@mail.gmail.com","threadId":"40178","inReplyTo":"vpqa8tetwfi.fsf@anie.imag.fr","subject":"Re: [PATCH v2 2/2] trailer: support multiline title","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-26T06:28:36Z","receivedAt":"2015-08-26T06:28:36Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 25, 2015 at 11:07 PM, Matthieu Moy\n wrote:\n> Christian Couder writes:\n>\n>> Unfortunately this does not work well when a commit is created with a\n>> line break in the title, using for example the following command:\n>>\n>> git commit -m 'place of\n>> code: change we made'\n>\n> I confirm that this patch fixes the behavior for me.\n>\n> Now, I found another issue: I still have this \"interpret-trailers\" in my\n> hooks/commit-msg, and it behaves badly when I use \"git commit -v\". With\n> -v, I get a diff in COMMIT_EDITMSG, and interpret-trailers tries to\n> insert my Sign-off within the diff, like this:\n>\n> # Do not touch the line above.\n> # Everything below will be removed.\n> diff --git a/git-multimail/README b/git-multimail/README\n> index f41906b..93d4751 100644\n>\n> Signed-off-by: Matthieu Moy \n> --- a/git-multimail/README\n> +++ b/git-multimail/README\n>\n> Either commit-msg should be called after stripping the diff from\n> COMMIT_MSG, or interpret-trailers should learn to stop reading when the\n> patch starts. I think the first option is better, since it means that\n> any commit-msg hook does not have to deal with the patch stuff (my guess\n> is that there are many broken commit-msg hooks out there, but people\n> didn't notice because they don't use \"commit -v\").\n>\n> Thanks,\n>\n\nIt's always confused me why commit -v doesn't prepend every inserted\nline with \"#\" to mark it as a comment.\n\nRegards,\nJake\n"},{"id":"268726","messageId":"CAP8UFD2x8i5wC9JP8d1zAH=d-2BMYWAvpaFmWnu09N5QSG==TA@mail.gmail.com","threadId":"40178","inReplyTo":"CA+P7+xqaAKckB0P5oqFS64BWU2H1OsVX8Oq=CLLW8PMJ5fdenQ@mail.gmail.com","subject":"Re: [PATCH v2 2/2] trailer: support multiline title","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-08-26T14:53:55Z","receivedAt":"2015-08-26T14:53:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Sorry I sent the part below privately by mistake:\n\nOn Tue, Aug 25, 2015 at 11:07 PM, Matthieu Moy\n wrote:\n>\n> Now, I found another issue: I still have this \"interpret-trailers\" in my\n> hooks/commit-msg, and it behaves badly when I use \"git commit -v\". With\n> -v, I get a diff in COMMIT_EDITMSG, and interpret-trailers tries to\n> insert my Sign-off within the diff, like this:\n>\n> # Do not touch the line above.\n> # Everything below will be removed.\n> diff --git a/git-multimail/README b/git-multimail/README\n> index f41906b..93d4751 100644\n>\n> Signed-off-by: Matthieu Moy \n> --- a/git-multimail/README\n> +++ b/git-multimail/README\n\nThis might be a bug. I will have a look.\n\n> Either commit-msg should be called after stripping the diff from\n> COMMIT_MSG, or interpret-trailers should learn to stop reading when the\n> patch starts.\n\nThere is already code to detect a patch in interpret-trailers, but it\nrelies on the patch starting with a line with only three dashes.\n\nSo another option would be to make \"commit -v\" emit a line with three\ndashes just under the \"# Everything below will be removed.\" line.\n\n> I think the first option is better, since it means that\n> any commit-msg hook does not have to deal with the patch stuff (my guess\n> is that there are many broken commit-msg hooks out there, but people\n> didn't notice because they don't use \"commit -v\").\n\nMaybe. I don't know if there is a reason why the commit-msg is called\nbefore removing the patch.\n\nOn Wed, Aug 26, 2015 at 8:28 AM, Jacob Keller wrote:\n>\n> It's always confused me why commit -v doesn't prepend every inserted\n> line with \"#\" to mark it as a comment.\n\nI think that would make interpret-trailers work properly too.\n\nThanks both,\nChristian.\n"},{"id":"268731","messageId":"xmqqk2siujak.fsf@gitster.dls.corp.google.com","threadId":"40178","inReplyTo":"CAP8UFD2x8i5wC9JP8d1zAH=d-2BMYWAvpaFmWnu09N5QSG==TA@mail.gmail.com","subject":"Re: [PATCH v2 2/2] trailer: support multiline title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-26T16:05:39Z","receivedAt":"2015-08-26T16:05:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> There is already code to detect a patch in interpret-trailers, but it\n> relies on the patch starting with a line with only three dashes.\n\nHmm, then it can be taught to notice \"everything below...\" as\nanother marker, right?\n\n> Maybe. I don't know if there is a reason why the commit-msg is called\n> before removing the patch.\n\nIs that \"removing\", or are you talking about changing the order from\n\n - prepare log template in-core\n - add comments and patch to that in-core copy\n - write in-core copy out\n - run hook\n - read the hook's result in-core\n - use the message\n\nto\n\n - prepare log template in-core\n - write in-core copy out\n - run hook\n - read the hook's result in-core\n - add comments and patch to that in-core copy\n - use the message\n\nWhile the reordering would certainly stop showing the comments and\npatch, I am not sure if that is a move in the right direction. It\nwill rob from the hooks information that they have traditionally\nbeen given---it will break some hooks.\n\nBut if interpret-trailers is almost there to reliably know where the\nlog message ends, teaching it the one last step would be the right\nthing to do anyway. After all, interpret-trailers was invented\nexactly because we did not want individual hooks to roll their own\nways to detect the end of the message proper, so the command should\nknow where the message ends.\n"},{"id":"268734","messageId":"vpqpp2akovk.fsf@anie.imag.fr","threadId":"40178","inReplyTo":"xmqqk2siujak.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/2] trailer: support multiline title","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-26T16:15:11Z","receivedAt":"2015-08-26T16:15:11Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> While the reordering would certainly stop showing the comments and\n> patch, I am not sure if that is a move in the right direction. It\n> will rob from the hooks information that they have traditionally\n> been given---\n\nThe information given in the comments do not have a 100% stable format,\nand the hook is executed after letting the user possibly edit or delete\nit, so I'm tempted to say that a hook using the commit comment is\nbroken.\n\nUsing the diff in the hook _is_ really broken: it relies on the user\ncalling \"git commit\" with -v, and there's nothing to garantee that.\n\n> it will break some hooks.\n\nIt will also repair some hooks that were broken, but whose breakage was\nnever noticed or never explained.\n\n> After all, interpret-trailers was invented exactly because we did not\n> want individual hooks to roll their own ways to detect the end of the\n> message proper, so the command should know where the message ends.\n\nRight, but you can't prevent people from writting\n\ncommand-that-shows-stuff >> \"$1\"\n\nin their commit-msg hook. And these people will keep wondering why their\nhook \"sometimes doesn't work\" (that's how I considered it for a while,\nit took me a few commits to notice the correlation between \"-v\" and lack\nof sign-off).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268750","messageId":"xmqqegip3k5y.fsf@gitster.dls.corp.google.com","threadId":"40178","inReplyTo":"1440557461-1078-2-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH v2 2/2] trailer: support multiline title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-26T19:48:57Z","receivedAt":"2015-08-26T19:48:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> We currently ignore the first line passed to `git interpret-trailers`,\n> when looking for the beginning of the trailers.\n>\n> Unfortunately this does not work well when a commit is created with a\n> line break in the title, using for example the following command:\n>\n> git commit -m 'place of\n> code: change we made'\n>\n> In this special case, it is best to look at the first line and if it\n> does not contain only spaces, consider that the second line is not a\n> trailer.\n> ---\n\nMissing sign-off, but more importantly, \"let's special case the\nfirst line\" followed by \"oh, that is not enough, we need to check\nthe found one is sensible and tweak it otherwise\" smells like\nincrementally papering over things.\n\nI think the analysis behind the first patch is correct. It stops\nthe backward scan of the main loop to reach there by realizing that\nthe first line, which must be the first line of the patch title\nparagraph, can never be a trailer.\n\nTo extend that correct realization to cover the case where the title\nparagraph has more than one line, the right thing to do is to scan\nforward from the beginning to find the first paragraph break, which\nmust be the end of the title paragraph, and exclude the whole thing,\nwouldn't it?\n\nThat is, I am wondering why the patch is not more like this (there\nmay be off-by-one, but just to illustrate the approach; I didn't\neven compile test this one so...)?\n\nPuzzled...\n\n static int find_trailer_start(struct strbuf **lines, int count)\n {\n-\tint start, only_spaces = 1;\n+\tint start, end_of_title, only_spaces = 1;\n+\n+\t/* The first paragraph is the title and cannot be trailer */\n+\tfor (start = 0; start < count; start++)\n+\t\tif (!lines[start]->len)\n+\t\t\tbreak; /* paragraph break */\n+\tend_of_title = start;\n \n \t/*\n \t * Get the start of the trailers by looking starting from the end\n \t * for a line with only spaces before lines with one separator.\n-\t * The first line must not be analyzed as the others as it\n-\t * should be either the message title or a blank line.\n \t */\n-\tfor (start = count - 1; start >= 1; start--) {\n+\tfor (start = count - 1; start >= end_of_title; start--) {\n \t\tif (lines[start]->buf[0] == comment_line_char)\n \t\t\tcontinue;\n \t\tif (contains_only_spaces(lines[start]->buf)) {\n"},{"id":"268880","messageId":"CAP8UFD2Vm0ObZXfH_D-AgHS3eOrL-9W6jqcAMqsZLWA4ArL=3g@mail.gmail.com","threadId":"40178","inReplyTo":"xmqqegip3k5y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/2] trailer: support multiline title","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-08-29T04:00:10Z","receivedAt":"2015-08-29T04:00:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Aug 26, 2015 at 9:48 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> We currently ignore the first line passed to `git interpret-trailers`,\n>> when looking for the beginning of the trailers.\n>>\n>> Unfortunately this does not work well when a commit is created with a\n>> line break in the title, using for example the following command:\n>>\n>> git commit -m 'place of\n>> code: change we made'\n>>\n>> In this special case, it is best to look at the first line and if it\n>> does not contain only spaces, consider that the second line is not a\n>> trailer.\n>> ---\n>\n> Missing sign-off,\n\nOk, will add it.\n\n[...]\n\n> I think the analysis behind the first patch is correct. It stops\n> the backward scan of the main loop to reach there by realizing that\n> the first line, which must be the first line of the patch title\n> paragraph, can never be a trailer.\n>\n> To extend that correct realization to cover the case where the title\n> paragraph has more than one line, the right thing to do is to scan\n> forward from the beginning to find the first paragraph break, which\n> must be the end of the title paragraph, and exclude the whole thing,\n> wouldn't it?\n>\n> That is, I am wondering why the patch is not more like this (there\n> may be off-by-one, but just to illustrate the approach; I didn't\n> even compile test this one so...)?\n>\n> Puzzled...\n>\n> static int find_trailer_start(struct strbuf **lines, int count)\n> {\n> - int start, only_spaces = 1;\n> + int start, end_of_title, only_spaces = 1;\n> +\n> + /* The first paragraph is the title and cannot be trailer */\n> + for (start = 0; start < count; start++)\n> + if (!lines[start]->len)\n> + break; /* paragraph break */\n> + end_of_title = start;\n>\n> /*\n> * Get the start of the trailers by looking starting from the end\n> * for a line with only spaces before lines with one separator.\n> - * The first line must not be analyzed as the others as it\n> - * should be either the message title or a blank line.\n> */\n> - for (start = count - 1; start >= 1; start--) {\n> + for (start = count - 1; start >= end_of_title; start--) {\n> if (lines[start]->buf[0] == comment_line_char)\n> continue;\n> if (contains_only_spaces(lines[start]->buf)) {\n\nYeah, we can do that. It will be clearer.\n\nThanks,\nChristian.\n"}]}