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
