[PATCH] format-patch: warn if commit msg contains a patch delimiter

[PATCH] format-patch: warn if commit msg contains a patch delimiter

[Matheus Tavares]

When applying a patch, `git am` looks for special delimiter strings
(such as "---") to know where the message ends and the actual diff
starts. If one of these strings appears in the commit message itself,
`am` might get confused and fail to apply the patch properly. This has
already caused inconveniences in the past [1][2]. To help avoid such
problem, let's make `git format-patch` warn on commit messages
containing one of the said strings.

[1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/
[2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/log.c           |  1 +
 log-tree.c              |  1 +
 mailinfo.c              |  4 ++--
 mailinfo.h              |  3 +++
 pretty.c                | 21 ++++++++++++++++++++-
 pretty.h                |  3 ++-
 revision.h              |  3 ++-
 t/t4014-format-patch.sh | 16 ++++++++++++++++
 8 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 56e2d95e86..edc84abaef 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1973,6 +1973,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diffopt.flags.recursive = 1;
 	rev.diffopt.no_free = 1;
 	rev.subject_prefix = fmt_patch_subject_prefix;
+	rev.check_in_body_patch_breaks = 1;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
diff --git a/log-tree.c b/log-tree.c
index 3e8c70ddcf..25ed5452b1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -766,6 +766,7 @@ void show_log(struct rev_info *opt)
 	ctx.after_subject = extra_headers;
 	ctx.preserve_subject = opt->preserve_subject;
 	ctx.encode_email_headers = opt->encode_email_headers;
+	ctx.check_in_body_patch_breaks = opt->check_in_body_patch_breaks;
 	ctx.reflog_info = opt->reflog_info;
 	ctx.fmt = opt->commit_format;
 	ctx.mailmap = opt->mailmap;
diff --git a/mailinfo.c b/mailinfo.c
index 9621ba62a3..9945ea6267 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -646,7 +646,7 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
 	free(ret);
 }
 
-static inline int patchbreak(const struct strbuf *line)
+int patchbreak(const struct strbuf *line)
 {
 	size_t i;
 
@@ -682,7 +682,7 @@ static inline int patchbreak(const struct strbuf *line)
 	return 0;
 }
 
-static int is_scissors_line(const char *line)
+int is_scissors_line(const char *line)
 {
 	const char *c;
 	int scissors = 0, gap = 0;
diff --git a/mailinfo.h b/mailinfo.h
index f2ffd0349e..8d4dda5deb 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -53,4 +53,7 @@ void setup_mailinfo(struct mailinfo *);
 int mailinfo(struct mailinfo *, const char *msg, const char *patch);
 void clear_mailinfo(struct mailinfo *);
 
+int patchbreak(const struct strbuf *line);
+int is_scissors_line(const char *line);
+
 #endif /* MAILINFO_H */
diff --git a/pretty.c b/pretty.c
index 6d819103fb..9f999029f5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -5,6 +5,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "string-list.h"
+#include "mailinfo.h"
 #include "mailmap.h"
 #include "log-tree.h"
 #include "notes.h"
@@ -2097,7 +2098,8 @@ void pp_remainder(struct pretty_print_context *pp,
 		  int indent)
 {
 	struct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;
-	int first = 1;
+	int first = 1, found_delimiter = 0;
+	struct strbuf linebuf = STRBUF_INIT;
 
 	for (;;) {
 		const char *line = *msg_p;
@@ -2107,6 +2109,17 @@ void pp_remainder(struct pretty_print_context *pp,
 		if (!linelen)
 			break;
 
+		if (pp->check_in_body_patch_breaks) {
+			strbuf_reset(&linebuf);
+			strbuf_add(&linebuf, line, linelen);
+			if (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {
+				strbuf_strip_suffix(&linebuf, "\n");
+				warning("commit message has a patch delimiter: '%s'",
+					linebuf.buf);
+				found_delimiter = 1;
+			}
+		}
+
 		if (is_blank_line(line, &linelen)) {
 			if (first)
 				continue;
@@ -2133,6 +2146,12 @@ void pp_remainder(struct pretty_print_context *pp,
 		}
 		strbuf_addch(sb, '\n');
 	}
+
+	if (found_delimiter)
+		warning("git am might fail to apply this patch. "
+			"Consider indenting the offending lines.");
+
+	strbuf_release(&linebuf);
 }
 
 void pretty_print_commit(struct pretty_print_context *pp,
diff --git a/pretty.h b/pretty.h
index f34e24c53a..12df2f4a39 100644
--- a/pretty.h
+++ b/pretty.h
@@ -49,7 +49,8 @@ struct pretty_print_context {
 	struct string_list *mailmap;
 	int color;
 	struct ident_split *from_ident;
-	unsigned encode_email_headers:1;
+	unsigned encode_email_headers:1,
+		 check_in_body_patch_breaks:1;
 	struct pretty_print_describe_status *describe_status;
 
 	/*
diff --git a/revision.h b/revision.h
index 61a9b1316b..f384ab716f 100644
--- a/revision.h
+++ b/revision.h
@@ -230,7 +230,8 @@ struct rev_info {
 			date_mode_explicit:1,
 			preserve_subject:1,
 			encode_email_headers:1,
-			include_header:1;
+			include_header:1,
+			check_in_body_patch_breaks:1;
 	unsigned int	disable_stdin:1;
 	/* --show-linear-break */
 	unsigned int	track_linear:1,
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index fbec8ad2ef..4868ea2b91 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2329,4 +2329,20 @@ test_expect_success 'interdiff: solo-patch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'warn if commit message contains patch delimiter' '
+	>delim &&
+	git add delim &&
+	GIT_EDITOR="printf \"title\n\n---\" >" git commit &&
+	git format-patch -1 2>stderr &&
+	grep "warning: commit message has a patch delimiter" stderr
+'
+
+test_expect_success 'warn if commit message contains scissors' '
+	>scissors &&
+	git add scissors &&
+	GIT_EDITOR="printf \"title\n\n-- >8 --\" >" git commit &&
+	git format-patch -1 2>stderr &&
+	grep "warning: commit message has a patch delimiter" stderr
+'
+
 test_done
-- 
2.37.2

Re: [PATCH] format-patch: warn if commit msg contains a patch delimiter

[Ævar Arnfjörð Bjarmason]

On Sun, Sep 04 2022, Matheus Tavares wrote:

> When applying a patch, `git am` looks for special delimiter strings
> (such as "---") to know where the message ends and the actual diff
> starts. If one of these strings appears in the commit message itself,
> `am` might get confused and fail to apply the patch properly. This has
> already caused inconveniences in the past [1][2]. To help avoid such
> problem, let's make `git format-patch` warn on commit messages
> containing one of the said strings.
>
> [1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/
> [2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/

I followed this topic with one eye, and have run into this myself in the
past. I'm not against this warning, but I wonder if we can't fix
"am/apply" to just be smarter. The cases I've seen are all ones where:

 * We have a copy/pasted git diff, but we could disambiguate based on
   (at least) the "---" line being a telltale for the "real" patch, and
   the "X file changed..." diffstat.
 * We have a not-quite-git-looking patch diff in the commit message
   (which we'd normally detect and apply), as in your [2].

Couldn't we just be a bit smarter about applying these, and do a
look-ahead and find what the user meant.

Is any case, having such a warning won't "settle" this issue, as we're
able to deal with this non-ambiguity in commit objects/the push/fetch
protocol. It's just "format-patch/am" as a "wire protocol" that has this
issue.

But anyway, that's the state of the world now, so warning() about it is
fair, even if we had a fix for the "apply" part we might want to warn
for a while to note that it's an issue on older gits.

> +		if (pp->check_in_body_patch_breaks) {
> +			strbuf_reset(&linebuf);
> +			strbuf_add(&linebuf, line, linelen);
> +			if (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {
> +				strbuf_strip_suffix(&linebuf, "\n");

Hrm, it's a (small) shame that the patchbreak() function takes a "struct
strbuf" rather than a char */size_t in this case (seemingly for no good
reason, as it's "const"?).

Because of that you need to make a copy here, instead of just finding
the "\n" and using the %*s format, anyway, small potatoes.

> +				warning("commit message has a patch delimiter: '%s'",
> +					linebuf.buf);

Missing _()?

> +test_expect_success 'warn if commit message contains patch delimiter' '
> +	>delim &&
> +	git add delim &&
> +	GIT_EDITOR="printf \"title\n\n---\" >" git commit &&

Maybe I'm missing something, but isn't this GIT_EDITOR/printf just
another way of saying something like:

	cat >msg <<-\EOF &&
	"title

	---" >
	EOF
	git commit -F msg && ...

Untested, so maybe not..

Re: [PATCH] format-patch: warn if commit msg contains a patch delimiter

[René Scharfe]

Am 05.09.22 um 10:01 schrieb Ævar Arnfjörð Bjarmason:
>
> On Sun, Sep 04 2022, Matheus Tavares wrote:
>
>> When applying a patch, `git am` looks for special delimiter strings
>> (such as "---") to know where the message ends and the actual diff
>> starts. If one of these strings appears in the commit message itself,
>> `am` might get confused and fail to apply the patch properly. This has
>> already caused inconveniences in the past [1][2]. To help avoid such
>> problem, let's make `git format-patch` warn on commit messages
>> containing one of the said strings.
>>
>> [1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/
>> [2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/
>
> I followed this topic with one eye, and have run into this myself in the
> past. I'm not against this warning, but I wonder if we can't fix
> "am/apply" to just be smarter. The cases I've seen are all ones where:
>
>  * We have a copy/pasted git diff, but we could disambiguate based on
>    (at least) the "---" line being a telltale for the "real" patch, and
>    the "X file changed..." diffstat.
>  * We have a not-quite-git-looking patch diff in the commit message
>    (which we'd normally detect and apply), as in your [2].
>
> Couldn't we just be a bit smarter about applying these, and do a
> look-ahead and find what the user meant.

Whatever we use to separate message from diff can be included in that
message by an unsuspecting user and "---" can be part of a diff.  An
earlier discussion yielded an idea, but no implementation:
https://lore.kernel.org/git/20200204010524-mutt-send-email-mst@kernel.org/

> Is any case, having such a warning won't "settle" this issue, as we're
> able to deal with this non-ambiguity in commit objects/the push/fetch
> protocol. It's just "format-patch/am" as a "wire protocol" that has this
> issue.
>
> But anyway, that's the state of the world now, so warning() about it is
> fair, even if we had a fix for the "apply" part we might want to warn
> for a while to note that it's an issue on older gits.
>
>> +		if (pp->check_in_body_patch_breaks) {
>> +			strbuf_reset(&linebuf);
>> +			strbuf_add(&linebuf, line, linelen);
>> +			if (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {
>> +				strbuf_strip_suffix(&linebuf, "\n");
>
> Hrm, it's a (small) shame that the patchbreak() function takes a "struct
> strbuf" rather than a char */size_t in this case (seemingly for no good
> reason, as it's "const"?).

A strbuf is NUL-terminated, a length-limited string (char */size_t)
doesn't have to be.  That means the current implementation can use
functions like starts_with(), but a faithful version that promises to
stay within a given length cannot.  So the reason is probably
convenience.  With skip_prefix_mem() it wouldn't be that bad, though:

---
 mailinfo.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 9621ba62a3..ae2e70e363 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -646,32 +646,30 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
 	free(ret);
 }

-static inline int patchbreak(const struct strbuf *line)
+static int patchbreak(const char *buf, size_t len)
 {
-	size_t i;
-
 	/* Beginning of a "diff -" header? */
-	if (starts_with(line->buf, "diff -"))
+	if (skip_prefix_mem(buf, len, "diff -", &buf, &len))
 		return 1;

 	/* CVS "Index: " line? */
-	if (starts_with(line->buf, "Index: "))
+	if (skip_prefix_mem(buf, len, "Index: ", &buf, &len))
 		return 1;

 	/*
 	 * "--- <filename>" starts patches without headers
 	 * "---<sp>*" is a manual separator
 	 */
-	if (line->len < 4)
+	if (len < 4)
 		return 0;

-	if (starts_with(line->buf, "---")) {
+	if (skip_prefix_mem(buf, len, "---", &buf, &len)) {
 		/* space followed by a filename? */
-		if (line->buf[3] == ' ' && !isspace(line->buf[4]))
+		if (len > 1 && buf[0] == ' ' && !isspace(buf[1]))
 			return 1;
 		/* Just whitespace? */
-		for (i = 3; i < line->len; i++) {
-			unsigned char c = line->buf[i];
+		for (; len; buf++, len--) {
+			unsigned char c = buf[0];
 			if (c == '\n')
 				return 1;
 			if (!isspace(c))
@@ -682,14 +680,14 @@ static inline int patchbreak(const struct strbuf *line)
 	return 0;
 }

-static int is_scissors_line(const char *line)
+static int is_scissors_line(const char *line, size_t len)
 {
 	const char *c;
 	int scissors = 0, gap = 0;
 	const char *first_nonblank = NULL, *last_nonblank = NULL;
 	int visible, perforation = 0, in_perforation = 0;

-	for (c = line; *c; c++) {
+	for (c = line; len; c++, len--) {
 		if (isspace(*c)) {
 			if (in_perforation) {
 				perforation++;
@@ -705,12 +703,14 @@ static int is_scissors_line(const char *line)
 			perforation++;
 			continue;
 		}
-		if (starts_with(c, ">8") || starts_with(c, "8<") ||
-		    starts_with(c, ">%") || starts_with(c, "%<")) {
+		if (skip_prefix_mem(c, len, ">8", &c, &len) ||
+		    skip_prefix_mem(c, len, "8<", &c, &len) ||
+		    skip_prefix_mem(c, len, ">%", &c, &len) ||
+		    skip_prefix_mem(c, len, "%<", &c, &len)) {
 			in_perforation = 1;
 			perforation += 2;
 			scissors += 2;
-			c++;
+			c--, len++;
 			continue;
 		}
 		in_perforation = 0;
@@ -747,7 +747,8 @@ static int check_inbody_header(struct mailinfo *mi, const struct strbuf *line)
 {
 	if (mi->inbody_header_accum.len &&
 	    (line->buf[0] == ' ' || line->buf[0] == '\t')) {
-		if (mi->use_scissors && is_scissors_line(line->buf)) {
+		if (mi->use_scissors &&
+		    is_scissors_line(line->buf, line->len)) {
 			/*
 			 * This is a scissors line; do not consider this line
 			 * as a header continuation line.
@@ -808,7 +809,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
 	if (convert_to_utf8(mi, line, mi->charset.buf))
 		return 0; /* mi->input_error already set */

-	if (mi->use_scissors && is_scissors_line(line->buf)) {
+	if (mi->use_scissors && is_scissors_line(line->buf, line->len)) {
 		int i;

 		strbuf_setlen(&mi->log_message, 0);
@@ -826,7 +827,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
 		return 0;
 	}

-	if (patchbreak(line)) {
+	if (patchbreak(line->buf, line->len)) {
 		if (mi->message_id)
 			strbuf_addf(&mi->log_message,
 				    "Message-Id: %s\n", mi->message_id);
--
2.37.2

[PATCH v2 0/2] format-patch: warn if commit msg contains a patch delimiter

[Matheus Tavares]

This makes format-patch warn on strings like "---" and "-- >8 --", which
can make a later git am fail to properly apply the generated patch.

Changes in v2:
- Use heredoc in tests.
- Add internationalization _()
- Incorporate René changes to use a buf/size pair.

René, I added your changes from [1] as a preparatory patch. Please let
me know if you are OK with that so that I can add your SoB in the next
re-roll.

[1]: https://lore.kernel.org/git/904b784d-a328-011f-c71a-c2092534e0f7@web.de/

Matheus Tavares (1):
  format-patch: warn if commit msg contains a patch delimiter

René Scharfe (1):
  patchbreak(), is_scissors_line(): work with a buf/len pair

 builtin/log.c           |  1 +
 log-tree.c              |  1 +
 mailinfo.c              | 37 +++++++++++++++++++------------------
 mailinfo.h              |  3 +++
 pretty.c                | 16 +++++++++++++++-
 pretty.h                |  3 ++-
 revision.h              |  3 ++-
 t/t4014-format-patch.sh | 26 ++++++++++++++++++++++++++
 8 files changed, 69 insertions(+), 21 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  99012733e4 patchbreak(), is_scissors_line(): work with a buf/len pair
1:  059811c85f ! 2:  a2c4514aa0 format-patch: warn if commit msg contains a patch delimiter
    @@ mailinfo.c: static void decode_transfer_encoding(struct mailinfo *mi, struct str
      	free(ret);
      }
      
    --static inline int patchbreak(const struct strbuf *line)
    -+int patchbreak(const struct strbuf *line)
    +-static inline int patchbreak(const char *buf, size_t len)
    ++int patchbreak(const char *buf, size_t len)
      {
    - 	size_t i;
    - 
    -@@ mailinfo.c: static inline int patchbreak(const struct strbuf *line)
    + 	/* Beginning of a "diff -" header? */
    + 	if (skip_prefix_mem(buf, len, "diff -", &buf, &len))
    +@@ mailinfo.c: static inline int patchbreak(const char *buf, size_t len)
      	return 0;
      }
      
    --static int is_scissors_line(const char *line)
    -+int is_scissors_line(const char *line)
    +-static int is_scissors_line(const char *line, size_t len)
    ++int is_scissors_line(const char *line, size_t len)
      {
      	const char *c;
      	int scissors = 0, gap = 0;
    @@ mailinfo.h: void setup_mailinfo(struct mailinfo *);
      int mailinfo(struct mailinfo *, const char *msg, const char *patch);
      void clear_mailinfo(struct mailinfo *);
      
    -+int patchbreak(const struct strbuf *line);
    -+int is_scissors_line(const char *line);
    ++int patchbreak(const char *line, size_t len);
    ++int is_scissors_line(const char *line, size_t len);
     +
      #endif /* MAILINFO_H */
     
    @@ pretty.c: void pp_remainder(struct pretty_print_context *pp,
      	struct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;
     -	int first = 1;
     +	int first = 1, found_delimiter = 0;
    -+	struct strbuf linebuf = STRBUF_INIT;
      
      	for (;;) {
      		const char *line = *msg_p;
    @@ pretty.c: void pp_remainder(struct pretty_print_context *pp,
      		if (!linelen)
      			break;
      
    -+		if (pp->check_in_body_patch_breaks) {
    -+			strbuf_reset(&linebuf);
    -+			strbuf_add(&linebuf, line, linelen);
    -+			if (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {
    -+				strbuf_strip_suffix(&linebuf, "\n");
    -+				warning("commit message has a patch delimiter: '%s'",
    -+					linebuf.buf);
    -+				found_delimiter = 1;
    -+			}
    ++		if (pp->check_in_body_patch_breaks &&
    ++		    (patchbreak(line, linelen) || is_scissors_line(line, linelen))) {
    ++			warning(_("commit message has a patch delimiter: '%.*s'"),
    ++				line[linelen - 1] == '\n' ? linelen - 1 : linelen,
    ++				line);
    ++			found_delimiter = 1;
     +		}
     +
      		if (is_blank_line(line, &linelen)) {
    @@ pretty.c: void pp_remainder(struct pretty_print_context *pp,
      		strbuf_addch(sb, '\n');
      	}
     +
    -+	if (found_delimiter)
    -+		warning("git am might fail to apply this patch. "
    -+			"Consider indenting the offending lines.");
    -+
    -+	strbuf_release(&linebuf);
    ++	if (found_delimiter) {
    ++		warning(_("git am might fail to apply this patch. "
    ++			  "Consider indenting the offending lines."));
    ++	}
      }
      
      void pretty_print_commit(struct pretty_print_context *pp,
    @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: solo-patch' '
     +test_expect_success 'warn if commit message contains patch delimiter' '
     +	>delim &&
     +	git add delim &&
    -+	GIT_EDITOR="printf \"title\n\n---\" >" git commit &&
    ++	cat >msg <<-\EOF &&
    ++	title
    ++
    ++	---
    ++	EOF
    ++	git commit -F msg &&
     +	git format-patch -1 2>stderr &&
     +	grep "warning: commit message has a patch delimiter" stderr
     +'
    @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: solo-patch' '
     +test_expect_success 'warn if commit message contains scissors' '
     +	>scissors &&
     +	git add scissors &&
    -+	GIT_EDITOR="printf \"title\n\n-- >8 --\" >" git commit &&
    ++	cat >msg <<-\EOF &&
    ++	title
    ++
    ++	-- >8 --
    ++	EOF
    ++	git commit -F msg &&
     +	git format-patch -1 2>stderr &&
     +	grep "warning: commit message has a patch delimiter" stderr
     +'
-- 
2.37.2

[PATCH v2 1/2] patchbreak(), is_scissors_line(): work with a buf/len pair

[Matheus Tavares]

From: René Scharfe <l.s.r@web.de>

The next patch will add calls to these two functions from code that
works with a char */size_t pair. So let's adapt the functions in
preparation.
---
 mailinfo.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 9621ba62a3..f0a690b6e8 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -646,32 +646,30 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
 	free(ret);
 }
 
-static inline int patchbreak(const struct strbuf *line)
+static inline int patchbreak(const char *buf, size_t len)
 {
-	size_t i;
-
 	/* Beginning of a "diff -" header? */
-	if (starts_with(line->buf, "diff -"))
+	if (skip_prefix_mem(buf, len, "diff -", &buf, &len))
 		return 1;
 
 	/* CVS "Index: " line? */
-	if (starts_with(line->buf, "Index: "))
+	if (skip_prefix_mem(buf, len, "Index: ", &buf, &len))
 		return 1;
 
 	/*
 	 * "--- <filename>" starts patches without headers
 	 * "---<sp>*" is a manual separator
 	 */
-	if (line->len < 4)
+	if (len < 4)
 		return 0;
 
-	if (starts_with(line->buf, "---")) {
+	if (skip_prefix_mem(buf, len, "---", &buf, &len)) {
 		/* space followed by a filename? */
-		if (line->buf[3] == ' ' && !isspace(line->buf[4]))
+		if (len > 1 && buf[0] == ' ' && !isspace(buf[1]))
 			return 1;
 		/* Just whitespace? */
-		for (i = 3; i < line->len; i++) {
-			unsigned char c = line->buf[i];
+		for (; len; buf++, len--) {
+			unsigned char c = buf[0];
 			if (c == '\n')
 				return 1;
 			if (!isspace(c))
@@ -682,14 +680,14 @@ static inline int patchbreak(const struct strbuf *line)
 	return 0;
 }
 
-static int is_scissors_line(const char *line)
+static int is_scissors_line(const char *line, size_t len)
 {
 	const char *c;
 	int scissors = 0, gap = 0;
 	const char *first_nonblank = NULL, *last_nonblank = NULL;
 	int visible, perforation = 0, in_perforation = 0;
 
-	for (c = line; *c; c++) {
+	for (c = line; len; c++, len--) {
 		if (isspace(*c)) {
 			if (in_perforation) {
 				perforation++;
@@ -705,12 +703,14 @@ static int is_scissors_line(const char *line)
 			perforation++;
 			continue;
 		}
-		if (starts_with(c, ">8") || starts_with(c, "8<") ||
-		    starts_with(c, ">%") || starts_with(c, "%<")) {
+		if (skip_prefix_mem(c, len, ">8", &c, &len) ||
+		    skip_prefix_mem(c, len, "8<", &c, &len) ||
+		    skip_prefix_mem(c, len, ">%", &c, &len) ||
+		    skip_prefix_mem(c, len, "%<", &c, &len)) {
 			in_perforation = 1;
 			perforation += 2;
 			scissors += 2;
-			c++;
+			c--, len++;
 			continue;
 		}
 		in_perforation = 0;
@@ -747,7 +747,8 @@ static int check_inbody_header(struct mailinfo *mi, const struct strbuf *line)
 {
 	if (mi->inbody_header_accum.len &&
 	    (line->buf[0] == ' ' || line->buf[0] == '\t')) {
-		if (mi->use_scissors && is_scissors_line(line->buf)) {
+		if (mi->use_scissors &&
+		    is_scissors_line(line->buf, line->len)) {
 			/*
 			 * This is a scissors line; do not consider this line
 			 * as a header continuation line.
@@ -808,7 +809,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
 	if (convert_to_utf8(mi, line, mi->charset.buf))
 		return 0; /* mi->input_error already set */
 
-	if (mi->use_scissors && is_scissors_line(line->buf)) {
+	if (mi->use_scissors && is_scissors_line(line->buf, line->len)) {
 		int i;
 
 		strbuf_setlen(&mi->log_message, 0);
@@ -826,7 +827,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
 		return 0;
 	}
 
-	if (patchbreak(line)) {
+	if (patchbreak(line->buf, line->len)) {
 		if (mi->message_id)
 			strbuf_addf(&mi->log_message,
 				    "Message-Id: %s\n", mi->message_id);
-- 
2.37.2

Re: [PATCH v2 1/2] patchbreak(), is_scissors_line(): work with a buf/len pair

[Phillip Wood]

On 07/09/2022 15:44, Matheus Tavares wrote:
> From: René Scharfe <l.s.r@web.de>
> 
> The next patch will add calls to these two functions from code that
> works with a char */size_t pair. So let's adapt the functions in
> preparation.

Reading this I wonder if we should add a starts_with_mem() function, 
rather than having to pass pointers to buf and len to skip_prefix_mem().

Best Wishes

Phillip

> ---
>   mailinfo.c | 37 +++++++++++++++++++------------------
>   1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/mailinfo.c b/mailinfo.c
> index 9621ba62a3..f0a690b6e8 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -646,32 +646,30 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
>   	free(ret);
>   }
>   
> -static inline int patchbreak(const struct strbuf *line)
> +static inline int patchbreak(const char *buf, size_t len)
>   {
> -	size_t i;
> -
>   	/* Beginning of a "diff -" header? */
> -	if (starts_with(line->buf, "diff -"))
> +	if (skip_prefix_mem(buf, len, "diff -", &buf, &len))
>   		return 1;
>   
>   	/* CVS "Index: " line? */
> -	if (starts_with(line->buf, "Index: "))
> +	if (skip_prefix_mem(buf, len, "Index: ", &buf, &len))
>   		return 1;
>   
>   	/*
>   	 * "--- <filename>" starts patches without headers
>   	 * "---<sp>*" is a manual separator
>   	 */
> -	if (line->len < 4)
> +	if (len < 4)
>   		return 0;
>   
> -	if (starts_with(line->buf, "---")) {
> +	if (skip_prefix_mem(buf, len, "---", &buf, &len)) {
>   		/* space followed by a filename? */
> -		if (line->buf[3] == ' ' && !isspace(line->buf[4]))
> +		if (len > 1 && buf[0] == ' ' && !isspace(buf[1]))
>   			return 1;
>   		/* Just whitespace? */
> -		for (i = 3; i < line->len; i++) {
> -			unsigned char c = line->buf[i];
> +		for (; len; buf++, len--) {
> +			unsigned char c = buf[0];
>   			if (c == '\n')
>   				return 1;
>   			if (!isspace(c))
> @@ -682,14 +680,14 @@ static inline int patchbreak(const struct strbuf *line)
>   	return 0;
>   }
>   
> -static int is_scissors_line(const char *line)
> +static int is_scissors_line(const char *line, size_t len)
>   {
>   	const char *c;
>   	int scissors = 0, gap = 0;
>   	const char *first_nonblank = NULL, *last_nonblank = NULL;
>   	int visible, perforation = 0, in_perforation = 0;
>   
> -	for (c = line; *c; c++) {
> +	for (c = line; len; c++, len--) {
>   		if (isspace(*c)) {
>   			if (in_perforation) {
>   				perforation++;
> @@ -705,12 +703,14 @@ static int is_scissors_line(const char *line)
>   			perforation++;
>   			continue;
>   		}
> -		if (starts_with(c, ">8") || starts_with(c, "8<") ||
> -		    starts_with(c, ">%") || starts_with(c, "%<")) {
> +		if (skip_prefix_mem(c, len, ">8", &c, &len) ||
> +		    skip_prefix_mem(c, len, "8<", &c, &len) ||
> +		    skip_prefix_mem(c, len, ">%", &c, &len) ||
> +		    skip_prefix_mem(c, len, "%<", &c, &len)) {
>   			in_perforation = 1;
>   			perforation += 2;
>   			scissors += 2;
> -			c++;
> +			c--, len++;
>   			continue;
>   		}
>   		in_perforation = 0;
> @@ -747,7 +747,8 @@ static int check_inbody_header(struct mailinfo *mi, const struct strbuf *line)
>   {
>   	if (mi->inbody_header_accum.len &&
>   	    (line->buf[0] == ' ' || line->buf[0] == '\t')) {
> -		if (mi->use_scissors && is_scissors_line(line->buf)) {
> +		if (mi->use_scissors &&
> +		    is_scissors_line(line->buf, line->len)) {
>   			/*
>   			 * This is a scissors line; do not consider this line
>   			 * as a header continuation line.
> @@ -808,7 +809,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
>   	if (convert_to_utf8(mi, line, mi->charset.buf))
>   		return 0; /* mi->input_error already set */
>   
> -	if (mi->use_scissors && is_scissors_line(line->buf)) {
> +	if (mi->use_scissors && is_scissors_line(line->buf, line->len)) {
>   		int i;
>   
>   		strbuf_setlen(&mi->log_message, 0);
> @@ -826,7 +827,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
>   		return 0;
>   	}
>   
> -	if (patchbreak(line)) {
> +	if (patchbreak(line->buf, line->len)) {
>   		if (mi->message_id)
>   			strbuf_addf(&mi->log_message,
>   				    "Message-Id: %s\n", mi->message_id);

Re: [PATCH v2 1/2] patchbreak(), is_scissors_line(): work with a buf/len pair

[Eric Sunshine]

On Wed, Sep 7, 2022 at 10:46 AM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
> The next patch will add calls to these two functions from code that
> works with a char */size_t pair. So let's adapt the functions in
> preparation.
> ---

Missing sign-off.

[PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter

[Matheus Tavares]

When applying a patch, `git am` looks for special delimiter strings
(such as "---") to know where the message ends and the actual diff
starts. If one of these strings appears in the commit message itself,
`am` might get confused and fail to apply the patch properly. This has
already caused inconveniences in the past [1][2]. To help avoid such
problem, let's make `git format-patch` warn on commit messages
containing one of the said strings.

[1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/
[2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/log.c           |  1 +
 log-tree.c              |  1 +
 mailinfo.c              |  4 ++--
 mailinfo.h              |  3 +++
 pretty.c                | 16 +++++++++++++++-
 pretty.h                |  3 ++-
 revision.h              |  3 ++-
 t/t4014-format-patch.sh | 26 ++++++++++++++++++++++++++
 8 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 56e2d95e86..edc84abaef 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1973,6 +1973,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diffopt.flags.recursive = 1;
 	rev.diffopt.no_free = 1;
 	rev.subject_prefix = fmt_patch_subject_prefix;
+	rev.check_in_body_patch_breaks = 1;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
diff --git a/log-tree.c b/log-tree.c
index 3e8c70ddcf..25ed5452b1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -766,6 +766,7 @@ void show_log(struct rev_info *opt)
 	ctx.after_subject = extra_headers;
 	ctx.preserve_subject = opt->preserve_subject;
 	ctx.encode_email_headers = opt->encode_email_headers;
+	ctx.check_in_body_patch_breaks = opt->check_in_body_patch_breaks;
 	ctx.reflog_info = opt->reflog_info;
 	ctx.fmt = opt->commit_format;
 	ctx.mailmap = opt->mailmap;
diff --git a/mailinfo.c b/mailinfo.c
index f0a690b6e8..d227397f1c 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -646,7 +646,7 @@ static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
 	free(ret);
 }
 
-static inline int patchbreak(const char *buf, size_t len)
+int patchbreak(const char *buf, size_t len)
 {
 	/* Beginning of a "diff -" header? */
 	if (skip_prefix_mem(buf, len, "diff -", &buf, &len))
@@ -680,7 +680,7 @@ static inline int patchbreak(const char *buf, size_t len)
 	return 0;
 }
 
-static int is_scissors_line(const char *line, size_t len)
+int is_scissors_line(const char *line, size_t len)
 {
 	const char *c;
 	int scissors = 0, gap = 0;
diff --git a/mailinfo.h b/mailinfo.h
index f2ffd0349e..347eefe856 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -53,4 +53,7 @@ void setup_mailinfo(struct mailinfo *);
 int mailinfo(struct mailinfo *, const char *msg, const char *patch);
 void clear_mailinfo(struct mailinfo *);
 
+int patchbreak(const char *line, size_t len);
+int is_scissors_line(const char *line, size_t len);
+
 #endif /* MAILINFO_H */
diff --git a/pretty.c b/pretty.c
index 6d819103fb..913d974b3a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -5,6 +5,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "string-list.h"
+#include "mailinfo.h"
 #include "mailmap.h"
 #include "log-tree.h"
 #include "notes.h"
@@ -2097,7 +2098,7 @@ void pp_remainder(struct pretty_print_context *pp,
 		  int indent)
 {
 	struct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;
-	int first = 1;
+	int first = 1, found_delimiter = 0;
 
 	for (;;) {
 		const char *line = *msg_p;
@@ -2107,6 +2108,14 @@ void pp_remainder(struct pretty_print_context *pp,
 		if (!linelen)
 			break;
 
+		if (pp->check_in_body_patch_breaks &&
+		    (patchbreak(line, linelen) || is_scissors_line(line, linelen))) {
+			warning(_("commit message has a patch delimiter: '%.*s'"),
+				line[linelen - 1] == '\n' ? linelen - 1 : linelen,
+				line);
+			found_delimiter = 1;
+		}
+
 		if (is_blank_line(line, &linelen)) {
 			if (first)
 				continue;
@@ -2133,6 +2142,11 @@ void pp_remainder(struct pretty_print_context *pp,
 		}
 		strbuf_addch(sb, '\n');
 	}
+
+	if (found_delimiter) {
+		warning(_("git am might fail to apply this patch. "
+			  "Consider indenting the offending lines."));
+	}
 }
 
 void pretty_print_commit(struct pretty_print_context *pp,
diff --git a/pretty.h b/pretty.h
index f34e24c53a..12df2f4a39 100644
--- a/pretty.h
+++ b/pretty.h
@@ -49,7 +49,8 @@ struct pretty_print_context {
 	struct string_list *mailmap;
 	int color;
 	struct ident_split *from_ident;
-	unsigned encode_email_headers:1;
+	unsigned encode_email_headers:1,
+		 check_in_body_patch_breaks:1;
 	struct pretty_print_describe_status *describe_status;
 
 	/*
diff --git a/revision.h b/revision.h
index 61a9b1316b..f384ab716f 100644
--- a/revision.h
+++ b/revision.h
@@ -230,7 +230,8 @@ struct rev_info {
 			date_mode_explicit:1,
 			preserve_subject:1,
 			encode_email_headers:1,
-			include_header:1;
+			include_header:1,
+			check_in_body_patch_breaks:1;
 	unsigned int	disable_stdin:1;
 	/* --show-linear-break */
 	unsigned int	track_linear:1,
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index fbec8ad2ef..4bbf1156e9 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2329,4 +2329,30 @@ test_expect_success 'interdiff: solo-patch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'warn if commit message contains patch delimiter' '
+	>delim &&
+	git add delim &&
+	cat >msg <<-\EOF &&
+	title
+
+	---
+	EOF
+	git commit -F msg &&
+	git format-patch -1 2>stderr &&
+	grep "warning: commit message has a patch delimiter" stderr
+'
+
+test_expect_success 'warn if commit message contains scissors' '
+	>scissors &&
+	git add scissors &&
+	cat >msg <<-\EOF &&
+	title
+
+	-- >8 --
+	EOF
+	git commit -F msg &&
+	git format-patch -1 2>stderr &&
+	grep "warning: commit message has a patch delimiter" stderr
+'
+
 test_done
-- 
2.37.2

Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter

[Phillip Wood]

Hi Matheus

On 07/09/2022 15:44, Matheus Tavares wrote:
> When applying a patch, `git am` looks for special delimiter strings
> (such as "---") to know where the message ends and the actual diff
> starts. If one of these strings appears in the commit message itself,
> `am` might get confused and fail to apply the patch properly. This has
> already caused inconveniences in the past [1][2]. To help avoid such
> problem, let's make `git format-patch` warn on commit messages
> containing one of the said strings.

Thanks for working on this, having a warning for this is a useful 
addition. If the user embeds a diff in their commit message then they 
will receive three warnings

warning: commit message has a patch delimiter: 'diff --git a/file b/file'
warning: commit message has a patch delimiter: '--- file'
warning: git am might fail to apply this patch. Consider indenting the 
offending lines.

I guess it's helpful to show all the lines that are considered 
delimiters but it gets quite noisy.


> diff --git a/pretty.c b/pretty.c
> index 6d819103fb..913d974b3a 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -2107,6 +2108,14 @@ void pp_remainder(struct pretty_print_context *pp,
>   		if (!linelen)
>   			break;
>   
> +		if (pp->check_in_body_patch_breaks &&
> +		    (patchbreak(line, linelen) || is_scissors_line(line, linelen))) {
> +			warning(_("commit message has a patch delimiter: '%.*s'"),
> +				line[linelen - 1] == '\n' ? linelen - 1 : linelen,
> +				line);
> +			found_delimiter = 1;
> +		}
> +
>   		if (is_blank_line(line, &linelen)) {
>   			if (first)
>   				continue;
> @@ -2133,6 +2142,11 @@ void pp_remainder(struct pretty_print_context *pp,
>   		}
>   		strbuf_addch(sb, '\n');
>   	}
> +
> +	if (found_delimiter) {
> +		warning(_("git am might fail to apply this patch. "
> +			  "Consider indenting the offending lines."));

The message says the patch might fail to apply, but isn't it guaranteed 
to fail?

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index fbec8ad2ef..4bbf1156e9 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -2329,4 +2329,30 @@ test_expect_success 'interdiff: solo-patch' '
>   	test_cmp expect actual
>   '
>   
> +test_expect_success 'warn if commit message contains patch delimiter' '
> +	>delim &&
> +	git add delim &&
> +	cat >msg <<-\EOF &&
> +	title
> +
> +	---
> +	EOF
> +	git commit -F msg &&
> +	git format-patch -1 2>stderr &&
> +	grep "warning: commit message has a patch delimiter" stderr

I think it would be worth checking for the second message as well in the 
tests.


Best Wishes

Phillip

Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Matheus
>
> On 07/09/2022 15:44, Matheus Tavares wrote:
>> When applying a patch, `git am` looks for special delimiter strings
>> (such as "---") to know where the message ends and the actual diff
>> starts. If one of these strings appears in the commit message itself,
>> `am` might get confused and fail to apply the patch properly. This has
>> already caused inconveniences in the past [1][2]. To help avoid such
>> problem, let's make `git format-patch` warn on commit messages
>> containing one of the said strings.
>
> Thanks for working on this, having a warning for this is a useful
> addition. If the user embeds a diff in their commit message then they
> will receive three warnings
>
> warning: commit message has a patch delimiter: 'diff --git a/file b/file'
> warning: commit message has a patch delimiter: '--- file'
> warning: git am might fail to apply this patch. Consider indenting the
> offending lines.
>
> I guess it's helpful to show all the lines that are considered
> delimiters but it gets quite noisy.

True.  I wonder if automatically indenting these lines is an option ;-)

>> +
>> +	if (found_delimiter) {
>> +		warning(_("git am might fail to apply this patch. "
>> +			  "Consider indenting the offending lines."));
>
> The message says the patch might fail to apply, but isn't it
> guaranteed to fail?

Worse is it may apply a wrong thing (i.e. an illustration patch in
the proposed log message gets applied and committed with a truncated
log message).

Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter

[Matheus Tavares]

On Wed, Sep 7, 2022 at 3:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Hi Matheus
> >
> > Thanks for working on this, having a warning for this is a useful
> > addition. If the user embeds a diff in their commit message then they
> > will receive three warnings
> >
> > warning: commit message has a patch delimiter: 'diff --git a/file b/file'
> > warning: commit message has a patch delimiter: '--- file'
> > warning: git am might fail to apply this patch. Consider indenting the
> > offending lines.
> >
> > I guess it's helpful to show all the lines that are considered
> > delimiters but it gets quite noisy.

Hmm, right :/ Perhaps we could avoid repeating the warning message:

warning: commit message has a patch delimiter(s):
diff --git a/file b/file
--- file
....
warning: git am might fail to apply this patch.

> True.  I wonder if automatically indenting these lines is an option ;-)

Makes sense. Perhaps under a config option? The difficult part would
be for the scissors; just indenting it with whitespaces wouldn't
suffice, right?

Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter

[Junio C Hamano]

Matheus Tavares <matheus.bernardino@usp.br> writes:

> Makes sense. Perhaps under a config option? The difficult part would
> be for the scissors; just indenting it with whitespaces wouldn't
> suffice, right?

It may be difficult not because of any mechanical reasons, but
because we cannot guess WHY the author wrote it there in the log in
the first place.  It could be that the author writes explanatory
text that is not to become part of the permanent history at the
beginning, place scissors, and follow that with log for posterity,
EXPECTING that all of them is output by format-patch and transmit to
the receiving end without modified.

Another thing is a three-dash marker line in the log message.  I
myself did use them to leave a note for myself (which should be left
outside the official history when it is sent to the list and then
applied), and I would have been upset if it was stripped or the tool
even warned against it---I knew what I was doing after all.

Compared to these two, an unindented "diff " and its output in the
log has no reason to be pre-recoded in the commit message and make
the rest of the message a part of the patch, so I am perfectly fine
if we unconditionally "escaped" them.  But I personally think
scissors and three-dash lines should be left intact.

Re: [PATCH v2 0/2] format-patch: warn if commit msg contains a patch delimiter

[René Scharfe]

Am 07.09.22 um 16:44 schrieb Matheus Tavares:
> This makes format-patch warn on strings like "---" and "-- >8 --", which
> can make a later git am fail to properly apply the generated patch.
>
> Changes in v2:
> - Use heredoc in tests.
> - Add internationalization _()
> - Incorporate René changes to use a buf/size pair.
>
> René, I added your changes from [1] as a preparatory patch. Please let
> me know if you are OK with that so that I can add your SoB in the next
> re-roll.

Fine with me, but I think they are trivial and you don't actually need my
sign-off.

René