Re: [PATCH v2 11/12] builtin/gc: fix signedness issues in ODB-related functionality

Patrick Steinhardt <[email protected]> writes:

> On Mon, Jul 13, 2026 at 09:28:02AM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <[email protected]> writes:
>> > diff --git a/builtin/gc.c b/builtin/gc.c
>> > index 3207182488..8cf3781313 100644
>> > --- a/builtin/gc.c
>> > @@ -456,7 +458,7 @@ static struct packed_git *find_base_packs(struct odb_source_files *files,
>> >  		if (e->pack->is_cruft)
>> >  			continue;
>> >  		if (limit) {
>> > -			if (e->pack->pack_size >= limit)
>> > +			if ((uintmax_t) e->pack->pack_size >= limit)
>> 
>> Here, just like in too_many_loose_objects(), 'limit' is of type
>> 'unsigned long'.  While it makes sense to convert both sides of
>> the comparison to an unsigned type, casting only the left side
>> to a type that differs from the right side puzzles me.
>> 
>> Presumably, the other side is of type 'off_t', which is signed,
>> explaining the desire to cast it to an unsigned type.  But I am
>> not sure what happens if 'off_t' is wider than 'unsigned long'.
>
> Yeah, `pack_size` is an `off_t`, which is signed. But we never populate
> it with a negative value, so casting it to `uintmax_t` in unnecessary.
> The right-hand side is already unsigned, so due to the usual arithmetic
> conversion rules it would be automatically promoted to `uintmax_t`, as
> well.

So we are in agreement that this hunk, unlike others, have little to
do with "fix signedness issues"?

I personally feel that the true fix in signedness issues is not to
use -Wsign-compare or teach compilers to be intelligent about when
to issue the warning when given that flag, but that is a separate
topic.

Thanks.