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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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.
>
> Since the only thing we're storing is an array, we could just allocate
> it directly. But instead I've put it into a named struct here. That
> further increases readability around the casts, and in particular helps
> differentiate us from other string_lists in the same file which use
> their util field differently. E.g., the existing_*_packs lists still do
> bit-twiddling, but their bits have different meaning than the ones in
> "names". This makes it hard to grep around the code to see how the util
> fields are used; now you can look for "generated_pack_data".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/repack.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> 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)];
> +};

OK, so instead of a single "unsigned" word holding six bits (the
number of elements in the exts[] array), we have one byte per the
file extension.  Since ...

> +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));

... this is allocated a real piece of storage and the function
returns a pointer to it, ...

> -		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
> +		item->util = populate_pack_exts(item->string);

... we no longer need to cast but the value can go straight to the
.util member, and ...

> -	string_list_clear(&names, 0);
> +	string_list_clear(&names, 1);

... we now need to free these structs pointed at by the .util
member.

All make sense.