git-svn SVN 1.7 fix, take 2

It's post OSCON so I can take another crack at this again.

I'm struggling with how best to present all this to you folks. There's etiquette for how one presents a git pull request... but there's conflicting etiquette about how one presents patches to a mailing list. I'm not sure which bit of which applies when here. Documentation/SubmittingPatches focuses on single patches and basic commit etiquette.

A big one is "do not blast 10 emails to a mailing list" but I gather that's ok here if a submission needs 10 commits to be well expressed and its done via git-send-email? And then if patch #3 needs revision I'm to do it in a rebase and resend the whole 10 commits? Am I to think of git-send-email less as a means of sending patches to a mailing list and more as a git transport mechanism?

I'm trying to bust it up into easier to digest pieces. I came into this cold without much knowledge of the problem ("something to do with canonicalization") and no knowledge of the code. While each commit is sharp, the work as a whole is mixed up.

Here's the first pieces, as I see them, along with their branches. The whole work is in https://github.com/schwern/git/tree/git-svn/fix-canonical

* Change the Makefile.PL so it automatically finds the .pm files.
https://github.com/schwern/git/tree/git-svn/easier_modules
(Going to remove the Error.pm movement as off-topic)

* Extract each of the internal Git::* packages from inside git-svn.
https://github.com/schwern/git/tree/git-svn/extract_classes

There's five classes, and I did each in at least two commits. First is a straight cut & paste with no further changes. Second (or more) fixes it so things work again. This is better for review (if it were done in a single commit the real change would be lost in the cut & paste), but it means you have a commit that breaks thing which will be a problem for bisecting. I'm inclined to stick with two commits and you folks can squash them if you decide bisecting is more important.

The Git::SVN extraction is more complicated than the rest, so I'll probably do that separately and bust it up into a few commits.

Next I'm going to...

1) Submit easier_modules.
2) Break up the Git::SVN fix into more commits.
3) Submit the Git::SVN extraction.

Reality is that which, when you stop believing in it, doesn't go away.
    -- Phillip K. Dick

Hi,

I have a crazy idea. You might not like it, but maybe it's worth giving it a try.

[...]

All of these changes are supposed to have zero functional effect, right?

Could you send the first five patches that *are* supposed to have a functional effect? I know that they will not apply cleanly to git-svn from git "master" or on top of each other; that's fine with me. If the approach looks right, interested people (read: probably Ben or I :)) can make the corresponding change in the code layout from "master". Afterwards, we can look into all that refactoring to make later changes easier.

What do you think?

Thanks,
Jonathan

Right. They're just class extraction refactoring. I don't even need to make new classes, they already exist. It's just moving them into their own file.

I think that would be a lot of extra work for me, create a big mess and be harder to understand. :-/

Since I'm creating new files to store the classes, the functional changes cannot be applied without the class extractions, so I'd have to rewrite them.
And they will be harder to review since the main git-svn code and the class
code is mixed up in one file. And they won't have unit tests, since git-svn cannot be loaded without it running.

The Git::SVN extraction isn't really complicated, it just requires a handful more work than the other classes. Just a few method extractions. The rest are essentially cut, paste & fix lexicals.

I'm not sure what you're going for, but you can glance through the existing changes here
https://github.com/schwern/git/commits/git-svn/fix-canonical

or you can do grab it as a remote and...

git log -p schwern/git-svn/extract-classes..schwern/git-svn/fix-canonical

That should give you the information you need... if I understand what you need. I feel like I don't and we're talking past each other.

Ahh email, my old friend.  Do you know that revenge is a dish that is best
served cold?  And it is very cold on the Internet!

Hi again,

[...]

I don't understand. Didn't I ask to send the changes as-is and say that *I or someone else interested* would do the work to get them to apply?

[...]

Basically, you are offering a code fix that's at least worth $500. Lots of people have wanted the bug fixed for a long time. As long as it does not involve sacrificing maintainability, we should apply the fix as soon as possible! It's great that you've done this work.

Meanwhile that change is being held hostage by lots of cleanups that are unquestionably going in the right direction but are going to be a pain in the neck to safely apply. And no one has reviewed your fix that comes _after_ the cleanups. Maybe the fix goes in a wrong direction --- we don't know yet. Maybe once we understand the fix we'll have ideas that obsolete the previous cleanups and can more simply accomplish the same thing by organizing the code a different way.

You are saying that, to make your life easier, we should take your cleanups and fixes as-is, all in one big pull. Maybe you're right! But it will take a lot longer this way than applying a smaller set of changes that just fixes the bug.

I am saying that that, before anything else, it would be helpful for us to *see* the relevant patches and understand your fix. You are more knowledgeable than anyone else about your code, so presumably it should be straightforward to pick out which patches are the important ones. Using "git format-patch -1 <commit>" you can get a patch for each. Then you can use your favorite text editor to edit their subject lines and change descriptions to describe what they do and where they fall in the series of patches you are sending. Finally you can use your favorite mail user agent (e.g., "git send-email") to send out the patches.

Jonathan

I guess so. May we have your sign-off on these changes? (A simple reply of "yes" is enough, no need to resend patches to do this.)

Here it is in patch form for reviewers. If I understand correctly, the idea is to replace accesses to $gs->{path} with calls to a $gs->path function that canonicalizes (and likewise for s/path/url/).

There are probably other subtleties, but that seems to be the gist.

---
 git-svn.perl                          |   96 +++++++--------------
 perl/Git/SVN.pm                       |  148 +++++++++++++++++++++------------
 perl/Git/SVN/Fetcher.pm               |    2 +-
 perl/Git/SVN/Migration.pm             |    6 +-
 perl/Git/SVN/Ra.pm                    |   96 ++++++++++++---------
 perl/Git/SVN/Utils.pm                 |  123 ++++++++++++++++++++++++++-
 t/Git-SVN/Ra/fix_dir.t                |   24 ++++++
 t/lib-git-svn.sh                      |    2 +
 t/t9118-git-svn-funky-branch-names.sh |    6 +-
 t/t9119-git-svn-info.sh               |    7 +-
 10 files changed, 341 insertions(+), 169 deletions(-)

index 7b54f43f..74aeb4df 100755
@@ -12,7 +12,12 @@ $VERSION = '@@GIT_VERSION@@';
 
 use Git::SVN;
 
-use Git::SVN::Utils qw(fatal can_compress);
+use Git::SVN::Utils qw(
+    fatal
+    can_compress
+    canonicalize_path
+    canonicalize_url
+);
 
 # From which subdir have we been invoked?
 my $cmd_dir_prefix = eval {

@@ -100,8 +105,6 @@ BEGIN {
 	Memoize::memoize 'Git::config_bool';
 }
 
-my ($SVN);
-
 $sha1 = qr/[a-f\d]{40}/;
 $sha1_short = qr/[a-f\d]{4,40}/;
 my ($_stdin, $_help, $_edit,

@@ -518,7 +521,6 @@ sub cmd_init {
 	}
 	my $url = shift or die "SVN repository location required ",
 	                       "as a command-line argument\n";
-	$url = canonicalize_url($url);
 	init_subdir(@_);
 	do_git_init_db();
 

@@ -1199,7 +1201,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;

@@ -1215,7 +1217,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;

@@ -1230,7 +1232,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;

@@ -1260,36 +1262,12 @@ sub cmd_mkdirs {
 	$gs->mkemptydirs($_revision);
 }
 
-sub canonicalize_path {
-	my ($path) = @_;
-	my $dot_slash_added = 0;
-	if (substr($path, 0, 1) ne "/") {
-		$path = "./" . $path;
-		$dot_slash_added = 1;
-	}
-	# File::Spec->canonpath doesn't collapse x/../y into y (for a
-	# good reason), so let's do this manually.
-	$path =~ s#/+#/#g;
-	$path =~ s#/\.(?:/|$)#/#g;
-	$path =~ s#/[^/]+/\.\.##g;
-	$path =~ s#/$##g;
-	$path =~ s#^\./## if $dot_slash_added;
-	$path =~ s#^/##;
-	$path =~ s#^\.$##;
-	return $path;
-}
-
-sub canonicalize_url {
-	my ($url) = @_;
-	$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
-	return $url;
-}
-
 # get_svnprops(PATH)
 # ------------------
 # Helper for cmd_propget and cmd_proplist below.
 sub get_svnprops {
 	my $path = shift;
+
 	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
 	$gs ||= Git::SVN->new;
 

@@ -1298,7 +1276,8 @@ 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 if $gs->path;
 
 	# canonicalize the path (otherwise libsvn will abort or fail to
 	# find the file)

@@ -1399,8 +1378,8 @@ sub cmd_commit_diff {
 			fatal("Needed URL or usable git-svn --id in ",
 			      "the command-line\n", $usage);
 		}
-		$url = $gs->{url};
-		$svn_path = $gs->{path};
+		$url = $gs->url;
+		$svn_path = $gs->path;
 	}
 	unless (defined $_revision) {
 		fatal("-r|--revision is a required argument\n", $usage);

@@ -1434,25 +1413,6 @@ sub cmd_commit_diff {
 	}
 }
 
-sub escape_uri_only {
-	my ($uri) = @_;
-	my @tmp;
-	foreach (split m{/}, $uri) {
-		s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf("%%%02X",ord($1))/eg;
-		push @tmp, $_;
-	}
-	join('/', @tmp);
-}
-
-sub escape_url {
-	my ($url) = @_;
-	if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) {
-		my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
-		$url = "$scheme://$domain$uri";
-	}
-	$url;
-}
-
 sub cmd_info {
 	my $path = canonicalize_path(defined($_[0]) ? $_[0] : ".");
 	my $fullpath = canonicalize_path($cmd_dir_prefix . $path);

@@ -1476,21 +1436,21 @@ sub cmd_info {
 	# canonicalize_path() will return "" to make libsvn 1.5.x happy,
 	$path = "." if $path eq "";
 
-	my $full_url = $url . ($fullpath eq "" ? "" : "/$fullpath");
+	my $full_url = canonicalize_url( $url . ($fullpath eq "" ? "" : "/$fullpath") );
 
 	if ($_url) {
-		print escape_url($full_url), "\n";
+		print $full_url, "\n";
 		return;
 	}
 
 	my $result = "Path: $path\n";
 	$result .= "Name: " . basename($path) . "\n" if $file_type ne "dir";
-	$result .= "URL: " . escape_url($full_url) . "\n";
+	$result .= "URL: $full_url\n";
 
 	eval {
 		my $repos_root = $gs->repos_root;
 		Git::SVN::remove_username($repos_root);
-		$result .= "Repository Root: " . escape_url($repos_root) . "\n";
+		$result .= "Repository Root: " . canonicalize_url($repos_root) . "\n";
 	};
 	if ($@) {
 		$result .= "Repository Root: (offline)\n";

@@ -1638,7 +1598,10 @@ sub post_fetch_checkout {
 
 sub complete_svn_url {
 	my ($url, $path) = @_;
-	$path =~ s#/+$##;
+
+	$path = canonicalize_path($path);
+
+	# 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 ",

@@ -1655,7 +1618,8 @@ sub complete_url_ls_init {
 		print STDERR "W: $switch not specified\n";
 		return;
 	}
-	$repo_path =~ s#/+$##;
+
+	$repo_path = canonicalize_path($repo_path);
 	if ($repo_path =~ m#^[a-z\+]+://#) {
 		$ra = Git::SVN::Ra->new($repo_path);
 		$repo_path = '';

@@ -1666,18 +1630,18 @@ 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;
-	my $remote_path = "$gs->{path}/$repo_path";
+	command_oneline('config', $k, $gs->url) unless $orig_url;
+
+	my $remote_path = canonicalize_path($gs->path . "/$repo_path");
 	$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
-	$remote_path =~ s#/+#/#g;
 	$remote_path =~ s#^/##g;
 	$remote_path .= "/*" if $remote_path !~ /\*/;
 	my ($n) = ($switch =~ /^--(\w+)/);

index 69486efe..b606fe5a 100644
@@ -84,7 +84,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;

index b17fe003..5a9d3c71 100644
@@ -180,14 +180,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};

new file mode 100644
index 00000000..ec0c858a
@@ -0,0 +1,24 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use Test::More 'no_plan';
+
+my $CLASS = 'Git::SVN::Ra';
+require_ok $CLASS;
+
+my %tests = (
+    "foo/.."            => "",
+    "/foo/.."           => "/",
+    "."                 => "",
+    "foo/../"           => "",
+    "foo/bar/baz"       => "foo/bar/baz",
+    "foo/bar/../baz"    => "foo/baz",
+);
+
+for my $have (keys %tests) {
+    my $want = $tests{$have};
+
+    is $CLASS->fix_dir($have), $want, "fix_dir $have => $want";
+}

index 199f22c2..c6911cdf 100644
@@ -68,6 +68,8 @@ svn_cmd () {
 	svn "$orig_svncmd" --config-dir "$svnconf" "$@"
 }
 
+SVN_VERSION=`svn_cmd --version | sed -n -e 's/^svn, version \(1\.[0-9]*\.[0-9]*\).*$/\1/p'`
+
 prepare_httpd () {
 	for d in \
 		"$SVN_HTTPD_PATH" \

index 63fc982c..66787acc 100755
@@ -32,6 +32,10 @@ test_expect_success 'setup svnrepo' '
 	start_httpd
 	'
 
+# SVN 1.7 will truncate "not-a%40{0]" to just "not-a".
+# Look at what SVN wound up naming the branch and use that.
+non_reflog=`svn_cmd ls "$svnrepo/pr ject/branches" | grep not-a | sed 's/\///'`
+
 test_expect_success 'test clone with funky branch names' '
 	git svn clone -s "$svnrepo/pr ject" project &&
 	(

@@ -42,7 +46,7 @@ test_expect_success 'test clone with funky branch names' '
 		git rev-parse "refs/remotes/%2Eleading_dot" &&
 		git rev-parse "refs/remotes/trailing_dot%2E" &&
 		git rev-parse "refs/remotes/trailing_dotlock%2Elock" &&
-		git rev-parse "refs/remotes/not-a%40{0}reflog"
+		git rev-parse "refs/remotes/$non_reflog"
 	)
 	'
 

index ff19695e..9b6075c6 100755
@@ -6,14 +6,11 @@ test_description='git svn info'
 
 . ./lib-git-svn.sh
 
-# Tested with: svn, version 1.4.4 (r25188)
-# Tested with: svn, version 1.6.[12345689]
-v=`svn_cmd --version | sed -n -e 's/^svn, version \(1\.[0-9]*\.[0-9]*\).*$/\1/p'`
-case $v in
+case $SVN_VERSION in
 1.[456].*)
 	;;
 *)
-	skip_all="skipping svn-info test (SVN version: $v not supported)"
+	skip_all="skipping svn-info test (SVN version: $SVN_VERSION not supported)"
 	test_done
 	;;
 esac

The impression I am getting is that the updated code wants to handle URL and paths without any funny encoding, but it is unclear from my cursory read (e.g. what goes on with escape_url?).

No, now it's just canonicalizing as early as possible. Preferably within the object accessor rather than at the point of use. So in the code below, $full_url is already escaped/canonicalized.

In general this blob patch isn't going to make a lot of overall sense. I'm working with Jonathan to get it submitted in manageable pieces.

Good point. More importantly, what was it trying to accomplish before and does it need to be preserved?

"If the URL is svn OR its http and needs to be escaped... do something special". I don't really understand what the special stuff in the following block is. Anything that undef's the invocant (ie. $self) is probably broken.

a51cdb0c0420ee3bef26bbd1a9aa75e1d464e5b7 and 2a679c7a3148978a3f58f1c12100383638e744c5 shed some light. 2a679 looks like it specifically holds off on escaping $full_url. It would be very nice if that was not necessary. It would be helpful if the bug mentioned in 2a679 could be reproduced to see if it still applies or can be dealt with in another way.

185. My name is not a killing word.
    -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
           http://skippyslist.com/list/

Yes, yes and whatever (even though I think send-email is just a better MUA/MSA when you want to send patches and isn't restricted for a _git_ transport, I do not think it matters how you look at it).

Perhaps it is a good idea to lurk and see how others submit their topics first?

#3 was not intended as a dig. If I can think about git-send-email like a funny way to do a git-push then that fits better in my head. I worry about sending too many emails to a list at once. I don't worry about sending too many commits in one push.

While I use git heavily I'm not invested in working on it. I work on a lot of projects. I'd like to be able to do the work, submit it, work through review, and get out without joining another mailing list and studying their culture.

Is there a document I could look at for submitting a large body of work, or could I help improve SubmittingPatches to document the process better?

I do have a cause though. It's obscenity. I'm for it.
    - Tom Lehrer

An alternative approach would be to _just_ explain how your patch series works, and then say

"Ok, here is the git branch. Have fun with it. If someone wants to incorporate it, that's great. Let me know if you have any questions."

That's totally fine. Then someone else can submit the patches and help to massage the patches into a form that is ready for application on your behalf.

I thought SubmittingPatches did describe how to send patch series. Could you point to the section that was lacking? Or are you just still in disbelief that people review each patch in a thread, one by one, sending emails to each other?

Grateful but impatient,
Jonathan

Let's start with this.

Is svn_path_canonicalize() idempotent? What does it do when it encounters a percent-sign?

Thanks,
Jonathan

Nothing, because paths are not URI escaped. :)

You probably meant svn_uri_canonicalize(). And no, it does not double escape, so its safe to escape as early as possible.

use SVN::Core;

   my $uri = "http://www.example.com/ foo";

    print SVN::_Core::svn_uri_canonicalize(
        SVN::_Core::svn_uri_canonicalize($uri)
    );

That produces "http://www.example.com/%20foo".

The API docs don't say it specifically, but if it were otherwise it would be impossible to use. You'd have to check first if anything were escaped before canonicalizing. And a user couldn't pass in an escaped URL without risking it being double escaped.

http://subversion.apache.org/docs/api/latest/svn__dirent__uri_8h.html#a8bae33a2fbf86857869f7b0e39a553e7

'All anyone gets in a mirror is themselves,' she said. 'But what you
gets in a good gumbo is everything.'
    -- "Witches Abroad" by Terry Prachett

Are you saying that the function assumes that a local pathname would not have '%' in it, returns its input as-is when it sees one, and if the caller really needs to express a path with '%' in it, it is the responsibility of the caller to escape it?

That makes it even more confusing....

In other words, if your DocumentRoot was /var/www and you have a directory /var/www/per%cent you want to expose to the outside world, you have to say "http://www.example.com/per%25cent" yourself and the "canonicalize" function will be an identity function?

I have this vague suspicion that Jonathan was asking about what your Git::SVN::Utils::canonicalize_path() sub does, so all of the above might be moot, though...

                                                        ^^^^^^^^

URI and path canonicalization are done differently and by different functions.
svn_uri_canonicalize() vs svn_dirent_canonicalize(). Or maybe you're
referring to the path portion of the URL? I don't think that makes a difference for what you're asking, but its important to keep in mind.

It appears that if the % is followed by hex it assumes its an escape. Otherwise it escapes it. Thus...

http://www.google.com/per%%nt -> http://www.google.com/per%25%25nt
http://www.google.com/per%ant -> http://www.google.com/per%25ant
http://www.google.com/per%cent -> http://www.google.com/per%CEnt

Which makes sense if the idea is to not double escape.

Straight out of the RFC.
http://pretty-rfc.herokuapp.com/RFC3986#when-to-percent-encode

Implementations must not percent-encode or decode the same string more than once, as decoding an already decoded string might lead to misinterpreting a percent data octet as the beginning of a percent- encoding, or vice versa in the case of percent-encoding an already percent-encoded string.

It makes it far simpler to use. You can't read the mind of the user, but its a fair guess that they're not really thinking too deeply about how escaping works. It makes URI and path canonicalization safer and simpler. Otherwise you'd need to keep track of whether a thing was already escaped or not! Just begging for loads of bugs. (If SVN were using URI and path objects they'd just take care of it and none of this would be a problem in the first place).

This way you have no double escaping concerns. No need to track if a thing is already canonicalized. Do it as often and as early as you like. Making a corner case a little harder is a small price to pay for making the common case much, much easier.

This also appears to be what Firefox does.

Yes. It can be made to work better.

There's a number of places in the code which effectively do this:

    my $full_url = $url . '/' . $path;

And I was canonicalizing them like this:

    my $full_url = canonicalize_url($url . '/' . $path);

I'd been pondering whether it would be worthwhile to have a function which added a path to a base URL and canonicalized. Now I see that yes, it would be to deal with this corner case.

    my $full_url = append_path_to_url($url, $path);

That would properly URI encode any % in $path before appending and then canonicalizing the whole thing as a URI.

I'm pretty sure the code in master doesn't handle this at all.

Its just a pass through to the SVN API.

44. I am not the atheist chaplain.
    -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
           http://skippyslist.com/list/