{"thread":{"id":"37156","subject":"[PATCH] git-svn: doublecheck if really file or dir","startedAt":"2014-07-18T04:20:01Z","lastAt":"2014-08-03T12:22:34Z","messageCount":5,"participants":["Andrej Manduch","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"246285","messageId":"1405657201-32035-1-git-send-email-amanduch@gmail.com","threadId":"37156","inReplyTo":null,"subject":"[PATCH] git-svn: doublecheck if really file or dir","fromName":"Andrej Manduch","fromEmail":"amanduch@gmail.com","sentAt":"2014-07-18T04:20:01Z","receivedAt":"2014-07-18T04:20:01Z","isPatch":true,"sender":{"key":"amanduch@gmail.com","avatar":null},"body":"* this fixes 'git svn info `pwd`' buggy behaviour\n\nSigned-off-by: Andrej Manduch <amanduch@gmail.com>\n---\n git-svn.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 0a32372..5bbfecf 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -2029,7 +2029,7 @@ sub find_file_type_and_diff_status {\n \tmy $mode = (split(' ', $ls_tree))[0] || \"\";\n \n \treturn (\"link\", $diff_status) if $mode eq \"120000\";\n-\treturn (\"dir\", $diff_status) if $mode eq \"040000\";\n+\treturn (\"dir\", $diff_status) if $mode eq \"040000\" or -d $path;\n \treturn (\"file\", $diff_status);\n }\n \n-- \n2.0.0.GIT\n"},{"id":"246615","messageId":"20140723220414.GA14145@dcvr.yhbt.net","threadId":"37156","inReplyTo":"1405657201-32035-1-git-send-email-amanduch@gmail.com","subject":"Re: [PATCH] git-svn: doublecheck if really file or dir","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2014-07-23T22:04:14Z","receivedAt":"2014-07-23T22:04:14Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Andrej Manduch <amanduch@gmail.com> wrote:\n> * this fixes 'git svn info `pwd`' buggy behaviour\n\nGood catch, the commit could use a better description, something like:\n--------------------------- 8< ----------------------------\nSubject: [PATCH] git-svn: \"info\" checks for dirs more carefully\n\nThis avoids a \"Reading from filehandle failed at ...\" error when\nrunning \"git svn info `pwd`\".\n\nSigned-off-by: Andrej Manduch <amanduch@gmail.com>\n--------------------------- 8< ----------------------------\n\nWhile your patch avoids an error, but the output isn't right, either.\nI tried it running in /home/ew/ruby, the URL field is bogus:\n\n    ~/ruby$ git svn info `pwd`\n    Path: /home/ew/ruby\n    URL: svn+ssh://svn.ruby-lang.org/ruby/trunk/home/ew/ruby\n    Repository Root: svn+ssh://svn.ruby-lang.org/ruby\n    Repository UUID: b2dd03c8-39d4-4d8f-98ff-823fe69b080e\n    Revision: 46901\n    Node Kind: directory\n    Schedule: normal\n    Last Changed Author: hsbt\n    Last Changed Rev: 46901\n    Last Changed Date: 2014-07-22 19:06:12 +0000 (Tue, 22 Jul 2014)\n\nThe URL should be:\n\n    URL: svn+ssh://svn.ruby-lang.org/ruby/trunk\n\nIt's better than an error, but it'd be nice if someone who uses\nthis command can fix it (*hint* :).\n\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -2029,7 +2029,7 @@ sub find_file_type_and_diff_status {\n>  \tmy $mode = (split(' ', $ls_tree))[0] || \"\";\n>  \n>  \treturn (\"link\", $diff_status) if $mode eq \"120000\";\n> -\treturn (\"dir\", $diff_status) if $mode eq \"040000\";\n> +\treturn (\"dir\", $diff_status) if $mode eq \"040000\" or -d $path;\n\n\"or\" has a lower precedence than \"||\", so I would do the following:\n\n\treturn (\"dir\", $diff_status) if $mode eq \"040000\" || -d $path;\n\nThe general rule I've learned is to use \"||\" for conditionals and\n\"or\" for control flow (e.g. do_something() or die(\"...\") ).\n\nI can take your patch with the above changes (no need to resend),\nbut I'd be happier to see the URL field corrected if you want\nto reroll.\n\nThanks.\n"},{"id":"246761","messageId":"53D46812.3020706@gmail.com","threadId":"37156","inReplyTo":"20140723220414.GA14145@dcvr.yhbt.net","subject":"Re: [PATCH] git-svn: doublecheck if really file or dir","fromName":"Andrej Manduch","fromEmail":"amanduch@gmail.com","sentAt":"2014-07-27T02:46:42Z","receivedAt":"2014-07-27T02:46:42Z","isPatch":true,"sender":{"key":"amanduch@gmail.com","avatar":null},"body":"Hi,\n\nOn 07/24/2014 12:04 AM, Eric Wong wrote:\n> Andrej Manduch <amanduch@gmail.com> wrote:\n>> * this fixes 'git svn info `pwd`' buggy behaviour\n> \n> Good catch, the commit could use a better description, something like:\n> --------------------------- 8< ----------------------------\n> Subject: [PATCH] git-svn: \"info\" checks for dirs more carefully\n> \n> This avoids a \"Reading from filehandle failed at ...\" error when\n> running \"git svn info `pwd`\".\n> \n> Signed-off-by: Andrej Manduch <amanduch@gmail.com>\n> --------------------------- 8< ----------------------------\n> \n> While your patch avoids an error, but the output isn't right, either.\n> I tried it running in /home/ew/ruby, the URL field is bogus:\n> \n>     ~/ruby$ git svn info `pwd`\n>     Path: /home/ew/ruby\n>     URL: svn+ssh://svn.ruby-lang.org/ruby/trunk/home/ew/ruby\n>     Repository Root: svn+ssh://svn.ruby-lang.org/ruby\n>     Repository UUID: b2dd03c8-39d4-4d8f-98ff-823fe69b080e\n>     Revision: 46901\n>     Node Kind: directory\n>     Schedule: normal\n>     Last Changed Author: hsbt\n>     Last Changed Rev: 46901\n>     Last Changed Date: 2014-07-22 19:06:12 +0000 (Tue, 22 Jul 2014)\n> \n> The URL should be:\n> \n>     URL: svn+ssh://svn.ruby-lang.org/ruby/trunk\n> \n> It's better than an error, but it'd be nice if someone who uses\n> this command can fix it (*hint* :).\n\nThx, I missed this. However this bug was not introduced with my patch,\nit was there before. If you try use `git svn info full_path` and\ndirectory is not a root dir this bug will occour even wihout my patch.\n\nHowever I'll try to find some time to fix this too.\n\nOn 07/24/2014 12:04 AM, Eric Wong wrote:\n> \n>> --- a/git-svn.perl\n>> +++ b/git-svn.perl\n>> @@ -2029,7 +2029,7 @@ sub find_file_type_and_diff_status {\n>>  \tmy $mode = (split(' ', $ls_tree))[0] || \"\";\n>>  \n>>  \treturn (\"link\", $diff_status) if $mode eq \"120000\";\n>> -\treturn (\"dir\", $diff_status) if $mode eq \"040000\";\n>> +\treturn (\"dir\", $diff_status) if $mode eq \"040000\" or -d $path;\n> \n> \"or\" has a lower precedence than \"||\", so I would do the following:\n> \n> \treturn (\"dir\", $diff_status) if $mode eq \"040000\" || -d $path;\n> \n> The general rule I've learned is to use \"||\" for conditionals and\n> \"or\" for control flow (e.g. do_something() or die(\"...\") ).\n> \n> I can take your patch with the above changes (no need to resend),\n> but I'd be happier to see the URL field corrected if you want\n> to reroll.\n\nI'll try to fix whis url bug, but it will be different patch 'cause I\nthink, this is different kind of a problem.\n\nOn 07/24/2014 12:04 AM, Eric Wong wrote:\n> \n> Thanks.\n> \n\nI thanks to you for great review.\n\n--\nBest Regards,\nb.\n"},{"id":"247188","messageId":"20140803024520.GA12880@dcvr.yhbt.net","threadId":"37156","inReplyTo":"53D46812.3020706@gmail.com","subject":"Re: [PATCH] git-svn: doublecheck if really file or dir","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2014-08-03T02:45:20Z","receivedAt":"2014-08-03T02:45:20Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Andrej Manduch <amanduch@gmail.com> wrote:\n> On 07/24/2014 12:04 AM, Eric Wong wrote:\n> > Andrej Manduch <amanduch@gmail.com> wrote:\n> >> * this fixes 'git svn info `pwd`' buggy behaviour\n> > \n> > While your patch avoids an error, but the output isn't right, either.\n> > I tried it running in /home/ew/ruby, the URL field is bogus:\n\n> Thx, I missed this. However this bug was not introduced with my patch,\n> it was there before. If you try use `git svn info full_path` and\n> directory is not a root dir this bug will occour even wihout my patch.\n\nHi Andrej, I could not help thinking your patch was obscuring\nanother bug.  I think I have an alternative to your patch which\nfixes both our bugs.  Can you give this a shot?  Thanks.\n\n--------------------------- 8< ----------------------------\nSubject: [PATCH] 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\nNoticed-by: Andrej Manduch <amanduch@gmail.com>\nSigned-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\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 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+\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>) {\ndiff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh\nindex 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-- \nEW\n"},{"id":"247193","messageId":"53DE298A.9090505@gmail.com","threadId":"37156","inReplyTo":"20140803024520.GA12880@dcvr.yhbt.net","subject":"Re: [PATCH] git-svn: doublecheck if really file or dir","fromName":"Andrej Manduch","fromEmail":"amanduch@gmail.com","sentAt":"2014-08-03T12:22:34Z","receivedAt":"2014-08-03T12:22:34Z","isPatch":true,"sender":{"key":"amanduch@gmail.com","avatar":null},"body":"Hi Eric,\n\nNice touch, It works like charm. However unfortunatelly now I think you\nintroduced new bug :)\n\nOn 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/?!!;\nI have problem with this line ^^^\n\nSuppose your $toplevel is \"/sometning\" and you type in command line\nsomething like that: \"git svn info /somethingsrc\" and as you see this\nshould end up with error. However \"$path =~ s!\\A\\Q$toplevel\\E/?!!;\"\nwill just cut \"/sometning\" from \"/somethingsrc\" and and up with same\nanswer as for \"svn git info src\" which is not equivalent query.\n\nSecond scenario is something which worries me more: If your query look\nlike this: \"git svn info /something//src\" it will just end up with error\nbecause it will set $path to \"/src\" witch is outside of repository.\n\nSecond scenario can be fixed with this:\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 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\nHowever I'm not sure if this will work on windows (where slashes are in\ndifferent orientation).\n\n\nOn 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"}]}