[PATCH] perl: Fix command_bidi_pipe() don't care about repository path

[PATCH] perl: Fix command_bidi_pipe() don't care about repository path

[Masatake Osanai]

command_bidi_pipe must care about repo_path() in case of repository
instance.
This also fixes error on cat_blob() and hash_and_insert_object()
in case of using outside of working tree.

Signed-off-by: Masatake Osanai <unpush@gmail.com>
---
 perl/Git.pm     |   25 ++++++++++++++++++++-----
 t/t9700/test.pl |   10 ++++++++++
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 205e48a..7b2aa46 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -99,7 +99,7 @@ increase notwithstanding).
 
 use Carp qw(carp croak); # but croak is bad - throw instead
 use Error qw(:try);
-use Cwd qw(abs_path);
+use Cwd qw(abs_path cwd);
 use IPC::Open2 qw(open2);
 use Fcntl qw(SEEK_SET SEEK_CUR);
 }
@@ -396,7 +396,16 @@ See C<command_close_bidi_pipe()> for details.
 
 sub command_bidi_pipe {
 	my ($pid, $in, $out);
+	my($self) = _maybe_self(@_);
+	local %ENV = %ENV;
+	my $cwd_save = undef;
+	if ($self) {
+		shift;
+		$cwd_save = cwd();
+		_setup_git_cmd_env($self);
+	}
 	$pid = open2($in, $out, 'git', @_);
+	chdir($cwd_save) if $cwd_save;
 	return ($pid, $in, $out, join(' ', @_));
 }
 
@@ -843,7 +852,7 @@ sub _open_hash_and_insert_object_if_needed {
 
 	($self->{hash_object_pid}, $self->{hash_object_in},
 	 $self->{hash_object_out}, $self->{hash_object_ctx}) =
-		command_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));
+		$self->command_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));
 }
 
 sub _close_hash_and_insert_object {
@@ -932,7 +941,7 @@ sub _open_cat_blob_if_needed {
 
 	($self->{cat_blob_pid}, $self->{cat_blob_in},
 	 $self->{cat_blob_out}, $self->{cat_blob_ctx}) =
-		command_bidi_pipe(qw(cat-file --batch));
+		$self->command_bidi_pipe(qw(cat-file --batch));
 }
 
 sub _close_cat_blob {
@@ -1279,6 +1288,14 @@ sub _command_common_pipe {
 # for the given repository and execute the git command.
 sub _cmd_exec {
 	my ($self, @args) = @_;
+	_setup_git_cmd_env($self);
+	_execv_git_cmd(@args);
+	die qq[exec "@args" failed: $!];
+}
+
+# set up the appropriate state for git command
+sub _setup_git_cmd_env {
+	my $self = shift;
 	if ($self) {
 		$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();
 		$self->repo_path() and $self->wc_path()
@@ -1286,8 +1303,6 @@ sub _cmd_exec {
 		$self->wc_path() and chdir($self->wc_path());
 		$self->wc_subdir() and chdir($self->wc_subdir());
 	}
-	_execv_git_cmd(@args);
-	die qq[exec "@args" failed: $!];
 }
 
 # Execute the given Git command ($_[0]) with arguments ($_[1..])
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index c15ca2d..13ba96e 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -113,6 +113,16 @@ like($last_commit, qr/^[0-9a-fA-F]{40}$/, 'rev-parse returned hash');
 my $dir_commit = $r2->command_oneline('log', '-n1', '--pretty=format:%H', '.');
 isnt($last_commit, $dir_commit, 'log . does not show last commit');
 
+# commands outside working tree
+chdir($abs_repo_dir . '/..');
+my $r3 = Git->repository(Directory => $abs_repo_dir);
+my $tmpfile3 = "$abs_repo_dir/file3.tmp";
+open TEMPFILE3, "+>$tmpfile3" or die "Can't open $tmpfile3: $!";
+is($r3->cat_blob($file1hash, \*TEMPFILE3), 15, "cat_blob(outside): size");
+close TEMPFILE3;
+unlink $tmpfile3;
+chdir($abs_repo_dir);
+
 printf "1..%d\n", Test::More->builder->current_test;
 
 my $is_passing = eval { Test::More->is_passing };
-- 
1.7.4

Re: [PATCH] perl: Fix command_bidi_pipe() don't care about repository path

[Junio C Hamano]

Masatake Osanai <unpush@gmail.com> writes:

> Subject: Re: [PATCH] perl: Fix command_bidi_pipe() don't care about repository path

Sorry but -ECANTPARSE.  It is unclear if you are saying "the command does
not care, which is incorrect and it should", or "the command shouldn't
care, fix it as it does currently".  It only turns out to be the former
after reading the body, but the subject must be understandable without
reading the body to keep "git shortlog" readable.

> command_bidi_pipe must care about repo_path() in case of repository
> instance.

Because...?

> This also fixes error on cat_blob() and hash_and_insert_object()
> in case of using outside of working tree.

Missing in your description are X and Y in: "Earier these functions did X
but they should do Y instead; the patch makes them do so".

> @@ -396,7 +396,16 @@ See C<command_close_bidi_pipe()> for details.
>  
>  sub command_bidi_pipe {
>  	my ($pid, $in, $out);
> +	my($self) = _maybe_self(@_);

Nit; s/my/my /.

> +	local %ENV = %ENV;
> +	my $cwd_save = undef;
> +	if ($self) {
> +		shift;
> +		$cwd_save = cwd();
> +		_setup_git_cmd_env($self);
> +	}

The POD description for this function says that it runs the same way as
command_output_pipe, which in turn uses _command_common_pipe that is
shared with command_input_pipe.  The reason these two other functions are
Ok without this patch is because _cmd_exec() has the logic to do the repo
dependent set-up, as far as I can tell.  But command_bidi_pipe() does not
use _cmd_exec(), and does not have a corresponding logic, and that is what
you are trying to fix.

Am I following your logic Ok so far?

It would have saved reviewers' time if you explained your patch a bit
better, perhaps like...

	When command_input_pipe and command_output_pipe are used as a
	method of a Git::repository instance, they eventually call into
	_cmd_exec method that sets up the execution environment such as
	GIT_DIR, GIT_WORK_TREE environment variables and the current
	working directory in the child process that interacts with the
	repository.

        command_bidi_pipe however didn't expect to be called as such, and
	lacked all these set-up.  Because of this, a program that did this
	did not work as expected:

            my $repo = Git->repository(Directory => '/some/where/else');
            my ($pid, $in, $out, $ctx) = 
            $repo->command_bidi_pipe(qw(hash-object -w --stdin-paths));

	This patch refactors the _cmd_exec into _setup_git_cmd_env that
	sets up the execution environment, and makes _cmd_exec and
	command_bidi_pipe to use it.

	Note that unlike _cmd_exec that execv's a git command as an
	external process, command_bidi_pipe is called from the main line
	of control, and the execution environment needs to be restored
	after open2() does its magic.

Thanks.

[PATCH v2] perl: command_bidi_pipe() should be set-up git environments

[Masatake Osanai]

When command_input_pipe and command_output_pipe are used as a
method of a Git::repository instance, they eventually call into
_cmd_exec method that sets up the execution environment such as
GIT_DIR, GIT_WORK_TREE environment variables and the current
working directory in the child process that interacts with the
repository.

command_bidi_pipe however didn't expect to be called as such, and
lacked all these set-up.  Because of this, a program that did this
did not work as expected:

    my $repo = Git->repository(Directory => '/some/where/else');
    my ($pid, $in, $out, $ctx) =
    $repo->command_bidi_pipe(qw(hash-object -w --stdin-paths));

This patch refactors the _cmd_exec into _setup_git_cmd_env that
sets up the execution environment, and makes _cmd_exec and
command_bidi_pipe to use it.

Note that unlike _cmd_exec that execv's a git command as an
external process, command_bidi_pipe is called from the main line
of control, and the execution environment needs to be restored
after open2() does its magic.

Signed-off-by: Masatake Osanai <unpush@gmail.com>
---

On Wed, Feb 09, 2011 at 12:59:29PM -0800, Junio C Hamano wrote:

> Nit; s/my/my /.

Oops, It was careless.

> The POD description for this function says that it runs the same way as
> command_output_pipe, which in turn uses _command_common_pipe that is
> shared with command_input_pipe.  The reason these two other functions are
> Ok without this patch is because _cmd_exec() has the logic to do the repo
> dependent set-up, as far as I can tell.  But command_bidi_pipe() does not
> use _cmd_exec(), and does not have a corresponding logic, and that is what
> you are trying to fix.
> 
> Am I following your logic Ok so far?

You're absolutely correct. That's what I meant.

> It would have saved reviewers' time if you explained your patch a bit
> better, perhaps like...

I'm sorry for lack of my explanation.
Thank you for your help and the lead.

Description of patch v2 is use entirely of your example because it is
so perfect.  Does this way have the problem?

 perl/Git.pm     |   25 ++++++++++++++++++++-----
 t/t9700/test.pl |   10 ++++++++++
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 205e48a..a86ab70 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -99,7 +99,7 @@ increase notwithstanding).
 
 use Carp qw(carp croak); # but croak is bad - throw instead
 use Error qw(:try);
-use Cwd qw(abs_path);
+use Cwd qw(abs_path cwd);
 use IPC::Open2 qw(open2);
 use Fcntl qw(SEEK_SET SEEK_CUR);
 }
@@ -396,7 +396,16 @@ See C<command_close_bidi_pipe()> for details.
 
 sub command_bidi_pipe {
 	my ($pid, $in, $out);
+	my ($self) = _maybe_self(@_);
+	local %ENV = %ENV;
+	my $cwd_save = undef;
+	if ($self) {
+		shift;
+		$cwd_save = cwd();
+		_setup_git_cmd_env($self);
+	}
 	$pid = open2($in, $out, 'git', @_);
+	chdir($cwd_save) if $cwd_save;
 	return ($pid, $in, $out, join(' ', @_));
 }
 
@@ -843,7 +852,7 @@ sub _open_hash_and_insert_object_if_needed {
 
 	($self->{hash_object_pid}, $self->{hash_object_in},
 	 $self->{hash_object_out}, $self->{hash_object_ctx}) =
-		command_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));
+		$self->command_bidi_pipe(qw(hash-object -w --stdin-paths --no-filters));
 }
 
 sub _close_hash_and_insert_object {
@@ -932,7 +941,7 @@ sub _open_cat_blob_if_needed {
 
 	($self->{cat_blob_pid}, $self->{cat_blob_in},
 	 $self->{cat_blob_out}, $self->{cat_blob_ctx}) =
-		command_bidi_pipe(qw(cat-file --batch));
+		$self->command_bidi_pipe(qw(cat-file --batch));
 }
 
 sub _close_cat_blob {
@@ -1279,6 +1288,14 @@ sub _command_common_pipe {
 # for the given repository and execute the git command.
 sub _cmd_exec {
 	my ($self, @args) = @_;
+	_setup_git_cmd_env($self);
+	_execv_git_cmd(@args);
+	die qq[exec "@args" failed: $!];
+}
+
+# set up the appropriate state for git command
+sub _setup_git_cmd_env {
+	my $self = shift;
 	if ($self) {
 		$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();
 		$self->repo_path() and $self->wc_path()
@@ -1286,8 +1303,6 @@ sub _cmd_exec {
 		$self->wc_path() and chdir($self->wc_path());
 		$self->wc_subdir() and chdir($self->wc_subdir());
 	}
-	_execv_git_cmd(@args);
-	die qq[exec "@args" failed: $!];
 }
 
 # Execute the given Git command ($_[0]) with arguments ($_[1..])
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index c15ca2d..13ba96e 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -113,6 +113,16 @@ like($last_commit, qr/^[0-9a-fA-F]{40}$/, 'rev-parse returned hash');
 my $dir_commit = $r2->command_oneline('log', '-n1', '--pretty=format:%H', '.');
 isnt($last_commit, $dir_commit, 'log . does not show last commit');
 
+# commands outside working tree
+chdir($abs_repo_dir . '/..');
+my $r3 = Git->repository(Directory => $abs_repo_dir);
+my $tmpfile3 = "$abs_repo_dir/file3.tmp";
+open TEMPFILE3, "+>$tmpfile3" or die "Can't open $tmpfile3: $!";
+is($r3->cat_blob($file1hash, \*TEMPFILE3), 15, "cat_blob(outside): size");
+close TEMPFILE3;
+unlink $tmpfile3;
+chdir($abs_repo_dir);
+
 printf "1..%d\n", Test::More->builder->current_test;
 
 my $is_passing = eval { Test::More->is_passing };
-- 
1.7.4

Re: [PATCH v2] perl: command_bidi_pipe() should be set-up git environments

[Junio C Hamano]

Thanks for the patch and the sanity checking of my reading of it.
Will queue.