Re: [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 7, 2022, 18:09 UTC
- Message-ID
- <4d750ff2-9df5-504f-9972-59b082000db0@gmail.com>
- In-Reply-To
- <a2c4514aa03657f3b1d822efe3dd630713287ee6.1662559356.git.matheus.bernardino@usp.br>
Hi Matheus
On 07/09/2022 15:44, Matheus Tavares wrote:
Show 7 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.
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.
Show 27 quoted lines
> 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?
Show 19 quoted lines
> 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