| Message ID | 834ee95cd2fe919c7a5a2d3cc3d647cfdeebe9e6.1564603467.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trace2: clean up formatting in perf target format | expand |
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. > 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);
On 8/1/2019 5:34 PM, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> 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: I was wondering about that too, but I didn't want to presume to know what all of the callers wanted. And didn't want to slip in such a change last-minute. I'll re-roll with a new sq_quote_ function that does not include the leading whitespace and convert/normalize all of my trace2/* uses to call it instead. This eliminates the Trace2 code from the larger conversation (and is a little more efficient than the strbuf_trim()). Jeff
Jeff Hostetler <git@jeffhostetler.com> writes: > On 8/1/2019 5:34 PM, Junio C Hamano wrote: >> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> 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: > > I was wondering about that too, but I didn't want to presume > to know what all of the callers wanted. And didn't want to > slip in such a change last-minute. I do not think this topic/discussion is a place to do so, either. And compensating on the caller's side, like your patch did, is perfectly fine. I was more disturbed by potential double quoting in some codepaths when I did a cursory audit of users of sq_quote_argv_pretty(), and would think that would be of more importance, though.
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);