[RFC PATCH 0/1] am: bug report and new patch format support

[RFC PATCH 0/1] am: bug report and new patch format support

[Chris Packham]

Hi List,

When I first tried to apply a patch someone gave me I got the following
errors.

  $ git am patch.patch
  tr: write error: Broken pipe
  tr: write error
  Patch format detection failed.

The patch itself was generated from gitk and while I'd seen the format
detection problem before but the tr errors were new to me. I haven't
looked at the tr problem yet but the gitk thing had been bugging me for
a while so here's a patch that understands the format generated by gitk.

 Documentation/git-am.txt |    3 ++-
 git-am.sh                |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

Chris Packham (1):
      am: add gitk patch format

[RFC PATCH 1/1] am: add gitk patch format

[Chris Packham]

Patches created using gitk's "write commit to file" functionality (which
uses 'git diff-tree -p --pretty' under the hood) need some massaging in
order to apply cleanly. This consists of dropping the 'commit' line
automatically determining the subject and removing leading whitespace.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
 Documentation/git-am.txt |    3 ++-
 git-am.sh                |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 9adce37..b59d2b3 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -101,7 +101,8 @@ default.   You can use `--no-utf8` to override this.
 	By default the command will try to detect the patch format
 	automatically. This option allows the user to bypass the automatic
 	detection and specify the patch format that the patch(es) should be
-	interpreted as. Valid formats are mbox, stgit, stgit-series and hg.
+	interpreted as. Valid formats are mbox, stgit, stgit-series, hg and
+	gitk.
 
 -i::
 --interactive::
diff --git a/git-am.sh b/git-am.sh
index ee61a77..73b0a86 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -227,6 +227,9 @@ check_patch_format () {
 		"# HG changeset patch")
 			patch_format=hg
 			;;
+		'commit '*)
+			patch_format=gitk
+			;;
 		*)
 			# if the second line is empty and the third is
 			# a From, Author or Date entry, this is very
@@ -357,6 +360,38 @@ split_patches () {
 		this=
 		msgnum=
 		;;
+	gitk)
+		# These patches are generates with 'git diff-tree -p --pretty'
+		# we discard the 'commit' line, after that the first line not
+		# starting with 'Author:' or 'Date:' is the subject. We also
+		# need to strip leading whitespace from the message body.
+		this=0
+		for gitk in "$@"
+		do
+			this=$(expr "$this" + 1)
+			msgnum=$(printf "%0${prec}d" $this)
+			@@PERL@@ -ne 'BEGIN { $subject = 0 }
+				s/^    // ;
+				if ($subject > 1) { print ; }
+				elsif (/^commit\s.*$/) { next ; }
+				elsif (/^\s+$/) { next ; }
+				elsif (/^Author:/) { s/Author/From/ ; print ;}
+				elsif (/^Date:/) { print ;}
+				elsif ($subject) {
+					$subject = 2 ;
+					print "\n" ;
+					print ;
+				} else {
+					print "Subject: ", $_ ;
+					$subject = 1;
+				}
+			' <"$gitk" >"$dotest/$msgnum" || clean_abort
+
+		done
+		echo "$this" >"$dotest/last"
+		this=
+		msgnum=
+		;;
 	*)
 		if test -n "$patch_format"
 		then
-- 
1.7.9.5

Re: [RFC PATCH 1/1] am: add gitk patch format

[Chris Packham]

Reviewing my own code.

On Wed, Sep 3, 2014 at 9:35 PM, Chris Packham <judge.packham@gmail.com> wrote:
> Patches created using gitk's "write commit to file" functionality (which
> uses 'git diff-tree -p --pretty' under the hood) need some massaging in
> order to apply cleanly. This consists of dropping the 'commit' line
> automatically determining the subject and removing leading whitespace.
>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---
>  Documentation/git-am.txt |    3 ++-
>  git-am.sh                |   35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index 9adce37..b59d2b3 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -101,7 +101,8 @@ default.   You can use `--no-utf8` to override this.
>         By default the command will try to detect the patch format
>         automatically. This option allows the user to bypass the automatic
>         detection and specify the patch format that the patch(es) should be
> -       interpreted as. Valid formats are mbox, stgit, stgit-series and hg.
> +       interpreted as. Valid formats are mbox, stgit, stgit-series, hg and
> +       gitk.
>
>  -i::
>  --interactive::
> diff --git a/git-am.sh b/git-am.sh
> index ee61a77..73b0a86 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -227,6 +227,9 @@ check_patch_format () {
>                 "# HG changeset patch")
>                         patch_format=hg
>                         ;;
> +               'commit '*)
> +                       patch_format=gitk
> +                       ;;
>                 *)
>                         # if the second line is empty and the third is
>                         # a From, Author or Date entry, this is very
> @@ -357,6 +360,38 @@ split_patches () {
>                 this=
>                 msgnum=
>                 ;;
> +       gitk)
> +               # These patches are generates with 'git diff-tree -p --pretty'
> +               # we discard the 'commit' line, after that the first line not
> +               # starting with 'Author:' or 'Date:' is the subject. We also
> +               # need to strip leading whitespace from the message body.
> +               this=0
> +               for gitk in "$@"
> +               do
> +                       this=$(expr "$this" + 1)
> +                       msgnum=$(printf "%0${prec}d" $this)
> +                       @@PERL@@ -ne 'BEGIN { $subject = 0 }
> +                               s/^    // ;

This is a little too aggressive. It'll also chomp whitespace from the
diff context. We should probably check for the 'diff --git' line and
stop stripping whitespace.

> +                               if ($subject > 1) { print ; }
> +                               elsif (/^commit\s.*$/) { next ; }
> +                               elsif (/^\s+$/) { next ; }
> +                               elsif (/^Author:/) { s/Author/From/ ; print ;}
> +                               elsif (/^Date:/) { print ;}
> +                               elsif ($subject) {
> +                                       $subject = 2 ;
> +                                       print "\n" ;
> +                                       print ;
> +                               } else {
> +                                       print "Subject: ", $_ ;
> +                                       $subject = 1;
> +                               }
> +                       ' <"$gitk" >"$dotest/$msgnum" || clean_abort
> +
> +               done
> +               echo "$this" >"$dotest/last"
> +               this=
> +               msgnum=
> +               ;;
>         *)
>                 if test -n "$patch_format"
>                 then
> --
> 1.7.9.5
>

Re: [RFC PATCH 0/1] am: bug report and new patch format support

[Chris Packham]

On Wed, Sep 3, 2014 at 9:35 PM, Chris Packham <judge.packham@gmail.com> wrote:
> Hi List,
>
> When I first tried to apply a patch someone gave me I got the following
> errors.
>
>   $ git am patch.patch
>   tr: write error: Broken pipe
>   tr: write error
>   Patch format detection failed.

I can't do this with every patch. It seems to be size related, if I
feed it a big enough patch it triggers the error.
Somewhere around 3000 lines triggers it for me. I think the cause is
the following construct in check_patch_format

  {
    ...
    tr -d '\015' <"$1" |
    ...
  } < "$1" || clean_abort

>
> The patch itself was generated from gitk and while I'd seen the format
> detection problem before but the tr errors were new to me. I haven't
> looked at the tr problem yet but the gitk thing had been bugging me for
> a while so here's a patch that understands the format generated by gitk.
>
>  Documentation/git-am.txt |    3 ++-
>  git-am.sh                |   35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> Chris Packham (1):
>       am: add gitk patch format
>
>

[RFC PATCHv2 0/2] am: bug fix and new patch format support

[Chris Packham]

I've done some digging into the tr error message. I think the extra
re-direction of stdin is unnecessary. I've removed it and the tests
still pass. The error message I was seeing is no longer seen but I've
found it hard to actually reproduce (even with different sized patches).

I've also updated 'am: avoid re-directing stdin twice' to add some tests
and only strip the description part of the patch.

Chris Packham (2):
  am: add gitk patch format
  am: avoid re-directing stdin twice

 Documentation/git-am.txt |  3 ++-
 git-am.sh                | 38 +++++++++++++++++++++++++++++++++++++-
 t/t4150-am.sh            | 13 +++++++++++++
 3 files changed, 52 insertions(+), 2 deletions(-)

-- 
2.0.4.2.gadd452d

[RFC PATCHv2 1/2] am: add gitk patch format

[Chris Packham]

Patches created using gitk's "write commit to file" functionality (which
uses 'git diff-tree -p --pretty' under the hood) need some massaging in
order to apply cleanly. This consists of dropping the 'commit' line
automatically determining the subject and removing leading whitespace.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
 Documentation/git-am.txt |  3 ++-
 git-am.sh                | 36 ++++++++++++++++++++++++++++++++++++
 t/t4150-am.sh            | 13 +++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 9adce37..b59d2b3 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -101,7 +101,8 @@ default.   You can use `--no-utf8` to override this.
 	By default the command will try to detect the patch format
 	automatically. This option allows the user to bypass the automatic
 	detection and specify the patch format that the patch(es) should be
-	interpreted as. Valid formats are mbox, stgit, stgit-series and hg.
+	interpreted as. Valid formats are mbox, stgit, stgit-series, hg and
+	gitk.
 
 -i::
 --interactive::
diff --git a/git-am.sh b/git-am.sh
index ee61a77..f979925 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -227,6 +227,9 @@ check_patch_format () {
 		"# HG changeset patch")
 			patch_format=hg
 			;;
+		'commit '*)
+			patch_format=gitk
+			;;
 		*)
 			# if the second line is empty and the third is
 			# a From, Author or Date entry, this is very
@@ -357,6 +360,39 @@ split_patches () {
 		this=
 		msgnum=
 		;;
+	gitk)
+		# These patches are generates with 'git diff-tree -p --pretty'
+		# we discard the 'commit' line, after that the first line not
+		# starting with 'Author:' or 'Date:' is the subject. We also
+		# need to strip leading whitespace from the message body.
+		this=0
+		for gitk in "$@"
+		do
+			this=$(expr "$this" + 1)
+			msgnum=$(printf "%0${prec}d" $this)
+			@@PERL@@ -ne 'BEGIN { $subject = 0; $diff = 0; }
+				if (!$diff) { s/^    // ; }
+				if ($subject > 1) { print ; }
+				elsif (/^commit\s.*$/) { next ; }
+				elsif (/^\s+$/) { next ; }
+				elsif (/^Author:/) { s/Author/From/ ; print ; }
+				elsif (/^Date:/) { print ;}
+				elsif (/^diff --git/) { $diff = 1 ; print ;}
+				elsif ($subject) {
+					$subject = 2 ;
+					print "\n" ;
+					print ;
+				} else {
+					print "Subject: ", $_ ;
+					$subject = 1;
+				}
+			' <"$gitk" >"$dotest/$msgnum" || clean_abort
+
+		done
+		echo "$this" >"$dotest/last"
+		this=
+		msgnum=
+		;;
 	*)
 		if test -n "$patch_format"
 		then
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 5edb79a..e36cd0b 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -103,6 +103,7 @@ test_expect_success setup '
 		echo "X-Fake-Field: Line Three" &&
 		git format-patch --stdout first | sed -e "1d"
 	} > patch1-ws.eml &&
+	git diff-tree -p --pretty second >patch1-gitk.eml &&
 
 	sed -n -e "3,\$p" msg >file &&
 	git add file &&
@@ -186,6 +187,18 @@ test_expect_success 'am applies patch e-mail with preceding whitespace' '
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
 '
 
+test_expect_success 'am applies patch generated by gitk' '
+	cat patch1-gitk.eml &&
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout first &&
+	git am patch1-gitk.eml &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code second &&
+	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
+'
+
 test_expect_success 'setup: new author and committer' '
 	GIT_AUTHOR_NAME="Another Thor" &&
 	GIT_AUTHOR_EMAIL="a.thor@example.com" &&
-- 
2.0.4.2.gadd452d

Re: [RFC PATCHv2 1/2] am: add gitk patch format

[Junio C Hamano]

Chris Packham <judge.packham@gmail.com> writes:

> Patches created using gitk's "write commit to file" functionality (which
> uses 'git diff-tree -p --pretty' under the hood) need some massaging in
> order to apply cleanly.

Shouldn't that output routine be the one to be corrected, then?  We
really do not need yet another format to express the same thing,
especially from the same suite of programs.

Re: [RFC PATCHv2 1/2] am: add gitk patch format

[Chris Packham]

On Thu, Sep 4, 2014 at 11:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Packham <judge.packham@gmail.com> writes:
>
>> Patches created using gitk's "write commit to file" functionality (which
>> uses 'git diff-tree -p --pretty' under the hood) need some massaging in
>> order to apply cleanly.
>
> Shouldn't that output routine be the one to be corrected, then?  We
> really do not need yet another format to express the same thing,
> especially from the same suite of programs.

That's an option. It shouldn't be too hard to make gitk use 'git
format-patch --stdout' instead. The problem for me is that it's easier
for me to update my git installation to get git am to accept the
current format than it is for me to ask the people generating these
patches to change their git/gitk installation to generate a different
format.

Another thing that I've since realised is that this 'gitk' format is
also what you've get from git show or git log -p. So this is actually
allowing (for better or worse) things like 'git show $sha1 | git am
--patch-format=gitk'[*1*]. That may mean that we should call the
format something else ("pretty" perhaps?) and note that this is what
gitk, git show and some incantations of git log generate.

--
[*1*] - Although I've just found a bug that affects the existing
--patch-format=hg|stgit where reading from stdin is not currently
supported. I'll send out a v3 of this series that includes some tests
for those a bit later.

Re: [RFC PATCHv2 1/2] am: add gitk patch format

[Junio C Hamano]

Chris Packham <judge.packham@gmail.com> writes:

> Another thing that I've since realised is that this 'gitk' format is
> also what you've get from git show or git log -p. So this is actually
> allowing (for better or worse) things like 'git show $sha1 | git am
> --patch-format=gitk'[*1*]. That may mean that we should call the
> format something else ("pretty" perhaps?) and note that this is what
> gitk, git show and some incantations of git log generate.

I would not call it "pretty", because "--pretty" is merely a
short-hand to "--pretty=<some format name>".

The output format indents the log message text by four spaces for
human reading to make it stand out from the patch text, and not
meant for machine consumption.  I doubt that a patchset that does
not update mailinfo and mailsplit to extract information and to undo
the indentation could be a right solution.  "am" itself should not
be mucking with the input files.

Re: [RFC PATCHv2 1/2] am: add gitk patch format

[Chris Packham]

Hi Junio,

On Fri, Sep 5, 2014 at 5:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Packham <judge.packham@gmail.com> writes:
>
>> Another thing that I've since realised is that this 'gitk' format is
>> also what you've get from git show or git log -p. So this is actually
>> allowing (for better or worse) things like 'git show $sha1 | git am
>> --patch-format=gitk'[*1*]. That may mean that we should call the
>> format something else ("pretty" perhaps?) and note that this is what
>> gitk, git show and some incantations of git log generate.
>
> I would not call it "pretty", because "--pretty" is merely a
> short-hand to "--pretty=<some format name>".
>
> The output format indents the log message text by four spaces for
> human reading to make it stand out from the patch text, and not
> meant for machine consumption.

Fair enough.

> I doubt that a patchset that does
> not update mailinfo and mailsplit to extract information and to undo
> the indentation could be a right solution.

I've read this sentence a couple of times and I can't understand it. I
get that you are against adding yet more special cases to 'git am' to
handle patches that weren't generated by 'git format-patch'. Are you
saying that this won't go in or that the solution should be
implemented differently.

> "am" itself should not
> be mucking with the input files.
>

At the very least we need to drop the first line and replace "Author"
with "From". Which would still leave the commit message indented.
Something like the following allows the patch to be applied

  sed -e '1d' -e 's/^Author:/From:/' <patch.patch | git am

But it'd be nice if am could do that for me to save my fingers some work.

Re: [RFC PATCHv2 1/2] am: add gitk patch format

[Chris Packham]

(added back git ml because I accidentally dropped the Cc when replying
to Junio).

On Fri, Sep 5, 2014 at 10:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> I doubt that a patchset that does
>>> not update mailinfo and mailsplit to extract information and to undo
>>> the indentation could be a right solution.
>>
>> I've read this sentence a couple of times and I can't understand it.
>
> "am" uses mailsplit on the input file to separate it into one or more
> input files, each of which represents a single change. On each of
> them, it uses mailinfo to extract the meta information (e.g.
> authorship), log message and the patch into separate files.

Except when --patch-format=hg|stgit is specified (or detected). In
these cases mailsplit is avoided.

> It does not make any sense for a support for a new input format that
> does not teach mailinfo how to handle that format. Transforming it
> into a pseudo e-mail format is not the way to go. If that approach were
> acceptable, that format conversion filter can be run completely outside
> "am" in the first place, no?

So teaching git mailinfo to do s/^    // (either when asked to or
using some heuristic) would be a better approach? I also think we
should accept "Author:" as an acceptable fallback if "From:" is not
present.

Re: [RFC PATCHv2 1/2] am: add gitk patch format

[Junio C Hamano]

Chris Packham <judge.packham@gmail.com> writes:

> So teaching git mailinfo to do s/^    // (either when asked to or
> using some heuristic) would be a better approach? I also think we
> should accept "Author:" as an acceptable fallback if "From:" is not
> present.

Not as "a fallback" in the sense that "Author:" should not be
treated any specially when "am" (which stands for "apply mail") is
operating on the patches in e-mails.  Whatever wants to convert the
output from "log --pretty" as if it came from "log --pretty=email"
would certainly need to flip "Author:" to "From:" (what should
happen when it sees "From:" in the input, though???), and whatgver
wannts to pick metainfo from "log --pretty" output like mailinfo
does for "log --pretty=email" output would certainly need to pick
the authorship from "Author:" (without paying any attention to
"From:" if one exists).

Re: [RFC PATCHv2 1/2] am: add gitk patch format

[Chris Packham]

On Sat, Sep 6, 2014 at 6:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Packham <judge.packham@gmail.com> writes:
>
>> So teaching git mailinfo to do s/^    // (either when asked to or
>> using some heuristic) would be a better approach? I also think we
>> should accept "Author:" as an acceptable fallback if "From:" is not
>> present.
>
> Not as "a fallback" in the sense that "Author:" should not be
> treated any specially when "am" (which stands for "apply mail") is\
> operating on the patches in e-mails.

I was proposing we avoid the "Patch does not have a valid e-mail
address." error when we can dwim and determine the email address from
"Author:". I originally was going to say "From:" should take
precedence but it would be another way to indicate that the true
author is not necessarily the person who sent the email.

> Whatever wants to convert the
> output from "log --pretty" as if it came from "log --pretty=email"
> would certainly need to flip "Author:" to "From:" (what should
> happen when it sees "From:" in the input, though???), and whatgver
> wannts to pick metainfo from "log --pretty" output like mailinfo
> does for "log --pretty=email" output would certainly need to pick
> the authorship from "Author:" (without paying any attention to
> "From:" if one exists).
>

Wow. I didn't know --pretty=email existed. Better yet it works for
diff-tree so gitk should probably be using that to produce something
that can be exported/imported easily.

I do wonder what the original use-case for "write commit to file" was.
Once it's been written to a file what is one supposed to do with it?
It's not something that 'git am' can consume (currently). Using 'git
apply' or 'patch' would lose the commit message plus you have to
manually stage/commit the changes.

[RFC PATCHv2 2/2] am: avoid re-directing stdin twice

[Chris Packham]

In check_patch_format we feed $1 to a block that attempts to determine
the patch format. Since we've already redirected $1 to stdin there is no
need to redirect it again when we invoke tr. This prevents the following
errors when invoking git am

  $ git am patch.patch
  tr: write error: Broken pipe
  tr: write error
  Patch format detection failed.

Cc: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
 git-am.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index f979925..5d69c89 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -253,7 +253,7 @@ check_patch_format () {
 			# discarding the indented remainder of folded lines,
 			# and see if it looks like that they all begin with the
 			# header field names...
-			tr -d '\015' <"$1" |
+			tr -d '\015' |
 			sed -n -e '/^$/q' -e '/^[ 	]/d' -e p |
 			sane_egrep -v '^[!-9;-~]+:' >/dev/null ||
 			patch_format=mbox
-- 
2.0.4.2.gadd452d