Re: [PATCH] rerere: keep a background gc from killing a rebase

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Patrick and Thomas

On 03/09/2026 09:32, Patrick Steinhardt wrote:
> On Thu, Sep 03, 2026 at 10:11:05AM +0200, Thomas Bachem wrote:
>> Hi Patrick,
>>
>> On Thu, Sep 03, 2026 at 09:40:04AM +0200, Patrick Steinhardt wrote:
>>> I think this hints that we should tweak the default value of
>>> "maintenance.rerere-gc.auto". The way it's currently written we indeed
>>> are quite aggressive with spawning `git rerere gc`, and I agree that we
>>> should tweak it. And in the best case we'd not only respect whether we
>>> have a specific number of entries, but we should also respect whether
>>> those would be garbage collected in the first place.
>>>
>>> I'll send a patch series later today to do this.
>>
>> Thanks. Checking whether anything would actually be pruned sounds
>> right to me. It takes the frequency away, not the race, so I'd still
>> do the sequencer part Phillip asked for.
> 
> Yes. Ideally, I'd think that we should both introduce the grace period
> for locking the file and adapting the heuristic used by the maintenance
> strategy. 

I agree

> Whether we should completely disable auto-maintenance when in
> the sequencer... I dunno. In any case, that feels like another separate
> topic that should probably be discussed in its own series.

We've seen other bugs reported related to auto-maintenance triggered 
during a rebase such as the one dscho fixed recently. While I can see 
repacking might be helpful during a very large rebase, I do not think 
garbage collection is useful - all the objects and rerere entries that 
are created during the rebase are going to be too fresh to be collected. 
So I think it would be a good idea to disable auto maintenance in a 
rebase and see if anyone complains. If it turns out to be a problem we 
can figure out how to make it repack incrementally.

>>> Having a locking timeout is sensible anyway, I think. It does not only
>>> solve races with a concurrent maintenance run, but also with concurrent
>>> writers.
>>
>> Phillip found the wait unfortunate and I offered to drop it. You would
>> keep it. I think the two fit together: wait up to rerere.lockTimeout,
>> then warn and return -1 instead of dying, so the caller goes on
>> without rerere this once. The gc passes 0 and does not wait. That
>> takes the die out, which is what broke the rebase. The wait stays,
>> bounded to a second, but skipping rerere is not free either: it can
>> mean resolving a conflict again that rerere had already recorded, and
>> a second is cheap next to that. With the sequencer no longer spawning
>> the gc and your heuristic change, it should rarely come to either.
>> Phillip, would that work for you?
> 
> I think that having the wait is a sensible thing to do, as the race was
> a preexisting one that was only uncovered by the change to the default
> maintenance strategy. It can also happen with two concurrent processes
> that both happen to write rerere entries. You wouldn't normally see the
> wait anyway, so in the happy path nobody will really care. And in the
> cases where you would see it the user is probably more happy to wait a
> bit than having Git die (or just not write a rerere entry at all).

I don't object to the timeout as part of the solution. My objection was 
based on it being the only solution as it is inconvenient to the user if 
they have to wait for background maintenance jobs and it does not stop 
the rebase from failing if the timeout is too short.

Thanks

Phillip