{"thread":{"id":"57044","subject":"[PATCH 0/6] refs: excessive hook execution with packed refs","startedAt":"2021-12-07T10:56:38Z","lastAt":"2022-01-24T07:13:21Z","messageCount":51,"participants":["Patrick Steinhardt","Junio C Hamano","Eric Sunshine","Ævar Arnfjörð Bjarmason","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"443252","messageId":"cover.1638874287.git.ps@pks.im","threadId":"57044","inReplyTo":null,"subject":"[PATCH 0/6] refs: excessive hook execution with packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T10:55:52Z","receivedAt":"2021-12-07T10:56:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nas reported by Waleed in [1], the reference-transaction hook is being\nexecuted when packing refs. Given that the hook ideally ought to track\nlogical updates to refs instead of leaking low-level implementation\ndetails of how the files backend works, this is understandably leading\nto some confusion.\n\nThis patch series aims to fix that by improving how the tandom of loose\nand packed refs backends interact such that we skip executing the hook\nwhen the loose backend:\n\n - repacks references.\n - needs to delete packed refs when deleting a loose ref would\n uncover that packed ref.\n\nPatrick\n\n[1]: \n\nPatrick Steinhardt (6):\n refs: open-code deletion of packed refs\n refs: allow passing flags when beginning transactions\n refs: allow skipping the reference-transaction hook\n refs: demonstrate excessive execution of the reference-transaction\n hook\n refs: do not execute reference-transaction hook on packing refs\n refs: skip hooks when deleting uncovered packed refs\n\n refs.c | 11 +++++--\n refs.h | 8 ++++-\n refs/files-backend.c | 25 +++++++++++-----\n refs/packed-backend.c | 30 ++++++++++++++-----\n refs/packed-backend.h | 6 ++++\n refs/refs-internal.h | 1 +\n sequencer.c | 2 +-\n t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++\n 8 files changed, 113 insertions(+), 20 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"443253","messageId":"282c104b9ddbfa7579bd18fba0e64c6b6cc566b7.1638874287.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1638874287.git.ps@pks.im","subject":"[PATCH 1/6] refs: open-code deletion of packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T10:55:56Z","receivedAt":"2021-12-07T10:56:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When deleting loose refs, then we also have to delete the refs in the\npacked backend. This is done by calling `refs_delete_refs()`, which\nthen uses the packed-backend's logic to delete refs. This doesn't allow\nus to exercise any control over the reference transaction which is being\ncreated in the packed backend, which is required in a subsequent commit.\n\nExtract a new function `packed_refs_delete_refs()`, which hosts most of\nthe logic to delete refs except for creating the transaction itself.\nLike this, we can easily create the transactionion in the files backend\nand thus exert more control over it.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 12 +++++++++---\n refs/packed-backend.c | 28 +++++++++++++++++++++-------\n refs/packed-backend.h | 6 ++++++\n 3 files changed, 36 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4b14f30d48..453adc38ea 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1250,6 +1250,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\tstruct ref_transaction *transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint i, result = 0;\n \n@@ -1259,10 +1260,14 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n-\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\tif (!transaction)\n+\t\tgoto error;\n+\n+\tresult = packed_refs_delete_refs(refs->packed_ref_store,\n+\t\t\t\t\t transaction, msg, refnames, flags);\n+\tif (result)\n \t\tgoto error;\n-\t}\n \n \tpacked_refs_unlock(refs->packed_ref_store);\n \n@@ -1289,6 +1294,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \telse\n \t\terror(_(\"could not delete references: %s\"), err.buf);\n \n+\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn -1;\n }\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 9da932a540..cb19b5291e 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\t struct string_list *refnames, unsigned int flags)\n {\n-\tstruct packed_ref_store *refs =\n-\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n-\tstruct string_list_item *item;\n \tint ret;\n \n-\t(void)refs; /* We need the check above, but don't use the variable */\n-\n \tif (!refnames->nr)\n \t\treturn 0;\n \n@@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (!transaction)\n \t\treturn -1;\n \n+\tret = packed_refs_delete_refs(ref_store, transaction,\n+\t\t\t\t msg, refnames, flags);\n+\n+\tref_transaction_free(transaction);\n+\treturn ret;\n+}\n+\n+int packed_refs_delete_refs(struct ref_store *ref_store,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *msg,\n+\t\t\t struct string_list *refnames,\n+\t\t\t unsigned int flags)\n+{\n+\tstruct packed_ref_store *refs =\n+\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\tint ret;\n+\n+\t(void)(refs); /* We need the check above, but don't use the variable */\n+\n \tfor_each_string_list_item(item, refnames) {\n \t\tif (ref_transaction_delete(transaction, item->string, NULL,\n \t\t\t\t\t flags, msg, &err)) {\n@@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t}\n \n \tret = ref_transaction_commit(transaction, &err);\n-\n \tif (ret) {\n \t\tif (refnames->nr == 1)\n \t\t\terror(_(\"could not delete reference %s: %s\"),\n@@ -1563,7 +1578,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\terror(_(\"could not delete references: %s\"), err.buf);\n \t}\n \n-\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn ret;\n }\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex f61a73ec25..5e0dd7d08e 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -27,6 +27,12 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n void packed_refs_unlock(struct ref_store *ref_store);\n int packed_refs_is_locked(struct ref_store *ref_store);\n \n+int packed_refs_delete_refs(struct ref_store *ref_store,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *msg,\n+\t\t\t struct string_list *refnames,\n+\t\t\t unsigned int flags);\n+\n /*\n * Return true if `transaction` really needs to be carried out against\n * the specified packed_ref_store, or false if it can be skipped\n-- \n2.34.1\n\n"},{"id":"443254","messageId":"f722ec5406d246f6b68237f8d8321c1a7a67109b.1638874287.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1638874287.git.ps@pks.im","subject":"[PATCH 2/6] refs: allow passing flags when beginning transactions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T10:56:00Z","receivedAt":"2021-12-07T10:56:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not currently have any flags when creating reference transactions,\nbut we'll add one to disable execution of the reference transaction hook\nin some cases.\n\nAllow passing flags to `ref_store_transaction_begin()` to prepare for\nthis change.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 8 +++++---\n refs.h | 3 ++-\n refs/files-backend.c | 10 +++++-----\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 1 +\n sequencer.c | 2 +-\n 6 files changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 996ac27164..1c8a0c01f1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -800,7 +800,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\ttransaction = ref_store_transaction_begin(refs, &err);\n+\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname, old_oid,\n \t\t\t\t flags, msg, &err) ||\n@@ -1005,6 +1005,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n }\n \n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_transaction *tr;\n@@ -1012,12 +1013,13 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n \n \tCALLOC_ARRAY(tr, 1);\n \ttr->ref_store = refs;\n+\ttr->flags = flags;\n \treturn tr;\n }\n \n struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n {\n-\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), err);\n+\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);\n }\n \n void ref_transaction_free(struct ref_transaction *transaction)\n@@ -1148,7 +1150,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tt = ref_store_transaction_begin(refs, &err);\n+\tt = ref_store_transaction_begin(refs, 0, &err);\n \tif (!t ||\n \t ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,\n \t\t\t\t &err) ||\ndiff --git a/refs.h b/refs.h\nindex 45c34e99e3..e3d3b7aa14 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -231,7 +231,7 @@ char *repo_default_branch_name(struct repository *r, int quiet);\n * struct strbuf err = STRBUF_INIT;\n * int ret = 0;\n *\n- * transaction = ref_store_transaction_begin(refs, &err);\n+ * transaction = ref_store_transaction_begin(refs, 0, &err);\n * if (!transaction ||\n * ref_transaction_update(...) ||\n * ref_transaction_create(...) ||\n@@ -551,6 +551,7 @@ enum action_on_err {\n * be freed by calling ref_transaction_free().\n */\n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err);\n struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 453adc38ea..95583dcd00 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1122,7 +1122,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base, &err);\n+\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1193,7 +1193,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n \tif (!transaction)\n \t\treturn -1;\n \n@@ -1260,7 +1260,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2767,7 +2767,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store, err);\n+\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3038,7 +3038,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, err);\n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex cb19b5291e..ac4d92460e 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1535,7 +1535,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t * updates into a single transaction.\n \t */\n \n-\ttransaction = ref_store_transaction_begin(ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(ref_store, 0, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex fb2c58ce3b..5bec9014ac 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -213,6 +213,7 @@ struct ref_transaction {\n \tsize_t nr;\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n+\tunsigned int flags;\n };\n \n /*\ndiff --git a/sequencer.c b/sequencer.c\nindex b4135a78c9..78122e0f0c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3599,7 +3599,7 @@ static int do_label(struct repository *r, const char *name, int len)\n \tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n \tstrbuf_addf(&msg, \"rebase (label) '%.*s'\", len, name);\n \n-\ttransaction = ref_store_transaction_begin(refs, &err);\n+\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction) {\n \t\terror(\"%s\", err.buf);\n \t\tret = -1;\n-- \n2.34.1\n\n"},{"id":"443255","messageId":"0e42fc07f80903135bae10ac91e139f85ca9db54.1638874287.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1638874287.git.ps@pks.im","subject":"[PATCH 3/6] refs: allow skipping the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T10:56:05Z","receivedAt":"2021-12-07T10:56:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reference-transaction hook is executing whenever we prepare, commit\nor abort a reference transaction. While this is mostly intentional, in\ncase of the files backend we're leaking the implementation detail that\nthe store is in fact a composite store with one loose and one packed\nbackend to the caller. So while we want to execute the hook for all\nlogical updates, executing it for such implementation details is\nunexpected.\n\nPrepare for a fix by adding a new flag which allows to skip execution of\nthe hook.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 3 +++\n refs.h | 5 +++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 1c8a0c01f1..462e629d37 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2076,6 +2076,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n \tconst char *hook;\n \tint ret = 0, i;\n \n+\tif (transaction->flags & REF_TRANSACTION_SKIP_HOOK)\n+\t\treturn 0;\n+\n \thook = find_hook(\"reference-transaction\");\n \tif (!hook)\n \t\treturn ret;\ndiff --git a/refs.h b/refs.h\nindex e3d3b7aa14..840fa02613 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -546,6 +546,11 @@ enum action_on_err {\n \tUPDATE_REFS_QUIET_ON_ERR\n };\n \n+/*\n+ * Skip executing the reference-transaction hook.\n+ */\n+#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n+\n /*\n * Begin a reference transaction. The reference transaction must\n * be freed by calling ref_transaction_free().\n-- \n2.34.1\n\n"},{"id":"443256","messageId":"972ba1bc34974cb5a93c9863fa86fbc471ee6606.1638874287.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1638874287.git.ps@pks.im","subject":"[PATCH 4/6] refs: demonstrate excessive execution of the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T10:56:09Z","receivedAt":"2021-12-07T10:56:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add tests which demonstate which demonstrates that we're executing the\nreference-transaction hook too often in some cases, which thus leaks\nimplementation details about the reference store's implementation\nitself. Behaviour will be fixed in follow-up commits.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 6c941027a8..0567fbdf0b 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' '\n \ttest_cmp expect target-repo.git/actual\n '\n \n+test_expect_success 'hook does not get called on packing refs' '\n+\t# Pack references first such that we are in a known state.\n+\tgit pack-refs --all &&\n+\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$@\" >>actual\n+\t\tcat >>actual\n+\tEOF\n+\trm -f actual &&\n+\n+\tgit update-ref refs/heads/unpacked-ref $POST_OID &&\n+\tgit pack-refs --all &&\n+\n+\t# We only expect a single hook invocation, which is the call to\n+\t# git-update-ref(1). But currently, packing refs will also trigger the\n+\t# hook.\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tprepared\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tprepared\n+\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'deleting packed ref calls hook once' '\n+\t# Create a reference and pack it.\n+\tgit update-ref refs/heads/to-be-deleted $POST_OID &&\n+\tgit pack-refs --all &&\n+\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$@\" >>actual\n+\t\tcat >>actual\n+\tEOF\n+\trm -f actual &&\n+\n+\tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n+\n+\t# We only expect a single hook invocation, which is the logical\n+\t# deletion. But currently, we see two interleaving transactions, once\n+\t# for deleting the loose refs and once for deleting the packed ref.\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tprepared\n+\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tcommitted\n+\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tcommitted\n+\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.34.1\n\n"},{"id":"443257","messageId":"e2120adf57f0c8b00a9ee66faad01325300a5254.1638874287.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1638874287.git.ps@pks.im","subject":"[PATCH 5/6] refs: do not execute reference-transaction hook on packing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T10:56:13Z","receivedAt":"2021-12-07T10:56:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reference-transaction hook is supposed to track logical changes to\nreferences, but it currently also gets executed when packing refs in a\nrepository. This is unexpected and ultimately not all that useful:\npacking refs is not supposed to result in any user-visible change to the\nrefs' state, and it ultimately is an implementation detail of how refs\nstores work.\n\nFix this excessive execution of the hook when packing refs.\n\nReported-by: Waleed Khan \nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 6 ++++--\n t/t1416-ref-transaction-hooks.sh | 11 +----------\n 2 files changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 95583dcd00..f2bc72f81b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1122,7 +1122,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n+\ttransaction = ref_store_transaction_begin(&refs->base,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1193,7 +1194,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 0567fbdf0b..f9d3d5213f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' '\n \tgit pack-refs --all &&\n \n \t# We only expect a single hook invocation, which is the call to\n-\t# git-update-ref(1). But currently, packing refs will also trigger the\n-\t# hook.\n+\t# git-update-ref(1).\n \tcat >expect <<-EOF &&\n \t\tprepared\n \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n \t\tcommitted\n \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tprepared\n-\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tcommitted\n-\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tprepared\n-\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n-\t\tcommitted\n-\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n \tEOF\n \n \ttest_cmp expect actual\n-- \n2.34.1\n\n"},{"id":"443258","messageId":"04d65ff7991f880920a1fcaab3e4450fad0f15de.1638874287.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1638874287.git.ps@pks.im","subject":"[PATCH 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T10:56:17Z","receivedAt":"2021-12-07T10:57:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When deleting refs from the loose-files refs backend, then we need to be\ncareful to also delete the same ref from the packed refs backend, if it\nexists. If we don't, then deleting the loose ref would \"uncover\" the\npacked ref. We thus always have to queue up deletions of refs for both\nthe loose and the packed refs backend. This is done in two separate\ntransactions, where the end result is that the reference-transaction\nhook is executed twice for the deleted refs.\n\nThis behaviour is quite misleading: it's exposing implementation details\nof how the files backend works to the user, in contrast to the logical\nupdates that we'd really want to expose via the hook. Worse yet, whether\nthe hook gets executed once or twice depends on how well-packed the\nrepository is: if the ref only exists as a loose ref, then we execute it\nonce, otherwise if it is also packed then we execute it twice.\n\nFix this behaviour and don't execute the reference-transaction hook at\nall when refs in the packed-refs backend if it's driven by the files\nbackend.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 9 ++++++---\n t/t1416-ref-transaction-hooks.sh | 7 +------\n 2 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f2bc72f81b..0a2b5ea5da 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1262,7 +1262,8 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2769,7 +2770,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n+\t\t\t\t\t\trefs->packed_ref_store,\n+\t\t\t\t\t\tREF_TRANSACTION_SKIP_HOOK, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3040,7 +3042,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex f9d3d5213f..4e1e84a91f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' '\n \tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n \n \t# We only expect a single hook invocation, which is the logical\n-\t# deletion. But currently, we see two interleaving transactions, once\n-\t# for deleting the loose refs and once for deleting the packed ref.\n+\t# deletion.\n \tcat >expect <<-EOF &&\n-\t\tprepared\n-\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n \t\tprepared\n \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n \t\tcommitted\n-\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n-\t\tcommitted\n \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n \tEOF\n \n-- \n2.34.1\n\n"},{"id":"445704","messageId":"cover.1641556319.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1638874287.git.ps@pks.im","subject":"[PATCH v2 0/6] refs: excessive hook execution with packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-07T11:55:09Z","receivedAt":"2022-01-07T11:55:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is a resend of version 1 of this patch series to hopefully entice\nsome reviews. The only change is that v2 is rebased onto the current\nmain branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The\nfollowing was from the orignial cover letter:\n\nAs reported by Waleed in [1], the reference-transaction hook is being\nexecuted when packing refs. Given that the hook ideally ought to track\nlogical updates to refs instead of leaking low-level implementation\ndetails of how the files backend works, this is understandably leading\nto some confusion.\n\nThis patch series aims to fix that by improving how the tandom of loose\nand packed refs backends interact such that we skip executing the hook\nwhen the loose backend:\n\n - repacks references.\n - needs to delete packed refs when deleting a loose ref would\n uncover that packed ref.\n\nPatrick\n\n[1]: \n\nPatrick Steinhardt (6):\n refs: open-code deletion of packed refs\n refs: allow passing flags when beginning transactions\n refs: allow skipping the reference-transaction hook\n refs: demonstrate excessive execution of the reference-transaction\n hook\n refs: do not execute reference-transaction hook on packing refs\n refs: skip hooks when deleting uncovered packed refs\n\n refs.c | 11 +++++--\n refs.h | 8 ++++-\n refs/files-backend.c | 25 +++++++++++-----\n refs/packed-backend.c | 30 ++++++++++++++-----\n refs/packed-backend.h | 6 ++++\n refs/refs-internal.h | 1 +\n sequencer.c | 2 +-\n t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++\n 8 files changed, 113 insertions(+), 20 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"445705","messageId":"0739f085b266e65b423ccc14f70cc00c88744459.1641556319.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"[PATCH v2 1/6] refs: open-code deletion of packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-07T11:55:13Z","receivedAt":"2022-01-07T11:55:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When deleting loose refs, then we also have to delete the refs in the\npacked backend. This is done by calling `refs_delete_refs()`, which\nthen uses the packed-backend's logic to delete refs. This doesn't allow\nus to exercise any control over the reference transaction which is being\ncreated in the packed backend, which is required in a subsequent commit.\n\nExtract a new function `packed_refs_delete_refs()`, which hosts most of\nthe logic to delete refs except for creating the transaction itself.\nLike this, we can easily create the transactionion in the files backend\nand thus exert more control over it.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 12 +++++++++---\n refs/packed-backend.c | 28 +++++++++++++++++++++-------\n refs/packed-backend.h | 6 ++++++\n 3 files changed, 36 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 90b671025a..5795e54020 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1249,6 +1249,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\tstruct ref_transaction *transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint i, result = 0;\n \n@@ -1258,10 +1259,14 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n-\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\tif (!transaction)\n+\t\tgoto error;\n+\n+\tresult = packed_refs_delete_refs(refs->packed_ref_store,\n+\t\t\t\t\t transaction, msg, refnames, flags);\n+\tif (result)\n \t\tgoto error;\n-\t}\n \n \tpacked_refs_unlock(refs->packed_ref_store);\n \n@@ -1288,6 +1293,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \telse\n \t\terror(_(\"could not delete references: %s\"), err.buf);\n \n+\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn -1;\n }\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 67152c664e..e97d67f932 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\t struct string_list *refnames, unsigned int flags)\n {\n-\tstruct packed_ref_store *refs =\n-\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n-\tstruct string_list_item *item;\n \tint ret;\n \n-\t(void)refs; /* We need the check above, but don't use the variable */\n-\n \tif (!refnames->nr)\n \t\treturn 0;\n \n@@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (!transaction)\n \t\treturn -1;\n \n+\tret = packed_refs_delete_refs(ref_store, transaction,\n+\t\t\t\t msg, refnames, flags);\n+\n+\tref_transaction_free(transaction);\n+\treturn ret;\n+}\n+\n+int packed_refs_delete_refs(struct ref_store *ref_store,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *msg,\n+\t\t\t struct string_list *refnames,\n+\t\t\t unsigned int flags)\n+{\n+\tstruct packed_ref_store *refs =\n+\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\tint ret;\n+\n+\t(void)(refs); /* We need the check above, but don't use the variable */\n+\n \tfor_each_string_list_item(item, refnames) {\n \t\tif (ref_transaction_delete(transaction, item->string, NULL,\n \t\t\t\t\t flags, msg, &err)) {\n@@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t}\n \n \tret = ref_transaction_commit(transaction, &err);\n-\n \tif (ret) {\n \t\tif (refnames->nr == 1)\n \t\t\terror(_(\"could not delete reference %s: %s\"),\n@@ -1563,7 +1578,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\terror(_(\"could not delete references: %s\"), err.buf);\n \t}\n \n-\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn ret;\n }\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex f61a73ec25..5e0dd7d08e 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -27,6 +27,12 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n void packed_refs_unlock(struct ref_store *ref_store);\n int packed_refs_is_locked(struct ref_store *ref_store);\n \n+int packed_refs_delete_refs(struct ref_store *ref_store,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *msg,\n+\t\t\t struct string_list *refnames,\n+\t\t\t unsigned int flags);\n+\n /*\n * Return true if `transaction` really needs to be carried out against\n * the specified packed_ref_store, or false if it can be skipped\n-- \n2.34.1\n\n"},{"id":"445706","messageId":"629be01d50be53c51273286febb9d4a890c968a5.1641556319.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"[PATCH v2 2/6] refs: allow passing flags when beginning transactions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-07T11:55:17Z","receivedAt":"2022-01-07T11:55:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not currently have any flags when creating reference transactions,\nbut we'll add one to disable execution of the reference transaction hook\nin some cases.\n\nAllow passing flags to `ref_store_transaction_begin()` to prepare for\nthis change.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 8 +++++---\n refs.h | 3 ++-\n refs/files-backend.c | 10 +++++-----\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 1 +\n sequencer.c | 2 +-\n 6 files changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4da4996c4d..7415864b62 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -800,7 +800,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\ttransaction = ref_store_transaction_begin(refs, &err);\n+\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname, old_oid,\n \t\t\t\t flags, msg, &err) ||\n@@ -1005,6 +1005,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n }\n \n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_transaction *tr;\n@@ -1012,12 +1013,13 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n \n \tCALLOC_ARRAY(tr, 1);\n \ttr->ref_store = refs;\n+\ttr->flags = flags;\n \treturn tr;\n }\n \n struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n {\n-\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), err);\n+\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);\n }\n \n void ref_transaction_free(struct ref_transaction *transaction)\n@@ -1156,7 +1158,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tt = ref_store_transaction_begin(refs, &err);\n+\tt = ref_store_transaction_begin(refs, 0, &err);\n \tif (!t ||\n \t ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,\n \t\t\t\t &err) ||\ndiff --git a/refs.h b/refs.h\nindex 92360e55a2..31f7bf9642 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -231,7 +231,7 @@ char *repo_default_branch_name(struct repository *r, int quiet);\n * struct strbuf err = STRBUF_INIT;\n * int ret = 0;\n *\n- * transaction = ref_store_transaction_begin(refs, &err);\n+ * transaction = ref_store_transaction_begin(refs, 0, &err);\n * if (!transaction ||\n * ref_transaction_update(...) ||\n * ref_transaction_create(...) ||\n@@ -573,6 +573,7 @@ enum action_on_err {\n * be freed by calling ref_transaction_free().\n */\n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err);\n struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5795e54020..ecf88cee04 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1121,7 +1121,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base, &err);\n+\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1192,7 +1192,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n \tif (!transaction)\n \t\treturn -1;\n \n@@ -1259,7 +1259,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2773,7 +2773,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store, err);\n+\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3044,7 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, err);\n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex e97d67f932..cfebcd0b46 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1535,7 +1535,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t * updates into a single transaction.\n \t */\n \n-\ttransaction = ref_store_transaction_begin(ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(ref_store, 0, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 46a839539e..a0af63f162 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -213,6 +213,7 @@ struct ref_transaction {\n \tsize_t nr;\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n+\tunsigned int flags;\n };\n \n /*\ndiff --git a/sequencer.c b/sequencer.c\nindex 6abd72160c..61edd39d7a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3588,7 +3588,7 @@ static int do_label(struct repository *r, const char *name, int len)\n \tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n \tstrbuf_addf(&msg, \"rebase (label) '%.*s'\", len, name);\n \n-\ttransaction = ref_store_transaction_begin(refs, &err);\n+\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction) {\n \t\terror(\"%s\", err.buf);\n \t\tret = -1;\n-- \n2.34.1\n\n"},{"id":"445707","messageId":"550d89a3236eb7afcd5ef627509368e8fc298662.1641556319.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"[PATCH v2 3/6] refs: allow skipping the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-07T11:55:21Z","receivedAt":"2022-01-07T11:55:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reference-transaction hook is executing whenever we prepare, commit\nor abort a reference transaction. While this is mostly intentional, in\ncase of the files backend we're leaking the implementation detail that\nthe store is in fact a composite store with one loose and one packed\nbackend to the caller. So while we want to execute the hook for all\nlogical updates, executing it for such implementation details is\nunexpected.\n\nPrepare for a fix by adding a new flag which allows to skip execution of\nthe hook.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 3 +++\n refs.h | 5 +++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 7415864b62..526bf5ed97 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2084,6 +2084,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n \tconst char *hook;\n \tint ret = 0, i;\n \n+\tif (transaction->flags & REF_TRANSACTION_SKIP_HOOK)\n+\t\treturn 0;\n+\n \thook = find_hook(\"reference-transaction\");\n \tif (!hook)\n \t\treturn ret;\ndiff --git a/refs.h b/refs.h\nindex 31f7bf9642..d4056f9fe2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -568,6 +568,11 @@ enum action_on_err {\n \tUPDATE_REFS_QUIET_ON_ERR\n };\n \n+/*\n+ * Skip executing the reference-transaction hook.\n+ */\n+#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n+\n /*\n * Begin a reference transaction. The reference transaction must\n * be freed by calling ref_transaction_free().\n-- \n2.34.1\n\n"},{"id":"445708","messageId":"b52e59cdac75e6c4530cb39f7dcb41bb327f50e2.1641556319.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"[PATCH v2 4/6] refs: demonstrate excessive execution of the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-07T11:55:26Z","receivedAt":"2022-01-07T11:56:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add tests which demonstate which demonstrates that we're executing the\nreference-transaction hook too often in some cases, which thus leaks\nimplementation details about the reference store's implementation\nitself. Behaviour will be fixed in follow-up commits.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 6c941027a8..0567fbdf0b 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' '\n \ttest_cmp expect target-repo.git/actual\n '\n \n+test_expect_success 'hook does not get called on packing refs' '\n+\t# Pack references first such that we are in a known state.\n+\tgit pack-refs --all &&\n+\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$@\" >>actual\n+\t\tcat >>actual\n+\tEOF\n+\trm -f actual &&\n+\n+\tgit update-ref refs/heads/unpacked-ref $POST_OID &&\n+\tgit pack-refs --all &&\n+\n+\t# We only expect a single hook invocation, which is the call to\n+\t# git-update-ref(1). But currently, packing refs will also trigger the\n+\t# hook.\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tprepared\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tprepared\n+\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'deleting packed ref calls hook once' '\n+\t# Create a reference and pack it.\n+\tgit update-ref refs/heads/to-be-deleted $POST_OID &&\n+\tgit pack-refs --all &&\n+\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$@\" >>actual\n+\t\tcat >>actual\n+\tEOF\n+\trm -f actual &&\n+\n+\tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n+\n+\t# We only expect a single hook invocation, which is the logical\n+\t# deletion. But currently, we see two interleaving transactions, once\n+\t# for deleting the loose refs and once for deleting the packed ref.\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tprepared\n+\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tcommitted\n+\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tcommitted\n+\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.34.1\n\n"},{"id":"445709","messageId":"0fbf68dbf434986aa971961e20598675b2194d51.1641556319.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"[PATCH v2 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-07T11:55:34Z","receivedAt":"2022-01-07T11:56:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When deleting refs from the loose-files refs backend, then we need to be\ncareful to also delete the same ref from the packed refs backend, if it\nexists. If we don't, then deleting the loose ref would \"uncover\" the\npacked ref. We thus always have to queue up deletions of refs for both\nthe loose and the packed refs backend. This is done in two separate\ntransactions, where the end result is that the reference-transaction\nhook is executed twice for the deleted refs.\n\nThis behaviour is quite misleading: it's exposing implementation details\nof how the files backend works to the user, in contrast to the logical\nupdates that we'd really want to expose via the hook. Worse yet, whether\nthe hook gets executed once or twice depends on how well-packed the\nrepository is: if the ref only exists as a loose ref, then we execute it\nonce, otherwise if it is also packed then we execute it twice.\n\nFix this behaviour and don't execute the reference-transaction hook at\nall when refs in the packed-refs backend if it's driven by the files\nbackend.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 9 ++++++---\n t/t1416-ref-transaction-hooks.sh | 7 +------\n 2 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3c0f3027fe..9a20cb8fa8 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1261,7 +1261,8 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2775,7 +2776,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n+\t\t\t\t\t\trefs->packed_ref_store,\n+\t\t\t\t\t\tREF_TRANSACTION_SKIP_HOOK, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3046,7 +3048,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex f9d3d5213f..4e1e84a91f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' '\n \tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n \n \t# We only expect a single hook invocation, which is the logical\n-\t# deletion. But currently, we see two interleaving transactions, once\n-\t# for deleting the loose refs and once for deleting the packed ref.\n+\t# deletion.\n \tcat >expect <<-EOF &&\n-\t\tprepared\n-\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n \t\tprepared\n \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n \t\tcommitted\n-\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n-\t\tcommitted\n \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n \tEOF\n \n-- \n2.34.1\n\n"},{"id":"445710","messageId":"1539e9711f8697bcbbc90b602c69c7f268ad7c37.1641556319.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"[PATCH v2 5/6] refs: do not execute reference-transaction hook on packing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-07T11:55:30Z","receivedAt":"2022-01-07T11:56:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reference-transaction hook is supposed to track logical changes to\nreferences, but it currently also gets executed when packing refs in a\nrepository. This is unexpected and ultimately not all that useful:\npacking refs is not supposed to result in any user-visible change to the\nrefs' state, and it ultimately is an implementation detail of how refs\nstores work.\n\nFix this excessive execution of the hook when packing refs.\n\nReported-by: Waleed Khan \nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 6 ++++--\n t/t1416-ref-transaction-hooks.sh | 11 +----------\n 2 files changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ecf88cee04..3c0f3027fe 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1121,7 +1121,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n+\ttransaction = ref_store_transaction_begin(&refs->base,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1192,7 +1193,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 0567fbdf0b..f9d3d5213f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' '\n \tgit pack-refs --all &&\n \n \t# We only expect a single hook invocation, which is the call to\n-\t# git-update-ref(1). But currently, packing refs will also trigger the\n-\t# hook.\n+\t# git-update-ref(1).\n \tcat >expect <<-EOF &&\n \t\tprepared\n \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n \t\tcommitted\n \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tprepared\n-\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tcommitted\n-\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tprepared\n-\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n-\t\tcommitted\n-\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n \tEOF\n \n \ttest_cmp expect actual\n-- \n2.34.1\n\n"},{"id":"445757","messageId":"xmqqiluv6vim.fsf@gitster.g","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"Re: [PATCH v2 0/6] refs: excessive hook execution with packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-07T22:17:21Z","receivedAt":"2022-01-07T22:17:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this is a resend of version 1 of this patch series to hopefully entice\n> some reviews. The only change is that v2 is rebased onto the current\n> main branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The\n> following was from the orignial cover letter:\n\nI'll add Ævar, who has been making a lot of changes to the refs\nsubsystem, and Han-Wen, whose work to add a new ref backend may need\nto interact with this change, as possible stake-holders to the CC list.\n\n> As reported by Waleed in [1], the reference-transaction hook is being\n> executed when packing refs. Given that the hook ideally ought to track\n> logical updates to refs instead of leaking low-level implementation\n> details of how the files backend works, this is understandably leading\n> to some confusion.\n>\n> This patch series aims to fix that by improving how the tandom of loose\n> and packed refs backends interact such that we skip executing the hook\n> when the loose backend:\n>\n> - repacks references.\n> - needs to delete packed refs when deleting a loose ref would\n> uncover that packed ref.\n>\n> Patrick\n>\n> [1]: \n>\n> Patrick Steinhardt (6):\n> refs: open-code deletion of packed refs\n> refs: allow passing flags when beginning transactions\n> refs: allow skipping the reference-transaction hook\n> refs: demonstrate excessive execution of the reference-transaction\n> hook\n> refs: do not execute reference-transaction hook on packing refs\n> refs: skip hooks when deleting uncovered packed refs\n>\n> refs.c | 11 +++++--\n> refs.h | 8 ++++-\n> refs/files-backend.c | 25 +++++++++++-----\n> refs/packed-backend.c | 30 ++++++++++++++-----\n> refs/packed-backend.h | 6 ++++\n> refs/refs-internal.h | 1 +\n> sequencer.c | 2 +-\n> t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++\n> 8 files changed, 113 insertions(+), 20 deletions(-)\n"},{"id":"445769","messageId":"xmqqo84n3uyo.fsf@gitster.g","threadId":"57044","inReplyTo":"0739f085b266e65b423ccc14f70cc00c88744459.1641556319.git.ps@pks.im","subject":"Re: [PATCH v2 1/6] refs: open-code deletion of packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-08T00:57:35Z","receivedAt":"2022-01-08T00:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\nThis breaks the static analysis job at\nhttps://github.com/git/git/runs/4744820766\n\n> refs/packed-backend.h | 6 ++++++\n> diff --git a/refs/packed-backend.h b/refs/packed-backend.h\n> index f61a73ec25..5e0dd7d08e 100644\n> --- a/refs/packed-backend.h\n> +++ b/refs/packed-backend.h\n> @@ -27,6 +27,12 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n> void packed_refs_unlock(struct ref_store *ref_store);\n> int packed_refs_is_locked(struct ref_store *ref_store);\n> \n> +int packed_refs_delete_refs(struct ref_store *ref_store,\n> +\t\t\t struct ref_transaction *transaction,\n> +\t\t\t const char *msg,\n> +\t\t\t struct string_list *refnames,\n> +\t\t\t unsigned int flags);\n> +\n> /*\n> * Return true if `transaction` really needs to be carried out against\n> * the specified packed_ref_store, or false if it can be skipped\n\n HDR reftable/block.h\nIn file included from refs/packed-backend.hcc:2:0:\n./refs/packed-backend.h:33:15: error: ‘struct string_list’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]\n struct string_list *refnames,\n ^~~~~~~~~~~\ncc1: all warnings being treated as errors\nmake: *** [refs/packed-backend.hco] Error 1\nmake: *** Waiting for unfinished jobs....\nMakefile:3029: recipe for target 'refs/packed-backend.hco' failed\nError: Process completed with exit code 1.\n"},{"id":"445774","messageId":"xmqqiluv3tev.fsf@gitster.g","threadId":"57044","inReplyTo":"b52e59cdac75e6c4530cb39f7dcb41bb327f50e2.1641556319.git.ps@pks.im","subject":"Re: [PATCH v2 4/6] refs: demonstrate excessive execution of the reference-transaction hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-08T01:31:04Z","receivedAt":"2022-01-08T01:31:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Add tests which demonstate which demonstrates that we're executing the\n\nYou demonstrate too often, which may be the point of the test, but\nlooks wrong.\n\nI actually think this should be done as part of the fix to the code\nitself, which presumably is a single-liner to tell the \"skip when\nrunning delete in packed-refs backend\". IOW, just fix the code and\ntest how the externally observable behaviour of the code should be\nin new tests, in the same commit.\n\n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> index 6c941027a8..0567fbdf0b 100755\n> --- a/t/t1416-ref-transaction-hooks.sh\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> @@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' '\n> \ttest_cmp expect target-repo.git/actual\n> '\n> \n> +test_expect_success 'hook does not get called on packing refs' '\n> +\t# Pack references first such that we are in a known state.\n> +\tgit pack-refs --all &&\n> +\n> +\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n> +\t\techo \"$@\" >>actual\n> +\t\tcat >>actual\n> +\tEOF\n> +\trm -f actual &&\n> +\n> +\tgit update-ref refs/heads/unpacked-ref $POST_OID &&\n> +\tgit pack-refs --all &&\n> +\n> +\t# We only expect a single hook invocation, which is the call to\n> +\t# git-update-ref(1). But currently, packing refs will also trigger the\n> +\t# hook.\n> +\tcat >expect <<-EOF &&\n> +\t\tprepared\n> +\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> +\t\tcommitted\n> +\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> +\t\tprepared\n> +\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> +\t\tcommitted\n> +\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> +\t\tprepared\n> +\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n> +\t\tcommitted\n> +\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n> +\tEOF\n> +\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'deleting packed ref calls hook once' '\n> +\t# Create a reference and pack it.\n> +\tgit update-ref refs/heads/to-be-deleted $POST_OID &&\n> +\tgit pack-refs --all &&\n> +\n> +\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n> +\t\techo \"$@\" >>actual\n> +\t\tcat >>actual\n> +\tEOF\n> +\trm -f actual &&\n> +\n> +\tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n> +\n> +\t# We only expect a single hook invocation, which is the logical\n> +\t# deletion. But currently, we see two interleaving transactions, once\n> +\t# for deleting the loose refs and once for deleting the packed ref.\n> +\tcat >expect <<-EOF &&\n> +\t\tprepared\n> +\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n> +\t\tprepared\n> +\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n> +\t\tcommitted\n> +\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n> +\t\tcommitted\n> +\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n> +\tEOF\n> +\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"445775","messageId":"xmqqczl33shj.fsf@gitster.g","threadId":"57044","inReplyTo":"0739f085b266e65b423ccc14f70cc00c88744459.1641556319.git.ps@pks.im","subject":"Re: [PATCH v2 1/6] refs: open-code deletion of packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-08T01:51:04Z","receivedAt":"2022-01-08T01:51:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When deleting loose refs, then we also have to delete the refs in the\n> packed backend. This is done by calling `refs_delete_refs()`, which\n> then uses the packed-backend's logic to delete refs. This doesn't allow\n> us to exercise any control over the reference transaction which is being\n> created in the packed backend, which is required in a subsequent commit.\n>\n> Extract a new function `packed_refs_delete_refs()`, which hosts most of\n> the logic to delete refs except for creating the transaction itself.\n> Like this, we can easily create the transactionion in the files backend\n\nionion?\n\n> and thus exert more control over it.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n\nThe title might be technically correct, but I think the primary\npoint, the benefit this step brings to the system, is now we have a\nhelper function we can call in a transaction we create ourselves.\n\n refs: extract packed_refs_delete_refs() to be used in our own transaction\n\nor something along that line, perhaps? I dunno.\n\n> refs/files-backend.c | 12 +++++++++---\n> refs/packed-backend.c | 28 +++++++++++++++++++++-------\n> refs/packed-backend.h | 6 ++++++\n> 3 files changed, 36 insertions(+), 10 deletions(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 90b671025a..5795e54020 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1249,6 +1249,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> {\n> \tstruct files_ref_store *refs =\n> \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n> +\tstruct ref_transaction *transaction = NULL;\n> \tstruct strbuf err = STRBUF_INIT;\n> \tint i, result = 0;\n> \n> @@ -1258,10 +1259,14 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n> \t\tgoto error;\n> \n> -\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n> -\t\tpacked_refs_unlock(refs->packed_ref_store);\n> +\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n> +\tif (!transaction)\n> +\t\tgoto error;\n> +\n> +\tresult = packed_refs_delete_refs(refs->packed_ref_store,\n> +\t\t\t\t\t transaction, msg, refnames, flags);\n> +\tif (result)\n> \t\tgoto error;\n> -\t}\n\nWithout looking at the later patches, my guess is that we'd create a\nsingle transaction for packed-refs backend and then issue multiple\ndelete requests in that single transaction?\n\nAh, no. We are already deleting multiple refs in one go, so that is\nnot what is happening here. Makes readers a bit curious. Hopefully\nwe will find it out in a later patch.\n\n> diff --git a/refs/packed-backend.h b/refs/packed-backend.h\n> index f61a73ec25..5e0dd7d08e 100644\n> --- a/refs/packed-backend.h\n> +++ b/refs/packed-backend.h\n> @@ -27,6 +27,12 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n> void packed_refs_unlock(struct ref_store *ref_store);\n> int packed_refs_is_locked(struct ref_store *ref_store);\n> \n> +int packed_refs_delete_refs(struct ref_store *ref_store,\n> +\t\t\t struct ref_transaction *transaction,\n> +\t\t\t const char *msg,\n> +\t\t\t struct string_list *refnames,\n> +\t\t\t unsigned int flags);\n> +\n> /*\n> * Return true if `transaction` really needs to be carried out against\n> * the specified packed_ref_store, or false if it can be skipped\n"},{"id":"445776","messageId":"xmqq7dbb3shg.fsf@gitster.g","threadId":"57044","inReplyTo":"629be01d50be53c51273286febb9d4a890c968a5.1641556319.git.ps@pks.im","subject":"Re: [PATCH v2 2/6] refs: allow passing flags when beginning transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-08T01:51:07Z","receivedAt":"2022-01-08T01:51:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We do not currently have any flags when creating reference transactions,\n> but we'll add one to disable execution of the reference transaction hook\n> in some cases.\n>\n> Allow passing flags to `ref_store_transaction_begin()` to prepare for\n> this change.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs.c | 8 +++++---\n> refs.h | 3 ++-\n> refs/files-backend.c | 10 +++++-----\n> refs/packed-backend.c | 2 +-\n> refs/refs-internal.h | 1 +\n> sequencer.c | 2 +-\n> 6 files changed, 15 insertions(+), 11 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 4da4996c4d..7415864b62 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -800,7 +800,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n> \tstruct ref_transaction *transaction;\n> \tstruct strbuf err = STRBUF_INIT;\n> \n> -\ttransaction = ref_store_transaction_begin(refs, &err);\n> +\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n\nAh, OK, the \"later\" came soon enough. It wasn't that we wanted to\ncram multiple calls for \"delete\" in a single transaction. It was\nthat we wanted to tweak how each transaction works by passing a\nflag word.\n\n> struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n> +\t\t\t\t\t\t unsigned int flags,\n> \t\t\t\t\t\t struct strbuf *err)\n\nOK. And in this step, everybody passes 0 (i.e. no bit set) to the\nfunction.\n\n"},{"id":"445777","messageId":"xmqq1r1j3s0v.fsf@gitster.g","threadId":"57044","inReplyTo":"0fbf68dbf434986aa971961e20598675b2194d51.1641556319.git.ps@pks.im","subject":"Re: [PATCH v2 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-08T02:01:04Z","receivedAt":"2022-01-08T02:01:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When deleting refs from the loose-files refs backend, then we need to be\n> careful to also delete the same ref from the packed refs backend, if it\n> exists. If we don't, then deleting the loose ref would \"uncover\" the\n> packed ref. We thus always have to queue up deletions of refs for both\n> the loose and the packed refs backend. This is done in two separate\n> transactions, where the end result is that the reference-transaction\n> hook is executed twice for the deleted refs.\n>\n> This behaviour is quite misleading: it's exposing implementation details\n> of how the files backend works to the user, in contrast to the logical\n> updates that we'd really want to expose via the hook. Worse yet, whether\n> the hook gets executed once or twice depends on how well-packed the\n> repository is: if the ref only exists as a loose ref, then we execute it\n> once, otherwise if it is also packed then we execute it twice.\n\nIf the ref only exists as a packed ref, what happens? ...\n\n> Fix this behaviour and don't execute the reference-transaction hook at\n> all when refs in the packed-refs backend if it's driven by the files\n> backend.\n\n... We try to remove from the loose backend, which would say \"nah,\nit did not exist in my store\". I am not sure if it should execute\nthe delete hook in such a case for the ref. But if it does not, not\nrunning the hook in the ref transaction for packed backend driven by\nthe loose backend would mean nobody notifies the deletion of the\nref, no?\n\nTo me, it seems that the only case this strategy would work\ncorrectly is when the files backend calls deletion hook for a\nrequest to delete nonexistent ref, which by itself sounds like a\nproblem.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/files-backend.c | 9 ++++++---\n> t/t1416-ref-transaction-hooks.sh | 7 +------\n> 2 files changed, 7 insertions(+), 9 deletions(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 3c0f3027fe..9a20cb8fa8 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1261,7 +1261,8 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n> \t\tgoto error;\n> \n> -\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n> +\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n> +\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> \tif (!transaction)\n> \t\tgoto error;\n> \n> @@ -2775,7 +2776,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> \t\t\t */\n> \t\t\tif (!packed_transaction) {\n> \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n> -\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n> +\t\t\t\t\t\trefs->packed_ref_store,\n> +\t\t\t\t\t\tREF_TRANSACTION_SKIP_HOOK, err);\n> \t\t\t\tif (!packed_transaction) {\n> \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n> \t\t\t\t\tgoto cleanup;\n> @@ -3046,7 +3048,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n> \t\t\t\t &affected_refnames))\n> \t\tBUG(\"initial ref transaction called with existing refs\");\n> \n> -\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n> +\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n> +\t\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, err);\n> \tif (!packed_transaction) {\n> \t\tret = TRANSACTION_GENERIC_ERROR;\n> \t\tgoto cleanup;\n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> index f9d3d5213f..4e1e84a91f 100755\n> --- a/t/t1416-ref-transaction-hooks.sh\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> @@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' '\n> \tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n> \n> \t# We only expect a single hook invocation, which is the logical\n> -\t# deletion. But currently, we see two interleaving transactions, once\n> -\t# for deleting the loose refs and once for deleting the packed ref.\n> +\t# deletion.\n> \tcat >expect <<-EOF &&\n> -\t\tprepared\n> -\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n> \t\tprepared\n> \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n> \t\tcommitted\n> -\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n> -\t\tcommitted\n> \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n> \tEOF\n"},{"id":"445781","messageId":"CAPig+cR=XAwUT9EdLjOdw3RrOXPUspR2VMW-wv5p8hOMc=iAxQ@mail.gmail.com","threadId":"57044","inReplyTo":"b52e59cdac75e6c4530cb39f7dcb41bb327f50e2.1641556319.git.ps@pks.im","subject":"Re: [PATCH v2 4/6] refs: demonstrate excessive execution of the reference-transaction hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-01-08T05:43:40Z","receivedAt":"2022-01-08T05:43:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 7, 2022 at 3:09 PM Patrick Steinhardt wrote:\n> Add tests which demonstate which demonstrates that we're executing the\n> reference-transaction hook too often in some cases, which thus leaks\n> implementation details about the reference store's implementation\n> itself. Behaviour will be fixed in follow-up commits.\n\ns/which demonstate which demonstrates/which demonstrate/\n\n> Signed-off-by: Patrick Steinhardt \n"},{"id":"445825","messageId":"Ydwsa0LiVfj+YP7N@ncase","threadId":"57044","inReplyTo":"xmqqiluv3tev.fsf@gitster.g","subject":"Re: [PATCH v2 4/6] refs: demonstrate excessive execution of the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-10T12:54:03Z","receivedAt":"2022-01-10T12:55:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 07, 2022 at 05:31:04PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > Add tests which demonstate which demonstrates that we're executing the\n> \n> You demonstrate too often, which may be the point of the test, but\n> looks wrong.\n> \n> I actually think this should be done as part of the fix to the code\n> itself, which presumably is a single-liner to tell the \"skip when\n> running delete in packed-refs backend\". IOW, just fix the code and\n> test how the externally observable behaviour of the code should be\n> in new tests, in the same commit.\n\nThe reason why I chose to split this out into a separate commit is that\nit makes it easier to see what behaviour exactly is changing. If it was\na single step, then a reader would only see the post-image behaviour but\ncannot reason about the pre-image behaviour without puzzling everything\ntogether.\n\nSo personally I'd prefer to keep it as a separate step, but I'm not\nopposed to merging them if you still disagree with my reasoning above.\n\nPatrick\n\n> > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> > index 6c941027a8..0567fbdf0b 100755\n> > --- a/t/t1416-ref-transaction-hooks.sh\n> > +++ b/t/t1416-ref-transaction-hooks.sh\n> > @@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' '\n> > \ttest_cmp expect target-repo.git/actual\n> > '\n> > \n> > +test_expect_success 'hook does not get called on packing refs' '\n> > +\t# Pack references first such that we are in a known state.\n> > +\tgit pack-refs --all &&\n> > +\n> > +\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n> > +\t\techo \"$@\" >>actual\n> > +\t\tcat >>actual\n> > +\tEOF\n> > +\trm -f actual &&\n> > +\n> > +\tgit update-ref refs/heads/unpacked-ref $POST_OID &&\n> > +\tgit pack-refs --all &&\n> > +\n> > +\t# We only expect a single hook invocation, which is the call to\n> > +\t# git-update-ref(1). But currently, packing refs will also trigger the\n> > +\t# hook.\n> > +\tcat >expect <<-EOF &&\n> > +\t\tprepared\n> > +\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> > +\t\tcommitted\n> > +\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> > +\t\tprepared\n> > +\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> > +\t\tcommitted\n> > +\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> > +\t\tprepared\n> > +\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n> > +\t\tcommitted\n> > +\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n> > +\tEOF\n> > +\n> > +\ttest_cmp expect actual\n> > +'\n> > +\n> > +test_expect_success 'deleting packed ref calls hook once' '\n> > +\t# Create a reference and pack it.\n> > +\tgit update-ref refs/heads/to-be-deleted $POST_OID &&\n> > +\tgit pack-refs --all &&\n> > +\n> > +\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n> > +\t\techo \"$@\" >>actual\n> > +\t\tcat >>actual\n> > +\tEOF\n> > +\trm -f actual &&\n> > +\n> > +\tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n> > +\n> > +\t# We only expect a single hook invocation, which is the logical\n> > +\t# deletion. But currently, we see two interleaving transactions, once\n> > +\t# for deleting the loose refs and once for deleting the packed ref.\n> > +\tcat >expect <<-EOF &&\n> > +\t\tprepared\n> > +\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n> > +\t\tprepared\n> > +\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n> > +\t\tcommitted\n> > +\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n> > +\t\tcommitted\n> > +\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n> > +\tEOF\n> > +\n> > +\ttest_cmp expect actual\n> > +'\n> > +\n> > test_done\n"},{"id":"445828","messageId":"YdwyFmqs0MhU0wA6@ncase","threadId":"57044","inReplyTo":"xmqq1r1j3s0v.fsf@gitster.g","subject":"Re: [PATCH v2 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-10T13:18:14Z","receivedAt":"2022-01-10T13:18:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 07, 2022 at 06:01:04PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > When deleting refs from the loose-files refs backend, then we need to be\n> > careful to also delete the same ref from the packed refs backend, if it\n> > exists. If we don't, then deleting the loose ref would \"uncover\" the\n> > packed ref. We thus always have to queue up deletions of refs for both\n> > the loose and the packed refs backend. This is done in two separate\n> > transactions, where the end result is that the reference-transaction\n> > hook is executed twice for the deleted refs.\n> >\n> > This behaviour is quite misleading: it's exposing implementation details\n> > of how the files backend works to the user, in contrast to the logical\n> > updates that we'd really want to expose via the hook. Worse yet, whether\n> > the hook gets executed once or twice depends on how well-packed the\n> > repository is: if the ref only exists as a loose ref, then we execute it\n> > once, otherwise if it is also packed then we execute it twice.\n> \n> If the ref only exists as a packed ref, what happens? ...\n> \n> > Fix this behaviour and don't execute the reference-transaction hook at\n> > all when refs in the packed-refs backend if it's driven by the files\n> > backend.\n> \n> ... We try to remove from the loose backend, which would say \"nah,\n> it did not exist in my store\". I am not sure if it should execute\n> the delete hook in such a case for the ref. But if it does not, not\n> running the hook in the ref transaction for packed backend driven by\n> the loose backend would mean nobody notifies the deletion of the\n> ref, no?\n\nThis is shown in the test I've added, \"deleting packed ref calls hook\nonce\". We create a new reference and pack it such that it doesn't exist\nas loose ref anymore, but only as a packed one. Updating that ref\nwould've caused us to execute the hook twice before, once via the\npacked-backend and once via the loose-backend. With my fix we only\nexecute it once via the loose-backend, even if it doesn't currently know\nit. This works because the loose-backend has to create a lock for the\nnonexistent reference such that no concurrent call touches the same ref.\n\n> To me, it seems that the only case this strategy would work\n> correctly is when the files backend calls deletion hook for a\n> request to delete nonexistent ref, which by itself sounds like a\n> problem.\n\nIt does so only if the ref exists in either the loose or packed backend\nthough. If trying to update a ref which exists in neither of those, then\nthe reference transaction would fail with an \"unable to resolve\nreference\" error in `lock_raw_ref()`.\n\nSo this should behave as expected: deleting a packed ref calls the hook\nonce, deleting a nonexistent ref fails and doesn't call the hook at all.\n\nPatrick\n\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > refs/files-backend.c | 9 ++++++---\n> > t/t1416-ref-transaction-hooks.sh | 7 +------\n> > 2 files changed, 7 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index 3c0f3027fe..9a20cb8fa8 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -1261,7 +1261,8 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> > \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n> > \t\tgoto error;\n> > \n> > -\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n> > +\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n> > +\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> > \tif (!transaction)\n> > \t\tgoto error;\n> > \n> > @@ -2775,7 +2776,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> > \t\t\t */\n> > \t\t\tif (!packed_transaction) {\n> > \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n> > -\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n> > +\t\t\t\t\t\trefs->packed_ref_store,\n> > +\t\t\t\t\t\tREF_TRANSACTION_SKIP_HOOK, err);\n> > \t\t\t\tif (!packed_transaction) {\n> > \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n> > \t\t\t\t\tgoto cleanup;\n> > @@ -3046,7 +3048,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n> > \t\t\t\t &affected_refnames))\n> > \t\tBUG(\"initial ref transaction called with existing refs\");\n> > \n> > -\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n> > +\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n> > +\t\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, err);\n> > \tif (!packed_transaction) {\n> > \t\tret = TRANSACTION_GENERIC_ERROR;\n> > \t\tgoto cleanup;\n> > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> > index f9d3d5213f..4e1e84a91f 100755\n> > --- a/t/t1416-ref-transaction-hooks.sh\n> > +++ b/t/t1416-ref-transaction-hooks.sh\n> > @@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' '\n> > \tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n> > \n> > \t# We only expect a single hook invocation, which is the logical\n> > -\t# deletion. But currently, we see two interleaving transactions, once\n> > -\t# for deleting the loose refs and once for deleting the packed ref.\n> > +\t# deletion.\n> > \tcat >expect <<-EOF &&\n> > -\t\tprepared\n> > -\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n> > \t\tprepared\n> > \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n> > \t\tcommitted\n> > -\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n> > -\t\tcommitted\n> > \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n> > \tEOF\n"},{"id":"445849","messageId":"xmqq4k6b1nfi.fsf@gitster.g","threadId":"57044","inReplyTo":"YdwyFmqs0MhU0wA6@ncase","subject":"Re: [PATCH v2 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-10T18:00:01Z","receivedAt":"2022-01-10T18:00:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> It does so only if the ref exists in either the loose or packed backend\n> though. If trying to update a ref which exists in neither of those, then\n> the reference transaction would fail with an \"unable to resolve\n> reference\" error in `lock_raw_ref()`.\n>\n> So this should behave as expected: deleting a packed ref calls the hook\n> once, deleting a nonexistent ref fails and doesn't call the hook at all.\n\nOK. Your explanation deserves to be in the log message for those\nwho ask questions next time, not lost in the archive in a message\njust answering my question and explaining it only for me. After\nall, that is why I ask questions in my reviews.\n\nThanks.\n\n\n"},{"id":"446076","messageId":"xmqqy23kjxdn.fsf@gitster.g","threadId":"57044","inReplyTo":"0739f085b266e65b423ccc14f70cc00c88744459.1641556319.git.ps@pks.im","subject":"Re: [PATCH v2 1/6] refs: open-code deletion of packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-13T00:24:36Z","receivedAt":"2022-01-13T00:24:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> diff --git a/refs/packed-backend.h b/refs/packed-backend.h\n> index f61a73ec25..5e0dd7d08e 100644\n> --- a/refs/packed-backend.h\n> +++ b/refs/packed-backend.h\n> @@ -27,6 +27,12 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n> void packed_refs_unlock(struct ref_store *ref_store);\n> int packed_refs_is_locked(struct ref_store *ref_store);\n> \n> +int packed_refs_delete_refs(struct ref_store *ref_store,\n> +\t\t\t struct ref_transaction *transaction,\n> +\t\t\t const char *msg,\n> +\t\t\t struct string_list *refnames,\n> +\t\t\t unsigned int flags);\n> +\n> /*\n> * Return true if `transaction` really needs to be carried out against\n> * the specified packed_ref_store, or false if it can be skipped\n\nThis breaks \"make hdr-check\" (which is part of CI -- static-analysis).\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nFrom: Junio C Hamano \nDate: Wed, 12 Jan 2022 16:24:13 -0800\nSubject: [PATCH] fixup! refs: open-code deletion of packed refs\n\n---\n refs/packed-backend.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex 5e0dd7d08e..a2cca5d9a3 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -3,6 +3,7 @@\n \n struct repository;\n struct ref_transaction;\n+struct string_list;\n \n /*\n * Support for storing references in a `packed-refs` file.\n-- \n2.35.0-rc0-185-gedd802a149\n\n\n\n"},{"id":"446099","messageId":"cover.1642054003.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"[PATCH v3 0/6] refs: excessive hook execution with packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-13T06:11:21Z","receivedAt":"2022-01-13T06:11:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of this patch series, which addresses an issue\nwhere the reference-transaction hook is being invoked twice when\ndeleting refs both in the packed-refs and loose-refs file.\n\nThe following things changed in comparison to v2:\n\n - Fixed some typos in commit messages.\n\n - Improved the subject of the first patch to more clearly highlight\n the purpose of it, not only say what the patch does.\n\n - Fixed a missing declaration for `struct string_list`.\n\n - Clarified why the last patch does the right thing even in case the\n ref only exists in the packed-refs backend, and in case it doesn't\n exist in either of the backends.\n\nThanks for your feedback!\n\nPatrick\n\nPatrick Steinhardt (6):\n refs: extract packed_refs_delete_refs() to allow control of\n transaction\n refs: allow passing flags when beginning transactions\n refs: allow skipping the reference-transaction hook\n refs: demonstrate excessive execution of the reference-transaction\n hook\n refs: do not execute reference-transaction hook on packing refs\n refs: skip hooks when deleting uncovered packed refs\n\n refs.c | 11 +++++--\n refs.h | 8 ++++-\n refs/files-backend.c | 25 +++++++++++-----\n refs/packed-backend.c | 30 ++++++++++++++-----\n refs/packed-backend.h | 7 +++++\n refs/refs-internal.h | 1 +\n sequencer.c | 2 +-\n t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++\n 8 files changed, 114 insertions(+), 20 deletions(-)\n\nRange-diff against v2:\n1: 0739f085b2 ! 1: abbc28822b refs: open-code deletion of packed refs\n @@ Metadata\n Author: Patrick Steinhardt \n \n ## Commit message ##\n - refs: open-code deletion of packed refs\n + refs: extract packed_refs_delete_refs() to allow control of transaction\n \n When deleting loose refs, then we also have to delete the refs in the\n packed backend. This is done by calling `refs_delete_refs()`, which\n @@ Commit message\n \n Extract a new function `packed_refs_delete_refs()`, which hosts most of\n the logic to delete refs except for creating the transaction itself.\n - Like this, we can easily create the transactionion in the files backend\n + Like this, we can easily create the transaction in the files backend\n and thus exert more control over it.\n \n Signed-off-by: Patrick Steinhardt \n @@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store\n }\n \n ## refs/packed-backend.h ##\n +@@\n + \n + struct repository;\n + struct ref_transaction;\n ++struct string_list;\n + \n + /*\n + * Support for storing references in a `packed-refs` file.\n @@ refs/packed-backend.h: int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n void packed_refs_unlock(struct ref_store *ref_store);\n int packed_refs_is_locked(struct ref_store *ref_store);\n2: 629be01d50 = 2: 9dd172a757 refs: allow passing flags when beginning transactions\n3: 550d89a323 = 3: be826bae3b refs: allow skipping the reference-transaction hook\n4: b52e59cdac ! 4: 662a6e6244 refs: demonstrate excessive execution of the reference-transaction hook\n @@ Metadata\n ## Commit message ##\n refs: demonstrate excessive execution of the reference-transaction hook\n \n - Add tests which demonstate which demonstrates that we're executing the\n + Add tests which demonstate that we're executing the\n reference-transaction hook too often in some cases, which thus leaks\n implementation details about the reference store's implementation\n itself. Behaviour will be fixed in follow-up commits.\n5: 1539e9711f = 5: d83f309b9c refs: do not execute reference-transaction hook on packing refs\n6: 0fbf68dbf4 ! 6: 279eadc41c refs: skip hooks when deleting uncovered packed refs\n @@ Commit message\n \n Fix this behaviour and don't execute the reference-transaction hook at\n all when refs in the packed-refs backend if it's driven by the files\n - backend.\n + backend. This works as expected even in case the refs to be deleted only\n + exist in the packed-refs backend because the loose-backend always queues\n + refs in its own transaction even if they don't exist such that they can\n + be locked for concurrent creation. And it also does the right thing in\n + case neither of the backends has the ref because that would cause the\n + transaction to fail completely.\n \n Signed-off-by: Patrick Steinhardt \n \n-- \n2.34.1\n\n"},{"id":"446100","messageId":"abbc28822b8fde78976422c775afa83bef76ca6c.1642054003.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642054003.git.ps@pks.im","subject":"[PATCH v3 1/6] refs: extract packed_refs_delete_refs() to allow control of transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-13T06:11:25Z","receivedAt":"2022-01-13T06:11:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When deleting loose refs, then we also have to delete the refs in the\npacked backend. This is done by calling `refs_delete_refs()`, which\nthen uses the packed-backend's logic to delete refs. This doesn't allow\nus to exercise any control over the reference transaction which is being\ncreated in the packed backend, which is required in a subsequent commit.\n\nExtract a new function `packed_refs_delete_refs()`, which hosts most of\nthe logic to delete refs except for creating the transaction itself.\nLike this, we can easily create the transaction in the files backend\nand thus exert more control over it.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 12 +++++++++---\n refs/packed-backend.c | 28 +++++++++++++++++++++-------\n refs/packed-backend.h | 7 +++++++\n 3 files changed, 37 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 90b671025a..5795e54020 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1249,6 +1249,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\tstruct ref_transaction *transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint i, result = 0;\n \n@@ -1258,10 +1259,14 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n-\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\tif (!transaction)\n+\t\tgoto error;\n+\n+\tresult = packed_refs_delete_refs(refs->packed_ref_store,\n+\t\t\t\t\t transaction, msg, refnames, flags);\n+\tif (result)\n \t\tgoto error;\n-\t}\n \n \tpacked_refs_unlock(refs->packed_ref_store);\n \n@@ -1288,6 +1293,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \telse\n \t\terror(_(\"could not delete references: %s\"), err.buf);\n \n+\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn -1;\n }\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 67152c664e..e97d67f932 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\t struct string_list *refnames, unsigned int flags)\n {\n-\tstruct packed_ref_store *refs =\n-\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n-\tstruct string_list_item *item;\n \tint ret;\n \n-\t(void)refs; /* We need the check above, but don't use the variable */\n-\n \tif (!refnames->nr)\n \t\treturn 0;\n \n@@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (!transaction)\n \t\treturn -1;\n \n+\tret = packed_refs_delete_refs(ref_store, transaction,\n+\t\t\t\t msg, refnames, flags);\n+\n+\tref_transaction_free(transaction);\n+\treturn ret;\n+}\n+\n+int packed_refs_delete_refs(struct ref_store *ref_store,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *msg,\n+\t\t\t struct string_list *refnames,\n+\t\t\t unsigned int flags)\n+{\n+\tstruct packed_ref_store *refs =\n+\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\tint ret;\n+\n+\t(void)(refs); /* We need the check above, but don't use the variable */\n+\n \tfor_each_string_list_item(item, refnames) {\n \t\tif (ref_transaction_delete(transaction, item->string, NULL,\n \t\t\t\t\t flags, msg, &err)) {\n@@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t}\n \n \tret = ref_transaction_commit(transaction, &err);\n-\n \tif (ret) {\n \t\tif (refnames->nr == 1)\n \t\t\terror(_(\"could not delete reference %s: %s\"),\n@@ -1563,7 +1578,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\terror(_(\"could not delete references: %s\"), err.buf);\n \t}\n \n-\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn ret;\n }\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex f61a73ec25..a2cca5d9a3 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -3,6 +3,7 @@\n \n struct repository;\n struct ref_transaction;\n+struct string_list;\n \n /*\n * Support for storing references in a `packed-refs` file.\n@@ -27,6 +28,12 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n void packed_refs_unlock(struct ref_store *ref_store);\n int packed_refs_is_locked(struct ref_store *ref_store);\n \n+int packed_refs_delete_refs(struct ref_store *ref_store,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *msg,\n+\t\t\t struct string_list *refnames,\n+\t\t\t unsigned int flags);\n+\n /*\n * Return true if `transaction` really needs to be carried out against\n * the specified packed_ref_store, or false if it can be skipped\n-- \n2.34.1\n\n"},{"id":"446101","messageId":"9dd172a75794729512d1868d80f54a3e962b82df.1642054003.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642054003.git.ps@pks.im","subject":"[PATCH v3 2/6] refs: allow passing flags when beginning transactions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-13T06:11:29Z","receivedAt":"2022-01-13T06:11:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not currently have any flags when creating reference transactions,\nbut we'll add one to disable execution of the reference transaction hook\nin some cases.\n\nAllow passing flags to `ref_store_transaction_begin()` to prepare for\nthis change.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 8 +++++---\n refs.h | 3 ++-\n refs/files-backend.c | 10 +++++-----\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 1 +\n sequencer.c | 2 +-\n 6 files changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4da4996c4d..7415864b62 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -800,7 +800,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\ttransaction = ref_store_transaction_begin(refs, &err);\n+\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname, old_oid,\n \t\t\t\t flags, msg, &err) ||\n@@ -1005,6 +1005,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n }\n \n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_transaction *tr;\n@@ -1012,12 +1013,13 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n \n \tCALLOC_ARRAY(tr, 1);\n \ttr->ref_store = refs;\n+\ttr->flags = flags;\n \treturn tr;\n }\n \n struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n {\n-\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), err);\n+\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);\n }\n \n void ref_transaction_free(struct ref_transaction *transaction)\n@@ -1156,7 +1158,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tt = ref_store_transaction_begin(refs, &err);\n+\tt = ref_store_transaction_begin(refs, 0, &err);\n \tif (!t ||\n \t ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,\n \t\t\t\t &err) ||\ndiff --git a/refs.h b/refs.h\nindex 92360e55a2..31f7bf9642 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -231,7 +231,7 @@ char *repo_default_branch_name(struct repository *r, int quiet);\n * struct strbuf err = STRBUF_INIT;\n * int ret = 0;\n *\n- * transaction = ref_store_transaction_begin(refs, &err);\n+ * transaction = ref_store_transaction_begin(refs, 0, &err);\n * if (!transaction ||\n * ref_transaction_update(...) ||\n * ref_transaction_create(...) ||\n@@ -573,6 +573,7 @@ enum action_on_err {\n * be freed by calling ref_transaction_free().\n */\n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err);\n struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5795e54020..ecf88cee04 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1121,7 +1121,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base, &err);\n+\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1192,7 +1192,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n \tif (!transaction)\n \t\treturn -1;\n \n@@ -1259,7 +1259,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2773,7 +2773,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store, err);\n+\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3044,7 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, err);\n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex e97d67f932..cfebcd0b46 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1535,7 +1535,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t * updates into a single transaction.\n \t */\n \n-\ttransaction = ref_store_transaction_begin(ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(ref_store, 0, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 46a839539e..a0af63f162 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -213,6 +213,7 @@ struct ref_transaction {\n \tsize_t nr;\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n+\tunsigned int flags;\n };\n \n /*\ndiff --git a/sequencer.c b/sequencer.c\nindex 6abd72160c..61edd39d7a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3588,7 +3588,7 @@ static int do_label(struct repository *r, const char *name, int len)\n \tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n \tstrbuf_addf(&msg, \"rebase (label) '%.*s'\", len, name);\n \n-\ttransaction = ref_store_transaction_begin(refs, &err);\n+\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction) {\n \t\terror(\"%s\", err.buf);\n \t\tret = -1;\n-- \n2.34.1\n\n"},{"id":"446102","messageId":"be826bae3b564fe003778ea10cb2e06fc753c21d.1642054003.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642054003.git.ps@pks.im","subject":"[PATCH v3 3/6] refs: allow skipping the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-13T06:11:34Z","receivedAt":"2022-01-13T06:11:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reference-transaction hook is executing whenever we prepare, commit\nor abort a reference transaction. While this is mostly intentional, in\ncase of the files backend we're leaking the implementation detail that\nthe store is in fact a composite store with one loose and one packed\nbackend to the caller. So while we want to execute the hook for all\nlogical updates, executing it for such implementation details is\nunexpected.\n\nPrepare for a fix by adding a new flag which allows to skip execution of\nthe hook.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 3 +++\n refs.h | 5 +++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 7415864b62..526bf5ed97 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2084,6 +2084,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n \tconst char *hook;\n \tint ret = 0, i;\n \n+\tif (transaction->flags & REF_TRANSACTION_SKIP_HOOK)\n+\t\treturn 0;\n+\n \thook = find_hook(\"reference-transaction\");\n \tif (!hook)\n \t\treturn ret;\ndiff --git a/refs.h b/refs.h\nindex 31f7bf9642..d4056f9fe2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -568,6 +568,11 @@ enum action_on_err {\n \tUPDATE_REFS_QUIET_ON_ERR\n };\n \n+/*\n+ * Skip executing the reference-transaction hook.\n+ */\n+#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n+\n /*\n * Begin a reference transaction. The reference transaction must\n * be freed by calling ref_transaction_free().\n-- \n2.34.1\n\n"},{"id":"446103","messageId":"662a6e624420d405d9459cec196e79280bf238a7.1642054003.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642054003.git.ps@pks.im","subject":"[PATCH v3 4/6] refs: demonstrate excessive execution of the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-13T06:11:38Z","receivedAt":"2022-01-13T06:11:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add tests which demonstate that we're executing the\nreference-transaction hook too often in some cases, which thus leaks\nimplementation details about the reference store's implementation\nitself. Behaviour will be fixed in follow-up commits.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 6c941027a8..0567fbdf0b 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' '\n \ttest_cmp expect target-repo.git/actual\n '\n \n+test_expect_success 'hook does not get called on packing refs' '\n+\t# Pack references first such that we are in a known state.\n+\tgit pack-refs --all &&\n+\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$@\" >>actual\n+\t\tcat >>actual\n+\tEOF\n+\trm -f actual &&\n+\n+\tgit update-ref refs/heads/unpacked-ref $POST_OID &&\n+\tgit pack-refs --all &&\n+\n+\t# We only expect a single hook invocation, which is the call to\n+\t# git-update-ref(1). But currently, packing refs will also trigger the\n+\t# hook.\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tprepared\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tprepared\n+\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'deleting packed ref calls hook once' '\n+\t# Create a reference and pack it.\n+\tgit update-ref refs/heads/to-be-deleted $POST_OID &&\n+\tgit pack-refs --all &&\n+\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$@\" >>actual\n+\t\tcat >>actual\n+\tEOF\n+\trm -f actual &&\n+\n+\tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n+\n+\t# We only expect a single hook invocation, which is the logical\n+\t# deletion. But currently, we see two interleaving transactions, once\n+\t# for deleting the loose refs and once for deleting the packed ref.\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tprepared\n+\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tcommitted\n+\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tcommitted\n+\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.34.1\n\n"},{"id":"446104","messageId":"d83f309b9c988d7cad9524ac56c0b4c81e2d863f.1642054003.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642054003.git.ps@pks.im","subject":"[PATCH v3 5/6] refs: do not execute reference-transaction hook on packing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-13T06:11:42Z","receivedAt":"2022-01-13T06:11:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reference-transaction hook is supposed to track logical changes to\nreferences, but it currently also gets executed when packing refs in a\nrepository. This is unexpected and ultimately not all that useful:\npacking refs is not supposed to result in any user-visible change to the\nrefs' state, and it ultimately is an implementation detail of how refs\nstores work.\n\nFix this excessive execution of the hook when packing refs.\n\nReported-by: Waleed Khan \nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 6 ++++--\n t/t1416-ref-transaction-hooks.sh | 11 +----------\n 2 files changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ecf88cee04..3c0f3027fe 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1121,7 +1121,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n+\ttransaction = ref_store_transaction_begin(&refs->base,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1192,7 +1193,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 0567fbdf0b..f9d3d5213f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' '\n \tgit pack-refs --all &&\n \n \t# We only expect a single hook invocation, which is the call to\n-\t# git-update-ref(1). But currently, packing refs will also trigger the\n-\t# hook.\n+\t# git-update-ref(1).\n \tcat >expect <<-EOF &&\n \t\tprepared\n \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n \t\tcommitted\n \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tprepared\n-\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tcommitted\n-\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tprepared\n-\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n-\t\tcommitted\n-\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n \tEOF\n \n \ttest_cmp expect actual\n-- \n2.34.1\n\n"},{"id":"446105","messageId":"279eadc41cda9ceea4c5317d6a4c358c18d50ce9.1642054003.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642054003.git.ps@pks.im","subject":"[PATCH v3 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-13T06:11:46Z","receivedAt":"2022-01-13T06:11:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When deleting refs from the loose-files refs backend, then we need to be\ncareful to also delete the same ref from the packed refs backend, if it\nexists. If we don't, then deleting the loose ref would \"uncover\" the\npacked ref. We thus always have to queue up deletions of refs for both\nthe loose and the packed refs backend. This is done in two separate\ntransactions, where the end result is that the reference-transaction\nhook is executed twice for the deleted refs.\n\nThis behaviour is quite misleading: it's exposing implementation details\nof how the files backend works to the user, in contrast to the logical\nupdates that we'd really want to expose via the hook. Worse yet, whether\nthe hook gets executed once or twice depends on how well-packed the\nrepository is: if the ref only exists as a loose ref, then we execute it\nonce, otherwise if it is also packed then we execute it twice.\n\nFix this behaviour and don't execute the reference-transaction hook at\nall when refs in the packed-refs backend if it's driven by the files\nbackend. This works as expected even in case the refs to be deleted only\nexist in the packed-refs backend because the loose-backend always queues\nrefs in its own transaction even if they don't exist such that they can\nbe locked for concurrent creation. And it also does the right thing in\ncase neither of the backends has the ref because that would cause the\ntransaction to fail completely.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 9 ++++++---\n t/t1416-ref-transaction-hooks.sh | 7 +------\n 2 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3c0f3027fe..9a20cb8fa8 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1261,7 +1261,8 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2775,7 +2776,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n+\t\t\t\t\t\trefs->packed_ref_store,\n+\t\t\t\t\t\tREF_TRANSACTION_SKIP_HOOK, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3046,7 +3048,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex f9d3d5213f..4e1e84a91f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' '\n \tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n \n \t# We only expect a single hook invocation, which is the logical\n-\t# deletion. But currently, we see two interleaving transactions, once\n-\t# for deleting the loose refs and once for deleting the packed ref.\n+\t# deletion.\n \tcat >expect <<-EOF &&\n-\t\tprepared\n-\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n \t\tprepared\n \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n \t\tcommitted\n-\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n-\t\tcommitted\n \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n \tEOF\n \n-- \n2.34.1\n\n"},{"id":"446123","messageId":"220113.86h7a7vlpc.gmgdl@evledraar.gmail.com","threadId":"57044","inReplyTo":"abbc28822b8fde78976422c775afa83bef76ca6c.1642054003.git.ps@pks.im","subject":"Re: [PATCH v3 1/6] refs: extract packed_refs_delete_refs() to allow control of transaction","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T12:43:18Z","receivedAt":"2022-01-13T12:56:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 13 2022, Patrick Steinhardt wrote:\n\n> [[PGP Signed Part:Undecided]]\n> When deleting loose refs, then we also have to delete the refs in the\n> packed backend. This is done by calling `refs_delete_refs()`, which\n> then uses the packed-backend's logic to delete refs. This doesn't allow\n> us to exercise any control over the reference transaction which is being\n> created in the packed backend, which is required in a subsequent commit.\n>\n> Extract a new function `packed_refs_delete_refs()`, which hosts most of\n> the logic to delete refs except for creating the transaction itself.\n> Like this, we can easily create the transaction in the files backend\n> and thus exert more control over it.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/files-backend.c | 12 +++++++++---\n> refs/packed-backend.c | 28 +++++++++++++++++++++-------\n> refs/packed-backend.h | 7 +++++++\n> 3 files changed, 37 insertions(+), 10 deletions(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 90b671025a..5795e54020 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1249,6 +1249,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> {\n> \tstruct files_ref_store *refs =\n> \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n> +\tstruct ref_transaction *transaction = NULL;\n\nnit: \"NULL initialization never needed?\n(re: https://lore.kernel.org/git/220113.86bl0gvtq3.gmgdl@evledraar.gmail.com/)\n\n> \tstruct strbuf err = STRBUF_INIT;\n> \tint i, result = 0;\n> \n> @@ -1258,10 +1259,14 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n> \t\tgoto error;\n> \n> -\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n> -\t\tpacked_refs_unlock(refs->packed_ref_store);\n> +\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n> +\tif (!transaction)\n> +\t\tgoto error;\n> +\n> +\tresult = packed_refs_delete_refs(refs->packed_ref_store,\n> +\t\t\t\t\t transaction, msg, refnames, flags);\n> +\tif (result)\n> \t\tgoto error;\n> -\t}\n> \n> \tpacked_refs_unlock(refs->packed_ref_store);\n> \n> @@ -1288,6 +1293,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> \telse\n> \t\terror(_(\"could not delete references: %s\"), err.buf);\n> \n> +\tref_transaction_free(transaction);\n> \tstrbuf_release(&err);\n> \treturn -1;\n> }\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 67152c664e..e97d67f932 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n> static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n> \t\t\t struct string_list *refnames, unsigned int flags)\n> {\n> -\tstruct packed_ref_store *refs =\n> -\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n> \tstruct strbuf err = STRBUF_INIT;\n> \tstruct ref_transaction *transaction;\n> -\tstruct string_list_item *item;\n> \tint ret;\n> \n> -\t(void)refs; /* We need the check above, but don't use the variable */\n> -\n> \tif (!refnames->nr)\n> \t\treturn 0;\n> \n> @@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n> \tif (!transaction)\n> \t\treturn -1;\n> \n> +\tret = packed_refs_delete_refs(ref_store, transaction,\n> +\t\t\t\t msg, refnames, flags);\n> +\n> +\tref_transaction_free(transaction);\n> +\treturn ret;\n> +}\n> +\n> +int packed_refs_delete_refs(struct ref_store *ref_store,\n> +\t\t\t struct ref_transaction *transaction,\n> +\t\t\t const char *msg,\n> +\t\t\t struct string_list *refnames,\n> +\t\t\t unsigned int flags)\n> +{\n> +\tstruct packed_ref_store *refs =\n> +\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tstruct string_list_item *item;\n> +\tint ret;\n> +\n> +\t(void)(refs); /* We need the check above, but don't use the variable */\n> +\n> \tfor_each_string_list_item(item, refnames) {\n> \t\tif (ref_transaction_delete(transaction, item->string, NULL,\n> \t\t\t\t\t flags, msg, &err)) {\n\nI see you're just moving this code around, but FWIW we can just do this\n(also in the pre-image):\n\n\tint packed_refs_delete_refs(...)\n\t{\n\t\t[declare variables]\n \n /* Assert ref store sanity */\n packed_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\")\n\n [...]\n\t}\n\nNot sure it's good to change it around just for this mostly-move, just a\nnote...\n\n> @@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n> \t}\n> \n> \tret = ref_transaction_commit(transaction, &err);\n> -\n\nStray whitespace change.\n"},{"id":"446124","messageId":"220113.86czkvvlf6.gmgdl@evledraar.gmail.com","threadId":"57044","inReplyTo":"d83f309b9c988d7cad9524ac56c0b4c81e2d863f.1642054003.git.ps@pks.im","subject":"Re: [PATCH v3 5/6] refs: do not execute reference-transaction hook on packing refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T13:00:10Z","receivedAt":"2022-01-13T13:02:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 13 2022, Patrick Steinhardt wrote:\n\n> [[PGP Signed Part:Undecided]]\n> The reference-transaction hook is supposed to track logical changes to\n> references, but it currently also gets executed when packing refs in a\n> repository. This is unexpected and ultimately not all that useful:\n> packing refs is not supposed to result in any user-visible change to the\n> refs' state, and it ultimately is an implementation detail of how refs\n> stores work.\n>\n> Fix this excessive execution of the hook when packing refs.\n>\n> Reported-by: Waleed Khan \n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/files-backend.c | 6 ++++--\n> t/t1416-ref-transaction-hooks.sh | 11 +----------\n> 2 files changed, 5 insertions(+), 12 deletions(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index ecf88cee04..3c0f3027fe 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1121,7 +1121,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n> \tif (check_refname_format(r->name, 0))\n> \t\treturn;\n> \n> -\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n> +\ttransaction = ref_store_transaction_begin(&refs->base,\n> +\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> \tif (!transaction)\n> \t\tgoto cleanup;\n> \tref_transaction_add_update(\n> @@ -1192,7 +1193,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n> \tstruct strbuf err = STRBUF_INIT;\n> \tstruct ref_transaction *transaction;\n> \n> -\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n> +\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n> +\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> \tif (!transaction)\n> \t\treturn -1;\n> \n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> index 0567fbdf0b..f9d3d5213f 100755\n> --- a/t/t1416-ref-transaction-hooks.sh\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> @@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' '\n> \tgit pack-refs --all &&\n> \n> \t# We only expect a single hook invocation, which is the call to\n> -\t# git-update-ref(1). But currently, packing refs will also trigger the\n> -\t# hook.\n> +\t# git-update-ref(1).\n> \tcat >expect <<-EOF &&\n> \t\tprepared\n> \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> \t\tcommitted\n> \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> -\t\tprepared\n> -\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> -\t\tcommitted\n> -\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> -\t\tprepared\n> -\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n> -\t\tcommitted\n> -\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n> \tEOF\n> \n> \ttest_cmp expect actual\n\nI wondered how we'd deal with cases where the loose ref != the\ncorresponding packed ref, but I can't think of ones where it won't be\ninvisible externally, i.e. we'll correctly update the packed-refs and\ndelete that loose ref as part of this transaction.\n\nI do wonder if the docs also need updating, currently they say:\n\n [The hook] executes whenever a reference transaction is prepared,\n committed or aborted[...]\n\nBut now we'll explicitly exclude certain classes of\ntransactions. Perhaps we should expand:\n\n \"The hook does not cover symbolic references (but that may change in\n the future).\"\n\nInto some list of types of changes we intentionally exclude, might\ninclude in the future etc.\n"},{"id":"446125","messageId":"220113.868rvjvkyn.gmgdl@evledraar.gmail.com","threadId":"57044","inReplyTo":"279eadc41cda9ceea4c5317d6a4c358c18d50ce9.1642054003.git.ps@pks.im","subject":"Re: [PATCH v3 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T13:04:38Z","receivedAt":"2022-01-13T13:12:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 13 2022, Patrick Steinhardt wrote:\n\n> [[PGP Signed Part:Undecided]]\n> When deleting refs from the loose-files refs backend, then we need to be\n> careful to also delete the same ref from the packed refs backend, if it\n> exists. If we don't, then deleting the loose ref would \"uncover\" the\n> packed ref. We thus always have to queue up deletions of refs for both\n> the loose and the packed refs backend. This is done in two separate\n> transactions, where the end result is that the reference-transaction\n> hook is executed twice for the deleted refs.\n\nBut do we (which would be an issue before this series) delete the loose\nand and then the packed one, thus racily exposing the stale ref to any\nconcurrent repository reader, or do we first update the packed ref to\nthe valu of the now-locked loose ref to avoid such a race?\n\n> [...]\n> Fix this behaviour and don't execute the reference-transaction hook at\n> all when refs in the packed-refs backend if it's driven by the files\n> backend. This works as expected even in case the refs to be deleted only\n> exist in the packed-refs backend because the loose-backend always queues\n> refs in its own transaction even if they don't exist such that they can\n> be locked for concurrent creation. And it also does the right thing in\n> case neither of the backends has the ref because that would cause the\n> transaction to fail completely.\n\nI do wonder if the fundimental approach here is the right\none. I.e. changing the hook to only expose \"real\" updates, as opposed to\nleaving it as a lower-level facility to listed in on any sort of ref\nupdates.\n\nIn such a scenario we could imagine adding a third parameter or\notherwise flag the update as \"real\" to the hook, so a dumber hook\nconsumer could ignore the more verbose inter-transactional chatter.\n\nI say that because this change does the right thing for the use-case you\nhave in mind, but if you e.g. imagine a more gentle background-friendly\n\"gc\" such a thing could be implemented by backing off as soon as it sees\nan ongoing transaction being started.\n\nWith my ae35e16cd43 (reflog expire: don't lock reflogs using previously\nseen OID, 2021-08-23) not getting that more chatty data should be be OK\nfor such a hypothetical hook.\n\nBut we might have more avoidable tripping over locks as the gc and ref\ntransaction race one another to lock various things in the repository.\n\nOr maybe nobody cares in practice, just food for thought.\n"},{"id":"446152","messageId":"220113.86zgnzu48q.gmgdl@evledraar.gmail.com","threadId":"57044","inReplyTo":"be826bae3b564fe003778ea10cb2e06fc753c21d.1642054003.git.ps@pks.im","subject":"Re: [PATCH v3 3/6] refs: allow skipping the reference-transaction hook","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T13:34:41Z","receivedAt":"2022-01-13T13:58:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 13 2022, Patrick Steinhardt wrote:\n\n> [[PGP Signed Part:Undecided]]\n> The reference-transaction hook is executing whenever we prepare, commit\n> or abort a reference transaction. While this is mostly intentional, in\n> case of the files backend we're leaking the implementation detail that\n> the store is in fact a composite store with one loose and one packed\n> backend to the caller. So while we want to execute the hook for all\n> logical updates, executing it for such implementation details is\n> unexpected.\n>\n> Prepare for a fix by adding a new flag which allows to skip execution of\n> the hook.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs.c | 3 +++\n> refs.h | 5 +++++\n> 2 files changed, 8 insertions(+)\n>\n> diff --git a/refs.c b/refs.c\n> index 7415864b62..526bf5ed97 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2084,6 +2084,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n> \tconst char *hook;\n> \tint ret = 0, i;\n> \n> +\tif (transaction->flags & REF_TRANSACTION_SKIP_HOOK)\n> +\t\treturn 0;\n> +\n> \thook = find_hook(\"reference-transaction\");\n> \tif (!hook)\n> \t\treturn ret;\n> diff --git a/refs.h b/refs.h\n> index 31f7bf9642..d4056f9fe2 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -568,6 +568,11 @@ enum action_on_err {\n> \tUPDATE_REFS_QUIET_ON_ERR\n> };\n> \n> +/*\n> + * Skip executing the reference-transaction hook.\n> + */\n> +#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n> +\n> /*\n> * Begin a reference transaction. The reference transaction must\n> * be freed by calling ref_transaction_free().\n\nThis isn't needed in refs.h, so let's put it in refs-internal.h where\ne.g. \"enum ref_transaction_state\" now lives:\n \n diff --git a/refs.h b/refs.h\n index d4056f9fe26..31f7bf96424 100644\n --- a/refs.h\n +++ b/refs.h\n @@ -568,11 +568,6 @@ enum action_on_err {\n UPDATE_REFS_QUIET_ON_ERR\n };\n \n -/*\n - * Skip executing the reference-transaction hook.\n - */\n -#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n -\n /*\n * Begin a reference transaction. The reference transaction must\n * be freed by calling ref_transaction_free().\n diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n index a0af63f162f..87da39243f7 100644\n --- a/refs/refs-internal.h\n +++ b/refs/refs-internal.h\n @@ -201,6 +201,11 @@ enum ref_transaction_state {\n REF_TRANSACTION_CLOSED = 2\n };\n \n +/*\n + * Skip executing the reference-transaction hook.\n + */\n +#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n +\n /*\n * Data structure for holding a reference transaction, which can\n * consist of checks and updates to multiple references, carried out\n\nA bit more odd is that this series ends up with a\nref_transaction_begin() that doesn't correspond to its ref_store_*()\nparent, i.e. the others pass the ref store for you, but now we omit the\nflags too.\n\nI see why you did that, to avoid tweaking every existing\nref_transaction_begin() caller.\n\nBut isn't something like the below a better approach? We can introduce a\nrefs-internal.h-only flag enum, and then just have a new\nref_store_transaction_begin_no_hook() called from these new\nfiles-backend.c users.\n\nThe diff on top is a bit verbose, but the exposed API is cleaner\n(presumably no \"public\" user should be allowed to skip the hook), and\nthe overall diff if this is squashed in is smaller.\n\ndiff --git a/refs.c b/refs.c\nindex 526bf5ed97a..ffaf34e9710 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -800,7 +800,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs, &err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname, old_oid,\n \t\t\t\t flags, msg, &err) ||\n@@ -1004,22 +1004,36 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n \treturn 1;\n }\n \n-struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n-\t\t\t\t\t\t unsigned int flags,\n-\t\t\t\t\t\t struct strbuf *err)\n+static struct ref_transaction *ref_store_transaction_begin_1(struct ref_store *refs,\n+\t\t\t\t\t\t\t unsigned int no_hook,\n+\t\t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_transaction *tr;\n \tassert(err);\n \n \tCALLOC_ARRAY(tr, 1);\n \ttr->ref_store = refs;\n-\ttr->flags = flags;\n+\tif (no_hook)\n+\t\ttr->flags &= REF_TRANSACTION_SKIP_HOOK;\n \treturn tr;\n }\n \n struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n {\n-\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);\n+\treturn ref_store_transaction_begin_1(get_main_ref_store(the_repository),\n+\t\t\t\t\t 0, err);\n+}\n+\n+struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t struct strbuf *err)\n+{\n+\treturn ref_store_transaction_begin_1(refs, 0, err);\n+}\n+\n+struct ref_transaction *ref_store_transaction_begin_no_hook(struct ref_store *refs,\n+\t\t\t\t\t\t\t struct strbuf *err)\n+{\n+\treturn ref_store_transaction_begin_1(refs, 1, err);\n }\n \n void ref_transaction_free(struct ref_transaction *transaction)\n@@ -1158,7 +1172,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tt = ref_store_transaction_begin(refs, 0, &err);\n+\tt = ref_store_transaction_begin(refs, &err);\n \tif (!t ||\n \t ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,\n \t\t\t\t &err) ||\ndiff --git a/refs.h b/refs.h\nindex 31f7bf96424..3663e0347ff 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -573,8 +573,9 @@ enum action_on_err {\n * be freed by calling ref_transaction_free().\n */\n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n-\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err);\n+struct ref_transaction *ref_store_transaction_begin_no_hook(struct ref_store *refs,\n+\t\t\t\t\t\t\t struct strbuf *err);\n struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n \n /*\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 9a20cb8fa89..f85c8f3a692 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1121,8 +1121,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base,\n-\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n+\ttransaction = ref_store_transaction_begin_no_hook(&refs->base, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1193,8 +1192,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n-\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n+\ttransaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, &err);\n \tif (!transaction)\n \t\treturn -1;\n \n@@ -1261,8 +1259,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n-\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n+\ttransaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2775,9 +2772,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t * packed-refs if it exists there.\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n-\t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store,\n-\t\t\t\t\t\tREF_TRANSACTION_SKIP_HOOK, err);\n+\t\t\t\tpacked_transaction = ref_store_transaction_begin_no_hook(\n+\t\t\t\t\t\trefs->packed_ref_store, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3048,8 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n-\t\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, err);\n+\tpacked_transaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex cfebcd0b46e..e97d67f9321 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1535,7 +1535,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t * updates into a single transaction.\n \t */\n \n-\ttransaction = ref_store_transaction_begin(ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(ref_store, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 87da39243f7..8ff9e0e6e3a 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -204,7 +204,9 @@ enum ref_transaction_state {\n /*\n * Skip executing the reference-transaction hook.\n */\n-#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n+enum ref_transaction_flags {\n+\tREF_TRANSACTION_SKIP_HOOK = 1 << 0,\n+};\n \n /*\n * Data structure for holding a reference transaction, which can\n@@ -218,7 +220,7 @@ struct ref_transaction {\n \tsize_t nr;\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n-\tunsigned int flags;\n+\tenum ref_transaction_flags flags;\n };\n \n /*\ndiff --git a/sequencer.c b/sequencer.c\nindex 61edd39d7a4..6abd72160cc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3588,7 +3588,7 @@ static int do_label(struct repository *r, const char *name, int len)\n \tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n \tstrbuf_addf(&msg, \"rebase (label) '%.*s'\", len, name);\n \n-\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs, &err);\n \tif (!transaction) {\n \t\terror(\"%s\", err.buf);\n \t\tret = -1;\n"},{"id":"446171","messageId":"CAFQ2z_P_SiaDbSQdAt5mSp79BnC0-k5B+X09oCVh+w--BO4_6w@mail.gmail.com","threadId":"57044","inReplyTo":"xmqqiluv6vim.fsf@gitster.g","subject":"Re: [PATCH v2 0/6] refs: excessive hook execution with packed refs","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-01-13T18:24:19Z","receivedAt":"2022-01-13T18:24:33Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Jan 7, 2022 at 11:17 PM Junio C Hamano wrote:\n> > this is a resend of version 1 of this patch series to hopefully entice\n> > some reviews. The only change is that v2 is rebased onto the current\n> > main branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The\n> > following was from the orignial cover letter:\n>\n> I'll add Ævar, who has been making a lot of changes to the refs\n> subsystem, and Han-Wen, whose work to add a new ref backend may need\n> to interact with this change, as possible stake-holders to the CC list.\n\nThanks for the consideration, Jun. As the hook is called from refs.c,\nit should not make a difference for reftable.\n\nI looked over the patches. I didn't look at the bottom change to\npacked/loose refs as I'm not an expert.\n\nThe individual transaction updates already support their own flags, so\nthis change generates some confusion. Consider:\n\nint refs_delete_ref(struct ref_store *refs, const char *msg,\n const char *refname,\n const struct object_id *old_oid,\n unsigned int flags)\n\nhow would one delete a ref skipping the transaction hook? It will be\neasy for someone to pass the SKIP_TRANSACTION_HOOK to\nrefs_delete_ref(), with surprising results.\n\nIt might make sense to not introduce a new flag namespace, but use\nupdate->flags instead. You'd have to add your new flag after\nREF_SKIP_REFNAME_VERIFICATION.\nBonus is that you can unittest the new flag using the existing\nref-store helper without extra work. (check that a transaction with &\nwithout the flag works as expected.)\n\nother than that, looks OK to me.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"446339","messageId":"YeUYNhJemN/hRG+6@ncase","threadId":"57044","inReplyTo":"CAFQ2z_P_SiaDbSQdAt5mSp79BnC0-k5B+X09oCVh+w--BO4_6w@mail.gmail.com","subject":"Re: [PATCH v2 0/6] refs: excessive hook execution with packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T07:18:14Z","receivedAt":"2022-01-17T07:18:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 13, 2022 at 07:24:19PM +0100, Han-Wen Nienhuys wrote:\n> On Fri, Jan 7, 2022 at 11:17 PM Junio C Hamano wrote:\n> > > this is a resend of version 1 of this patch series to hopefully entice\n> > > some reviews. The only change is that v2 is rebased onto the current\n> > > main branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The\n> > > following was from the orignial cover letter:\n> >\n> > I'll add Ævar, who has been making a lot of changes to the refs\n> > subsystem, and Han-Wen, whose work to add a new ref backend may need\n> > to interact with this change, as possible stake-holders to the CC list.\n> \n> Thanks for the consideration, Jun. As the hook is called from refs.c,\n> it should not make a difference for reftable.\n> \n> I looked over the patches. I didn't look at the bottom change to\n> packed/loose refs as I'm not an expert.\n> \n> The individual transaction updates already support their own flags, so\n> this change generates some confusion. Consider:\n> \n> int refs_delete_ref(struct ref_store *refs, const char *msg,\n> const char *refname,\n> const struct object_id *old_oid,\n> unsigned int flags)\n> \n> how would one delete a ref skipping the transaction hook? It will be\n> easy for someone to pass the SKIP_TRANSACTION_HOOK to\n> refs_delete_ref(), with surprising results.\n> \n> It might make sense to not introduce a new flag namespace, but use\n> update->flags instead. You'd have to add your new flag after\n> REF_SKIP_REFNAME_VERIFICATION.\n> Bonus is that you can unittest the new flag using the existing\n> ref-store helper without extra work. (check that a transaction with &\n> without the flag works as expected.)\n> \n> other than that, looks OK to me.\n\nThanks for your feedback!\n\nIn fact the first version I had locally did exactly that. But I found\nthe end version result harder to reason about, most importantly because\nit's not a 100% clear to the reader whether all callsites which delete\nrefs in the packed-backend via the files-backend are adapted or whether\nany of the callsites was missing. By having the flag in a central place\nit's immediately clear that the hook won't be run at all, which is\nexactly what we want here.\n\nPatrick\n"},{"id":"446341","messageId":"YeUcdorVFY7w9qR2@ncase","threadId":"57044","inReplyTo":"220113.86h7a7vlpc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 1/6] refs: extract packed_refs_delete_refs() to allow control of transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T07:36:22Z","receivedAt":"2022-01-17T07:36:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 13, 2022 at 01:43:18PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Jan 13 2022, Patrick Steinhardt wrote:\n> \n> > [[PGP Signed Part:Undecided]]\n> > When deleting loose refs, then we also have to delete the refs in the\n> > packed backend. This is done by calling `refs_delete_refs()`, which\n> > then uses the packed-backend's logic to delete refs. This doesn't allow\n> > us to exercise any control over the reference transaction which is being\n> > created in the packed backend, which is required in a subsequent commit.\n> >\n> > Extract a new function `packed_refs_delete_refs()`, which hosts most of\n> > the logic to delete refs except for creating the transaction itself.\n> > Like this, we can easily create the transaction in the files backend\n> > and thus exert more control over it.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > refs/files-backend.c | 12 +++++++++---\n> > refs/packed-backend.c | 28 +++++++++++++++++++++-------\n> > refs/packed-backend.h | 7 +++++++\n> > 3 files changed, 37 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index 90b671025a..5795e54020 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -1249,6 +1249,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> > {\n> > \tstruct files_ref_store *refs =\n> > \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n> > +\tstruct ref_transaction *transaction = NULL;\n> \n> nit: \"NULL initialization never needed?\n> (re: https://lore.kernel.org/git/220113.86bl0gvtq3.gmgdl@evledraar.gmail.com/)\n\nIt is needed: if locking the packed-refs file fails we `goto error;` and\ntry to free the transaction without it having been initialized yet.\n\n> > \tstruct strbuf err = STRBUF_INIT;\n> > \tint i, result = 0;\n> > \n> > @@ -1258,10 +1259,14 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> > \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n> > \t\tgoto error;\n> > \n> > -\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n> > -\t\tpacked_refs_unlock(refs->packed_ref_store);\n> > +\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n> > +\tif (!transaction)\n> > +\t\tgoto error;\n> > +\n> > +\tresult = packed_refs_delete_refs(refs->packed_ref_store,\n> > +\t\t\t\t\t transaction, msg, refnames, flags);\n> > +\tif (result)\n> > \t\tgoto error;\n> > -\t}\n> > \n> > \tpacked_refs_unlock(refs->packed_ref_store);\n> > \n> > @@ -1288,6 +1293,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> > \telse\n> > \t\terror(_(\"could not delete references: %s\"), err.buf);\n> > \n> > +\tref_transaction_free(transaction);\n> > \tstrbuf_release(&err);\n> > \treturn -1;\n> > }\n> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> > index 67152c664e..e97d67f932 100644\n> > --- a/refs/packed-backend.c\n> > +++ b/refs/packed-backend.c\n> > @@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n> > static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n> > \t\t\t struct string_list *refnames, unsigned int flags)\n> > {\n> > -\tstruct packed_ref_store *refs =\n> > -\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n> > \tstruct strbuf err = STRBUF_INIT;\n> > \tstruct ref_transaction *transaction;\n> > -\tstruct string_list_item *item;\n> > \tint ret;\n> > \n> > -\t(void)refs; /* We need the check above, but don't use the variable */\n> > -\n> > \tif (!refnames->nr)\n> > \t\treturn 0;\n> > \n> > @@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n> > \tif (!transaction)\n> > \t\treturn -1;\n> > \n> > +\tret = packed_refs_delete_refs(ref_store, transaction,\n> > +\t\t\t\t msg, refnames, flags);\n> > +\n> > +\tref_transaction_free(transaction);\n> > +\treturn ret;\n> > +}\n> > +\n> > +int packed_refs_delete_refs(struct ref_store *ref_store,\n> > +\t\t\t struct ref_transaction *transaction,\n> > +\t\t\t const char *msg,\n> > +\t\t\t struct string_list *refnames,\n> > +\t\t\t unsigned int flags)\n> > +{\n> > +\tstruct packed_ref_store *refs =\n> > +\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n> > +\tstruct strbuf err = STRBUF_INIT;\n> > +\tstruct string_list_item *item;\n> > +\tint ret;\n> > +\n> > +\t(void)(refs); /* We need the check above, but don't use the variable */\n> > +\n> > \tfor_each_string_list_item(item, refnames) {\n> > \t\tif (ref_transaction_delete(transaction, item->string, NULL,\n> > \t\t\t\t\t flags, msg, &err)) {\n> \n> I see you're just moving this code around, but FWIW we can just do this\n> (also in the pre-image):\n> \n> \tint packed_refs_delete_refs(...)\n> \t{\n> \t\t[declare variables]\n> \n> /* Assert ref store sanity */\n> packed_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\")\n> \n> [...]\n> \t}\n> \n> Not sure it's good to change it around just for this mostly-move, just a\n> note...\n\nI think this change is trivial enough to make while at it.\n\nPatrick\n\n> > @@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n> > \t}\n> > \n> > \tret = ref_transaction_commit(transaction, &err);\n> > -\n> \n> Stray whitespace change.\n"},{"id":"446342","messageId":"YeUeTAlYypptDUBW@ncase","threadId":"57044","inReplyTo":"220113.86czkvvlf6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 5/6] refs: do not execute reference-transaction hook on packing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T07:44:12Z","receivedAt":"2022-01-17T07:44:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 13, 2022 at 02:00:10PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Jan 13 2022, Patrick Steinhardt wrote:\n> \n> > [[PGP Signed Part:Undecided]]\n> > The reference-transaction hook is supposed to track logical changes to\n> > references, but it currently also gets executed when packing refs in a\n> > repository. This is unexpected and ultimately not all that useful:\n> > packing refs is not supposed to result in any user-visible change to the\n> > refs' state, and it ultimately is an implementation detail of how refs\n> > stores work.\n> >\n> > Fix this excessive execution of the hook when packing refs.\n> >\n> > Reported-by: Waleed Khan \n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > refs/files-backend.c | 6 ++++--\n> > t/t1416-ref-transaction-hooks.sh | 11 +----------\n> > 2 files changed, 5 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index ecf88cee04..3c0f3027fe 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -1121,7 +1121,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n> > \tif (check_refname_format(r->name, 0))\n> > \t\treturn;\n> > \n> > -\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n> > +\ttransaction = ref_store_transaction_begin(&refs->base,\n> > +\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> > \tif (!transaction)\n> > \t\tgoto cleanup;\n> > \tref_transaction_add_update(\n> > @@ -1192,7 +1193,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n> > \tstruct strbuf err = STRBUF_INIT;\n> > \tstruct ref_transaction *transaction;\n> > \n> > -\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n> > +\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n> > +\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> > \tif (!transaction)\n> > \t\treturn -1;\n> > \n> > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> > index 0567fbdf0b..f9d3d5213f 100755\n> > --- a/t/t1416-ref-transaction-hooks.sh\n> > +++ b/t/t1416-ref-transaction-hooks.sh\n> > @@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' '\n> > \tgit pack-refs --all &&\n> > \n> > \t# We only expect a single hook invocation, which is the call to\n> > -\t# git-update-ref(1). But currently, packing refs will also trigger the\n> > -\t# hook.\n> > +\t# git-update-ref(1).\n> > \tcat >expect <<-EOF &&\n> > \t\tprepared\n> > \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> > \t\tcommitted\n> > \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> > -\t\tprepared\n> > -\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> > -\t\tcommitted\n> > -\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n> > -\t\tprepared\n> > -\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n> > -\t\tcommitted\n> > -\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n> > \tEOF\n> > \n> > \ttest_cmp expect actual\n> \n> I wondered how we'd deal with cases where the loose ref != the\n> corresponding packed ref, but I can't think of ones where it won't be\n> invisible externally, i.e. we'll correctly update the packed-refs and\n> delete that loose ref as part of this transaction.\n\nWith the previous code we'd see two hook executions with different old\nOIDs. Given that we only care about logical updates though the user'd\nonly want to see the one which deletes the user-visible OID, which is\nwhat's stored in the loose ref. And with the fixes in this series that's\nthe hook invocation we retain.\n\n> I do wonder if the docs also need updating, currently they say:\n> \n> [The hook] executes whenever a reference transaction is prepared,\n> committed or aborted[...]\n> \n> But now we'll explicitly exclude certain classes of\n> transactions. Perhaps we should expand:\n> \n> \"The hook does not cover symbolic references (but that may change in\n> the future).\"\n> \n> Into some list of types of changes we intentionally exclude, might\n> include in the future etc.\n\nWell, from the user's perspective we do execute the hook whenever we\ndrive a reference transaction: all modifications to the files backend\nare still visible to the hook after the changes in this series. The\nissue is that with the files backend being a combination of two\nbackends, we essentially saw a subset of refs executing the hook twice,\nwhich really is an implementation detail.\n\nPatrick\n"},{"id":"446344","messageId":"YeUhMPSlC/X6HRBF@ncase","threadId":"57044","inReplyTo":"220113.868rvjvkyn.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T07:56:32Z","receivedAt":"2022-01-17T07:56:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 13, 2022 at 02:04:38PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Jan 13 2022, Patrick Steinhardt wrote:\n> \n> > [[PGP Signed Part:Undecided]]\n> > When deleting refs from the loose-files refs backend, then we need to be\n> > careful to also delete the same ref from the packed refs backend, if it\n> > exists. If we don't, then deleting the loose ref would \"uncover\" the\n> > packed ref. We thus always have to queue up deletions of refs for both\n> > the loose and the packed refs backend. This is done in two separate\n> > transactions, where the end result is that the reference-transaction\n> > hook is executed twice for the deleted refs.\n> \n> But do we (which would be an issue before this series) delete the loose\n> and and then the packed one, thus racily exposing the stale ref to any\n> concurrent repository reader, or do we first update the packed ref to\n> the valu of the now-locked loose ref to avoid such a race?\n\nWe first commit the packed-refs file so that the stale ref is not\nexposed.\n\n> > [...]\n> > Fix this behaviour and don't execute the reference-transaction hook at\n> > all when refs in the packed-refs backend if it's driven by the files\n> > backend. This works as expected even in case the refs to be deleted only\n> > exist in the packed-refs backend because the loose-backend always queues\n> > refs in its own transaction even if they don't exist such that they can\n> > be locked for concurrent creation. And it also does the right thing in\n> > case neither of the backends has the ref because that would cause the\n> > transaction to fail completely.\n> \n> I do wonder if the fundimental approach here is the right\n> one. I.e. changing the hook to only expose \"real\" updates, as opposed to\n> leaving it as a lower-level facility to listed in on any sort of ref\n> updates.\n> \n> In such a scenario we could imagine adding a third parameter or\n> otherwise flag the update as \"real\" to the hook, so a dumber hook\n> consumer could ignore the more verbose inter-transactional chatter.\n> \n> I say that because this change does the right thing for the use-case you\n> have in mind, but if you e.g. imagine a more gentle background-friendly\n> \"gc\" such a thing could be implemented by backing off as soon as it sees\n> an ongoing transaction being started.\n\nI've mostly been acting on the original report by Waleed. And I tend to\nagree with his report given that we also got a workaround at GitLab\nwhich filters out reference transactions which only consist of force\ndeletions because they're likely to be pruning refs in the packed\nbackend which are about to be uncovered. The result is that execution of\nthe reftx hook is dependent on how well-packed a repository's refs are:\nwhen refs are packed we execute the hook twice, whereas we execute it\nonce when it's not well-packed. This is surprising behaviour, even\nthough one can definitely argue that it's just working as intended.\n\nI think ultimately the question boils down to whether we want to treat\nthe files backend as a single compound backend and whether the reftx\nhook should treat it like that. If we treat it as a single backend, then\nwe shouldn't report a change in refs when pruning about-to-be-uncovered\nrefs given that it wouldn't have been visible, but it's only internal\ncleanup. And neither should we report ref changes when repacking refs\ninto a single file given that from the backend's perspective nothing is\nabout to change.\n\nPatrick\n\n> With my ae35e16cd43 (reflog expire: don't lock reflogs using previously\n> seen OID, 2021-08-23) not getting that more chatty data should be be OK\n> for such a hypothetical hook.\n> \n> But we might have more avoidable tripping over locks as the gc and ref\n> transaction race one another to lock various things in the repository.\n> \n> Or maybe nobody cares in practice, just food for thought.\n"},{"id":"446345","messageId":"YeUi77PE6MBwTBVQ@ncase","threadId":"57044","inReplyTo":"220113.86zgnzu48q.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 3/6] refs: allow skipping the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T08:03:59Z","receivedAt":"2022-01-17T08:04:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 13, 2022 at 02:34:41PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Jan 13 2022, Patrick Steinhardt wrote:\n> \n> > [[PGP Signed Part:Undecided]]\n> > The reference-transaction hook is executing whenever we prepare, commit\n> > or abort a reference transaction. While this is mostly intentional, in\n> > case of the files backend we're leaking the implementation detail that\n> > the store is in fact a composite store with one loose and one packed\n> > backend to the caller. So while we want to execute the hook for all\n> > logical updates, executing it for such implementation details is\n> > unexpected.\n> >\n> > Prepare for a fix by adding a new flag which allows to skip execution of\n> > the hook.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > refs.c | 3 +++\n> > refs.h | 5 +++++\n> > 2 files changed, 8 insertions(+)\n> >\n> > diff --git a/refs.c b/refs.c\n> > index 7415864b62..526bf5ed97 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -2084,6 +2084,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n> > \tconst char *hook;\n> > \tint ret = 0, i;\n> > \n> > +\tif (transaction->flags & REF_TRANSACTION_SKIP_HOOK)\n> > +\t\treturn 0;\n> > +\n> > \thook = find_hook(\"reference-transaction\");\n> > \tif (!hook)\n> > \t\treturn ret;\n> > diff --git a/refs.h b/refs.h\n> > index 31f7bf9642..d4056f9fe2 100644\n> > --- a/refs.h\n> > +++ b/refs.h\n> > @@ -568,6 +568,11 @@ enum action_on_err {\n> > \tUPDATE_REFS_QUIET_ON_ERR\n> > };\n> > \n> > +/*\n> > + * Skip executing the reference-transaction hook.\n> > + */\n> > +#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n> > +\n> > /*\n> > * Begin a reference transaction. The reference transaction must\n> > * be freed by calling ref_transaction_free().\n> \n> This isn't needed in refs.h, so let's put it in refs-internal.h where\n> e.g. \"enum ref_transaction_state\" now lives:\n\nOn the other hand we got the flags parameter in the function just two\nlines later. A reader would thus wonder \"which flags?!\" if we didn't\ncarry the definition of flags nearby and would have to go search the\ncodebase for the set of supported flags.\n\n> diff --git a/refs.h b/refs.h\n> index d4056f9fe26..31f7bf96424 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -568,11 +568,6 @@ enum action_on_err {\n> UPDATE_REFS_QUIET_ON_ERR\n> };\n> \n> -/*\n> - * Skip executing the reference-transaction hook.\n> - */\n> -#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n> -\n> /*\n> * Begin a reference transaction. The reference transaction must\n> * be freed by calling ref_transaction_free().\n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index a0af63f162f..87da39243f7 100644\n> --- a/refs/refs-internal.h\n> +++ b/refs/refs-internal.h\n> @@ -201,6 +201,11 @@ enum ref_transaction_state {\n> REF_TRANSACTION_CLOSED = 2\n> };\n> \n> +/*\n> + * Skip executing the reference-transaction hook.\n> + */\n> +#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n> +\n> /*\n> * Data structure for holding a reference transaction, which can\n> * consist of checks and updates to multiple references, carried out\n> \n> A bit more odd is that this series ends up with a\n> ref_transaction_begin() that doesn't correspond to its ref_store_*()\n> parent, i.e. the others pass the ref store for you, but now we omit the\n> flags too.\n> \n> I see why you did that, to avoid tweaking every existing\n> ref_transaction_begin() caller.\n> \n> But isn't something like the below a better approach? We can introduce a\n> refs-internal.h-only flag enum, and then just have a new\n> ref_store_transaction_begin_no_hook() called from these new\n> files-backend.c users.\n> \n> The diff on top is a bit verbose, but the exposed API is cleaner\n> (presumably no \"public\" user should be allowed to skip the hook), and\n> the overall diff if this is squashed in is smaller.\n\nWhile the diff does look sensible it makes me wonder how maintainable it\nis in the future. If we ever were to accept another flag then we might\nhave to duplicate your `ref_store_transaction_begin_no_hook()` to also\ncarry flags, or add `ref_store_transaciton_begin_another_option()` to\nstay consistent.\n\nSo for now I tend to prefer the current version given that it feels more\nextensible going forward. But I'm happy to change it if others agree\nwith you.\n\nPatrick\n\n> diff --git a/refs.c b/refs.c\n> index 526bf5ed97a..ffaf34e9710 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -800,7 +800,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n> \tstruct ref_transaction *transaction;\n> \tstruct strbuf err = STRBUF_INIT;\n> \n> -\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n> +\ttransaction = ref_store_transaction_begin(refs, &err);\n> \tif (!transaction ||\n> \t ref_transaction_delete(transaction, refname, old_oid,\n> \t\t\t\t flags, msg, &err) ||\n> @@ -1004,22 +1004,36 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n> \treturn 1;\n> }\n> \n> -struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n> -\t\t\t\t\t\t unsigned int flags,\n> -\t\t\t\t\t\t struct strbuf *err)\n> +static struct ref_transaction *ref_store_transaction_begin_1(struct ref_store *refs,\n> +\t\t\t\t\t\t\t unsigned int no_hook,\n> +\t\t\t\t\t\t\t struct strbuf *err)\n> {\n> \tstruct ref_transaction *tr;\n> \tassert(err);\n> \n> \tCALLOC_ARRAY(tr, 1);\n> \ttr->ref_store = refs;\n> -\ttr->flags = flags;\n> +\tif (no_hook)\n> +\t\ttr->flags &= REF_TRANSACTION_SKIP_HOOK;\n> \treturn tr;\n> }\n> \n> struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n> {\n> -\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);\n> +\treturn ref_store_transaction_begin_1(get_main_ref_store(the_repository),\n> +\t\t\t\t\t 0, err);\n> +}\n> +\n> +struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n> +\t\t\t\t\t\t struct strbuf *err)\n> +{\n> +\treturn ref_store_transaction_begin_1(refs, 0, err);\n> +}\n> +\n> +struct ref_transaction *ref_store_transaction_begin_no_hook(struct ref_store *refs,\n> +\t\t\t\t\t\t\t struct strbuf *err)\n> +{\n> +\treturn ref_store_transaction_begin_1(refs, 1, err);\n> }\n> \n> void ref_transaction_free(struct ref_transaction *transaction)\n> @@ -1158,7 +1172,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,\n> \tstruct strbuf err = STRBUF_INIT;\n> \tint ret = 0;\n> \n> -\tt = ref_store_transaction_begin(refs, 0, &err);\n> +\tt = ref_store_transaction_begin(refs, &err);\n> \tif (!t ||\n> \t ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,\n> \t\t\t\t &err) ||\n> diff --git a/refs.h b/refs.h\n> index 31f7bf96424..3663e0347ff 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -573,8 +573,9 @@ enum action_on_err {\n> * be freed by calling ref_transaction_free().\n> */\n> struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n> -\t\t\t\t\t\t unsigned int flags,\n> \t\t\t\t\t\t struct strbuf *err);\n> +struct ref_transaction *ref_store_transaction_begin_no_hook(struct ref_store *refs,\n> +\t\t\t\t\t\t\t struct strbuf *err);\n> struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n> \n> /*\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 9a20cb8fa89..f85c8f3a692 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1121,8 +1121,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n> \tif (check_refname_format(r->name, 0))\n> \t\treturn;\n> \n> -\ttransaction = ref_store_transaction_begin(&refs->base,\n> -\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> +\ttransaction = ref_store_transaction_begin_no_hook(&refs->base, &err);\n> \tif (!transaction)\n> \t\tgoto cleanup;\n> \tref_transaction_add_update(\n> @@ -1193,8 +1192,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n> \tstruct strbuf err = STRBUF_INIT;\n> \tstruct ref_transaction *transaction;\n> \n> -\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n> -\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> +\ttransaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, &err);\n> \tif (!transaction)\n> \t\treturn -1;\n> \n> @@ -1261,8 +1259,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n> \t\tgoto error;\n> \n> -\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n> -\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n> +\ttransaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, &err);\n> \tif (!transaction)\n> \t\tgoto error;\n> \n> @@ -2775,9 +2772,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> \t\t\t * packed-refs if it exists there.\n> \t\t\t */\n> \t\t\tif (!packed_transaction) {\n> -\t\t\t\tpacked_transaction = ref_store_transaction_begin(\n> -\t\t\t\t\t\trefs->packed_ref_store,\n> -\t\t\t\t\t\tREF_TRANSACTION_SKIP_HOOK, err);\n> +\t\t\t\tpacked_transaction = ref_store_transaction_begin_no_hook(\n> +\t\t\t\t\t\trefs->packed_ref_store, err);\n> \t\t\t\tif (!packed_transaction) {\n> \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n> \t\t\t\t\tgoto cleanup;\n> @@ -3048,8 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n> \t\t\t\t &affected_refnames))\n> \t\tBUG(\"initial ref transaction called with existing refs\");\n> \n> -\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n> -\t\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, err);\n> +\tpacked_transaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, err);\n> \tif (!packed_transaction) {\n> \t\tret = TRANSACTION_GENERIC_ERROR;\n> \t\tgoto cleanup;\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index cfebcd0b46e..e97d67f9321 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1535,7 +1535,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n> \t * updates into a single transaction.\n> \t */\n> \n> -\ttransaction = ref_store_transaction_begin(ref_store, 0, &err);\n> +\ttransaction = ref_store_transaction_begin(ref_store, &err);\n> \tif (!transaction)\n> \t\treturn -1;\n> \n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index 87da39243f7..8ff9e0e6e3a 100644\n> --- a/refs/refs-internal.h\n> +++ b/refs/refs-internal.h\n> @@ -204,7 +204,9 @@ enum ref_transaction_state {\n> /*\n> * Skip executing the reference-transaction hook.\n> */\n> -#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n> +enum ref_transaction_flags {\n> +\tREF_TRANSACTION_SKIP_HOOK = 1 << 0,\n> +};\n> \n> /*\n> * Data structure for holding a reference transaction, which can\n> @@ -218,7 +220,7 @@ struct ref_transaction {\n> \tsize_t nr;\n> \tenum ref_transaction_state state;\n> \tvoid *backend_data;\n> -\tunsigned int flags;\n> +\tenum ref_transaction_flags flags;\n> };\n> \n> /*\n> diff --git a/sequencer.c b/sequencer.c\n> index 61edd39d7a4..6abd72160cc 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3588,7 +3588,7 @@ static int do_label(struct repository *r, const char *name, int len)\n> \tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n> \tstrbuf_addf(&msg, \"rebase (label) '%.*s'\", len, name);\n> \n> -\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n> +\ttransaction = ref_store_transaction_begin(refs, &err);\n> \tif (!transaction) {\n> \t\terror(\"%s\", err.buf);\n> \t\tret = -1;\n"},{"id":"446346","messageId":"cover.1642406989.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1641556319.git.ps@pks.im","subject":"[PATCH v4 0/6] refs: excessive hook execution with packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T08:12:27Z","receivedAt":"2022-01-17T08:12:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the fourth version of this patch series, which addresses an\nissue where the reference-transaction hook is being invoked twice when\ndeleting refs both in the packed-refs and loose-refs file.\n\nThe following things changed in comparison to v3:\n\n - Fixed a memory leak in `files_delete_refs()`.\n\n - Refactored the `packed_downcast()` invocation such that we don't\n have to mark its unused returned variable as used.\n\n - Removed a spurious whitespace change.\n\nPatrick\n\nPatrick Steinhardt (6):\n refs: extract packed_refs_delete_refs() to allow control of\n transaction\n refs: allow passing flags when beginning transactions\n refs: allow skipping the reference-transaction hook\n refs: demonstrate excessive execution of the reference-transaction\n hook\n refs: do not execute reference-transaction hook on packing refs\n refs: skip hooks when deleting uncovered packed refs\n\n refs.c | 11 +++++--\n refs.h | 8 ++++-\n refs/files-backend.c | 26 ++++++++++++-----\n refs/packed-backend.c | 28 +++++++++++++-----\n refs/packed-backend.h | 7 +++++\n refs/refs-internal.h | 1 +\n sequencer.c | 2 +-\n t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++\n 8 files changed, 114 insertions(+), 19 deletions(-)\n\nRange-diff against v3:\n1: abbc28822b ! 1: 14775046e1 refs: extract packed_refs_delete_refs() to allow control of transaction\n @@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store,\n \n \tpacked_refs_unlock(refs->packed_ref_store);\n \n +@@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n + \t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n + \t}\n + \n ++\tref_transaction_free(transaction);\n + \tstrbuf_release(&err);\n + \treturn result;\n + \n @@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \telse\n \t\terror(_(\"could not delete references: %s\"), err.buf);\n @@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store\n +\t\t\t struct string_list *refnames,\n +\t\t\t unsigned int flags)\n +{\n -+\tstruct packed_ref_store *refs =\n -+\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n +\tstruct strbuf err = STRBUF_INIT;\n +\tstruct string_list_item *item;\n +\tint ret;\n +\n -+\t(void)(refs); /* We need the check above, but don't use the variable */\n ++\t/* Assert that the ref store refers to a packed backend. */\n ++\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n +\n \tfor_each_string_list_item(item, refnames) {\n \t\tif (ref_transaction_delete(transaction, item->string, NULL,\n \t\t\t\t\t flags, msg, &err)) {\n -@@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n - \t}\n - \n - \tret = ref_transaction_commit(transaction, &err);\n --\n - \tif (ret) {\n - \t\tif (refnames->nr == 1)\n - \t\t\terror(_(\"could not delete reference %s: %s\"),\n @@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\terror(_(\"could not delete references: %s\"), err.buf);\n \t}\n2: 9dd172a757 = 2: d4ac24c8b8 refs: allow passing flags when beginning transactions\n3: be826bae3b = 3: f4a07fe9a8 refs: allow skipping the reference-transaction hook\n4: 662a6e6244 = 4: a8981baef7 refs: demonstrate excessive execution of the reference-transaction hook\n5: d83f309b9c = 5: 23c344854e refs: do not execute reference-transaction hook on packing refs\n6: 279eadc41c = 6: d6c7d765af refs: skip hooks when deleting uncovered packed refs\n-- \n2.34.1\n\n"},{"id":"446347","messageId":"14775046e1fb73a65b2198e72e72428b16d3e65c.1642406989.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642406989.git.ps@pks.im","subject":"[PATCH v4 1/6] refs: extract packed_refs_delete_refs() to allow control of transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T08:12:31Z","receivedAt":"2022-01-17T08:12:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When deleting loose refs, then we also have to delete the refs in the\npacked backend. This is done by calling `refs_delete_refs()`, which\nthen uses the packed-backend's logic to delete refs. This doesn't allow\nus to exercise any control over the reference transaction which is being\ncreated in the packed backend, which is required in a subsequent commit.\n\nExtract a new function `packed_refs_delete_refs()`, which hosts most of\nthe logic to delete refs except for creating the transaction itself.\nLike this, we can easily create the transaction in the files backend\nand thus exert more control over it.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 13 ++++++++++---\n refs/packed-backend.c | 26 ++++++++++++++++++++------\n refs/packed-backend.h | 7 +++++++\n 3 files changed, 37 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 90b671025a..459f18dbc1 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1249,6 +1249,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\tstruct ref_transaction *transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint i, result = 0;\n \n@@ -1258,10 +1259,14 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n-\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\tif (!transaction)\n+\t\tgoto error;\n+\n+\tresult = packed_refs_delete_refs(refs->packed_ref_store,\n+\t\t\t\t\t transaction, msg, refnames, flags);\n+\tif (result)\n \t\tgoto error;\n-\t}\n \n \tpacked_refs_unlock(refs->packed_ref_store);\n \n@@ -1272,6 +1277,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n \t}\n \n+\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn result;\n \n@@ -1288,6 +1294,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \telse\n \t\terror(_(\"could not delete references: %s\"), err.buf);\n \n+\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn -1;\n }\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 67152c664e..c964dd1617 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\t struct string_list *refnames, unsigned int flags)\n {\n-\tstruct packed_ref_store *refs =\n-\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n-\tstruct string_list_item *item;\n \tint ret;\n \n-\t(void)refs; /* We need the check above, but don't use the variable */\n-\n \tif (!refnames->nr)\n \t\treturn 0;\n \n@@ -1544,6 +1539,26 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (!transaction)\n \t\treturn -1;\n \n+\tret = packed_refs_delete_refs(ref_store, transaction,\n+\t\t\t\t msg, refnames, flags);\n+\n+\tref_transaction_free(transaction);\n+\treturn ret;\n+}\n+\n+int packed_refs_delete_refs(struct ref_store *ref_store,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *msg,\n+\t\t\t struct string_list *refnames,\n+\t\t\t unsigned int flags)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\tint ret;\n+\n+\t/* Assert that the ref store refers to a packed backend. */\n+\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\n \tfor_each_string_list_item(item, refnames) {\n \t\tif (ref_transaction_delete(transaction, item->string, NULL,\n \t\t\t\t\t flags, msg, &err)) {\n@@ -1563,7 +1578,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\terror(_(\"could not delete references: %s\"), err.buf);\n \t}\n \n-\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn ret;\n }\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex f61a73ec25..a2cca5d9a3 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -3,6 +3,7 @@\n \n struct repository;\n struct ref_transaction;\n+struct string_list;\n \n /*\n * Support for storing references in a `packed-refs` file.\n@@ -27,6 +28,12 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n void packed_refs_unlock(struct ref_store *ref_store);\n int packed_refs_is_locked(struct ref_store *ref_store);\n \n+int packed_refs_delete_refs(struct ref_store *ref_store,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *msg,\n+\t\t\t struct string_list *refnames,\n+\t\t\t unsigned int flags);\n+\n /*\n * Return true if `transaction` really needs to be carried out against\n * the specified packed_ref_store, or false if it can be skipped\n-- \n2.34.1\n\n"},{"id":"446348","messageId":"d4ac24c8b8ab8020f7af1c646041b4413b56ec9e.1642406989.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642406989.git.ps@pks.im","subject":"[PATCH v4 2/6] refs: allow passing flags when beginning transactions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T08:12:35Z","receivedAt":"2022-01-17T08:12:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not currently have any flags when creating reference transactions,\nbut we'll add one to disable execution of the reference transaction hook\nin some cases.\n\nAllow passing flags to `ref_store_transaction_begin()` to prepare for\nthis change.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 8 +++++---\n refs.h | 3 ++-\n refs/files-backend.c | 10 +++++-----\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 1 +\n sequencer.c | 2 +-\n 6 files changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4da4996c4d..7415864b62 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -800,7 +800,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\ttransaction = ref_store_transaction_begin(refs, &err);\n+\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname, old_oid,\n \t\t\t\t flags, msg, &err) ||\n@@ -1005,6 +1005,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n }\n \n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_transaction *tr;\n@@ -1012,12 +1013,13 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n \n \tCALLOC_ARRAY(tr, 1);\n \ttr->ref_store = refs;\n+\ttr->flags = flags;\n \treturn tr;\n }\n \n struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n {\n-\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), err);\n+\treturn ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);\n }\n \n void ref_transaction_free(struct ref_transaction *transaction)\n@@ -1156,7 +1158,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tt = ref_store_transaction_begin(refs, &err);\n+\tt = ref_store_transaction_begin(refs, 0, &err);\n \tif (!t ||\n \t ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,\n \t\t\t\t &err) ||\ndiff --git a/refs.h b/refs.h\nindex 92360e55a2..31f7bf9642 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -231,7 +231,7 @@ char *repo_default_branch_name(struct repository *r, int quiet);\n * struct strbuf err = STRBUF_INIT;\n * int ret = 0;\n *\n- * transaction = ref_store_transaction_begin(refs, &err);\n+ * transaction = ref_store_transaction_begin(refs, 0, &err);\n * if (!transaction ||\n * ref_transaction_update(...) ||\n * ref_transaction_create(...) ||\n@@ -573,6 +573,7 @@ enum action_on_err {\n * be freed by calling ref_transaction_free().\n */\n struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n+\t\t\t\t\t\t unsigned int flags,\n \t\t\t\t\t\t struct strbuf *err);\n struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 459f18dbc1..4d4f0c2099 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1121,7 +1121,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base, &err);\n+\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1192,7 +1192,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n \tif (!transaction)\n \t\treturn -1;\n \n@@ -1259,7 +1259,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2774,7 +2774,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store, err);\n+\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3045,7 +3045,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, err);\n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex c964dd1617..0b45598e18 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1535,7 +1535,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t * updates into a single transaction.\n \t */\n \n-\ttransaction = ref_store_transaction_begin(ref_store, &err);\n+\ttransaction = ref_store_transaction_begin(ref_store, 0, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 46a839539e..a0af63f162 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -213,6 +213,7 @@ struct ref_transaction {\n \tsize_t nr;\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n+\tunsigned int flags;\n };\n \n /*\ndiff --git a/sequencer.c b/sequencer.c\nindex 6abd72160c..61edd39d7a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3588,7 +3588,7 @@ static int do_label(struct repository *r, const char *name, int len)\n \tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n \tstrbuf_addf(&msg, \"rebase (label) '%.*s'\", len, name);\n \n-\ttransaction = ref_store_transaction_begin(refs, &err);\n+\ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction) {\n \t\terror(\"%s\", err.buf);\n \t\tret = -1;\n-- \n2.34.1\n\n"},{"id":"446349","messageId":"f4a07fe9a8823d382e010a8b4978fa8a3fc3c943.1642406989.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642406989.git.ps@pks.im","subject":"[PATCH v4 3/6] refs: allow skipping the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T08:12:39Z","receivedAt":"2022-01-17T08:12:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reference-transaction hook is executing whenever we prepare, commit\nor abort a reference transaction. While this is mostly intentional, in\ncase of the files backend we're leaking the implementation detail that\nthe store is in fact a composite store with one loose and one packed\nbackend to the caller. So while we want to execute the hook for all\nlogical updates, executing it for such implementation details is\nunexpected.\n\nPrepare for a fix by adding a new flag which allows to skip execution of\nthe hook.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 3 +++\n refs.h | 5 +++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 7415864b62..526bf5ed97 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2084,6 +2084,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n \tconst char *hook;\n \tint ret = 0, i;\n \n+\tif (transaction->flags & REF_TRANSACTION_SKIP_HOOK)\n+\t\treturn 0;\n+\n \thook = find_hook(\"reference-transaction\");\n \tif (!hook)\n \t\treturn ret;\ndiff --git a/refs.h b/refs.h\nindex 31f7bf9642..d4056f9fe2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -568,6 +568,11 @@ enum action_on_err {\n \tUPDATE_REFS_QUIET_ON_ERR\n };\n \n+/*\n+ * Skip executing the reference-transaction hook.\n+ */\n+#define REF_TRANSACTION_SKIP_HOOK (1 << 0)\n+\n /*\n * Begin a reference transaction. The reference transaction must\n * be freed by calling ref_transaction_free().\n-- \n2.34.1\n\n"},{"id":"446350","messageId":"a8981baef74b4622b5819cddaae80c4356f440c4.1642406989.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642406989.git.ps@pks.im","subject":"[PATCH v4 4/6] refs: demonstrate excessive execution of the reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T08:12:44Z","receivedAt":"2022-01-17T08:12:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Add tests which demonstate that we're executing the\nreference-transaction hook too often in some cases, which thus leaks\nimplementation details about the reference store's implementation\nitself. Behaviour will be fixed in follow-up commits.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 6c941027a8..0567fbdf0b 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' '\n \ttest_cmp expect target-repo.git/actual\n '\n \n+test_expect_success 'hook does not get called on packing refs' '\n+\t# Pack references first such that we are in a known state.\n+\tgit pack-refs --all &&\n+\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$@\" >>actual\n+\t\tcat >>actual\n+\tEOF\n+\trm -f actual &&\n+\n+\tgit update-ref refs/heads/unpacked-ref $POST_OID &&\n+\tgit pack-refs --all &&\n+\n+\t# We only expect a single hook invocation, which is the call to\n+\t# git-update-ref(1). But currently, packing refs will also trigger the\n+\t# hook.\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tprepared\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n+\t\tprepared\n+\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n+\t\tcommitted\n+\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'deleting packed ref calls hook once' '\n+\t# Create a reference and pack it.\n+\tgit update-ref refs/heads/to-be-deleted $POST_OID &&\n+\tgit pack-refs --all &&\n+\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$@\" >>actual\n+\t\tcat >>actual\n+\tEOF\n+\trm -f actual &&\n+\n+\tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n+\n+\t# We only expect a single hook invocation, which is the logical\n+\t# deletion. But currently, we see two interleaving transactions, once\n+\t# for deleting the loose refs and once for deleting the packed ref.\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tprepared\n+\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tcommitted\n+\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n+\t\tcommitted\n+\t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.34.1\n\n"},{"id":"446351","messageId":"23c344854e499d25721502405763d185ac2ae2ea.1642406989.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642406989.git.ps@pks.im","subject":"[PATCH v4 5/6] refs: do not execute reference-transaction hook on packing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T08:12:48Z","receivedAt":"2022-01-17T08:12:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reference-transaction hook is supposed to track logical changes to\nreferences, but it currently also gets executed when packing refs in a\nrepository. This is unexpected and ultimately not all that useful:\npacking refs is not supposed to result in any user-visible change to the\nrefs' state, and it ultimately is an implementation detail of how refs\nstores work.\n\nFix this excessive execution of the hook when packing refs.\n\nReported-by: Waleed Khan \nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 6 ++++--\n t/t1416-ref-transaction-hooks.sh | 11 +----------\n 2 files changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4d4f0c2099..565929210a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1121,7 +1121,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tif (check_refname_format(r->name, 0))\n \t\treturn;\n \n-\ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n+\ttransaction = ref_store_transaction_begin(&refs->base,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n \tref_transaction_add_update(\n@@ -1192,7 +1193,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\treturn -1;\n \ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 0567fbdf0b..f9d3d5213f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' '\n \tgit pack-refs --all &&\n \n \t# We only expect a single hook invocation, which is the call to\n-\t# git-update-ref(1). But currently, packing refs will also trigger the\n-\t# hook.\n+\t# git-update-ref(1).\n \tcat >expect <<-EOF &&\n \t\tprepared\n \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n \t\tcommitted\n \t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tprepared\n-\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tcommitted\n-\t\t$ZERO_OID $POST_OID refs/heads/unpacked-ref\n-\t\tprepared\n-\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n-\t\tcommitted\n-\t\t$POST_OID $ZERO_OID refs/heads/unpacked-ref\n \tEOF\n \n \ttest_cmp expect actual\n-- \n2.34.1\n\n"},{"id":"446352","messageId":"d6c7d765aff13df449d61f0f6efbeb66fe86006d.1642406989.git.ps@pks.im","threadId":"57044","inReplyTo":"cover.1642406989.git.ps@pks.im","subject":"[PATCH v4 6/6] refs: skip hooks when deleting uncovered packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-17T08:12:53Z","receivedAt":"2022-01-17T08:12:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When deleting refs from the loose-files refs backend, then we need to be\ncareful to also delete the same ref from the packed refs backend, if it\nexists. If we don't, then deleting the loose ref would \"uncover\" the\npacked ref. We thus always have to queue up deletions of refs for both\nthe loose and the packed refs backend. This is done in two separate\ntransactions, where the end result is that the reference-transaction\nhook is executed twice for the deleted refs.\n\nThis behaviour is quite misleading: it's exposing implementation details\nof how the files backend works to the user, in contrast to the logical\nupdates that we'd really want to expose via the hook. Worse yet, whether\nthe hook gets executed once or twice depends on how well-packed the\nrepository is: if the ref only exists as a loose ref, then we execute it\nonce, otherwise if it is also packed then we execute it twice.\n\nFix this behaviour and don't execute the reference-transaction hook at\nall when refs in the packed-refs backend if it's driven by the files\nbackend. This works as expected even in case the refs to be deleted only\nexist in the packed-refs backend because the loose-backend always queues\nrefs in its own transaction even if they don't exist such that they can\nbe locked for concurrent creation. And it also does the right thing in\ncase neither of the backends has the ref because that would cause the\ntransaction to fail completely.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 9 ++++++---\n t/t1416-ref-transaction-hooks.sh | 7 +------\n 2 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 565929210a..844918cbd8 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1261,7 +1261,8 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);\n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, &err);\n \tif (!transaction)\n \t\tgoto error;\n \n@@ -2776,7 +2777,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tif (!packed_transaction) {\n \t\t\t\tpacked_transaction = ref_store_transaction_begin(\n-\t\t\t\t\t\trefs->packed_ref_store, 0, err);\n+\t\t\t\t\t\trefs->packed_ref_store,\n+\t\t\t\t\t\tREF_TRANSACTION_SKIP_HOOK, err);\n \t\t\t\tif (!packed_transaction) {\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n@@ -3047,7 +3049,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n-\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err);\n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n+\t\t\t\t\t\t\t REF_TRANSACTION_SKIP_HOOK, err);\n \tif (!packed_transaction) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex f9d3d5213f..4e1e84a91f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' '\n \tgit update-ref -d refs/heads/to-be-deleted $POST_OID &&\n \n \t# We only expect a single hook invocation, which is the logical\n-\t# deletion. But currently, we see two interleaving transactions, once\n-\t# for deleting the loose refs and once for deleting the packed ref.\n+\t# deletion.\n \tcat >expect <<-EOF &&\n-\t\tprepared\n-\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n \t\tprepared\n \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n \t\tcommitted\n-\t\t$ZERO_OID $ZERO_OID refs/heads/to-be-deleted\n-\t\tcommitted\n \t\t$POST_OID $ZERO_OID refs/heads/to-be-deleted\n \tEOF\n \n-- \n2.34.1\n\n"},{"id":"446355","messageId":"CAFQ2z_M+2D-5qPXN=0FEveZ36Yq0rps3HxRkgCmiupq1DD7Kig@mail.gmail.com","threadId":"57044","inReplyTo":"YeUYNhJemN/hRG+6@ncase","subject":"Re: [PATCH v2 0/6] refs: excessive hook execution with packed refs","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-01-17T11:37:39Z","receivedAt":"2022-01-17T11:37:53Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Mon, Jan 17, 2022 at 8:18 AM Patrick Steinhardt wrote:\n> > It might make sense to not introduce a new flag namespace, but use\n> > update->flags instead. You'd have to add your new flag after\n> > REF_SKIP_REFNAME_VERIFICATION.\n> > Bonus is that you can unittest the new flag using the existing\n> > ref-store helper without extra work. (check that a transaction with &\n> > without the flag works as expected.)\n> >\n> > other than that, looks OK to me.\n>\n> Thanks for your feedback!\n>\n> In fact the first version I had locally did exactly that. But I found\n> the end version result harder to reason about, most importantly because\n> it's not a 100% clear to the reader whether all callsites which delete\n> refs in the packed-backend via the files-backend are adapted or whether\n> any of the callsites was missing. By having the flag in a central place\n> it's immediately clear that the hook won't be run at all, which is\n> exactly what we want here.\n\nIf you do it like this, can you find the callsites that take update\nflags (but not transaction flags), and update the signature to say\n\"update_flags\" rather than \"flags\", and document appropriately?\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"446735","messageId":"Ye5Rgx71QSpkIgT7@ncase","threadId":"57044","inReplyTo":"CAFQ2z_M+2D-5qPXN=0FEveZ36Yq0rps3HxRkgCmiupq1DD7Kig@mail.gmail.com","subject":"Re: [PATCH v2 0/6] refs: excessive hook execution with packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-01-24T07:13:07Z","receivedAt":"2022-01-24T07:13:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 17, 2022 at 12:37:39PM +0100, Han-Wen Nienhuys wrote:\n> On Mon, Jan 17, 2022 at 8:18 AM Patrick Steinhardt wrote:\n> > > It might make sense to not introduce a new flag namespace, but use\n> > > update->flags instead. You'd have to add your new flag after\n> > > REF_SKIP_REFNAME_VERIFICATION.\n> > > Bonus is that you can unittest the new flag using the existing\n> > > ref-store helper without extra work. (check that a transaction with &\n> > > without the flag works as expected.)\n> > >\n> > > other than that, looks OK to me.\n> >\n> > Thanks for your feedback!\n> >\n> > In fact the first version I had locally did exactly that. But I found\n> > the end version result harder to reason about, most importantly because\n> > it's not a 100% clear to the reader whether all callsites which delete\n> > refs in the packed-backend via the files-backend are adapted or whether\n> > any of the callsites was missing. By having the flag in a central place\n> > it's immediately clear that the hook won't be run at all, which is\n> > exactly what we want here.\n> \n> If you do it like this, can you find the callsites that take update\n> flags (but not transaction flags), and update the signature to say\n> \"update_flags\" rather than \"flags\", and document appropriately?\n\nAll functions which accept \"flags\" document that they pass the parameter\ndirectly to `ref_transaction_{update,create,delete,verify}` already. And\nin the context of those latter functions it cannot possibly be related\nto the flags passed to `refs_store_transaction_begin()` because they\nalready get a transaction as parameter and thus don't have to create\na new one.\n\nSo we could apply below patch, and I'm happy to polish and add it to the\nseries. But it feels to me like it adds churn while only stating things\nthat are documented already. It could only be me though given that I'm\nobviously biased, so please feel free to disagree.\n\nPatrick\n\n-- >8 --\n\nFrom 91c77a69b6326b1b6cc743ebba6fb0970f0d01c2 Mon Sep 17 00:00:00 2001\nMessage-Id: <91c77a69b6326b1b6cc743ebba6fb0970f0d01c2.1643008212.git.ps@pks.im>\nFrom: Patrick Steinhardt \nDate: Mon, 24 Jan 2022 08:09:45 +0100\nSubject: [PATCH] refs: rename generic `flags` field to `update_flags` for\n clarity\n\n---\n refs.c | 50 +++++++++++++++++++++++++-------------------------\n refs.h | 20 ++++++++++----------\n 2 files changed, 35 insertions(+), 35 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 526bf5ed97..c15e8bd58d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -795,7 +795,7 @@ long get_files_ref_lock_timeout_ms(void)\n int refs_delete_ref(struct ref_store *refs, const char *msg,\n \t\t const char *refname,\n \t\t const struct object_id *old_oid,\n-\t\t unsigned int flags)\n+\t\t unsigned int update_flags)\n {\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n@@ -803,7 +803,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n \ttransaction = ref_store_transaction_begin(refs, 0, &err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname, old_oid,\n-\t\t\t\t flags, msg, &err) ||\n+\t\t\t\t update_flags, msg, &err) ||\n \t ref_transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n@@ -816,10 +816,10 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,\n }\n \n int delete_ref(const char *msg, const char *refname,\n-\t const struct object_id *old_oid, unsigned int flags)\n+\t const struct object_id *old_oid, unsigned int update_flags)\n {\n \treturn refs_delete_ref(get_main_ref_store(the_repository), msg, refname,\n-\t\t\t old_oid, flags);\n+\t\t\t old_oid, update_flags);\n }\n \n static void copy_reflog_msg(struct strbuf *sb, const char *msg)\n@@ -1080,12 +1080,12 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *new_oid,\n \t\t\t const struct object_id *old_oid,\n-\t\t\t unsigned int flags, const char *msg,\n+\t\t\t unsigned int update_flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tassert(err);\n \n-\tif (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&\n+\tif (!(update_flags & REF_SKIP_REFNAME_VERIFICATION) &&\n \t ((new_oid && !is_null_oid(new_oid)) ?\n \t\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :\n \t\t\t !refname_is_safe(refname))) {\n@@ -1094,19 +1094,19 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\tif (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n-\t\tBUG(\"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n+\tif (update_flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n+\t\tBUG(\"illegal flags 0x%x passed to ref_transaction_update()\", update_flags);\n \n \t/*\n \t * Clear flags outside the allowed set; this should be a noop because\n \t * of the BUG() check above, but it works around a -Wnonnull warning\n \t * with some versions of \"gcc -O3\".\n \t */\n-\tflags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;\n+\tupdate_flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;\n \n-\tflags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n+\tupdate_flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n \n-\tref_transaction_add_update(transaction, refname, flags,\n+\tref_transaction_add_update(transaction, refname, update_flags,\n \t\t\t\t new_oid, old_oid, msg);\n \treturn 0;\n }\n@@ -1114,44 +1114,44 @@ int ref_transaction_update(struct ref_transaction *transaction,\n int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *new_oid,\n-\t\t\t unsigned int flags, const char *msg,\n+\t\t\t unsigned int update_flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tif (!new_oid || is_null_oid(new_oid))\n \t\tBUG(\"create called without valid new_oid\");\n \treturn ref_transaction_update(transaction, refname, new_oid,\n-\t\t\t\t null_oid(), flags, msg, err);\n+\t\t\t\t null_oid(), update_flags, msg, err);\n }\n \n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *old_oid,\n-\t\t\t unsigned int flags, const char *msg,\n+\t\t\t unsigned int update_flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tif (old_oid && is_null_oid(old_oid))\n \t\tBUG(\"delete called with old_oid set to zeros\");\n \treturn ref_transaction_update(transaction, refname,\n \t\t\t\t null_oid(), old_oid,\n-\t\t\t\t flags, msg, err);\n+\t\t\t\t update_flags, msg, err);\n }\n \n int ref_transaction_verify(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *old_oid,\n-\t\t\t unsigned int flags,\n+\t\t\t unsigned int update_flags,\n \t\t\t struct strbuf *err)\n {\n \tif (!old_oid)\n \t\tBUG(\"verify called with old_oid set to NULL\");\n \treturn ref_transaction_update(transaction, refname,\n \t\t\t\t NULL, old_oid,\n-\t\t\t\t flags, NULL, err);\n+\t\t\t\t update_flags, NULL, err);\n }\n \n int refs_update_ref(struct ref_store *refs, const char *msg,\n \t\t const char *refname, const struct object_id *new_oid,\n-\t\t const struct object_id *old_oid, unsigned int flags,\n+\t\t const struct object_id *old_oid, unsigned int update_flags,\n \t\t enum action_on_err onerr)\n {\n \tstruct ref_transaction *t = NULL;\n@@ -1160,7 +1160,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,\n \n \tt = ref_store_transaction_begin(refs, 0, &err);\n \tif (!t ||\n-\t ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,\n+\t ref_transaction_update(t, refname, new_oid, old_oid, update_flags, msg,\n \t\t\t\t &err) ||\n \t ref_transaction_commit(t, &err)) {\n \t\tret = 1;\n@@ -1191,10 +1191,10 @@ int refs_update_ref(struct ref_store *refs, const char *msg,\n int update_ref(const char *msg, const char *refname,\n \t const struct object_id *new_oid,\n \t const struct object_id *old_oid,\n-\t unsigned int flags, enum action_on_err onerr)\n+\t unsigned int update_flags, enum action_on_err onerr)\n {\n \treturn refs_update_ref(get_main_ref_store(the_repository), msg, refname, new_oid,\n-\t\t\t old_oid, flags, onerr);\n+\t\t\t old_oid, update_flags, onerr);\n }\n \n char *refs_shorten_unambiguous_ref(struct ref_store *refs,\n@@ -2435,21 +2435,21 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n }\n \n int refs_delete_refs(struct ref_store *refs, const char *logmsg,\n-\t\t struct string_list *refnames, unsigned int flags)\n+\t\t struct string_list *refnames, unsigned int update_flags)\n {\n \tchar *msg;\n \tint retval;\n \n \tmsg = normalize_reflog_message(logmsg);\n-\tretval = refs->be->delete_refs(refs, msg, refnames, flags);\n+\tretval = refs->be->delete_refs(refs, msg, refnames, update_flags);\n \tfree(msg);\n \treturn retval;\n }\n \n int delete_refs(const char *msg, struct string_list *refnames,\n-\t\tunsigned int flags)\n+\t\tunsigned int update_flags)\n {\n-\treturn refs_delete_refs(get_main_ref_store(the_repository), msg, refnames, flags);\n+\treturn refs_delete_refs(get_main_ref_store(the_repository), msg, refnames, update_flags);\n }\n \n int refs_rename_ref(struct ref_store *refs, const char *oldref,\ndiff --git a/refs.h b/refs.h\nindex d4056f9fe2..5131db7e48 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -442,9 +442,9 @@ int reflog_exists(const char *refname);\n int refs_delete_ref(struct ref_store *refs, const char *msg,\n \t\t const char *refname,\n \t\t const struct object_id *old_oid,\n-\t\t unsigned int flags);\n+\t\t unsigned int update_flags);\n int delete_ref(const char *msg, const char *refname,\n-\t const struct object_id *old_oid, unsigned int flags);\n+\t const struct object_id *old_oid, unsigned int update_flags);\n \n /*\n * Delete the specified references. If there are any problems, emit\n@@ -453,9 +453,9 @@ int delete_ref(const char *msg, const char *refname,\n * ref_transaction_delete().\n */\n int refs_delete_refs(struct ref_store *refs, const char *msg,\n-\t\t struct string_list *refnames, unsigned int flags);\n+\t\t struct string_list *refnames, unsigned int update_flags);\n int delete_refs(const char *msg, struct string_list *refnames,\n-\t\tunsigned int flags);\n+\t\tunsigned int update_flags);\n \n /** Delete a reflog */\n int refs_delete_reflog(struct ref_store *refs, const char *refname);\n@@ -680,7 +680,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *new_oid,\n \t\t\t const struct object_id *old_oid,\n-\t\t\t unsigned int flags, const char *msg,\n+\t\t\t unsigned int update_flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -695,7 +695,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *new_oid,\n-\t\t\t unsigned int flags, const char *msg,\n+\t\t\t unsigned int update_flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -709,7 +709,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *old_oid,\n-\t\t\t unsigned int flags, const char *msg,\n+\t\t\t unsigned int update_flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -723,7 +723,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n int ref_transaction_verify(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *old_oid,\n-\t\t\t unsigned int flags,\n+\t\t\t unsigned int update_flags,\n \t\t\t struct strbuf *err);\n \n /* Naming conflict (for example, the ref names A and A/B conflict). */\n@@ -796,10 +796,10 @@ void ref_transaction_free(struct ref_transaction *transaction);\n */\n int refs_update_ref(struct ref_store *refs, const char *msg, const char *refname,\n \t\t const struct object_id *new_oid, const struct object_id *old_oid,\n-\t\t unsigned int flags, enum action_on_err onerr);\n+\t\t unsigned int update_flags, enum action_on_err onerr);\n int update_ref(const char *msg, const char *refname,\n \t const struct object_id *new_oid, const struct object_id *old_oid,\n-\t unsigned int flags, enum action_on_err onerr);\n+\t unsigned int update_flags, enum action_on_err onerr);\n \n int parse_hide_refs_config(const char *var, const char *value, const char *);\n \n-- \n2.34.1\n\n"}]}