refs/files-backend: handle packed transaction prepare failure

In files_transaction_prepare(), if we have to delete some refs, we use a
subordinate packed_transaction to do so. It's rare for that
sub-transaction's prepare step to fail, since we hold the packed-refs
lock. But if it does, we trigger a BUG() due to these steps:

  - we've attached the packed transaction to the files transaction as
    backend_data->packed_transaction

  - when the prepare step fails, the packed transaction cleans itself
    up, putting itself into the CLOSED state

  - the error value from preparing the packed transaction lets us know
    in files_transaction_prepare() that we should also clean up and
    return an error. We call files_transaction_cleanup(), which tries to
    abort backend_data->packed_transaction. Since it's already CLOSED,
    that triggers an assertion in ref_transaction_abort().

We can fix that by disconnecting the packed transaction from the outer
files transaction, and then free-ing (not aborting!) it ourselves.

A few other options/alternatives I considered:

  - we could just make it a noop to abort a CLOSED transaction. But that
    seems less safe, since clearly this code expects (and enforces) a
    particular set of state transitions.

  - we could have files_transaction_cleanup() selectively call abort()
    vs free() based on the state of the on the packed transaction.
    That's basically a more restricted version of the above, but also
    potentially unsafe.

  - instead of disconnecting backend_data->packed_transaction on error,
    we could wait to install it until we successfully prepare. That
    might make the flow a little simpler, but it introduces a hassle.
    Earlier parts of files_transaction_prepare() that encounter an error
    will jump to the cleanup label, and expect that cleaning up the
    outer transaction will clean up the packed transaction, too. We'd
    have to adjust those sites to clean up the packed transaction.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

1 files changed, 10 insertions, 0 deletions

diff --git a/refs/files-backend.c b/refs/files-backend.c
index dd8abe9185..0a2929bf6a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2696,6 +2696,16 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 		if (is_packed_transaction_needed(refs->packed_ref_store,
 						 packed_transaction)) {
 			ret = ref_transaction_prepare(packed_transaction, err);
+			/*
+			 * A failure during the prepare step will abort
+			 * itself, but not free. Do that now, and disconnect
+			 * from the files_transaction so it does not try to
+			 * abort us when we hit the cleanup code below.
+			 */
+			if (ret) {
+				ref_transaction_free(packed_transaction);
+				backend_data->packed_transaction = NULL;
+			}
 		} else {
 			/*
 			 * We can skip rewriting the `packed-refs`