From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 13 Aug 2024 13:19:20 GMT
Subject: Re: [PATCH 7/7] builtin/maintenance: fix auto-detach with non-standard tasks
Message-ID: <a02e21f1-b2a4-499a-b767-3426876d31be@gmail.com>
In-Reply-To: <ZrtKoVFci6cdvMS_@tanuki>

On 13/08/2024 12:59, Patrick Steinhardt wrote:
> On Tue, Aug 13, 2024 at 12:29:47PM +0100, Phillip Wood wrote:
>> 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.
> 
> It's actually the reverse: the old behaviour when run via a scheduler
> was to detach by default, because git-gc(1) did.

Oh, I  misunderstood what this patch is changing. So despite being 
tagged builtin/maintenance and talking about "git maintenance" it does 
not actually touch builtin/maintenance.c or change its behavior. What it 
is actually doing is changing how other git commands run "git 
maintenance --auto" so that it is always run in the background unless 
the user configures maintenance.autoDetach=false. That sounds like a 
good change.

Thanks for clarifying

Phillip

> We now ask it to not
> detach anymore, which fixes this. Furthermore, the default behaviour of
> `git maintenance run` did not change either: it stays in the foreground
> unless the `--detach` flag is given. So the thing you worry about is
> actually getting fixed by this series :)
> 
> What _does_ change though is when we run `git maintenance` via our
> auto-maintenance framework. Here we now do detach the whole maintenance
> process, instead of only git-gc(1). This logic is only being executed by
> random commands (git-rebase, git-pull, git-commit etc), and I'd argue it
> is the expected behaviour.
> 
> Patrick
>