Re: [PATCH 2/4] repack: populate extension bits incrementally

[Jeff King <peff@peff.net>]

On Fri, Oct 21, 2022 at 07:34:13PM -0400, Jeff King wrote:

> On Fri, Oct 21, 2022 at 07:20:48PM -0400, Taylor Blau wrote:
> 
> > On Fri, Oct 21, 2022 at 05:43:46PM -0400, Jeff King wrote:
> > > There are two small problems with this:
> > >
> > >   - repack_promisor_objects() may have added entries to "names", and
> > >     already called populate_pack_exts() for them. This is mostly just
> > >     wasteful, as we'll stat() the filename with each possible extension,
> > >     get the same result, and just overwrite our bits. But it makes the
> > >     code flow confusing, and it will become a problem if we try to make
> > >     populate_pack_exts() do more things.
> > 
> > Hmm. I agree with you that repack_promisor_objects() calling
> > populate_pack_exts() itself is at best weird, and at worst wasteful.
> 
> I don't think it's weird, really. It is setting up the entries in the
> string-list completely when we add them, rather than annotating later.
> If there were some performance gain from doing them all at once, I could
> see it, but otherwise I like that it means the entries are always in a
> consistent state.

I think my original didn't explain my thinking very well. And its "two
small problems" is really a bit of a lie. It is really one small
problem, and a tweak I want in order to make a future patch work. :)

So here's what I wrote instead:

-- >8 --
There's one small problem with this. In repack_promisor_objects(), we
may add entries to "names" and call populate_pack_exts() for them.
Calling it again is mostly just wasteful, as we'll stat() the filename
with each possible extension, get the same result, and just overwrite
our bits.

So we could drop the call there, and leave the final loop to populate
all of the bits. But instead, this patch does the reverse: drops the
final loop, and teaches the other two sites to populate the bits as they
add entries.

This makes the code easier to reason about, as you never have to worry
about when the util field is valid; it is always valid for each entry.

It also serves my ulterior purpose: recording the generated filenames as
soon as possible will make it easier for a future patch to use them for
cleaning up from a failed operation.
-- >8 --

-Peff