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, 13:33 UTC
Message-ID: <20180112133355.GE27499@sigill.intra.peff.net>
In-Reply-To: <20180112095607.18293-5-pclouds@gmail.com>

On Fri, Jan 12, 2018 at 04:56:07PM +0700, Nguyễn Thái Ngọc Duy wrote:

Show 8 quoted lines

> The previous concatenate_env() is kinda dumb. It receives a env delta
> in child_process and blindly follows it. If the run_command() user
> "adds" more vars of the same value, or deletes vars that do not exist
> in parent env in the first place (*), then why bother to print them?
> 
> This patch checks child_process.env against parent environment and
> only print the actual differences. The unset syntax (and later on cwd)
> follows closely shell syntax for easy reading/guessing and copy/paste.

I like it.

Show 11 quoted lines

> There is an interesting thing with this change. In the previous patch,
> we unconditionally print new vars. With submodule code we may have
> something like this
> 
>     trace: ... GIT_DIR='.git' git 'status' '--porcelain=2'
> 
> but since parent's GIT_DIR usually has the same value '.git' too, this
> change suppress that, now we can't see that the above command runs in
> a separate repo. This is the run for printing cwd. Now we have
> 
>     trace: ... cd foo; git 'status' '--porcelain=2'

Makes sense (though s/run/reason/ in the last paragraph?).

> Another equally interesting thing is, some caller can do "unset GIT_DIR;
> set GIT_DIR=.git". Since parent env can have the same value '.git', we
> don't re-print GIT_DIR=.git. In that case must NOT print "unset GIT_DIR"
> or the user will be misled to think the new command has no GIT_DIR.

Interesting. I wonder if any callers actually do that. It seems like kind of an odd thing, but maybe a caller sets GIT_DIR on top of the clearing of local_repo_env.

> A note about performance. Yes we're burning a lot more cycles for
> displaying something this nice. But because it's protected by
> $GIT_TRACE, and run_command() should not happen often anyway, it should
> not feel any slower than before.

I'd agree that performance probably doesn't matter much here.

Show 7 quoted lines

> +		/*
> +		 * Do we have a sequence of "unset GIT_DIR; GIT_DIR=foo"?
> +		 * Then don't bother with the unset thing.
> +		 */
> +		if (i + 1 < envs.nr &&
> +		    !strcmp(env, envs.items[i + 1].string))
>  			continue;

Are we guaranteed that "FOO" and "FOO=bar" appear next to each other in the sorted list? I think "FOO123=bar" could come between.

I also think this is a special case of a more general problem. FOO could appear any number of times in the "env" array, as a deletion or with multiple values. Our prep_childenv() would treat that as "last one wins", I think. Could we just do the same here?

-Peff