On 10/8/21 1:19 PM, Junio C Hamano wrote: > Victoria Dye <vdye@xxxxxxxxxx> 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
