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

On 06/07/2016 08:10 PM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

From: David Turner <redacted>

Signed-off-by: David Turner <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Michael Haggerty <redacted>
---
 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