On 1/23/2021 4:10 PM, Junio C Hamano wrote:
> Elijah Newren <newren@xxxxxxxxx> writes:
>
>>> - for (i = 0; i < entries - 1; i++) {
>>> + for (i = 0; i + 1 < istate->cache_nr; i++) {
>>> /* path/file always comes after path because of the way
>>> * the cache is sorted. Also path can appear only once,
>>> * which means conflicting one would immediately follow.
>>> */
>>> - const struct cache_entry *this_ce = cache[i];
>>> - const struct cache_entry *next_ce = cache[i + 1];
>>> + const struct cache_entry *this_ce = istate->cache[i];
>>> + const struct cache_entry *next_ce = istate->cache[i + 1];
>>
>> Makes sense. Thanks for explaining the i + 1 < istate->cache_nr bit
>> in the commit message; made it easier to read through quickly. I'm
>> curious if it deserves a comment in the code too, since it does feel
>> slightly unusual.
>
> I think this is entirely my fault.
>
> It probably reads more natural to start from 1 and interate through
> to the end of the array, comparing the current one with the previous
> entry, i.e.
>
> for (i = 1; i < istate->cache_nr; i++) {
> prev = cache[i - 1];
> this = cache[i];
> compare(prev, this);
This would be another natural way to make the loop extremely clear.
It's a bigger diff, changing the names of 'this' and 'next', but
perhaps worthwhile.
Thanks,
-Stolee
