Re: [PATCH v3 2/9] cache-tree: simplify verify_cache() prototype

Elijah Newren [off-list ref] writes:

quoted

-       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);

On 1/23/2021 4:10 PM, Junio C Hamano wrote:

Elijah Newren [off-list ref] writes:

quoted

quoted

-       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