Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()
- Date: Fri, 12 Jan 2018 08:33:55 -0500
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()
On Fri, Jan 12, 2018 at 04:56:07PM +0700, Nguyễn Thái Ngọc Duy wrote:
> 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.
> 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.
> + /*
> + * 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))
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?