[PATCH] 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>
---

Greetings,

It has been reported[1] that executing git-fetch(1) using transactions can result in an unexpected bug message appearing in some scenarios where it previously did not. This issue can be reproduced with the following commands:

        git init foo &&
        git -C foo commit --allow-empty -m 1 &&
        git clone --mirror foo bar &&
        git -C foo commit --allow-empty -m 2 &&
        touch bar/refs/heads/master.lock &&
        git -C bar fetch --atomic origin master

In this example, the reference updates for the fetch are done in a transaction. Since one of the references is already locked, the transaction is expected to fail, but an unexpected "BUG: refs.c:2435: abort called on a closed reference transaction" message also gets printed.

This issue bisects to c92abe71df (builtin/fetch: fix leaking transaction with `--atomic`, 2024-08-22) which changes how transaction cleanup is handled to address a memory leak. This change starts relying on using `ref_transaction_abort()` to free the transaction when an error occurs during `ref_transaction_commit()`. This is problematic because `ref_transaction_commit()` always closes the transaction and `ref_transaction_abort()` cannot be invoked on a closed transaction. This explains why we start to see the `BUG()` message.

This patch takes a simple approach to fix the issue by explicitly freeing the transaction and setting it to NULL if the commit phase fails.

I've included a test that expects the reference transaction to fail when a reference is already locked. If the `BUG()` was still being printed the process would be aborted resulting in the test failing. This test unfortunately relies on the files backend to create the lock file. I would prefer to be reference backend agnostic, but am unsure of an easy way to exercise the issue.

-Justin

[1]: <e8789a03-41ea-42e2-9f2d-5124b849277a.likui@oschina.cn>

---
 builtin/fetch.c  |  9 ++++++++-
 t/t5510-fetch.sh | 13 +++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)