Re: [PATCH 2/9] commit-reach: fix index used to loop through unsigned integer

From: Jeff King <peff@peff.net>
Date: Dec 27, 2024, 14:46 UTC
Message-ID: <20241227144610.GD88306@coredump.intra.peff.net>
In-Reply-To: <20241227-b4-pks-commit-reach-sign-compare-v1-2-07c59c2aa632@pks.im>

On Fri, Dec 27, 2024 at 11:46:22AM +0100, Patrick Steinhardt wrote:

Show 6 quoted lines

> In 62e745ced2 (prio-queue: use size_t rather than int for size,
> 2024-12-20), we refactored `struct prio_queue` to track the number of
> contained entries via a `size_t`. While the refactoring adapted one of
> the users of that variable, it forgot to also adapt "commit-reach.c"
> accordingly. This was missed because that file has -Wsign-conversion
> disabled.

Yes, that's exactly what happened (I used a merge of my series and your -Wsign-compare one and relied on the compiler).

I grepped through for other instances and couldn't find any problems. Many places just do:

  while (queue->nr)
     ...remove top...

which is fine with either type (which might explain why the problem wasn't more common).

I thought there was one more instance in commit-graph.c, which looks at "info->commits->nr", and I was grepping for "commits->nr" since rev_info->commits is a prio_queue. But it is actually a totally different type, compute_generation_info, and "commits" there is a packed_commit_list.

That data type _also_ has the same int/size_t problem. ;) But it is totally separate, so it can happen later in its own patch (and there are a ton of other -Wsign-compare warnings in that file).

Thanks for cleaning up after me.

-Peff