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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Peter Jones <pjones@xxxxxxxxxx> writes:

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

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

> +			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?

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

> +		echo "$this" > "$dotest/last"
> +		this=
> +		msgnum=
> +		;;
>  	hg)
>  		this=0
>  		for hg in "$@"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]