threads / patch / 28248

patch, 2 partsam: foreign patch support fixes

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

## tl;dr

9 messages between Aug 29, 2011 and Aug 31, 2011. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Giuseppe Bilotta· Aug 29, 2011, 16:44 UTC · lore

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

Giuseppe Bilotta· Aug 29, 2011, 16:44 UTC · re: Giuseppe Bilotta · lore

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

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

Show changes to git-am.sh +34 −0

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

Junio C Hamano· Aug 29, 2011, 16:57 UTC · re: Giuseppe Bilotta · lore

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

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.

Show 13 quoted lines

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

Show 24 quoted lines

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

Giuseppe Bilotta· Aug 29, 2011, 17:51 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 29, 2011 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

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

Show 13 quoted lines

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

Junio C Hamano· Aug 29, 2011, 21:05 UTC · re: Giuseppe Bilotta · lore

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

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

Giuseppe Bilotta· Aug 30, 2011, 08:28 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 29, 2011 at 11:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

> 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

Junio C Hamano· Aug 30, 2011, 17:02 UTC · re: Giuseppe Bilotta · lore

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

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

Show 17 quoted lines

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

Sverre Rabbelier· Aug 31, 2011, 14:22 UTC · re: Junio C Hamano · lore

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

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

Giuseppe Bilotta· Aug 29, 2011, 16:44 UTC · re: Giuseppe Bilotta · lore

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

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

Show changes to git-am.sh +1 −1

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