Derrick Stolee wrote: > 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. > This helps clarify what I was misinterpreting in the test. It isn't looking for "default" behavior, it's verifying whether trace2 logs capture index expansion and collapse when those operations are expected to happen, regardless of whether that's because `command_requires_full_index` is 1 or because the command needs to use entries inside of sparse directories. With that interpretation, I can replace the command with `git reset update-folder1 -- folder1/a` and get the same result (without needing to change the test in the future *or* add a new `git` command option / `test-tool` subcommand). > 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? > I think this is a worthwhile change, but I'd prefer submitting it separately (either in an upcoming sparse index integration or on its own). It's not directly needed by anything in this series, and I'd like to avoid adding features to the scope if possible.
