| Message ID | cover-v5-0.2-00000000000-20220602T131858Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hook API: connect hooks to the TTY again, fixes a v2.36.0 regression | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > This series fixes a v2.36.0 regression[1]. See [2] for the v4. The > reasons for why a regression needs this relatively large change to > move forward is discussed in past rounds, e.g. around [3]. CI at > https://github.com/avar/git/actions/runs/2428475773 > > Changes since v4, mainly to address comments by Johannes (thanks for > the review!): This version looks good to me. > @@ run-command.c: static void pp_init(struct parallel_processes *pp, > for (i = 0; i < n; i++) { > strbuf_init(&pp->children[i].err, 0); > child_process_init(&pp->children[i].process); > -+ if (!pp->pfd) > -+ continue; > - pp->pfd[i].events = POLLIN | POLLHUP; > - pp->pfd[i].fd = -1; > +- 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; > ++ } > } This change is merely a personal taste---it does not match mine but that is Meh ;-) > -@@ run-command.c: static void pp_cleanup(struct parallel_processes *pp) > - */ > - static int pp_start_one(struct parallel_processes *pp) > - { > -+ const int ungroup = pp->ungroup; It may have made the resulting code easier to read if the local variable was kept as a synonym as "pp->" is short enough but is repeated often, but what is written is good enough and I do not see a need to flip-flop. > -+static void pp_mark_ungrouped_for_cleanup(struct parallel_processes *pp) > -+{ > -+ int i; > -+ > -+ if (!pp->ungroup) > -+ BUG("only reachable if 'ungrouped'"); > -+ > -+ for (i = 0; i < pp->max_processes; i++) > -+ pp->children[i].state = GIT_CP_WAIT_CLEANUP; > -+} Good to see this inlined. I find the caller easier to follow without it. Thanks for a quick succession of rerolling. Will queue.
Hi Ævar On 02/06/2022 15:07, Ævar Arnfjörð Bjarmason wrote: > This series fixes a v2.36.0 regression[1]. See [2] for the v4. The > reasons for why a regression needs this relatively large change to > move forward is discussed in past rounds, e.g. around [3]. CI at > https://github.com/avar/git/actions/runs/2428475773 > > Changes since v4, mainly to address comments by Johannes (thanks for > the review!): > > * First, some things like renaming "ungroup" to something else & > rewriting the tests I didn't do because I thought keeping the > inter/range-diff down in size outweighed re-arranging or changing > the code at this late stage. > > In the case of the suggested shorter test in > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206011827300.349@tvgsbejvaqbjf.bet/ > the replacement wasn't testing the same thing. I.e. we don't see > what's connected to a TTY if we redirect one of stdout or stderr > anymore, which is important to get right. I'm a bit confused by this, the proposed test uses this hook script write_script .git/hooks/pre-commit <<-EOF test -t 1 && echo "stdout is a TTY" >out test -t 2 && echo "stderr is a TTY" >>out EOF if either of stderr or stdout is redirected then the corresponding "test -t" should fail and so we will detect that it is not a tty. Best Wishes Phillip
On Fri, Jun 03 2022, Phillip Wood wrote: > Hi Ævar > > On 02/06/2022 15:07, Ævar Arnfjörð Bjarmason wrote: >> This series fixes a v2.36.0 regression[1]. See [2] for the v4. The >> reasons for why a regression needs this relatively large change to >> move forward is discussed in past rounds, e.g. around [3]. CI at >> https://github.com/avar/git/actions/runs/2428475773 >> Changes since v4, mainly to address comments by Johannes (thanks for >> the review!): >> * First, some things like renaming "ungroup" to something else & >> rewriting the tests I didn't do because I thought keeping the >> inter/range-diff down in size outweighed re-arranging or changing >> the code at this late stage. >> In the case of the suggested shorter test in >> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206011827300.349@tvgsbejvaqbjf.bet/ >> the replacement wasn't testing the same thing. I.e. we don't see >> what's connected to a TTY if we redirect one of stdout or stderr >> anymore, which is important to get right. > > I'm a bit confused by this, the proposed test uses this hook script > > write_script .git/hooks/pre-commit <<-EOF > test -t 1 && echo "stdout is a TTY" >out > test -t 2 && echo "stderr is a TTY" >>out > EOF > > if either of stderr or stdout is redirected then the corresponding > "test -t" should fail and so we will detect that it is not a tty. Yes, exactly, but the proposed test doesn't test that, in that case both of them are connected, the test in 2/2 does test that case. Can that snippet bebe made to work? Sure, but I know the test I have works, and that proposed replacement didn't even pass chainlint (i.e. hasn't been run even once in our test suite). So I didn't think that trying to micro-optimize the test length was worth it in this case. It's also getting much of that length reduction e.g. by not cleaning up after itself, which the test in 2/2 does.
Hi Ævar On 03/06/2022 10:20, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Jun 03 2022, Phillip Wood wrote: > >> Hi Ævar >> >> On 02/06/2022 15:07, Ævar Arnfjörð Bjarmason wrote: >>> This series fixes a v2.36.0 regression[1]. See [2] for the v4. The >>> reasons for why a regression needs this relatively large change to >>> move forward is discussed in past rounds, e.g. around [3]. CI at >>> https://github.com/avar/git/actions/runs/2428475773 >>> Changes since v4, mainly to address comments by Johannes (thanks for >>> the review!): >>> * First, some things like renaming "ungroup" to something else & >>> rewriting the tests I didn't do because I thought keeping the >>> inter/range-diff down in size outweighed re-arranging or changing >>> the code at this late stage. >>> In the case of the suggested shorter test in >>> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206011827300.349@tvgsbejvaqbjf.bet/ >>> the replacement wasn't testing the same thing. I.e. we don't see >>> what's connected to a TTY if we redirect one of stdout or stderr >>> anymore, which is important to get right. >> >> I'm a bit confused by this, the proposed test uses this hook script >> >> write_script .git/hooks/pre-commit <<-EOF >> test -t 1 && echo "stdout is a TTY" >out >> test -t 2 && echo "stderr is a TTY" >>out >> EOF >> >> if either of stderr or stdout is redirected then the corresponding >> "test -t" should fail and so we will detect that it is not a tty. > > Yes, exactly, but the proposed test doesn't test that, in that case both > of them are connected, the test in 2/2 does test that case. I think I must be missing something. As I understand it we want to check that the hook can see a tty on stdout and stderr. In the test above we'll get a line printed for each fd that is a tty. Your test always redirects one of stdout and stderr - why is it important to test that? - it feels like it is testing the shell's redirection code rather than git. I was concerned that we had also regressed the handling of stdin but looking at (the now deleted) run_hook_ve() it used to set .no_stdin = 1 so that is unchanged in the new code. Best Wishes Phillip > Can that snippet bebe made to work? Sure, but I know the test I have > works, and that proposed replacement didn't even pass chainlint > (i.e. hasn't been run even once in our test suite). So I didn't think > that trying to micro-optimize the test length was worth it in this case. > > It's also getting much of that length reduction e.g. by not cleaning up > after itself, which the test in 2/2 does. >
On Fri, Jun 03 2022, Phillip Wood wrote: > Hi Ævar > > On 03/06/2022 10:20, Ævar Arnfjörð Bjarmason wrote: >> On Fri, Jun 03 2022, Phillip Wood wrote: >> >>> Hi Ævar >>> >>> On 02/06/2022 15:07, Ævar Arnfjörð Bjarmason wrote: >>>> This series fixes a v2.36.0 regression[1]. See [2] for the v4. The >>>> reasons for why a regression needs this relatively large change to >>>> move forward is discussed in past rounds, e.g. around [3]. CI at >>>> https://github.com/avar/git/actions/runs/2428475773 >>>> Changes since v4, mainly to address comments by Johannes (thanks for >>>> the review!): >>>> * First, some things like renaming "ungroup" to something else & >>>> rewriting the tests I didn't do because I thought keeping the >>>> inter/range-diff down in size outweighed re-arranging or changing >>>> the code at this late stage. >>>> In the case of the suggested shorter test in >>>> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206011827300.349@tvgsbejvaqbjf.bet/ >>>> the replacement wasn't testing the same thing. I.e. we don't see >>>> what's connected to a TTY if we redirect one of stdout or stderr >>>> anymore, which is important to get right. >>> >>> I'm a bit confused by this, the proposed test uses this hook script >>> >>> write_script .git/hooks/pre-commit <<-EOF >>> test -t 1 && echo "stdout is a TTY" >out >>> test -t 2 && echo "stderr is a TTY" >>out >>> EOF >>> >>> if either of stderr or stdout is redirected then the corresponding >>> "test -t" should fail and so we will detect that it is not a tty. >> Yes, exactly, but the proposed test doesn't test that, in that case >> both >> of them are connected, the test in 2/2 does test that case. > > I think I must be missing something. As I understand it we want to > check that the hook can see a tty on stdout and stderr. In the test > above we'll get a line printed for each fd that is a tty. Your test > always redirects one of stdout and stderr - why is it important to > test that? - it feels like it is testing the shell's redirection code > rather than git. Yes, I think I'm the one who was missing something. I looked at this again and I thought I'd been testing that e.g. one of the two not returning true from isatty() wasn't making both "not TTY", i.e. that run-command.c wasn't performing some shenanigans. But that was probably too paranoid, and in any case I couldn't find a good way to test it. > I was concerned that we had also regressed the handling of stdin but > looking at (the now deleted) run_hook_ve() it used to set .no_stdin = > 1 so that is unchanged in the new code. *nod* I re-rolled a v6 just now which I think should address your comments here: https://lore.kernel.org/git/cover-v6-0.2-00000000000-20220606T170356Z-avarab@gmail.com/ I've still kept the "clean up after yourself" etc. behavior in the test, and since it was easy we now test both "git hook run" and "git commit". Thanks a lot for the careful review.
This series fixes a v2.36.0 regression[1]. See [2] for the v4. The reasons for why a regression needs this relatively large change to move forward is discussed in past rounds, e.g. around [3]. CI at https://github.com/avar/git/actions/runs/2428475773 Changes since v4, mainly to address comments by Johannes (thanks for the review!): * First, some things like renaming "ungroup" to something else & rewriting the tests I didn't do because I thought keeping the inter/range-diff down in size outweighed re-arranging or changing the code at this late stage. In the case of the suggested shorter test in https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206011827300.349@tvgsbejvaqbjf.bet/ the replacement wasn't testing the same thing. I.e. we don't see what's connected to a TTY if we redirect one of stdout or stderr anymore, which is important to get right. * Ditto the suggestion to e.g. add a parameter for "ungroup". I agree that's better, but that approach was in the earlier and much larger round[4], here we're trying to aim for the smallest possible regression fix by line count & complexity. * I retained the performance test(s) for "parallel" and "git hook run" in 1/2 and 2/2. Yes, the former isn't ours, but I think it helps to explain the code, implementation and resulting performance with reference to existing well-known software that's doing the exact same thing we're doing here. * Stopped using "const" in "const int ungroup", and dropped some of those variables entirely. * Inlined the pp_mark_ungrouped_for_cleanup() function. I added an "int i" in the inner scope in run_processes_parallel() even though we have one in the outer, just to make it clear that we're not caring about the other one (or clobbering it). * I just got rid of the two added BUG(). It's obvious enough from the calling code that those two functions are !ungroup only, so we can do without the sprinkling of BUG() and larger resulting diff. * Passed an --ungroup parameter in the tests instead of passing a parameter by environment variable. * Fixed a minor s/reported in/reported against/ phrasing in the 2/2 commit message. 1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/ 2. https://lore.kernel.org/git/cover-v4-0.2-00000000000-20220531T173005Z-avarab@gmail.com/ 3. https://lore.kernel.org/git/220526.86pmk060xa.gmgdl@evledraar.gmail.com/ 4. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/ Ævar Arnfjörð Bjarmason (2): run-command: add an "ungroup" option to run_process_parallel() hook API: fix v2.36.0 regression: hooks should be connected to a TTY hook.c | 1 + run-command.c | 70 +++++++++++++++++++++++++++---------- run-command.h | 30 ++++++++++++---- t/helper/test-run-command.c | 22 ++++++++++-- t/t0061-run-command.sh | 30 ++++++++++++++++ t/t1800-hook.sh | 37 ++++++++++++++++++++ 6 files changed, 161 insertions(+), 29 deletions(-) Range-diff against v4: 1: f1170b02553 ! 1: d018b7c4441 run-command: add an "ungroup" option to run_process_parallel() @@ Commit message NTTY Another is as GNU parallel's documentation notes a potential for - optimization. Our results will be a bit different, but in cases where + optimization. As demonstrated in next commit our results with "git + hook run" will be similar, but generally speaking this shows that if you want to run processes in parallel where the exact order isn't important this can be a lot faster: @@ run-command.c: static void pp_init(struct parallel_processes *pp, start_failure_fn start_failure, task_finished_fn task_finished, - void *data) -+ void *data, const int ungroup) ++ void *data, int ungroup) { int i; @@ run-command.c: static void pp_init(struct parallel_processes *pp, for (i = 0; i < n; i++) { strbuf_init(&pp->children[i].err, 0); child_process_init(&pp->children[i].process); -+ if (!pp->pfd) -+ continue; - pp->pfd[i].events = POLLIN | POLLHUP; - pp->pfd[i].fd = -1; +- 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; ++ } } -@@ run-command.c: static void pp_cleanup(struct parallel_processes *pp) - */ - static int pp_start_one(struct parallel_processes *pp) - { -+ const int ungroup = pp->ungroup; - int i, code; - for (i = 0; i < pp->max_processes; i++) + pp_for_signal = pp; @@ run-command.c: 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, -+ ungroup ? NULL : &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 (!ungroup) { ++ if (!pp->ungroup) { + strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); + strbuf_reset(&pp->children[i].err); + } @@ 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; -+ if (!ungroup) { ++ if (!pp->ungroup) { + pp->children[i].process.err = -1; + pp->children[i].process.stdout_to_stderr = 1; + } @@ run-command.c: static int pp_start_one(struct parallel_processes *pp) if (start_command(&pp->children[i].process)) { - code = pp->start_failure(&pp->children[i].err, -+ code = pp->start_failure(ungroup ? NULL : &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 (!ungroup) { ++ if (!pp->ungroup) { + strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); + strbuf_reset(&pp->children[i].err); + } @@ run-command.c: static int pp_start_one(struct parallel_processes *pp) return 0; } -+static void pp_mark_ungrouped_for_cleanup(struct parallel_processes *pp) -+{ -+ int i; -+ -+ if (!pp->ungroup) -+ BUG("only reachable if 'ungrouped'"); -+ -+ for (i = 0; i < pp->max_processes; i++) -+ pp->children[i].state = GIT_CP_WAIT_CLEANUP; -+} -+ - static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) - { - int i; - -+ if (pp->ungroup) -+ BUG("unreachable with 'ungrouped'"); -+ - while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) { - if (errno == EINTR) - continue; @@ run-command.c: 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->ungroup) -+ BUG("unreachable with 'ungrouped'"); + if (pp->children[i].state == GIT_CP_WORKING && pp->children[i].err.len) { strbuf_write(&pp->children[i].err, stderr); -@@ run-command.c: static void pp_output(struct parallel_processes *pp) - - static int pp_collect_finished(struct parallel_processes *pp) - { -+ const int ungroup = pp->ungroup; - int i, code; - int n = pp->max_processes; - int result = 0; @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp) + code = finish_command(&pp->children[i].process); - code = pp->task_finished(code, -- &pp->children[i].err, pp->data, -- pp->children[i].data); -+ ungroup ? NULL : &pp->children[i].err, -+ pp->data, pp->children[i].data); +- code = pp->task_finished(code, ++ code = pp->task_finished(code, pp->ungroup ? NULL : + &pp->children[i].err, pp->data, + pp->children[i].data); - if (code) - result = code; @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp) pp->nr_processes--; @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp) child_process_init(&pp->children[i].process); - if (i != pp->output_owner) { -+ if (ungroup) { ++ 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 { @@ run-command.c: 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; -+ const int ungroup = run_processes_parallel_ungroup; - pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb); + /* unset for the next API user */ @@ run-command.c: int run_processes_parallel(int n, - pp_buffer_stderr(&pp, output_timeout); - pp_output(&pp); + if (ungroup) { -+ pp_mark_ungrouped_for_cleanup(&pp); ++ 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); @@ 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); + if (!strcmp(argv[1], "run-command")) + exit(run_command(&proc)); -+ if (getenv("RUN_PROCESSES_PARALLEL_UNGROUP")) ++ if (!strcmp(argv[1], "--ungroup")) { ++ argv += 1; ++ argc -= 1; + run_processes_parallel_ungroup = 1; ++ } + - if (!strcmp(argv[1], "run-command-parallel")) - exit(run_processes_parallel(jobs, parallel_next, - NULL, NULL, &proc)); + jobs = atoi(argv[2]); + strvec_clear(&proc.args); + strvec_pushv(&proc.args, (const char **)argv + 3); ## t/t0061-run-command.sh ## @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with more jobs available than @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m ' +test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' ' -+ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ -+ test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && ++ 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 +' @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m ' +test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' ' -+ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ -+ test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && ++ 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 +' @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m ' +test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' ' -+ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ -+ test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && ++ 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 +' @@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort grace ' +test_expect_success 'run_command is asked to abort gracefully (ungroup)' ' -+ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ -+ test-tool run-command run-command-abort 3 false >out 2>err && ++ test-tool run-command --ungroup run-command-abort 3 false >out 2>err && + test_must_be_empty out && + test_line_count = 6 err +' @@ t/t0061-run-command.sh: test_expect_success 'run_command outputs ' ' ' +test_expect_success 'run_command outputs (ungroup) ' ' -+ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ -+ test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && ++ 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 +' 2: 8ab09f28729 ! 2: b0f0dc7492a hook API: fix v2.36.0 regression: hooks should be connected to a TTY @@ Metadata ## Commit message ## hook API: fix v2.36.0 regression: hooks should be connected to a TTY - Fix a regression reported[1] in f443246b9f2 (commit: convert + Fix a regression reported[1] against f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and