Re: [PATCH 9/9] t1092: test interesting sparse-checkout scenarios
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jan 21, 2021, 11:59 UTC
- Message-ID
- <bd81bbda-152d-6230-bfc5-f4aa60d0fe9e@gmail.com>
- In-Reply-To
- <CABPp-BH=E4bR2K21e-p+ABTG02feDF7YNvrOKhsMW5Dtet6y7Q@mail.gmail.com>
On 1/20/2021 2:40 PM, Elijah Newren wrote:
Show 12 quoted lines
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Derrick Stolee <dstolee@microsoft.com> >> >> These also document some behaviors that differ from a full checkout, and >> possibly in a way that is not intended. > > I'm in favor. I should turn some of my noted weird behaviors from [1] > into testcases as well. > > [1] https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/
That would be helpful, and adding them here would be helpful.
For the cases where things don't work correctly, it is good to add "test_expect_failure" cases as a TODO list for the feature space.
Show 26 quoted lines
>> + >> +test_expect_success 'add, commit, checkout' ' >> + init_repos && >> + >> + write_script edit-contents <<-\EOF && >> + echo text >>$1 >> + EOF >> + run_on_all "../edit-contents README.md" && >> + >> + test_all_match git add README.md && >> + test_all_match git status --porcelain=v2 && >> + test_all_match git commit -m "Add README.md" && >> + >> + test_all_match git checkout HEAD~1 && >> + test_all_match git checkout - && >> + >> + run_on_all "../edit-contents README.md" && >> + >> + test_all_match git add -A && >> + test_all_match git status --porcelain=v2 && >> + test_all_match git commit -m "Extend README.md" && >> + >> + test_all_match git checkout HEAD~1 && >> + test_all_match git checkout - && > > Up to here, this test is identical to the previous one. Why repeat it?
Unintentional. I cleaned this up in a later commit of my prototype, but forgot to delete that back in this patch. Will fix.
Show 20 quoted lines
>> + >> + run_on_all "../edit-contents deep/newfile" && >> + >> + test_all_match git status --porcelain=v2 -uno && >> + test_all_match git status --porcelain=v2 && >> + test_all_match git add . && >> + test_all_match git status --porcelain=v2 && >> + test_all_match git commit -m "add deep/newfile" && >> + >> + test_all_match git checkout HEAD~1 && >> + test_all_match git checkout - >> +' > > Think out loud...so you are only adding files that were not previously > tracked and that would have been part of the sparse cone. You aren't > trying to add files that would be outside the sparse cone, or manually > creating files missing from the working tree due to sparseness and > then attempting to add them. (Which is fine, we have to start > somewhere with our testing. Also, I think my testcases didn't look at > the case you did, and only covered one of these other two cases.)
Yes, these tests are currently focusing on the "happy" cases of what is happening within the sparse cone. I plan to expand to the more complicated cases later, as I start implementing them correctly with the sparse-index. However, it would be fine to have the tests here earlier. Extra documentation of the expected behavior (or how the current implementation is not desirable) would be good.
Show 50 quoted lines
>> + >> +test_expect_success 'checkout and reset --hard' ' >> + init_repos && >> + >> + test_all_match git checkout update-folder1 && >> + test_all_match git status --porcelain=v2 && >> + >> + test_all_match git checkout update-deep && >> + test_all_match git status --porcelain=v2 && >> + >> + test_all_match git checkout -b reset-test && >> + test_all_match git reset --hard deepest && >> + test_all_match git reset --hard update-folder1 && >> + test_all_match git reset --hard update-folder2 >> +' >> + >> +test_expect_success 'diff --staged' ' >> + init_repos && >> + >> + write_script edit-contents <<-\EOF && >> + echo text >>README.md >> + EOF >> + run_on_all "../edit-contents" && >> + >> + test_all_match git diff && >> + test_all_match git diff --staged && >> + test_all_match git add README.md && >> + test_all_match git diff && >> + test_all_match git diff --staged >> +' > > Also a case where you're operating on a file that matches the sparsity > patterns (in cone mode, everything in the root directory is included). > >> +test_expect_success 'diff with renames' ' >> + init_repos && >> + >> + for branch in rename-out-to-out rename-out-to-in rename-in-to-out >> + do >> + test_all_match git checkout rename-base && >> + test_all_match git checkout $branch -- .&& >> + test_all_match git diff --staged && >> + test_all_match git diff --staged --find-renames || return 1 > > Aren't these last two lines the same? (diff.renames defaults to true > ever since commit 5404c116aa, "diff: activate diff.renames by > default", 2016-02-25) Are they only different because you have a > tweaked config that turns off renames by default? > > Perhaps the first diff line should have a --no-renames flag.
Thanks! Will do.
Show 37 quoted lines
>> + done >> +' >> + >> +test_expect_success 'log with pathspec outside sparse definition' ' >> + init_repos && >> + >> + test_all_match git log -- a && >> + test_all_match git log -- folder1/a && >> + test_all_match git log -- folder2/a && >> + test_all_match git log -- deep/a && >> + test_all_match git log -- deep/deeper1/a && >> + test_all_match git log -- deep/deeper1/deepest/a && >> + >> + test_all_match git checkout update-folder1 && >> + test_all_match git log -- folder1/a >> +' >> + >> +test_expect_success 'blame with pathspec inside sparse definition' ' >> + init_repos && >> + >> + test_all_match git blame a && >> + test_all_match git blame deep/a && >> + test_all_match git blame deep/deeper1/a && >> + test_all_match git blame deep/deeper1/deepest/a >> +' > > Good check. > > On a side note going back to a piece of the other thread I didn't get > a response to, I'm still curious whether > git blame -C -C $PATH_INSIDE_SPARSE_DEFINITION > should (optionally?) behave differently in a sparse checkout. In > particular, should it limit its copy detection to other paths also in > the sparse checkout, or should it always search all other files within > the repository for copied lines? Searching just within the sparse > checkout seems like it could be a really nice performance > optimization.
All of the "find movements or copies" logic could benefit from a "universal" option to restrict to the sparse-checkout definition.
Show 8 quoted lines
>> + >> +# TODO: blame currently does not support blaming files outside of the >> +# sparse definition. It complains that the file doesn't exist locally. > > Nice catch. Yeah, blame tries to check the local working copy for > changes, and shows those lines with a changed in commit 0000000000. > We should add a check that says that if the file is SKIP_WORKTREE, > then we treat it the same as `git blame $PATH HEAD`.
Right. If it's not in the working directory, then we should interpret that as HEAD.
Show 25 quoted lines
>> +test_expect_failure 'blame with pathspec outside sparse definition' ' >> + init_repos && >> + >> + test_all_match git blame folder1/a && >> + test_all_match git blame folder2/a && >> + test_all_match git blame deep/deeper2/a && >> + test_all_match git blame deep/deeper2/deepest/a >> +' >> + >> +# TODO: reset currently does not behave as expected when in a >> +# sparse-checkout. > > I'm going to go to test this out to see what it does. It's the first > testcase you listed that I didn't know how it worked and couldn't > figure it out from your comments. However it turns out, definitely a > good test to have. > >> +test_expect_failure 'checkout and reset (mixed)' ' >> + init_repos && >> + >> + test_all_match git checkout -b reset-test update-deep && >> + test_all_match git reset deepest && >> + test_all_match git reset update-folder1 && >> + test_all_match git reset update-folder2 >> +'
Here are the failures:
after "git checkout -b reset-test update-deep"
--- full-checkout-out 2021-01-21 11:51:30.059713246 +0000 +++ sparse-checkout-out 2021-01-21 11:51:30.063713235 +0000
@@ -1,5 +1,5 @@ Unstaged changes after reset: M deep/a M deep/deeper1/deepest/a -M folder1/a -M folder2/a +D folder1/a +D folder2/a If we comment out that line, the failures for "git reset update-folder1" is: --- full-checkout-out 2021-01-21 11:52:36.203509990 +0000 +++ sparse-checkout-out 2021-01-21 11:52:36.207509976 +0000 @@ -1,2 +1,2 @@ Unstaged changes after reset: -M folder1/a +D folder1/a If we comment out that line, then the failure for "git reset update-folder2" is: --- full-checkout-out 2021-01-21 11:53:55.199197608 +0000 +++ sparse-checkout-out 2021-01-21 11:53:55.207197573 +0000 @@ -1,3 +1,3 @@ Unstaged changes after reset: -M folder1/a -M folder2/a +D folder1/a +D folder2/a Oddly, when I merge this into our branch in microsoft/git, these failures disappear. There is something in those commits that resolve this particular case. I hope to figure that out sometime. Thanks, -Stolee