Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set
- From
Stefan Beller <sbeller@google.com>
- Date
- Jan 10, 2018, 19:14 UTC
- Message-ID
- <CAGZ79ka0mhPy776fRYtvnaqd5P1dwvuaeONozEWpaJVZxSneiA@mail.gmail.com>
- In-Reply-To
- <20180110180945.GA53941@google.com>
On Wed, Jan 10, 2018 at 10:09 AM, Brandon Williams <bmwill@google.com> wrote:
> On 01/10, Nguyễn Thái Ngọc Duy wrote: >> 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.
Speaking of GIT_DIR, we may also want to print the dir that the command is executed in as the GIT_DIR for submodules usually is only ".git" assuming the run-command struct set the .dir to the submodules worktree. I think I had a patch for printing the cwd of a process on the list once upon a time, but I am unable to find it again. Maybe we'd want to include the cwd of a spawned process if it differs from the current process?
Show 13 quoted lines
>> >> Print env variables in this case. Note that the code deliberately ignore >> variables unsetting because there are so many of them (to keep git >> environment clean for the next process) and really hard to read. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> >> --- >> A minor thing that I ignored in this patch is quoting the environment >> variables. For me it's meh. Perhaps I should just dumb the env >> without quoting at all. > > A patch like this would have been very helpful to me on some previous > occasions, so thanks for writing it up.
The patch I mentioned above (very similar though less powerful than this one) was carried by locally for some time, so I definitely see value in this patch. Thanks for writing it!
Show 55 quoted lines
>
>>
>> run-command.c | 3 ++-
>> trace.c | 38 +++++++++++++++++++++++++++++++++++---
>> trace.h | 18 +++++++++++++++---
>> 3 files changed, 52 insertions(+), 7 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 31fc5ea86e..235367087d 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)
>> cmd->err = fderr[0];
>> }
>>
>> - trace_argv_printf(cmd->argv, "trace: run_command:");
>> + trace_env_argv_printf(cmd->env, cmd->argv, "trace: run_command:");
>> +
>> fflush(NULL);
>>
>> #ifndef GIT_WINDOWS_NATIVE
>> diff --git a/trace.c b/trace.c
>> index b7530b51a9..d8967b66e8 100644
>> --- a/trace.c
>> +++ b/trace.c
>> @@ -146,7 +146,26 @@ static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,
>> print_trace_line(key, &buf);
>> }
>>
>> +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
>> + * e.g. GIT_DIR, ignore the unsetting to reduce noise.
>> + */
>
> I think you're missing a word, maybe:
> 'The main interesting part is setting new vars'
>
>> + if (!strchr(env[i], '='))
>> + continue;
>> + strbuf_addch(dst, ' ');
>> + sq_quote_buf(dst, env[i]);
>> + }
>
> At first when i read this I was under the impression that the whole
> environment was going to be printed out...but i now realize that this
> tracing will only print out the delta's or the additions to the
> environment that the child will see. Maybe this should be called out
> more clearly in the commit message?It only adds newly set variables, I wonder why deletions are noisy? I could not find an example of a removal of environment variables specific to submodules that would be noisy.
Stefan