Re: [PATCH 3/5] sparse-index API: fix TODO, BUG() out on NULL ensure_full_index()

[Derrick Stolee <derrickstolee@github.com>]

On 1/10/2023 1:17 AM, Ævar Arnfjörð Bjarmason wrote: 
> The only reason it needed to be this lax was due to interaction with
> repo_index_has_changes(). See the addition of that code in . This
> fixes one of the TODO comments added in 0c18c059a15, the other one was
> already removed in [3].

> 2. 0c18c059a15 (read-cache: ensure full index, 2021-04-01)
> 3. d76723ee531 (status: use sparse-index throughout, 2021-07-14).

> -	} else {
> -		/* TODO: audit for interaction with sparse-index. */

Please don't drop this comment. It was inserted on purpose before the
"ensure_full_index(istate)" as an indicator that the following loop
has not been checked to see if it could be run on a sparse index.

Removing the comment is like saying "this loop was checked and we
_must_ use a full index here". The case of the TODO being removed in
[3] was because the loop was audited as being safe on a sparse index
(and the ensure_full_index() call was removed). I don't believe that
is what you have done here.

> +	} else if (istate) {
>  		ensure_full_index(istate);
>  		for (i = 0; sb && i < istate->cache_nr; i++) {
>  			if (i)

Further, this block has all sorts of direct uses of 'istate'
that would cause a segfault if 'istate' was NULL. Why do we need
to check for non-NULL now?

Looking earlier in the function, 'istate' is initialized to
'repo->index', so the function already assumed the repository had
an initialized index (or "tree || !get_oid_tree("HEAD", &cmp)"
was satisfied, which doesn't seem connected to a NULL index).

So, I'm thinking that we don't actually need any change to
repo_index_has_changes() unless it's being called in new ways
further down in the series.

Thanks,
-Stolee