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.
> But I'm sure future patch you're referring to cares about knowing
> these as soon as possible, since that's the point of this series ;-).
Yes. :)
> I think a reasonable middle ground here is to do something like the
> following on top of this patch:
>
> --- >8 ---
> diff --git a/builtin/repack.c b/builtin/repack.c
> index b5bd9e5fed..16a941f48b 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -1002,6 +1002,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> return ret;
> }
>
> + for_each_string_list_item(item, &names) {
> + if (!item->util)
> + BUG("missing generated_pack_data for pack %s",
> + item->string);
> + }
> +
> string_list_sort(&names);
>
> close_object_store(the_repository->objects);
> --- 8< ---
>
> which still lets you eagerly keep track of the generated pack extensions
> while also protecting against forgetful callers. Obviously we're relying
> on a runtime check which is going to be somewhat weaker. But I think
I don't think we need that. The renaming loop a few lines below will
happily segfault if anybody forgot to populate it. With a less nice
message, obviously, but if the point is to notice a bug, it will get the
job done.
-Peff
