From: Eric Sunshine Date: Thu, 27 Aug 2020 05:03:43 GMT Subject: Re: [PATCH] run_command: teach API users to use embedded 'args' more Message-ID: In-Reply-To: <20200827044420.GA3360616@coredump.intra.peff.net> On Thu, Aug 27, 2020 at 12:44 AM Jeff King 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.