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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 21, 2022, 23:20 UTC
Message-ID: <Y1MpUHBQtq8uP5Uy@nand.local>
In-Reply-To: <Y1MSkubbDBh0Ma1G@coredump.intra.peff.net>

On Fri, Oct 21, 2022 at 05:43:46PM -0400, Jeff King wrote:

Show 8 quoted lines

> 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