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

[Phillip Wood <phillip.wood123@gmail.com>]

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