Re: [PATCH] git-am: Handle "git show" output correctly

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2012, 20:06 UTC
Message-ID: <7vtxv3atvu.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1347473304-21418-1-git-send-email-pjones@redhat.com>

Peter Jones <pjones@redhat.com> writes:

Show 35 quoted lines

> This patch adds the ability for "git am" to accept patches in the format
> generated by "git show".  Some people erroneously use "git show" instead
> of "git format-patch", and it's nice as a maintainer to be able to
> easily take their patch rather than going back and forth with them to
> get a "correctly" formatted patch containing exactly the same actual
> information.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
>  git-am.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/git-am.sh b/git-am.sh
> index c682d34..210e9fe 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -216,6 +216,21 @@ check_patch_format () {
>  		read l2
>  		read l3
>  		case "$l1" in
> +		"commit "*)
> +			case "$l2" in
> +			"Author: "*)
> +				case "$l3" in
> +				"Date: "*)
> +					patch_format=gitshow
> +					;;
> +				*)
> +					;;
> +				esac
> +				;;
> +			*)
> +				;;
> +			esac
> +			;;

At least the inner one could become easier to read by losing one level of nesting, e.g.

	case "$l2,,$l3" in
        "Author: *",,"Date: ")
		found it
                ;;
	esac

I wonder what the severity of the damage if we misidentify the patch format in this function would be? If it is severe enough, the check for the first line may want to become a bit more strict to avoid misidentification (e.g. expr "$l1" : 'commit [0-9a-f]\{40\}$'). Perhaps we don't care. I dunno.

Show 8 quoted lines

> @@ -321,6 +336,51 @@ split_patches () {
>  		this=
>  		msgnum=
>  		;;
> +	gitshow)
> +		this=0
> +		for patch in "$@"
> +		do

So each input file is expected to be nothing but an output from "git show" for a single commit; in other words, not concatenation of them, nor just an e-mail message that has "git show" output copy&pasted in the body with some other cruft, but plausibly was delibered as a separate attachment file.

I somehow was visualizing that you were trying to accept mails I sometimes see here like:

	From: somebody
        Date: someday

        Hi, a long winded discussion that talks about the motivation
        behind the patch comes here.

	commit 4d8c4db13c8c4c79b6fc0a38ff52d85d3543aa7a
        Author: A U Thor <author@example.com>
        Date: Tue Sep 11 12:34:56 2012 +0900

	    a one liner that just says "bugfix" and nothing else

	diff --git ....

and that was one of the reasons I thought (but didn't say in my responses) "Why bother? When running 'am' on such a message you will have to edit the message to move things around anyway".

If the target is a stand-alone "git show" output, at least we do not have to worry about such a case.

Show 29 quoted lines

> +			this=`expr "$this" + 1`
> +			msgnum=`printf "%0${prec}d" $this`
> +			# The first nonemptyline after an empty line is the
> +			# subject, and the body starts with the next nonempty
> +			# line.
> +			perl -ne 'BEGIN {
> +					$diff = 0; $subject = 0; $subjtext="";
> +				}
> +				if ($diff == 1 || /^diff/ || /^---$/) {
> +					$diff = 1 ;
> +					print ;
> +				} elsif ($subject > 1) {
> +					s/^    // ;
> +					print ;
> +				} elsif ($subject == 1 && !/^\s+$/) {
> +					s/^    // ;
> +					$subjtext = "$subjtext $_";
> +				} elsif ($subject == 1) {
> +					$subject = 2 ;
> +					print "Subject: ", $subjtext ;
> +					s/^    // ;
> +					print ;
> +				} elsif ($subject) {
> +					print "\n" ;
> +					s/^    // ;
> +					print ;
> +				} elsif (/^\s+$/) { next ; }
> +				elsif (/^Author:/) { s/Author/From/ ; print ;}
> +				elsif (/^(From|Date)/) { print ; }

Where does "^From" come from? Should this be /^Date: / instead?

Show 8 quoted lines

> +				elsif (/^commit/) { next ; }
> +				else {
> +					s/^    // ;
> +					$subjtext = $_;
> +					$subject = 1;
> +				}
> +			' < "$patch" > "$dotest/$msgnum" || clean_abort
> +		done

This reminds me of another reason why I am hesitant to make "am" take output from "git show". Unlike format-patch output, "Author:" and "Date:" in its output, because it is meant for human consumption, might be a fair game for i18n/l10n (the first line "commit [0-9][a-f]{40}" is not likely to change, though).

Show 7 quoted lines

> +		echo "$this" > "$dotest/last"
> +		this=
> +		msgnum=
> +		;;
>  	hg)
>  		this=0
>  		for hg in "$@"