Re: [PATCH v2 2/2] templates: detect commit messages containing diffs

[Junio C Hamano <gitster@pobox.com>]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the body of a commit message contains a diff that is not indented
> then "git am" will treat that diff as part of the patch rather than
> as part of the commit message. This allows it to apply email messages
> that were created by adding a commit message in front of a regular diff
> without adding the "---" separator used by "git format-patch". This
> often surprises users [1-4] so add a check to the sample "commit-msg"
> hook to reject messages that would confuse "git am". Even if a project
> does not use an email based workflow it is not uncommon for people
> to generate patches from it and apply them with "git am". Therefore
> it is still worth discouraging the creation of commit messages that
> would not be applied correctly.
>
> A further source of confusion when applying patches with "git am" is
> the "---" separator that is added by "git format patch". If a commit
> message body contains that line then it will be truncated by "git am".
> As this is often used by patch authors to add some commentary that
> they do not want to end up in the commit message when the patch is
> applied, the hook does not complain about the presence of "---" lines
> in the message.

"git format match" -> "git format-patch".

> Detecting if the message contains a diff is complicated by the
> hook being passed the message before it is cleaned up so we need to
> ignore any diffs below the scissors line.

Sorry, but I do not quite understand the logic here.  In e-mailed
messages, the way the scissors line is most commonly used is to have
something like this.

	Hi, I read your problem report, and I think what is going on
	is ... (lengthy discussion here).

	Can you try this patch?

	--- >8 ---
	Subject: frotz: try working around nitfol

	As we cannot easily tell if the gostak will distim these
	patciular doshes, let's be careful to see ...

	diff - will be used to confuse the mailinfo

	Signed-off-by: a.u.thour
	---
	(diffstat here)
	(patch here)

and "diff - will be used to confuse" is something we would want to
notice.  But I am not sure if the use case of committing a scissors
line.  You help those who write a three-dash line and materials
meant to be kept outside of the final commit at the end, so if is
this an attempt to help those who write a scissors line and
materials meant to be kept outside of the final commit at the
beginning, I can understand, but then don't you want to notice "diff
-" that appears after the scissors line?  I do not offhand remember
what happens to a "diff -" that appears before the scissors (i.e.,
if you write "diff -" before "Can you try this patch?"), but I
wouldn't be surprised if mailinfo stopped there long before it sees
the scissors.

> There are also two possible
> config keys to check to find the comment character at the start of
> the scissors line.

Also I do not think scissors requires to be a comment.

So, I am a bit confused.

> The first paragraph of the commit message becomes
> the email subject header which beings "Subject: " and so does not
> need to be checked.

Great.

> The trailing ".*" when matching commented lines
> ensures that if the comment string ends with a "$" it is not treated
> as an anchor.

I am not sure what this means.  Wouldn't these three

	sed -e '/^#/d'
	sed -e '/^#.*/d'
	sed -e '/^#.*$/d'

work exactly the same way?

Thanks.

> [1] https://lore.kernel.org/git/bcqvh7ahjjgzpgxwnr4kh3hfkksfruf54refyry3ha7qk7dldf@fij5calmscvm
> [2] https://lore.kernel.org/git/ca13705ae4817ffba16f97530637411b59c9eb19.camel@scientia.org/
> [3] https://lore.kernel.org/git/d0b577825124ac684ab304d3a1395f3d2d0708e8.1662333027.git.matheus.bernardino@usp.br/
> [4] https://lore.kernel.org/git/CAFOYHZC6Qd9wkoWPcTJDxAs9u=FGpHQTkjE-guhwkya0DRVA6g@mail.gmail.com/
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  templates/hooks/commit-msg.sample | 54 +++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/templates/hooks/commit-msg.sample b/templates/hooks/commit-msg.sample
> index b58d1184a9d..f7458efe62f 100755
> --- a/templates/hooks/commit-msg.sample
> +++ b/templates/hooks/commit-msg.sample
> @@ -15,10 +15,60 @@
>  # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
>  # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
>  
> -# This example catches duplicate Signed-off-by lines.
> +# This example catches duplicate Signed-off-by lines and messages that
> +# would confuse 'git am'.
> +
> +ret=0
>  
>  test "" = "$(grep '^Signed-off-by: ' "$1" |
>  	 sort | uniq -c | sed -e '/^[ 	]*1[ 	]/d')" || {
>  	echo >&2 Duplicate Signed-off-by lines.
> -	exit 1
> +	ret=1
>  }
> +
> +comment_re="$(
> +	{
> +		git config --get-regexp "^core\.comment(char|string)\$" ||
> +			echo '#'
> +	} | sed -n -e '
> +		${
> +			s/^[^ ]* //
> +			s|[][*./\]|\\&|g
> +			s/^auto$/[#;@!$%^&|:]/
> +			p
> +		}'
> +)"
> +scissors_line="^${comment_re} -\{8,\} >8 -\{8,\}\$"
> +comment_line="^${comment_re}.*"
> +blank_line='^[ 	]*$'
> +# Disallow lines starting with "diff -" or "Index: " in the body of the
> +# message. Stop looking if we see a scissors line.
> +line="$(sed -n -e "
> +	# Skip comments and blank lines at the start of the file.
> +	/${scissors_line}/q
> +	/${comment_line}/d
> +	/${blank_line}/d
> +	# The first paragraph will become the subject header so
> +	# does not need to be checked.
> +	: subject
> +	n
> +	/${scissors_line}/q
> +	/${blank_line}/!b subject
> +	# Check the body of the message for problematic
> +	# prefixes.
> +	: body
> +	n
> +	/${scissors_line}/q
> +	/${comment_line}/b body
> +	/^diff -/{p;q;}
> +	/^Index: /{p;q;}
> +	b body
> +	" "$1")"
> +if test -n "$line"
> +then
> +	echo >&2 "Message contains a diff that will confuse 'git am'."
> +	echo >&2 "To fix this indent the diff."
> +	ret=1
> +fi
> +
> +exit $ret