From: Michael Heemskerk <mheemskerk@atlassian.com>
To: Jiang Xin <worldhello.net@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
Patrick Steinhardt <ps@pks.im>, Git List <git@vger.kernel.org>,
Jiang Xin <zhiyou.jx@alibaba-inc.com>
Subject: Re: [PATCH 5/9] refs: avoid duplicate running of the reference-transaction hook
Date: Tue, 2 Aug 2022 14:18:33 +0200 [thread overview]
Message-ID: <CAJDSCnOFSnhOU06UHF6T9RT31Gq56BhJn5fQ4_ys2ERrN1stSw@mail.gmail.com> (raw)
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:
>
> 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
> + 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
next prev parent reply other threads:[~2022-08-02 12:19 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-29 10:12 [PATCH 0/9] Fix issues of reference-transaction hook for various git commands Jiang Xin
2022-07-29 10:12 ` [PATCH 1/9] t1416: more testcases for reference-transaction hook Jiang Xin
2022-07-30 6:44 ` Eric Sunshine
2022-07-31 3:25 ` Jiang Xin
2022-07-29 10:12 ` [PATCH 2/9] refs: update missing old-oid in transaction from lockfile Jiang Xin
2022-07-29 10:12 ` [PATCH 3/9] refs: add new field in transaction for running transaction hook Jiang Xin
2022-07-29 10:12 ` [PATCH 4/9] refs: do not run transaction hook for git-pack-refs Jiang Xin
2022-07-29 10:12 ` [PATCH 5/9] refs: avoid duplicate running of the reference-transaction hook Jiang Xin
2022-08-02 12:18 ` Michael Heemskerk [this message]
2022-08-05 1:41 ` Jiang Xin
2022-08-19 3:21 ` [PATCH v2 0/9] Fix issues of refx-txn hook for various git commands Jiang Xin
2022-08-19 3:21 ` [PATCH v2 1/9] t1416: more testcases for reference-transaction hook Jiang Xin
2022-08-19 3:21 ` [PATCH v2 2/9] refs: update missing old-oid in transaction from lockfile Jiang Xin
2022-08-19 3:21 ` [PATCH v2 3/9] refs: add new field in transaction for running transaction hook Jiang Xin
2022-08-19 3:21 ` [PATCH v2 4/9] refs: do not run transaction hook for git-pack-refs Jiang Xin
2022-08-19 3:21 ` [PATCH v2 5/9] refs: avoid duplicate running of the reference-transaction hook Jiang Xin
2022-08-19 3:21 ` [PATCH v2 6/9] refs: add reflog_info to hold more fields for reflog entry Jiang Xin
2022-08-19 3:21 ` [PATCH v2 7/9] refs: get error message via refs_update_ref_extended() Jiang Xin
2022-08-19 3:21 ` [PATCH v2 8/9] refs: reimplement files_copy_or_rename_ref() to run refs-txn hook Jiang Xin
2022-08-19 3:21 ` [PATCH v2 9/9] refs: reimplement refs_delete_refs() and run hook once Jiang Xin
2022-07-29 10:12 ` [PATCH 6/9] refs: add reflog_info to hold more fields for reflog entry Jiang Xin
2022-08-01 11:32 ` Jiang Xin
2022-07-29 10:12 ` [PATCH 7/9] refs: get error message via refs_update_ref_extended() Jiang Xin
2022-07-29 10:12 ` [PATCH 8/9] refs: reimplement files_copy_or_rename_ref() to run hook Jiang Xin
2022-07-29 10:12 ` [PATCH 9/9] refs: reimplement refs_delete_refs() and run hook once Jiang Xin
2022-08-02 12:42 ` Michael Heemskerk
2022-08-09 11:05 ` Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJDSCnOFSnhOU06UHF6T9RT31Gq56BhJn5fQ4_ys2ERrN1stSw@mail.gmail.com \
--to=mheemskerk@atlassian.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=ps@pks.im \
--cc=worldhello.net@gmail.com \
--cc=zhiyou.jx@alibaba-inc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).