Re: [PATCH 7/7] builtin/maintenance: fix auto-detach with non-standard tasks
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Aug 15, 2024, 14:00 UTC
- 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:
Show 8 quoted lines
> @@ -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.
Show 9 quoted lines
> + /*
> + * 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.
Show 18 quoted lines
> 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.
Show 24 quoted lines
> +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