| Message ID | 9712aae82bcb51dd94fdc10f4156e9c78e4b6d8c.1724053639.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fixups for git-maintenance(1) tests | expand |
On Mon, Aug 19, 2024 at 09:48:02AM +0200, Patrick Steinhardt wrote: > In t7900, we exercise the `--detach` logic by checking whether the > command ended up writing anything to its output or not. This supposedly > works because we close stdin, stdout and stderr when daemonizing. But > one, it breaks on platforms where daemonize is a no-op, like Windows. > And second, that git-maintenance(1) outputs anything at all in these > tests is a bug in the first place that we'll fix in a subsequent commit. > > Introduce a new trace2 region around the detach which allows us to more > explicitly check whether the detaching logic was executed. This is a > much more direct way to exercise the logic, provides a potentially > useful signal to tracing logs and also works alright on platforms which > do not have the ability to daemonize. Nice, this is so much cleaner than the way the existing test worked. The code looks good, but... > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 074eadcd1c..46a61d66fb 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -950,8 +950,9 @@ test_expect_success '--no-detach causes maintenance to not run in background' ' > # We have no better way to check whether or not the task ran in > # the background than to verify whether it output anything. The > # next testcase checks the reverse, making this somewhat safer. > - git maintenance run --no-detach >out 2>&1 && > - test_line_count = 1 out > + GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ > + git maintenance run --no-detach >out 2>&1 && > + ! test_region maintenance detach trace.txt > ) > ' ...I think this "we have no better way..." comment is now out of date (and can probably just be dropped). -Peff
On Mon, Aug 19, 2024 at 04:51:05AM -0400, Jeff King wrote: > On Mon, Aug 19, 2024 at 09:48:02AM +0200, Patrick Steinhardt wrote: > > > In t7900, we exercise the `--detach` logic by checking whether the > > command ended up writing anything to its output or not. This supposedly > > works because we close stdin, stdout and stderr when daemonizing. But > > one, it breaks on platforms where daemonize is a no-op, like Windows. > > And second, that git-maintenance(1) outputs anything at all in these > > tests is a bug in the first place that we'll fix in a subsequent commit. > > > > Introduce a new trace2 region around the detach which allows us to more > > explicitly check whether the detaching logic was executed. This is a > > much more direct way to exercise the logic, provides a potentially > > useful signal to tracing logs and also works alright on platforms which > > do not have the ability to daemonize. > > Nice, this is so much cleaner than the way the existing test worked. The > code looks good, but... > > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > > index 074eadcd1c..46a61d66fb 100755 > > --- a/t/t7900-maintenance.sh > > +++ b/t/t7900-maintenance.sh > > @@ -950,8 +950,9 @@ test_expect_success '--no-detach causes maintenance to not run in background' ' > > # We have no better way to check whether or not the task ran in > > # the background than to verify whether it output anything. The > > # next testcase checks the reverse, making this somewhat safer. > > - git maintenance run --no-detach >out 2>&1 && > > - test_line_count = 1 out > > + GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ > > + git maintenance run --no-detach >out 2>&1 && > > + ! test_region maintenance detach trace.txt > > ) > > ' > > ...I think this "we have no better way..." comment is now out of date > (and can probably just be dropped). Oops, yes, that one is definitely stale. I'll drop it in the next version of this patch series. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> ...I think this "we have no better way..." comment is now out of date >> (and can probably just be dropped). > > Oops, yes, that one is definitely stale. I'll drop it in the next > version of this patch series. I am not sure if there is a need for "the next version"; in the meantime, let me do this. I'd prefer to merge the main topic down to 'master' soonish. Thanks. 1: 759b453f9f = 1: 759b453f9f t7900: fix flaky test due to leaking background job 2: b64db3e437 ! 2: 51a0b8a2a7 t7900: exercise detaching via trace2 regions @@ Commit message do not have the ability to daemonize. Signed-off-by: Patrick Steinhardt <ps@pks.im> + [jc: dropped a stale in-code comment from a test] Signed-off-by: Junio C Hamano <gitster@pobox.com> ## builtin/gc.c ## @@ builtin/gc.c: static int maintenance_run_tasks(struct maintenance_run_opts *opts ## t/t7900-maintenance.sh ## @@ t/t7900-maintenance.sh: test_expect_success '--no-detach causes maintenance to not run in background' ' - # We have no better way to check whether or not the task ran in - # the background than to verify whether it output anything. The - # next testcase checks the reverse, making this somewhat safer. + git config set maintenance.loose-objects.auto 1 && + git config set maintenance.incremental-repack.enabled true && + +- # We have no better way to check whether or not the task ran in +- # the background than to verify whether it output anything. The +- # next testcase checks the reverse, making this somewhat safer. - git maintenance run --no-detach >out 2>&1 && - test_line_count = 1 out + GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ 3: eac44feff3 = 3: 8311e3b551 builtin/maintenance: fix loose objects task emitting pack hash
On Wed, Aug 21, 2024 at 11:38:02AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> ...I think this "we have no better way..." comment is now out of date > >> (and can probably just be dropped). > > > > Oops, yes, that one is definitely stale. I'll drop it in the next > > version of this patch series. > > I am not sure if there is a need for "the next version"; in the > meantime, let me do this. I'd prefer to merge the main topic down > to 'master' soonish. > > Thanks. Thanks, this looks good to me. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Wed, Aug 21, 2024 at 11:38:02AM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> >> >> ...I think this "we have no better way..." comment is now out of date >> >> (and can probably just be dropped). >> > >> > Oops, yes, that one is definitely stale. I'll drop it in the next >> > version of this patch series. >> >> I am not sure if there is a need for "the next version"; in the >> meantime, let me do this. I'd prefer to merge the main topic down >> to 'master' soonish. >> >> Thanks. > > Thanks, this looks good to me. OK. Let me merge the whole thing to 'next', cook it for a few days and then merge it together with the base topic down to 'master', then. Thanks.
diff --git a/builtin/gc.c b/builtin/gc.c index bafee330a2..13bc0572a3 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1428,8 +1428,11 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts, free(lock_path); /* Failure to daemonize is ok, we'll continue in foreground. */ - if (opts->detach > 0) + if (opts->detach > 0) { + trace2_region_enter("maintenance", "detach", the_repository); daemonize(); + trace2_region_leave("maintenance", "detach", the_repository); + } for (i = 0; !found_selected && i < TASK__COUNT; i++) found_selected = tasks[i].selected_order >= 0; diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 074eadcd1c..46a61d66fb 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -950,8 +950,9 @@ test_expect_success '--no-detach causes maintenance to not run in background' ' # We have no better way to check whether or not the task ran in # the background than to verify whether it output anything. The # next testcase checks the reverse, making this somewhat safer. - git maintenance run --no-detach >out 2>&1 && - test_line_count = 1 out + GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ + git maintenance run --no-detach >out 2>&1 && + ! test_region maintenance detach trace.txt ) ' @@ -971,9 +972,9 @@ test_expect_success '--detach causes maintenance to run in background' ' # process, and by reading stdout we thus essentially wait for # that descriptor to get closed, which indicates that the child # is done, too. - output=$(git maintenance run --detach 2>&1 9>&1) && - printf "%s" "$output" >output && - test_must_be_empty output + does_not_matter=$(GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ + git maintenance run --detach 9>&1) && + test_region maintenance detach trace.txt ) '
In t7900, we exercise the `--detach` logic by checking whether the command ended up writing anything to its output or not. This supposedly works because we close stdin, stdout and stderr when daemonizing. But one, it breaks on platforms where daemonize is a no-op, like Windows. And second, that git-maintenance(1) outputs anything at all in these tests is a bug in the first place that we'll fix in a subsequent commit. Introduce a new trace2 region around the detach which allows us to more explicitly check whether the detaching logic was executed. This is a much more direct way to exercise the logic, provides a potentially useful signal to tracing logs and also works alright on platforms which do not have the ability to daemonize. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/gc.c | 5 ++++- t/t7900-maintenance.sh | 11 ++++++----- 2 files changed, 10 insertions(+), 6 deletions(-)