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

[Stefan Beller <sbeller@xxxxxxxxxx>]

On Wed, Jan 10, 2018 at 10:09 AM, Brandon Williams <bmwill@xxxxxxxxxx> 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?

>>
>> 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@xxxxxxxxx>
>> ---
>>  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!

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