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

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

[Mattias Nissler]

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(-)

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

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

[Eric Wong]

Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 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!

>  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

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

[Mattias Nissler]

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.

> 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

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

[Mattias Nissler]

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(-)

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

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

[Mattias Nissler]

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 )
 '
-- 
1.6.3.3

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

[Eric Wong]

Mattias Nissler <mattias.nissler@gmx.de> wrote:
> 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...

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

> 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

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

[Marc Branchaud]

Loveley!  That aspect of -d was bothersome...

Acked-by: Marc Branchaud <marcnarc@xiplink.com>

		M.


Mattias Nissler wrote:
> 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 )
>  '