From: Thomas Bachem Date: Sun, 11 Oct 2026 14:18:09 GMT Subject: Re: [PATCH v7 1/2] rerere: wait for MERGE_RR.lock before giving up Message-ID: In-Reply-To: Hi Junio, Thanks for the careful review. On 11/10/2026 02:39, Junio C Hamano wrote: >> + When the time is up, the command fails as it does >> + for any other lock it cannot take. > > Is it necessary to say this? No, I'll drop it. > The backgrounded unlocker sleeps for a second. Is the idea that, > even in a heavily loaded CI environment, the backgrounded unlocker > will have a sufficient chance to sleep for a second and unlock while > the 5000-millisecond timeout waits for it? Yes, the unlocker can be up to four seconds late. t0610 and t1300 use the same numbers. > Is the idea behind this 'wait' that the 'rerere.locktimeout' > implementation might break in the future and we could reach this > point before the background unlocker has finished sleeping for a > full second? And we want to ensure it has exited before proceeding > by waiting for it ourselves. If that is the case, perhaps a comment > is warranted after '&&', such as: > > wait && # just in case the background unlocker is still active It is. Instead of the comment, I'll move the wait into test_when_finished, as pack-refs-tests.sh does, so it runs however the test ends. > Or make the string a bit more specific, such as: > > test_grep ! "Unable to create.*MERGE_RR\.lock" err && I'll use your pattern there and in the other tests. > Should we ensure that there is no leftover pid file by removing it > before creating .git/MERGE_RR.lock, by the way? I don't think we need to. Git only writes MERGE_RR~pid.lock with core.lockfilePid set, and t4200 never sets it. Even a stale one would only change the text after the "Unable to create" line, not whether git gets the lock. > I'll stop here, but you can grasp the principles used in reviewing > this test and apply them to the remaining tests to ensure they are > clearly written. Will do. Thanks, Thomas