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

Re: [PATCH 0/8] run-command: remove run_command_v_*()

From
René Scharfe <l.s.r@web.de>
Date
Oct 28, 2022, 16:04 UTC
Message-ID
<749f6adc-928a-0978-e3a1-2ede9f07def0@web.de>
In-Reply-To
<221028.86edusan0k.gmgdl@evledraar.gmail.com>
Am 27.10.22 um 23:46 schrieb Ævar Arnfjörð Bjarmason:
Show 40 quoted lines
>
> On Thu, Oct 27 2022, René Scharfe wrote:
>
>> 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.
>>
>> This is a broken-out and polished version of the original scratch at
>> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/
>>
>> Ævar Arnfjörð Bjarmason (1):
>>   merge: remove always-the-same "verbose" arguments
>>
>> René Scharfe (7):
>>   bisect--helper: factor out do_bisect_run()
>>   use child_process members "args" and "env" directly
>>   use child_process member "args" instead of string array variable
>>   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()
>
> Even though I had a an earlier alternate series series[1] for this I'd
> be happy to see this version go in. I left some comments here and there,
> but with/without a re-roll am happy to give this:
>
> 	Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> I think I would have just gone for this in the first place, but thought
> that people loved the convenience functions too much. It can be hard to
> judge sentiments in advance :)
>
> One reason I hadn't re-submitted something is that there were
> outstanding new conflicts with "seen", and I see from applying this
> topic & merging it that it conflicts somewhat heavily. Junio seems to be
> on-board with this though, so maybe he won't mind.
>
> I didn't see any glaring instances where this made things worse, so
> maybe we didn't need these convenience wrappers in the first place.
Right, and I think this is important.
Show 21 quoted lines
>
> But from the earlier discussion it does seema bit like we tossed the
> very notion out because some didn't like the duplicating of struct
> members with the flags (which I also doen't like).
>
> So I came up with the below experiment on top, it's not an attempt to
> convert all callers, just a demo.
>
> Maybe you think some ideas here are worth using, I probably won't pursue
> it (but maybe as ideas for some other future API).
>
> It's a combination of stuff, some of which you might like, some not,
> namely:
>
> - Have the API work the same way, but just have a run_commandl(&opt,
>   ...) in addition to the run_command(). It's just a trivial wrapper to
>   push stuff into &cmd.args for you.
>
> - I saw a few callers that could have perhaps used a similarly trivial
>   run_commandv(), but that doesn't benefit from LAST_ARG_MUST_BE_NULL,
>   so I didn't bother.

I thought about that as well, but at least for me is probably a just a case of loss aversion, which I have aplenty. run_command_v_opt() alone isn't bad and patch 8 on its own probably wouldn't fly, so I mentally cling to it. But without it the API is untangled and simpler. Only a single way to specify flags and arguments, no shortcuts for special combinations. Overall easier to use once we forget the old ways.

Show 9 quoted lines
>
> - I wish C had a nicer syntax for not just declaring but squashing
>   together compile_time bracketed lists (think set operations). But the
>   below "CHILD_PROCESS_INIT_LIST" is a pretty good poor man's version.
>
>   I see gcc/clang nicely give us safety rails for that with
>   "-Woverride-init", for this sort of "opts struct with internal stuff,
>   but also user options" I think it works out very nicely.
>
That's a nice and simple macro.  I played with a gross variant à la
  #define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }
which would allow e.g.
  struct child_process cmd = CHILD_PROCESS_INIT_EX(.git_cmd = 1);

Yours is better, but they share the downside of not actually saving any lines of code..

> - We have quite a few callers that want "on error, die", so maybe we
>   should have something like that "on_error" sooner than later.

We could add a die_on_error bit for that, or start_command_or_die() and run_command_or_die() variants (there I go again, multiplying APIs..). They could report the failed command, which a caller can't do because the internal strvec is already cleared once it learns of the failure.

Show 15 quoted lines
>
> On clever (but perhaps overly clever) thing I didn't use, but played
> with recently in another context, is that now with C99 you can also do:
>
> 	int run_commandl(struct child_process *cmd, ...);
> 	#define run_command(...) run_command_1(__VA_ARGS__, NULL)
>
> I.e. make the API itself support all of:
>
> 	run_command(&cmd);
> 	run_command(&cmd, "reboot");
> 	run_command(&cmd, "reboot", NULL);
>
> I haven't made up my mind on whether that's just overly clever, or
> outright insane :)

Neither, I'd say. By combining two operations it's less flexible than a pure run method plus direct access to a strvec member. It's a shortcut that may save a line but requires more effort to extend its callers, e.g. to conditionally add a new argument.

Show 282 quoted lines
>
> diff --git a/bisect.c b/bisect.c
> index ec7487e6836..ef4f80650f7 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -740,9 +740,8 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
>  		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))
> +		if (run_commandl(&cmd, "checkout", "-q",
> +				 oid_to_hex(bisect_rev), "--", NULL))
>  			/*
>  			 * Errors in `run_command()` itself, signaled by res < 0,
>  			 * and errors in the child process, signaled by res > 0
> diff --git a/builtin/am.c b/builtin/am.c
> index 20aea0d2487..3b7df32ce22 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2189,10 +2189,9 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
>  	if (!is_null_oid(&state->orig_commit)) {
>  		struct child_process cmd = CHILD_PROCESS_INIT;
>
> -		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
> -			     "--", NULL);
>  		cmd.git_cmd = 1;
> -		return run_command(&cmd);
> +		return run_commandl(&cmd, "show", oid_to_hex(&state->orig_commit),
> +				    "--", NULL);
>  	}
>
>  	switch (sub_mode) {
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1d2ce8a0e12..087d21c614a 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -764,12 +764,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>  		strbuf_trim(&start_head);
>  		if (!no_checkout) {
> -			struct child_process cmd = CHILD_PROCESS_INIT;
> -
> -			cmd.git_cmd = 1;
> -			strvec_pushl(&cmd.args, "checkout", start_head.buf,
> -				     "--", NULL);
> -			if (run_command(&cmd)) {
> +			struct child_process cmd = {
> +				CHILD_PROCESS_INIT_LIST,
> +				.git_cmd = 1,
> +			};
> +			if (run_commandl(&cmd, "checkout", start_head.buf,
> +					 "--", NULL)) {
>  				res = error(_("checking out '%s' failed."
>  						 " Try 'git bisect start "
>  						 "<valid-branch>'."),
> @@ -1147,8 +1147,7 @@ static int do_bisect_run(const char *command)
>
>  	printf(_("running %s\n"), command);
>  	cmd.use_shell = 1;
> -	strvec_push(&cmd.args, command);
> -	return run_command(&cmd);
> +	return run_commandl(&cmd, command, NULL);
>  }
>
>  static int verify_good(const struct bisect_terms *terms, const char *command)
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0e4348686b6..80d09e0fbf1 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -655,7 +655,6 @@ static int git_sparse_checkout_init(const char *repo)
>  {
>  	struct child_process cmd = CHILD_PROCESS_INIT;
>  	int result = 0;
> -	strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);
>
>  	/*
>  	 * We must apply the setting in the current process
> @@ -664,7 +663,7 @@ static int git_sparse_checkout_init(const char *repo)
>  	core_apply_sparse_checkout = 1;
>
>  	cmd.git_cmd = 1;
> -	if (run_command(&cmd)) {
> +	if (run_commandl(&cmd, "-C", repo, "sparse-checkout", "set", NULL)) {
>  		error(_("failed to initialize sparse-checkout"));
>  		result = 1;
>  	}
> @@ -868,13 +867,14 @@ static void dissociate_from_references(void)
>  	char *alternates = git_pathdup("objects/info/alternates");
>
>  	if (!access(alternates, F_OK)) {
> -		struct child_process cmd = CHILD_PROCESS_INIT;
> -
> -		cmd.git_cmd = 1;
> -		cmd.no_stdin = 1;
> -		strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL);
> -		if (run_command(&cmd))
> -			die(_("cannot repack to clean up"));
> +		struct child_process cmd = {
> +			CHILD_PROCESS_INIT_LIST,
> +			.git_cmd = 1,
> +			.no_stdin = 1,
> +			.on_error = CHILD_PROCESS_ON_ERROR_DIE,
> +		};
> +
> +		run_commandl(&cmd, "repack", "-a", "-d", NULL);
>  		if (unlink(alternates) && errno != ENOENT)
>  			die_errno(_("cannot unlink temporary alternates file"));
>  	}
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index d7f08c8a7fa..b4165b5a8ae 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -44,11 +44,12 @@ static int difftool_config(const char *var, const char *value, void *cb)
>
>  static int print_tool_help(void)
>  {
> -	struct child_process cmd = CHILD_PROCESS_INIT;
> -
> -	cmd.git_cmd = 1;
> -	strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL);
> -	return run_command(&cmd);
> +	struct child_process cmd = {
> +		CHILD_PROCESS_INIT_LIST,
> +		.git_cmd = 1,
> +	};
> +
> +	return run_commandl(&cmd, "mergetool", "--tool-help=diff", NULL);
>  }
>
>  static int parse_index_info(char *p, int *mode1, int *mode2,
> diff --git a/git.c b/git.c
> index 6662548986f..93179f44f78 100644
> --- a/git.c
> +++ b/git.c
> @@ -724,7 +724,13 @@ static void handle_builtin(int argc, const char **argv)
>
>  static void execv_dashed_external(const char **argv)
>  {
> -	struct child_process cmd = CHILD_PROCESS_INIT;
> +	struct child_process cmd = {
> +		CHILD_PROCESS_INIT_LIST,
> +		.clean_on_exit = 1,
> +		.wait_after_clean = 1,
> +		.silent_exec_failure = 1,
> +		.trace2_child_class = "dashed",
> +	};
>  	int status;
>
>  	if (get_super_prefix())
> @@ -736,10 +742,6 @@ static void execv_dashed_external(const char **argv)
>
>  	strvec_pushf(&cmd.args, "git-%s", argv[0]);
>  	strvec_pushv(&cmd.args, argv + 1);
> -	cmd.clean_on_exit = 1;
> -	cmd.wait_after_clean = 1;
> -	cmd.silent_exec_failure = 1;
> -	cmd.trace2_child_class = "dashed";
>
>  	trace2_cmd_name("_run_dashed_");
>
> diff --git a/run-command.c b/run-command.c
> index 23e100dffc4..4b20aa1b577 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -993,15 +993,45 @@ int finish_command_in_signal(struct child_process *cmd)
>
>  int run_command(struct child_process *cmd)
>  {
> -	int code;
> +	int starting = 1;
> +	int code = 0;
>
>  	if (cmd->out < 0 || cmd->err < 0)
>  		BUG("run_command with a pipe can cause deadlock");
>
>  	code = start_command(cmd);
>  	if (code)
> +		goto error;
> +	starting = 0;
> +	code = finish_command(cmd);
> +	if (!code)
> +		return 0;
> +error:
> +	switch (cmd->on_error) {
> +	case CHILD_PROCESS_ON_ERROR_DIE:
> +		die(starting ?
> +		    _("start_command() for '%s' failed (error code %d)") :
> +		    _("'%s': got non-zero exit code '%d'"),
> +		    cmd->args.v[0], code);
> +		break;
> +	case CHILD_PROCESS_ON_ERROR_RETURN:
>  		return code;
> -	return finish_command(cmd);
> +	default:
> +		BUG("unreachable");
> +	}
> +}
> +
> +int run_commandl(struct child_process *cmd, ...)
> +{
> +	va_list ap;
> +	const char *arg;
> +
> +	va_start(ap, cmd);
> +	while ((arg = va_arg(ap, const char *)))
> +		strvec_push(&cmd->args, arg);
> +	va_end(ap);
> +
> +	return run_command(cmd);
>  }
>
>  #ifndef NO_PTHREADS
> diff --git a/run-command.h b/run-command.h
> index fe2717ad11e..71e390350ed 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -15,7 +15,22 @@
>   * produces in the caller in order to process it.
>   */
>
> +enum child_process_on_error {
> +	/**
> +	 * Return a status code from run_command(). Set to 0 so that
> +	 * it'll be { 0 } init'd. If it's specified in a
> +	 * CHILD_PROCESS_INIT_LIST initialization we don't want a
> +	 * "-Woverride-init" warning.
> +	 */
> +	CHILD_PROCESS_ON_ERROR_RETURN = 0,
>
> +	/**
> +	 * die() with some sensible message if run_command() would
> +	 * have returned a non-zero exit code.
> +	 */
> +	CHILD_PROCESS_ON_ERROR_DIE,
> +};
> +
>  /**
>   * This describes the arguments, redirections, and environment of a
>   * command to run in a sub-process.
> @@ -42,6 +57,10 @@
>   *		redirected.
>   */
>  struct child_process {
> +	/**
> +	 * Error behavior, see "enum child_process_on_error" above.
> +	 */
> +	enum child_process_on_error on_error;
>
>  	/**
>  	 * The .args is a `struct strvec', use that API to manipulate
> @@ -144,10 +163,11 @@ struct child_process {
>  	void (*clean_on_exit_handler)(struct child_process *process);
>  };
>
> -#define CHILD_PROCESS_INIT { \
> +#define CHILD_PROCESS_INIT_LIST \
> +	/* .on_error = CHILD_PROCESS_ON_ERROR_RETURN */ \
>  	.args = STRVEC_INIT, \
> -	.env = STRVEC_INIT, \
> -}
> +	.env = STRVEC_INIT
> +#define CHILD_PROCESS_INIT { CHILD_PROCESS_INIT_LIST }
>
>  /**
>   * The functions: child_process_init, start_command, finish_command,
> @@ -218,6 +238,14 @@ int finish_command_in_signal(struct child_process *);
>   */
>  int run_command(struct child_process *);
>
> +/**
> + * Like run_command() but takes a variable number of arguments, which
> + * will be appended with the equivalent of strvec_pushl(&cmd.args,
> + * ...) before invoking run_command().
> + */
> +LAST_ARG_MUST_BE_NULL
> +int run_commandl(struct child_process *cmd, ...);
> +
>  /*
>   * Trigger an auto-gc
>   */
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 28 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.