Subject: Re: [PATCH v3 03/10] commit-graph: fix type for some write options

From: Junio C Hamano <[email-masked]>
Date: Thu, 07 Aug 2025 15:40:50 -0700

Patrick Steinhardt <[email-masked]> writes:

> The options "max-commits" and "size-multiple" are both supposed to be
> positive integers and are documented as such, but we use a signed
> integer field to store them. This causes sign comparison warnings in
> `split_graph_merge_strategy()` because we end up comparing the option
> values with the observed number of commits.
>
> Fix the issue by converting the fields to be unsigned and convert the
> options to use `OPT_UNSIGNED()` accordingly. This macro has only been
> introduced recently, which might explain why the option values were
> signed in the first place.

These are platform natural "int" from their inception at c2bc6e6a
(commit-graph: create options for split files, 2019-06-18), which
way predates the recent push to appease -Wsign-compare, so yes, it
does explain it.  But because the developer who wrote it in the
first place is around and with us, why not ask them instead of
speculating?

As the max_commits member is comparable to 4-byte network byte order
integer that is .num_commits in the file, using platform natural
"int" or int32_t is not correct, because you may not be able to tell
the command to hold 3 billion objects before splitting, even though
the underlying file format does support such settings.  It has to be
uint32_t or wider (but if it is wider, you'd need to be prepared to
correctly compare max_commits with num_commits, and take an overly
large max as "unlimited", or something).  And unsigned usually is at
least that wide, so the change may be justified.  I do not see a
reason why we want to avoid using uint32_t, though.

As to size_multiple, it appears to me that the number is really
designed to be a small integer (for which even 100 is probably way
too many), so I do not see any reason to insist it to be unsigned.
Even "short" _ought_ to do fine.  And if our macros and compiler
settings do not support it well and DEVELOPER=YesPlease build
complain, that is what we need to fix.  Papering over the problem by
using unnecessarily wide type, or by using signedness that happens
to squelch the misguided compiler warnings, is skirting around it.

If there is any bug, it is st_mult(size_mult, num_commit) and the
compiler warning that complains about its CPP expansion, even though
it should be able to ensure that "int size_mult" (or "short
size_mult") that gets promoted to unsigned to match uint32_t
num_commit would not overflow the unsigned multiplication.

So my take on this change is that "fix type for ..." on the subject
line greatly misrepresents what this change does.  It smells to me
that "squelch -Wsign-compare warnings" would be closer to what is
happening.

It really is arithmetic overflowing and wrapping around that we want
to be careful about.  -Wsign-compare alone is not doing a good job
to do so, is it?