{"thread":{"id":"37284","subject":"Re: [PATCH] git-svn: doublecheck if really file or dir","startedAt":"2014-08-03T13:12:10Z","lastAt":"2014-08-04T04:02:12Z","messageCount":2,"participants":["Andrej Manduch","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"247197","messageId":"53DE352A.5050505@gmail.com","threadId":"37284","inReplyTo":"53DE31E8.3070405@gmail.com","subject":"Re: [PATCH] git-svn: doublecheck if really file or dir","fromName":"Andrej Manduch","fromEmail":"amanduch@gmail.com","sentAt":"2014-08-03T13:12:10Z","receivedAt":"2014-08-03T13:12:10Z","isPatch":true,"sender":{"key":"amanduch@gmail.com","avatar":null},"body":"On 08/03/2014 02:22 PM, Andrej Manduch wrote:\n> Hi Eric,\n> \n> Nice touch, It works like charm. However unfortunatelly now I think you\n> introduced new bug :)\n> \n> On 08/03/2014 04:45 AM, Eric Wong wrote:\n>> Hi Andrej, I could not help thinking your patch was obscuring\n>> another bug.  I think I have an alternative to your patch which\n>> fixes both our bugs.  Can you give this a shot?  Thanks.\n>>\n>> --------------------------- 8< ----------------------------\n>> Subject: [PATCH] git svn: info: correctly handle absolute path args\n>>\n>> Calling \"git svn info $(pwd)\" would hit:\n>>   \"Reading from filehandle failed at ...\"\n>> errors due to improper prefixing and canonicalization.\n>>\n>> Strip the toplevel path from absolute filesystem paths to ensure\n>> downstream canonicalization routines are only exposed to paths\n>> tracked in git (or SVN).\n>>\n>> Noticed-by: Andrej Manduch <amanduch@gmail.com>\n>> Signed-off-by: Eric Wong <normalperson@yhbt.net>\n>> ---\n>>  git-svn.perl            | 21 +++++++++++++++------\n>>  t/t9119-git-svn-info.sh | 10 ++++++++++\n>>  2 files changed, 25 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/git-svn.perl b/git-svn.perl\n>> index 1f41ee1..1f9582b 100755\n>> --- a/git-svn.perl\n>> +++ b/git-svn.perl\n>> @@ -1477,10 +1477,19 @@ 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 =~ s!\\A\\Q$toplevel\\E/?!!;\n> I have problem with this line ^^^\n> \n> Suppose your $toplevel is \"/sometning\" and you type in command line\n> something like that: \"git svn info /somethingsrc\" and as you see this\n> should end up with error. However \"$path =~ s!\\A\\Q$toplevel\\E/?!!;\"\n> will just cut \"/sometning\" from \"/somethingsrc\" and and up with same\n> answer as for \"svn git info src\" which is not equivalent query.\n> \n> Second scenario is something which worries me more: If your query look\n> like this: \"git svn info /something//src\" it will just end up with error\n> because it will set $path to \"/src\" witch is outside of repository.\n> \n> Second scenario can be fixed with this:\n> \n> diff --git a/git-svn.perl b/git-svn.perl\n> index a69f0fc..00f9d01 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -1483,7 +1483,7 @@ sub cmd_info {\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 =~ s!\\A\\Q$toplevel\\E/?!!;\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> \n\nActualy this will be even better:\n\n\nSigned-off-by: Andrej Manduch <amanduch@gmail.com>\n---\n git-svn.perl | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex a69f0fc..58df866 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1483,6 +1483,7 @@ sub cmd_info {\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-- \n2.0.0.GIT\n\nBecause this will have not problem with really weird query like: \"git\nsvn info /media/../media/something//src\"\n\n> \n> However I'm not sure if this will work on windows (where slashes are in\n> different orientation).\n> \n> \n> On 08/03/2014 04:45 AM, Eric Wong wrote:\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 +1510,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 +1548,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>) {\n>> diff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh\n>> index ff19695..4f6e669 100755\n>> --- a/t/t9119-git-svn-info.sh\n>> +++ b/t/t9119-git-svn-info.sh\n>> @@ -74,6 +74,16 @@ 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 --url .' '\n>>  \ttest \"$(cd gitwc; git svn info --url .)\" = \"$quoted_svnrepo\"\n>>  \t'\n>>\n"},{"id":"247202","messageId":"20140804040212.GA14629@dcvr.yhbt.net","threadId":"37284","inReplyTo":"53DE352A.5050505@gmail.com","subject":"Re: [PATCH] git-svn: doublecheck if really file or dir","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2014-08-04T04:02:12Z","receivedAt":"2014-08-04T04:02:12Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Andrej Manduch <amanduch@gmail.com> wrote:\n> On 08/03/2014 02:22 PM, Andrej Manduch wrote:\n> > Nice touch, It works like charm. However unfortunatelly now I think you\n> > introduced new bug :)\n\nGood catch!\n\n> > On 08/03/2014 04:45 AM, Eric Wong wrote:\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 =~ s!\\A\\Q$toplevel\\E/?!!;\n> > I have problem with this line ^^^\n> > \n> > Suppose your $toplevel is \"/sometning\" and you type in command line\n> > something like that: \"git svn info /somethingsrc\" and as you see this\n> > should end up with error. However \"$path =~ s!\\A\\Q$toplevel\\E/?!!;\"\n> > will just cut \"/sometning\" from \"/somethingsrc\" and and up with same\n> > answer as for \"svn git info src\" which is not equivalent query.\n> > \n> > Second scenario is something which worries me more: If your query look\n> > like this: \"git svn info /something//src\" it will just end up with error\n> > because it will set $path to \"/src\" witch is outside of repository.\n> > \n> > Second scenario can be fixed with this:\n> > \n> \n> Actualy this will be even better:\n\nThanks Andrej.  I'll queue that on top of mine.\nCan you turn that into a proper commit message with Subject?\nThanks.\n(The English-generating part of my brain is too tired)\n\n> Signed-off-by: Andrej Manduch <amanduch@gmail.com>\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -1483,6 +1483,7 @@ sub cmd_info {\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\n> Because this will have not problem with really weird query like: \"git\n> svn info /media/../media/something//src\"\n\nI've also started working on the following test cases,\nwill squash:\n\ndiff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh\nindex 4f6e669..f16f323 100755\n--- a/t/t9119-git-svn-info.sh\n+++ b/t/t9119-git-svn-info.sh\n@@ -84,6 +84,26 @@ test_expect_success '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"}]}