[PATCH 24/24] ref_transaction_commit: bail out on failure to remove a ref

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 2, 2014, 02:35 UTC
Message-ID: <20141002023510.GQ1175@google.com>
In-Reply-To: <20141002014817.GS1175@google.com>

Date: Thu, 28 Aug 2014 17:01:35 -0700

When removal of a loose or packed ref fails, bail out instead of trying to finish the transaction. This way, a single error message can be printed (instead of multiple messages being concatenated by mistake) and the operator can try to solve the underlying problem before there is a chance to muck things up even more.

In particular, when git fails to remove a ref, git goes on to try to delete the reflog. Exiting early lets us keep the reflog.

When git succeeds in deleting a ref A and fails to remove a ref B, it goes on to try to delete both reflogs. It would be better to just remove the reflog for A, but that would be a more invasive change. Failing early means we keep both reflogs, which puts the operator in a good position to understand the problem and recover.

A long term goal is to avoid these problems altogether and roll back the transaction on failure. That kind of transactionality will have to wait for a later series (the plan for which is to make all destructive work happen in a single update of the packed-refs file).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
That's the end of the series.  Thanks for reading.

 refs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index e13f843..d9d327d 100644
--- a/refs.c
+++ b/refs.c
@@ -3753,16 +3753,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (update->lock) {
-			if (delete_ref_loose(update->lock, update->type, err))
+			if (delete_ref_loose(update->lock, update->type, err)) {
 				ret = TRANSACTION_GENERIC_ERROR;
+				goto cleanup;
+			}
 
 			if (!(update->flags & REF_ISPRUNING))
 				delnames[delnum++] = update->lock->ref_name;
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err))
+	if (repack_without_refs(delnames, delnum, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
+		goto cleanup;
+	}
 	for (i = 0; i < delnum; i++)
 		unlink_or_warn(git_path("logs/%s", delnames[i]));
 	clear_loose_ref_cache(&ref_cache);

-- 
2.1.0.rc2.206.gedb03e5