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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org, l.s.r@web.de, gitster@pobox.com
Subject: Re: [PATCH] format-patch: warn if commit msg contains a patch delimiter
Date: Mon, 05 Sep 2022 10:01:16 +0200	[thread overview]
Message-ID: <220905.864jxmme0a.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <d0b577825124ac684ab304d3a1395f3d2d0708e8.1662333027.git.matheus.bernardino@usp.br>

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