[PATCH 0/2] am: foreign patch support fixes

[PATCH 0/2] am: foreign patch support fixes

[Giuseppe Bilotta]

Two small patches to fix/enhance support for foreign patchset.

The first patch adds support for hg patches, which have been detected
(but not supported) for a while. I've used it to import a couple of
patches successfully.

The second patch fixes a rather long-standing issue with stgit patches,
when Author was used instead of From. Apparently not many patches with
this format are encountered in the wild, since nobody had an issue with
it so far.

Giuseppe Bilotta (2):
  am: preliminary support for hg patches
  am: fix stgit patch mangling

 git-am.sh |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

-- 
1.7.7.rc0.331.g25483.dirty

[PATCH 1/2] am: preliminary support for hg patches

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 git-am.sh |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 4fff195..729ee51 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -311,6 +311,40 @@ split_patches ()
 		this=
 		msgnum=
 		;;
+	hg)
+		this=0
+		for hg in "$@"
+		do
+			this=`expr "$this" + 1`
+			msgnum=`printf "%0${prec}d" $this`
+			# hg stores changeset metadata in #-commented lines preceding
+			# the commit message and diff(s). The only metadata we care about
+			# are the User and Date (Node ID and Parent are hashes which are
+			# only relevant to the hg repository and thus not useful to us)
+			# 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
+			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
+				if ($subject) { print ; }
+				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
+				elsif (/^\# Date /) {
+					my ($hashsign, $str, $time, $tz) = split ;
+					$tz = sprintf "%+05d", (0-$tz)/36;
+					print "Date: " .
+					      strftime("%a, %d %b %Y %H:%M:%S ",
+						       localtime($time))
+					      . "$tz\n";
+				} elsif (/^\# /) { next ; }
+				else {
+					print "\n", $_ ;
+					$subject = 1;
+				}
+			' < "$hg" > "$dotest/$msgnum" || clean_abort
+		done
+		echo "$this" > "$dotest/last"
+		this=
+		msgnum=
+		;;
 	*)
 		if test -n "$patch_format" ; then
 			clean_abort "$(eval_gettext "Patch format \$patch_format is not supported.")"
-- 
1.7.7.rc0.331.g25483.dirty

Re: [PATCH 1/2] am: preliminary support for hg patches

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

I'll leave nitpicking of this patch and helping to improve it to people
who actually have to deal with Hg generated patches for now.

> +	hg)
> +		this=0
> +		for hg in "$@"
> +		do
> +			this=`expr "$this" + 1`
> +			msgnum=`printf "%0${prec}d" $this`
> +			# hg stores changeset metadata in #-commented lines preceding
> +			# the commit message and diff(s). The only metadata we care about
> +			# are the User and Date (Node ID and Parent are hashes which are
> +			# only relevant to the hg repository and thus not useful to us)
> +			# 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

Personally I am a bit worried about the phoney "diff --git" output Hg
seems to (be able to) produce. Do they have "index ..." line that express
the blob object names in git terms (implausible), for example? We _might_
want to strip s/diff --git /diff / so that apply won't be confused if that
turns out to be a problem.

Thanks.

> +			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
> +				if ($subject) { print ; }
> +				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
> +				elsif (/^\# Date /) {
> +					my ($hashsign, $str, $time, $tz) = split ;
> +					$tz = sprintf "%+05d", (0-$tz)/36;
> +					print "Date: " .
> +					      strftime("%a, %d %b %Y %H:%M:%S ",
> +						       localtime($time))
> +					      . "$tz\n";
> +				} elsif (/^\# /) { next ; }
> +				else {
> +					print "\n", $_ ;
> +					$subject = 1;
> +				}
> +			' < "$hg" > "$dotest/$msgnum" || clean_abort
> +		done
> +		echo "$this" > "$dotest/last"
> +		this=
> +		msgnum=
> +		;;
>  	*)
>  		if test -n "$patch_format" ; then
>  			clean_abort "$(eval_gettext "Patch format \$patch_format is not supported.")"

Re: [PATCH 1/2] am: preliminary support for hg patches

[Giuseppe Bilotta]

On Mon, Aug 29, 2011 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>
> I'll leave nitpicking of this patch and helping to improve it to people
> who actually have to deal with Hg generated patches for now.

[snip]

>> +                     # hg stores changeset metadata in #-commented lines preceding
>> +                     # the commit message and diff(s). The only metadata we care about
>> +                     # are the User and Date (Node ID and Parent are hashes which are
>> +                     # only relevant to the hg repository and thus not useful to us)
>> +                     # 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
>
> Personally I am a bit worried about the phoney "diff --git" output Hg
> seems to (be able to) produce. Do they have "index ..." line that express
> the blob object names in git terms (implausible), for example? We _might_
> want to strip s/diff --git /diff / so that apply won't be confused if that
> turns out to be a problem.

Nope, it doesn't have index .... lines. Still, the patches seems to
apply correctly. Well, the couple of patches I tested did, at least,
even though they were marked as diff --git and they were lacking the
index ... lines.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 1/2] am: preliminary support for hg patches

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> Nope, it doesn't have index .... lines. Still, the patches seems to
> apply correctly. Well, the couple of patches I tested did, at least,
> even though they were marked as diff --git and they were lacking the
> index ... lines.

Does "am -3" do the right thing when the patch does not apply cleanly, for
example? What about renaming patches?

Until we are reasonably sure that we can grok it reasonably well, I'd
sleep better if we stripped " --git" part from a patch that is known to be
produced by somebody that does not fully re-implement "git diff".

Re: [PATCH 1/2] am: preliminary support for hg patches

[Giuseppe Bilotta]

On Mon, Aug 29, 2011 at 11:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> Nope, it doesn't have index .... lines. Still, the patches seems to
>> apply correctly. Well, the couple of patches I tested did, at least,
>> even though they were marked as diff --git and they were lacking the
>> index ... lines.
>
> Does "am -3" do the right thing when the patch does not apply cleanly, for
> example?

three-way merges are impossible because all hash information is being
stripped (hg stores them in the Node ID and Parent metadata, which we
strip, and has no index metadata for the actual diff blocks). This is
correctly detected by -3, with

Applying: Threeway test
fatal: sha1 information is lacking or useless (dir.h).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Threeway test

The message is a bit misleading (it's not the repo lacking the blobs,
it's the patch missing the information), but the process fails as
expected.

> What about renaming patches?

They lack similarity indices, but they seem to be properly formated
(and the simple cases I tested apply correctly).

In fact, stripping the --git from an hg patch containing a rename
makes the patch unusable:

error: datetime.move: does not exist in index
Patch failed at 0001 Move

So I think that keeping the --git is the right choice.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 1/2] am: preliminary support for hg patches

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> ... This is
> correctly detected by -3, with
>
> Applying: Threeway test
> fatal: sha1 information is lacking or useless (dir.h).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Threeway test
>
> The message is a bit misleading (it's not the repo lacking the blobs,
> it's the patch missing the information), but the process fails as
> expected.
>
>> What about renaming patches?
>
> They lack similarity indices, but they seem to be properly formated
> (and the simple cases I tested apply correctly).

These were exactly what I wanted to know. Thanks for experimenting.

> So I think that keeping the --git is the right choice.

Yeah, sounds like we are safe and better off keeping it.

Re: [PATCH 1/2] am: preliminary support for hg patches

[Sverre Rabbelier]

Heya,

On Tue, Aug 30, 2011 at 19:02, Junio C Hamano <gitster@pobox.com> wrote:
> These were exactly what I wanted to know. Thanks for experimenting.

And thanks for working on this, Giuseppe!

-- 
Cheers,

Sverre Rabbelier

[PATCH 2/2] am: fix stgit patch mangling

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 git-am.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 729ee51..14696d4 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -295,7 +295,7 @@ split_patches ()
 			perl -ne 'BEGIN { $subject = 0 }
 				if ($subject > 1) { print ; }
 				elsif (/^\s+$/) { next ; }
-				elsif (/^Author:/) { print s/Author/From/ ; }
+				elsif (/^Author:/) { s/Author/From/ ; print ;}
 				elsif (/^(From|Date)/) { print ; }
 				elsif ($subject) {
 					$subject = 2 ;
-- 
1.7.7.rc0.331.g25483.dirty