Re: [PATCH v2] run-command.c: print env vars when GIT_TRACE is set

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2018, 19:27 UTC
Message-ID: <xmqq4lnsyz4l.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20180111094712.2551-1-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 7 quoted lines

> Occasionally submodule code could execute new commands with GIT_DIR set
> to some submodule. GIT_TRACE prints just the command line which makes it
> hard to tell that it's not really executed on this repository.
>
> Print modified env variables (compared to parent environment) in this
> case. Actually only modified or new variables are printed. Variable
> deletion is suppressed because ...

When I read the above, I imagined that you are reducing noise from the output by showing only modified or new variables, but it appears that the implementation of concatenate_env() just blindly copies the variables without checking if they are setting the same value.

I wonder how common it would be to attempt exporting a variable with the same value, and to attempt unsetting a variable that is not exported, which might help you reduce the clutter by hiding stuff that truly do not matter, while keeping what matters (possibly including the "unset" part).

Show 9 quoted lines

> +static void concatenate_env(struct strbuf *dst, const char *const *env)
> +{
> +	int i;
> +
> +	/* Copy into destination buffer. */
> +	strbuf_grow(dst, 255);
> +	for (i = 0; env[i]; ++i) {
> +		/*
> +		 * the main interesting is setting new vars

I'll do s/interesting/& part/ while queuing.

Show 6 quoted lines

> +		 * e.g. GIT_DIR, ignore the unsetting to reduce noise.
> +		 */
> +		if (!strchr(env[i], '='))
> +			continue;
> +		strbuf_addch(dst, ' ');
> +		sq_quote_buf(dst, env[i]);

I think you'd tweak the quoting around here in a later iteration, based on the distinction between the two:

	$ 'GIT_DIR=.git' git foo
	$ GIT_DIR='.git' git foo

that was pointed out in a near-by message.

Thanks.