Re: [PATCH v2 2/7] update-index: add --force-full-index option for expand/collapse test

[Elijah Newren <newren@gmail.com>]

On Wed, Oct 6, 2021 at 1:40 PM Victoria Dye <vdye@github.com> wrote:
>
> Elijah Newren wrote:
> > On Tue, Oct 5, 2021 at 6:20 AM Victoria Dye via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Victoria Dye <vdye@github.com>
> >>
> >> Add a new `--force-full-index` option to `git update-index`, which skips
> >> explicitly setting `command_requires_full_index`. This lets `git
> >> update-index --force-full-index` run as a command without sparse index
> >> compatibility implemented, even after it receives sparse index compatibility
> >> updates.
> >>
> >> By using `git update-index --force-full-index` in the `t1092` test
> >> `sparse-index is expanded and converted back`, commands can continue to
> >> integrate with the sparse index without the need to keep modifying the
> >> command used in the test.
> >
> > So...we're adding a permanent user-facing command line flag, whose
> > purpose is just to help us with the transition work of implementing
> > sparse indexes everywhere?  Am I reading that right, or is that just
> > the reason for t1092 and there are more reasons for it elsewhere?
> >
> > Also, I'm curious if update-index is the right place to add this.  If
> > you don't want a sparse index anymore, wouldn't a user want to run
> >    git sparse-checkout disable
> > ?  Or is the point that you do want to keep the sparse checkout, but
> > you just don't want the index to also be sparse?  Still, even in that
> > case, it seems like adding a subcommand or flag to an existing
> > sparse-checkout subcommand would feel more natural, since
> > sparse-checkout is the command the user uses to request to get into a
> > sparse-checkout and sparse index.
> >
>
> This came out of a conversation [1] on an earlier version of this patch.
> Because the `t1092 - sparse-index is expanded and converted back` test
> verifies sparse index compatibility (i.e., expand the index when reading,
> collapse back to sparse when writing) on commands that don't have any sparse
> index integration, it needed to be changed from `git reset` to something
> else. However, as we keep integrating commands with sparse index we'd need
> to keep changing the command in the test, creating a bunch of patches doing
> effectively the same thing for no long-term benefit.
>
> The `--force-full-index` flag isn't meant to be used externally or modify
> the index in any "new" way - it's really just a "test" version of `git
> update-index` that we guarantee will accurately represent a command using
> the default settings. Right now, it does exactly what `git update-index`
> (without the flag) does, and will only behave differently once `git
> update-index` is integrated with sparse index. Using `--force-full-index`,
> the test won't need to be regularly updated and will continue to catch
> errors like:
>
> 1. Changing the default value of `command_requires_full_index` to 0
> 2. Not expanding a sparse index to full when `command_requires_full_index`
>    is 1
> 3. Not collapsing the index back to sparse if sparse index is enabled
>
> I see the issue of introducing a test-only option (when sparse index is
> integrated everywhere, shouldn't it be deprecated?). If there's a way to
> make this more obviously internal/temporary, I'm happy to modify it. Or, if
> semi-frequent updates of the command in the test aren't a huge issue, I can
> revert to V1.

If it's a test-only capability you need, I'd say add it under
t/helpers/ somewhere, either a new flag for an existing subcommand of
test-tool, or a new subcommand for test-tool.