From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 15 Aug 2024 14:00:13 GMT
Subject: Re: [PATCH 7/7] builtin/maintenance: fix auto-detach with non-standard tasks
Message-ID: <eddcc0a0-a366-4627-8878-f1b5d0af2436@gmail.com>
In-Reply-To: <8d6cbae951177718b49d5cfbbeca2d5b0073e266.1723533091.git.ps@pks.im>

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