threads / rfc / 20027

RFC patchgit-svn: Fix branch detection when repository root is inaccessible

Subject: [PATCH/RFC] git-svn: Fix branch detection when repository root is inaccessible

## tl;dr

7 messages between Jul 5, 2009 and Jul 7, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Mattias Nissler· Jul 5, 2009, 11:51 UTC · lore

For the case of multiple projects sharing a single SVN repository, it is common practice to create the standard SVN directory layout within a subdirectory for each project. In such setups, access control is often used to limit what projects a given user may access. git-svn failed to detect branches (e.g. when passing --stdlayout to clone) because it relied on having access to the root directory in the repository. This patch solves this problem by making git-svn use paths relative to the given repository URL instead of the repository root.

Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
---

I've already posted this on May 31st but received no feedback, so here is the patch again against current git.git. I'm using this at work to be able to checkout a complete SVN project including all branches and tags in one go without having to create an individual remote for each branch and tag. Any chance we can get this into mainline git?

Cheers,
Mattias
 git-svn.perl |   83 +++++++++++++++++++++++++--------------------------------
 1 files changed, 36 insertions(+), 47 deletions(-)
Show changes to git-svn.perl +36 −47
diff --git a/git-svn.perl b/git-svn.perl
index d1af1a3..cc15731 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -876,10 +876,6 @@ sub cmd_multi_init {
 		usage(1);
 	}
 
-	# there are currently some bugs that prevent multi-init/multi-fetch
-	# setups from working well without this.
-	$Git::SVN::_minimize_url = 1;
-
 	$_prefix = '' unless defined $_prefix;
 	if (defined $url) {
 		$url = canonicalize_url($url);
@@ -1180,7 +1176,7 @@ sub complete_url_ls_init {
 		    "wanted to set to: $gs->{url}\n";
 	}
 	command_oneline('config', $k, $gs->{url}) unless $orig_url;
-	my $remote_path = "$ra->{svn_path}/$repo_path";
+	my $remote_path = "$gs->{path}/$repo_path";
 	$remote_path =~ s#/+#/#g;
 	$remote_path =~ s#^/##g;
 	$remote_path .= "/*" if $remote_path !~ /\*/;
@@ -2177,16 +2173,6 @@ sub ra {
 	$ra;
 }
 
-sub rel_path {
-	my ($self) = @_;
-	my $repos_root = $self->ra->{repos_root};
-	return $self->{path} if ($self->{url} eq $repos_root);
-	my $url = $self->{url} .
-	          (length $self->{path} ? "/$self->{path}" : $self->{path});
-	$url =~ s!^\Q$repos_root\E(?:/+|$)!!g;
-	$url;
-}
-
 # prop_walk(PATH, REV, SUB)
 # -------------------------
 # Recursively traverse PATH at revision REV and invoke SUB for each
@@ -2512,10 +2498,7 @@ sub match_paths {
 	if (my $path = $paths->{"/$self->{path}"}) {
 		return ($path->{action} eq 'D') ? 0 : 1;
 	}
-	my $repos_root = $self->ra->{repos_root};
-	my $extended_path = $self->{url} . '/' . $self->{path};
-	$extended_path =~ s#^\Q$repos_root\E(/|$)##;
-	$self->{path_regex} ||= qr/^\/\Q$extended_path\E\//;
+	$self->{path_regex} ||= qr/^\/\Q$self->{path}\E\//;
 	if (grep /$self->{path_regex}/, keys %$paths) {
 		return 1;
 	}
@@ -2538,15 +2521,14 @@ sub find_parent_branch {
 	unless (defined $paths) {
 		my $err_handler = $SVN::Error::handler;
 		$SVN::Error::handler = \&Git::SVN::Ra::skip_unknown_revs;
-		$self->ra->get_log([$self->{path}], $rev, $rev, 0, 1, 1, sub {
-		                   $paths =
-				      Git::SVN::Ra::dup_changed_paths($_[0]) });
+		$self->ra->get_log([$self->{path}], $rev, $rev, 0, 1, 1,
+			sub { $paths = $_[0] });
 		$SVN::Error::handler = $err_handler;
 	}
 	return undef unless defined $paths;
 
 	# look for a parent from another branch:
-	my @b_path_components = split m#/#, $self->rel_path;
+	my @b_path_components = split m#/#, $self->{path};
 	my @a_path_components;
 	my $i;
 	while (@b_path_components) {
@@ -2564,11 +2546,11 @@ sub find_parent_branch {
 	my $r = $i->{copyfrom_rev};
 	my $repos_root = $self->ra->{repos_root};
 	my $url = $self->ra->{url};
-	my $new_url = $repos_root . $branch_from;
+	my $new_url = $url . $branch_from;
 	print STDERR  "Found possible branch point: ",
 	              "$new_url => ", $self->full_url, ", $r\n";
 	$branch_from =~ s#^/##;
-	my $gs = $self->other_gs($new_url, $url, $repos_root,
+	my $gs = $self->other_gs($new_url, $url,
 		                 $branch_from, $r, $self->{ref_id});
 	my ($r0, $parent) = $gs->find_rev_before($r, 1);
 	{
@@ -2753,9 +2735,9 @@ sub parse_svn_date {
 }
 
 sub other_gs {
-	my ($self, $new_url, $url, $repos_root,
+	my ($self, $new_url, $url,
 	    $branch_from, $r, $old_ref_id) = @_;
-	my $gs = Git::SVN->find_by_url($new_url, $repos_root, $branch_from);
+	my $gs = Git::SVN->find_by_url($new_url, $url, $branch_from);
 	unless ($gs) {
 		my $ref_id = $old_ref_id;
 		$ref_id =~ s/\@\d+$//;
@@ -4431,6 +4413,31 @@ sub get_log {
 	my ($self, @args) = @_;
 	my $pool = SVN::Pool->new;
 
+	# svn_log_changed_path_t objects passed to get_log are likely to be
+	# overwritten even if only the refs are copied to an external variable,
+	# so we should dup the structures in their entirety.  Using an
+	# externally passed pool (instead of our temporary and quickly cleared
+	# pool in Git::SVN::Ra) does not help matters at all...
+	my $receiver = pop @args;
+	my $prefix = "/".$self->{svn_path};
+	$prefix =~ s#/+($)##;
+	my $prefix_regex = qr#^\Q$prefix\E#;
+	push(@args, sub {
+		my ($paths) = $_[0];
+		return &$receiver(@_) unless $paths;
+		$_[0] = ();
+		foreach my $p (keys %$paths) {
+			my $i = $paths->{$p};
+			# Make path relative to our url, not repos_root
+			$p =~ s/$prefix_regex//;
+			my %s = map { $_ => $i->$_; }
+				qw/copyfrom_path copyfrom_rev action/;
+			$s{'copyfrom_path'} =~ s/$prefix_regex// if $s{'copyfrom_path'};
+			$_[0]{$p} = \%s;
+		}
+		&$receiver(@_);
+	});
+
 	# the limit parameter was not supported in SVN 1.1.x, so we
 	# drop it.  Therefore, the receiver callback passed to it
 	# is made aware of this limitation by being wrapped if
@@ -4442,6 +4449,7 @@ sub get_log {
 			push(@args, sub { &$receiver(@_) if (--$limit >= 0) });
 		}
 	}
+
 	my $ret = $self->SUPER::get_log(@args, $pool);
 	$pool->clear;
 	$ret;
@@ -4600,8 +4608,7 @@ sub gs_fetch_loop_common {
 		};
 		sub _cb {
 			my ($paths, $r, $author, $date, $log) = @_;
-			[ dup_changed_paths($paths),
-			  { author => $author, date => $date, log => $log } ];
+			[ $paths, { author => $author, date => $date, log => $log } ];
 		}
 		$self->get_log([$longest_path], $min, $max, 0, 1, 1,
 		               sub { $revs{$_[1]} = _cb(@_) });
@@ -4823,24 +4830,6 @@ sub skip_unknown_revs {
 	die "Error from SVN, ($errno): ", $err->expanded_message,"\n";
 }
 
-# svn_log_changed_path_t objects passed to get_log are likely to be
-# overwritten even if only the refs are copied to an external variable,
-# so we should dup the structures in their entirety.  Using an externally
-# passed pool (instead of our temporary and quickly cleared pool in
-# Git::SVN::Ra) does not help matters at all...
-sub dup_changed_paths {
-	my ($paths) = @_;
-	return undef unless $paths;
-	my %ret;
-	foreach my $p (keys %$paths) {
-		my $i = $paths->{$p};
-		my %s = map { $_ => $i->$_ }
-		              qw/copyfrom_path copyfrom_rev action/;
-		$ret{$p} = \%s;
-	}
-	\%ret;
-}
-
 package Git::SVN::Log;
 use strict;
 use warnings;
-- 
1.6.3.3
Eric Wong· Jul 6, 2009, 21:27 UTC · re: Mattias Nissler · lore

Re: [PATCH/RFC] git-svn: Fix branch detection when repository root is inaccessible

Mattias Nissler <mattias.nissler@gmx.de> wrote:
Show 19 quoted lines
> For the case of multiple projects sharing a single SVN repository, it is
> common practice to create the standard SVN directory layout within a
> subdirectory for each project. In such setups, access control is often
> used to limit what projects a given user may access. git-svn failed to
> detect branches (e.g. when passing --stdlayout to clone) because it
> relied on having access to the root directory in the repository. This
> patch solves this problem by making git-svn use paths relative to the
> given repository URL instead of the repository root.
> 
> Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
> ---
> 
> I've already posted this on May 31st but received no feedback, so here
> is the patch again against current git.git. I'm using this at work to be
> able to checkout a complete SVN project including all branches and tags
> in one go without having to create an individual remote for each branch
> and tag. Any chance we can get this into mainline git?
> 
> Cheers,

Thanks Mattias, sorry about missing your previous posting. Feel free to email me directly if I don't respond in a week or so in the future.

Fixing the minimize logic is definitely a wanted improvement I haven't been able to fix myself. Unfortunately, your patch breaks the t9138-git-svn-multiple-branches.sh test for me. I haven't looked at it at all, but I'll try to take a look at it later today after I process other things in my overflowing work queue; but I definitely want access issues fixed for people with restrictive repo setups.

One thing I would do is to split out the making of dup_changed_paths() the default behavior into a separate patch. I think this is a good change since it makes things less surprising for future hackers of git svn.

Thanks again!
Show 178 quoted lines
>  git-svn.perl |   83 +++++++++++++++++++++++++--------------------------------
>  1 files changed, 36 insertions(+), 47 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index d1af1a3..cc15731 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -876,10 +876,6 @@ sub cmd_multi_init {
>  		usage(1);
>  	}
>  
> -	# there are currently some bugs that prevent multi-init/multi-fetch
> -	# setups from working well without this.
> -	$Git::SVN::_minimize_url = 1;
> -
>  	$_prefix = '' unless defined $_prefix;
>  	if (defined $url) {
>  		$url = canonicalize_url($url);
> @@ -1180,7 +1176,7 @@ sub complete_url_ls_init {
>  		    "wanted to set to: $gs->{url}\n";
>  	}
>  	command_oneline('config', $k, $gs->{url}) unless $orig_url;
> -	my $remote_path = "$ra->{svn_path}/$repo_path";
> +	my $remote_path = "$gs->{path}/$repo_path";
>  	$remote_path =~ s#/+#/#g;
>  	$remote_path =~ s#^/##g;
>  	$remote_path .= "/*" if $remote_path !~ /\*/;
> @@ -2177,16 +2173,6 @@ sub ra {
>  	$ra;
>  }
>  
> -sub rel_path {
> -	my ($self) = @_;
> -	my $repos_root = $self->ra->{repos_root};
> -	return $self->{path} if ($self->{url} eq $repos_root);
> -	my $url = $self->{url} .
> -	          (length $self->{path} ? "/$self->{path}" : $self->{path});
> -	$url =~ s!^\Q$repos_root\E(?:/+|$)!!g;
> -	$url;
> -}
> -
>  # prop_walk(PATH, REV, SUB)
>  # -------------------------
>  # Recursively traverse PATH at revision REV and invoke SUB for each
> @@ -2512,10 +2498,7 @@ sub match_paths {
>  	if (my $path = $paths->{"/$self->{path}"}) {
>  		return ($path->{action} eq 'D') ? 0 : 1;
>  	}
> -	my $repos_root = $self->ra->{repos_root};
> -	my $extended_path = $self->{url} . '/' . $self->{path};
> -	$extended_path =~ s#^\Q$repos_root\E(/|$)##;
> -	$self->{path_regex} ||= qr/^\/\Q$extended_path\E\//;
> +	$self->{path_regex} ||= qr/^\/\Q$self->{path}\E\//;
>  	if (grep /$self->{path_regex}/, keys %$paths) {
>  		return 1;
>  	}
> @@ -2538,15 +2521,14 @@ sub find_parent_branch {
>  	unless (defined $paths) {
>  		my $err_handler = $SVN::Error::handler;
>  		$SVN::Error::handler = \&Git::SVN::Ra::skip_unknown_revs;
> -		$self->ra->get_log([$self->{path}], $rev, $rev, 0, 1, 1, sub {
> -		                   $paths =
> -				      Git::SVN::Ra::dup_changed_paths($_[0]) });
> +		$self->ra->get_log([$self->{path}], $rev, $rev, 0, 1, 1,
> +			sub { $paths = $_[0] });
>  		$SVN::Error::handler = $err_handler;
>  	}
>  	return undef unless defined $paths;
>  
>  	# look for a parent from another branch:
> -	my @b_path_components = split m#/#, $self->rel_path;
> +	my @b_path_components = split m#/#, $self->{path};
>  	my @a_path_components;
>  	my $i;
>  	while (@b_path_components) {
> @@ -2564,11 +2546,11 @@ sub find_parent_branch {
>  	my $r = $i->{copyfrom_rev};
>  	my $repos_root = $self->ra->{repos_root};
>  	my $url = $self->ra->{url};
> -	my $new_url = $repos_root . $branch_from;
> +	my $new_url = $url . $branch_from;
>  	print STDERR  "Found possible branch point: ",
>  	              "$new_url => ", $self->full_url, ", $r\n";
>  	$branch_from =~ s#^/##;
> -	my $gs = $self->other_gs($new_url, $url, $repos_root,
> +	my $gs = $self->other_gs($new_url, $url,
>  		                 $branch_from, $r, $self->{ref_id});
>  	my ($r0, $parent) = $gs->find_rev_before($r, 1);
>  	{
> @@ -2753,9 +2735,9 @@ sub parse_svn_date {
>  }
>  
>  sub other_gs {
> -	my ($self, $new_url, $url, $repos_root,
> +	my ($self, $new_url, $url,
>  	    $branch_from, $r, $old_ref_id) = @_;
> -	my $gs = Git::SVN->find_by_url($new_url, $repos_root, $branch_from);
> +	my $gs = Git::SVN->find_by_url($new_url, $url, $branch_from);
>  	unless ($gs) {
>  		my $ref_id = $old_ref_id;
>  		$ref_id =~ s/\@\d+$//;
> @@ -4431,6 +4413,31 @@ sub get_log {
>  	my ($self, @args) = @_;
>  	my $pool = SVN::Pool->new;
>  
> +	# svn_log_changed_path_t objects passed to get_log are likely to be
> +	# overwritten even if only the refs are copied to an external variable,
> +	# so we should dup the structures in their entirety.  Using an
> +	# externally passed pool (instead of our temporary and quickly cleared
> +	# pool in Git::SVN::Ra) does not help matters at all...
> +	my $receiver = pop @args;
> +	my $prefix = "/".$self->{svn_path};
> +	$prefix =~ s#/+($)##;
> +	my $prefix_regex = qr#^\Q$prefix\E#;
> +	push(@args, sub {
> +		my ($paths) = $_[0];
> +		return &$receiver(@_) unless $paths;
> +		$_[0] = ();
> +		foreach my $p (keys %$paths) {
> +			my $i = $paths->{$p};
> +			# Make path relative to our url, not repos_root
> +			$p =~ s/$prefix_regex//;
> +			my %s = map { $_ => $i->$_; }
> +				qw/copyfrom_path copyfrom_rev action/;
> +			$s{'copyfrom_path'} =~ s/$prefix_regex// if $s{'copyfrom_path'};
> +			$_[0]{$p} = \%s;
> +		}
> +		&$receiver(@_);
> +	});
> +
>  	# the limit parameter was not supported in SVN 1.1.x, so we
>  	# drop it.  Therefore, the receiver callback passed to it
>  	# is made aware of this limitation by being wrapped if
> @@ -4442,6 +4449,7 @@ sub get_log {
>  			push(@args, sub { &$receiver(@_) if (--$limit >= 0) });
>  		}
>  	}
> +
>  	my $ret = $self->SUPER::get_log(@args, $pool);
>  	$pool->clear;
>  	$ret;
> @@ -4600,8 +4608,7 @@ sub gs_fetch_loop_common {
>  		};
>  		sub _cb {
>  			my ($paths, $r, $author, $date, $log) = @_;
> -			[ dup_changed_paths($paths),
> -			  { author => $author, date => $date, log => $log } ];
> +			[ $paths, { author => $author, date => $date, log => $log } ];
>  		}
>  		$self->get_log([$longest_path], $min, $max, 0, 1, 1,
>  		               sub { $revs{$_[1]} = _cb(@_) });
> @@ -4823,24 +4830,6 @@ sub skip_unknown_revs {
>  	die "Error from SVN, ($errno): ", $err->expanded_message,"\n";
>  }
>  
> -# svn_log_changed_path_t objects passed to get_log are likely to be
> -# overwritten even if only the refs are copied to an external variable,
> -# so we should dup the structures in their entirety.  Using an externally
> -# passed pool (instead of our temporary and quickly cleared pool in
> -# Git::SVN::Ra) does not help matters at all...
> -sub dup_changed_paths {
> -	my ($paths) = @_;
> -	return undef unless $paths;
> -	my %ret;
> -	foreach my $p (keys %$paths) {
> -		my $i = $paths->{$p};
> -		my %s = map { $_ => $i->$_ }
> -		              qw/copyfrom_path copyfrom_rev action/;
> -		$ret{$p} = \%s;
> -	}
> -	\%ret;
> -}
> -
>  package Git::SVN::Log;
>  use strict;
>  use warnings;
> -- 
> 1.6.3.3
Mattias Nissler· Jul 6, 2009, 22:28 UTC · re: Eric Wong · lore

Re: [PATCH/RFC] git-svn: Fix branch detection when repository root is inaccessible

Hi Eric,
On Mon, 2009-07-06 at 14:27 -0700, Eric Wong wrote:
> Fixing the minimize logic is definitely a wanted improvement I haven't
> been able to fix myself.  Unfortunately, your patch breaks the
> t9138-git-svn-multiple-branches.sh test for me.  

I see the same here, not sure why I've missed it before. I've had a look at the problem and found that the test is actually depending on the minimizing logic, i.e. needs to specify -d project1/<tag-or-branch-dir> for the tag or branch destination where the project1 prefix is due to the minimization at clone time. Dropping the project1 part makes the test succeed. I'll simply update the test to do so.

I also think this is much better from a usability point of view since you can now use the original branch or tag dir you've given to clone in order to specify the branch or tag destination.

Show 9 quoted lines
> I haven't looked at it
> at all, but I'll try to take a look at it later today after I process other
> things in my overflowing work queue; but I definitely want access issues
> fixed for people with restrictive repo setups.
> 
> One thing I would do is to split out the making of dup_changed_paths()
> the default behavior into a separate patch.  I think this is a good
> change since it makes things less surprising for future hackers of
> git svn.
Ok, will do so. I'll send updated patches in reply to this mail.

Finally, a big thanks for the nice job on git-svn. I've just experienced lots of trouble with the mercurial SVN integration and it's really a pain in the ass even to just get a simple clean rebase done properly. git-svn is really way more mature and it's a pleasure to use it for everyday work.

Mattias
Eric Wong· Jul 7, 2009, 07:05 UTC · re: Mattias Nissler · lore

Re: [PATCH/RFC] git-svn: Fix branch detection when repository root is inaccessible

Mattias Nissler <mattias.nissler@gmx.de> wrote:
Show 17 quoted lines
> Hi Eric,
> 
> On Mon, 2009-07-06 at 14:27 -0700, Eric Wong wrote:
> > Fixing the minimize logic is definitely a wanted improvement I haven't
> > been able to fix myself.  Unfortunately, your patch breaks the
> > t9138-git-svn-multiple-branches.sh test for me.  
> 
> I see the same here, not sure why I've missed it before. I've had a look
> at the problem and found that the test is actually depending on the
> minimizing logic, i.e. needs to specify -d project1/<tag-or-branch-dir>
> for the tag or branch destination where the project1 prefix is due to
> the minimization at clone time. Dropping the project1 part makes the
> test succeed. I'll simply update the test to do so.
> 
> I also think this is much better from a usability point of view since
> you can now use the original branch or tag dir you've given to clone in
> order to specify the branch or tag destination.

OK. I was slightly concerned about potential breakage but I guess existing imports will continue to work fine, so I suppose it's alright...

Show 11 quoted lines
> > I haven't looked at it
> > at all, but I'll try to take a look at it later today after I process other
> > things in my overflowing work queue; but I definitely want access issues
> > fixed for people with restrictive repo setups.
> > 
> > One thing I would do is to split out the making of dup_changed_paths()
> > the default behavior into a separate patch.  I think this is a good
> > change since it makes things less surprising for future hackers of
> > git svn.
> 
> Ok, will do so. I'll send updated patches in reply to this mail.

Thanks again Mattias. Acked and pushed out with minor formatting fixes to git://git.bogomips.org/git-svn along with Yann's documentation patches.

Show 5 quoted lines
> Finally, a big thanks for the nice job on git-svn. I've just experienced
> lots of trouble with the mercurial SVN integration and it's really a
> pain in the ass even to just get a simple clean rebase done properly.
> git-svn is really way more mature and it's a pleasure to use it for
> everyday work.
No problem :)
-- 
Eric Wong
Mattias Nissler· Jul 6, 2009, 23:39 UTC · re: Eric Wong · lore

[PATCH 1/2] git-svn: Always duplicate paths returned from get_log

This makes get_log more safe to use because callers cannot run into path clobbering any more. The additional overhead will not affect performance since the critical calls from the fetch loop need the path duplication anyway and the rest of the call sites is not performance critical.

Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
---
 git-svn.perl |   46 +++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 23 deletions(-)
Show changes to git-svn.perl +23 −23
diff --git a/git-svn.perl b/git-svn.perl
index d1af1a3..57d13af 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2538,9 +2538,8 @@ sub find_parent_branch {
 	unless (defined $paths) {
 		my $err_handler = $SVN::Error::handler;
 		$SVN::Error::handler = \&Git::SVN::Ra::skip_unknown_revs;
-		$self->ra->get_log([$self->{path}], $rev, $rev, 0, 1, 1, sub {
-		                   $paths =
-				      Git::SVN::Ra::dup_changed_paths($_[0]) });
+		$self->ra->get_log([$self->{path}], $rev, $rev, 0, 1, 1,
+				   sub { $paths = $_[0] });
 		$SVN::Error::handler = $err_handler;
 	}
 	return undef unless defined $paths;
@@ -4431,6 +4430,26 @@ sub get_log {
 	my ($self, @args) = @_;
 	my $pool = SVN::Pool->new;
 
+	# svn_log_changed_path_t objects passed to get_log are likely to be
+	# overwritten even if only the refs are copied to an external variable,
+	# so we should dup the structures in their entirety.  Using an externally
+	# passed pool (instead of our temporary and quickly cleared pool in
+	# Git::SVN::Ra) does not help matters at all...
+	my $receiver = pop @args;
+	push(@args, sub {
+		my ($paths) = $_[0];
+		return &$receiver(@_) unless $paths;
+		$_[0] = ();
+		foreach my $p (keys %$paths) {
+			my $i = $paths->{$p};
+			my %s = map { $_ => $i->$_ }
+				      qw/copyfrom_path copyfrom_rev action/;
+			$_[0]{$p} = \%s;
+		}
+		&$receiver(@_);
+	});
+
+
 	# the limit parameter was not supported in SVN 1.1.x, so we
 	# drop it.  Therefore, the receiver callback passed to it
 	# is made aware of this limitation by being wrapped if
@@ -4600,8 +4619,7 @@ sub gs_fetch_loop_common {
 		};
 		sub _cb {
 			my ($paths, $r, $author, $date, $log) = @_;
-			[ dup_changed_paths($paths),
-			  { author => $author, date => $date, log => $log } ];
+			[ $paths, { author => $author, date => $date, log => $log } ];
 		}
 		$self->get_log([$longest_path], $min, $max, 0, 1, 1,
 		               sub { $revs{$_[1]} = _cb(@_) });
@@ -4823,24 +4841,6 @@ sub skip_unknown_revs {
 	die "Error from SVN, ($errno): ", $err->expanded_message,"\n";
 }
 
-# svn_log_changed_path_t objects passed to get_log are likely to be
-# overwritten even if only the refs are copied to an external variable,
-# so we should dup the structures in their entirety.  Using an externally
-# passed pool (instead of our temporary and quickly cleared pool in
-# Git::SVN::Ra) does not help matters at all...
-sub dup_changed_paths {
-	my ($paths) = @_;
-	return undef unless $paths;
-	my %ret;
-	foreach my $p (keys %$paths) {
-		my $i = $paths->{$p};
-		my %s = map { $_ => $i->$_ }
-		              qw/copyfrom_path copyfrom_rev action/;
-		$ret{$p} = \%s;
-	}
-	\%ret;
-}
-
 package Git::SVN::Log;
 use strict;
 use warnings;
-- 
1.6.3.3
Mattias Nissler· Jul 6, 2009, 23:40 UTC · re: Eric Wong · lore

[PATCH 2/2] git-svn: Fix branch detection when repository root is inaccessible

For the case of multiple projects sharing a single SVN repository, it is common practice to create the standard SVN directory layout within a subdirectory for each project. In such setups, access control is often used to limit what projects a given user may access. git-svn failed to detect branches (e.g. when passing --stdlayout to clone) because it relied on having access to the root directory in the repository. This patch solves this problem by making git-svn use paths relative to the given repository URL instead of the repository root.

Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
---
 git-svn.perl                         |   42 +++++++++++++---------------------
 t/t9138-git-svn-multiple-branches.sh |    8 +++---
 2 files changed, 20 insertions(+), 30 deletions(-)
Show changes to 2 files +20 −30

git-svn.perl, t/t9138-git-svn-multiple-branches.sh

diff --git a/git-svn.perl b/git-svn.perl
index 57d13af..cf3948c 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -876,10 +876,6 @@ sub cmd_multi_init {
 		usage(1);
 	}
 
-	# there are currently some bugs that prevent multi-init/multi-fetch
-	# setups from working well without this.
-	$Git::SVN::_minimize_url = 1;
-
 	$_prefix = '' unless defined $_prefix;
 	if (defined $url) {
 		$url = canonicalize_url($url);
@@ -1180,7 +1176,7 @@ sub complete_url_ls_init {
 		    "wanted to set to: $gs->{url}\n";
 	}
 	command_oneline('config', $k, $gs->{url}) unless $orig_url;
-	my $remote_path = "$ra->{svn_path}/$repo_path";
+	my $remote_path = "$gs->{path}/$repo_path";
 	$remote_path =~ s#/+#/#g;
 	$remote_path =~ s#^/##g;
 	$remote_path .= "/*" if $remote_path !~ /\*/;
@@ -2177,16 +2173,6 @@ sub ra {
 	$ra;
 }
 
-sub rel_path {
-	my ($self) = @_;
-	my $repos_root = $self->ra->{repos_root};
-	return $self->{path} if ($self->{url} eq $repos_root);
-	my $url = $self->{url} .
-	          (length $self->{path} ? "/$self->{path}" : $self->{path});
-	$url =~ s!^\Q$repos_root\E(?:/+|$)!!g;
-	$url;
-}
-
 # prop_walk(PATH, REV, SUB)
 # -------------------------
 # Recursively traverse PATH at revision REV and invoke SUB for each
@@ -2512,10 +2498,7 @@ sub match_paths {
 	if (my $path = $paths->{"/$self->{path}"}) {
 		return ($path->{action} eq 'D') ? 0 : 1;
 	}
-	my $repos_root = $self->ra->{repos_root};
-	my $extended_path = $self->{url} . '/' . $self->{path};
-	$extended_path =~ s#^\Q$repos_root\E(/|$)##;
-	$self->{path_regex} ||= qr/^\/\Q$extended_path\E\//;
+	$self->{path_regex} ||= qr/^\/\Q$self->{path}\E\//;
 	if (grep /$self->{path_regex}/, keys %$paths) {
 		return 1;
 	}
@@ -2545,7 +2528,7 @@ sub find_parent_branch {
 	return undef unless defined $paths;
 
 	# look for a parent from another branch:
-	my @b_path_components = split m#/#, $self->rel_path;
+	my @b_path_components = split m#/#, $self->{path};
 	my @a_path_components;
 	my $i;
 	while (@b_path_components) {
@@ -2563,11 +2546,11 @@ sub find_parent_branch {
 	my $r = $i->{copyfrom_rev};
 	my $repos_root = $self->ra->{repos_root};
 	my $url = $self->ra->{url};
-	my $new_url = $repos_root . $branch_from;
+	my $new_url = $url . $branch_from;
 	print STDERR  "Found possible branch point: ",
 	              "$new_url => ", $self->full_url, ", $r\n";
 	$branch_from =~ s#^/##;
-	my $gs = $self->other_gs($new_url, $url, $repos_root,
+	my $gs = $self->other_gs($new_url, $url,
 		                 $branch_from, $r, $self->{ref_id});
 	my ($r0, $parent) = $gs->find_rev_before($r, 1);
 	{
@@ -2752,9 +2735,9 @@ sub parse_svn_date {
 }
 
 sub other_gs {
-	my ($self, $new_url, $url, $repos_root,
+	my ($self, $new_url, $url,
 	    $branch_from, $r, $old_ref_id) = @_;
-	my $gs = Git::SVN->find_by_url($new_url, $repos_root, $branch_from);
+	my $gs = Git::SVN->find_by_url($new_url, $url, $branch_from);
 	unless ($gs) {
 		my $ref_id = $old_ref_id;
 		$ref_id =~ s/\@\d+$//;
@@ -4436,14 +4419,20 @@ sub get_log {
 	# passed pool (instead of our temporary and quickly cleared pool in
 	# Git::SVN::Ra) does not help matters at all...
 	my $receiver = pop @args;
+	my $prefix = "/".$self->{svn_path};
+	$prefix =~ s#/+($)##;
+	my $prefix_regex = qr#^\Q$prefix\E#;
 	push(@args, sub {
 		my ($paths) = $_[0];
 		return &$receiver(@_) unless $paths;
 		$_[0] = ();
 		foreach my $p (keys %$paths) {
 			my $i = $paths->{$p};
-			my %s = map { $_ => $i->$_ }
-				      qw/copyfrom_path copyfrom_rev action/;
+			# Make path relative to our url, not repos_root
+			$p =~ s/$prefix_regex//;
+			my %s = map { $_ => $i->$_; }
+				qw/copyfrom_path copyfrom_rev action/;
+			$s{'copyfrom_path'} =~ s/$prefix_regex// if $s{'copyfrom_path'};
 			$_[0]{$p} = \%s;
 		}
 		&$receiver(@_);
@@ -4461,6 +4450,7 @@ sub get_log {
 			push(@args, sub { &$receiver(@_) if (--$limit >= 0) });
 		}
 	}
+
 	my $ret = $self->SUPER::get_log(@args, $pool);
 	$pool->clear;
 	$ret;
diff --git a/t/t9138-git-svn-multiple-branches.sh b/t/t9138-git-svn-multiple-branches.sh
index cb9a6d2..3cd0671 100755
--- a/t/t9138-git-svn-multiple-branches.sh
+++ b/t/t9138-git-svn-multiple-branches.sh
@@ -99,22 +99,22 @@ test_expect_success 'Multiple branch or tag paths require -d' '
 
 test_expect_success 'create new branches and tags' '
 	( cd git_project &&
-		git svn branch -m "New branch 1" -d project/b_one New1 ) &&
+		git svn branch -m "New branch 1" -d b_one New1 ) &&
 	( cd svn_project &&
 		svn_cmd up && test -e b_one/New1/a.file ) &&
 
 	( cd git_project &&
-		git svn branch -m "New branch 2" -d project/b_two New2 ) &&
+		git svn branch -m "New branch 2" -d b_two New2 ) &&
 	( cd svn_project &&
 		svn_cmd up && test -e b_two/New2/a.file ) &&
 
 	( cd git_project &&
-		git svn branch -t -m "New tag 1" -d project/tags_A Tag1 ) &&
+		git svn branch -t -m "New tag 1" -d tags_A Tag1 ) &&
 	( cd svn_project &&
 		svn_cmd up && test -e tags_A/Tag1/a.file ) &&
 
 	( cd git_project &&
-		git svn tag -m "New tag 2" -d project/tags_B Tag2 ) &&
+		git svn tag -m "New tag 2" -d tags_B Tag2 ) &&
 	( cd svn_project &&
 		svn_cmd up && test -e tags_B/Tag2/a.file )
 '
-- 
1.6.3.3
Marc Branchaud· Jul 7, 2009, 15:30 UTC · re: Mattias Nissler · lore

Re: [PATCH 2/2] git-svn: Fix branch detection when repository root is inaccessible

Loveley!  That aspect of -d was bothersome...
Acked-by: Marc Branchaud <marcnarc@xiplink.com>
		M.
Mattias Nissler wrote:
Show 165 quoted lines
> For the case of multiple projects sharing a single SVN repository, it is
> common practice to create the standard SVN directory layout within a
> subdirectory for each project. In such setups, access control is often
> used to limit what projects a given user may access. git-svn failed to
> detect branches (e.g. when passing --stdlayout to clone) because it
> relied on having access to the root directory in the repository. This
> patch solves this problem by making git-svn use paths relative to the
> given repository URL instead of the repository root.
> 
> Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
> ---
>  git-svn.perl                         |   42 +++++++++++++---------------------
>  t/t9138-git-svn-multiple-branches.sh |    8 +++---
>  2 files changed, 20 insertions(+), 30 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 57d13af..cf3948c 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -876,10 +876,6 @@ sub cmd_multi_init {
>  		usage(1);
>  	}
>  
> -	# there are currently some bugs that prevent multi-init/multi-fetch
> -	# setups from working well without this.
> -	$Git::SVN::_minimize_url = 1;
> -
>  	$_prefix = '' unless defined $_prefix;
>  	if (defined $url) {
>  		$url = canonicalize_url($url);
> @@ -1180,7 +1176,7 @@ sub complete_url_ls_init {
>  		    "wanted to set to: $gs->{url}\n";
>  	}
>  	command_oneline('config', $k, $gs->{url}) unless $orig_url;
> -	my $remote_path = "$ra->{svn_path}/$repo_path";
> +	my $remote_path = "$gs->{path}/$repo_path";
>  	$remote_path =~ s#/+#/#g;
>  	$remote_path =~ s#^/##g;
>  	$remote_path .= "/*" if $remote_path !~ /\*/;
> @@ -2177,16 +2173,6 @@ sub ra {
>  	$ra;
>  }
>  
> -sub rel_path {
> -	my ($self) = @_;
> -	my $repos_root = $self->ra->{repos_root};
> -	return $self->{path} if ($self->{url} eq $repos_root);
> -	my $url = $self->{url} .
> -	          (length $self->{path} ? "/$self->{path}" : $self->{path});
> -	$url =~ s!^\Q$repos_root\E(?:/+|$)!!g;
> -	$url;
> -}
> -
>  # prop_walk(PATH, REV, SUB)
>  # -------------------------
>  # Recursively traverse PATH at revision REV and invoke SUB for each
> @@ -2512,10 +2498,7 @@ sub match_paths {
>  	if (my $path = $paths->{"/$self->{path}"}) {
>  		return ($path->{action} eq 'D') ? 0 : 1;
>  	}
> -	my $repos_root = $self->ra->{repos_root};
> -	my $extended_path = $self->{url} . '/' . $self->{path};
> -	$extended_path =~ s#^\Q$repos_root\E(/|$)##;
> -	$self->{path_regex} ||= qr/^\/\Q$extended_path\E\//;
> +	$self->{path_regex} ||= qr/^\/\Q$self->{path}\E\//;
>  	if (grep /$self->{path_regex}/, keys %$paths) {
>  		return 1;
>  	}
> @@ -2545,7 +2528,7 @@ sub find_parent_branch {
>  	return undef unless defined $paths;
>  
>  	# look for a parent from another branch:
> -	my @b_path_components = split m#/#, $self->rel_path;
> +	my @b_path_components = split m#/#, $self->{path};
>  	my @a_path_components;
>  	my $i;
>  	while (@b_path_components) {
> @@ -2563,11 +2546,11 @@ sub find_parent_branch {
>  	my $r = $i->{copyfrom_rev};
>  	my $repos_root = $self->ra->{repos_root};
>  	my $url = $self->ra->{url};
> -	my $new_url = $repos_root . $branch_from;
> +	my $new_url = $url . $branch_from;
>  	print STDERR  "Found possible branch point: ",
>  	              "$new_url => ", $self->full_url, ", $r\n";
>  	$branch_from =~ s#^/##;
> -	my $gs = $self->other_gs($new_url, $url, $repos_root,
> +	my $gs = $self->other_gs($new_url, $url,
>  		                 $branch_from, $r, $self->{ref_id});
>  	my ($r0, $parent) = $gs->find_rev_before($r, 1);
>  	{
> @@ -2752,9 +2735,9 @@ sub parse_svn_date {
>  }
>  
>  sub other_gs {
> -	my ($self, $new_url, $url, $repos_root,
> +	my ($self, $new_url, $url,
>  	    $branch_from, $r, $old_ref_id) = @_;
> -	my $gs = Git::SVN->find_by_url($new_url, $repos_root, $branch_from);
> +	my $gs = Git::SVN->find_by_url($new_url, $url, $branch_from);
>  	unless ($gs) {
>  		my $ref_id = $old_ref_id;
>  		$ref_id =~ s/\@\d+$//;
> @@ -4436,14 +4419,20 @@ sub get_log {
>  	# passed pool (instead of our temporary and quickly cleared pool in
>  	# Git::SVN::Ra) does not help matters at all...
>  	my $receiver = pop @args;
> +	my $prefix = "/".$self->{svn_path};
> +	$prefix =~ s#/+($)##;
> +	my $prefix_regex = qr#^\Q$prefix\E#;
>  	push(@args, sub {
>  		my ($paths) = $_[0];
>  		return &$receiver(@_) unless $paths;
>  		$_[0] = ();
>  		foreach my $p (keys %$paths) {
>  			my $i = $paths->{$p};
> -			my %s = map { $_ => $i->$_ }
> -				      qw/copyfrom_path copyfrom_rev action/;
> +			# Make path relative to our url, not repos_root
> +			$p =~ s/$prefix_regex//;
> +			my %s = map { $_ => $i->$_; }
> +				qw/copyfrom_path copyfrom_rev action/;
> +			$s{'copyfrom_path'} =~ s/$prefix_regex// if $s{'copyfrom_path'};
>  			$_[0]{$p} = \%s;
>  		}
>  		&$receiver(@_);
> @@ -4461,6 +4450,7 @@ sub get_log {
>  			push(@args, sub { &$receiver(@_) if (--$limit >= 0) });
>  		}
>  	}
> +
>  	my $ret = $self->SUPER::get_log(@args, $pool);
>  	$pool->clear;
>  	$ret;
> diff --git a/t/t9138-git-svn-multiple-branches.sh b/t/t9138-git-svn-multiple-branches.sh
> index cb9a6d2..3cd0671 100755
> --- a/t/t9138-git-svn-multiple-branches.sh
> +++ b/t/t9138-git-svn-multiple-branches.sh
> @@ -99,22 +99,22 @@ test_expect_success 'Multiple branch or tag paths require -d' '
>  
>  test_expect_success 'create new branches and tags' '
>  	( cd git_project &&
> -		git svn branch -m "New branch 1" -d project/b_one New1 ) &&
> +		git svn branch -m "New branch 1" -d b_one New1 ) &&
>  	( cd svn_project &&
>  		svn_cmd up && test -e b_one/New1/a.file ) &&
>  
>  	( cd git_project &&
> -		git svn branch -m "New branch 2" -d project/b_two New2 ) &&
> +		git svn branch -m "New branch 2" -d b_two New2 ) &&
>  	( cd svn_project &&
>  		svn_cmd up && test -e b_two/New2/a.file ) &&
>  
>  	( cd git_project &&
> -		git svn branch -t -m "New tag 1" -d project/tags_A Tag1 ) &&
> +		git svn branch -t -m "New tag 1" -d tags_A Tag1 ) &&
>  	( cd svn_project &&
>  		svn_cmd up && test -e tags_A/Tag1/a.file ) &&
>  
>  	( cd git_project &&
> -		git svn tag -m "New tag 2" -d project/tags_B Tag2 ) &&
> +		git svn tag -m "New tag 2" -d tags_B Tag2 ) &&
>  	( cd svn_project &&
>  		svn_cmd up && test -e tags_B/Tag2/a.file )
>  '

← back to recent threads