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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2011, 20:59 UTC
Message-ID: <7v7hd8q5zi.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1297091360-11409-1-git-send-email-unpush@gmail.com>

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".

Show 5 quoted lines

> @@ -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 /.

Show 7 quoted lines

> +	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.