{"thread":{"id":"57841","subject":"[PATCH] refs: honor reference-transaction semantics when deleting refs","startedAt":"2022-05-04T15:00:43Z","lastAt":"2022-05-09T13:49:05Z","messageCount":4,"participants":["Michael Heemskerk via GitGitGadget","Patrick Steinhardt","Michael Heemskerk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"454815","messageId":"pull.1228.git.1651676435634.gitgitgadget@gmail.com","threadId":"57841","inReplyTo":null,"subject":"[PATCH] refs: honor reference-transaction semantics when deleting refs","fromName":"Michael Heemskerk via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-04T15:00:35Z","receivedAt":"2022-05-04T15:00:43Z","isPatch":true,"sender":{"key":"mheemskerk@atlassian.com","avatar":"https://gravatar.com/avatar/c3ebe086c3c55529246291e9879e5ef7af3c0ffcadbd81b5508b5fcdf66aa065?d=mp&s=160"},"body":"From: Michael Heemskerk <mheemskerk@atlassian.com>\n\nWhen deleting refs from the loose-files refs backend, files_delete_refs\nfirst rewrites packed-refs if any of the to-be-deleted refs were packed\nand then removes loose refs. While making those changes, it invokes the\nreference-transaction hook incorrectly; a single transaction is used to\nprepare and commit the changes to packed-refs, followed by another\ntransaction per deleted ref, each with another prepared and committed\nreference-transaction hook invocation.\n\nThis behaviour is problematic for a number of reasons. First of all,\ndeleting a ref through `git branch -d` or `git tag -d` results in a\ndifferent sequence of reference-transaction callbacks than deleting the\nsame ref through `git update-ref`:\n\n- update-ref of a loose ref: aborted, prepared, committed\n- update-ref of a packed ref: prepared, prepared, committed, commited\n- branch -d: prepared, committed, aborted, prepared, committed\n\nThe bigger problem is that the packed-refs update is committed before\nthe prepared reference-transaction invocation is done for the loose\nref. Returning a non-zero exit code from the second\nreference-transaction callback leads to an invalid sequence of\nreference-transaction callbacks:\n\n1. prepared - hook returns 0, so the change is allowed to go through.\n2. committed - git informs the hook that the changes are now final,\n   but are they really? Any loose refs may still survive if the\n   subsequent prepared callback is canceled.\n3. aborted - 'fake' invocation from the packed-transaction because the\n   ref does not exist in packed-refs.\n4. prepared - hook returns 1, so the change should be blocked.\n5. aborted - git informs the hook that it has rolled back the change,\n   but it really hasn't; packed-refs has already been rewritten and\n   if the ref only existed as a packed ref, it is now gone.\n\nThe changes to the reference-transaction invocations that were planned\nfor git 2.36 but have been reverted make the problem more pronounced.\nThose changes suppress the 'fake' invocations for the packed-transaction\n(invocations 1-3 in the above list). In that case, the prepared callback\nin step 4 cannot prevent a ref from being deleted if it only existed in\npacked-refs.\n\nTo address the issue, the use a separate transactions to update the\npacked and loose refs has been removed from files_delete_refs. Instead,\nit now uses a single transaction, queues up the refs-to-be-deleted and\nrelies on the standard transaction mechanism to invoke the\nreference-transaction hooks as expected.\n\nSigned-off-by: Michael Heemskerk <mheemskerk@atlassian.com>\n---\n    refs: honor reference-transaction semantics when deleting refs\n    \n    When deleting refs from the loose-files refs backend, files_delete_refs\n    first rewrites packed-refs if any of the to-be-deleted refs were packed\n    and then removes loose refs. While making those changes, it invokes the\n    reference-transaction hook incorrectly; a single transaction is used to\n    prepare and commit the changes to packed-refs, followed by another\n    transaction per deleted ref, each with another prepared and committed\n    reference-transaction hook invocation.\n    \n    This behaviour is problematic for a number of reasons. First of all,\n    deleting a ref through git branch -d or git tag -d results in a\n    different sequence of reference-transaction callbacks than deleting the\n    same ref through git update-ref:\n    \n     * update-ref of a loose ref: aborted, prepared, committed\n     * update-ref of a packed ref: prepared, prepared, committed, commited\n     * branch -d: prepared, committed, aborted, prepared, committed\n    \n    The bigger problem is that the packed-refs update is committed before\n    the prepared reference-transaction invocation is done for the loose ref.\n    Returning a non-zero exit code from the second reference-transaction\n    callback leads to an invalid sequence of reference-transaction\n    callbacks:\n    \n     1. prepared - hook returns 0, so the change is allowed to go through.\n     2. committed - git informs the hook that the changes are now final, but\n        are they really? Any loose refs may still survive if the subsequent\n        prepared callback is canceled.\n     3. aborted - 'fake' invocation from the packed-transaction because the\n        ref does not exist in packed-refs.\n     4. prepared - hook returns 1, so the change should be blocked.\n     5. aborted - git informs the hook that it has rolled back the change,\n        but it really hasn't; packed-refs has already been rewritten and if\n        the ref only existed as a packed ref, it is now gone.\n    \n    The changes to the reference-transaction invocations that were planned\n    for git 2.36 but have been reverted make the problem more pronounced.\n    Those changes suppress the 'fake' invocations for the packed-transaction\n    (invocations 1-3 in the above list). In that case, the prepared callback\n    in step 4 cannot prevent a ref from being deleted if it only existed in\n    packed-refs.\n    \n    To address the issue, the use a separate transactions to update the\n    packed and loose refs has been removed from files_delete_refs. Instead,\n    it now uses a single transaction, queues up the refs-to-be-deleted and\n    relies on the standard transaction mechanism to invoke the\n    reference-transaction hooks as expected.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1228%2Fmheemskerk%2Ffiles-delete-ref-reference-transaction-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1228/mheemskerk/files-delete-ref-reference-transaction-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1228\n\n refs/files-backend.c             | 33 +++++++--------\n t/t1416-ref-transaction-hooks.sh | 70 ++++++++++++++++++++++++++++++++\n t/t5510-fetch.sh                 |  6 +--\n 3 files changed, 87 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8db7882aacb..5c23277eda7 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1265,44 +1265,39 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n \tstruct strbuf err = STRBUF_INIT;\n-\tint i, result = 0;\n+\tint i;\n+\tstruct ref_transaction *transaction;\n \n \tif (!refnames->nr)\n \t\treturn 0;\n \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->base, &err);\n+\tif (!transaction)\n \t\tgoto error;\n-\t}\n-\n-\tpacked_refs_unlock(refs->packed_ref_store);\n \n \tfor (i = 0; i < refnames->nr; i++) {\n \t\tconst char *refname = refnames->items[i].string;\n-\n-\t\tif (refs_delete_ref(&refs->base, msg, refname, NULL, flags))\n-\t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n+\t\tif (ref_transaction_delete(transaction, refname, NULL,\n+\t\t\t\t\t   flags, msg, &err))\n+\t\t\tgoto error;\n \t}\n \n+\tif (ref_transaction_commit(transaction, &err))\n+\t\tgoto error;\n+\n+\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n-\treturn result;\n+\treturn 0;\n \n error:\n-\t/*\n-\t * If we failed to rewrite the packed-refs file, then it is\n-\t * unsafe to try to remove loose refs, because doing so might\n-\t * expose an obsolete packed value for a reference that might\n-\t * even point at an object that has been garbage collected.\n-\t */\n \tif (refnames->nr == 1)\n \t\terror(_(\"could not delete reference %s: %s\"),\n \t\t      refnames->items[0].string, err.buf);\n \telse\n \t\terror(_(\"could not delete references: %s\"), err.buf);\n \n+\tif (transaction)\n+\t\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn -1;\n }\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 27731722a5b..e3d4fe624f7 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -133,4 +133,74 @@ test_expect_success 'interleaving hook calls succeed' '\n \ttest_cmp expect target-repo.git/actual\n '\n \n+test_expect_success 'hook allows deleting loose ref if successful' '\n+\ttest_when_finished \"rm actual\" &&\n+\tgit branch to-be-deleted $PRE_OID &&\n+\ttest_hook reference-transaction <<-\\EOF &&\n+\t\techo \"$*\" >>actual\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\taborted\n+\t\tprepared\n+\t\tcommitted\n+\tEOF\n+\tgit branch -d to-be-deleted &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git rev-parse refs/heads/to-be-deleted\n+'\n+\n+test_expect_success 'hook allows deleting packed ref if successful' '\n+\ttest_when_finished \"rm actual\" &&\n+\tgit branch to-be-deleted $PRE_OID &&\n+\tgit pack-refs --all --prune &&\n+\ttest_hook reference-transaction <<-\\EOF &&\n+\t\techo \"$*\" >>actual\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\tprepared\n+\t\tcommitted\n+\t\tcommitted\n+\tEOF\n+\tgit branch -d to-be-deleted &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git rev-parse refs/heads/to-be-deleted\n+'\n+\n+test_expect_success 'hook aborts deleting loose ref in prepared state' '\n+\ttest_when_finished \"rm actual\" &&\n+\ttest_when_finished \"git branch -d to-be-deleted\" &&\n+\tgit branch to-be-deleted $PRE_OID &&\n+\ttest_hook reference-transaction <<-\\EOF &&\n+\t\techo \"$*\" >>actual\n+\t\texit 1\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\taborted\n+\t\tprepared\n+\t\taborted\n+\tEOF\n+\ttest_must_fail git branch -d to-be-deleted &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse refs/heads/to-be-deleted\n+'\n+\n+test_expect_success 'hook aborts deleting packed ref in prepared state' '\n+\ttest_when_finished \"rm actual\" &&\n+\ttest_when_finished \"git branch -d to-be-deleted\" &&\n+\tgit branch to-be-deleted $PRE_OID &&\n+\tgit pack-refs --all --prune &&\n+\ttest_hook reference-transaction <<-\\EOF &&\n+\t\techo \"$*\" >>actual\n+\t\texit 1\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\taborted\n+\tEOF\n+\ttest_must_fail git branch -d to-be-deleted &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse refs/heads/to-be-deleted\n+'\n+\n test_done\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 4620f0ca7fa..8b09a99c2e8 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -169,10 +169,10 @@ test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n \tgit clone . prune-fail &&\n \tcd prune-fail &&\n \tgit update-ref refs/remotes/origin/extrabranch main &&\n-\t: this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds  &&\n-\t>.git/packed-refs.new &&\n+\t: this will prevent --prune from locking refs/remotes/origin/extra for deletion &&\n+\t>.git/refs/remotes/origin/extrabranch.lock &&\n \n-\ttest_must_fail git fetch --prune origin\n+\ttest_must_fail git fetch --prune origin > outputs 2> errors\n '\n \n test_expect_success 'fetch --atomic works with a single branch' '\n\nbase-commit: 0f828332d5ac36fc63b7d8202652efa152809856\n-- \ngitgitgadget\n"},{"id":"454991","messageId":"YnizZ/a/hQ6hpRPB@ncase","threadId":"57841","inReplyTo":"pull.1228.git.1651676435634.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs: honor reference-transaction semantics when deleting refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-05-09T06:23:35Z","receivedAt":"2022-05-09T06:28:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 04, 2022 at 03:00:35PM +0000, Michael Heemskerk via GitGitGadget wrote:\n> From: Michael Heemskerk <mheemskerk@atlassian.com>\n> \n> When deleting refs from the loose-files refs backend, files_delete_refs\n> first rewrites packed-refs if any of the to-be-deleted refs were packed\n> and then removes loose refs. While making those changes, it invokes the\n> reference-transaction hook incorrectly; a single transaction is used to\n> prepare and commit the changes to packed-refs, followed by another\n> transaction per deleted ref, each with another prepared and committed\n> reference-transaction hook invocation.\n> \n> This behaviour is problematic for a number of reasons. First of all,\n> deleting a ref through `git branch -d` or `git tag -d` results in a\n> different sequence of reference-transaction callbacks than deleting the\n> same ref through `git update-ref`:\n> \n> - update-ref of a loose ref: aborted, prepared, committed\n> - update-ref of a packed ref: prepared, prepared, committed, commited\n> - branch -d: prepared, committed, aborted, prepared, committed\n> \n> The bigger problem is that the packed-refs update is committed before\n> the prepared reference-transaction invocation is done for the loose\n> ref. Returning a non-zero exit code from the second\n> reference-transaction callback leads to an invalid sequence of\n> reference-transaction callbacks:\n> \n> 1. prepared - hook returns 0, so the change is allowed to go through.\n> 2. committed - git informs the hook that the changes are now final,\n>    but are they really? Any loose refs may still survive if the\n>    subsequent prepared callback is canceled.\n> 3. aborted - 'fake' invocation from the packed-transaction because the\n>    ref does not exist in packed-refs.\n> 4. prepared - hook returns 1, so the change should be blocked.\n> 5. aborted - git informs the hook that it has rolled back the change,\n>    but it really hasn't; packed-refs has already been rewritten and\n>    if the ref only existed as a packed ref, it is now gone.\n> \n> The changes to the reference-transaction invocations that were planned\n> for git 2.36 but have been reverted make the problem more pronounced.\n> Those changes suppress the 'fake' invocations for the packed-transaction\n> (invocations 1-3 in the above list). In that case, the prepared callback\n> in step 4 cannot prevent a ref from being deleted if it only existed in\n> packed-refs.\n> \n> To address the issue, the use a separate transactions to update the\n> packed and loose refs has been removed from files_delete_refs. Instead,\n> it now uses a single transaction, queues up the refs-to-be-deleted and\n> relies on the standard transaction mechanism to invoke the\n> reference-transaction hooks as expected.\n> \n> Signed-off-by: Michael Heemskerk <mheemskerk@atlassian.com>\n> ---\n>     refs: honor reference-transaction semantics when deleting refs\n>     \n>     When deleting refs from the loose-files refs backend, files_delete_refs\n>     first rewrites packed-refs if any of the to-be-deleted refs were packed\n>     and then removes loose refs. While making those changes, it invokes the\n>     reference-transaction hook incorrectly; a single transaction is used to\n>     prepare and commit the changes to packed-refs, followed by another\n>     transaction per deleted ref, each with another prepared and committed\n>     reference-transaction hook invocation.\n>     \n>     This behaviour is problematic for a number of reasons. First of all,\n>     deleting a ref through git branch -d or git tag -d results in a\n>     different sequence of reference-transaction callbacks than deleting the\n>     same ref through git update-ref:\n>     \n>      * update-ref of a loose ref: aborted, prepared, committed\n>      * update-ref of a packed ref: prepared, prepared, committed, commited\n>      * branch -d: prepared, committed, aborted, prepared, committed\n>     \n>     The bigger problem is that the packed-refs update is committed before\n>     the prepared reference-transaction invocation is done for the loose ref.\n>     Returning a non-zero exit code from the second reference-transaction\n>     callback leads to an invalid sequence of reference-transaction\n>     callbacks:\n>     \n>      1. prepared - hook returns 0, so the change is allowed to go through.\n>      2. committed - git informs the hook that the changes are now final, but\n>         are they really? Any loose refs may still survive if the subsequent\n>         prepared callback is canceled.\n>      3. aborted - 'fake' invocation from the packed-transaction because the\n>         ref does not exist in packed-refs.\n>      4. prepared - hook returns 1, so the change should be blocked.\n>      5. aborted - git informs the hook that it has rolled back the change,\n>         but it really hasn't; packed-refs has already been rewritten and if\n>         the ref only existed as a packed ref, it is now gone.\n>     \n>     The changes to the reference-transaction invocations that were planned\n>     for git 2.36 but have been reverted make the problem more pronounced.\n>     Those changes suppress the 'fake' invocations for the packed-transaction\n>     (invocations 1-3 in the above list). In that case, the prepared callback\n>     in step 4 cannot prevent a ref from being deleted if it only existed in\n>     packed-refs.\n>     \n>     To address the issue, the use a separate transactions to update the\n>     packed and loose refs has been removed from files_delete_refs. Instead,\n>     it now uses a single transaction, queues up the refs-to-be-deleted and\n>     relies on the standard transaction mechanism to invoke the\n>     reference-transaction hooks as expected.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1228%2Fmheemskerk%2Ffiles-delete-ref-reference-transaction-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1228/mheemskerk/files-delete-ref-reference-transaction-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1228\n> \n>  refs/files-backend.c             | 33 +++++++--------\n>  t/t1416-ref-transaction-hooks.sh | 70 ++++++++++++++++++++++++++++++++\n>  t/t5510-fetch.sh                 |  6 +--\n>  3 files changed, 87 insertions(+), 22 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 8db7882aacb..5c23277eda7 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1265,44 +1265,39 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n>  \tstruct files_ref_store *refs =\n>  \t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n>  \tstruct strbuf err = STRBUF_INIT;\n> -\tint i, result = 0;\n> +\tint i;\n> +\tstruct ref_transaction *transaction;\n>  \n>  \tif (!refnames->nr)\n>  \t\treturn 0;\n>  \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->base, &err);\n> +\tif (!transaction)\n>  \t\tgoto error;\n> -\t}\n> -\n> -\tpacked_refs_unlock(refs->packed_ref_store);\n>  \n>  \tfor (i = 0; i < refnames->nr; i++) {\n>  \t\tconst char *refname = refnames->items[i].string;\n> -\n> -\t\tif (refs_delete_ref(&refs->base, msg, refname, NULL, flags))\n> -\t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n> +\t\tif (ref_transaction_delete(transaction, refname, NULL,\n> +\t\t\t\t\t   flags, msg, &err))\n> +\t\t\tgoto error;\n>  \t}\n>  \n> +\tif (ref_transaction_commit(transaction, &err))\n> +\t\tgoto error;\n> +\n> +\tref_transaction_free(transaction);\n>  \tstrbuf_release(&err);\n> -\treturn result;\n> +\treturn 0;\n>  \n>  error:\n> -\t/*\n> -\t * If we failed to rewrite the packed-refs file, then it is\n> -\t * unsafe to try to remove loose refs, because doing so might\n> -\t * expose an obsolete packed value for a reference that might\n> -\t * even point at an object that has been garbage collected.\n> -\t */\n>  \tif (refnames->nr == 1)\n>  \t\terror(_(\"could not delete reference %s: %s\"),\n>  \t\t      refnames->items[0].string, err.buf);\n>  \telse\n>  \t\terror(_(\"could not delete references: %s\"), err.buf);\n>  \n> +\tif (transaction)\n> +\t\tref_transaction_free(transaction);\n>  \tstrbuf_release(&err);\n>  \treturn -1;\n>  }\n\nI really like these changes given that they simplify things, but I\nwonder whether we can do them. In the preimage we're eagerly removing\nloose refs: any error encountered when deleting a reference is recorded,\nbut we keep on trying to remove the other refs, as well. With the new\nbehaviour we now create a single transaction for all refs and try to\ncommit it. This also means that we'll abort the transaction when locking\nany of the refs fails, which is a change in behaviour.\n\nThe current behaviour is explicitly documented in `refs.h:refs_delete_refs()`:\n\n    /*\n     * Delete the specified references. If there are any problems, emit\n     * errors but attempt to keep going (i.e., the deletes are not done in\n     * an all-or-nothing transaction). msg and flags are passed through to\n     * ref_transaction_delete().\n     */\n    int refs_delete_refs(struct ref_store *refs, const char *msg,\n                         struct string_list *refnames, unsigned int flags);\n\nThere are multiple callsites of this function via `delete_refs()`. Now\nhonestly, most of these callsites look somewhat broken:\n\n    - `bisect.c` simply does its best to clean up bisect state. This\n      usecase looks fine to me.\n\n    - `builtin/branch.c` reports the branches as deleted even if\n      `delete_refs()` failed.\n\n    - `builtin/remote.c` also misreports the deleted branches for the\n      `prune` verb. The `rm` verb looks alright: if deletion of any\n      branch failed then it doesn't prune the remote's config in the end\n      and reports an error.\n\n    - `builtin/fetch.c` also misreports deleted branches with `--prune`.\n\nSo most of these commands incorrectly handle the case where only a\nsubset of branches has been deleted. This raises the question whether\nthe interface provided by `refs_delete_refs()` is actually sensible if\nit's so easy to get wrong. It doesn't even report which branches could\nbe removed and which couldn't. Furthermore, the question is whether new\nbackends like the reftable backend which write all refs into a single\nslice would actually even be in a position to efficiently retain\nsemantics of this function.\n\nI'm torn. There are valid usecases for eagerly deleting refs even if a\nsubset of deletions failed, making this change a tough sell, but most of\nthe callsites don't actually handle this correctly in the first place.\n\n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> index 27731722a5b..e3d4fe624f7 100755\n> --- a/t/t1416-ref-transaction-hooks.sh\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> @@ -133,4 +133,74 @@ test_expect_success 'interleaving hook calls succeed' '\n>  \ttest_cmp expect target-repo.git/actual\n>  '\n>  \n> +test_expect_success 'hook allows deleting loose ref if successful' '\n> +\ttest_when_finished \"rm actual\" &&\n> +\tgit branch to-be-deleted $PRE_OID &&\n> +\ttest_hook reference-transaction <<-\\EOF &&\n> +\t\techo \"$*\" >>actual\n> +\tEOF\n> +\tcat >expect <<-EOF &&\n> +\t\taborted\n> +\t\tprepared\n> +\t\tcommitted\n> +\tEOF\n> +\tgit branch -d to-be-deleted &&\n> +\ttest_cmp expect actual &&\n> +\ttest_must_fail git rev-parse refs/heads/to-be-deleted\n> +'\n> +\n> +test_expect_success 'hook allows deleting packed ref if successful' '\n> +\ttest_when_finished \"rm actual\" &&\n> +\tgit branch to-be-deleted $PRE_OID &&\n> +\tgit pack-refs --all --prune &&\n> +\ttest_hook reference-transaction <<-\\EOF &&\n> +\t\techo \"$*\" >>actual\n> +\tEOF\n> +\tcat >expect <<-EOF &&\n> +\t\tprepared\n> +\t\tprepared\n> +\t\tcommitted\n> +\t\tcommitted\n> +\tEOF\n> +\tgit branch -d to-be-deleted &&\n> +\ttest_cmp expect actual &&\n> +\ttest_must_fail git rev-parse refs/heads/to-be-deleted\n> +'\n> +\n> +test_expect_success 'hook aborts deleting loose ref in prepared state' '\n> +\ttest_when_finished \"rm actual\" &&\n> +\ttest_when_finished \"git branch -d to-be-deleted\" &&\n> +\tgit branch to-be-deleted $PRE_OID &&\n> +\ttest_hook reference-transaction <<-\\EOF &&\n> +\t\techo \"$*\" >>actual\n> +\t\texit 1\n> +\tEOF\n> +\tcat >expect <<-EOF &&\n> +\t\taborted\n> +\t\tprepared\n> +\t\taborted\n> +\tEOF\n\nIt's not really clear why we get the first \"aborted\" result here. I know\nthat it is because we didn't queue up any refs to the packed backend,\nand thus we don't even try to prepare the transaction. But it's likely\nconfusing to a reader and might thus warrant a comment. On the other\nhand this is going away anyway if and when my patch series lands again\nthat stops calling the hook from the nested packed backend.\n\n> +\ttest_must_fail git branch -d to-be-deleted &&\n> +\ttest_cmp expect actual &&\n> +\tgit rev-parse refs/heads/to-be-deleted\n> +'\n> +\n> +test_expect_success 'hook aborts deleting packed ref in prepared state' '\n> +\ttest_when_finished \"rm actual\" &&\n> +\ttest_when_finished \"git branch -d to-be-deleted\" &&\n> +\tgit branch to-be-deleted $PRE_OID &&\n> +\tgit pack-refs --all --prune &&\n> +\ttest_hook reference-transaction <<-\\EOF &&\n> +\t\techo \"$*\" >>actual\n> +\t\texit 1\n> +\tEOF\n> +\tcat >expect <<-EOF &&\n> +\t\tprepared\n> +\t\taborted\n> +\tEOF\n> +\ttest_must_fail git branch -d to-be-deleted &&\n> +\ttest_cmp expect actual &&\n> +\tgit rev-parse refs/heads/to-be-deleted\n> +'\n> +\n\nFrom your description one more interesting case is when the packed-refs\ntransaction is committed, but the loose-refs backend is aborted. It's a\nbit harder to test given that we have no way to indicate what backend\nthe reftx hook is being invoked from though.\n\nOne thing that worries me is that these patches kind of set the current\nbehaviour of driving the reftx hook via both packed and loose backend\ninto stone. My patch series that got reverted is going to change that\nbehaviour though so that we don't execute the hook from the packed\nbackend, and consequentially we'd have to change all these tests again\nto match the new behaviour. This makes it a lot harder to argue though\nthat we can safely switch to the new behaviour without breaking any\nassumptions when we even codified our current assumptions into tests.\n\nTaking a step back I wonder whether my previous approach to just hide\nthe hook for the packed backend was the right thing to do though. An\nalternative would be to instead expose additional information to the\nhook so that it can decide by itself whether it wants to execute the\nhook or not. This could e.g. trivially be done by exposing a new\n\"GIT_REFS_BACKEND\" environment variable to the reftx hook that gets set\nto either \"packed-refs\", \"loose-refs\" or \"reftables\" depending on which\nbackend is currently in use. Equipped with this information a hook\nscript can then easily ignore any updates to the packed-refs file by\nitself without having to change the way it is invoked right now and thus\nwe wouldn't regress any currently existing hooks.\n\n>  test_done\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 4620f0ca7fa..8b09a99c2e8 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -169,10 +169,10 @@ test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n>  \tgit clone . prune-fail &&\n>  \tcd prune-fail &&\n>  \tgit update-ref refs/remotes/origin/extrabranch main &&\n> -\t: this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds  &&\n> -\t>.git/packed-refs.new &&\n> +\t: this will prevent --prune from locking refs/remotes/origin/extra for deletion &&\n> +\t>.git/refs/remotes/origin/extrabranch.lock &&\n>  \n> -\ttest_must_fail git fetch --prune origin\n> +\ttest_must_fail git fetch --prune origin > outputs 2> errors\n\nIt would be nice to have an explanation why exactly this change is\nneeded, and why it is fine that the visible behaviour changes here.\n\nPatrick\n\n>  \n>  test_expect_success 'fetch --atomic works with a single branch' '\n> \n> base-commit: 0f828332d5ac36fc63b7d8202652efa152809856\n> -- \n> gitgitgadget\n"},{"id":"454995","messageId":"CAJDSCnPfGhL+_AL5Q1uAs8LLvW=Yckx=Ev0tQy1myCvyNF955g@mail.gmail.com","threadId":"57841","inReplyTo":"YnizZ/a/hQ6hpRPB@ncase","subject":"Re: [PATCH] refs: honor reference-transaction semantics when deleting refs","fromName":"Michael Heemskerk","fromEmail":"mheemskerk@atlassian.com","sentAt":"2022-05-09T10:18:51Z","receivedAt":"2022-05-09T10:19:18Z","isPatch":true,"sender":{"key":"mheemskerk@atlassian.com","avatar":"https://gravatar.com/avatar/c3ebe086c3c55529246291e9879e5ef7af3c0ffcadbd81b5508b5fcdf66aa065?d=mp&s=160"},"body":"On Mon, May 9, 2022 at 8:23 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> I really like these changes given that they simplify things, but I\n> wonder whether we can do them. In the preimage we're eagerly removing\n> loose refs: any error encountered when deleting a reference is recorded,\n> but we keep on trying to remove the other refs, as well. With the new\n> behaviour we now create a single transaction for all refs and try to\n> commit it. This also means that we'll abort the transaction when locking\n> any of the refs fails, which is a change in behaviour.\n>\n> The current behaviour is explicitly documented in `refs.h:refs_delete_refs()`:\n>\n>     /*\n>      * Delete the specified references. If there are any problems, emit\n>      * errors but attempt to keep going (i.e., the deletes are not done in\n>      * an all-or-nothing transaction). msg and flags are passed through to\n>      * ref_transaction_delete().\n>      */\n>     int refs_delete_refs(struct ref_store *refs, const char *msg,\n>                          struct string_list *refnames, unsigned int flags);\n>\n> There are multiple callsites of this function via `delete_refs()`. Now\n> honestly, most of these callsites look somewhat broken:\n>\n>     - `bisect.c` simply does its best to clean up bisect state. This\n>       usecase looks fine to me.\n>\n>     - `builtin/branch.c` reports the branches as deleted even if\n>       `delete_refs()` failed.\n>\n>     - `builtin/remote.c` also misreports the deleted branches for the\n>       `prune` verb. The `rm` verb looks alright: if deletion of any\n>       branch failed then it doesn't prune the remote's config in the end\n>       and reports an error.\n>\n>     - `builtin/fetch.c` also misreports deleted branches with `--prune`.\n>\n> So most of these commands incorrectly handle the case where only a\n> subset of branches has been deleted. This raises the question whether\n> the interface provided by `refs_delete_refs()` is actually sensible if\n> it's so easy to get wrong. It doesn't even report which branches could\n> be removed and which couldn't. Furthermore, the question is whether new\n> backends like the reftable backend which write all refs into a single\n> slice would actually even be in a position to efficiently retain\n> semantics of this function.\n>\n> I'm torn. There are valid usecases for eagerly deleting refs even if a\n> subset of deletions failed, making this change a tough sell, but most of\n> the callsites don't actually handle this correctly in the first place.\n\nExactly; this is the reason that I initially suggested fixing the issue by\njust removing the upfront rewrite of packed-refs. With that rewrite removed,\nthe refs-to-be-deleted are deleted in individual transactions, which may or\nmay not rewrite packed-refs. The downside, as you correctly pointed out,\nis that we could end up rewriting packed-refs multiple times, which could\ncome at a significant performance penalty for repositories with large\npacked-refs files.\n\nUnfortunately, the current approach of updating packed-refs in one\ntransaction and updating the loose refs in individual transactions doesn't\nwork either.\n\nSo what are our options?\n\n- delete each of the refs in a separate transaction and pay a (potentially\n  significant) performance penalty in repositories with large packed-refs\n  files when deleting many refs. I'll note that this scenario is similar to\n  deleting a set of refs through a non-atomic push.\n- switch to a single transaction and update refs.h:refs_delete_refs to use\n  an all-or-nothing approach (the approach I've taken in my patch).\n- improve the reference-transaction mechanism to support the\n  'batch-of-transactions' mode more efficiently. If I remember correctly,\n  something like that has been suggested before, but I'm not sure if it's\n  actually been built or spiked. In this batch-of-transactions mode, git\n  could try to prepare all refs, and only invoke the hook for the refs that\n  could be successfully prepared. The hook should then be able to\n  reject individual ref updates, and git would then apply only the\n  non-rejected ref updates. While such a change would make many\n  scenarios where multiple refs are being updated more efficient, it's also\n  a much bigger change that's hard to make without breaking the current\n  reference-transaction protocol.\n\nSticking to a transaction per ref and rewriting packed-refs multiple times\nis the safer option. Deleting the refs in a single transaction is the more\nperformant option, but changes the behavior. A stay/stale lock file could\nthen make it impossible to remove a remote, or to prune /refs/remotes/\nrefs.\n\nMy suggestion would be to stick to a transaction per ref and pay the\nsame performance penalty as you'd get when deleting many refs through\na non-atomic push.\n\n> > +test_expect_success 'hook aborts deleting loose ref in prepared state' '\n> > +     test_when_finished \"rm actual\" &&\n> > +     test_when_finished \"git branch -d to-be-deleted\" &&\n> > +     git branch to-be-deleted $PRE_OID &&\n> > +     test_hook reference-transaction <<-\\EOF &&\n> > +             echo \"$*\" >>actual\n> > +             exit 1\n> > +     EOF\n> > +     cat >expect <<-EOF &&\n> > +             aborted\n> > +             prepared\n> > +             aborted\n> > +     EOF\n>\n> It's not really clear why we get the first \"aborted\" result here. I know\n> that it is because we didn't queue up any refs to the packed backend,\n> and thus we don't even try to prepare the transaction. But it's likely\n> confusing to a reader and might thus warrant a comment. On the other\n> hand this is going away anyway if and when my patch series lands again\n> that stops calling the hook from the nested packed backend.\n\nI can add a comment explaining why we get the 'aborted' callback\nand also add a comment that that 'aborted' callback is expected to be\nremoved in an upcoming version.\n\n> > +     test_must_fail git branch -d to-be-deleted &&\n> > +     test_cmp expect actual &&\n> > +     git rev-parse refs/heads/to-be-deleted\n> > +'\n> > +\n> > +test_expect_success 'hook aborts deleting packed ref in prepared state' '\n> > +     test_when_finished \"rm actual\" &&\n> > +     test_when_finished \"git branch -d to-be-deleted\" &&\n> > +     git branch to-be-deleted $PRE_OID &&\n> > +     git pack-refs --all --prune &&\n> > +     test_hook reference-transaction <<-\\EOF &&\n> > +             echo \"$*\" >>actual\n> > +             exit 1\n> > +     EOF\n> > +     cat >expect <<-EOF &&\n> > +             prepared\n> > +             aborted\n> > +     EOF\n> > +     test_must_fail git branch -d to-be-deleted &&\n> > +     test_cmp expect actual &&\n> > +     git rev-parse refs/heads/to-be-deleted\n> > +'\n> > +\n>\n> From your description one more interesting case is when the packed-refs\n> transaction is committed, but the loose-refs backend is aborted. It's a\n> bit harder to test given that we have no way to indicate what backend\n> the reftx hook is being invoked from though.\n\nThat is indeed the more interesting case, and exactly the case we saw\nfail when the packed-refs reference-transactions callbacks had been\nsuppressed in 2.36.0-rc1 and -rc2. But as you said, they're pretty hard to\ntest. In Bitbucket's tests we now verify from the reference-transaction\ncallback that the ref-updates have not been committed (yet) when the\nprepared or aborted callback is received, and that they _have_ been\ncommitted when the committed callback is received. Perhaps a similar\napproach could be used in git's reference-transaction tests for more\nthorough testing.\n\n> One thing that worries me is that these patches kind of set the current\n> behaviour of driving the reftx hook via both packed and loose backend\n> into stone. My patch series that got reverted is going to change that\n> behaviour though so that we don't execute the hook from the packed\n> backend, and consequentially we'd have to change all these tests again\n> to match the new behaviour. This makes it a lot harder to argue though\n> that we can safely switch to the new behaviour without breaking any\n> assumptions when we even codified our current assumptions into tests.\n\nThe counter argument to that is that it's kind of scary if you could remove\n half of the reference-transaction callbacks without needing to update a\ntest. I'd rather have tests that verify current behavior that you need to\nupdate when you intentionally change the behavior, then not have those\ntests?\n\n> Taking a step back I wonder whether my previous approach to just hide\n> the hook for the packed backend was the right thing to do though. An\n> alternative would be to instead expose additional information to the\n> hook so that it can decide by itself whether it wants to execute the\n> hook or not. This could e.g. trivially be done by exposing a new\n> \"GIT_REFS_BACKEND\" environment variable to the reftx hook that gets set\n> to either \"packed-refs\", \"loose-refs\" or \"reftables\" depending on which\n> backend is currently in use. Equipped with this information a hook\n> script can then easily ignore any updates to the packed-refs file by\n> itself without having to change the way it is invoked right now and thus\n> we wouldn't regress any currently existing hooks.\n\nFrom the reference-transaction hook writer's perspective, the backend\ninvolved is an implementation detail that the hook should not have to\ncare about. Getting separate callbacks for the loose and the packed\nbackends makes it a lot harder to write a good reference-transaction\nhook, especially when the callbacks differ if a ref is packed or not.\n\nIMO, there should really be a \"files\" backend transaction, that internally\ntakes care of locking individual refs and possibly \"packed-refs\" in case\nof a deletion. In addition, not getting \"artificial\" packed callbacks also\nsaves us a few extra reference-transaction callbacks when deleting refs.\nEven if those take only a few ms per invocation, when deleting hundreds\nof refs, it's still something that we'd like to avoid if we can.\n\n> >  test_done\n> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> > index 4620f0ca7fa..8b09a99c2e8 100755\n> > --- a/t/t5510-fetch.sh\n> > +++ b/t/t5510-fetch.sh\n> > @@ -169,10 +169,10 @@ test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n> >       git clone . prune-fail &&\n> >       cd prune-fail &&\n> >       git update-ref refs/remotes/origin/extrabranch main &&\n> > -     : this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds  &&\n> > -     >.git/packed-refs.new &&\n> > +     : this will prevent --prune from locking refs/remotes/origin/extra for deletion &&\n> > +     >.git/refs/remotes/origin/extrabranch.lock &&\n> >\n> > -     test_must_fail git fetch --prune origin\n> > +     test_must_fail git fetch --prune origin > outputs 2> errors\n>\n> It would be nice to have an explanation why exactly this change is\n> needed, and why it is fine that the visible behaviour changes here.\n\nThe  test was relying on the fact that the packed-refs file was locked when\ngit fetch --prune is called. My patch replaces that unconditional lock with\na transaction. The transaction only takes out the lock if packed-refs\nactually needs to be updated, and since the ref being pruned only exists\nas a loose ref, git fetch --prune failed to fail.\n\nI've replaced the lock on packed-refs with a lock on the loose ref that\nshould be pruned.\n\nMichael\n"},{"id":"455002","messageId":"YnkbxByewguRY5sZ@ncase","threadId":"57841","inReplyTo":"CAJDSCnPfGhL+_AL5Q1uAs8LLvW=Yckx=Ev0tQy1myCvyNF955g@mail.gmail.com","subject":"Re: [PATCH] refs: honor reference-transaction semantics when deleting refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-05-09T13:48:52Z","receivedAt":"2022-05-09T13:49:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 09, 2022 at 12:18:51PM +0200, Michael Heemskerk wrote:\n> On Mon, May 9, 2022 at 8:23 AM Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > I really like these changes given that they simplify things, but I\n> > wonder whether we can do them. In the preimage we're eagerly removing\n> > loose refs: any error encountered when deleting a reference is recorded,\n> > but we keep on trying to remove the other refs, as well. With the new\n> > behaviour we now create a single transaction for all refs and try to\n> > commit it. This also means that we'll abort the transaction when locking\n> > any of the refs fails, which is a change in behaviour.\n> >\n> > The current behaviour is explicitly documented in `refs.h:refs_delete_refs()`:\n> >\n> >     /*\n> >      * Delete the specified references. If there are any problems, emit\n> >      * errors but attempt to keep going (i.e., the deletes are not done in\n> >      * an all-or-nothing transaction). msg and flags are passed through to\n> >      * ref_transaction_delete().\n> >      */\n> >     int refs_delete_refs(struct ref_store *refs, const char *msg,\n> >                          struct string_list *refnames, unsigned int flags);\n> >\n> > There are multiple callsites of this function via `delete_refs()`. Now\n> > honestly, most of these callsites look somewhat broken:\n> >\n> >     - `bisect.c` simply does its best to clean up bisect state. This\n> >       usecase looks fine to me.\n> >\n> >     - `builtin/branch.c` reports the branches as deleted even if\n> >       `delete_refs()` failed.\n> >\n> >     - `builtin/remote.c` also misreports the deleted branches for the\n> >       `prune` verb. The `rm` verb looks alright: if deletion of any\n> >       branch failed then it doesn't prune the remote's config in the end\n> >       and reports an error.\n> >\n> >     - `builtin/fetch.c` also misreports deleted branches with `--prune`.\n> >\n> > So most of these commands incorrectly handle the case where only a\n> > subset of branches has been deleted. This raises the question whether\n> > the interface provided by `refs_delete_refs()` is actually sensible if\n> > it's so easy to get wrong. It doesn't even report which branches could\n> > be removed and which couldn't. Furthermore, the question is whether new\n> > backends like the reftable backend which write all refs into a single\n> > slice would actually even be in a position to efficiently retain\n> > semantics of this function.\n> >\n> > I'm torn. There are valid usecases for eagerly deleting refs even if a\n> > subset of deletions failed, making this change a tough sell, but most of\n> > the callsites don't actually handle this correctly in the first place.\n> \n> Exactly; this is the reason that I initially suggested fixing the issue by\n> just removing the upfront rewrite of packed-refs. With that rewrite removed,\n> the refs-to-be-deleted are deleted in individual transactions, which may or\n> may not rewrite packed-refs. The downside, as you correctly pointed out,\n> is that we could end up rewriting packed-refs multiple times, which could\n> come at a significant performance penalty for repositories with large\n> packed-refs files.\n> \n> Unfortunately, the current approach of updating packed-refs in one\n> transaction and updating the loose refs in individual transactions doesn't\n> work either.\n> \n> So what are our options?\n> \n> - delete each of the refs in a separate transaction and pay a (potentially\n>   significant) performance penalty in repositories with large packed-refs\n>   files when deleting many refs. I'll note that this scenario is similar to\n>   deleting a set of refs through a non-atomic push.\n\nBy chance I know that on gitlab.com we've had huge performance issues\nwith access patterns like this. In some cases with repos that have\nmillions of refs I have seen that certain actions were completely\ndominated by rewriting the packed-refs file. So I'd rather avoid going\nthere given that it would be a serious performance regression.\n\n> - switch to a single transaction and update refs.h:refs_delete_refs to use\n>   an all-or-nothing approach (the approach I've taken in my patch).\n\nThis is the easiest approach, but also backwards incompatible as I've\nlayed out above. I personally wouldn't mind, and as said I think that\nmost of the usecases are broken anyway because of implementations which\nmisreport the case where we only partially deleted refs. But it's not a\ndecision we can make without more discussion, I guess.\n\n> - improve the reference-transaction mechanism to support the\n>   'batch-of-transactions' mode more efficiently. If I remember correctly,\n>   something like that has been suggested before, but I'm not sure if it's\n>   actually been built or spiked. In this batch-of-transactions mode, git\n>   could try to prepare all refs, and only invoke the hook for the refs that\n>   could be successfully prepared. The hook should then be able to\n>   reject individual ref updates, and git would then apply only the\n>   non-rejected ref updates. While such a change would make many\n>   scenarios where multiple refs are being updated more efficient, it's also\n>   a much bigger change that's hard to make without breaking the current\n>   reference-transaction protocol.\n\nPeff and I had such a discussion in the past, and having transactions\nwith eager semantics would also fix some edge cases we have in other\nparts of the codebase. You already mentioned git-fetch(1) without\n`--atomic`, and there are likely others that could benefit.\n\nI did have a go at this several times already, but none of the\napproaches I took resulted in a clean-to-use API. I consider this to be\nthe best solution, but also the hardest one to implement, unfortunately.\n\n> Sticking to a transaction per ref and rewriting packed-refs multiple times\n> is the safer option. Deleting the refs in a single transaction is the more\n> performant option, but changes the behavior. A stay/stale lock file could\n> then make it impossible to remove a remote, or to prune /refs/remotes/\n> refs.\n> \n> My suggestion would be to stick to a transaction per ref and pay the\n> same performance penalty as you'd get when deleting many refs through\n> a non-atomic push.\n\nI don't think we should do this. The one-transaction-per-ref issue is\nwhy I added the `--atomic` flag to git-fetch(1) in the first place,\nbecause it has been biting us in repos with millions of refs. Quoting\n583bc41923 (fetch: make `--atomic` flag cover pruning of refs,\n2022-02-17), which introduces this flag:\n\n    Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)\n      Time (mean ± σ):      2.366 s ±  0.021 s    [User: 0.858 s, System: 1.508 s]\n      Range (min … max):    2.328 s …  2.407 s    10 runs\n\n    Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)\n      Time (mean ± σ):      1.369 s ±  0.017 s    [User: 0.715 s, System: 0.641 s]\n      Range (min … max):    1.346 s …  1.400 s    10 runs\n\n    Summary\n      'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran\n        1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'\n\nAnd this is only with a 100k references. The issue gets a lot worse when\nyou're in the millions of refs, both because you have 10x more refs to\nprune, but also because the packed-refs file is 10x larger. So\nperformance doesn't scale linearly with the number of refs, but is a\nproduct of file size and number of refs.\n\n[snip]\n> > One thing that worries me is that these patches kind of set the current\n> > behaviour of driving the reftx hook via both packed and loose backend\n> > into stone. My patch series that got reverted is going to change that\n> > behaviour though so that we don't execute the hook from the packed\n> > backend, and consequentially we'd have to change all these tests again\n> > to match the new behaviour. This makes it a lot harder to argue though\n> > that we can safely switch to the new behaviour without breaking any\n> > assumptions when we even codified our current assumptions into tests.\n> \n> The counter argument to that is that it's kind of scary if you could remove\n>  half of the reference-transaction callbacks without needing to update a\n> test. I'd rather have tests that verify current behavior that you need to\n> update when you intentionally change the behavior, then not have those\n> tests?\n\nOh, I certainly agree there. And we have stated in the past that the\nreftx hook is _not_ providing a stable interface with regards to exactly\nwhen it is called. We only want to guarantee that it is called on a\ntransaction.\n\n> > Taking a step back I wonder whether my previous approach to just hide\n> > the hook for the packed backend was the right thing to do though. An\n> > alternative would be to instead expose additional information to the\n> > hook so that it can decide by itself whether it wants to execute the\n> > hook or not. This could e.g. trivially be done by exposing a new\n> > \"GIT_REFS_BACKEND\" environment variable to the reftx hook that gets set\n> > to either \"packed-refs\", \"loose-refs\" or \"reftables\" depending on which\n> > backend is currently in use. Equipped with this information a hook\n> > script can then easily ignore any updates to the packed-refs file by\n> > itself without having to change the way it is invoked right now and thus\n> > we wouldn't regress any currently existing hooks.\n> \n> From the reference-transaction hook writer's perspective, the backend\n> involved is an implementation detail that the hook should not have to\n> care about. Getting separate callbacks for the loose and the packed\n> backends makes it a lot harder to write a good reference-transaction\n> hook, especially when the callbacks differ if a ref is packed or not.\n>\n> IMO, there should really be a \"files\" backend transaction, that internally\n> takes care of locking individual refs and possibly \"packed-refs\" in case\n> of a deletion. In addition, not getting \"artificial\" packed callbacks also\n> saves us a few extra reference-transaction callbacks when deleting refs.\n> Even if those take only a few ms per invocation, when deleting hundreds\n> of refs, it's still something that we'd like to avoid if we can.\n\nTrue. In this case it's just an alternative way to tackle this specific\nissue given that one wouldn't typically care about the packed-refs\nchanges. And it's very much an artifact of the files-backend really\nbeing two backends in one. In any case I can see arguments for both\napproaches, and ultimately I agree that it would be best if the\nfiles-backend behaved as if it was a single one.\n\n> > >  test_done\n> > > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> > > index 4620f0ca7fa..8b09a99c2e8 100755\n> > > --- a/t/t5510-fetch.sh\n> > > +++ b/t/t5510-fetch.sh\n> > > @@ -169,10 +169,10 @@ test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n> > >       git clone . prune-fail &&\n> > >       cd prune-fail &&\n> > >       git update-ref refs/remotes/origin/extrabranch main &&\n> > > -     : this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds  &&\n> > > -     >.git/packed-refs.new &&\n> > > +     : this will prevent --prune from locking refs/remotes/origin/extra for deletion &&\n> > > +     >.git/refs/remotes/origin/extrabranch.lock &&\n> > >\n> > > -     test_must_fail git fetch --prune origin\n> > > +     test_must_fail git fetch --prune origin > outputs 2> errors\n> >\n> > It would be nice to have an explanation why exactly this change is\n> > needed, and why it is fine that the visible behaviour changes here.\n> \n> The  test was relying on the fact that the packed-refs file was locked when\n> git fetch --prune is called. My patch replaces that unconditional lock with\n> a transaction. The transaction only takes out the lock if packed-refs\n> actually needs to be updated, and since the ref being pruned only exists\n> as a loose ref, git fetch --prune failed to fail.\n> \n> I've replaced the lock on packed-refs with a lock on the loose ref that\n> should be pruned.\n\nAh, thanks. I was missing the fact that the lock for the packed-refs\nfile was taken unconditionally before.\n\nPatrick\n"}]}