[PATCH v2 0/8] hook API: connect hooks to the TTY again, fixes a v2.36.0 regression
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 18, 2022, 20:05 UTC
- Message-ID
- <cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com>
- In-Reply-To
- <cover-0.6-00000000000-20220421T122108Z-avarab@gmail.com>
A re-roll of v1[1]. I believe this addresses all comments on the v1 (but perhaps I missed something). Changes:
* The run_processes_parallel() now takes only one argument, the new "opts" struct which has options, callbacks etc. This will also make the subsequent config-based hooks topic less churny (it needs new callbacks).
As a result the whole internal *_tr2() wrapper/static function are gone.
* Replaced checks of "ungroup" with whether we have a NULL or not (e.g. for pp->pfd), also for free().
* Typo/grammar fixes in commit messages.
* Hopefully the 8/8 is less confusing vis-a-vis https://lore.kernel.org/git/xmqqfslva3mx.fsf@gitster.g/; I.e. now we only add "stdout_to_stderr".
* The 01/08 and 04/08 are new: Splitting those out made subsequent diffs smaller.
* Tweaked 5/8 a bit to make the diff smaller.
* Used "err" and "out", not "actual" and "out" in tests, per Junio's suggestion.
Passing CI for this series at: https://github.com/avar/git/actions/runs/2346571047
1. https://lore.kernel.org/git/cover-0.6-00000000000-20220421T122108Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (8):
run-command tests: change if/if/... to if/else if/else
run-command API: use "opts" struct for run_processes_parallel{,_tr2}()
run-command tests: test stdout of run_command_parallel()
run-command.c: add an initializer for "struct parallel_processes"
run-command: add an "ungroup" option to run_process_parallel()
hook tests: fix redirection logic error in 96e7225b310
hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
hook API: fix v2.36.0 regression: hooks should be connected to a TTYbuiltin/fetch.c | 18 +++-- builtin/submodule--helper.c | 15 ++-- hook.c | 28 +++++--- run-command.c | 132 +++++++++++++++++++++++------------- run-command.h | 66 ++++++++++++++---- submodule.c | 18 +++-- t/helper/test-run-command.c | 65 ++++++++++++------ t/t0061-run-command.sh | 55 ++++++++++++--- t/t1800-hook.sh | 39 ++++++++++- 9 files changed, 316 insertions(+), 120 deletions(-)
Range-diff against v1:
-: ----------- > 1: 26a81eff267 run-command tests: change if/if/... to if/else if/else
1: 8bf71ce63dd ! 2: 5f0a6e9925f run-command API: replace run_processes_parallel_tr2() with opts struct
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- run-command API: replace run_processes_parallel_tr2() with opts struct
+ run-command API: use "opts" struct for run_processes_parallel{,_tr2}()
- 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.
+ Add a new "struct run_process_parallel_opts" to replace the growing
+ run_processes_parallel() and run_processes_parallel_tr2() argument
+ lists. This refactoring makes it easier to add new options and
+ parameters easier.
- 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.
+ The *_tr2() variant of the function was added in ee4512ed481 (trace2:
+ create new combined trace facility, 2019-02-22), and has subsequently
+ been used by every caller except t/helper/test-run-command.c.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
@@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_chi
+ struct run_process_parallel_opts run_opts = {
+ .tr2_category = "fetch",
+ .tr2_label = "parallel/fetch",
++
++ .jobs = max_children,
++
++ .get_next_task = &fetch_next_remote,
++ .start_failure = &fetch_failed_to_start,
++ .task_finished = &fetch_finished,
++ .data = &state,
+ };
strvec_push(&argv, "--end-of-options");
@@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_chi
- &fetch_finished,
- &state,
- "fetch", "parallel/fetch");
-+ result = run_processes_parallel(max_children,
-+ &fetch_next_remote,
-+ &fetch_failed_to_start,
-+ &fetch_finished, &state,
-+ &run_opts);
++ result = run_processes_parallel(&run_opts);
if (!result)
result = state.result;
@@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up
+ struct run_process_parallel_opts run_opts = {
+ .tr2_category = "submodule",
+ .tr2_label = "parallel/update",
++
++ .get_next_task = update_clone_get_next_task,
++ .start_failure = update_clone_start_failure,
++ .task_finished = update_clone_task_finished,
++ .data = &suc,
+ };
suc.update_data = update_data;
@@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up
- 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);
++ run_opts.jobs = suc.update_data->max_jobs;
++ run_processes_parallel(&run_opts);
/*
* We saved the output and put it out all at once now.
## hook.c ##
@@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
+ .options = options,
+ };
const char *const hook_path = find_hook(hook_name);
- int jobs = 1;
+- int jobs = 1;
++ const int jobs = 1;
int ret = 0;
+ struct run_process_parallel_opts run_opts = {
+ .tr2_category = "hook",
+ .tr2_label = hook_name,
++
++ .jobs = jobs,
++
++ .get_next_task = pick_next_hook,
++ .start_failure = notify_start_failure,
++ .task_finished = notify_hook_finished,
++ .data = &cb_data,
+ };
if (!options)
@@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
- &cb_data,
- "hook",
- hook_name);
-+ run_processes_parallel(jobs, pick_next_hook, notify_start_failure,
-+ notify_hook_finished, &cb_data, &run_opts);
++ run_processes_parallel(&run_opts);
ret = cb_data.rc;
cleanup:
strbuf_release(&abs_path);
## run-command.c ##
+@@ run-command.c: static void handle_children_on_signal(int signo)
+ }
+
+ static void pp_init(struct parallel_processes *pp,
+- int n,
+- get_next_task_fn get_next_task,
+- start_failure_fn start_failure,
+- task_finished_fn task_finished,
+- void *data)
++ struct run_process_parallel_opts *opts)
+ {
+ int i;
++ int n = opts->jobs;
++ void *data = opts->data;
++ get_next_task_fn get_next_task = opts->get_next_task;
++ start_failure_fn start_failure = opts->start_failure;
++ task_finished_fn task_finished = opts->task_finished;
+
+ if (n < 1)
+ n = online_cpus();
@@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
return result;
}
@@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
- 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 run_processes_parallel(struct run_process_parallel_opts *opts)
{
int i, code;
int output_timeout = 100;
+ int spawn_cap = 4;
+ struct parallel_processes pp;
++ const char *tr2_category = opts->tr2_category;
++ const char *tr2_label = opts->tr2_label;
++ const int do_trace2 = tr2_category && tr2_label;
++ const int n = opts->jobs;
+
+- pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
++ if (do_trace2)
++ trace2_region_enter_printf(tr2_category, tr2_label, NULL,
++ "max:%d", ((n < 1) ? online_cpus()
++ : n));
++
++ pp_init(&pp, opts);
+ while (1) {
+ for (i = 0;
+ i < spawn_cap && !pp.shutdown &&
@@ run-command.c: int run_processes_parallel(int n,
- return 0;
- }
+ }
+ pp_cleanup(&pp);
+- 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;
+-{
+- int result;
- trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
- ((n < 1) ? online_cpus() : n));
+- trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
+- ((n < 1) ? online_cpus() : n));
++ if (do_trace2)
++ trace2_region_leave(tr2_category, tr2_label, NULL);
- 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;
+-
+- trace2_region_leave(tr2_category, tr2_label, NULL);
+-
+- return result;
++ return 0;
}
-+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);
-+
-+ return run_processes_parallel_1(n, get_next_task, start_failure,
-+ task_finished, pp_cb, opts);
-+}
-+
-+
int run_auto_maintenance(int quiet)
- {
- int enabled;
## run-command.h ##
@@ run-command.h: 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.
++ *
++ * jobs: see 'n' in run_processes_parallel() below.
++ *
++ * *_fn & data: see run_processes_parallel() below.
+ */
+struct run_process_parallel_opts
+{
+ const char *tr2_category;
+ const char *tr2_label;
++
++ int jobs;
++
++ get_next_task_fn get_next_task;
++ start_failure_fn start_failure;
++ task_finished_fn task_finished;
++ void *data;
+};
+
- /**
++/**
++ * Options are passed via the "struct run_process_parallel_opts" above.
++
* 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
+ * required to start another child process.
@@ run-command.h: 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,
@@ run-command.h: typedef int (*task_finished_fn)(int result,
-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);
++int run_processes_parallel(struct run_process_parallel_opts *opts);
/**
* Convenience function which prepares env_array for a command to be run in a
@@ submodule.c: int fetch_submodules(struct repository *r,
+ struct run_process_parallel_opts run_opts = {
+ .tr2_category = "submodule",
+ .tr2_label = "parallel/fetch",
++
++ .jobs = max_parallel_jobs,
++
++ .get_next_task = get_next_submodule,
++ .start_failure = fetch_start_failure,
++ .task_finished = fetch_finish,
++ .data = &spf,
+ };
spf.r = r;
@@ submodule.c: int fetch_submodules(struct repository *r,
- fetch_finish,
- &spf,
- "submodule", "parallel/fetch");
-+ run_processes_parallel(max_parallel_jobs, get_next_submodule,
-+ fetch_start_failure, fetch_finish, &spf,
-+ &run_opts);
++ run_processes_parallel(&run_opts);
if (spf.submodules_with_errors.len > 0)
fprintf(stderr, _("Errors during submodule fetch:\n%s"),
## t/helper/test-run-command.c ##
@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
+ "write JUnit-style XML files"),
+ OPT_END()
+ };
++ struct run_process_parallel_opts run_opts = {
++ .get_next_task = next_test,
++ .start_failure = test_failed,
++ .task_finished = test_finished,
++ .data = &suite,
++ };
+
+ argc = parse_options(argc, argv, NULL, options,
+ testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
+
+ fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n",
(uintmax_t)suite.tests.nr, max_jobs);
++ run_opts.jobs = max_jobs;
- ret = run_processes_parallel(max_jobs, next_test, test_failed,
+- ret = run_processes_parallel(max_jobs, next_test, test_failed,
- test_finished, &suite);
-+ test_finished, &suite, NULL);
++ ret = run_processes_parallel(&run_opts);
if (suite.failed.nr > 0) {
ret = 1;
@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
- {
- struct child_process proc = CHILD_PROCESS_INIT;
int jobs;
+ get_next_task_fn next_fn = NULL;
+ task_finished_fn finished_fn = NULL;
+ struct run_process_parallel_opts opts = { 0 };
if (argc > 1 && !strcmp(argv[1], "testsuite"))
exit(testsuite(argc - 1, argv + 1));
@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
+ jobs = atoi(argv[2]);
+ strvec_clear(&proc.args);
+ strvec_pushv(&proc.args, (const char **)argv + 3);
++ opts.jobs = jobs;
++ opts.data = &proc;
- 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));
+ if (!strcmp(argv[1], "run-command-parallel")) {
+ next_fn = parallel_next;
+@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
+ return 1;
+ }
- fprintf(stderr, "check usage\n");
- return 1;
+- exit(run_processes_parallel(jobs, next_fn, NULL, finished_fn, &proc));
++ opts.get_next_task = next_fn;
++ opts.task_finished = finished_fn;
++ exit(run_processes_parallel(&opts));
+ }
2: d9c9b158130 ! 3: a8e1fc07b65 run-command tests: test stdout of run_command_parallel()
@@ t/t0061-run-command.sh: World
test_expect_success 'run_command runs in parallel with more jobs available than tasks' '
- test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
-+ test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+- test_cmp expect actual
++ test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+ test_must_be_empty out &&
- test_cmp expect actual
++ test_cmp expect err
'
test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
- test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
-+ test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+- test_cmp expect actual
++ test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+ test_must_be_empty out &&
- test_cmp expect actual
++ test_cmp expect err
'
test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
- test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
-+ test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+- test_cmp expect actual
++ test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+ test_must_be_empty out &&
- test_cmp expect actual
++ test_cmp expect err
'
+ cat >expect <<-EOF
@@ t/t0061-run-command.sh: asking for a quick stop
EOF
test_expect_success 'run_command is asked to abort gracefully' '
- test-tool run-command run-command-abort 3 false 2>actual &&
-+ test-tool run-command run-command-abort 3 false >out 2>actual &&
+- test_cmp expect actual
++ test-tool run-command run-command-abort 3 false >out 2>err &&
+ test_must_be_empty out &&
- test_cmp expect actual
++ test_cmp expect err
'
+ cat >expect <<-EOF
@@ t/t0061-run-command.sh: no further jobs available
EOF
test_expect_success 'run_command outputs ' '
- test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
-+ test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+- test_cmp expect actual
++ test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+ test_must_be_empty out &&
- test_cmp expect actual
++ test_cmp expect err
'
+ test_trace () {
-: ----------- > 4: 663936fb4ad run-command.c: add an initializer for "struct parallel_processes"
3: d76f63c2948 ! 5: c2e015ed840 run-command: add an "ungroup" option to run_process_parallel()
@@ run-command.c: struct parallel_processes {
struct pollfd *pfd;
unsigned shutdown : 1;
-+ unsigned ungroup:1;
++ unsigned ungroup : 1;
int output_owner;
struct strbuf buffered_output; /* of finished children */
@@ run-command.c: static void pp_init(struct parallel_processes *pp,
- get_next_task_fn get_next_task,
- start_failure_fn start_failure,
- task_finished_fn task_finished,
-- void *data)
-+ void *data, struct run_process_parallel_opts *opts)
- {
-+ const int ungroup = opts->ungroup;
- int i;
-
- if (n < 1)
-@@ run-command.c: static void pp_init(struct parallel_processes *pp,
- pp->start_failure = start_failure ? start_failure : default_start_failure;
- pp->task_finished = task_finished ? task_finished : default_task_finished;
-
-+ pp->ungroup = ungroup;
-+
pp->nr_processes = 0;
pp->output_owner = 0;
pp->shutdown = 0;
++ pp->ungroup = opts->ungroup;
CALLOC_ARRAY(pp->children, n);
- CALLOC_ARRAY(pp->pfd, n);
-+ if (!ungroup)
++ if (!pp->ungroup)
+ CALLOC_ARRAY(pp->pfd, n);
-+
- strbuf_init(&pp->buffered_output, 0);
for (i = 0; i < n; i++) {
strbuf_init(&pp->children[i].err, 0);
child_process_init(&pp->children[i].process);
-+ if (ungroup)
++ if (!pp->pfd)
+ continue;
pp->pfd[i].events = POLLIN | POLLHUP;
pp->pfd[i].fd = -1;
}
-@@ run-command.c: static void pp_init(struct parallel_processes *pp,
-
- static void pp_cleanup(struct parallel_processes *pp)
- {
-+ const int ungroup = pp->ungroup;
- int i;
-
- trace_printf("run_processes_parallel: done");
@@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
- }
-
- free(pp->children);
-- free(pp->pfd);
-+ if (!ungroup)
-+ free(pp->pfd);
-
- /*
* When get_next_task added messages to the buffer in its last
* iteration, the buffered output is non empty.
*/
- strbuf_write(&pp->buffered_output, stderr);
-- strbuf_release(&pp->buffered_output);
-+ if (!ungroup) {
++ if (!pp->ungroup)
+ strbuf_write(&pp->buffered_output, stderr);
-+ strbuf_release(&pp->buffered_output);
-+ }
+ strbuf_release(&pp->buffered_output);
sigchain_pop_common();
- }
@@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
*/
static int pp_start_one(struct parallel_processes *pp)
@@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
}
- pp->children[i].process.err = -1;
- pp->children[i].process.stdout_to_stderr = 1;
-- pp->children[i].process.no_stdin = 1;
-+
+ if (!ungroup) {
+ pp->children[i].process.err = -1;
+ pp->children[i].process.stdout_to_stderr = 1;
-+ pp->children[i].process.no_stdin = 1;
+ }
+ pp->children[i].process.no_stdin = 1;
if (start_command(&pp->children[i].process)) {
- code = pp->start_failure(&pp->children[i].err,
@@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
pp->nr_processes++;
pp->children[i].state = GIT_CP_WORKING;
- pp->pfd[i].fd = pp->children[i].process.err;
-+ if (!ungroup)
++ if (pp->pfd)
+ pp->pfd[i].fd = pp->children[i].process.err;
return 0;
}
@@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
pp->nr_processes--;
pp->children[i].state = GIT_CP_FREE;
- pp->pfd[i].fd = -1;
-+ if (!ungroup)
++ if (pp->pfd)
+ pp->pfd[i].fd = -1;
child_process_init(&pp->children[i].process);
- if (i != pp->output_owner) {
+ if (ungroup) {
-+ /* no strbuf_*() work to do here */
++ ; /* no strbuf_*() work to do here */
+ } else if (i != pp->output_owner) {
strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
strbuf_reset(&pp->children[i].err);
} else {
-@@ run-command.c: static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
- void *pp_cb,
- struct run_process_parallel_opts *opts)
- {
-+ const int ungroup = opts->ungroup;
- int i, code;
- int output_timeout = 100;
- int spawn_cap = 4;
- struct parallel_processes pp;
-
-- pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
-+ pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,
-+ opts);
- while (1) {
- for (i = 0;
- i < spawn_cap && !pp.shutdown &&
-@@ run-command.c: static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
+@@ run-command.c: int run_processes_parallel(struct run_process_parallel_opts *opts)
}
if (!pp.nr_processes)
break;
- pp_buffer_stderr(&pp, output_timeout);
- pp_output(&pp);
-+ if (ungroup) {
++ if (opts->ungroup) {
+ pp_mark_working_for_cleanup(&pp);
+ } else {
+ pp_buffer_stderr(&pp, output_timeout);
@@ run-command.h: typedef int (*start_failure_fn)(struct strbuf *out,
* pp_task_cb is the callback cookie as passed into get_next_task_fn.
@@ run-command.h: typedef int (*task_finished_fn)(int result,
*
- * tr2_category & tr2_label: sets the trace2 category and label for
- * logging. These must either be unset, or both of them must be set.
-+ *
+ * jobs: see 'n' in run_processes_parallel() below.
+ *
+ * ungroup: Ungroup output. Output is printed as soon as possible and
+ * bypasses run-command's internal processing. This may cause output
+ * from different commands to be mixed.
++ *
+ * *_fn & data: see run_processes_parallel() below.
*/
struct run_process_parallel_opts
- {
- const char *tr2_category;
+@@ run-command.h: struct run_process_parallel_opts
const char *tr2_label;
+
+ int jobs;
+ unsigned int ungroup:1;
- };
- /**
+ get_next_task_fn get_next_task;
+ start_failure_fn start_failure;
@@ run-command.h: struct run_process_parallel_opts
*
* The children started via this function run in parallel. Their output
@@ run-command.h: struct run_process_parallel_opts
*
* 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".
-+ * Options are passed via a "struct run_process_parallel_opts". If the
-+ * "ungroup" option isn't specified the callbacks will get a pointer
-+ * to a "struct strbuf *out", and must not write to stdout or stderr
-+ * as such output will mess up the output of the other parallel
++ *
++ * If the "ungroup" option isn't specified the callbacks will get a
++ * pointer to a "struct strbuf *out", and must not write to stdout or
++ * stderr as such output will mess up the output of the other parallel
+ * processes. If "ungroup" option is specified callbacks will get a
+ * NULL "struct strbuf *out" parameter, and are responsible for
+ * emitting their own output, including dealing with any race
+ * conditions due to writing in parallel to stdout and stderr.
*/
- int run_processes_parallel(int n, get_next_task_fn, start_failure_fn,
- task_finished_fn, void *pp_cb,
+ int run_processes_parallel(struct run_process_parallel_opts *opts);
+
## t/helper/test-run-command.c ##
@@ t/helper/test-run-command.c: static int parallel_next(struct child_process *cp,
@@ t/helper/test-run-command.c: static int task_finished(int result,
}
@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
- strvec_clear(&proc.args);
- strvec_pushv(&proc.args, (const char **)argv + 3);
+ opts.jobs = jobs;
+ opts.data = &proc;
-- if (!strcmp(argv[1], "run-command-parallel"))
+- if (!strcmp(argv[1], "run-command-parallel")) {
+ if (!strcmp(argv[1], "run-command-parallel") ||
+ !strcmp(argv[1], "run-command-parallel-ungroup")) {
-+ opts.ungroup = !strcmp(argv[1], "run-command-parallel-ungroup");
- exit(run_processes_parallel(jobs, parallel_next,
- NULL, NULL, &proc, &opts));
-+ }
-
-- if (!strcmp(argv[1], "run-command-abort"))
-+ if (!strcmp(argv[1], "run-command-abort") ||
-+ !strcmp(argv[1], "run-command-abort-ungroup")) {
-+ opts.ungroup = !strcmp(argv[1], "run-command-abort-ungroup");
- exit(run_processes_parallel(jobs, parallel_next, NULL,
- task_finished, &proc, &opts));
-+ }
-
-- if (!strcmp(argv[1], "run-command-no-jobs"))
-+ if (!strcmp(argv[1], "run-command-no-jobs") ||
-+ !strcmp(argv[1], "run-command-no-jobs-ungroup")) {
-+ opts.ungroup = !strcmp(argv[1], "run-command-no-jobs-ungroup");
- exit(run_processes_parallel(jobs, no_job, NULL, task_finished,
- &proc, &opts));
-+ }
+ next_fn = parallel_next;
+- } else if (!strcmp(argv[1], "run-command-abort")) {
++ } else if (!strcmp(argv[1], "run-command-abort") ||
++ !strcmp(argv[1], "run-command-abort-ungroup")) {
+ next_fn = parallel_next;
+ finished_fn = task_finished;
+- } else if (!strcmp(argv[1], "run-command-no-jobs")) {
++ } else if (!strcmp(argv[1], "run-command-no-jobs") ||
++ !strcmp(argv[1], "run-command-no-jobs-ungroup")) {
+ next_fn = no_job;
+ finished_fn = task_finished;
+ } else {
+@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
+ return 1;
+ }
- fprintf(stderr, "check usage\n");
- return 1;
++ opts.ungroup = ends_with(argv[1], "-ungroup");
+ opts.get_next_task = next_fn;
+ opts.task_finished = finished_fn;
+ exit(run_processes_parallel(&opts));
## t/t0061-run-command.sh ##
@@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with more jobs available than
- test_cmp expect actual
+ test_cmp expect err
'
+test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
@@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
+'
+
test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
- test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+ test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
test_must_be_empty out &&
- test_cmp expect actual
+ test_cmp expect err
'
+test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
@@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
+'
+
test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
- test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+ test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
test_must_be_empty out &&
- test_cmp expect actual
+ test_cmp expect err
'
+test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
@@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
preloaded output of a child
asking for a quick stop
@@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort gracefully' '
- test_cmp expect actual
+ test_cmp expect err
'
+test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
@@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort grace
no further jobs available
EOF
@@ t/t0061-run-command.sh: test_expect_success 'run_command outputs ' '
- test_cmp expect actual
+ test_cmp expect err
'
+test_expect_success 'run_command outputs (ungroup) ' '
-+ test-tool run-command run-command-no-jobs-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
++ test-tool run-command run-command-no-jobs-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+ test_must_be_empty out &&
-+ test_cmp expect actual
++ test_cmp expect err
+'
+
test_trace () {
4: cf62569b2e0 = 6: 84e92c6f7c7 hook tests: fix redirection logic error in 96e7225b310
5: 98c26c9917b ! 7: bf7d871565f hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
@@ Commit message
hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
Amend code added in 96e7225b310 (hook: add 'run' subcommand,
- 2021-12-22) top stop setting these two flags. We use the
+ 2021-12-22) to stop setting these two flags. We use the
run_process_parallel() API added in c553c72eed6 (run-command: add an
asynchronous parallel child processor, 2015-12-15), which always sets
these in pp_start_one() (in addition to setting .err = -1).
6: de3664f6d2b ! 8: 238155fcb9d hook API: fix v2.36.0 regression: hooks should be connected to a TTY
@@ Commit message
'./git hook run seq-hook' in 'HEAD~0' ran
1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master'
- In the preceding commit we removed the "no_stdin=1" and
- "stdout_to_stderr=1" assignments. This change brings them back as with
- ".ungroup=1" the run_process_parallel() function doesn't provide them
- for us implicitly.
+ In the preceding commit we removed the "stdout_to_stderr=1" assignment
+ as being redundant. This change brings it back as with ".ungroup=1"
+ the run_process_parallel() function doesn't provide them for us
+ implicitly.
As an aside omitting the stdout_to_stderr=1 here would have all tests
pass, except those that test "git hook run" itself in
@@ Commit message
## hook.c ##
@@ hook.c: static int pick_next_hook(struct child_process *cp,
- if (!hook_path)
return 0;
-+ cp->no_stdin = 1;
strvec_pushv(&cp->env_array, hook_cb->options->env.v);
-+ cp->stdout_to_stderr = 1;
++ cp->stdout_to_stderr = 1; /* because of .ungroup = 1 */
cp->trace2_hook_name = hook_cb->hook_name;
cp->dir = hook_cb->options->dir;
@@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
- .options = options,
- };
- const char *const hook_path = find_hook(hook_name);
-- int jobs = 1;
-+ const int jobs = 1;
- int ret = 0;
- struct run_process_parallel_opts run_opts = {
- .tr2_category = "hook",
.tr2_label = hook_name,
+
+ .jobs = jobs,
+ .ungroup = jobs == 1,
- };
+ .get_next_task = pick_next_hook,
+ .start_failure = notify_start_failure,
+@@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
if (!options)
BUG("a struct run_hooks_opt must be provided to run_hooks");
-- 2.36.1.952.g0ae626f6cd7