{"thread":{"id":"37507","subject":"[PATCH v2] git svn: info: correctly handle absolute path args","startedAt":"2014-09-07T08:06:26Z","lastAt":"2014-09-09T06:38:26Z","messageCount":4,"participants":["Eric Wong","Johannes Sixt","brian m. carlson"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"249005","messageId":"20140907080626.GA6273@dcvr.yhbt.net","threadId":"37507","inReplyTo":null,"subject":"[PATCH v2] git svn: info: correctly handle absolute path args","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2014-09-07T08:06:26Z","receivedAt":"2014-09-07T08:06:26Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Calling \"git svn info $(pwd)\" would hit:\n  \"Reading from filehandle failed at ...\"\nerrors due to improper prefixing and canonicalization.\n\nStrip the toplevel path from absolute filesystem paths to ensure\ndownstream canonicalization routines are only exposed to paths\ntracked in git (or SVN).\n\nThanks to Andrej Manduch <amanduch@gmail.com> for originally\nnoticing the issue and fixing my original version of this to handle\nmore corner cases such as \"/path/to/top/../top\" and\n\"/path/to/top/../top/file\" as shown in the new test cases.\n\nSigned-off-by: Andrej Manduch <amanduch@gmail.com>\nSigned-off-by: Eric Wong <normalperson@yhbt.net>\n---\n    Eric Wong <normalperson@yhbt.net> wrote:\n    > Thanks Andrej.  I'll queue that on top of mine.\n    > Can you turn that into a proper commit message with Subject?\n    > Thanks.\n\n    I just squashed in your change and gave you credit.\n    Sorry, I forgot about this completely.  Will test a bit more\n    and ask Junio to pull.\n\n git-svn.perl            | 22 ++++++++++++++++------\n t/t9119-git-svn-info.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 6 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 1f41ee1..47cd6ea 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1477,10 +1477,20 @@ sub cmd_commit_diff {\n \t}\n }\n \n-\n sub cmd_info {\n-\tmy $path = canonicalize_path(defined($_[0]) ? $_[0] : \".\");\n-\tmy $fullpath = canonicalize_path($cmd_dir_prefix . $path);\n+\tmy $path_arg = defined($_[0]) ? $_[0] : '.';\n+\tmy $path = $path_arg;\n+\tif ($path =~ m!\\A/!) {\n+\t\tmy $toplevel = eval {\n+\t\t\tmy @cmd = qw/rev-parse --show-toplevel/;\n+\t\t\tcommand_oneline(\\@cmd, STDERR => 0);\n+\t\t};\n+\t\t$path = canonicalize_path($path);\n+\t\t$path =~ s!\\A\\Q$toplevel\\E/?!!;\n+\t\t$path = canonicalize_path($path);\n+\t} else {\n+\t\t$path = canonicalize_path($cmd_dir_prefix . $path);\n+\t}\n \tif (exists $_[1]) {\n \t\tdie \"Too many arguments specified\\n\";\n \t}\n@@ -1501,14 +1511,14 @@ 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 = canonicalize_url( add_path_to_url( $url, $fullpath ) );\n+\tmy $full_url = canonicalize_url( add_path_to_url( $url, $path ) );\n \n \tif ($_url) {\n \t\tprint \"$full_url\\n\";\n \t\treturn;\n \t}\n \n-\tmy $result = \"Path: $path\\n\";\n+\tmy $result = \"Path: $path_arg\\n\";\n \t$result .= \"Name: \" . basename($path) . \"\\n\" if $file_type ne \"dir\";\n \t$result .= \"URL: $full_url\\n\";\n \n@@ -1539,7 +1549,7 @@ sub cmd_info {\n \t}\n \n \tmy ($lc_author, $lc_rev, $lc_date_utc);\n-\tmy @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, \"--\", $fullpath);\n+\tmy @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, \"--\", $path);\n \tmy $log = command_output_pipe(@args);\n \tmy $esc_color = qr/(?:\\033\\[(?:(?:\\d+;)*\\d*)?m)*/;\n \twhile (<$log>) {\ndiff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh\nindex ff19695..f16f323 100755\n--- a/t/t9119-git-svn-info.sh\n+++ b/t/t9119-git-svn-info.sh\n@@ -74,6 +74,36 @@ test_expect_success 'info .' \"\n \ttest_cmp_info expected.info-dot actual.info-dot\n \t\"\n \n+test_expect_success 'info $(pwd)' '\n+\t(cd svnwc; svn info \"$(pwd)\") >expected.info-pwd &&\n+\t(cd gitwc; git svn info \"$(pwd)\") >actual.info-pwd &&\n+\tgrep -v ^Path: <expected.info-pwd >expected.info-np &&\n+\tgrep -v ^Path: <actual.info-pwd >actual.info-np &&\n+\ttest_cmp_info expected.info-np actual.info-np &&\n+\ttest \"$(sed -ne \\\"/^Path:/ s!/svnwc!!\\\" <expected.info-pwd)\" = \\\n+\t     \"$(sed -ne \\\"/^Path:/ s!/gitwc!!\\\" <actual.info-pwd)\"\n+\t'\n+\n+test_expect_success 'info $(pwd)/../___wc' '\n+\t(cd svnwc; svn info \"$(pwd)/../svnwc\") >expected.info-pwd &&\n+\t(cd gitwc; git svn info \"$(pwd)/../gitwc\") >actual.info-pwd &&\n+\tgrep -v ^Path: <expected.info-pwd >expected.info-np &&\n+\tgrep -v ^Path: <actual.info-pwd >actual.info-np &&\n+\ttest_cmp_info expected.info-np actual.info-np &&\n+\ttest \"$(sed -ne \\\"/^Path:/ s!/svnwc!!\\\" <expected.info-pwd)\" = \\\n+\t     \"$(sed -ne \\\"/^Path:/ s!/gitwc!!\\\" <actual.info-pwd)\"\n+\t'\n+\n+test_expect_success 'info $(pwd)/../___wc//file' '\n+\t(cd svnwc; svn info \"$(pwd)/../svnwc//file\") >expected.info-pwd &&\n+\t(cd gitwc; git svn info \"$(pwd)/../gitwc//file\") >actual.info-pwd &&\n+\tgrep -v ^Path: <expected.info-pwd >expected.info-np &&\n+\tgrep -v ^Path: <actual.info-pwd >actual.info-np &&\n+\ttest_cmp_info expected.info-np actual.info-np &&\n+\ttest \"$(sed -ne \\\"/^Path:/ s!/svnwc!!\\\" <expected.info-pwd)\" = \\\n+\t     \"$(sed -ne \\\"/^Path:/ s!/gitwc!!\\\" <actual.info-pwd)\"\n+\t'\n+\n test_expect_success 'info --url .' '\n \ttest \"$(cd gitwc; git svn info --url .)\" = \"$quoted_svnrepo\"\n \t'\n-- \nEW\n"},{"id":"249007","messageId":"540C1E07.7030105@kdbg.org","threadId":"37507","inReplyTo":"20140907080626.GA6273@dcvr.yhbt.net","subject":"Re: [PATCH v2] git svn: info: correctly handle absolute path args","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-09-07T08:57:43Z","receivedAt":"2014-09-07T08:57:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.09.2014 10:06, schrieb Eric Wong:\n> diff --git a/git-svn.perl b/git-svn.perl\n> index 1f41ee1..47cd6ea 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -1477,10 +1477,20 @@ sub cmd_commit_diff {\n>  \t}\n>  }\n>  \n> -\n>  sub cmd_info {\n> -\tmy $path = canonicalize_path(defined($_[0]) ? $_[0] : \".\");\n> -\tmy $fullpath = canonicalize_path($cmd_dir_prefix . $path);\n> +\tmy $path_arg = defined($_[0]) ? $_[0] : '.';\n> +\tmy $path = $path_arg;\n> +\tif ($path =~ m!\\A/!) {\n\nThere must be a more portable way to check for an absolute path. Think\nof DOS-style paths...\n\n> +\t\tmy $toplevel = eval {\n> +\t\t\tmy @cmd = qw/rev-parse --show-toplevel/;\n> +\t\t\tcommand_oneline(\\@cmd, STDERR => 0);\n> +\t\t};\n> +\t\t$path = canonicalize_path($path);\n> +\t\t$path =~ s!\\A\\Q$toplevel\\E/?!!;\n> +\t\t$path = canonicalize_path($path);\n> +\t} else {\n> +\t\t$path = canonicalize_path($cmd_dir_prefix . $path);\n> +\t}\n>  \tif (exists $_[1]) {\n>  \t\tdie \"Too many arguments specified\\n\";\n>  \t}\n\n-- Hannes\n"},{"id":"249021","messageId":"20140908002256.GB43902@vauxhall.crustytoothpaste.net","threadId":"37507","inReplyTo":"540C1E07.7030105@kdbg.org","subject":"Re: [PATCH v2] git svn: info: correctly handle absolute path args","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-09-08T00:22:56Z","receivedAt":"2014-09-08T00:22:56Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Sep 07, 2014 at 10:57:43AM +0200, Johannes Sixt wrote:\n> Am 07.09.2014 10:06, schrieb Eric Wong:\n> >  sub cmd_info {\n> > -\tmy $path = canonicalize_path(defined($_[0]) ? $_[0] : \".\");\n> > -\tmy $fullpath = canonicalize_path($cmd_dir_prefix . $path);\n> > +\tmy $path_arg = defined($_[0]) ? $_[0] : '.';\n> > +\tmy $path = $path_arg;\n> > +\tif ($path =~ m!\\A/!) {\n> \n> There must be a more portable way to check for an absolute path. Think\n> of DOS-style paths...\n\nYou probably want File::Spec->file_name_is_absolute($path).  Either that\nor always turn the path into absolute or relative form with\nFile::Spec->abs2rel or File::Spec->rel2abs, and then go from there.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"249071","messageId":"20140909063825.GA6545@dcvr.yhbt.net","threadId":"37507","inReplyTo":"20140908002256.GB43902@vauxhall.crustytoothpaste.net","subject":"[PATCH v3] git svn: info: correctly handle absolute path args","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2014-09-09T06:38:26Z","receivedAt":"2014-09-09T06:38:26Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Thanks Johannes and brian.  Diff against v2:\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1480,13 +1480,30 @@ sub cmd_commit_diff {\n sub cmd_info {\n\tmy $path_arg = defined($_[0]) ? $_[0] : '.';\n\tmy $path = $path_arg;\n-\tif ($path =~ m!\\A/!) {\n+\tif (File::Spec->file_name_is_absolute($path)) {\n+\t\t$path = canonicalize_path($path);\n+\n\t\tmy $toplevel = eval {\n\t\t\tmy @cmd = qw/rev-parse --show-toplevel/;\n\t\t\tcommand_oneline(\\@cmd, STDERR => 0);\n\t\t};\n-\t\t$path = canonicalize_path($path);\n-\t\t$path =~ s!\\A\\Q$toplevel\\E/?!!;\n+\n+\t\t# remove $toplevel from the absolute path:\n+\t\tmy ($vol, $dirs, $file) = File::Spec->splitpath($path);\n+\t\tmy (undef, $tdirs, $tfile) = File::Spec->splitpath($toplevel);\n+\t\tmy @dirs = File::Spec->splitdir($dirs);\n+\t\tmy @tdirs = File::Spec->splitdir($tdirs);\n+\t\tpop @dirs if $dirs[-1] eq '';\n+\t\tpop @tdirs if $tdirs[-1] eq '';\n+\t\tpush @dirs, $file;\n+\t\tpush @tdirs, $tfile;\n+\t\twhile (@tdirs && @dirs && $tdirs[0] eq $dirs[0]) {\n+\t\t\tshift @dirs;\n+\t\t\tshift @tdirs;\n+\t\t}\n+\t\t$dirs = File::Spec->catdir(@dirs);\n+\t\t$path = File::Spec->catpath($vol, $dirs);\n+\n\t\t$path = canonicalize_path($path);\n\t} else {\n\t\t$path = canonicalize_path($cmd_dir_prefix . $path);\n------------------8<-------------------\nFrom: Eric Wong <normalperson@yhbt.net>\nSubject: [PATCH v3] git svn: info: correctly handle absolute path args\n\nCalling \"git svn info $(pwd)\" would hit:\n  \"Reading from filehandle failed at ...\"\nerrors due to improper prefixing and canonicalization.\n\nStrip the toplevel path from absolute filesystem paths to ensure\ndownstream canonicalization routines are only exposed to paths\ntracked in git (or SVN).\n\nv2:\n  Thanks to Andrej Manduch for originally noticing the issue\n  and fixing my original version of this to handle\n  more corner cases such as \"/path/to/top/../top\" and\n  \"/path/to/top/../top/file\" as shown in the new test cases.\n\nv3:\n  Fix pathname portability problems pointed out by Johannes Sixt\n  with a hint from brian m. carlson.\n\nCc: Johannes Sixt <j6t@kdbg.org>\nCc: \"brian m. carlson\" <sandals@crustytoothpaste.net>\nSigned-off-by: Andrej Manduch <amanduch@gmail.com>\nSigned-off-by: Eric Wong <normalperson@yhbt.net>\n---\n git-svn.perl            | 39 +++++++++++++++++++++++++++++++++------\n t/t9119-git-svn-info.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 6 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 1f41ee1..40565cd 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1477,10 +1477,37 @@ sub cmd_commit_diff {\n \t}\n }\n \n-\n sub cmd_info {\n-\tmy $path = canonicalize_path(defined($_[0]) ? $_[0] : \".\");\n-\tmy $fullpath = canonicalize_path($cmd_dir_prefix . $path);\n+\tmy $path_arg = defined($_[0]) ? $_[0] : '.';\n+\tmy $path = $path_arg;\n+\tif (File::Spec->file_name_is_absolute($path)) {\n+\t\t$path = canonicalize_path($path);\n+\n+\t\tmy $toplevel = eval {\n+\t\t\tmy @cmd = qw/rev-parse --show-toplevel/;\n+\t\t\tcommand_oneline(\\@cmd, STDERR => 0);\n+\t\t};\n+\n+\t\t# remove $toplevel from the absolute path:\n+\t\tmy ($vol, $dirs, $file) = File::Spec->splitpath($path);\n+\t\tmy (undef, $tdirs, $tfile) = File::Spec->splitpath($toplevel);\n+\t\tmy @dirs = File::Spec->splitdir($dirs);\n+\t\tmy @tdirs = File::Spec->splitdir($tdirs);\n+\t\tpop @dirs if $dirs[-1] eq '';\n+\t\tpop @tdirs if $tdirs[-1] eq '';\n+\t\tpush @dirs, $file;\n+\t\tpush @tdirs, $tfile;\n+\t\twhile (@tdirs && @dirs && $tdirs[0] eq $dirs[0]) {\n+\t\t\tshift @dirs;\n+\t\t\tshift @tdirs;\n+\t\t}\n+\t\t$dirs = File::Spec->catdir(@dirs);\n+\t\t$path = File::Spec->catpath($vol, $dirs);\n+\n+\t\t$path = canonicalize_path($path);\n+\t} else {\n+\t\t$path = canonicalize_path($cmd_dir_prefix . $path);\n+\t}\n \tif (exists $_[1]) {\n \t\tdie \"Too many arguments specified\\n\";\n \t}\n@@ -1501,14 +1528,14 @@ 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 = canonicalize_url( add_path_to_url( $url, $fullpath ) );\n+\tmy $full_url = canonicalize_url( add_path_to_url( $url, $path ) );\n \n \tif ($_url) {\n \t\tprint \"$full_url\\n\";\n \t\treturn;\n \t}\n \n-\tmy $result = \"Path: $path\\n\";\n+\tmy $result = \"Path: $path_arg\\n\";\n \t$result .= \"Name: \" . basename($path) . \"\\n\" if $file_type ne \"dir\";\n \t$result .= \"URL: $full_url\\n\";\n \n@@ -1539,7 +1566,7 @@ sub cmd_info {\n \t}\n \n \tmy ($lc_author, $lc_rev, $lc_date_utc);\n-\tmy @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, \"--\", $fullpath);\n+\tmy @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, \"--\", $path);\n \tmy $log = command_output_pipe(@args);\n \tmy $esc_color = qr/(?:\\033\\[(?:(?:\\d+;)*\\d*)?m)*/;\n \twhile (<$log>) {\ndiff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh\nindex ff19695..f16f323 100755\n--- a/t/t9119-git-svn-info.sh\n+++ b/t/t9119-git-svn-info.sh\n@@ -74,6 +74,36 @@ test_expect_success 'info .' \"\n \ttest_cmp_info expected.info-dot actual.info-dot\n \t\"\n \n+test_expect_success 'info $(pwd)' '\n+\t(cd svnwc; svn info \"$(pwd)\") >expected.info-pwd &&\n+\t(cd gitwc; git svn info \"$(pwd)\") >actual.info-pwd &&\n+\tgrep -v ^Path: <expected.info-pwd >expected.info-np &&\n+\tgrep -v ^Path: <actual.info-pwd >actual.info-np &&\n+\ttest_cmp_info expected.info-np actual.info-np &&\n+\ttest \"$(sed -ne \\\"/^Path:/ s!/svnwc!!\\\" <expected.info-pwd)\" = \\\n+\t     \"$(sed -ne \\\"/^Path:/ s!/gitwc!!\\\" <actual.info-pwd)\"\n+\t'\n+\n+test_expect_success 'info $(pwd)/../___wc' '\n+\t(cd svnwc; svn info \"$(pwd)/../svnwc\") >expected.info-pwd &&\n+\t(cd gitwc; git svn info \"$(pwd)/../gitwc\") >actual.info-pwd &&\n+\tgrep -v ^Path: <expected.info-pwd >expected.info-np &&\n+\tgrep -v ^Path: <actual.info-pwd >actual.info-np &&\n+\ttest_cmp_info expected.info-np actual.info-np &&\n+\ttest \"$(sed -ne \\\"/^Path:/ s!/svnwc!!\\\" <expected.info-pwd)\" = \\\n+\t     \"$(sed -ne \\\"/^Path:/ s!/gitwc!!\\\" <actual.info-pwd)\"\n+\t'\n+\n+test_expect_success 'info $(pwd)/../___wc//file' '\n+\t(cd svnwc; svn info \"$(pwd)/../svnwc//file\") >expected.info-pwd &&\n+\t(cd gitwc; git svn info \"$(pwd)/../gitwc//file\") >actual.info-pwd &&\n+\tgrep -v ^Path: <expected.info-pwd >expected.info-np &&\n+\tgrep -v ^Path: <actual.info-pwd >actual.info-np &&\n+\ttest_cmp_info expected.info-np actual.info-np &&\n+\ttest \"$(sed -ne \\\"/^Path:/ s!/svnwc!!\\\" <expected.info-pwd)\" = \\\n+\t     \"$(sed -ne \\\"/^Path:/ s!/gitwc!!\\\" <actual.info-pwd)\"\n+\t'\n+\n test_expect_success 'info --url .' '\n \ttest \"$(cd gitwc; git svn info --url .)\" = \"$quoted_svnrepo\"\n \t'\n-- \nEW\n"}]}