Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()

From: Jeff King <peff@peff.net>
Date: Jan 12, 2018, 19:23 UTC
Message-ID: <20180112192314.GA19004@sigill.intra.peff.net>
In-Reply-To: <xmqqlgh2x4tr.fsf@gitster.mtv.corp.google.com>

On Fri, Jan 12, 2018 at 11:19:44AM -0800, Junio C Hamano wrote:

Show 23 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > The actual prep_childenv() code looks like it would always do
> > last-one-wins, so we should treat FOO as unset in that final case. But
> > that only kicks in on non-Windows.
> >
> > On Windows we feed cmd->env straight to mingw_spawnvpe().  That ends up
> > in make_environment_block(), which looks like it does something similar.
> >
> > It's too bad the prep code is not shared, since then we could probably
> > just ask _it_ which deltas it applied.
> 
> Yeah, but that function does a lot more than computing delta.  
> 
> It's primary point, which comes from ae25394b ("run-command: prepare
> child environment before forking", 2017-04-19), is to create a full
> copy of the environment, not just a series of putenv/unsetenv that
> describes what gets changed, and that is done to avoid any
> allocation after fork before exec in the child process.
> 
> I guess prep_childenv() could take an extra and optional string-list
> to report what "deltas" it applied to the tracing machinery.  I am
> not sure if that is worth it, though.

Yes, that's exactly what I meant. I think if it covered all platforms it might be worth it (not so much for code de-duping, but because then we would know our display logic always matched what we exec'd). But unless somebody wants to refactor all of the Windows spawn code, it's definitely not a good idea.

-Peff