Merge pull request #5106 from tiennou/fix/ref-api-fixes

git_refdb API fixes

3 files changed, 158 insertions, 66 deletions

diff --git a/include/git2/sys/refdb_backend.h b/include/git2/sys/refdb_backend.h
index 2ed6efd5a..8e22c4f02 100644
--- a/include/git2/sys/refdb_backend.h
+++ b/include/git2/sys/refdb_backend.h
@@ -58,11 +58,12 @@ struct git_reference_iterator {
 
 /** An instance for a custom backend */
 struct git_refdb_backend {
-	unsigned int version;
+	unsigned int version; /**< The backend API version */
 
 	/**
-	 * Queries the refdb backend to determine if the given ref_name
-	 * exists.  A refdb implementation must provide this function.
+	 * Queries the refdb backend for the existence of a reference.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(exists)(
 		int *exists,
@@ -70,8 +71,9 @@ struct git_refdb_backend {
 		const char *ref_name);
 
 	/**
-	 * Queries the refdb backend for a given reference.  A refdb
-	 * implementation must provide this function.
+	 * Queries the refdb backend for a given reference.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(lookup)(
 		git_reference **out,
@@ -88,82 +90,116 @@ struct git_refdb_backend {
 		struct git_refdb_backend *backend,
 		const char *glob);
 
-	/*
-	 * Writes the given reference to the refdb.  A refdb implementation
-	 * must provide this function.
+	/**
+	 * Writes the given reference to the refdb.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(write)(git_refdb_backend *backend,
 		     const git_reference *ref, int force,
 		     const git_signature *who, const char *message,
 		     const git_oid *old, const char *old_target);
 
+	/**
+	 * Rename a reference in the refdb.
+	 *
+	 * A refdb implementation must provide this function.
+	 */
 	int GIT_CALLBACK(rename)(
 		git_reference **out, git_refdb_backend *backend,
 		const char *old_name, const char *new_name, int force,
 		const git_signature *who, const char *message);
 
 	/**
-	 * Deletes the given reference (and if necessary its reflog)
-	 * from the refdb.  A refdb implementation must provide this
-	 * function.
+	 * Deletes the given reference from the refdb.
+	 *
+	 * If it exists, its reflog should be deleted as well.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(del)(git_refdb_backend *backend, const char *ref_name, const git_oid *old_id, const char *old_target);
 
 	/**
 	 * Suggests that the given refdb compress or optimize its references.
-	 * This mechanism is implementation specific.  (For on-disk reference
-	 * databases, this may pack all loose references.)    A refdb
-	 * implementation may provide this function; if it is not provided,
-	 * nothing will be done.
+	 *
+	 * This mechanism is implementation specific. For on-disk reference
+	 * databases, this may pack all loose references.
+	 *
+	 * A refdb implementation may provide this function; if it is not
+	 * provided, nothing will be done.
 	 */
 	int GIT_CALLBACK(compress)(git_refdb_backend *backend);
 
 	/**
 	 * Query whether a particular reference has a log (may be empty)
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(has_log)(git_refdb_backend *backend, const char *refname);
 
 	/**
 	 * Make sure a particular reference will have a reflog which
 	 * will be appended to on writes.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(ensure_log)(git_refdb_backend *backend, const char *refname);
 
 	/**
 	 * Frees any resources held by the refdb (including the `git_refdb_backend`
-	 * itself). A refdb backend implementation must provide this function.
+	 * itself).
+	 *
+	 * A refdb backend implementation must provide this function.
 	 */
 	void GIT_CALLBACK(free)(git_refdb_backend *backend);
 
 	/**
 	 * Read the reflog for the given reference name.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(reflog_read)(git_reflog **out, git_refdb_backend *backend, const char *name);
 
 	/**
 	 * Write a reflog to disk.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(reflog_write)(git_refdb_backend *backend, git_reflog *reflog);
 
 	/**
-	 * Rename a reflog
+	 * Rename a reflog.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(reflog_rename)(git_refdb_backend *_backend, const char *old_name, const char *new_name);
 
 	/**
 	 * Remove a reflog.
+	 *
+	 * A refdb implementation must provide this function.
 	 */
 	int GIT_CALLBACK(reflog_delete)(git_refdb_backend *backend, const char *name);
 
 	/**
-	 * Lock a reference. The opaque parameter will be passed to the unlock function
+	 * Lock a reference.
+	 *
+	 * The opaque parameter will be passed to the unlock function.
+	 *
+	 * A refdb implementation may provide this function; if it is not
+	 * provided, the transaction API will fail to work.
 	 */
 	int GIT_CALLBACK(lock)(void **payload_out, git_refdb_backend *backend, const char *refname);
 
 	/**
-	 * Unlock a reference. Only one of target or symbolic_target
-	 * will be set. success indicates whether to update the
-	 * reference or discard the lock (if it's false)
+	 * Unlock a reference.
+	 *
+	 * Only one of target or symbolic_target will be set.
+	 * `success` will be true if the reference should be update, false if
+	 * the lock must be discarded.
+	 *
+	 * A refdb implementation must provide this function if a `lock`
+	 * implementation is provided.
 	 */
 	int GIT_CALLBACK(unlock)(git_refdb_backend *backend, void *payload, int success, int update_reflog,
 		      const git_reference *ref, const git_signature *sig, const char *message);
diff --git a/src/refdb.c b/src/refdb.c
index b466153a7..fbbf5193c 100644
--- a/src/refdb.c
+++ b/src/refdb.c
@@ -66,6 +66,18 @@ static void refdb_free_backend(git_refdb *db)
 
 int git_refdb_set_backend(git_refdb *db, git_refdb_backend *backend)
 {
+	GIT_ERROR_CHECK_VERSION(backend, GIT_REFDB_BACKEND_VERSION, "git_refdb_backend");
+
+	if (!backend->exists || !backend->lookup || !backend->iterator ||
+	    !backend->write || !backend->rename || !backend->del ||
+	    !backend->has_log || !backend->ensure_log || !backend->free ||
+	    !backend->reflog_read || !backend->reflog_write ||
+	    !backend->reflog_rename || !backend->reflog_delete ||
+	    (backend->lock && !backend->unlock)) {
+		git_error_set(GIT_ERROR_REFERENCE, "incomplete refdb backend implementation");
+		return GIT_EINVALID;
+	}
+
 	refdb_free_backend(db);
 	db->backend = backend;
 
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index a20f1ebcc..9fb66a8e7 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -858,16 +858,17 @@ static int refdb_fs_backend__write_tail(
 	const git_reference *ref,
 	git_filebuf *file,
 	int update_reflog,
-	const git_signature *who,
-	const char *message,
 	const git_oid *old_id,
-	const char *old_target);
+	const char *old_target,
+	const git_signature *who,
+	const char *message);
 
 static int refdb_fs_backend__delete_tail(
 	git_refdb_backend *_backend,
 	git_filebuf *file,
 	const char *ref_name,
-	const git_oid *old_id, const char *old_target);
+	const git_oid *old_id,
+	const char *old_target);
 
 static int refdb_fs_backend__unlock(git_refdb_backend *backend, void *payload, int success, int update_reflog,
 				    const git_reference *ref, const git_signature *sig, const char *message)
@@ -878,7 +879,7 @@ static int refdb_fs_backend__unlock(git_refdb_backend *backend, void *payload, i
 	if (success == 2)
 		error = refdb_fs_backend__delete_tail(backend, lock, ref->name, NULL, NULL);
 	else if (success)
-		error = refdb_fs_backend__write_tail(backend, ref, lock, update_reflog, sig, message, NULL, NULL);
+		error = refdb_fs_backend__write_tail(backend, ref, lock, update_reflog, NULL, NULL, sig, message);
 	else
 		git_filebuf_cleanup(lock);
 
@@ -1097,6 +1098,35 @@ fail:
 	return error;
 }
 
+static int packed_delete(refdb_fs_backend *backend, const char *ref_name)
+{
+	size_t pack_pos;
+	int error, found = 0;
+
+	if ((error = packed_reload(backend)) < 0)
+		goto cleanup;
+
+	if ((error = git_sortedcache_wlock(backend->refcache)) < 0)
+		goto cleanup;
+
+	/* If a packed reference exists, remove it from the packfile and repack if necessary */
+	error = git_sortedcache_lookup_index(&pack_pos, backend->refcache, ref_name);
+	if (error == 0) {
+		error = git_sortedcache_remove(backend->refcache, pack_pos);
+		found = 1;
+	}
+	if (error == GIT_ENOTFOUND)
+		error = 0;
+
+	git_sortedcache_wunlock(backend->refcache);
+
+	if (found)
+		error = packed_write(backend);
+
+cleanup:
+	return error;
+}
+
 static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, const git_oid *old, const git_oid *new, const git_signature *author, const char *message);
 static int has_reflog(git_repository *repo, const char *name);
 
@@ -1262,7 +1292,7 @@ static int refdb_fs_backend__write(
 	if ((error = loose_lock(&file, backend, ref->name)) < 0)
 		return error;
 
-	return refdb_fs_backend__write_tail(_backend, ref, &file, true, who, message, old_id, old_target);
+	return refdb_fs_backend__write_tail(_backend, ref, &file, true, old_id, old_target, who, message);
 }
 
 static int refdb_fs_backend__write_tail(
@@ -1270,10 +1300,10 @@ static int refdb_fs_backend__write_tail(
 	const git_reference *ref,
 	git_filebuf *file,
 	int update_reflog,
-	const git_signature *who,
-	const char *message,
 	const git_oid *old_id,
-	const char *old_target)
+	const char *old_target,
+	const git_signature *who,
+	const char *message)
 {
 	refdb_fs_backend *backend = GIT_CONTAINER_OF(_backend, refdb_fs_backend, parent);
 	int error = 0, cmp = 0, should_write;
@@ -1323,10 +1353,10 @@ on_error:
         return error;
 }
 
-static void refdb_fs_backend__try_delete_empty_ref_hierarchie(
+static void refdb_fs_backend__prune_refs(
 	refdb_fs_backend *backend,
 	const char *ref_name,
-	bool reflog)
+	const char *prefix)
 {
 	git_buf relative_path = GIT_BUF_INIT;
 	git_buf base_path = GIT_BUF_INIT;
@@ -1344,8 +1374,8 @@ static void refdb_fs_backend__try_delete_empty_ref_hierarchie(
 
 		git_buf_truncate(&relative_path, commonlen);
 
-		if (reflog) {
-			if (git_buf_join3(&base_path, '/', backend->commonpath, GIT_REFLOG_DIR, git_buf_cstr(&relative_path)) < 0)
+		if (prefix) {
+			if (git_buf_join3(&base_path, '/', backend->commonpath, prefix, git_buf_cstr(&relative_path)) < 0)
 				goto cleanup;
 		} else {
 			if (git_buf_joinpath(&base_path, backend->commonpath, git_buf_cstr(&relative_path)) < 0)
@@ -1382,6 +1412,25 @@ static int refdb_fs_backend__delete(
 	return refdb_fs_backend__delete_tail(_backend, &file, ref_name, old_id, old_target);
 }
 
+static int loose_delete(refdb_fs_backend *backend, const char *ref_name)
+{
+	git_buf loose_path = GIT_BUF_INIT;
+	int error = 0;
+
+	if (git_buf_joinpath(&loose_path, backend->commonpath, ref_name) < 0)
+		return -1;
+
+	error = p_unlink(loose_path.ptr);
+	if (error < 0 && errno == ENOENT)
+		error = GIT_ENOTFOUND;
+	else if (error != 0)
+		error = -1;
+
+	git_buf_dispose(&loose_path);
+
+	return error;
+}
+
 static int refdb_fs_backend__delete_tail(
 	git_refdb_backend *_backend,
 	git_filebuf *file,
@@ -1389,10 +1438,8 @@ static int refdb_fs_backend__delete_tail(
 	const git_oid *old_id, const char *old_target)
 {
 	refdb_fs_backend *backend = GIT_CONTAINER_OF(_backend, refdb_fs_backend, parent);
-	git_buf loose_path = GIT_BUF_INIT;
-	size_t pack_pos;
 	int error = 0, cmp = 0;
-	bool loose_deleted = 0;
+	bool packed_deleted = 0;
 
 	error = cmp_old_ref(&cmp, _backend, ref_name, old_id, old_target);
 	if (error < 0)
@@ -1404,44 +1451,41 @@ static int refdb_fs_backend__delete_tail(
 		goto cleanup;
 	}
 
-	/* If a loose reference exists, remove it from the filesystem */
-	if (git_buf_joinpath(&loose_path, backend->commonpath, ref_name) < 0)
-		return -1;
-
-
-	error = p_unlink(loose_path.ptr);
-	if (error < 0 && errno == ENOENT)
-		error = 0;
-	else if (error < 0)
+	/*
+	 * To ensure that an external observer will see either the current ref value
+	 * (because the loose ref still exists), or a missing ref (after the packed-file is
+	 * unlocked, there will be nothing left), we must ensure things happen in the
+	 * following order:
+	 *
+	 * - the packed-ref file is locked and loaded, as well as a loose one, if it exists
+	 * - we optimistically delete a packed ref, keeping track of whether it existed
+	 * - we delete the loose ref, note that we have its .lock
+	 * - the loose ref is "unlocked", then the packed-ref file is rewritten and unlocked
+	 * - we should prune the path components if a loose ref was deleted
+	 *
+	 * Note that, because our packed backend doesn't expose its filesystem lock,
+	 * we might not be able to guarantee that this is what actually happens (ie.
+	 * as our current code never write packed-refs.lock, nothing stops observers
+	 * from grabbing a "stale" value from there).
+	 */
+	if ((error = packed_delete(backend, ref_name)) < 0 && error != GIT_ENOTFOUND)
 		goto cleanup;
-	else if (error == 0)
-		loose_deleted = 1;
 
-	if ((error = packed_reload(backend)) < 0)
-		goto cleanup;
+	if (error == 0)
+		packed_deleted = 1;
 
-	/* If a packed reference exists, remove it from the packfile and repack */
-	if ((error = git_sortedcache_wlock(backend->refcache)) < 0)
+	if ((error = loose_delete(backend, ref_name)) < 0 && error != GIT_ENOTFOUND)
 		goto cleanup;
 
-	if (!(error = git_sortedcache_lookup_index(
-			&pack_pos, backend->refcache, ref_name)))
-		error = git_sortedcache_remove(backend->refcache, pack_pos);
-
-	git_sortedcache_wunlock(backend->refcache);
-
 	if (error == GIT_ENOTFOUND) {
-		error = loose_deleted ? 0 : ref_error_notfound(ref_name);
+		error = packed_deleted ? 0 : ref_error_notfound(ref_name);
 		goto cleanup;
 	}
 
-	error = packed_write(backend);
-
 cleanup:
-	git_buf_dispose(&loose_path);
 	git_filebuf_cleanup(file);
-	if (loose_deleted)
-		refdb_fs_backend__try_delete_empty_ref_hierarchie(backend, ref_name, false);
+	if (error == 0)
+		refdb_fs_backend__prune_refs(backend, ref_name, "");
 	return error;
 }
 
@@ -1904,7 +1948,7 @@ static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, co
 	    !(old && new))
 		return 0;
 
-	/* From here on is_symoblic also means that it's HEAD */
+	/* From here on is_symbolic also means that it's HEAD */
 
 	if (old) {
 		git_oid_cpy(&old_id, old);
@@ -2071,7 +2115,7 @@ static int refdb_reflog_fs__delete(git_refdb_backend *_backend, const char *name
 	if ((error = p_unlink(path.ptr)) < 0)
 		goto out;
 
-	refdb_fs_backend__try_delete_empty_ref_hierarchie(backend, name, true);
+	refdb_fs_backend__prune_refs(backend, name, GIT_REFLOG_DIR);
 
 out:
 	git_buf_dispose(&path);