Re: [PATCH 6/9] builtin/log: use `size_t` to track indices

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jan 02, 2025 at 07:58:24PM -0600, Justin Tobler wrote:
> On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> > Similar as with the preceding commit, adapt "builtin/log.c" so that it
> > tracks array indices via `size_t` instead of using signed integers. This
> > fixes a couple of -Wsign-compare warnings and prepares the code for
> > a similar refactoring of `repo_get_merge_bases_many()` in a subsequent
> > commit.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  builtin/log.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> [snip]
> >  	if (show_progress)
> >  		progress = start_delayed_progress(_("Generating patches"), total);
> > -	while (0 <= --nr) {
> > +	for (i = 0; i < nr; i++) {
> > +		size_t idx = nr - i - 1;
> >  		int shown;
> > -		display_progress(progress, total - nr);
> > -		commit = list[nr];
> > -		rev.nr = total - nr + (start_number - 1);
> > +
> > +		display_progress(progress, total - idx);
> > +		commit = list[idx];
> > +		rev.nr = total - idx + (start_number - 1);
> 
> Along with updating array indices variables to use `size_t`, the loop
> structure here is also changed. Instead of iterating backwards from
> `nr`, the loop iterator increases and each iteration computes the index
> starting from the end. This is functionally the same behavior and it
> looks like it was done to improve readability.

It wasn't really done to improve readability, but to retain correctness.
Before this change we relied on `nr` being signed, and thus it could
also have a negative value. Now that it's a `size_t` it would overflow
eventually and that would make the loop condition a tautology. So I had
to refactor the condition so that it doesn't rely on such an overflow
anymore.

Patrick