On Tue, Nov 24, 2020 at 02:08:01PM -0800, Junio C Hamano wrote:
> Simão Afonso <simao.afonso@xxxxxxxxxxxxxxxxxxx> writes:
>
> > +credentialStore.fileTimeout::
> > + The length of time, in milliseconds, for git-credential-store to retry
> > + when trying to lock the credentials file. Value 0 means not to retry at
> > + all; -1 means to try indefinitely. Default is 1000 (i.e., retry for
> > + 1s).
>
> I do not remember what was said in the first round of the review,
> but I wonder if this is the best name for users. I think it is good
> enough, but do ".lockTimeout" or ".lockTimeoutMS" make it even
> easier to grok, perhaps?
Yeah, I think those are a bit more obvious.
> > +
> > + if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
> > + die_errno("unable to get credential storage lock in %d ms", timeout_ms);
>
> Should this be die_errno()? Looking at lock_file_timeout(), I am
> not sure if the value of errno is valid in all codepaths that return
> failure.
I think it's the right thing here. Inside hold_lock_file_for_update_timeout(),
we'd pass errno to unable_to_lock_die(), etc. So if there is a code path
in lock_file_timeout() that isn't setting errno properly, we should
probably be fixing that.
Another option would be to just pass LOCK_DIE_ON_ERROR here, but I think
for this use I prefer the smaller "unable to lock" to the big "another
git process may have crashed" advice message we'd give in that case.
-Peff
