{"thread":{"id":"31088","subject":"git-svn SVN 1.7 fix, take 2","startedAt":"2012-07-24T21:46:11Z","lastAt":"2012-07-25T09:53:55Z","messageCount":14,"participants":["Michael G Schwern","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"195664","messageId":"500F17A3.60307@pobox.com","threadId":"31088","inReplyTo":null,"subject":"git-svn SVN 1.7 fix, take 2","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-24T21:46:11Z","receivedAt":"2012-07-24T21:46:11Z","isPatch":false,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"It's post OSCON so I can take another crack at this again.\n\nI'm struggling with how best to present all this to you folks.  There's\netiquette for how one presents a git pull request... but there's conflicting\netiquette about how one presents patches to a mailing list.  I'm not sure\nwhich bit of which applies when here.  Documentation/SubmittingPatches focuses\non single patches and basic commit etiquette.\n\nA big one is \"do not blast 10 emails to a mailing list\" but I gather that's ok\nhere if a submission needs 10 commits to be well expressed and its done via\ngit-send-email?  And then if patch #3 needs revision I'm to do it in a rebase\nand resend the whole 10 commits?  Am I to think of git-send-email less as a\nmeans of sending patches to a mailing list and more as a git transport mechanism?\n\nI'm trying to bust it up into easier to digest pieces.  I came into this cold\nwithout much knowledge of the problem (\"something to do with\ncanonicalization\") and no knowledge of the code.  While each commit is sharp,\nthe work as a whole is mixed up.\n\nHere's the first pieces, as I see them, along with their branches.  The whole\nwork is in https://github.com/schwern/git/tree/git-svn/fix-canonical\n\n* Change the Makefile.PL so it automatically finds the .pm files.\nhttps://github.com/schwern/git/tree/git-svn/easier_modules\n(Going to remove the Error.pm movement as off-topic)\n\n* Extract each of the internal Git::* packages from inside git-svn.\nhttps://github.com/schwern/git/tree/git-svn/extract_classes\n\nThere's five classes, and I did each in at least two commits.  First is a\nstraight cut & paste with no further changes.  Second (or more) fixes it so\nthings work again.  This is better for review (if it were done in a single\ncommit the real change would be lost in the cut & paste), but it means you\nhave a commit that breaks thing which will be a problem for bisecting.  I'm\ninclined to stick with two commits and you folks can squash them if you decide\nbisecting is more important.\n\nThe Git::SVN extraction is more complicated than the rest, so I'll probably do\nthat separately and bust it up into a few commits.\n\nNext I'm going to...\n\n1) Submit easier_modules.\n2) Break up the Git::SVN fix into more commits.\n3) Submit the Git::SVN extraction.\n\n\n-- \nReality is that which, when you stop believing in it, doesn't go away.\n    -- Phillip K. Dick\n"},{"id":"195665","messageId":"7vipdcesk6.fsf@alter.siamese.dyndns.org","threadId":"31088","inReplyTo":"500F17A3.60307@pobox.com","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-24T21:51:37Z","receivedAt":"2012-07-24T21:51:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael G Schwern <schwern@pobox.com> writes:\n\n> A big one is \"do not blast 10 emails to a mailing list\" but I gather that's ok\n> here if a submission needs 10 commits to be well expressed and its done via\n> git-send-email?  And then if patch #3 needs revision I'm to do it in a rebase\n> and resend the whole 10 commits?  Am I to think of git-send-email less as a\n> means of sending patches to a mailing list and more as a git transport mechanism?\n\nYes, yes and whatever (even though I think send-email is just a\nbetter MUA/MSA when you want to send patches and isn't restricted\nfor a _git_ transport, I do not think it matters how you look at it).\n\n> I'm trying to bust it up into easier to digest pieces.  I came into this cold\n> without much knowledge of the problem (\"something to do with\n> canonicalization\") and no knowledge of the code.\n\nPerhaps it is a good idea to lurk and see how others submit their\ntopics first?\n"},{"id":"195666","messageId":"20120724220207.GA15969@burratino","threadId":"31088","inReplyTo":"500F17A3.60307@pobox.com","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-24T22:02:08Z","receivedAt":"2012-07-24T22:02:08Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael G Schwern wrote:\n\n> I'm trying to bust it up into easier to digest pieces.\n\nI have a crazy idea.  You might not like it, but maybe it's worth giving\nit a try.\n\n[...]\n> The Git::SVN extraction is more complicated than the rest, so I'll probably do\n> that separately and bust it up into a few commits.\n\nAll of these changes are supposed to have zero functional effect,\nright?\n\nCould you send the first five patches that *are* supposed to have a\nfunctional effect?  I know that they will not apply cleanly to git-svn\nfrom git \"master\" or on top of each other; that's fine with me.  If\nthe approach looks right, interested people (read: probably Ben or I :))\ncan make the corresponding change in the code layout from \"master\".\nAfterwards, we can look into all that refactoring to make later\nchanges easier.\n\nWhat do you think?\n\nThanks,\nJonathan\n"},{"id":"195671","messageId":"500F26BF.7090501@pobox.com","threadId":"31088","inReplyTo":"20120724220207.GA15969@burratino","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-24T22:50:39Z","receivedAt":"2012-07-24T22:50:39Z","isPatch":false,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.24 3:02 PM, Jonathan Nieder wrote:\n>> The Git::SVN extraction is more complicated than the rest, so I'll probably do\n>> that separately and bust it up into a few commits.\n> \n> All of these changes are supposed to have zero functional effect,\n> right?\n\nRight.  They're just class extraction refactoring.  I don't even need to make\nnew classes, they already exist.  It's just moving them into their own file.\n\n\n> Could you send the first five patches that *are* supposed to have a\n> functional effect?  I know that they will not apply cleanly to git-svn\n> from git \"master\" or on top of each other; that's fine with me.  If\n> the approach looks right, interested people (read: probably Ben or I :))\n> can make the corresponding change in the code layout from \"master\".\n> Afterwards, we can look into all that refactoring to make later\n> changes easier.\n> \n> What do you think?\n\nI think that would be a lot of extra work for me, create a big mess and be\nharder to understand. :-/\n\nSince I'm creating new files to store the classes, the functional changes\ncannot be applied without the class extractions, so I'd have to rewrite them.\n And they will be harder to review since the main git-svn code and the class\ncode is mixed up in one file.  And they won't have unit tests, since git-svn\ncannot be loaded without it running.\n\nThe Git::SVN extraction isn't really complicated, it just requires a handful\nmore work than the other classes.  Just a few method extractions.  The rest\nare essentially cut, paste & fix lexicals.\n\nI'm not sure what you're going for, but you can glance through the existing\nchanges here\nhttps://github.com/schwern/git/commits/git-svn/fix-canonical\n\nor you can do grab it as a remote and...\n\n  git log -p schwern/git-svn/extract-classes..schwern/git-svn/fix-canonical\n\nThat should give you the information you need... if I understand what you\nneed.  I feel like I don't and we're talking past each other.\n\n\n-- \nAhh email, my old friend.  Do you know that revenge is a dish that is best\nserved cold?  And it is very cold on the Internet!\n"},{"id":"195673","messageId":"20120724230300.GA16333@burratino","threadId":"31088","inReplyTo":"500F26BF.7090501@pobox.com","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-24T23:03:00Z","receivedAt":"2012-07-24T23:03:00Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nMichael G Schwern wrote:\n> On 2012.7.24 3:02 PM, Jonathan Nieder wrote:\n\n>> Could you send the first five patches that *are* supposed to have a\n>> functional effect?  I know that they will not apply cleanly to git-svn\n>> from git \"master\" or on top of each other; that's fine with me.  If\n>> the approach looks right, interested people (read: probably Ben or I :))\n>> can make the corresponding change in the code layout from \"master\".\n[...]\n> I think that would be a lot of extra work for me, create a big mess and be\n> harder to understand. :-/\n>\n> Since I'm creating new files to store the classes, the functional changes\n> cannot be applied without the class extractions, so I'd have to rewrite them.\n\nI don't understand.  Didn't I ask to send the changes as-is and say\nthat *I or someone else interested* would do the work to get them to\napply?\n\n[...]\n> That should give you the information you need... if I understand what you\n> need.  I feel like I don't and we're talking past each other.\n\nBasically, you are offering a code fix that's at least worth $500.\nLots of people have wanted the bug fixed for a long time.  As long as\nit does not involve sacrificing maintainability, we should apply the\nfix as soon as possible!  It's great that you've done this work.\n\nMeanwhile that change is being held hostage by lots of cleanups that\nare unquestionably going in the right direction but are going to be a\npain in the neck to safely apply.  And no one has reviewed your fix\nthat comes _after_ the cleanups.  Maybe the fix goes in a wrong\ndirection --- we don't know yet.  Maybe once we understand the fix\nwe'll have ideas that obsolete the previous cleanups and can more\nsimply accomplish the same thing by organizing the code a different\nway.\n\nYou are saying that, to make your life easier, we should take your\ncleanups and fixes as-is, all in one big pull.  Maybe you're right!\nBut it will take a lot longer this way than applying a smaller set of\nchanges that just fixes the bug.\n\nI am saying that that, before anything else, it would be helpful for\nus to *see* the relevant patches and understand your fix.  You are\nmore knowledgeable than anyone else about your code, so presumably it\nshould be straightforward to pick out which patches are the important\nones.  Using \"git format-patch -1 <commit>\" you can get a patch for\neach.  Then you can use your favorite text editor to edit their\nsubject lines and change descriptions to describe what they do and\nwhere they fall in the series of patches you are sending.  Finally you\ncan use your favorite mail user agent (e.g., \"git send-email\") to send\nout the patches.\n\nJonathan\n"},{"id":"195674","messageId":"500F2A73.4000502@pobox.com","threadId":"31088","inReplyTo":"7vipdcesk6.fsf@alter.siamese.dyndns.org","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-24T23:06:27Z","receivedAt":"2012-07-24T23:06:27Z","isPatch":false,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.24 2:51 PM, Junio C Hamano wrote:\n> Michael G Schwern <schwern@pobox.com> writes:\n> \n>> A big one is \"do not blast 10 emails to a mailing list\" but I gather that's ok\n>> here if a submission needs 10 commits to be well expressed and its done via\n>> git-send-email?  And then if patch #3 needs revision I'm to do it in a rebase\n>> and resend the whole 10 commits?  Am I to think of git-send-email less as a\n>> means of sending patches to a mailing list and more as a git transport mechanism?\n> \n> Yes, yes and whatever (even though I think send-email is just a\n> better MUA/MSA when you want to send patches and isn't restricted\n> for a _git_ transport, I do not think it matters how you look at it).\n\n#3 was not intended as a dig.  If I can think about git-send-email like a\nfunny way to do a git-push then that fits better in my head.  I worry about\nsending too many emails to a list at once.  I don't worry about sending too\nmany commits in one push.\n\n\n>> I'm trying to bust it up into easier to digest pieces.  I came into this cold\n>> without much knowledge of the problem (\"something to do with\n>> canonicalization\") and no knowledge of the code.\n> \n> Perhaps it is a good idea to lurk and see how others submit their\n> topics first?\n\nWhile I use git heavily I'm not invested in working on it.  I work on a lot of\nprojects.  I'd like to be able to do the work, submit it, work through review,\nand get out without joining another mailing list and studying their culture.\n\nIs there a document I could look at for submitting a large body of work, or\ncould I help improve SubmittingPatches to document the process better?\n\n\n-- \nI do have a cause though. It's obscenity. I'm for it.\n    - Tom Lehrer\n"},{"id":"195675","messageId":"20120724231225.GB16333@burratino","threadId":"31088","inReplyTo":"500F2A73.4000502@pobox.com","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-24T23:12:25Z","receivedAt":"2012-07-24T23:12:25Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael G Schwern wrote:\n\n> While I use git heavily I'm not invested in working on it.  I work on a lot of\n> projects.  I'd like to be able to do the work, submit it, work through review,\n> and get out without joining another mailing list and studying their culture.\n\nAn alternative approach would be to _just_ explain how your patch\nseries works, and then say\n\n\t\"Ok, here is the git branch.  Have fun with it.  If someone wants to\n\tincorporate it, that's great.  Let me know if you have any questions.\"\n\nThat's totally fine.  Then someone else can submit the patches and\nhelp to massage the patches into a form that is ready for application\non your behalf.\n\n> Is there a document I could look at for submitting a large body of work, or\n> could I help improve SubmittingPatches to document the process better?\n\nI thought SubmittingPatches did describe how to send patch series.\nCould you point to the section that was lacking?  Or are you just\nstill in disbelief that people review each patch in a thread, one by\none, sending emails to each other?\n\nGrateful but impatient,\nJonathan\n"},{"id":"195677","messageId":"20120724233128.GD16333@burratino","threadId":"31088","inReplyTo":"500F26BF.7090501@pobox.com","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-24T23:31:28Z","receivedAt":"2012-07-24T23:31:28Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael G Schwern wrote:\n\n>   git log -p schwern/git-svn/extract-classes..schwern/git-svn/fix-canonical\n>\n> That should give you the information you need...\n\nI guess so.  May we have your sign-off on these changes?  (A simple\nreply of \"yes\" is enough, no need to resend patches to do this.)\n\nHere it is in patch form for reviewers.  If I understand correctly,\nthe idea is to replace accesses to $gs->{path} with calls to a\n$gs->path function that canonicalizes (and likewise for s/path/url/).\n\nThere are probably other subtleties, but that seems to be the gist.\n---\n git-svn.perl                          |   96 +++++++--------------\n perl/Git/SVN.pm                       |  148 +++++++++++++++++++++------------\n perl/Git/SVN/Fetcher.pm               |    2 +-\n perl/Git/SVN/Migration.pm             |    6 +-\n perl/Git/SVN/Ra.pm                    |   96 ++++++++++++---------\n perl/Git/SVN/Utils.pm                 |  123 ++++++++++++++++++++++++++-\n t/Git-SVN/Ra/fix_dir.t                |   24 ++++++\n t/lib-git-svn.sh                      |    2 +\n t/t9118-git-svn-funky-branch-names.sh |    6 +-\n t/t9119-git-svn-info.sh               |    7 +-\n 10 files changed, 341 insertions(+), 169 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 7b54f43f..74aeb4df 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -12,7 +12,12 @@ $VERSION = '@@GIT_VERSION@@';\n \n use Git::SVN;\n \n-use Git::SVN::Utils qw(fatal can_compress);\n+use Git::SVN::Utils qw(\n+    fatal\n+    can_compress\n+    canonicalize_path\n+    canonicalize_url\n+);\n \n # From which subdir have we been invoked?\n my $cmd_dir_prefix = eval {\n@@ -100,8 +105,6 @@ BEGIN {\n \tMemoize::memoize 'Git::config_bool';\n }\n \n-my ($SVN);\n-\n $sha1 = qr/[a-f\\d]{40}/;\n $sha1_short = qr/[a-f\\d]{4,40}/;\n my ($_stdin, $_help, $_edit,\n@@ -518,7 +521,6 @@ sub cmd_init {\n \t}\n \tmy $url = shift or die \"SVN repository location required \",\n \t                       \"as a command-line argument\\n\";\n-\t$url = canonicalize_url($url);\n \tinit_subdir(@_);\n \tdo_git_init_db();\n \n@@ -1199,7 +1201,7 @@ sub cmd_show_ignore {\n \tmy ($url, $rev, $uuid, $gs) = working_head_info('HEAD');\n \t$gs ||= Git::SVN->new;\n \tmy $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);\n-\t$gs->prop_walk($gs->{path}, $r, sub {\n+\t$gs->prop_walk($gs->path, $r, sub {\n \t\tmy ($gs, $path, $props) = @_;\n \t\tprint STDOUT \"\\n# $path\\n\";\n \t\tmy $s = $props->{'svn:ignore'} or return;\n@@ -1215,7 +1217,7 @@ sub cmd_show_externals {\n \tmy ($url, $rev, $uuid, $gs) = working_head_info('HEAD');\n \t$gs ||= Git::SVN->new;\n \tmy $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);\n-\t$gs->prop_walk($gs->{path}, $r, sub {\n+\t$gs->prop_walk($gs->path, $r, sub {\n \t\tmy ($gs, $path, $props) = @_;\n \t\tprint STDOUT \"\\n# $path\\n\";\n \t\tmy $s = $props->{'svn:externals'} or return;\n@@ -1230,7 +1232,7 @@ sub cmd_create_ignore {\n \tmy ($url, $rev, $uuid, $gs) = working_head_info('HEAD');\n \t$gs ||= Git::SVN->new;\n \tmy $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);\n-\t$gs->prop_walk($gs->{path}, $r, sub {\n+\t$gs->prop_walk($gs->path, $r, sub {\n \t\tmy ($gs, $path, $props) = @_;\n \t\t# $path is of the form /path/to/dir/\n \t\t$path = '.' . $path;\n@@ -1260,36 +1262,12 @@ sub cmd_mkdirs {\n \t$gs->mkemptydirs($_revision);\n }\n \n-sub canonicalize_path {\n-\tmy ($path) = @_;\n-\tmy $dot_slash_added = 0;\n-\tif (substr($path, 0, 1) ne \"/\") {\n-\t\t$path = \"./\" . $path;\n-\t\t$dot_slash_added = 1;\n-\t}\n-\t# File::Spec->canonpath doesn't collapse x/../y into y (for a\n-\t# good reason), so let's do this manually.\n-\t$path =~ s#/+#/#g;\n-\t$path =~ s#/\\.(?:/|$)#/#g;\n-\t$path =~ s#/[^/]+/\\.\\.##g;\n-\t$path =~ s#/$##g;\n-\t$path =~ s#^\\./## if $dot_slash_added;\n-\t$path =~ s#^/##;\n-\t$path =~ s#^\\.$##;\n-\treturn $path;\n-}\n-\n-sub canonicalize_url {\n-\tmy ($url) = @_;\n-\t$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;\n-\treturn $url;\n-}\n-\n # get_svnprops(PATH)\n # ------------------\n # Helper for cmd_propget and cmd_proplist below.\n sub get_svnprops {\n \tmy $path = shift;\n+\n \tmy ($url, $rev, $uuid, $gs) = working_head_info('HEAD');\n \t$gs ||= Git::SVN->new;\n \n@@ -1298,7 +1276,8 @@ sub get_svnprops {\n \t$path = $cmd_dir_prefix . $path;\n \tfatal(\"No such file or directory: $path\") unless -e $path;\n \tmy $is_dir = -d $path ? 1 : 0;\n-\t$path = $gs->{path} . '/' . $path;\n+\n+\t$path = $gs->path . '/' . $path if $gs->path;\n \n \t# canonicalize the path (otherwise libsvn will abort or fail to\n \t# find the file)\n@@ -1399,8 +1378,8 @@ sub cmd_commit_diff {\n \t\t\tfatal(\"Needed URL or usable git-svn --id in \",\n \t\t\t      \"the command-line\\n\", $usage);\n \t\t}\n-\t\t$url = $gs->{url};\n-\t\t$svn_path = $gs->{path};\n+\t\t$url = $gs->url;\n+\t\t$svn_path = $gs->path;\n \t}\n \tunless (defined $_revision) {\n \t\tfatal(\"-r|--revision is a required argument\\n\", $usage);\n@@ -1434,25 +1413,6 @@ sub cmd_commit_diff {\n \t}\n }\n \n-sub escape_uri_only {\n-\tmy ($uri) = @_;\n-\tmy @tmp;\n-\tforeach (split m{/}, $uri) {\n-\t\ts/([^~\\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf(\"%%%02X\",ord($1))/eg;\n-\t\tpush @tmp, $_;\n-\t}\n-\tjoin('/', @tmp);\n-}\n-\n-sub escape_url {\n-\tmy ($url) = @_;\n-\tif ($url =~ m#^([^:]+)://([^/]*)(.*)$#) {\n-\t\tmy ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));\n-\t\t$url = \"$scheme://$domain$uri\";\n-\t}\n-\t$url;\n-}\n-\n sub cmd_info {\n \tmy $path = canonicalize_path(defined($_[0]) ? $_[0] : \".\");\n \tmy $fullpath = canonicalize_path($cmd_dir_prefix . $path);\n@@ -1476,21 +1436,21 @@ sub cmd_info {\n \t# canonicalize_path() will return \"\" to make libsvn 1.5.x happy,\n \t$path = \".\" if $path eq \"\";\n \n-\tmy $full_url = $url . ($fullpath eq \"\" ? \"\" : \"/$fullpath\");\n+\tmy $full_url = canonicalize_url( $url . ($fullpath eq \"\" ? \"\" : \"/$fullpath\") );\n \n \tif ($_url) {\n-\t\tprint escape_url($full_url), \"\\n\";\n+\t\tprint $full_url, \"\\n\";\n \t\treturn;\n \t}\n \n \tmy $result = \"Path: $path\\n\";\n \t$result .= \"Name: \" . basename($path) . \"\\n\" if $file_type ne \"dir\";\n-\t$result .= \"URL: \" . escape_url($full_url) . \"\\n\";\n+\t$result .= \"URL: $full_url\\n\";\n \n \teval {\n \t\tmy $repos_root = $gs->repos_root;\n \t\tGit::SVN::remove_username($repos_root);\n-\t\t$result .= \"Repository Root: \" . escape_url($repos_root) . \"\\n\";\n+\t\t$result .= \"Repository Root: \" . canonicalize_url($repos_root) . \"\\n\";\n \t};\n \tif ($@) {\n \t\t$result .= \"Repository Root: (offline)\\n\";\n@@ -1638,7 +1598,10 @@ sub post_fetch_checkout {\n \n sub complete_svn_url {\n \tmy ($url, $path) = @_;\n-\t$path =~ s#/+$##;\n+\n+\t$path = canonicalize_path($path);\n+\n+\t# The path is not a URL\n \tif ($path !~ m#^[a-z\\+]+://#) {\n \t\tif (!defined $url || $url !~ m#^[a-z\\+]+://#) {\n \t\t\tfatal(\"E: '$path' is not a complete URL \",\n@@ -1655,7 +1618,8 @@ sub complete_url_ls_init {\n \t\tprint STDERR \"W: $switch not specified\\n\";\n \t\treturn;\n \t}\n-\t$repo_path =~ s#/+$##;\n+\n+\t$repo_path = canonicalize_path($repo_path);\n \tif ($repo_path =~ m#^[a-z\\+]+://#) {\n \t\t$ra = Git::SVN::Ra->new($repo_path);\n \t\t$repo_path = '';\n@@ -1666,18 +1630,18 @@ sub complete_url_ls_init {\n \t\t\t      \"and a separate URL is not specified\");\n \t\t}\n \t}\n-\tmy $url = $ra->{url};\n+\tmy $url = $ra->url;\n \tmy $gs = Git::SVN->init($url, undef, undef, undef, 1);\n \tmy $k = \"svn-remote.$gs->{repo_id}.url\";\n \tmy $orig_url = eval { command_oneline(qw/config --get/, $k) };\n-\tif ($orig_url && ($orig_url ne $gs->{url})) {\n+\tif ($orig_url && ($orig_url ne $gs->url)) {\n \t\tdie \"$k already set: $orig_url\\n\",\n-\t\t    \"wanted to set to: $gs->{url}\\n\";\n+\t\t    \"wanted to set to: $gs->url\\n\";\n \t}\n-\tcommand_oneline('config', $k, $gs->{url}) unless $orig_url;\n-\tmy $remote_path = \"$gs->{path}/$repo_path\";\n+\tcommand_oneline('config', $k, $gs->url) unless $orig_url;\n+\n+\tmy $remote_path = canonicalize_path($gs->path . \"/$repo_path\");\n \t$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;\n-\t$remote_path =~ s#/+#/#g;\n \t$remote_path =~ s#^/##g;\n \t$remote_path .= \"/*\" if $remote_path !~ /\\*/;\n \tmy ($n) = ($switch =~ /^--(\\w+)/);\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 247ee1d0..ebfa8da5 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -23,7 +23,12 @@ BEGIN {\n }\n \n use Git qw(command command_oneline command_noisy command_output_pipe command_close_pipe);\n-use Git::SVN::Utils qw(fatal can_compress);\n+use Git::SVN::Utils qw(\n+    fatal\n+    can_compress\n+    canonicalize_url\n+    canonicalize_path\n+);\n \n our $_follow_parent  = 1;\n our $_minimize_url   = 'unset';\n@@ -310,12 +315,12 @@ sub init_remote_config {\n \t\t\t\tprint STDERR \"Using higher level of URL: \",\n \t\t\t\t\t     \"$url => $min_url\\n\";\n \t\t\t}\n-\t\t\tmy $old_path = $self->{path};\n-\t\t\t$self->{path} = $url;\n-\t\t\t$self->{path} =~ s!^\\Q$min_url\\E(/|$)!!;\n+\t\t\tmy $old_path = $self->path;\n+\t\t\t$url =~ s!^\\Q$min_url\\E(/|$)!!;\n \t\t\tif (length $old_path) {\n-\t\t\t\t$self->{path} .= \"/$old_path\";\n+\t\t\t\t$url .= \"/$old_path\";\n \t\t\t}\n+\t\t\t$self->path($url);\n \t\t\t$url = $min_url;\n \t\t}\n \t}\n@@ -339,13 +344,15 @@ sub init_remote_config {\n \tunless ($no_write) {\n \t\tcommand_noisy('config',\n \t\t\t      \"svn-remote.$self->{repo_id}.url\", $url);\n-\t\t$self->{path} =~ s{^/}{};\n-\t\t$self->{path} =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;\n+\t\tmy $path = $self->path;\n+\t\t$path =~ s{^/}{};\n+\t\t$path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;\n+\t\t$self->path($path);\n \t\tcommand_noisy('config', '--add',\n \t\t\t      \"svn-remote.$self->{repo_id}.fetch\",\n-\t\t\t      \"$self->{path}:\".$self->refname);\n+\t\t\t      $self->path.\":\".$self->refname);\n \t}\n-\t$self->{url} = $url;\n+\t$self->url($url);\n }\n \n sub find_by_url { # repos_root and, path are optional\n@@ -431,20 +438,19 @@ sub new {\n \t\t}\n \t}\n \tmy $self = _new($class, $repo_id, $ref_id, $path);\n-\tif (!defined $self->{path} || !length $self->{path}) {\n+\tif (!defined $self->path || !length $self->path) {\n \t\tmy $fetch = command_oneline('config', '--get',\n \t\t                            \"svn-remote.$repo_id.fetch\",\n \t\t                            \":$ref_id\\$\") or\n \t\t     die \"Failed to read \\\"svn-remote.$repo_id.fetch\\\" \",\n \t\t         \"\\\":$ref_id\\$\\\" in config\\n\";\n-\t\t($self->{path}, undef) = split(/\\s*:\\s*/, $fetch);\n+\t\tmy($path) = split(/\\s*:\\s*/, $fetch);\n+\t\t$self->path($path);\n \t}\n-\t$self->{path} =~ s{/+}{/}g;\n-\t$self->{path} =~ s{\\A/}{};\n-\t$self->{path} =~ s{/\\z}{};\n-\t$self->{url} = command_oneline('config', '--get',\n-\t                               \"svn-remote.$repo_id.url\") or\n+\tmy $url = command_oneline('config', '--get',\n+\t                          \"svn-remote.$repo_id.url\") or\n                   die \"Failed to read \\\"svn-remote.$repo_id.url\\\" in config\\n\";\n+\t$self->url($url);\n \t$self->{pushurl} = eval { command_oneline('config', '--get',\n \t                          \"svn-remote.$repo_id.pushurl\") };\n \t$self->rebuild;\n@@ -548,7 +554,7 @@ sub _set_svm_vars {\n \t\t# username is of no interest\n \t\t$src =~ s{(^[a-z\\+]*://)[^/@]*@}{$1};\n \n-\t\tmy $replace = $ra->{url};\n+\t\tmy $replace = $ra->url;\n \t\t$replace .= \"/$path\" if length $path;\n \n \t\tmy $section = \"svn-remote.$self->{repo_id}\";\n@@ -563,20 +569,21 @@ sub _set_svm_vars {\n \t}\n \n \tmy $r = $ra->get_latest_revnum;\n-\tmy $path = $self->{path};\n+\tmy $path = $self->path;\n \tmy %tried;\n \twhile (length $path) {\n-\t\tunless ($tried{\"$self->{url}/$path\"}) {\n+\t\tmy $try = $self->url . \"/$path\";\n+\t\tunless ($tried{$try}) {\n \t\t\treturn $ra if $self->read_svm_props($ra, $path, $r);\n-\t\t\t$tried{\"$self->{url}/$path\"} = 1;\n+\t\t\t$tried{$try} = 1;\n \t\t}\n \t\t$path =~ s#/?[^/]+$##;\n \t}\n \tdie \"Path: '$path' should be ''\\n\" if $path ne '';\n \treturn $ra if $self->read_svm_props($ra, $path, $r);\n-\t$tried{\"$self->{url}/$path\"} = 1;\n+\t$tried{$self->url.\"/$path\"} = 1;\n \n-\tif ($ra->{repos_root} eq $self->{url}) {\n+\tif ($ra->{repos_root} eq $self->url) {\n \t\tdie @err, (map { \"  $_\\n\" } keys %tried), \"\\n\";\n \t}\n \n@@ -586,20 +593,21 @@ sub _set_svm_vars {\n \t$path = $ra->{svn_path};\n \t$ra = Git::SVN::Ra->new($ra->{repos_root});\n \twhile (length $path) {\n-\t\tunless ($tried{\"$ra->{url}/$path\"}) {\n+\t\tmy $try = $ra->url .\"/$path\";\n+\t\tunless ($tried{$try}) {\n \t\t\t$ok = $self->read_svm_props($ra, $path, $r);\n \t\t\tlast if $ok;\n-\t\t\t$tried{\"$ra->{url}/$path\"} = 1;\n+\t\t\t$tried{$try} = 1;\n \t\t}\n \t\t$path =~ s#/?[^/]+$##;\n \t}\n \tdie \"Path: '$path' should be ''\\n\" if $path ne '';\n \t$ok ||= $self->read_svm_props($ra, $path, $r);\n-\t$tried{\"$ra->{url}/$path\"} = 1;\n+\t$tried{$ra->url .\"/$path\"} = 1;\n \tif (!$ok) {\n \t\tdie @err, (map { \"  $_\\n\" } keys %tried), \"\\n\";\n \t}\n-\tGit::SVN::Ra->new($self->{url});\n+\tGit::SVN::Ra->new($self->url);\n }\n \n sub svnsync {\n@@ -666,7 +674,7 @@ sub ra_uuid {\n \t\tif (!$@ && $uuid && $uuid =~ /^([a-f\\d\\-]{30,})$/i) {\n \t\t\t$self->{ra_uuid} = $uuid;\n \t\t} else {\n-\t\t\tdie \"ra_uuid called without URL\\n\" unless $self->{url};\n+\t\t\tdie \"ra_uuid called without URL\\n\" unless $self->url;\n \t\t\t$self->{ra_uuid} = $self->ra->get_uuid;\n \t\t\ttmp_config('--add', $key, $self->{ra_uuid});\n \t\t}\n@@ -690,7 +698,7 @@ sub repos_root {\n \n sub ra {\n \tmy ($self) = shift;\n-\tmy $ra = Git::SVN::Ra->new($self->{url});\n+\tmy $ra = Git::SVN::Ra->new($self->url);\n \t$self->_set_repos_root($ra->{repos_root});\n \tif ($self->use_svm_props && !$self->{svm}) {\n \t\tif ($self->no_metadata) {\n@@ -724,7 +732,7 @@ sub prop_walk {\n \t$path =~ s#^/*#/#g;\n \tmy $p = $path;\n \t# Strip the irrelevant part of the path.\n-\t$p =~ s#^/+\\Q$self->{path}\\E(/|$)#/#;\n+\t$p =~ s#^/+\\Q@{[$self->path]}\\E(/|$)#/#;\n \t# Ensure the path is terminated by a `/'.\n \t$p =~ s#/*$#/#;\n \n@@ -745,7 +753,7 @@ sub prop_walk {\n \n \tforeach (sort keys %$dirent) {\n \t\tnext if $dirent->{$_}->{kind} != $SVN::Node::dir;\n-\t\t$self->prop_walk($self->{path} . $p . $_, $rev, $sub);\n+\t\t$self->prop_walk($self->path . $p . $_, $rev, $sub);\n \t}\n }\n \n@@ -915,20 +923,20 @@ sub rewrite_uuid {\n \n sub metadata_url {\n \tmy ($self) = @_;\n-\t($self->rewrite_root || $self->{url}) .\n-\t   (length $self->{path} ? '/' . $self->{path} : '');\n+\t($self->rewrite_root || $self->url) .\n+\t   (length $self->path ? '/' . $self->path : '');\n }\n \n sub full_url {\n \tmy ($self) = @_;\n-\t$self->{url} . (length $self->{path} ? '/' . $self->{path} : '');\n+\treturn canonicalize_url( $self->url . (length $self->path ? '/' . $self->path : '') );\n }\n \n sub full_pushurl {\n \tmy ($self) = @_;\n \tif ($self->{pushurl}) {\n-\t\treturn $self->{pushurl} . (length $self->{path} ? '/' .\n-\t\t       $self->{path} : '');\n+\t\treturn $self->{pushurl} . (length $self->path ? '/' .\n+\t\t       $self->path : '');\n \t} else {\n \t\treturn $self->full_url;\n \t}\n@@ -1044,20 +1052,20 @@ sub do_git_commit {\n \n sub match_paths {\n \tmy ($self, $paths, $r) = @_;\n-\treturn 1 if $self->{path} eq '';\n-\tif (my $path = $paths->{\"/$self->{path}\"}) {\n+\treturn 1 if $self->path eq '';\n+\tif (my $path = $paths->{\"/\".$self->path}) {\n \t\treturn ($path->{action} eq 'D') ? 0 : 1;\n \t}\n-\t$self->{path_regex} ||= qr/^\\/\\Q$self->{path}\\E\\//;\n+\t$self->{path_regex} ||= qr{^/\\Q@{[$self->path]}\\E/};\n \tif (grep /$self->{path_regex}/, keys %$paths) {\n \t\treturn 1;\n \t}\n \tmy $c = '';\n-\tforeach (split m#/#, $self->{path}) {\n+\tforeach (split m#/#, $self->path) {\n \t\t$c .= \"/$_\";\n \t\tnext unless ($paths->{$c} &&\n \t\t             ($paths->{$c}->{action} =~ /^[AR]$/));\n-\t\tif ($self->ra->check_path($self->{path}, $r) ==\n+\t\tif ($self->ra->check_path($self->path, $r) ==\n \t\t    $SVN::Node::dir) {\n \t\t\treturn 1;\n \t\t}\n@@ -1071,14 +1079,14 @@ sub find_parent_branch {\n \tunless (defined $paths) {\n \t\tmy $err_handler = $SVN::Error::handler;\n \t\t$SVN::Error::handler = \\&Git::SVN::Ra::skip_unknown_revs;\n-\t\t$self->ra->get_log([$self->{path}], $rev, $rev, 0, 1, 1,\n+\t\t$self->ra->get_log([$self->path], $rev, $rev, 0, 1, 1,\n \t\t\t\t   sub { $paths = $_[0] });\n \t\t$SVN::Error::handler = $err_handler;\n \t}\n \treturn undef unless defined $paths;\n \n \t# look for a parent from another branch:\n-\tmy @b_path_components = split m#/#, $self->{path};\n+\tmy @b_path_components = split m#/#, $self->path;\n \tmy @a_path_components;\n \tmy $i;\n \twhile (@b_path_components) {\n@@ -1089,14 +1097,12 @@ sub find_parent_branch {\n \treturn undef unless defined $i && defined $i->{copyfrom_path};\n \tmy $branch_from = $i->{copyfrom_path};\n \tif (@a_path_components) {\n-\t\tprint STDERR \"branch_from: $branch_from => \";\n \t\t$branch_from .= '/'.join('/', @a_path_components);\n-\t\tprint STDERR $branch_from, \"\\n\";\n \t}\n \tmy $r = $i->{copyfrom_rev};\n \tmy $repos_root = $self->ra->{repos_root};\n-\tmy $url = $self->ra->{url};\n-\tmy $new_url = $url . $branch_from;\n+\tmy $url = $self->ra->url;\n+\tmy $new_url = canonicalize_url($url . $branch_from);\n \tprint STDERR  \"Found possible branch point: \",\n \t              \"$new_url => \", $self->full_url, \", $r\\n\"\n \t              unless $::_q > 1;\n@@ -1110,7 +1116,7 @@ sub find_parent_branch {\n \t\t\t($base, $head) = parse_revision_argument(0, $r);\n \t\t} else {\n \t\t\tif ($r0 < $r) {\n-\t\t\t\t$gs->ra->get_log([$gs->{path}], $r0 + 1, $r, 1,\n+\t\t\t\t$gs->ra->get_log([$gs->path], $r0 + 1, $r, 1,\n \t\t\t\t\t0, 1, sub { $base = $_[1] - 1 });\n \t\t\t}\n \t\t}\n@@ -1132,7 +1138,7 @@ sub find_parent_branch {\n \t\t\t# at the moment), so we can't rely on it\n \t\t\t$self->{last_rev} = $r0;\n \t\t\t$self->{last_commit} = $parent;\n-\t\t\t$ed = Git::SVN::Fetcher->new($self, $gs->{path});\n+\t\t\t$ed = Git::SVN::Fetcher->new($self, $gs->path);\n \t\t\t$gs->ra->gs_do_switch($r0, $rev, $gs,\n \t\t\t\t\t      $self->full_url, $ed)\n \t\t\t  or die \"SVN connection failed somewhere...\\n\";\n@@ -1231,7 +1237,7 @@ sub mkemptydirs {\n \t\tclose $fh;\n \t}\n \n-\tmy $strip = qr/\\A\\Q$self->{path}\\E(?:\\/|$)/;\n+\tmy $strip = qr/\\A\\Q@{[$self->path]}\\E(?:\\/|$)/;\n \tforeach my $d (sort keys %empty_dirs) {\n \t\t$d = uri_decode($d);\n \t\t$d =~ s/$strip//;\n@@ -1425,7 +1431,7 @@ sub find_extra_svk_parents {\n \tfor my $ticket ( @tickets ) {\n \t\tmy ($uuid, $path, $rev) = split /:/, $ticket;\n \t\tif ( $uuid eq $self->ra_uuid ) {\n-\t\t\tmy $url = $self->{url};\n+\t\t\tmy $url = $self->url;\n \t\t\tmy $repos_root = $url;\n \t\t\tmy $branch_from = $path;\n \t\t\t$branch_from =~ s{^/}{};\n@@ -1671,7 +1677,7 @@ sub find_extra_svn_parents {\n \t# are now marked as merge, we can add the tip as a parent.\n \tmy @merges = split \"\\n\", $mergeinfo;\n \tmy @merge_tips;\n-\tmy $url = $self->{url};\n+\tmy $url = $self->url;\n \tmy $uuid = $self->ra_uuid;\n \tmy %ranges;\n \tfor my $merge ( @merges ) {\n@@ -1854,7 +1860,7 @@ sub make_log_entry {\n \t\t$commit_email ||= \"$author\\@$uuid\";\n \t} elsif ($self->use_svnsync_props) {\n \t\tmy $full_url = $self->svnsync->{url};\n-\t\t$full_url .= \"/$self->{path}\" if length $self->{path};\n+\t\t$full_url .= \"/\".$self->path if length $self->path;\n \t\tremove_username($full_url);\n \t\tmy $uuid = $self->svnsync->{uuid};\n \t\t$log_entry{metadata} = \"$full_url\\@$rev $uuid\";\n@@ -1901,7 +1907,7 @@ sub set_tree {\n \t                tree_b => $tree,\n \t                editor_cb => sub {\n \t\t\t       $self->set_tree_cb($log_entry, $tree, @_) },\n-\t                svn_path => $self->{path} );\n+\t                svn_path => $self->path );\n \tif (!Git::SVN::Editor->new(\\%ed_opts)->apply_diff) {\n \t\tprint \"No changes\\nr$self->{last_rev} = $tree\\n\";\n \t}\n@@ -2272,12 +2278,44 @@ sub _new {\n \n \t$_[3] = $path = '' unless (defined $path);\n \tmkpath([$dir]);\n-\tbless {\n+\tmy $obj = bless {\n \t\tref_id => $ref_id, dir => $dir, index => \"$dir/index\",\n-\t        path => $path, config => \"$ENV{GIT_DIR}/svn/config\",\n+\t        config => \"$ENV{GIT_DIR}/svn/config\",\n \t        map_root => \"$dir/.rev_map\", repo_id => $repo_id }, $class;\n+\n+\t# Ensure it gets canonicalized\n+\t$obj->path($path);\n+\n+\treturn $obj;\n+}\n+\n+\n+sub path {\n+    my $self = shift;\n+\n+    if( @_ ) {\n+        my $path = shift;\n+        $self->{path} = canonicalize_path($path);\n+        return;\n+    }\n+\n+    return $self->{path};\n }\n \n+\n+sub url {\n+    my $self = shift;\n+\n+    if( @_ ) {\n+        my $url = shift;\n+        $self->{url} = canonicalize_url($url);\n+        return;\n+    }\n+\n+    return $self->{url};\n+}\n+\n+\n # for read-only access of old .rev_db formats\n sub unlink_rev_db_symlink {\n \tmy ($self) = @_;\ndiff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\nindex 69486efe..b606fe5a 100644\n--- a/perl/Git/SVN/Fetcher.pm\n+++ b/perl/Git/SVN/Fetcher.pm\n@@ -84,7 +84,7 @@ sub _mark_empty_symlinks {\n \tchomp(my $empty_blob = `git hash-object -t blob --stdin < /dev/null`);\n \tmy ($ls, $ctx) = command_output_pipe(qw/ls-tree -r -z/, $cmt);\n \tlocal $/ = \"\\0\";\n-\tmy $pfx = defined($switch_path) ? $switch_path : $git_svn->{path};\n+\tmy $pfx = defined($switch_path) ? $switch_path : $git_svn->path;\n \t$pfx .= '/' if length($pfx);\n \twhile (<$ls>) {\n \t\tchomp;\ndiff --git a/perl/Git/SVN/Migration.pm b/perl/Git/SVN/Migration.pm\nindex b17fe003..5a9d3c71 100644\n--- a/perl/Git/SVN/Migration.pm\n+++ b/perl/Git/SVN/Migration.pm\n@@ -180,14 +180,14 @@ sub minimize_connections {\n \t\tmy $ra = Git::SVN::Ra->new($url);\n \n \t\t# skip existing cases where we already connect to the root\n-\t\tif (($ra->{url} eq $ra->{repos_root}) ||\n+\t\tif (($ra->url eq $ra->{repos_root}) ||\n \t\t    ($ra->{repos_root} eq $repo_id)) {\n-\t\t\t$root_repos->{$ra->{url}} = $repo_id;\n+\t\t\t$root_repos->{$ra->url} = $repo_id;\n \t\t\tnext;\n \t\t}\n \n \t\tmy $root_ra = Git::SVN::Ra->new($ra->{repos_root});\n-\t\tmy $root_path = $ra->{url};\n+\t\tmy $root_path = $ra->url;\n \t\t$root_path =~ s#^\\Q$ra->{repos_root}\\E(/|$)##;\n \t\tforeach my $path (keys %$fetch) {\n \t\t\tmy $ref_id = $fetch->{$path};\ndiff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\nindex 23ff43e8..7bd6db6f 100644\n--- a/perl/Git/SVN/Ra.pm\n+++ b/perl/Git/SVN/Ra.pm\n@@ -8,6 +8,11 @@ BEGIN {\n \t@ISA = qw(SVN::Ra);\n }\n \n+use Git::SVN::Utils qw(\n+    canonicalize_url\n+    canonicalize_path\n+);\n+\n my ($ra_invalid, $can_do_switch, %ignored_err, $RA);\n \n BEGIN {\n@@ -62,29 +67,10 @@ sub _auth_providers () {\n \t\\@rv;\n }\n \n-sub escape_uri_only {\n-\tmy ($uri) = @_;\n-\tmy @tmp;\n-\tforeach (split m{/}, $uri) {\n-\t\ts/([^~\\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf(\"%%%02X\",ord($1))/eg;\n-\t\tpush @tmp, $_;\n-\t}\n-\tjoin('/', @tmp);\n-}\n-\n-sub escape_url {\n-\tmy ($url) = @_;\n-\tif ($url =~ m#^(https?)://([^/]+)(.*)$#) {\n-\t\tmy ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));\n-\t\t$url = \"$scheme://$domain$uri\";\n-\t}\n-\t$url;\n-}\n-\n sub new {\n \tmy ($class, $url) = @_;\n \t$url =~ s!/+$!!;\n-\treturn $RA if ($RA && $RA->{url} eq $url);\n+\treturn $RA if ($RA && $RA->url eq $url);\n \n \t::_req_svn();\n \n@@ -115,19 +101,37 @@ sub new {\n \t\t\t$Git::SVN::Prompt::_no_auth_cache = 1;\n \t\t}\n \t} # no warnings 'once'\n-\tmy $self = SVN::Ra->new(url => escape_url($url), auth => $baton,\n+\tmy $self = SVN::Ra->new(url => canonicalize_url($url), auth => $baton,\n \t                      config => $config,\n \t\t\t      pool => SVN::Pool->new,\n \t                      auth_provider_callbacks => $callbacks);\n-\t$self->{url} = $url;\n+\t$RA = bless $self, $class;\n+\n+\t# Make sure its canonicalized\n+\t$self->url($url);\n \t$self->{svn_path} = $url;\n \t$self->{repos_root} = $self->get_repos_root;\n \t$self->{svn_path} =~ s#^\\Q$self->{repos_root}\\E(/|$)##;\n \t$self->{cache} = { check_path => { r => 0, data => {} },\n \t                   get_dir => { r => 0, data => {} } };\n-\t$RA = bless $self, $class;\n+\n+\treturn $RA;\n }\n \n+\n+sub url {\n+    my $self = shift;\n+\n+    if( @_ ) {\n+        my $url = shift;\n+        $self->{url} = canonicalize_url($url);\n+        return;\n+    }\n+\n+    return $self->{url};\n+}\n+\n+\n sub check_path {\n \tmy ($self, $path, $r) = @_;\n \tmy $cache = $self->{cache}->{check_path};\n@@ -146,6 +150,9 @@ sub check_path {\n \n sub get_dir {\n \tmy ($self, $dir, $r) = @_;\n+\n+        $dir = $self->fix_dir($dir);\n+\n \tmy $cache = $self->{cache}->{get_dir};\n \tif ($r == $cache->{r}) {\n \t\tif (my $x = $cache->{data}->{$dir}) {\n@@ -164,6 +171,20 @@ sub get_dir {\n \twantarray ? (\\%dirents, $r, $props) : \\%dirents;\n }\n \n+sub fix_dir {\n+    my $self = shift;\n+    my $dir = shift;\n+\n+    # SVN::Ra->get_dir will fail if given foo/.. and the\n+    # canonicalize functions will not fix that.\n+    $dir =~ s{[^/]+/\\.\\./?}{}g;\n+\n+    # It also doesn't like a single dot.\n+    $dir =~ s{^\\.$}{};\n+\n+    return $dir;\n+}\n+\n sub DESTROY {\n \t# do not call the real DESTROY since we store ourselves in $RA\n }\n@@ -195,6 +216,7 @@ sub get_log {\n \t\t\t\tqw/copyfrom_path copyfrom_rev action/;\n \t\t\tif ($s{'copyfrom_path'}) {\n \t\t\t\t$s{'copyfrom_path'} =~ s/$prefix_regex//;\n+\t\t\t\t$s{'copyfrom_path'} = canonicalize_path($s{'copyfrom_path'});\n \t\t\t}\n \t\t\t$_[0]{$p} = \\%s;\n \t\t}\n@@ -246,7 +268,7 @@ sub get_commit_editor {\n sub gs_do_update {\n \tmy ($self, $rev_a, $rev_b, $gs, $editor) = @_;\n \tmy $new = ($rev_a == $rev_b);\n-\tmy $path = $gs->{path};\n+\tmy $path = $gs->path;\n \n \tif ($new && -e $gs->{index}) {\n \t\tunlink $gs->{index} or die\n@@ -282,17 +304,17 @@ sub gs_do_update {\n # svn_ra_reparent didn't work before 1.4)\n sub gs_do_switch {\n \tmy ($self, $rev_a, $rev_b, $gs, $url_b, $editor) = @_;\n-\tmy $path = $gs->{path};\n+\tmy $path = $gs->path;\n \tmy $pool = SVN::Pool->new;\n \n-\tmy $full_url = $self->{url};\n+\tmy $full_url = $self->url;\n \tmy $old_url = $full_url;\n-\t$full_url .= '/' . $path if length $path;\n+\t$full_url = canonicalize_url( $full_url . '/' . $path ) if length $path;\n \tmy ($ra, $reparented);\n \n \tif ($old_url =~ m#^svn(\\+ssh)?://# ||\n \t    ($full_url =~ m#^https?://# &&\n-\t     escape_url($full_url) ne $full_url)) {\n+\t     $full_url ne $full_url)) {\n \t\t$_[0] = undef;\n \t\t$self = undef;\n \t\t$RA = undef;\n@@ -300,12 +322,12 @@ sub gs_do_switch {\n \t\t$ra_invalid = 1;\n \t} elsif ($old_url ne $full_url) {\n \t\tSVN::_Ra::svn_ra_reparent($self->{session}, $full_url, $pool);\n-\t\t$self->{url} = $full_url;\n+\t\t$self->url($full_url);\n \t\t$reparented = 1;\n \t}\n \n \t$ra ||= $self;\n-\t$url_b = escape_url($url_b);\n+\t$url_b = canonicalize_url($url_b);\n \tmy $reporter = $ra->do_switch($rev_b, '', 1, $url_b, $editor, $pool);\n \tmy @lock = (::compare_svn_version('1.2.0') >= 0) ? (undef) : ();\n \t$reporter->set_path('', $rev_a, 0, @lock, $pool);\n@@ -313,7 +335,7 @@ sub gs_do_switch {\n \n \tif ($reparented) {\n \t\tSVN::_Ra::svn_ra_reparent($self->{session}, $old_url, $pool);\n-\t\t$self->{url} = $old_url;\n+\t\t$self->url($old_url);\n \t}\n \n \t$pool->clear;\n@@ -326,7 +348,7 @@ sub longest_common_path {\n \tmy $common_max = scalar @$gsv;\n \n \tforeach my $gs (@$gsv) {\n-\t\tmy @tmp = split m#/#, $gs->{path};\n+\t\tmy @tmp = split m#/#, $gs->path;\n \t\tmy $p = '';\n \t\tforeach (@tmp) {\n \t\t\t$p .= length($p) ? \"/$_\" : $_;\n@@ -362,7 +384,7 @@ sub gs_fetch_loop_common {\n \tmy $inc = $_log_window_size;\n \tmy ($min, $max) = ($base, $head < $base + $inc ? $head : $base + $inc);\n \tmy $longest_path = longest_common_path($gsv, $globs);\n-\tmy $ra_url = $self->{url};\n+\tmy $ra_url = $self->url;\n \tmy $find_trailing_edge;\n \twhile (1) {\n \t\tmy %revs;\n@@ -508,7 +530,7 @@ sub match_globs {\n \t\t\t\t ($self->check_path($p, $r) !=\n \t\t\t\t  $SVN::Node::dir));\n \t\t\tnext unless $p =~ /$g->{path}->{regex}/;\n-\t\t\t$exists->{$p} = Git::SVN->init($self->{url}, $p, undef,\n+\t\t\t$exists->{$p} = Git::SVN->init($self->url, $p, undef,\n \t\t\t\t\t $g->{ref}->full_path($de), 1);\n \t\t}\n \t}\n@@ -532,7 +554,7 @@ sub match_globs {\n \t\t\tnext if ($self->check_path($pathname, $r) !=\n \t\t\t         $SVN::Node::dir);\n \t\t\t$exists->{$pathname} = Git::SVN->init(\n-\t\t\t                      $self->{url}, $pathname, undef,\n+\t\t\t                      $self->url, $pathname, undef,\n \t\t\t                      $g->{ref}->full_path($p), 1);\n \t\t}\n \t\tmy $c = '';\n@@ -548,7 +570,7 @@ sub match_globs {\n \n sub minimize_url {\n \tmy ($self) = @_;\n-\treturn $self->{url} if ($self->{url} eq $self->{repos_root});\n+\treturn $self->url if ($self->url eq $self->{repos_root});\n \tmy $url = $self->{repos_root};\n \tmy @components = split(m!/!, $self->{svn_path});\n \tmy $c = '';\n@@ -568,7 +590,7 @@ sub can_do_switch {\n \tunless (defined $can_do_switch) {\n \t\tmy $pool = SVN::Pool->new;\n \t\tmy $rep = eval {\n-\t\t\t$self->do_switch(1, '', 0, $self->{url},\n+\t\t\t$self->do_switch(1, '', 0, $self->url,\n \t\t\t                 SVN::Delta::Editor->new, $pool);\n \t\t};\n \t\tif ($@) {\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex d8f63e68..e7087435 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -5,10 +5,54 @@ use warnings;\n \n use base qw(Exporter);\n \n-our @EXPORT_OK = qw(fatal can_compress);\n+our @EXPORT_OK = qw(\n+    fatal\n+    can_compress\n+    canonicalize_path\n+    canonicalize_url\n+);\n \n+\n+=head1 NAME\n+\n+Git::SVN::Utils - utility functions used across Git::SVN\n+\n+=head1 SYNOPSIS\n+\n+    use Git::SVN::Utils qw(functions to import);\n+\n+=head1 DESCRIPTION\n+\n+This module contains functions which are useful across many different\n+parts of Git::SVN.  Mostly it's a place to put utility functions\n+rather than duplicate the code or have classes grabbing at other\n+classes.\n+\n+=head1 FUNCTIONS\n+\n+All functions can be imported only on request.\n+\n+=head3 fatal\n+\n+    fatal(@message);\n+\n+Display a message and exit with a fatal error code.\n+\n+=cut\n+\n+# Note: not certain why this is in use instead of die.  Probably because\n+# the exit code of die is 255?  Doesn't appear to be used consistently.\n sub fatal (@) { print STDERR \"@_\\n\"; exit 1 }\n \n+\n+=head3 can_compress\n+\n+    my $can_compress = can_compress;\n+\n+Returns true if Compress::Zlib is available, false otherwise.\n+\n+=cut\n+\n my $can_compress;\n sub can_compress {\n     return $can_compress if defined $can_compress;\n@@ -16,4 +60,81 @@ sub can_compress {\n     return $can_compress = eval { require Compress::Zlib; } ? 1 : 0;\n }\n \n+\n+=head3 canonicalize_path\n+\n+    my $canonical_path = canonicalize_path($path);\n+\n+Cleans up the $path so its acceptable to the SVN API's strict idea of\n+what a path should look like.\n+\n+=cut\n+\n+# SVN 1.7 changed its canonicalization API, so decide on the best way to handle the problem.\n+sub canonicalize_path {\n+    my $path = shift;\n+\n+    # The 1.7 way to do it\n+    if( defined &SVN::_Core::svn_dirent_canonicalize ) {\n+        return SVN::_Core::svn_dirent_canonicalize($path);\n+    }\n+    # The 1.6 way to do it\n+    elsif( defined &SVN::_Core::svn_path_canonicalize ) {\n+        return SVN::_Core::svn_path_canonicalize($path);\n+    }\n+    # No SVN API canonicalization is available, do it ourselves\n+    else {\n+        return _canonicalize_path_ourselves($path);\n+    }\n+}\n+\n+\n+# A pure Perl implementation of path canonicalization in case the SVN\n+# API ones are unavailable.\n+sub _canonicalize_path_ourselves {\n+\tmy ($path) = @_;\n+\tmy $dot_slash_added = 0;\n+\tif (substr($path, 0, 1) ne \"/\") {\n+\t\t$path = \"./\" . $path;\n+\t\t$dot_slash_added = 1;\n+\t}\n+\t# File::Spec->canonpath doesn't collapse x/../y into y (for a\n+\t# good reason), so let's do this manually.\n+\t$path =~ s#/+#/#g;\n+\t$path =~ s#/\\.(?:/|$)#/#g;\n+\t$path =~ s#/[^/]+/\\.\\.##g;\n+\t$path =~ s#/$##g;\n+\t$path =~ s#^\\./## if $dot_slash_added;\n+\t$path =~ s#^/##;\n+\t$path =~ s#^\\.$##;\n+\treturn $path;\n+}\n+\n+\n+=head3 canonicalize_url\n+\n+    my $canonical_url = canonicalize_url($url);\n+\n+Cleans up the $url so its acceptable to the SVN API's strict idea of\n+what a URL should look like.\n+\n+=cut\n+\n+# SVN 1.7 changed its canonicalization API, so decide on the best way to handle the problem.\n+sub canonicalize_url {\n+    my $url = shift;\n+\n+    # The 1.7 way to do it\n+    if( defined &SVN::_Core::svn_uri_canonicalize ) {\n+        $url = SVN::_Core::svn_uri_canonicalize($url);\n+    }\n+    # There wasn't a 1.6 way to do it, so we do it ourself.\n+    else {\n+        $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;\n+    }\n+\n+    return $url;\n+}\n+\n+\n 1;\ndiff --git a/t/Git-SVN/Ra/fix_dir.t b/t/Git-SVN/Ra/fix_dir.t\nnew file mode 100644\nindex 00000000..ec0c858a\n--- /dev/null\n+++ b/t/Git-SVN/Ra/fix_dir.t\n@@ -0,0 +1,24 @@\n+#!/usr/bin/env perl\n+\n+use strict;\n+use warnings;\n+\n+use Test::More 'no_plan';\n+\n+my $CLASS = 'Git::SVN::Ra';\n+require_ok $CLASS;\n+\n+my %tests = (\n+    \"foo/..\"            => \"\",\n+    \"/foo/..\"           => \"/\",\n+    \".\"                 => \"\",\n+    \"foo/../\"           => \"\",\n+    \"foo/bar/baz\"       => \"foo/bar/baz\",\n+    \"foo/bar/../baz\"    => \"foo/baz\",\n+);\n+\n+for my $have (keys %tests) {\n+    my $want = $tests{$have};\n+\n+    is $CLASS->fix_dir($have), $want, \"fix_dir $have => $want\";\n+}\ndiff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\nindex 199f22c2..c6911cdf 100644\n--- a/t/lib-git-svn.sh\n+++ b/t/lib-git-svn.sh\n@@ -68,6 +68,8 @@ svn_cmd () {\n \tsvn \"$orig_svncmd\" --config-dir \"$svnconf\" \"$@\"\n }\n \n+SVN_VERSION=`svn_cmd --version | sed -n -e 's/^svn, version \\(1\\.[0-9]*\\.[0-9]*\\).*$/\\1/p'`\n+\n prepare_httpd () {\n \tfor d in \\\n \t\t\"$SVN_HTTPD_PATH\" \\\ndiff --git a/t/t9118-git-svn-funky-branch-names.sh b/t/t9118-git-svn-funky-branch-names.sh\nindex 63fc982c..66787acc 100755\n--- a/t/t9118-git-svn-funky-branch-names.sh\n+++ b/t/t9118-git-svn-funky-branch-names.sh\n@@ -32,6 +32,10 @@ test_expect_success 'setup svnrepo' '\n \tstart_httpd\n \t'\n \n+# SVN 1.7 will truncate \"not-a%40{0]\" to just \"not-a\".\n+# Look at what SVN wound up naming the branch and use that.\n+non_reflog=`svn_cmd ls \"$svnrepo/pr ject/branches\" | grep not-a | sed 's/\\///'`\n+\n test_expect_success 'test clone with funky branch names' '\n \tgit svn clone -s \"$svnrepo/pr ject\" project &&\n \t(\n@@ -42,7 +46,7 @@ test_expect_success 'test clone with funky branch names' '\n \t\tgit rev-parse \"refs/remotes/%2Eleading_dot\" &&\n \t\tgit rev-parse \"refs/remotes/trailing_dot%2E\" &&\n \t\tgit rev-parse \"refs/remotes/trailing_dotlock%2Elock\" &&\n-\t\tgit rev-parse \"refs/remotes/not-a%40{0}reflog\"\n+\t\tgit rev-parse \"refs/remotes/$non_reflog\"\n \t)\n \t'\n \ndiff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh\nindex ff19695e..9b6075c6 100755\n--- a/t/t9119-git-svn-info.sh\n+++ b/t/t9119-git-svn-info.sh\n@@ -6,14 +6,11 @@ test_description='git svn info'\n \n . ./lib-git-svn.sh\n \n-# Tested with: svn, version 1.4.4 (r25188)\n-# Tested with: svn, version 1.6.[12345689]\n-v=`svn_cmd --version | sed -n -e 's/^svn, version \\(1\\.[0-9]*\\.[0-9]*\\).*$/\\1/p'`\n-case $v in\n+case $SVN_VERSION in\n 1.[456].*)\n \t;;\n *)\n-\tskip_all=\"skipping svn-info test (SVN version: $v not supported)\"\n+\tskip_all=\"skipping svn-info test (SVN version: $SVN_VERSION not supported)\"\n \ttest_done\n \t;;\n esac\n"},{"id":"195678","messageId":"7va9yoenb2.fsf@alter.siamese.dyndns.org","threadId":"31088","inReplyTo":"20120724233128.GD16333@burratino","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-24T23:45:05Z","receivedAt":"2012-07-24T23:45:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Michael G Schwern wrote:\n>\n>>   git log -p schwern/git-svn/extract-classes..schwern/git-svn/fix-canonical\n>>\n>> That should give you the information you need...\n>\n> I guess so.  May we have your sign-off on these changes?  (A simple\n> reply of \"yes\" is enough, no need to resend patches to do this.)\n>\n> Here it is in patch form for reviewers.  If I understand correctly,\n> the idea is to replace accesses to $gs->{path} with calls to a\n> $gs->path function that canonicalizes (and likewise for s/path/url/).\n>\n> There are probably other subtleties, but that seems to be the gist.\n\nThe impression I am getting is that the updated code wants to handle\nURL and paths without any funny encoding, but it is unclear from my\ncursory read (e.g. what goes on with escape_url?).\n\n>  \tif ($old_url =~ m#^svn(\\+ssh)?://# ||\n>  \t    ($full_url =~ m#^https?://# &&\n> -\t     escape_url($full_url) ne $full_url)) {\n> +\t     $full_url ne $full_url)) {\n\nHow can the latter part of this conditional be true?\n"},{"id":"195679","messageId":"500F4530.7030207@pobox.com","threadId":"31088","inReplyTo":"7va9yoenb2.fsf@alter.siamese.dyndns.org","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-25T01:00:32Z","receivedAt":"2012-07-25T01:00:32Z","isPatch":false,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.24 4:45 PM, Junio C Hamano wrote:\n>>>   git log -p schwern/git-svn/extract-classes..schwern/git-svn/fix-canonical\n>>>\n>>> That should give you the information you need...\n>>\n>> I guess so.  May we have your sign-off on these changes?  (A simple\n>> reply of \"yes\" is enough, no need to resend patches to do this.)\n>>\n>> Here it is in patch form for reviewers.  If I understand correctly,\n>> the idea is to replace accesses to $gs->{path} with calls to a\n>> $gs->path function that canonicalizes (and likewise for s/path/url/).\n>>\n>> There are probably other subtleties, but that seems to be the gist.\n> \n> The impression I am getting is that the updated code wants to handle\n> URL and paths without any funny encoding, but it is unclear from my\n> cursory read (e.g. what goes on with escape_url?).\n\nNo, now it's just canonicalizing as early as possible.  Preferably within the\nobject accessor rather than at the point of use.  So in the code below,\n$full_url is already escaped/canonicalized.\n\nIn general this blob patch isn't going to make a lot of overall sense.  I'm\nworking with Jonathan to get it submitted in manageable pieces.\n\n\n>>  \tif ($old_url =~ m#^svn(\\+ssh)?://# ||\n>>  \t    ($full_url =~ m#^https?://# &&\n>> -\t     escape_url($full_url) ne $full_url)) {\n>> +\t     $full_url ne $full_url)) {\n> \n> How can the latter part of this conditional be true?\n\nGood point.  More importantly, what was it trying to accomplish before and\ndoes it need to be preserved?\n\n\"If the URL is svn OR its http and needs to be escaped... do something\nspecial\".  I don't really understand what the special stuff in the following\nblock is.  Anything that undef's the invocant (ie. $self) is probably broken.\n\na51cdb0c0420ee3bef26bbd1a9aa75e1d464e5b7 and\n2a679c7a3148978a3f58f1c12100383638e744c5 shed some light.  2a679 looks like it\nspecifically holds off on escaping $full_url.  It would be very nice if that\nwas not necessary.  It would be helpful if the bug mentioned in 2a679 could be\nreproduced to see if it still applies or can be dealt with in another way.\n\n\n-- \n185. My name is not a killing word.\n    -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army\n           http://skippyslist.com/list/\n"},{"id":"195710","messageId":"20120725045327.GD3055@burratino","threadId":"31088","inReplyTo":"500F4530.7030207@pobox.com","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-25T04:53:27Z","receivedAt":"2012-07-25T04:53:27Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael G Schwern wrote:\n\n> No, now it's just canonicalizing as early as possible.  Preferably within the\n> object accessor rather than at the point of use.  So in the code below,\n> $full_url is already escaped/canonicalized.\n\nLet's start with this.\n\nIs svn_path_canonicalize() idempotent?  What does it do when it\nencounters a percent-sign?\n\nThanks,\nJonathan\n"},{"id":"195711","messageId":"500F85F7.8050304@pobox.com","threadId":"31088","inReplyTo":"20120725045327.GD3055@burratino","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-25T05:36:55Z","receivedAt":"2012-07-25T05:36:55Z","isPatch":false,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.24 9:53 PM, Jonathan Nieder wrote:\n> Michael G Schwern wrote:\n> \n>> No, now it's just canonicalizing as early as possible.  Preferably within the\n>> object accessor rather than at the point of use.  So in the code below,\n>> $full_url is already escaped/canonicalized.\n> \n> Let's start with this.\n> \n> Is svn_path_canonicalize() idempotent?  What does it do when it\n> encounters a percent-sign?\n\nNothing, because paths are not URI escaped. :)\n\nYou probably meant svn_uri_canonicalize().  And no, it does not double escape,\nso its safe to escape as early as possible.\n\n   use SVN::Core;\n\n   my $uri = \"http://www.example.com/ foo\";\n\n    print SVN::_Core::svn_uri_canonicalize(\n        SVN::_Core::svn_uri_canonicalize($uri)\n    );\n\nThat produces \"http://www.example.com/%20foo\".\n\nThe API docs don't say it specifically, but if it were otherwise it would be\nimpossible to use.  You'd have to check first if anything were escaped before\ncanonicalizing.  And a user couldn't pass in an escaped URL without risking it\nbeing double escaped.\n\nhttp://subversion.apache.org/docs/api/latest/svn__dirent__uri_8h.html#a8bae33a2fbf86857869f7b0e39a553e7\n\n\n-- \n'All anyone gets in a mirror is themselves,' she said. 'But what you\ngets in a good gumbo is everything.'\n    -- \"Witches Abroad\" by Terry Prachett\n"},{"id":"195723","messageId":"7v1uk0e2hy.fsf@alter.siamese.dyndns.org","threadId":"31088","inReplyTo":"500F85F7.8050304@pobox.com","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T07:14:33Z","receivedAt":"2012-07-25T07:14:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael G Schwern <schwern@pobox.com> writes:\n\n> On 2012.7.24 9:53 PM, Jonathan Nieder wrote:\n>> Michael G Schwern wrote:\n>> \n>>> No, now it's just canonicalizing as early as possible.  Preferably within the\n>>> object accessor rather than at the point of use.  So in the code below,\n>>> $full_url is already escaped/canonicalized.\n>> \n>> Let's start with this.\n>> \n>> Is svn_path_canonicalize() idempotent?  What does it do when it\n>> encounters a percent-sign?\n>\n> Nothing, because paths are not URI escaped. :)\n>\n> You probably meant svn_uri_canonicalize().  And no, it does not double escape,\n> so its safe to escape as early as possible.\n\nAre you saying that the function assumes that a local pathname would\nnot have '%' in it, returns its input as-is when it sees one, and if\nthe caller really needs to express a path with '%' in it, it is the\nresponsibility of the caller to escape it?\n\nThat makes it even more confusing....\n\n>    my $uri = \"http://www.example.com/ foo\";\n>\n>     print SVN::_Core::svn_uri_canonicalize(\n>         SVN::_Core::svn_uri_canonicalize($uri)\n>     );\n>\n> That produces \"http://www.example.com/%20foo\".\n\nIn other words, if your DocumentRoot was /var/www and you have a\ndirectory /var/www/per%cent you want to expose to the outside world,\nyou have to say \"http://www.example.com/per%25cent\" yourself and the\n\"canonicalize\" function will be an identity function?\n\nI have this vague suspicion that Jonathan was asking about what your\nGit::SVN::Utils::canonicalize_path() sub does, so all of the above\nmight be moot, though...\n"},{"id":"195734","messageId":"500FC233.8090709@pobox.com","threadId":"31088","inReplyTo":"7v1uk0e2hy.fsf@alter.siamese.dyndns.org","subject":"Re: git-svn SVN 1.7 fix, take 2","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-25T09:53:55Z","receivedAt":"2012-07-25T09:53:55Z","isPatch":false,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.25 12:14 AM, Junio C Hamano wrote:\n>> Nothing, because paths are not URI escaped. :)\n>>\n>> You probably meant svn_uri_canonicalize().  And no, it does not double escape,\n>> so its safe to escape as early as possible.\n> \n> Are you saying that the function assumes that a local pathname would\n                                                        ^^^^^^^^\n\nURI and path canonicalization are done differently and by different functions.\n svn_uri_canonicalize() vs svn_dirent_canonicalize().  Or maybe you're\nreferring to the path portion of the URL?  I don't think that makes a\ndifference for what you're asking, but its important to keep in mind.\n\n\n> not have '%' in it, returns its input as-is when it sees one, and if\n> the caller really needs to express a path with '%' in it, it is the\n> responsibility of the caller to escape it?\n\nIt appears that if the % is followed by hex it assumes its an escape.\nOtherwise it escapes it.  Thus...\n\n   http://www.google.com/per%%nt -> http://www.google.com/per%25%25nt\n   http://www.google.com/per%ant -> http://www.google.com/per%25ant\n   http://www.google.com/per%cent -> http://www.google.com/per%CEnt\n\nWhich makes sense if the idea is to not double escape.\n\n\n> That makes it even more confusing....\n\nStraight out of the RFC.\nhttp://pretty-rfc.herokuapp.com/RFC3986#when-to-percent-encode\n\n    Implementations must not percent-encode or decode the same string more\n    than once, as decoding an already decoded string might lead to\n    misinterpreting a percent data octet as the beginning of a percent-\n    encoding, or vice versa in the case of percent-encoding an already\n    percent-encoded string.\n\nIt makes it far simpler to use.  You can't read the mind of the user, but its\na fair guess that they're not really thinking too deeply about how escaping\nworks.  It makes URI and path canonicalization safer and simpler.  Otherwise\nyou'd need to keep track of whether a thing was already escaped or not!  Just\nbegging for loads of bugs.  (If SVN were using URI and path objects they'd\njust take care of it and none of this would be a problem in the first place).\n\nThis way you have no double escaping concerns.  No need to track if a thing is\nalready canonicalized.  Do it as often and as early as you like.  Making a\ncorner case a little harder is a small price to pay for making the common case\nmuch, much easier.\n\nThis also appears to be what Firefox does.\n\n\n>>    my $uri = \"http://www.example.com/ foo\";\n>>\n>>     print SVN::_Core::svn_uri_canonicalize(\n>>         SVN::_Core::svn_uri_canonicalize($uri)\n>>     );\n>>\n>> That produces \"http://www.example.com/%20foo\".\n> \n> In other words, if your DocumentRoot was /var/www and you have a\n> directory /var/www/per%cent you want to expose to the outside world,\n> you have to say \"http://www.example.com/per%25cent\" yourself and the\n> \"canonicalize\" function will be an identity function?\n\nYes.  It can be made to work better.\n\nThere's a number of places in the code which effectively do this:\n\n    my $full_url = $url . '/' . $path;\n\nAnd I was canonicalizing them like this:\n\n    my $full_url = canonicalize_url($url . '/' . $path);\n\nI'd been pondering whether it would be worthwhile to have a function which\nadded a path to a base URL and canonicalized.  Now I see that yes, it would be\nto deal with this corner case.\n\n    my $full_url = append_path_to_url($url, $path);\n\nThat would properly URI encode any % in $path before appending and then\ncanonicalizing the whole thing as a URI.\n\nI'm pretty sure the code in master doesn't handle this at all.\n\n\n> I have this vague suspicion that Jonathan was asking about what your\n> Git::SVN::Utils::canonicalize_path() sub does, so all of the above\n> might be moot, though...\n\nIts just a pass through to the SVN API.\n\n\n-- \n44. I am not the atheist chaplain.\n    -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army\n           http://skippyslist.com/list/\n"}]}