[bug][bisected] git-svn with root branches

[bug][bisected] git-svn with root branches

[Daniel Cordero]

Hello,

when trying to clone a svn repo with the command-line:

	$ git svn clone -b / http://svn.collab.net/repos/svn/

(that is, each folder in the root of the repo should be considered it's
own branch)
the clone sometimes[1] fails saying:

	ref: 'refs/remotes/' ends with a trailing slash, this is not permitted by git nor Subversion

The offending config is:
[svn-remote "svn"]
        url = http://svn.collab.net/repos/svn
        branches = /*:refs/remotes/*


This used to work in the past; I bisected the bad commit to

commit 6f5748e14cc5bb0a836b649fb8e2d6a5eb166f1d
Author: Adam Brewster <adambrewster@gmail.com>
Date:   Tue Aug 11 23:14:03 2009 -0400

    svn: allow branches outside of refs/remotes


Thanks in advance.


[1] It does work when the URL has at least 1 folder of depth
(e.g. suffix "trunk" to the above URL).

Its config section is:
[svn-remote "svn"]
        url = http://svn.collab.net/repos/svn
	branches = trunk//*:refs/remotes/*

Re: [bug][bisected] git-svn with root branches

[Eric Wong]

Daniel Cordero <theappleman@gmail.com> wrote:
> Hello,
> 
> when trying to clone a svn repo with the command-line:
> 
> 	$ git svn clone -b / http://svn.collab.net/repos/svn/
> 
> (that is, each folder in the root of the repo should be considered it's
> own branch)
> the clone sometimes[1] fails saying:
> 
> 	ref: 'refs/remotes/' ends with a trailing slash, this is not permitted by git nor Subversion
> 
> The offending config is:
> [svn-remote "svn"]
>         url = http://svn.collab.net/repos/svn
>         branches = /*:refs/remotes/*
> 
> 
> This used to work in the past; I bisected the bad commit to
> 
> commit 6f5748e14cc5bb0a836b649fb8e2d6a5eb166f1d
> Author: Adam Brewster <adambrewster@gmail.com>
> Date:   Tue Aug 11 23:14:03 2009 -0400
> 
>     svn: allow branches outside of refs/remotes
> 
> 
> Thanks in advance.

Thanks for bisecting it for us!

Reverting the left hand side of these two regexps from Adam's commit
seems to fix the problem.

diff --git a/git-svn.perl b/git-svn.perl
index eb4b75a..56af221 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1765,7 +1765,7 @@ sub read_all_remotes {
 	my $use_svm_props = eval { command_oneline(qw/config --bool
 	    svn.useSvmProps/) };
 	$use_svm_props = $use_svm_props eq 'true' if $use_svm_props;
-	my $svn_refspec = qr{\s*/?(.*?)\s*:\s*(.+?)\s*};
+	my $svn_refspec = qr{\s*(.*?)\s*:\s*(.+?)\s*};
 	foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
 		if (m!^(.+)\.fetch=$svn_refspec$!) {
 			my ($remote, $local_ref, $remote_ref) = ($1, $2, $3);
@@ -1979,7 +1979,7 @@ sub find_ref {
 	my ($ref_id) = @_;
 	foreach (command(qw/config -l/)) {
 		next unless m!^svn-remote\.(.+)\.fetch=
-		              \s*/?(.*?)\s*:\s*(.+?)\s*$!x;
+		              \s*(.*?)\s*:\s*(.+?)\s*$!x;
 		my ($repo_id, $path, $ref) = ($1, $2, $3);
 		if ($ref eq $ref_id) {
 			$path = '' if ($path =~ m#^\./?#);
---

I'm not sure why Adam decided the leading slash needed to be removed for
the git refspec.  That said, the globbing/branching code still makes me
want to hide under a rock and I'm generally afraid to touch it.
I'll wait for Adam to chime in since he's braver than I am :)

-- 
Eric Wong

Re: [bug][bisected] git-svn with root branches

[Eric Wong]

Adam Brewster <adambrewster@gmail.com> wrote:
> > > The offending config is:
> > > [svn-remote "svn"]
> > >         url = http://svn.collab.net/repos/svn
> > >         branches = /*:refs/remotes/*
> > >
> >
> > Reverting the left hand side of these two regexps from Adam's commit
> > seems to fix the problem.
> >
> > diff --git a/git-svn.perl b/git-svn.perl
> > index eb4b75a..56af221 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -1765,7 +1765,7 @@ sub read_all_remotes {
> >        my $use_svm_props = eval { command_oneline(qw/config --bool
> >            svn.useSvmProps/) };
> >        $use_svm_props = $use_svm_props eq 'true' if $use_svm_props;
> > -       my $svn_refspec = qr{\s*/?(.*?)\s*:\s*(.+?)\s*};
> > +       my $svn_refspec = qr{\s*(.*?)\s*:\s*(.+?)\s*};
> >        foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
> >                if (m!^(.+)\.fetch=$svn_refspec$!) {
> >                        my ($remote, $local_ref, $remote_ref) = ($1, $2,
> > $3);
> > @@ -1979,7 +1979,7 @@ sub find_ref {
> >        my ($ref_id) = @_;
> >        foreach (command(qw/config -l/)) {
> >                next unless m!^svn-remote\.(.+)\.fetch=
> > -                             \s*/?(.*?)\s*:\s*(.+?)\s*$!x;
> > +                             \s*(.*?)\s*:\s*(.+?)\s*$!x;
> >                my ($repo_id, $path, $ref) = ($1, $2, $3);
> >                if ($ref eq $ref_id) {
> >                        $path = '' if ($path =~ m#^\./?#);
> > ---
> >
> > I'm not sure why Adam decided the leading slash needed to be removed for
> > the git refspec.  That said, the globbing/branching code still makes me
> > want to hide under a rock and I'm generally afraid to touch it.
> > I'll wait for Adam to chime in since he's braver than I am :)
> >
> >
> How's this for brave:  I'm not sure why I did that either.
> 
> Looking at it again it seems correct for the leading / to be ignored because
> it has no meaning.  What's the difference between the above config and
> "branches = *:refs/remotes/*" (besides the fact that one works and one
> doesn't)?  In both cases I think I've asked for all of the top-level
> directories to be branches.  But that has nothing to do with the rest of the
> patch, so it shouldn't have been included.
>
> Given all of that I would be more inclined to fix this with something like
> the following:
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index eb4b75a..cd8a93b 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1768,7 +1768,7 @@ sub read_all_remotes {
>      my $svn_refspec = qr{\s*/?(.*?)\s*:\s*(.+?)\s*};
>      foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
>          if (m!^(.+)\.fetch=$svn_refspec$!) {
> -            my ($remote, $local_ref, $remote_ref) = ($1, $2, $3);
> +            my ($remote, $local_ref, $remote_ref) = ($1, "/$2", $3);
>              die("svn-remote.$remote: remote ref '$remote_ref' "
>                  . "must start with 'refs/'\n")
>                  unless $remote_ref =~ m{^refs/};
> @@ -1780,7 +1780,7 @@ sub read_all_remotes {
>              $r->{$1}->{url} = $2;
>          } elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
>              my ($remote, $t, $local_ref, $remote_ref) =
> -                                                 ($1, $2, $3, $4);
> +                                                 ($1, $2, "/$3", $4);
>              die("svn-remote.$remote: remote ref '$remote_ref' ($t) "
>                  . "must start with 'refs/'\n")
>                  unless $remote_ref =~ m{^refs/};
> @@ -1980,7 +1980,7 @@ sub find_ref {
>      foreach (command(qw/config -l/)) {
>          next unless m!^svn-remote\.(.+)\.fetch=
>                        \s*/?(.*?)\s*:\s*(.+?)\s*$!x;
> -        my ($repo_id, $path, $ref) = ($1, $2, $3);
> +        my ($repo_id, $path, $ref) = ($1, "/$2", $3);
>          if ($ref eq $ref_id) {
>              $path = '' if ($path =~ m#^\./?#);
>              return ($repo_id, $path);
> -- 
> 1.6.5.1.63.ga9d7.dirty
> 
> That is, continue disregard the / on the actual input because it means
> nothing, and add a / in places where the code will crash if it's not there.
> Even better would be to find out _why_ $path and $local_ref need a leading /
> and fix it that way, but that's more work that I don't have time for right
> now.

Thanks for the feedback Adam,

Yeah, I'm confused by that myself :x  /me kicks himself again

> On the other hand it doesn't really matter because git svn init won't accept
> -b '' (to create the configuration I proposed above), so I have no practical
> objection to Eric's solution.

Meanwhile, your patch looks good to me.  Let me know if you want write
a commit message for it or if you want me to do it.

I actually tried editing my $GIT_CONFIG in $EDITOR to have a bare '*' as
the ref glob:

	[svn-remote "svn"]
		url = http://svn.collab.net/repos/svn
		branches = *:refs/remotes/*

Since I've tried it, I suspect others may try editing $GIT_CONFIG in the
same way as well.

-- 
Eric Wong

Re: [bug][bisected] git-svn with root branches

[Eric Wong]

Adam Brewster <adambrewster@gmail.com> wrote:
> > I'm not sure why Adam decided the leading slash needed to be removed for
> > the git refspec.  That said, the globbing/branching code still makes me
> > want to hide under a rock and I'm generally afraid to touch it.
> > I'll wait for Adam to chime in since he's braver than I am :)
> >
> How's this for brave:  I'm not sure why I did that either.
> 
> Looking at it again it seems correct for the leading / to be ignored because
> it has no meaning.  What's the difference between the above config and
> "branches = *:refs/remotes/*" (besides the fact that one works and one
> doesn't)?  In both cases I think I've asked for all of the top-level
> directories to be branches.  But that has nothing to do with the rest of the
> patch, so it shouldn't have been included.

<snip>

> That is, continue disregard the / on the actual input because it means
> nothing, and add a / in places where the code will crash if it's not there.
> Even better would be to find out _why_ $path and $local_ref need a leading /
> and fix it that way, but that's more work that I don't have time for right
> now.

Actually, SVN 1.5+ does not like the leading "/" and trips on an
assertion failure on all cases except a standalone "/".  That is, "/*"
continues to work since we send "/" to SVN::Ra::get_log().

-- 
Eric Wong

[PATCH] git svn: fix fetch where glob is on the top-level URL

[Eric Wong]

>From 3abaf9fdf216fd0307bb9e9f03772bd80a64177c Mon Sep 17 00:00:00 2001
From: Adam Brewster <adambrewster@gmail.com>
Date: Thu, 22 Oct 2009 21:26:32 -0700
Subject: [PATCH] git svn: fix fetch where glob is on the top-level URL

In cases where the top-level URL we're tracking is the path we
glob against, we can once again track odd repositories that keep
branches/tags at the top level.  This regression was introduced
in commit 6f5748e14cc5bb0a836b649fb8e2d6a5eb166f1d.

Additionally, the leading slash is now optional when tracking
the top-level path to be consistent with non-top-level paths.
We now allow both of the following "branches" in [svn-remote
"foo"] sections of $GIT_CONFIG:

	; with a leading slash (this worked before 6f5748e1)
        branches = /*:refs/remotes/*

	; now it it also works without a leading slash
        branches = *:refs/remotes/*

Thanks to Daniel Cordero for the original bug report and
bisection.

[ew: commit message]

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---

  Daniel Cordero <theappleman@gmail.com> wrote:
  > Hello,
  > 
  > when trying to clone a svn repo with the command-line:
  > 
  > 	$ git svn clone -b / http://svn.collab.net/repos/svn/
  > 
  > (that is, each folder in the root of the repo should be considered it's
  > own branch)
  > the clone sometimes[1] fails saying:
  > 
  > 	ref: 'refs/remotes/' ends with a trailing slash, this is not permitted by git nor Subversion
  > 
  > The offending config is:
  > [svn-remote "svn"]
  >         url = http://svn.collab.net/repos/svn
  >         branches = /*:refs/remotes/*
  > 
  > 
  > This used to work in the past; I bisected the bad commit to
  > 
  > commit 6f5748e14cc5bb0a836b649fb8e2d6a5eb166f1d
  > Author: Adam Brewster <adambrewster@gmail.com>
  > Date:   Tue Aug 11 23:14:03 2009 -0400
  > 
  >     svn: allow branches outside of refs/remotes
  > 
  > 
  > Thanks in advance.
  > 
  > 
  > [1] It does work when the URL has at least 1 folder of depth
  > (e.g. suffix "trunk" to the above URL).
  > 
  > Its config section is:
  > [svn-remote "svn"]
  >         url = http://svn.collab.net/repos/svn
  > 	branches = trunk//*:refs/remotes/*
  > 

 git-svn.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index eb4b75a..2e9a720 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1768,7 +1768,7 @@ sub read_all_remotes {
 	my $svn_refspec = qr{\s*/?(.*?)\s*:\s*(.+?)\s*};
 	foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
 		if (m!^(.+)\.fetch=$svn_refspec$!) {
-			my ($remote, $local_ref, $remote_ref) = ($1, $2, $3);
+			my ($remote, $local_ref, $remote_ref) = ($1, "/$2", $3);
 			die("svn-remote.$remote: remote ref '$remote_ref' "
 			    . "must start with 'refs/'\n")
 				unless $remote_ref =~ m{^refs/};
@@ -1780,7 +1780,7 @@ sub read_all_remotes {
 			$r->{$1}->{url} = $2;
 		} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
 			my ($remote, $t, $local_ref, $remote_ref) =
-			                                     ($1, $2, $3, $4);
+			                                   ($1, $2, "/$3", $4);
 			die("svn-remote.$remote: remote ref '$remote_ref' ($t) "
 			    . "must start with 'refs/'\n")
 				unless $remote_ref =~ m{^refs/};
@@ -1980,7 +1980,7 @@ sub find_ref {
 	foreach (command(qw/config -l/)) {
 		next unless m!^svn-remote\.(.+)\.fetch=
 		              \s*/?(.*?)\s*:\s*(.+?)\s*$!x;
-		my ($repo_id, $path, $ref) = ($1, $2, $3);
+		my ($repo_id, $path, $ref) = ($1, "/$2", $3);
 		if ($ref eq $ref_id) {
 			$path = '' if ($path =~ m#^\./?#);
 			return ($repo_id, $path);
-- 
Eric Wong

Re: [PATCH] git svn: fix fetch where glob is on the top-level URL

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> From 3abaf9fdf216fd0307bb9e9f03772bd80a64177c Mon Sep 17 00:00:00 2001
> From: Adam Brewster <adambrewster@gmail.com>
> Date: Thu, 22 Oct 2009 21:26:32 -0700
> Subject: [PATCH] git svn: fix fetch where glob is on the top-level URL

Actually, no, something is broken here it seems...  Ugh, falling asleep :x

-- 
Eric Wong

Re: [PATCH] git svn: fix fetch where glob is on the top-level URL

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Eric Wong <normalperson@yhbt.net> wrote:
> > From 3abaf9fdf216fd0307bb9e9f03772bd80a64177c Mon Sep 17 00:00:00 2001
> > From: Adam Brewster <adambrewster@gmail.com>
> > Date: Thu, 22 Oct 2009 21:26:32 -0700
> > Subject: [PATCH] git svn: fix fetch where glob is on the top-level URL
> 
> Actually, no, something is broken here it seems...  Ugh, falling asleep :x

Oops, I was running the tests from the wrong machine that didn't have
the patch on that one.

As noted in my previous email, the leading slashes cause
SVN::Ra::get_log to fail with an assertion if the path passed to it
has a leading slash but is not a standalone slash.

Here's my original patch which seems to work fine (but doesn't allow the
more flexible, bare "*" in "branches = *:refs/remotes/*" Adam's one did.

>From 42079a567bef745996b6272ad546d682dfcf57d6 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Thu, 22 Oct 2009 23:39:04 -0700
Subject: [PATCH] git svn: fix fetch where glob is on the top-level URL

In cases where the top-level URL we're tracking is the path we
glob against, we can once again track odd repositories that keep
branches/tags at the top level.  This regression was introduced
in commit 6f5748e14cc5bb0a836b649fb8e2d6a5eb166f1d.

Thanks to Daniel Cordero for the original bug report and
bisection.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 git-svn.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index eb4b75a..56af221 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1765,7 +1765,7 @@ sub read_all_remotes {
 	my $use_svm_props = eval { command_oneline(qw/config --bool
 	    svn.useSvmProps/) };
 	$use_svm_props = $use_svm_props eq 'true' if $use_svm_props;
-	my $svn_refspec = qr{\s*/?(.*?)\s*:\s*(.+?)\s*};
+	my $svn_refspec = qr{\s*(.*?)\s*:\s*(.+?)\s*};
 	foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
 		if (m!^(.+)\.fetch=$svn_refspec$!) {
 			my ($remote, $local_ref, $remote_ref) = ($1, $2, $3);
@@ -1979,7 +1979,7 @@ sub find_ref {
 	my ($ref_id) = @_;
 	foreach (command(qw/config -l/)) {
 		next unless m!^svn-remote\.(.+)\.fetch=
-		              \s*/?(.*?)\s*:\s*(.+?)\s*$!x;
+		              \s*(.*?)\s*:\s*(.+?)\s*$!x;
 		my ($repo_id, $path, $ref) = ($1, $2, $3);
 		if ($ref eq $ref_id) {
 			$path = '' if ($path =~ m#^\./?#);
-- 
Eric Wong