Subject: Re: [PATCH v2 10/17] lock_ref_sha1_basic(): on SCLD_VANISHED, retry

From: Junio C Hamano <[email-masked]>
Date: Mon, 06 Jan 2014 09:54:25 -0800

Michael Haggerty <[email-masked]> writes:

> 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 <[email-masked]>
> ---
>  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;