Re: [PATCH v6 2/3] rerere: add "gc --skip-locked" for auto maintenance

From: Thomas Bachem <mail@thomasbachem.com>
Date: Oct 10, 2026, 09:09 UTC
Message-ID: <CAA0xjtrna99gE6U14JZbYMXSb8rjE5eay5ug8+v6-j1vBS4f3g@mail.gmail.com>
In-Reply-To: <asiugInq7YTj4Qbe@pks.im>

Hi Patrick,

On 09/10/2026 11:06, Patrick Steinhardt wrote:

> And this reads quite awkward, too. How about:

I'll take your message as it is, thanks. I'd only add a last paragraph on why the flag is hidden:

  Only auto-maintenance needs that flag, so hide it, like the
  "--skip-foreground-tasks" flag that `git maintenance run` passes to
  `git gc`.

> This comment is basically a layering violation, as you now assume who
> passes `RERERE_NOWAIT`. It's a generic mechanism though, so I'd just
> drop that part.

Right, I'll drop it.

> It's a tiny bit fishy that we return an error in the case where we have
> been asked to skip locking and we indeed weren't able to acquire the
> lock. To me it doesn't really indicate an error, as it matches the
> intent of the caller. But I guess that's debatable.

setup_rerere() already returns -1 when rerere is disabled, and every caller takes that as nothing to do rather than as an error. So I'd keep the -1 and say so in the comment on RERERE_NOWAIT.

> "free" is a bit unusual for a term for a lock.

That's the comment I'd change anyway, so it would read:

  /* If MERGE_RR.lock is taken, return -1 as if rerere were disabled */

> Given that these flags are new now, and given that none of the other
> flags apply to `rerere_gc`, shouldn't we instead have a separate list of
> flags specific to this function?

Yes, I'll add enum rerere_gc_flags as you wrote it, and have rerere_gc() pass RERERE_NOWAIT to setup_rerere() for it.

> You verify that --skip-locked skips when locked, but you don't verify
> that it doesn't skip when unlocked.

The second half of that test does: it removes the lock, runs "git rerere gc --skip-locked" again and checks that the preimage is gone. That's easy to miss, so I'll make it a test of its own.

Thanks, Thomas