| Message ID | ea061164-b36b-485c-963f-8c13e813a47e@web.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | run-command: remove run_command_v_*() | expand |
Am 30.10.22 um 12:40 schrieb René Scharfe: > Replace the convenience functions run_command_v_opt() et. al. and use > struct child_process and run_command() directly instead, for an overall > code reduction and a simpler and more flexible API that allows creating > argument lists without magic numbers and reduced risk of memory leaks. > > Changes since v1: > - Do the return value fix earlier; it was only an afterthought before. > Keep the colon (no "while at it, ..."). > - Break out the xstrdup(), oid_to_hex_r() and C99 cleanups. > - Convert tricky string arrays before strvecs because Ævar didn't like > the opposite order. > - Extend the example code in tmp-objdir.h so it still only requires > "cmd". Forgot one: - Fix grammar error in run-command.h added by the series in a comment that goes away at the end. René
On Sun, Oct 30, 2022 at 12:58:13PM +0100, René Scharfe wrote: > > Changes since v1: > > - Do the return value fix earlier; it was only an afterthought before. > > Keep the colon (no "while at it, ..."). > > - Break out the xstrdup(), oid_to_hex_r() and C99 cleanups. > > - Convert tricky string arrays before strvecs because Ævar didn't like > > the opposite order. > > - Extend the example code in tmp-objdir.h so it still only requires > > "cmd". > > Forgot one: > - Fix grammar error in run-command.h added by the series in a comment > that goes away at the end. > > René Thanks, replaced. This round is looking good to me, though let's hear from ÆVar before we start merging this down. Thanks, Taylor
On Sun, Oct 30 2022, Taylor Blau wrote: > On Sun, Oct 30, 2022 at 12:58:13PM +0100, René Scharfe wrote: >> > Changes since v1: >> > - Do the return value fix earlier; it was only an afterthought before. >> > Keep the colon (no "while at it, ..."). >> > - Break out the xstrdup(), oid_to_hex_r() and C99 cleanups. >> > - Convert tricky string arrays before strvecs because Ævar didn't like >> > the opposite order. >> > - Extend the example code in tmp-objdir.h so it still only requires >> > "cmd". >> >> Forgot one: >> - Fix grammar error in run-command.h added by the series in a comment >> that goes away at the end. >> >> René > > Thanks, replaced. This round is looking good to me, though let's hear > from ÆVar before we start merging this down. This LGTM. I don't think any of these are worth a re-roll, but just to clarify in reply to the comment in the v2 CL: * I thought the C99-specific stuff, free() etc. might be worth splitting out[1], but not to the extent of churning through first moving the existing API use around, and then converting it to run_command(). I.e. v1 had e.g. the am.c change as part of the large 3/8. Here there's 3/12, and we then re-visit that same caller in 12/12. I think that part of the v2 was better in v1. I.e. maybe split them out, but to the extent that it needs special explanation we could have just done it in one go with some explanation.. * I suggested just squashing what's now the v2's 08-11/12 into the respective commits that represented the last API user, or if they're worth splitting out do split-out with that last API user converted (and also removing the run-commit.h flags that went unused then, if applicable). Anyway, LGTM, and the stuff that was odd in the end state (e.g. 02/12) is now fixed. Thanks both! https://lore.kernel.org/git/Y11%2F2hyApN5NLruq@nand.local/T/#md2b7af02b1af34fa118779d3e1f4444604f95cb9
Replace the convenience functions run_command_v_opt() et. al. and use struct child_process and run_command() directly instead, for an overall code reduction and a simpler and more flexible API that allows creating argument lists without magic numbers and reduced risk of memory leaks. Changes since v1: - Do the return value fix earlier; it was only an afterthought before. Keep the colon (no "while at it, ..."). - Break out the xstrdup(), oid_to_hex_r() and C99 cleanups. - Convert tricky string arrays before strvecs because Ævar didn't like the opposite order. - Extend the example code in tmp-objdir.h so it still only requires "cmd". Ævar Arnfjörð Bjarmason (1): merge: remove always-the-same "verbose" arguments René Scharfe (11): run-command: fix return value comment am: simplify building "show" argument list bisect: simplify building "checkout" argument list bisect--helper: factor out do_bisect_run() sequencer: simplify building argument list in do_exec() use child_process member "args" instead of string array variable use child_process members "args" and "env" directly replace and remove run_command_v_opt_cd_env() replace and remove run_command_v_opt_tr2() replace and remove run_command_v_opt_cd_env_tr2() replace and remove run_command_v_opt() add-interactive.c | 9 ++- bisect.c | 12 ++-- builtin/add.c | 19 +++-- builtin/am.c | 12 ++-- builtin/bisect--helper.c | 68 +++++++++--------- builtin/clone.c | 41 ++++++----- builtin/difftool.c | 24 ++++--- builtin/fetch.c | 9 ++- builtin/gc.c | 55 ++++++++++----- builtin/merge-index.c | 4 +- builtin/merge.c | 53 ++++++-------- builtin/pull.c | 147 +++++++++++++++++++-------------------- builtin/remote.c | 40 +++++------ compat/mingw.c | 11 +-- diff.c | 27 ++++--- fsmonitor-ipc.c | 10 ++- git.c | 15 ++-- ll-merge.c | 7 +- merge.c | 18 ++--- run-command.c | 35 ---------- run-command.h | 34 +-------- scalar.c | 13 ++-- sequencer.c | 32 ++++----- shell.c | 17 +++-- t/helper/test-fake-ssh.c | 7 +- t/helper/test-trace2.c | 4 +- tmp-objdir.h | 6 +- 27 files changed, 346 insertions(+), 383 deletions(-) Range-Diff gegen v1: 1: c0b2b88500 = 1: 113a9e0a81 merge: remove always-the-same "verbose" arguments -: ---------- > 2: d10e70460b run-command: fix return value comment -: ---------- > 3: c8cd913e39 am: simplify building "show" argument list -: ---------- > 4: 4bb142e4a9 bisect: simplify building "checkout" argument list 2: 387db545d1 = 5: 5fcbe94eb4 bisect--helper: factor out do_bisect_run() -: ---------- > 6: 962403cf22 sequencer: simplify building argument list in do_exec() 4: 348bc6d32c ! 7: f1689abe85 use child_process member "args" instead of string array variable @@ Commit message Use run_command() with a struct child_process variable and populate its "args" member directly instead of building a string array and passing it - to run_command_v_opt(). This avoids the use of magic index numbers. - - ## bisect.c ## -@@ bisect.c: static struct oid_array skipped_revs; - - static struct object_id *current_bad_oid; - --static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL}; -- - static const char *term_bad; - static const char *term_good; - -@@ bisect.c: static int is_expected_rev(const struct object_id *oid) - enum bisect_error bisect_checkout(const struct object_id *bisect_rev, - int no_checkout) - { -- char bisect_rev_hex[GIT_MAX_HEXSZ + 1]; - struct commit *commit; - struct pretty_print_context pp = {0}; - struct strbuf commit_msg = STRBUF_INIT; - -- oid_to_hex_r(bisect_rev_hex, bisect_rev); - update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR); - -- argv_checkout[2] = bisect_rev_hex; - if (no_checkout) { - update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0, - UPDATE_REFS_DIE_ON_ERR); - } else { -- if (run_command_v_opt(argv_checkout, RUN_GIT_CMD)) -+ struct child_process cmd = CHILD_PROCESS_INIT; -+ -+ cmd.git_cmd = 1; -+ strvec_pushl(&cmd.args, "checkout", "-q", -+ oid_to_hex(bisect_rev), "--", NULL); -+ if (run_command(&cmd)) - /* - * Errors in `run_command()` itself, signaled by res < 0, - * and errors in the child process, signaled by res > 0 - - ## builtin/am.c ## -@@ builtin/am.c: static int show_patch(struct am_state *state, enum show_patch_type sub_mode) - int len; - - if (!is_null_oid(&state->orig_commit)) { -- const char *av[4] = { "show", NULL, "--", NULL }; -- char *new_oid_str; -- int ret; -+ struct child_process cmd = CHILD_PROCESS_INIT; - -- av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit)); -- ret = run_command_v_opt(av, RUN_GIT_CMD); -- free(new_oid_str); -- return ret; -+ strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit), -+ "--", NULL); -+ cmd.git_cmd = 1; -+ return run_command(&cmd); - } - - switch (sub_mode) { + to run_command_v_opt(). This avoids the use of magic index numbers and + makes simplifies the possible addition of more arguments in the future. ## builtin/difftool.c ## @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, @@ ll-merge.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive if (fd < 0) goto bad; - ## sequencer.c ## -@@ sequencer.c: static int error_failed_squash(struct repository *r, - - static int do_exec(struct repository *r, const char *command_line) - { -- const char *child_argv[] = { NULL, NULL }; -+ struct child_process cmd = CHILD_PROCESS_INIT; - int dirty, status; - - fprintf(stderr, _("Executing: %s\n"), command_line); -- child_argv[0] = command_line; -- status = run_command_v_opt(child_argv, RUN_USING_SHELL); -+ cmd.use_shell = 1; -+ strvec_push(&cmd.args, command_line); -+ status = run_command(&cmd); - - /* force re-reading of the cache */ - if (discard_index(r->index) < 0 || repo_read_index(r) < 0) - ## t/helper/test-fake-ssh.c ## @@ t/helper/test-fake-ssh.c: int cmd_main(int argc, const char **argv) struct strbuf buf = STRBUF_INIT; 3: 7745e16df4 = 8: a467a4ecb5 use child_process members "args" and "env" directly 5: eeaa6aa86d ! 9: dc27358775 replace and remove run_command_v_opt_cd_env() @@ run-command.h @@ run-command.h: struct child_process { /** - * The functions: child_process_init, start_command, finish_command, -- * run_command, run_command_v_opt, run_command_v_opt_cd_env, child_process_clear -- * do the following: -+ * run_command, run_command_v_opt, child_process_clear do the following: + * The functions: start_command, finish_command, run_command, +- * run_command_v_opt, run_command_v_opt_cd_env do the following: ++ * run_command_v_opt do the following: * * - If a system call failed, errno is set and -1 is returned. A diagnostic * is printed. @@ run-command.h: int run_command_v_opt_tr2(const char **argv, int opt, const char ## tmp-objdir.h ## @@ + * * Example: * ++ * struct child_process child = CHILD_PROCESS_INIT; * struct tmp_objdir *t = tmp_objdir_create("incoming"); - * if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) && - * !tmp_objdir_migrate(t)) -+ * strvec_pushv(&cmd.env, tmp_objdir_env(t)); -+ * if (!run_command(&cmd)) && !tmp_objdir_migrate(t)) ++ * strvec_push(&child.args, cmd); ++ * strvec_pushv(&child.env, tmp_objdir_env(t)); ++ * if (!run_command(&child)) && !tmp_objdir_migrate(t)) * printf("success!\n"); * else * die("failed...tmp_objdir will clean up for us"); 6: 95b5dcbb66 = 10: dcd65580c6 replace and remove run_command_v_opt_tr2() 7: 5e111ab053 ! 11: 389ec8ef54 replace and remove run_command_v_opt_cd_env_tr2() @@ run-command.h: int run_auto_maintenance(int quiet); /** - * Convenience functions that encapsulate a sequence of -+ * Convenience function that encapsulate a sequence of ++ * Convenience function that encapsulates a sequence of * start_command() followed by finish_command(). The argument argv * specifies the program and its arguments. The argument opt is zero * or more of the flags `RUN_COMMAND_NO_STDIN`, `RUN_GIT_CMD`, 8: f1f438d724 ! 12: a6e7135e31 replace and remove run_command_v_opt() @@ Commit message Suggested-by: Jeff King <peff@peff.net> + ## bisect.c ## +@@ bisect.c: enum bisect_error bisect_checkout(const struct object_id *bisect_rev, + update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0, + UPDATE_REFS_DIE_ON_ERR); + } else { +- const char *argv_checkout[] = { +- "checkout", "-q", oid_to_hex(bisect_rev), "--", NULL +- }; ++ struct child_process cmd = CHILD_PROCESS_INIT; + +- if (run_command_v_opt(argv_checkout, RUN_GIT_CMD)) ++ cmd.git_cmd = 1; ++ strvec_pushl(&cmd.args, "checkout", "-q", ++ oid_to_hex(bisect_rev), "--", NULL); ++ if (run_command(&cmd)) + /* + * Errors in `run_command()` itself, signaled by res < 0, + * and errors in the child process, signaled by res > 0 + + ## builtin/am.c ## +@@ builtin/am.c: static int show_patch(struct am_state *state, enum show_patch_type sub_mode) + int len; + + if (!is_null_oid(&state->orig_commit)) { +- const char *av[] = { +- "show", oid_to_hex(&state->orig_commit), "--", NULL +- }; ++ struct child_process cmd = CHILD_PROCESS_INIT; + +- return run_command_v_opt(av, RUN_GIT_CMD); ++ strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit), ++ "--", NULL); ++ cmd.git_cmd = 1; ++ return run_command(&cmd); + } + + switch (sub_mode) { + ## builtin/bisect--helper.c ## @@ builtin/bisect--helper.c: static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a strbuf_read_file(&start_head, git_path_bisect_start(), 0); @@ run-command.c: int run_command(struct child_process *cmd) ## run-command.h ## @@ run-command.h: struct child_process { + } /** - * The functions: child_process_init, start_command, finish_command, -- * run_command, run_command_v_opt, child_process_clear do the following: -+ * run_command, child_process_clear do the following: +- * The functions: start_command, finish_command, run_command, +- * run_command_v_opt do the following: ++ * The functions: start_command, finish_command, run_command do the following: * * - If a system call failed, errno is set and -1 is returned. A diagnostic * is printed. @@ run-command.h: int run_command(struct child_process *); -#define RUN_CLOSE_OBJECT_STORE (1<<7) - -/** -- * Convenience function that encapsulate a sequence of +- * Convenience function that encapsulates a sequence of - * start_command() followed by finish_command(). The argument argv - * specifies the program and its arguments. The argument opt is zero - * or more of the flags `RUN_COMMAND_NO_STDIN`, `RUN_GIT_CMD`, @@ run-command.h: int run_command(struct child_process *); * Execute the given command, sending "in" to its stdin, and capturing its * stdout and stderr in the "out" and "err" strbufs. Any of the three may + ## sequencer.c ## +@@ sequencer.c: static int error_failed_squash(struct repository *r, + + static int do_exec(struct repository *r, const char *command_line) + { +- const char *child_argv[] = { command_line, NULL }; ++ struct child_process cmd = CHILD_PROCESS_INIT; + int dirty, status; + + fprintf(stderr, _("Executing: %s\n"), command_line); +- status = run_command_v_opt(child_argv, RUN_USING_SHELL); ++ cmd.use_shell = 1; ++ strvec_push(&cmd.args, command_line); ++ status = run_command(&cmd); + + /* force re-reading of the cache */ + if (discard_index(r->index) < 0 || repo_read_index(r) < 0) + ## shell.c ## @@ shell.c: static void cd_to_homedir(void) static void run_shell(void) 9: 59677c9598 < -: ---------- run-command: fix return value comment -- 2.38.1