[PATCH] Handle "git show" output correctly.

[PATCH] Handle "git show" output correctly.

[Peter Jones]

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 git-am.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/git-am.sh b/git-am.sh
index c682d34..ebcbff7 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
+                        ;;
 		"From "* | "From: "*)
 			patch_format=mbox
 			;;
@@ -321,6 +336,37 @@ split_patches () {
 		this=
 		msgnum=
 		;;
+        gitshow)
+		this=0
+		for stgit in "$@"
+		do
+			this=`expr "$this" + 1`
+			msgnum=`printf "%0${prec}d" $this`
+			# Perl version of The first nonemptyline after an
+                        # empty line is the subject, and the body starts with
+                        # the next nonempty line.
+			perl -ne 'BEGIN { $subject = 0 }
+				if ($subject > 1) { print ; }
+				elsif (/^\s+$/) { next ; }
+				elsif (/^Author:/) { s/Author/From/ ; print ;}
+				elsif (/^(From|Date)/) { print ; }
+                                elsif (/^commit/) { next ; }
+				elsif ($subject) {
+					$subject = 2 ;
+					print "\n" ;
+                                        s/^    // ;
+					print ;
+				} else {
+					print "Subject: ", $_ ;
+					$subject = 1;
+				}
+			' < "$stgit" > "$dotest/$msgnum" || clean_abort
+		done
+		echo "$this" > "$dotest/last"
+		this=
+		msgnum=
+		;;
+
 	hg)
 		this=0
 		for hg in "$@"
-- 
1.7.11.4

[PATCH] Handle "git show" output correctly.

[Peter Jones]

(this version with fixed tabs)

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 git-am.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/git-am.sh b/git-am.sh
index c682d34..4a1a768 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
+			;;
 		"From "* | "From: "*)
 			patch_format=mbox
 			;;
@@ -321,6 +336,36 @@ split_patches () {
 		this=
 		msgnum=
 		;;
+	gitshow)
+		this=0
+		for stgit in "$@"
+		do
+			this=`expr "$this" + 1`
+			msgnum=`printf "%0${prec}d" $this`
+			# Perl version of The first nonemptyline after an
+			# empty line is the subject, and the body starts with
+			# the next nonempty line.
+			perl -ne 'BEGIN { $subject = 0 }
+				if ($subject > 1) { print ; }
+				elsif (/^\s+$/) { next ; }
+				elsif (/^Author:/) { s/Author/From/ ; print ;}
+				elsif (/^(From|Date)/) { print ; }
+				elsif (/^commit/) { next ; }
+				elsif ($subject) {
+					$subject = 2 ;
+					print "\n" ;
+					s/^    // ;
+					print ;
+				} else {
+					print "Subject: ", $_ ;
+					$subject = 1;
+				}
+			' < "$stgit" > "$dotest/$msgnum" || clean_abort
+		done
+		echo "$this" > "$dotest/last"
+		this=
+		msgnum=
+		;;
 	hg)
 		this=0
 		for hg in "$@"
-- 
1.7.11.4

Re: [PATCH] Handle "git show" output correctly.

[Matthieu Moy]

> Subject: Re: [PATCH] Handle "git show" output correctly.

No final period please.

This does not say which part of git is made to handle "git show". What
about

[PATCH] am: handle "git show" output correctly

Peter Jones <pjones@redhat.com> writes:

This lacks a proper commit message, i.e. an answer to the "why is this
change good?" question.

> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
>  git-am.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++

Documentation?

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

Your code is indented with space, Git indents with tabs. Please fix this
in your next version.

>  			patch_format=mbox
>  			;;
> @@ -321,6 +336,37 @@ split_patches () {
>  		this=
>  		msgnum=
>  		;;
> +        gitshow)
> +		this=0
> +		for stgit in "$@"

Probably a cut-and-paste from the stgit version, but your variable
naming doesn't make sense here.

> +		do
> +			this=`expr "$this" + 1`
> +			msgnum=`printf "%0${prec}d" $this`
> +			# Perl version of The first nonemptyline after an

Wrong cut-and-paste again, the sentense doesn't parse.

> +                        # empty line is the subject, and the body starts with
> +                        # the next nonempty line.
> +			perl -ne 'BEGIN { $subject = 0 }
> +				if ($subject > 1) { print ; }
> +				elsif (/^\s+$/) { next ; }
> +				elsif (/^Author:/) { s/Author/From/ ; print ;}
> +				elsif (/^(From|Date)/) { print ; }
> +                                elsif (/^commit/) { next ; }
> +				elsif ($subject) {
> +					$subject = 2 ;
> +					print "\n" ;
> +                                        s/^    // ;
> +					print ;
> +				} else {
> +					print "Subject: ", $_ ;
> +					$subject = 1;
> +				}

How does this react to multi-line subject, e.g

This should be the
subject line.

And this is the body.

?

git format-patch will merge the lines in a single Subject: header, and
your version seems to take only the first line.

A test showing this would be welcome.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] Handle "git show" output correctly.

[Matthieu Moy]

Peter Jones <pjones@redhat.com> writes:

> (this version with fixed tabs)

This will end up being the commit message. If you add text here, then
the maintainer will have to manually fix it when applying (or reject
your patch). Please, be nice with him and put your comments below the
--- :

> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---

(ie. here)

>  git-am.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH] Handle "git show" output correctly.

[Peter Jones]

(this version with fixed tabs and the comment fixed to be actual English)

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 git-am.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/git-am.sh b/git-am.sh
index c682d34..cfd7b09 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
+			;;
 		"From "* | "From: "*)
 			patch_format=mbox
 			;;
@@ -321,6 +336,36 @@ split_patches () {
 		this=
 		msgnum=
 		;;
+	gitshow)
+		this=0
+		for stgit in "$@"
+		do
+			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 { $subject = 0 }
+				if ($subject > 1) { print ; }
+				elsif (/^\s+$/) { next ; }
+				elsif (/^Author:/) { s/Author/From/ ; print ;}
+				elsif (/^(From|Date)/) { print ; }
+				elsif (/^commit/) { next ; }
+				elsif ($subject) {
+					$subject = 2 ;
+					print "\n" ;
+					s/^    // ;
+					print ;
+				} else {
+					print "Subject: ", $_ ;
+					$subject = 1;
+				}
+			' < "$stgit" > "$dotest/$msgnum" || clean_abort
+		done
+		echo "$this" > "$dotest/last"
+		this=
+		msgnum=
+		;;
 	hg)
 		this=0
 		for hg in "$@"
-- 
1.7.11.4

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

[Peter Jones]

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 | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/git-am.sh b/git-am.sh
index c682d34..cfd7b09 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
+			;;
 		"From "* | "From: "*)
 			patch_format=mbox
 			;;
@@ -321,6 +336,36 @@ split_patches () {
 		this=
 		msgnum=
 		;;
+	gitshow)
+		this=0
+		for stgit in "$@"
+		do
+			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 { $subject = 0 }
+				if ($subject > 1) { print ; }
+				elsif (/^\s+$/) { next ; }
+				elsif (/^Author:/) { s/Author/From/ ; print ;}
+				elsif (/^(From|Date)/) { print ; }
+				elsif (/^commit/) { next ; }
+				elsif ($subject) {
+					$subject = 2 ;
+					print "\n" ;
+					s/^    // ;
+					print ;
+				} else {
+					print "Subject: ", $_ ;
+					$subject = 1;
+				}
+			' < "$stgit" > "$dotest/$msgnum" || clean_abort
+		done
+		echo "$this" > "$dotest/last"
+		this=
+		msgnum=
+		;;
 	hg)
 		this=0
 		for hg in "$@"
-- 
1.7.11.4

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

[Matthieu Moy]

Peter Jones <pjones@redhat.com> writes:

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

The convention in Git is "<subsystem>: <summary of change>" (i.e. no
brackets around git-am, just am: and no capital for Handle).

My other concerns (name of stgit, multi-lines subject lines and lack of
documentation) still hold.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Peter Jones <pjones@redhat.com> writes:
>
>> Subject: [PATCH] [git-am] Handle "git show" output correctly
>
> The convention in Git is "<subsystem>: <summary of change>" (i.e. no
> brackets around git-am, just am: and no capital for Handle).
>
> My other concerns (name of stgit, multi-lines subject lines and lack of
> documentation) still hold.

We do not want to apply "git show" output that munges the log
message, period.

If you want to give patches to somebody (or to yourself) via e-mail
or via sneaker-net, "git format-patch" is there for you.  Do not
butcher "am" to accept a format that is not meant for patch
transport in the first place.

If you want to screw something in to your shelf, you would use a
screw and a screwdriver.  You do not try to hammer a nail using your
screwdriver, find that the screwdriver is not very useful as a
hammer and modify the screwdriver to hit your nail.

Re: [PATCH] Handle "git show" output correctly.

[Peter Jones]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

On Wed, 2012-09-12 at 17:40 +0200, Matthieu Moy wrote:
> 
> How does this react to multi-line subject, e.g
> 
> This should be the
> subject line.
> 
> And this is the body.
> 
> ?
> 
> git format-patch will merge the lines in a single Subject: header, and
> your version seems to take only the first line.
> 
> A test showing this would be welcome. 

An updated patch to fix this will be my next mail.  It's not as succinct
as it once was, but such is life.

The two attached commits to this message can be used as a test case.
Basically, do (in any repo)

git am 0001* 0002*
git show > foo.patch
git reset HEAD^ --hard
git am foo.patch
git show # check the output here
git format-patch -1
# check 0001-bar-bar-bar-this-is-a-very-very-long-line-I-am-tired.patch
# here.

It winds up merging the subject lines before the rest of git-am does - I
couldn't get it to work if I preserved the newline; for some reason I
always get a second newline and that's /more/ wrong.

-- 
  Peter

[-- Attachment #2: 0001-this-is-an-example-with-a-very-long-subject-line-whi.patch --]
[-- Type: text/x-patch, Size: 514 bytes --]

>From f7521f88731f9fc696dcd8e32de58cc9d98ed892 Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Wed, 12 Sep 2012 13:17:11 -0400
Subject: [PATCH 1/2] this is an example with a very long subject line which
 is completely unreasonable and nevertheless a thing.

It also has other stuff here.
---
 foo | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 foo

diff --git a/foo b/foo
new file mode 100644
index 0000000..fa5ef85
--- /dev/null
+++ b/foo
@@ -0,0 +1 @@
+    za za za
-- 
1.7.11.4


[-- Attachment #3: 0002-bar-bar-bar-this-is-a-very-very-long-line-I-am-tired.patch --]
[-- Type: text/x-patch, Size: 506 bytes --]

>From bc471f2b89ada6e6ddf35b5ec2538242b5639836 Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Wed, 12 Sep 2012 13:39:56 -0400
Subject: [PATCH 2/2] bar bar bar this is a very very long line I am tired of
 this game and it is quite annoying.

this is really annoying.  I hate perl.
    zonk.

yes.
    no.
---
 foo | 1 +
 1 file changed, 1 insertion(+)

diff --git a/foo b/foo
index fa5ef85..3b572f4 100644
--- a/foo
+++ b/foo
@@ -1 +1,2 @@
     za za za
+    bar bar bar
-- 
1.7.11.4

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

[Peter Jones]

On Wed, 2012-09-12 at 10:32 -0700, Junio C Hamano wrote:
> We do not want to apply "git show" output that munges the log
> message, period.
> 
> If you want to give patches to somebody (or to yourself) via e-mail
> or via sneaker-net, "git format-patch" is there for you.  Do not
> butcher "am" to accept a format that is not meant for patch
> transport in the first place.
> 
> If you want to screw something in to your shelf, you would use a
> screw and a screwdriver.  You do not try to hammer a nail using your
> screwdriver, find that the screwdriver is not very useful as a
> hammer and modify the screwdriver to hit your nail.

That seems to be completely missing the point - people /send/ them
without knowing, and as a maintainer of several projects, it's /hostile/
to people who are trying to help by sending patches to go around in
circles with them about the fact that they typed the wrong command. I'd
rather just take the patch, but right now the tools won't let me, and
for completely arbitrary reasons.

Let me put it a different way - if you won't accept git-am handling "git
show" output because "git show" has output that wasn't designed to be
parsed ever, would you be opposed to a patch that switches the "git
show" output to be something usable?

-- 
  Peter

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

[Peter Jones]

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
+			;;
 		"From "* | "From: "*)
 			patch_format=mbox
 			;;
@@ -321,6 +336,51 @@ split_patches () {
 		this=
 		msgnum=
 		;;
+	gitshow)
+		this=0
+		for patch in "$@"
+		do
+			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 ; }
+				elsif (/^commit/) { next ; }
+				else {
+					s/^    // ;
+					$subjtext = $_;
+					$subject = 1;
+				}
+			' < "$patch" > "$dotest/$msgnum" || clean_abort
+		done
+		echo "$this" > "$dotest/last"
+		this=
+		msgnum=
+		;;
 	hg)
 		this=0
 		for hg in "$@"
-- 
1.7.11.4

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

[Junio C Hamano]

Peter Jones <pjones@redhat.com> writes:

> Let me put it a different way - if you won't accept git-am handling "git
> show" output because "git show" has output that wasn't designed to be
> parsed ever, would you be opposed to a patch that switches the "git
> show" output to be something usable?

The output from the command is optimized for humans, but you could
invoke "git show --pretty=email" if you want to, so I do not think
you need any patch to do that.

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

[Junio C Hamano]

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

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

> +			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 "$@"

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

[Peter Jones]

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

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

[Peter Jones]

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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/git-am.sh b/git-am.sh
index c682d34..d20f249 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -216,6 +216,18 @@ check_patch_format () {
 		read l2
 		read l3
 		case "$l1" in
+		"commit "*)
+			case "$l2,,$l3" in
+			"Author: "*,,"Date: "*)
+				if expr "$l1" : 'commit [0-9a-f]\{40\}$' \
+						>/dev/null ; then
+					patch_format=gitshow
+				fi
+				;;
+			*)
+				;;
+			esac
+			;;
 		"From "* | "From: "*)
 			patch_format=mbox
 			;;
@@ -321,6 +333,51 @@ split_patches () {
 		this=
 		msgnum=
 		;;
+	gitshow)
+		this=0
+		for patch in "$@"
+		do
+			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 (/^Date:/) { print ; }
+				elsif (/^commit/) { next ; }
+				else {
+					s/^    // ;
+					$subjtext = $_;
+					$subject = 1;
+				}
+			' < "$patch" > "$dotest/$msgnum" || clean_abort
+		done
+		echo "$this" > "$dotest/last"
+		this=
+		msgnum=
+		;;
 	hg)
 		this=0
 		for hg in "$@"
-- 
1.7.11.4

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

[Junio C Hamano]

Peter Jones <pjones@redhat.com> writes:

> Well, if that happens, maybe we could regexp match on
> "[[:alnum:]_-]+: /someexprthatlookslikeanemailaddress/" ?

I doubt that would be even reliably done.

> But we could
> also just wait to cross that bridge until we get to it?

Not really.  If we start encouraging people to use "git show" output
as a kosher input to "am", we would have to support such use
forever, and we end up painting ourselves in a corner we cannot get
out of easily.

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

[Dan Johnson]

On Wed, Sep 12, 2012 at 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Peter Jones <pjones@redhat.com> writes:
>
>> Well, if that happens, maybe we could regexp match on
>> "[[:alnum:]_-]+: /someexprthatlookslikeanemailaddress/" ?
>
> I doubt that would be even reliably done.
>
>> But we could
>> also just wait to cross that bridge until we get to it?
>
> Not really.  If we start encouraging people to use "git show" output
> as a kosher input to "am", we would have to support such use
> forever, and we end up painting ourselves in a corner we cannot get
> out of easily.

If git am emitted a warning when accepting "git show" output, it seems
like it would support Peter's use-case without encouraging bad
behavior?

-- 
-Dan

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

[Junio C Hamano]

Dan Johnson <computerdruid@gmail.com> writes:

>> Not really.  If we start encouraging people to use "git show" output
>> as a kosher input to "am", we would have to support such use
>> forever, and we end up painting ourselves in a corner we cannot get
>> out of easily.
>
> If git am emitted a warning when accepting "git show" output, it seems
> like it would support Peter's use-case without encouraging bad
> behavior?

Are you seriously suggesting me to sell to our users a new feature
saying "this does not work reliably, we would not recommend using
it, no, really, don't trust it." from the day the feature is
introduced, especially when we know it will not be "the feature does
not work well yet, but it will, we promise" but is "and it may become
worse in the future"?

I do not see much point in doing that.

Besides, what bad behaviour do we avoid from encouraging with such
an approach?  As Peter said, the problem is not on the part of the
user who ended up with an output from "git show", when he really
wants output from "git format-patch".  Giving the warning to the
user of "git am" is too late.

I may be able to be pursuaded to swallow a new script somewhere in
the contrib/ hierarchy that takes a "git show" output and formats it
to look like "format-patch" output to be fed to "git am".  That way,
when a user has trouble with its parsing of "git show" output, at
least we can ask for the output of the format massaging step to help
us diagnose where the problem lies.

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

[Dan Johnson]

On Wed, Sep 12, 2012 at 6:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dan Johnson <computerdruid@gmail.com> writes:
>
>>> Not really.  If we start encouraging people to use "git show" output
>>> as a kosher input to "am", we would have to support such use
>>> forever, and we end up painting ourselves in a corner we cannot get
>>> out of easily.
>>
>> If git am emitted a warning when accepting "git show" output, it seems
>> like it would support Peter's use-case without encouraging bad
>> behavior?
>
> Are you seriously suggesting me to sell to our users a new feature
> saying "this does not work reliably, we would not recommend using
> it, no, really, don't trust it." from the day the feature is
> introduced, especially when we know it will not be "the feature does
> not work well yet, but it will, we promise" but is "and it may become
> worse in the future"?
>
> I do not see much point in doing that.
Fair enough.

> Besides, what bad behaviour do we avoid from encouraging with such
> an approach?  As Peter said, the problem is not on the part of the
> user who ended up with an output from "git show", when he really
> wants output from "git format-patch".  Giving the warning to the
> user of "git am" is too late.
I was assuming Peter would accept the patch, and reply with a "in the
future, please submit the output of format-patch", thus correcting the
submitter's behavior. This warning would serve someone who did not
know that they wanted the output of format-patch, and hopefully teach
them to send such a reply message.

> I may be able to be pursuaded to swallow a new script somewhere in
> the contrib/ hierarchy that takes a "git show" output and formats it
> to look like "format-patch" output to be fed to "git am".  That way,
> when a user has trouble with its parsing of "git show" output, at
> least we can ask for the output of the format massaging step to help
> us diagnose where the problem lies.

That sounds like a better approach to me as well.

-- 
-Dan

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

[Andreas Ericsson]

On 09/13/2012 12:19 AM, Junio C Hamano wrote:
> Dan Johnson <computerdruid@gmail.com> writes:
> 
>>> Not really.  If we start encouraging people to use "git show" output
>>> as a kosher input to "am", we would have to support such use
>>> forever, and we end up painting ourselves in a corner we cannot get
>>> out of easily.
>>
>> If git am emitted a warning when accepting "git show" output, it seems
>> like it would support Peter's use-case without encouraging bad
>> behavior?
> 
> Are you seriously suggesting me to sell to our users a new feature
> saying "this does not work reliably, we would not recommend using
> it, no, really, don't trust it." from the day the feature is
> introduced, especially when we know it will not be "the feature does
> not work well yet, but it will, we promise" but is "and it may become
> worse in the future"?
> 
> I do not see much point in doing that.
> 
> Besides, what bad behaviour do we avoid from encouraging with such
> an approach?  As Peter said, the problem is not on the part of the
> user who ended up with an output from "git show", when he really
> wants output from "git format-patch".  Giving the warning to the
> user of "git am" is too late.
> 

It might be enough to either enable format-patch output or print a
warning to stderr when stdout is not a tty. I believe that would at
least mitigate the problem, and it might educate the user as well.
We already modify output format when stdout is not a tty (removing
colors), so we're not giving guarantees about its format when it's
piped somewhere. I believe that would provide almost every scenario
with the expected outcome (including 'git show | grep'), but there
will be a handful of very surprised people as well.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

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

[Junio C Hamano]

Dan Johnson <computerdruid@gmail.com> writes:

> I was assuming Peter would accept the patch, and reply with a "in the
> future, please submit the output of format-patch", thus correcting the
> submitter's behavior. This warning would serve someone who did not
> know that they wanted the output of format-patch, and hopefully teach
> them to send such a reply message.

"Next time, please do this" rarely has worked in practice.  This is
because the moment you accepted the current patch, you have already
lost the "carrot" ;-)