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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 11, 2025, 08:24 UTC
Message-ID: <aJmouVRjPrBlLvrT@pks.im>
In-Reply-To: <xmqqv7mykb65.fsf@gitster.g>

On Thu, Aug 07, 2025 at 03:40:50PM -0700, Junio C Hamano wrote:

Show 30 quoted lines

> Patrick Steinhardt <ps@pks.im> 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.

Using `uint32_t` might cause regressions on some platforms. If for example a signed integer was 64 bits on certain platforms and we restrict it to `uint32_t` then we'd now refuse to take any values between `2^32` and `2^63`, even though those are valid values that we accepted beforehand. So if somebody was using any such value to say "make this essentially unlimited" then we'd now die.

But by using `unsigned` we avoid this pitfall, as we only extend the range of accepted valid values.

Show 8 quoted lines

> 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.

Yeah, it is. At one point I was pondering whether we should extend our
parse-options interface to allow restricting to arbitrary values like
100. I even implemented all of this, but ultimately discarded it because
it wasn't easy to decide where we can retroactively harden accepted
values without causing regressions.

While large values for options may frequently be unreasonable, it wouldn't be the first time that I see users doing unreasonable things intentfully. And sometimes the outcome is even something sensible.

Patrick