Re: [PATCH 1/3] refs/reftable: introduce "reftable.lockTimeout"

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 18, 2024, 04:31 UTC
Message-ID: <ZupXlaLP3QcP8SPb@pks.im>
In-Reply-To: <CAPig+cQ_+C65V6Rj9uCBKJy-rdymRd93-0S_w-bfSSJj82Zsrw@mail.gmail.com>

On Tue, Sep 17, 2024 at 01:50:27PM -0400, Eric Sunshine wrote:

Show 22 quoted lines

> On Tue, Sep 17, 2024 at 1:46 PM karthik nayak <karthik.188@gmail.com> wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > > +test_expect_success 'ref transaction: retry acquiring tables.list lock' '
> > > +     test_when_finished "rm -rf repo" &&
> > > +     git init repo &&
> > > +     (
> > > +             cd repo &&
> > > +             test_commit initial &&
> > > +             LOCK=.git/reftable/tables.list.lock &&
> > > +             >$LOCK &&
> > > +             {
> > > +                     ( sleep 1 && rm -f $LOCK ) &
> > > +             } &&
> > > +             git -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD
> > > +     )
> > > +'
> >
> > Nit: This does stall the test for 1s. Which is slightly annoying when
> > running single tests locally. Couldn't we achieve this by doing `sleep
> > 0.1`?
> 
> `sleep 0.1` is neither POSIX nor portable.

The above test also verifies that the timeout can in fact be overridden. If we only had `sleep 0.1` we wouldn't be able to reliably verify that, as the default is a timeout of 100ms.

We also have essentially the same tests in t0601 for packed-refs, I mostly just copied the logic over from there. It's not exactly pretty, and I'm not a huge fan of introducing timing sensitive tests, but in the end I guess it's okayish.

Patrick