Re: [PATCH] run_command: teach API users to use embedded 'args' more

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Aug 27, 2020 at 12:44 AM Jeff King <peff@peff.net> wrote:
> On Thu, Aug 27, 2020 at 12:31:52AM -0400, Eric Sunshine wrote:
> > 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:
>
> -       cp.env = child_env.v;
> +       strvec_pushv(&cp.env_array, child_env.v);

Nice and simple.

> I think there are other opportunities for cleanup, too:

True on both counts.

>   - 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).

Right. I wonder why the author of 7f44e3d1de (worktree: make setup of
new HEAD distinct from worktree population, 2015-07-17) chose to clear
cp.args manually like that.

>   - check_clean_worktree() only uses it once, and could drop the
>     separate child_env (and in fact appears to leak it)

Perhaps this unnecessary 'child_env' strvec was a copy/paste from
add_worktree()? But certainly cp.env_array would be simpler and avoid
the leak.