Re: [PATCH v5 4/7] trace.c: introduce trace_run_command()

From: Jeff King <peff@peff.net>
Date: Jan 17, 2018, 22:21 UTC
Message-ID: <20180117222143.GA15317@sigill.intra.peff.net>
In-Reply-To: <20180115105949.18328-5-pclouds@gmail.com>

On Mon, Jan 15, 2018 at 05:59:46PM +0700, Nguyễn Thái Ngọc Duy wrote:

Show 23 quoted lines

> diff --git a/trace.c b/trace.c
> index 7f3b08e148..da3db301e7 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);
> @@ -275,6 +276,21 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,
>  #endif /* HAVE_VARIADIC_MACROS */
>  
>  
> +void trace_run_command(const struct child_process *cp)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	if (!prepare_trace_line(__FILE__, __LINE__,
> +				&trace_default_key, &buf))
> +		return;

I think technically this should be TRACE_CONTEXT instead of __FILE__, though I wonder if anybody has ever set that (there is not even a Makefile knob for it, so you'd have to -DTRACE_CONTEXT manually).

I actually wonder if it would be nicer to keep this whole thing in run-command.c, and just prepare it all in a buffer. That's basically what we're doing here anyway, and then we could get rid of the funny __FILE__ stuff. I.e., something like:

  struct strbuf buf = STRBUF_INIT;

  if (!trace_want(&trace_default_key))
	return;

  strbuf_addf(&buf, "trace: run_command: ");
  sq_quote_argv_pretty(&buf, cp->argv);

  trace_printf("%s", buf.buf);
  strbuf_release(&buf);

AFAICT we aren't really using any internals of trace.c in our new function. That would get rid of this __FILE__ bit, it would eliminate the need for trace.h to know about "struct child_process", and it would mean the output still says "run-command.c" in it.

-Peff