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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Matheus Tavares <matheus.bernardino@usp.br>, git@vger.kernel.org
Cc: gitster@pobox.com, avarab@gmail.com, l.s.r@web.de
Subject: Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter
Date: Wed, 7 Sep 2022 19:09:05 +0100	[thread overview]
Message-ID: <4d750ff2-9df5-504f-9972-59b082000db0@gmail.com> (raw)
In-Reply-To: <a2c4514aa03657f3b1d822efe3dd630713287ee6.1662559356.git.matheus.bernardino@usp.br>

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