Masatake Osanai <unpush@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html