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

On 10/8/21 1:19 PM, Junio C Hamano wrote:

Victoria Dye [off-list ref] writes:

quoted

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.

quoted

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