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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Michael Haggerty <mhagger@xxxxxxxxxxxx> 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 <mhagger@xxxxxxxxxxxx>
> ---
>  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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]