Re: [PATCH v2 0/6] refs: excessive hook execution with packed refs

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jan 17, 2022, 11:37 UTC
Message-ID: <CAFQ2z_M+2D-5qPXN=0FEveZ36Yq0rps3HxRkgCmiupq1DD7Kig@mail.gmail.com>
In-Reply-To: <YeUYNhJemN/hRG+6@ncase>

On Mon, Jan 17, 2022 at 8:18 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 18 quoted lines

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

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado