| Message ID | cover.1641556319.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | refs: excessive hook execution with packed refs | expand |
Patrick Steinhardt <ps@pks.im> writes: > Hi, > > this is a resend of version 1 of this patch series to hopefully entice > some reviews. The only change is that v2 is rebased onto the current > main branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The > following was from the orignial cover letter: I'll add Ævar, who has been making a lot of changes to the refs subsystem, and Han-Wen, whose work to add a new ref backend may need to interact with this change, as possible stake-holders to the CC list. > As reported by Waleed in [1], the reference-transaction hook is being > executed when packing refs. Given that the hook ideally ought to track > logical updates to refs instead of leaking low-level implementation > details of how the files backend works, this is understandably leading > to some confusion. > > This patch series aims to fix that by improving how the tandom of loose > and packed refs backends interact such that we skip executing the hook > when the loose backend: > > - repacks references. > - needs to delete packed refs when deleting a loose ref would > uncover that packed ref. > > Patrick > > [1]: <CAKjfCeBcuYC3OXRVtxxDGWRGOxC38Fb7CNuSh_dMmxpGVip_9Q@mail.gmail.com> > > Patrick Steinhardt (6): > refs: open-code deletion of packed refs > refs: allow passing flags when beginning transactions > refs: allow skipping the reference-transaction hook > refs: demonstrate excessive execution of the reference-transaction > hook > refs: do not execute reference-transaction hook on packing refs > refs: skip hooks when deleting uncovered packed refs > > refs.c | 11 +++++-- > refs.h | 8 ++++- > refs/files-backend.c | 25 +++++++++++----- > refs/packed-backend.c | 30 ++++++++++++++----- > refs/packed-backend.h | 6 ++++ > refs/refs-internal.h | 1 + > sequencer.c | 2 +- > t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++ > 8 files changed, 113 insertions(+), 20 deletions(-)
On Fri, Jan 7, 2022 at 11:17 PM Junio C Hamano <gitster@pobox.com> wrote: > > this is a resend of version 1 of this patch series to hopefully entice > > some reviews. The only change is that v2 is rebased onto the current > > main branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The > > following was from the orignial cover letter: > > I'll add Ævar, who has been making a lot of changes to the refs > subsystem, and Han-Wen, whose work to add a new ref backend may need > to interact with this change, as possible stake-holders to the CC list. Thanks for the consideration, Jun. As the hook is called from refs.c, it should not make a difference for reftable. I looked over the patches. I didn't look at the bottom change to packed/loose refs as I'm not an expert. The individual transaction updates already support their own flags, so this change generates some confusion. Consider: int refs_delete_ref(struct ref_store *refs, const char *msg, const char *refname, const struct object_id *old_oid, unsigned int flags) how would one delete a ref skipping the transaction hook? It will be easy for someone to pass the SKIP_TRANSACTION_HOOK to refs_delete_ref(), with surprising results. It might make sense to not introduce a new flag namespace, but use update->flags instead. You'd have to add your new flag after REF_SKIP_REFNAME_VERIFICATION. Bonus is that you can unittest the new flag using the existing ref-store helper without extra work. (check that a transaction with & without the flag works as expected.) other than that, looks OK to me.
On Thu, Jan 13, 2022 at 07:24:19PM +0100, Han-Wen Nienhuys wrote: > On Fri, Jan 7, 2022 at 11:17 PM Junio C Hamano <gitster@pobox.com> wrote: > > > this is a resend of version 1 of this patch series to hopefully entice > > > some reviews. The only change is that v2 is rebased onto the current > > > main branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The > > > following was from the orignial cover letter: > > > > I'll add Ævar, who has been making a lot of changes to the refs > > subsystem, and Han-Wen, whose work to add a new ref backend may need > > to interact with this change, as possible stake-holders to the CC list. > > Thanks for the consideration, Jun. As the hook is called from refs.c, > it should not make a difference for reftable. > > I looked over the patches. I didn't look at the bottom change to > packed/loose refs as I'm not an expert. > > The individual transaction updates already support their own flags, so > this change generates some confusion. Consider: > > int refs_delete_ref(struct ref_store *refs, const char *msg, > const char *refname, > const struct object_id *old_oid, > unsigned int flags) > > how would one delete a ref skipping the transaction hook? It will be > easy for someone to pass the SKIP_TRANSACTION_HOOK to > refs_delete_ref(), with surprising results. > > It might make sense to not introduce a new flag namespace, but use > update->flags instead. You'd have to add your new flag after > REF_SKIP_REFNAME_VERIFICATION. > Bonus is that you can unittest the new flag using the existing > ref-store helper without extra work. (check that a transaction with & > without the flag works as expected.) > > other than that, looks OK to me. Thanks for your feedback! In fact the first version I had locally did exactly that. But I found the end version result harder to reason about, most importantly because it's not a 100% clear to the reader whether all callsites which delete refs in the packed-backend via the files-backend are adapted or whether any of the callsites was missing. By having the flag in a central place it's immediately clear that the hook won't be run at all, which is exactly what we want here. Patrick
On Mon, Jan 17, 2022 at 8:18 AM Patrick Steinhardt <ps@pks.im> wrote: > > It might make sense to not introduce a new flag namespace, but use > > update->flags instead. You'd have to add your new flag after > > REF_SKIP_REFNAME_VERIFICATION. > > Bonus is that you can unittest the new flag using the existing > > ref-store helper without extra work. (check that a transaction with & > > without the flag works as expected.) > > > > other than that, looks OK to me. > > Thanks for your feedback! > > In fact the first version I had locally did exactly that. But I found > the end version result harder to reason about, most importantly because > it's not a 100% clear to the reader whether all callsites which delete > refs in the packed-backend via the files-backend are adapted or whether > any of the callsites was missing. By having the flag in a central place > it's immediately clear that the hook won't be run at all, which is > exactly what we want here. If you do it like this, can you find the callsites that take update flags (but not transaction flags), and update the signature to say "update_flags" rather than "flags", and document appropriately?
On Mon, Jan 17, 2022 at 12:37:39PM +0100, Han-Wen Nienhuys wrote: > On Mon, Jan 17, 2022 at 8:18 AM Patrick Steinhardt <ps@pks.im> wrote: > > > It might make sense to not introduce a new flag namespace, but use > > > update->flags instead. You'd have to add your new flag after > > > REF_SKIP_REFNAME_VERIFICATION. > > > Bonus is that you can unittest the new flag using the existing > > > ref-store helper without extra work. (check that a transaction with & > > > without the flag works as expected.) > > > > > > other than that, looks OK to me. > > > > Thanks for your feedback! > > > > In fact the first version I had locally did exactly that. But I found > > the end version result harder to reason about, most importantly because > > it's not a 100% clear to the reader whether all callsites which delete > > refs in the packed-backend via the files-backend are adapted or whether > > any of the callsites was missing. By having the flag in a central place > > it's immediately clear that the hook won't be run at all, which is > > exactly what we want here. > > If you do it like this, can you find the callsites that take update > flags (but not transaction flags), and update the signature to say > "update_flags" rather than "flags", and document appropriately? All functions which accept "flags" document that they pass the parameter directly to `ref_transaction_{update,create,delete,verify}` already. And in the context of those latter functions it cannot possibly be related to the flags passed to `refs_store_transaction_begin()` because they already get a transaction as parameter and thus don't have to create a new one. So we could apply below patch, and I'm happy to polish and add it to the series. But it feels to me like it adds churn while only stating things that are documented already. It could only be me though given that I'm obviously biased, so please feel free to disagree. Patrick -- >8 -- From 91c77a69b6326b1b6cc743ebba6fb0970f0d01c2 Mon Sep 17 00:00:00 2001 Message-Id: <91c77a69b6326b1b6cc743ebba6fb0970f0d01c2.1643008212.git.ps@pks.im> From: Patrick Steinhardt <ps@pks.im> Date: Mon, 24 Jan 2022 08:09:45 +0100 Subject: [PATCH] refs: rename generic `flags` field to `update_flags` for clarity --- refs.c | 50 +++++++++++++++++++++++++------------------------- refs.h | 20 ++++++++++---------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/refs.c b/refs.c index 526bf5ed97..c15e8bd58d 100644 --- a/refs.c +++ b/refs.c @@ -795,7 +795,7 @@ long get_files_ref_lock_timeout_ms(void) int refs_delete_ref(struct ref_store *refs, const char *msg, const char *refname, const struct object_id *old_oid, - unsigned int flags) + unsigned int update_flags) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; @@ -803,7 +803,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, transaction = ref_store_transaction_begin(refs, 0, &err); if (!transaction || ref_transaction_delete(transaction, refname, old_oid, - flags, msg, &err) || + update_flags, msg, &err) || ref_transaction_commit(transaction, &err)) { error("%s", err.buf); ref_transaction_free(transaction); @@ -816,10 +816,10 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, } int delete_ref(const char *msg, const char *refname, - const struct object_id *old_oid, unsigned int flags) + const struct object_id *old_oid, unsigned int update_flags) { return refs_delete_ref(get_main_ref_store(the_repository), msg, refname, - old_oid, flags); + old_oid, update_flags); } static void copy_reflog_msg(struct strbuf *sb, const char *msg) @@ -1080,12 +1080,12 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, const struct object_id *old_oid, - unsigned int flags, const char *msg, + unsigned int update_flags, const char *msg, struct strbuf *err) { assert(err); - if (!(flags & REF_SKIP_REFNAME_VERIFICATION) && + if (!(update_flags & REF_SKIP_REFNAME_VERIFICATION) && ((new_oid && !is_null_oid(new_oid)) ? check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) : !refname_is_safe(refname))) { @@ -1094,19 +1094,19 @@ int ref_transaction_update(struct ref_transaction *transaction, return -1; } - if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS) - BUG("illegal flags 0x%x passed to ref_transaction_update()", flags); + if (update_flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS) + BUG("illegal flags 0x%x passed to ref_transaction_update()", update_flags); /* * Clear flags outside the allowed set; this should be a noop because * of the BUG() check above, but it works around a -Wnonnull warning * with some versions of "gcc -O3". */ - flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS; + update_flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS; - flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0); + update_flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0); - ref_transaction_add_update(transaction, refname, flags, + ref_transaction_add_update(transaction, refname, update_flags, new_oid, old_oid, msg); return 0; } @@ -1114,44 +1114,44 @@ int ref_transaction_update(struct ref_transaction *transaction, int ref_transaction_create(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, - unsigned int flags, const char *msg, + unsigned int update_flags, const char *msg, struct strbuf *err) { if (!new_oid || is_null_oid(new_oid)) BUG("create called without valid new_oid"); return ref_transaction_update(transaction, refname, new_oid, - null_oid(), flags, msg, err); + null_oid(), update_flags, msg, err); } int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const struct object_id *old_oid, - unsigned int flags, const char *msg, + unsigned int update_flags, const char *msg, struct strbuf *err) { if (old_oid && is_null_oid(old_oid)) BUG("delete called with old_oid set to zeros"); return ref_transaction_update(transaction, refname, null_oid(), old_oid, - flags, msg, err); + update_flags, msg, err); } int ref_transaction_verify(struct ref_transaction *transaction, const char *refname, const struct object_id *old_oid, - unsigned int flags, + unsigned int update_flags, struct strbuf *err) { if (!old_oid) BUG("verify called with old_oid set to NULL"); return ref_transaction_update(transaction, refname, NULL, old_oid, - flags, NULL, err); + update_flags, NULL, err); } int refs_update_ref(struct ref_store *refs, const char *msg, const char *refname, const struct object_id *new_oid, - const struct object_id *old_oid, unsigned int flags, + const struct object_id *old_oid, unsigned int update_flags, enum action_on_err onerr) { struct ref_transaction *t = NULL; @@ -1160,7 +1160,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg, t = ref_store_transaction_begin(refs, 0, &err); if (!t || - ref_transaction_update(t, refname, new_oid, old_oid, flags, msg, + ref_transaction_update(t, refname, new_oid, old_oid, update_flags, msg, &err) || ref_transaction_commit(t, &err)) { ret = 1; @@ -1191,10 +1191,10 @@ int refs_update_ref(struct ref_store *refs, const char *msg, int update_ref(const char *msg, const char *refname, const struct object_id *new_oid, const struct object_id *old_oid, - unsigned int flags, enum action_on_err onerr) + unsigned int update_flags, enum action_on_err onerr) { return refs_update_ref(get_main_ref_store(the_repository), msg, refname, new_oid, - old_oid, flags, onerr); + old_oid, update_flags, onerr); } char *refs_shorten_unambiguous_ref(struct ref_store *refs, @@ -2435,21 +2435,21 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction, } int refs_delete_refs(struct ref_store *refs, const char *logmsg, - struct string_list *refnames, unsigned int flags) + struct string_list *refnames, unsigned int update_flags) { char *msg; int retval; msg = normalize_reflog_message(logmsg); - retval = refs->be->delete_refs(refs, msg, refnames, flags); + retval = refs->be->delete_refs(refs, msg, refnames, update_flags); free(msg); return retval; } int delete_refs(const char *msg, struct string_list *refnames, - unsigned int flags) + unsigned int update_flags) { - return refs_delete_refs(get_main_ref_store(the_repository), msg, refnames, flags); + return refs_delete_refs(get_main_ref_store(the_repository), msg, refnames, update_flags); } int refs_rename_ref(struct ref_store *refs, const char *oldref, diff --git a/refs.h b/refs.h index d4056f9fe2..5131db7e48 100644 --- a/refs.h +++ b/refs.h @@ -442,9 +442,9 @@ int reflog_exists(const char *refname); int refs_delete_ref(struct ref_store *refs, const char *msg, const char *refname, const struct object_id *old_oid, - unsigned int flags); + unsigned int update_flags); int delete_ref(const char *msg, const char *refname, - const struct object_id *old_oid, unsigned int flags); + const struct object_id *old_oid, unsigned int update_flags); /* * Delete the specified references. If there are any problems, emit @@ -453,9 +453,9 @@ int delete_ref(const char *msg, const char *refname, * ref_transaction_delete(). */ int refs_delete_refs(struct ref_store *refs, const char *msg, - struct string_list *refnames, unsigned int flags); + struct string_list *refnames, unsigned int update_flags); int delete_refs(const char *msg, struct string_list *refnames, - unsigned int flags); + unsigned int update_flags); /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); @@ -680,7 +680,7 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, const struct object_id *old_oid, - unsigned int flags, const char *msg, + unsigned int update_flags, const char *msg, struct strbuf *err); /* @@ -695,7 +695,7 @@ int ref_transaction_update(struct ref_transaction *transaction, int ref_transaction_create(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, - unsigned int flags, const char *msg, + unsigned int update_flags, const char *msg, struct strbuf *err); /* @@ -709,7 +709,7 @@ int ref_transaction_create(struct ref_transaction *transaction, int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const struct object_id *old_oid, - unsigned int flags, const char *msg, + unsigned int update_flags, const char *msg, struct strbuf *err); /* @@ -723,7 +723,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, int ref_transaction_verify(struct ref_transaction *transaction, const char *refname, const struct object_id *old_oid, - unsigned int flags, + unsigned int update_flags, struct strbuf *err); /* Naming conflict (for example, the ref names A and A/B conflict). */ @@ -796,10 +796,10 @@ void ref_transaction_free(struct ref_transaction *transaction); */ int refs_update_ref(struct ref_store *refs, const char *msg, const char *refname, const struct object_id *new_oid, const struct object_id *old_oid, - unsigned int flags, enum action_on_err onerr); + unsigned int update_flags, enum action_on_err onerr); int update_ref(const char *msg, const char *refname, const struct object_id *new_oid, const struct object_id *old_oid, - unsigned int flags, enum action_on_err onerr); + unsigned int update_flags, enum action_on_err onerr); int parse_hide_refs_config(const char *var, const char *value, const char *);
Hi, this is a resend of version 1 of this patch series to hopefully entice some reviews. The only change is that v2 is rebased onto the current main branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The following was from the orignial cover letter: As reported by Waleed in [1], the reference-transaction hook is being executed when packing refs. Given that the hook ideally ought to track logical updates to refs instead of leaking low-level implementation details of how the files backend works, this is understandably leading to some confusion. This patch series aims to fix that by improving how the tandom of loose and packed refs backends interact such that we skip executing the hook when the loose backend: - repacks references. - needs to delete packed refs when deleting a loose ref would uncover that packed ref. Patrick [1]: <CAKjfCeBcuYC3OXRVtxxDGWRGOxC38Fb7CNuSh_dMmxpGVip_9Q@mail.gmail.com> Patrick Steinhardt (6): refs: open-code deletion of packed refs refs: allow passing flags when beginning transactions refs: allow skipping the reference-transaction hook refs: demonstrate excessive execution of the reference-transaction hook refs: do not execute reference-transaction hook on packing refs refs: skip hooks when deleting uncovered packed refs refs.c | 11 +++++-- refs.h | 8 ++++- refs/files-backend.c | 25 +++++++++++----- refs/packed-backend.c | 30 ++++++++++++++----- refs/packed-backend.h | 6 ++++ refs/refs-internal.h | 1 + sequencer.c | 2 +- t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++ 8 files changed, 113 insertions(+), 20 deletions(-)