Re: [PATCH v2 7/7] run-command: fix detaching when running auto maintenance
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Aug 16, 2024, 08:06 UTC
- Message-ID
- <Zr8InHskJNixuSWW@tanuki>
- In-Reply-To
- <xmqq4j7lvk5z.fsf@gitster.g>
On Thu, Aug 15, 2024 at 09:13:28AM -0700, Junio C Hamano wrote:
Show 28 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
>
> > 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;
>
> I think this needs somehow documented. Something like this,
> perhaps?Indeed, I totally forgot doing that.
Show 14 quoted lines
> --- c/Documentation/config/maintenance.txt > +++ w/Documentation/config/maintenance.txt > @@ -3,6 +3,15 @@ maintenance.auto:: > `git maintenance run --auto` after doing their normal work. Defaults > to true. > > +maintenance.autoDetach:: > + Tasks that are run via `git maintenance run --auto` by > + default runs in the background, if the system supports it. > + Setting this configuration variable to `true` explicitly > + asks them to run in the background, and setting it to > + `false` forces them to run in the foreground. If this > + variable is not set, `gc.autoDetach` works as a fallback > + variable and behaves the same way.
This isn't entirely true. `git maintenance run --auto` will not background, because that'd change preexisting behaviour. It also would not make a lot of sense, because here the `--auto` trigger tells the command to do maintenance as-needed. Coupling that with whether or not to detach was a misdesign of git-gc(1), I think. What it does control is whether we detach or not when automatically executing maintenance via `prepare_auto_maintenance()`.
Anyway, my fault for not documenting it, not yours for getting it slightly wrong.
Patrick