Re: [PATCH 2/3] trace2: trim whitespace in start message in perf target format

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2019, 21:34 UTC
Message-ID: <xmqqef24muxe.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <834ee95cd2fe919c7a5a2d3cc3d647cfdeebe9e6.1564603467.git.gitgitgadget@gmail.com>

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Trim leading/trailing whitespace from the command line
> printed in the "start" message in the perf target format.
>
> We use `sq_quote_argv_pretty()` to format the message
> and it adds a leading space to the output.  Trim that.

strbuf_trim() not just drops a single leading space, but removes consecutive spaces from both ends. But the first char after the SP comes from the first arg, and it can never be a whitespace (as a payload that begins with a whitespace will be quoted, so it will be a single quote), and the last char in the buffer would also be either a closing single quote (if the last argument ends with a whitespace) or a non whitespace "safe" character, so it is safe to use strbuf_trim() here.

I wonder if we want to lose the prepending of SP from sq_quote_argv_pretty(), though:

 * run-command.c::trace_run_command() does rely on having SP there,
   so the caller needs adjusting if we did so.

 * trace.c::trace_argv_vprintf_fl() also needs SP there after the
   caller supplied format.

 * trace.c::print_command_performance_atexit() expects command_line
   begins with the extra SP left by the sq_quote_argv_pretty()
   called by the trace_command_performance(); the format string
   given to trace_performance_leave() there needs adjusting.

By the way, use of sq_quote_argv_pretty() in builtin/rebase.c on opts->git_am_opts.argv done in run_specific_rebase() is dubious. The argv array is made into a single string that safely uses sq, appropriate to feed a shell. But that string is passed as the "value" parameter to add_var() helper that expects to receive a raw value (hence it calls sq_quote_buf() on the value), resulting in a string that is doubly quoted. I am not sure if that was intended.

In any case, the patch itself obviously look correct.

Show 17 quoted lines

> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  trace2/tr2_tgt_perf.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
> index 4a9d99218b..ed4e708f28 100644
> --- a/trace2/tr2_tgt_perf.c
> +++ b/trace2/tr2_tgt_perf.c
> @@ -185,6 +185,7 @@ static void fn_start_fl(const char *file, int line,
>  	struct strbuf buf_payload = STRBUF_INIT;
>  
>  	sq_quote_argv_pretty(&buf_payload, argv);
> +	strbuf_trim(&buf_payload);
>  
>  	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
>  			 NULL, NULL, &buf_payload);