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

[Derrick Stolee <stolee@gmail.com>]

On 10/8/21 1:19 PM, Junio C Hamano wrote:
> Victoria Dye <vdye@github.com> writes:
> 
>> I tried coming up with a user-facing name that wasn't too focused on the
>> internal implementation, but it ends up being misleading. The intention was
>> to have this be a variation of `git update-index` that uses the default
>> setting for `command_requires_full_index` but then proceeds to read and
>> write the index as `update-index` normally would. Something like
>> `--use-default-index-sparsity` might have been more accurate?
> 
> The option name in the reviewed patch does imply "we force expanding
> to full" and not "use the default", so it probably needs renaming,
> if we want the "use the default" semantics.  But is that useful in
> the context of the test you are using it in place of "reset" or "mv"?
> Even if the default is somehow flipped to use sparse always, wouldn't
> the particular test want the index expanded?  I dunno.
> 
>> In the test's use-case, `active_cache_changed` ends up set to
>> `CACHE_TREE_CHANGED`, which forces writing the index. It is still
>> effectively a no-op, but it serves the needs of the test.
> 
> Ah, cache-tree is updated, then it's OK.
> 
> As to test-tool vs end-user-accessible-command, I do not have a
> strong opinion, but use your imagination and ask Derrick or somebody
> else for their imagination to see if such a "force expand" feature
> may be something the end-users might need an access to in order to
> dig themselves out of a hole (in which case, it may be better to
> make it end-user-accessible) or not (in which case, test-tool is
> more appropriate).

I think there is something to be said about the name being confusing,
because the current implementation focuses on "expand a sparse index
upon read" but it also allows the index to be written as sparse.

Conversely, if the user runs

  git -c index.sparse=false update-index ...

then the index.sparse config setting forbids conversion from full to
sparse, but does not say anything about expanding to full.

Perhaps this should be corrected: the index.sparse=false setting
should expand a sparse index to a full one, then prevent it from
being converted to a sparse one on write.

This diff should do it:

diff --git a/read-cache.c b/read-cache.c
index 564283c7e7e..04df1051e18 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2376,7 +2376,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	if (!istate->repo)
 		istate->repo = the_repository;
 	prepare_repo_settings(istate->repo);
-	if (istate->repo->settings.command_requires_full_index)
+	if (!istate->repo->settings.sparse_index ||
+	    istate->repo->settings.command_requires_full_index)
 		ensure_full_index(istate);
 
 	return istate->cache_nr;

Victoria, what are your thoughts about including such a change?

Junio, would it be better to change the config setting, and then
update this test to use the config setting over a command-line flag?
This would allow us to punt on the --force-full-index flag until we
have time to focus on the 'git update-index' command and interactions
like this.

Thanks,
-Stolee