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

From: Jeff King <peff@peff.net>
Date: Oct 21, 2022, 23:34 UTC
Message-ID: <Y1MsdAgL8fdIRtxH@coredump.intra.peff.net>
In-Reply-To: <Y1MpUHBQtq8uP5Uy@nand.local>

On Fri, Oct 21, 2022 at 07:20:48PM -0400, Taylor Blau wrote:

Show 12 quoted lines

> 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. :)

Show 26 quoted lines

> 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