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

[Peter Jones <pjones@redhat.com>]

On Wed, 2012-09-12 at 13:06 -0700, Junio C Hamano wrote:
> Peter Jones <pjones@redhat.com> 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@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

Yeah, I can do that.

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

I hadn't really even considered it - are there other formats that use
commit and author which git-am.sh is supposed to support? It seems as
though if we get something wrong you'll wind up in clean_abort at some
point anyway.  At worst your patch will still be there and you'll need
to do "git am --abort".

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

Yeah, that sounds like madness.

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

Right.

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

Entirely copy-paste error on my part.  I'll fix it for the next version.

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

Well, if that happens, maybe we could regexp match on
"[[:alnum:]_-]+: /someexprthatlookslikeanemailaddress/" ?  But we could
also just wait to cross that bridge until we get to it?  Even if that
does get translated later, the current patch would continue to work for
English, so even in that case it's not totally worthless.

-- 
  Peter