[v3,0/6] refs: excessive hook execution with packed refs

Message

Patrick Steinhardt Jan. 13, 2022, 6:11 a.m. UTC

Hi,

this is the third version of this patch series, which addresses an issue
where the reference-transaction hook is being invoked twice when
deleting refs both in the packed-refs and loose-refs file.

The following things changed in comparison to v2:

    - Fixed some typos in commit messages.

    - Improved the subject of the first patch to more clearly highlight
      the purpose of it, not only say what the patch does.

    - Fixed a missing declaration for `struct string_list`.

    - Clarified why the last patch does the right thing even in case the
      ref only exists in the packed-refs backend, and in case it doesn't
      exist in either of the backends.

Thanks for your feedback!

Patrick

Patrick Steinhardt (6):
  refs: extract packed_refs_delete_refs() to allow control of
    transaction
  refs: allow passing flags when beginning transactions
  refs: allow skipping the reference-transaction hook
  refs: demonstrate excessive execution of the reference-transaction
    hook
  refs: do not execute reference-transaction hook on packing refs
  refs: skip hooks when deleting uncovered packed refs

 refs.c                           | 11 +++++--
 refs.h                           |  8 ++++-
 refs/files-backend.c             | 25 +++++++++++-----
 refs/packed-backend.c            | 30 ++++++++++++++-----
 refs/packed-backend.h            |  7 +++++
 refs/refs-internal.h             |  1 +
 sequencer.c                      |  2 +-
 t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++
 8 files changed, 114 insertions(+), 20 deletions(-)

Range-diff against v2:
1:  0739f085b2 ! 1:  abbc28822b refs: open-code deletion of packed refs
    @@ Metadata
     Author: Patrick Steinhardt <[e-mail]>
     
      ## Commit message ##
    -    refs: open-code deletion of packed refs
    +    refs: extract packed_refs_delete_refs() to allow control of transaction
     
         When deleting loose refs, then we also have to delete the refs in the
         packed backend. This is done by calling `refs_delete_refs()`, which
    @@ Commit message
     
         Extract a new function `packed_refs_delete_refs()`, which hosts most of
         the logic to delete refs except for creating the transaction itself.
    -    Like this, we can easily create the transactionion in the files backend
    +    Like this, we can easily create the transaction in the files backend
         and thus exert more control over it.
     
         Signed-off-by: Patrick Steinhardt <[e-mail]>
    @@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store
      }
     
      ## refs/packed-backend.h ##
    +@@
    + 
    + struct repository;
    + struct ref_transaction;
    ++struct string_list;
    + 
    + /*
    +  * Support for storing references in a `packed-refs` file.
     @@ refs/packed-backend.h: int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
      void packed_refs_unlock(struct ref_store *ref_store);
      int packed_refs_is_locked(struct ref_store *ref_store);
2:  629be01d50 = 2:  9dd172a757 refs: allow passing flags when beginning transactions
3:  550d89a323 = 3:  be826bae3b refs: allow skipping the reference-transaction hook
4:  b52e59cdac ! 4:  662a6e6244 refs: demonstrate excessive execution of the reference-transaction hook
    @@ Metadata
      ## Commit message ##
         refs: demonstrate excessive execution of the reference-transaction hook
     
    -    Add tests which demonstate which demonstrates that we're executing the
    +    Add tests which demonstate that we're executing the
         reference-transaction hook too often in some cases, which thus leaks
         implementation details about the reference store's implementation
         itself. Behaviour will be fixed in follow-up commits.
5:  1539e9711f = 5:  d83f309b9c refs: do not execute reference-transaction hook on packing refs
6:  0fbf68dbf4 ! 6:  279eadc41c refs: skip hooks when deleting uncovered packed refs
    @@ Commit message
     
         Fix this behaviour and don't execute the reference-transaction hook at
         all when refs in the packed-refs backend if it's driven by the files
    -    backend.
    +    backend. This works as expected even in case the refs to be deleted only
    +    exist in the packed-refs backend because the loose-backend always queues
    +    refs in its own transaction even if they don't exist such that they can
    +    be locked for concurrent creation. And it also does the right thing in
    +    case neither of the backends has the ref because that would cause the
    +    transaction to fail completely.
     
         Signed-off-by: Patrick Steinhardt <[e-mail]>