From: Junio C Hamano <firstname.lastname@example.org>
To: "Simão Afonso" <email@example.com>
Cc: firstname.lastname@example.org, Jeff King <email@example.com>
Subject: Re: [PATCH] credential-store: use timeout when locking file
Date: Fri, 30 Oct 2020 12:49:56 -0700 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
In-Reply-To: <20201030180718.4i7txqkgye7r6pkb@safonso-t430> (=?utf-8?Q?=22?= =?utf-8?Q?Sim=C3=A3o?= Afonso"'s message of "Fri, 30 Oct 2020 18:07:18 +0000")
Simão Afonso <email@example.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.
> The timeout is hardcoded to 1 second, since this is just to solve a race
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:
> 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)
> parse_credential_file(fn, c, NULL, print_line);
next prev parent reply other threads:[~2020-10-30 19:50 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-30 18:07 Simão Afonso
2020-10-30 19:49 ` Junio C Hamano [this message]
2020-11-24 19:32 ` [PATCH v2] crendential-store: " Simão Afonso
2020-11-24 22:08 ` Junio C Hamano
2020-11-25 9:37 ` Jeff King
2020-11-25 18:31 ` [PATCH v3] " Simão Afonso
2020-11-26 7:37 ` Jeff King
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this inbox:
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).