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