On Tue, Nov 23, 2021 at 7:08 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
> The clearing of "argv" was added in 7f44e3d1de0 (worktree: make setup
> of new HEAD distinct from worktree population, 2015-07-17) when the
> "cp" variable wasn't initialized. It hasn't been needed since
> 542aa25d974 (use CHILD_PROCESS_INIT to initialize automatic variables,
> 2016-08-05).
>
> Let's remove it to make a later change that gets rid of the "argv"
> member from "struct child_process" smaller.
Let me preface with the caveat that (due to lack of time) I haven't
dug into this deeply and I haven't been following changes to the
run_command() machinery closely, so what I say below may be
inaccurate, but...
Although it will be safe to drop these builtin/worktree.c assignments
when the series eventually removes the child_process::argv member, I
think that the commit message is misleading (or outright wrong), and
that this change so early in the series potentially breaks
git-worktree, leaving it in a state where it works only "by accident".
The reason that this code repeatedly clears `cp.argv` is that the `cp`
structure is reused for multiple run_command() invocations (which, in
retrospect, is pretty ugly since worktree.c is too familiar with the
internals of run-command). The reason that `cp.argv` needs to be
cleared between each invocation is due to this code from
start_command():
if (!cmd->argv)
cmd->argv = cmd->args.v;
worktree.c re-populates child_process::args between run_command()
invocations and needs to clear child_process::argv to ensure that the
latter gets re-initialized from child_process::args each time. The use
of CHILD_PROCESS_INIT does not change anything in regard to the
requirement; child_process::argv still needs to be cleared between
run_command() invocations.
As to why this change so early in the series potentially breaks
git-worktree: with the removal of these assignments,
child_process::argv now _always_ points at the _initial_ value of
child_process::args.v even though that vector gets cleared between
run_command() invocations. At best, following this change,
git-worktree is only working "by accident" if the underlying
child_process::args.v doesn't get reallocated between run_command()
invocations. Relying upon this "by accident" behavior feels rather
unsafe.
I think perhaps the simplest thing to do is merely to squash this
patch into the patch which ultimately removes the child_process::argv
member (and the removal of these lines from worktree.c probably
doesn't even need mention in the commit message -- or maybe just a
minor mention).
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index d22ece93e1a..7264a5b5de0 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -355,7 +355,6 @@ static int add_worktree(const char *path, const char *refname,
> 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)
> @@ -390,7 +389,6 @@ static int add_worktree(const char *path, const char *refname,
> cp.stdout_to_stderr = 1;
> cp.dir = path;
> cp.env = env;
> - cp.argv = NULL;
> cp.trace2_hook_name = "post-checkout";
> strvec_pushl(&cp.args, absolute_path(hook),
> oid_to_hex(null_oid()),
> --
> 2.34.0.831.gd33babec0d1
