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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jan 10, 2023, 14:58 UTC
Message-ID: <14271f92-0ee4-906c-2e12-954ce74fd3ef@github.com>
In-Reply-To: <patch-3.5-d96388acef6-20230110T060340Z-avarab@gmail.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