Re: [PATCH 7/7] builtin/maintenance: fix auto-detach with non-standard tasks

Phillip Wood
2024-08-13T11:29:52Z

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