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

Message

Patrick Steinhardt Jan. 17, 2022, 8:12 a.m. UTC

Hi,

this is the fourth 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 v3:

    - Fixed a memory leak in `files_delete_refs()`.

    - Refactored the `packed_downcast()` invocation such that we don't
      have to mark its unused returned variable as used.

    - Removed a spurious whitespace change.

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             | 26 ++++++++++++-----
 refs/packed-backend.c            | 28 +++++++++++++-----
 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(+), 19 deletions(-)

Range-diff against v3:
1:  abbc28822b ! 1:  14775046e1 refs: extract packed_refs_delete_refs() to allow control of transaction
    @@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store,
      
      	packed_refs_unlock(refs->packed_ref_store);
      
    +@@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store, const char *msg,
    + 			result |= error(_("could not remove reference %s"), refname);
    + 	}
    + 
    ++	ref_transaction_free(transaction);
    + 	strbuf_release(&err);
    + 	return result;
    + 
     @@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store, const char *msg,
      	else
      		error(_("could not delete references: %s"), err.buf);
    @@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store
     +			    struct string_list *refnames,
     +			    unsigned int flags)
     +{
    -+	struct packed_ref_store *refs =
    -+		packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
     +	struct strbuf err = STRBUF_INIT;
     +	struct string_list_item *item;
     +	int ret;
     +
    -+	(void)(refs); /* We need the check above, but don't use the variable */
    ++	/* Assert that the ref store refers to a packed backend. */
    ++	packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
     +
      	for_each_string_list_item(item, refnames) {
      		if (ref_transaction_delete(transaction, item->string, NULL,
      					   flags, msg, &err)) {
    -@@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
    - 	}
    - 
    - 	ret = ref_transaction_commit(transaction, &err);
    --
    - 	if (ret) {
    - 		if (refnames->nr == 1)
    - 			error(_("could not delete reference %s: %s"),
     @@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
      			error(_("could not delete references: %s"), err.buf);
      	}
2:  9dd172a757 = 2:  d4ac24c8b8 refs: allow passing flags when beginning transactions
3:  be826bae3b = 3:  f4a07fe9a8 refs: allow skipping the reference-transaction hook
4:  662a6e6244 = 4:  a8981baef7 refs: demonstrate excessive execution of the reference-transaction hook
5:  d83f309b9c = 5:  23c344854e refs: do not execute reference-transaction hook on packing refs
6:  279eadc41c = 6:  d6c7d765af refs: skip hooks when deleting uncovered packed refs