On Thu, May 15, 2025 at 01:11:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 66bd91fd523d..fba0dded64a7 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1022,7 +1022,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> for (i = 0; i < the_repository->index->cache_nr; i++)
> if (ce_intent_to_add(the_repository->index->cache[i]))
> ita_nr++;
> - committable = the_repository->index->cache_nr - ita_nr > 0;
> + committable = the_repository->index->cache_nr > ita_nr;
I guess it is not possible for ita_nr to be greater than cache_nr, since
we are counting up entries in the loop above. If ita_nr were greater,
the original would wrap around and set committable to true, but yours
would not.
So really, I think the original was equivalent to:
committable = cache_nr != ita_nr;
but I think ">" probably expresses the intent better (we want to know if
there are any non-ita entries). Though in that case I'd think:
committable = 0;
for (i = 0; i < cache_nr; i++) {
if (!ce_intent_to_add(...) {
committable = 1;
break;
}
}
would be the most clear, since we do not otherwise care about the actual
number of ita entries. And lets us break out of the loop early.
I dunno if it is worth refactoring further, though. Your patch does the
correct thing and fixes the codeql complaint (which I do think is a
false positive, because ita_nr must be less than cache_nr).
-Peff
