Re: [PATCH 9/9] t1092: test interesting sparse-checkout scenarios

From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren <newren@gmail.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 9/9] t1092: test interesting sparse-checkout scenarios
Date: Thu, 21 Jan 2021 06:59:18 -0500	[thread overview]
Message-ID: <bd81bbda-152d-6230-bfc5-f4aa60d0fe9e@gmail.com> (raw)
In-Reply-To: <CABPp-BH=E4bR2K21e-p+ABTG02feDF7YNvrOKhsMW5Dtet6y7Q@mail.gmail.com>

On 1/20/2021 2:40 PM, Elijah Newren wrote:
> 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.

>> +
>> +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.

>> +
>> +       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.

>> +
>> +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.

>> +       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.

>> +
>> +# 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.

>> +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
