From: Justin Tobler <jltobler@gmail.com>
Date: Fri, 03 Jan 2025 02:08:15 GMT
Subject: Re: [PATCH 9/9] commit-reach: use `size_t` to track indices when computing merge bases
Message-ID: <s7b4o6elbbxrzp55mbgtj5tmfdtmdi4kczxad5ogy7quotrfhj@yq7ppxdjp4kd>
In-Reply-To: <20241227-b4-pks-commit-reach-sign-compare-v1-9-07c59c2aa632@pks.im>

On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> The functions `repo_get_merge_bases_many()` and friends accepts an array
> of commits as well as a parameter that indicates how large that array
> is. This parameter is using a signed integer, which leads to a couple of
> warnings with -Wsign-compare.
> 
> Refactor the code to use `size_t` to track indices instead and adapt
> callers accordingly. While most callers are trivial, there are two
> callers that require a bit more scrutiny:
> 
>   - builtin/merge-base.c:show_merge_base() subtracts `1` from the
>     `rev_nr` before calling `repo_get_merge_bases_many_dirty()`, so if
>     the variable was `0` it would wrap. This code is fine though because
>     its only caller will execute that code only when `argc >= 2`, and it
>     follows that `rev_nr >= 2`, as well.
> 
>   - bisect.ccheck_merge_bases() similarly subtracts `1` from `rev_nr`.

s/ccheck/check/

Small typo, but probably not worth rerolling.

>     Again, there is only a single caller that populates `rev_nr` with
>     `good_revs.nr`. And because a bisection always requires at least one
>     good revision it follws that `rev_nr >= 1`.
> 
> Mark the file as -Wsign-compare-clean.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Thanks Patrick! I reviewed the series and overall it looks good to me :)

-Justin