On Sun, Nov 21, 2021 at 06:10:08PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > We'd usually leave of "Reviewed-by" until the reviewer has had a chance
> > to see _this_ version of the patch. I.e., usually it would not be added
> > by the submitter, but by the maintainer (unless you are resending
> > verbatim a patch that already got review).
> >
> >> diff --git a/run-command.c b/run-command.c
> >> index f329391154ae..a7bf81025afb 100644
> >> --- a/run-command.c
> >> +++ b/run-command.c
> >> @@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child)
> >> {
> >> strvec_clear(&child->args);
> >> strvec_clear(&child->env_array);
> >> + child_process_init(child);
> >> }
> >
> > And naturally I agree that the patch itself looks good. :)
>
> Well, not to me X-<. This is way too aggressive a change to be made
> lightly without auditing the current users of run_command API.
Yikes. Thanks for a dose of sanity. I was looking too much at just the
pager tests.
> It is rather common for us to reuse "struct child_process" in a code
> path, e.g. builtin/worktree.c::add_worktree() prepares a single
> instance of such a struct, sets cp.git_cmd to true, and runs either
> "update-ref" or "symbolic-ref" to update "HEAD". After successful
> invocation of such a git subcommand, it then runs "git reset --hard",
> with this piece of code:
>
> cp.git_cmd = 1;
>
> if (!is_branch)
> strvec_pushl(&cp.args, "update-ref", "HEAD",
> oid_to_hex(&commit->object.oid), NULL);
> else {
> strvec_pushl(&cp.args, "symbolic-ref", "HEAD",
> symref.buf, NULL);
> if (opts->quiet)
> strvec_push(&cp.args, "--quiet");
> }
>
> cp.env = child_env.v;
> ret = run_command(&cp);
> if (ret)
> goto done;
>
> if (opts->checkout) {
> cp.argv = NULL;
> strvec_clear(&cp.args);
> strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
> if (opts->quiet)
> strvec_push(&cp.args, "--quiet");
> cp.env = child_env.v;
> ret = run_command(&cp);
> if (ret)
> goto done;
> }
This is a pretty horrid interface, in that the caller has to understand
which bits of "cp" need to be adjusted: setting cp.argv to NULL, but
also potentially cp.env (if cp.env_array was used), and clearing any
stdin/out/err descriptors created as pipes in the previous command. And
probably more; that's just off the top of my head.
But clearly there's a bunch of code relying on the current state of
affairs.
> Now, we could argue that this existing caller is too lazy to assume
> that cp.git_cmd bit will be retained after run_command()
> successfully finishes and can reuse the structure without setting
> the bit again, and it should be more defensive. And "successful
> run_command() clears the child process so that you'll get a clean
> slate" may even be a better API in the longer term.
>
> But then a change like this one that changes the world order to make
> it a better place is also responsible to ensure that the callers are
> already following the new world order.
Yep. But I do worry a bit about changing the interface in such a subtle
way, as nothing would catch topics in flight.
> When merged to 'seen', this literally destroys tons of tests (the
> first and easiest one to observe may be t0002).
Forget 'seen'. Applying it on master shows plenty of breakages. :)
I think we should probably punt on this direction, and just make sure
that setup_pager() either reinitializes the child_process as appropriate
(as in the patch I showed in the earlier thread) or just refuses to try
running the pager twice (I didn't show a patch, but it should just be a
matter of setting a static flag).
-Peff
