Re: [PATCH] credential-store: use timeout when locking file

[Junio C Hamano <gitster@pobox.com>]

Simão Afonso <simao.afonso@powertools-tech.com> writes:

> When holding the lock for rewriting the credential file, use a timeout
> to avoid race conditions when the credentials file needs to be updated
> in parallel.
>
> An example would be doing `fetch --all` on a repository with several
> remotes that need credentials, using parallel fetching.

OK.

> The timeout is hardcoded to 1 second, since this is just to solve a race
> condition.

It is unclear what this sentence wants to explain.  How does "this
is to solve a race" justifies the choice of "1 second" (as opposed
to say 10 seconds or 0.5 second)?  Or is this trying to justify why
there is no configurability?  If so, why is it OK to hardcode it if
it is done to solve a race?  Are we assuming certain maximum rate
of operation that is "reasonable"?

> This was reported here:
> https://lore.kernel.org/git/20201029192020.mcri76ylbdure2o7@safonso-t430/
> ---

Missing sign-off.

cf. https://git-scm.com/docs/SubmittingPatches.html#sign-off

>  builtin/credential-store.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/credential-store.c b/builtin/credential-store.c
> index 5331ab151..acff4abae 100644
> --- a/builtin/credential-store.c
> +++ b/builtin/credential-store.c
> @@ -58,8 +58,9 @@ static void print_line(struct strbuf *buf)
>  static void rewrite_credential_file(const char *fn, struct credential *c,
>  				    struct strbuf *extra)
>  {
> -	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
> -		die_errno("unable to get credential storage lock");
> +	static long timeout = 1000;

Why "static"?  It would make your code easier to follow if you limit
use of "static" to only cases where you want to take advantage of
the fact that the value previously left by the earlier call is seen
by the next call, and/or the address of the variable must be valid
even after the control returns to the caller.

I would understand if this were "const long timeout = 1000".

If this were an identifier with longer lifespan, I would have
suggested to include some scale in the variable name (e.g.
timeout_ms to clarify that it is counted in milliseconds), but it is
just for this short function, so let's say "timeout" is just fine.

> +	if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout) < 0)
> +		die_errno("unable to get credential storage lock in %ld ms", timeout);
>  	if (extra)
>  		print_line(extra);
>  	parse_credential_file(fn, c, NULL, print_line);

Thanks.