Re: [PATCH 21/49] builtin/repack.c: factor our "generated_pack_install"

[Taylor Blau <me@ttaylorr.com>]

On Fri, Oct 10, 2025 at 02:14:49AM -0400, Jeff King wrote:
> On Tue, Oct 07, 2025 at 04:26:26PM -0400, Taylor Blau wrote:
>
> > > > +	for_each_string_list_item(item, &names)
> > > > +		generated_pack_install((struct generated_pack *)item->util,
> > >
> > > This cast should be unnecessary, right? `item->util` is a void pointer,
> > > so C should do the cast implicitly.
> >
> > It's unnecessary, but I dislike implicit casts from 'void*' to any other
> > type. This makes it clearer how we're supposed to interpret the value in
> > item->util, but I'm happy to change it to use the implicit cast if you
> > feel strongly about it.
>
> I tend to avoid casts when possible, because they can mask unexpected
> conversions. E.g., if item->util's type changed, your cast means we
> would never notice it.
>
> But that cuts both ways. If the prototype for generated_pack_install()
> changes, it is only your cast that would tell the compiler that this
> caller needs to be updated. Ultimately it is the spot that _sets_
> item->util that needs to change, but hopefully flagging this spot would
> point us in the right direction.
>
> And that does seem more likely than item->util changing away from void.
> (To be clear, I don't think either is that likely, I'm just musing on
> whether casts like this are helpful in general).

OK, that's a very compelling argument. I'll drop the cast on my local
version.

(As a side note, it doesn't look like we mention this convention in our
CodingGuidelines, and we probably should, including the rationale that
you gave above here. That seems like another good candidate for
#leftoverbits.)

Thanks,
Taylor