ref_transaction_commit: bail out on failure to remove a ref

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index 097fb4b60ef5e0f84d13b088fa13dea09c58f538..0368ed461f3ef913a60da322247bca0f9d121a82 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -3809,16 +3809,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);