{"thread":{"id":"45943","subject":"[PATCH 0/3] interpret-trailers + commit -v bugfix","startedAt":"2017-05-12T18:44:24Z","lastAt":"2017-05-16T06:42:12Z","messageCount":24,"participants":["Brian Malehorn","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"319497","messageId":"20170512050347.30765-1-bmalehorn@gmail.com","threadId":"45943","inReplyTo":null,"subject":"[PATCH 0/3] interpret-trailers + commit -v bugfix","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-12T05:03:44Z","receivedAt":"2017-05-12T18:44:24Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"\nHi all,\n\nThis patch series addresses a bug in interpret-trailers. If the commit\nthat is being editted is \"verbose\", it will contain a scissors string\n(\"-- >8 --\") and a diff. interpret-trailers doesn't interpret the\nscissors and therefore places trailer information after the diff. A\nsimple reproduction is:\n\n\tgit config commit.verbose true\n\tGIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n\t\tgit commit --amend\n\nThis patch series fixes the issue by obeying scissors when computing\ntrailers location.\n\nP.S. This is my first patch series to the git mailing list, to feel free\nto point out any mistakes I made when submitting.\n\n commit.c                      | 65 ++++++++++++++++++++++++++++++++++++++++---\n commit.h                      |  1 +\n mailinfo.c                    | 53 +----------------------------------\n t/t7513-interpret-trailers.sh | 17 +++++++++++\n 4 files changed, 80 insertions(+), 56 deletions(-)\n"},{"id":"319507","messageId":"20170512083131.nb5zyurmeg6n5auo@sigill.intra.peff.net","threadId":"45943","inReplyTo":"20170512050347.30765-2-bmalehorn@gmail.com","subject":"Re: [PATCH 1/3] mailinfo.c: is_scissors_line ends on newline","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-12T08:31:31Z","receivedAt":"2017-05-12T18:44:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 10:03:45PM -0700, Brian Malehorn wrote:\n\n> Needed to work with git interpret-trailers. Since \"line\" is, of course,\n> a line, it will always end with \"\\n\\0\" and therefore we can safely end\n> on \"\\n\".\n> [...]\n> -\tfor (c = line; *c; c++) {\n> +\tfor (c = line; *c != '\\n'; c++) {\n>  \t\tif (isspace(*c)) {\n\nHrm. So I don't think this is _wrong_ per se, but the first thing we do\nin the loop is check isspace(), which a newline should match. And you\ncan't see it from the diff context, but it always hits a continue, which\nwould then advance to the NUL and exit the loop.\n\nSo I'm puzzled why this change is necessary. Am I missing something?\nWe would increase the gap and perforation if we're in them, so I guess\nit could have an impact on the line-length heuristics that come at the\nend of the function. Is that it?\n\n-Peff\n"},{"id":"319512","messageId":"20170512050347.30765-3-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"20170512050347.30765-1-bmalehorn@gmail.com","subject":"[PATCH 2/3] commit.c: add is_scissors_line","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-12T05:03:46Z","receivedAt":"2017-05-12T18:44:54Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"Move is_scissors_line to commit.c and expose it through commit.h.\nThis is needed in commit.c, and mailinfo.c shouldn't really own it.\n---\n commit.c   | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n commit.h   |  1 +\n mailinfo.c | 53 +----------------------------------------------------\n 3 files changed, 54 insertions(+), 52 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex fab826973..041cfa5a9 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1646,6 +1646,58 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n \treturn NULL;\n }\n \n+int is_scissors_line(const char *line)\n+{\n+\tconst char *c;\n+\tint scissors = 0, gap = 0;\n+\tconst char *first_nonblank = NULL, *last_nonblank = NULL;\n+\tint visible, perforation = 0, in_perforation = 0;\n+\n+\tfor (c = line; *c != '\\n'; c++) {\n+\t\tif (isspace(*c)) {\n+\t\t\tif (in_perforation) {\n+\t\t\t\tperforation++;\n+\t\t\t\tgap++;\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\t}\n+\t\tlast_nonblank = c;\n+\t\tif (first_nonblank == NULL)\n+\t\t\tfirst_nonblank = c;\n+\t\tif (*c == '-') {\n+\t\t\tin_perforation = 1;\n+\t\t\tperforation++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif ((!memcmp(c, \">8\", 2) || !memcmp(c, \"8<\", 2) ||\n+\t\t     !memcmp(c, \">%\", 2) || !memcmp(c, \"%<\", 2))) {\n+\t\t\tin_perforation = 1;\n+\t\t\tperforation += 2;\n+\t\t\tscissors += 2;\n+\t\t\tc++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tin_perforation = 0;\n+\t}\n+\n+\t/*\n+\t * The mark must be at least 8 bytes long (e.g. \"-- >8 --\").\n+\t * Even though there can be arbitrary cruft on the same line\n+\t * (e.g. \"cut here\"), in order to avoid misidentification, the\n+\t * perforation must occupy more than a third of the visible\n+\t * width of the line, and dashes and scissors must occupy more\n+\t * than half of the perforation.\n+\t */\n+\n+\tif (first_nonblank && last_nonblank)\n+\t\tvisible = last_nonblank - first_nonblank + 1;\n+\telse\n+\t\tvisible = 0;\n+\treturn (scissors && 8 <= visible &&\n+\t\tvisible < perforation * 3 &&\n+\t\tgap * 2 < perforation);\n+}\n+\n /*\n  * Inspect the given string and determine the true \"end\" of the log message, in\n  * order to find where to put a new Signed-off-by: line.  Ignored are\ndiff --git a/commit.h b/commit.h\nindex 9c12abb91..58cbab1cd 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -353,6 +353,7 @@ extern void free_commit_extra_headers(struct commit_extra_header *extra);\n  */\n extern const char *find_commit_header(const char *msg, const char *key,\n \t\t\t\t      size_t *out_len);\n+extern int is_scissors_line(const char *line);\n \n /* Find the end of the log message, the right place for a new trailer. */\n extern int ignore_non_trailer(const char *buf, size_t len);\ndiff --git a/mailinfo.c b/mailinfo.c\nindex eadd0597f..52af800a5 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"utf8.h\"\n #include \"strbuf.h\"\n+#include \"commit.h\"\n #include \"mailinfo.h\"\n \n static void cleanup_space(struct strbuf *sb)\n@@ -654,58 +655,6 @@ static inline int patchbreak(const struct strbuf *line)\n \treturn 0;\n }\n \n-static int is_scissors_line(const char *line)\n-{\n-\tconst char *c;\n-\tint scissors = 0, gap = 0;\n-\tconst char *first_nonblank = NULL, *last_nonblank = NULL;\n-\tint visible, perforation = 0, in_perforation = 0;\n-\n-\tfor (c = line; *c != '\\n'; c++) {\n-\t\tif (isspace(*c)) {\n-\t\t\tif (in_perforation) {\n-\t\t\t\tperforation++;\n-\t\t\t\tgap++;\n-\t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n-\t\tlast_nonblank = c;\n-\t\tif (first_nonblank == NULL)\n-\t\t\tfirst_nonblank = c;\n-\t\tif (*c == '-') {\n-\t\t\tin_perforation = 1;\n-\t\t\tperforation++;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif ((!memcmp(c, \">8\", 2) || !memcmp(c, \"8<\", 2) ||\n-\t\t     !memcmp(c, \">%\", 2) || !memcmp(c, \"%<\", 2))) {\n-\t\t\tin_perforation = 1;\n-\t\t\tperforation += 2;\n-\t\t\tscissors += 2;\n-\t\t\tc++;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tin_perforation = 0;\n-\t}\n-\n-\t/*\n-\t * The mark must be at least 8 bytes long (e.g. \"-- >8 --\").\n-\t * Even though there can be arbitrary cruft on the same line\n-\t * (e.g. \"cut here\"), in order to avoid misidentification, the\n-\t * perforation must occupy more than a third of the visible\n-\t * width of the line, and dashes and scissors must occupy more\n-\t * than half of the perforation.\n-\t */\n-\n-\tif (first_nonblank && last_nonblank)\n-\t\tvisible = last_nonblank - first_nonblank + 1;\n-\telse\n-\t\tvisible = 0;\n-\treturn (scissors && 8 <= visible &&\n-\t\tvisible < perforation * 3 &&\n-\t\tgap * 2 < perforation);\n-}\n-\n static void flush_inbody_header_accum(struct mailinfo *mi)\n {\n \tif (!mi->inbody_header_accum.len)\n-- \n2.12.3.3.g39c96af\n\n"},{"id":"319515","messageId":"20170512090032.coddhlsrs6s3zm2f@sigill.intra.peff.net","threadId":"45943","inReplyTo":"20170512050347.30765-1-bmalehorn@gmail.com","subject":"Re: [PATCH 0/3] interpret-trailers + commit -v bugfix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-12T09:00:33Z","receivedAt":"2017-05-12T18:44:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 10:03:44PM -0700, Brian Malehorn wrote:\n\n> This patch series addresses a bug in interpret-trailers. If the commit\n> that is being editted is \"verbose\", it will contain a scissors string\n> (\"-- >8 --\") and a diff. interpret-trailers doesn't interpret the\n> scissors and therefore places trailer information after the diff. A\n> simple reproduction is:\n> \n> \tgit config commit.verbose true\n> \tGIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n> \t\tgit commit --amend\n\nAh, I should have read your cover letter more carefully before\nresponding to the third patch (or alternatively, you should have put\nyour motivating example into the third patch's commit message :) ).\n\nYour example definitely makes sense. But it does make me wonder if this\nshould be an option to interpret-trailers, so that it doesn't\naccidentally trigger. Unfortunately you'd have to manually specify it\n(even though you'd presumably have commit.verbose in your config and\nhave long forgotten about it). And at that point, you might as well just\nsay \"--no-verbose\" on the commit command-line.\n\n> P.S. This is my first patch series to the git mailing list, to feel free\n> to point out any mistakes I made when submitting.\n\nMy responses so far have all been critical, so let me be positive for a\nmoment. :)\n\nWelcome to the community. Everything in your patches generally looks\nwell-formatted, etc. And I do think you're on the right track with your\nsolution.\n\nAs I said, I'm a little iffy on doing this unconditionally, but it may\nbe the least-bad solution. I'd just worry about collateral damage to\nsomebody who doesn't use commit.verbose, but has something scissors-like\nin their commit message.\n\nIf you were to switch out is_scissors_line() for checking the exact\ncut_line[] from wt-status.c, I think that would be a big improvement.\nWe'd still have the possibility of a false positive, but it would be\nmuch less likely in practice.\n\n-Peff\n"},{"id":"319548","messageId":"20170512050347.30765-2-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"20170512050347.30765-1-bmalehorn@gmail.com","subject":"[PATCH 1/3] mailinfo.c: is_scissors_line ends on newline","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-12T05:03:45Z","receivedAt":"2017-05-12T18:45:58Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"Needed to work with git interpret-trailers. Since \"line\" is, of course,\na line, it will always end with \"\\n\\0\" and therefore we can safely end\non \"\\n\".\n---\n mailinfo.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a489d9d0f..eadd0597f 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -661,7 +661,7 @@ static int is_scissors_line(const char *line)\n \tconst char *first_nonblank = NULL, *last_nonblank = NULL;\n \tint visible, perforation = 0, in_perforation = 0;\n \n-\tfor (c = line; *c; c++) {\n+\tfor (c = line; *c != '\\n'; c++) {\n \t\tif (isspace(*c)) {\n \t\t\tif (in_perforation) {\n \t\t\t\tperforation++;\n-- \n2.12.3.3.g39c96af\n\n"},{"id":"319553","messageId":"20170512050347.30765-4-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"20170512050347.30765-1-bmalehorn@gmail.com","subject":"[PATCH 3/3] commit.c: skip scissors when computing trailers","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-12T05:03:47Z","receivedAt":"2017-05-12T18:46:09Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"\"scissors\" (\"----- >8 -----\") can be automatically added to commit\nmessages by setting commit.verbose = true. Prevent this from interfering\nwith trailer calculations by automatically skipping over scissors,\ninstead of (usually) treating them as a comment.\n---\n commit.c                      | 13 +++++++++----\n t/t7513-interpret-trailers.sh | 17 +++++++++++++++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 041cfa5a9..9a7b41d09 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1701,10 +1701,10 @@ int is_scissors_line(const char *line)\n /*\n  * Inspect the given string and determine the true \"end\" of the log message, in\n  * order to find where to put a new Signed-off-by: line.  Ignored are\n- * trailing comment lines and blank lines, and also the traditional\n- * \"Conflicts:\" block that is not commented out, so that we can use\n- * \"git commit -s --amend\" on an existing commit that forgot to remove\n- * it.\n+ * trailing comment lines and blank lines.  To support \"git commit -s\n+ * --amend\" on an existing commit, we also ignore \"Conflicts:\".  To\n+ * support \"git commit -v\", we truncate at \"---- >8 ----\" and similar\n+ * scissors lines.\n  *\n  * Returns the number of bytes from the tail to ignore, to be fed as\n  * the second parameter to append_signoff().\n@@ -1723,6 +1723,11 @@ int ignore_non_trailer(const char *buf, size_t len)\n \t\telse\n \t\t\tnext_line++;\n \n+\t\tif (is_scissors_line(&buf[bol])) {\n+\t\t\tif (!boc)\n+\t\t\t\tboc = bol;\n+\t\t\tbreak;\n+\t\t}\n \t\tif (buf[bol] == comment_line_char || buf[bol] == '\\n') {\n \t\t\t/* is this the first of the run of comments? */\n \t\t\tif (!boc)\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 4dd1d7c52..d88d4a4ff 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1258,4 +1258,21 @@ test_expect_success 'with no command and no key' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with scissors' '\n+\tcat >expected <<-EOF &&\n+\t\tmy subject\n+\n+\t\treview: Brian\n+\t\tsign: A U Thor <author@example.com>\n+\t\t# ------------------------ >8 ------------------------\n+\t\tignore this\n+\tEOF\n+\tgit interpret-trailers --trailer review:Brian >actual <<-EOF &&\n+\t\tmy subject\n+\t\t# ------------------------ >8 ------------------------\n+\t\tignore this\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.12.3.3.g39c96af\n\n"},{"id":"319554","messageId":"20170512085253.p3oohlbu2utco2tk@sigill.intra.peff.net","threadId":"45943","inReplyTo":"20170512050347.30765-4-bmalehorn@gmail.com","subject":"Re: [PATCH 3/3] commit.c: skip scissors when computing trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-12T08:52:53Z","receivedAt":"2017-05-12T18:46:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 10:03:47PM -0700, Brian Malehorn wrote:\n\n> \"scissors\" (\"----- >8 -----\") can be automatically added to commit\n> messages by setting commit.verbose = true. Prevent this from interfering\n> with trailer calculations by automatically skipping over scissors,\n> instead of (usually) treating them as a comment.\n\nWhat's the sequence of commands where you end up with a scissors line in\nyour \"commit -v\" output and it gets fed to interpret-trailers? Is it\nwhen you run interpret-trailers from a commit-msg hook? Or do we invoke\nit as part of \"commit -v\" itself?\n\nI ask because I think we can probably come up with a more realistic\ntest, which may impact what the solution looks like (see below).\nGrepping for ignore_non_trailer(), it looks like the issue may be the\nappend_signoff() call in builtin/commit.c? I couldn't get \"git commit -s\n-v\" to fail, though (it handles the signoff with the verbose bits\nremoved).\n\n> diff --git a/commit.c b/commit.c\n> index 041cfa5a9..9a7b41d09 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1701,10 +1701,10 @@ int is_scissors_line(const char *line)\n>  /*\n>   * Inspect the given string and determine the true \"end\" of the log message, in\n>   * order to find where to put a new Signed-off-by: line.  Ignored are\n> - * trailing comment lines and blank lines, and also the traditional\n> - * \"Conflicts:\" block that is not commented out, so that we can use\n> - * \"git commit -s --amend\" on an existing commit that forgot to remove\n> - * it.\n> + * trailing comment lines and blank lines.  To support \"git commit -s\n> + * --amend\" on an existing commit, we also ignore \"Conflicts:\".  To\n> + * support \"git commit -v\", we truncate at \"---- >8 ----\" and similar\n> + * scissors lines.\n>   *\n>   * Returns the number of bytes from the tail to ignore, to be fed as\n>   * the second parameter to append_signoff().\n> @@ -1723,6 +1723,11 @@ int ignore_non_trailer(const char *buf, size_t len)\n>  \t\telse\n>  \t\t\tnext_line++;\n>  \n> +\t\tif (is_scissors_line(&buf[bol])) {\n> +\t\t\tif (!boc)\n> +\t\t\t\tboc = bol;\n> +\t\t\tbreak;\n> +\t\t}\n\nThis unconditionally ignores scissors lines. But that means if you have\nany inside your commit message, we may quietly corrupt the commit\nmessage. It would be better to remove the scissors lines only when we\nknow that we've added them.\n\nAnd that's part of my question above. If this is a call happening inside\nbuiltin/commit.c, then we definitely know when we've added scissors. But\nif it's interpret-trailers being run by a hook, passing the information\ndown to this function is a little tricky.\n\nMaybe I'm being overly picky. We already do a lot of gross heuristic\nstuff here like skipping comments and old-style Conflicts blocks (that\nwe don't even generate anymore!).\n\n-Peff\n"},{"id":"319559","messageId":"20170512084028.f6vv4uz6of6ofadu@sigill.intra.peff.net","threadId":"45943","inReplyTo":"20170512050347.30765-3-bmalehorn@gmail.com","subject":"Re: [PATCH 2/3] commit.c: add is_scissors_line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-12T08:40:28Z","receivedAt":"2017-05-12T18:46:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 10:03:46PM -0700, Brian Malehorn wrote:\n\n> Move is_scissors_line to commit.c and expose it through commit.h.\n> This is needed in commit.c, and mailinfo.c shouldn't really own it.\n\nIt was fine for mailinfo to own it until now, since it was the only\nuser. :) I think there are some unsaid bits in your rationale. Perhaps\nsomething like:\n\n Subject: mailinfo: make is_scissors_line() public\n\n We parse scissors lines only when looking at emails, and thus the\n parsing function has always lived in mailinfo.c. However, we also\n generate scissors lines as part of \"git commit -v\". We should be able\n to reuse the same function to parse them.\n\n Once public, it doesn't really belong in mailinfo.c anymore. A better\n place is commit.c because...[I had trouble filling this in; it's really\n _not_ about commits in particular, but rather about the in-editor\n representation of the commit message used by git-commit].\n\nThinking on it more, though...are the scissors lines generated by \"-v\"\nreally the same as the ones parsed by mailinfo? In the case of mailinfo,\nwe are parsing an externally generated string according to a heuristic\nmicroformat. But with \"commit -v\", we are the ones who wrote the cut\nline in the first place. Shouldn't we be a bit more picky and make sure\nwe parse the exact same cut line?\n\nAnd indeed, we _do_ actually have to parse these cut lines already, when\nwe read the commit message back in. The code is in\nwt_status_truncate_message_at_cut_line(), and it does do an exact match.\nWe should probably be using that rather than making the mailinfo\nis_scissors_line() public.\n\n-Peff\n"},{"id":"319694","messageId":"20170514033923.12870-1-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"20170512090032.coddhlsrs6s3zm2f@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] interpret-trailers + commit -v bugfix","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-14T03:39:22Z","receivedAt":"2017-05-14T03:39:38Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"\n> As I said, I'm a little iffy on doing this unconditionally, but it may\n> be the least-bad solution. I'd just worry about collateral damage to\n> somebody who doesn't use commit.verbose, but has something scissors-like\n> in their commit message.\n> \n> If you were to switch out is_scissors_line() for checking the exact\n> cut_line[] from wt-status.c, I think that would be a big improvement.\n> We'd still have the possibility of a false positive, but it would be\n> much less likely in practice.\n\nYes, you're probably right. Using is_scissors_line() was the path of\nleast resistance to fix my bug, but wasn't really the Right Thing.\n\nI've made a new patch that uses the wt-status.c code to determine\nscissors lines. \"git interpret-trailers\" now uses the same logic as\n\"git commit\". This patch replaces the original 3.\n\nAnd yeah, this is yet another heuristic in interpret-trailers aimed at\ngit commit messages. But it's hardly the first heuristic we've added,\nand I'd say it makes more sense for interpret-trailers and commit to\nparse the same format.\n"},{"id":"319695","messageId":"20170514033923.12870-2-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"20170514033923.12870-1-bmalehorn@gmail.com","subject":"[PATCH] interpret-trailers: obey scissors lines","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-14T03:39:23Z","receivedAt":"2017-05-14T03:39:41Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"If a commit message is being editted as \"verbose\", it will contain a\nscissors string (\"-- >8 --\") and a diff:\n\n    my subject\n\n    # ------------------------ >8 ------------------------\n    # Do not touch the line above.\n    # Everything below will be removed.\n    diff --git a/foo.txt b/foo.txt\n    index 5716ca5..7601807 100644\n    --- a/foo.txt\n    +++ b/foo.txt\n    @@ -1 +1 @@\n    -bar\n    +baz\n\ninterpret-trailers doesn't interpret the scissors and therefore places\ntrailer information after the diff. A simple reproduction is:\n\n    git config commit.verbose true\n    GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n        git commit --amend\n\nThis commit resolves the issue by teaching \"git interpret-trailers\" to\nobey scissors the same way \"git commit\" does.\n---\n builtin/commit.c              |  3 ++-\n commit.c                      |  6 ++++--\n t/t7513-interpret-trailers.sh | 17 +++++++++++++++++\n wt-status.c                   | 11 ++++++-----\n wt-status.h                   |  2 +-\n 5 files changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2de5f6cc6..2ce9c339d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tif (verbose || /* Truncate the message just before the diff, if any. */\n \t    cleanup_mode == CLEANUP_SCISSORS)\n-\t\twt_status_truncate_message_at_cut_line(&sb);\n+\t\tstrbuf_setlen(&sb,\n+\t\t\t      wt_status_last_nonscissors_index(sb.buf, sb.len));\n \n \tif (cleanup_mode != CLEANUP_NONE)\n \t\tstrbuf_stripspace(&sb, cleanup_mode == CLEANUP_ALL);\ndiff --git a/commit.c b/commit.c\nindex fab826973..5d791b703 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -11,6 +11,7 @@\n #include \"commit-slab.h\"\n #include \"prio-queue.h\"\n #include \"sha1-lookup.h\"\n+#include \"wt-status.h\"\n \n static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);\n \n@@ -1662,8 +1663,9 @@ int ignore_non_trailer(const char *buf, size_t len)\n \tint boc = 0;\n \tint bol = 0;\n \tint in_old_conflicts_block = 0;\n+\tsize_t cutoff = wt_status_last_nonscissors_index(buf, len);\n \n-\twhile (bol < len) {\n+\twhile (bol < cutoff) {\n \t\tconst char *next_line = memchr(buf + bol, '\\n', len - bol);\n \n \t\tif (!next_line)\n@@ -1689,5 +1691,5 @@ int ignore_non_trailer(const char *buf, size_t len)\n \t\t}\n \t\tbol = next_line - buf;\n \t}\n-\treturn boc ? len - boc : 0;\n+\treturn boc ? len - boc : len - cutoff;\n }\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 4dd1d7c52..d88d4a4ff 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1258,4 +1258,21 @@ test_expect_success 'with no command and no key' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with scissors' '\n+\tcat >expected <<-EOF &&\n+\t\tmy subject\n+\n+\t\treview: Brian\n+\t\tsign: A U Thor <author@example.com>\n+\t\t# ------------------------ >8 ------------------------\n+\t\tignore this\n+\tEOF\n+\tgit interpret-trailers --trailer review:Brian >actual <<-EOF &&\n+\t\tmy subject\n+\t\t# ------------------------ >8 ------------------------\n+\t\tignore this\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 4bb46781c..8b807d11f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -883,17 +883,18 @@ static void wt_longstatus_print_other(struct wt_status *s,\n \tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n }\n \n-void wt_status_truncate_message_at_cut_line(struct strbuf *buf)\n+size_t wt_status_last_nonscissors_index(const char *s, size_t len)\n {\n \tconst char *p;\n \tstruct strbuf pattern = STRBUF_INIT;\n \n \tstrbuf_addf(&pattern, \"\\n%c %s\", comment_line_char, cut_line);\n-\tif (starts_with(buf->buf, pattern.buf + 1))\n-\t\tstrbuf_setlen(buf, 0);\n-\telse if ((p = strstr(buf->buf, pattern.buf)))\n-\t\tstrbuf_setlen(buf, p - buf->buf + 1);\n+\tif (starts_with(s, pattern.buf + 1))\n+\t\tlen = 0;\n+\telse if ((p = strstr(s, pattern.buf)))\n+\t\tlen = p - s + 1;\n \tstrbuf_release(&pattern);\n+\treturn len;\n }\n \n void wt_status_add_cut_line(FILE *fp)\ndiff --git a/wt-status.h b/wt-status.h\nindex 54fec7703..36a21b492 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -112,7 +112,7 @@ struct wt_status_state {\n \tunsigned char cherry_pick_head_sha1[20];\n };\n \n-void wt_status_truncate_message_at_cut_line(struct strbuf *);\n+size_t wt_status_last_nonscissors_index(const char *s, size_t len);\n void wt_status_add_cut_line(FILE *fp);\n void wt_status_prepare(struct wt_status *s);\n void wt_status_print(struct wt_status *s);\n-- \n2.12.3.9.g050893b\n\n"},{"id":"319697","messageId":"20170514035652.rn5npxxflku6s5k4@sigill.intra.peff.net","threadId":"45943","inReplyTo":"20170514033923.12870-2-bmalehorn@gmail.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-14T03:56:53Z","receivedAt":"2017-05-14T03:57:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 13, 2017 at 08:39:23PM -0700, Brian Malehorn wrote:\n\n> If a commit message is being editted as \"verbose\", it will contain a\n> scissors string (\"-- >8 --\") and a diff:\n> \n>     my subject\n> \n>     # ------------------------ >8 ------------------------\n>     # Do not touch the line above.\n>     # Everything below will be removed.\n>     diff --git a/foo.txt b/foo.txt\n>     index 5716ca5..7601807 100644\n>     --- a/foo.txt\n>     +++ b/foo.txt\n>     @@ -1 +1 @@\n>     -bar\n>     +baz\n> \n> interpret-trailers doesn't interpret the scissors and therefore places\n> trailer information after the diff. A simple reproduction is:\n> \n>     git config commit.verbose true\n>     GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n>         git commit --amend\n> \n> This commit resolves the issue by teaching \"git interpret-trailers\" to\n> obey scissors the same way \"git commit\" does.\n\nOverall, this patch looks good to me. A few comments below.\n\nThe commit message explains the situation much better than the original.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2de5f6cc6..2ce9c339d 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \n>  \tif (verbose || /* Truncate the message just before the diff, if any. */\n>  \t    cleanup_mode == CLEANUP_SCISSORS)\n> -\t\twt_status_truncate_message_at_cut_line(&sb);\n> +\t\tstrbuf_setlen(&sb,\n> +\t\t\t      wt_status_last_nonscissors_index(sb.buf, sb.len));\n\nThis hunk surprised me at first (that we would need to touch commit.c at\nall), but the refactoring makes sense.\n\n> @@ -1662,8 +1663,9 @@ int ignore_non_trailer(const char *buf, size_t len)\n>  \tint boc = 0;\n>  \tint bol = 0;\n>  \tint in_old_conflicts_block = 0;\n> +\tsize_t cutoff = wt_status_last_nonscissors_index(buf, len);\n>  \n> -\twhile (bol < len) {\n> +\twhile (bol < cutoff) {\n>  \t\tconst char *next_line = memchr(buf + bol, '\\n', len - bol);\n>  \n>  \t\tif (!next_line)\n> @@ -1689,5 +1691,5 @@ int ignore_non_trailer(const char *buf, size_t len)\n>  \t\t}\n>  \t\tbol = next_line - buf;\n>  \t}\n> -\treturn boc ? len - boc : 0;\n> +\treturn boc ? len - boc : len - cutoff;\n>  }\n\nThe change to interpret-trailers here ended up delightfully simple (and\nlooks right to me).\n\n> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> index 4dd1d7c52..d88d4a4ff 100755\n> --- a/t/t7513-interpret-trailers.sh\n> +++ b/t/t7513-interpret-trailers.sh\n> @@ -1258,4 +1258,21 @@ test_expect_success 'with no command and no key' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'with scissors' '\n> +\tcat >expected <<-EOF &&\n> +\t\tmy subject\n> +\n> +\t\treview: Brian\n> +\t\tsign: A U Thor <author@example.com>\n> +\t\t# ------------------------ >8 ------------------------\n> +\t\tignore this\n> +\tEOF\n\nTwo minor style nits. One, we'd usually use \"\\EOF\" here unless you\nreally do want to interpolate inside the here document. And two, we\nusually indent the contents to the same level as the outer cat/EOF pair\n(I actually don't mind at all how yours looks, but I just happened to\nnotice that it is slightly unlike our usual style).\n\n> diff --git a/wt-status.c b/wt-status.c\n> index 4bb46781c..8b807d11f 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -883,17 +883,18 @@ static void wt_longstatus_print_other(struct wt_status *s,\n>  \tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n>  }\n>  \n> -void wt_status_truncate_message_at_cut_line(struct strbuf *buf)\n> +size_t wt_status_last_nonscissors_index(const char *s, size_t len)\n\nI can see how the refactor changes this function (and it makes sense),\nbut the \"last nonscissors index\" seems like a funny term. It is really\nthe length, isn't, and therefore one past the last nonscissors index (or\nanother way of putting it: it's the first index of the scissors).\n\nI wonder if it makes sense to call it \"length\".\n\nAnother way to think of it is still as a truncation. Our strip_suffix()\nhelper behaves quite similarly to this (not actually writing into the\nbuffer, but returning the new length). Perhaps something like\n\"wt_status_strip_scissors\" would work.\n\n-Peff\n"},{"id":"319721","messageId":"CACBZZX6hjbiTUzK9CCR1_taE8hzoDm9vH8tGTk-_DW_36Wia+A@mail.gmail.com","threadId":"45943","inReplyTo":"20170514033923.12870-2-bmalehorn@gmail.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-14T07:02:19Z","receivedAt":"2017-05-14T07:02:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 14, 2017 at 5:39 AM, Brian Malehorn <bmalehorn@gmail.com> wrote:\n> If a commit message is being editted as \"verbose\", it will contain a\n\nTypo, should be \"edited\": https://en.wiktionary.org/wiki/editted\n"},{"id":"319724","messageId":"20170514083349.24979-1-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"20170514035652.rn5npxxflku6s5k4@sigill.intra.peff.net","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-14T08:33:48Z","receivedAt":"2017-05-14T08:34:08Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"\n> One, we'd usually use \"\\EOF\" here unless you\n> really do want to interpolate inside the here document.\n\nFixed, and I learned something new.\n\n> And two, we usually indent the contents to the same level as the outer\n> cat/EOF pair\n\nFixed.\n\nI was indenting the same as the other tests in that file. But if the way\nyou described is the preferred way, then sure.\n\n> Another way to think of it is still as a truncation. Our strip_suffix()\n> helper behaves quite similarly to this (not actually writing into the\n> buffer, but returning the new length). Perhaps something like\n> \"wt_status_strip_scissors\" would work.\n\nI agree the name was pretty awkward. I was trying to avoid using the\nword \"truncate\" or \"strip\", since it doesn't make any change to the\nbuffer. But if strip_suffix() is already around it shouldn't be to\nsurprising.\n\nI've now renamed it to \"wt_status_strip_scissors\".\n"},{"id":"319725","messageId":"20170514083349.24979-2-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"20170514083349.24979-1-bmalehorn@gmail.com","subject":"[PATCH] interpret-trailers: obey scissors lines","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-14T08:33:49Z","receivedAt":"2017-05-14T08:34:10Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"If a commit message is being edited as \"verbose\", it will contain a\nscissors string (\"-- >8 --\") and a diff:\n\n    my subject\n\n    # ------------------------ >8 ------------------------\n    # Do not touch the line above.\n    # Everything below will be removed.\n    diff --git a/foo.txt b/foo.txt\n    index 5716ca5..7601807 100644\n    --- a/foo.txt\n    +++ b/foo.txt\n    @@ -1 +1 @@\n    -bar\n    +baz\n\ninterpret-trailers doesn't interpret the scissors and therefore places\ntrailer information after the diff. A simple reproduction is:\n\n    git config commit.verbose true\n    GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n        git commit --amend\n\nThis commit resolves the issue by teaching \"git interpret-trailers\" to\nobey scissors the same way \"git commit\" does.\n---\n builtin/commit.c              |  3 ++-\n commit.c                      | 13 +++++++------\n t/t7513-interpret-trailers.sh | 17 +++++++++++++++++\n wt-status.c                   | 11 ++++++-----\n wt-status.h                   |  2 +-\n 5 files changed, 33 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2de5f6cc6..d223cf3ea 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tif (verbose || /* Truncate the message just before the diff, if any. */\n \t    cleanup_mode == CLEANUP_SCISSORS)\n-\t\twt_status_truncate_message_at_cut_line(&sb);\n+\t\tstrbuf_setlen(&sb,\n+\t\t\t      wt_status_strip_scissors(sb.buf, sb.len));\n \n \tif (cleanup_mode != CLEANUP_NONE)\n \t\tstrbuf_stripspace(&sb, cleanup_mode == CLEANUP_ALL);\ndiff --git a/commit.c b/commit.c\nindex fab826973..6c26ac9b9 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -11,6 +11,7 @@\n #include \"commit-slab.h\"\n #include \"prio-queue.h\"\n #include \"sha1-lookup.h\"\n+#include \"wt-status.h\"\n \n static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);\n \n@@ -1649,10 +1650,9 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n /*\n  * Inspect the given string and determine the true \"end\" of the log message, in\n  * order to find where to put a new Signed-off-by: line.  Ignored are\n- * trailing comment lines and blank lines, and also the traditional\n- * \"Conflicts:\" block that is not commented out, so that we can use\n- * \"git commit -s --amend\" on an existing commit that forgot to remove\n- * it.\n+ * trailing comment lines and blank lines.  To support \"git commit -s\n+ * --amend\" on an existing commit, we also ignore \"Conflicts:\".  To\n+ * support \"git commit -v\", we truncate at scissor lines.\n  *\n  * Returns the number of bytes from the tail to ignore, to be fed as\n  * the second parameter to append_signoff().\n@@ -1662,8 +1662,9 @@ int ignore_non_trailer(const char *buf, size_t len)\n \tint boc = 0;\n \tint bol = 0;\n \tint in_old_conflicts_block = 0;\n+\tsize_t cutoff = wt_status_strip_scissors(buf, len);\n \n-\twhile (bol < len) {\n+\twhile (bol < cutoff) {\n \t\tconst char *next_line = memchr(buf + bol, '\\n', len - bol);\n \n \t\tif (!next_line)\n@@ -1689,5 +1690,5 @@ int ignore_non_trailer(const char *buf, size_t len)\n \t\t}\n \t\tbol = next_line - buf;\n \t}\n-\treturn boc ? len - boc : 0;\n+\treturn boc ? len - boc : len - cutoff;\n }\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 4dd1d7c52..b16d8c5d9 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1258,4 +1258,21 @@ test_expect_success 'with no command and no key' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with scissors' '\n+\tcat >expected <<-\\EOF &&\n+\tmy subject\n+\n+\treview: Brian\n+\tsign: A U Thor <author@example.com>\n+\t# ------------------------ >8 ------------------------\n+\tignore this\n+\tEOF\n+\tgit interpret-trailers --trailer review:Brian >actual <<-\\EOF &&\n+\tmy subject\n+\t# ------------------------ >8 ------------------------\n+\tignore this\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 4bb46781c..2a6bba205 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -883,17 +883,18 @@ static void wt_longstatus_print_other(struct wt_status *s,\n \tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n }\n \n-void wt_status_truncate_message_at_cut_line(struct strbuf *buf)\n+size_t wt_status_strip_scissors(const char *s, size_t len)\n {\n \tconst char *p;\n \tstruct strbuf pattern = STRBUF_INIT;\n \n \tstrbuf_addf(&pattern, \"\\n%c %s\", comment_line_char, cut_line);\n-\tif (starts_with(buf->buf, pattern.buf + 1))\n-\t\tstrbuf_setlen(buf, 0);\n-\telse if ((p = strstr(buf->buf, pattern.buf)))\n-\t\tstrbuf_setlen(buf, p - buf->buf + 1);\n+\tif (starts_with(s, pattern.buf + 1))\n+\t\tlen = 0;\n+\telse if ((p = strstr(s, pattern.buf)))\n+\t\tlen = p - s + 1;\n \tstrbuf_release(&pattern);\n+\treturn len;\n }\n \n void wt_status_add_cut_line(FILE *fp)\ndiff --git a/wt-status.h b/wt-status.h\nindex 54fec7703..52a05fd51 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -112,7 +112,7 @@ struct wt_status_state {\n \tunsigned char cherry_pick_head_sha1[20];\n };\n \n-void wt_status_truncate_message_at_cut_line(struct strbuf *);\n+size_t wt_status_strip_scissors(const char *s, size_t len);\n void wt_status_add_cut_line(FILE *fp);\n void wt_status_prepare(struct wt_status *s);\n void wt_status_print(struct wt_status *s);\n-- \n2.12.3.9.g050893b\n\n"},{"id":"319739","messageId":"xmqqfug6yiks.fsf@gitster.mtv.corp.google.com","threadId":"45943","inReplyTo":"20170514035652.rn5npxxflku6s5k4@sigill.intra.peff.net","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-15T02:12:03Z","receivedAt":"2017-05-15T02:12:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> If a commit message is being editted as \"verbose\", it will contain a\n>> scissors string (\"-- >8 --\") and a diff:\n>> \n>>     my subject\n>> \n>>     # ------------------------ >8 ------------------------\n>>     # Do not touch the line above.\n>>     # Everything below will be removed.\n>>     diff --git a/foo.txt b/foo.txt\n>>     index 5716ca5..7601807 100644\n>>     --- a/foo.txt\n>>     +++ b/foo.txt\n>>     @@ -1 +1 @@\n>>     -bar\n>>     +baz\n>> \n>> interpret-trailers doesn't interpret the scissors and therefore places\n>> trailer information after the diff. A simple reproduction is:\n>> \n>>     git config commit.verbose true\n>>     GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n>>         git commit --amend\n\nSo, \"git commit --amend --verbose\" prepares something like the above\nexample and passes it to the GIT_EDITOR.  The problem description\nindicates that because interpret-trailers (which happens to be used\nas the GIT_EDITOR) treats the cut-line used by \"git commit\" just\nlike any other random line, it tries to find the trailer block way\nbelow, in the patch text.  I agree that it is a problem.  But...\n\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 2de5f6cc6..2ce9c339d 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>  \n>>  \tif (verbose || /* Truncate the message just before the diff, if any. */\n>>  \t    cleanup_mode == CLEANUP_SCISSORS)\n>> -\t\twt_status_truncate_message_at_cut_line(&sb);\n>> +\t\tstrbuf_setlen(&sb,\n>> +\t\t\t      wt_status_last_nonscissors_index(sb.buf, sb.len));\n>\n> This hunk surprised me at first (that we would need to touch commit.c at\n> all), but the refactoring makes sense.\n\nThis still surprises me.  If the problem is in interpret-trailers,\nwhy do we even need to touch cmd_commit()?  If GIT_EDITOR returns us\n\n     my subject\n \n     Acked-by: me\n     # ------------------------ >8 ------------------------\n     # Do not touch the line above.\n     # Everything below will be removed.\n     diff --git a/foo.txt b/foo.txt\n     index 5716ca5..7601807 100644\n     --- a/foo.txt\n     +++ b/foo.txt\n     @@ -1 +1 @@\n     -bar\n     +baz\n\nafter this patch fixes interpret-trailers, wouldn't the cmd_commit()\nfunction work as expected without any change?\n\nPuzzled.\n\nThe proposed log message calls the cut-line \"scissors\", but that is\nprobably a source of this confusion.  The cut-line and scissors do\nnot have much in commmon.  For one thing, scissors is a mechanism to\ndiscard everything _ABOVE_ it.  The cut-line we see in this example,\non the other hand, is about discarding everything _BELOW_ it.\n"},{"id":"319745","messageId":"20170515030726.mmbb43zxqgtbrbuy@sigill.intra.peff.net","threadId":"45943","inReplyTo":"xmqqfug6yiks.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T03:07:26Z","receivedAt":"2017-05-15T03:07:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 11:12:03AM +0900, Junio C Hamano wrote:\n\n> >> diff --git a/builtin/commit.c b/builtin/commit.c\n> >> index 2de5f6cc6..2ce9c339d 100644\n> >> --- a/builtin/commit.c\n> >> +++ b/builtin/commit.c\n> >> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> >>  \n> >>  \tif (verbose || /* Truncate the message just before the diff, if any. */\n> >>  \t    cleanup_mode == CLEANUP_SCISSORS)\n> >> -\t\twt_status_truncate_message_at_cut_line(&sb);\n> >> +\t\tstrbuf_setlen(&sb,\n> >> +\t\t\t      wt_status_last_nonscissors_index(sb.buf, sb.len));\n> >\n> > This hunk surprised me at first (that we would need to touch commit.c at\n> > all), but the refactoring makes sense.\n> \n> This still surprises me.  If the problem is in interpret-trailers,\n> why do we even need to touch cmd_commit()?  If GIT_EDITOR returns us\n\nThe behavior of cmd_commit() shouldn't be changed by the patch. But to\nmake the interface suitable for the new callsite (which doesn't have a\nstrbuf, but a ptr/len buffer), it needs to return the length rather than\nshortening the strbuf. We could leave in place:\n\n  void wt_status_truncate_message_at_cut_line(struct strbuf *sb)\n  {\n\tstrbuf_setlen(sb, wt_status_last_nonscissors_index(sb->buf, sb->len));\n  }\n\nbut it would only have this one caller.\n\nIf I were doing the patch series, I'd probably have split that\nrefactoring into its own patch and discussed the reason separately. I\nwaffled on whether or not to ask Brian to do so (and obviously didn't in\nthe end).\n\n> The proposed log message calls the cut-line \"scissors\", but that is\n> probably a source of this confusion.  The cut-line and scissors do\n> not have much in commmon.  For one thing, scissors is a mechanism to\n> discard everything _ABOVE_ it.  The cut-line we see in this example,\n> on the other hand, is about discarding everything _BELOW_ it.\n\nI suspect the confusion comes from calling it CLEANUP_SCISSORS in the\nquoted context above, then.  Certainly we use scissors for \"snip\neverything above this line\" in mailinfo, but here it is \"snip everything\nafter this line\". I don't think it's wrong to refer to it as a scissors\nline, even though the operation is different.\n\n-Peff\n"},{"id":"319746","messageId":"20170515030833.i4g2e6mx3bjrha3u@sigill.intra.peff.net","threadId":"45943","inReplyTo":"20170514083349.24979-1-bmalehorn@gmail.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T03:08:33Z","receivedAt":"2017-05-15T03:08:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 14, 2017 at 01:33:48AM -0700, Brian Malehorn wrote:\n\n> > And two, we usually indent the contents to the same level as the outer\n> > cat/EOF pair\n> \n> Fixed.\n> \n> I was indenting the same as the other tests in that file. But if the way\n> you described is the preferred way, then sure.\n\nDoh. I didn't notice that. That file _is_ unlike the rest of the code\nbase, but it's generally OK to match the style of surrounding code.\n\n-Peff\n"},{"id":"319748","messageId":"xmqqpofax0af.fsf@gitster.mtv.corp.google.com","threadId":"45943","inReplyTo":"20170515030726.mmbb43zxqgtbrbuy@sigill.intra.peff.net","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-15T03:32:24Z","receivedAt":"2017-05-15T03:32:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, May 15, 2017 at 11:12:03AM +0900, Junio C Hamano wrote:\n>\n>> >> diff --git a/builtin/commit.c b/builtin/commit.c\n>> >> index 2de5f6cc6..2ce9c339d 100644\n>> >> --- a/builtin/commit.c\n>> >> +++ b/builtin/commit.c\n>> >> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>> >>  \n>> >>  \tif (verbose || /* Truncate the message just before the diff, if any. */\n>> >>  \t    cleanup_mode == CLEANUP_SCISSORS)\n>> >> -\t\twt_status_truncate_message_at_cut_line(&sb);\n>> >> +\t\tstrbuf_setlen(&sb,\n>> >> +\t\t\t      wt_status_last_nonscissors_index(sb.buf, sb.len));\n>> >\n>> > This hunk surprised me at first (that we would need to touch commit.c at\n>> > all), but the refactoring makes sense.\n>> \n>> This still surprises me.  If the problem is in interpret-trailers,\n>> why do we even need to touch cmd_commit()?  If GIT_EDITOR returns us\n>\n> The behavior of cmd_commit() shouldn't be changed by the patch. But to\n> make the interface suitable for the new callsite (which doesn't have a\n> strbuf, but a ptr/len buffer), it needs to return the length rather than\n> shortening the strbuf. We could leave in place:\n>\n>   void wt_status_truncate_message_at_cut_line(struct strbuf *sb)\n>   {\n> \tstrbuf_setlen(sb, wt_status_last_nonscissors_index(sb->buf, sb->len));\n>   }\n>\n> but it would only have this one caller.\n>\n> If I were doing the patch series, I'd probably have split that\n> refactoring into its own patch and discussed the reason separately. I\n> waffled on whether or not to ask Brian to do so (and obviously didn't in\n> the end).\n\nI suspect that you would have just explained \"since there is only\none caller, let's open-code it\" in the log message, without making\nthis a two-patch series, and that would also have been perfectly\nunderstandable (and in this case probably preferrable).\n\nSo the patch text would be OK; it was surprising to have the change\nwithout being explained, that's all.\n\nThanks.\n\n"},{"id":"319749","messageId":"20170515033333.g25hdr4fgzuay536@sigill.intra.peff.net","threadId":"45943","inReplyTo":"xmqqpofax0af.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T03:33:33Z","receivedAt":"2017-05-15T03:33:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 12:32:24PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Mon, May 15, 2017 at 11:12:03AM +0900, Junio C Hamano wrote:\n> >\n> >> >> diff --git a/builtin/commit.c b/builtin/commit.c\n> >> >> index 2de5f6cc6..2ce9c339d 100644\n> >> >> --- a/builtin/commit.c\n> >> >> +++ b/builtin/commit.c\n> >> >> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> >> >>  \n> >> >>  \tif (verbose || /* Truncate the message just before the diff, if any. */\n> >> >>  \t    cleanup_mode == CLEANUP_SCISSORS)\n> >> >> -\t\twt_status_truncate_message_at_cut_line(&sb);\n> >> >> +\t\tstrbuf_setlen(&sb,\n> >> >> +\t\t\t      wt_status_last_nonscissors_index(sb.buf, sb.len));\n> >> >\n> >> > This hunk surprised me at first (that we would need to touch commit.c at\n> >> > all), but the refactoring makes sense.\n> >> \n> >> This still surprises me.  If the problem is in interpret-trailers,\n> >> why do we even need to touch cmd_commit()?  If GIT_EDITOR returns us\n> >\n> > The behavior of cmd_commit() shouldn't be changed by the patch. But to\n> > make the interface suitable for the new callsite (which doesn't have a\n> > strbuf, but a ptr/len buffer), it needs to return the length rather than\n> > shortening the strbuf. We could leave in place:\n> >\n> >   void wt_status_truncate_message_at_cut_line(struct strbuf *sb)\n> >   {\n> > \tstrbuf_setlen(sb, wt_status_last_nonscissors_index(sb->buf, sb->len));\n> >   }\n> >\n> > but it would only have this one caller.\n> >\n> > If I were doing the patch series, I'd probably have split that\n> > refactoring into its own patch and discussed the reason separately. I\n> > waffled on whether or not to ask Brian to do so (and obviously didn't in\n> > the end).\n> \n> I suspect that you would have just explained \"since there is only\n> one caller, let's open-code it\" in the log message, without making\n> this a two-patch series, and that would also have been perfectly\n> understandable (and in this case probably preferrable).\n> \n> So the patch text would be OK; it was surprising to have the change\n> without being explained, that's all.\n\nYeah, I'd agree it could use a better explanation in the commit message.\nI was surprised to see it, too. Since both of us were surprised, that's\nprobably a good indicator. :)\n\n-Peff\n"},{"id":"319751","messageId":"xmqqk25iwz7l.fsf@gitster.mtv.corp.google.com","threadId":"45943","inReplyTo":"20170514083349.24979-2-bmalehorn@gmail.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-15T03:55:42Z","receivedAt":"2017-05-15T03:55:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Malehorn <bmalehorn@gmail.com> writes:\n\n> If a commit message is being edited as \"verbose\", it will contain a\n> scissors string (\"-- >8 --\") and a diff:\n>\n>     my subject\n>\n>     # ------------------------ >8 ------------------------\n>     # Do not touch the line above.\n>     # Everything below will be removed.\n>     diff --git a/foo.txt b/foo.txt\n>     index 5716ca5..7601807 100644\n>     --- a/foo.txt\n>     +++ b/foo.txt\n>     @@ -1 +1 @@\n>     -bar\n>     +baz\n>\n> interpret-trailers doesn't interpret the scissors and therefore places\n> trailer information after the diff. A simple reproduction is:\n>\n>     git config commit.verbose true\n>     GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n>         git commit --amend\n>\n> This commit resolves the issue by teaching \"git interpret-trailers\" to\n> obey scissors the same way \"git commit\" does.\n\nI'd prefer to see s/obey scissors lines/honor the cut line/, and the\nlast paragraph rephrased somewhat, perhaps like:\n\n        Subject: interpret-trailers: honor the cut line\n\n        If a commit message is edited with the \"verbose\" option, the\n        buffer will have a cut line and diff after the log message,\n        like so:\n\n             my subject\n\n             # ------------------------ >8 ------------------------\n             # Do not touch the line above.\n             # Everything below will be removed.\n             diff --git a/foo.txt b/foo.txt\n             index 5716ca5..7601807 100644\n             --- a/foo.txt\n             +++ b/foo.txt\n             @@ -1 +1 @@\n             -bar\n             +baz\n\n        \"git interpret-trailers\" is unaware of the cut line, and\n        assumes the trailer block would be at the end of the whole\n        thing.  This can easily be seen with:\n\n             $ GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n               git commit --amend -v\n\n        Teach \"git interpret-trailers\" to notice the cut-line and\n        ignore the remainder of the input when looking for a place\n        to add new trailer block.  This makes it consistent with how\n        \"git commit -v -s\" inserts a new Signed-off-by: line.\n\n        This can be done by the same logic as the existing helper\n        function, wt_status_truncate_message_at_cut_line(), uses,\n        but it wants the caller to pass a strbuf to it.  Because the\n        helper function ignore_non_trailer() used by the command\n        takes a <pointer, length> pair, not a strbuf, steal the\n        logic from wt_status_truncate_message_at_cut_line() to\n        create a new wt_status_strip_scissors() helper function that\n        takes <poiter, length> pair, and make ignore_non_trailer()\n        call it to help \"interpret-trailers\".  Since there is only\n        one caller of wt_status_truncate_message_at_cut_line() in\n        cmd_commit(), rewrite it to call wt_status_strip_scissors()\n        helper instead and remove the old helper that no longer has\n        any caller.\n\nThe last paragraph would have saved me from getting puzzled.\n\nThe mention of \"'commit -v -s' works that way, too\" was my attempt\nto justify why it is OK to make this change unconditionally to\nintepret-trailers, but I am still not 100% convinced with that\nreasoning (or your original log message) that it is a safe thing to\ndo.  It is not like its sole purpose is to serve as GIT_EDITOR for\nthe \"git commit\" command.\n\nThe patch text itself looks sensible.  \n\nWe still need to see your patch with your sign-off, though.\n\nThanks.\n"},{"id":"319754","messageId":"xmqq7f1iwxxd.fsf@gitster.mtv.corp.google.com","threadId":"45943","inReplyTo":"xmqqk25iwz7l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-15T04:23:26Z","receivedAt":"2017-05-15T04:23:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>         This can be done by the same logic as the existing helper\n>         function, wt_status_truncate_message_at_cut_line(), uses,\n>         but it wants the caller to pass a strbuf to it.  Because the\n>         helper function ignore_non_trailer() used by the command\n>         takes a <pointer, length> pair, not a strbuf, steal the\n>         logic from wt_status_truncate_message_at_cut_line() to\n>         create a new wt_status_strip_scissors() helper function that\n>         takes <poiter, length> pair, and make ignore_non_trailer()\n>         call it to help \"interpret-trailers\".  Since there is only\n>         one caller of wt_status_truncate_message_at_cut_line() in\n>         cmd_commit(), rewrite it to call wt_status_strip_scissors()\n>         helper instead and remove the old helper that no longer has\n>         any caller.\n>\n> The last paragraph would have saved me from getting puzzled.\n\nAnd re-reading the above, wt_status_strip_scissors() does not sound\nright to me.  \n\nYes, I am bikeshedding at this point, but names matter.  I prefer to\nkeep the distinction between the two clear by not calling the\ncut_line[] array \"scissors\", but in any case, the new function is\nnot about \"finding\" the cut-line or scissors, in other words,\nwt_status_locate_scissors() is not a good name either (we do not say\n\"not found\" when there is none).\n\nWe are locating the logical end of the commit log message.  It ends\nat \"---\\n\", if exists, in the output of \"git format-patch\", and it\nends at the cut-line, if exists, in \"commit -v\" editor buffer, and\nif there aren't these funny End-Of-Message marks, then we return the\nlocation of the last byte.  IOW, the helper function you added will\nbe the place to add more logic in the future if we ever found the\nneed to notice other kinds of \"logical end\" markers (which may be\nones we will invent in the future) while accepting a proposed commit\nlog message.\n\nSo how about calling it wt_status_locate_end() or something like\nthat, without limiting ourselves only to the \"scissors\"?\n"},{"id":"319892","messageId":"20170516060649.29110-1-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"xmqq7f1iwxxd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-16T06:06:48Z","receivedAt":"2017-05-16T06:07:03Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"\nAlright, I've made a new patch based on this feedback:\n\n* make indentation consistent with rest of test file again\n* rename function to wt_status_locate_end()\n* rephrase commit message; add paragraph about wt_status_locate_end()\n* s/scissors/cut line/\n* Signed-off-by: me\n\n> The mention of \"'commit -v -s' works that way, too\" was my attempt\n> to justify why it is OK to make this change unconditionally to\n> intepret-trailers, but I am still not 100% convinced with that\n> reasoning (or your original log message) that it is a safe thing to\n> do.  It is not like its sole purpose is to serve as GIT_EDITOR for\n> the \"git commit\" command.\n\nYup, this is a heuristic and it will sometimes hit false.  But a far\nmore grievous heuristic is ignoring all lines that begin with '#', yet\nwe do that anyway for its convenience in editing commit messages.  Since\nwe already *partially* honor commit message syntax in\ninterpret-trailers, we might as well *completely* honor it by parsing\ncut lines as well.  At least, that's my reasoning.\n"},{"id":"319893","messageId":"20170516060649.29110-2-bmalehorn@gmail.com","threadId":"45943","inReplyTo":"20170516060649.29110-1-bmalehorn@gmail.com","subject":"[PATCH] interpret-trailers: honor the cut line","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2017-05-16T06:06:49Z","receivedAt":"2017-05-16T06:07:05Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"If a commit message is edited with the \"verbose\" option, the buffer will\nhave a cut line and diff after the log message, like so:\n\n    my subject\n\n    # ------------------------ >8 ------------------------\n    # Do not touch the line above.\n    # Everything below will be removed.\n    diff --git a/foo.txt b/foo.txt\n    index 5716ca5..7601807 100644\n    --- a/foo.txt\n    +++ b/foo.txt\n    @@ -1 +1 @@\n    -bar\n    +baz\n\n\"git interpret-trailers\" is unaware of the cut line, and assumes the\ntrailer block would be at the end of the whole thing.  This can easily\nbe seen with:\n\n     $ GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \\\n       git commit --amend -v\n\nTeach \"git interpret-trailers\" to notice the cut-line and ignore the\nremainder of the input when looking for a place to add new trailer\nblock.  This makes it consistent with how \"git commit -v -s\" inserts a\nnew Signed-off-by: line.\n\nThis can be done by the same logic as the existing helper function,\nwt_status_truncate_message_at_cut_line(), uses, but it wants the caller\nto pass a strbuf to it.  Because the helper function\nignore_non_trailer() used by the command takes a <pointer, length> pair,\nnot a strbuf, steal the logic from\nwt_status_truncate_message_at_cut_line() to create a new\nwt_status_locate_end() helper function that takes <pointer, length>\npair, and make ignore_non_trailer() call it to help\n\"interpret-trailers\".  Since there is only one caller of\nwt_status_truncate_message_at_cut_line() in cmd_commit(), rewrite it to\ncall wt_status_locate_end() helper instead and remove the old helper\nthat no longer has any caller.\n\nSigned-off-by: Brian Malehorn <bmalehorn@gmail.com>\n---\n builtin/commit.c              |  2 +-\n commit.c                      | 13 +++++++------\n t/t7513-interpret-trailers.sh | 17 +++++++++++++++++\n wt-status.c                   | 11 ++++++-----\n wt-status.h                   |  2 +-\n 5 files changed, 32 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2de5f6cc6..6c606d965 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1735,7 +1735,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tif (verbose || /* Truncate the message just before the diff, if any. */\n \t    cleanup_mode == CLEANUP_SCISSORS)\n-\t\twt_status_truncate_message_at_cut_line(&sb);\n+\t\tstrbuf_setlen(&sb, wt_status_locate_end(sb.buf, sb.len));\n \n \tif (cleanup_mode != CLEANUP_NONE)\n \t\tstrbuf_stripspace(&sb, cleanup_mode == CLEANUP_ALL);\ndiff --git a/commit.c b/commit.c\nindex fab826973..fa353acac 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -11,6 +11,7 @@\n #include \"commit-slab.h\"\n #include \"prio-queue.h\"\n #include \"sha1-lookup.h\"\n+#include \"wt-status.h\"\n \n static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);\n \n@@ -1649,10 +1650,9 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n /*\n  * Inspect the given string and determine the true \"end\" of the log message, in\n  * order to find where to put a new Signed-off-by: line.  Ignored are\n- * trailing comment lines and blank lines, and also the traditional\n- * \"Conflicts:\" block that is not commented out, so that we can use\n- * \"git commit -s --amend\" on an existing commit that forgot to remove\n- * it.\n+ * trailing comment lines and blank lines.  To support \"git commit -s\n+ * --amend\" on an existing commit, we also ignore \"Conflicts:\".  To\n+ * support \"git commit -v\", we truncate at cut lines.\n  *\n  * Returns the number of bytes from the tail to ignore, to be fed as\n  * the second parameter to append_signoff().\n@@ -1662,8 +1662,9 @@ int ignore_non_trailer(const char *buf, size_t len)\n \tint boc = 0;\n \tint bol = 0;\n \tint in_old_conflicts_block = 0;\n+\tsize_t cutoff = wt_status_locate_end(buf, len);\n \n-\twhile (bol < len) {\n+\twhile (bol < cutoff) {\n \t\tconst char *next_line = memchr(buf + bol, '\\n', len - bol);\n \n \t\tif (!next_line)\n@@ -1689,5 +1690,5 @@ int ignore_non_trailer(const char *buf, size_t len)\n \t\t}\n \t\tbol = next_line - buf;\n \t}\n-\treturn boc ? len - boc : 0;\n+\treturn boc ? len - boc : len - cutoff;\n }\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 4dd1d7c52..0c6f91c43 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1258,4 +1258,21 @@ test_expect_success 'with no command and no key' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with cut line' '\n+\tcat >expected <<-\\EOF &&\n+\t\tmy subject\n+\n+\t\treview: Brian\n+\t\tsign: A U Thor <author@example.com>\n+\t\t# ------------------------ >8 ------------------------\n+\t\tignore this\n+\tEOF\n+\tgit interpret-trailers --trailer review:Brian >actual <<-\\EOF &&\n+\t\tmy subject\n+\t\t# ------------------------ >8 ------------------------\n+\t\tignore this\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 4bb46781c..afff86c18 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -883,17 +883,18 @@ static void wt_longstatus_print_other(struct wt_status *s,\n \tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n }\n \n-void wt_status_truncate_message_at_cut_line(struct strbuf *buf)\n+size_t wt_status_locate_end(const char *s, size_t len)\n {\n \tconst char *p;\n \tstruct strbuf pattern = STRBUF_INIT;\n \n \tstrbuf_addf(&pattern, \"\\n%c %s\", comment_line_char, cut_line);\n-\tif (starts_with(buf->buf, pattern.buf + 1))\n-\t\tstrbuf_setlen(buf, 0);\n-\telse if ((p = strstr(buf->buf, pattern.buf)))\n-\t\tstrbuf_setlen(buf, p - buf->buf + 1);\n+\tif (starts_with(s, pattern.buf + 1))\n+\t\tlen = 0;\n+\telse if ((p = strstr(s, pattern.buf)))\n+\t\tlen = p - s + 1;\n \tstrbuf_release(&pattern);\n+\treturn len;\n }\n \n void wt_status_add_cut_line(FILE *fp)\ndiff --git a/wt-status.h b/wt-status.h\nindex 54fec7703..889a8d682 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -112,7 +112,7 @@ struct wt_status_state {\n \tunsigned char cherry_pick_head_sha1[20];\n };\n \n-void wt_status_truncate_message_at_cut_line(struct strbuf *);\n+size_t wt_status_locate_end(const char *s, size_t len);\n void wt_status_add_cut_line(FILE *fp);\n void wt_status_prepare(struct wt_status *s);\n void wt_status_print(struct wt_status *s);\n-- \n2.12.3.9.g050893b\n\n"},{"id":"319895","messageId":"xmqqlgpx1ewy.fsf@gitster.mtv.corp.google.com","threadId":"45943","inReplyTo":"20170516060649.29110-1-bmalehorn@gmail.com","subject":"Re: [PATCH] interpret-trailers: obey scissors lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-16T06:42:05Z","receivedAt":"2017-05-16T06:42:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Malehorn <bmalehorn@gmail.com> writes:\n\n>> The mention of \"'commit -v -s' works that way, too\" was my attempt\n>> to justify why it is OK to make this change unconditionally to\n>> intepret-trailers, but I am still not 100% convinced with that\n>> reasoning (or your original log message) that it is a safe thing to\n>> do.  It is not like its sole purpose is to serve as GIT_EDITOR for\n>> the \"git commit\" command.\n>\n> Yup, this is a heuristic and it will sometimes hit false.  But a far\n> more grievous heuristic is ignoring all lines that begin with '#', yet\n> we do that anyway for its convenience in editing commit messages.  Since\n> we already *partially* honor commit message syntax in\n> interpret-trailers, we might as well *completely* honor it by parsing\n> cut lines as well.  At least, that's my reasoning.\n\nI was primarily wondering if we want to protect this behind an\noption to \"interpret-trailers\", instead of making this change\nunconditionally.  But your above reasoning makes sense to me.\n\nThanks.\n"}]}