Re: [PATCH 1/4] repack: convert "names" util bitfield to array

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 21, 2022, 23:10 UTC
Message-ID: <Y1Mm9zTX3XwN3IWu@nand.local>
In-Reply-To: <Y1MSWAx+baTklfLL@coredump.intra.peff.net>

On Fri, Oct 21, 2022 at 05:42:48PM -0400, Jeff King wrote:

Show 9 quoted lines

> We keep a string_list "names" containing the hashes of packs generated
> on our behalf by pack-objects. The util field of each item is treated as
> a bitfield that tells us which extensions (.pack, .idx, .rev, etc) are
> present for each name.
>
> Let's switch this to allocating a real array. That will give us room in
> a future patch to store more data than just a single bit per extension.
> And it makes the code a little easier to read, as we avoid casting back
> and forth between uintptr_t and a void pointer.

Thanks, this is a nice summary that matches my own recollection from working in this area.

Show 12 quoted lines

> diff --git a/builtin/repack.c b/builtin/repack.c
> index a5bacc7797..8e71230bf7 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -247,11 +247,15 @@ static struct {
>  	{".idx"},
>  };
>
> -static unsigned populate_pack_exts(char *name)
> +struct generated_pack_data {
> +	char exts[ARRAY_SIZE(exts)];
> +};

Makes sense, this replaces the individual bits of the `void *` we were treating as an array of "which extensions associated with this pack were generated by pack-objects"?

Show 7 quoted lines

> +
> +static struct generated_pack_data *populate_pack_exts(const char *name)
>  {
>  	struct stat statbuf;
>  	struct strbuf path = STRBUF_INIT;
> -	unsigned ret = 0;
> +	struct generated_pack_data *data = xcalloc(1, sizeof(*data));

I'm nitpicking, but we could replace this with;

  struct generated_pack_data *data;

  CALLOC_ARRAY(data, 1);

so that we don't have to rely on calling sizeof(*data). But sizeof(*data) will always give us the right answer anyway, even if the name of data's type changed, so what you have is fine, too.

Show 6 quoted lines

> @@ -261,11 +265,11 @@ static unsigned populate_pack_exts(char *name)
>  		if (stat(path.buf, &statbuf))
>  			continue;
>
> -		ret |= (1 << i);
> +		data->exts[i] = 1;

Great. I'm happy to see this bit-twiddling go away, but..

Show 6 quoted lines

> @@ -320,7 +324,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
>  					  line.buf);
>  		write_promisor_file(promisor_name, NULL, 0);
>
> -		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
> +		item->util = populate_pack_exts(item->string);

I'm even happier to see this go away ;-).

Show 6 quoted lines

> @@ -1115,7 +1121,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		write_midx_file(get_object_directory(), NULL, NULL, flags);
>  	}
>
> -	string_list_clear(&names, 0);
> +	string_list_clear(&names, 1);

Good. Now that we're actually allocating memory in the `->util` field, we care about freeing it, too. Thanks for remembering this.

Thanks, Taylor