| Message ID | 8d6cbae951177718b49d5cfbbeca2d5b0073e266.1723533091.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/maintenance: fix auto-detach with non-standard tasks | expand |
Hi Patrick On 13/08/2024 08:18, Patrick Steinhardt wrote: > > Fix this bug by asking git-gc(1) to not detach when it is being invoked > via git-maintenance(1). Instead, the latter command now respects a new > config "maintenance.autoDetach", the equivalent of "gc.autoDetach", and > detaches itself into the background if not told otherwise. This should > continue to behave the same for all users which use the git-gc(1) task, > only. For others though, it means that we now properly perform all tasks > in the background. I fear that users who are running "git maintenance" from a scheduler such as cron are likely to be surprised by this change in behavior. At the very least "git maintenance" will no-longer return a meaningful exit code. Perhaps we could switch the logic to be opt in and pass "--detach" (or "-c maintenance.autoDetach=true") when running "git maintenance" automatically from "git rebase" etc. Best Wishes Phillip > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/gc.c | 1 + > run-command.c | 12 ++++++++++- > t/t5616-partial-clone.sh | 6 +++--- > t/t7900-maintenance.sh | 43 +++++++++++++++++++++++++++++++--------- > 4 files changed, 49 insertions(+), 13 deletions(-) > > diff --git a/builtin/gc.c b/builtin/gc.c > index 63106e2028..bafee330a2 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -1063,6 +1063,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts, > strvec_push(&child.args, "--quiet"); > else > strvec_push(&child.args, "--no-quiet"); > + strvec_push(&child.args, "--no-detach"); > > return run_command(&child); > } > diff --git a/run-command.c b/run-command.c > index 45ba544932..94f2f3079f 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -1808,16 +1808,26 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) > > int prepare_auto_maintenance(int quiet, struct child_process *maint) > { > - int enabled; > + int enabled, auto_detach; > > if (!git_config_get_bool("maintenance.auto", &enabled) && > !enabled) > return 0; > > + /* > + * When `maintenance.autoDetach` isn't set, then we fall back to > + * honoring `gc.autoDetach`. This is somewhat weird, but required to > + * retain behaviour from when we used to run git-gc(1) here. > + */ > + if (git_config_get_bool("maintenance.autodetach", &auto_detach) && > + git_config_get_bool("gc.autodetach", &auto_detach)) > + auto_detach = 1; > + > maint->git_cmd = 1; > maint->close_object_store = 1; > strvec_pushl(&maint->args, "maintenance", "run", "--auto", NULL); > strvec_push(&maint->args, quiet ? "--quiet" : "--no-quiet"); > + strvec_push(&maint->args, auto_detach ? "--detach" : "--no-detach"); > > return 1; > } > diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh > index 2da7291e37..8415884754 100755 > --- a/t/t5616-partial-clone.sh > +++ b/t/t5616-partial-clone.sh > @@ -229,7 +229,7 @@ test_expect_success 'fetch --refetch triggers repacking' ' > > GIT_TRACE2_EVENT="$PWD/trace1.event" \ > git -C pc1 fetch --refetch origin && > - test_subcommand git maintenance run --auto --no-quiet <trace1.event && > + test_subcommand git maintenance run --auto --no-quiet --detach <trace1.event && > grep \"param\":\"gc.autopacklimit\",\"value\":\"1\" trace1.event && > grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"-1\" trace1.event && > > @@ -238,7 +238,7 @@ test_expect_success 'fetch --refetch triggers repacking' ' > -c gc.autoPackLimit=0 \ > -c maintenance.incremental-repack.auto=1234 \ > -C pc1 fetch --refetch origin && > - test_subcommand git maintenance run --auto --no-quiet <trace2.event && > + test_subcommand git maintenance run --auto --no-quiet --detach <trace2.event && > grep \"param\":\"gc.autopacklimit\",\"value\":\"0\" trace2.event && > grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"-1\" trace2.event && > > @@ -247,7 +247,7 @@ test_expect_success 'fetch --refetch triggers repacking' ' > -c gc.autoPackLimit=1234 \ > -c maintenance.incremental-repack.auto=0 \ > -C pc1 fetch --refetch origin && > - test_subcommand git maintenance run --auto --no-quiet <trace3.event && > + test_subcommand git maintenance run --auto --no-quiet --detach <trace3.event && > grep \"param\":\"gc.autopacklimit\",\"value\":\"1\" trace3.event && > grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"0\" trace3.event > ' > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 771525aa4b..06ab43cfb5 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -49,22 +49,47 @@ test_expect_success 'run [--auto|--quiet]' ' > git maintenance run --auto 2>/dev/null && > GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \ > git maintenance run --no-quiet 2>/dev/null && > - test_subcommand git gc --quiet <run-no-auto.txt && > - test_subcommand ! git gc --auto --quiet <run-auto.txt && > - test_subcommand git gc --no-quiet <run-no-quiet.txt > + test_subcommand git gc --quiet --no-detach <run-no-auto.txt && > + test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt && > + test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt > ' > > test_expect_success 'maintenance.auto config option' ' > GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 && > - test_subcommand git maintenance run --auto --quiet <default && > + test_subcommand git maintenance run --auto --quiet --detach <default && > GIT_TRACE2_EVENT="$(pwd)/true" \ > git -c maintenance.auto=true \ > commit --quiet --allow-empty -m 2 && > - test_subcommand git maintenance run --auto --quiet <true && > + test_subcommand git maintenance run --auto --quiet --detach <true && > GIT_TRACE2_EVENT="$(pwd)/false" \ > git -c maintenance.auto=false \ > commit --quiet --allow-empty -m 3 && > - test_subcommand ! git maintenance run --auto --quiet <false > + test_subcommand ! git maintenance run --auto --quiet --detach <false > +' > + > +for cfg in maintenance.autoDetach gc.autoDetach > +do > + test_expect_success "$cfg=true config option" ' > + test_when_finished "rm -f trace" && > + test_config $cfg true && > + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && > + test_subcommand git maintenance run --auto --quiet --detach <trace > + ' > + > + test_expect_success "$cfg=false config option" ' > + test_when_finished "rm -f trace" && > + test_config $cfg false && > + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && > + test_subcommand git maintenance run --auto --quiet --no-detach <trace > + ' > +done > + > +test_expect_success "maintenance.autoDetach overrides gc.autoDetach" ' > + test_when_finished "rm -f trace" && > + test_config maintenance.autoDetach false && > + test_config gc.autoDetach true && > + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && > + test_subcommand git maintenance run --auto --quiet --no-detach <trace > ' > > test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' > @@ -129,9 +154,9 @@ test_expect_success 'run --task=<task>' ' > git maintenance run --task=commit-graph 2>/dev/null && > GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \ > git maintenance run --task=commit-graph --task=gc 2>/dev/null && > - test_subcommand ! git gc --quiet <run-commit-graph.txt && > - test_subcommand git gc --quiet <run-gc.txt && > - test_subcommand git gc --quiet <run-both.txt && > + test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt && > + test_subcommand git gc --quiet --no-detach <run-gc.txt && > + test_subcommand git gc --quiet --no-detach <run-both.txt && > test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt && > test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt && > test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt
On Tue, Aug 13, 2024 at 12:29:47PM +0100, Phillip Wood wrote: > Hi Patrick > > On 13/08/2024 08:18, Patrick Steinhardt wrote: > > > > Fix this bug by asking git-gc(1) to not detach when it is being invoked > > via git-maintenance(1). Instead, the latter command now respects a new > > config "maintenance.autoDetach", the equivalent of "gc.autoDetach", and > > detaches itself into the background if not told otherwise. This should > > continue to behave the same for all users which use the git-gc(1) task, > > only. For others though, it means that we now properly perform all tasks > > in the background. > > I fear that users who are running "git maintenance" from a scheduler such as > cron are likely to be surprised by this change in behavior. At the very > least "git maintenance" will no-longer return a meaningful exit code. > Perhaps we could switch the logic to be opt in and pass "--detach" (or "-c > maintenance.autoDetach=true") when running "git maintenance" automatically > from "git rebase" etc. It's actually the reverse: the old behaviour when run via a scheduler was to detach by default, because git-gc(1) did. We now ask it to not detach anymore, which fixes this. Furthermore, the default behaviour of `git maintenance run` did not change either: it stays in the foreground unless the `--detach` flag is given. So the thing you worry about is actually getting fixed by this series :) What _does_ change though is when we run `git maintenance` via our auto-maintenance framework. Here we now do detach the whole maintenance process, instead of only git-gc(1). This logic is only being executed by random commands (git-rebase, git-pull, git-commit etc), and I'd argue it is the expected behaviour. Patrick
On 13/08/2024 12:59, Patrick Steinhardt wrote: > On Tue, Aug 13, 2024 at 12:29:47PM +0100, Phillip Wood wrote: >> Hi Patrick >> >> On 13/08/2024 08:18, Patrick Steinhardt wrote: >>> >>> Fix this bug by asking git-gc(1) to not detach when it is being invoked >>> via git-maintenance(1). Instead, the latter command now respects a new >>> config "maintenance.autoDetach", the equivalent of "gc.autoDetach", and >>> detaches itself into the background if not told otherwise. This should >>> continue to behave the same for all users which use the git-gc(1) task, >>> only. For others though, it means that we now properly perform all tasks >>> in the background. >> >> I fear that users who are running "git maintenance" from a scheduler such as >> cron are likely to be surprised by this change in behavior. At the very >> least "git maintenance" will no-longer return a meaningful exit code. >> Perhaps we could switch the logic to be opt in and pass "--detach" (or "-c >> maintenance.autoDetach=true") when running "git maintenance" automatically >> from "git rebase" etc. > > It's actually the reverse: the old behaviour when run via a scheduler > was to detach by default, because git-gc(1) did. Oh, I misunderstood what this patch is changing. So despite being tagged builtin/maintenance and talking about "git maintenance" it does not actually touch builtin/maintenance.c or change its behavior. What it is actually doing is changing how other git commands run "git maintenance --auto" so that it is always run in the background unless the user configures maintenance.autoDetach=false. That sounds like a good change. Thanks for clarifying Phillip > We now ask it to not > detach anymore, which fixes this. Furthermore, the default behaviour of > `git maintenance run` did not change either: it stays in the foreground > unless the `--detach` flag is given. So the thing you worry about is > actually getting fixed by this series :) > > What _does_ change though is when we run `git maintenance` via our > auto-maintenance framework. Here we now do detach the whole maintenance > process, instead of only git-gc(1). This logic is only being executed by > random commands (git-rebase, git-pull, git-commit etc), and I'd argue it > is the expected behaviour. > > Patrick >
On Tue, Aug 13, 2024 at 02:19:20PM +0100, Phillip Wood wrote: > On 13/08/2024 12:59, Patrick Steinhardt wrote: > > On Tue, Aug 13, 2024 at 12:29:47PM +0100, Phillip Wood wrote: > > > Hi Patrick > > > > > > On 13/08/2024 08:18, Patrick Steinhardt wrote: > > > > > > > > Fix this bug by asking git-gc(1) to not detach when it is being invoked > > > > via git-maintenance(1). Instead, the latter command now respects a new > > > > config "maintenance.autoDetach", the equivalent of "gc.autoDetach", and > > > > detaches itself into the background if not told otherwise. This should > > > > continue to behave the same for all users which use the git-gc(1) task, > > > > only. For others though, it means that we now properly perform all tasks > > > > in the background. > > > > > > I fear that users who are running "git maintenance" from a scheduler such as > > > cron are likely to be surprised by this change in behavior. At the very > > > least "git maintenance" will no-longer return a meaningful exit code. > > > Perhaps we could switch the logic to be opt in and pass "--detach" (or "-c > > > maintenance.autoDetach=true") when running "git maintenance" automatically > > > from "git rebase" etc. > > > > It's actually the reverse: the old behaviour when run via a scheduler > > was to detach by default, because git-gc(1) did. > > Oh, I misunderstood what this patch is changing. So despite being tagged > builtin/maintenance and talking about "git maintenance" it does not actually > touch builtin/maintenance.c or change its behavior. What it is actually > doing is changing how other git commands run "git maintenance --auto" so > that it is always run in the background unless the user configures > maintenance.autoDetach=false. That sounds like a good change. > > Thanks for clarifying Yes. I should've probably prefixed this with "run-command:", not with "builtin/maintenance". Patrick
On 14/08/2024 05:15, Patrick Steinhardt wrote: > On Tue, Aug 13, 2024 at 02:19:20PM +0100, Phillip Wood wrote: >> On 13/08/2024 12:59, Patrick Steinhardt wrote: >> >> Oh, I misunderstood what this patch is changing. So despite being tagged >> builtin/maintenance and talking about "git maintenance" it does not actually >> touch builtin/maintenance.c or change its behavior. What it is actually >> doing is changing how other git commands run "git maintenance --auto" so >> that it is always run in the background unless the user configures >> maintenance.autoDetach=false. That sounds like a good change. >> >> Thanks for clarifying Sorry my message sounds grumpier than I intended. > Yes. I should've probably prefixed this with "run-command:", not with > "builtin/maintenance". That's a good idea, I think the important thing we want to convey is that we're changing the way we run "git maintenance --auto", not the behavior of "git maintenance" itself. Thanks Phillip > Patrick >
On Wed, Aug 14, 2024 at 04:13:05PM +0100, Phillip Wood wrote: > On 14/08/2024 05:15, Patrick Steinhardt wrote: > > On Tue, Aug 13, 2024 at 02:19:20PM +0100, Phillip Wood wrote: > > > On 13/08/2024 12:59, Patrick Steinhardt wrote: > > > > > > Oh, I misunderstood what this patch is changing. So despite being tagged > > > builtin/maintenance and talking about "git maintenance" it does not actually > > > touch builtin/maintenance.c or change its behavior. What it is actually > > > doing is changing how other git commands run "git maintenance --auto" so > > > that it is always run in the background unless the user configures > > > maintenance.autoDetach=false. That sounds like a good change. > > > > > > Thanks for clarifying > > Sorry my message sounds grumpier than I intended. No worries, I didn't receive it as grumpy. > > Yes. I should've probably prefixed this with "run-command:", not with > > "builtin/maintenance". > > That's a good idea, I think the important thing we want to convey is that > we're changing the way we run "git maintenance --auto", not the behavior of > "git maintenance" itself. I'll have a look in case I need to reroll this series. Patrick
On Tue Aug 13, 2024 at 5:18 PM AEST, Patrick Steinhardt wrote: > diff --git a/run-command.c b/run-command.c > index 45ba544932..94f2f3079f 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -1808,16 +1808,26 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) > > int prepare_auto_maintenance(int quiet, struct child_process *maint) > { > - int enabled; > + int enabled, auto_detach; > > if (!git_config_get_bool("maintenance.auto", &enabled) && > !enabled) > return 0; > > + /* > + * When `maintenance.autoDetach` isn't set, then we fall back to > + * honoring `gc.autoDetach`. This is somewhat weird, but required to > + * retain behaviour from when we used to run git-gc(1) here. > + */ > + if (git_config_get_bool("maintenance.autodetach", &auto_detach) && > + git_config_get_bool("gc.autodetach", &auto_detach)) > + auto_detach = 1; > + Do the two `*.autodetach` values need to be camel-cased or does it not matter? I've noticed a mix of both through the codebase so I suppose it's not case-sensitive.
On Thu, Aug 15, 2024 at 04:40:51PM +1000, James Liu wrote: > On Tue Aug 13, 2024 at 5:18 PM AEST, Patrick Steinhardt wrote: > > diff --git a/run-command.c b/run-command.c > > index 45ba544932..94f2f3079f 100644 > > --- a/run-command.c > > +++ b/run-command.c > > @@ -1808,16 +1808,26 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) > > > > int prepare_auto_maintenance(int quiet, struct child_process *maint) > > { > > - int enabled; > > + int enabled, auto_detach; > > > > if (!git_config_get_bool("maintenance.auto", &enabled) && > > !enabled) > > return 0; > > > > + /* > > + * When `maintenance.autoDetach` isn't set, then we fall back to > > + * honoring `gc.autoDetach`. This is somewhat weird, but required to > > + * retain behaviour from when we used to run git-gc(1) here. > > + */ > > + if (git_config_get_bool("maintenance.autodetach", &auto_detach) && > > + git_config_get_bool("gc.autodetach", &auto_detach)) > > + auto_detach = 1; > > + > > Do the two `*.autodetach` values need to be camel-cased or does it not > matter? I've noticed a mix of both through the codebase so I suppose > it's not case-sensitive. Config keys are case-insensitive in general, and as far as I am aware we typically use the all-lowercase variant when retrieving config keys. On the other hand, in our docs we spell the config keys camel-cased to help the user make sense of it. Patrick
On 8/13/24 3:18 AM, Patrick Steinhardt wrote: > @@ -1063,6 +1063,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts, > strvec_push(&child.args, "--quiet"); > else > strvec_push(&child.args, "--no-quiet"); > + strvec_push(&child.args, "--no-detach"); > > return run_command(&child); > } I was looking for this earlier, as it could have been placed in either of the previous two patches. I can understand not putting it in Patch 5 because then the default of 'git maintenance run --auto' would not daemonize the gc subprocess. But it seems like patch 6 would also have been fine. Here is good, too. > + /* > + * When `maintenance.autoDetach` isn't set, then we fall back to > + * honoring `gc.autoDetach`. This is somewhat weird, but required to > + * retain behaviour from when we used to run git-gc(1) here. > + */ > + if (git_config_get_bool("maintenance.autodetach", &auto_detach) && > + git_config_get_bool("gc.autodetach", &auto_detach)) > + auto_detach = 1; > + This && caught me by surprise, but it's really "if both of these config options are unset, then set a default." Makes sense after thinking a bit harder. > maint->git_cmd = 1; > maint->close_object_store = 1; > strvec_pushl(&maint->args, "maintenance", "run", "--auto", NULL); > strvec_push(&maint->args, quiet ? "--quiet" : "--no-quiet"); > + strvec_push(&maint->args, auto_detach ? "--detach" : "--no-detach"); > > return 1; > } > diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh > index 2da7291e37..8415884754 100755 > --- a/t/t5616-partial-clone.sh > +++ b/t/t5616-partial-clone.sh > @@ -229,7 +229,7 @@ test_expect_success 'fetch --refetch triggers repacking' ' > > GIT_TRACE2_EVENT="$PWD/trace1.event" \ > git -C pc1 fetch --refetch origin && > - test_subcommand git maintenance run --auto --no-quiet <trace1.event && > + test_subcommand git maintenance run --auto --no-quiet --detach <trace1.event && And this changes because it's the new default. You don't need config change to the test repo to make this happen. Good. > +for cfg in maintenance.autoDetach gc.autoDetach > +do > + test_expect_success "$cfg=true config option" ' > + test_when_finished "rm -f trace" && > + test_config $cfg true && > + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && > + test_subcommand git maintenance run --auto --quiet --detach <trace > + ' > + > + test_expect_success "$cfg=false config option" ' > + test_when_finished "rm -f trace" && > + test_config $cfg false && > + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && > + test_subcommand git maintenance run --auto --quiet --no-detach <trace > + ' > +done > + > +test_expect_success "maintenance.autoDetach overrides gc.autoDetach" ' > + test_when_finished "rm -f trace" && > + test_config maintenance.autoDetach false && > + test_config gc.autoDetach true && > + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && > + test_subcommand git maintenance run --auto --quiet --no-detach <trace > ' I appreciate the care taken in these test cases. It verifies the logic around the if statement that I had to read twice. Thanks, -Stolee
diff --git a/builtin/gc.c b/builtin/gc.c index 63106e2028..bafee330a2 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1063,6 +1063,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts, strvec_push(&child.args, "--quiet"); else strvec_push(&child.args, "--no-quiet"); + strvec_push(&child.args, "--no-detach"); return run_command(&child); } diff --git a/run-command.c b/run-command.c index 45ba544932..94f2f3079f 100644 --- a/run-command.c +++ b/run-command.c @@ -1808,16 +1808,26 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) int prepare_auto_maintenance(int quiet, struct child_process *maint) { - int enabled; + int enabled, auto_detach; if (!git_config_get_bool("maintenance.auto", &enabled) && !enabled) return 0; + /* + * When `maintenance.autoDetach` isn't set, then we fall back to + * honoring `gc.autoDetach`. This is somewhat weird, but required to + * retain behaviour from when we used to run git-gc(1) here. + */ + if (git_config_get_bool("maintenance.autodetach", &auto_detach) && + git_config_get_bool("gc.autodetach", &auto_detach)) + auto_detach = 1; + maint->git_cmd = 1; maint->close_object_store = 1; strvec_pushl(&maint->args, "maintenance", "run", "--auto", NULL); strvec_push(&maint->args, quiet ? "--quiet" : "--no-quiet"); + strvec_push(&maint->args, auto_detach ? "--detach" : "--no-detach"); return 1; } diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 2da7291e37..8415884754 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -229,7 +229,7 @@ test_expect_success 'fetch --refetch triggers repacking' ' GIT_TRACE2_EVENT="$PWD/trace1.event" \ git -C pc1 fetch --refetch origin && - test_subcommand git maintenance run --auto --no-quiet <trace1.event && + test_subcommand git maintenance run --auto --no-quiet --detach <trace1.event && grep \"param\":\"gc.autopacklimit\",\"value\":\"1\" trace1.event && grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"-1\" trace1.event && @@ -238,7 +238,7 @@ test_expect_success 'fetch --refetch triggers repacking' ' -c gc.autoPackLimit=0 \ -c maintenance.incremental-repack.auto=1234 \ -C pc1 fetch --refetch origin && - test_subcommand git maintenance run --auto --no-quiet <trace2.event && + test_subcommand git maintenance run --auto --no-quiet --detach <trace2.event && grep \"param\":\"gc.autopacklimit\",\"value\":\"0\" trace2.event && grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"-1\" trace2.event && @@ -247,7 +247,7 @@ test_expect_success 'fetch --refetch triggers repacking' ' -c gc.autoPackLimit=1234 \ -c maintenance.incremental-repack.auto=0 \ -C pc1 fetch --refetch origin && - test_subcommand git maintenance run --auto --no-quiet <trace3.event && + test_subcommand git maintenance run --auto --no-quiet --detach <trace3.event && grep \"param\":\"gc.autopacklimit\",\"value\":\"1\" trace3.event && grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"0\" trace3.event ' diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 771525aa4b..06ab43cfb5 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -49,22 +49,47 @@ test_expect_success 'run [--auto|--quiet]' ' git maintenance run --auto 2>/dev/null && GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \ git maintenance run --no-quiet 2>/dev/null && - test_subcommand git gc --quiet <run-no-auto.txt && - test_subcommand ! git gc --auto --quiet <run-auto.txt && - test_subcommand git gc --no-quiet <run-no-quiet.txt + test_subcommand git gc --quiet --no-detach <run-no-auto.txt && + test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt && + test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt ' test_expect_success 'maintenance.auto config option' ' GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 && - test_subcommand git maintenance run --auto --quiet <default && + test_subcommand git maintenance run --auto --quiet --detach <default && GIT_TRACE2_EVENT="$(pwd)/true" \ git -c maintenance.auto=true \ commit --quiet --allow-empty -m 2 && - test_subcommand git maintenance run --auto --quiet <true && + test_subcommand git maintenance run --auto --quiet --detach <true && GIT_TRACE2_EVENT="$(pwd)/false" \ git -c maintenance.auto=false \ commit --quiet --allow-empty -m 3 && - test_subcommand ! git maintenance run --auto --quiet <false + test_subcommand ! git maintenance run --auto --quiet --detach <false +' + +for cfg in maintenance.autoDetach gc.autoDetach +do + test_expect_success "$cfg=true config option" ' + test_when_finished "rm -f trace" && + test_config $cfg true && + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && + test_subcommand git maintenance run --auto --quiet --detach <trace + ' + + test_expect_success "$cfg=false config option" ' + test_when_finished "rm -f trace" && + test_config $cfg false && + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && + test_subcommand git maintenance run --auto --quiet --no-detach <trace + ' +done + +test_expect_success "maintenance.autoDetach overrides gc.autoDetach" ' + test_when_finished "rm -f trace" && + test_config maintenance.autoDetach false && + test_config gc.autoDetach true && + GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 && + test_subcommand git maintenance run --auto --quiet --no-detach <trace ' test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' @@ -129,9 +154,9 @@ test_expect_success 'run --task=<task>' ' git maintenance run --task=commit-graph 2>/dev/null && GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \ git maintenance run --task=commit-graph --task=gc 2>/dev/null && - test_subcommand ! git gc --quiet <run-commit-graph.txt && - test_subcommand git gc --quiet <run-gc.txt && - test_subcommand git gc --quiet <run-both.txt && + test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt && + test_subcommand git gc --quiet --no-detach <run-gc.txt && + test_subcommand git gc --quiet --no-detach <run-both.txt && test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt && test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt && test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt
In the past, we used to execute `git gc --auto` as part of our automatic housekeeping routines. As git-gc(1) may require quite some time to perform the housekeeping, it knows to detach itself and run in the background so that the user can continue their work. Eventually, we refactored our automatic housekeeping to instead use the more flexible git-maintenance(1) command. The upside of this new infra is that the user can configure which maintenance tasks are performed, at least to a certain degree. So while it continues to run git-gc(1) by default, it can also be adapted to e.g. use git-multi-pack-index(1) for maintenance of the object database. The auto-detach of the new infra is somewhat broken though once the user configures non-standard tasks. The problem is essentially that we detach at the wrong level in the process hierarchy: git-maintenance(1) never detaches itself, but instead it continues to be git-gc(1) which does. When configured to only run the git-gc(1) maintenance task, then the result is basically the same as before. But when configured to run other tasks, then git-maintenance(1) will wait for these to run to completion. Even worse, it may be that git-gc(1) runs concurrently with other housekeeping tasks, stomping on each others feet. Fix this bug by asking git-gc(1) to not detach when it is being invoked via git-maintenance(1). Instead, the latter command now respects a new config "maintenance.autoDetach", the equivalent of "gc.autoDetach", and detaches itself into the background if not told otherwise. This should continue to behave the same for all users which use the git-gc(1) task, only. For others though, it means that we now properly perform all tasks in the background. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/gc.c | 1 + run-command.c | 12 ++++++++++- t/t5616-partial-clone.sh | 6 +++--- t/t7900-maintenance.sh | 43 +++++++++++++++++++++++++++++++--------- 4 files changed, 49 insertions(+), 13 deletions(-)