| Message ID | f7cb9013d46731855c3ed42add62b021c0ad0c73.1633440057.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse Index: integrate with reset | expand |
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. > Signed-off-by: Victoria Dye <vdye@github.com> > --- > Documentation/git-update-index.txt | 5 +++++ > builtin/update-index.c | 11 +++++++++++ > t/t1092-sparse-checkout-compatibility.sh | 2 +- > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt > index 2853f168d97..06255e321a3 100644 > --- a/Documentation/git-update-index.txt > +++ b/Documentation/git-update-index.txt > @@ -24,6 +24,7 @@ SYNOPSIS > [--[no-]fsmonitor] > [--really-refresh] [--unresolve] [--again | -g] > [--info-only] [--index-info] > + [--force-full-index] > [-z] [--stdin] [--index-version <n>] > [--verbose] > [--] [<file>...] > @@ -170,6 +171,10 @@ time. Version 4 is relatively young (first released in 1.8.0 in > October 2012). Other Git implementations such as JGit and libgit2 > may not support it yet. > > +--force-full-index:: > + Force the command to operate on a full index, expanding a sparse > + index if necessary. > + > -z:: > Only meaningful with `--stdin` or `--index-info`; paths are > separated with NUL character instead of LF. > diff --git a/builtin/update-index.c b/builtin/update-index.c > index 187203e8bb5..32ada3ead77 100644 > --- a/builtin/update-index.c > +++ b/builtin/update-index.c > @@ -964,6 +964,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) > int split_index = -1; > int force_write = 0; > int fsmonitor = -1; > + int use_default_full_index = 0; > struct lock_file lock_file = LOCK_INIT; > struct parse_opt_ctx_t ctx; > strbuf_getline_fn getline_fn; > @@ -1069,6 +1070,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) > {OPTION_SET_INT, 0, "no-fsmonitor-valid", &mark_fsmonitor_only, NULL, > N_("clear fsmonitor valid bit"), > PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG}, > + OPT_SET_INT(0, "force-full-index", &use_default_full_index, > + N_("run with full index explicitly required"), 1), > OPT_END() > }; > > @@ -1082,6 +1085,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) > if (newfd < 0) > lock_error = errno; > > + /* > + * If --force-full-index is set, the command should skip manually > + * setting `command_requires_full_index`. > + */ > + prepare_repo_settings(r); > + if (!use_default_full_index) > + r->settings.command_requires_full_index = 1; > + > entries = read_cache(); > if (entries < 0) > die("cache corrupted"); > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index c5977152661..b3c0d3b98ee 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -642,7 +642,7 @@ test_expect_success 'sparse-index is expanded and converted back' ' > init_repos && > > GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \ > - git -C sparse-index -c core.fsmonitor="" reset --hard && > + git -C sparse-index -c core.fsmonitor="" update-index --force-full-index && > test_region index convert_to_sparse trace2.txt && > test_region index ensure_full_index trace2.txt > ' > -- > gitgitgadget
On 05/10/21 20.20, Victoria Dye via GitGitGadget 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. `... explicitly setting ...` or `... explicitly set ...`? I thought of the latter.
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. [1] https://lore.kernel.org/git/xmqqr1d58v9x.fsf@gitster.g/
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.
Elijah Newren <newren@gmail.com> writes: >> 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. Is the ability to force expanding to full index completely useless in the field? For diagnosing breakage the end-users may see in the wild, or perhaps in a specialist usecase for whatever reason working on full index is preferable and the user may want to force it once to correct an earlier mistake to enable sparse-index before toggling the configuration off, or something? If we do not foresee any such reason, I'd agree it is good to move that to t/helpers/; otherwise, I think update-index is as good as any other place, and the option will sit well next to other options like "--[no-]skip-worktree", "--[no-]assume-unchanged". It would most likely need to be used together with "--force-write-index" (or be made to imply the latter) to be useful, I suspect. Thanks.
diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt index 2853f168d97..06255e321a3 100644 --- a/Documentation/git-update-index.txt +++ b/Documentation/git-update-index.txt @@ -24,6 +24,7 @@ SYNOPSIS [--[no-]fsmonitor] [--really-refresh] [--unresolve] [--again | -g] [--info-only] [--index-info] + [--force-full-index] [-z] [--stdin] [--index-version <n>] [--verbose] [--] [<file>...] @@ -170,6 +171,10 @@ time. Version 4 is relatively young (first released in 1.8.0 in October 2012). Other Git implementations such as JGit and libgit2 may not support it yet. +--force-full-index:: + Force the command to operate on a full index, expanding a sparse + index if necessary. + -z:: Only meaningful with `--stdin` or `--index-info`; paths are separated with NUL character instead of LF. diff --git a/builtin/update-index.c b/builtin/update-index.c index 187203e8bb5..32ada3ead77 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -964,6 +964,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) int split_index = -1; int force_write = 0; int fsmonitor = -1; + int use_default_full_index = 0; struct lock_file lock_file = LOCK_INIT; struct parse_opt_ctx_t ctx; strbuf_getline_fn getline_fn; @@ -1069,6 +1070,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) {OPTION_SET_INT, 0, "no-fsmonitor-valid", &mark_fsmonitor_only, NULL, N_("clear fsmonitor valid bit"), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG}, + OPT_SET_INT(0, "force-full-index", &use_default_full_index, + N_("run with full index explicitly required"), 1), OPT_END() }; @@ -1082,6 +1085,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) if (newfd < 0) lock_error = errno; + /* + * If --force-full-index is set, the command should skip manually + * setting `command_requires_full_index`. + */ + prepare_repo_settings(r); + if (!use_default_full_index) + r->settings.command_requires_full_index = 1; + entries = read_cache(); if (entries < 0) die("cache corrupted"); diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index c5977152661..b3c0d3b98ee 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -642,7 +642,7 @@ test_expect_success 'sparse-index is expanded and converted back' ' init_repos && GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \ - git -C sparse-index -c core.fsmonitor="" reset --hard && + git -C sparse-index -c core.fsmonitor="" update-index --force-full-index && test_region index convert_to_sparse trace2.txt && test_region index ensure_full_index trace2.txt '