Re: [PATCH v3 3/4] trace.c: print env vars in trace_run_command()

From: Jeff King <peff@peff.net>
Date: Jan 12, 2018, 13:13 UTC
Message-ID: <20180112131301.GD27499@sigill.intra.peff.net>
In-Reply-To: <20180112095607.18293-4-pclouds@gmail.com>

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

Show 12 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 they are often used to clean up
> repo-specific variables that git passes around between processes. When
> submodule code executes commands on another repo, it clears all these
> variables, _many_ of these, that make the output really noisy.
> 
> (*) sort of. More on this in the next patch.

OK. I think we could probably just squash patches 3 and 4, but I'm OK with them separate, too.

The code looks fine, though I have one nit:

Show 6 quoted lines

> @@ -281,8 +299,17 @@ void trace_run_command(const struct child_process *cp)
>  				&trace_default_key, &buf))
>  		return;
>  
> +	strbuf_grow(&buf, 255);
> +

IMHO these magic-numbers grows detract from the readability (because you wonder if they're meaningful), and I doubt if they provide any real performance gain in practice.

-Peff