On Fri, Oct 21, 2022 at 05:42:48PM -0400, Jeff King wrote:
> 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.
> 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"?
> +
> +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.
> @@ -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..
> @@ -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 ;-).
> @@ -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
