diff options
| author | Ævar Arnfjörð Bjarmason <avarab@gmail.com> | 2021-11-26 01:52:22 +0300 |
|---|---|---|
| committer | Junio C Hamano <gitster@pobox.com> | 2021-11-26 09:15:07 +0300 |
| commit | d3b2159712019a06f1f495d3e42bd6aa6e76e848 (patch) | |
| tree | 4004da77565772efada1d01d9b34c6613b57e083 /trace2 | |
| parent | 7f14609e296d5737f34607d4c3e40bb1b063ef04 (diff) | |
run-command API: remove "argv" member, always use "args"
Remove the "argv" member from the run-command API, ever since "args"
was added in c460c0ecdca (run-command: store an optional argv_array,
2014-05-15) being able to provide either "argv" or "args" has led to
some confusion and bugs.
If we hadn't gone in that direction and only had an "argv" our
problems wouldn't have been solved either, as noted in [1] (and in the
documentation amended here) it comes with inherent memory management
issues: The caller would have to hang on to the "argv" until the
run-command API was finished. If the "argv" was an argument to main()
this wasn't an issue, but if it it was manually constructed using the
API might be painful.
We also have a recent report[2] of a user of the API segfaulting,
which is a direct result of it being complex to use. This commit
addresses the root cause of that bug.
This change is larger than I'd like, but there's no easy way to avoid
it that wouldn't involve even more verbose intermediate steps. We use
the "argv" as the source of truth over the "args", so we need to
change all parts of run-command.[ch] itself, as well as the trace2
logging at the same time.
The resulting Windows-specific code in start_command() is a bit nasty,
as we're now assigning to a strvec's "v" member, instead of to our own
"argv". There was a suggestion of some alternate approaches in reply
to an earlier version of this commit[3], but let's leave larger a
larger and needless refactoring of this code for now.
1. http://lore.kernel.org/git/YT6BnnXeAWn8BycF@coredump.intra.peff.net
2. https://lore.kernel.org/git/20211120194048.12125-1-ematsumiya@suse.de/
3. https://lore.kernel.org/git/patch-5.5-ea1011f7473-20211122T153605Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Diffstat (limited to 'trace2')
| -rw-r--r-- | trace2/tr2_tgt_event.c | 2 | ||||
| -rw-r--r-- | trace2/tr2_tgt_normal.c | 2 | ||||
| -rw-r--r-- | trace2/tr2_tgt_perf.c | 4 |
3 files changed, 4 insertions, 4 deletions
diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c index 3a0014417c..bd17ecdc32 100644 --- a/trace2/tr2_tgt_event.c +++ b/trace2/tr2_tgt_event.c @@ -354,7 +354,7 @@ static void fn_child_start_fl(const char *file, int line, jw_object_inline_begin_array(&jw, "argv"); if (cmd->git_cmd) jw_array_string(&jw, "git"); - jw_array_argv(&jw, cmd->argv); + jw_array_argv(&jw, cmd->args.v); jw_end(&jw); jw_end(&jw); diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c index 58d9e430f0..6e429a3fb9 100644 --- a/trace2/tr2_tgt_normal.c +++ b/trace2/tr2_tgt_normal.c @@ -232,7 +232,7 @@ static void fn_child_start_fl(const char *file, int line, strbuf_addch(&buf_payload, ' '); if (cmd->git_cmd) strbuf_addstr(&buf_payload, "git "); - sq_append_quote_argv_pretty(&buf_payload, cmd->argv); + sq_append_quote_argv_pretty(&buf_payload, cmd->args.v); normal_io_write_fl(file, line, &buf_payload); strbuf_release(&buf_payload); diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c index e4acca13d6..2ff9cf7083 100644 --- a/trace2/tr2_tgt_perf.c +++ b/trace2/tr2_tgt_perf.c @@ -335,10 +335,10 @@ static void fn_child_start_fl(const char *file, int line, strbuf_addstr(&buf_payload, " argv:["); if (cmd->git_cmd) { strbuf_addstr(&buf_payload, "git"); - if (cmd->argv[0]) + if (cmd->args.nr) strbuf_addch(&buf_payload, ' '); } - sq_append_quote_argv_pretty(&buf_payload, cmd->argv); + sq_append_quote_argv_pretty(&buf_payload, cmd->args.v); strbuf_addch(&buf_payload, ']'); perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute, |