| Message ID | a1fa7c080aed2056afaad6415186c125c04a80cb.1633013461.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse Index: integrate with reset | expand |
On Thu, Sep 30, 2021 at 02:50:56PM +0000, Victoria Dye via GitGitGadget wrote: > From: Victoria Dye <vdye@github.com> > > In anticipation of multiple commands being fully integrated with sparse > index, update the test for index expand and collapse for non-sparse index > integrated commands to use `mv`. > > Signed-off-by: Victoria Dye <vdye@github.com> > --- > t/t1092-sparse-checkout-compatibility.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index c5977152661..aed8683e629 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="" mv a b && Double-checking my understanding as somebody who is not so familiar with t1092: this test is to ensure that commands which don't yet understand the sparse index can temporarily expand it in order to do their work? If so, makes sense to me. And renaming 'a' to 'b' is arbitrary and fine to do since we end up recreating the sparse-index repository each time via init_repos. Looks good to me. Thanks, Taylor
Taylor Blau wrote: > On Thu, Sep 30, 2021 at 02:50:56PM +0000, Victoria Dye via GitGitGadget wrote: >> From: Victoria Dye <vdye@github.com> >> >> In anticipation of multiple commands being fully integrated with sparse >> index, update the test for index expand and collapse for non-sparse index >> integrated commands to use `mv`. >> >> Signed-off-by: Victoria Dye <vdye@github.com> >> --- >> t/t1092-sparse-checkout-compatibility.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh >> index c5977152661..aed8683e629 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="" mv a b && > > Double-checking my understanding as somebody who is not so familiar with > t1092: this test is to ensure that commands which don't yet understand > the sparse index can temporarily expand it in order to do their work? Exactly - if a command doesn't explicitly enable use of the sparse index by setting `command_requires_full_index` to 0, the index is expanded if/when it is first read during the command's execution and collapsed if/when it is written to disk. This test makes sure that mechanism works as intended. -Victoria
Victoria Dye <vdye@github.com> writes: > Taylor Blau wrote: >> On Thu, Sep 30, 2021 at 02:50:56PM +0000, Victoria Dye via GitGitGadget wrote: >>> From: Victoria Dye <vdye@github.com> >>> >>> In anticipation of multiple commands being fully integrated with sparse >>> index, update the test for index expand and collapse for non-sparse index >>> integrated commands to use `mv`. >>> ... >>> 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="" mv a b && >> >> Double-checking my understanding as somebody who is not so familiar with >> t1092: this test is to ensure that commands which don't yet understand >> the sparse index can temporarily expand it in order to do their work? > > Exactly - if a command doesn't explicitly enable use of the sparse index by > setting `command_requires_full_index` to 0, the index is expanded if/when it > is first read during the command's execution and collapsed if/when it is > written to disk. This test makes sure that mechanism works as intended. Sorry, I do not quite follow. So is this "before this series of patches, 'reset --hard' can be used to as a sample of a command that expands and then collapses, but because it no longer is a good sample of a command so we replace it with 'mv a b'"? Do we need to update this further when "mv a b" learns to expand and then collapse?
Junio C Hamano wrote: > Victoria Dye <vdye@github.com> writes: > >> Taylor Blau wrote: >>> On Thu, Sep 30, 2021 at 02:50:56PM +0000, Victoria Dye via GitGitGadget wrote: >>>> From: Victoria Dye <vdye@github.com> >>>> >>>> In anticipation of multiple commands being fully integrated with sparse >>>> index, update the test for index expand and collapse for non-sparse index >>>> integrated commands to use `mv`. >>>> ... >>>> 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="" mv a b && >>> >>> Double-checking my understanding as somebody who is not so familiar with >>> t1092: this test is to ensure that commands which don't yet understand >>> the sparse index can temporarily expand it in order to do their work? >> >> Exactly - if a command doesn't explicitly enable use of the sparse index by >> setting `command_requires_full_index` to 0, the index is expanded if/when it >> is first read during the command's execution and collapsed if/when it is >> written to disk. This test makes sure that mechanism works as intended. > > Sorry, I do not quite follow. > > So is this "before this series of patches, 'reset --hard' can be > used to as a sample of a command that expands and then collapses, > but because it no longer is a good sample of a command so we replace > it with 'mv a b'"? Yes, because this series enables sparse index integration in `git reset`, the test no longer applies to that command (but it does apply to `git mv`). > Do we need to update this further when "mv a b" > learns to expand and then collapse? Unfortunately, yes. `git mv` was picked more-or-less at random from the set of commands that read the index and don't already have sparse index integrations (excluding those I know are planned for sparse index integration in the near future). If `git mv` were to be updated to disable `command_requires_full_index`, the command in the test would need to change again. For what it's worth, I do think the test itself is valuable, since it makes sure a command's capability to use the sparse index is always the result of an intentional update to (and review of) the code.
Victoria Dye <vdye@github.com> writes: >> Do we need to update this further when "mv a b" >> learns to expand and then collapse? > > Unfortunately, yes. `git mv` was picked more-or-less at random from the set > of commands that read the index and don't already have sparse index > integrations (excluding those I know are planned for sparse index > integration in the near future). If `git mv` were to be updated to disable > `command_requires_full_index`, the command in the test would need to change > again. > > For what it's worth, I do think the test itself is valuable, since it makes > sure a command's capability to use the sparse index is always the result of > an intentional update to (and review of) the code. Oh, of course. I was actually wondering if it woudl be a good idea to leave a command that will never be "converted" so that we can keep using it for testing. Perhaps a new option that is invented exactly for the purpose added to a plumbing e.g. "git update-index --expand-collapse"?
On 30/09/21 21.50, Victoria Dye via GitGitGadget wrote: > From: Victoria Dye <vdye@github.com> > > In anticipation of multiple commands being fully integrated with sparse > index, update the test for index expand and collapse for non-sparse index > integrated commands to use `mv`. > We can say "use git sparse-index mv instead of git sparse-index reset". Why is mv used for this case?
Junio C Hamano wrote: > Victoria Dye <vdye@github.com> writes: > >>> Do we need to update this further when "mv a b" >>> learns to expand and then collapse? >> >> Unfortunately, yes. `git mv` was picked more-or-less at random from the set >> of commands that read the index and don't already have sparse index >> integrations (excluding those I know are planned for sparse index >> integration in the near future). If `git mv` were to be updated to disable >> `command_requires_full_index`, the command in the test would need to change >> again. >> >> For what it's worth, I do think the test itself is valuable, since it makes >> sure a command's capability to use the sparse index is always the result of >> an intentional update to (and review of) the code. > > Oh, of course. > > I was actually wondering if it woudl be a good idea to leave a > command that will never be "converted" so that we can keep using it > for testing. > > Perhaps a new option that is invented exactly for the purpose added > to a plumbing e.g. "git update-index --expand-collapse"? > That sounds good to me! I'll add an `update-index --expand-collapse` implementation and update the test in v2 of this series.
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index c5977152661..aed8683e629 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="" mv a b && test_region index convert_to_sparse trace2.txt && test_region index ensure_full_index trace2.txt '