Subject: Re: [PATCH v3 3/8] update-index: add --force-full-index option for expand/collapse test

From: Junio C Hamano <[email-masked]>
Date: Thu, 07 Oct 2021 22:24:14 -0700

"Victoria Dye via GitGitGadget" <[email-masked]> writes:

> +	/*
> +	 * If --force-full-index is set, the command should skip manually
> +	 * setting `command_requires_full_index`.
> +	 */

Hmph, doesn't that feel unnaturally backwards, though?

The settings.command_requires_full_index bit forces read-cache to
call ensure_full_index() immediately after the in-core index is read
from the disk.  If we are forcing operating on the full index, I'd
imagine that we'd be making sure that ensure_full_index() to be
called.

I do not see anything in the code that ensures active_cache_changed
to be flipped on.  So the new test that says

    git -C sparse-index -c core.fsmonitor="" update-index --force-full-index

may not call ensure_full_index(), but because nothing marks
the_index as changed, I think we won't call write_locked_index() at
the end of cmd_update_index().  IOW, what we have in the test patch
may be an expensive noop, no?

Or perhaps I am reading the patch completely incorrectly.  I dunno.

> +	prepare_repo_settings(r);
> +	if (!use_default_full_index)
> +		r->settings.command_requires_full_index = 1;
> +
>  	entries = read_cache();
>  	if (entries < 0)
>  		die("cache corrupted");
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 889079f55b8..4aa4fef7b4f 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -635,7 +635,7 @@ test_expect_success 'sparse-index is expanded and converted back' '
>  	init_repos &&
>  
>  	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
> -		git -C sparse-index -c core.fsmonitor="" reset --hard &&
> +		git -C sparse-index -c core.fsmonitor="" update-index --force-full-index &&
>  	test_region index convert_to_sparse trace2.txt &&
>  	test_region index ensure_full_index trace2.txt
>  '