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
