From: Thomas Bachem Date: Sat, 10 Oct 2026 09:09:25 GMT Subject: Re: [PATCH v6 2/3] rerere: add "gc --skip-locked" for auto maintenance Message-ID: In-Reply-To: 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