[PATCH 1/3] refs: push lock management into packed backend

["Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com>]

From: Han-Wen Nienhuys <hanwen@google.com>

Introduces a ref backend method transaction_begin, which for the
packed backend takes packed-refs.lock.

This decouples the files backend from the packed backend, allowing the
latter to be swapped out by another ref backend.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c                |  8 +++++
 refs/debug.c          | 15 +++++++++
 refs/files-backend.c  | 78 +++++++++++++++----------------------------
 refs/packed-backend.c | 41 ++++++++++++++---------
 refs/packed-backend.h | 10 ------
 refs/refs-internal.h  |  5 +++
 6 files changed, 79 insertions(+), 78 deletions(-)

diff --git a/refs.c b/refs.c
index fcae5dddc60..a2d1012520e 100644
--- a/refs.c
+++ b/refs.c
@@ -1130,10 +1130,18 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
 						    struct strbuf *err)
 {
 	struct ref_transaction *tr;
+	int ret = 0;
 	assert(err);
 
 	CALLOC_ARRAY(tr, 1);
 	tr->ref_store = refs;
+
+	if (refs->be->transaction_begin)
+		ret = refs->be->transaction_begin(refs, tr, err);
+	if (ret) {
+		free(tr);
+		return NULL;
+	}
 	return tr;
 }
 
diff --git a/refs/debug.c b/refs/debug.c
index b7ffc4ce67e..964806e9301 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -41,6 +41,20 @@ static int debug_init_db(struct ref_store *refs, struct strbuf *err)
 	return res;
 }
 
+static int debug_transaction_begin(struct ref_store *refs,
+				   struct ref_transaction *transaction,
+				   struct strbuf *err)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+	int res;
+	transaction->ref_store = drefs->refs;
+	res = drefs->refs->be->transaction_begin(drefs->refs, transaction,
+						   err);
+	trace_printf_key(&trace_refs, "transaction_begin: %d \"%s\"\n", res,
+			 err->buf);
+	return res;
+}
+
 static int debug_transaction_prepare(struct ref_store *refs,
 				     struct ref_transaction *transaction,
 				     struct strbuf *err)
@@ -451,6 +465,7 @@ struct ref_storage_be refs_be_debug = {
 	 * has a function we should also have a wrapper for it here.
 	 * Test the output with "GIT_TRACE_REFS=1".
 	 */
+	.transaction_begin = debug_transaction_begin,
 	.transaction_prepare = debug_transaction_prepare,
 	.transaction_finish = debug_transaction_finish,
 	.transaction_abort = debug_transaction_abort,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 341354182bb..4a6781af783 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1219,10 +1219,10 @@ static int files_pack_refs(struct ref_store *ref_store,
 	struct ref_transaction *transaction;
 
 	transaction = ref_store_transaction_begin(refs->packed_ref_store, &err);
-	if (!transaction)
+	if (!transaction) {
+		die("could not start transaction: %s", err.buf);
 		return -1;
-
-	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
+	}
 
 	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
 					the_repository, 0);
@@ -1262,8 +1262,6 @@ static int files_pack_refs(struct ref_store *ref_store,
 
 	ref_transaction_free(transaction);
 
-	packed_refs_unlock(refs->packed_ref_store);
-
 	prune_refs(refs, &refs_to_prune);
 	strbuf_release(&err);
 	return 0;
@@ -1280,16 +1278,10 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 	if (!refnames->nr)
 		return 0;
 
-	if (packed_refs_lock(refs->packed_ref_store, 0, &err))
-		goto error;
-
 	if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
-		packed_refs_unlock(refs->packed_ref_store);
 		goto error;
 	}
 
-	packed_refs_unlock(refs->packed_ref_store);
-
 	for (i = 0; i < refnames->nr; i++) {
 		const char *refname = refnames->items[i].string;
 
@@ -2640,7 +2632,7 @@ out:
 
 struct files_transaction_backend_data {
 	struct ref_transaction *packed_transaction;
-	int packed_refs_locked;
+	int packed_transaction_needed;
 };
 
 /*
@@ -2672,10 +2664,8 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
 			strbuf_release(&err);
 		}
 
-		if (backend_data->packed_refs_locked)
-			packed_refs_unlock(refs->packed_ref_store);
-
 		free(backend_data);
+		transaction->backend_data = NULL;
 	}
 
 	transaction->state = REF_TRANSACTION_CLOSED;
@@ -2804,14 +2794,9 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 	}
 
 	if (packed_transaction) {
-		if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
-			ret = TRANSACTION_GENERIC_ERROR;
-			goto cleanup;
-		}
-		backend_data->packed_refs_locked = 1;
-
-		if (is_packed_transaction_needed(refs->packed_ref_store,
-						 packed_transaction)) {
+		backend_data->packed_transaction_needed = is_packed_transaction_needed(refs->packed_ref_store,
+										       packed_transaction);
+		if (backend_data->packed_transaction_needed) {
 			ret = ref_transaction_prepare(packed_transaction, err);
 			/*
 			 * A failure during the prepare step will abort
@@ -2823,22 +2808,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 				ref_transaction_free(packed_transaction);
 				backend_data->packed_transaction = NULL;
 			}
-		} else {
-			/*
-			 * We can skip rewriting the `packed-refs`
-			 * file. But we do need to leave it locked, so
-			 * that somebody else doesn't pack a reference
-			 * that we are trying to delete.
-			 *
-			 * We need to disconnect our transaction from
-			 * backend_data, since the abort (whether successful or
-			 * not) will free it.
-			 */
-			backend_data->packed_transaction = NULL;
-			if (ref_transaction_abort(packed_transaction, err)) {
-				ret = TRANSACTION_GENERIC_ERROR;
-				goto cleanup;
-			}
 		}
 	}
 
@@ -2940,13 +2909,24 @@ static int files_transaction_finish(struct ref_store *ref_store,
 	 * First delete any packed versions of the references, while
 	 * retaining the packed-refs lock:
 	 */
-	if (packed_transaction) {
-		ret = ref_transaction_commit(packed_transaction, err);
-		ref_transaction_free(packed_transaction);
-		packed_transaction = NULL;
-		backend_data->packed_transaction = NULL;
-		if (ret)
-			goto cleanup;
+	if (backend_data->packed_transaction) { 
+		if (backend_data->packed_transaction_needed) {
+			ret = ref_transaction_commit(packed_transaction, err);
+			if (ret)
+				goto cleanup;
+			/* TODO: leaks on error path. */
+			ref_transaction_free(packed_transaction);
+			packed_transaction = NULL;
+			backend_data->packed_transaction = NULL;
+		} else {
+			ret = ref_transaction_abort(packed_transaction, err);
+			if (ret)
+				goto cleanup;
+
+			/* transaction_commit doesn't free the data, but transaction_abort does. Go figure. */
+			packed_transaction = NULL;
+			backend_data->packed_transaction = NULL;
+		}
 	}
 
 	/* Now delete the loose versions of the references: */
@@ -3087,16 +3067,10 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 					   NULL);
 	}
 
-	if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
-		ret = TRANSACTION_GENERIC_ERROR;
-		goto cleanup;
-	}
-
 	if (initial_ref_transaction_commit(packed_transaction, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 	}
 
-	packed_refs_unlock(refs->packed_ref_store);
 cleanup:
 	if (packed_transaction)
 		ref_transaction_free(packed_transaction);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 59c78d7941f..5df7fa8004f 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1552,8 +1552,6 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
 
 struct packed_transaction_backend_data {
 	/* True iff the transaction owns the packed-refs lock. */
-	int own_lock;
-
 	struct string_list updates;
 };
 
@@ -1568,9 +1566,8 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,
 		if (is_tempfile_active(refs->tempfile))
 			delete_tempfile(&refs->tempfile);
 
-		if (data->own_lock && is_lock_file_locked(&refs->lock)) {
+		if (is_lock_file_locked(&refs->lock)) {
 			packed_refs_unlock(&refs->base);
-			data->own_lock = 0;
 		}
 
 		free(data);
@@ -1580,6 +1577,28 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 
+static int packed_transaction_begin(struct ref_store *ref_store,
+				    struct ref_transaction *transaction,
+				    struct strbuf *err)
+{
+	struct packed_ref_store *refs = packed_downcast(
+			ref_store,
+			REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
+			"ref_transaction_begin");
+	struct packed_transaction_backend_data *data;
+
+	CALLOC_ARRAY(data, 1);
+	string_list_init_nodup(&data->updates);
+
+	if (!is_lock_file_locked(&refs->lock)) {
+		if (packed_refs_lock(ref_store, 0, err))
+			/* todo: leaking data */
+			return -1;
+	}
+	transaction->backend_data = data;
+	return 0;
+}
+
 static int packed_transaction_prepare(struct ref_store *ref_store,
 				      struct ref_transaction *transaction,
 				      struct strbuf *err)
@@ -1588,7 +1607,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
 			ref_store,
 			REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
 			"ref_transaction_prepare");
-	struct packed_transaction_backend_data *data;
+	struct packed_transaction_backend_data *data = transaction->backend_data;
 	size_t i;
 	int ret = TRANSACTION_GENERIC_ERROR;
 
@@ -1601,11 +1620,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
 	 * do so itself.
 	 */
 
-	CALLOC_ARRAY(data, 1);
-	string_list_init_nodup(&data->updates);
-
-	transaction->backend_data = data;
-
 	/*
 	 * Stick the updates in a string list by refname so that we
 	 * can sort them:
@@ -1623,12 +1637,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
 	if (ref_update_reject_duplicates(&data->updates, err))
 		goto failure;
 
-	if (!is_lock_file_locked(&refs->lock)) {
-		if (packed_refs_lock(ref_store, 0, err))
-			goto failure;
-		data->own_lock = 1;
-	}
-
 	if (write_with_updates(refs, &data->updates, err))
 		goto failure;
 
@@ -1758,6 +1766,7 @@ struct ref_storage_be refs_be_packed = {
 	.name = "packed",
 	.init = packed_ref_store_create,
 	.init_db = packed_init_db,
+	.transaction_begin = packed_transaction_begin,
 	.transaction_prepare = packed_transaction_prepare,
 	.transaction_finish = packed_transaction_finish,
 	.transaction_abort = packed_transaction_abort,
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 9dd8a344c34..ade3c8a5ac4 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -17,16 +17,6 @@ struct ref_store *packed_ref_store_create(struct repository *repo,
 					  const char *gitdir,
 					  unsigned int store_flags);
 
-/*
- * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success. On errors, write
- * an error message to `err` and return a nonzero value.
- */
-int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err);
-
-void packed_refs_unlock(struct ref_store *ref_store);
-int packed_refs_is_locked(struct ref_store *ref_store);
-
 /*
  * Return true if `transaction` really needs to be carried out against
  * the specified packed_ref_store, or false if it can be skipped
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 9db8aec4da8..87f8e8b51d6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -531,6 +531,10 @@ typedef struct ref_store *ref_store_init_fn(struct repository *repo,
 
 typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
 
+typedef int ref_transaction_begin_fn(struct ref_store *refs,
+				     struct ref_transaction *transaction,
+				     struct strbuf *err);
+
 typedef int ref_transaction_prepare_fn(struct ref_store *refs,
 				       struct ref_transaction *transaction,
 				       struct strbuf *err);
@@ -670,6 +674,7 @@ struct ref_storage_be {
 	ref_store_init_fn *init;
 	ref_init_db_fn *init_db;
 
+	ref_transaction_begin_fn *transaction_begin;
 	ref_transaction_prepare_fn *transaction_prepare;
 	ref_transaction_finish_fn *transaction_finish;
 	ref_transaction_abort_fn *transaction_abort;
-- 
gitgitgadget