Re: [PATCH] run_command: teach API users to use embedded 'args' more
On Thu, Aug 27, 2020 at 12:31:52AM -0400, Eric Sunshine wrote:
Show 18 quoted lines
> On Thu, Aug 27, 2020 at 12:22 AM Jeff King <peff@peff.net> wrote:
> > I've actually considered dropping child_process.argv entirely. Having
> > two separate ways to do the same thing gives the potential for
> > confusion. [...]
> >
> > Likewise for child_process.env_array.
>
> builtin/worktree.c:add_worktree() is a case in which an environment
> strvec is built once and re-used for multiple run_command()
> invocations by re-assigning it to child_process.env before each
> run_command(). It uses child_process.env rather than
> child_process.env_array because run_command() clears
> child_process.env_array upon completion, which makes it difficult to
> reuse a prepared environment strvec repeatedly.
>
> Nevertheless, that isn't much of a reason to keep child_process.env.
> Refactoring add_worktree() a bit to rebuild the environment strvec
> on-demand wouldn't be very difficult.
I think they'd still be one-liner changes, like:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 378f332b5d..b40f0d4cea 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -422,7 +422,7 @@ static int add_worktree(const char *path, const char *refname,
strvec_push(&cp.args, "--quiet");
}
- cp.env = child_env.v;
+ strvec_pushv(&cp.env_array, child_env.v);
ret = run_command(&cp);
if (ret)
goto done;
@@ -433,7 +433,7 @@ static int add_worktree(const char *path, const char *refname,
strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
if (opts->quiet)
strvec_push(&cp.args, "--quiet");
- cp.env = child_env.v;
+ strvec_pushv(&cp.env_array, child_env.v);
ret = run_command(&cp);
if (ret)
goto done;
I think there are other opportunities for cleanup, too:
- the code right above the second hunk clears cp.args manually. That
shouldn't be necessary because run_command() will leave it in a
blank state (and we're already relying on that, since otherwise we'd
be left with cruft in other fields from the previous run).
- check_clean_worktree() only uses it once, and could drop the
separate child_env (and in fact appears to leak it)
-Peff
Jeff King <peff@peff.net>