Re: [PATCH 3/9] commit-graph: fix type for some write options

[Taylor Blau <me@ttaylorr.com>]

On Mon, Aug 04, 2025 at 10:17:19AM +0200, Patrick Steinhardt wrote:
> 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.

I have the same general feeling about this patch as the previous one:
the -Wsign-compare warnings here are a bit of a red herring.

That said, I do think that we need stricter validation for these
options. Following the --size-multiple example, for instance, we get
this rather unfriendly error message if we pass a bogus value:

    $ git.compile commit-graph write --split --size-multiple=-1 --reachable
    fatal: size_t overflow: 18446744073709551615 * 34

This happens as a result of in
commit-graph.c::split_graph_merge_strategy() doing the following:

    while (g && (g->num_commits <= st_mult(size_mult, num_commits) ||
                 (max_commits && num_commits > max_commits))) {

, where size_mult is changed from a size_t to an int implicitly when
assigning it to the stack-local "size_mult" variable.

Indeed, this patch does improve that warning, but I am not convinced
that changing the type is the right way to go about improving the error
message in and of itself.

Thanks,
Taylor