git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Brandon Williams <bmwill@google.com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: git@vger.kernel.org, Stefan Beller <sbeller@google.com>
Subject: Re: [PATCH v2] run-command.c: print env vars when GIT_TRACE is set
Date: Thu, 11 Jan 2018 09:53:16 -0800	[thread overview]
Message-ID: <20180111175316.GD91567@google.com> (raw)
In-Reply-To: <20180111094712.2551-1-pclouds@gmail.com>

On 01/11, 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.
> 
> 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.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  v2 fixes up commit message to clarify about "env delta" and why var
>  deletion is not printed.
> 
>  It also keeps child_process tracing in one place/function, this
>  should make it easier to trace more e.g. cwd and stuff.
> 
>  Though, Stefan, while i'm not opposed to trace every single setting
>  in child_process, including variable deletion, cwd and even more, it
>  may be not that often needed for a "casual" developer.
>  
>  I suggest we have something like $GIT_TRACE_EXEC instead that could
>  be super verbose when we need it and leave $GIT_TRACE with a
>  reasonable subset.
> 
>  run-command.c |  3 ++-
>  trace.c       | 39 +++++++++++++++++++++++++++++++++++++++
>  trace.h       |  3 +++
>  3 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/run-command.c b/run-command.c
> index 31fc5ea86e..002074b128 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_run_command(cmd);
> +
>  	fflush(NULL);
>  
>  #ifndef GIT_WINDOWS_NATIVE
> diff --git a/trace.c b/trace.c
> index b7530b51a9..e5e46e2a09 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -23,6 +23,7 @@
>  
>  #include "cache.h"
>  #include "quote.h"
> +#include "run-command.h"
>  
>  struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
>  struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
> @@ -272,6 +273,44 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,
>  #endif /* HAVE_VARIADIC_MACROS */
>  
>  
> +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.
> +		 */

Patch looks good to me! Only nit is this comment which reads funny which i
pointed out in v1.

> +		if (!strchr(env[i], '='))
> +			continue;
> +		strbuf_addch(dst, ' ');
> +		sq_quote_buf(dst, env[i]);
> +	}
> +}
> +
> +void trace_run_command(const struct child_process *cp)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	if (!prepare_trace_line(NULL, 0, &trace_default_key, &buf))
> +		return;
> +
> +	strbuf_addf(&buf, "trace: run_command:");
> +
> +	/*
> +	 * caller is responsible for setting this if the main source
> +	 * is actually in cp->env_array
> +	 */
> +	if (cp->env)
> +		concatenate_env(&buf, cp->env);
> +
> +	sq_quote_argv(&buf, cp->argv, 0);
> +	print_trace_line(&trace_default_key, &buf);
> +}
> +
>  static const char *quote_crnl(const char *path)
>  {
>  	static struct strbuf new_path = STRBUF_INIT;
> diff --git a/trace.h b/trace.h
> index 88055abef7..e54c687f26 100644
> --- a/trace.h
> +++ b/trace.h
> @@ -4,6 +4,8 @@
>  #include "git-compat-util.h"
>  #include "strbuf.h"
>  
> +struct child_process;
> +
>  struct trace_key {
>  	const char * const key;
>  	int fd;
> @@ -17,6 +19,7 @@ extern struct trace_key trace_default_key;
>  extern struct trace_key trace_perf_key;
>  
>  extern void trace_repo_setup(const char *prefix);
> +extern void trace_run_command(const struct child_process *cp);
>  extern int trace_want(struct trace_key *key);
>  extern void trace_disable(struct trace_key *key);
>  extern uint64_t getnanotime(void);
> -- 
> 2.15.1.600.g899a5f85c6
> 

-- 
Brandon Williams

  parent reply	other threads:[~2018-01-11 17:53 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-10 10:48 [PATCH] " Nguyễn Thái Ngọc Duy
2018-01-10 18:09 ` Brandon Williams
2018-01-10 19:14   ` Stefan Beller
2018-01-10 19:26     ` Brandon Williams
2018-01-10 19:35       ` Stefan Beller
2018-01-11  9:47 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2018-01-11 11:25   ` Duy Nguyen
2018-01-11 17:53   ` Brandon Williams [this message]
2018-01-11 18:20     ` Stefan Beller
2018-01-11 19:27   ` Junio C Hamano
2018-01-12  9:23     ` Duy Nguyen
2018-01-12  9:56   ` [PATCH v3 0/4] " Nguyễn Thái Ngọc Duy
2018-01-12  9:56     ` [PATCH v3 1/4] trace.c: introduce trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-12 13:05       ` Jeff King
2018-01-12 13:11         ` Jeff King
2018-01-12  9:56     ` [PATCH v3 2/4] trace.c: print program 'git' when child_process.git_cmd is set Nguyễn Thái Ngọc Duy
2018-01-12 13:05       ` Jeff King
2018-01-12  9:56     ` [PATCH v3 3/4] trace.c: print env vars in trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-12 13:13       ` Jeff King
2018-01-12  9:56     ` [PATCH v3 4/4] trace.c: be smart about what env to print " Nguyễn Thái Ngọc Duy
2018-01-12 13:33       ` Jeff King
2018-01-12 18:24         ` Junio C Hamano
2018-01-12 18:45           ` Jeff King
2018-01-12 19:19             ` Junio C Hamano
2018-01-12 19:23               ` Jeff King
2018-01-12 20:28                 ` Brandon Williams
2018-01-12 22:54         ` Junio C Hamano
2018-01-13  4:54           ` Duy Nguyen
2018-01-13  6:47             ` Duy Nguyen
2018-01-12 13:36     ` [PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set Jeff King
2018-01-12 13:38       ` [PATCH 5/4] sq_quote_argv: drop maxlen parameter Jeff King
2018-01-12 23:20         ` Junio C Hamano
2018-01-12 13:39       ` [PATCH 6/4] trace: avoid unnecessary quoting Jeff King
2018-01-12 18:06       ` [PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set Stefan Beller
2018-01-12 17:19     ` Stefan Beller
2018-01-13  6:54       ` Duy Nguyen
2018-01-13  6:49     ` [PATCH v4 0/7] Trace env variables in run_command() Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 1/7] sq_quote_argv: drop maxlen parameter Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 2/7] trace: avoid unnecessary quoting Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 3/7] trace.c: move strbuf_release() out of print_trace_line() Nguyễn Thái Ngọc Duy
2018-01-13  7:14         ` Jeff King
2018-01-13  6:49       ` [PATCH v4 4/7] trace.c: introduce trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 5/7] trace.c: print program 'git' when child_process.git_cmd is set Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 6/7] trace.c: print env vars in trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-13  7:25         ` Jeff King
2018-01-16 22:13         ` Junio C Hamano
2018-01-16 22:20           ` Stefan Beller
2018-01-17  1:32             ` Junio C Hamano
2018-01-13  6:49       ` [PATCH v4 7/7] trace.c: print new cwd " Nguyễn Thái Ngọc Duy
2018-01-13  7:26       ` [PATCH v4 0/7] Trace env variables in run_command() Jeff King
2018-01-15 10:59       ` [PATCH v5 " Nguyễn Thái Ngọc Duy
2018-01-15 10:59         ` [PATCH v5 1/7] sq_quote_argv: drop maxlen parameter Nguyễn Thái Ngọc Duy
2018-01-15 10:59         ` [PATCH v5 2/7] trace: avoid unnecessary quoting Nguyễn Thái Ngọc Duy
2018-01-15 10:59         ` [PATCH v5 3/7] trace.c: move strbuf_release() out of print_trace_line() Nguyễn Thái Ngọc Duy
2018-01-16 18:24           ` Brandon Williams
2018-01-15 10:59         ` [PATCH v5 4/7] trace.c: introduce trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-17 22:21           ` Jeff King
2018-01-17 22:41             ` Junio C Hamano
2018-01-15 10:59         ` [PATCH v5 5/7] trace.c: print program 'git' when child_process.git_cmd is set Nguyễn Thái Ngọc Duy
2018-01-15 10:59         ` [PATCH v5 6/7] trace.c: print env vars in trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-16 18:32           ` Brandon Williams
2018-01-15 10:59         ` [PATCH v5 7/7] trace.c: print new cwd " Nguyễn Thái Ngọc Duy
2018-01-18  9:45         ` [PATCH v6 0/7] Trace env variables in run_command() Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 1/7] sq_quote_argv: drop maxlen parameter Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 2/7] trace: avoid unnecessary quoting Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 3/7] trace.c: move strbuf_release() out of print_trace_line() Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 4/7] run-command.c: introduce trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 5/7] run-command.c: print program 'git' when tracing git_cmd mode Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 6/7] run-command.c: print env vars in trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 7/7] run-command.c: print new cwd " Nguyễn Thái Ngọc Duy
2018-01-19 21:39           ` [PATCH v6 0/7] Trace env variables in run_command() Jeff King
2018-01-11 10:07 ` [PATCH] run-command.c: print env vars when GIT_TRACE is set Jeff King
2018-01-11 11:13   ` Duy Nguyen
2018-01-11 18:21   ` Johannes Sixt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180111175316.GD91567@google.com \
    --to=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=sbeller@google.com \
    --subject='Re: [PATCH v2] run-command.c: print env vars when GIT_TRACE is set' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).