Re: [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task

> Le 30 mai 2025 à 11:31, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> On Fri, May 30, 2025 at 11:10:27AM -0400, Ben Knoble wrote:
>> 
>>>> Le 30 mai 2025 à 10:05, Patrick Steinhardt <ps@pks.im> a écrit :
>>> 
>>> On Fri, May 30, 2025 at 08:55:49AM -0400, Ben Knoble wrote:
>>>>> @@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts,
>>>>> else
>>>>>     strvec_push(&child.args, "--no-quiet");
>>>>> strvec_push(&child.args, "--no-detach");
>>>>> +    strvec_push(&child.args, "--skip-maintenance-before-detach");
>>>> 
>>>> I suspect this would be more obvious to me if I had the manual
>>>> available right now, but if we are not detaching (« --no-detach ») why
>>>> do we need to skip something before detaching (that presumably won’t
>>>> happen)?
>>> 
>>> We have two levels here: git-maintenance(1) and git-gc(1), where the
>>> former executes the latter when the "gc" task is configured. What is
>>> important to realize is that in this setup it is not git-gc(1) which
>>> detaches -- it is git-maintenance(1). So git-maintenance(1) runs in the
>>> background, but any tasks it invokes itself must run synchronously in
>>> the foreground.
>>> 
>>> The flow thus looks like this:
>>> 
>>> 1. git-maintenance(1) starts.
>>> 2. We perform the pre-detach tasks from git-gc(1) in the same process.
>>> 3. We detach and thus the main process exits.
>>> 4. We execute git-gc(1) in the already-detached process.
>>> 5. We wait for git-gc(1) to exit.
>>> 6. The detached git-maintenance(1) exits.
>>> 
>>> So because (4) is running in the already-detached process we ask
>>> git-gc(1) to not detach again. And because we already ran the pre-detach
>>> tasks we also ask it to not run those again.
>>> 
>>> Patrick
>> 
>> Aha, thanks! I thought I understood the sequence, but I was wrong
>> about some details.
>> 
>> I was wondering if not detaching should just imply skipping work
>> before a (non-existent) detach—if there’s no detach, should we do any
>> pre-detach work at all? But presumably that does the wrong thing for
>> (non-detaching) invocations that come from outside git-maintenance,
>> doesn’t it? Hm.
> 
> Yeah, we always want to do these tasks no matter whether we detach or
> not.
> 
>> Maybe the flip-around for me is that « pre-detach work » here actually
>> refers to « foreground work », which we obviously want to do even if
>> we aren’t detaching, and which maintenance (which has already done
>> this) needs to skip.
> 
> Hm. That's actually a better way to put it, agreed. Too bad I already
> sent out the new version a couple minutes ago :) I'll have a look on
> Monday and rephrase this part.