git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 0/12] run-command: remove run_command_v_*()

From
René Scharfe <l.s.r@web.de>
Date
Oct 30, 2022, 11:40 UTC
Message-ID
<ea061164-b36b-485c-963f-8c13e813a47e@web.de>
In-Reply-To
<7407e074-4bd8-b351-7fa4-baf59b41880c@web.de>

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
Previous: Taylor BlauNext: René Scharfe
Message 34 of 49 in “run-command: remove run_command_v_*()”
  1. 0/8 run-command: remove run_command_v_*()René Scharfe, Oct 27, 2022
  2. 1/8 merge: remove always-the-same "verbose" argumentsRené Scharfe, Oct 27, 2022
  3. Taylor BlauOct 29, 2022
  4. 2/8 bisect--helper: factor out do_bisect_run()René Scharfe, Oct 27, 2022
  5. Ævar Arnfjörð BjarmasonOct 27, 2022
  6. Taylor BlauOct 29, 2022
  7. 3/8 use child_process members "args" and "env" directlyRené Scharfe, Oct 27, 2022
  8. Junio C HamanoOct 27, 2022
  9. Ævar Arnfjörð BjarmasonOct 27, 2022
  10. 4/8 use child_process member "args" instead of string array variableRené Scharfe, Oct 27, 2022
  11. Ævar Arnfjörð BjarmasonOct 27, 2022
  12. René ScharfeOct 28, 2022
  13. Taylor BlauOct 29, 2022
  14. Taylor BlauOct 29, 2022
  15. 5/8 replace and remove run_command_v_opt_cd_env()René Scharfe, Oct 27, 2022
  16. Ævar Arnfjörð BjarmasonOct 27, 2022
  17. Taylor BlauOct 29, 2022
  18. 6/8 replace and remove run_command_v_opt_tr2()René Scharfe, Oct 27, 2022
  19. 7/8 replace and remove run_command_v_opt_cd_env_tr2()René Scharfe, Oct 27, 2022
  20. Ævar Arnfjörð BjarmasonOct 27, 2022
  21. René ScharfeOct 28, 2022
  22. 8/8 replace and remove run_command_v_opt()René Scharfe, Oct 27, 2022
  23. Ævar Arnfjörð BjarmasonOct 27, 2022
  24. René ScharfeOct 28, 2022
  25. Jeff KingOct 27, 2022
  26. René ScharfeOct 28, 2022
  27. Ævar Arnfjörð BjarmasonOct 27, 2022
  28. René ScharfeOct 28, 2022
  29. Ævar Arnfjörð BjarmasonOct 28, 2022
  30. René ScharfeOct 28, 2022
  31. Ævar Arnfjörð BjarmasonOct 29, 2022
  32. René ScharfeOct 29, 2022
  33. Taylor BlauOct 29, 2022
  34. 0/12 run-command: remove run_command_v_*()René Scharfe, Oct 30, 2022
  35. 01/12 merge: remove always-the-same "verbose" argumentsRené Scharfe, Oct 30, 2022
  36. 02/12 run-command: fix return value commentRené Scharfe, Oct 30, 2022
  37. 03/12 am: simplify building "show" argument listRené Scharfe, Oct 30, 2022
  38. 04/12 bisect: simplify building "checkout" argument listRené Scharfe, Oct 30, 2022
  39. 05/12 bisect--helper: factor out do_bisect_run()René Scharfe, Oct 30, 2022
  40. 06/12 sequencer: simplify building argument list in do_exec()René Scharfe, Oct 30, 2022
  41. 07/12 use child_process member "args" instead of string array variableRené Scharfe, Oct 30, 2022
  42. 08/12 use child_process members "args" and "env" directlyRené Scharfe, Oct 30, 2022
  43. 09/12 replace and remove run_command_v_opt_cd_env()René Scharfe, Oct 30, 2022
  44. 10/12 replace and remove run_command_v_opt_tr2()René Scharfe, Oct 30, 2022
  45. 11/12 replace and remove run_command_v_opt_cd_env_tr2()René Scharfe, Oct 30, 2022
  46. 12/12 replace and remove run_command_v_opt()René Scharfe, Oct 30, 2022
  47. René ScharfeOct 30, 2022
  48. Taylor BlauOct 30, 2022
  49. Ævar Arnfjörð BjarmasonOct 31, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.