On 01/06/2014 06:54 PM, Junio C Hamano wrote:
> 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
> ;-)).
remove_dir_recurse() only tries deleting directories once (I haven't
changed that). And from a broader perspective, it would be pretty silly
for any tidy-up-directories function to try deleting things more than
once. So I don't think it is a problem. But even in the worst case,
this function only tries three times before giving up, so it shouldn't
be a disaster.
> 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.
I just renamed the variable to attempts_remaining. (I thought I was
following your suggestion, but now I see that I put the words in the
opposite order; oh well, I think it's fine either way.)
Thanks for your review! I will wait a day or so for any additional
comments, and then send a v3.
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
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