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
