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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 5, 2022, 08:01 UTC
Message-ID: <220905.864jxmme0a.gmgdl@evledraar.gmail.com>
In-Reply-To: <d0b577825124ac684ab304d3a1395f3d2d0708e8.1662333027.git.matheus.bernardino@usp.br>

On Sun, Sep 04 2022, Matheus Tavares wrote:

Show 10 quoted lines

> 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.

Show 5 quoted lines

> +		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..