[PATCH v4 3/3] refs.c: change ref_transaction_commit to run the commit loops once all work is finished

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Apr 14, 2014, 18:29 UTC
Message-ID: <1397500163-7617-4-git-send-email-sahlberg@google.com>
In-Reply-To: <1397500163-7617-1-git-send-email-sahlberg@google.com>

During a transaction commit we will both update and delete refs. Since both update and delete now use the same pattern

    lock = lock_ref_sha1_basic() (or varient of)
    write_ref_sha1(lock)/delete_ref_loose(lock)
    unlock_ref(lock) | commit_ref_lock(lock)

we can now simplify ref_transaction_commit to have one loop that locks all involved refs. A second loop that writes or flags for deletion, but does not commit, all the refs. And a final third loop that commits all the refs once all the work and preparations are complete.

This makes updating/deleting multiple refs more atomic since we will not start the commit phase until all the preparations have completed successfully.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index a14addb..87193c7 100644
--- a/refs.c
+++ b/refs.c
@@ -3467,42 +3467,47 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		}
 	}
 
-	/* Perform updates first so live commits remain referenced */
+	/* Prepare all the updates/deletes */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (!is_null_sha1(update->new_sha1)) {
+		if (!is_null_sha1(update->new_sha1))
 			ret = update_ref_write(msg,
 					       update->refname,
 					       update->new_sha1,
 					       update->lock, onerr);
-			if (ret)
-				unlock_ref(update->lock);
-			else
-				commit_ref_lock(update->lock);
-			update->lock = NULL;
-			if (ret)
-				goto cleanup;
+		else {
+			delnames[delnum++] = update->refname;
+			ret = delete_ref_loose(update->lock, update->type);
 		}
+		if (ret)
+			goto cleanup;
 	}
 
-	/* Perform deletes now that updates are safely completed */
+	ret |= repack_without_refs(delnames, delnum);
+	for (i = 0; i < delnum; i++)
+		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	clear_loose_ref_cache(&ref_cache);
+
+	/* Perform updates first so live commits remain referenced */
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (update->lock && !update->lock->delete_ref) {
+			ret |= commit_ref_lock(update->lock);
+			update->lock = NULL;
+		}
+	}
+	/* And finally perform all deletes */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
 		if (update->lock) {
-			delnames[delnum++] = update->refname;
-			ret |= delete_ref_loose(update->lock, update->type);
 			ret |= commit_ref_lock(update->lock);
 			update->lock = NULL;
 		}
 	}
 
-	ret |= repack_without_refs(delnames, delnum);
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
-	clear_loose_ref_cache(&ref_cache);
-
 cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)

-- 
1.9.1.505.gd05696d