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.
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;
}
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.
When merged to 'seen', this literally destroys tons of tests (the
first and easiest one to observe may be t0002).
Thanks.
