Re: Credential Store: Don't acquire lock when only reading the file
- From
Jeff King <peff@peff.net>
- Date
- Oct 30, 2020, 15:01 UTC
- Message-ID
- <20201030150136.GA3282152@coredump.intra.peff.net>
- In-Reply-To
- <20201030120540.dna7k3bmnrrw4cck@safonso-t430>
On Fri, Oct 30, 2020 at 12:05:40PM +0000, Simão Afonso wrote:
Show 13 quoted lines
> On 2020-10-30 01:55:41, Jeff King wrote: > > - teach it to try the lock until hitting a timeout. I think just > > swapping out hold_lock_file_for_update() for > > hold_lock_file_for_update_timeout() would do it (I probably > > would have used it back then, but it didn't exist yet). > > So I tried to patch the credential store with that timeout function and > it seems to solve it! Thanks. > > https://github.com/git/git/blob/v2.29.2/builtin/credential-store.c#L61 > > This is easily reproduced if you do a `fetch --all` in parallel on the > same repository (seen on the attached image):
Great!
Show 9 quoted lines
> > $ for n in $(seq 100); do git fetch --all; sleep 0.5; done > > About the timeout, I put it at infinite retries, but should this be > configurable? Or should it be a different default? > > There aren't many timeout configurations. I found > "core.filesRefLockTimeout" and "core.packedRefsTimeout", doesn't sound > like something that should be re-used. > Should it be something like this with a different name?
Yeah, I don't think it makes sense to reuse those. I kind of doubt that it is worth making it configurable, as credential-store does not otherwise read any config at all.
The defaults for those other two values are 100ms and 1s respectively. So probably just setting a 1s timeout would be plenty, and saves us from looping forever if we see a stale lockfile.
-Peff