Re: [PATCH, 2nd version] git-archimport: allow...

Re: [PATCH, 2nd version] git-archimport: allow remapping branch names

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:58

Paolo Bonzini [off-list ref] writes:

	Great idea.  Thanks.  By the way, this patch was tested on
	my Arch repositories.

Just to make sure.  Was it tested with AND WITHOUT the new colon
feature?  I am asking how likely is there a regression.

quoted hunk

diff --git a/git-archimport.perl b/git-archimport.perl
index 0fcb156..1044695 100755
--- a/git-archimport.perl
+++ b/git-archimport.perl

@@ -89,7 +89,11 @@ usage if $opt_h;
 # values associated with keys:
 #   =1 - Arch version / git 'branch' detected via abrowse on a limit
 #   >1 - Arch version / git 'branch' of an auxiliary branch we've merged
-my %arch_branches = map { $_ => 1 } @ARGV;
+my %arch_branches = map { my $branch = $_; $branch =~ s/:.*//; $branch => 1 } @ARGV;

I am a bit puzzled here.  Let's consider four cases.

 (0) No colon in arch archive, no mapping
 (1) No colon in arch archive, mapped to 'master'
 (2) A colon in arch archive, no mapping specified
 (3) A colon in arch archive, mapped to 'master'

The above s/:.*// would do:

 (0) bonzini-2005/devo ==> bonzini-2005/devo
 (1) bonzini-2005/devo:master ==> bonzini-2005/devo
 (2) bonzini:gnu-2005/devo ==> bonzini
 (3) bonzini:gnu-2005/devo:master ==> bonzini

and then later branch_name_map splits them to:

 (0) bonzini-2005/devo ==> no entry in %branch_name_map
 (1) bonzini-2005/devo ==> no entry in %branch_name_map
 (2) bonzini:gnu-2005/devo ==> (bonzini => gnu-2005/devo)
 (3) bonzini:gnu-2005/devo:master ==> (bonzini:gnu-2005/devo => master)

In effect, (2) is an error, which is fine, because if the arch
archive had colon in its name then the user can ask explicitly
to map it by having an extra ':<branchname>'.  However, I think
your s/:.*// above breaks case (3).

quoted hunk

+
+# $branch_name_map:
+# maps arch branches to git branch names
+my %branch_name_map = map { m/^(.*):([^:]*)$/; $1 => $2 } grep { m/:/ } @ARGV;
...

@@ -104,6 +108,7 @@ ...
+sub git_branchname {
+    my $revision = shift;
+    my $name = extract_versionname($revision);
+
+    if (defined $branch_name_map{$name}) {
+	return $branch_name_map{$name};

This is purely a style issue, but I tend to prefer 'exists' over
'defined' when checking if a name is something that was placed
in a hash we set up earlier by reading configuration or command,
even if we _know_ that the hash was created with 'defined' values
in them.

quoted hunk

 sub process_patchset_accurate {
     my $ps = shift;

@@ -333,19 +365,23 @@ sub process_patchset_accurate {
         if ($ps->{tag} && (my $branchpoint = eval { ptag($ps->{tag}) })) {
             
             # find where we are supposed to branch from
-            system('git-checkout','-f','-b',$ps->{branch},
-                            $branchpoint) == 0 or die "$! $?\n";
-            
+	    if (! -e "$git_dir/refs/heads/$ps->{branch}") {
+		system('git-branch',$ps->{branch},$branchpoint) == 0 or die "$! $?\n";

Strictly speaking (-e "$git_dir/refs/heads/$branch") test would
not work if the repository was pack-ref'ed with --all option.
Run "git show-ref -q --verify refs/heads/$branch" and check its
exit status, or run it without -q and read its output.

quoted hunk

@@ -830,8 +871,9 @@ sub tag {
     if ($opt_o) {
         $tag =~ s|/|--|g;
     } else {
-        # don't use subdirs for tags yet, it could screw up other porcelains
-        $tag =~ s|/|,|g;
+	my $patchname = $tag;
+	$patchname =~ s/.*--//;
+        $tag = git_branchname ($tag) . '--' . $patchname;
     }
     
     if ($commit) {

The call to git_branchname() is essential for mapping the name
we get from arch using the command line mapping, but if the user
is not using this remapping feature, does this keep the original
behaviour?

With the original code, a tag "t--a/g" was mapped to "t--a,g" in
the else clause, but the new code yields git_branchname("t--a/g")
followed by '--' followed by "g", which would evaluate to I do
not know what exactly, but I am sure it would not evaluate to
"t--a,g".  Would it be a non-issue?  As archimport seems to support
incremental import, I suspect it might upset existing users.

Re: [PATCH, 2nd version] git-archimport: allow remapping branch names

From: Paolo Bonzini <hidden>
Date: 2016-06-15 22:42:58

Just to make sure.  Was it tested with AND WITHOUT the new colon
feature?  I am asking how likely is there a regression.

Yes.  But my Arch repositories do not have colons in the name (I mean I tested on real-world repositories and not weird ones created exactly to make the patch fail).

quoted

-my %arch_branches = map { $_ => 1 } @ARGV;
+my %arch_branches = map { my $branch = $_; $branch =~ s/:.*//; $branch => 1 } @ARGV;

In fact, I was too headlong on your suggestion.  I should have noticed that this substitution must be changed to

$branch =~ s/:[^:]*$//

I just realized, however, that branches with colons in the names would have failed before my patch too, because git would have failed creating a branch with a colon in it.  So this is not a regression, strictly speaking.

Strictly speaking (-e "$git_dir/refs/heads/$branch") test would
not work if the repository was pack-ref'ed with --all option.
Run "git show-ref -q --verify refs/heads/$branch" and check its
exit status, or run it without -q and read its output.

Unfortunately this is pervasive in git-archimport.  I can fix it, but I think it belongs in a separate patch.

With the original code, a tag "t--a/g" was mapped to "t--a,g" in
the else clause, but the new code yields git_branchname("t--a/g")
followed by '--' followed by "g", which would evaluate to I do
not know what exactly, but I am sure it would not evaluate to
"t--a,g".  Would it be a non-issue?  As archimport seems to support
incremental import, I suspect it might upset existing users.

In this case, this input will always be of the form t--a/g--REVISION.

So the old one would change to t--a,g--REVISION; the new one would
strip to t--a/g and convert it to t--a,g (using git_default_branchname)
and file tack --REVISION at the end again.

I will shortly send an updated patch

Paolo

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help