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

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 4, 2025, 21:52 UTC
Message-ID: <aJErqjDHudSeyNHr@nand.local>
In-Reply-To: <20250804-b4-pks-commit-graph-wo-the-repository-v1-3-850d626eb2e8@pks.im>

On Mon, Aug 04, 2025 at 10:17:19AM +0200, Patrick Steinhardt wrote:

Show 10 quoted lines

> 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