Re: [PATCH 02/38] rename_ref_available(): add docstring

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/07/2016 08:10 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> From: David Turner <dturner@twopensource.com>
>>
>> Signed-off-by: David Turner <dturner@twopensource.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  refs/refs-internal.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index efe5847..d8a2606 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -240,6 +240,11 @@ const char *find_descendant_ref(const char *dirname,
>>  				const struct string_list *extras,
>>  				const struct string_list *skip);
>>  
>> +/*
>> + * Check if the new name does not conflict with any existing refs
>> + * (other than possibly the old ref).  Return 0 if the ref can be
>> + * renamed to the new name.
>> + */
>>  int rename_ref_available(const char *oldname, const char *newname);
> 
> I do not quite understand the comment.  Partly because it is unclear
> what "conflict" means here, but I guess it means a D/F conflict that
> is explained near verify_refname_available()?

Yes.

> A new name can conflict with an existing, possibly old ref?  Are you
> referring to this condition?
> 
>     You are trying to rename "refs/a/b" to "refs/a", which would
>     conflict, but as long as there is no other ref that share the
>     prefix "refs/a/", e.g. "refs/a/c", the new name "refs/a" is
>     available.

That is correct.

> I wonder if it is necessary to document that this function is not
> meant to protect us from others racing with us.  That is, when you
> are renaming something to "refs/a", you call this function and it
> finds, by calling verify_refname_available(), that the repository
> has nothing that conflicts with the name and says "OK", but before
> you actually do the rename, somebody may push from sideways to
> create "refs/a/b", making the result of an earlier check with this
> function invalid.
> 
> Or is this to be called only under a lock that protects us from such
> a race?

It would be really awkward (maybe impossible?) to guard against all such
races even using locks. One problem is that the lockfiles for the old
and new refnames would themselves, in some cases, not be able to coexist
due to D/F conflicts. Also, there is no way to prevent the creation of
"any reference in `refs/a/*`" except by creating the reference `refs/a`
(the presence of `refs/a.lock` is not enough), but by that time it is
too late.

In the end, this function mostly exists as a pre-check that
`rename_ref()` is *likely* to succeed, so that the latter function is
less likely to detect a problem after it has started moving things
around. But `rename_ref()` is the final arbiter and is a bit more robust
than this check.

I also noticed that the docstring in this patch got the polarity of the
return value backwards.

I propose to change the parameter names to `old_refname` and
`new_refname` and to change the docstring to

/*
 * Check whether an attempt to rename old_refname to new_refname would
 * cause a D/F conflict with any existing reference (other than
 * possibly old_refname). If there would be a conflict, emit an error
 * message and return false; otherwise, return true.
 *
 * Note that this function is not safe against all races with other
 * processes (though rename_ref() catches some races that might get by
 * this check).
 */

Does that sound good?

Michael