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

Patrick Steinhardt [off-list ref] writes:

On Mon, Jul 13, 2026 at 09:28:02AM -0700, Junio C Hamano wrote:

quoted

Patrick Steinhardt [off-list ref] writes:

quoted

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.