trace2: clean up formatting in perf target format
cover trace2: clean up formatting in perf target format
This patch series contains a few column alignment and whitespace fixes for perf target format. This should make it a little easier to read.
Jeff Hostetler (3):
trace2: cleanup column alignment in perf target format
trace2: trim whitespace in start message in perf target format
trace2: trim whitespace in region messages in perf target format
trace2/tr2_tgt_perf.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-)
base-commit: 026dd738a6e5f1e42ef0f390feacb5ed6acc4ee8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-298%2Fjeffhostetler%2Fupstream-2230-cleanup-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-298/jeffhostetler/upstream-2230-cleanup-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/298
Signature · 1 line
gitgitgadget
PATCH 1/3 trace2: cleanup column alignment in perf target format
From: Jeff Hostetler <[email protected]>
Truncate/elide very long "filename:linenumber" field. Truncate region and data "category" field if necessary. Adjust overall column widths.
--- trace2/tr2_tgt_perf.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-)
trace2/tr2_tgt_perf.c +29 -10
@@ -21,10 +21,10 @@ static struct tr2_dst tr2dst_perf = { TR2_SYSENV_PERF, 0, 0, 0 };*/ static int tr2env_perf_be_brief; -#define TR2FMT_PERF_FL_WIDTH (50) +#define TR2FMT_PERF_FL_WIDTH (28)#define TR2FMT_PERF_MAX_EVENT_NAME (12) -#define TR2FMT_PERF_REPO_WIDTH (4) -#define TR2FMT_PERF_CATEGORY_WIDTH (10) +#define TR2FMT_PERF_REPO_WIDTH (3) +#define TR2FMT_PERF_CATEGORY_WIDTH (12)#define TR2_DOTS_BUFFER_SIZE (100)#define TR2_INDENT (2)
@@ -79,17 +79,36 @@ static void perf_fmt_prepare(const char *event_name,if (!tr2env_perf_be_brief) {struct tr2_tbuf tb_now; +size_t fl_end_col;tr2_tbuf_local_time (&tb_now);strbuf_addstr (buf, tb_now.buf);strbuf_addch (buf,' ' ); -if (file && *file) -strbuf_addf (buf,"%s:%d " , file, line); -while (buf->len <TR2FMT_PERF_FL_WIDTH ) + fl_end_col = buf->len +TR2FMT_PERF_FL_WIDTH ; + +if (file && *file) { +struct strbuf buf_fl =STRBUF_INIT ; + +strbuf_addf (&buf_fl,"%s:%d" , file, line); + +if (buf_fl.len <=TR2FMT_PERF_FL_WIDTH ) +strbuf_addbuf (buf, &buf_fl); +else { +size_t avail =TR2FMT_PERF_FL_WIDTH -3 ; +strbuf_addstr (buf,"..." ); +strbuf_add (buf, + &buf_fl.buf[buf_fl.len - avail], + avail); + } + +strbuf_release (&buf_fl); + } + +while (buf->len < fl_end_col)strbuf_addch (buf,' ' ); -strbuf_addstr (buf,"| " ); +strbuf_addstr (buf," | " ); }strbuf_addf (buf,"d%d | " ,tr2_sid_depth ());
@@ -102,7 +121,7 @@ static void perf_fmt_prepare(const char *event_name,strbuf_addf (buf,"r%d " , repo->trace2_repo_id);while (buf->len < len)strbuf_addch (buf,' ' ); -strbuf_addstr (buf,"| " ); +strbuf_addstr (buf," | " );if (p_us_elapsed_absolute) strbuf_addf(buf, "%9.6f | ",
@@ -116,8 +135,8 @@ static void perf_fmt_prepare(const char *event_name,else strbuf_addf (buf,"%9s | " ," " ); -strbuf_addf (buf,"%-*s | " ,TR2FMT_PERF_CATEGORY_WIDTH , - (category ? category :"" )); +strbuf_addf (buf,"%-*.*s | " ,TR2FMT_PERF_CATEGORY_WIDTH , +TR2FMT_PERF_CATEGORY_WIDTH , (category ? category :"" ));if (ctx->nr_open_regions >0 ) {int len_indent =TR2_INDENT_LENGTH (ctx);
Signature · 1 line
gitgitgadget
PATCH 3/3 trace2: trim whitespace in region messages in perf target format
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Trim trailing whitespace in "region_enter" and "region_leave" messages in perf target format.
--- trace2/tr2_tgt_perf.c | 2 ++ 1 file changed, 2 insertions(+)
trace2/tr2_tgt_perf.c +2 -0
@@ -455,6 +455,7 @@ static void fn_region_enter_printf_va_fl(const char *file, int line,if (label)strbuf_addf (&buf_payload,"label:%s " , label);maybe_append_string_va (&buf_payload, fmt, ap); +strbuf_rtrim (&buf_payload);perf_io_write_fl (file, line, event_name, repo, &us_elapsed_absolute,NULL , category, &buf_payload);
@@ -472,6 +473,7 @@ static void fn_region_leave_printf_va_fl(if (label)strbuf_addf (&buf_payload,"label:%s " , label);maybe_append_string_va (&buf_payload, fmt, ap); +strbuf_rtrim (&buf_payload);perf_io_write_fl (file, line, event_name, repo, &us_elapsed_absolute, &us_elapsed_region, category, &buf_payload);
Signature · 1 line
gitgitgadget
PATCH 2/3 trace2: trim whitespace in start message in perf target format
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
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.
--- trace2/tr2_tgt_perf.c | 1 + 1 file changed, 1 insertion(+)
trace2/tr2_tgt_perf.c +1 -0
@@ -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);
Signature · 1 line
gitgitgadget
Re: [PATCH 2/3] trace2: trim whitespace in start message in perf target format
Quoted 7 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
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.
Quoted 17 lines
Signed-off-by Jeff Hostetler <[email protected]>--- trace2/tr2_tgt_perf.c | 1 + 1 file changed, 1 insertion(+)trace2/tr2_tgt_perf.c +1
-0@@ -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);
Quoted 8 lines · Jeff Hostetler via GitGitGadget
This patch series contains a few column alignment and whitespace fixes for perf target format. This should make it a little easier to read.
Jeff Hostetler (3):
trace2: cleanup column alignment in perf target format
trace2: trim whitespace in start message in perf target format
trace2: trim whitespace in region messages in perf target format
Quoted 10 lines
trace2/tr2_tgt_perf.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-)base-commit:
026dd738a6e5f1e42ef0f390feacb5ed6acc4ee8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-298%2Fjeffhostetler%2Fupstream-2230-cleanup-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-298/jeffhostetler/upstream-2230-cleanup-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/298Signature · 1 line
gitgitgadget
The series looks good to me.
Re: [PATCH 2/3] trace2: trim whitespace in start message in perf target format
In reply to Junio C HamanoQuoted 19 lines · Junio C Hamano
Quoted 7 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
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 usestrbuf_trim()here.
Quoted 2 lines
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
Re: [PATCH 2/3] trace2: trim whitespace in start message in perf target format
Quoted 20 lines · Jeff Hostetler
Quoted 19 lines · Junio C Hamano
Quoted 7 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
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 usestrbuf_trim()here.
Quoted 6 lines
Quoted 2 lines
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.
v2 cover trace2: clean up formatting in perf target format
In reply to Jeff Hostetler via GitGitGadgetV2 of this patch series cleans up some whitespace and column alignment issues in the trace2 perf and normal formats. It also removes some dead code.
Jeff Hostetler (7):
trace2: cleanup column alignment in perf target format
trace2: trim whitespace in region messages in perf target format
trace2: remove dead code in maybe_add_string_va()
trace2: trim trailing whitespace in normal format error message
quote: add sq_quote_argv_pretty_ltrim
trace2: cleanup whitespace in normal format
trace2: cleanup whitespace in perf format
quote.c | 11 +++++ quote.h | 1 + t/t0211-trace2-perf.sh | 4 +- trace2/tr2_tgt_event.c | 5 --- trace2/tr2_tgt_normal.c | 33 +++++++-------- trace2/tr2_tgt_perf.c | 89 +++++++++++++++++++++++++++-------------- 6 files changed, 90 insertions(+), 53 deletions(-)
base-commit: 026dd738a6e5f1e42ef0f390feacb5ed6acc4ee8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-298%2Fjeffhostetler%2Fupstream-2230-cleanup-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-298/jeffhostetler/upstream-2230-cleanup-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/298
Range-diff vs v1:
1: 38b53da31f = 1: 38b53da31f trace2: cleanup column alignment in perf target format 2: 834ee95cd2 < -: ---------- trace2: trim whitespace in start message in perf target format 3: d2f7cf3908 < -: ---------- trace2: trim whitespace in region messages in perf target format -: ---------- > 2: db35099712 trace2: trim whitespace in region messages in perf target format -: ---------- > 3: 09dff12a7d trace2: remove dead code in maybe_add_string_va() -: ---------- > 4: d5b5a70c1b trace2: trim trailing whitespace in normal format error message -: ---------- > 5: 5059776248 quote: add sq_quote_argv_pretty_ltrim -: ---------- > 6: bb64bb2513 trace2: cleanup whitespace in normal format -: ---------- > 7: a6e5e7c70b trace2: cleanup whitespace in perf format
Signature · 1 line
gitgitgadget
PATCH v2 1/7 trace2: cleanup column alignment in perf target format
From: Jeff Hostetler <[email protected]>
Truncate/elide very long "filename:linenumber" field. Truncate region and data "category" field if necessary. Adjust overall column widths.
--- trace2/tr2_tgt_perf.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-)
trace2/tr2_tgt_perf.c +29 -10
@@ -21,10 +21,10 @@ static struct tr2_dst tr2dst_perf = { TR2_SYSENV_PERF, 0, 0, 0 };*/ static int tr2env_perf_be_brief; -#define TR2FMT_PERF_FL_WIDTH (50) +#define TR2FMT_PERF_FL_WIDTH (28)#define TR2FMT_PERF_MAX_EVENT_NAME (12) -#define TR2FMT_PERF_REPO_WIDTH (4) -#define TR2FMT_PERF_CATEGORY_WIDTH (10) +#define TR2FMT_PERF_REPO_WIDTH (3) +#define TR2FMT_PERF_CATEGORY_WIDTH (12)#define TR2_DOTS_BUFFER_SIZE (100)#define TR2_INDENT (2)
@@ -79,17 +79,36 @@ static void perf_fmt_prepare(const char *event_name,if (!tr2env_perf_be_brief) {struct tr2_tbuf tb_now; +size_t fl_end_col;tr2_tbuf_local_time (&tb_now);strbuf_addstr (buf, tb_now.buf);strbuf_addch (buf,' ' ); -if (file && *file) -strbuf_addf (buf,"%s:%d " , file, line); -while (buf->len <TR2FMT_PERF_FL_WIDTH ) + fl_end_col = buf->len +TR2FMT_PERF_FL_WIDTH ; + +if (file && *file) { +struct strbuf buf_fl =STRBUF_INIT ; + +strbuf_addf (&buf_fl,"%s:%d" , file, line); + +if (buf_fl.len <=TR2FMT_PERF_FL_WIDTH ) +strbuf_addbuf (buf, &buf_fl); +else { +size_t avail =TR2FMT_PERF_FL_WIDTH -3 ; +strbuf_addstr (buf,"..." ); +strbuf_add (buf, + &buf_fl.buf[buf_fl.len - avail], + avail); + } + +strbuf_release (&buf_fl); + } + +while (buf->len < fl_end_col)strbuf_addch (buf,' ' ); -strbuf_addstr (buf,"| " ); +strbuf_addstr (buf," | " ); }strbuf_addf (buf,"d%d | " ,tr2_sid_depth ());
@@ -102,7 +121,7 @@ static void perf_fmt_prepare(const char *event_name,strbuf_addf (buf,"r%d " , repo->trace2_repo_id);while (buf->len < len)strbuf_addch (buf,' ' ); -strbuf_addstr (buf,"| " ); +strbuf_addstr (buf," | " );if (p_us_elapsed_absolute) strbuf_addf(buf, "%9.6f | ",
@@ -116,8 +135,8 @@ static void perf_fmt_prepare(const char *event_name,else strbuf_addf (buf,"%9s | " ," " ); -strbuf_addf (buf,"%-*s | " ,TR2FMT_PERF_CATEGORY_WIDTH , - (category ? category :"" )); +strbuf_addf (buf,"%-*.*s | " ,TR2FMT_PERF_CATEGORY_WIDTH , +TR2FMT_PERF_CATEGORY_WIDTH , (category ? category :"" ));if (ctx->nr_open_regions >0 ) {int len_indent =TR2_INDENT_LENGTH (ctx);
Signature · 1 line
gitgitgadget
PATCH v2 3/7 trace2: remove dead code in maybe_add_string_va()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Remove an unnecessary "if" block in maybe_add_string_va().
Commit "ad006fe419e trace2: NULL is not allowed for va_list" changed "if (fmt && *fmt && ap)" to just "if (fmt && *fmt)" because it isn't safe to treat 'ap' as a pointer. This made the "if" block following it unnecessary.
--- trace2/tr2_tgt_event.c | 5 ----- trace2/tr2_tgt_normal.c | 5 ----- trace2/tr2_tgt_perf.c | 5 ----- 3 files changed, 15 deletions(-)
trace2/tr2_tgt_event.c +0 -5
@@ -205,11 +205,6 @@ static void maybe_add_string_va(struct json_writer *jw, const char *field_name,strbuf_release (&buf);return ; } - -if (fmt && *fmt) { -jw_object_string (jw, field_name, fmt); -return ; - } } static void fn_error_va_fl(const char *file, int line, const char *fmt,
trace2/tr2_tgt_normal.c +0 -5
@@ -135,11 +135,6 @@ static void maybe_append_string_va(struct strbuf *buf, const char *fmt,va_end (copy_ap);return ; } - -if (fmt && *fmt) { -strbuf_addstr (buf, fmt); -return ; - } } static void fn_error_va_fl(const char *file, int line, const char *fmt,
trace2/tr2_tgt_perf.c +0 -5
@@ -239,11 +239,6 @@ static void maybe_append_string_va(struct strbuf *buf, const char *fmt,va_end (copy_ap);return ; } - -if (fmt && *fmt) { -strbuf_addstr (buf, fmt); -return ; - } } static void fn_error_va_fl(const char *file, int line, const char *fmt,
Signature · 1 line
gitgitgadget
PATCH v2 5/7 quote: add sq_quote_argv_pretty_ltrim
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create version of sq_quote_argv_pretty() that does not insert a leading space before argv[0].
--- quote.c | 11 +++++++++++ quote.h | 1 + 2 files changed, 12 insertions(+)
quote.c +11 -0
@@ -94,6 +94,17 @@ void sq_quote_argv_pretty(struct strbuf *dst, const char **argv) } } +void sq_quote_argv_pretty_ltrim (struct strbuf *dst,const char **argv) +{ +int i; + +for (i =0 ; argv[i]; i++) { +if (i >0 ) +strbuf_addch (dst,' ' ); +sq_quote_buf_pretty (dst, argv[i]); + } +} +static char *sq_dequote_step (char *arg,char **next) {char *dst = arg;
quote.h +1 -0
@@ -40,6 +40,7 @@ void sq_quotef(struct strbuf *, const char *fmt, ...);*/ void sq_quote_buf_pretty (struct strbuf *,const char *src);void sq_quote_argv_pretty (struct strbuf *,const char **argv); +void sq_quote_argv_pretty_ltrim (struct strbuf *,const char **argv);/* This unwraps what sq_quote() produces in place, but returns * NULL if the input does not look like what sq_quote would have
Signature · 1 line
gitgitgadget
PATCH v2 6/7 trace2: cleanup whitespace in normal format
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Make use of new sq_quote_argv_pretty_ltrim() to normalize how we handle leading whitespace in normal format messages.
--- trace2/tr2_tgt_normal.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
trace2/tr2_tgt_normal.c +12 -9
@@ -87,7 +87,7 @@ static void fn_start_fl(const char *file, int line,struct strbuf buf_payload =STRBUF_INIT ;strbuf_addstr (&buf_payload,"start " ); -sq_quote_argv_pretty (&buf_payload, argv); +sq_quote_argv_pretty_ltrim (&buf_payload, argv);normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload); }
@@ -186,8 +186,8 @@ static void fn_alias_fl(const char *file, int line, const char *alias, {struct strbuf buf_payload =STRBUF_INIT ; -strbuf_addf (&buf_payload,"alias %s ->" , alias); -sq_quote_argv_pretty (&buf_payload, argv); +strbuf_addf (&buf_payload,"alias %s -> " , alias); +sq_quote_argv_pretty_ltrim (&buf_payload, argv);normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload); }
@@ -198,12 +198,12 @@ static void fn_child_start_fl(const char *file, int line, {struct strbuf buf_payload =STRBUF_INIT ; -strbuf_addf (&buf_payload,"child_start[%d] " , cmd->trace2_child_id); +strbuf_addf (&buf_payload,"child_start[%d]" , cmd->trace2_child_id);if (cmd->dir) {strbuf_addstr (&buf_payload," cd" );sq_quote_buf_pretty (&buf_payload, cmd->dir); -strbuf_addstr (&buf_payload,"; " ); +strbuf_addstr (&buf_payload,";" ); }/*
@@ -211,9 +211,10 @@ static void fn_child_start_fl(const char *file, int line,* See trace_add_env() in run-command.c as used by original trace.c */ +strbuf_addch (&buf_payload,' ' );if (cmd->git_cmd) -strbuf_addstr (&buf_payload,"git" ); -sq_quote_argv_pretty (&buf_payload, cmd->argv); +strbuf_addstr (&buf_payload,"git " ); +sq_quote_argv_pretty_ltrim (&buf_payload, cmd->argv);normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload);
@@ -238,9 +239,11 @@ static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,struct strbuf buf_payload =STRBUF_INIT ;strbuf_addf (&buf_payload,"exec[%d] " , exec_id); -if (exe) +if (exe) {strbuf_addstr (&buf_payload, exe); -sq_quote_argv_pretty (&buf_payload, argv); +strbuf_addch (&buf_payload,' ' ); + } +sq_quote_argv_pretty_ltrim (&buf_payload, argv);normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload); }
Signature · 1 line
gitgitgadget
PATCH v2 7/7 trace2: cleanup whitespace in perf format
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Make use of new sq_quote_argv_pretty_ltrim() to normalize how we handle leading whitespace in perf format messages.
--- t/t0211-trace2-perf.sh | 4 ++-- trace2/tr2_tgt_perf.c | 31 ++++++++++++++++++++----------- 2 files changed, 22 insertions(+), 13 deletions(-)
t/t0211-trace2-perf.sh +2 -2
@@ -130,11 +130,11 @@ test_expect_success 'perf stream, child processes' ' d0|main|version|||||$V d0|main|start||_T_ABS_|||_EXE_ trace2 004child test-tool trace2 004child test-tool trace2 001return 0 d0|main|cmd_name|||||trace2 (trace2) - d0|main|child_start||_T_ABS_|||[ch0] class:? argv: test-tool trace2 004child test-tool trace2 001return 0 + d0|main|child_start||_T_ABS_|||[ch0] class:? argv:[test-tool trace2 004child test-tool trace2 001return 0] d1|main|version|||||$V d1|main|start||_T_ABS_|||_EXE_ trace2 004child test-tool trace2 001return 0 d1|main|cmd_name|||||trace2 (trace2/trace2) - d1|main|child_start||_T_ABS_|||[ch0] class:? argv: test-tool trace2 001return 0 + d1|main|child_start||_T_ABS_|||[ch0] class:? argv:[test-tool trace2 001return 0] d2|main|version|||||$V d2|main|start||_T_ABS_|||_EXE_ trace2 001return 0 d2|main|cmd_name|||||trace2 (trace2/trace2/trace2)
trace2/tr2_tgt_perf.c +20 -11
@@ -184,7 +184,7 @@ static void fn_start_fl(const char *file, int line,const char *event_name ="start" ;struct strbuf buf_payload =STRBUF_INIT ; -sq_quote_argv_pretty (&buf_payload, argv); +sq_quote_argv_pretty_ltrim (&buf_payload, argv);perf_io_write_fl (file, line, event_name,NULL , &us_elapsed_absolute,NULL ,NULL , &buf_payload);
@@ -299,8 +299,9 @@ static void fn_alias_fl(const char *file, int line, const char *alias,const char *event_name ="alias" ;struct strbuf buf_payload =STRBUF_INIT ; -strbuf_addf (&buf_payload,"alias:%s argv:" , alias); -sq_quote_argv_pretty (&buf_payload, argv); +strbuf_addf (&buf_payload,"alias:%s argv:[" , alias); +sq_quote_argv_pretty_ltrim (&buf_payload, argv); +strbuf_addch (&buf_payload,']' );perf_io_write_fl (file, line, event_name,NULL ,NULL ,NULL ,NULL , &buf_payload);
@@ -329,10 +330,14 @@ static void fn_child_start_fl(const char *file, int line,sq_quote_buf_pretty (&buf_payload, cmd->dir); } -strbuf_addstr (&buf_payload," argv:" ); -if (cmd->git_cmd) -strbuf_addstr (&buf_payload," git" ); -sq_quote_argv_pretty (&buf_payload, cmd->argv); +strbuf_addstr (&buf_payload," argv:[" ); +if (cmd->git_cmd) { +strbuf_addstr (&buf_payload,"git" ); +if (cmd->argv[0 ]) +strbuf_addch (&buf_payload,' ' ); + } +sq_quote_argv_pretty_ltrim (&buf_payload, cmd->argv); +strbuf_addch (&buf_payload,']' );perf_io_write_fl (file, line, event_name,NULL , &us_elapsed_absolute,NULL ,NULL , &buf_payload);
@@ -383,10 +388,14 @@ static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,struct strbuf buf_payload =STRBUF_INIT ;strbuf_addf (&buf_payload,"id:%d " , exec_id); -strbuf_addstr (&buf_payload,"argv:" ); -if (exe) -strbuf_addf (&buf_payload," %s" , exe); -sq_quote_argv_pretty (&buf_payload, argv); +strbuf_addstr (&buf_payload,"argv:[" ); +if (exe) { +strbuf_addstr (&buf_payload, exe); +if (argv[0 ]) +strbuf_addch (&buf_payload,' ' ); + } +sq_quote_argv_pretty_ltrim (&buf_payload, argv); +strbuf_addch (&buf_payload,']' );perf_io_write_fl (file, line, event_name,NULL , &us_elapsed_absolute,NULL ,NULL , &buf_payload);
Signature · 1 line
gitgitgadget
PATCH v2 2/7 trace2: trim whitespace in region messages in perf target format
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Avoid unecessary trailing whitespace in "region_enter" and "region_leave" messages in perf target format.
--- trace2/tr2_tgt_perf.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
trace2/tr2_tgt_perf.c +10 -4
@@ -452,8 +452,11 @@ static void fn_region_enter_printf_va_fl(const char *file, int line,struct strbuf buf_payload =STRBUF_INIT ;if (label) -strbuf_addf (&buf_payload,"label:%s " , label); -maybe_append_string_va (&buf_payload, fmt, ap); +strbuf_addf (&buf_payload,"label:%s" , label); +if (fmt && *fmt) { +strbuf_addch (&buf_payload,' ' ); +maybe_append_string_va (&buf_payload, fmt, ap); + }perf_io_write_fl (file, line, event_name, repo, &us_elapsed_absolute,NULL , category, &buf_payload);
@@ -469,8 +472,11 @@ static void fn_region_leave_printf_va_fl(struct strbuf buf_payload =STRBUF_INIT ;if (label) -strbuf_addf (&buf_payload,"label:%s " , label); -maybe_append_string_va (&buf_payload, fmt, ap); +strbuf_addf (&buf_payload,"label:%s" , label); +if (fmt && *fmt) { +strbuf_addch (&buf_payload,' ' ); +maybe_append_string_va (&buf_payload, fmt, ap); + }perf_io_write_fl (file, line, event_name, repo, &us_elapsed_absolute, &us_elapsed_region, category, &buf_payload);
Signature · 1 line
gitgitgadget
PATCH v2 4/7 trace2: trim trailing whitespace in normal format error message
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Avoid creating unnecessary trailing whitespace in normal target format error messages when the message is omitted.
--- trace2/tr2_tgt_normal.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
trace2/tr2_tgt_normal.c +5 -2
@@ -142,8 +142,11 @@ static void fn_error_va_fl(const char *file, int line, const char *fmt, {struct strbuf buf_payload =STRBUF_INIT ; -strbuf_addstr (&buf_payload,"error " ); -maybe_append_string_va (&buf_payload, fmt, ap); +strbuf_addstr (&buf_payload,"error" ); +if (fmt && *fmt) { +strbuf_addch (&buf_payload,' ' ); +maybe_append_string_va (&buf_payload, fmt, ap); + }normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload); }
Signature · 1 line
gitgitgadget
Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim
In reply to Jeff Hostetler via GitGitGadgetQuoted 10 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Create version of
sq_quote_argv_pretty()that does not insert a leading space before argv[0].Signed-off-by Jeff Hostetler <[email protected]>--- quote.c | 11 +++++++++++ quote.h | 1 + 2 files changed, 12 insertions(+)
I am OK with the basic idea, but I am somewhat unhappy about this particular patch for two reasons:
- If we were to keep this as a part of proper API in the longer term, the current sq_quote_argv_pretty() should be rewritten to use this to avoid repetition (e.g. as long as !!*argv, add a SP and then call this new thing);
- something_ltrim() sounds as if you munge what is passed to you and chop off the left end, but that is not what this does.
Now, what is the right name for this new thing? What does it do?
It looks to me that it appends each element of argv[], quoting it as needed, and with SP in between. So the right name for the family of these functions should be around "append", which is the primary thing they do, with "quoted" somewhere.
Having made the primary purpose of the helper clearer leads me to wonder if "do not add SP before the first element, i.e. argv[0]", is really what we want. If we always clear the *dst strbuf before starting to serialize argv[] into it, then the behaviour would make sense, but we do not---we are "appending".
As long as we are appending, would we be better off doing something sillily magical like this instead, I have to wonder?
void sq_append_strings_quoted(struct strbuf *buf, const char **av)
{
int i;
for (i = 0; av[i]; i++) {
if (buf->len)
strbuf_addch(buf, ' ');
sq_quote_buf_pretty(buf, argv[0]);
}
}That is, "if we are appending to an existing string, have SP to separate the first element from that existing string; treat the remaining elements the same way (if the buffer is empty, there is no point adding SP at the beginning)".
I may have found a long-standing bug in sq_quote_buf_pretty(), by the way. What does it produce when *src is an empty string of length 0? It does not add anything to dst, but shouldn't we be adding two single-quotes (i.e. an empty string inside sq pair)?
Quoted 34 lines
quote.c +11
-0@@ -94,6 +94,17 @@ void sq_quote_argv_pretty(struct strbuf *dst, const char **argv) } } +void sq_quote_argv_pretty_ltrim (struct strbuf *dst,const char **argv) +{ +int i; + +for (i =0 ; argv[i]; i++) { +if (i >0 ) +strbuf_addch (dst,' ' ); +sq_quote_buf_pretty (dst, argv[i]); + } +} +static char *sq_dequote_step (char *arg,char **next) {char *dst = arg;quote.h +1
-0@@ -40,6 +40,7 @@ void sq_quotef(struct strbuf *, const char *fmt, ...);*/ void sq_quote_buf_pretty (struct strbuf *,const char *src);void sq_quote_argv_pretty (struct strbuf *,const char **argv); +void sq_quote_argv_pretty_ltrim (struct strbuf *,const char **argv);/* This unwraps what sq_quote() produces in place, but returns * NULL if the input does not look like what sq_quote would have
Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim
Quoted 21 lines · Junio C Hamano
Quoted 10 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
Create version of
sq_quote_argv_pretty()that does not insert a leading space before argv[0].Signed-off-by Jeff Hostetler <[email protected]>--- quote.c | 11 +++++++++++ quote.h | 1 + 2 files changed, 12 insertions(+)I am OK with the basic idea, but I am somewhat unhappy about this particular patch for two reasons:
- If we were to keep this as a part of proper API in the longer term, the current
sq_quote_argv_pretty()should be rewritten to use this to avoid repetition (e.g. as long as !!*argv, add a SP and then call this new thing);
Quoted 4 lines
-
something_ltrim()sounds as if you munge what is passed to you and chop off the left end, but that is not what this does.Now, what is the right name for this new thing? What does it do?
I struggled with the proper name for this.
And even thought about adding a 3rd arg to the current function to indicate whether to have the leading SP before argv[0], but wasn't sure if that was too disruptive.
Quoted 26 lines
It looks to me that it appends each element of argv[], quoting it as needed, and with SP in between. So the right name for the family of these functions should be around "append", which is the primary thing they do, with "quoted" somewhere.
Having made the primary purpose of the helper clearer leads me to wonder if "do not add SP before the first element, i.e. argv[0]", is really what we want. If we always clear the *dst strbuf before starting to serialize argv[] into it, then the behaviour would make sense, but we do not---we are "appending".
As long as we are appending, would we be better off doing something sillily magical like this instead, I have to wonder?
void sq_append_strings_quoted(struct strbuf *buf, const char **av) { int i; for (i = 0; av[i]; i++) { if (buf->len) strbuf_addch(buf, ' '); sq_quote_buf_pretty(buf, argv[0]); } }
Quoted 4 lines
That is, "if we are appending to an existing string, have SP to separate the first element from that existing string; treat the remaining elements the same way (if the buffer is empty, there is no point adding SP at the beginning)".
I don't think that would do what we want. We don't know what the caller's expectations are. In my uses in commits 6/7 and 7/7 I already added the leading chars I wanted in the strbuf before calling sq_quote_argv_pretty_ltrim() and assumed the output would be a true append. For example:
+ strbuf_addf(&buf_payload, "alias:%s argv:[", alias);
+ sq_quote_argv_pretty_ltrim(&buf_payload, argv);
+ strbuf_addch(&buf_payload, ']');
I like your suggestion of putting my new function in the _append_ category. I think I'll add the 3rd arg to this and then it will be completely specified and I can get rid of the _ltrim suffix.
I'll re-roll this.
Quoted 5 lines
I may have found a long-standing bug in
sq_quote_buf_pretty(), by the way. What does it produce when *src is an empty string of length 0? It does not add anything to dst, but shouldn't we be adding two single-quotes (i.e. an empty string inside sq pair)?
I would think so. I did a quick grep and most of the calls looked guarded, so I don't think this is urgent. I'll address this in a separate commit shortly.
Thanks
Jeff
Quoted 35 lines
Quoted 35 lines
quote.c +11
-0@@ -94,6 +94,17 @@ void sq_quote_argv_pretty(struct strbuf *dst, const char **argv) } } +void sq_quote_argv_pretty_ltrim (struct strbuf *dst,const char **argv) +{ +int i; + +for (i =0 ; argv[i]; i++) { +if (i >0 ) +strbuf_addch (dst,' ' ); +sq_quote_buf_pretty (dst, argv[i]); + } +} +static char *sq_dequote_step (char *arg,char **next) {char *dst = arg;quote.h +1
-0@@ -40,6 +40,7 @@ void sq_quotef(struct strbuf *, const char *fmt, ...);*/ void sq_quote_buf_pretty (struct strbuf *,const char *src);void sq_quote_argv_pretty (struct strbuf *,const char **argv); +void sq_quote_argv_pretty_ltrim (struct strbuf *,const char **argv);/* This unwraps what sq_quote() produces in place, but returns * NULL if the input does not look like what sq_quote would have
Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim
Quoted 6 lines · Jeff Hostetler
Quoted 4 lines
That is, "if we are appending to an existing string, have SP to separate the first element from that existing string; treat the remaining elements the same way (if the buffer is empty, there is no point adding SP at the beginning)".
I don't think that would do what we want.
I know that there are current callers of quote_argv_pretty that either (1) expects that it will always get the leading SP for free, or (2) has to work the unwanted SP around (basically, the places you changed from quote_argv_pretty to quote_argv_pretty_ltrim in your series). But I wanted to see if we can come up with a single helper whose behaviour is easy to explain and understand that both existing and new callers can adopt---and if the resulting codebase becomes easy to understand and maintain overall. And if that would give us the ideal longer term direction.
Quoted 8 lines
Quoted 4 lines
I may have found a long-standing bug in
sq_quote_buf_pretty(), by the way. What does it produce when *src is an empty string of length 0? It does not add anything to dst, but shouldn't we be adding two single-quotes (i.e. an empty string inside sq pair)?I would think so. I did a quick grep and most of the calls looked guarded, so I don't think this is urgent. I'll address this in a separate commit shortly.
Thanks.
Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim
In reply to Jeff HostetlerAm 08.08.19 um 21:04 schrieb Jeff Hostetler:
Quoted 32 lines · Junio C Hamano
Quoted 20 lines
Having made the primary purpose of the helper clearer leads me to wonder if "do not add SP before the first element, i.e. argv[0]", is really what we want. If we always clear the *dst strbuf before starting to serialize argv[] into it, then the behaviour would make sense, but we do not---we are "appending".
As long as we are appending, would we be better off doing something sillily magical like this instead, I have to wonder?
void
sq_append_strings_quoted(struct strbuf *buf, const char **av){
int i;for (i = 0; av[i]; i++) {
if (buf->len)
strbuf_addch(buf, ' ');
sq_quote_buf_pretty(buf, argv[0]);
}
}Quoted 4 lines
That is, "if we are appending to an existing string, have SP to separate the first element from that existing string; treat the remaining elements the same way (if the buffer is empty, there is no point adding SP at the beginning)".
I don't think that would do what we want. We don't know what the caller's expectations are. In my uses in commits 6/7 and 7/7 I already added the leading chars I wanted in the strbuf before calling
sq_quote_argv_pretty_ltrim()and assumed the output would be a true append. For example:
Quoted 7 lines
+
strbuf_addf(&buf_payload, "alias:%s argv:[", alias); +sq_quote_argv_pretty_ltrim(&buf_payload, argv); +strbuf_addch(&buf_payload, ']');I like your suggestion of putting my new function in the _append_ category. I think I'll add the 3rd arg to this and then it will be completely specified and I can get rid of the
_ltrimsuffix.
Two observations:
If callers want to add something before a joined delimited list, they already can with a strbuf_add* call. No need to add that feature to a function that joins lists.
And repetitions of repetitions (loops) are boring.
Apologies in advance for any coffee stains on your monitor, but here's how I would start, probably followed by attempts to inline the functions that become trivial wrappers:
--- quote.c | 18 ++++-------------- strbuf.c | 20 +++++++++++++------- strbuf.h | 8 ++++++++ 3 files changed, 25 insertions(+), 21 deletions(-)
quote.c +4 -14
@@ -74,24 +74,14 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ...)void sq_quote_argv (struct strbuf *dst,const char **argv) { -int i; - -/* Copy into destination buffer. */ -strbuf_grow (dst,255 ); -for (i =0 ; argv[i]; ++i) { -strbuf_addch (dst,' ' ); -sq_quote_buf (dst, argv[i]); - } +strbuf_addch (dst,' ' ); +strbuf_map_join_argv (dst, argv, sq_quote_buf," " ); }void sq_quote_argv_pretty (struct strbuf *dst,const char **argv) { -int i; - -for (i =0 ; argv[i]; i++) { -strbuf_addch (dst,' ' ); -sq_quote_buf_pretty (dst, argv[i]); - } +strbuf_addch (dst,' ' ); +strbuf_map_join_argv (dst, argv, sq_quote_buf_pretty," " ); }static char *sq_dequote_step (char *arg,char **next)
strbuf.c +13 -7
@@ -304,17 +304,23 @@ void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)strbuf_setlen (sb, sb->len + sb2->len); } +void strbuf_map_join_argv (struct strbuf *sb,const char **argv, +void (*fn)(struct strbuf *,const char *), +const char *separator) +{ +while (*argv) { +fn (sb, *argv++); +if (*argv) +strbuf_addstr (sb, separator); + } +} +const char *strbuf_join_argv (struct strbuf *buf,int argc,const char **argv,char delim) { -if (!argc) -return buf->buf; +char separator[] = { delim,'\0' }; -strbuf_addstr (buf, *argv); -while (--argc) { -strbuf_addch (buf, delim); -strbuf_addstr (buf, *(++argv)); - } +strbuf_map_join_argv (buf, argv, strbuf_addstr, separator);return buf->buf; }
strbuf.h +8 -0
@@ -297,6 +297,14 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)*/ void strbuf_addbuf (struct strbuf *sb,const struct strbuf *sb2); +/** +* Apply `fn` to `sb` and each element of the NULL-terminated array +* `argv`. Add `separator` between these invocations. +*/ +void strbuf_map_join_argv (struct strbuf *sb,const char **argv, +void (*fn)(struct strbuf *,const char *), +const char *separator); +/** * Join the arguments into a buffer. `delim` is put between every * two arguments.
--
2.22.0
v3 cover trace2: clean up formatting in perf target format
In reply to Jeff Hostetler via GitGitGadgetV2 of this patch series cleans up some whitespace and column alignment issues in the trace2 perf and normal formats. It also removes some dead code.
Jeff Hostetler (7):
trace2: cleanup column alignment in perf target format
trace2: trim whitespace in region messages in perf target format
trace2: remove dead code in maybe_add_string_va()
trace2: trim trailing whitespace in normal format error message
quote: add sq_append_quote_argv_pretty()
trace2: cleanup whitespace in normal format
trace2: cleanup whitespace in perf format
quote.c | 18 ++++++++- quote.h | 1 + t/t0211-trace2-perf.sh | 4 +- trace2/tr2_tgt_event.c | 5 --- trace2/tr2_tgt_normal.c | 35 ++++++++-------- trace2/tr2_tgt_perf.c | 89 +++++++++++++++++++++++++++-------------- 6 files changed, 97 insertions(+), 55 deletions(-)
base-commit: 026dd738a6e5f1e42ef0f390feacb5ed6acc4ee8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-298%2Fjeffhostetler%2Fupstream-2230-cleanup-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-298/jeffhostetler/upstream-2230-cleanup-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/298
Range-diff vs v2:
1: 38b53da31f = 1: 38b53da31f trace2: cleanup column alignment in perf target format
2: db35099712 = 2: db35099712 trace2: trim whitespace in region messages in perf target format
3: 09dff12a7d = 3: 09dff12a7d trace2: remove dead code in maybe_add_string_va()
4: d5b5a70c1b = 4: d5b5a70c1b trace2: trim trailing whitespace in normal format error message
5: 5059776248 < -: ---------- quote: add sq_quote_argv_pretty_ltrim
-: ---------- > 5: 2002108a30 quote: add sq_append_quote_argv_pretty()
6: bb64bb2513 ! 6: d02a0608d7 trace2: cleanup whitespace in normal format
@@ -2,7 +2,7 @@
trace2: cleanup whitespace in normal format
- Make use of new sq_quote_argv_pretty_ltrim() to normalize
+ Make use of new sq_append_quote_argv_pretty() to normalize
how we handle leading whitespace in normal format messages.
Signed-off-by: Jeff Hostetler <[email protected]>
@@ -15,7 +15,7 @@
strbuf_addstr(&buf_payload, "start ");
- sq_quote_argv_pretty(&buf_payload, argv);
-+ sq_quote_argv_pretty_ltrim(&buf_payload, argv);
++ sq_append_quote_argv_pretty(&buf_payload, argv);
normal_io_write_fl(file, line, &buf_payload);
strbuf_release(&buf_payload);
}
@@ -26,7 +26,7 @@
- strbuf_addf(&buf_payload, "alias %s ->", alias);
- sq_quote_argv_pretty(&buf_payload, argv);
+ strbuf_addf(&buf_payload, "alias %s -> ", alias);
-+ sq_quote_argv_pretty_ltrim(&buf_payload, argv);
++ sq_append_quote_argv_pretty(&buf_payload, argv);
normal_io_write_fl(file, line, &buf_payload);
strbuf_release(&buf_payload);
}
@@ -38,7 +38,8 @@
+ strbuf_addf(&buf_payload, "child_start[%d]", cmd->trace2_child_id);
if (cmd->dir) {
- strbuf_addstr(&buf_payload, " cd");
+- strbuf_addstr(&buf_payload, " cd");
++ strbuf_addstr(&buf_payload, " cd ");
sq_quote_buf_pretty(&buf_payload, cmd->dir);
- strbuf_addstr(&buf_payload, "; ");
+ strbuf_addstr(&buf_payload, ";");
@@ -54,7 +55,7 @@
- strbuf_addstr(&buf_payload, "git");
- sq_quote_argv_pretty(&buf_payload, cmd->argv);
+ strbuf_addstr(&buf_payload, "git ");
-+ sq_quote_argv_pretty_ltrim(&buf_payload, cmd->argv);
++ sq_append_quote_argv_pretty(&buf_payload, cmd->argv);
normal_io_write_fl(file, line, &buf_payload);
strbuf_release(&buf_payload);
@@ -68,7 +69,7 @@
- sq_quote_argv_pretty(&buf_payload, argv);
+ strbuf_addch(&buf_payload, ' ');
+ }
-+ sq_quote_argv_pretty_ltrim(&buf_payload, argv);
++ sq_append_quote_argv_pretty(&buf_payload, argv);
normal_io_write_fl(file, line, &buf_payload);
strbuf_release(&buf_payload);
}
7: a6e5e7c70b ! 7: 9ab7393985 trace2: cleanup whitespace in perf format
@@ -2,7 +2,7 @@
trace2: cleanup whitespace in perf format
- Make use of new sq_quote_argv_pretty_ltrim() to normalize
+ Make use of new sq_append_quote_argv_pretty() to normalize
how we handle leading whitespace in perf format messages.
Signed-off-by: Jeff Hostetler <[email protected]>
@@ -33,7 +33,7 @@
struct strbuf buf_payload = STRBUF_INIT;
- sq_quote_argv_pretty(&buf_payload, argv);
-+ sq_quote_argv_pretty_ltrim(&buf_payload, argv);
++ sq_append_quote_argv_pretty(&buf_payload, argv);
perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
NULL, NULL, &buf_payload);
@@ -44,7 +44,7 @@
- strbuf_addf(&buf_payload, "alias:%s argv:", alias);
- sq_quote_argv_pretty(&buf_payload, argv);
+ strbuf_addf(&buf_payload, "alias:%s argv:[", alias);
-+ sq_quote_argv_pretty_ltrim(&buf_payload, argv);
++ sq_append_quote_argv_pretty(&buf_payload, argv);
+ strbuf_addch(&buf_payload, ']');
perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
@@ -63,7 +63,7 @@
+ if (cmd->argv[0])
+ strbuf_addch(&buf_payload, ' ');
+ }
-+ sq_quote_argv_pretty_ltrim(&buf_payload, cmd->argv);
++ sq_append_quote_argv_pretty(&buf_payload, cmd->argv);
+ strbuf_addch(&buf_payload, ']');
perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
@@ -82,7 +82,7 @@
+ if (argv[0])
+ strbuf_addch(&buf_payload, ' ');
+ }
-+ sq_quote_argv_pretty_ltrim(&buf_payload, argv);
++ sq_append_quote_argv_pretty(&buf_payload, argv);
+ strbuf_addch(&buf_payload, ']');
perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,Signature · 1 line
gitgitgadget
PATCH v3 1/7 trace2: cleanup column alignment in perf target format
From: Jeff Hostetler <[email protected]>
Truncate/elide very long "filename:linenumber" field. Truncate region and data "category" field if necessary. Adjust overall column widths.
--- trace2/tr2_tgt_perf.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-)
trace2/tr2_tgt_perf.c +29 -10
@@ -21,10 +21,10 @@ static struct tr2_dst tr2dst_perf = { TR2_SYSENV_PERF, 0, 0, 0 };*/ static int tr2env_perf_be_brief; -#define TR2FMT_PERF_FL_WIDTH (50) +#define TR2FMT_PERF_FL_WIDTH (28)#define TR2FMT_PERF_MAX_EVENT_NAME (12) -#define TR2FMT_PERF_REPO_WIDTH (4) -#define TR2FMT_PERF_CATEGORY_WIDTH (10) +#define TR2FMT_PERF_REPO_WIDTH (3) +#define TR2FMT_PERF_CATEGORY_WIDTH (12)#define TR2_DOTS_BUFFER_SIZE (100)#define TR2_INDENT (2)
@@ -79,17 +79,36 @@ static void perf_fmt_prepare(const char *event_name,if (!tr2env_perf_be_brief) {struct tr2_tbuf tb_now; +size_t fl_end_col;tr2_tbuf_local_time (&tb_now);strbuf_addstr (buf, tb_now.buf);strbuf_addch (buf,' ' ); -if (file && *file) -strbuf_addf (buf,"%s:%d " , file, line); -while (buf->len <TR2FMT_PERF_FL_WIDTH ) + fl_end_col = buf->len +TR2FMT_PERF_FL_WIDTH ; + +if (file && *file) { +struct strbuf buf_fl =STRBUF_INIT ; + +strbuf_addf (&buf_fl,"%s:%d" , file, line); + +if (buf_fl.len <=TR2FMT_PERF_FL_WIDTH ) +strbuf_addbuf (buf, &buf_fl); +else { +size_t avail =TR2FMT_PERF_FL_WIDTH -3 ; +strbuf_addstr (buf,"..." ); +strbuf_add (buf, + &buf_fl.buf[buf_fl.len - avail], + avail); + } + +strbuf_release (&buf_fl); + } + +while (buf->len < fl_end_col)strbuf_addch (buf,' ' ); -strbuf_addstr (buf,"| " ); +strbuf_addstr (buf," | " ); }strbuf_addf (buf,"d%d | " ,tr2_sid_depth ());
@@ -102,7 +121,7 @@ static void perf_fmt_prepare(const char *event_name,strbuf_addf (buf,"r%d " , repo->trace2_repo_id);while (buf->len < len)strbuf_addch (buf,' ' ); -strbuf_addstr (buf,"| " ); +strbuf_addstr (buf," | " );if (p_us_elapsed_absolute) strbuf_addf(buf, "%9.6f | ",
@@ -116,8 +135,8 @@ static void perf_fmt_prepare(const char *event_name,else strbuf_addf (buf,"%9s | " ," " ); -strbuf_addf (buf,"%-*s | " ,TR2FMT_PERF_CATEGORY_WIDTH , - (category ? category :"" )); +strbuf_addf (buf,"%-*.*s | " ,TR2FMT_PERF_CATEGORY_WIDTH , +TR2FMT_PERF_CATEGORY_WIDTH , (category ? category :"" ));if (ctx->nr_open_regions >0 ) {int len_indent =TR2_INDENT_LENGTH (ctx);
Signature · 1 line
gitgitgadget
PATCH v3 2/7 trace2: trim whitespace in region messages in perf target format
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Avoid unecessary trailing whitespace in "region_enter" and "region_leave" messages in perf target format.
--- trace2/tr2_tgt_perf.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
trace2/tr2_tgt_perf.c +10 -4
@@ -452,8 +452,11 @@ static void fn_region_enter_printf_va_fl(const char *file, int line,struct strbuf buf_payload =STRBUF_INIT ;if (label) -strbuf_addf (&buf_payload,"label:%s " , label); -maybe_append_string_va (&buf_payload, fmt, ap); +strbuf_addf (&buf_payload,"label:%s" , label); +if (fmt && *fmt) { +strbuf_addch (&buf_payload,' ' ); +maybe_append_string_va (&buf_payload, fmt, ap); + }perf_io_write_fl (file, line, event_name, repo, &us_elapsed_absolute,NULL , category, &buf_payload);
@@ -469,8 +472,11 @@ static void fn_region_leave_printf_va_fl(struct strbuf buf_payload =STRBUF_INIT ;if (label) -strbuf_addf (&buf_payload,"label:%s " , label); -maybe_append_string_va (&buf_payload, fmt, ap); +strbuf_addf (&buf_payload,"label:%s" , label); +if (fmt && *fmt) { +strbuf_addch (&buf_payload,' ' ); +maybe_append_string_va (&buf_payload, fmt, ap); + }perf_io_write_fl (file, line, event_name, repo, &us_elapsed_absolute, &us_elapsed_region, category, &buf_payload);
Signature · 1 line
gitgitgadget
PATCH v3 4/7 trace2: trim trailing whitespace in normal format error message
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Avoid creating unnecessary trailing whitespace in normal target format error messages when the message is omitted.
--- trace2/tr2_tgt_normal.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
trace2/tr2_tgt_normal.c +5 -2
@@ -142,8 +142,11 @@ static void fn_error_va_fl(const char *file, int line, const char *fmt, {struct strbuf buf_payload =STRBUF_INIT ; -strbuf_addstr (&buf_payload,"error " ); -maybe_append_string_va (&buf_payload, fmt, ap); +strbuf_addstr (&buf_payload,"error" ); +if (fmt && *fmt) { +strbuf_addch (&buf_payload,' ' ); +maybe_append_string_va (&buf_payload, fmt, ap); + }normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload); }
Signature · 1 line
gitgitgadget
PATCH v3 6/7 trace2: cleanup whitespace in normal format
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Make use of new sq_append_quote_argv_pretty() to normalize how we handle leading whitespace in normal format messages.
--- trace2/tr2_tgt_normal.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
trace2/tr2_tgt_normal.c +13 -10
@@ -87,7 +87,7 @@ static void fn_start_fl(const char *file, int line,struct strbuf buf_payload =STRBUF_INIT ;strbuf_addstr (&buf_payload,"start " ); -sq_quote_argv_pretty (&buf_payload, argv); +sq_append_quote_argv_pretty (&buf_payload, argv);normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload); }
@@ -186,8 +186,8 @@ static void fn_alias_fl(const char *file, int line, const char *alias, {struct strbuf buf_payload =STRBUF_INIT ; -strbuf_addf (&buf_payload,"alias %s ->" , alias); -sq_quote_argv_pretty (&buf_payload, argv); +strbuf_addf (&buf_payload,"alias %s -> " , alias); +sq_append_quote_argv_pretty (&buf_payload, argv);normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload); }
@@ -198,12 +198,12 @@ static void fn_child_start_fl(const char *file, int line, {struct strbuf buf_payload =STRBUF_INIT ; -strbuf_addf (&buf_payload,"child_start[%d] " , cmd->trace2_child_id); +strbuf_addf (&buf_payload,"child_start[%d]" , cmd->trace2_child_id);if (cmd->dir) { -strbuf_addstr (&buf_payload," cd" ); +strbuf_addstr (&buf_payload," cd " );sq_quote_buf_pretty (&buf_payload, cmd->dir); -strbuf_addstr (&buf_payload,"; " ); +strbuf_addstr (&buf_payload,";" ); }/*
@@ -211,9 +211,10 @@ static void fn_child_start_fl(const char *file, int line,* See trace_add_env() in run-command.c as used by original trace.c */ +strbuf_addch (&buf_payload,' ' );if (cmd->git_cmd) -strbuf_addstr (&buf_payload,"git" ); -sq_quote_argv_pretty (&buf_payload, cmd->argv); +strbuf_addstr (&buf_payload,"git " ); +sq_append_quote_argv_pretty (&buf_payload, cmd->argv);normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload);
@@ -238,9 +239,11 @@ static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,struct strbuf buf_payload =STRBUF_INIT ;strbuf_addf (&buf_payload,"exec[%d] " , exec_id); -if (exe) +if (exe) {strbuf_addstr (&buf_payload, exe); -sq_quote_argv_pretty (&buf_payload, argv); +strbuf_addch (&buf_payload,' ' ); + } +sq_append_quote_argv_pretty (&buf_payload, argv);normal_io_write_fl (file, line, &buf_payload);strbuf_release (&buf_payload); }
Signature · 1 line
gitgitgadget
PATCH v3 7/7 trace2: cleanup whitespace in perf format
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Make use of new sq_append_quote_argv_pretty() to normalize how we handle leading whitespace in perf format messages.
--- t/t0211-trace2-perf.sh | 4 ++-- trace2/tr2_tgt_perf.c | 31 ++++++++++++++++++++----------- 2 files changed, 22 insertions(+), 13 deletions(-)
t/t0211-trace2-perf.sh +2 -2
@@ -130,11 +130,11 @@ test_expect_success 'perf stream, child processes' ' d0|main|version|||||$V d0|main|start||_T_ABS_|||_EXE_ trace2 004child test-tool trace2 004child test-tool trace2 001return 0 d0|main|cmd_name|||||trace2 (trace2) - d0|main|child_start||_T_ABS_|||[ch0] class:? argv: test-tool trace2 004child test-tool trace2 001return 0 + d0|main|child_start||_T_ABS_|||[ch0] class:? argv:[test-tool trace2 004child test-tool trace2 001return 0] d1|main|version|||||$V d1|main|start||_T_ABS_|||_EXE_ trace2 004child test-tool trace2 001return 0 d1|main|cmd_name|||||trace2 (trace2/trace2) - d1|main|child_start||_T_ABS_|||[ch0] class:? argv: test-tool trace2 001return 0 + d1|main|child_start||_T_ABS_|||[ch0] class:? argv:[test-tool trace2 001return 0] d2|main|version|||||$V d2|main|start||_T_ABS_|||_EXE_ trace2 001return 0 d2|main|cmd_name|||||trace2 (trace2/trace2/trace2)
trace2/tr2_tgt_perf.c +20 -11
@@ -184,7 +184,7 @@ static void fn_start_fl(const char *file, int line,const char *event_name ="start" ;struct strbuf buf_payload =STRBUF_INIT ; -sq_quote_argv_pretty (&buf_payload, argv); +sq_append_quote_argv_pretty (&buf_payload, argv);perf_io_write_fl (file, line, event_name,NULL , &us_elapsed_absolute,NULL ,NULL , &buf_payload);
@@ -299,8 +299,9 @@ static void fn_alias_fl(const char *file, int line, const char *alias,const char *event_name ="alias" ;struct strbuf buf_payload =STRBUF_INIT ; -strbuf_addf (&buf_payload,"alias:%s argv:" , alias); -sq_quote_argv_pretty (&buf_payload, argv); +strbuf_addf (&buf_payload,"alias:%s argv:[" , alias); +sq_append_quote_argv_pretty (&buf_payload, argv); +strbuf_addch (&buf_payload,']' );perf_io_write_fl (file, line, event_name,NULL ,NULL ,NULL ,NULL , &buf_payload);
@@ -329,10 +330,14 @@ static void fn_child_start_fl(const char *file, int line,sq_quote_buf_pretty (&buf_payload, cmd->dir); } -strbuf_addstr (&buf_payload," argv:" ); -if (cmd->git_cmd) -strbuf_addstr (&buf_payload," git" ); -sq_quote_argv_pretty (&buf_payload, cmd->argv); +strbuf_addstr (&buf_payload," argv:[" ); +if (cmd->git_cmd) { +strbuf_addstr (&buf_payload,"git" ); +if (cmd->argv[0 ]) +strbuf_addch (&buf_payload,' ' ); + } +sq_append_quote_argv_pretty (&buf_payload, cmd->argv); +strbuf_addch (&buf_payload,']' );perf_io_write_fl (file, line, event_name,NULL , &us_elapsed_absolute,NULL ,NULL , &buf_payload);
@@ -383,10 +388,14 @@ static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,struct strbuf buf_payload =STRBUF_INIT ;strbuf_addf (&buf_payload,"id:%d " , exec_id); -strbuf_addstr (&buf_payload,"argv:" ); -if (exe) -strbuf_addf (&buf_payload," %s" , exe); -sq_quote_argv_pretty (&buf_payload, argv); +strbuf_addstr (&buf_payload,"argv:[" ); +if (exe) { +strbuf_addstr (&buf_payload, exe); +if (argv[0 ]) +strbuf_addch (&buf_payload,' ' ); + } +sq_append_quote_argv_pretty (&buf_payload, argv); +strbuf_addch (&buf_payload,']' );perf_io_write_fl (file, line, event_name,NULL , &us_elapsed_absolute,NULL ,NULL , &buf_payload);
Signature · 1 line
gitgitgadget
PATCH v3 3/7 trace2: remove dead code in maybe_add_string_va()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Remove an unnecessary "if" block in maybe_add_string_va().
Commit "ad006fe419e trace2: NULL is not allowed for va_list" changed "if (fmt && *fmt && ap)" to just "if (fmt && *fmt)" because it isn't safe to treat 'ap' as a pointer. This made the "if" block following it unnecessary.
--- trace2/tr2_tgt_event.c | 5 ----- trace2/tr2_tgt_normal.c | 5 ----- trace2/tr2_tgt_perf.c | 5 ----- 3 files changed, 15 deletions(-)
trace2/tr2_tgt_event.c +0 -5
@@ -205,11 +205,6 @@ static void maybe_add_string_va(struct json_writer *jw, const char *field_name,strbuf_release (&buf);return ; } - -if (fmt && *fmt) { -jw_object_string (jw, field_name, fmt); -return ; - } } static void fn_error_va_fl(const char *file, int line, const char *fmt,
trace2/tr2_tgt_normal.c +0 -5
@@ -135,11 +135,6 @@ static void maybe_append_string_va(struct strbuf *buf, const char *fmt,va_end (copy_ap);return ; } - -if (fmt && *fmt) { -strbuf_addstr (buf, fmt); -return ; - } } static void fn_error_va_fl(const char *file, int line, const char *fmt,
trace2/tr2_tgt_perf.c +0 -5
@@ -239,11 +239,6 @@ static void maybe_append_string_va(struct strbuf *buf, const char *fmt,va_end (copy_ap);return ; } - -if (fmt && *fmt) { -strbuf_addstr (buf, fmt); -return ; - } } static void fn_error_va_fl(const char *file, int line, const char *fmt,
Signature · 1 line
gitgitgadget
PATCH v3 5/7 quote: add sq_append_quote_argv_pretty()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
sq_quote_argv_pretty() builds a "pretty" string from the given argv. It inserts whitespace before each value, rather than just between them, so the resulting string always has a leading space. Lets give callers an option to not have the leading space or have to ltrim() it later.
Create sq_append_quote_argv_pretty() to convert an argv into a pretty, quoted if necessary, string with space delimiters and without a leading space.
Convert the existing sq_quote_argv_pretty() to use this new routine while preserving the leading space behavior.
--- quote.c | 18 +++++++++++++++++- quote.h | 1 + 2 files changed, 18 insertions(+), 1 deletion(-)
quote.c +17 -1
@@ -84,12 +84,28 @@ void sq_quote_argv(struct strbuf *dst, const char **argv) } } +/* +* Legacy function to append each argv value, quoted as necessasry, +* with whitespace before each value. This results in a leading +* space in the result. +*/ void sq_quote_argv_pretty (struct strbuf *dst,const char **argv) +{ +if (argv[0 ]) +strbuf_addch (dst,' ' ); +sq_append_quote_argv_pretty (dst, argv); +} + +/* +* Append each argv value, quoted as necessary, with whitespace between them. +*/ +void sq_append_quote_argv_pretty (struct strbuf *dst,const char **argv) {int i;for (i =0 ; argv[i]; i++) { -strbuf_addch (dst,' ' ); +if (i >0 ) +strbuf_addch (dst,' ' );sq_quote_buf_pretty (dst, argv[i]); } }
quote.h +1 -0
@@ -40,6 +40,7 @@ void sq_quotef(struct strbuf *, const char *fmt, ...);*/ void sq_quote_buf_pretty (struct strbuf *,const char *src);void sq_quote_argv_pretty (struct strbuf *,const char **argv); +void sq_append_quote_argv_pretty (struct strbuf *dst,const char **argv);/* This unwraps what sq_quote() produces in place, but returns * NULL if the input does not look like what sq_quote would have
Signature · 1 line
gitgitgadget
Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim
In reply to René ScharfeQuoted 47 lines · René Scharfe
Am 08.08.19 um 21:04 schrieb Jeff Hostetler:
Quoted 32 lines · Junio C Hamano
Quoted 20 lines
Having made the primary purpose of the helper clearer leads me to wonder if "do not add SP before the first element, i.e. argv[0]", is really what we want. If we always clear the *dst strbuf before starting to serialize argv[] into it, then the behaviour would make sense, but we do not---we are "appending".
As long as we are appending, would we be better off doing something sillily magical like this instead, I have to wonder?
void
sq_append_strings_quoted(struct strbuf *buf, const char **av){
int i;for (i = 0; av[i]; i++) {
if (buf->len)
strbuf_addch(buf, ' ');
sq_quote_buf_pretty(buf, argv[0]);
}
}Quoted 4 lines
That is, "if we are appending to an existing string, have SP to separate the first element from that existing string; treat the remaining elements the same way (if the buffer is empty, there is no point adding SP at the beginning)".
I don't think that would do what we want. We don't know what the caller's expectations are. In my uses in commits 6/7 and 7/7 I already added the leading chars I wanted in the strbuf before calling
sq_quote_argv_pretty_ltrim()and assumed the output would be a true append. For example:Quoted 7 lines
+
strbuf_addf(&buf_payload, "alias:%s argv:[", alias); +sq_quote_argv_pretty_ltrim(&buf_payload, argv); +strbuf_addch(&buf_payload, ']');I like your suggestion of putting my new function in the _append_ category. I think I'll add the 3rd arg to this and then it will be completely specified and I can get rid of the
_ltrimsuffix.Two observations:
If callers want to add something before a joined delimited list, they already can with a strbuf_add* call. No need to add that feature to a function that joins lists.
Quoted 5 lines
And repetitions of repetitions (loops) are boring.
Apologies in advance for any coffee stains on your monitor, but here's how I would start, probably followed by attempts to inline the functions that become trivial wrappers:
Um, yeah, I must say that I didn't expect the conversation to turn to map-style functions and a change in design styles. I think it would be better to have that conversation in a different patch series and not mix it with my trace2 janitoring.
I'm going to push a V3 that does just the minimum to have a sq_ function that joins the args with a space delimiter (and without the leading space) and re-write the existing function to call it after adding the legacy leading space. This will let existing callers continue to work as is. And they can be converted if/when anyone wants to dig into them.
Quoted 26 lines
--- quote.c | 18 ++++-------------- strbuf.c | 20 +++++++++++++------- strbuf.h | 8 ++++++++ 3 files changed, 25 insertions(+), 21 deletions(-)quote.c +2
-8@@ -74,24 +74,14 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ...)void sq_quote_argv (struct strbuf *dst,const char **argv) { -int i; - -/* Copy into destination buffer. */ -strbuf_grow (dst,255 ); -for (i =0 ; argv[i]; ++i) { -strbuf_addch (dst,' ' ); -sq_quote_buf (dst, argv[i]); - } +strbuf_addch (dst,' ' ); +strbuf_map_join_argv (dst, argv, sq_quote_buf," " ); }
quote.c void sq_quotef(struct strbuf *dst, const char *fmt, ...) · line 83
Quoted 10 lines
quote.c +1
-6void sq_quote_argv_pretty (struct strbuf *dst,const char **argv) { -int i; - -for (i =0 ; argv[i]; i++) { -strbuf_addch (dst,' ' ); -sq_quote_buf_pretty (dst, argv[i]); - } +strbuf_addch (dst,' ' );
If I'm reading this correctly, this has slightly different behavior than the original version. Perhaps:
if (argv[0]) strbuf_addch(dst, ' ');
Quoted 61 lines
quote.c +1
-0+strbuf_map_join_argv (dst, argv, sq_quote_buf_pretty," " ); }static char *sq_dequote_step (char *arg,char **next)strbuf.c +13
-7@@ -304,17 +304,23 @@ void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)strbuf_setlen (sb, sb->len + sb2->len); } +void strbuf_map_join_argv (struct strbuf *sb,const char **argv, +void (*fn)(struct strbuf *,const char *), +const char *separator) +{ +while (*argv) { +fn (sb, *argv++); +if (*argv) +strbuf_addstr (sb, separator); + } +} +const char *strbuf_join_argv (struct strbuf *buf,int argc,const char **argv,char delim) { -if (!argc) -return buf->buf; +char separator[] = { delim,'\0' }; -strbuf_addstr (buf, *argv); -while (--argc) { -strbuf_addch (buf, delim); -strbuf_addstr (buf, *(++argv)); - } +strbuf_map_join_argv (buf, argv, strbuf_addstr, separator);return buf->buf; }strbuf.h +8
-1@@ -297,6 +297,14 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)*/ void strbuf_addbuf (struct strbuf *sb,const struct strbuf *sb2); +/** +* Apply `fn` to `sb` and each element of the NULL-terminated array +* `argv`. Add `separator` between these invocations. +*/ +void strbuf_map_join_argv (struct strbuf *sb,const char **argv, +void (*fn)(struct strbuf *,const char *), +const char *separator); +/** * Join the arguments into a buffer. `delim` is put between every * two arguments. -- 2.22.0
Re: [PATCH v3 5/7] quote: add sq_append_quote_argv_pretty()
In reply to Jeff Hostetler via GitGitGadgetquote.c void sq_quote_argv(struct strbuf *dst, const char **argv) · line 96
Quoted 10 lines · Jeff Hostetler via GitGitGadget
quote.c +9
-0+/* +* Legacy function to append each argv value, quoted as necessasry, +* with whitespace before each value. This results in a leading +* space in the result. +*/ void sq_quote_argv_pretty (struct strbuf *dst,const char **argv) +{ +if (argv[0 ]) +strbuf_addch (dst,' ' ); +sq_append_quote_argv_pretty (dst, argv);
Micronit (not worth a reroll): if !argv[0], there is no need to call append, either.
Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim
In reply to Jeff HostetlerAm 09.08.19 um 19:13 schrieb Jeff Hostetler:
Quoted 11 lines · René Scharfe
Quoted 9 lines
void
sq_quote_argv_pretty(struct strbuf *dst, const char **argv){
- int i;
-
- for (i = 0; argv[i]; i++) {
-strbuf_addch(dst, ' ');
-sq_quote_buf_pretty(dst, argv[i]);
- }
+strbuf_addch(dst, ' ');
Quoted 5 lines
If I'm reading this correctly, this has slightly different behavior than the original version. Perhaps:
if (argv[0])
strbuf_addch(dst, ' ');
Oh, yes, thanks for spotting this.
René
Thanks. Will queue.
Re: [PATCH v3 5/7] quote: add sq_append_quote_argv_pretty()
In reply to Junio C HamanoQuoted 16 lines · Junio C Hamano
quote.c void sq_quote_argv(struct strbuf *dst, const char **argv) · line 96
Quoted 10 lines · Jeff Hostetler via GitGitGadget
quote.c +9
-0+/* +* Legacy function to append each argv value, quoted as necessasry, +* with whitespace before each value. This results in a leading +* space in the result. +*/ void sq_quote_argv_pretty (struct strbuf *dst,const char **argv) +{ +if (argv[0 ]) +strbuf_addch (dst,' ' ); +sq_append_quote_argv_pretty (dst, argv);Micronit (not worth a reroll): if !argv[0], there is no need to call append, either.
good catch. thanks!