[PATCH 0/5] am: improve test coverage and touch up foreign patch parsing

[PATCH 0/5] am: improve test coverage and touch up foreign patch parsing

[Paul Tan]

git-am is able to parse StGit and mercurial patches. However, there are no
regression tests in the test suite for these patch formats, and there are some
small bugs as well:

* the mercurial and stgit patch parsers does not support reading from stdin

* the mercurial patch parser parsed the patch date wrongly and git-am is thus
  unable to reconstruct the exact commit.

Most patches are based on Chris' patch series[1], which I've credited accordingly.

[1] http://thread.gmane.org/gmane.comp.version-control.git/256502


Paul Tan (5):
  t4150: test applying StGit patch
  am: teach StGit patch parser how to read from stdin
  t4150: test applying StGit series
  am: use gmtime() to parse mercurial patch date
  am: teach mercurial patch parser how to read from stdin

 git-am.sh     | 15 ++++++-----
 t/t4150-am.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 6 deletions(-)

-- 
2.1.4

[PATCH 1/5] t4150: test applying StGit patch

[Paul Tan]

By default, an StGit patch separates the subject from the commit message
and headers as follows:

	$subject

	From: $author_name <$author_email>

	$message
	---
	$diffstats

We test git-am's ability to detect such a patch as an StGit patch, and
its ability to be able to extract the commit author, date and message
from such a patch.

Based-on-patch-by: Chris Packham <judge.packham@gmail.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t4150-am.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 306e6f3..0ead529 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -104,6 +104,18 @@ test_expect_success setup '
 		echo "X-Fake-Field: Line Three" &&
 		git format-patch --stdout first | sed -e "1d"
 	} > patch1-ws.eml &&
+	{
+		sed -ne "1p" msg &&
+		echo &&
+		echo "From: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" &&
+		echo "Date: $GIT_AUTHOR_DATE" &&
+		echo &&
+		sed -e "1,2d" msg &&
+		echo &&
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
+		echo "---" &&
+		git diff-tree --no-commit-id --stat -p second
+	} >patch1-stgit.eml &&
 
 	sed -n -e "3,\$p" msg >file &&
 	git add file &&
@@ -187,6 +199,16 @@ 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 stgit patch' '
+	rm -fr .git/rebase-apply &&
+	git checkout -f first &&
+	git am patch1-stgit.eml &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code second &&
+	test_cmp_rev second HEAD &&
+	test_cmp_rev second^ HEAD^
+'
+
 test_expect_success 'setup: new author and committer' '
 	GIT_AUTHOR_NAME="Another Thor" &&
 	GIT_AUTHOR_EMAIL="a.thor@example.com" &&
-- 
2.1.4

[PATCH 2/5] am: teach StGit patch parser how to read from stdin

[Paul Tan]

git-mailsplit, which splits mbox patches, will read the patch from stdin
when the filename is "-" or there are no files listed on the
command-line.

To be consistent with this behavior, teach the StGit patch parser to
read from stdin if the filename is "-" or no files are listed on the
command-line.

Based-on-patch-by: Chris Packham <judge.packham@gmail.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh     |  5 +++--
 t/t4150-am.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 761befb..83f2ea6 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -297,6 +297,7 @@ split_patches () {
 		;;
 	stgit)
 		this=0
+		test 0 -eq "$#" && set -- -
 		for stgit in "$@"
 		do
 			this=$(expr "$this" + 1)
@@ -305,7 +306,7 @@ split_patches () {
 			# not starting with Author, From or Date is the
 			# subject, and the body starts with the next nonempty
 			# line not starting with Author, From or Date
-			@@PERL@@ -ne 'BEGIN { $subject = 0 }
+			cat "$stgit" | @@PERL@@ -ne 'BEGIN { $subject = 0 }
 				if ($subject > 1) { print ; }
 				elsif (/^\s+$/) { next ; }
 				elsif (/^Author:/) { s/Author/From/ ; print ;}
@@ -318,7 +319,7 @@ split_patches () {
 					print "Subject: ", $_ ;
 					$subject = 1;
 				}
-			' < "$stgit" > "$dotest/$msgnum" || clean_abort
+			' >"$dotest/$msgnum" || clean_abort
 		done
 		echo "$this" > "$dotest/last"
 		this=
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 0ead529..51962e4 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -209,6 +209,16 @@ test_expect_success 'am applies stgit patch' '
 	test_cmp_rev second^ HEAD^
 '
 
+test_expect_success 'am --patch-format=stgit applies stgit patch' '
+	rm -fr .git/rebase-apply &&
+	git checkout -f first &&
+	git am --patch-format=stgit <patch1-stgit.eml &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code second &&
+	test_cmp_rev second HEAD &&
+	test_cmp_rev second^ HEAD^
+'
+
 test_expect_success 'setup: new author and committer' '
 	GIT_AUTHOR_NAME="Another Thor" &&
 	GIT_AUTHOR_EMAIL="a.thor@example.com" &&
-- 
2.1.4

[PATCH 3/5] t4150: test applying StGit series

[Paul Tan]

A StGit series is a directory containing a "series" file which begins
with the line:

	# This series applies on GIT commit XXXXX

where XXXXX is the commit ID that the patch series applies on. Every
following line names a patch in the directory to be applied.

Test that git-am, when given this "series" file, is able to detect it as
an StGit series and apply all the patches in the series.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t4150-am.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 51962e4..7aad8f8 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -116,6 +116,13 @@ test_expect_success setup '
 		echo "---" &&
 		git diff-tree --no-commit-id --stat -p second
 	} >patch1-stgit.eml &&
+	mkdir stgit-series &&
+	cp patch1-stgit.eml stgit-series/patch &&
+	{
+		echo "# This series applies on GIT commit $(git rev-parse first)" &&
+		echo "patch"
+	} >stgit-series/series &&
+
 
 	sed -n -e "3,\$p" msg >file &&
 	git add file &&
@@ -219,6 +226,16 @@ test_expect_success 'am --patch-format=stgit applies stgit patch' '
 	test_cmp_rev second^ HEAD^
 '
 
+test_expect_success 'am applies stgit series' '
+	rm -fr .git/rebase-apply &&
+	git checkout -f first &&
+	git am stgit-series/series &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code second &&
+	test_cmp_rev second HEAD &&
+	test_cmp_rev second^ HEAD^
+'
+
 test_expect_success 'setup: new author and committer' '
 	GIT_AUTHOR_NAME="Another Thor" &&
 	GIT_AUTHOR_EMAIL="a.thor@example.com" &&
-- 
2.1.4

[PATCH 4/5] am: use gmtime() to parse mercurial patch date

[Paul Tan]

An example of the line in a mercurial patch that specifies the date of
the commit would be:

	# Date 1433753301 25200

where the first number is the number of seconds since the unix epoch (in
UTC), and the second number is the offset of the timezone, in second s
west of UTC (negative if the timezone is east of UTC).

git-am uses localtime() to break down the first number into its
components (year, month, day, hours, minutes, seconds etc.). However,
the returned components are relative to the user's time zone. As a
result, if the user's time zone does not match the time zone specified
in the patch, the resulting commit will have the wrong author date.

Fix this by using gmtime() instead, which uses UTC instead of the user's
time zone.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh     |  6 +++---
 t/t4150-am.sh | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 83f2ea6..d97da85 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -343,11 +343,11 @@ split_patches () {
 				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
 				elsif (/^\# Date /) {
 					my ($hashsign, $str, $time, $tz) = split ;
-					$tz = sprintf "%+05d", (0-$tz)/36;
+					$tz_str = sprintf "%+05d", (0-$tz)/36;
 					print "Date: " .
 					      strftime("%a, %d %b %Y %H:%M:%S ",
-						       localtime($time))
-					      . "$tz\n";
+						       gmtime($time-$tz))
+					      . "$tz_str\n";
 				} elsif (/^\# /) { next ; }
 				else {
 					print "\n", $_ ;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 7aad8f8..4beb4b3 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -122,6 +122,19 @@ test_expect_success setup '
 		echo "# This series applies on GIT commit $(git rev-parse first)" &&
 		echo "patch"
 	} >stgit-series/series &&
+	{
+		echo "# HG changeset patch" &&
+		echo "# User $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" &&
+		echo "# Date $test_tick 25200" &&
+		echo "#      $(git show --pretty="%aD" -s second)" &&
+		echo "# Node ID $_z40" &&
+		echo "# Parent  $_z40" &&
+		cat msg &&
+		echo &&
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
+		echo &&
+		git diff-tree --no-commit-id -p second
+	} >patch1-hg.eml &&
 
 
 	sed -n -e "3,\$p" msg >file &&
@@ -236,6 +249,16 @@ test_expect_success 'am applies stgit series' '
 	test_cmp_rev second^ HEAD^
 '
 
+test_expect_success 'am applies hg patch' '
+	rm -fr .git/rebase-apply &&
+	git checkout -f first &&
+	git am patch1-hg.eml &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code second &&
+	test_cmp_rev second HEAD &&
+	test_cmp_rev second^ HEAD^
+'
+
 test_expect_success 'setup: new author and committer' '
 	GIT_AUTHOR_NAME="Another Thor" &&
 	GIT_AUTHOR_EMAIL="a.thor@example.com" &&
-- 
2.1.4

[PATCH 5/5] am: teach mercurial patch parser how to read from stdin

[Paul Tan]

git-mailsplit, which splits mbox patches, will read the patch from stdin
when the filename is "-" or there are no files listed on the
command-line.

To be consistent with this behavior, teach the mercurial patch parser to
read from stdin if the filename is "-" or no files are listed on the
command-line.

Based-on-patch-by: Chris Packham <judge.packham@gmail.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh     |  4 +++-
 t/t4150-am.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index d97da85..0a40d46 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -327,6 +327,7 @@ split_patches () {
 		;;
 	hg)
 		this=0
+		test 0 -eq "$#" && set -- -
 		for hg in "$@"
 		do
 			this=$(( $this + 1 ))
@@ -338,6 +339,7 @@ split_patches () {
 			# Since we cannot guarantee that the commit message is in
 			# git-friendly format, we put no Subject: line and just consume
 			# all of the message as the body
+			cat "$hg" |
 			LANG=C LC_ALL=C @@PERL@@ -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
 				if ($subject) { print ; }
 				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
@@ -353,7 +355,7 @@ split_patches () {
 					print "\n", $_ ;
 					$subject = 1;
 				}
-			' <"$hg" >"$dotest/$msgnum" || clean_abort
+			' >"$dotest/$msgnum" || clean_abort
 		done
 		echo "$this" >"$dotest/last"
 		this=
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 4beb4b3..3ebafd9 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -259,6 +259,16 @@ test_expect_success 'am applies hg patch' '
 	test_cmp_rev second^ HEAD^
 '
 
+test_expect_success 'am --patch-format=hg applies hg patch' '
+	rm -fr .git/rebase-apply &&
+	git checkout -f first &&
+	git am --patch-format=hg <patch1-hg.eml &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code second &&
+	test_cmp_rev second HEAD &&
+	test_cmp_rev second^ HEAD^
+'
+
 test_expect_success 'setup: new author and committer' '
 	GIT_AUTHOR_NAME="Another Thor" &&
 	GIT_AUTHOR_EMAIL="a.thor@example.com" &&
-- 
2.1.4

Re: [PATCH 5/5] am: teach mercurial patch parser how to read from stdin

[Stefan Beller]

On Mon, Jun 8, 2015 at 8:48 AM, Paul Tan <pyokagan@gmail.com> wrote:
> git-mailsplit, which splits mbox patches, will read the patch from stdin
> when the filename is "-" or there are no files listed on the
> command-line.
>
> To be consistent with this behavior, teach the mercurial patch parser to
> read from stdin if the filename is "-" or no files are listed on the
> command-line.
>
> Based-on-patch-by: Chris Packham <judge.packham@gmail.com>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>

The whole series looks good to me.
Thanks,
Stefan

> ---
>  git-am.sh     |  4 +++-
>  t/t4150-am.sh | 10 ++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index d97da85..0a40d46 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -327,6 +327,7 @@ split_patches () {
>                 ;;
>         hg)
>                 this=0
> +               test 0 -eq "$#" && set -- -
>                 for hg in "$@"
>                 do
>                         this=$(( $this + 1 ))
> @@ -338,6 +339,7 @@ split_patches () {
>                         # Since we cannot guarantee that the commit message is in
>                         # git-friendly format, we put no Subject: line and just consume
>                         # all of the message as the body
> +                       cat "$hg" |
>                         LANG=C LC_ALL=C @@PERL@@ -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
>                                 if ($subject) { print ; }
>                                 elsif (/^\# User /) { s/\# User/From:/ ; print ; }
> @@ -353,7 +355,7 @@ split_patches () {
>                                         print "\n", $_ ;
>                                         $subject = 1;
>                                 }
> -                       ' <"$hg" >"$dotest/$msgnum" || clean_abort
> +                       ' >"$dotest/$msgnum" || clean_abort
>                 done
>                 echo "$this" >"$dotest/last"
>                 this=
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 4beb4b3..3ebafd9 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -259,6 +259,16 @@ test_expect_success 'am applies hg patch' '
>         test_cmp_rev second^ HEAD^
>  '
>
> +test_expect_success 'am --patch-format=hg applies hg patch' '
> +       rm -fr .git/rebase-apply &&
> +       git checkout -f first &&
> +       git am --patch-format=hg <patch1-hg.eml &&
> +       test_path_is_missing .git/rebase-apply &&
> +       git diff --exit-code second &&
> +       test_cmp_rev second HEAD &&
> +       test_cmp_rev second^ HEAD^
> +'
> +
>  test_expect_success 'setup: new author and committer' '
>         GIT_AUTHOR_NAME="Another Thor" &&
>         GIT_AUTHOR_EMAIL="a.thor@example.com" &&
> --
> 2.1.4
>

Re: [PATCH 2/5] am: teach StGit patch parser how to read from stdin

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> git-mailsplit, which splits mbox patches, will read the patch from stdin
> when the filename is "-" or there are no files listed on the
> command-line.
>
> To be consistent with this behavior, teach the StGit patch parser to
> read from stdin if the filename is "-" or no files are listed on the
> command-line.

Hmm, doesn't

	perl -ne 'processing for each line'

with or without a BEGIN {} block, read from the standard input (if
no filename is given) or the given file (if given), and more
importantly, doesn't it treat a lone "-" as STDIN anyway?

That is, wouldn't it make more sense to do something like:

	test $# != 0 || set -- -
        for stgit
        do
        	...
                @@PERL@@ -ne 'BEGIN { $subject = 0 }
                	...
		' "$stgit" >"$dotest/$msgnum" || clean_abort
	done

Same for patch 5/5.

Other than that, the entire series looked great from a cursory read.

Thanks.

Re: [PATCH 2/5] am: teach StGit patch parser how to read from stdin

[Paul Tan]

On Tue, Jun 9, 2015 at 3:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> git-mailsplit, which splits mbox patches, will read the patch from stdin
>> when the filename is "-" or there are no files listed on the
>> command-line.
>>
>> To be consistent with this behavior, teach the StGit patch parser to
>> read from stdin if the filename is "-" or no files are listed on the
>> command-line.
>
> Hmm, doesn't
>
>         perl -ne 'processing for each line'
>
> with or without a BEGIN {} block, read from the standard input (if
> no filename is given) or the given file (if given), and more
> importantly, doesn't it treat a lone "-" as STDIN anyway?
>
> That is, wouldn't it make more sense to do something like:
>
>         test $# != 0 || set -- -
>         for stgit
>         do
>                 ...
>                 @@PERL@@ -ne 'BEGIN { $subject = 0 }
>                         ...
>                 ' "$stgit" >"$dotest/$msgnum" || clean_abort
>         done
>
> Same for patch 5/5.

Ah yes, this makes more sense.

Thanks,
Paul