On Fri, Jul 29, 2022 at 12:20 PM Jiang Xin [off-list ref] wrote:

From: Jiang Xin <redacted>

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