[PATCH 0/3] trace2: clean up formatting in perf target format

[PATCH 0/3] trace2: clean up formatting in perf target format

[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

 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
-- 
gitgitgadget

[PATCH 1/3] trace2: cleanup column alignment in perf target format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Truncate/elide very long "filename:linenumber" field.
Truncate region and data "category" field if necessary.
Adjust overall column widths.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_perf.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index ea0cbbe13e..4a9d99218b 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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);
-- 
gitgitgadget

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

[Jeff Hostetler via GitGitGadget]

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.

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);
-- 
gitgitgadget

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

[Junio C Hamano]

"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);

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

[Jeff Hostetler]

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

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

[Junio C Hamano]

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.

[PATCH 3/3] trace2: trim whitespace in region messages in perf target format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Trim trailing whitespace in "region_enter" and "region_leave"
messages in perf target format.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_perf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index ed4e708f28..3c7ffbeb8a 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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);
-- 
gitgitgadget

Re: [PATCH 0/3] trace2: clean up formatting in perf target format

[Josh Steadmon]

On 2019.07.31 13:04, Jeff Hostetler via GitGitGadget wrote:
> 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
> -- 
> gitgitgadget

The series looks good to me.

[PATCH v2 0/7] trace2: clean up formatting in perf target format

[Jeff Hostetler via GitGitGadget]

V2 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

-- 
gitgitgadget

[PATCH v2 1/7] trace2: cleanup column alignment in perf target format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Truncate/elide very long "filename:linenumber" field.
Truncate region and data "category" field if necessary.
Adjust overall column widths.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_perf.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index ea0cbbe13e..4a9d99218b 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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);
-- 
gitgitgadget

[PATCH v2 2/7] trace2: trim whitespace in region messages in perf target format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Avoid unecessary trailing whitespace in "region_enter" and "region_leave"
messages in perf target format.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_perf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index 4a9d99218b..fb845cb627 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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);
-- 
gitgitgadget

[PATCH v2 3/7] trace2: remove dead code in maybe_add_string_va()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_event.c  | 5 -----
 trace2/tr2_tgt_normal.c | 5 -----
 trace2/tr2_tgt_perf.c   | 5 -----
 3 files changed, 15 deletions(-)

diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index c2852d1bd2..9bcac20d1b 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -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,
diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index 00b116d797..47a1882557 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -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,
diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index fb845cb627..8e52395de2 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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,
-- 
gitgitgadget

[PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create version of sq_quote_argv_pretty() that does not
insert a leading space before argv[0].

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 quote.c | 11 +++++++++++
 quote.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/quote.c b/quote.c
index 7f2aa6faa4..7cad8798ac 100644
--- a/quote.c
+++ b/quote.c
@@ -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;
diff --git a/quote.h b/quote.h
index fb08dc085c..3b3d041a61 100644
--- a/quote.h
+++ b/quote.h
@@ -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
-- 
gitgitgadget

Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim

[Junio C Hamano]

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

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Create version of sq_quote_argv_pretty() that does not
> insert a leading space before argv[0].
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  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)?

> diff --git a/quote.c b/quote.c
> index 7f2aa6faa4..7cad8798ac 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -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;
> diff --git a/quote.h b/quote.h
> index fb08dc085c..3b3d041a61 100644
> --- a/quote.h
> +++ b/quote.h
> @@ -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

[Jeff Hostetler]

On 8/8/2019 2:05 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Create version of sq_quote_argv_pretty() that does not
>> insert a leading space before argv[0].
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> ---
>>   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?

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.

> 
> 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 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.

> 
> 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


> 
>> diff --git a/quote.c b/quote.c
>> index 7f2aa6faa4..7cad8798ac 100644
>> --- a/quote.c
>> +++ b/quote.c
>> @@ -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;
>> diff --git a/quote.h b/quote.h
>> index fb08dc085c..3b3d041a61 100644
>> --- a/quote.h
>> +++ b/quote.h
>> @@ -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

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

>> 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.

>> 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

[René Scharfe]

Am 08.08.19 um 21:04 schrieb Jeff Hostetler:
> On 8/8/2019 2:05 PM, Junio C Hamano wrote:
>> 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 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.

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(-)

diff --git a/quote.c b/quote.c
index 7f2aa6faa4..f422188852 100644
--- a/quote.c
+++ b/quote.c
@@ -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)
diff --git a/strbuf.c b/strbuf.c
index d30f916858..d337853b53 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -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;
 }
diff --git a/strbuf.h b/strbuf.h
index f62278a0be..7adeff94a7 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -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 v2 5/7] quote: add sq_quote_argv_pretty_ltrim

[Jeff Hostetler]

On 8/8/2019 6:49 PM, René Scharfe wrote:
> Am 08.08.19 um 21:04 schrieb Jeff Hostetler:
>> On 8/8/2019 2:05 PM, Junio C Hamano wrote:
>>> 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 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.
> 
> 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:


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.


> 
> ---
>   quote.c  | 18 ++++--------------
>   strbuf.c | 20 +++++++++++++-------
>   strbuf.h |  8 ++++++++
>   3 files changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/quote.c b/quote.c
> index 7f2aa6faa4..f422188852 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -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, ' ');

If I'm reading this correctly, this has slightly different behavior
than the original version.  Perhaps:

	if (argv[0])
		strbuf_addch(dst, ' ');

> +	strbuf_map_join_argv(dst, argv, sq_quote_buf_pretty, " ");
>   }
> 
>   static char *sq_dequote_step(char *arg, char **next)
> diff --git a/strbuf.c b/strbuf.c
> index d30f916858..d337853b53 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -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;
>   }
> diff --git a/strbuf.h b/strbuf.h
> index f62278a0be..7adeff94a7 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -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 v2 5/7] quote: add sq_quote_argv_pretty_ltrim

[René Scharfe]

Am 09.08.19 um 19:13 schrieb Jeff Hostetler:
> On 8/8/2019 6:49 PM, René Scharfe wrote:
>>   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, ' ');
>
> 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é

[PATCH v2 4/7] trace2: trim trailing whitespace in normal format error message

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Avoid creating unnecessary trailing whitespace in normal
target format error messages when the message is omitted.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_normal.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index 47a1882557..213724d5cb 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -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);
 }
-- 
gitgitgadget

[PATCH v2 6/7] trace2: cleanup whitespace in normal format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Make use of new sq_quote_argv_pretty_ltrim() to normalize
how we handle leading whitespace in normal format messages.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_normal.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index 213724d5cb..5de2f8214c 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -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);
 }
-- 
gitgitgadget

[PATCH v2 7/7] trace2: cleanup whitespace in perf format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Make use of new sq_quote_argv_pretty_ltrim() to normalize
how we handle leading whitespace in perf format messages.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t0211-trace2-perf.sh |  4 ++--
 trace2/tr2_tgt_perf.c  | 31 ++++++++++++++++++++-----------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 2c3ad6e8c1..6ee8ee3b67 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -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)
diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index 8e52395de2..59c03a5ed4 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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);
-- 
gitgitgadget

[PATCH v3 0/7] trace2: clean up formatting in perf target format

[Jeff Hostetler via GitGitGadget]

V2 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 <jeffhost@microsoft.com>
     @@ -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 <jeffhost@microsoft.com>
     @@ -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,

-- 
gitgitgadget

[PATCH v3 2/7] trace2: trim whitespace in region messages in perf target format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Avoid unecessary trailing whitespace in "region_enter" and "region_leave"
messages in perf target format.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_perf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index 4a9d99218b..fb845cb627 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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);
-- 
gitgitgadget

[PATCH v3 1/7] trace2: cleanup column alignment in perf target format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Truncate/elide very long "filename:linenumber" field.
Truncate region and data "category" field if necessary.
Adjust overall column widths.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_perf.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index ea0cbbe13e..4a9d99218b 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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);
-- 
gitgitgadget

[PATCH v3 3/7] trace2: remove dead code in maybe_add_string_va()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_event.c  | 5 -----
 trace2/tr2_tgt_normal.c | 5 -----
 trace2/tr2_tgt_perf.c   | 5 -----
 3 files changed, 15 deletions(-)

diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index c2852d1bd2..9bcac20d1b 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -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,
diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index 00b116d797..47a1882557 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -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,
diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index fb845cb627..8e52395de2 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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,
-- 
gitgitgadget

[PATCH v3 4/7] trace2: trim trailing whitespace in normal format error message

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Avoid creating unnecessary trailing whitespace in normal
target format error messages when the message is omitted.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_normal.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index 47a1882557..213724d5cb 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -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);
 }
-- 
gitgitgadget

[PATCH v3 5/7] quote: add sq_append_quote_argv_pretty()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 quote.c | 18 +++++++++++++++++-
 quote.h |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/quote.c b/quote.c
index 7f2aa6faa4..c8ba6b397a 100644
--- a/quote.c
+++ b/quote.c
@@ -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]);
 	}
 }
diff --git a/quote.h b/quote.h
index fb08dc085c..ca8ee3144a 100644
--- a/quote.h
+++ b/quote.h
@@ -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
-- 
gitgitgadget

Re: [PATCH v3 5/7] quote: add sq_append_quote_argv_pretty()

[Junio C Hamano]

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

> +/*
> + * 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 v3 5/7] quote: add sq_append_quote_argv_pretty()

[Jeff Hostetler]

On 8/9/2019 1:54 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +/*
>> + * 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!

[PATCH v3 6/7] trace2: cleanup whitespace in normal format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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 <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_normal.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index 213724d5cb..438ed05a40 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -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);
 }
-- 
gitgitgadget

[PATCH v3 7/7] trace2: cleanup whitespace in perf format

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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 <jeffhost@microsoft.com>
---
 t/t0211-trace2-perf.sh |  4 ++--
 trace2/tr2_tgt_perf.c  | 31 ++++++++++++++++++++-----------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 2c3ad6e8c1..6ee8ee3b67 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -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)
diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index 8e52395de2..fd979db4ad 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -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);
-- 
gitgitgadget

Re: [PATCH v3 0/7] trace2: clean up formatting in perf target format

[Junio C Hamano]

Thanks.  Will queue.