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.
>
> - it would be nice to record the generated filenames as soon as
> possible. We don't currently use them for cleaning up from a failed
> operation, but a future patch will do so.
That said, I think that it's worth having a single spot within
cmd_repack() that is responsible for populating the generated pack
extensions, since it protects against a caller forgetting to do so (and
then tricking repack into thinking that we didn't generate *any* of the
corresponding extensions).
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 ;-).
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
The patch itself looks good.
Thanks,
Taylor
