Re: [PATCH v2 10/17] lock_ref_sha1_basic(): on SCLD_VANISHED, retry
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 6, 2014, 17:54 UTC
- Message-ID
- <xmqq4n5hj8ry.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1389015935-21936-11-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 42 quoted lines
> If safe_create_leading_directories() fails because a file along the
> path unexpectedly vanished, try again (up to 3 times).
>
> This can occur if another process is deleting directories at the same
> time as we are trying to make them. For example, "git pack-refs
> --all" tries to delete the loose refs and any empty directories that
> are left behind. If a pack-refs process is running, then it might
> delete a directory that we need to put a new loose reference in.
>
> If safe_create_leading_directories() thinks this might have happened,
> then take its advice and try again (maximum three attempts).
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 3926136..6eb8a02 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2039,6 +2039,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
> int type, lflags;
> int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
> int missing = 0;
> + int attempts = 3;
>
> lock = xcalloc(1, sizeof(struct ref_lock));
> lock->lock_fd = -1;
> @@ -2093,7 +2094,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
> if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
> lock->force_write = 1;
>
> - if (safe_create_leading_directories(ref_file)) {
> + retry:
> + switch (safe_create_leading_directories(ref_file)) {
> + case SCLD_OK:
> + break; /* success */
> + case SCLD_VANISHED:
> + if (--attempts > 0)
> + goto retry;
> + /* fall through */Hmph.
Having no backoff/sleep at all might be OK here as long as the other side that removes does not retry (and I do not think the other side would, even though I haven't read through the series to the end yet ;-)).
This may be just a style thing, but I find that the variable name "attempts" that starts out as 3 quite misleading, as its value is not "the number of attempts made" but "the remaining number of attempts allowed." Starting it from 0 and then
if (attempts++ < MAX_ATTEMPTS) goto retry;
would be one way to clarify it. Renaming it to remaining_attempts would be another.
> + default:
> last_errno = errno;
> error("unable to create directory for %s", ref_file);
> goto error_return;