Re: [PATCH 5/9] refs: avoid duplicate running of the reference-transaction hook

From: Michael Heemskerk <mheemskerk@atlassian.com>
Date: Aug 2, 2022, 12:18 UTC
Message-ID: <CAJDSCnOFSnhOU06UHF6T9RT31Gq56BhJn5fQ4_ys2ERrN1stSw@mail.gmail.com>
In-Reply-To: <20220729101245.6469-6-worldhello.net@gmail.com>

On Fri, Jul 29, 2022 at 12:20 PM Jiang Xin <worldhello.net@gmail.com> wrote:

Show 15 quoted lines

>
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> 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

Show 5 quoted lines

> +                               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?

Show 8 quoted lines

> 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