Re: [PATCH 01/11] commit: simplify code
- From
Jeff King <peff@peff.net>
- Date
- May 15, 2025, 19:48 UTC
- Message-ID
- <20250515194806.GC3320240@coredump.intra.peff.net>
- In-Reply-To
- <767b1e74fc70d78c7e3c84a2dc5eb7b607252f4f.1747314709.git.gitgitgadget@gmail.com>
On Thu, May 15, 2025 at 01:11:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 10 quoted lines
> 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