{"thread":{"id":"58396","subject":"[PATCH] format-patch: warn if commit msg contains a patch delimiter","startedAt":"2022-09-04T23:12:24Z","lastAt":"2022-09-09T16:48:42Z","messageCount":13,"participants":["Matheus Tavares","Ævar Arnfjörð Bjarmason","René Scharfe","Phillip Wood","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"462584","messageId":"d0b577825124ac684ab304d3a1395f3d2d0708e8.1662333027.git.matheus.bernardino@usp.br","threadId":"58396","inReplyTo":null,"subject":"[PATCH] format-patch: warn if commit msg contains a patch delimiter","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-09-04T23:12:05Z","receivedAt":"2022-09-04T23:12:24Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When applying a patch, `git am` looks for special delimiter strings\n(such as \"---\") to know where the message ends and the actual diff\nstarts. If one of these strings appears in the commit message itself,\n`am` might get confused and fail to apply the patch properly. This has\nalready caused inconveniences in the past [1][2]. To help avoid such\nproblem, let's make `git format-patch` warn on commit messages\ncontaining one of the said strings.\n\n[1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/\n[2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n builtin/log.c           |  1 +\n log-tree.c              |  1 +\n mailinfo.c              |  4 ++--\n mailinfo.h              |  3 +++\n pretty.c                | 21 ++++++++++++++++++++-\n pretty.h                |  3 ++-\n revision.h              |  3 ++-\n t/t4014-format-patch.sh | 16 ++++++++++++++++\n 8 files changed, 47 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 56e2d95e86..edc84abaef 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1973,6 +1973,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.diffopt.flags.recursive = 1;\n \trev.diffopt.no_free = 1;\n \trev.subject_prefix = fmt_patch_subject_prefix;\n+\trev.check_in_body_patch_breaks = 1;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.def = \"HEAD\";\n \ts_r_opt.revarg_opt = REVARG_COMMITTISH;\ndiff --git a/log-tree.c b/log-tree.c\nindex 3e8c70ddcf..25ed5452b1 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -766,6 +766,7 @@ void show_log(struct rev_info *opt)\n \tctx.after_subject = extra_headers;\n \tctx.preserve_subject = opt->preserve_subject;\n \tctx.encode_email_headers = opt->encode_email_headers;\n+\tctx.check_in_body_patch_breaks = opt->check_in_body_patch_breaks;\n \tctx.reflog_info = opt->reflog_info;\n \tctx.fmt = opt->commit_format;\n \tctx.mailmap = opt->mailmap;\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 9621ba62a3..9945ea6267 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -646,7 +646,7 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n \tfree(ret);\n }\n \n-static inline int patchbreak(const struct strbuf *line)\n+int patchbreak(const struct strbuf *line)\n {\n \tsize_t i;\n \n@@ -682,7 +682,7 @@ static inline int patchbreak(const struct strbuf *line)\n \treturn 0;\n }\n \n-static int is_scissors_line(const char *line)\n+int is_scissors_line(const char *line)\n {\n \tconst char *c;\n \tint scissors = 0, gap = 0;\ndiff --git a/mailinfo.h b/mailinfo.h\nindex f2ffd0349e..8d4dda5deb 100644\n--- a/mailinfo.h\n+++ b/mailinfo.h\n@@ -53,4 +53,7 @@ void setup_mailinfo(struct mailinfo *);\n int mailinfo(struct mailinfo *, const char *msg, const char *patch);\n void clear_mailinfo(struct mailinfo *);\n \n+int patchbreak(const struct strbuf *line);\n+int is_scissors_line(const char *line);\n+\n #endif /* MAILINFO_H */\ndiff --git a/pretty.c b/pretty.c\nindex 6d819103fb..9f999029f5 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -5,6 +5,7 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"string-list.h\"\n+#include \"mailinfo.h\"\n #include \"mailmap.h\"\n #include \"log-tree.h\"\n #include \"notes.h\"\n@@ -2097,7 +2098,8 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\t  int indent)\n {\n \tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n-\tint first = 1;\n+\tint first = 1, found_delimiter = 0;\n+\tstruct strbuf linebuf = STRBUF_INIT;\n \n \tfor (;;) {\n \t\tconst char *line = *msg_p;\n@@ -2107,6 +2109,17 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\tif (!linelen)\n \t\t\tbreak;\n \n+\t\tif (pp->check_in_body_patch_breaks) {\n+\t\t\tstrbuf_reset(&linebuf);\n+\t\t\tstrbuf_add(&linebuf, line, linelen);\n+\t\t\tif (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {\n+\t\t\t\tstrbuf_strip_suffix(&linebuf, \"\\n\");\n+\t\t\t\twarning(\"commit message has a patch delimiter: '%s'\",\n+\t\t\t\t\tlinebuf.buf);\n+\t\t\t\tfound_delimiter = 1;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (is_blank_line(line, &linelen)) {\n \t\t\tif (first)\n \t\t\t\tcontinue;\n@@ -2133,6 +2146,12 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\t}\n \t\tstrbuf_addch(sb, '\\n');\n \t}\n+\n+\tif (found_delimiter)\n+\t\twarning(\"git am might fail to apply this patch. \"\n+\t\t\t\"Consider indenting the offending lines.\");\n+\n+\tstrbuf_release(&linebuf);\n }\n \n void pretty_print_commit(struct pretty_print_context *pp,\ndiff --git a/pretty.h b/pretty.h\nindex f34e24c53a..12df2f4a39 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -49,7 +49,8 @@ struct pretty_print_context {\n \tstruct string_list *mailmap;\n \tint color;\n \tstruct ident_split *from_ident;\n-\tunsigned encode_email_headers:1;\n+\tunsigned encode_email_headers:1,\n+\t\t check_in_body_patch_breaks:1;\n \tstruct pretty_print_describe_status *describe_status;\n \n \t/*\ndiff --git a/revision.h b/revision.h\nindex 61a9b1316b..f384ab716f 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -230,7 +230,8 @@ struct rev_info {\n \t\t\tdate_mode_explicit:1,\n \t\t\tpreserve_subject:1,\n \t\t\tencode_email_headers:1,\n-\t\t\tinclude_header:1;\n+\t\t\tinclude_header:1,\n+\t\t\tcheck_in_body_patch_breaks:1;\n \tunsigned int\tdisable_stdin:1;\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex fbec8ad2ef..4868ea2b91 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2329,4 +2329,20 @@ test_expect_success 'interdiff: solo-patch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'warn if commit message contains patch delimiter' '\n+\t>delim &&\n+\tgit add delim &&\n+\tGIT_EDITOR=\"printf \\\"title\\n\\n---\\\" >\" git commit &&\n+\tgit format-patch -1 2>stderr &&\n+\tgrep \"warning: commit message has a patch delimiter\" stderr\n+'\n+\n+test_expect_success 'warn if commit message contains scissors' '\n+\t>scissors &&\n+\tgit add scissors &&\n+\tGIT_EDITOR=\"printf \\\"title\\n\\n-- >8 --\\\" >\" git commit &&\n+\tgit format-patch -1 2>stderr &&\n+\tgrep \"warning: commit message has a patch delimiter\" stderr\n+'\n+\n test_done\n-- \n2.37.2\n\n"},{"id":"462585","messageId":"220905.864jxmme0a.gmgdl@evledraar.gmail.com","threadId":"58396","inReplyTo":"d0b577825124ac684ab304d3a1395f3d2d0708e8.1662333027.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] format-patch: warn if commit msg contains a patch delimiter","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-05T08:01:16Z","receivedAt":"2022-09-05T08:18:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Sep 04 2022, Matheus Tavares wrote:\n\n> When applying a patch, `git am` looks for special delimiter strings\n> (such as \"---\") to know where the message ends and the actual diff\n> starts. If one of these strings appears in the commit message itself,\n> `am` might get confused and fail to apply the patch properly. This has\n> already caused inconveniences in the past [1][2]. To help avoid such\n> problem, let's make `git format-patch` warn on commit messages\n> containing one of the said strings.\n>\n> [1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/\n> [2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/\n\nI followed this topic with one eye, and have run into this myself in the\npast. I'm not against this warning, but I wonder if we can't fix\n\"am/apply\" to just be smarter. The cases I've seen are all ones where:\n\n * We have a copy/pasted git diff, but we could disambiguate based on\n   (at least) the \"---\" line being a telltale for the \"real\" patch, and\n   the \"X file changed...\" diffstat.\n * We have a not-quite-git-looking patch diff in the commit message\n   (which we'd normally detect and apply), as in your [2].\n\nCouldn't we just be a bit smarter about applying these, and do a\nlook-ahead and find what the user meant.\n\nIs any case, having such a warning won't \"settle\" this issue, as we're\nable to deal with this non-ambiguity in commit objects/the push/fetch\nprotocol. It's just \"format-patch/am\" as a \"wire protocol\" that has this\nissue.\n\nBut anyway, that's the state of the world now, so warning() about it is\nfair, even if we had a fix for the \"apply\" part we might want to warn\nfor a while to note that it's an issue on older gits.\n\n> +\t\tif (pp->check_in_body_patch_breaks) {\n> +\t\t\tstrbuf_reset(&linebuf);\n> +\t\t\tstrbuf_add(&linebuf, line, linelen);\n> +\t\t\tif (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {\n> +\t\t\t\tstrbuf_strip_suffix(&linebuf, \"\\n\");\n\nHrm, it's a (small) shame that the patchbreak() function takes a \"struct\nstrbuf\" rather than a char */size_t in this case (seemingly for no good\nreason, as it's \"const\"?).\n\nBecause of that you need to make a copy here, instead of just finding\nthe \"\\n\" and using the %*s format, anyway, small potatoes.\n\n> +\t\t\t\twarning(\"commit message has a patch delimiter: '%s'\",\n> +\t\t\t\t\tlinebuf.buf);\n\nMissing _()?\n\n> +test_expect_success 'warn if commit message contains patch delimiter' '\n> +\t>delim &&\n> +\tgit add delim &&\n> +\tGIT_EDITOR=\"printf \\\"title\\n\\n---\\\" >\" git commit &&\n\nMaybe I'm missing something, but isn't this GIT_EDITOR/printf just\nanother way of saying something like:\n\n\tcat >msg <<-\\EOF &&\n\t\"title\n\n\t---\" >\n\tEOF\n\tgit commit -F msg && ...\n\nUntested, so maybe not..\n"},{"id":"462626","messageId":"904b784d-a328-011f-c71a-c2092534e0f7@web.de","threadId":"58396","inReplyTo":"220905.864jxmme0a.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] format-patch: warn if commit msg contains a patch delimiter","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-05T10:57:45Z","receivedAt":"2022-09-05T10:58:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.09.22 um 10:01 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sun, Sep 04 2022, Matheus Tavares wrote:\n>\n>> When applying a patch, `git am` looks for special delimiter strings\n>> (such as \"---\") to know where the message ends and the actual diff\n>> starts. If one of these strings appears in the commit message itself,\n>> `am` might get confused and fail to apply the patch properly. This has\n>> already caused inconveniences in the past [1][2]. To help avoid such\n>> problem, let's make `git format-patch` warn on commit messages\n>> containing one of the said strings.\n>>\n>> [1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/\n>> [2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/\n>\n> I followed this topic with one eye, and have run into this myself in the\n> past. I'm not against this warning, but I wonder if we can't fix\n> \"am/apply\" to just be smarter. The cases I've seen are all ones where:\n>\n>  * We have a copy/pasted git diff, but we could disambiguate based on\n>    (at least) the \"---\" line being a telltale for the \"real\" patch, and\n>    the \"X file changed...\" diffstat.\n>  * We have a not-quite-git-looking patch diff in the commit message\n>    (which we'd normally detect and apply), as in your [2].\n>\n> Couldn't we just be a bit smarter about applying these, and do a\n> look-ahead and find what the user meant.\n\nWhatever we use to separate message from diff can be included in that\nmessage by an unsuspecting user and \"---\" can be part of a diff.  An\nearlier discussion yielded an idea, but no implementation:\nhttps://lore.kernel.org/git/20200204010524-mutt-send-email-mst@kernel.org/\n\n> Is any case, having such a warning won't \"settle\" this issue, as we're\n> able to deal with this non-ambiguity in commit objects/the push/fetch\n> protocol. It's just \"format-patch/am\" as a \"wire protocol\" that has this\n> issue.\n>\n> But anyway, that's the state of the world now, so warning() about it is\n> fair, even if we had a fix for the \"apply\" part we might want to warn\n> for a while to note that it's an issue on older gits.\n>\n>> +\t\tif (pp->check_in_body_patch_breaks) {\n>> +\t\t\tstrbuf_reset(&linebuf);\n>> +\t\t\tstrbuf_add(&linebuf, line, linelen);\n>> +\t\t\tif (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {\n>> +\t\t\t\tstrbuf_strip_suffix(&linebuf, \"\\n\");\n>\n> Hrm, it's a (small) shame that the patchbreak() function takes a \"struct\n> strbuf\" rather than a char */size_t in this case (seemingly for no good\n> reason, as it's \"const\"?).\n\nA strbuf is NUL-terminated, a length-limited string (char */size_t)\ndoesn't have to be.  That means the current implementation can use\nfunctions like starts_with(), but a faithful version that promises to\nstay within a given length cannot.  So the reason is probably\nconvenience.  With skip_prefix_mem() it wouldn't be that bad, though:\n\n---\n mailinfo.c | 37 +++++++++++++++++++------------------\n 1 file changed, 19 insertions(+), 18 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 9621ba62a3..ae2e70e363 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -646,32 +646,30 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n \tfree(ret);\n }\n\n-static inline int patchbreak(const struct strbuf *line)\n+static int patchbreak(const char *buf, size_t len)\n {\n-\tsize_t i;\n-\n \t/* Beginning of a \"diff -\" header? */\n-\tif (starts_with(line->buf, \"diff -\"))\n+\tif (skip_prefix_mem(buf, len, \"diff -\", &buf, &len))\n \t\treturn 1;\n\n \t/* CVS \"Index: \" line? */\n-\tif (starts_with(line->buf, \"Index: \"))\n+\tif (skip_prefix_mem(buf, len, \"Index: \", &buf, &len))\n \t\treturn 1;\n\n \t/*\n \t * \"--- <filename>\" starts patches without headers\n \t * \"---<sp>*\" is a manual separator\n \t */\n-\tif (line->len < 4)\n+\tif (len < 4)\n \t\treturn 0;\n\n-\tif (starts_with(line->buf, \"---\")) {\n+\tif (skip_prefix_mem(buf, len, \"---\", &buf, &len)) {\n \t\t/* space followed by a filename? */\n-\t\tif (line->buf[3] == ' ' && !isspace(line->buf[4]))\n+\t\tif (len > 1 && buf[0] == ' ' && !isspace(buf[1]))\n \t\t\treturn 1;\n \t\t/* Just whitespace? */\n-\t\tfor (i = 3; i < line->len; i++) {\n-\t\t\tunsigned char c = line->buf[i];\n+\t\tfor (; len; buf++, len--) {\n+\t\t\tunsigned char c = buf[0];\n \t\t\tif (c == '\\n')\n \t\t\t\treturn 1;\n \t\t\tif (!isspace(c))\n@@ -682,14 +680,14 @@ static inline int patchbreak(const struct strbuf *line)\n \treturn 0;\n }\n\n-static int is_scissors_line(const char *line)\n+static int is_scissors_line(const char *line, size_t len)\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; c++) {\n+\tfor (c = line; len; c++, len--) {\n \t\tif (isspace(*c)) {\n \t\t\tif (in_perforation) {\n \t\t\t\tperforation++;\n@@ -705,12 +703,14 @@ static int is_scissors_line(const char *line)\n \t\t\tperforation++;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(c, \">8\") || starts_with(c, \"8<\") ||\n-\t\t    starts_with(c, \">%\") || starts_with(c, \"%<\")) {\n+\t\tif (skip_prefix_mem(c, len, \">8\", &c, &len) ||\n+\t\t    skip_prefix_mem(c, len, \"8<\", &c, &len) ||\n+\t\t    skip_prefix_mem(c, len, \">%\", &c, &len) ||\n+\t\t    skip_prefix_mem(c, len, \"%<\", &c, &len)) {\n \t\t\tin_perforation = 1;\n \t\t\tperforation += 2;\n \t\t\tscissors += 2;\n-\t\t\tc++;\n+\t\t\tc--, len++;\n \t\t\tcontinue;\n \t\t}\n \t\tin_perforation = 0;\n@@ -747,7 +747,8 @@ static int check_inbody_header(struct mailinfo *mi, const struct strbuf *line)\n {\n \tif (mi->inbody_header_accum.len &&\n \t    (line->buf[0] == ' ' || line->buf[0] == '\\t')) {\n-\t\tif (mi->use_scissors && is_scissors_line(line->buf)) {\n+\t\tif (mi->use_scissors &&\n+\t\t    is_scissors_line(line->buf, line->len)) {\n \t\t\t/*\n \t\t\t * This is a scissors line; do not consider this line\n \t\t\t * as a header continuation line.\n@@ -808,7 +809,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \tif (convert_to_utf8(mi, line, mi->charset.buf))\n \t\treturn 0; /* mi->input_error already set */\n\n-\tif (mi->use_scissors && is_scissors_line(line->buf)) {\n+\tif (mi->use_scissors && is_scissors_line(line->buf, line->len)) {\n \t\tint i;\n\n \t\tstrbuf_setlen(&mi->log_message, 0);\n@@ -826,7 +827,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \t\treturn 0;\n \t}\n\n-\tif (patchbreak(line)) {\n+\tif (patchbreak(line->buf, line->len)) {\n \t\tif (mi->message_id)\n \t\t\tstrbuf_addf(&mi->log_message,\n \t\t\t\t    \"Message-Id: %s\\n\", mi->message_id);\n--\n2.37.2\n\n"},{"id":"462723","messageId":"cover.1662559356.git.matheus.bernardino@usp.br","threadId":"58396","inReplyTo":"220905.864jxmme0a.gmgdl@evledraar.gmail.com","subject":"[PATCH v2 0/2] format-patch: warn if commit msg contains a patch delimiter","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-09-07T14:44:55Z","receivedAt":"2022-09-07T14:45:28Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This makes format-patch warn on strings like \"---\" and \"-- >8 --\", which\ncan make a later git am fail to properly apply the generated patch.\n\nChanges in v2:\n- Use heredoc in tests.\n- Add internationalization _()\n- Incorporate René changes to use a buf/size pair.\n\nRené, I added your changes from [1] as a preparatory patch. Please let\nme know if you are OK with that so that I can add your SoB in the next\nre-roll.\n\n[1]: https://lore.kernel.org/git/904b784d-a328-011f-c71a-c2092534e0f7@web.de/\n\nMatheus Tavares (1):\n  format-patch: warn if commit msg contains a patch delimiter\n\nRené Scharfe (1):\n  patchbreak(), is_scissors_line(): work with a buf/len pair\n\n builtin/log.c           |  1 +\n log-tree.c              |  1 +\n mailinfo.c              | 37 +++++++++++++++++++------------------\n mailinfo.h              |  3 +++\n pretty.c                | 16 +++++++++++++++-\n pretty.h                |  3 ++-\n revision.h              |  3 ++-\n t/t4014-format-patch.sh | 26 ++++++++++++++++++++++++++\n 8 files changed, 69 insertions(+), 21 deletions(-)\n\nRange-diff against v1:\n-:  ---------- > 1:  99012733e4 patchbreak(), is_scissors_line(): work with a buf/len pair\n1:  059811c85f ! 2:  a2c4514aa0 format-patch: warn if commit msg contains a patch delimiter\n    @@ mailinfo.c: static void decode_transfer_encoding(struct mailinfo *mi, struct str\n      \tfree(ret);\n      }\n      \n    --static inline int patchbreak(const struct strbuf *line)\n    -+int patchbreak(const struct strbuf *line)\n    +-static inline int patchbreak(const char *buf, size_t len)\n    ++int patchbreak(const char *buf, size_t len)\n      {\n    - \tsize_t i;\n    - \n    -@@ mailinfo.c: static inline int patchbreak(const struct strbuf *line)\n    + \t/* Beginning of a \"diff -\" header? */\n    + \tif (skip_prefix_mem(buf, len, \"diff -\", &buf, &len))\n    +@@ mailinfo.c: static inline int patchbreak(const char *buf, size_t len)\n      \treturn 0;\n      }\n      \n    --static int is_scissors_line(const char *line)\n    -+int is_scissors_line(const char *line)\n    +-static int is_scissors_line(const char *line, size_t len)\n    ++int is_scissors_line(const char *line, size_t len)\n      {\n      \tconst char *c;\n      \tint scissors = 0, gap = 0;\n    @@ mailinfo.h: void setup_mailinfo(struct mailinfo *);\n      int mailinfo(struct mailinfo *, const char *msg, const char *patch);\n      void clear_mailinfo(struct mailinfo *);\n      \n    -+int patchbreak(const struct strbuf *line);\n    -+int is_scissors_line(const char *line);\n    ++int patchbreak(const char *line, size_t len);\n    ++int is_scissors_line(const char *line, size_t len);\n     +\n      #endif /* MAILINFO_H */\n     \n    @@ pretty.c: void pp_remainder(struct pretty_print_context *pp,\n      \tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n     -\tint first = 1;\n     +\tint first = 1, found_delimiter = 0;\n    -+\tstruct strbuf linebuf = STRBUF_INIT;\n      \n      \tfor (;;) {\n      \t\tconst char *line = *msg_p;\n    @@ pretty.c: void pp_remainder(struct pretty_print_context *pp,\n      \t\tif (!linelen)\n      \t\t\tbreak;\n      \n    -+\t\tif (pp->check_in_body_patch_breaks) {\n    -+\t\t\tstrbuf_reset(&linebuf);\n    -+\t\t\tstrbuf_add(&linebuf, line, linelen);\n    -+\t\t\tif (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {\n    -+\t\t\t\tstrbuf_strip_suffix(&linebuf, \"\\n\");\n    -+\t\t\t\twarning(\"commit message has a patch delimiter: '%s'\",\n    -+\t\t\t\t\tlinebuf.buf);\n    -+\t\t\t\tfound_delimiter = 1;\n    -+\t\t\t}\n    ++\t\tif (pp->check_in_body_patch_breaks &&\n    ++\t\t    (patchbreak(line, linelen) || is_scissors_line(line, linelen))) {\n    ++\t\t\twarning(_(\"commit message has a patch delimiter: '%.*s'\"),\n    ++\t\t\t\tline[linelen - 1] == '\\n' ? linelen - 1 : linelen,\n    ++\t\t\t\tline);\n    ++\t\t\tfound_delimiter = 1;\n     +\t\t}\n     +\n      \t\tif (is_blank_line(line, &linelen)) {\n    @@ pretty.c: void pp_remainder(struct pretty_print_context *pp,\n      \t\tstrbuf_addch(sb, '\\n');\n      \t}\n     +\n    -+\tif (found_delimiter)\n    -+\t\twarning(\"git am might fail to apply this patch. \"\n    -+\t\t\t\"Consider indenting the offending lines.\");\n    -+\n    -+\tstrbuf_release(&linebuf);\n    ++\tif (found_delimiter) {\n    ++\t\twarning(_(\"git am might fail to apply this patch. \"\n    ++\t\t\t  \"Consider indenting the offending lines.\"));\n    ++\t}\n      }\n      \n      void pretty_print_commit(struct pretty_print_context *pp,\n    @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: solo-patch' '\n     +test_expect_success 'warn if commit message contains patch delimiter' '\n     +\t>delim &&\n     +\tgit add delim &&\n    -+\tGIT_EDITOR=\"printf \\\"title\\n\\n---\\\" >\" git commit &&\n    ++\tcat >msg <<-\\EOF &&\n    ++\ttitle\n    ++\n    ++\t---\n    ++\tEOF\n    ++\tgit commit -F msg &&\n     +\tgit format-patch -1 2>stderr &&\n     +\tgrep \"warning: commit message has a patch delimiter\" stderr\n     +'\n    @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: solo-patch' '\n     +test_expect_success 'warn if commit message contains scissors' '\n     +\t>scissors &&\n     +\tgit add scissors &&\n    -+\tGIT_EDITOR=\"printf \\\"title\\n\\n-- >8 --\\\" >\" git commit &&\n    ++\tcat >msg <<-\\EOF &&\n    ++\ttitle\n    ++\n    ++\t-- >8 --\n    ++\tEOF\n    ++\tgit commit -F msg &&\n     +\tgit format-patch -1 2>stderr &&\n     +\tgrep \"warning: commit message has a patch delimiter\" stderr\n     +'\n-- \n2.37.2\n\n"},{"id":"462724","messageId":"a2c4514aa03657f3b1d822efe3dd630713287ee6.1662559356.git.matheus.bernardino@usp.br","threadId":"58396","inReplyTo":"cover.1662559356.git.matheus.bernardino@usp.br","subject":"[PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-09-07T14:44:57Z","receivedAt":"2022-09-07T14:45:36Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When applying a patch, `git am` looks for special delimiter strings\n(such as \"---\") to know where the message ends and the actual diff\nstarts. If one of these strings appears in the commit message itself,\n`am` might get confused and fail to apply the patch properly. This has\nalready caused inconveniences in the past [1][2]. To help avoid such\nproblem, let's make `git format-patch` warn on commit messages\ncontaining one of the said strings.\n\n[1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/\n[2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n builtin/log.c           |  1 +\n log-tree.c              |  1 +\n mailinfo.c              |  4 ++--\n mailinfo.h              |  3 +++\n pretty.c                | 16 +++++++++++++++-\n pretty.h                |  3 ++-\n revision.h              |  3 ++-\n t/t4014-format-patch.sh | 26 ++++++++++++++++++++++++++\n 8 files changed, 52 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 56e2d95e86..edc84abaef 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1973,6 +1973,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.diffopt.flags.recursive = 1;\n \trev.diffopt.no_free = 1;\n \trev.subject_prefix = fmt_patch_subject_prefix;\n+\trev.check_in_body_patch_breaks = 1;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.def = \"HEAD\";\n \ts_r_opt.revarg_opt = REVARG_COMMITTISH;\ndiff --git a/log-tree.c b/log-tree.c\nindex 3e8c70ddcf..25ed5452b1 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -766,6 +766,7 @@ void show_log(struct rev_info *opt)\n \tctx.after_subject = extra_headers;\n \tctx.preserve_subject = opt->preserve_subject;\n \tctx.encode_email_headers = opt->encode_email_headers;\n+\tctx.check_in_body_patch_breaks = opt->check_in_body_patch_breaks;\n \tctx.reflog_info = opt->reflog_info;\n \tctx.fmt = opt->commit_format;\n \tctx.mailmap = opt->mailmap;\ndiff --git a/mailinfo.c b/mailinfo.c\nindex f0a690b6e8..d227397f1c 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -646,7 +646,7 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n \tfree(ret);\n }\n \n-static inline int patchbreak(const char *buf, size_t len)\n+int patchbreak(const char *buf, size_t len)\n {\n \t/* Beginning of a \"diff -\" header? */\n \tif (skip_prefix_mem(buf, len, \"diff -\", &buf, &len))\n@@ -680,7 +680,7 @@ static inline int patchbreak(const char *buf, size_t len)\n \treturn 0;\n }\n \n-static int is_scissors_line(const char *line, size_t len)\n+int is_scissors_line(const char *line, size_t len)\n {\n \tconst char *c;\n \tint scissors = 0, gap = 0;\ndiff --git a/mailinfo.h b/mailinfo.h\nindex f2ffd0349e..347eefe856 100644\n--- a/mailinfo.h\n+++ b/mailinfo.h\n@@ -53,4 +53,7 @@ void setup_mailinfo(struct mailinfo *);\n int mailinfo(struct mailinfo *, const char *msg, const char *patch);\n void clear_mailinfo(struct mailinfo *);\n \n+int patchbreak(const char *line, size_t len);\n+int is_scissors_line(const char *line, size_t len);\n+\n #endif /* MAILINFO_H */\ndiff --git a/pretty.c b/pretty.c\nindex 6d819103fb..913d974b3a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -5,6 +5,7 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"string-list.h\"\n+#include \"mailinfo.h\"\n #include \"mailmap.h\"\n #include \"log-tree.h\"\n #include \"notes.h\"\n@@ -2097,7 +2098,7 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\t  int indent)\n {\n \tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n-\tint first = 1;\n+\tint first = 1, found_delimiter = 0;\n \n \tfor (;;) {\n \t\tconst char *line = *msg_p;\n@@ -2107,6 +2108,14 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\tif (!linelen)\n \t\t\tbreak;\n \n+\t\tif (pp->check_in_body_patch_breaks &&\n+\t\t    (patchbreak(line, linelen) || is_scissors_line(line, linelen))) {\n+\t\t\twarning(_(\"commit message has a patch delimiter: '%.*s'\"),\n+\t\t\t\tline[linelen - 1] == '\\n' ? linelen - 1 : linelen,\n+\t\t\t\tline);\n+\t\t\tfound_delimiter = 1;\n+\t\t}\n+\n \t\tif (is_blank_line(line, &linelen)) {\n \t\t\tif (first)\n \t\t\t\tcontinue;\n@@ -2133,6 +2142,11 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\t}\n \t\tstrbuf_addch(sb, '\\n');\n \t}\n+\n+\tif (found_delimiter) {\n+\t\twarning(_(\"git am might fail to apply this patch. \"\n+\t\t\t  \"Consider indenting the offending lines.\"));\n+\t}\n }\n \n void pretty_print_commit(struct pretty_print_context *pp,\ndiff --git a/pretty.h b/pretty.h\nindex f34e24c53a..12df2f4a39 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -49,7 +49,8 @@ struct pretty_print_context {\n \tstruct string_list *mailmap;\n \tint color;\n \tstruct ident_split *from_ident;\n-\tunsigned encode_email_headers:1;\n+\tunsigned encode_email_headers:1,\n+\t\t check_in_body_patch_breaks:1;\n \tstruct pretty_print_describe_status *describe_status;\n \n \t/*\ndiff --git a/revision.h b/revision.h\nindex 61a9b1316b..f384ab716f 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -230,7 +230,8 @@ struct rev_info {\n \t\t\tdate_mode_explicit:1,\n \t\t\tpreserve_subject:1,\n \t\t\tencode_email_headers:1,\n-\t\t\tinclude_header:1;\n+\t\t\tinclude_header:1,\n+\t\t\tcheck_in_body_patch_breaks:1;\n \tunsigned int\tdisable_stdin:1;\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex fbec8ad2ef..4bbf1156e9 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2329,4 +2329,30 @@ test_expect_success 'interdiff: solo-patch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'warn if commit message contains patch delimiter' '\n+\t>delim &&\n+\tgit add delim &&\n+\tcat >msg <<-\\EOF &&\n+\ttitle\n+\n+\t---\n+\tEOF\n+\tgit commit -F msg &&\n+\tgit format-patch -1 2>stderr &&\n+\tgrep \"warning: commit message has a patch delimiter\" stderr\n+'\n+\n+test_expect_success 'warn if commit message contains scissors' '\n+\t>scissors &&\n+\tgit add scissors &&\n+\tcat >msg <<-\\EOF &&\n+\ttitle\n+\n+\t-- >8 --\n+\tEOF\n+\tgit commit -F msg &&\n+\tgit format-patch -1 2>stderr &&\n+\tgrep \"warning: commit message has a patch delimiter\" stderr\n+'\n+\n test_done\n-- \n2.37.2\n\n"},{"id":"462725","messageId":"99012733e440be15afc7fd45272e738c71b3ef27.1662559356.git.matheus.bernardino@usp.br","threadId":"58396","inReplyTo":"cover.1662559356.git.matheus.bernardino@usp.br","subject":"[PATCH v2 1/2] patchbreak(), is_scissors_line(): work with a buf/len pair","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-09-07T14:44:56Z","receivedAt":"2022-09-07T14:45:38Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"From: René Scharfe <l.s.r@web.de>\n\nThe next patch will add calls to these two functions from code that\nworks with a char */size_t pair. So let's adapt the functions in\npreparation.\n---\n mailinfo.c | 37 +++++++++++++++++++------------------\n 1 file changed, 19 insertions(+), 18 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 9621ba62a3..f0a690b6e8 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -646,32 +646,30 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n \tfree(ret);\n }\n \n-static inline int patchbreak(const struct strbuf *line)\n+static inline int patchbreak(const char *buf, size_t len)\n {\n-\tsize_t i;\n-\n \t/* Beginning of a \"diff -\" header? */\n-\tif (starts_with(line->buf, \"diff -\"))\n+\tif (skip_prefix_mem(buf, len, \"diff -\", &buf, &len))\n \t\treturn 1;\n \n \t/* CVS \"Index: \" line? */\n-\tif (starts_with(line->buf, \"Index: \"))\n+\tif (skip_prefix_mem(buf, len, \"Index: \", &buf, &len))\n \t\treturn 1;\n \n \t/*\n \t * \"--- <filename>\" starts patches without headers\n \t * \"---<sp>*\" is a manual separator\n \t */\n-\tif (line->len < 4)\n+\tif (len < 4)\n \t\treturn 0;\n \n-\tif (starts_with(line->buf, \"---\")) {\n+\tif (skip_prefix_mem(buf, len, \"---\", &buf, &len)) {\n \t\t/* space followed by a filename? */\n-\t\tif (line->buf[3] == ' ' && !isspace(line->buf[4]))\n+\t\tif (len > 1 && buf[0] == ' ' && !isspace(buf[1]))\n \t\t\treturn 1;\n \t\t/* Just whitespace? */\n-\t\tfor (i = 3; i < line->len; i++) {\n-\t\t\tunsigned char c = line->buf[i];\n+\t\tfor (; len; buf++, len--) {\n+\t\t\tunsigned char c = buf[0];\n \t\t\tif (c == '\\n')\n \t\t\t\treturn 1;\n \t\t\tif (!isspace(c))\n@@ -682,14 +680,14 @@ static inline int patchbreak(const struct strbuf *line)\n \treturn 0;\n }\n \n-static int is_scissors_line(const char *line)\n+static int is_scissors_line(const char *line, size_t len)\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; c++) {\n+\tfor (c = line; len; c++, len--) {\n \t\tif (isspace(*c)) {\n \t\t\tif (in_perforation) {\n \t\t\t\tperforation++;\n@@ -705,12 +703,14 @@ static int is_scissors_line(const char *line)\n \t\t\tperforation++;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(c, \">8\") || starts_with(c, \"8<\") ||\n-\t\t    starts_with(c, \">%\") || starts_with(c, \"%<\")) {\n+\t\tif (skip_prefix_mem(c, len, \">8\", &c, &len) ||\n+\t\t    skip_prefix_mem(c, len, \"8<\", &c, &len) ||\n+\t\t    skip_prefix_mem(c, len, \">%\", &c, &len) ||\n+\t\t    skip_prefix_mem(c, len, \"%<\", &c, &len)) {\n \t\t\tin_perforation = 1;\n \t\t\tperforation += 2;\n \t\t\tscissors += 2;\n-\t\t\tc++;\n+\t\t\tc--, len++;\n \t\t\tcontinue;\n \t\t}\n \t\tin_perforation = 0;\n@@ -747,7 +747,8 @@ static int check_inbody_header(struct mailinfo *mi, const struct strbuf *line)\n {\n \tif (mi->inbody_header_accum.len &&\n \t    (line->buf[0] == ' ' || line->buf[0] == '\\t')) {\n-\t\tif (mi->use_scissors && is_scissors_line(line->buf)) {\n+\t\tif (mi->use_scissors &&\n+\t\t    is_scissors_line(line->buf, line->len)) {\n \t\t\t/*\n \t\t\t * This is a scissors line; do not consider this line\n \t\t\t * as a header continuation line.\n@@ -808,7 +809,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \tif (convert_to_utf8(mi, line, mi->charset.buf))\n \t\treturn 0; /* mi->input_error already set */\n \n-\tif (mi->use_scissors && is_scissors_line(line->buf)) {\n+\tif (mi->use_scissors && is_scissors_line(line->buf, line->len)) {\n \t\tint i;\n \n \t\tstrbuf_setlen(&mi->log_message, 0);\n@@ -826,7 +827,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \t\treturn 0;\n \t}\n \n-\tif (patchbreak(line)) {\n+\tif (patchbreak(line->buf, line->len)) {\n \t\tif (mi->message_id)\n \t\t\tstrbuf_addf(&mi->log_message,\n \t\t\t\t    \"Message-Id: %s\\n\", mi->message_id);\n-- \n2.37.2\n\n"},{"id":"462736","messageId":"05a52191-89b0-eb55-e6bc-1ed2642842de@web.de","threadId":"58396","inReplyTo":"cover.1662559356.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 0/2] format-patch: warn if commit msg contains a patch delimiter","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-07T17:44:25Z","receivedAt":"2022-09-07T17:44:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.09.22 um 16:44 schrieb Matheus Tavares:\n> This makes format-patch warn on strings like \"---\" and \"-- >8 --\", which\n> can make a later git am fail to properly apply the generated patch.\n>\n> Changes in v2:\n> - Use heredoc in tests.\n> - Add internationalization _()\n> - Incorporate René changes to use a buf/size pair.\n>\n> René, I added your changes from [1] as a preparatory patch. Please let\n> me know if you are OK with that so that I can add your SoB in the next\n> re-roll.\n\nFine with me, but I think they are trivial and you don't actually need my\nsign-off.\n\nRené\n"},{"id":"462737","messageId":"4d750ff2-9df5-504f-9972-59b082000db0@gmail.com","threadId":"58396","inReplyTo":"a2c4514aa03657f3b1d822efe3dd630713287ee6.1662559356.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-07T18:09:05Z","receivedAt":"2022-09-07T18:09:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Matheus\n\nOn 07/09/2022 15:44, Matheus Tavares wrote:\n> When applying a patch, `git am` looks for special delimiter strings\n> (such as \"---\") to know where the message ends and the actual diff\n> starts. If one of these strings appears in the commit message itself,\n> `am` might get confused and fail to apply the patch properly. This has\n> already caused inconveniences in the past [1][2]. To help avoid such\n> problem, let's make `git format-patch` warn on commit messages\n> containing one of the said strings.\n\nThanks for working on this, having a warning for this is a useful \naddition. If the user embeds a diff in their commit message then they \nwill receive three warnings\n\nwarning: commit message has a patch delimiter: 'diff --git a/file b/file'\nwarning: commit message has a patch delimiter: '--- file'\nwarning: git am might fail to apply this patch. Consider indenting the \noffending lines.\n\nI guess it's helpful to show all the lines that are considered \ndelimiters but it gets quite noisy.\n\n\n> diff --git a/pretty.c b/pretty.c\n> index 6d819103fb..913d974b3a 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -2107,6 +2108,14 @@ void pp_remainder(struct pretty_print_context *pp,\n>   \t\tif (!linelen)\n>   \t\t\tbreak;\n>   \n> +\t\tif (pp->check_in_body_patch_breaks &&\n> +\t\t    (patchbreak(line, linelen) || is_scissors_line(line, linelen))) {\n> +\t\t\twarning(_(\"commit message has a patch delimiter: '%.*s'\"),\n> +\t\t\t\tline[linelen - 1] == '\\n' ? linelen - 1 : linelen,\n> +\t\t\t\tline);\n> +\t\t\tfound_delimiter = 1;\n> +\t\t}\n> +\n>   \t\tif (is_blank_line(line, &linelen)) {\n>   \t\t\tif (first)\n>   \t\t\t\tcontinue;\n> @@ -2133,6 +2142,11 @@ void pp_remainder(struct pretty_print_context *pp,\n>   \t\t}\n>   \t\tstrbuf_addch(sb, '\\n');\n>   \t}\n> +\n> +\tif (found_delimiter) {\n> +\t\twarning(_(\"git am might fail to apply this patch. \"\n> +\t\t\t  \"Consider indenting the offending lines.\"));\n\nThe message says the patch might fail to apply, but isn't it guaranteed \nto fail?\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index fbec8ad2ef..4bbf1156e9 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -2329,4 +2329,30 @@ test_expect_success 'interdiff: solo-patch' '\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success 'warn if commit message contains patch delimiter' '\n> +\t>delim &&\n> +\tgit add delim &&\n> +\tcat >msg <<-\\EOF &&\n> +\ttitle\n> +\n> +\t---\n> +\tEOF\n> +\tgit commit -F msg &&\n> +\tgit format-patch -1 2>stderr &&\n> +\tgrep \"warning: commit message has a patch delimiter\" stderr\n\nI think it would be worth checking for the second message as well in the \ntests.\n\n\nBest Wishes\n\nPhillip\n\n"},{"id":"462743","messageId":"435b2b8b-2577-0af4-6f69-b250f64cbdf8@gmail.com","threadId":"58396","inReplyTo":"99012733e440be15afc7fd45272e738c71b3ef27.1662559356.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 1/2] patchbreak(), is_scissors_line(): work with a buf/len pair","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-07T18:20:31Z","receivedAt":"2022-09-07T18:20:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/09/2022 15:44, Matheus Tavares wrote:\n> From: René Scharfe <l.s.r@web.de>\n> \n> The next patch will add calls to these two functions from code that\n> works with a char */size_t pair. So let's adapt the functions in\n> preparation.\n\nReading this I wonder if we should add a starts_with_mem() function, \nrather than having to pass pointers to buf and len to skip_prefix_mem().\n\nBest Wishes\n\nPhillip\n\n> ---\n>   mailinfo.c | 37 +++++++++++++++++++------------------\n>   1 file changed, 19 insertions(+), 18 deletions(-)\n> \n> diff --git a/mailinfo.c b/mailinfo.c\n> index 9621ba62a3..f0a690b6e8 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -646,32 +646,30 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n>   \tfree(ret);\n>   }\n>   \n> -static inline int patchbreak(const struct strbuf *line)\n> +static inline int patchbreak(const char *buf, size_t len)\n>   {\n> -\tsize_t i;\n> -\n>   \t/* Beginning of a \"diff -\" header? */\n> -\tif (starts_with(line->buf, \"diff -\"))\n> +\tif (skip_prefix_mem(buf, len, \"diff -\", &buf, &len))\n>   \t\treturn 1;\n>   \n>   \t/* CVS \"Index: \" line? */\n> -\tif (starts_with(line->buf, \"Index: \"))\n> +\tif (skip_prefix_mem(buf, len, \"Index: \", &buf, &len))\n>   \t\treturn 1;\n>   \n>   \t/*\n>   \t * \"--- <filename>\" starts patches without headers\n>   \t * \"---<sp>*\" is a manual separator\n>   \t */\n> -\tif (line->len < 4)\n> +\tif (len < 4)\n>   \t\treturn 0;\n>   \n> -\tif (starts_with(line->buf, \"---\")) {\n> +\tif (skip_prefix_mem(buf, len, \"---\", &buf, &len)) {\n>   \t\t/* space followed by a filename? */\n> -\t\tif (line->buf[3] == ' ' && !isspace(line->buf[4]))\n> +\t\tif (len > 1 && buf[0] == ' ' && !isspace(buf[1]))\n>   \t\t\treturn 1;\n>   \t\t/* Just whitespace? */\n> -\t\tfor (i = 3; i < line->len; i++) {\n> -\t\t\tunsigned char c = line->buf[i];\n> +\t\tfor (; len; buf++, len--) {\n> +\t\t\tunsigned char c = buf[0];\n>   \t\t\tif (c == '\\n')\n>   \t\t\t\treturn 1;\n>   \t\t\tif (!isspace(c))\n> @@ -682,14 +680,14 @@ static inline int patchbreak(const struct strbuf *line)\n>   \treturn 0;\n>   }\n>   \n> -static int is_scissors_line(const char *line)\n> +static int is_scissors_line(const char *line, size_t len)\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; c++) {\n> +\tfor (c = line; len; c++, len--) {\n>   \t\tif (isspace(*c)) {\n>   \t\t\tif (in_perforation) {\n>   \t\t\t\tperforation++;\n> @@ -705,12 +703,14 @@ static int is_scissors_line(const char *line)\n>   \t\t\tperforation++;\n>   \t\t\tcontinue;\n>   \t\t}\n> -\t\tif (starts_with(c, \">8\") || starts_with(c, \"8<\") ||\n> -\t\t    starts_with(c, \">%\") || starts_with(c, \"%<\")) {\n> +\t\tif (skip_prefix_mem(c, len, \">8\", &c, &len) ||\n> +\t\t    skip_prefix_mem(c, len, \"8<\", &c, &len) ||\n> +\t\t    skip_prefix_mem(c, len, \">%\", &c, &len) ||\n> +\t\t    skip_prefix_mem(c, len, \"%<\", &c, &len)) {\n>   \t\t\tin_perforation = 1;\n>   \t\t\tperforation += 2;\n>   \t\t\tscissors += 2;\n> -\t\t\tc++;\n> +\t\t\tc--, len++;\n>   \t\t\tcontinue;\n>   \t\t}\n>   \t\tin_perforation = 0;\n> @@ -747,7 +747,8 @@ static int check_inbody_header(struct mailinfo *mi, const struct strbuf *line)\n>   {\n>   \tif (mi->inbody_header_accum.len &&\n>   \t    (line->buf[0] == ' ' || line->buf[0] == '\\t')) {\n> -\t\tif (mi->use_scissors && is_scissors_line(line->buf)) {\n> +\t\tif (mi->use_scissors &&\n> +\t\t    is_scissors_line(line->buf, line->len)) {\n>   \t\t\t/*\n>   \t\t\t * This is a scissors line; do not consider this line\n>   \t\t\t * as a header continuation line.\n> @@ -808,7 +809,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n>   \tif (convert_to_utf8(mi, line, mi->charset.buf))\n>   \t\treturn 0; /* mi->input_error already set */\n>   \n> -\tif (mi->use_scissors && is_scissors_line(line->buf)) {\n> +\tif (mi->use_scissors && is_scissors_line(line->buf, line->len)) {\n>   \t\tint i;\n>   \n>   \t\tstrbuf_setlen(&mi->log_message, 0);\n> @@ -826,7 +827,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n>   \t\treturn 0;\n>   \t}\n>   \n> -\tif (patchbreak(line)) {\n> +\tif (patchbreak(line->buf, line->len)) {\n>   \t\tif (mi->message_id)\n>   \t\t\tstrbuf_addf(&mi->log_message,\n>   \t\t\t\t    \"Message-Id: %s\\n\", mi->message_id);\n\n"},{"id":"462746","messageId":"xmqqa67buj4m.fsf@gitster.g","threadId":"58396","inReplyTo":"4d750ff2-9df5-504f-9972-59b082000db0@gmail.com","subject":"Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T18:36:25Z","receivedAt":"2022-09-07T18:36:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Hi Matheus\n>\n> On 07/09/2022 15:44, Matheus Tavares wrote:\n>> When applying a patch, `git am` looks for special delimiter strings\n>> (such as \"---\") to know where the message ends and the actual diff\n>> starts. If one of these strings appears in the commit message itself,\n>> `am` might get confused and fail to apply the patch properly. This has\n>> already caused inconveniences in the past [1][2]. To help avoid such\n>> problem, let's make `git format-patch` warn on commit messages\n>> containing one of the said strings.\n>\n> Thanks for working on this, having a warning for this is a useful\n> addition. If the user embeds a diff in their commit message then they\n> will receive three warnings\n>\n> warning: commit message has a patch delimiter: 'diff --git a/file b/file'\n> warning: commit message has a patch delimiter: '--- file'\n> warning: git am might fail to apply this patch. Consider indenting the\n> offending lines.\n>\n> I guess it's helpful to show all the lines that are considered\n> delimiters but it gets quite noisy.\n\nTrue.  I wonder if automatically indenting these lines is an option ;-)\n\n>> +\n>> +\tif (found_delimiter) {\n>> +\t\twarning(_(\"git am might fail to apply this patch. \"\n>> +\t\t\t  \"Consider indenting the offending lines.\"));\n>\n> The message says the patch might fail to apply, but isn't it\n> guaranteed to fail?\n\nWorse is it may apply a wrong thing (i.e. an illustration patch in\nthe proposed log message gets applied and committed with a truncated\nlog message).\n"},{"id":"462780","messageId":"CAPig+cTZeSf8UQxU+tMsXC-uKp2eCZ5NEzEkFj93zac4mNAf_Q@mail.gmail.com","threadId":"58396","inReplyTo":"99012733e440be15afc7fd45272e738c71b3ef27.1662559356.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 1/2] patchbreak(), is_scissors_line(): work with a buf/len pair","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-08T00:35:43Z","receivedAt":"2022-09-08T00:36:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 7, 2022 at 10:46 AM Matheus Tavares\n<matheus.bernardino@usp.br> wrote:\n> The next patch will add calls to these two functions from code that\n> works with a char */size_t pair. So let's adapt the functions in\n> preparation.\n> ---\n\nMissing sign-off.\n"},{"id":"462843","messageId":"CAHd-oW4Z-UbFWy=fj=L-CqiG9QP0x3ZLRg0icgK5Xgu=THd4Lw@mail.gmail.com","threadId":"58396","inReplyTo":"xmqqa67buj4m.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-09-09T01:08:19Z","receivedAt":"2022-09-09T01:08:38Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Sep 7, 2022 at 3:36 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n>\n> > Hi Matheus\n> >\n> > Thanks for working on this, having a warning for this is a useful\n> > addition. If the user embeds a diff in their commit message then they\n> > will receive three warnings\n> >\n> > warning: commit message has a patch delimiter: 'diff --git a/file b/file'\n> > warning: commit message has a patch delimiter: '--- file'\n> > warning: git am might fail to apply this patch. Consider indenting the\n> > offending lines.\n> >\n> > I guess it's helpful to show all the lines that are considered\n> > delimiters but it gets quite noisy.\n\nHmm, right :/ Perhaps we could avoid repeating the warning message:\n\nwarning: commit message has a patch delimiter(s):\ndiff --git a/file b/file\n--- file\n....\nwarning: git am might fail to apply this patch.\n\n> True.  I wonder if automatically indenting these lines is an option ;-)\n\nMakes sense. Perhaps under a config option? The difficult part would\nbe for the scissors; just indenting it with whitespaces wouldn't\nsuffice, right?\n"},{"id":"462871","messageId":"xmqqwnaclck0.fsf@gitster.g","threadId":"58396","inReplyTo":"CAHd-oW4Z-UbFWy=fj=L-CqiG9QP0x3ZLRg0icgK5Xgu=THd4Lw@mail.gmail.com","subject":"Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-09T16:47:43Z","receivedAt":"2022-09-09T16:48:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares <matheus.bernardino@usp.br> writes:\n\n> Makes sense. Perhaps under a config option? The difficult part would\n> be for the scissors; just indenting it with whitespaces wouldn't\n> suffice, right?\n\nIt may be difficult not because of any mechanical reasons, but\nbecause we cannot guess WHY the author wrote it there in the log in\nthe first place.  It could be that the author writes explanatory\ntext that is not to become part of the permanent history at the\nbeginning, place scissors, and follow that with log for posterity,\nEXPECTING that all of them is output by format-patch and transmit to\nthe receiving end without modified.\n\nAnother thing is a three-dash marker line in the log message.  I\nmyself did use them to leave a note for myself (which should be left\noutside the official history when it is sent to the list and then\napplied), and I would have been upset if it was stripped or the tool\neven warned against it---I knew what I was doing after all.\n\nCompared to these two, an unindented \"diff \" and its output in the\nlog has no reason to be pre-recoded in the commit message and make\nthe rest of the message a part of the patch, so I am perfectly fine\nif we unconditionally \"escaped\" them.  But I personally think\nscissors and three-dash lines should be left intact.\n"}]}