Fix git-svn for SVN 1.7

Fix git-svn for SVN 1.7

[Michael G. Schwern]

This patch series fixes git-svn for SVN 1.7 tested against SVN 1.7.5 and
1.6.18.  Patch 7/8 is where SVN 1.7 starts passing.

There is one exception.  t9100-git-svn-basic.sh fails 11-13.  This appears
to be due to a bug in SVN to do with symlinks.  Leave that for somebody
else, this is the final submission in the series.

The work was difficult because the code relies on simple string equalty
when comparing URLs and paths.  Turning on canonicalization in one part
of the code would cause another part to fail if it also did not
canonicalize.  There's likely still issues.

A better solution would be to have path and URL objects which overload
the eq operator and automatically stringify canonicalized and escaped.

This patch series should be placed on top of the previous which
made accessors canonicalize.  I'm getting a bit ahead of things
by submitting it now, but I'd like to get the review process
rolling.

[PATCH 1/8] SVN 1.7 will truncate "not-a%40{0}" to just "not-a".

[Michael G. Schwern]

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

Rather than guess what SVN is going to do for each version, make the test use
the branch name that was actually created.
---
 t/t9118-git-svn-funky-branch-names.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/t9118-git-svn-funky-branch-names.sh b/t/t9118-git-svn-funky-branch-names.sh
index 63fc982..193d3ca 100755
--- a/t/t9118-git-svn-funky-branch-names.sh
+++ b/t/t9118-git-svn-funky-branch-names.sh
@@ -32,6 +32,11 @@ 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.
+# Be sure to escape the @ if it shows up.
+non_reflog=`svn_cmd ls "$svnrepo/pr ject/branches" | grep not-a | sed 's/\///' | sed 's/@/%40/'`
+
 test_expect_success 'test clone with funky branch names' '
 	git svn clone -s "$svnrepo/pr ject" project &&
 	(
@@ -42,7 +47,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"
 	)
 	'
 
-- 
1.7.11.3

Re: [PATCH 1/8] SVN 1.7 will truncate "not-a%40{0}" to just "not-a".

[Jonathan Nieder]

Michael G. Schwern wrote:

> Rather than guess what SVN is going to do for each version, make the test use
> the branch name that was actually created.
[...]
> -		git rev-parse "refs/remotes/not-a%40{0}reflog"
> +		git rev-parse "refs/remotes/$non_reflog"

Doesn't this defeat the point of the testcase (checking that git-svn
is able to avoid creating git refs containing @{, following the rules
from git-check-ref-format(1))?

Do you know when SVN truncates the directory name?  Would historical
SVN repositories or historical SVN servers be able to have a directory
named with a %40 in it, or has this been disallowed completely,
leaving problematic historical repositories to be dumped with old SVN,
tweaked, and reloaded with new SVN?

Thanks,
Jonathan

Re: [PATCH 1/8] SVN 1.7 will truncate "not-a%40{0}" to just "not-a".

[Michael G Schwern]

On 2012.7.28 7:16 AM, Jonathan Nieder wrote:
> Michael G. Schwern wrote:
> 
>> Rather than guess what SVN is going to do for each version, make the test use
>> the branch name that was actually created.
> [...]
>> -		git rev-parse "refs/remotes/not-a%40{0}reflog"
>> +		git rev-parse "refs/remotes/$non_reflog"
> 
> Doesn't this defeat the point of the testcase (checking that git-svn
> is able to avoid creating git refs containing @{, following the rules
> from git-check-ref-format(1))?

Unless I messed up, entirely possible as I'm not a shell programmer, the test
is still useful for testing SVN 1.6.  Under SVN 1.6 $non_reflog should be
'not-a%40{0}reflog' as before.


> Do you know when SVN truncates the directory name?

IIRC its silently does it during the "svn cp".


> Would historical
> SVN repositories or historical SVN servers be able to have a directory
> named with a %40 in it, or has this been disallowed completely,
> leaving problematic historical repositories to be dumped with old SVN,
> tweaked, and reloaded with new SVN?

Dunno, lemme check...

$ source ~/bin/svn16
$ svnadmin --version
svnadmin, version 1.6.18 (r1303927)
...
$ svnadmin create svnrepo
$ mkdir project project/trunk project/branches project/tags
$ echo foo > project/trunk/foo
$ svn import -m 'test import' project
file:///Users/schwern/tmp/test/svnrepo/project
Adding         project/tags
Adding         project/trunk
Adding         project/trunk/foo
Adding         project/branches

Committed revision 1.
$ rm -rf project/
$ svn cp -m 'reflog' file:///Users/schwern/tmp/test/svnrepo/project/trunk
'file:///Users/schwern/tmp/test/svnrepo/project/branches/not-a%40{0}reflog'

Committed revision 2.
$ svn ls file:///Users/schwern/tmp/test/svnrepo/project/branches
not-a@{0}reflog/
$ source ~/bin/svn17
$ svn --version
svn, version 1.7.5 (r1336830)
...
$ svn ls file:///Users/schwern/tmp/test/svnrepo/project/branches
not-a@{0}reflog/

If you make it with SVN 1.6 its still there with SVN 1.7.  That's good, it
means you can ship a prebuilt repository and check it against SVN 1.7.

The bad news is the new code segfaults on it.  I don't know if that's the SVN
1.7 API choking on its own stuff or because of my changes or both.  If you set
up the test I can try and fix it.  Otherwise I'll just flounder in shell.


-- 
"I went to college, which is a lot like being in the Army, except when
 stupid people yell at me for stupid things, I can hit them."
    -- Jonathan Schwarz

[PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Jonathan Nieder]

This test script uses "svn cp" to create a branch with an @-sign in
its name:

	svn cp "pr ject/trunk" "pr ject/branches/not-a@{0}reflog"

That sets up for later tests that fetch the branch and check that git
svn mangles the refname appropriately.

Unfortunately, modern svn versions interpret path arguments with an
@-sign as an example of path@revision syntax (which pegs a path to a
particular revision) and truncate the path or error out with message
"svn: E205000: Syntax error parsing peg revision '{0}reflog'".

When using subversion 1.6.x, escaping the @ sign as %40 avoids trouble
(see 08fd28bb, 2010-07-08).  Newer versions are stricter:

	$ svn cp "$repo/pr ject/trunk" "$repo/pr ject/branches/not-a%40{reflog}"
	svn: E205000: Syntax error parsing peg revision '%7B0%7Dreflog'

The recommended method for escaping a literal @ sign in a path passed
to subversion is to add an empty peg revision at the end of the path
("branches/not-a@{0}reflog@").  Do that.

Pre-1.6.12 versions of Subversion probably treat the trailing @ as
another literal @-sign (svn issue 3651).  Luckily ever since
v1.8.0-rc0~155^2~7 (t9118: workaround inconsistency between SVN
versions, 2012-07-28) the test can survive that.

Tested with Debian Subversion 1.6.12dfsg-6 and 1.7.5-1 and r1395837
of Subversion trunk (1.8.x).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Michael G Schwern wrote:
> On 2012.7.28 7:16 AM, Jonathan Nieder wrote:
>> Michael G. Schwern wrote:

>>> -		git rev-parse "refs/remotes/not-a%40{0}reflog"
>>> +		git rev-parse "refs/remotes/$non_reflog"
>>
>> Doesn't this defeat the point of the testcase (checking that git-svn
>> is able to avoid creating git refs containing @{, following the rules
>> from git-check-ref-format(1))?
>
> Unless I messed up, entirely possible as I'm not a shell programmer, the test
> is still useful for testing SVN 1.6.  Under SVN 1.6 $non_reflog should be
> 'not-a%40{0}reflog' as before.

Here's a patch to make the test useful again for SVN 1.7.  Sensible?

 t/t9118-git-svn-funky-branch-names.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9118-git-svn-funky-branch-names.sh b/t/t9118-git-svn-funky-branch-names.sh
index 193d3cab..15f93b4c 100755
--- a/t/t9118-git-svn-funky-branch-names.sh
+++ b/t/t9118-git-svn-funky-branch-names.sh
@@ -28,7 +28,7 @@ test_expect_success 'setup svnrepo' '
 	svn_cmd cp -m "trailing .lock" "$svnrepo/pr ject/trunk" \
 			"$svnrepo/pr ject/branches/trailing_dotlock.lock" &&
 	svn_cmd cp -m "reflog" "$svnrepo/pr ject/trunk" \
-			"$svnrepo/pr ject/branches/not-a%40{0}reflog" &&
+			"$svnrepo/pr ject/branches/not-a@{0}reflog@" &&
 	start_httpd
 	'
 
-- 
1.7.10.4

Re: [PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 09.10.2012 10:41:
> This test script uses "svn cp" to create a branch with an @-sign in
> its name:
> 
> 	svn cp "pr ject/trunk" "pr ject/branches/not-a@{0}reflog"
> 
> That sets up for later tests that fetch the branch and check that git
> svn mangles the refname appropriately.
> 
> Unfortunately, modern svn versions interpret path arguments with an
> @-sign as an example of path@revision syntax (which pegs a path to a
> particular revision) and truncate the path or error out with message
> "svn: E205000: Syntax error parsing peg revision '{0}reflog'".
> 
> When using subversion 1.6.x, escaping the @ sign as %40 avoids trouble
> (see 08fd28bb, 2010-07-08).  Newer versions are stricter:
> 
> 	$ svn cp "$repo/pr ject/trunk" "$repo/pr ject/branches/not-a%40{reflog}"
> 	svn: E205000: Syntax error parsing peg revision '%7B0%7Dreflog'
> 
> The recommended method for escaping a literal @ sign in a path passed
> to subversion is to add an empty peg revision at the end of the path
> ("branches/not-a@{0}reflog@").  Do that.
> 
> Pre-1.6.12 versions of Subversion probably treat the trailing @ as
> another literal @-sign (svn issue 3651).  Luckily ever since
> v1.8.0-rc0~155^2~7 (t9118: workaround inconsistency between SVN
> versions, 2012-07-28) the test can survive that.
> 
> Tested with Debian Subversion 1.6.12dfsg-6 and 1.7.5-1 and r1395837
> of Subversion trunk (1.8.x).
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Tested with Subversion 1.6.18.

> ---
> Michael G Schwern wrote:
>> On 2012.7.28 7:16 AM, Jonathan Nieder wrote:
>>> Michael G. Schwern wrote:
> 
>>>> -		git rev-parse "refs/remotes/not-a%40{0}reflog"
>>>> +		git rev-parse "refs/remotes/$non_reflog"
>>>
>>> Doesn't this defeat the point of the testcase (checking that git-svn
>>> is able to avoid creating git refs containing @{, following the rules
>>> from git-check-ref-format(1))?
>>
>> Unless I messed up, entirely possible as I'm not a shell programmer, the test
>> is still useful for testing SVN 1.6.  Under SVN 1.6 $non_reflog should be
>> 'not-a%40{0}reflog' as before.
> 
> Here's a patch to make the test useful again for SVN 1.7.  Sensible?
> 
>  t/t9118-git-svn-funky-branch-names.sh |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t9118-git-svn-funky-branch-names.sh b/t/t9118-git-svn-funky-branch-names.sh
> index 193d3cab..15f93b4c 100755
> --- a/t/t9118-git-svn-funky-branch-names.sh
> +++ b/t/t9118-git-svn-funky-branch-names.sh
> @@ -28,7 +28,7 @@ test_expect_success 'setup svnrepo' '
>  	svn_cmd cp -m "trailing .lock" "$svnrepo/pr ject/trunk" \
>  			"$svnrepo/pr ject/branches/trailing_dotlock.lock" &&
>  	svn_cmd cp -m "reflog" "$svnrepo/pr ject/trunk" \
> -			"$svnrepo/pr ject/branches/not-a%40{0}reflog" &&
> +			"$svnrepo/pr ject/branches/not-a@{0}reflog@" &&
>  	start_httpd
>  	'

I haven't checked other svn versions but this approach looks perfectly
sensible. It makes us test branch names which can't even be created
easily with current svn. Does svn really deserve this much attention? ;)

Seriously, our tests prepare us well for an svn remote helper...

Re: [PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Jonathan Nieder]

Michael J Gruber wrote:
> Jonathan Nieder venit, vidit, dixit 09.10.2012 10:41:

>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Tested with Subversion 1.6.18.
[...]
> I haven't checked other svn versions but this approach looks perfectly
> sensible. It makes us test branch names which can't even be created
> easily with current svn. Does svn really deserve this much attention? ;)

Thanks for the quick and thorough feedback.  I'm glad to hear it seems
sane. ;-)

> Seriously, our tests prepare us well for an svn remote helper...

That might be a good reason to make a mock implementation of the
existing git-svn interface on top of git-remote-svn.  Sounds fun but
hard.

Ciao,
Jonathan

Re: [PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Eric Wong]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> Michael J Gruber wrote:
> > Jonathan Nieder venit, vidit, dixit 09.10.2012 10:41:
> 
> >> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> >
> > Tested with Subversion 1.6.18.

Thanks both.  Also pushed to "master" on git://bogomips.org/git-svn.git
(commit 44bc5ac71fd99f195bf1a3bea63c11139d2d535f)

Jonathan Nieder (2):
      git svn: work around SVN 1.7 mishandling of svn:special changes
      svn test: escape peg revision separator using empty peg rev

Re: [PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Jonathan Nieder]

Eric Wong wrote:

> Thanks both.  Also pushed to "master" on git://bogomips.org/git-svn.git
> (commit 44bc5ac71fd99f195bf1a3bea63c11139d2d535f)
>
> Jonathan Nieder (2):
>       git svn: work around SVN 1.7 mishandling of svn:special changes
>       svn test: escape peg revision separator using empty peg rev

Thanks.  Here's the $deletions nit as a patch on top.

-- >8 --
Subject: Git::SVN::Editor::T: pass $deletions to ->A and ->D

This shouldn't make a difference because the $deletions hash is
only used when adding a directory (see 379862ec, 2012-02-20) but
it's nice to be consistent to make reading smoother anyway.  No
functional change intended.

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

diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm
index 3bbc20a0..178920c8 100644
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -358,12 +358,12 @@ sub T {
 			mode_a => $m->{mode_a}, mode_b => '000000',
 			sha1_a => $m->{sha1_a}, sha1_b => '0' x 40,
 			chg => 'D', file_b => $m->{file_b}
-		});
+		}, $deletions);
 		$self->A({
 			mode_a => '000000', mode_b => $m->{mode_b},
 			sha1_a => '0' x 40, sha1_b => $m->{sha1_b},
 			chg => 'A', file_b => $m->{file_b}
-		});
+		}, $deletions);
 		return;
 	}
 
-- 
1.7.10.4

Re: [PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Eric Wong]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> Eric Wong wrote:
> 
> > Thanks both.  Also pushed to "master" on git://bogomips.org/git-svn.git
> > (commit 44bc5ac71fd99f195bf1a3bea63c11139d2d535f)
> >
> > Jonathan Nieder (2):
> >       git svn: work around SVN 1.7 mishandling of svn:special changes
> >       svn test: escape peg revision separator using empty peg rev
> 
> Thanks.  Here's the $deletions nit as a patch on top.
> 
> -- >8 --
> Subject: Git::SVN::Editor::T: pass $deletions to ->A and ->D

For future reference, it'd be slightly easier for me to apply if you
included the From: (and Date:) headers so I don't have to yank+paste
them myself :>

> This shouldn't make a difference because the $deletions hash is
> only used when adding a directory (see 379862ec, 2012-02-20) but
> it's nice to be consistent to make reading smoother anyway.  No
> functional change intended.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

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

And pushed to master on git://bogomips.org/git-svn.git
(commit a9608896587718549e82c5bae11740f2c0eac4c6)

Jonathan Nieder (3):
      git svn: work around SVN 1.7 mishandling of svn:special changes
      svn test: escape peg revision separator using empty peg rev
      Git::SVN::Editor::T: pass $deletions to ->A and ->D

Re: [PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Jonathan Nieder]

Eric Wong wrote:
> Jonathan Nieder <jrnieder@gmail.com> wrote:

>> -- >8 --
>> Subject: Git::SVN::Editor::T: pass $deletions to ->A and ->D
>
> For future reference, it'd be slightly easier for me to apply if you
> included the From: (and Date:) headers so I don't have to yank+paste
> them myself :>

Ah, I assumed you were using "git am --scissors".  Will do next time.

Jonathan

Re: [PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Eric Wong]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> Eric Wong wrote:
> > Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> >> -- >8 --
> >> Subject: Git::SVN::Editor::T: pass $deletions to ->A and ->D
> >
> > For future reference, it'd be slightly easier for me to apply if you
> > included the From: (and Date:) headers so I don't have to yank+paste
> > them myself :>
> 
> Ah, I assumed you were using "git am --scissors".  Will do next time.

I missed the addition of --scissors.  Will use it in the future :>

Re: [PATCH/RFC] svn test: escape peg revision separator using empty peg rev

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Michael J Gruber wrote:
>> > Jonathan Nieder venit, vidit, dixit 09.10.2012 10:41:
>> 
>> >> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> >
>> > Tested with Subversion 1.6.18.
>
> Thanks both.  Also pushed to "master" on git://bogomips.org/git-svn.git
> (commit 44bc5ac71fd99f195bf1a3bea63c11139d2d535f)
>
> Jonathan Nieder (2):
>       git svn: work around SVN 1.7 mishandling of svn:special changes
>       svn test: escape peg revision separator using empty peg rev

Thanks; pulled.

[PATCH 2/8] Fix typo in test

[Michael G. Schwern]

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

Test to check that the migration got rid of the old style git-svn directory.
It wasn't failing, just throwing a message to STDERR.
---
 t/t9107-git-svn-migrate.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 289fc31..cfb4453 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -32,7 +32,7 @@ test_expect_success 'initialize old-style (v0) git svn layout' '
 	echo "$svnrepo" > "$GIT_DIR"/git-svn/info/url &&
 	echo "$svnrepo" > "$GIT_DIR"/svn/info/url &&
 	git svn migrate &&
-	! test -d "$GIT_DIR"/git svn &&
+	! test -d "$GIT_DIR"/git-svn &&
 	git rev-parse --verify refs/${remotes_git_svn}^0 &&
 	git rev-parse --verify refs/remotes/svn^0 &&
 	test "$(git config --get svn-remote.svn.url)" = "$svnrepo" &&
-- 
1.7.11.3

[PATCH 3/8] Improve our URL canonicalization to be more like SVN 1.7's.

[Michael G. Schwern]

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

Previously, our URL canonicalization didn't do much of anything.
Now it actually escapes and collapses slashes.  This is mostly a cut & paste
of escape_url from git-svn.

This is closer to how SVN 1.7's canonicalization behaves.  Doing it with
1.6 lets us chase down some problems caused by more effective canonicalization
without having to deal with all the other 1.7 issues on top of that.

* Remote URLs have to be canonicalized otherwise Git::SVN->find_existing_remote
  will think they're different.

* The SVN remote is now written to the git config canonicalized.  That
  should be ok.  Adjust a test to account for that.
---
 perl/Git/SVN.pm                    |  4 ++--
 perl/Git/SVN/Utils.pm              | 19 +++++++++++++++++--
 t/Git-SVN/Utils/canonicalize_url.t | 26 ++++++++++++++++++++++++++
 t/t9107-git-svn-migrate.sh         |  4 +++-
 4 files changed, 48 insertions(+), 5 deletions(-)
 create mode 100644 t/Git-SVN/Utils/canonicalize_url.t

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 798f6c4..cb6d83a 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -201,9 +201,9 @@ sub read_all_remotes {
 		} elsif (m!^(.+)\.usesvmprops=\s*(.*)\s*$!) {
 			$r->{$1}->{svm} = {};
 		} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
-			$r->{$1}->{url} = $2;
+			$r->{$1}->{url} = canonicalize_url($2);
 		} elsif (m!^(.+)\.pushurl=\s*(.*)\s*$!) {
-			$r->{$1}->{pushurl} = $2;
+			$r->{$1}->{pushurl} = canonicalize_url($2);
 		} elsif (m!^(.+)\.ignore-refs=\s*(.*)\s*$!) {
 			$r->{$1}->{ignore_refs_regex} = $2;
 		} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index 7ae6fac..dab6e4d 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -147,10 +147,25 @@ sub canonicalize_url {
 }
 
 
+sub _canonicalize_url_path {
+	my ($uri_path) = @_;
+
+	my @parts;
+	foreach my $part (split m{/+}, $uri_path) {
+		$part =~ s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf("%%%02X",ord($1))/eg;
+		push @parts, $part;
+	}
+
+	return join('/', @parts);
+}
+
 sub _canonicalize_url_ourselves {
 	my ($url) = @_;
-	$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
-	return $url;
+	if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) {
+		my ($scheme, $domain, $uri) = ($1, $2, _canonicalize_url_path(canonicalize_path($3)));
+		$url = "$scheme://$domain$uri";
+	}
+	$url;
 }
 
 
diff --git a/t/Git-SVN/Utils/canonicalize_url.t b/t/Git-SVN/Utils/canonicalize_url.t
new file mode 100644
index 0000000..05795ab
--- /dev/null
+++ b/t/Git-SVN/Utils/canonicalize_url.t
@@ -0,0 +1,26 @@
+#!/usr/bin/env perl
+
+# Test our own home rolled URL canonicalizer.  Test the private one
+# directly because we can't predict what the SVN API is doing to do.
+
+use strict;
+use warnings;
+
+use Test::More 'no_plan';
+
+use Git::SVN::Utils;
+my $canonicalize_url = \&Git::SVN::Utils::_canonicalize_url_ourselves;
+
+my %tests = (
+	"http://x.com"			=> "http://x.com",
+	"http://x.com/"			=> "http://x.com",
+	"http://x.com/foo/bar"		=> "http://x.com/foo/bar",
+	"http://x.com//foo//bar//"	=> "http://x.com/foo/bar",
+	"http://x.com/  /%/"		=> "http://x.com/%20%20/%25",
+);
+
+for my $arg (keys %tests) {
+	my $want = $tests{$arg};
+
+	is $canonicalize_url->($arg), $want, "canonicalize_url('$arg') => $want";
+}
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index cfb4453..ee73013 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -27,6 +27,8 @@ test_expect_success 'setup old-looking metadata' '
 head=`git rev-parse --verify refs/heads/git-svn-HEAD^0`
 test_expect_success 'git-svn-HEAD is a real HEAD' "test -n '$head'"
 
+svnrepo_escaped=`echo $svnrepo | sed 's/ /%20/'`
+
 test_expect_success 'initialize old-style (v0) git svn layout' '
 	mkdir -p "$GIT_DIR"/git-svn/info "$GIT_DIR"/svn/info &&
 	echo "$svnrepo" > "$GIT_DIR"/git-svn/info/url &&
@@ -35,7 +37,7 @@ test_expect_success 'initialize old-style (v0) git svn layout' '
 	! test -d "$GIT_DIR"/git-svn &&
 	git rev-parse --verify refs/${remotes_git_svn}^0 &&
 	git rev-parse --verify refs/remotes/svn^0 &&
-	test "$(git config --get svn-remote.svn.url)" = "$svnrepo" &&
+	test "$(git config --get svn-remote.svn.url)" = "$svnrepo_escaped" &&
 	test `git config --get svn-remote.svn.fetch` = \
              ":refs/${remotes_git_svn}"
 	'
-- 
1.7.11.3

[PATCH 4/8] Replace hand rolled URL escapes with canonicalization

[Michael G. Schwern]

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

Continuing to move towards getting everything canonicalizing the same way.

* Git::SVN->init_remote_config and Git::SVN::Ra->minimize_url both
  have to canonicalize the same way else init_remote_config
  will incorrectly think they're different URLs causing
  t9107-git-svn-migrate.sh to fail.
---
 git-svn.perl       | 24 +++---------------------
 perl/Git/SVN.pm    |  2 +-
 perl/Git/SVN/Ra.pm | 27 +++++----------------------
 3 files changed, 9 insertions(+), 44 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 6e3e240..6e97545 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1412,24 +1412,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] : ".");
@@ -1457,18 +1439,18 @@ sub cmd_info {
 	my $full_url = $url . ($fullpath eq "" ? "" : "/$fullpath");
 
 	if ($_url) {
-		print escape_url($full_url), "\n";
+		print canonicalize_url($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: " . canonicalize_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";
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index cb6d83a..4219e5b 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -296,7 +296,7 @@ sub find_existing_remote {
 
 sub init_remote_config {
 	my ($self, $url, $no_write) = @_;
-	$url =~ s!/+$!!; # strip trailing slash
+	$url = canonicalize_url($url);
 	my $r = read_all_remotes();
 	my $existing = find_existing_remote($url, $r);
 	if ($existing) {
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index ef7b3dd..ed9dbe9 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -66,24 +66,6 @@ sub _auth_providers () {
 	\@rv;
 }
 
-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#^(https?)://([^/]+)(.*)$#) {
-		my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
-		$url = "$scheme://$domain$uri";
-	}
-	$url;
-}
 
 sub new {
 	my ($class, $url) = @_;
@@ -119,7 +101,7 @@ sub new {
 			$Git::SVN::Prompt::_no_auth_cache = 1;
 		}
 	} # no warnings 'once'
-	my $self = SVN::Ra->new(url => escape_url($url), auth => $baton,
+	my $self = SVN::Ra->new(url => canonicalize_url($url), auth => $baton,
 	                      config => $config,
 			      pool => SVN::Pool->new,
 	                      auth_provider_callbacks => $callbacks);
@@ -314,7 +296,7 @@ sub gs_do_switch {
 
 	if ($old_url =~ m#^svn(\+ssh)?://# ||
 	    ($full_url =~ m#^https?://# &&
-	     escape_url($full_url) ne $full_url)) {
+	     canonicalize_url($full_url) ne $full_url)) {
 		$_[0] = undef;
 		$self = undef;
 		$RA = undef;
@@ -327,7 +309,7 @@ sub gs_do_switch {
 	}
 
 	$ra ||= $self;
-	$url_b = escape_url($url_b);
+	$url_b = canonicalize_url($url_b);
 	my $reporter = $ra->do_switch($rev_b, '', 1, $url_b, $editor, $pool);
 	my @lock = (::compare_svn_version('1.2.0') >= 0) ? (undef) : ();
 	$reporter->set_path('', $rev_a, 0, @lock, $pool);
@@ -582,7 +564,8 @@ sub minimize_url {
 			$ra->get_log("", $latest, 0, 1, 0, 1, sub {});
 		};
 	} while ($@ && ($c = shift @components));
-	$url;
+
+	return canonicalize_url($url);
 }
 
 sub can_do_switch {
-- 
1.7.11.3

[PATCH 5/8] Canonicalize earlier in a couple spots.

[Michael G. Schwern]

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

Just a few things I noticed.  Its good to canonicalize as early as
possible.
---
 git-svn.perl       | 6 +++---
 perl/Git/SVN/Ra.pm | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 6e97545..6b90765 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1436,16 +1436,16 @@ 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 canonicalize_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: " . canonicalize_url($full_url) . "\n";
+	$result .= "URL: $full_url\n";
 
 	eval {
 		my $repos_root = $gs->repos_root;
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index ed9dbe9..eee7c00 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -69,7 +69,7 @@ sub _auth_providers () {
 
 sub new {
 	my ($class, $url) = @_;
-	$url =~ s!/+$!!;
+	$url = canonicalize_url($url);
 	return $RA if ($RA && $RA->url eq $url);
 
 	::_req_svn();
@@ -101,7 +101,7 @@ sub new {
 			$Git::SVN::Prompt::_no_auth_cache = 1;
 		}
 	} # no warnings 'once'
-	my $self = SVN::Ra->new(url => canonicalize_url($url), auth => $baton,
+	my $self = SVN::Ra->new(url => $url, auth => $baton,
 	                      config => $config,
 			      pool => SVN::Pool->new,
 	                      auth_provider_callbacks => $callbacks);
-- 
1.7.11.3

[PATCH 6/8] Add function to append a path to a URL.

[Michael G. Schwern]

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

Remove the ad-hoc versions.

This is mostly to normalize the process and ensure the URLs produced
don't have double slashes or anything.

Also provides a place to fix the corner case where a file path
contains a percent sign.
---
 git-svn.perl                      |  3 ++-
 perl/Git/SVN.pm                   | 33 +++++++++++++++------------------
 perl/Git/SVN/Ra.pm                |  8 ++++----
 perl/Git/SVN/Utils.pm             | 27 +++++++++++++++++++++++++++
 t/Git-SVN/Utils/add_path_to_url.t | 27 +++++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 23 deletions(-)
 create mode 100644 t/Git-SVN/Utils/add_path_to_url.t

diff --git a/git-svn.perl b/git-svn.perl
index 6b90765..3d120d5 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -35,6 +35,7 @@ use Git::SVN::Utils qw(
 	canonicalize_path
 	canonicalize_url
 	join_paths
+	add_path_to_url
 );
 
 use Git qw(
@@ -1436,7 +1437,7 @@ sub cmd_info {
 	# canonicalize_path() will return "" to make libsvn 1.5.x happy,
 	$path = "." if $path eq "";
 
-	my $full_url = canonicalize_url( $url . ($fullpath eq "" ? "" : "/$fullpath") );
+	my $full_url = canonicalize_url( add_path_to_url( $url, $fullpath ) );
 
 	if ($_url) {
 		print "$full_url\n";
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 4219e5b..22bf207 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -29,6 +29,7 @@ use Git::SVN::Utils qw(
 	join_paths
 	canonicalize_path
 	canonicalize_url
+	add_path_to_url
 );
 
 my $can_use_yaml;
@@ -564,8 +565,7 @@ sub _set_svm_vars {
 		# username is of no interest
 		$src =~ s{(^[a-z\+]*://)[^/@]*@}{$1};
 
-		my $replace = $ra->url;
-		$replace .= "/$path" if length $path;
+		my $replace = add_path_to_url($ra->url, $path);
 
 		my $section = "svn-remote.$self->{repo_id}";
 		tmp_config("$section.svm-source", $src);
@@ -582,7 +582,7 @@ sub _set_svm_vars {
 	my $path = $self->path;
 	my %tried;
 	while (length $path) {
-		my $try = $self->url . "/$path";
+		my $try = add_path_to_url($self->url, $path);
 		unless ($tried{$try}) {
 			return $ra if $self->read_svm_props($ra, $path, $r);
 			$tried{$try} = 1;
@@ -591,7 +591,7 @@ sub _set_svm_vars {
 	}
 	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{ add_path_to_url($self->url, $path) } = 1;
 
 	if ($ra->{repos_root} eq $self->url) {
 		die @err, (map { "  $_\n" } keys %tried), "\n";
@@ -603,7 +603,7 @@ sub _set_svm_vars {
 	$path = $ra->{svn_path};
 	$ra = Git::SVN::Ra->new($ra->{repos_root});
 	while (length $path) {
-		my $try = $ra->url ."/$path";
+		my $try = add_path_to_url($ra->url, $path);
 		unless ($tried{$try}) {
 			$ok = $self->read_svm_props($ra, $path, $r);
 			last if $ok;
@@ -613,7 +613,7 @@ sub _set_svm_vars {
 	}
 	die "Path: '$path' should be ''\n" if $path ne '';
 	$ok ||= $self->read_svm_props($ra, $path, $r);
-	$tried{$ra->url ."/$path"} = 1;
+	$tried{ add_path_to_url($ra->url, $path) } = 1;
 	if (!$ok) {
 		die @err, (map { "  $_\n" } keys %tried), "\n";
 	}
@@ -933,20 +933,19 @@ sub rewrite_uuid {
 
 sub metadata_url {
 	my ($self) = @_;
-	($self->rewrite_root || $self->url) .
-	   (length $self->path ? '/' . $self->path : '');
+	my $url = $self->rewrite_root || $self->url;
+	return add_path_to_url( $url, $self->path );
 }
 
 sub full_url {
 	my ($self) = @_;
-	$self->url . (length $self->path ? '/' . $self->path : '');
+	return add_path_to_url( $self->url, $self->path );
 }
 
 sub full_pushurl {
 	my ($self) = @_;
 	if ($self->{pushurl}) {
-		return $self->{pushurl} . (length $self->path ? '/' .
-		       $self->path : '');
+		return add_path_to_url( $self->{pushurl}, $self->path );
 	} else {
 		return $self->full_url;
 	}
@@ -1114,7 +1113,7 @@ sub find_parent_branch {
 	my $r = $i->{copyfrom_rev};
 	my $repos_root = $self->ra->{repos_root};
 	my $url = $self->ra->url;
-	my $new_url = $url . $branch_from;
+	my $new_url = add_path_to_url( $url, $branch_from );
 	print STDERR  "Found possible branch point: ",
 	              "$new_url => ", $self->full_url, ", $r\n"
 	              unless $::_q > 1;
@@ -1443,12 +1442,11 @@ 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 $repos_root = $url;
+			my $repos_root = $self->url;
 			my $branch_from = $path;
 			$branch_from =~ s{^/}{};
-			my $gs = $self->other_gs($repos_root."/".$branch_from,
-			                         $url,
+			my $gs = $self->other_gs(add_path_to_url( $repos_root, $branch_from ),
+			                         $repos_root,
 			                         $branch_from,
 			                         $rev,
 			                         $self->{ref_id});
@@ -1871,8 +1869,7 @@ sub make_log_entry {
 		$email ||= "$author\@$uuid";
 		$commit_email ||= "$author\@$uuid";
 	} elsif ($self->use_svnsync_props) {
-		my $full_url = $self->svnsync->{url};
-		$full_url .= "/".$self->path if length $self->path;
+		my $full_url = add_path_to_url( $self->svnsync->{url}, $self->path );
 		remove_username($full_url);
 		my $uuid = $self->svnsync->{uuid};
 		$log_entry{metadata} = "$full_url\@$rev $uuid";
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index eee7c00..788e642 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -5,6 +5,7 @@ use warnings;
 use SVN::Client;
 use Git::SVN::Utils qw(
 	canonicalize_url
+	add_path_to_url
 );
 
 use SVN::Ra;
@@ -289,9 +290,8 @@ sub gs_do_switch {
 	my $path = $gs->path;
 	my $pool = SVN::Pool->new;
 
-	my $full_url = $self->url;
-	my $old_url = $full_url;
-	$full_url .= '/' . $path if length $path;
+	my $old_url = $self->url;
+	my $full_url = add_path_to_url( $self->url, $path );
 	my ($ra, $reparented);
 
 	if ($old_url =~ m#^svn(\+ssh)?://# ||
@@ -557,7 +557,7 @@ sub minimize_url {
 	my @components = split(m!/!, $self->{svn_path});
 	my $c = '';
 	do {
-		$url .= "/$c" if length $c;
+		$url = add_path_to_url($url, $c);
 		eval {
 			my $ra = (ref $self)->new($url);
 			my $latest = $ra->get_latest_revnum;
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index dab6e4d..3aec207 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -13,6 +13,7 @@ our @EXPORT_OK = qw(
 	canonicalize_path
 	canonicalize_url
 	join_paths
+	add_path_to_url
 );
 
 
@@ -200,4 +201,30 @@ sub join_paths {
 	return $new_path .= "/$last_path";
 }
 
+
+=head3 add_path_to_url
+
+    my $new_url = add_path_to_url($url, $path);
+
+Appends $path onto the $url.  If $path is empty, $url is returned unchanged.
+
+=cut
+
+sub add_path_to_url {
+	my($url, $path) = @_;
+
+	return $url if !defined $path or !length $path;
+
+	# Strip trailing and leading slashes so we don't
+	# wind up with http://x.com///path
+	$url  =~ s{/+$}{};
+	$path =~ s{^/+}{};
+
+	# If a path has a % in it, URI escape it so it's not
+	# mistaken for a URI escape later.
+	$path =~ s{%}{%25}g;
+
+	return join '/', $url, $path;
+}
+
 1;
diff --git a/t/Git-SVN/Utils/add_path_to_url.t b/t/Git-SVN/Utils/add_path_to_url.t
new file mode 100644
index 0000000..bfbd878
--- /dev/null
+++ b/t/Git-SVN/Utils/add_path_to_url.t
@@ -0,0 +1,27 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use Test::More 'no_plan';
+
+use Git::SVN::Utils qw(
+	add_path_to_url
+);
+
+# A reference cannot be a hash key, so we use an array.
+my @tests = (
+	["http://x.com", "bar"]			=> 'http://x.com/bar',
+	["http://x.com", ""]			=> 'http://x.com',
+	["http://x.com/foo/", undef]		=> 'http://x.com/foo/',
+	["http://x.com/foo/", "/bar/baz/"]	=> 'http://x.com/foo/bar/baz/',
+	["http://x.com", 'per%cent']		=> 'http://x.com/per%25cent',
+);
+
+while(@tests) {
+	my($have, $want) = splice @tests, 0, 2;
+
+	my $args = join ", ", map { qq['$_'] } map { defined($_) ? $_ : 'undef' } @$have;
+	my $name = "add_path_to_url($args) eq $want";
+	is add_path_to_url(@$have), $want, $name;
+}
-- 
1.7.11.3

[PATCH 7/8] Turn on canonicalization on newly minted URLs.

[Michael G. Schwern]

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

Go through all the spots that use the new add_path_to_url() to
make a new URL and canonicalize them.

* copyfrom_path has to be canonicalized else find_parent_branch
  will get confused

* due to the `canonicalize_url($full_url) ne $full_url)` line of
  logic in gs_do_switch(), $full_url is left alone until after.

At this point SVN 1.7 passes except for 3 tests in
t9100-git-svn-basic.sh that look like an SVN bug to do with
symlinks.
---
 perl/Git/SVN.pm    | 19 ++++++++++++++-----
 perl/Git/SVN/Ra.pm |  9 ++++++++-
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 22bf207..e5f7acc 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -362,6 +362,8 @@ sub init_remote_config {
 sub find_by_url { # repos_root and, path are optional
 	my ($class, $full_url, $repos_root, $path) = @_;
 
+	$full_url = canonicalize_url($full_url);
+
 	return undef unless defined $full_url;
 	remove_username($full_url);
 	remove_username($repos_root) if defined $repos_root;
@@ -400,6 +402,11 @@ sub find_by_url { # repos_root and, path are optional
 			}
 			$p =~ s#^\Q$z\E(?:/|$)#$prefix# or next;
 		}
+
+		# remote fetch paths are not URI escaped.  Decode ours
+		# so they match
+		$p = uri_decode($p);
+
 		foreach my $f (keys %$fetch) {
 			next if $f ne $p;
 			return Git::SVN->new($fetch->{$f}, $repo_id, $f);
@@ -934,18 +941,18 @@ sub rewrite_uuid {
 sub metadata_url {
 	my ($self) = @_;
 	my $url = $self->rewrite_root || $self->url;
-	return add_path_to_url( $url, $self->path );
+	return canonicalize_url( add_path_to_url( $url, $self->path ) );
 }
 
 sub full_url {
 	my ($self) = @_;
-	return add_path_to_url( $self->url, $self->path );
+	return canonicalize_url( add_path_to_url( $self->url, $self->path ) );
 }
 
 sub full_pushurl {
 	my ($self) = @_;
 	if ($self->{pushurl}) {
-		return add_path_to_url( $self->{pushurl}, $self->path );
+		return canonicalize_url( add_path_to_url( $self->{pushurl}, $self->path ) );
 	} else {
 		return $self->full_url;
 	}
@@ -1113,7 +1120,7 @@ sub find_parent_branch {
 	my $r = $i->{copyfrom_rev};
 	my $repos_root = $self->ra->{repos_root};
 	my $url = $self->ra->url;
-	my $new_url = add_path_to_url( $url, $branch_from );
+	my $new_url = canonicalize_url( add_path_to_url( $url, $branch_from ) );
 	print STDERR  "Found possible branch point: ",
 	              "$new_url => ", $self->full_url, ", $r\n"
 	              unless $::_q > 1;
@@ -1869,7 +1876,9 @@ sub make_log_entry {
 		$email ||= "$author\@$uuid";
 		$commit_email ||= "$author\@$uuid";
 	} elsif ($self->use_svnsync_props) {
-		my $full_url = add_path_to_url( $self->svnsync->{url}, $self->path );
+		my $full_url = canonicalize_url(
+			add_path_to_url( $self->svnsync->{url}, $self->path )
+		);
 		remove_username($full_url);
 		my $uuid = $self->svnsync->{uuid};
 		$log_entry{metadata} = "$full_url\@$rev $uuid";
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 788e642..29b46e8 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -5,6 +5,7 @@ use warnings;
 use SVN::Client;
 use Git::SVN::Utils qw(
 	canonicalize_url
+	canonicalize_path
 	add_path_to_url
 );
 
@@ -102,6 +103,7 @@ sub new {
 			$Git::SVN::Prompt::_no_auth_cache = 1;
 		}
 	} # no warnings 'once'
+
 	my $self = SVN::Ra->new(url => $url, auth => $baton,
 	                      config => $config,
 			      pool => SVN::Pool->new,
@@ -200,6 +202,7 @@ sub get_log {
 				qw/copyfrom_path copyfrom_rev action/;
 			if ($s{'copyfrom_path'}) {
 				$s{'copyfrom_path'} =~ s/$prefix_regex//;
+				$s{'copyfrom_path'} = canonicalize_path($s{'copyfrom_path'});
 			}
 			$_[0]{$p} = \%s;
 		}
@@ -303,7 +306,11 @@ sub gs_do_switch {
 		$ra = Git::SVN::Ra->new($full_url);
 		$ra_invalid = 1;
 	} elsif ($old_url ne $full_url) {
-		SVN::_Ra::svn_ra_reparent($self->{session}, $full_url, $pool);
+		SVN::_Ra::svn_ra_reparent(
+			$self->{session},
+			canonicalize_url($full_url),
+			$pool
+		);
 		$self->url($full_url);
 		$reparented = 1;
 	}
-- 
1.7.11.3

[PATCH/RFC] test: work around SVN 1.7 mishandling of svn:special changes

[Jonathan Nieder]

Subversion represents symlinks as ordinary files with content
starting with "link " and the svn:special property set to "*".  Thus a
file can switch between being a symlink and a non-symlink simply by
toggling its svn:special property, and new checkouts will
automatically write a file of the appropriate type.  Likewise, in
subversion 1.6 and older, running "svn update" would notice changes
in filetype and update the working copy appropriately.

Unfortunately, starting in subversion 1.7 ,changes to the svn:special
property trip an assertion instead:

	$ svn up svn-tree
	Updating 'svn-tree':
	svn: E235000: In file 'subversion/libsvn_wc/update_editor.c' \
	line 1583: assertion failed (action == svn_wc_conflict_action_edit \
	|| action == svn_wc_conflict_action_delete || action == \
	svn_wc_conflict_action_replace)

This is a known bug in "svn update" (Subversion issue 4091) and for
the sake of old repositories it will need to be fixed some day.

Revisions prepared with ordinary svn commands ("svn add" and not "svn
propset") don't trip this because they represent filetype changes
using a replace operation, which is approximately equivalent to
removal followed by adding a new file and works fine.  Perhaps "git
svn" should mimic that, but for now let's teach the test suite to
recover from the bug by testing the content of HEAD with a new
checkout.

After this change, tests t9100.11-13 pass again.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi Eric,

Michael G. Schwern wrote:

> At this point SVN 1.7 passes except for 3 tests in
> t9100-git-svn-basic.sh that look like an SVN bug to do with
> symlinks.

How about this patch?

I didn't add a new xfail test for "svn up" working because I'm not yet
sure what good git-svn behavior would be.  Probably it would be better
to track down that svn bug and get a fix backported to the 1.7.x
branch.

Reference: http://subversion.tigris.org/issues/show_bug.cgi?id=4160

 t/t9100-git-svn-basic.sh |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 749b75e8..34d3485f 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -19,6 +19,15 @@ case "$GIT_SVN_LC_ALL" in
 	;;
 esac
 
+svn_up_avoiding_issue4091 () {
+	if ! svn_cmd_up "$SVN_TREE"
+	then
+		# work around Subversion issue 4091
+		rm -r "$SVN_TREE" &&
+		svn_cmd checkout "$svnrepo" "$SVN_TREE"
+	fi
+}
+
 test_expect_success \
     'initialize git svn' '
 	mkdir import &&
@@ -148,7 +157,7 @@ test_expect_success "$name" '
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
 		${remotes_git_svn}..mybranch5 &&
-	svn_cmd up "$SVN_TREE" &&
+	svn_up_avoiding_issue4091 &&
 	test -h "$SVN_TREE"/exec.sh'
 
 name='new symlink is added to a file that was also just made executable'
@@ -173,7 +182,7 @@ test_expect_success "$name" '
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
 		${remotes_git_svn}..mybranch5 &&
-	svn_cmd up "$SVN_TREE" &&
+	svn_up_avoiding_issue4091 &&
 	test -f "$SVN_TREE"/exec-2.sh &&
 	test ! -h "$SVN_TREE"/exec-2.sh &&
 	test_cmp help "$SVN_TREE"/exec-2.sh'
-- 
1.7.10.4

[PATCH/RFC v2] git svn: work around SVN 1.7 mishandling of svn:special changes

[Jonathan Nieder]

Subversion represents symlinks as ordinary files with content starting
with "link " and the svn:special property set to "*".  Thus a file can
switch between being a symlink and a non-symlink simply by toggling
its svn:special property, and new checkouts will automatically write a
file of the appropriate type.  Likewise, in subversion 1.6 and older,
running "svn update" would notice changes in filetype and update the
working copy appropriately.

Starting in subversion 1.7 (issue 4091), changes to the svn:special
property trip an assertion instead:

	$ svn up svn-tree
	Updating 'svn-tree':
	svn: E235000: In file 'subversion/libsvn_wc/update_editor.c' \
	line 1583: assertion failed (action == svn_wc_conflict_action_edit \
	|| action == svn_wc_conflict_action_delete || action == \
	svn_wc_conflict_action_replace)

Revisions prepared with ordinary svn commands ("svn add" and not "svn
propset") don't trip this because they represent these filetype
changes using a replace operation, which is approximately equivalent
to removal followed by adding a new file and works fine.  Follow suit.

Noticed using t9100.  After this change, git-svn's file-to-symlink
changes are sent in a format that modern "svn update" can handle and
tests t9100.11-13 pass again.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:

> Revisions prepared with ordinary svn commands ("svn add" and not "svn
> propset") don't trip this because they represent filetype changes
> using a replace operation [...]
>                                                        Perhaps "git
> svn" should mimic that,

... and here's what that looks like.  I like this more than ignoring
the problem in tests, and I suppose something like this is necessary
regardless of how quickly issue 4091 is fixed for compatibility with
the broken versions of svn.

It would be nice if the patch could be more concise, though.  What do
you think?

 git-svn.perl |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/git-svn.perl b/git-svn.perl
index 9d57aa0c..40ccdd50 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5439,7 +5439,30 @@ sub M {
 	$self->close_file($fbat,undef,$self->{pool});
 }
 
-sub T { shift->M(@_) }
+sub T {
+	my ($self, $m, $deletions) = @_;
+
+	# Work around subversion issue 4091: toggling the "is a
+	# symlink" property requires removing and re-adding a
+	# file or else "svn up" on affected clients trips an
+	# assertion and aborts.
+	if (($m->{mode_b} =~ /^120/ && $m->{mode_a} !~ /^120/) ||
+	    ($m->{mode_b} !~ /^120/ && $m->{mode_a} =~ /^120/)) {
+		$self->D({
+			mode_a => $m->{mode_a}, mode_b => '000000',
+			sha1_a => $m->{sha1_a}, sha1_b => '0' x 40,
+			chg => 'D', file_b => $m->{file_b}
+		});
+		$self->A({
+			mode_a => '000000', mode_b => $m->{mode_b},
+			sha1_a => '0' x 40, sha1_b => $m->{sha1_b},
+			chg => 'A', file_b => $m->{file_b}
+		});
+		return;
+	}
+
+	$self->M($m, $deletions);
+}
 
 sub change_file_prop {
 	my ($self, $fbat, $pname, $pval) = @_;
-- 
1.7.10.4

Re: [PATCH/RFC v2] git svn: work around SVN 1.7 mishandling of svn:special changes

[Eric Wong]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
> 
> > Revisions prepared with ordinary svn commands ("svn add" and not "svn
> > propset") don't trip this because they represent filetype changes
> > using a replace operation [...]
> >                                                        Perhaps "git
> > svn" should mimic that,
> 
> ... and here's what that looks like.  I like this more than ignoring
> the problem in tests, and I suppose something like this is necessary
> regardless of how quickly issue 4091 is fixed for compatibility with
> the broken versions of svn.

I prefer this v2 more than ignoring the problem in tests, too.

> It would be nice if the patch could be more concise, though.  What do
> you think?

I think it's fine.

>  git-svn.perl |   25 ++++++++++++++++++++++++-

I needed to filter the patch through:

    s,git-svn\.perl,perl/Git/SVN/Editor.pm,g

though...  Will push the edited version to my master on
git://bogomips.org/git-svn

>From b8c78e2a9d6141589202e98b898f477861fcb111 Mon Sep 17 00:00:00 2001
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Tue, 9 Oct 2012 03:12:39 -0700
Subject: [PATCH] git svn: work around SVN 1.7 mishandling of svn:special
 changes

Subversion represents symlinks as ordinary files with content starting
with "link " and the svn:special property set to "*".  Thus a file can
switch between being a symlink and a non-symlink simply by toggling
its svn:special property, and new checkouts will automatically write a
file of the appropriate type.  Likewise, in subversion 1.6 and older,
running "svn update" would notice changes in filetype and update the
working copy appropriately.

Starting in subversion 1.7 (issue 4091), changes to the svn:special
property trip an assertion instead:

	$ svn up svn-tree
	Updating 'svn-tree':
	svn: E235000: In file 'subversion/libsvn_wc/update_editor.c' \
	line 1583: assertion failed (action == svn_wc_conflict_action_edit \
	|| action == svn_wc_conflict_action_delete || action == \
	svn_wc_conflict_action_replace)

Revisions prepared with ordinary svn commands ("svn add" and not "svn
propset") don't trip this because they represent these filetype
changes using a replace operation, which is approximately equivalent
to removal followed by adding a new file and works fine.  Follow suit.

Noticed using t9100.  After this change, git-svn's file-to-symlink
changes are sent in a format that modern "svn update" can handle and
tests t9100.11-13 pass again.

[ew: s,git-svn\.perl,perl/Git/SVN/Editor.pm,g]

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

diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm
index 755092f..3bbc20a 100644
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -345,7 +345,30 @@ sub M {
 	$self->close_file($fbat,undef,$self->{pool});
 }
 
-sub T { shift->M(@_) }
+sub T {
+	my ($self, $m, $deletions) = @_;
+
+	# Work around subversion issue 4091: toggling the "is a
+	# symlink" property requires removing and re-adding a
+	# file or else "svn up" on affected clients trips an
+	# assertion and aborts.
+	if (($m->{mode_b} =~ /^120/ && $m->{mode_a} !~ /^120/) ||
+	    ($m->{mode_b} !~ /^120/ && $m->{mode_a} =~ /^120/)) {
+		$self->D({
+			mode_a => $m->{mode_a}, mode_b => '000000',
+			sha1_a => $m->{sha1_a}, sha1_b => '0' x 40,
+			chg => 'D', file_b => $m->{file_b}
+		});
+		$self->A({
+			mode_a => '000000', mode_b => $m->{mode_b},
+			sha1_a => '0' x 40, sha1_b => $m->{sha1_b},
+			chg => 'A', file_b => $m->{file_b}
+		});
+		return;
+	}
+
+	$self->M($m, $deletions);
+}
 
 sub change_file_prop {
 	my ($self, $fbat, $pname, $pval) = @_;
-- 
1.8.0.rc0.42.gb8c78e2

[PATCH v3] git svn: work around SVN 1.7 mishandling of svn:special changes

[Jonathan Nieder]

Subversion represents symlinks as ordinary files with content starting
with "link " and the svn:special property set to "*".  Thus a file can
switch between being a symlink and a non-symlink simply by toggling
its svn:special property, and new checkouts will automatically write a
file of the appropriate type.  Likewise, in subversion 1.6 and older,
running "svn update" would notice changes in filetype and update the
working copy appropriately.

Starting in subversion 1.7 (issue 4091), changes to the svn:special
property trip an assertion instead:

	$ svn up svn-tree
	Updating 'svn-tree':
	svn: E235000: In file 'subversion/libsvn_wc/update_editor.c' \
	line 1583: assertion failed (action == svn_wc_conflict_action_edit \
	|| action == svn_wc_conflict_action_delete || action == \
	svn_wc_conflict_action_replace)

Revisions prepared with ordinary svn commands ("svn add" and not "svn
propset") don't trip this because they represent these filetype
changes using a replace operation, which is approximately equivalent
to removal followed by adding a new file and works fine.  Follow suit.

Noticed using t9100.  After this change, git-svn's file-to-symlink
changes are sent in a format that modern "svn update" can handle and
tests t9100.11-13 pass again.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Eric Wong wrote:

> I needed to filter the patch through:
>
>     s,git-svn\.perl,perl/Git/SVN/Editor.pm,g
>
> though...

Yeah, good catch.  Here's a v3 tested against "master".  Unlike in v2,
it remembers to pass the $deletions parameter to D() and A() --- which
shouldn't make a difference because we are not adding a directory, but
it's nice to be consistent to make reading smoother.

 perl/Git/SVN/Editor.pm |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm
index 755092fd..178920c8 100644
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -345,7 +345,30 @@ sub M {
 	$self->close_file($fbat,undef,$self->{pool});
 }
 
-sub T { shift->M(@_) }
+sub T {
+	my ($self, $m, $deletions) = @_;
+
+	# Work around subversion issue 4091: toggling the "is a
+	# symlink" property requires removing and re-adding a
+	# file or else "svn up" on affected clients trips an
+	# assertion and aborts.
+	if (($m->{mode_b} =~ /^120/ && $m->{mode_a} !~ /^120/) ||
+	    ($m->{mode_b} !~ /^120/ && $m->{mode_a} =~ /^120/)) {
+		$self->D({
+			mode_a => $m->{mode_a}, mode_b => '000000',
+			sha1_a => $m->{sha1_a}, sha1_b => '0' x 40,
+			chg => 'D', file_b => $m->{file_b}
+		}, $deletions);
+		$self->A({
+			mode_a => '000000', mode_b => $m->{mode_b},
+			sha1_a => '0' x 40, sha1_b => $m->{sha1_b},
+			chg => 'A', file_b => $m->{file_b}
+		}, $deletions);
+		return;
+	}
+
+	$self->M($m, $deletions);
+}
 
 sub change_file_prop {
 	my ($self, $fbat, $pname, $pval) = @_;
-- 
1.7.10.4

[PATCH 8/8] Remove some ad hoc canonicalizations.

[Michael G. Schwern]

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

---
 git-svn.perl    | 8 ++++----
 perl/Git/SVN.pm | 1 -
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 3d120d5..56d1ba7 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -36,6 +36,7 @@ use Git::SVN::Utils qw(
 	canonicalize_url
 	join_paths
 	add_path_to_url
+	join_paths
 );
 
 use Git qw(
@@ -1598,7 +1599,7 @@ sub post_fetch_checkout {
 
 sub complete_svn_url {
 	my ($url, $path) = @_;
-	$path =~ s#/+$##;
+	$path = canonicalize_path($path);
 
 	# If the path is not a URL...
 	if ($path !~ m#^[a-z\+]+://#) {
@@ -1617,7 +1618,7 @@ 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 = '';
@@ -1638,9 +1639,8 @@ sub complete_url_ls_init {
 	}
 	command_oneline('config', $k, $gs->url) unless $orig_url;
 
-	my $remote_path = $gs->path . "/$repo_path";
+	my $remote_path = join_paths( $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+)/);
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index e5f7acc..3c68c09 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -460,7 +460,6 @@ sub new {
 	}
 	{
 		my $path = $self->path;
-		$path =~ s{/+}{/}g;
 		$path =~ s{\A/}{};
 		$path =~ s{/\z}{};
 		$self->path($path);
-- 
1.7.11.3

Re: Fix git-svn for SVN 1.7

[Eric Wong]

"Michael G. Schwern" <schwern@pobox.com> wrote:
> There is one exception.  t9100-git-svn-basic.sh fails 11-13.  This appears
> to be due to a bug in SVN to do with symlinks.  Leave that for somebody
> else, this is the final submission in the series.

That's fine, a few failing tests is better than completely failing.

> The work was difficult because the code relies on simple string equalty
> when comparing URLs and paths.  Turning on canonicalization in one part
> of the code would cause another part to fail if it also did not
> canonicalize.  There's likely still issues.
> 
> A better solution would be to have path and URL objects which overload
> the eq operator and automatically stringify canonicalized and escaped.

Perhaps we can depend on the URI.pm module?  It seems to be
widely-available and not be a significant barrier to installation.  On
the other hand, I don't know its history, either (especially since we're
now dealing with SVN changes...).

Anyways, I don't like relying on operator overloading, it makes code
harder to read and review.

Re: Fix git-svn for SVN 1.7

[Michael G Schwern]

On 2012.7.30 1:38 PM, Eric Wong wrote:
>> A better solution would be to have path and URL objects which overload
>> the eq operator and automatically stringify canonicalized and escaped.
> 
> Perhaps we can depend on the URI.pm module?  It seems to be
> widely-available and not be a significant barrier to installation.  On
> the other hand, I don't know its history, either (especially since we're
> now dealing with SVN changes...).

If you want to go down the road of having CPAN dependencies, then it should
definitely be used rather than rolling our own and generating our own bugs.
It's a very commonly needed Perl module.

You'd make a subclass and put any special work arounds for SVN in there.


> Anyways, I don't like relying on operator overloading, it makes code
> harder to read and review.

Right now, canonicalization is a bug generator.  Paths and URLs have to be in
the same form when they're compared.  This requires meticulous care on the
part of the coder and reviewer to check every comparison.  It scatters the
logic for proper comparison all over the code.  Redundant logic scattered
around the code is a Bad Thing.  It makes it more likely a coder will forget
the logic, or get it wrong, and a human reviewer must be far more vigilant.

Right now I'm pretty sure there's still a ton of bugs.

It also slows things down.  As strings, URLs and paths have to be
canonicalized every time they're used or compared.  An object representing the
URI or path can cache the canonicalization.

With string comparison overloaded, you'd no longer have to meticulously check
that URLs and paths are always in the same form when they're compared.  It
just does it.  The logic is in one place.  We don't even have to care if one
of them is a string (or which one), it works even if only one half of the
comparison is an object.  A new coder to the project doesn't need to know
anything special about URIs and paths, they just treat them as strings.
Finally, they can be slipped into existing code without having to rewrite
everything.

Overloaded comparison and stringification can even be used as a tool to find
all the places in the code where URLs and paths are being used, where they're
being turned into strings, and where URL and path manipulation is being done
ad-hoc.  For example, if comparison sees one of its arguments as a string, or
if concatenation is used.

The only downside is when chasing down a bug related to canonicalization one
might have to realize that eq is overloaded.  But we'd have far less bugs due
to canonicalization.  So worth it.


-- 
Being faith-based doesn't trump reality.
	-- Bruce Sterling

Re: Fix git-svn for SVN 1.7

[Eric Wong]

Michael G Schwern <schwern@pobox.com> wrote:
> On 2012.7.30 1:38 PM, Eric Wong wrote:
> > Anyways, I don't like relying on operator overloading, it makes code
> > harder to read and review.
> 
> Right now, canonicalization is a bug generator.  Paths and URLs have to be in
> the same form when they're compared.  This requires meticulous care on the
> part of the coder and reviewer to check every comparison.  It scatters the
> logic for proper comparison all over the code.  Redundant logic scattered
> around the code is a Bad Thing.  It makes it more likely a coder will forget
> the logic, or get it wrong, and a human reviewer must be far more vigilant.

<snip>  I agree completely with canonicalization.

> The only downside is when chasing down a bug related to canonicalization one
> might have to realize that eq is overloaded.

Having to realize eq is overloaded is a huge downside to me.

Re: Fix git-svn for SVN 1.7

[Michael G Schwern]

On 2012.7.30 3:15 PM, Eric Wong wrote:
>> Right now, canonicalization is a bug generator.  Paths and URLs have to be in
>> the same form when they're compared.  This requires meticulous care on the
>> part of the coder and reviewer to check every comparison.  It scatters the
>> logic for proper comparison all over the code.  Redundant logic scattered
>> around the code is a Bad Thing.  It makes it more likely a coder will forget
>> the logic, or get it wrong, and a human reviewer must be far more vigilant.
> 
> <snip>  I agree completely with canonicalization.

Sorry, I'm not sure what you're agreeing with.


>> The only downside is when chasing down a bug related to canonicalization one
>> might have to realize that eq is overloaded.
> 
> Having to realize eq is overloaded is a huge downside to me.

Presumably you'd be reviewing the change which implements the overloaded
objects, so you'd know about it.  And it would be documented.

I've listed a bunch of concrete positives for using comparison overloaded
URI/path objects vs how it's currently being done.  How about you voice some
of the downsides in concrete terms?  Or an alternative that solves the current
problems?


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

Re: Fix git-svn for SVN 1.7

[Eric Wong]

Michael G Schwern <schwern@pobox.com> wrote:
> On 2012.7.30 3:15 PM, Eric Wong wrote:
> >> Right now, canonicalization is a bug generator.  Paths and URLs have to be in
> >> the same form when they're compared.  This requires meticulous care on the
> >> part of the coder and reviewer to check every comparison.  It scatters the
> >> logic for proper comparison all over the code.  Redundant logic scattered
> >> around the code is a Bad Thing.  It makes it more likely a coder will forget
> >> the logic, or get it wrong, and a human reviewer must be far more vigilant.
> > 
> > <snip>  I agree completely with canonicalization.
> 
> Sorry, I'm not sure what you're agreeing with.

That's it's a bug generator and we shouldn't have redundant logic.
Having functions to compare objects themselves is a good thing.

> >> The only downside is when chasing down a bug related to canonicalization one
> >> might have to realize that eq is overloaded.
> > 
> > Having to realize eq is overloaded is a huge downside to me.
> 
> Presumably you'd be reviewing the change which implements the overloaded
> objects, so you'd know about it.  And it would be documented.

The change itself is easy to review.   Picking up the code a few
months/years down the line and having to know "eq" is overloaded
tends to bite people.

> I've listed a bunch of concrete positives for using comparison overloaded
> URI/path objects vs how it's currently being done.  How about you voice some
> of the downsides in concrete terms?  Or an alternative that solves the current
> problems?

Any custom comparison function would do the trick (e.g. URI::eq()).

I _want_ URI/path objects.  I do not want a bare "eq" operator to
obscure the fact it's calling URI::eq() behind-the-scenes.

That said, I don't mind overloads when it's obvious an overload is being
used (e.g. stringify).  It's things like "eq" which obscure the fact
function calls are happening in the background.

Re: Fix git-svn for SVN 1.7

[Michael G Schwern]

On 2012.7.30 7:18 PM, Eric Wong wrote:
> Michael G Schwern <schwern@pobox.com> wrote:
>> On 2012.7.30 3:15 PM, Eric Wong wrote:
>>>> Right now, canonicalization is a bug generator.  Paths and URLs have to be in
>>>> the same form when they're compared.  This requires meticulous care on the
>>>> part of the coder and reviewer to check every comparison.  It scatters the
>>>> logic for proper comparison all over the code.  Redundant logic scattered
>>>> around the code is a Bad Thing.  It makes it more likely a coder will forget
>>>> the logic, or get it wrong, and a human reviewer must be far more vigilant.
>>>
>>> <snip>  I agree completely with canonicalization.
>>
>> Sorry, I'm not sure what you're agreeing with.
> 
> That's it's a bug generator and we shouldn't have redundant logic.
> Having functions to compare objects themselves is a good thing.

That doesn't make it much better than what we have now.  One still has to
remember to pepper those special comparisons all over the code.


>>>> The only downside is when chasing down a bug related to canonicalization one
>>>> might have to realize that eq is overloaded.
>>>
>>> Having to realize eq is overloaded is a huge downside to me.
>>
>> Presumably you'd be reviewing the change which implements the overloaded
>> objects, so you'd know about it.  And it would be documented.
> 
> The change itself is easy to review.   Picking up the code a few
> months/years down the line and having to know "eq" is overloaded
> tends to bite people.

Why does a reviewer, or a reader of the code, have to know eq is overloaded?

How often would string comparing an overloaded uri/path object be the wrong
thing to do?  Just about never.  Compare that to how often it would be
incorrect to string compare a non-overloaded uri/path object.  Most of the
time.  Do you feel it would be otherwise?

If they're overloaded, somebody patching the code doesn't have to know to use
a special uri_eq() function.  It'll just happen when they naturally string
compare.  The coder doesn't have to know or do anything special.  The reviewer
doesn't have to do any special work.

If they're not overloaded, coders must know about the special URI and path
requirements.  Each string comparison is suspect and must be scrutinized by
the reviewer.  They have to think "is this actually a uri or path comparison?
 Should it be using the special comparison functions?"

Which procedure offers more opportunities for mistakes?


>> I've listed a bunch of concrete positives for using comparison overloaded
>> URI/path objects vs how it's currently being done.  How about you voice some
>> of the downsides in concrete terms?  Or an alternative that solves the current
>> problems?
> 
> Any custom comparison function would do the trick (e.g. URI::eq()).
>
> I _want_ URI/path objects.  I do not want a bare "eq" operator to
> obscure the fact it's calling URI::eq() behind-the-scenes.
>
> That said, I don't mind overloads when it's obvious an overload is being
> used (e.g. stringify).  It's things like "eq" which obscure the fact
> function calls are happening in the background.

Is that a problem?  If so, why?

If the objects stringify, but comparing them as strings is generally the wrong
thing to do (even if the object stringifies to the canonical form, you don't
know the other side of the operator is an object), isn't that asking for bugs?
 If the objects are going to act like strings, shouldn't they act like strings
completely?

Object overloading fails when the encapsulation is incomplete.


-- 
151. The proper way to report to my Commander is "Specialist Schwarz,
     reporting as ordered, Sir" not "You can't prove a thing!"
    -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
           http://skippyslist.com/list/

Re: Fix git-svn for SVN 1.7

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Perhaps we can depend on the URI.pm module?  It seems to be
> widely-available and not be a significant barrier to installation.  On
> the other hand, I don't know its history, either (especially since we're
> now dealing with SVN changes...).
>
> Anyways, I don't like relying on operator overloading, it makes code
> harder to read and review.

I think code that uses operator overloading, when printed in a
textbook, cast in stone and makes the reader aware that it is never
going to change, is indeed "easy" to read through.  But I suspect
that it may be merely giving a false illusion that it is easy to
readers.

The problem is that use of such obscure overloading tends to hurt
maintainability. If the initial version Michael produces converts
all the external strings into instances of CanonicalizedPath class,
according to the "convert as early as possible" principle, you can
be assured that all "eq" you see are about the normalized strings
the svn library wants to see, and that may allow us sleep safely.

But the real problem begins six months down the road, when somebody
wants to add a new codepath that reads a new string from an external
source (e.g. perhaps you add a new configuration variable that
specifies a path in the svn repository and does something special
when that path is touched by a revision; the exact nature of the new
feature does not matter in this discussion).  The new code can
forget to follow the "convert early" principle, and pass a bare
string read from the configuration around.

A comparison between such a new string and another variable that
holds path that comes from the existing codepath (i.e. Michael's
initial code that perfectly follows the "convert early" principle)
will still use the overloaded eq in "$new_str eq $old_path", thanks
to the language rule of Perl (namely, even though the new string is
a non object, the other side is still an instance of the class).

When the code needs to compare two or more such "new" strings (e.g.
perhaps it wants to remove duplicates from the set of paths it reads
from the configuration), however, "eq" silently turns back to a
simple string comparison, as "$new_1 eq $new_2" will not magically
turn into "Canonicalize($new_1)->cmp(Canonicalize($new_2))".

This kind of error is unnecessarily hard to catch mostly because the
previous "$new_str eq $old_path" does work; it masks the problem.
Overloading of "eq" is making it harder to spot new bugs.

If the code never uses "eq" to compare canonicalized paths, and all
the surrounding code compare paths using explicit method call on
objects, it makes it crystal clear to the readers that paths held in
a bare string is unwelcome in the codepath.  It makes it harder to
add new code that uses and passes around a bare string by mistake to
such a codepath, I would think.

Re: Fix git-svn for SVN 1.7

[Michael G Schwern]

It just doesn't matter.

Why are we arguing over which solution will be 4% better two years from now,
or if my commits are formatted perfectly, when tremendous amounts of basic
work to be done improving git-svn?  The code is undocumented, lacking unit
tests, difficult to understand and riddled with bugs.

Either solution would be a vast improvement.  The most important thing is that
one of them actually gets done.  If both solutions offer a huge improvement,
do it the way the person actually writing the code wants to do it.  It'll be
more enjoyable for them, they'll be more likely to complete the work, and more
likely to stick around and code some more.

Re: Fix git-svn for SVN 1.7

[Eric Wong]

Michael G Schwern <schwern@pobox.com> wrote:
> It just doesn't matter.
> 
> Why are we arguing over which solution will be 4% better two years from now,
> or if my commits are formatted perfectly, when tremendous amounts of basic
> work to be done improving git-svn?  The code is undocumented, lacking unit
> tests, difficult to understand and riddled with bugs.

Yes it does matter.

git-svn has the problems it has because it traditionally had lower
review standards than the rest of git.  So yes, we're being more careful
nowadays about the long-term ramifications of changes.

> Either solution would be a vast improvement.  The most important thing is that
> one of them actually gets done.  If both solutions offer a huge improvement,
> do it the way the person actually writing the code wants to do it.  It'll be
> more enjoyable for them, they'll be more likely to complete the work, and more
> likely to stick around and code some more.

The self-obsoleting nature of git-svn makes it hard for anybody to stick
around.  Most of the original contributors (including myself) hardly see
an SVN repo anymore, so users/contributors forget about it and new ones
come along...

I want to make sure things stay consistent with the core parts of git,
especially if the Perl were to be replaced with a pure C version.

Re: Fix git-svn for SVN 1.7

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Michael G Schwern <schwern@pobox.com> wrote:
>> It just doesn't matter.
>> 
>> Why are we arguing over which solution will be 4% better two years from now,
>> or if my commits are formatted perfectly, when tremendous amounts of basic
>> work to be done improving git-svn?  The code is undocumented, lacking unit
>> tests, difficult to understand and riddled with bugs.
>
> Yes it does matter.
>
> git-svn has the problems it has because it traditionally had lower
> review standards than the rest of git.  So yes, we're being more careful
> nowadays about the long-term ramifications of changes.

Thanks.  I know it takes guts to publicly admit that over time your
own creation has become less ideal than you wish it to be, but it
needed to be said.

Michael, please realize that the only reason people comment on the
patch series is because they care about what the series brings to
us.  In other words, your effort is appreciated.  For a change that
we want to have in our codebase, the functionality of the code
immediately after the change is applied of course is important, but
the maintainability of the result also matters.

We want to make sure that anybody who wants to understand and
improve the system can read the code without distraction from
inconsistent coding styles used in different sections of code.  We
want "git log" (or "git log git-svn.perl perl/") output to tell a
coherent story about how the code evolved and why these changes are
made in a consistent voice to the readers.  We want people to be
able to "git log | grep Signed-off-by:" to count the contributors.

A contributor has enough room to be creative in how his or her code
is designed.  Updating the code to follow the "convert as early as
possible", and (during subsequent discussion with Eric) suggesting
use of class instances instead of bare strings to make it harder to
mistakenly use bare unconverted strings are two examples you already
showed creativity in areas that matter.

There is no need to be creative in ChangeLog and coding styles; it
only hurts maintainability.

Regarding the operator overloading of "eq" for comparing the
converted strings, I still think it will hurt maintainablity (we
want to make sure that it is harder, not easier, to make wrong
changes to the code in the future), but I may be mistaken and you
may have better ideas.  If you can use overloading in such a way
that it won't harm maintainability and yet makes the resulting code
easier to read, I don't have any objection.

What I won't accept is "maintainability does not matter".  It does.

Thanks.

Re: Fix git-svn for SVN 1.7

[Michael G Schwern]

On 2012.7.31 4:05 PM, Junio C Hamano wrote:
> What I won't accept is "maintainability does not matter".  It does.

I'm sorry, that's not what I intended to convey at all.  My reply to Eric lays
it out more clearly, I think.


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

Re: Fix git-svn for SVN 1.7

[Michael G Schwern]

On 2012.7.31 1:01 PM, Eric Wong wrote:
> Michael G Schwern <schwern@pobox.com> wrote:
>> It just doesn't matter.
>>
>> Why are we arguing over which solution will be 4% better two years from now,
>> or if my commits are formatted perfectly, when tremendous amounts of basic
>> work to be done improving git-svn?  The code is undocumented, lacking unit
>> tests, difficult to understand and riddled with bugs.
> 
> Yes it does matter.
> 
> git-svn has the problems it has because it traditionally had lower
> review standards than the rest of git.  So yes, we're being more careful
> nowadays about the long-term ramifications of changes.

Yes, review does matter.  And so far we've been arguing over whether reviewing
objects-with-overloading or objects-without-overloading would be better.  And
we can argue about that forever.

That's the part that doesn't matter.  People matter.

I think we can all agree that either solution is a vast improvement along
multiple axes, including review.  So what really matters is making sure one of
them gets done.  Once either of them is done, we can see how it works out in
practice instead of arguing theoretical futures.  Once either of them is done,
it's much easier to switch to the other.

What I'm trying to say is I have much less interest in doing it without the
overloading.  It's not interesting to me.  It's no fun.  No fun means no
patch.  No patch means no improvement.  No improvement is the worst of all
possible options.

I had a lot of enthusiasm for this project when I came in.  I like refactoring
Perl code.  I like git.  That's all but sunk at how painful and slow and
nit-picking the process has been.  We've barely talked about the content of
the patches I've submitted, it's all process.  This is no fun.

We're all volunteers here and we're all getting something personal out of
this.  Some form of personal enjoyment.  I'm not getting that, so I'm unlikely
to stick around.


-- 
Defender of Lexical Encapsulation

Re: Fix git-svn for SVN 1.7

[Eric Wong]

Michael G Schwern <schwern@pobox.com> wrote:
> That's the part that doesn't matter.  People matter.

> What I'm trying to say is I have much less interest in doing it without the
> overloading.  It's not interesting to me.  It's no fun.  No fun means no
> patch.  No patch means no improvement.  No improvement is the worst of all
> possible options.

We want to ensure the code you contribute can be improved by others, not
just you.  I thank you for your changes so far, other developers should
find it easier to contribute to git-svn.

> I had a lot of enthusiasm for this project when I came in.  I like refactoring
> Perl code.  I like git.  That's all but sunk at how painful and slow and
> nit-picking the process has been.  We've barely talked about the content of
> the patches I've submitted, it's all process.  This is no fun.

I haven't found objections to the actual code you've contributed so far.
I'll be applying your changes once I've had a chance to reread/test
them.

Yes, we are nitpicky about process, but I think it's important to
maintain that consistency given the number of contributors we attract.

I'll also need to review/rewrite some of the Subject: lines so they make
sense when read in --pretty=oneline/shortlog output. (unless you want to
volunteer to resubmit that).

Re: Fix git-svn for SVN 1.7

[Eric Wong]

"Michael G. Schwern" <schwern@pobox.com> wrote:
> This patch series fixes git-svn for SVN 1.7 tested against SVN 1.7.5 and
> 1.6.18.  Patch 7/8 is where SVN 1.7 starts passing.

Thanks Michael.  I've made minor editorial changes (mostly rewording
commit titles to fit the larger project).

Junio:

The following changes since commit 05a20c87abd08441c98dfcca0606bc0f8432ab26:

  Merge git://github.com/git-l10n/git-po (2012-08-01 15:59:08 -0700)

are available in the git repository at:


  git://bogomips.org/git-svn master

for you to fetch changes up to db7c5388b6d843f7cd248dc465af4507d1de7918:

  git-svn: remove ad-hoc canonicalizations (2012-08-02 09:42:25 +0000)

----------------------------------------------------------------
Michael G. Schwern (20):
      Git::SVN: use accessors internally for path
      Git::SVN: use accessor for URLs internally
      Git::SVN::Ra: use accessor for URLs
      use Git::SVN->path accessor globally
      use Git::SVN{,::RA}->url accessor globally
      git-svn: move canonicalization to Git::SVN::Utils
      git-svn: use SVN 1.7 to canonicalize when possible
      git-svn: factor out _collapse_dotdot function
      git-svn: add join_paths() to safely concatenate paths
      Git::SVN::Utils: remove irrelevant comment
      git-svn: path canonicalization uses SVN API
      Git::SVN{,::Ra}: canonicalize earlier
      t9118: workaround inconsistency between SVN versions
      t9107: fix typo
      git-svn: attempt to mimic SVN 1.7 URL canonicalization
      git-svn: replace URL escapes with canonicalization
      git-svn: canonicalize earlier
      git-svn: introduce add_path_to_url function
      git-svn: canonicalize newly-minted URLs
      git-svn: remove ad-hoc canonicalizations

 git-svn.perl                          |  92 ++++++------------
 perl/Git/SVN.pm                       | 174 ++++++++++++++++++++++------------
 perl/Git/SVN/Fetcher.pm               |   2 +-
 perl/Git/SVN/Migration.pm             |   6 +-
 perl/Git/SVN/Ra.pm                    |  92 ++++++++++--------
 perl/Git/SVN/Utils.pm                 | 173 ++++++++++++++++++++++++++++++++-
 t/Git-SVN/Utils/add_path_to_url.t     |  27 ++++++
 t/Git-SVN/Utils/canonicalize_url.t    |  26 +++++
 t/Git-SVN/Utils/collapse_dotdot.t     |  23 +++++
 t/Git-SVN/Utils/join_paths.t          |  32 +++++++
 t/t9107-git-svn-migrate.sh            |   6 +-
 t/t9118-git-svn-funky-branch-names.sh |   7 +-
 12 files changed, 486 insertions(+), 174 deletions(-)
 create mode 100644 t/Git-SVN/Utils/add_path_to_url.t
 create mode 100644 t/Git-SVN/Utils/canonicalize_url.t
 create mode 100644 t/Git-SVN/Utils/collapse_dotdot.t
 create mode 100644 t/Git-SVN/Utils/join_paths.t

Re: Fix git-svn for SVN 1.7

[Jonathan Nieder]

Hi,

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

>> This patch series fixes git-svn for SVN 1.7 tested against SVN 1.7.5 and
>> 1.6.18.  Patch 7/8 is where SVN 1.7 starts passing.
>
> Thanks Michael.  I've made minor editorial changes (mostly rewording
> commit titles to fit the larger project).

Thanks from me as well.  I'm still worried about whether the increased
use of canonicalize_url will introduce regressions for the existing
SVN 1.6 support, and I should have time to look it over this weekend.

The comment in canonicalize_url "There wasn't a 1.6 way to do it" is
not true.  The relevant thread on the git list had a little
conversation about keeping svn 1.4 support, but I'm not sure why
that's relevant, given that svn_canonicalize_path has worked largely
the same way starting with SVN 1.1 (and on the other hand had
significant changes in SVN 1.7).

Hopefully you've looked this over carefully already and I'm worrying
needlessly.

Hope that helps,
Jonathan

Re: Fix git-svn for SVN 1.7

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi,
>
> Eric Wong wrote:
>> "Michael G. Schwern" <schwern@pobox.com> wrote:
>
>>> This patch series fixes git-svn for SVN 1.7 tested against SVN 1.7.5 and
>>> 1.6.18.  Patch 7/8 is where SVN 1.7 starts passing.
>>
>> Thanks Michael.  I've made minor editorial changes (mostly rewording
>> commit titles to fit the larger project).
>
> Thanks from me as well.  I'm still worried about whether the increased
> use of canonicalize_url will introduce regressions for the existing
> SVN 1.6 support, and I should have time to look it over this weekend.

Likewise.  I'd prefer to see it cook during the feature freeze and
not merge to 'master' until post 1.7.12 cycle opens.

Re: Fix git-svn for SVN 1.7

[Robin H. Johnson]

On Thu, Aug 02, 2012 at 11:58:08AM -0700,  Junio C Hamano wrote:
> > Thanks from me as well.  I'm still worried about whether the increased
> > use of canonicalize_url will introduce regressions for the existing
> > SVN 1.6 support, and I should have time to look it over this weekend.
> 
> Likewise.  I'd prefer to see it cook during the feature freeze and
> not merge to 'master' until post 1.7.12 cycle opens.
I'm going to spin it and include in Gentoo's 1.7.12 packages, as we're
in need of this explicitly, and this is why we funded Michael to do the
work.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85

Re: Fix git-svn for SVN 1.7

[Eric Wong]

"Robin H. Johnson" <robbat2@gentoo.org> wrote:
> On Thu, Aug 02, 2012 at 11:58:08AM -0700,  Junio C Hamano wrote:
> > > Thanks from me as well.  I'm still worried about whether the increased
> > > use of canonicalize_url will introduce regressions for the existing
> > > SVN 1.6 support, and I should have time to look it over this weekend.
> > 
> > Likewise.  I'd prefer to see it cook during the feature freeze and
> > not merge to 'master' until post 1.7.12 cycle opens.
> 
> I'm going to spin it and include in Gentoo's 1.7.12 packages, as we're
> in need of this explicitly, and this is why we funded Michael to do the
> work.

Thanks.  Btw, have you gotten the chance to report the new SVN 1.7 test
failures Michael mentioned to the SVN folks?

Re: Fix git-svn for SVN 1.7

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Hi,
>>
>> Eric Wong wrote:
>>> "Michael G. Schwern" <schwern@pobox.com> wrote:
>>
>>>> This patch series fixes git-svn for SVN 1.7 tested against SVN 1.7.5 and
>>>> 1.6.18.  Patch 7/8 is where SVN 1.7 starts passing.
>>>
>>> Thanks Michael.  I've made minor editorial changes (mostly rewording
>>> commit titles to fit the larger project).
>>
>> Thanks from me as well.  I'm still worried about whether the increased
>> use of canonicalize_url will introduce regressions for the existing
>> SVN 1.6 support, and I should have time to look it over this weekend.
>
> Likewise.  I'd prefer to see it cook during the feature freeze and
> not merge to 'master' until post 1.7.12 cycle opens.

So we had a chance to cook this late topic outside 'master' during
the feature freeze.  As you already queued and signed it off, I am
going to fast-track this down to 'master' as promised.

Unless you found a reason not to in the meantime, that is.  Is what
I have on 'pu' still good, or do you (Eric and/or Michael) have any
updates you'd rather have me pull instead?

Thanks.

Re: Fix git-svn for SVN 1.7

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Unless you found a reason not to in the meantime, that is.  Is what
> I have on 'pu' still good, or do you (Eric and/or Michael) have any
> updates you'd rather have me pull instead?

No updates, everything is still good.

Re: Fix git-svn for SVN 1.7

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> Unless you found a reason not to in the meantime, that is.  Is what
>> I have on 'pu' still good, or do you (Eric and/or Michael) have any
>> updates you'd rather have me pull instead?
>
> No updates, everything is still good.

Thanks.

Re: Fix git-svn for SVN 1.7

[Eric Wong]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> Thanks from me as well.  I'm still worried about whether the increased
> use of canonicalize_url will introduce regressions for the existing
> SVN 1.6 support, and I should have time to look it over this weekend.
> 
> The comment in canonicalize_url "There wasn't a 1.6 way to do it" is
> not true.  The relevant thread on the git list had a little
> conversation about keeping svn 1.4 support, but I'm not sure why
> that's relevant, given that svn_canonicalize_path has worked largely
> the same way starting with SVN 1.1 (and on the other hand had
> significant changes in SVN 1.7).
> 
> Hopefully you've looked this over carefully already and I'm worrying
> needlessly.

Thanks for reminding me, I went back to an old chroot 1.4.2 indeed
does fail canonicalization.

Will bisect and squash a fix in.

Re: Fix git-svn for SVN 1.7

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Thanks from me as well.  I'm still worried about whether the increased
>> use of canonicalize_url will introduce regressions for the existing
>> SVN 1.6 support, and I should have time to look it over this weekend.
>> 
>> The comment in canonicalize_url "There wasn't a 1.6 way to do it" is
>> not true.  The relevant thread on the git list had a little
>> conversation about keeping svn 1.4 support, but I'm not sure why
>> that's relevant, given that svn_canonicalize_path has worked largely
>> the same way starting with SVN 1.1 (and on the other hand had
>> significant changes in SVN 1.7).
>> 
>> Hopefully you've looked this over carefully already and I'm worrying
>> needlessly.
>
> Thanks for reminding me, I went back to an old chroot 1.4.2 indeed
> does fail canonicalization.
>
> Will bisect and squash a fix in.

Oops; should I eject this out of next and wait for a reroll, then?

Re: Fix git-svn for SVN 1.7

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> > Thanks for reminding me, I went back to an old chroot 1.4.2 indeed
> > does fail canonicalization.
> >
> > Will bisect and squash a fix in.
> 
> Oops; should I eject this out of next and wait for a reroll, then?

Your call, I doubt anybody on next uses SVN 1.4.2.   Rerolling now.

Re: Fix git-svn for SVN 1.7

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Wong <normalperson@yhbt.net> writes:
> > > Thanks for reminding me, I went back to an old chroot 1.4.2 indeed
> > > does fail canonicalization.
> > >
> > > Will bisect and squash a fix in.
> > 
> > Oops; should I eject this out of next and wait for a reroll, then?
> 
> Your call, I doubt anybody on next uses SVN 1.4.2.   Rerolling now.

OK, rerolled with the patch in
http://mid.gmane.org/20120802215141.GA5284@dcvr.yhbt.net squashed into
[PATCH 11/20] git-svn: path canonicalization uses SVN API

The following changes since commit 05a20c87abd08441c98dfcca0606bc0f8432ab26:

  Merge git://github.com/git-l10n/git-po (2012-08-01 15:59:08 -0700)

are available in the git repository at:


  git://bogomips.org/git-svn master

for you to fetch changes up to 5eaa1fd086e826b1ac8d9346a740527edbdb3c34:

  git-svn: remove ad-hoc canonicalizations (2012-08-02 21:46:06 +0000)

----------------------------------------------------------------
Michael G. Schwern (20):
      Git::SVN: use accessors internally for path
      Git::SVN: use accessor for URLs internally
      Git::SVN::Ra: use accessor for URLs
      use Git::SVN->path accessor globally
      use Git::SVN{,::RA}->url accessor globally
      git-svn: move canonicalization to Git::SVN::Utils
      git-svn: use SVN 1.7 to canonicalize when possible
      git-svn: factor out _collapse_dotdot function
      git-svn: add join_paths() to safely concatenate paths
      Git::SVN::Utils: remove irrelevant comment
      git-svn: path canonicalization uses SVN API
      Git::SVN{,::Ra}: canonicalize earlier
      t9118: workaround inconsistency between SVN versions
      t9107: fix typo
      git-svn: attempt to mimic SVN 1.7 URL canonicalization
      git-svn: replace URL escapes with canonicalization
      git-svn: canonicalize earlier
      git-svn: introduce add_path_to_url function
      git-svn: canonicalize newly-minted URLs
      git-svn: remove ad-hoc canonicalizations

 git-svn.perl                          |  92 ++++++------------
 perl/Git/SVN.pm                       | 174 +++++++++++++++++++++------------
 perl/Git/SVN/Fetcher.pm               |   2 +-
 perl/Git/SVN/Migration.pm             |   6 +-
 perl/Git/SVN/Ra.pm                    |  92 ++++++++++--------
 perl/Git/SVN/Utils.pm                 | 176 +++++++++++++++++++++++++++++++++-
 t/Git-SVN/Utils/add_path_to_url.t     |  27 ++++++
 t/Git-SVN/Utils/canonicalize_url.t    |  26 +++++
 t/Git-SVN/Utils/collapse_dotdot.t     |  23 +++++
 t/Git-SVN/Utils/join_paths.t          |  32 +++++++
 t/t9107-git-svn-migrate.sh            |   6 +-
 t/t9118-git-svn-funky-branch-names.sh |   7 +-
 12 files changed, 489 insertions(+), 174 deletions(-)
 create mode 100644 t/Git-SVN/Utils/add_path_to_url.t
 create mode 100644 t/Git-SVN/Utils/canonicalize_url.t
 create mode 100644 t/Git-SVN/Utils/collapse_dotdot.t
 create mode 100644 t/Git-SVN/Utils/join_paths.t

Re: Fix git-svn for SVN 1.7

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Eric Wong <normalperson@yhbt.net> wrote:
>> Junio C Hamano <gitster@pobox.com> wrote:
>> > Eric Wong <normalperson@yhbt.net> writes:
>> > > Thanks for reminding me, I went back to an old chroot 1.4.2 indeed
>> > > does fail canonicalization.
>> > >
>> > > Will bisect and squash a fix in.
>> > 
>> > Oops; should I eject this out of next and wait for a reroll, then?
>> 
>> Your call, I doubt anybody on next uses SVN 1.4.2.   Rerolling now.
>
> OK, rerolled with the patch in
> http://mid.gmane.org/20120802215141.GA5284@dcvr.yhbt.net squashed into
> [PATCH 11/20] git-svn: path canonicalization uses SVN API
>
> The following changes since commit 05a20c87abd08441c98dfcca0606bc0f8432ab26:
>
>   Merge git://github.com/git-l10n/git-po (2012-08-01 15:59:08 -0700)
>
> are available in the git repository at:
>
>
>   git://bogomips.org/git-svn master
>
> for you to fetch changes up to 5eaa1fd086e826b1ac8d9346a740527edbdb3c34:
>
>   git-svn: remove ad-hoc canonicalizations (2012-08-02 21:46:06 +0000)

Thanks.

Will park in 'pu' for now, and depending how it goes, I may be
tempted to merge it down to 'next', but seeing how quickly an issue
was found, it is not likely I'd feel it safe enough to merge it to
'master' before the upcoming release.