From: Phillip Wood Date: Tue, 13 Aug 2024 11:29:47 GMT Subject: Re: [PATCH 7/7] builtin/maintenance: fix auto-detach with non-standard tasks Message-ID: <779795d2-eefd-4fac-b29f-9943f98bc83b@gmail.com> In-Reply-To: <8d6cbae951177718b49d5cfbbeca2d5b0073e266.1723533091.git.ps@pks.im> 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 > --- > 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 + test_subcommand git maintenance run --auto --no-quiet --detach 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 + test_subcommand git maintenance run --auto --no-quiet --detach 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 + test_subcommand git maintenance run --auto --no-quiet --detach 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 - test_subcommand ! git gc --auto --quiet - test_subcommand git gc --no-quiet + test_subcommand git gc --quiet --no-detach + test_subcommand ! git gc --auto --quiet --no-detach + test_subcommand git gc --no-quiet --no-detach ' > > 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 + test_subcommand git maintenance run --auto --quiet --detach GIT_TRACE2_EVENT="$(pwd)/true" \ > git -c maintenance.auto=true \ > commit --quiet --allow-empty -m 2 && > - test_subcommand git maintenance run --auto --quiet + test_subcommand git maintenance run --auto --quiet --detach GIT_TRACE2_EVENT="$(pwd)/false" \ > git -c maintenance.auto=false \ > commit --quiet --allow-empty -m 3 && > - test_subcommand ! git maintenance run --auto --quiet + test_subcommand ! git maintenance run --auto --quiet --detach +' > + > +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 + ' > + > + 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 + ' > +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 ' > > test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' > @@ -129,9 +154,9 @@ test_expect_success 'run --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 - test_subcommand git gc --quiet - test_subcommand git gc --quiet + test_subcommand ! git gc --quiet --no-detach + test_subcommand git gc --quiet --no-detach + test_subcommand git gc --quiet --no-detach test_subcommand git commit-graph write --split --reachable --no-progress test_subcommand ! git commit-graph write --split --reachable --no-progress test_subcommand git commit-graph write --split --reachable --no-progress