builtin/fetch: avoid aborting closed reference transaction

As part of the reference transaction commit phase, the transaction is
set to a closed state regardless of whether it was successful of not.
Attempting to abort a closed transaction via `ref_transaction_abort()`
results in a `BUG()`.

In c92abe71df (builtin/fetch: fix leaking transaction with `--atomic`,
2024-08-22), logic to free a transaction after the commit phase is moved
to the centralized exit path. In cases where the transaction commit
failed, this results in a closed transaction being aborted and signaling
a bug.

Free the transaction and set it to NULL when the commit fails. This
allows the exit path to correctly handle the error without attempting to
abort the transaction.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 80a64d0d269ed60708d7533977f9ba5c51a4b084..5fb6d6bcf1353e3163dafbf0b73efc4fe652444d 100644 (file)
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1732,8 +1732,15 @@ static int do_fetch(struct transport *transport,
                        goto cleanup;
 
                retcode = ref_transaction_commit(transaction, &err);
-               if (retcode)
+               if (retcode) {
+                       /*
+                        * Explicitly handle transaction cleanup to avoid
+                        * aborting an already closed transaction.
+                        */
+                       ref_transaction_free(transaction);
+                       transaction = NULL;
                        goto cleanup;
+               }
        }
 
        commit_fetch_head(&fetch_head);

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 0890b9f61c56cc4b936fe1206c0378a8d5b15585..4fcf2209f5e751ff3e82e95b2c3b86563d7dd573 100755 (executable)
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -345,6 +345,19 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
        test_cmp expected atomic/.git/FETCH_HEAD
 '
 
+test_expect_success REFFILES 'fetch --atomic fails transaction if reference locked' '
+       test_when_finished "rm -rf upstream repo" &&
+
+       git init upstream &&
+       git -C upstream commit --allow-empty -m 1 &&
+       git -C upstream switch -c foobar &&
+       git clone --mirror upstream repo &&
+       git -C upstream commit --allow-empty -m 2 &&
+       touch repo/refs/heads/foobar.lock &&
+
+       test_must_fail git -C repo fetch --atomic origin
+'
+
 test_expect_success '--refmap="" ignores configured refspec' '
        cd "$TRASH_DIRECTORY" &&
        git clone "$D" remote-refs &&