On Fri, Jul 29, 2022 at 12:20 PM Jiang Xin <worldhello.net@xxxxxxxxx> wrote: > > From: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx> > > If there are references to be deleted in a transaction, we should remove > each reference from both loose references and the "packed-refs" file. > The "reference-transaction" hook will run twice, once for the primary > ref-store (loose references), and another for the second ref-store (i.e. > packed ref-store). > > To avoid duplicate running of the "reference-trancaction" hook, we pass > a special "hook-flags" parameter to initialize the second ref-store. > The "REF_TRANSACTION_RUN_PREPARED_HOOK" bit is preserved for the > transaction of the second ref-store because we may still want to call > command "reference-trancaction prepared" for some pre-checks, such as > terminate unwanted transaction for the "packed-refs" file. Can you elaborate on the rationale for continuing to invoke the "prepared" reference-transaction hook for the "packed-refs" file? Did you have a specific type of check in mind? As far as I can tell, any ref update included in this 'extra' callback is also provided to the "prepared" callback for the "files" backend, and that "files" callback always happens - even when deleting a ref that only exists in packed-refs. As a result, it _should_ be safe to also suppress the "prepared" callback for the "packed-refs" backend. Patrick has a patch series that does exactly that (suppress all "packed-refs" reference-transaction callbacks). Your patch series will achieve the same (and more) if you change > + packed_transaction = ref_store_transaction_begin_extended( > + refs->packed_ref_store, > + transaction->hook_flags & > + REF_TRANSACTION_RUN_PREPARED_HOOK, > + err); to > + packed_transaction = ref_store_transaction_begin_extended( > + refs->packed_ref_store, > + 0, > + err); For illustration, here are some callbacks I see with your patches applied (without the change I suggested above): deleting a ref that only exists as a loose ref: 1. prepared (for 'files') d6edcbf924697ab811a867421dab60d954ccad99 0000000000000000000000000000000000000000 refs/heads/basic_branching 2. committed (for 'files') d6edcbf924697ab811a867421dab60d954ccad99 0000000000000000000000000000000000000000 refs/heads/basic_branching deleting a ref that only exists in packed-refs: 1. prepared (for 'packed-refs') 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/basic_branching 2. prepared (for 'files') d6edcbf924697ab811a867421dab60d954ccad99 0000000000000000000000000000000000000000 refs/heads/basic_branching 3. committed (for 'files') d6edcbf924697ab811a867421dab60d954ccad99 0000000000000000000000000000000000000000 refs/heads/basic_branching deleting a ref that only exists both in packed-refs and as a loose ref: 1. prepared (for 'packed-refs') 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/basic_branching 2. prepared (for 'files') d6edcbf924697ab811a867421dab60d954ccad99 0000000000000000000000000000000000000000 refs/heads/basic_branching 3. committed (for 'files') d6edcbf924697ab811a867421dab60d954ccad99 0000000000000000000000000000000000000000 refs/heads/basic_branching a mixed update, containing some adds, deletes and updates where some refs existed in packed-refs: 1. prepared (for 'packed-refs') 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/branch_mod_merge 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/basic_branching 2. prepared (for 'files') 6053a1eaa1c009dd11092d09a72f3c41af1b59ad 0000000000000000000000000000000000000000 refs/heads/branch_mod_merge d6edcbf924697ab811a867421dab60d954ccad99 0000000000000000000000000000000000000000 refs/heads/basic_branching 2e10dd2d1d5eea9291b296e78312e8a703964a95 9fbc34a0d905950131d73f352abe68520c6db2a3 refs/heads/out_of_order_branch 0000000000000000000000000000000000000000 9b4e781dea0769888fe270e06ad76675f73851b1 refs/tags/retagged_2 2. committed (for 'files') 6053a1eaa1c009dd11092d09a72f3c41af1b59ad 0000000000000000000000000000000000000000 refs/heads/branch_mod_merge d6edcbf924697ab811a867421dab60d954ccad99 0000000000000000000000000000000000000000 refs/heads/basic_branching 2e10dd2d1d5eea9291b296e78312e8a703964a95 9fbc34a0d905950131d73f352abe68520c6db2a3 refs/heads/out_of_order_branch 0000000000000000000000000000000000000000 9b4e781dea0769888fe270e06ad76675f73851b1 refs/tags/retagged_2 In all of these cases, the "prepared (for 'packed-refs')" callback seems unnecessary overhead? > The behavior of the following git commands and five testcases have been > fixed in t1416: > > * git cherry-pick > * git rebase > * git revert > * git update-ref -d <ref> > * git update-ref --stdin # delete refs Thanks for adding (and fixing) these test cases! We have a similar set of test cases in Bitbucket Server to validate that we're interpreting the reference-transaction callbacks correctly in various scenarios. These tests still pass against git with your patch series applied, so from our end they look good. Cheers, Michael Heemskerk
