[PATCH 12/13] initial_ref_transaction_commit(): check for ref D/F conflicts

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jun 8, 2015, 11:45 UTC
Message-ID: <bfce44d80a716a076e743ab406265dad9a8a8e88.1433763494.git.mhagger@alum.mit.edu>
In-Reply-To: <cover.1433763494.git.mhagger@alum.mit.edu>

In initial_ref_transaction_commit(), check for D/F conflicts (i.e., the type of conflict that exists between "refs/foo" and "refs/foo/bar") among the references being created and between the references being created and any hypothetical existing references.

Ideally, there shouldn't *be* any existing references when this function is called. But, at least in the case of the "testgit" remote helper, "clone" can be called after the remote-tracking "HEAD" and "master" branches have already been created. So let's just do the full-blown check.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

Notes (discussion):
    I'm a bit torn on this commit. Part of me says that we should avoid
    the overhead of the extra checks against loose_refs and packed_refs
    because there shouldn't be any references there anyway. Instead, the
    "testgit" remote helper should be fixed.
    
    But these tests should be pretty cheap, and we would want to check for
    ref D/F conflicts among the new references in any case, so it seems
    safer to include these checks. Also, our documentation suggests that
    users refer to git-remote-testgit(1) as an example when writing their
    own remote helpers, so it could be that there is other code out there
    that has imitated its light misuse of this functionality.

 refs.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/refs.c b/refs.c
index ced4e53..2c4d9ab 100644
--- a/refs.c
+++ b/refs.c
@@ -4058,9 +4058,19 @@ cleanup:
 	return ret;
 }
 
+static int ref_present(const char *refname,
+		       const struct object_id *oid, int flags, void *cb_data)
+{
+	struct string_list *affected_refnames = cb_data;
+
+	return string_list_has_string(affected_refnames, refname);
+}
+
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
 				   struct strbuf *err)
 {
+	struct ref_dir *loose_refs = get_loose_refs(&ref_cache);
+	struct ref_dir *packed_refs = get_packed_refs(&ref_cache);
 	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
@@ -4080,12 +4090,36 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 		goto cleanup;
 	}
 
+	/*
+	 * It's really undefined to call this function in an active
+	 * repository or when there are existing references: we are
+	 * only locking and changing packed-refs, so (1) any
+	 * simultaneous processes might try to change a reference at
+	 * the same time we do, and (2) any existing loose versions of
+	 * the references that we are setting would have precedence
+	 * over our values. But some remote helpers create the remote
+	 * "HEAD" and "master" branches before calling this function,
+	 * so here we really only check that none of the references
+	 * that we are creating already exists.
+	 */
+	if (for_each_rawref(ref_present, &affected_refnames))
+		die("BUG: initial ref transaction called with existing refs");
+
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
 		if ((update->flags & REF_HAVE_OLD) &&
 		    !is_null_sha1(update->old_sha1))
 			die("BUG: initial ref transaction with old_sha1 set");
+		if (verify_refname_available(update->refname,
+					     &affected_refnames, NULL,
+					     loose_refs, err) ||
+		    verify_refname_available(update->refname,
+					     &affected_refnames, NULL,
+					     packed_refs, err)) {
+			ret = TRANSACTION_NAME_CONFLICT;
+			goto cleanup;
+		}
 	}
 
 	if (lock_packed_refs(0)) {

-- 
2.1.4