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

Derrick Stolee
2024-08-15T14:00:15Z

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