Skip to content

Commit

Permalink
Merge branch 'jh/trace2-pretty-output'
Browse files Browse the repository at this point in the history 
Output from trace2 subsystem is formatted more prettily now.

* jh/trace2-pretty-output:
  trace2: cleanup whitespace in perf format
  trace2: cleanup whitespace in normal format
  quote: add sq_append_quote_argv_pretty()
  trace2: trim trailing whitespace in normal format error message
  trace2: remove dead code in maybe_add_string_va()
  trace2: trim whitespace in region messages in perf target format
  trace2: cleanup column alignment in perf target format
  • Loading branch information
@gitster
gitster committed Sep 30, 2019
2 parents 640f9cd + 742ed63 commit 93fc876
Show file tree
Hide file tree
Showing 6 changed files with 97 additions and 55 deletions.
18 changes: 17 additions & 1 deletion quote.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}
}
Expand Down
1 change: 1 addition & 0 deletions quote.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions t/t0211-trace2-perf.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 0 additions & 5 deletions trace2/tr2_tgt_event.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
35 changes: 18 additions & 17 deletions trace2/tr2_tgt_normal.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -135,20 +135,18 @@ 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,
va_list ap)
{
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);
}
Expand Down Expand Up @@ -188,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);
}
Expand All @@ -200,22 +198,23 @@ 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, ";");
}

/*
* TODO if (cmd->env) { Consider dumping changes to environment. }
* 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);
Expand All @@ -240,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);
}
Expand Down
89 changes: 59 additions & 30 deletions trace2/tr2_tgt_perf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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());
Expand All @@ -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 | ",
Expand All @@ -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);
Expand Down Expand Up @@ -165,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);
Expand Down Expand Up @@ -220,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,
Expand Down Expand Up @@ -285,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);
Expand Down Expand Up @@ -315,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);
Expand Down Expand Up @@ -369,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);
Expand Down Expand Up @@ -433,8 +456,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);
Expand All @@ -450,8 +476,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);
Expand Down

0 comments on commit 93fc876

Please sign in to comment.