{"thread":{"id":"31119","subject":"Canonicalize the git-svn path & url accessors","startedAt":"2012-07-28T09:38:25Z","lastAt":"2012-10-23T22:58:12Z","messageCount":39,"participants":["Michael G. Schwern","Jonathan Nieder","Michael G Schwern","Eric Wong","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"195998","messageId":"1343468312-72024-1-git-send-email-schwern@pobox.com","threadId":"31119","inReplyTo":null,"subject":"Canonicalize the git-svn path & url accessors","fromName":"Michael G. Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T09:38:25Z","receivedAt":"2012-07-28T09:38:25Z","isPatch":false,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"This patch turns on canonicalization in the Git::SVN and Git::SVN::Ra\npath and url accessors.\n\nIt also makes the canonicalizers use the SVN API when available.\n\nAll patches pass with SVN 1.6.  Next patch series will fix SVN 1.7.\n\nThis should be placed on top of the previous patch series which added\npath and url accessors.\n"},{"id":"196004","messageId":"1343468312-72024-2-git-send-email-schwern@pobox.com","threadId":"31119","inReplyTo":"1343468312-72024-1-git-send-email-schwern@pobox.com","subject":"[PATCH 1/7] Move the canonicalization functions to Git::SVN::Utils","fromName":"Michael G. Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T09:38:26Z","receivedAt":"2012-07-28T09:38:26Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"From: \"Michael G. Schwern\" <schwern@pobox.com>\n\nSo they can be used by others.\n\nI'd like to test them, but they're going to become SVN API wrappers shortly\nand those aren't predictable.\n\nNo functional change.\n---\n git-svn.perl          | 33 +++++++-------------------------\n perl/Git/SVN/Utils.pm | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 58 insertions(+), 27 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex de1ddd1..a857484 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -29,7 +29,13 @@ use Git::SVN::Prompt;\n use Git::SVN::Log;\n use Git::SVN::Migration;\n \n-use Git::SVN::Utils qw(fatal can_compress);\n+use Git::SVN::Utils qw(\n+\tfatal\n+\tcan_compress\n+\tcanonicalize_path\n+\tcanonicalize_url\n+);\n+\n use Git qw(\n \tgit_cmd_try\n \tcommand\n@@ -1256,31 +1262,6 @@ 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.\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex 3d0bfa4..c842d00 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -5,7 +5,12 @@ use warnings;\n \n use base qw(Exporter);\n \n-our @EXPORT_OK = qw(fatal can_compress);\n+our @EXPORT_OK = qw(\n+\tfatal\n+\tcan_compress\n+\tcanonicalize_path\n+\tcanonicalize_url\n+);\n \n \n =head1 NAME\n@@ -56,4 +61,49 @@ sub can_compress {\n }\n \n \n+=head3 canonicalize_path\n+\n+    my $canoncalized_path = canonicalize_path($path);\n+\n+Converts $path into a canonical form which is safe to pass to the SVN\n+API as a file path.\n+\n+=cut\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+\n+=head3 canonicalize_url\n+\n+    my $canonicalized_url = canonicalize_url($url);\n+\n+Converts $url into a canonical form which is safe to pass to the SVN\n+API as a URL.\n+\n+=cut\n+\n+sub canonicalize_url {\n+\tmy ($url) = @_;\n+\t$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;\n+\treturn $url;\n+}\n+\n+\n 1;\n-- \n1.7.11.3\n"},{"id":"196005","messageId":"1343468312-72024-3-git-send-email-schwern@pobox.com","threadId":"31119","inReplyTo":"1343468312-72024-1-git-send-email-schwern@pobox.com","subject":"[PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Michael G. Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T09:38:27Z","receivedAt":"2012-07-28T09:38:27Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"From: \"Michael G. Schwern\" <schwern@pobox.com>\n\nNo change on SVN 1.6.  The tests all pass with SVN 1.6 if\ncanonicalize_url() does nothing, so tests passing doesn't have\nmuch meaning.\n\nThe tests are so messed up right now with SVN 1.7 it isn't really\nuseful to check.  They will be useful later.\n---\n perl/Git/SVN/Utils.pm | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex c842d00..9d5d3c5 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -3,6 +3,8 @@ package Git::SVN::Utils;\n use strict;\n use warnings;\n \n+use SVN::Core;\n+\n use base qw(Exporter);\n \n our @EXPORT_OK = qw(\n@@ -100,6 +102,20 @@ API as a URL.\n =cut\n \n sub canonicalize_url {\n+\tmy $url = shift;\n+\n+\t# The 1.7 way to do it\n+\tif ( defined &SVN::_Core::svn_uri_canonicalize ) {\n+\t\treturn SVN::_Core::svn_uri_canonicalize($url);\n+\t}\n+\t# There wasn't a 1.6 way to do it, so we do it ourself.\n+\telse {\n+\t\treturn _canonicalize_url_ourselves($url);\n+\t}\n+}\n+\n+\n+sub _canonicalize_url_ourselves {\n \tmy ($url) = @_;\n \t$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;\n \treturn $url;\n-- \n1.7.11.3\n"},{"id":"196001","messageId":"1343468312-72024-4-git-send-email-schwern@pobox.com","threadId":"31119","inReplyTo":"1343468312-72024-1-git-send-email-schwern@pobox.com","subject":"[PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Michael G. Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T09:38:28Z","receivedAt":"2012-07-28T09:38:28Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"From: \"Michael G. Schwern\" <schwern@pobox.com>\n\nThe SVN API functions will not accept ../foo but their canonicalization\nfunctions will not collapse it.  So we'll have to do it ourselves.\n\n_collapse_dotdot() works better than the existing regex did.\n\nThis will be used shortly when canonicalize_path() starts using the\nSVN API.\n---\n perl/Git/SVN/Utils.pm             | 14 +++++++++++++-\n t/Git-SVN/Utils/collapse_dotdot.t | 23 +++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 1 deletion(-)\n create mode 100644 t/Git-SVN/Utils/collapse_dotdot.t\n\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex 9d5d3c5..7314e52 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -72,6 +72,18 @@ API as a file path.\n \n =cut\n \n+# Turn foo/../bar into bar\n+sub _collapse_dotdot {\n+\tmy $path = shift;\n+\n+\t1 while $path =~ s{/[^/]+/+\\.\\.}{};\n+\t1 while $path =~ s{[^/]+/+\\.\\./}{};\n+\t1 while $path =~ s{[^/]+/+\\.\\.}{};\n+\n+\treturn $path;\n+}\n+\n+\n sub canonicalize_path {\n \tmy ($path) = @_;\n \tmy $dot_slash_added = 0;\n@@ -83,7 +95,7 @@ sub canonicalize_path {\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 = _collapse_dotdot($path);\n \t$path =~ s#/$##g;\n \t$path =~ s#^\\./## if $dot_slash_added;\n \t$path =~ s#^/##;\ndiff --git a/t/Git-SVN/Utils/collapse_dotdot.t b/t/Git-SVN/Utils/collapse_dotdot.t\nnew file mode 100644\nindex 0000000..1da1cce\n--- /dev/null\n+++ b/t/Git-SVN/Utils/collapse_dotdot.t\n@@ -0,0 +1,23 @@\n+#!/usr/bin/env perl\n+\n+use strict;\n+use warnings;\n+\n+use Test::More 'no_plan';\n+\n+use Git::SVN::Utils;\n+my $collapse_dotdot = \\&Git::SVN::Utils::_collapse_dotdot;\n+\n+my %tests = (\n+\t\"foo/bar/baz\"\t\t\t=> \"foo/bar/baz\",\n+\t\"..\"\t\t\t\t=> \"..\",\n+\t\"foo/..\"\t\t\t=> \"\",\n+\t\"/foo/bar/../../baz\"\t\t=> \"/baz\",\n+\t\"deeply/.././deeply/nested\"\t=> \"./deeply/nested\",\n+);\n+\n+for my $arg (keys %tests) {\n+\tmy $want = $tests{$arg};\n+\n+\tis $collapse_dotdot->($arg), $want, \"_collapse_dotdot('$arg') => $want\";\n+}\n-- \n1.7.11.3\n"},{"id":"196003","messageId":"1343468312-72024-5-git-send-email-schwern@pobox.com","threadId":"31119","inReplyTo":"1343468312-72024-1-git-send-email-schwern@pobox.com","subject":"[PATCH 4/7] Add join_paths() to safely concatenate paths.","fromName":"Michael G. Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T09:38:29Z","receivedAt":"2012-07-28T09:38:29Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"From: \"Michael G. Schwern\" <schwern@pobox.com>\n\nOtherwise you might wind up with things like...\n\n    my $path1 = undef;\n    my $path2 = 'foo';\n    my $path = $path1 . '/' . $path2;\n\ncreating '/foo'.  Or this...\n\n    my $path1 = 'foo/';\n    my $path2 = 'bar';\n    my $path = $path1 . '/' . $path2;\n\ncreating 'foo//bar'.\n\nCould have used File::Spec, but I'm shying away from it due to SVN\n1.7's pickiness about paths.  Felt it would be better to have our own\nwe can control completely.\n---\n git-svn.perl                 |  3 ++-\n perl/Git/SVN.pm              | 10 ++++++----\n perl/Git/SVN/Utils.pm        | 32 ++++++++++++++++++++++++++++++++\n t/Git-SVN/Utils/join_paths.t | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 72 insertions(+), 5 deletions(-)\n create mode 100644 t/Git-SVN/Utils/join_paths.t\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex a857484..6e3e240 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -34,6 +34,7 @@ use Git::SVN::Utils qw(\n \tcan_compress\n \tcanonicalize_path\n \tcanonicalize_url\n+\tjoin_paths\n );\n \n use Git qw(\n@@ -1275,7 +1276,7 @@ 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+\t$path = join_paths($gs->{path}, $path);\n \n \t# canonicalize the path (otherwise libsvn will abort or fail to\n \t# find the file)\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 7913d8f..b0ed3ea 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -23,7 +23,11 @@ use Git qw(\n     command_output_pipe\n     command_close_pipe\n );\n-use Git::SVN::Utils qw(fatal can_compress);\n+use Git::SVN::Utils qw(\n+\tfatal\n+\tcan_compress\n+\tjoin_paths\n+);\n \n my $can_use_yaml;\n BEGIN {\n@@ -316,9 +320,7 @@ sub init_remote_config {\n \t\t\t}\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$url .= \"/$old_path\";\n-\t\t\t}\n+\t\t\t$url = join_paths($url, $old_path);\n \t\t\t$self->path($url);\n \t\t\t$url = $min_url;\n \t\t}\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex 7314e52..deade07 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -12,6 +12,7 @@ our @EXPORT_OK = qw(\n \tcan_compress\n \tcanonicalize_path\n \tcanonicalize_url\n+\tjoin_paths\n );\n \n \n@@ -134,4 +135,35 @@ sub _canonicalize_url_ourselves {\n }\n \n \n+=head3 join_paths\n+\n+    my $new_path = join_paths(@paths);\n+\n+Appends @paths together into a single path.  Any empty paths are ignored.\n+\n+=cut\n+\n+sub join_paths {\n+\tmy @paths = @_;\n+\n+\t@paths = grep { defined $_ && length $_ } @paths;\n+\n+\treturn '' unless @paths;\n+\treturn $paths[0] if @paths == 1;\n+\n+\tmy $new_path = shift @paths;\n+\t$new_path =~ s{/+$}{};\n+\n+\tmy $last_path = pop @paths;\n+\t$last_path =~ s{^/+}{};\n+\n+\tfor my $path (@paths) {\n+\t\t$path =~ s{^/+}{};\n+\t\t$path =~ s{/+$}{};\n+\t\t$new_path .= \"/$path\";\n+\t}\n+\n+\treturn $new_path .= \"/$last_path\";\n+}\n+\n 1;\ndiff --git a/t/Git-SVN/Utils/join_paths.t b/t/Git-SVN/Utils/join_paths.t\nnew file mode 100644\nindex 0000000..d4488e7\n--- /dev/null\n+++ b/t/Git-SVN/Utils/join_paths.t\n@@ -0,0 +1,32 @@\n+#!/usr/bin/env perl\n+\n+use strict;\n+use warnings;\n+\n+use Test::More 'no_plan';\n+\n+use Git::SVN::Utils qw(\n+\tjoin_paths\n+);\n+\n+# A reference cannot be a hash key, so we use an array.\n+my @tests = (\n+\t[]\t\t\t\t\t=> '',\n+\t[\"/x.com\", \"bar\"]\t\t\t=> '/x.com/bar',\n+\t[\"x.com\", \"\"]\t\t\t\t=> 'x.com',\n+\t[\"/x.com/foo/\", undef, \"bar\"]\t\t=> '/x.com/foo/bar',\n+\t[\"x.com/foo/\", \"/bar/baz/\"]\t\t=> 'x.com/foo/bar/baz/',\n+\t[\"foo\", \"bar\"]\t\t\t\t=> 'foo/bar',\n+\t[\"/foo/bar\", \"baz\", \"/biff\"]\t\t=> '/foo/bar/baz/biff',\n+\t[\"\", undef, \".\"]\t\t\t=> '.',\n+\t[]\t\t\t\t\t=> '',\n+\n+);\n+\n+while(@tests) {\n+\tmy($have, $want) = splice @tests, 0, 2;\n+\n+\tmy $args = join \", \", map { qq['$_'] } map { defined($_) ? $_ : 'undef' } @$have;\n+\tmy $name = \"join_paths($args) eq '$want'\";\n+\tis join_paths(@$have), $want, $name;\n+}\n-- \n1.7.11.3\n"},{"id":"196002","messageId":"1343468312-72024-6-git-send-email-schwern@pobox.com","threadId":"31119","inReplyTo":"1343468312-72024-1-git-send-email-schwern@pobox.com","subject":"[PATCH 5/7] Remove irrelevant comment.","fromName":"Michael G. Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T09:38:30Z","receivedAt":"2012-07-28T09:38:30Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"From: \"Michael G. Schwern\" <schwern@pobox.com>\n\nThe code doesn't use File::Spec.\n---\n perl/Git/SVN/Utils.pm | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex deade07..6c8ae53 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -92,8 +92,6 @@ sub canonicalize_path {\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 = _collapse_dotdot($path);\n-- \n1.7.11.3\n"},{"id":"195999","messageId":"1343468312-72024-7-git-send-email-schwern@pobox.com","threadId":"31119","inReplyTo":"1343468312-72024-1-git-send-email-schwern@pobox.com","subject":"[PATCH 6/7] Switch path canonicalization to use the SVN API.","fromName":"Michael G. Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T09:38:31Z","receivedAt":"2012-07-28T09:38:31Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"From: \"Michael G. Schwern\" <schwern@pobox.com>\n\nAll tests pass with SVN 1.6.  SVN 1.7 remains broken, not worrying\nabout it yet.\n\nSVN changed its path canonicalization API between 1.6 and 1.7.\nhttp://svnbook.red-bean.com/en/1.6/svn.developer.usingapi.html#svn.developer.usingapi.urlpath\nhttp://svnbook.red-bean.com/en/1.7/svn.developer.usingapi.html#svn.developer.usingapi.urlpath\n\nThe SVN API does not accept foo/.. but it also doesn't canonicalize\nit.  We have to do it ourselves.\n---\n perl/Git/SVN/Utils.pm | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex 6c8ae53..7ae6fac 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -86,6 +86,27 @@ sub _collapse_dotdot {\n \n \n sub canonicalize_path {\n+\tmy $path = shift;\n+\n+\t# The 1.7 way to do it\n+\tif ( defined &SVN::_Core::svn_dirent_canonicalize ) {\n+\t\t$path = _collapse_dotdot($path);\n+\t\treturn SVN::_Core::svn_dirent_canonicalize($path);\n+\t}\n+\t# The 1.6 way to do it\n+\telsif ( defined &SVN::_Core::svn_path_canonicalize ) {\n+\t\t$path = _collapse_dotdot($path);\n+\t\treturn SVN::_Core::svn_path_canonicalize($path);\n+\t}\n+\t# No SVN API canonicalization is available, do it ourselves\n+\telse {\n+\t\t$path = _canonicalize_path_ourselves($path);\n+\t\treturn $path;\n+\t}\n+}\n+\n+\n+sub _canonicalize_path_ourselves {\n \tmy ($path) = @_;\n \tmy $dot_slash_added = 0;\n \tif (substr($path, 0, 1) ne \"/\") {\n-- \n1.7.11.3\n"},{"id":"196000","messageId":"1343468312-72024-8-git-send-email-schwern@pobox.com","threadId":"31119","inReplyTo":"1343468312-72024-1-git-send-email-schwern@pobox.com","subject":"[PATCH 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls.","fromName":"Michael G. Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T09:38:32Z","receivedAt":"2012-07-28T09:38:32Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"From: \"Michael G. Schwern\" <schwern@pobox.com>\n\nThis canonicalizes paths and urls as early as possible so we don't\nhave to remember to do it at the point of use.  It will fix a swath\nof SVN 1.7 problems in one go.\n\nIts ok to double canonicalize things.\n\nSVN 1.7 still fails, still not worrying about that.\n---\n perl/Git/SVN.pm    | 6 ++++--\n perl/Git/SVN/Ra.pm | 6 +++++-\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex b0ed3ea..798f6c4 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -27,6 +27,8 @@ use Git::SVN::Utils qw(\n \tfatal\n \tcan_compress\n \tjoin_paths\n+\tcanonicalize_path\n+\tcanonicalize_url\n );\n \n my $can_use_yaml;\n@@ -2305,7 +2307,7 @@ sub path {\n \n     if( @_ ) {\n         my $path = shift;\n-        $self->{path} = $path;\n+        $self->{path} = canonicalize_path($path);\n         return;\n     }\n \n@@ -2318,7 +2320,7 @@ sub url {\n \n     if( @_ ) {\n         my $url = shift;\n-        $self->{url} = $url;\n+        $self->{url} = canonicalize_url($url);\n         return;\n     }\n \ndiff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\nindex 27dcdd5..ef7b3dd 100644\n--- a/perl/Git/SVN/Ra.pm\n+++ b/perl/Git/SVN/Ra.pm\n@@ -3,6 +3,10 @@ use vars qw/@ISA $config_dir $_ignore_refs_regex $_log_window_size/;\n use strict;\n use warnings;\n use SVN::Client;\n+use Git::SVN::Utils qw(\n+\tcanonicalize_url\n+);\n+\n use SVN::Ra;\n BEGIN {\n \t@ISA = qw(SVN::Ra);\n@@ -138,7 +142,7 @@ sub url {\n \n     if( @_ ) {\n         my $url = shift;\n-        $self->{url} = $url;\n+        $self->{url} = canonicalize_url($url);\n         return;\n     }\n \n-- \n1.7.11.3\n"},{"id":"196022","messageId":"20120728135018.GB9715@burratino","threadId":"31119","inReplyTo":"1343468312-72024-3-git-send-email-schwern@pobox.com","subject":"Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-28T13:50:18Z","receivedAt":"2012-07-28T13:50:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael G. Schwern wrote:\n\n> --- a/perl/Git/SVN/Utils.pm\n> +++ b/perl/Git/SVN/Utils.pm\n[...]\n> @@ -100,6 +102,20 @@ API as a URL.\n>  =cut\n>  \n>  sub canonicalize_url {\n> +\tmy $url = shift;\n> +\n> +\t# The 1.7 way to do it\n> +\tif ( defined &SVN::_Core::svn_uri_canonicalize ) {\n> +\t\treturn SVN::_Core::svn_uri_canonicalize($url);\n> +\t}\n> +\t# There wasn't a 1.6 way to do it, so we do it ourself.\n> +\telse {\n> +\t\treturn _canonicalize_url_ourselves($url);\n> +\t}\n> +}\n> +\n> +\n> +sub _canonicalize_url_ourselves {\n>  \tmy ($url) = @_;\n>  \t$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;\n\nLeaves me a bit nervous.\n\nWhat effect should we expect this change to have?  Is our emulation\nof svn_uri_canonicalize already perfect and this change just a little\nfutureproofing in case svn_uri_canonicalize gets even better, or is\nthis a trap waiting to happen when new callers of canonicalize_url\nstart relying on, e.g., %-encoding of special characters?\n\nIf I am reading Subversion r873487 correctly, in ancient times,\nsvn_path_canonicalize() did the appropriate tweaking for URIs.  Today\nits implementation is comforting:\n\n\tconst char *\n\tsvn_path_canonicalize(const char *path, apr_pool_t *pool)\n\t{\n\t  if (svn_path_is_url(path))\n\t    return svn_uri_canonicalize(path, pool);\n\t  else\n\t    return svn_dirent_canonicalize(path, pool);\n\t}\n\nIt might be easier to rely on that on pre-1.7 systems.\n\nThanks,\nJonathan\n"},{"id":"196023","messageId":"20120728135502.GC9715@burratino","threadId":"31119","inReplyTo":"1343468312-72024-7-git-send-email-schwern@pobox.com","subject":"Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-28T13:55:02Z","receivedAt":"2012-07-28T13:55:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael G. Schwern wrote:\n\n> --- a/perl/Git/SVN/Utils.pm\n> +++ b/perl/Git/SVN/Utils.pm\n> @@ -86,6 +86,27 @@ sub _collapse_dotdot {\n>  \n>  \n>  sub canonicalize_path {\n> +\tmy $path = shift;\n> +\n> +\t# The 1.7 way to do it\n> +\tif ( defined &SVN::_Core::svn_dirent_canonicalize ) {\n> +\t\t$path = _collapse_dotdot($path);\n> +\t\treturn SVN::_Core::svn_dirent_canonicalize($path);\n> +\t}\n> +\t# The 1.6 way to do it\n> +\telsif ( defined &SVN::_Core::svn_path_canonicalize ) {\n> +\t\t$path = _collapse_dotdot($path);\n> +\t\treturn SVN::_Core::svn_path_canonicalize($path);\n> +\t}\n> +\t# No SVN API canonicalization is available, do it ourselves\n> +\telse {\n\nWhen would this \"else\" case trip?  Would it be safe to make it\nreturn an error message, or even to do something like the following?\n\n\tsub canonicalize_path {\n\t\tmy $path = shift;\n\t\t$path = _collapse_dotdot($path);\n\n\t\t# Subversion 1.7 split svn_path_canonicalize() into\n\t\t# svn_dirent_canonicalize() and svn_uri_canonicalize().\n\t\tif (!defined &SVN::_Core::svn_dirent_canonicalize) {\n\t\t\treturn SVN::_Core::svn_path_canonicalize($path);\n\t\t}\n\n\t\treturn SVN::_Core::svn_dirent_canonicalize($path);\n\t}\n\nThanks,\nJonathan\n"},{"id":"196024","messageId":"20120728141126.GD9715@burratino","threadId":"31119","inReplyTo":"1343468312-72024-8-git-send-email-schwern@pobox.com","subject":"Re: [PATCH 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-28T14:11:26Z","receivedAt":"2012-07-28T14:11:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael G. Schwern wrote:\n\n> This canonicalizes paths and urls as early as possible so we don't\n> have to remember to do it at the point of use.\n\nYay!  Am I correct in imagining this makes the following sequence of\ncommands[1] no longer trip an assertion failure in svn_path_join[2]\nwith SVN 1.6?\n\n\tgit svn init -Thttp://trac-hacks.org/svn/tagsplugin/trunk \\\n\t\t-thttp://trac-hacks.org/svn/tagsplugin/tags \\\n\t\t-bhttp://trac-hacks.org/svn/tagsplugin/branches\n\tgit svn fetch\n\n[1] http://bugs.debian.org/616168\n[2] \n  $ git svn fetch\n  W: Ignoring error from SVN, path probably does not exist: (160013): Filesystem has no item: File not found: revision 100, path '/tagsplugin'\n  W: Do not be alarmed at the above message git-svn is just searching aggressively for old history.\n  This may take a while on large repositories\n  perl: /build/buildd-subversion_1.6.17dfsg-4-i386-MgNPeW/subversion-1.6.17dfsg/subversion/libsvn_subr/path.c:115: svn_path_join: Assertion `svn_path_is_canonical(component, pool)' failed.\n  error: git-svn died of signal 6\n"},{"id":"196041","messageId":"50143700.80900@pobox.com","threadId":"31119","inReplyTo":"20120728135018.GB9715@burratino","subject":"Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T19:01:20Z","receivedAt":"2012-07-28T19:01:20Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.28 6:50 AM, Jonathan Nieder wrote:\n>> --- a/perl/Git/SVN/Utils.pm\n>> +++ b/perl/Git/SVN/Utils.pm\n> [...]\n>> @@ -100,6 +102,20 @@ API as a URL.\n>>  =cut\n>>  \n>>  sub canonicalize_url {\n>> +\tmy $url = shift;\n>> +\n>> +\t# The 1.7 way to do it\n>> +\tif ( defined &SVN::_Core::svn_uri_canonicalize ) {\n>> +\t\treturn SVN::_Core::svn_uri_canonicalize($url);\n>> +\t}\n>> +\t# There wasn't a 1.6 way to do it, so we do it ourself.\n>> +\telse {\n>> +\t\treturn _canonicalize_url_ourselves($url);\n>> +\t}\n>> +}\n>> +\n>> +\n>> +sub _canonicalize_url_ourselves {\n>>  \tmy ($url) = @_;\n>>  \t$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;\n> \n> Leaves me a bit nervous.\n\nAs it should, SVN dumped a mess on us.\n\n\n> What effect should we expect this change to have?  Is our emulation\n> of svn_uri_canonicalize already perfect and this change just a little\n> futureproofing in case svn_uri_canonicalize gets even better, or is\n> this a trap waiting to happen when new callers of canonicalize_url\n> start relying on, e.g., %-encoding of special characters?\n\nThis change is *just* about sliding in the SVN API call and seeing if git-svn\nstill works with SVN 1.6.  It should have no effect on SVN 1.6.  These patches\nare a very slow and careful refactoring doing just one thing at a time.  Every\ntime I tried to do too many things at once, tests broke and I had to tease the\npatch apart.\n\nAt this point in the patch series the code is not ready for canonicalization.\n Until 3/8 in the next patch series, canonicalize_url() basically does nothing\non SVN 1.6 so the code has never had to deal with the problem.  3/8 deals with\nimproving _canonicalize_url_ourselves() to work more like\nsvn_uri_canonicalize() and thus \"turns on\" canonicalization for SVN 1.6 and\ndeals with the breakage.\n\n\n> If I am reading Subversion r873487 correctly, in ancient times,\n> svn_path_canonicalize() did the appropriate tweaking for URIs.  Today\n> its implementation is comforting:\n> \n> \tconst char *\n> \tsvn_path_canonicalize(const char *path, apr_pool_t *pool)\n> \t{\n> \t  if (svn_path_is_url(path))\n> \t    return svn_uri_canonicalize(path, pool);\n> \t  else\n> \t    return svn_dirent_canonicalize(path, pool);\n> \t}\n> \n> It might be easier to rely on that on pre-1.7 systems.\n\nI didn't know about that.  I don't know what your SVN backwards compat\nrequirements are, but if that behavior goes back far enough in SVN to satisfy\nyou folks, then canonicalize_url() should fall back to\nSVN::_Core::svn_path_canonicalize().  But try it at the end of the patch\nseries.  The code has to be prepared for canonicalization first.  Then how it\nactually does it can be improved.\n\n\n-- \nDefender of Lexical Encapsulation\n"},{"id":"196042","messageId":"5014387C.50903@pobox.com","threadId":"31119","inReplyTo":"20120728135502.GC9715@burratino","subject":"Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T19:07:40Z","receivedAt":"2012-07-28T19:07:40Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.28 6:55 AM, Jonathan Nieder wrote:\n> Michael G. Schwern wrote:\n>> --- a/perl/Git/SVN/Utils.pm\n>> +++ b/perl/Git/SVN/Utils.pm\n>> @@ -86,6 +86,27 @@ sub _collapse_dotdot {\n>>  \n>>  \n>>  sub canonicalize_path {\n>> +\tmy $path = shift;\n>> +\n>> +\t# The 1.7 way to do it\n>> +\tif ( defined &SVN::_Core::svn_dirent_canonicalize ) {\n>> +\t\t$path = _collapse_dotdot($path);\n>> +\t\treturn SVN::_Core::svn_dirent_canonicalize($path);\n>> +\t}\n>> +\t# The 1.6 way to do it\n>> +\telsif ( defined &SVN::_Core::svn_path_canonicalize ) {\n>> +\t\t$path = _collapse_dotdot($path);\n>> +\t\treturn SVN::_Core::svn_path_canonicalize($path);\n>> +\t}\n>> +\t# No SVN API canonicalization is available, do it ourselves\n>> +\telse {\n> \n> When would this \"else\" case trip?\n\nWhen svn_path_canonicalize() does not exist in the SVN API, presumably because\ntheir SVN is too old.\n\n\n> Would it be safe to make it\n> return an error message, or even to do something like the following?\n\nI don't know what your SVN backwards compat requirements are, or when\nsvn_path_canonicalize() appears in the API, so I left it as is.  git-svn's\nhome rolled path canonicalization worked and its no work to leave it working.\n No reason to break it IMO.\n\n\n> \tsub canonicalize_path {\n> \t\tmy $path = shift;\n> \t\t$path = _collapse_dotdot($path);\n> \n> \t\t# Subversion 1.7 split svn_path_canonicalize() into\n> \t\t# svn_dirent_canonicalize() and svn_uri_canonicalize().\n> \t\tif (!defined &SVN::_Core::svn_dirent_canonicalize) {\n> \t\t\treturn SVN::_Core::svn_path_canonicalize($path);\n> \t\t}\n> \n> \t\treturn SVN::_Core::svn_dirent_canonicalize($path);\n> \t}\n\nAs a side note...\n\"If they don't have Mars bar, get me a Twix.  Else get me a Mars bar.\"\n\"If they have a Mars bar, get me one.  Else get me a Twix.\"\n\n\n-- \nLook at me talking when there's science to do.\nWhen I look out there it makes me glad I'm not you.\nI've experiments to be run.\nThere is research to be done\nOn the people who are still alive.\n    -- Jonathan Coulton, \"Still Alive\"\n"},{"id":"196043","messageId":"50143A58.5050600@pobox.com","threadId":"31119","inReplyTo":"20120728141126.GD9715@burratino","subject":"Re: [PATCH 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls.","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T19:15:36Z","receivedAt":"2012-07-28T19:15:36Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.28 7:11 AM, Jonathan Nieder wrote:\n> Yay!  Am I correct in imagining this makes the following sequence of\n> commands[1] no longer trip an assertion failure in svn_path_join[2]\n> with SVN 1.6?\n> \n> \tgit svn init -Thttp://trac-hacks.org/svn/tagsplugin/trunk \\\n> \t\t-thttp://trac-hacks.org/svn/tagsplugin/tags \\\n> \t\t-bhttp://trac-hacks.org/svn/tagsplugin/branches\n> \tgit svn fetch\n\nWorks For Me™!\n\n  ...\n  Checked out HEAD:\n    http://trac-hacks.org/svn/tagsplugin/trunk r11776\n\n\n-- \nInsulting our readers is part of our business model.\n        http://somethingpositive.net/sp07122005.shtml\n"},{"id":"196044","messageId":"20120728193029.GB3107@burratino","threadId":"31119","inReplyTo":"50143700.80900@pobox.com","subject":"Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-28T19:30:29Z","receivedAt":"2012-07-28T19:30:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael G Schwern wrote:\n> On 2012.7.28 6:50 AM, Jonathan Nieder wrote:\n\n>> If I am reading Subversion r873487 correctly, in ancient times,\n>> svn_path_canonicalize() did the appropriate tweaking for URIs.  Today\n>> its implementation is comforting:\n>> \n>> \tconst char *\n>> \tsvn_path_canonicalize(const char *path, apr_pool_t *pool)\n>> \t{\n>> \t  if (svn_path_is_url(path))\n>> \t    return svn_uri_canonicalize(path, pool);\n>> \t  else\n>> \t    return svn_dirent_canonicalize(path, pool);\n>> \t}\n[...]\n> I didn't know about that.  I don't know what your SVN backwards compat\n> requirements are, but if that behavior goes back far enough in SVN to satisfy\n> you folks, then canonicalize_url() should fall back to\n> SVN::_Core::svn_path_canonicalize().\n\nsvn_path_canonicalize() has been usable for this kind of thing since\nSVN 1.1, possibly earlier.\n\n>                                      But try it at the end of the patch\n> series.  The code has to be prepared for canonicalization first.  Then how it\n> actually does it can be improved.\n\nSince this part of the series is not tested with SVN 1.7, this is\nbasically adding dead code, right?  That could be avoided by\nreordering the changes to keep \"canonicalize_url\" as-is until later in\nthe series when the switchover is safe.\n\nThanks.  Will play around with this code more.\n\nJonathan\n"},{"id":"196046","messageId":"501442D5.6080207@pobox.com","threadId":"31119","inReplyTo":"20120728193029.GB3107@burratino","subject":"Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T19:51:49Z","receivedAt":"2012-07-28T19:51:49Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.28 12:30 PM, Jonathan Nieder wrote:\n>> I didn't know about that.  I don't know what your SVN backwards compat\n>> requirements are, but if that behavior goes back far enough in SVN to satisfy\n>> you folks, then canonicalize_url() should fall back to\n>> SVN::_Core::svn_path_canonicalize().\n> \n> svn_path_canonicalize() has been usable for this kind of thing since\n> SVN 1.1, possibly earlier.\n\nGreat!  Then _canonicalize_url_ourselves() can probably be replaced with that.\n Just take my advice and do it after 1.7 is working and the code is ready for\ncanonicalization.\n\n\n>>                                      But try it at the end of the patch\n>> series.  The code has to be prepared for canonicalization first.  Then how it\n>> actually does it can be improved.\n> \n> Since this part of the series is not tested with SVN 1.7, this is\n> basically adding dead code, right?  That could be avoided by\n> reordering the changes to keep \"canonicalize_url\" as-is until later in\n> the series when the switchover is safe.\n\nI would suggest that worrying whether a few lines of code are introduced now\nor 10 patches later in the same branch which is all going to be merged in one\ngo (and retesting the patches after it) is not the most important thing.  The\ncode needs humans looking over it and deciding if canonicalizations were\nmissed or applied inappropriately.  Or hey, work on that path and url object\nidea that makes a lot of real code mess go away.\n\n\n-- \nROCKS FALL! EVERYONE DIES!\n\thttp://www.somethingpositive.net/sp05032002.shtml\n"},{"id":"196047","messageId":"20120728195733.GC3107@burratino","threadId":"31119","inReplyTo":"501442D5.6080207@pobox.com","subject":"Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-28T19:57:33Z","receivedAt":"2012-07-28T19:57:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael G Schwern wrote:\n> On 2012.7.28 12:30 PM, Jonathan Nieder wrote:\n\n>> Since this part of the series is not tested with SVN 1.7, this is\n>> basically adding dead code, right?  That could be avoided by\n>> reordering the changes to keep \"canonicalize_url\" as-is until later in\n>> the series when the switchover is safe.\n>\n> I would suggest that worrying whether a few lines of code are introduced now\n> or 10 patches later in the same branch which is all going to be merged in one\n> go (and retesting the patches after it) is not the most important thing.  The\n> code needs humans looking over it and deciding if canonicalizations were\n> missed or applied inappropriately.  Or hey, work on that path and url object\n> idea that makes a lot of real code mess go away.\n\nIn that case they should be one patch, I'd think.\n\nThe advantage of introducing changes gradually is that (1) the changes\ncan be examined and tested one at a time, and (2) if later a change\nproves to be problematic, it can be isolated, understood, and fixed\nmore easily.  The strategy you are suggesting would have neither of\nthose advantages.\n\nJonathan\n"},{"id":"196048","messageId":"20120728200047.GA4188@burratino","threadId":"31119","inReplyTo":"20120728195733.GC3107@burratino","subject":"Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-28T20:02:06Z","receivedAt":"2012-07-28T20:02:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Michael G Schwern wrote:\n\n>> I would suggest that worrying whether a few lines of code are introduced now\n>> or 10 patches later in the same branch which is all going to be merged in one\n>> go (and retesting the patches after it) is not the most important thing.\n[...]\n> In that case they should be one patch, I'd think.\n>\n> The advantage of introducing changes gradually is that (1) the changes\n> can be examined and tested one at a time, and (2) if later a change\n> proves to be problematic, it can be isolated, understood, and fixed\n> more easily.  The strategy you are suggesting would have neither of\n> those advantages.\n\n(To avoid confusion: by \"The strategy you are suggesting\" I mean\nintroducing dead code first and activating it later, not the path and\nurl object idea.  The path and url object approach would be very\nnice. :))\n\nSorry for the lack of clarity.\nJonathan\n"},{"id":"196049","messageId":"50144A8A.3040307@pobox.com","threadId":"31119","inReplyTo":"20120728200047.GA4188@burratino","subject":"Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-28T20:24:42Z","receivedAt":"2012-07-28T20:24:42Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.28 1:02 PM, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n>> Michael G Schwern wrote:\n> \n>>> I would suggest that worrying whether a few lines of code are introduced now\n>>> or 10 patches later in the same branch which is all going to be merged in one\n>>> go (and retesting the patches after it) is not the most important thing.\n> [...]\n>> In that case they should be one patch, I'd think.\n>>\n>> The advantage of introducing changes gradually is that (1) the changes\n>> can be examined and tested one at a time, and (2) if later a change\n>> proves to be problematic, it can be isolated, understood, and fixed\n>> more easily.  The strategy you are suggesting would have neither of\n>> those advantages.\n> \n> (To avoid confusion: by \"The strategy you are suggesting\" I mean\n> introducing dead code first and activating it later, not the path and\n> url object idea.  The path and url object approach would be very\n> nice. :))\n\nIf this is all a topic branch then it doesn't matter much whether a couple\nlines of code is introduced at patch 8 of a branch or patch 13.  Sure, it\nmatters a little, but...\nhttps://secure.wikimedia.org/wikipedia/en/wiki/Opportunity_cost\n\nIf it *isn't* going in a topic branch, if its not visible as a collected work\nin history, if its going to be rebased on top of master, then yeah I can see\nwhy you're so concerned.\n\n\n-- \nAlligator sandwich, and make it snappy!\n"},{"id":"196187","messageId":"20120730195108.GA20137@dcvr.yhbt.net","threadId":"31119","inReplyTo":"1343468312-72024-4-git-send-email-schwern@pobox.com","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-07-30T19:51:08Z","receivedAt":"2012-07-30T19:51:08Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"\"Michael G. Schwern\" <schwern@pobox.com> wrote:\n> From: \"Michael G. Schwern\" <schwern@pobox.com>\n> \n> The SVN API functions will not accept ../foo but their canonicalization\n> functions will not collapse it.  So we'll have to do it ourselves.\n> \n> _collapse_dotdot() works better than the existing regex did.\n\nI don't dispute it's better, but it's worth explaining in the commit\nmessage to reviewers why something is \"better\".\n\n> This will be used shortly when canonicalize_path() starts using the\n> SVN API.\n> ---\n\n> +# Turn foo/../bar into bar\n> +sub _collapse_dotdot {\n> +\tmy $path = shift;\n> +\n> +\t1 while $path =~ s{/[^/]+/+\\.\\.}{};\n> +\t1 while $path =~ s{[^/]+/+\\.\\./}{};\n> +\t1 while $path =~ s{[^/]+/+\\.\\.}{};\n\nThis is a bug that's gone unnoticed[1] for over 5 years now,\nbut I've just noticed this doesn' handle \"foo/..bar\"  or \"foo/...bar\"\ncases correctly.\n\n>  sub canonicalize_path {\n>  \tmy ($path) = @_;\n>  \tmy $dot_slash_added = 0;\n> @@ -83,7 +95,7 @@ sub canonicalize_path {\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 = _collapse_dotdot($path);\n\n[1] - I doubt anybody uses paths like these, though...\n"},{"id":"196188","messageId":"20120730200444.GB20137@dcvr.yhbt.net","threadId":"31119","inReplyTo":"5014387C.50903@pobox.com","subject":"Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-07-30T20:04:44Z","receivedAt":"2012-07-30T20:04:44Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Michael G Schwern <schwern@pobox.com> wrote:\n> On 2012.7.28 6:55 AM, Jonathan Nieder wrote:\n> > Michael G. Schwern wrote:\n> >> --- a/perl/Git/SVN/Utils.pm\n> >> +++ b/perl/Git/SVN/Utils.pm\n> >> @@ -86,6 +86,27 @@ sub _collapse_dotdot {\n> >>  \n> >>  \n> >>  sub canonicalize_path {\n> >> +\tmy $path = shift;\n> >> +\n> >> +\t# The 1.7 way to do it\n> >> +\tif ( defined &SVN::_Core::svn_dirent_canonicalize ) {\n> >> +\t\t$path = _collapse_dotdot($path);\n> >> +\t\treturn SVN::_Core::svn_dirent_canonicalize($path);\n> >> +\t}\n> >> +\t# The 1.6 way to do it\n> >> +\telsif ( defined &SVN::_Core::svn_path_canonicalize ) {\n> >> +\t\t$path = _collapse_dotdot($path);\n> >> +\t\treturn SVN::_Core::svn_path_canonicalize($path);\n> >> +\t}\n> >> +\t# No SVN API canonicalization is available, do it ourselves\n> >> +\telse {\n> > \n> > When would this \"else\" case trip?\n> \n> When svn_path_canonicalize() does not exist in the SVN API, presumably because\n> their SVN is too old.\n> \n> \n> > Would it be safe to make it\n> > return an error message, or even to do something like the following?\n> \n> I don't know what your SVN backwards compat requirements are, or when\n> svn_path_canonicalize() appears in the API, so I left it as is.  git-svn's\n> home rolled path canonicalization worked and its no work to leave it working.\n>  No reason to break it IMO.\n\nI agree there's no reason to break something on older SVN.\n\ngit-svn should work with whatever SVN is in CentOS 5.x and similar\ndistros (SVN 1.4.2).  As long as an active \"long-term\" distro supports\na version of SVN, I think we should support that if it's not too\ndifficult.\n"},{"id":"196194","messageId":"5016F2A5.1090102@pobox.com","threadId":"31119","inReplyTo":"20120730195108.GA20137@dcvr.yhbt.net","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-07-30T20:46:29Z","receivedAt":"2012-07-30T20:46:29Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.7.30 12:51 PM, Eric Wong wrote:\n>> The SVN API functions will not accept ../foo but their canonicalization\n>> functions will not collapse it.  So we'll have to do it ourselves.\n>>\n>> _collapse_dotdot() works better than the existing regex did.\n> \n> I don't dispute it's better, but it's worth explaining in the commit\n> message to reviewers why something is \"better\".\n\nYeah.  I figured the tests covered that.\n\n\n>> +# Turn foo/../bar into bar\n>> +sub _collapse_dotdot {\n>> +\tmy $path = shift;\n>> +\n>> +\t1 while $path =~ s{/[^/]+/+\\.\\.}{};\n>> +\t1 while $path =~ s{[^/]+/+\\.\\./}{};\n>> +\t1 while $path =~ s{[^/]+/+\\.\\.}{};\n> \n> This is a bug that's gone unnoticed[1] for over 5 years now,\n> but I've just noticed this doesn' handle \"foo/..bar\"  or \"foo/...bar\"\n> cases correctly.\n\nGood catch.  Woo unit tests!  :)  You could add them as TODO tests.\n\nA more accurate way to do it would be to split the path, collapse using the\nresulting list, and rejoin it.\n\n\n> [1] - I doubt anybody uses paths like these, though...\n\nNot for an svnroot or branch name, no.\n\n\n-- \nHating the web since 1994.\n"},{"id":"196377","messageId":"20120802215141.GA5284@dcvr.yhbt.net","threadId":"31119","inReplyTo":"20120730200444.GB20137@dcvr.yhbt.net","subject":"Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-08-02T21:51:41Z","receivedAt":"2012-08-02T21:51:41Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Wong <normalperson@yhbt.net> wrote:\n> Michael G Schwern <schwern@pobox.com> wrote:\n> > On 2012.7.28 6:55 AM, Jonathan Nieder wrote:\n> > > Michael G. Schwern wrote:\n> > >> --- a/perl/Git/SVN/Utils.pm\n> > >> +++ b/perl/Git/SVN/Utils.pm\n> > >> @@ -86,6 +86,27 @@ sub _collapse_dotdot {\n> > >>  \n> > >>  \n> > >>  sub canonicalize_path {\n> > >> +\tmy $path = shift;\n> > >> +\n> > >> +\t# The 1.7 way to do it\n> > >> +\tif ( defined &SVN::_Core::svn_dirent_canonicalize ) {\n> > >> +\t\t$path = _collapse_dotdot($path);\n> > >> +\t\treturn SVN::_Core::svn_dirent_canonicalize($path);\n> > >> +\t}\n> > >> +\t# The 1.6 way to do it\n> > >> +\telsif ( defined &SVN::_Core::svn_path_canonicalize ) {\n> > >> +\t\t$path = _collapse_dotdot($path);\n> > >> +\t\treturn SVN::_Core::svn_path_canonicalize($path);\n> > >> +\t}\n> > >> +\t# No SVN API canonicalization is available, do it ourselves\n> > >> +\telse {\n> > > \n> > > When would this \"else\" case trip?\n> > \n> > When svn_path_canonicalize() does not exist in the SVN API, presumably because\n> > their SVN is too old.\n\nsvn_path_canonicalize() may be accessible in some versions of SVN,\nbut it'll return undef.\n\nI'm squashing the change below to have it fall back to\n_canonicalize_path_ourselves in the case svn_path_canonicalize()\nis present but unusable.\n\n> > > Would it be safe to make it\n> > > return an error message, or even to do something like the following?\n> > \n> > I don't know what your SVN backwards compat requirements are, or when\n> > svn_path_canonicalize() appears in the API, so I left it as is.  git-svn's\n> > home rolled path canonicalization worked and its no work to leave it working.\n> >  No reason to break it IMO.\n> \n> I agree there's no reason to break something on older SVN.\n> \n> git-svn should work with whatever SVN is in CentOS 5.x and similar\n> distros (SVN 1.4.2).  As long as an active \"long-term\" distro supports\n> a version of SVN, I think we should support that if it's not too\n> difficult.\n\nI've tested the following on an old CentOS 5.2 chroot with SVN 1.4.2:\n\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex b7727db..4bb4dde 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -88,22 +88,25 @@ sub _collapse_dotdot {\n \n sub canonicalize_path {\n \tmy $path = shift;\n+\tmy $rv;\n \n \t# The 1.7 way to do it\n \tif ( defined &SVN::_Core::svn_dirent_canonicalize ) {\n \t\t$path = _collapse_dotdot($path);\n-\t\treturn SVN::_Core::svn_dirent_canonicalize($path);\n+\t\t$rv = SVN::_Core::svn_dirent_canonicalize($path);\n \t}\n \t# The 1.6 way to do it\n+\t# This can return undef on subversion-perl-1.4.2-2.el5 (CentOS 5.2)\n \telsif ( defined &SVN::_Core::svn_path_canonicalize ) {\n \t\t$path = _collapse_dotdot($path);\n-\t\treturn SVN::_Core::svn_path_canonicalize($path);\n-\t}\n-\t# No SVN API canonicalization is available, do it ourselves\n-\telse {\n-\t\t$path = _canonicalize_path_ourselves($path);\n-\t\treturn $path;\n+\t\t$rv = SVN::_Core::svn_path_canonicalize($path);\n \t}\n+\n+\treturn $rv if defined $rv;\n+\n+\t# No SVN API canonicalization is available, or the SVN API\n+\t# didn't return a successful result, do it ourselves\n+\treturn _canonicalize_path_ourselves($path);\n }\n \n \n-- \nEric Wong\n"},{"id":"196393","messageId":"501B0AB8.7060401@pobox.com","threadId":"31119","inReplyTo":"20120802215141.GA5284@dcvr.yhbt.net","subject":"Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.","fromName":"Michael G Schwern","fromEmail":"schwern@pobox.com","sentAt":"2012-08-02T23:18:16Z","receivedAt":"2012-08-02T23:18:16Z","isPatch":true,"sender":{"key":"schwern@pobox.com","avatar":"https://avatars.githubusercontent.com/u/25888?v=4"},"body":"On 2012.8.2 2:51 PM, Eric Wong wrote:\n> svn_path_canonicalize() may be accessible in some versions of SVN,\n> but it'll return undef.\n\nYuck!  Good catch!\n\n\n> I've tested the following on an old CentOS 5.2 chroot with SVN 1.4.2:\n\nLooks good to me.\n\n\n-- \nAlligator sandwich, and make it snappy!\n"},{"id":"199972","messageId":"20120926194504.GA5013@elie.Belkin","threadId":"31119","inReplyTo":"5016F2A5.1090102@pobox.com","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-09-26T19:45:05Z","receivedAt":"2012-09-26T19:45:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael G Schwern wrote:\n> On 2012.7.30 12:51 PM, Eric Wong wrote:\n>> Michael G Schwern wrote:\n\n>>> _collapse_dotdot() works better than the existing regex did.\n>>\n>> I don't dispute it's better, but it's worth explaining in the commit\n>> message to reviewers why something is \"better\".\n>\n> Yeah.  I figured the tests covered that.\n\nNow I'm tripping up on the same thing.  Eric, did you ever find out\nwhat the motivation for this patch was?  Is SVN 1.7 more persnickety\nabout runs of multiple slashes in a row or something, or is it more\nof an aesthetic thing?\n\nPuzzled,\nJonathan\n"},{"id":"199975","messageId":"20120926205122.GA29906@elie.Belkin","threadId":"31119","inReplyTo":"1343468312-72024-5-git-send-email-schwern@pobox.com","subject":"Re: [PATCH 4/7] Add join_paths() to safely concatenate paths.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-09-26T20:51:22Z","receivedAt":"2012-09-26T20:51:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael G. Schwern wrote:\n\n> Otherwise you might wind up with things like...\n>\n>     my $path1 = undef;\n>     my $path2 = 'foo';\n>     my $path = $path1 . '/' . $path2;\n>\n> creating '/foo'.  Or this...\n>\n>     my $path1 = 'foo/';\n>     my $path2 = 'bar';\n>     my $path = $path1 . '/' . $path2;\n>\n> creating 'foo//bar'.\n\nI'm still puzzled by this one, too.  I don't understand the\nmotivation.  Is this to make joining paths less fragile, by preserving\nthe property that join_paths($a, $b) names the directory you would get\nto by first chdir-ing into $a and then into $b?\n\nIt would be easier to understand as two patches: first, one that\nextracts join_paths without any functional change, and then one that\nchanges its implementation with an explanation for what positive\nfunctional effect that would have.\n\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n[...]\n> @@ -1275,7 +1276,7 @@ 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> +\t$path = join_paths($gs->{path}, $path);\n>  \n>  \t# canonicalize the path (otherwise libsvn will abort or fail to\n>  \t# find the file)\n\nThis can't be for the //-collapsing effect since the path is about\nto be canonicalized.  It can't be for the initial-/ effect since\nthat is stripped away by canonicalization, too.\n\nSo no functional effect here, good or bad.\n\n[...]\n> --- a/perl/Git/SVN.pm\n> +++ b/perl/Git/SVN.pm\n[...]\n> @@ -316,9 +320,7 @@ sub init_remote_config {\n>  \t\t\t}\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$url .= \"/$old_path\";\n> -\t\t\t}\n> +\t\t\t$url = join_paths($url, $old_path);\n>  \t\t\t$self->path($url);\n\nThis is probably not for the normal //-collapsing effect because\n$url already has its trailing / stripped off.  Maybe it is for\ncases where $old_path has leading slashes or $min_url has multiple\ntrailing ones?\n\nIn the end it shouldn't make a difference, once a later patch teaches\nGit::SVN->path to canonicalize.\n\nIs the functional change in this patch for aesthetic reasons, or is\nthere some other component (perhaps in a later patch) that relies on\nit?\n\nThanks again for your help,\nJonathan\n"},{"id":"199977","messageId":"20120926205851.GA2166@dcvr.yhbt.net","threadId":"31119","inReplyTo":"20120926194504.GA5013@elie.Belkin","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-09-26T20:58:51Z","receivedAt":"2012-09-26T20:58:51Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Hi,\n> \n> Michael G Schwern wrote:\n> > On 2012.7.30 12:51 PM, Eric Wong wrote:\n> >> Michael G Schwern wrote:\n> \n> >>> _collapse_dotdot() works better than the existing regex did.\n> >>\n> >> I don't dispute it's better, but it's worth explaining in the commit\n> >> message to reviewers why something is \"better\".\n> >\n> > Yeah.  I figured the tests covered that.\n> \n> Now I'm tripping up on the same thing.  Eric, did you ever find out\n> what the motivation for this patch was?  Is SVN 1.7 more persnickety\n> about runs of multiple slashes in a row or something, or is it more\n> of an aesthetic thing?\n\nI'm not sure about this case specifically, but SVN has (and will likely\nbecome) more persnickety over time.  I haven't had a chance to check SVN\nitself, but I think being defensive and giving it prettier paths will\nbe safer in the future.\n\nThat said, I'd favor an implementation that split on m{/+} and\ncollapsed as Michael mentioned.\n"},{"id":"199978","messageId":"20120926213831.GB30131@elie.Belkin","threadId":"31119","inReplyTo":"20120926205851.GA2166@dcvr.yhbt.net","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-09-26T21:38:32Z","receivedAt":"2012-09-26T21:38:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Eric Wong wrote:\n\n> That said, I'd favor an implementation that split on m{/+} and\n> collapsed as Michael mentioned.\n\nSounds sensible.  Is canonicalize_path responsible for collapsing\nruns of slashes?  What should _collapse_dotdot do to\n\"c:/..\" or \"http://www.example.com/..\"?\n"},{"id":"199983","messageId":"20120926215429.GA4637@dcvr.yhbt.net","threadId":"31119","inReplyTo":"20120926213831.GB30131@elie.Belkin","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-09-26T21:54:29Z","receivedAt":"2012-09-26T21:54:29Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Eric Wong wrote:\n> > That said, I'd favor an implementation that split on m{/+} and\n> > collapsed as Michael mentioned.\n> \n> Sounds sensible.  Is canonicalize_path responsible for collapsing\n> runs of slashes?  What should _collapse_dotdot do to\n> \"c:/..\" or \"http://www.example.com/..\"?\n\nIt should probably just return the root path (\"c:/\" and\n\"http://www.example.com/\" respectively).\n"},{"id":"199985","messageId":"20120926224307.GA31456@elie.Belkin","threadId":"31119","inReplyTo":"20120926215429.GA4637@dcvr.yhbt.net","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-09-26T22:43:07Z","receivedAt":"2012-09-26T22:43:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Eric Wong wrote:\n\n> It should probably just return the root path (\"c:/\" and\n> \"http://www.example.com/\" respectively).\n\nThat means recognizing drive letters and URLs.  Hm.\n\nSubversion commands seem to use svn_client_args_to_target_array2\nto canonicalize arguments.  It does something like the following:\n\n 1. split at @PEG revision\n 2. check if it looks like a URL (svn_path_is_url).  If so:\n\n    i.   urlencode characters with high bit set (svn_path_uri_from_iri)\n    ii.  urlencode some other special characters (svn_path_uri_autoescape)\n         (that is: [ \"<>\\\\^`{|}])\n    iii. (on Windows) convert backslashes to forward slashes\n    iv.  complain if there are any '..' (svn_path_is_backpath_present)\n    v.   make url scheme and hostname lowercase, strip default portnumber,\n\t strip trailing '/', collapse '/'-es including urlencoded %2F,\n\t strip '.' and '%2E' components, make drive letter in file:///\n         URLs uppercase, urlencode uri-special characters\n         (svn_uri_canonicalize)\n\n   Otherwise:\n\n    i.   canonicalize case, handle '..' and '.' components, and make\n\t path separators into '/'\n         (apr_filepath_merge(APR_FILEPATH_TRUENAME))\n    ii.  strip trailing '/', collapse '/'-es except in UNC paths,\n         strip '.' components, make drive letter uppercase\n         (svn_dirent_canonicalize)\n    iii. deal with \"truepath collisions\" (unwanted case\n         canonicalizations)\n    iv.  reject .svn and _svn directories\n\nMaybe we can use apr_filepath_merge() to avoid reinventing the wheel?\n"},{"id":"199986","messageId":"20120927001506.GA9515@dcvr.yhbt.net","threadId":"31119","inReplyTo":"20120926224307.GA31456@elie.Belkin","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-09-27T00:15:06Z","receivedAt":"2012-09-27T00:15:06Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Maybe we can use apr_filepath_merge() to avoid reinventing the wheel?\n\nIdeally, yes.  Is there an easy way to access that from Perl? (and\nfor the older versions of SVN folks people are running).\n\nPerhaps we can expose equivalent functionality in git via\ngit-rev-parse instead?\n"},{"id":"199989","messageId":"20120927021152.GE31456@elie.Belkin","threadId":"31119","inReplyTo":"20120927001506.GA9515@dcvr.yhbt.net","subject":"Re: [PATCH 3/7] Extract, test and enhance the logic to collapse ../foo paths.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-09-27T02:11:52Z","receivedAt":"2012-09-27T02:11:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Eric Wong wrote:\n\n> Ideally, yes.  Is there an easy way to access that from Perl? (and\n> for the older versions of SVN folks people are running).\n\nSubversion's swig bindings only wrap a few apr functions and do not\ndepend on fuller apr bindings.\n\nSomething like svn_dirent_is_under_root() could be useful.  It uses\nwhatever base path you choose.  I haven't tried using base_path=\"\"\nyet.  New in Subversion 1.7, but that would be ok --- an imperfect\nfallback for older Subversion versions would be fine.\n\nUnfortunately, from swig/core.i: \"SWIG can't digest these functions\nyet, so ignore them for now. TODO: make them work.\"\n\nsvn_client_args_to_target_array2() is exposed and would probably be\nperfect.  I can't seem to find how to create an apr_array_header_t\nto pass as its first argument, alas.\n\n> Perhaps we can expose equivalent functionality in git via\n> git-rev-parse instead?\n\nIt might be possible to add a git-svn--helper command that links to\nlibsvn or apr, but ick.  The trouble is it's not clear at all how\nSubversion's perl API was *designed* to be used.\n\nHm.\nJonathan\n"},{"id":"200598","messageId":"20121005070430.GA23572@elie.Belkin","threadId":"31119","inReplyTo":"5014387C.50903@pobox.com","subject":"[PATCH] git-svn: keep leading slash when canonicalizing paths (fallback case)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-05T07:04:31Z","receivedAt":"2012-10-05T07:04:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Subversion's svn_dirent_canonicalize() and svn_path_canonicalize()\nAPIs keep a leading slash in the return value if one was present on\nthe argument, which can be useful since it allows relative and\nabsolute paths to be distinguished.\n\nWhen git-svn's canonicalize_path() learned to use these functions if\navailable, its semantics changed in the corresponding way.  Some new\ncallers rely on the leading slash --- for example, if the slash is\nstripped out then _canonicalize_url_ourselves() will transform\n\"proto://host/path/to/resource\" to \"proto://hostpath/to/resource\".\n\nUnfortunately the fallback _canonicalize_path_ourselves(), used when\nthe appropriate SVN APIs are not usable, still follows the old\nsemantics, so if that code path is exercised then it breaks.  Fix it\nto follow the new convention.\n\nNoticed by forcing the fallback on and running tests.  Without this\npatch, t9101.4 fails:\n\n Bad URL passed to RA layer: Unable to open an ra_local session to \\\n URL: Local URL 'file://homejrnsrcgit-scratch/t/trash%20directory.\\\n t9101-git-svn-props/svnrepo' contains unsupported hostname at \\\n /home/jrn/src/git-scratch/perl/blib/lib/Git/SVN.pm line 148\n\nWith it, the git-svn tests pass again.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nHi Eric,\n\nMichael G Schwern wrote:\n> On 2012.7.28 6:55 AM, Jonathan Nieder wrote:\n\n>> When would this \"else\" case trip?\n>\n> When svn_path_canonicalize() does not exist in the SVN API, presumably because\n> their SVN is too old.\n\nI accidentally tested this \"else\" branch by making the other cases\nfalse.  t9101.4 failed as described above, or in other words,\ncanonicalize_url_ourselves() stripped out a few too many slashes.  For\nreference:\n\n| sub _canonicalize_url_ourselves {\n|         my ($url) = @_;\n|         if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) {\n|                 my ($scheme, $domain, $uri) = ($1, $2, _canonicalize_url_path(canonicalize_path($3)));\n|                 $url = \"$scheme://$domain$uri\";\n|         }\n|         $url;\n| }\n\nWhen $url is http://host/path/to/resource,\n\n\t$1 = \"http\", $2 = \"host\", $3 = \"/path/to/resource\"\n\tcanonicalize_path($3) = \"path/to/resource\" <--- (??)\n\t_canonicalize_url_path(ditto) = \"path/to/resource\"\n\t$url = \"http://hostpath/to/resource\"\n\nHow about this patch?\n\n perl/Git/SVN/Utils.pm |    1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex 4bb4dde8..8b8cf375 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -122,7 +122,6 @@ sub _canonicalize_path_ourselves {\n \t$path = _collapse_dotdot($path);\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-- \n1.7.10.4\n"},{"id":"200645","messageId":"20121005231207.GA22903@dcvr.yhbt.net","threadId":"31119","inReplyTo":"20121005070430.GA23572@elie.Belkin","subject":"Re: [PATCH] git-svn: keep leading slash when canonicalizing paths (fallback case)","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-10-05T23:12:07Z","receivedAt":"2012-10-05T23:12:07Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Noticed by forcing the fallback on and running tests.  Without this\n> patch, t9101.4 fails:\n> \n>  Bad URL passed to RA layer: Unable to open an ra_local session to \\\n>  URL: Local URL 'file://homejrnsrcgit-scratch/t/trash%20directory.\\\n>  t9101-git-svn-props/svnrepo' contains unsupported hostname at \\\n>  /home/jrn/src/git-scratch/perl/blib/lib/Git/SVN.pm line 148\n> \n> With it, the git-svn tests pass again.\n> \n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for noticing this.\nSigned-off-by: Eric Wong <normalperson@yhbt.net>\nand pushed to my master at git://bogomips.org/git-svn\n"},{"id":"200650","messageId":"7vfw5srwk1.fsf@alter.siamese.dyndns.org","threadId":"31119","inReplyTo":"20121005231207.GA22903@dcvr.yhbt.net","subject":"Re: [PATCH] git-svn: keep leading slash when canonicalizing paths (fallback case)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-06T05:36:46Z","receivedAt":"2012-10-06T05:36:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n> Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> Noticed by forcing the fallback on and running tests.  Without this\n>> patch, t9101.4 fails:\n>> \n>>  Bad URL passed to RA layer: Unable to open an ra_local session to \\\n>>  URL: Local URL 'file://homejrnsrcgit-scratch/t/trash%20directory.\\\n>>  t9101-git-svn-props/svnrepo' contains unsupported hostname at \\\n>>  /home/jrn/src/git-scratch/perl/blib/lib/Git/SVN.pm line 148\n>> \n>> With it, the git-svn tests pass again.\n>> \n>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n> Thanks for noticing this.\n> Signed-off-by: Eric Wong <normalperson@yhbt.net>\n> and pushed to my master at git://bogomips.org/git-svn\n\nWill pull before 1.8.0-rc1.  Thanks.\n"},{"id":"201162","messageId":"20121014114234.GA18127@elie.Belkin","threadId":"31119","inReplyTo":"50143700.80900@pobox.com","subject":"[PATCH/RFC 0/2] Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-14T11:42:34Z","receivedAt":"2012-10-14T11:42:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Eric,\n\nMichael G Schwern wrote:\n> On 2012.7.28 6:50 AM, Jonathan Nieder wrote:\n>> Michael G Schwern wrote:\n\n>>> --- a/perl/Git/SVN/Utils.pm\n>>> +++ b/perl/Git/SVN/Utils.pm\n>> [...]\n>>> @@ -100,6 +102,20 @@ API as a URL.\n>>>  =cut\n>>>  \n>>>  sub canonicalize_url {\n>>> +\tmy $url = shift;\n>>> +\n>>> +\t# The 1.7 way to do it\n>>> +\tif ( defined &SVN::_Core::svn_uri_canonicalize ) {\n>>> +\t\treturn SVN::_Core::svn_uri_canonicalize($url);\n>>> +\t}\n>>> +\t# There wasn't a 1.6 way to do it, so we do it ourself.\n>>> +\telse {\n>>> +\t\treturn _canonicalize_url_ourselves($url);\n[...]\n>> Leaves me a bit nervous.\n>\n> As it should, SVN dumped a mess on us.\n\nHere's a pair of patches that address some of the bugs I was alluding\nto.  Patch 1 makes _canonicalize_url_ourselves() match Subversion's\nown canonicalization behavior more closely, though even with that\npatch it still does not meet Subversion's requirements perfectly\n(e.g., \"%ab\" is not canonicalized to \"%AB\").  Patch 2 makes that not\nmatter by using svn_path_canonicalize() when possible, which is the\nstandard way to do this kind of thing.\n\nSorry for the lack of clarity before.\n\nJonathan Nieder (2):\n  git svn: do not overescape URLs (fallback case)\n  Git::SVN::Utils::canonicalize_url: use svn_path_canonicalize when\n    available\n\n perl/Git/SVN/Utils.pm | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n"},{"id":"201163","messageId":"20121014114521.GA21106@elie.Belkin","threadId":"31119","inReplyTo":"20121014114234.GA18127@elie.Belkin","subject":"[PATCH 1/2] git svn: do not overescape URLs (fallback case)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-14T11:45:21Z","receivedAt":"2012-10-14T11:45:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Subversion's canonical URLs are intended to make URL comparison easy\nand therefore have strict rules about what characters are special\nenough to urlencode and what characters should be left alone.\n\nWhen in the fallback codepath because unable to use libsvn's own\ncanonicalization function for some reason, escape special characters\nin URIs according to the svn_uri__char_validity[] table in\nsubversion/libsvn_subr/path.c (r935829).  The libsvn versions that\ntrigger this code path are not likely to be strict enough to care, but\nit's nicer to be consistent.\n\nNoticed by using SVN 1.6.17 perl bindings, which do not provide\nSVN::_Core::svn_uri_canonicalize (triggering the fallback code),\nwith libsvn 1.7.5, whose do_switch is fussy enough to care:\n\n  Committing to file:///home/jrn/src/git/t/trash%20directory.\\\n  t9118-git-svn-funky-branch-names/svnrepo/pr%20ject/branches\\\n  /more%20fun%20plugin%21 ...\n  svn: E235000: In file '[...]/subversion/libsvn_subr/dirent_uri.c' \\\n  line 2291: assertion failed (svn_uri_is_canonical(url, pool))\n  error: git-svn died of signal 6\n  not ok - 3 test dcommit to funky branch\n\nAfter this change, the '!' in 'more%20fun%20plugin!' is not urlencoded\nand t9118 passes again.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n perl/Git/SVN/Utils.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex 8b8cf375..3d1a0933 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -155,7 +155,7 @@ sub _canonicalize_url_path {\n \n \tmy @parts;\n \tforeach my $part (split m{/+}, $uri_path) {\n-\t\t$part =~ s/([^~\\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf(\"%%%02X\",ord($1))/eg;\n+\t\t$part =~ s/([^!\\$%&'()*+,.\\/\\w:=\\@_`~-]|%(?![a-fA-F0-9]{2}))/sprintf(\"%%%02X\",ord($1))/eg;\n \t\tpush @parts, $part;\n \t}\n \n-- \n1.8.0.rc2\n"},{"id":"201164","messageId":"20121014114857.GB21106@elie.Belkin","threadId":"31119","inReplyTo":"20121014114234.GA18127@elie.Belkin","subject":"[PATCH 2/2] git svn: canonicalize_url(): use svn_path_canonicalize when available","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-14T11:48:57Z","receivedAt":"2012-10-14T11:48:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Until Subversion 1.7 (more precisely r873487), the standard way to\ncanonicalize a URI was to call svn_path_canonicalize().  Use it.\n\nThis saves \"git svn\" from having to rely on our imperfect\nreimplementation of the same.  If the function doesn't exist or\nreturns undef, though, it can use the fallback code, which we keep to\nbe conservative.  Since svn_path_canonicalize() was added before\nSubversion 1.1, hopefully that doesn't happen often.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n perl/Git/SVN/Utils.pm | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex 3d1a0933..40f7c799 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -138,15 +138,19 @@ API as a URL.\n \n sub canonicalize_url {\n \tmy $url = shift;\n+\tmy $rv;\n \n \t# The 1.7 way to do it\n \tif ( defined &SVN::_Core::svn_uri_canonicalize ) {\n-\t\treturn SVN::_Core::svn_uri_canonicalize($url);\n+\t\t$rv = SVN::_Core::svn_uri_canonicalize($url);\n \t}\n-\t# There wasn't a 1.6 way to do it, so we do it ourself.\n-\telse {\n-\t\treturn _canonicalize_url_ourselves($url);\n+\t# The 1.6 way to do it\n+\telsif ( defined &SVN::_Core::svn_path_canonicalize ) {\n+\t\t$rv = SVN::_Core::svn_path_canonicalize($url);\n \t}\n+\treturn $rv if defined $rv;\n+\n+\treturn _canonicalize_url_ourselves($url);\n }\n \n \n-- \n1.8.0.rc2\n"},{"id":"201776","messageId":"20121023225812.GA21716@dcvr.yhbt.net","threadId":"31119","inReplyTo":"20121014114857.GB21106@elie.Belkin","subject":"Re: [PATCH 2/2] git svn: canonicalize_url(): use svn_path_canonicalize when available","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-10-23T22:58:12Z","receivedAt":"2012-10-23T22:58:12Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Until Subversion 1.7 (more precisely r873487), the standard way to\n> canonicalize a URI was to call svn_path_canonicalize().  Use it.\n> \n> This saves \"git svn\" from having to rely on our imperfect\n> reimplementation of the same.  If the function doesn't exist or\n> returns undef, though, it can use the fallback code, which we keep to\n> be conservative.  Since svn_path_canonicalize() was added before\n> Subversion 1.1, hopefully that doesn't happen often.\n\nHi Jonathan, this fails for me using http (but not file:// or svn://).\n1/2 of this RFC looks fine, though.\n\nsubversion 1.6.12dfsg-6, apache2-mpm-prefork 2.2.16-6+squeeze8\n(Debian squeeze)\n\n$ SVN_HTTPD_PORT=12345 sh t9118-git-svn-funky-branch-names.sh -v\nInitialized empty Git repository in /home/ew/git-core/t/trash directory.t9118-git-svn-funky-branch-names/.git/\nexpecting success: \n\tmkdir project project/trunk project/branches project/tags &&\n\techo foo > project/trunk/foo &&\n\tsvn_cmd import -m \"$test_description\" project \"$svnrepo/pr ject\" &&\n\trm -rf project &&\n\tsvn_cmd cp -m \"fun\" \"$svnrepo/pr ject/trunk\" \\\n\t                \"$svnrepo/pr ject/branches/fun plugin\" &&\n\tsvn_cmd cp -m \"more fun!\" \"$svnrepo/pr ject/branches/fun plugin\" \\\n\t                      \"$svnrepo/pr ject/branches/more fun plugin!\" &&\n\tsvn_cmd cp -m \"scary\" \"$svnrepo/pr ject/branches/fun plugin\" \\\n\t              \"$svnrepo/pr ject/branches/$scary_uri\" &&\n\tsvn_cmd cp -m \"leading dot\" \"$svnrepo/pr ject/trunk\" \\\n\t\t\t\"$svnrepo/pr ject/branches/.leading_dot\" &&\n\tsvn_cmd cp -m \"trailing dot\" \"$svnrepo/pr ject/trunk\" \\\n\t\t\t\"$svnrepo/pr ject/branches/trailing_dot.\" &&\n\tsvn_cmd cp -m \"trailing .lock\" \"$svnrepo/pr ject/trunk\" \\\n\t\t\t\"$svnrepo/pr ject/branches/trailing_dotlock.lock\" &&\n\tsvn_cmd cp -m \"reflog\" \"$svnrepo/pr ject/trunk\" \\\n\t\t\t\"$svnrepo/pr ject/branches/not-a@{0}reflog@\" &&\n\tstart_httpd\n\t\nAdding         project/trunk\nAdding         project/trunk/foo\nAdding         project/branches\nAdding         project/tags\n\nCommitted revision 1.\n\nCommitted revision 2.\n\nCommitted revision 3.\n\nCommitted revision 4.\n\nCommitted revision 5.\n\nCommitted revision 6.\n\nCommitted revision 7.\n\nCommitted revision 8.\nok 1 - setup svnrepo\n\nexpecting success: \n\tgit svn clone -s \"$svnrepo/pr ject\" project &&\n\t(\n\t\tcd project &&\n\t\tgit rev-parse \"refs/remotes/fun%20plugin\" &&\n\t\tgit rev-parse \"refs/remotes/more%20fun%20plugin!\" &&\n\t\tgit rev-parse \"refs/remotes/$scary_ref\" &&\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/$non_reflog\"\n\t)\n\t\nInitialized empty Git repository in /home/ew/git-core/t/trash directory.t9118-git-svn-funky-branch-names/project/.git/\nBad URL passed to RA layer: URL 'http://127.0.0.1:12345/pr ject' is malformed or the scheme or host or path is missing at /home/ew/git-core/perl/blib/lib/Git/SVN.pm line 310\n\nnot ok - 2 test clone with funky branch names\n#\t\n#\t\tgit svn clone -s \"$svnrepo/pr ject\" project &&\n#\t\t(\n#\t\t\tcd project &&\n#\t\t\tgit rev-parse \"refs/remotes/fun%20plugin\" &&\n#\t\t\tgit rev-parse \"refs/remotes/more%20fun%20plugin!\" &&\n#\t\t\tgit rev-parse \"refs/remotes/$scary_ref\" &&\n#\t\t\tgit rev-parse \"refs/remotes/%2Eleading_dot\" &&\n#\t\t\tgit rev-parse \"refs/remotes/trailing_dot%2E\" &&\n#\t\t\tgit rev-parse \"refs/remotes/trailing_dotlock%2Elock\" &&\n#\t\t\tgit rev-parse \"refs/remotes/$non_reflog\"\n#\t\t)\n#\t\t\n\nexpecting success: \n\t(\n\t\tcd project &&\n\t\tgit reset --hard 'refs/remotes/more%20fun%20plugin!' &&\n\t\techo hello >> foo &&\n\t\tgit commit -m 'hello' -- foo &&\n\t\tgit svn dcommit\n\t)\n\t\nfatal: ambiguous argument 'refs/remotes/more%20fun%20plugin!': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\nnot ok - 3 test dcommit to funky branch\n#\t\n#\t\t(\n#\t\t\tcd project &&\n#\t\t\tgit reset --hard 'refs/remotes/more%20fun%20plugin!' &&\n#\t\t\techo hello >> foo &&\n#\t\t\tgit commit -m 'hello' -- foo &&\n#\t\t\tgit svn dcommit\n#\t\t)\n#\t\t\n\nexpecting success: \n\t(\n\t\tcd project &&\n\t\tgit reset --hard \"refs/remotes/$scary_ref\" &&\n\t\techo urls are scary >> foo &&\n\t\tgit commit -m \"eep\" -- foo &&\n\t\tgit svn dcommit\n\t)\n\t\nfatal: ambiguous argument 'refs/remotes/Abo-Uebernahme%20(Bug%20#994)': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\nnot ok - 4 test dcommit to scary branch\n#\t\n#\t\t(\n#\t\t\tcd project &&\n#\t\t\tgit reset --hard \"refs/remotes/$scary_ref\" &&\n#\t\t\techo urls are scary >> foo &&\n#\t\t\tgit commit -m \"eep\" -- foo &&\n#\t\t\tgit svn dcommit\n#\t\t)\n#\t\t\n\nexpecting success: \n\t(\n\t\tcd project &&\n\t\tgit reset --hard \"refs/remotes/trailing_dotlock%2Elock\" &&\n\t\techo who names branches like this anyway? >> foo &&\n\t\tgit commit -m \"bar\" -- foo &&\n\t\tgit svn dcommit\n\t)\n\t\nfatal: ambiguous argument 'refs/remotes/trailing_dotlock%2Elock': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\nnot ok - 5 test dcommit to trailing_dotlock branch\n#\t\n#\t\t(\n#\t\t\tcd project &&\n#\t\t\tgit reset --hard \"refs/remotes/trailing_dotlock%2Elock\" &&\n#\t\t\techo who names branches like this anyway? >> foo &&\n#\t\t\tgit commit -m \"bar\" -- foo &&\n#\t\t\tgit svn dcommit\n#\t\t)\n#\t\t\n\n# failed 4 among 5 test(s)\n1..5\n"}]}