[PATCH/RFC] svn manipulate refs ourside refs/remotes

[PATCH/RFC] svn manipulate refs ourside refs/remotes

[Adam Brewster]

I took some time this weekend and tried to allow git-svn to manipulate
branches outside of refs/remotes.  Along the way I came across a couple
of other issues that may or may not be bugs.

I'd appreciate it if somebody could take a look at the following
patches.  The first two are small and well described by their respective
commit messages.  The third has the bulk of the changes I made.

Note that make t/t9107-git-svn-migrate.sh fails when the first or third
patches are applied.  I can't tell if this is a false alarm or if the
code is really doing something it shouldn't.

 - Adam Brewster

[PATCH] svn: Add && to t9107-git-svn-migrarte.sh

[Adam Brewster]

It was probably intended for the test to fail unless all of the
commands succeed.
---
 t/t9107-git-svn-migrate.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 3a9e077..145a367 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -93,7 +93,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
 	grep "^branches/b:refs/remotes/b$" fetch.out &&
 	grep "^tags/0\.1:refs/remotes/tags/0\.1$" fetch.out &&
 	grep "^tags/0\.2:refs/remotes/tags/0\.2$" fetch.out &&
-	grep "^tags/0\.3:refs/remotes/tags/0\.3$" fetch.out
+	grep "^tags/0\.3:refs/remotes/tags/0\.3$" fetch.out &&
 	grep "^:refs/${remotes_git_svn}" fetch.out
 	'
 
-- 
1.6.0.6

[PATCH] svn: Honor --prefix option in init without --stdlayout

[Adam Brewster]

Most users who type

  git svn init file:///tmp/repo --prefix=my-svn/

would expect the root of the svn repository to be tracked by
refs/remotes/my-svn/git-svn.
---
 git-svn.perl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 9369acc..51e3f92 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3266,7 +3266,8 @@ sub _new {
 		$repo_id = $Git::SVN::default_repo_id;
 	}
 	unless (defined $ref_id && length $ref_id) {
-		$_[2] = $ref_id = $Git::SVN::default_ref_id;
+		$_prefix = '' unless defined($_prefix);
+		$_[2] = $ref_id = $_prefix . $Git::SVN::default_ref_id;
 	}
 	$_[1] = $repo_id;
 	my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
-- 
1.6.0.6

[PATCH] svn: allow git-svn to update remotes outside of refs/remotes

[Adam Brewster]

It may be convenient for some users to store svn remote tracking
branches outside of the refs/remotes/ heirarchy.  [1] and [2] indicate
that there is interest in making this possible.

[1]  http://www.gitready.com/advanced/2009/02/16/convert-git-svn-tag-branches-to-real-tags.html
[2] http://kerneltrap.org/mailarchive/git/2007/8/30/256359
---
 git-svn.perl                     |   75 ++++++++++++++++++++------------------
 t/lib-git-svn.sh                 |    2 +-
 t/t9104-git-svn-follow-parent.sh |   10 +++---
 t/t9107-git-svn-migrate.sh       |   14 +++----
 4 files changed, 51 insertions(+), 50 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 51e3f92..4d7175b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -883,7 +883,7 @@ sub cmd_multi_init {
 	}
 	do_git_init_db();
 	if (defined $_trunk) {
-		my $trunk_ref = $_prefix . 'trunk';
+		my $trunk_ref = 'refs/remotes/' . $_prefix . 'trunk';
 		# try both old-style and new-style lookups:
 		my $gs_trunk = eval { Git::SVN->new($trunk_ref) };
 		unless ($gs_trunk) {
@@ -1590,23 +1590,23 @@ sub resolve_local_globs {
 	return unless defined $glob_spec;
 	my $ref = $glob_spec->{ref};
 	my $path = $glob_spec->{path};
-	foreach (command(qw#for-each-ref --format=%(refname) refs/remotes#)) {
-		next unless m#^refs/remotes/$ref->{regex}$#;
+	foreach (command(qw#for-each-ref --format=%(refname) refs/#)) {
+		next unless m#^$ref->{regex}$#;
 		my $p = $1;
 		my $pathname = desanitize_refname($path->full_path($p));
 		my $refname = desanitize_refname($ref->full_path($p));
 		if (my $existing = $fetch->{$pathname}) {
 			if ($existing ne $refname) {
 				die "Refspec conflict:\n",
-				    "existing: refs/remotes/$existing\n",
-				    " globbed: refs/remotes/$refname\n";
+				    "existing: $existing\n",
+				    " globbed: $refname\n";
 			}
-			my $u = (::cmt_metadata("refs/remotes/$refname"))[0];
+			my $u = (::cmt_metadata("$refname"))[0];
 			$u =~ s!^\Q$url\E(/|$)!! or die
-			  "refs/remotes/$refname: '$url' not found in '$u'\n";
+			  "$refname: '$url' not found in '$u'\n";
 			if ($pathname ne $u) {
 				warn "W: Refspec glob conflict ",
-				     "(ref: refs/remotes/$refname):\n",
+				     "(ref: $refname):\n",
 				     "expected path: $pathname\n",
 				     "    real path: $u\n",
 				     "Continuing ahead with $u\n";
@@ -1684,33 +1684,34 @@ 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*};
 	foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
-		if (m!^(.+)\.fetch=\s*(.*)\s*:\s*(.+)\s*$!) {
-			my ($remote, $local_ref, $_remote_ref) = ($1, $2, $3);
-			die("svn-remote.$remote: remote ref '$_remote_ref' "
-			    . "must start with 'refs/remotes/'\n")
-				unless $_remote_ref =~ m{^refs/remotes/(.+)};
-			my $remote_ref = $1;
-			$local_ref =~ s{^/}{};
+		if (m!^(.+)\.fetch=$svn_refspec$!) {
+			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/};
 			$r->{$remote}->{fetch}->{$local_ref} = $remote_ref;
 			$r->{$remote}->{svm} = {} if $use_svm_props;
 		} elsif (m!^(.+)\.usesvmprops=\s*(.*)\s*$!) {
 			$r->{$1}->{svm} = {};
 		} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
 			$r->{$1}->{url} = $2;
-		} elsif (m!^(.+)\.(branches|tags)=
-		           (.*):refs/remotes/(.+)\s*$/!x) {
-			my ($p, $g) = ($3, $4);
+		} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
+			my ($remote, $t, $local_ref, $remote_ref) = ($1, $2, $3, $4);
+			die("svn-remote.$remote: remote ref '$remote_ref' ($t) "
+			    . "must start with 'refs/'\n")
+				unless $remote_ref =~ m{^refs/};
 			my $rs = {
-			    t => $2,
-			    remote => $1,
-			    path => Git::SVN::GlobSpec->new($p),
-			    ref => Git::SVN::GlobSpec->new($g) };
+			    t => $t,
+			    remote => $remote,
+			    path => Git::SVN::GlobSpec->new($local_ref),
+			    ref => Git::SVN::GlobSpec->new($remote_ref) };
 			if (length($rs->{ref}->{right}) != 0) {
 				die "The '*' glob character must be the last ",
-				    "character of '$g'\n";
+				    "character of '$remote_ref'\n";
 			}
-			push @{ $r->{$1}->{$2} }, $rs;
+			push @{ $r->{$remote}->{$t} }, $rs;
 		}
 	}
 
@@ -1818,9 +1819,9 @@ sub init_remote_config {
 		}
 	}
 	my ($xrepo_id, $xpath) = find_ref($self->refname);
-	if (defined $xpath) {
+	if (!$no_write && defined $xpath) {
 		die "svn-remote.$xrepo_id.fetch already set to track ",
-		    "$xpath:refs/remotes/", $self->refname, "\n";
+		    "$xpath:", $self->refname, "\n";
 	}
 	unless ($no_write) {
 		command_noisy('config',
@@ -1895,7 +1896,7 @@ sub find_ref {
 	my ($ref_id) = @_;
 	foreach (command(qw/config -l/)) {
 		next unless m!^svn-remote\.(.+)\.fetch=
-		              \s*(.*)\s*:\s*refs/remotes/(.+)\s*$!x;
+		              \s*/?(.*?)\s*:\s*(.+?)\s*$!x;
 		my ($repo_id, $path, $ref) = ($1, $2, $3);
 		if ($ref eq $ref_id) {
 			$path = '' if ($path =~ m#^\./?#);
@@ -1912,16 +1913,16 @@ sub new {
 		if (!defined $repo_id) {
 			die "Could not find a \"svn-remote.*.fetch\" key ",
 			    "in the repository configuration matching: ",
-			    "refs/remotes/$ref_id\n";
+			    "$ref_id\n";
 		}
 	}
 	my $self = _new($class, $repo_id, $ref_id, $path);
 	if (!defined $self->{path} || !length $self->{path}) {
 		my $fetch = command_oneline('config', '--get',
 		                            "svn-remote.$repo_id.fetch",
-		                            ":refs/remotes/$ref_id\$") or
+		                            ":$ref_id\$") or
 		     die "Failed to read \"svn-remote.$repo_id.fetch\" ",
-		         "\":refs/remotes/$ref_id\$\" in config\n";
+		         "\":$ref_id\$\" in config\n";
 		($self->{path}, undef) = split(/\s*:\s*/, $fetch);
 	}
 	$self->{url} = command_oneline('config', '--get',
@@ -1932,7 +1933,7 @@ sub new {
 }
 
 sub refname {
-	my ($refname) = "refs/remotes/$_[0]->{ref_id}" ;
+	my ($refname) = $_[0]->{ref_id} ;
 
 	# It cannot end with a slash /, we'll throw up on this because
 	# SVN can't have directories with a slash in their name, either:
@@ -3267,12 +3268,12 @@ sub _new {
 	}
 	unless (defined $ref_id && length $ref_id) {
 		$_prefix = '' unless defined($_prefix);
-		$_[2] = $ref_id = $_prefix . $Git::SVN::default_ref_id;
+		$_[2] = $ref_id = 'refs/remotes/' . $_prefix . $Git::SVN::default_ref_id;
 	}
 	$_[1] = $repo_id;
 	my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
 	$_[3] = $path = '' unless (defined $path);
-	mkpath(["$ENV{GIT_DIR}/svn"]);
+	mkpath(["$ENV{GIT_DIR}/svn/$ref_id"]);
 	bless {
 		ref_id => $ref_id, dir => $dir, index => "$dir/index",
 	        path => $path, config => "$ENV{GIT_DIR}/svn/config",
@@ -3301,7 +3302,9 @@ sub rev_db_path {
 sub map_path {
 	my ($self, $uuid) = @_;
 	$uuid ||= $self->ra_uuid;
-	"$self->{map_root}.$uuid";
+	my $map_path = "$self->{map_root}.$uuid";
+	( ! -d $map_path && $map_path =~ m{^refs/remotes/(.*)} && -d $1 ) ?
+		$1 : $map_path;
 }
 
 sub uri_encode {
@@ -5441,7 +5444,7 @@ sub minimize_connections {
 			my $pfx = "svn-remote.$x->{old_repo_id}";
 
 			my $old_fetch = quotemeta("$x->{old_path}:".
-			                          "refs/remotes/$x->{ref_id}");
+			                          "$x->{ref_id}");
 			command_noisy(qw/config --unset/,
 			              "$pfx.fetch", '^'. $old_fetch . '$');
 			delete $r->{$x->{old_repo_id}}->
@@ -5510,7 +5513,7 @@ sub new {
 	my ($class, $glob) = @_;
 	my $re = $glob;
 	$re =~ s!/+$!!g; # no need for trailing slashes
-	$re =~ m!^([^*]*)(\*(?:/\*)*)([^*]*)$!;
+	$re =~ m!^([^*]*)(\*(?:/\*)*)(.*)$!;
 	my $temp = $re;
 	my ($left, $right) = ($1, $3);
 	$re = $2;
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 5654962..fd8631f 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -14,7 +14,7 @@ if ! test_have_prereq PERL; then
 fi
 
 GIT_DIR=$PWD/.git
-GIT_SVN_DIR=$GIT_DIR/svn/git-svn
+GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn
 SVN_TREE=$GIT_SVN_DIR/svn-tree
 
 svn >/dev/null 2>&1
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index 78610b6..bbfd7f4 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -172,11 +172,11 @@ test_expect_success "follow-parent is atomic" '
 	git update-ref refs/remotes/flunk@18 refs/remotes/stunk~2 &&
 	git update-ref -d refs/remotes/stunk &&
 	git config --unset svn-remote.svn.fetch stunk &&
-	mkdir -p "$GIT_DIR"/svn/flunk@18 &&
-	rev_map=$(cd "$GIT_DIR"/svn/stunk && ls .rev_map*) &&
-	dd if="$GIT_DIR"/svn/stunk/$rev_map \
-	   of="$GIT_DIR"/svn/flunk@18/$rev_map bs=24 count=1 &&
-	rm -rf "$GIT_DIR"/svn/stunk &&
+	mkdir -p "$GIT_DIR"/svn/refs/remotes/flunk@18 &&
+	rev_map=$(cd "$GIT_DIR"/svn/refs/remotes/stunk && ls .rev_map*) &&
+	dd if="$GIT_DIR"/svn/refs/remotes/stunk/$rev_map \
+	   of="$GIT_DIR"/svn/refs/remotes/flunk@18/$rev_map bs=24 count=1 &&
+	rm -rf "$GIT_DIR"/svn/refs/remotes/stunk &&
 	git svn init --minimize-url -i flunk "$svnrepo"/flunk &&
 	git svn fetch -i flunk &&
 	git svn init --minimize-url -i stunk "$svnrepo"/stunk &&
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 145a367..3a84eb5 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -16,9 +16,7 @@ test_expect_success 'setup old-looking metadata' '
 		cd .. &&
 	git svn init "$svnrepo" &&
 	git svn fetch &&
-	mv "$GIT_DIR"/svn/* "$GIT_DIR"/ &&
-	mv "$GIT_DIR"/svn/.metadata "$GIT_DIR"/ &&
-	rmdir "$GIT_DIR"/svn &&
+	rm -rf "$GIT_DIR"/svn &&
 	git update-ref refs/heads/git-svn-HEAD refs/${remotes_git_svn} &&
 	git update-ref refs/heads/svn-HEAD refs/${remotes_git_svn} &&
 	git update-ref -d refs/${remotes_git_svn} refs/${remotes_git_svn}
@@ -79,7 +77,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
 	rm -rf "$GIT_DIR"/svn &&
 	for i in `cat fetch.out`; do
 		path=`expr $i : "\([^:]*\):.*$"`
-		ref=`expr $i : "[^:]*:refs/remotes/\(.*\)$"`
+		ref=`expr $i : "[^:]*:\(refs/remotes/.*\)$"`
 		if test -z "$ref"; then continue; fi
 		if test -n "$path"; then path="/$path"; fi
 		( mkdir -p "$GIT_DIR"/svn/$ref/info/ &&
@@ -99,16 +97,16 @@ test_expect_success 'migrate --minimize on old inited layout' '
 
 test_expect_success  ".rev_db auto-converted to .rev_map.UUID" '
 	git svn fetch -i trunk &&
-	test -z "$(ls "$GIT_DIR"/svn/trunk/.rev_db.* 2>/dev/null)" &&
-	expect="$(ls "$GIT_DIR"/svn/trunk/.rev_map.*)" &&
+	test -z "$(ls "$GIT_DIR"/svn/refs/remotes/trunk/.rev_db.* 2>/dev/null)" &&
+	expect="$(ls "$GIT_DIR"/svn/refs/remotes/trunk/.rev_map.*)" &&
 	test -n "$expect" &&
 	rev_db="$(echo $expect | sed -e "s,_map,_db,")" &&
 	convert_to_rev_db "$expect" "$rev_db" &&
 	rm -f "$expect" &&
 	test -f "$rev_db" &&
 	git svn fetch -i trunk &&
-	test -z "$(ls "$GIT_DIR"/svn/trunk/.rev_db.* 2>/dev/null)" &&
-	test ! -e "$GIT_DIR"/svn/trunk/.rev_db &&
+	test -z "$(ls "$GIT_DIR"/svn/refs/remotes/trunk/.rev_db.* 2>/dev/null)" &&
+	test ! -e "$GIT_DIR"/svn/refs/remotes/trunk/.rev_db &&
 	test -f "$expect"
 	'
 
-- 
1.6.0.6

Re: [PATCH] svn: allow git-svn to update remotes outside of refs/remotes

[Eric Wong]

Adam Brewster <adambrewster@gmail.com> wrote:
> It may be convenient for some users to store svn remote tracking
> branches outside of the refs/remotes/ heirarchy.  [1] and [2] indicate
> that there is interest in making this possible.
> 
> [1]  http://www.gitready.com/advanced/2009/02/16/convert-git-svn-tag-branches-to-real-tags.html
> [2] http://kerneltrap.org/mailarchive/git/2007/8/30/256359

I definitely agree with the direction of this...

We'll need a mechanism for migrating existing repos from
.git/svn/foo/* => .git/svn/refs/remotes/foo/* to avoid breaking
things for people with existing repos.  I guess that's why
you've been looking at (and helping fix) the migrate test :)

-- 
Eric Wong

Re: [PATCH] svn: Add && to t9107-git-svn-migrarte.sh

[Eric Wong]

Adam Brewster <adambrewster@gmail.com> wrote:
> It was probably intended for the test to fail unless all of the
> commands succeed.

I think you've uncovered some major breakage in this test that never got
noticed until now.  I'll try to take a harder look tonight or this
weekend.

Fortunately, this functionality is rarely needed these days :)
Thanks for the heads up!

> ---
>  t/t9107-git-svn-migrate.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
> index 3a9e077..145a367 100755
> --- a/t/t9107-git-svn-migrate.sh
> +++ b/t/t9107-git-svn-migrate.sh
> @@ -93,7 +93,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
>  	grep "^branches/b:refs/remotes/b$" fetch.out &&
>  	grep "^tags/0\.1:refs/remotes/tags/0\.1$" fetch.out &&
>  	grep "^tags/0\.2:refs/remotes/tags/0\.2$" fetch.out &&
> -	grep "^tags/0\.3:refs/remotes/tags/0\.3$" fetch.out
> +	grep "^tags/0\.3:refs/remotes/tags/0\.3$" fetch.out &&
>  	grep "^:refs/${remotes_git_svn}" fetch.out

Re: [PATCH] svn: Add && to t9107-git-svn-migrarte.sh

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Adam Brewster <adambrewster@gmail.com> wrote:
> > It was probably intended for the test to fail unless all of the
> > commands succeed.
> 
> I think you've uncovered some major breakage in this test that never got
> noticed until now.  I'll try to take a harder look tonight or this
> weekend.
> 
> Fortunately, this functionality is rarely needed these days :)
> Thanks for the heads up!

Actually, the test should look like this, I'll push it out
along with the other one in a few.

>From 4ebe6e92c3b62d5192b6cc8158fde701bf2c0dcb Mon Sep 17 00:00:00 2001
From: Adam Brewster <adambrewster@gmail.com>
Date: Mon, 3 Aug 2009 21:40:37 -0400
Subject: [PATCH] svn: Add && to t9107-git-svn-migrate.sh

It was probably intended for the test to fail unless all of the
commands succeed.

[ew: fixed tests to actually work]

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 t/t9107-git-svn-migrate.sh |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 3a9e077..c0098d9 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -56,7 +56,15 @@ test_expect_success 'initialize a multi-repository repo' '
 	git config --add svn-remote.svn.fetch "branches/b:refs/remotes/b" &&
 	for i in tags/0.1 tags/0.2 tags/0.3; do
 		git config --add svn-remote.svn.fetch \
-		                 $i:refs/remotes/$i || exit 1; done
+		                 $i:refs/remotes/$i || exit 1; done &&
+	git config --get-all svn-remote.svn.fetch > fetch.out &&
+	grep "^trunk:refs/remotes/trunk$" fetch.out &&
+	grep "^branches/a:refs/remotes/a$" fetch.out &&
+	grep "^branches/b:refs/remotes/b$" fetch.out &&
+	grep "^tags/0\.1:refs/remotes/tags/0\.1$" fetch.out &&
+	grep "^tags/0\.2:refs/remotes/tags/0\.2$" fetch.out &&
+	grep "^tags/0\.3:refs/remotes/tags/0\.3$" fetch.out &&
+	grep "^:refs/${remotes_git_svn}" fetch.out
 	'
 
 # refs should all be different, but the trees should all be the same:
@@ -86,14 +94,14 @@ test_expect_success 'migrate --minimize on old inited layout' '
 		echo "$svnrepo"$path > "$GIT_DIR"/svn/$ref/info/url ) || exit 1;
 	done &&
 	git svn migrate --minimize &&
-	test -z "`git config -l |grep -v "^svn-remote\.git-svn\."`" &&
+	test -z "`git config -l | grep "^svn-remote\.git-svn\."`" &&
 	git config --get-all svn-remote.svn.fetch > fetch.out &&
 	grep "^trunk:refs/remotes/trunk$" fetch.out &&
 	grep "^branches/a:refs/remotes/a$" fetch.out &&
 	grep "^branches/b:refs/remotes/b$" fetch.out &&
 	grep "^tags/0\.1:refs/remotes/tags/0\.1$" fetch.out &&
 	grep "^tags/0\.2:refs/remotes/tags/0\.2$" fetch.out &&
-	grep "^tags/0\.3:refs/remotes/tags/0\.3$" fetch.out
+	grep "^tags/0\.3:refs/remotes/tags/0\.3$" fetch.out &&
 	grep "^:refs/${remotes_git_svn}" fetch.out
 	'
 
-- 
Eric Wong

Re: [PATCH] svn: Add && to t9107-git-svn-migrarte.sh

[Adam Brewster]

Eric,

Any thoughts on the other patch?

Am I close or is there a better way to go about this?

I didn't really know what to do with .git/svn/*.  The easy answer is

  mkdir -p $GIT_DIR/svn/refs/remotes &&
  mv $GIT_DIR/svn/* $GIT_DIR/svn/refs/remotes

>From the comments in the Migration module, it seems like that's frowned
upon, so I came up with looking for .rev_map (or .rev_db) in both
locations (.git/svn/git-svn and .git/svn/refs/remotes/svn) and letting
it stay in whichever location it already exists.  (The next email has a
slightly improved version of the patch.)

This solution is particularly inelegant in it's handling of
unhandled.log, but as far as I know that file is unused.

There's also the problem of what to do if someone has a ref called
ref/remotes/refs/remotes/..., but that seems unlikely enough to not
cause concern.

Adam

[PATCH] svn: allow branches outside of refs/remotes

[Adam Brewster]

It may be convenient for some users to store svn remote tracking
branches outside of the refs/remotes/ heirarchy.

To accomplish this feat, this patch includes the entire path to
the ref in $r->{'refname'} in &read_all_remotes and tries to change
references to this entry so the new value makes sense.

Signed-off-by: Adam Brewster <adambrewster@gmail.com>
---
 git-svn.perl                     |   82 +++++++++++++++++++++----------------
 t/lib-git-svn.sh                 |    2 +-
 t/t9104-git-svn-follow-parent.sh |   10 ++--
 t/t9107-git-svn-migrate.sh       |   14 +++----
 t/t9143-git-svn-gc.sh            |   10 ++--
 5 files changed, 63 insertions(+), 55 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index b0bfb74..cafd7fe 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -909,7 +909,7 @@ sub cmd_multi_init {
 	}
 	do_git_init_db();
 	if (defined $_trunk) {
-		my $trunk_ref = $_prefix . 'trunk';
+		my $trunk_ref = 'refs/remotes/' . $_prefix . 'trunk';
 		# try both old-style and new-style lookups:
 		my $gs_trunk = eval { Git::SVN->new($trunk_ref) };
 		unless ($gs_trunk) {
@@ -1643,23 +1643,23 @@ sub resolve_local_globs {
 	return unless defined $glob_spec;
 	my $ref = $glob_spec->{ref};
 	my $path = $glob_spec->{path};
-	foreach (command(qw#for-each-ref --format=%(refname) refs/remotes#)) {
-		next unless m#^refs/remotes/$ref->{regex}$#;
+	foreach (command(qw#for-each-ref --format=%(refname) refs/#)) {
+		next unless m#^$ref->{regex}$#;
 		my $p = $1;
 		my $pathname = desanitize_refname($path->full_path($p));
 		my $refname = desanitize_refname($ref->full_path($p));
 		if (my $existing = $fetch->{$pathname}) {
 			if ($existing ne $refname) {
 				die "Refspec conflict:\n",
-				    "existing: refs/remotes/$existing\n",
-				    " globbed: refs/remotes/$refname\n";
+				    "existing: $existing\n",
+				    " globbed: $refname\n";
 			}
-			my $u = (::cmt_metadata("refs/remotes/$refname"))[0];
+			my $u = (::cmt_metadata("$refname"))[0];
 			$u =~ s!^\Q$url\E(/|$)!! or die
-			  "refs/remotes/$refname: '$url' not found in '$u'\n";
+			  "$refname: '$url' not found in '$u'\n";
 			if ($pathname ne $u) {
 				warn "W: Refspec glob conflict ",
-				     "(ref: refs/remotes/$refname):\n",
+				     "(ref: $refname):\n",
 				     "expected path: $pathname\n",
 				     "    real path: $u\n",
 				     "Continuing ahead with $u\n";
@@ -1737,33 +1737,34 @@ 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*};
 	foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
-		if (m!^(.+)\.fetch=\s*(.*)\s*:\s*(.+)\s*$!) {
-			my ($remote, $local_ref, $_remote_ref) = ($1, $2, $3);
-			die("svn-remote.$remote: remote ref '$_remote_ref' "
-			    . "must start with 'refs/remotes/'\n")
-				unless $_remote_ref =~ m{^refs/remotes/(.+)};
-			my $remote_ref = $1;
-			$local_ref =~ s{^/}{};
+		if (m!^(.+)\.fetch=$svn_refspec$!) {
+			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/};
 			$r->{$remote}->{fetch}->{$local_ref} = $remote_ref;
 			$r->{$remote}->{svm} = {} if $use_svm_props;
 		} elsif (m!^(.+)\.usesvmprops=\s*(.*)\s*$!) {
 			$r->{$1}->{svm} = {};
 		} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
 			$r->{$1}->{url} = $2;
-		} elsif (m!^(.+)\.(branches|tags)=
-		           (.*):refs/remotes/(.+)\s*$/!x) {
-			my ($p, $g) = ($3, $4);
+		} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
+			my ($remote, $t, $local_ref, $remote_ref) = ($1, $2, $3, $4);
+			die("svn-remote.$remote: remote ref '$remote_ref' ($t) "
+			    . "must start with 'refs/'\n")
+				unless $remote_ref =~ m{^refs/};
 			my $rs = {
-			    t => $2,
-			    remote => $1,
-			    path => Git::SVN::GlobSpec->new($p),
-			    ref => Git::SVN::GlobSpec->new($g) };
+			    t => $t,
+			    remote => $remote,
+			    path => Git::SVN::GlobSpec->new($local_ref),
+			    ref => Git::SVN::GlobSpec->new($remote_ref) };
 			if (length($rs->{ref}->{right}) != 0) {
 				die "The '*' glob character must be the last ",
-				    "character of '$g'\n";
+				    "character of '$remote_ref'\n";
 			}
-			push @{ $r->{$1}->{$2} }, $rs;
+			push @{ $r->{$remote}->{$t} }, $rs;
 		}
 	}
 
@@ -1871,9 +1872,9 @@ sub init_remote_config {
 		}
 	}
 	my ($xrepo_id, $xpath) = find_ref($self->refname);
-	if (defined $xpath) {
+	if (!$no_write && defined $xpath) {
 		die "svn-remote.$xrepo_id.fetch already set to track ",
-		    "$xpath:refs/remotes/", $self->refname, "\n";
+		    "$xpath:", $self->refname, "\n";
 	}
 	unless ($no_write) {
 		command_noisy('config',
@@ -1948,7 +1949,7 @@ sub find_ref {
 	my ($ref_id) = @_;
 	foreach (command(qw/config -l/)) {
 		next unless m!^svn-remote\.(.+)\.fetch=
-		              \s*(.*)\s*:\s*refs/remotes/(.+)\s*$!x;
+		              \s*/?(.*?)\s*:\s*(.+?)\s*$!x;
 		my ($repo_id, $path, $ref) = ($1, $2, $3);
 		if ($ref eq $ref_id) {
 			$path = '' if ($path =~ m#^\./?#);
@@ -1965,16 +1966,16 @@ sub new {
 		if (!defined $repo_id) {
 			die "Could not find a \"svn-remote.*.fetch\" key ",
 			    "in the repository configuration matching: ",
-			    "refs/remotes/$ref_id\n";
+			    "$ref_id\n";
 		}
 	}
 	my $self = _new($class, $repo_id, $ref_id, $path);
 	if (!defined $self->{path} || !length $self->{path}) {
 		my $fetch = command_oneline('config', '--get',
 		                            "svn-remote.$repo_id.fetch",
-		                            ":refs/remotes/$ref_id\$") or
+		                            ":$ref_id\$") or
 		     die "Failed to read \"svn-remote.$repo_id.fetch\" ",
-		         "\":refs/remotes/$ref_id\$\" in config\n";
+		         "\":$ref_id\$\" in config\n";
 		($self->{path}, undef) = split(/\s*:\s*/, $fetch);
 	}
 	$self->{url} = command_oneline('config', '--get',
@@ -1985,7 +1986,7 @@ sub new {
 }
 
 sub refname {
-	my ($refname) = "refs/remotes/$_[0]->{ref_id}" ;
+	my ($refname) = $_[0]->{ref_id} ;
 
 	# It cannot end with a slash /, we'll throw up on this because
 	# SVN can't have directories with a slash in their name, either:
@@ -3320,12 +3321,12 @@ sub _new {
 	}
 	unless (defined $ref_id && length $ref_id) {
 		$_prefix = '' unless defined($_prefix);
-		$_[2] = $ref_id = $_prefix . $Git::SVN::default_ref_id;
+		$_[2] = $ref_id = 'refs/remotes/' . $_prefix . $Git::SVN::default_ref_id;
 	}
 	$_[1] = $repo_id;
 	my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
 	$_[3] = $path = '' unless (defined $path);
-	mkpath(["$ENV{GIT_DIR}/svn"]);
+	mkpath(["$ENV{GIT_DIR}/svn/$ref_id"]);
 	bless {
 		ref_id => $ref_id, dir => $dir, index => "$dir/index",
 	        path => $path, config => "$ENV{GIT_DIR}/svn/config",
@@ -3354,7 +3355,16 @@ sub rev_db_path {
 sub map_path {
 	my ($self, $uuid) = @_;
 	$uuid ||= $self->ra_uuid;
-	"$self->{map_root}.$uuid";
+	my $map_path = "$self->{map_root}.$uuid";
+	return $map_path if ( -f $map_path );
+
+	my $db_path = $map_path;
+	$db_path =~ s/\.rev_map/.rev_db/;
+	return $map_path if ( -f $db_path );
+
+	return $1 if ($map_path =~ m(refs/remotes/(.*)) && -f $1);
+	return $1 if ($db_path =~ m(refs/remotes/(.*)) && -f $1);
+	return $map_path;
 }
 
 sub uri_encode {
@@ -5498,7 +5508,7 @@ sub minimize_connections {
 			my $pfx = "svn-remote.$x->{old_repo_id}";
 
 			my $old_fetch = quotemeta("$x->{old_path}:".
-			                          "refs/remotes/$x->{ref_id}");
+			                          "$x->{ref_id}");
 			command_noisy(qw/config --unset/,
 			              "$pfx.fetch", '^'. $old_fetch . '$');
 			delete $r->{$x->{old_repo_id}}->
@@ -5567,7 +5577,7 @@ sub new {
 	my ($class, $glob) = @_;
 	my $re = $glob;
 	$re =~ s!/+$!!g; # no need for trailing slashes
-	$re =~ m!^([^*]*)(\*(?:/\*)*)([^*]*)$!;
+	$re =~ m!^([^*]*)(\*(?:/\*)*)(.*)$!;
 	my $temp = $re;
 	my ($left, $right) = ($1, $3);
 	$re = $2;
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 5654962..fd8631f 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -14,7 +14,7 @@ if ! test_have_prereq PERL; then
 fi
 
 GIT_DIR=$PWD/.git
-GIT_SVN_DIR=$GIT_DIR/svn/git-svn
+GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn
 SVN_TREE=$GIT_SVN_DIR/svn-tree
 
 svn >/dev/null 2>&1
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index 78610b6..bbfd7f4 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -172,11 +172,11 @@ test_expect_success "follow-parent is atomic" '
 	git update-ref refs/remotes/flunk@18 refs/remotes/stunk~2 &&
 	git update-ref -d refs/remotes/stunk &&
 	git config --unset svn-remote.svn.fetch stunk &&
-	mkdir -p "$GIT_DIR"/svn/flunk@18 &&
-	rev_map=$(cd "$GIT_DIR"/svn/stunk && ls .rev_map*) &&
-	dd if="$GIT_DIR"/svn/stunk/$rev_map \
-	   of="$GIT_DIR"/svn/flunk@18/$rev_map bs=24 count=1 &&
-	rm -rf "$GIT_DIR"/svn/stunk &&
+	mkdir -p "$GIT_DIR"/svn/refs/remotes/flunk@18 &&
+	rev_map=$(cd "$GIT_DIR"/svn/refs/remotes/stunk && ls .rev_map*) &&
+	dd if="$GIT_DIR"/svn/refs/remotes/stunk/$rev_map \
+	   of="$GIT_DIR"/svn/refs/remotes/flunk@18/$rev_map bs=24 count=1 &&
+	rm -rf "$GIT_DIR"/svn/refs/remotes/stunk &&
 	git svn init --minimize-url -i flunk "$svnrepo"/flunk &&
 	git svn fetch -i flunk &&
 	git svn init --minimize-url -i stunk "$svnrepo"/stunk &&
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index c0098d9..901b8e0 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -16,9 +16,7 @@ test_expect_success 'setup old-looking metadata' '
 		cd .. &&
 	git svn init "$svnrepo" &&
 	git svn fetch &&
-	mv "$GIT_DIR"/svn/* "$GIT_DIR"/ &&
-	mv "$GIT_DIR"/svn/.metadata "$GIT_DIR"/ &&
-	rmdir "$GIT_DIR"/svn &&
+	rm -rf "$GIT_DIR"/svn &&
 	git update-ref refs/heads/git-svn-HEAD refs/${remotes_git_svn} &&
 	git update-ref refs/heads/svn-HEAD refs/${remotes_git_svn} &&
 	git update-ref -d refs/${remotes_git_svn} refs/${remotes_git_svn}
@@ -87,7 +85,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
 	rm -rf "$GIT_DIR"/svn &&
 	for i in `cat fetch.out`; do
 		path=`expr $i : "\([^:]*\):.*$"`
-		ref=`expr $i : "[^:]*:refs/remotes/\(.*\)$"`
+		ref=`expr $i : "[^:]*:\(refs/remotes/.*\)$"`
 		if test -z "$ref"; then continue; fi
 		if test -n "$path"; then path="/$path"; fi
 		( mkdir -p "$GIT_DIR"/svn/$ref/info/ &&
@@ -107,16 +105,16 @@ test_expect_success 'migrate --minimize on old inited layout' '
 
 test_expect_success  ".rev_db auto-converted to .rev_map.UUID" '
 	git svn fetch -i trunk &&
-	test -z "$(ls "$GIT_DIR"/svn/trunk/.rev_db.* 2>/dev/null)" &&
-	expect="$(ls "$GIT_DIR"/svn/trunk/.rev_map.*)" &&
+	test -z "$(ls "$GIT_DIR"/svn/refs/remotes/trunk/.rev_db.* 2>/dev/null)" &&
+	expect="$(ls "$GIT_DIR"/svn/refs/remotes/trunk/.rev_map.*)" &&
 	test -n "$expect" &&
 	rev_db="$(echo $expect | sed -e "s,_map,_db,")" &&
 	convert_to_rev_db "$expect" "$rev_db" &&
 	rm -f "$expect" &&
 	test -f "$rev_db" &&
 	git svn fetch -i trunk &&
-	test -z "$(ls "$GIT_DIR"/svn/trunk/.rev_db.* 2>/dev/null)" &&
-	test ! -e "$GIT_DIR"/svn/trunk/.rev_db &&
+	test -z "$(ls "$GIT_DIR"/svn/refs/remotes/trunk/.rev_db.* 2>/dev/null)" &&
+	test ! -e "$GIT_DIR"/svn/refs/remotes/trunk/.rev_db &&
 	test -f "$expect"
 	'
 
diff --git a/t/t9143-git-svn-gc.sh b/t/t9143-git-svn-gc.sh
index f2ba2d1..99f69c6 100755
--- a/t/t9143-git-svn-gc.sh
+++ b/t/t9143-git-svn-gc.sh
@@ -28,26 +28,26 @@ test_expect_success 'Setup repo' 'git svn init "$svnrepo"'
 test_expect_success 'Fetch repo' 'git svn fetch'
 
 test_expect_success 'make backup copy of unhandled.log' '
-	 cp .git/svn/git-svn/unhandled.log tmp
+	 cp .git/svn/refs/remotes/git-svn/unhandled.log tmp
 	'
 
-test_expect_success 'create leftover index' '> .git/svn/git-svn/index'
+test_expect_success 'create leftover index' '> .git/svn/refs/remotes/git-svn/index'
 
 test_expect_success 'git svn gc runs' 'git svn gc'
 
-test_expect_success 'git svn index removed' '! test -f .git/svn/git-svn/index'
+test_expect_success 'git svn index removed' '! test -f .git/svn/refs/remotes/git-svn/index'
 
 if perl -MCompress::Zlib -e 0 2>/dev/null
 then
 	test_expect_success 'git svn gc produces a valid gzip file' '
-		 gunzip .git/svn/git-svn/unhandled.log.gz
+		 gunzip .git/svn/refs/remotes/git-svn/unhandled.log.gz
 		'
 else
 	say "Perl Compress::Zlib unavailable, skipping gunzip test"
 fi
 
 test_expect_success 'git svn gc does not change unhandled.log files' '
-	 test_cmp .git/svn/git-svn/unhandled.log tmp/unhandled.log
+	 test_cmp .git/svn/refs/remotes/git-svn/unhandled.log tmp/unhandled.log
 	'
 
 test_done
-- 
1.6.0.6

Re: [PATCH] svn: Add && to t9107-git-svn-migrarte.sh

[Eric Wong]

Adam Brewster <adambrewster@gmail.com> wrote:
> Eric,
> 
> Any thoughts on the other patch?
> 
> Am I close or is there a better way to go about this?
> 
> I didn't really know what to do with .git/svn/*.  The easy answer is
> 
>   mkdir -p $GIT_DIR/svn/refs/remotes &&
>   mv $GIT_DIR/svn/* $GIT_DIR/svn/refs/remotes

That should be acceptable.  I don't think it's worth the trouble to
force the majority of existing repos to move over to the new layout
(in case the user wants to switch between multiple versions of git).

> From the comments in the Migration module, it seems like that's frowned
> upon, so I came up with looking for .rev_map (or .rev_db) in both
> locations (.git/svn/git-svn and .git/svn/refs/remotes/svn) and letting
> it stay in whichever location it already exists.  (The next email has a
> slightly improved version of the patch.)
> 
> This solution is particularly inelegant in it's handling of
> unhandled.log, but as far as I know that file is unused.

I'd still rather avoid creating unhandled.log in two places and
having an unused set of directories.

I think the following change on top of yours would make most
sense:

diff --git a/git-svn.perl b/git-svn.perl
index cafd7fe..1117f28 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3325,8 +3325,18 @@ sub _new {
 	}
 	$_[1] = $repo_id;
 	my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
+
+	# Older repos imported by us used $GIT_DIR/svn/foo instead of
+	# $GIT_DIR/svn/refs/remotes/foo when tracking refs/remotes/foo
+	if ($ref_id =~ m{^refs/remotes/(.*)}) {
+		my $old_dir = "$ENV{GIT_DIR}/svn/$1";
+		if (-d $old_dir && ! -d $dir) {
+			$dir = $old_dir;
+		}
+	}
+
 	$_[3] = $path = '' unless (defined $path);
-	mkpath(["$ENV{GIT_DIR}/svn/$ref_id"]);
+	mkpath([$dir]);
 	bless {
 		ref_id => $ref_id, dir => $dir, index => "$dir/index",
 	        path => $path, config => "$ENV{GIT_DIR}/svn/config",
@@ -3355,16 +3365,7 @@ sub rev_db_path {
 sub map_path {
 	my ($self, $uuid) = @_;
 	$uuid ||= $self->ra_uuid;
-	my $map_path = "$self->{map_root}.$uuid";
-	return $map_path if ( -f $map_path );
-
-	my $db_path = $map_path;
-	$db_path =~ s/\.rev_map/.rev_db/;
-	return $map_path if ( -f $db_path );
-
-	return $1 if ($map_path =~ m(refs/remotes/(.*)) && -f $1);
-	return $1 if ($db_path =~ m(refs/remotes/(.*)) && -f $1);
-	return $map_path;
+	"$self->{map_root}.$uuid";
 }
 
 sub uri_encode {
----

The map_path() changes you originally made didn't work, either, since
the -f $1 never took GIT_DIR or GIT_DIR/svn into account.

> There's also the problem of what to do if someone has a ref called
> ref/remotes/refs/remotes/..., but that seems unlikely enough to not
> cause concern.

I concur.  There's always cases where things will be ambiguous
and cases where a user is just asking for trouble :)

I think the below is a good enough test case to for compatibility
against existing repos.  Let me know what you think, thanks!

diff --git a/t/t9144-git-svn-old-rev_map.sh b/t/t9144-git-svn-old-rev_map.sh
new file mode 100755
index 0000000..7600a35
--- /dev/null
+++ b/t/t9144-git-svn-old-rev_map.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Eric Wong
+
+test_description='git svn old rev_map preservd'
+. ./lib-git-svn.sh
+
+test_expect_success 'setup test repository with old layout' '
+	mkdir i &&
+	(cd i && > a) &&
+	svn_cmd import -m- i "$svnrepo" &&
+	git svn init "$svnrepo" &&
+	git svn fetch &&
+	test -d .git/svn/refs/remotes/git-svn/ &&
+	! test -e .git/svn/git-svn/ &&
+	mv .git/svn/refs/remotes/git-svn .git/svn/ &&
+	rm -r .git/svn/refs
+'
+
+test_expect_success 'old layout continues to work' '
+	svn_cmd import -m- i "$svnrepo/b" &&
+	git svn rebase &&
+	echo a >> b/a &&
+	git add b/a &&
+	git commit -m- -a &&
+	git svn dcommit &&
+	! test -d .git/svn/refs/ &&
+	test -e .git/svn/git-svn/
+'
+
+test_done
-- 
Eric Wong

Re: [PATCH] svn: Add && to t9107-git-svn-migrarte.sh

[Adam Brewster]

Eric,

Thanks the help in getting this right.

> I think the following change on top of yours would make most
> sense:
>

Looks good to me.

> The map_path() changes you originally made didn't work, either, since
> the -f $1 never took GIT_DIR or GIT_DIR/svn into account.
>

It was ugly, too.

> I think the below is a good enough test case to for compatibility
> against existing repos.  Let me know what you think, thanks!
>

Looks about right.

Thanks,
Adam

Re: [PATCH] svn: Add && to t9107-git-svn-migrarte.sh

[Eric Wong]

Adam Brewster <adambrewster@gmail.com> wrote:
> Eric,
> 
> Thanks the help in getting this right.

No problem!  Thanks for fixing a long-standing issue with git svn.

I've pushed this last patch out along with a few others to
git://yhbt.net/git-svn for Junio

-- 
Eric Wong