threads / discuss / 31120

Fix git-svn for SVN 1.7

Subject: Fix git-svn for SVN 1.7

## tl;dr

50 messages between Jul 28, 2012 and Oct 10, 2012.

replies: 49people: 6as markdown or json

Michael G. Schwern· Jul 28, 2012, 09:47 UTC · lore

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.

Michael G. Schwern· Jul 28, 2012, 09:47 UTC · re: Michael G. Schwern · lore

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

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
Jonathan Nieder· Jul 28, 2012, 14:16 UTC · re: Michael G. Schwern · lore

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

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

Michael G Schwern· Jul 28, 2012, 19:32 UTC · re: Jonathan Nieder · lore

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

On 2012.7.28 7:16 AM, Jonathan Nieder wrote:
Show 11 quoted lines
> 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".
Show 5 quoted lines
> 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
Jonathan Nieder· Oct 9, 2012, 08:41 UTC · re: Michael G Schwern · lore

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

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:
Show 10 quoted lines
>>> -		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
Michael J Gruber· Oct 9, 2012, 09:47 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder venit, vidit, dixit 09.10.2012 10:41:
Show 32 quoted lines
> 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.
Show 33 quoted lines
> ---
> 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...
Jonathan Nieder· Oct 9, 2012, 10:19 UTC · re: Michael J Gruber · lore

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

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

Eric Wong· Oct 10, 2012, 20:37 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 6 quoted lines
> 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
Jonathan Nieder· Oct 10, 2012, 21:02 UTC · re: Eric Wong · lore

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

Eric Wong wrote:
Show 6 quoted lines
> 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
Eric Wong· Oct 10, 2012, 21:31 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 13 quoted lines
> 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 :>

Show 6 quoted lines
> 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
Jonathan Nieder· Oct 10, 2012, 21:42 UTC · re: Eric Wong · lore

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

Eric Wong wrote:
> Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 6 quoted lines
>> -- >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
Eric Wong· Oct 10, 2012, 22:16 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 11 quoted lines
> 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 :>
Junio C Hamano· Oct 10, 2012, 22:33 UTC · re: Eric Wong · lore

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

Eric Wong <normalperson@yhbt.net> writes:
Show 14 quoted lines
> 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.
Michael G. Schwern· Jul 28, 2012, 09:47 UTC · re: Michael G. Schwern · lore

[PATCH 2/8] Fix typo in test

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
Michael G. Schwern· Jul 28, 2012, 09:47 UTC · re: Michael G. Schwern · lore

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

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
Michael G. Schwern· Jul 28, 2012, 09:47 UTC · re: Michael G. Schwern · lore

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

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
Michael G. Schwern· Jul 28, 2012, 09:47 UTC · re: Michael G. Schwern · lore

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

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
Michael G. Schwern· Jul 28, 2012, 09:47 UTC · re: Michael G. Schwern · lore

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

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
Michael G. Schwern· Jul 28, 2012, 09:47 UTC · re: Michael G. Schwern · lore

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

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
Jonathan Nieder· Oct 6, 2012, 19:24 UTC · re: Michael G. Schwern · lore

[PATCH/RFC] test: 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.

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
Jonathan Nieder· Oct 9, 2012, 10:12 UTC · re: Jonathan Nieder · lore

[PATCH/RFC v2] 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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:
Show 5 quoted lines
> 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
Eric Wong· Oct 10, 2012, 20:11 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 12 quoted lines
> 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
Jonathan Nieder· Oct 10, 2012, 20:47 UTC · re: Eric Wong · lore

[PATCH v3] 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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Eric Wong wrote:
Show 5 quoted lines
> 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
Michael G. Schwern· Jul 28, 2012, 09:47 UTC · re: Michael G. Schwern · lore

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

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
Eric Wong· Jul 30, 2012, 20:38 UTC · re: Michael G. Schwern · lore

Re: Fix git-svn for SVN 1.7

"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.
Show 7 quoted lines
> 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.

Michael G Schwern· Jul 30, 2012, 21:10 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

On 2012.7.30 1:38 PM, Eric Wong wrote:
Show 7 quoted lines
>> 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
Eric Wong· Jul 30, 2012, 22:15 UTC · re: Michael G Schwern · lore

Re: Fix git-svn for SVN 1.7

Michael G Schwern <schwern@pobox.com> wrote:
Show 10 quoted lines
> 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.
Michael G Schwern· Jul 31, 2012, 01:04 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

On 2012.7.30 3:15 PM, Eric Wong wrote:
Show 8 quoted lines
>> 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!
Eric Wong· Jul 31, 2012, 02:18 UTC · re: Michael G Schwern · lore

Re: Fix git-svn for SVN 1.7

Michael G Schwern <schwern@pobox.com> wrote:
Show 11 quoted lines
> 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.

Show 7 quoted lines
> >> 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.

Michael G Schwern· Jul 31, 2012, 04:30 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

On 2012.7.30 7:18 PM, Eric Wong wrote:
Show 15 quoted lines
> 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.

Show 11 quoted lines
>>>> 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?
Show 13 quoted lines
>> 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/
Junio C Hamano· Jul 31, 2012, 06:53 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

Eric Wong <normalperson@yhbt.net> writes:
Show 7 quoted lines
> 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.

Michael G Schwern· Jul 31, 2012, 09:54 UTC · re: Junio C Hamano · lore

Re: Fix git-svn for SVN 1.7

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.

Eric Wong· Jul 31, 2012, 20:01 UTC · re: Michael G Schwern · lore

Re: Fix git-svn for SVN 1.7

Michael G Schwern <schwern@pobox.com> wrote:
Show 6 quoted lines
> 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.

Show 5 quoted lines
> 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.

Junio C Hamano· Jul 31, 2012, 23:05 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

Eric Wong <normalperson@yhbt.net> writes:
Show 13 quoted lines
> 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.
Michael G Schwern· Jul 31, 2012, 23:28 UTC · re: Junio C Hamano · lore

Re: Fix git-svn for SVN 1.7

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
Michael G Schwern· Jul 31, 2012, 23:24 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

On 2012.7.31 1:01 PM, Eric Wong wrote:
Show 13 quoted lines
> 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
Eric Wong· Aug 1, 2012, 21:30 UTC · re: Michael G Schwern · lore

Re: Fix git-svn for SVN 1.7

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).

Eric Wong· Aug 2, 2012, 10:31 UTC · re: Michael G. Schwern · lore

Re: Fix git-svn for SVN 1.7

"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
Jonathan Nieder· Aug 2, 2012, 16:07 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

Hi,
Eric Wong wrote:
> "Michael G. Schwern" <schwern@pobox.com> wrote:
Show 5 quoted lines
>> 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

Junio C Hamano· Aug 2, 2012, 18:58 UTC · re: Jonathan Nieder · lore

Re: Fix git-svn for SVN 1.7

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 14 quoted lines
> 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.

Robin H. Johnson· Aug 2, 2012, 19:50 UTC · re: Junio C Hamano · lore

Re: Fix git-svn for SVN 1.7

On Thu, Aug 02, 2012 at 11:58:08AM -0700,  Junio C Hamano wrote:
Show 6 quoted lines
> > 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
Eric Wong· Aug 2, 2012, 22:10 UTC · re: Robin H. Johnson · lore

Re: Fix git-svn for SVN 1.7

"Robin H. Johnson" <robbat2@gentoo.org> wrote:
Show 11 quoted lines
> 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?

Junio C Hamano· Aug 21, 2012, 04:04 UTC · re: Junio C Hamano · lore

Re: Fix git-svn for SVN 1.7

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> 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.
Eric Wong· Aug 21, 2012, 21:03 UTC · re: Junio C Hamano · lore

Re: Fix git-svn for SVN 1.7

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.
Junio C Hamano· Aug 21, 2012, 21:34 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

Eric Wong <normalperson@yhbt.net> writes:
Show 6 quoted lines
> 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.
Eric Wong· Aug 2, 2012, 20:51 UTC · re: Jonathan Nieder · lore

Re: Fix git-svn for SVN 1.7

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 13 quoted lines
> 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.
Junio C Hamano· Aug 2, 2012, 21:22 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

Eric Wong <normalperson@yhbt.net> writes:
Show 19 quoted lines
> 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?
Eric Wong· Aug 2, 2012, 21:42 UTC · re: Junio C Hamano · lore

Re: Fix git-svn for SVN 1.7

Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> 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.
Eric Wong· Aug 2, 2012, 21:55 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

Eric Wong <normalperson@yhbt.net> wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Aug 2, 2012, 22:05 UTC · re: Eric Wong · lore

Re: Fix git-svn for SVN 1.7

Eric Wong <normalperson@yhbt.net> writes:
Show 28 quoted lines
> 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.

← back to recent threads