{"thread":{"id":"26436","subject":"[PATCH] perl: Fix command_bidi_pipe() don't care about repository path","startedAt":"2011-02-07T15:09:20Z","lastAt":"2011-02-14T23:29:43Z","messageCount":4,"participants":["Masatake Osanai","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"160611","messageId":"1297091360-11409-1-git-send-email-unpush@gmail.com","threadId":"26436","inReplyTo":null,"subject":"[PATCH] perl: Fix command_bidi_pipe() don't care about repository path","fromName":"Masatake Osanai","fromEmail":"unpush@gmail.com","sentAt":"2011-02-07T15:09:20Z","receivedAt":"2011-02-07T15:09:20Z","isPatch":true,"sender":{"key":"unpush@gmail.com","avatar":null},"body":"command_bidi_pipe must care about repo_path() in case of repository\ninstance.\nThis also fixes error on cat_blob() and hash_and_insert_object()\nin case of using outside of working tree.\n\nSigned-off-by: Masatake Osanai <unpush@gmail.com>\n---\n perl/Git.pm     |   25 ++++++++++++++++++++-----\n t/t9700/test.pl |   10 ++++++++++\n 2 files changed, 30 insertions(+), 5 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 205e48a..7b2aa46 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -99,7 +99,7 @@ increase notwithstanding).\n \n use Carp qw(carp croak); # but croak is bad - throw instead\n use Error qw(:try);\n-use Cwd qw(abs_path);\n+use Cwd qw(abs_path cwd);\n use IPC::Open2 qw(open2);\n use Fcntl qw(SEEK_SET SEEK_CUR);\n }\n@@ -396,7 +396,16 @@ See C<command_close_bidi_pipe()> for details.\n \n sub command_bidi_pipe {\n \tmy ($pid, $in, $out);\n+\tmy($self) = _maybe_self(@_);\n+\tlocal %ENV = %ENV;\n+\tmy $cwd_save = undef;\n+\tif ($self) {\n+\t\tshift;\n+\t\t$cwd_save = cwd();\n+\t\t_setup_git_cmd_env($self);\n+\t}\n \t$pid = open2($in, $out, 'git', @_);\n+\tchdir($cwd_save) if $cwd_save;\n \treturn ($pid, $in, $out, join(' ', @_));\n }\n \n@@ -843,7 +852,7 @@ sub _open_hash_and_insert_object_if_needed {\n \n \t($self->{hash_object_pid}, $self->{hash_object_in},\n \t $self->{hash_object_out}, $self->{hash_object_ctx}) =\n-\t\tcommand_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));\n+\t\t$self->command_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));\n }\n \n sub _close_hash_and_insert_object {\n@@ -932,7 +941,7 @@ sub _open_cat_blob_if_needed {\n \n \t($self->{cat_blob_pid}, $self->{cat_blob_in},\n \t $self->{cat_blob_out}, $self->{cat_blob_ctx}) =\n-\t\tcommand_bidi_pipe(qw(cat-file --batch));\n+\t\t$self->command_bidi_pipe(qw(cat-file --batch));\n }\n \n sub _close_cat_blob {\n@@ -1279,6 +1288,14 @@ sub _command_common_pipe {\n # for the given repository and execute the git command.\n sub _cmd_exec {\n \tmy ($self, @args) = @_;\n+\t_setup_git_cmd_env($self);\n+\t_execv_git_cmd(@args);\n+\tdie qq[exec \"@args\" failed: $!];\n+}\n+\n+# set up the appropriate state for git command\n+sub _setup_git_cmd_env {\n+\tmy $self = shift;\n \tif ($self) {\n \t\t$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();\n \t\t$self->repo_path() and $self->wc_path()\n@@ -1286,8 +1303,6 @@ sub _cmd_exec {\n \t\t$self->wc_path() and chdir($self->wc_path());\n \t\t$self->wc_subdir() and chdir($self->wc_subdir());\n \t}\n-\t_execv_git_cmd(@args);\n-\tdie qq[exec \"@args\" failed: $!];\n }\n \n # Execute the given Git command ($_[0]) with arguments ($_[1..])\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex c15ca2d..13ba96e 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -113,6 +113,16 @@ like($last_commit, qr/^[0-9a-fA-F]{40}$/, 'rev-parse returned hash');\n my $dir_commit = $r2->command_oneline('log', '-n1', '--pretty=format:%H', '.');\n isnt($last_commit, $dir_commit, 'log . does not show last commit');\n \n+# commands outside working tree\n+chdir($abs_repo_dir . '/..');\n+my $r3 = Git->repository(Directory => $abs_repo_dir);\n+my $tmpfile3 = \"$abs_repo_dir/file3.tmp\";\n+open TEMPFILE3, \"+>$tmpfile3\" or die \"Can't open $tmpfile3: $!\";\n+is($r3->cat_blob($file1hash, \\*TEMPFILE3), 15, \"cat_blob(outside): size\");\n+close TEMPFILE3;\n+unlink $tmpfile3;\n+chdir($abs_repo_dir);\n+\n printf \"1..%d\\n\", Test::More->builder->current_test;\n \n my $is_passing = eval { Test::More->is_passing };\n-- \n1.7.4\n"},{"id":"160783","messageId":"7v7hd8q5zi.fsf@alter.siamese.dyndns.org","threadId":"26436","inReplyTo":"1297091360-11409-1-git-send-email-unpush@gmail.com","subject":"Re: [PATCH] perl: Fix command_bidi_pipe() don't care about repository path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-09T20:59:29Z","receivedAt":"2011-02-09T20:59:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masatake Osanai <unpush@gmail.com> writes:\n\n> Subject: Re: [PATCH] perl: Fix command_bidi_pipe() don't care about repository path\n\nSorry but -ECANTPARSE.  It is unclear if you are saying \"the command does\nnot care, which is incorrect and it should\", or \"the command shouldn't\ncare, fix it as it does currently\".  It only turns out to be the former\nafter reading the body, but the subject must be understandable without\nreading the body to keep \"git shortlog\" readable.\n\n> command_bidi_pipe must care about repo_path() in case of repository\n> instance.\n\nBecause...?\n\n> This also fixes error on cat_blob() and hash_and_insert_object()\n> in case of using outside of working tree.\n\nMissing in your description are X and Y in: \"Earier these functions did X\nbut they should do Y instead; the patch makes them do so\".\n\n> @@ -396,7 +396,16 @@ See C<command_close_bidi_pipe()> for details.\n>  \n>  sub command_bidi_pipe {\n>  \tmy ($pid, $in, $out);\n> +\tmy($self) = _maybe_self(@_);\n\nNit; s/my/my /.\n\n> +\tlocal %ENV = %ENV;\n> +\tmy $cwd_save = undef;\n> +\tif ($self) {\n> +\t\tshift;\n> +\t\t$cwd_save = cwd();\n> +\t\t_setup_git_cmd_env($self);\n> +\t}\n\nThe POD description for this function says that it runs the same way as\ncommand_output_pipe, which in turn uses _command_common_pipe that is\nshared with command_input_pipe.  The reason these two other functions are\nOk without this patch is because _cmd_exec() has the logic to do the repo\ndependent set-up, as far as I can tell.  But command_bidi_pipe() does not\nuse _cmd_exec(), and does not have a corresponding logic, and that is what\nyou are trying to fix.\n\nAm I following your logic Ok so far?\n\nIt would have saved reviewers' time if you explained your patch a bit\nbetter, perhaps like...\n\n\tWhen command_input_pipe and command_output_pipe are used as a\n\tmethod of a Git::repository instance, they eventually call into\n\t_cmd_exec method that sets up the execution environment such as\n\tGIT_DIR, GIT_WORK_TREE environment variables and the current\n\tworking directory in the child process that interacts with the\n\trepository.\n\n        command_bidi_pipe however didn't expect to be called as such, and\n\tlacked all these set-up.  Because of this, a program that did this\n\tdid not work as expected:\n\n            my $repo = Git->repository(Directory => '/some/where/else');\n            my ($pid, $in, $out, $ctx) = \n            $repo->command_bidi_pipe(qw(hash-object -w --stdin-paths));\n\n\tThis patch refactors the _cmd_exec into _setup_git_cmd_env that\n\tsets up the execution environment, and makes _cmd_exec and\n\tcommand_bidi_pipe to use it.\n\n\tNote that unlike _cmd_exec that execv's a git command as an\n\texternal process, command_bidi_pipe is called from the main line\n\tof control, and the execution environment needs to be restored\n\tafter open2() does its magic.\n\nThanks.\n"},{"id":"161119","messageId":"1297721584-20572-1-git-send-email-unpush@gmail.com","threadId":"26436","inReplyTo":"7v7hd8q5zi.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] perl: command_bidi_pipe() should be set-up git environments","fromName":"Masatake Osanai","fromEmail":"unpush@gmail.com","sentAt":"2011-02-14T22:13:04Z","receivedAt":"2011-02-14T22:13:04Z","isPatch":true,"sender":{"key":"unpush@gmail.com","avatar":null},"body":"When command_input_pipe and command_output_pipe are used as a\nmethod of a Git::repository instance, they eventually call into\n_cmd_exec method that sets up the execution environment such as\nGIT_DIR, GIT_WORK_TREE environment variables and the current\nworking directory in the child process that interacts with the\nrepository.\n\ncommand_bidi_pipe however didn't expect to be called as such, and\nlacked all these set-up.  Because of this, a program that did this\ndid not work as expected:\n\n    my $repo = Git->repository(Directory => '/some/where/else');\n    my ($pid, $in, $out, $ctx) =\n    $repo->command_bidi_pipe(qw(hash-object -w --stdin-paths));\n\nThis patch refactors the _cmd_exec into _setup_git_cmd_env that\nsets up the execution environment, and makes _cmd_exec and\ncommand_bidi_pipe to use it.\n\nNote that unlike _cmd_exec that execv's a git command as an\nexternal process, command_bidi_pipe is called from the main line\nof control, and the execution environment needs to be restored\nafter open2() does its magic.\n\nSigned-off-by: Masatake Osanai <unpush@gmail.com>\n---\n\nOn Wed, Feb 09, 2011 at 12:59:29PM -0800, Junio C Hamano wrote:\n\n> Nit; s/my/my /.\n\nOops, It was careless.\n\n> The POD description for this function says that it runs the same way as\n> command_output_pipe, which in turn uses _command_common_pipe that is\n> shared with command_input_pipe.  The reason these two other functions are\n> Ok without this patch is because _cmd_exec() has the logic to do the repo\n> dependent set-up, as far as I can tell.  But command_bidi_pipe() does not\n> use _cmd_exec(), and does not have a corresponding logic, and that is what\n> you are trying to fix.\n> \n> Am I following your logic Ok so far?\n\nYou're absolutely correct. That's what I meant.\n\n> It would have saved reviewers' time if you explained your patch a bit\n> better, perhaps like...\n\nI'm sorry for lack of my explanation.\nThank you for your help and the lead.\n\nDescription of patch v2 is use entirely of your example because it is\nso perfect.  Does this way have the problem?\n\n perl/Git.pm     |   25 ++++++++++++++++++++-----\n t/t9700/test.pl |   10 ++++++++++\n 2 files changed, 30 insertions(+), 5 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 205e48a..a86ab70 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -99,7 +99,7 @@ increase notwithstanding).\n \n use Carp qw(carp croak); # but croak is bad - throw instead\n use Error qw(:try);\n-use Cwd qw(abs_path);\n+use Cwd qw(abs_path cwd);\n use IPC::Open2 qw(open2);\n use Fcntl qw(SEEK_SET SEEK_CUR);\n }\n@@ -396,7 +396,16 @@ See C<command_close_bidi_pipe()> for details.\n \n sub command_bidi_pipe {\n \tmy ($pid, $in, $out);\n+\tmy ($self) = _maybe_self(@_);\n+\tlocal %ENV = %ENV;\n+\tmy $cwd_save = undef;\n+\tif ($self) {\n+\t\tshift;\n+\t\t$cwd_save = cwd();\n+\t\t_setup_git_cmd_env($self);\n+\t}\n \t$pid = open2($in, $out, 'git', @_);\n+\tchdir($cwd_save) if $cwd_save;\n \treturn ($pid, $in, $out, join(' ', @_));\n }\n \n@@ -843,7 +852,7 @@ sub _open_hash_and_insert_object_if_needed {\n \n \t($self->{hash_object_pid}, $self->{hash_object_in},\n \t $self->{hash_object_out}, $self->{hash_object_ctx}) =\n-\t\tcommand_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));\n+\t\t$self->command_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));\n }\n \n sub _close_hash_and_insert_object {\n@@ -932,7 +941,7 @@ sub _open_cat_blob_if_needed {\n \n \t($self->{cat_blob_pid}, $self->{cat_blob_in},\n \t $self->{cat_blob_out}, $self->{cat_blob_ctx}) =\n-\t\tcommand_bidi_pipe(qw(cat-file --batch));\n+\t\t$self->command_bidi_pipe(qw(cat-file --batch));\n }\n \n sub _close_cat_blob {\n@@ -1279,6 +1288,14 @@ sub _command_common_pipe {\n # for the given repository and execute the git command.\n sub _cmd_exec {\n \tmy ($self, @args) = @_;\n+\t_setup_git_cmd_env($self);\n+\t_execv_git_cmd(@args);\n+\tdie qq[exec \"@args\" failed: $!];\n+}\n+\n+# set up the appropriate state for git command\n+sub _setup_git_cmd_env {\n+\tmy $self = shift;\n \tif ($self) {\n \t\t$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();\n \t\t$self->repo_path() and $self->wc_path()\n@@ -1286,8 +1303,6 @@ sub _cmd_exec {\n \t\t$self->wc_path() and chdir($self->wc_path());\n \t\t$self->wc_subdir() and chdir($self->wc_subdir());\n \t}\n-\t_execv_git_cmd(@args);\n-\tdie qq[exec \"@args\" failed: $!];\n }\n \n # Execute the given Git command ($_[0]) with arguments ($_[1..])\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex c15ca2d..13ba96e 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -113,6 +113,16 @@ like($last_commit, qr/^[0-9a-fA-F]{40}$/, 'rev-parse returned hash');\n my $dir_commit = $r2->command_oneline('log', '-n1', '--pretty=format:%H', '.');\n isnt($last_commit, $dir_commit, 'log . does not show last commit');\n \n+# commands outside working tree\n+chdir($abs_repo_dir . '/..');\n+my $r3 = Git->repository(Directory => $abs_repo_dir);\n+my $tmpfile3 = \"$abs_repo_dir/file3.tmp\";\n+open TEMPFILE3, \"+>$tmpfile3\" or die \"Can't open $tmpfile3: $!\";\n+is($r3->cat_blob($file1hash, \\*TEMPFILE3), 15, \"cat_blob(outside): size\");\n+close TEMPFILE3;\n+unlink $tmpfile3;\n+chdir($abs_repo_dir);\n+\n printf \"1..%d\\n\", Test::More->builder->current_test;\n \n my $is_passing = eval { Test::More->is_passing };\n-- \n1.7.4\n"},{"id":"161131","messageId":"7vk4h2qjo8.fsf@alter.siamese.dyndns.org","threadId":"26436","inReplyTo":"1297721584-20572-1-git-send-email-unpush@gmail.com","subject":"Re: [PATCH v2] perl: command_bidi_pipe() should be set-up git environments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-14T23:29:43Z","receivedAt":"2011-02-14T23:29:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks for the patch and the sanity checking of my reading of it.\nWill queue.\n"}]}