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

Re: [PATCH 1/6] run-command API: replace run_processes_parallel_tr2() with opts struct

From
Emily Shaffer <emilyshaffer@google.com>
Date
Apr 28, 2022, 23:16 UTC
Message-ID
<YmsgWj5vPEWNyGFA@google.com>
In-Reply-To
<patch-1.6-8bf71ce63dd-20220421T122108Z-avarab@gmail.com>
On Thu, Apr 21, 2022 at 02:25:26PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 65 quoted lines
> 
> Add a new "struct run_process_parallel_opts" to cover the trace2
> use-case added in ee4512ed481 (trace2: create new combined trace
> facility, 2019-02-22). A subsequent commit will add more options, and
> having a proliferation of new functions or extra parameters would
> result in needless churn.
> 
> It makes for a smaller change to make run_processes_parallel() and
> run_processes_parallel_tr2() wrapper functions for the new "static"
> run_processes_parallel_1(), which contains the main logic. We pass
> down "opts" to the *_1() function even though it isn't used there
> yet (only in the *_tr2() function), a subsequent commit will make more
> use of it.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/fetch.c             | 15 ++++++++------
>  builtin/submodule--helper.c | 12 +++++++----
>  hook.c                      | 13 ++++++------
>  run-command.c               | 40 +++++++++++++++++++++++++++----------
>  run-command.h               | 26 ++++++++++++++++--------
>  submodule.c                 | 13 ++++++------
>  t/helper/test-run-command.c | 13 ++++++------
>  7 files changed, 84 insertions(+), 48 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index e3791f09ed5..9bc99183191 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1948,14 +1948,17 @@ static int fetch_multiple(struct string_list *list, int max_children)
>  
>  	if (max_children != 1 && list->nr != 1) {
>  		struct parallel_fetch_state state = { argv.v, list, 0, 0 };
> +		struct run_process_parallel_opts run_opts = {
> +			.tr2_category = "fetch",
> +			.tr2_label = "parallel/fetch",
> +		};
>  
>  		strvec_push(&argv, "--end-of-options");
> -		result = run_processes_parallel_tr2(max_children,
> -						    &fetch_next_remote,
> -						    &fetch_failed_to_start,
> -						    &fetch_finished,
> -						    &state,
> -						    "fetch", "parallel/fetch");
> +		result = run_processes_parallel(max_children,
> +						&fetch_next_remote,
> +						&fetch_failed_to_start,
> +						&fetch_finished, &state,
> +						&run_opts);
>  
>  		if (!result)
>  			result = state.result;
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2c87ef9364f..c3d1aace546 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2652,12 +2652,16 @@ static int update_submodules(struct update_data *update_data)
>  {
>  	int i, res = 0;
>  	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
> +	struct run_process_parallel_opts run_opts = {
> +		.tr2_category = "submodule",
> +		.tr2_label = "parallel/update",
> +	};

Hm, so now it is possible for a callsite to forget to set these, rather than to grep for "run_processes_parallel" and notice that everybody else is already calling "run_processes_parallel_tr2". There are not any callers of 'run_processes_parallel()' except for a test helper today, so why do we make this seem optional?

If I'm being honest, I'd rather see everything _but_ the trace2 stuff go into an opts struct, and then see the same entry points we have today (run_processes_parallel that takes a struct, run_processes_parallel_tr2 that takes a struct and two tr2 string args). Or, I guess, a single run_processes_parallel() that only takes a struct, does the right thing with the trace args, and entirely removes the run_processes_parallel_tr2 call.

Show 105 quoted lines
>  
>  	suc.update_data = update_data;
> -	run_processes_parallel_tr2(suc.update_data->max_jobs, update_clone_get_next_task,
> -				   update_clone_start_failure,
> -				   update_clone_task_finished, &suc, "submodule",
> -				   "parallel/update");
> +	run_processes_parallel(suc.update_data->max_jobs,
> +			       update_clone_get_next_task,
> +			       update_clone_start_failure,
> +			       update_clone_task_finished, &suc, &run_opts);
>  
>  	/*
>  	 * We saved the output and put it out all at once now.
> diff --git a/hook.c b/hook.c
> index 1d51be3b77a..eadb2d58a7b 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -123,6 +123,10 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
>  	const char *const hook_path = find_hook(hook_name);
>  	int jobs = 1;
>  	int ret = 0;
> +	struct run_process_parallel_opts run_opts = {
> +		.tr2_category = "hook",
> +		.tr2_label = hook_name,
> +	};
>  
>  	if (!options)
>  		BUG("a struct run_hooks_opt must be provided to run_hooks");
> @@ -144,13 +148,8 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
>  		cb_data.hook_path = abs_path.buf;
>  	}
>  
> -	run_processes_parallel_tr2(jobs,
> -				   pick_next_hook,
> -				   notify_start_failure,
> -				   notify_hook_finished,
> -				   &cb_data,
> -				   "hook",
> -				   hook_name);
> +	run_processes_parallel(jobs, pick_next_hook, notify_start_failure,
> +			       notify_hook_finished, &cb_data, &run_opts);
>  	ret = cb_data.rc;
>  cleanup:
>  	strbuf_release(&abs_path);
> diff --git a/run-command.c b/run-command.c
> index a8501e38ceb..7b8159aa235 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1738,11 +1738,11 @@ static int pp_collect_finished(struct parallel_processes *pp)
>  	return result;
>  }
>  
> -int run_processes_parallel(int n,
> -			   get_next_task_fn get_next_task,
> -			   start_failure_fn start_failure,
> -			   task_finished_fn task_finished,
> -			   void *pp_cb)
> +static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
> +				    start_failure_fn start_failure,
> +				    task_finished_fn task_finished,
> +				    void *pp_cb,
> +				    struct run_process_parallel_opts *opts)
>  {
>  	int i, code;
>  	int output_timeout = 100;
> @@ -1780,24 +1780,42 @@ int run_processes_parallel(int n,
>  	return 0;
>  }
>  
> -int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
> -			       start_failure_fn start_failure,
> -			       task_finished_fn task_finished, void *pp_cb,
> -			       const char *tr2_category, const char *tr2_label)
> +static int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
> +				      start_failure_fn start_failure,
> +				      task_finished_fn task_finished,
> +				      void *pp_cb,
> +				      struct run_process_parallel_opts *opts)
>  {
> +	const char *tr2_category = opts->tr2_category;
> +	const char *tr2_label = opts->tr2_label;
>  	int result;
>  
>  	trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
>  				   ((n < 1) ? online_cpus() : n));
>  
> -	result = run_processes_parallel(n, get_next_task, start_failure,
> -					task_finished, pp_cb);
> +	result = run_processes_parallel_1(n, get_next_task, start_failure,
> +					  task_finished, pp_cb, opts);
>  
>  	trace2_region_leave(tr2_category, tr2_label, NULL);
>  
>  	return result;
>  }
>  
> +int run_processes_parallel(int n, get_next_task_fn get_next_task,
> +			   start_failure_fn start_failure,
> +			   task_finished_fn task_finished, void *pp_cb,
> +			   struct run_process_parallel_opts *opts)
> +{
> +	if (opts->tr2_category && opts->tr2_label)
> +		return run_processes_parallel_tr2(n, get_next_task,
> +						  start_failure, task_finished,
> +						  pp_cb, opts);

What is the point for this extra layer of indirection? I am confused why we might not just change the arg list for run_processes_parallel_tr2 and call it good.

If the final result was to reduce the number of run_processes_parallel.* functions available I'd be happy to see this change, but as it introduces even more various entry points into run_processes_parallel.* I'm not so sure.

Show 130 quoted lines
> +
> +	return run_processes_parallel_1(n, get_next_task, start_failure,
> +					task_finished, pp_cb, opts);
> +}
> +
> +
>  int run_auto_maintenance(int quiet)
>  {
>  	int enabled;
> diff --git a/run-command.h b/run-command.h
> index 07bed6c31b4..66e7bebd88a 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -458,6 +458,19 @@ typedef int (*task_finished_fn)(int result,
>  				void *pp_cb,
>  				void *pp_task_cb);
>  
> +/**
> + * Options to pass to run_processes_parallel(), { 0 }-initialized
> + * means no options. Fields:
> + *
> + * tr2_category & tr2_label: sets the trace2 category and label for
> + * logging. These must either be unset, or both of them must be set.
> + */
> +struct run_process_parallel_opts
> +{
> +	const char *tr2_category;
> +	const char *tr2_label;
> +};
> +
>  /**
>   * Runs up to n processes at the same time. Whenever a process can be
>   * started, the callback get_next_task_fn is called to obtain the data
> @@ -469,15 +482,12 @@ typedef int (*task_finished_fn)(int result,
>   *
>   * start_failure_fn and task_finished_fn can be NULL to omit any
>   * special handling.
> + *
> + * Options are passed via a "struct run_process_parallel_opts".
>   */
> -int run_processes_parallel(int n,
> -			   get_next_task_fn,
> -			   start_failure_fn,
> -			   task_finished_fn,
> -			   void *pp_cb);
> -int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,
> -			       task_finished_fn, void *pp_cb,
> -			       const char *tr2_category, const char *tr2_label);
> +int run_processes_parallel(int n, get_next_task_fn, start_failure_fn,
> +			   task_finished_fn, void *pp_cb,
> +			   struct run_process_parallel_opts *opts);
>  
>  /**
>   * Convenience function which prepares env_array for a command to be run in a
> diff --git a/submodule.c b/submodule.c
> index 86c8f0f89db..256c6bb4b8f 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1817,6 +1817,10 @@ int fetch_submodules(struct repository *r,
>  {
>  	int i;
>  	struct submodule_parallel_fetch spf = SPF_INIT;
> +	struct run_process_parallel_opts run_opts = {
> +		.tr2_category = "submodule",
> +		.tr2_label = "parallel/fetch",
> +	};
>  
>  	spf.r = r;
>  	spf.command_line_option = command_line_option;
> @@ -1838,12 +1842,9 @@ int fetch_submodules(struct repository *r,
>  
>  	calculate_changed_submodule_paths(r, &spf.changed_submodule_names);
>  	string_list_sort(&spf.changed_submodule_names);
> -	run_processes_parallel_tr2(max_parallel_jobs,
> -				   get_next_submodule,
> -				   fetch_start_failure,
> -				   fetch_finish,
> -				   &spf,
> -				   "submodule", "parallel/fetch");
> +	run_processes_parallel(max_parallel_jobs, get_next_submodule,
> +			       fetch_start_failure, fetch_finish, &spf,
> +			       &run_opts);
>  
>  	if (spf.submodules_with_errors.len > 0)
>  		fprintf(stderr, _("Errors during submodule fetch:\n%s"),
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index f3b90aa834a..9b21f2f9f83 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -183,7 +183,7 @@ static int testsuite(int argc, const char **argv)
>  		(uintmax_t)suite.tests.nr, max_jobs);
>  
>  	ret = run_processes_parallel(max_jobs, next_test, test_failed,
> -				     test_finished, &suite);
> +				     test_finished, &suite, NULL);
>  
>  	if (suite.failed.nr > 0) {
>  		ret = 1;
> @@ -371,6 +371,7 @@ int cmd__run_command(int argc, const char **argv)
>  {
>  	struct child_process proc = CHILD_PROCESS_INIT;
>  	int jobs;
> +	struct run_process_parallel_opts opts = { 0 };
>  
>  	if (argc > 1 && !strcmp(argv[1], "testsuite"))
>  		exit(testsuite(argc - 1, argv + 1));
> @@ -413,15 +414,15 @@ int cmd__run_command(int argc, const char **argv)
>  
>  	if (!strcmp(argv[1], "run-command-parallel"))
>  		exit(run_processes_parallel(jobs, parallel_next,
> -					    NULL, NULL, &proc));
> +					    NULL, NULL, &proc, &opts));
>  
>  	if (!strcmp(argv[1], "run-command-abort"))
> -		exit(run_processes_parallel(jobs, parallel_next,
> -					    NULL, task_finished, &proc));
> +		exit(run_processes_parallel(jobs, parallel_next, NULL,
> +					    task_finished, &proc, &opts));
>  
>  	if (!strcmp(argv[1], "run-command-no-jobs"))
> -		exit(run_processes_parallel(jobs, no_job,
> -					    NULL, task_finished, &proc));
> +		exit(run_processes_parallel(jobs, no_job, NULL, task_finished,
> +					    &proc, &opts));
>  
>  	fprintf(stderr, "check usage\n");
>  	return 1;
> -- 
> 2.36.0.893.g80a51c675f6
>
Previous: Junio C HamanoNext: Junio C Hamano
Message 18 of 85 in “git 2.36.0 regression: pre-commit hooks no longer have stdout/stderr as tty”
  1. Anthony SottileApr 19, 2022
  2. Emily ShafferApr 19, 2022
  3. Anthony SottileApr 19, 2022
  4. Phillip WoodApr 20, 2022
  5. Ævar Arnfjörð BjarmasonApr 20, 2022
  6. Emily ShafferApr 20, 2022
  7. Junio C HamanoApr 20, 2022
  8. Emily ShafferApr 20, 2022
  9. Junio C HamanoApr 20, 2022
  10. Emily ShafferApr 20, 2022
  11. Ævar Arnfjörð BjarmasonApr 21, 2022
  12. Junio C HamanoApr 21, 2022
  13. Junio C HamanoApr 21, 2022
  14. Junio C HamanoApr 20, 2022
  15. 0/6 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, Apr 21, 2022
  16. 1/6 run-command API: replace run_processes_parallel_tr2() with opts structÆvar Arnfjörð Bjarmason, Apr 21, 2022
  17. Junio C HamanoApr 23, 2022
  18. Emily ShafferApr 28, 2022
  19. Junio C HamanoApr 29, 2022
  20. 2/6 run-command tests: test stdout of run_command_parallel()Ævar Arnfjörð Bjarmason, Apr 21, 2022
  21. Junio C HamanoApr 23, 2022
  22. 6/6 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, Apr 21, 2022
  23. Emily ShafferApr 28, 2022
  24. Junio C HamanoApr 29, 2022
  25. 3/6 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, Apr 21, 2022
  26. Junio C HamanoApr 23, 2022
  27. Emily ShafferApr 28, 2022
  28. 4/6 hook tests: fix redirection logic error in 96e7225b310Ævar Arnfjörð Bjarmason, Apr 21, 2022
  29. Junio C HamanoApr 23, 2022
  30. 5/6 hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"Ævar Arnfjörð Bjarmason, Apr 21, 2022
  31. Junio C HamanoApr 29, 2022
  32. Junio C HamanoApr 21, 2022
  33. Ævar Arnfjörð BjarmasonApr 21, 2022
  34. 0/8 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, May 18, 2022
  35. 1/8 run-command tests: change if/if/... to if/else if/elseÆvar Arnfjörð Bjarmason, May 18, 2022
  36. 2/8 run-command API: use "opts" struct for run_processes_parallel{,_tr2}()Ævar Arnfjörð Bjarmason, May 18, 2022
  37. Junio C HamanoMay 18, 2022
  38. Emily ShafferMay 25, 2022
  39. 3/8 run-command tests: test stdout of run_command_parallel()Ævar Arnfjörð Bjarmason, May 18, 2022
  40. 4/8 run-command.c: add an initializer for "struct parallel_processes"Ævar Arnfjörð Bjarmason, May 18, 2022
  41. 7/8 hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"Ævar Arnfjörð Bjarmason, May 18, 2022
  42. 6/8 hook tests: fix redirection logic error in 96e7225b310Ævar Arnfjörð Bjarmason, May 18, 2022
  43. 8/8 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, May 18, 2022
  44. Junio C HamanoMay 18, 2022
  45. Emily ShafferMay 26, 2022
  46. Ævar Arnfjörð BjarmasonMay 26, 2022
  47. Emily ShafferMay 26, 2022
  48. 5/8 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, May 18, 2022
  49. Junio C HamanoMay 18, 2022
  50. Emily ShafferMay 26, 2022
  51. Junio C HamanoMay 27, 2022
  52. Johannes SchindelinMay 25, 2022
  53. Ævar Arnfjörð BjarmasonMay 25, 2022
  54. Junio C HamanoMay 25, 2022
  55. Junio C HamanoMay 26, 2022
  56. Ævar Arnfjörð BjarmasonMay 26, 2022
  57. Junio C HamanoMay 26, 2022
  58. Ævar Arnfjörð BjarmasonMay 26, 2022
  59. Junio C HamanoMay 26, 2022
  60. 0/2 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, May 27, 2022
  61. 1/2 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, May 27, 2022
  62. Junio C HamanoMay 27, 2022
  63. 2/2 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, May 27, 2022
  64. Junio C HamanoMay 27, 2022
  65. 0/2 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, May 31, 2022
  66. 1/2 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, May 31, 2022
  67. Johannes SchindelinJun 1, 2022
  68. Junio C HamanoJun 1, 2022
  69. 2/2 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, May 31, 2022
  70. Johannes SchindelinJun 1, 2022
  71. Johannes SchindelinJun 1, 2022
  72. 0/2 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, Jun 2, 2022
  73. 1/2 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, Jun 2, 2022
  74. 2/2 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, Jun 2, 2022
  75. Junio C HamanoJun 2, 2022
  76. Phillip WoodJun 3, 2022
  77. Ævar Arnfjörð BjarmasonJun 3, 2022
  78. Phillip WoodJun 3, 2022
  79. Ævar Arnfjörð BjarmasonJun 7, 2022
  80. 0/2 hook API: connect hooks to the TTY again, fixes a v2.36.0 regressionÆvar Arnfjörð Bjarmason, Jun 7, 2022
  81. 2/2 hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason, Jun 7, 2022
  82. Junio C HamanoJun 7, 2022
  83. 1/2 run-command: add an "ungroup" option to run_process_parallel()Ævar Arnfjörð Bjarmason, Jun 7, 2022
  84. Emily ShafferJun 17, 2022
  85. Junio C HamanoJun 7, 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.