threads / discuss / 31117

Make git-svn Use accessors for paths and urls

Subject: Make git-svn Use accessors for paths and urls

## tl;dr

18 messages between Jul 27, 2012 and Sep 18, 2012.

replies: 17people: 4as markdown or json

Michael G. Schwern· Jul 27, 2012, 20:00 UTC · lore

This patch series gives Git::SVN and Git::SVN::Ra accessors for path and url and then makes the rest of the code use them, rather than grab at $obj->{path} and $obj->{url}. This then will give us the control necessary to canonicalize them as early as possible (done in the next patch series).

There are plenty of other places in the code which will benefit from accessors and functions, but those will come later. path and url were the most obvious.

This is a refactoring and has no functional change. All git-svn tests pass with SVN 1.6 for each patch.

This goes on top of my previous patch series to extract other classes. That hasn't been reviewed yet, but both that and this are a simple patch series and I figure we can review a bit ahead.

This is the last refactoring patch series. After this bugs, start getting fixed.

Michael G. Schwern· Jul 27, 2012, 20:00 UTC · re: Michael G. Schwern · lore

[PATCH 1/5] Make Git::SVN use accessors internally for path.

From: "Michael G. Schwern" <schwern@pobox.com>

Then later it can be canonicalized automatically rather than everywhere its used.

Later patch will make other things use it.
---
 perl/Git/SVN.pm | 87 +++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 56 insertions(+), 31 deletions(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index b8b3474..0aff9d0 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -314,12 +314,12 @@ sub init_remote_config {
 				print STDERR "Using higher level of URL: ",
 					     "$url => $min_url\n";
 			}
-			my $old_path = $self->{path};
-			$self->{path} = $url;
-			$self->{path} =~ s!^\Q$min_url\E(/|$)!!;
+			my $old_path = $self->path;
+			$url =~ s!^\Q$min_url\E(/|$)!!;
 			if (length $old_path) {
-				$self->{path} .= "/$old_path";
+				$url .= "/$old_path";
 			}
+			$self->path($url);
 			$url = $min_url;
 		}
 	}
@@ -343,11 +343,13 @@ sub init_remote_config {
 	unless ($no_write) {
 		command_noisy('config',
 			      "svn-remote.$self->{repo_id}.url", $url);
-		$self->{path} =~ s{^/}{};
-		$self->{path} =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
+		my $path = $self->path;
+		$path =~ s{^/}{};
+		$path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
+		$self->path($path);
 		command_noisy('config', '--add',
 			      "svn-remote.$self->{repo_id}.fetch",
-			      "$self->{path}:".$self->refname);
+			      $self->path.":".$self->refname);
 	}
 	$self->{url} = $url;
 }
@@ -435,17 +437,22 @@ sub new {
 		}
 	}
 	my $self = _new($class, $repo_id, $ref_id, $path);
-	if (!defined $self->{path} || !length $self->{path}) {
+	if (!defined $self->path || !length $self->path) {
 		my $fetch = command_oneline('config', '--get',
 		                            "svn-remote.$repo_id.fetch",
 		                            ":$ref_id\$") or
 		     die "Failed to read \"svn-remote.$repo_id.fetch\" ",
 		         "\":$ref_id\$\" in config\n";
-		($self->{path}, undef) = split(/\s*:\s*/, $fetch);
+		my($path) = split(/\s*:\s*/, $fetch);
+		$self->path($path);
+	}
+	{
+		my $path = $self->path;
+		$path =~ s{/+}{/}g;
+		$path =~ s{\A/}{};
+		$path =~ s{/\z}{};
+		$self->path($path);
 	}
-	$self->{path} =~ s{/+}{/}g;
-	$self->{path} =~ s{\A/}{};
-	$self->{path} =~ s{/\z}{};
 	$self->{url} = command_oneline('config', '--get',
 	                               "svn-remote.$repo_id.url") or
                   die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
@@ -567,7 +574,7 @@ sub _set_svm_vars {
 	}
 
 	my $r = $ra->get_latest_revnum;
-	my $path = $self->{path};
+	my $path = $self->path;
 	my %tried;
 	while (length $path) {
 		unless ($tried{"$self->{url}/$path"}) {
@@ -728,7 +735,7 @@ sub prop_walk {
 	$path =~ s#^/*#/#g;
 	my $p = $path;
 	# Strip the irrelevant part of the path.
-	$p =~ s#^/+\Q$self->{path}\E(/|$)#/#;
+	$p =~ s#^/+\Q@{[$self->path]}\E(/|$)#/#;
 	# Ensure the path is terminated by a `/'.
 	$p =~ s#/*$#/#;
 
@@ -749,7 +756,7 @@ sub prop_walk {
 
 	foreach (sort keys %$dirent) {
 		next if $dirent->{$_}->{kind} != $SVN::Node::dir;
-		$self->prop_walk($self->{path} . $p . $_, $rev, $sub);
+		$self->prop_walk($self->path . $p . $_, $rev, $sub);
 	}
 }
 
@@ -920,19 +927,19 @@ sub rewrite_uuid {
 sub metadata_url {
 	my ($self) = @_;
 	($self->rewrite_root || $self->{url}) .
-	   (length $self->{path} ? '/' . $self->{path} : '');
+	   (length $self->path ? '/' . $self->path : '');
 }
 
 sub full_url {
 	my ($self) = @_;
-	$self->{url} . (length $self->{path} ? '/' . $self->{path} : '');
+	$self->{url} . (length $self->path ? '/' . $self->path : '');
 }
 
 sub full_pushurl {
 	my ($self) = @_;
 	if ($self->{pushurl}) {
-		return $self->{pushurl} . (length $self->{path} ? '/' .
-		       $self->{path} : '');
+		return $self->{pushurl} . (length $self->path ? '/' .
+		       $self->path : '');
 	} else {
 		return $self->full_url;
 	}
@@ -1048,20 +1055,20 @@ sub do_git_commit {
 
 sub match_paths {
 	my ($self, $paths, $r) = @_;
-	return 1 if $self->{path} eq '';
-	if (my $path = $paths->{"/$self->{path}"}) {
+	return 1 if $self->path eq '';
+	if (my $path = $paths->{"/".$self->path}) {
 		return ($path->{action} eq 'D') ? 0 : 1;
 	}
-	$self->{path_regex} ||= qr/^\/\Q$self->{path}\E\//;
+	$self->{path_regex} ||= qr{^/\Q@{[$self->path]}\E/};
 	if (grep /$self->{path_regex}/, keys %$paths) {
 		return 1;
 	}
 	my $c = '';
-	foreach (split m#/#, $self->{path}) {
+	foreach (split m#/#, $self->path) {
 		$c .= "/$_";
 		next unless ($paths->{$c} &&
 		             ($paths->{$c}->{action} =~ /^[AR]$/));
-		if ($self->ra->check_path($self->{path}, $r) ==
+		if ($self->ra->check_path($self->path, $r) ==
 		    $SVN::Node::dir) {
 			return 1;
 		}
@@ -1075,14 +1082,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,
+		$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->{path};
+	my @b_path_components = split m#/#, $self->path;
 	my @a_path_components;
 	my $i;
 	while (@b_path_components) {
@@ -1235,7 +1242,7 @@ sub mkemptydirs {
 		close $fh;
 	}
 
-	my $strip = qr/\A\Q$self->{path}\E(?:\/|$)/;
+	my $strip = qr/\A\Q@{[$self->path]}\E(?:\/|$)/;
 	foreach my $d (sort keys %empty_dirs) {
 		$d = uri_decode($d);
 		$d =~ s/$strip//;
@@ -1858,7 +1865,7 @@ sub make_log_entry {
 		$commit_email ||= "$author\@$uuid";
 	} elsif ($self->use_svnsync_props) {
 		my $full_url = $self->svnsync->{url};
-		$full_url .= "/$self->{path}" if length $self->{path};
+		$full_url .= "/".$self->path if length $self->path;
 		remove_username($full_url);
 		my $uuid = $self->svnsync->{uuid};
 		$log_entry{metadata} = "$full_url\@$rev $uuid";
@@ -1905,7 +1912,7 @@ sub set_tree {
 	                tree_b => $tree,
 	                editor_cb => sub {
 			       $self->set_tree_cb($log_entry, $tree, @_) },
-	                svn_path => $self->{path} );
+	                svn_path => $self->path );
 	if (!Git::SVN::Editor->new(\%ed_opts)->apply_diff) {
 		print "No changes\nr$self->{last_rev} = $tree\n";
 	}
@@ -2276,10 +2283,28 @@ sub _new {
 
 	$_[3] = $path = '' unless (defined $path);
 	mkpath([$dir]);
-	bless {
+	my $obj = bless {
 		ref_id => $ref_id, dir => $dir, index => "$dir/index",
-	        path => $path, config => "$ENV{GIT_DIR}/svn/config",
+	        config => "$ENV{GIT_DIR}/svn/config",
 	        map_root => "$dir/.rev_map", repo_id => $repo_id }, $class;
+
+	# Ensure it gets canonicalized
+	$obj->path($path);
+
+	return $obj;
+}
+
+
+sub path {
+    my $self = shift;
+
+    if( @_ ) {
+        my $path = shift;
+        $self->{path} = $path;
+        return;
+    }
+
+    return $self->{path};
 }
 
 # for read-only access of old .rev_db formats

-- 
1.7.11.3

Jonathan Nieder· Sep 17, 2012, 09:04 UTC · re: Michael G. Schwern · lore

Re: [PATCH 1/5] Make Git::SVN use accessors internally for path.

Hi Eric et al,

Michael G. Schwern wrote:

> Then later it can be canonicalized automatically rather than everywhere
> its used.
>
> Later patch will make other things use it.

Wow am I slow. I've finally got around to starting to parse these patches to apply to a 1.7.10.y tree so they can (hopefully) be part of Debian 7.0 when it comes out.

Do I understand correctly that this patch splits logically into the following steps? The result is only cosmetically different from the original patch --- interdiff below the shortlog.

The completeness of the conversion to accessors is checked by renaming the underlying variable in patch 5.

Jonathan Nieder (1):
  Git::SVN: rename private path field

Michael G. Schwern (4):
  Git::SVN: introduce path accessor
  Git::SVN: use accessor to read path
  Git::SVN: use accessor to write path
  Git::SVN::_new: use accessor to write path field

 git-svn.perl            |   12 +++----
 perl/Git/SVN.pm         |   84 ++++++++++++++++++++++++++++-------------------
 perl/Git/SVN/Fetcher.pm |    2 +-
 perl/Git/SVN/Ra.pm      |    8 ++---
 4 files changed, 62 insertions(+), 44 deletions(-)

---

diff --git c/git-svn.perl w/git-svn.perl
index 5711c571..af7d5308 100755
--- c/git-svn.perl
+++ w/git-svn.perl
@@ -1195,7 +1195,7 @@ sub cmd_show_ignore {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		print STDOUT "\n# $path\n";
 		my $s = $props->{'svn:ignore'} or return;
@@ -1211,7 +1211,7 @@ sub cmd_show_externals {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		print STDOUT "\n# $path\n";
 		my $s = $props->{'svn:externals'} or return;
@@ -1226,7 +1226,7 @@ sub cmd_create_ignore {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		# $path is of the form /path/to/dir/
 		$path = '.' . $path;
@@ -1294,7 +1294,7 @@ sub get_svnprops {
 	$path = $cmd_dir_prefix . $path;
 	fatal("No such file or directory: $path") unless -e $path;
 	my $is_dir = -d $path ? 1 : 0;
-	$path = $gs->{path} . '/' . $path;
+	$path = $gs->path . '/' . $path;
 
 	# canonicalize the path (otherwise libsvn will abort or fail to
 	# find the file)
@@ -1396,7 +1396,7 @@ sub cmd_commit_diff {
 			      "the command-line\n", $usage);
 		}
 		$url = $gs->{url};
-		$svn_path = $gs->{path};
+		$svn_path = $gs->path;
 	}
 	unless (defined $_revision) {
 		fatal("-r|--revision is a required argument\n", $usage);
@@ -1670,7 +1670,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 = "$gs->{path}/$repo_path";
+	my $remote_path = $gs->path . "/$repo_path";
 	$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
 	$remote_path =~ s#/+#/#g;
 	$remote_path =~ s#^/##g;
diff --git c/perl/Git/SVN.pm w/perl/Git/SVN.pm
index a93ac61b..3aa20109 100644
--- c/perl/Git/SVN.pm
+++ w/perl/Git/SVN.pm
@@ -437,22 +437,19 @@ sub new {
 		}
 	}
 	my $self = _new($class, $repo_id, $ref_id, $path);
-	if (!defined $self->path || !length $self->path) {
+	$path = $self->path;
+	if (!defined $path || !length $path) {
 		my $fetch = command_oneline('config', '--get',
 		                            "svn-remote.$repo_id.fetch",
 		                            ":$ref_id\$") or
 		     die "Failed to read \"svn-remote.$repo_id.fetch\" ",
 		         "\":$ref_id\$\" in config\n";
-		my($path) = split(/\s*:\s*/, $fetch);
-		$self->path($path);
-	}
-	{
-		my $path = $self->path;
-		$path =~ s{/+}{/}g;
-		$path =~ s{\A/}{};
-		$path =~ s{/\z}{};
-		$self->path($path);
+		($path, undef) = split(/\s*:\s*/, $fetch);
 	}
+	$path =~ s{/+}{/}g;
+	$path =~ s{\A/}{};
+	$path =~ s{/\z}{};
+	$self->path($path);
 	$self->{url} = command_oneline('config', '--get',
 	                               "svn-remote.$repo_id.url") or
                   die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
@@ -1059,7 +1056,7 @@ sub match_paths {
 	if (my $path = $paths->{"/".$self->path}) {
 		return ($path->{action} eq 'D') ? 0 : 1;
 	}
-	$self->{path_regex} ||= qr{^/\Q@{[$self->path]}\E/};
+	$self->{path_regex} ||= qr/^\/\Q@{[$self->path]}\E\//;
 	if (grep /$self->{path_regex}/, keys %$paths) {
 		return 1;
 	}
@@ -1121,7 +1118,7 @@ sub find_parent_branch {
 			($base, $head) = parse_revision_argument(0, $r);
 		} else {
 			if ($r0 < $r) {
-				$gs->ra->get_log([$gs->{path}], $r0 + 1, $r, 1,
+				$gs->ra->get_log([$gs->path], $r0 + 1, $r, 1,
 					0, 1, sub { $base = $_[1] - 1 });
 			}
 		}
@@ -1143,7 +1140,7 @@ sub find_parent_branch {
 			# at the moment), so we can't rely on it
 			$self->{last_rev} = $r0;
 			$self->{last_commit} = $parent;
-			$ed = Git::SVN::Fetcher->new($self, $gs->{path});
+			$ed = Git::SVN::Fetcher->new($self, $gs->path);
 			$gs->ra->gs_do_switch($r0, $rev, $gs,
 					      $self->full_url, $ed)
 			  or die "SVN connection failed somewhere...\n";
@@ -2287,10 +2284,7 @@ sub _new {
 		ref_id => $ref_id, dir => $dir, index => "$dir/index",
 	        config => "$ENV{GIT_DIR}/svn/config",
 	        map_root => "$dir/.rev_map", repo_id => $repo_id }, $class;
-
-	# Ensure it gets canonicalized
 	$obj->path($path);
-
 	return $obj;
 }
 
diff --git c/perl/Git/SVN/Fetcher.pm w/perl/Git/SVN/Fetcher.pm
index 76fae9bc..046a7a2f 100644
--- c/perl/Git/SVN/Fetcher.pm
+++ w/perl/Git/SVN/Fetcher.pm
@@ -83,7 +83,7 @@ sub _mark_empty_symlinks {
 	chomp(my $empty_blob = `git hash-object -t blob --stdin < /dev/null`);
 	my ($ls, $ctx) = command_output_pipe(qw/ls-tree -r -z/, $cmt);
 	local $/ = "\0";
-	my $pfx = defined($switch_path) ? $switch_path : $git_svn->{path};
+	my $pfx = defined($switch_path) ? $switch_path : $git_svn->path;
 	$pfx .= '/' if length($pfx);
 	while (<$ls>) {
 		chomp;
diff --git c/perl/Git/SVN/Ra.pm w/perl/Git/SVN/Ra.pm
index 23ff43e8..64d00672 100644
--- c/perl/Git/SVN/Ra.pm
+++ w/perl/Git/SVN/Ra.pm
@@ -246,7 +246,7 @@ sub get_commit_editor {
 sub gs_do_update {
 	my ($self, $rev_a, $rev_b, $gs, $editor) = @_;
 	my $new = ($rev_a == $rev_b);
-	my $path = $gs->{path};
+	my $path = $gs->path;
 
 	if ($new && -e $gs->{index}) {
 		unlink $gs->{index} or die
@@ -282,7 +282,7 @@ sub gs_do_update {
 # svn_ra_reparent didn't work before 1.4)
 sub gs_do_switch {
 	my ($self, $rev_a, $rev_b, $gs, $url_b, $editor) = @_;
-	my $path = $gs->{path};
+	my $path = $gs->path;
 	my $pool = SVN::Pool->new;
 
 	my $full_url = $self->{url};
@@ -326,7 +326,7 @@ sub longest_common_path {
 	my $common_max = scalar @$gsv;
 
 	foreach my $gs (@$gsv) {
-		my @tmp = split m#/#, $gs->{path};
+		my @tmp = split m#/#, $gs->path;
 		my $p = '';
 		foreach (@tmp) {
 			$p .= length($p) ? "/$_" : $_;
@@ -407,7 +407,7 @@ sub gs_fetch_loop_common {
 		}
 		$SVN::Error::handler = $err_handler;
 
-		my %exists = map { $_->{path} => $_ } @$gsv;
+		my %exists = map { $_->path => $_ } @$gsv;
 		foreach my $r (sort {$a <=> $b} keys %revs) {
 			my ($paths, $logged) = @{$revs{$r}};
 


---
diff --git c/git-svn.perl w/git-svn.perl
index 5711c571..af7d5308 100755
--- c/git-svn.perl
+++ w/git-svn.perl
@@ -1195,7 +1195,7 @@ sub cmd_show_ignore {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		print STDOUT "\n# $path\n";
 		my $s = $props->{'svn:ignore'} or return;
@@ -1211,7 +1211,7 @@ sub cmd_show_externals {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		print STDOUT "\n# $path\n";
 		my $s = $props->{'svn:externals'} or return;
@@ -1226,7 +1226,7 @@ sub cmd_create_ignore {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		# $path is of the form /path/to/dir/
 		$path = '.' . $path;
@@ -1294,7 +1294,7 @@ sub get_svnprops {
 	$path = $cmd_dir_prefix . $path;
 	fatal("No such file or directory: $path") unless -e $path;
 	my $is_dir = -d $path ? 1 : 0;
-	$path = $gs->{path} . '/' . $path;
+	$path = $gs->path . '/' . $path;
 
 	# canonicalize the path (otherwise libsvn will abort or fail to
 	# find the file)
@@ -1396,7 +1396,7 @@ sub cmd_commit_diff {
 			      "the command-line\n", $usage);
 		}
 		$url = $gs->{url};
-		$svn_path = $gs->{path};
+		$svn_path = $gs->path;
 	}
 	unless (defined $_revision) {
 		fatal("-r|--revision is a required argument\n", $usage);
@@ -1670,7 +1670,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 = "$gs->{path}/$repo_path";
+	my $remote_path = $gs->path . "/$repo_path";
 	$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
 	$remote_path =~ s#/+#/#g;
 	$remote_path =~ s#^/##g;
diff --git c/perl/Git/SVN.pm w/perl/Git/SVN.pm
index a93ac61b..3aa20109 100644
--- c/perl/Git/SVN.pm
+++ w/perl/Git/SVN.pm
@@ -437,22 +437,19 @@ sub new {
 		}
 	}
 	my $self = _new($class, $repo_id, $ref_id, $path);
-	if (!defined $self->path || !length $self->path) {
+	$path = $self->path;
+	if (!defined $path || !length $path) {
 		my $fetch = command_oneline('config', '--get',
 		                            "svn-remote.$repo_id.fetch",
 		                            ":$ref_id\$") or
 		     die "Failed to read \"svn-remote.$repo_id.fetch\" ",
 		         "\":$ref_id\$\" in config\n";
-		my($path) = split(/\s*:\s*/, $fetch);
-		$self->path($path);
-	}
-	{
-		my $path = $self->path;
-		$path =~ s{/+}{/}g;
-		$path =~ s{\A/}{};
-		$path =~ s{/\z}{};
-		$self->path($path);
+		($path, undef) = split(/\s*:\s*/, $fetch);
 	}
+	$path =~ s{/+}{/}g;
+	$path =~ s{\A/}{};
+	$path =~ s{/\z}{};
+	$self->path($path);
 	$self->{url} = command_oneline('config', '--get',
 	                               "svn-remote.$repo_id.url") or
                   die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
@@ -1059,7 +1056,7 @@ sub match_paths {
 	if (my $path = $paths->{"/".$self->path}) {
 		return ($path->{action} eq 'D') ? 0 : 1;
 	}
-	$self->{path_regex} ||= qr{^/\Q@{[$self->path]}\E/};
+	$self->{path_regex} ||= qr/^\/\Q@{[$self->path]}\E\//;
 	if (grep /$self->{path_regex}/, keys %$paths) {
 		return 1;
 	}
@@ -1121,7 +1118,7 @@ sub find_parent_branch {
 			($base, $head) = parse_revision_argument(0, $r);
 		} else {
 			if ($r0 < $r) {
-				$gs->ra->get_log([$gs->{path}], $r0 + 1, $r, 1,
+				$gs->ra->get_log([$gs->path], $r0 + 1, $r, 1,
 					0, 1, sub { $base = $_[1] - 1 });
 			}
 		}
@@ -1143,7 +1140,7 @@ sub find_parent_branch {
 			# at the moment), so we can't rely on it
 			$self->{last_rev} = $r0;
 			$self->{last_commit} = $parent;
-			$ed = Git::SVN::Fetcher->new($self, $gs->{path});
+			$ed = Git::SVN::Fetcher->new($self, $gs->path);
 			$gs->ra->gs_do_switch($r0, $rev, $gs,
 					      $self->full_url, $ed)
 			  or die "SVN connection failed somewhere...\n";
@@ -2287,10 +2284,7 @@ sub _new {
 		ref_id => $ref_id, dir => $dir, index => "$dir/index",
 	        config => "$ENV{GIT_DIR}/svn/config",
 	        map_root => "$dir/.rev_map", repo_id => $repo_id }, $class;
-
-	# Ensure it gets canonicalized
 	$obj->path($path);
-
 	return $obj;
 }
 
diff --git c/perl/Git/SVN/Fetcher.pm w/perl/Git/SVN/Fetcher.pm
index 76fae9bc..046a7a2f 100644
--- c/perl/Git/SVN/Fetcher.pm
+++ w/perl/Git/SVN/Fetcher.pm
@@ -83,7 +83,7 @@ sub _mark_empty_symlinks {
 	chomp(my $empty_blob = `git hash-object -t blob --stdin < /dev/null`);
 	my ($ls, $ctx) = command_output_pipe(qw/ls-tree -r -z/, $cmt);
 	local $/ = "\0";
-	my $pfx = defined($switch_path) ? $switch_path : $git_svn->{path};
+	my $pfx = defined($switch_path) ? $switch_path : $git_svn->path;
 	$pfx .= '/' if length($pfx);
 	while (<$ls>) {
 		chomp;
diff --git c/perl/Git/SVN/Ra.pm w/perl/Git/SVN/Ra.pm
index 23ff43e8..64d00672 100644
--- c/perl/Git/SVN/Ra.pm
+++ w/perl/Git/SVN/Ra.pm
@@ -246,7 +246,7 @@ sub get_commit_editor {
 sub gs_do_update {
 	my ($self, $rev_a, $rev_b, $gs, $editor) = @_;
 	my $new = ($rev_a == $rev_b);
-	my $path = $gs->{path};
+	my $path = $gs->path;
 
 	if ($new && -e $gs->{index}) {
 		unlink $gs->{index} or die
@@ -282,7 +282,7 @@ sub gs_do_update {
 # svn_ra_reparent didn't work before 1.4)
 sub gs_do_switch {
 	my ($self, $rev_a, $rev_b, $gs, $url_b, $editor) = @_;
-	my $path = $gs->{path};
+	my $path = $gs->path;
 	my $pool = SVN::Pool->new;
 
 	my $full_url = $self->{url};
@@ -326,7 +326,7 @@ sub longest_common_path {
 	my $common_max = scalar @$gsv;
 
 	foreach my $gs (@$gsv) {
-		my @tmp = split m#/#, $gs->{path};
+		my @tmp = split m#/#, $gs->path;
 		my $p = '';
 		foreach (@tmp) {
 			$p .= length($p) ? "/$_" : $_;
@@ -407,7 +407,7 @@ sub gs_fetch_loop_common {
 		}
 		$SVN::Error::handler = $err_handler;
 
-		my %exists = map { $_->{path} => $_ } @$gsv;
+		my %exists = map { $_->path => $_ } @$gsv;
 		foreach my $r (sort {$a <=> $b} keys %revs) {
 			my ($paths, $logged) = @{$revs{$r}};

Jonathan Nieder· Sep 17, 2012, 09:08 UTC · re: Jonathan Nieder · lore

[FYI/PATCH 1/5] Git::SVN: introduce path accessor

From: Michael G. Schwern <schwern@pobox.com>
Date: Fri, 27 Jul 2012 13:00:48 -0700

Each Git::SVN handle has a (base) URL and a (relative) path pointing to the top-level directory of the branch it handles. Introduce a getter and setter for the path as preparation for automatically canonicalizing it when reading or writing.

For example, instead of

	$oldpath = $gs->{path};
	$gs->{path} = $url;
	$gs->{path} =~ s!^\Q$min_url\E(/|$)!!;

now you can write

	$oldpath = $gs->path;
	$url =~ s!^\Q$min_url\E(/|$)!!;
	$gs->path($url);

[jn: split from a larger patch]

Signed-off-by: Eric Wong <normalperson@yhbt.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 perl/Git/SVN.pm |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index b8b34744..268e0e84 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -2282,6 +2282,18 @@ sub _new {
 	        map_root => "$dir/.rev_map", repo_id => $repo_id }, $class;
 }
 
+sub path {
+	my $self = shift;
+
+	if (@_) {
+		my $path = shift;
+		$self->{path} = $path;
+		return;
+	}
+
+	return $self->{path};
+}
+
 # for read-only access of old .rev_db formats
 sub unlink_rev_db_symlink {
 	my ($self) = @_;

-- 
1.7.10.4

Jonathan Nieder· Sep 17, 2012, 09:09 UTC · re: Jonathan Nieder · lore

[FYI/PATCH 2/5] Git::SVN: use accessor to read path

From: Michael G. Schwern <schwern@pobox.com>
Date: Fri, 27 Jul 2012 13:00:48 -0700

This patch only touches the simplest cases that simply read the Git::SVN field rather than assigning to or applying a substitution to it.

Code to change found by searching for the term {path}.

[jn: extracted from a larger patch]

Signed-off-by: Eric Wong <normalperson@yhbt.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-svn.perl            |   12 ++++++------
 perl/Git/SVN.pm         |   44 ++++++++++++++++++++++----------------------
 perl/Git/SVN/Fetcher.pm |    2 +-
 perl/Git/SVN/Ra.pm      |    8 ++++----
 4 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 5711c571..af7d5308 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1195,7 +1195,7 @@ sub cmd_show_ignore {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		print STDOUT "\n# $path\n";
 		my $s = $props->{'svn:ignore'} or return;
@@ -1211,7 +1211,7 @@ sub cmd_show_externals {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		print STDOUT "\n# $path\n";
 		my $s = $props->{'svn:externals'} or return;
@@ -1226,7 +1226,7 @@ sub cmd_create_ignore {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		# $path is of the form /path/to/dir/
 		$path = '.' . $path;
@@ -1294,7 +1294,7 @@ sub get_svnprops {
 	$path = $cmd_dir_prefix . $path;
 	fatal("No such file or directory: $path") unless -e $path;
 	my $is_dir = -d $path ? 1 : 0;
-	$path = $gs->{path} . '/' . $path;
+	$path = $gs->path . '/' . $path;
 
 	# canonicalize the path (otherwise libsvn will abort or fail to
 	# find the file)
@@ -1396,7 +1396,7 @@ sub cmd_commit_diff {
 			      "the command-line\n", $usage);
 		}
 		$url = $gs->{url};
-		$svn_path = $gs->{path};
+		$svn_path = $gs->path;
 	}
 	unless (defined $_revision) {
 		fatal("-r|--revision is a required argument\n", $usage);
@@ -1670,7 +1670,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 = "$gs->{path}/$repo_path";
+	my $remote_path = $gs->path . "/$repo_path";
 	$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
 	$remote_path =~ s#/+#/#g;
 	$remote_path =~ s#^/##g;
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 268e0e84..02d5abc0 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -314,7 +314,7 @@ sub init_remote_config {
 				print STDERR "Using higher level of URL: ",
 					     "$url => $min_url\n";
 			}
-			my $old_path = $self->{path};
+			my $old_path = $self->path;
 			$self->{path} = $url;
 			$self->{path} =~ s!^\Q$min_url\E(/|$)!!;
 			if (length $old_path) {
@@ -347,7 +347,7 @@ sub init_remote_config {
 		$self->{path} =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
 		command_noisy('config', '--add',
 			      "svn-remote.$self->{repo_id}.fetch",
-			      "$self->{path}:".$self->refname);
+			      $self->path.":".$self->refname);
 	}
 	$self->{url} = $url;
 }
@@ -435,7 +435,7 @@ sub new {
 		}
 	}
 	my $self = _new($class, $repo_id, $ref_id, $path);
-	if (!defined $self->{path} || !length $self->{path}) {
+	if (!defined $self->path || !length $self->path) {
 		my $fetch = command_oneline('config', '--get',
 		                            "svn-remote.$repo_id.fetch",
 		                            ":$ref_id\$") or
@@ -567,7 +567,7 @@ sub _set_svm_vars {
 	}
 
 	my $r = $ra->get_latest_revnum;
-	my $path = $self->{path};
+	my $path = $self->path;
 	my %tried;
 	while (length $path) {
 		unless ($tried{"$self->{url}/$path"}) {
@@ -728,7 +728,7 @@ sub prop_walk {
 	$path =~ s#^/*#/#g;
 	my $p = $path;
 	# Strip the irrelevant part of the path.
-	$p =~ s#^/+\Q$self->{path}\E(/|$)#/#;
+	$p =~ s#^/+\Q@{[$self->path]}\E(/|$)#/#;
 	# Ensure the path is terminated by a `/'.
 	$p =~ s#/*$#/#;
 
@@ -749,7 +749,7 @@ sub prop_walk {
 
 	foreach (sort keys %$dirent) {
 		next if $dirent->{$_}->{kind} != $SVN::Node::dir;
-		$self->prop_walk($self->{path} . $p . $_, $rev, $sub);
+		$self->prop_walk($self->path . $p . $_, $rev, $sub);
 	}
 }
 
@@ -920,19 +920,19 @@ sub rewrite_uuid {
 sub metadata_url {
 	my ($self) = @_;
 	($self->rewrite_root || $self->{url}) .
-	   (length $self->{path} ? '/' . $self->{path} : '');
+	   (length $self->path ? '/' . $self->path : '');
 }
 
 sub full_url {
 	my ($self) = @_;
-	$self->{url} . (length $self->{path} ? '/' . $self->{path} : '');
+	$self->{url} . (length $self->path ? '/' . $self->path : '');
 }
 
 sub full_pushurl {
 	my ($self) = @_;
 	if ($self->{pushurl}) {
-		return $self->{pushurl} . (length $self->{path} ? '/' .
-		       $self->{path} : '');
+		return $self->{pushurl} . (length $self->path ? '/' .
+		       $self->path : '');
 	} else {
 		return $self->full_url;
 	}
@@ -1048,20 +1048,20 @@ sub do_git_commit {
 
 sub match_paths {
 	my ($self, $paths, $r) = @_;
-	return 1 if $self->{path} eq '';
-	if (my $path = $paths->{"/$self->{path}"}) {
+	return 1 if $self->path eq '';
+	if (my $path = $paths->{"/".$self->path}) {
 		return ($path->{action} eq 'D') ? 0 : 1;
 	}
-	$self->{path_regex} ||= qr/^\/\Q$self->{path}\E\//;
+	$self->{path_regex} ||= qr/^\/\Q@{[$self->path]}\E\//;
 	if (grep /$self->{path_regex}/, keys %$paths) {
 		return 1;
 	}
 	my $c = '';
-	foreach (split m#/#, $self->{path}) {
+	foreach (split m#/#, $self->path) {
 		$c .= "/$_";
 		next unless ($paths->{$c} &&
 		             ($paths->{$c}->{action} =~ /^[AR]$/));
-		if ($self->ra->check_path($self->{path}, $r) ==
+		if ($self->ra->check_path($self->path, $r) ==
 		    $SVN::Node::dir) {
 			return 1;
 		}
@@ -1075,14 +1075,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,
+		$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->{path};
+	my @b_path_components = split m#/#, $self->path;
 	my @a_path_components;
 	my $i;
 	while (@b_path_components) {
@@ -1114,7 +1114,7 @@ sub find_parent_branch {
 			($base, $head) = parse_revision_argument(0, $r);
 		} else {
 			if ($r0 < $r) {
-				$gs->ra->get_log([$gs->{path}], $r0 + 1, $r, 1,
+				$gs->ra->get_log([$gs->path], $r0 + 1, $r, 1,
 					0, 1, sub { $base = $_[1] - 1 });
 			}
 		}
@@ -1136,7 +1136,7 @@ sub find_parent_branch {
 			# at the moment), so we can't rely on it
 			$self->{last_rev} = $r0;
 			$self->{last_commit} = $parent;
-			$ed = Git::SVN::Fetcher->new($self, $gs->{path});
+			$ed = Git::SVN::Fetcher->new($self, $gs->path);
 			$gs->ra->gs_do_switch($r0, $rev, $gs,
 					      $self->full_url, $ed)
 			  or die "SVN connection failed somewhere...\n";
@@ -1235,7 +1235,7 @@ sub mkemptydirs {
 		close $fh;
 	}
 
-	my $strip = qr/\A\Q$self->{path}\E(?:\/|$)/;
+	my $strip = qr/\A\Q@{[$self->path]}\E(?:\/|$)/;
 	foreach my $d (sort keys %empty_dirs) {
 		$d = uri_decode($d);
 		$d =~ s/$strip//;
@@ -1858,7 +1858,7 @@ sub make_log_entry {
 		$commit_email ||= "$author\@$uuid";
 	} elsif ($self->use_svnsync_props) {
 		my $full_url = $self->svnsync->{url};
-		$full_url .= "/$self->{path}" if length $self->{path};
+		$full_url .= "/".$self->path if length $self->path;
 		remove_username($full_url);
 		my $uuid = $self->svnsync->{uuid};
 		$log_entry{metadata} = "$full_url\@$rev $uuid";
@@ -1905,7 +1905,7 @@ sub set_tree {
 	                tree_b => $tree,
 	                editor_cb => sub {
 			       $self->set_tree_cb($log_entry, $tree, @_) },
-	                svn_path => $self->{path} );
+	                svn_path => $self->path );
 	if (!Git::SVN::Editor->new(\%ed_opts)->apply_diff) {
 		print "No changes\nr$self->{last_rev} = $tree\n";
 	}
diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index 76fae9bc..046a7a2f 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -83,7 +83,7 @@ sub _mark_empty_symlinks {
 	chomp(my $empty_blob = `git hash-object -t blob --stdin < /dev/null`);
 	my ($ls, $ctx) = command_output_pipe(qw/ls-tree -r -z/, $cmt);
 	local $/ = "\0";
-	my $pfx = defined($switch_path) ? $switch_path : $git_svn->{path};
+	my $pfx = defined($switch_path) ? $switch_path : $git_svn->path;
 	$pfx .= '/' if length($pfx);
 	while (<$ls>) {
 		chomp;
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 23ff43e8..64d00672 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -246,7 +246,7 @@ sub get_commit_editor {
 sub gs_do_update {
 	my ($self, $rev_a, $rev_b, $gs, $editor) = @_;
 	my $new = ($rev_a == $rev_b);
-	my $path = $gs->{path};
+	my $path = $gs->path;
 
 	if ($new && -e $gs->{index}) {
 		unlink $gs->{index} or die
@@ -282,7 +282,7 @@ sub gs_do_update {
 # svn_ra_reparent didn't work before 1.4)
 sub gs_do_switch {
 	my ($self, $rev_a, $rev_b, $gs, $url_b, $editor) = @_;
-	my $path = $gs->{path};
+	my $path = $gs->path;
 	my $pool = SVN::Pool->new;
 
 	my $full_url = $self->{url};
@@ -326,7 +326,7 @@ sub longest_common_path {
 	my $common_max = scalar @$gsv;
 
 	foreach my $gs (@$gsv) {
-		my @tmp = split m#/#, $gs->{path};
+		my @tmp = split m#/#, $gs->path;
 		my $p = '';
 		foreach (@tmp) {
 			$p .= length($p) ? "/$_" : $_;
@@ -407,7 +407,7 @@ sub gs_fetch_loop_common {
 		}
 		$SVN::Error::handler = $err_handler;
 
-		my %exists = map { $_->{path} => $_ } @$gsv;
+		my %exists = map { $_->path => $_ } @$gsv;
 		foreach my $r (sort {$a <=> $b} keys %revs) {
 			my ($paths, $logged) = @{$revs{$r}};

-- 
1.7.10.4

Jonathan Nieder· Sep 17, 2012, 09:10 UTC · re: Jonathan Nieder · lore

[FYI/PATCH 3/5] Git::SVN: use accessor to write path

From: Michael G. Schwern <schwern@pobox.com>
Date: Fri, 27 Jul 2012 13:00:48 -0700

This patch only touches cases where the path field is written to using $gs->{path}. Cases where the path is set directly in a hash literal will be addressed separately.

[jn: split from a larger patch]

Signed-off-by: Eric Wong <normalperson@yhbt.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 perl/Git/SVN.pm |   24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 02d5abc0..826a7fa6 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -315,11 +315,11 @@ sub init_remote_config {
 					     "$url => $min_url\n";
 			}
 			my $old_path = $self->path;
-			$self->{path} = $url;
-			$self->{path} =~ s!^\Q$min_url\E(/|$)!!;
+			$url =~ s!^\Q$min_url\E(/|$)!!;
 			if (length $old_path) {
-				$self->{path} .= "/$old_path";
+				$url .= "/$old_path";
 			}
+			$self->path($url);
 			$url = $min_url;
 		}
 	}
@@ -343,8 +343,10 @@ sub init_remote_config {
 	unless ($no_write) {
 		command_noisy('config',
 			      "svn-remote.$self->{repo_id}.url", $url);
-		$self->{path} =~ s{^/}{};
-		$self->{path} =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
+		my $path = $self->path;
+		$path =~ s{^/}{};
+		$path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
+		$self->path($path);
 		command_noisy('config', '--add',
 			      "svn-remote.$self->{repo_id}.fetch",
 			      $self->path.":".$self->refname);
@@ -435,17 +437,19 @@ sub new {
 		}
 	}
 	my $self = _new($class, $repo_id, $ref_id, $path);
-	if (!defined $self->path || !length $self->path) {
+	$path = $self->path;
+	if (!defined $path || !length $path) {
 		my $fetch = command_oneline('config', '--get',
 		                            "svn-remote.$repo_id.fetch",
 		                            ":$ref_id\$") or
 		     die "Failed to read \"svn-remote.$repo_id.fetch\" ",
 		         "\":$ref_id\$\" in config\n";
-		($self->{path}, undef) = split(/\s*:\s*/, $fetch);
+		($path, undef) = split(/\s*:\s*/, $fetch);
 	}
-	$self->{path} =~ s{/+}{/}g;
-	$self->{path} =~ s{\A/}{};
-	$self->{path} =~ s{/\z}{};
+	$path =~ s{/+}{/}g;
+	$path =~ s{\A/}{};
+	$path =~ s{/\z}{};
+	$self->path($path);
 	$self->{url} = command_oneline('config', '--get',
 	                               "svn-remote.$repo_id.url") or
                   die "Failed to read \"svn-remote.$repo_id.url\" in config\n";

-- 
1.7.10.4

Jonathan Nieder· Sep 17, 2012, 09:12 UTC · re: Jonathan Nieder · lore

[FYI/PATCH 4/5] Git::SVN::_new: use accessor to write path field

From: Michael G. Schwern <schwern@pobox.com>
Date: Fri, 27 Jul 2012 13:00:48 -0700

If some day the setter is taught to canonicalize paths, make sure the path gets canonicalized at construction time, too.

[jn: split from a larger patch]

Signed-off-by: Eric Wong <normalperson@yhbt.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 perl/Git/SVN.pm |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 826a7fa6..3aa20109 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -2280,10 +2280,12 @@ sub _new {
 
 	$_[3] = $path = '' unless (defined $path);
 	mkpath([$dir]);
-	bless {
+	my $obj = bless {
 		ref_id => $ref_id, dir => $dir, index => "$dir/index",
-	        path => $path, config => "$ENV{GIT_DIR}/svn/config",
+	        config => "$ENV{GIT_DIR}/svn/config",
 	        map_root => "$dir/.rev_map", repo_id => $repo_id }, $class;
+	$obj->path($path);
+	return $obj;
 }
 
 sub path {

-- 
1.7.10.4

Jonathan Nieder· Sep 17, 2012, 09:13 UTC · re: Jonathan Nieder · lore

[PATCH/RFC 5/5] Git::SVN: rename private path field

All users of $gs->{path} should have been converted to use the accessor by now. Check our work by renaming the underlying variable to break callers that try to use it directly.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 perl/Git/SVN.pm |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 3aa20109..33f15682 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -2293,11 +2293,11 @@ sub path {
 
 	if (@_) {
 		my $path = shift;
-		$self->{path} = $path;
+		$self->{_path} = $path;
 		return;
 	}
 
-	return $self->{path};
+	return $self->{_path};
 }
 
 # for read-only access of old .rev_db formats

-- 
1.7.10.4

Junio C Hamano· Sep 18, 2012, 00:00 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC 5/5] Git::SVN: rename private path field

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 6 quoted lines

> All users of $gs->{path} should have been converted to use the
> accessor by now.  Check our work by renaming the underlying variable
> to break callers that try to use it directly.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---

I like this ;-) If we know we have good coverage, this would be a sensible way to catch remaining code that hasn't been converted.

Show 21 quoted lines

>  perl/Git/SVN.pm |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 3aa20109..33f15682 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -2293,11 +2293,11 @@ sub path {
>  
>  	if (@_) {
>  		my $path = shift;
> -		$self->{path} = $path;
> +		$self->{_path} = $path;
>  		return;
>  	}
>  
> -	return $self->{path};
> +	return $self->{_path};
>  }
>  
>  # for read-only access of old .rev_db formats

Eric Wong· Sep 18, 2012, 00:07 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC 5/5] Git::SVN: rename private path field

Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 5 quoted lines

> All users of $gs->{path} should have been converted to use the
> accessor by now.  Check our work by renaming the underlying variable
> to break callers that try to use it directly.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

I think this is a good patch for master, too.  Thanks.

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

> ---
>  perl/Git/SVN.pm |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> --

I'll apply the following (on top of a patch which fixes some {path} usages):

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 88b9164..59215fa 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -2331,11 +2331,11 @@ sub path {
 
 	if (@_) {
 		my $path = shift;
-		$self->{path} = canonicalize_path($path);
+		$self->{_path} = canonicalize_path($path);
 		return;
 	}
 
-	return $self->{path};
+	return $self->{_path};
 }
 
 sub url {

Michael G. Schwern· Jul 27, 2012, 20:00 UTC · re: Michael G. Schwern · lore

[PATCH 2/5] Make Git::SVN use an accessor for URLs internally.

From: "Michael G. Schwern" <schwern@pobox.com>

So later it can do automatic canonicalization.

A later patch will make other things use the accessor.

No functional change here.
---
 perl/Git/SVN.pm | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 0aff9d0..59bca51 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -351,7 +351,7 @@ sub init_remote_config {
 			      "svn-remote.$self->{repo_id}.fetch",
 			      $self->path.":".$self->refname);
 	}
-	$self->{url} = $url;
+	$self->url($url);
 }
 
 sub find_by_url { # repos_root and, path are optional
@@ -453,9 +453,10 @@ sub new {
 		$path =~ s{/\z}{};
 		$self->path($path);
 	}
-	$self->{url} = command_oneline('config', '--get',
-	                               "svn-remote.$repo_id.url") or
+	my $url = command_oneline('config', '--get',
+	                          "svn-remote.$repo_id.url") or
                   die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
+	$self->url($url);
 	$self->{pushurl} = eval { command_oneline('config', '--get',
 	                          "svn-remote.$repo_id.pushurl") };
 	$self->rebuild;
@@ -577,17 +578,18 @@ sub _set_svm_vars {
 	my $path = $self->path;
 	my %tried;
 	while (length $path) {
-		unless ($tried{"$self->{url}/$path"}) {
+		my $try = $self->url . "/$path";
+		unless ($tried{$try}) {
 			return $ra if $self->read_svm_props($ra, $path, $r);
-			$tried{"$self->{url}/$path"} = 1;
+			$tried{$try} = 1;
 		}
 		$path =~ s#/?[^/]+$##;
 	}
 	die "Path: '$path' should be ''\n" if $path ne '';
 	return $ra if $self->read_svm_props($ra, $path, $r);
-	$tried{"$self->{url}/$path"} = 1;
+	$tried{$self->url."/$path"} = 1;
 
-	if ($ra->{repos_root} eq $self->{url}) {
+	if ($ra->{repos_root} eq $self->url) {
 		die @err, (map { "  $_\n" } keys %tried), "\n";
 	}
 
@@ -610,7 +612,7 @@ sub _set_svm_vars {
 	if (!$ok) {
 		die @err, (map { "  $_\n" } keys %tried), "\n";
 	}
-	Git::SVN::Ra->new($self->{url});
+	Git::SVN::Ra->new($self->url);
 }
 
 sub svnsync {
@@ -677,7 +679,7 @@ sub ra_uuid {
 		if (!$@ && $uuid && $uuid =~ /^([a-f\d\-]{30,})$/i) {
 			$self->{ra_uuid} = $uuid;
 		} else {
-			die "ra_uuid called without URL\n" unless $self->{url};
+			die "ra_uuid called without URL\n" unless $self->url;
 			$self->{ra_uuid} = $self->ra->get_uuid;
 			tmp_config('--add', $key, $self->{ra_uuid});
 		}
@@ -701,7 +703,7 @@ sub repos_root {
 
 sub ra {
 	my ($self) = shift;
-	my $ra = Git::SVN::Ra->new($self->{url});
+	my $ra = Git::SVN::Ra->new($self->url);
 	$self->_set_repos_root($ra->{repos_root});
 	if ($self->use_svm_props && !$self->{svm}) {
 		if ($self->no_metadata) {
@@ -926,13 +928,13 @@ sub rewrite_uuid {
 
 sub metadata_url {
 	my ($self) = @_;
-	($self->rewrite_root || $self->{url}) .
+	($self->rewrite_root || $self->url) .
 	   (length $self->path ? '/' . $self->path : '');
 }
 
 sub full_url {
 	my ($self) = @_;
-	$self->{url} . (length $self->path ? '/' . $self->path : '');
+	$self->url . (length $self->path ? '/' . $self->path : '');
 }
 
 sub full_pushurl {
@@ -1436,7 +1438,7 @@ sub find_extra_svk_parents {
 	for my $ticket ( @tickets ) {
 		my ($uuid, $path, $rev) = split /:/, $ticket;
 		if ( $uuid eq $self->ra_uuid ) {
-			my $url = $self->{url};
+			my $url = $self->url;
 			my $repos_root = $url;
 			my $branch_from = $path;
 			$branch_from =~ s{^/}{};
@@ -1682,7 +1684,7 @@ sub find_extra_svn_parents {
 	# are now marked as merge, we can add the tip as a parent.
 	my @merges = split "\n", $mergeinfo;
 	my @merge_tips;
-	my $url = $self->{url};
+	my $url = $self->url;
 	my $uuid = $self->ra_uuid;
 	my %ranges;
 	for my $merge ( @merges ) {
@@ -2307,6 +2309,20 @@ sub path {
     return $self->{path};
 }
 
+
+sub url {
+    my $self = shift;
+
+    if( @_ ) {
+        my $url = shift;
+        $self->{url} = $url;
+        return;
+    }
+
+    return $self->{url};
+}
+
+
 # for read-only access of old .rev_db formats
 sub unlink_rev_db_symlink {
 	my ($self) = @_;

-- 
1.7.11.3

Michael G. Schwern· Jul 27, 2012, 20:00 UTC · re: Michael G. Schwern · lore

[PATCH 3/5] Make Git::SVN::Ra use an accessor for URLs

From: "Michael G. Schwern" <schwern@pobox.com>

Later it can canonicalize automatically.

A later change will make other things use the accessor.

No functional change.
---
 perl/Git/SVN/Ra.pm | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 23ff43e..329f855 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -84,7 +84,7 @@ sub escape_url {
 sub new {
 	my ($class, $url) = @_;
 	$url =~ s!/+$!!;
-	return $RA if ($RA && $RA->{url} eq $url);
+	return $RA if ($RA && $RA->url eq $url);
 
 	::_req_svn();
 
@@ -119,15 +119,33 @@ sub new {
 	                      config => $config,
 			      pool => SVN::Pool->new,
 	                      auth_provider_callbacks => $callbacks);
-	$self->{url} = $url;
+	$RA = bless $self, $class;
+
+	# Make sure its canonicalized
+	$self->url($url);
 	$self->{svn_path} = $url;
 	$self->{repos_root} = $self->get_repos_root;
 	$self->{svn_path} =~ s#^\Q$self->{repos_root}\E(/|$)##;
 	$self->{cache} = { check_path => { r => 0, data => {} },
 	                   get_dir => { r => 0, data => {} } };
-	$RA = bless $self, $class;
+
+	return $RA;
+}
+
+
+sub url {
+    my $self = shift;
+
+    if( @_ ) {
+        my $url = shift;
+        $self->{url} = $url;
+        return;
+    }
+
+    return $self->{url};
 }
 
+
 sub check_path {
 	my ($self, $path, $r) = @_;
 	my $cache = $self->{cache}->{check_path};
@@ -285,7 +303,7 @@ sub gs_do_switch {
 	my $path = $gs->{path};
 	my $pool = SVN::Pool->new;
 
-	my $full_url = $self->{url};
+	my $full_url = $self->url;
 	my $old_url = $full_url;
 	$full_url .= '/' . $path if length $path;
 	my ($ra, $reparented);
@@ -300,7 +318,7 @@ sub gs_do_switch {
 		$ra_invalid = 1;
 	} elsif ($old_url ne $full_url) {
 		SVN::_Ra::svn_ra_reparent($self->{session}, $full_url, $pool);
-		$self->{url} = $full_url;
+		$self->url($full_url);
 		$reparented = 1;
 	}
 
@@ -313,7 +331,7 @@ sub gs_do_switch {
 
 	if ($reparented) {
 		SVN::_Ra::svn_ra_reparent($self->{session}, $old_url, $pool);
-		$self->{url} = $old_url;
+		$self->url($old_url);
 	}
 
 	$pool->clear;
@@ -362,7 +380,7 @@ sub gs_fetch_loop_common {
 	my $inc = $_log_window_size;
 	my ($min, $max) = ($base, $head < $base + $inc ? $head : $base + $inc);
 	my $longest_path = longest_common_path($gsv, $globs);
-	my $ra_url = $self->{url};
+	my $ra_url = $self->url;
 	my $find_trailing_edge;
 	while (1) {
 		my %revs;
@@ -508,7 +526,7 @@ sub match_globs {
 				 ($self->check_path($p, $r) !=
 				  $SVN::Node::dir));
 			next unless $p =~ /$g->{path}->{regex}/;
-			$exists->{$p} = Git::SVN->init($self->{url}, $p, undef,
+			$exists->{$p} = Git::SVN->init($self->url, $p, undef,
 					 $g->{ref}->full_path($de), 1);
 		}
 	}
@@ -532,7 +550,7 @@ sub match_globs {
 			next if ($self->check_path($pathname, $r) !=
 			         $SVN::Node::dir);
 			$exists->{$pathname} = Git::SVN->init(
-			                      $self->{url}, $pathname, undef,
+			                      $self->url, $pathname, undef,
 			                      $g->{ref}->full_path($p), 1);
 		}
 		my $c = '';
@@ -548,7 +566,7 @@ sub match_globs {
 
 sub minimize_url {
 	my ($self) = @_;
-	return $self->{url} if ($self->{url} eq $self->{repos_root});
+	return $self->url if ($self->url eq $self->{repos_root});
 	my $url = $self->{repos_root};
 	my @components = split(m!/!, $self->{svn_path});
 	my $c = '';
@@ -568,7 +586,7 @@ sub can_do_switch {
 	unless (defined $can_do_switch) {
 		my $pool = SVN::Pool->new;
 		my $rep = eval {
-			$self->do_switch(1, '', 0, $self->{url},
+			$self->do_switch(1, '', 0, $self->url,
 			                 SVN::Delta::Editor->new, $pool);
 		};
 		if ($@) {

-- 
1.7.11.3

Michael G. Schwern· Jul 27, 2012, 20:00 UTC · re: Michael G. Schwern · lore

[PATCH 4/5] Change the rest of the code to use Git::SVN->path instead of the hash directly.

From: "Michael G. Schwern" <schwern@pobox.com>

No functional change.
---
 git-svn.perl            | 12 +++++++-----
 perl/Git/SVN.pm         |  4 ++--
 perl/Git/SVN/Fetcher.pm |  2 +-
 perl/Git/SVN/Ra.pm      |  6 +++---
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 5711c57..039623e 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1195,7 +1195,7 @@ sub cmd_show_ignore {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		print STDOUT "\n# $path\n";
 		my $s = $props->{'svn:ignore'} or return;
@@ -1211,7 +1211,7 @@ sub cmd_show_externals {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		print STDOUT "\n# $path\n";
 		my $s = $props->{'svn:externals'} or return;
@@ -1226,7 +1226,7 @@ sub cmd_create_ignore {
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
-	$gs->prop_walk($gs->{path}, $r, sub {
+	$gs->prop_walk($gs->path, $r, sub {
 		my ($gs, $path, $props) = @_;
 		# $path is of the form /path/to/dir/
 		$path = '.' . $path;
@@ -1396,7 +1396,7 @@ sub cmd_commit_diff {
 			      "the command-line\n", $usage);
 		}
 		$url = $gs->{url};
-		$svn_path = $gs->{path};
+		$svn_path = $gs->path;
 	}
 	unless (defined $_revision) {
 		fatal("-r|--revision is a required argument\n", $usage);
@@ -1634,6 +1634,8 @@ sub post_fetch_checkout {
 sub complete_svn_url {
 	my ($url, $path) = @_;
 	$path =~ s#/+$##;
+
+	# If the path is not a URL...
 	if ($path !~ m#^[a-z\+]+://#) {
 		if (!defined $url || $url !~ m#^[a-z\+]+://#) {
 			fatal("E: '$path' is not a complete URL ",
@@ -1670,7 +1672,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 = "$gs->{path}/$repo_path";
+	my $remote_path = $gs->path . "/$repo_path";
 	$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
 	$remote_path =~ s#/+#/#g;
 	$remote_path =~ s#^/##g;
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 59bca51..fc907a0 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1123,7 +1123,7 @@ sub find_parent_branch {
 			($base, $head) = parse_revision_argument(0, $r);
 		} else {
 			if ($r0 < $r) {
-				$gs->ra->get_log([$gs->{path}], $r0 + 1, $r, 1,
+				$gs->ra->get_log([$gs->path], $r0 + 1, $r, 1,
 					0, 1, sub { $base = $_[1] - 1 });
 			}
 		}
@@ -1145,7 +1145,7 @@ sub find_parent_branch {
 			# at the moment), so we can't rely on it
 			$self->{last_rev} = $r0;
 			$self->{last_commit} = $parent;
-			$ed = Git::SVN::Fetcher->new($self, $gs->{path});
+			$ed = Git::SVN::Fetcher->new($self, $gs->path);
 			$gs->ra->gs_do_switch($r0, $rev, $gs,
 					      $self->full_url, $ed)
 			  or die "SVN connection failed somewhere...\n";
diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index 76fae9b..046a7a2 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -83,7 +83,7 @@ sub _mark_empty_symlinks {
 	chomp(my $empty_blob = `git hash-object -t blob --stdin < /dev/null`);
 	my ($ls, $ctx) = command_output_pipe(qw/ls-tree -r -z/, $cmt);
 	local $/ = "\0";
-	my $pfx = defined($switch_path) ? $switch_path : $git_svn->{path};
+	my $pfx = defined($switch_path) ? $switch_path : $git_svn->path;
 	$pfx .= '/' if length($pfx);
 	while (<$ls>) {
 		chomp;
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 329f855..27dcdd5 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -264,7 +264,7 @@ sub get_commit_editor {
 sub gs_do_update {
 	my ($self, $rev_a, $rev_b, $gs, $editor) = @_;
 	my $new = ($rev_a == $rev_b);
-	my $path = $gs->{path};
+	my $path = $gs->path;
 
 	if ($new && -e $gs->{index}) {
 		unlink $gs->{index} or die
@@ -300,7 +300,7 @@ sub gs_do_update {
 # svn_ra_reparent didn't work before 1.4)
 sub gs_do_switch {
 	my ($self, $rev_a, $rev_b, $gs, $url_b, $editor) = @_;
-	my $path = $gs->{path};
+	my $path = $gs->path;
 	my $pool = SVN::Pool->new;
 
 	my $full_url = $self->url;
@@ -344,7 +344,7 @@ sub longest_common_path {
 	my $common_max = scalar @$gsv;
 
 	foreach my $gs (@$gsv) {
-		my @tmp = split m#/#, $gs->{path};
+		my @tmp = split m#/#, $gs->path;
 		my $p = '';
 		foreach (@tmp) {
 			$p .= length($p) ? "/$_" : $_;

-- 
1.7.11.3

Michael G. Schwern· Jul 27, 2012, 20:00 UTC · re: Michael G. Schwern · lore

[PATCH 5/5] Change the rest of the code to use the Git::SVN and Git::SVN::Ra url accessors.

From: "Michael G. Schwern" <schwern@pobox.com>

Note: The structure returned from Git::SVN->read_all_remotes() does not appear to
contain objects, so I'm leaving them alone.

That's everything converted over to the url and path accessors.

No functional change.
---
 git-svn.perl              | 11 ++++++-----
 perl/Git/SVN.pm           | 11 ++++++-----
 perl/Git/SVN/Migration.pm |  6 +++---
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 039623e..de1ddd1 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1395,7 +1395,7 @@ sub cmd_commit_diff {
 			fatal("Needed URL or usable git-svn --id in ",
 			      "the command-line\n", $usage);
 		}
-		$url = $gs->{url};
+		$url = $gs->url;
 		$svn_path = $gs->path;
 	}
 	unless (defined $_revision) {
@@ -1663,15 +1663,16 @@ sub complete_url_ls_init {
 			      "and a separate URL is not specified");
 		}
 	}
-	my $url = $ra->{url};
+	my $url = $ra->url;
 	my $gs = Git::SVN->init($url, undef, undef, undef, 1);
 	my $k = "svn-remote.$gs->{repo_id}.url";
 	my $orig_url = eval { command_oneline(qw/config --get/, $k) };
-	if ($orig_url && ($orig_url ne $gs->{url})) {
+	if ($orig_url && ($orig_url ne $gs->url)) {
 		die "$k already set: $orig_url\n",
-		    "wanted to set to: $gs->{url}\n";
+		    "wanted to set to: $gs->url\n";
 	}
-	command_oneline('config', $k, $gs->{url}) unless $orig_url;
+	command_oneline('config', $k, $gs->url) unless $orig_url;
+
 	my $remote_path = $gs->path . "/$repo_path";
 	$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
 	$remote_path =~ s#/+#/#g;
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index fc907a0..7913d8f 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -560,7 +560,7 @@ sub _set_svm_vars {
 		# username is of no interest
 		$src =~ s{(^[a-z\+]*://)[^/@]*@}{$1};
 
-		my $replace = $ra->{url};
+		my $replace = $ra->url;
 		$replace .= "/$path" if length $path;
 
 		my $section = "svn-remote.$self->{repo_id}";
@@ -599,16 +599,17 @@ sub _set_svm_vars {
 	$path = $ra->{svn_path};
 	$ra = Git::SVN::Ra->new($ra->{repos_root});
 	while (length $path) {
-		unless ($tried{"$ra->{url}/$path"}) {
+		my $try = $ra->url ."/$path";
+		unless ($tried{$try}) {
 			$ok = $self->read_svm_props($ra, $path, $r);
 			last if $ok;
-			$tried{"$ra->{url}/$path"} = 1;
+			$tried{$try} = 1;
 		}
 		$path =~ s#/?[^/]+$##;
 	}
 	die "Path: '$path' should be ''\n" if $path ne '';
 	$ok ||= $self->read_svm_props($ra, $path, $r);
-	$tried{"$ra->{url}/$path"} = 1;
+	$tried{$ra->url ."/$path"} = 1;
 	if (!$ok) {
 		die @err, (map { "  $_\n" } keys %tried), "\n";
 	}
@@ -1108,7 +1109,7 @@ sub find_parent_branch {
 	}
 	my $r = $i->{copyfrom_rev};
 	my $repos_root = $self->ra->{repos_root};
-	my $url = $self->ra->{url};
+	my $url = $self->ra->url;
 	my $new_url = $url . $branch_from;
 	print STDERR  "Found possible branch point: ",
 	              "$new_url => ", $self->full_url, ", $r\n"
diff --git a/perl/Git/SVN/Migration.pm b/perl/Git/SVN/Migration.pm
index 75d7429..30daf35 100644
--- a/perl/Git/SVN/Migration.pm
+++ b/perl/Git/SVN/Migration.pm
@@ -177,14 +177,14 @@ sub minimize_connections {
 		my $ra = Git::SVN::Ra->new($url);
 
 		# skip existing cases where we already connect to the root
-		if (($ra->{url} eq $ra->{repos_root}) ||
+		if (($ra->url eq $ra->{repos_root}) ||
 		    ($ra->{repos_root} eq $repo_id)) {
-			$root_repos->{$ra->{url}} = $repo_id;
+			$root_repos->{$ra->url} = $repo_id;
 			next;
 		}
 
 		my $root_ra = Git::SVN::Ra->new($ra->{repos_root});
-		my $root_path = $ra->{url};
+		my $root_path = $ra->url;
 		$root_path =~ s#^\Q$ra->{repos_root}\E(/|$)##;
 		foreach my $path (keys %$fetch) {
 			my $ref_id = $fetch->{$path};

-- 
1.7.11.3

Eric Wong· Jul 28, 2012, 02:59 UTC · re: Michael G. Schwern · lore

Re: [PATCH 5/5] Change the rest of the code to use the Git::SVN and Git::SVN::Ra url accessors.

"Michael G. Schwern" <schwern@pobox.com> wrote:

We try to keep Subject: lines short (~50 char soft limit) as documented in SubmittingPatches.

How about:

Subject: [PATCH 4/5] use Git::SVN->path accessor globally
Subject: [PATCH 5/5] use Git::SVN{,::RA}->url accessor globally

I can make the changes to the commit message on my side before I push for Junio.

> Note: The structure returned from Git::SVN->read_all_remotes() does
> not appear to contain objects, so I'm leaving them alone.

Right, just hashrefs and strings there.

> No functional change.

This series looks good with minor edits.

I made minor edits to reformat the new accessor subs (please do so on your end before sending in the future).

Jonathan Nieder· Jul 28, 2012, 03:10 UTC · re: Michael G. Schwern · lore

Re: Make git-svn Use accessors for paths and urls

Michael G. Schwern wrote:

> This is the last refactoring patch series.  After this bugs, start
> getting fixed.

I just wanted to say thanks for your thoughtful presentation of this code. I was worried before, but these have been pleasantly submitted.

If you have a chance at some point to offer advice, I'd love to add the information to Documentation/SubmittingPatches that was missing. Proposed text is ideal, but outline form or a list of missing aspects and confusing existing coverage would be fine, too.

Jonathan

Michael G Schwern· Jul 28, 2012, 07:40 UTC · re: Jonathan Nieder · lore

Re: Make git-svn Use accessors for paths and urls

On 2012.7.27 8:10 PM, Jonathan Nieder wrote:

Show 5 quoted lines

>> This is the last refactoring patch series.  After this bugs, start
>> getting fixed.
> 
> I just wanted to say thanks for your thoughtful presentation of this
> code.  I was worried before, but these have been pleasantly submitted.

You're welcome.  I've gained at least three levels in rebasing in the process.

> If you have a chance at some point to offer advice, I'd love to add
> the information to Documentation/SubmittingPatches that was missing.
> Proposed text is ideal, but outline form or a list of missing aspects
> and confusing existing coverage would be fine, too.

Remind me when I'm done with the 1.7 fix please?

-- 
You are wicked and wrong to have broken inside and peeked at the
implementation and then relied upon it.
	-- tchrist in <31832.969261130@chthon>

Jonathan Nieder· Jul 28, 2012, 07:54 UTC · re: Michael G Schwern · lore

Re: Make git-svn Use accessors for paths and urls

Michael G Schwern wrote:

> On 2012.7.27 8:10 PM, Jonathan Nieder wrote:

>> If you have a chance at some point to offer advice, I'd love to add
>> the information to Documentation/SubmittingPatches that was missing.

[...]

> Remind me when I'm done with the 1.7 fix please?

Sure, if I remember to. :)

Thanks, Jonathan