[PATCH] refs: honor reference-transaction semantics when deleting refs

From: Michael Heemskerk <mheemskerk@atlassian.com>

When deleting refs from the loose-files refs backend, files_delete_refs first rewrites packed-refs if any of the to-be-deleted refs were packed and then removes loose refs. While making those changes, it invokes the reference-transaction hook incorrectly; a single transaction is used to prepare and commit the changes to packed-refs, followed by another transaction per deleted ref, each with another prepared and committed reference-transaction hook invocation.

This behaviour is problematic for a number of reasons. First of all, deleting a ref through `git branch -d` or `git tag -d` results in a different sequence of reference-transaction callbacks than deleting the same ref through `git update-ref`:

- update-ref of a loose ref: aborted, prepared, committed
- update-ref of a packed ref: prepared, prepared, committed, commited
- branch -d: prepared, committed, aborted, prepared, committed

The bigger problem is that the packed-refs update is committed before the prepared reference-transaction invocation is done for the loose ref. Returning a non-zero exit code from the second reference-transaction callback leads to an invalid sequence of reference-transaction callbacks:

1. prepared - hook returns 0, so the change is allowed to go through.
2. committed - git informs the hook that the changes are now final,
   but are they really? Any loose refs may still survive if the
   subsequent prepared callback is canceled.
3. aborted - 'fake' invocation from the packed-transaction because the
   ref does not exist in packed-refs.
4. prepared - hook returns 1, so the change should be blocked.
5. aborted - git informs the hook that it has rolled back the change,
   but it really hasn't; packed-refs has already been rewritten and
   if the ref only existed as a packed ref, it is now gone.

The changes to the reference-transaction invocations that were planned for git 2.36 but have been reverted make the problem more pronounced. Those changes suppress the 'fake' invocations for the packed-transaction (invocations 1-3 in the above list). In that case, the prepared callback in step 4 cannot prevent a ref from being deleted if it only existed in packed-refs.

To address the issue, the use a separate transactions to update the packed and loose refs has been removed from files_delete_refs. Instead, it now uses a single transaction, queues up the refs-to-be-deleted and relies on the standard transaction mechanism to invoke the reference-transaction hooks as expected.

Signed-off-by: Michael Heemskerk <mheemskerk@atlassian.com>
---
    refs: honor reference-transaction semantics when deleting refs
    
    When deleting refs from the loose-files refs backend, files_delete_refs
    first rewrites packed-refs if any of the to-be-deleted refs were packed
    and then removes loose refs. While making those changes, it invokes the
    reference-transaction hook incorrectly; a single transaction is used to
    prepare and commit the changes to packed-refs, followed by another
    transaction per deleted ref, each with another prepared and committed
    reference-transaction hook invocation.
    
    This behaviour is problematic for a number of reasons. First of all,
    deleting a ref through git branch -d or git tag -d results in a
    different sequence of reference-transaction callbacks than deleting the
    same ref through git update-ref:
    
     * update-ref of a loose ref: aborted, prepared, committed
     * update-ref of a packed ref: prepared, prepared, committed, commited
     * branch -d: prepared, committed, aborted, prepared, committed
    
    The bigger problem is that the packed-refs update is committed before
    the prepared reference-transaction invocation is done for the loose ref.
    Returning a non-zero exit code from the second reference-transaction
    callback leads to an invalid sequence of reference-transaction
    callbacks:
    
     1. prepared - hook returns 0, so the change is allowed to go through.
     2. committed - git informs the hook that the changes are now final, but
        are they really? Any loose refs may still survive if the subsequent
        prepared callback is canceled.
     3. aborted - 'fake' invocation from the packed-transaction because the
        ref does not exist in packed-refs.
     4. prepared - hook returns 1, so the change should be blocked.
     5. aborted - git informs the hook that it has rolled back the change,
        but it really hasn't; packed-refs has already been rewritten and if
        the ref only existed as a packed ref, it is now gone.
    
    The changes to the reference-transaction invocations that were planned
    for git 2.36 but have been reverted make the problem more pronounced.
    Those changes suppress the 'fake' invocations for the packed-transaction
    (invocations 1-3 in the above list). In that case, the prepared callback
    in step 4 cannot prevent a ref from being deleted if it only existed in
    packed-refs.
    
    To address the issue, the use a separate transactions to update the
    packed and loose refs has been removed from files_delete_refs. Instead,
    it now uses a single transaction, queues up the refs-to-be-deleted and
    relies on the standard transaction mechanism to invoke the
    reference-transaction hooks as expected.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1228%2Fmheemskerk%2Ffiles-delete-ref-reference-transaction-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1228/mheemskerk/files-delete-ref-reference-transaction-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1228

 refs/files-backend.c             | 33 +++++++--------
 t/t1416-ref-transaction-hooks.sh | 70 ++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh                 |  6 +--
 3 files changed, 87 insertions(+), 22 deletions(-)