undef/git-commit-vandalism
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'ab/hooks-regression-fix'

Browse Source 
In Git 2.36 we revamped the way how hooks are invoked.  One change
that is end-user visible is that the output of a hook is no longer
directly connected to the standard output of "git" that spawns the
hook, which was noticed post release.  This is getting corrected.

* ab/hooks-regression-fix:
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY
  run-command: add an "ungroup" option to run_process_parallel()
This commit is contained in:
Junio C Hamano 2022-06-13 15:53:41 -07:00
commit 1a7f6be5b1
6 changed files with 155 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
hook.c
View File

@ -144,6 +144,7 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
cb_data.hook_path = abs_path.buf;
}
run_processes_parallel_ungroup = 1;
run_processes_parallel_tr2(jobs,
pick_next_hook,
notify_start_failure,

70
run-command.c
View File

@ -1472,6 +1472,7 @@ enum child_state {
GIT_CP_WAIT_CLEANUP,
};
int run_processes_parallel_ungroup;
struct parallel_processes {
void *data;
@ -1495,6 +1496,7 @@ struct parallel_processes {
struct pollfd *pfd;
unsigned shutdown : 1;
unsigned ungroup : 1;
int output_owner;
struct strbuf buffered_output; /* of finished children */
@ -1538,7 +1540,7 @@ 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, int ungroup)
{
int i;
@ -1560,15 +1562,21 @@ static void pp_init(struct parallel_processes *pp,
pp->nr_processes = 0;
pp->output_owner = 0;
pp->shutdown = 0;
pp->ungroup = ungroup;
CALLOC_ARRAY(pp->children, n);
CALLOC_ARRAY(pp->pfd, n);
if (pp->ungroup)
pp->pfd = NULL;
else
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);
pp->pfd[i].events = POLLIN | POLLHUP;
pp->pfd[i].fd = -1;
if (pp->pfd) {
pp->pfd[i].events = POLLIN | POLLHUP;
pp->pfd[i].fd = -1;
}
}
pp_for_signal = pp;
@ -1616,24 +1624,31 @@ static int pp_start_one(struct parallel_processes *pp)
BUG("bookkeeping is hard");
code = pp->get_next_task(&pp->children[i].process,
&pp->children[i].err,
pp->ungroup ? NULL : &pp->children[i].err,
pp->data,
&pp->children[i].data);
if (!code) {
strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
strbuf_reset(&pp->children[i].err);
if (!pp->ungroup) {
strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
strbuf_reset(&pp->children[i].err);
}
return 1;
}
pp->children[i].process.err = -1;
pp->children[i].process.stdout_to_stderr = 1;
if (!pp->ungroup) {
pp->children[i].process.err = -1;
pp->children[i].process.stdout_to_stderr = 1;
}
pp->children[i].process.no_stdin = 1;
if (start_command(&pp->children[i].process)) {
code = pp->start_failure(&pp->children[i].err,
code = pp->start_failure(pp->ungroup ? NULL :
&pp->children[i].err,
pp->data,
pp->children[i].data);
strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
strbuf_reset(&pp->children[i].err);
if (!pp->ungroup) {
strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
strbuf_reset(&pp->children[i].err);
}
if (code)
pp->shutdown = 1;
return code;
@ -1641,7 +1656,8 @@ 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 (pp->pfd)
pp->pfd[i].fd = pp->children[i].process.err;
return 0;
}
@ -1675,6 +1691,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
static void pp_output(struct parallel_processes *pp)
{
int i = pp->output_owner;
if (pp->children[i].state == GIT_CP_WORKING &&
pp->children[i].err.len) {
strbuf_write(&pp->children[i].err, stderr);
@ -1697,7 +1714,7 @@ static int pp_collect_finished(struct parallel_processes *pp)
code = finish_command(&pp->children[i].process);
code = pp->task_finished(code,
code = pp->task_finished(code, pp->ungroup ? NULL :
&pp->children[i].err, pp->data,
pp->children[i].data);
@ -1708,10 +1725,13 @@ 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 (pp->pfd)
pp->pfd[i].fd = -1;
child_process_init(&pp->children[i].process);
if (i != pp->output_owner) {
if (pp->ungroup) {
; /* 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 {
@ -1748,9 +1768,14 @@ int run_processes_parallel(int n,
int i, code;
int output_timeout = 100;
int spawn_cap = 4;
int ungroup = run_processes_parallel_ungroup;
struct parallel_processes pp;
pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
/* unset for the next API user */
run_processes_parallel_ungroup = 0;
pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,
ungroup);
while (1) {
for (i = 0;
i < spawn_cap && !pp.shutdown &&
@ -1767,8 +1792,15 @@ int run_processes_parallel(int n,
}
if (!pp.nr_processes)
break;
pp_buffer_stderr(&pp, output_timeout);
pp_output(&pp);
if (ungroup) {
int i;
for (i = 0; i < pp.max_processes; i++)
pp.children[i].state = GIT_CP_WAIT_CLEANUP;
} else {
pp_buffer_stderr(&pp, output_timeout);
pp_output(&pp);
}
code = pp_collect_finished(&pp);
if (code) {
pp.shutdown = 1;

30
run-command.h
View File

@ -405,6 +405,9 @@ void check_pipe(int err);
* pp_cb is the callback cookie as passed to run_processes_parallel.
* You can store a child process specific callback cookie in pp_task_cb.
*
* See run_processes_parallel() below for a discussion of the "struct
* strbuf *out" parameter.
*
* Even after returning 0 to indicate that there are no more processes,
* this function will be called again until there are no more running
* child processes.
@ -423,9 +426,8 @@ typedef int (*get_next_task_fn)(struct child_process *cp,
* This callback is called whenever there are problems starting
* a new process.
*
* You must not write to stdout or stderr in this function. Add your
* message to the strbuf out instead, which will be printed without
* messing up the output of the other parallel processes.
* See run_processes_parallel() below for a discussion of the "struct
* strbuf *out" parameter.
*
* pp_cb is the callback cookie as passed into run_processes_parallel,
* pp_task_cb is the callback cookie as passed into get_next_task_fn.
@ -441,9 +443,8 @@ typedef int (*start_failure_fn)(struct strbuf *out,
/**
* This callback is called on every child process that finished processing.
*
* You must not write to stdout or stderr in this function. Add your
* message to the strbuf out instead, which will be printed without
* messing up the output of the other parallel processes.
* See run_processes_parallel() below for a discussion of the "struct
* strbuf *out" parameter.
*
* pp_cb is the callback cookie as passed into run_processes_parallel,
* pp_task_cb is the callback cookie as passed into get_next_task_fn.
@ -464,11 +465,26 @@ typedef int (*task_finished_fn)(int result,
*
* The children started via this function run in parallel. Their output
* (both stdout and stderr) is routed to stderr in a manner that output
* from different tasks does not interleave.
* from different tasks does not interleave (but see "ungroup" below).
*
* start_failure_fn and task_finished_fn can be NULL to omit any
* special handling.
*
* If the "ungroup" option isn't specified, the API will set the
* "stdout_to_stderr" parameter in "struct child_process" and provide
* the callbacks with a "struct strbuf *out" parameter to write output
* to. In this case the callbacks 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.
* The "ungroup" option can be enabled by setting the global
* "run_processes_parallel_ungroup" to "1" before invoking
* run_processes_parallel(), it will be set back to "0" as soon as the
* API reads that setting.
*/
extern int run_processes_parallel_ungroup;
int run_processes_parallel(int n,
get_next_task_fn,
start_failure_fn,

22
t/helper/test-run-command.c
View File

@ -31,7 +31,11 @@ static int parallel_next(struct child_process *cp,
return 0;
strvec_pushv(&cp->args, d->args.v);
strbuf_addstr(err, "preloaded output of a child\n");
if (err)
strbuf_addstr(err, "preloaded output of a child\n");
else
fprintf(stderr, "preloaded output of a child\n");
number_callbacks++;
return 1;
}
@ -41,7 +45,10 @@ static int no_job(struct child_process *cp,
void *cb,
void **task_cb)
{
strbuf_addstr(err, "no further jobs available\n");
if (err)
strbuf_addstr(err, "no further jobs available\n");
else
fprintf(stderr, "no further jobs available\n");
return 0;
}
@ -50,7 +57,10 @@ static int task_finished(int result,
void *pp_cb,
void *pp_task_cb)
{
strbuf_addstr(err, "asking for a quick stop\n");
if (err)
strbuf_addstr(err, "asking for a quick stop\n");
else
fprintf(stderr, "asking for a quick stop\n");
return 1;
}
@ -407,6 +417,12 @@ int cmd__run_command(int argc, const char **argv)
if (!strcmp(argv[1], "run-command"))
exit(run_command(&proc));
if (!strcmp(argv[1], "--ungroup")) {
argv += 1;
argc -= 1;
run_processes_parallel_ungroup = 1;
}
jobs = atoi(argv[2]);
strvec_clear(&proc.args);
strvec_pushv(&proc.args, (const char **)argv + 3);

30
t/t0061-run-command.sh
View File

@ -134,16 +134,34 @@ test_expect_success 'run_command runs in parallel with more jobs available than
test_cmp expect actual
'
test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
test-tool run-command --ungroup run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
test_line_count = 8 out &&
test_line_count = 4 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_cmp expect actual
'
test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
test-tool run-command --ungroup run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
test_line_count = 8 out &&
test_line_count = 4 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_cmp expect actual
'
test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
test-tool run-command --ungroup run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
test_line_count = 8 out &&
test_line_count = 4 err
'
cat >expect <<-EOF
preloaded output of a child
asking for a quick stop
@ -158,6 +176,12 @@ test_expect_success 'run_command is asked to abort gracefully' '
test_cmp expect actual
'
test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
test-tool run-command --ungroup run-command-abort 3 false >out 2>err &&
test_must_be_empty out &&
test_line_count = 6 err
'
cat >expect <<-EOF
no further jobs available
EOF
@ -167,6 +191,12 @@ test_expect_success 'run_command outputs ' '
test_cmp expect actual
'
test_expect_success 'run_command outputs (ungroup) ' '
test-tool run-command --ungroup 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 err
'
test_trace () {
expect="$1"
shift

31
t/t1800-hook.sh
View File

@ -4,6 +4,7 @@ test_description='git-hook command'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-terminal.sh
test_expect_success 'git hook usage' '
test_expect_code 129 git hook &&
@ -120,4 +121,34 @@ test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
test_cmp expect actual
'
test_hook_tty () {
cat >expect <<-\EOF
STDOUT TTY
STDERR TTY
EOF
test_when_finished "rm -rf repo" &&
git init repo &&
test_commit -C repo A &&
test_commit -C repo B &&
git -C repo reset --soft HEAD^ &&
test_hook -C repo pre-commit <<-EOF &&
test -t 1 && echo STDOUT TTY >>actual || echo STDOUT NO TTY >>actual &&
test -t 2 && echo STDERR TTY >>actual || echo STDERR NO TTY >>actual
EOF
test_terminal git -C repo "$@" &&
test_cmp expect repo/actual
}
test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY' '
test_hook_tty hook run pre-commit
'
test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' '
test_hook_tty commit -m"B.new"
'
test_done