Re: [PATCH v5 13/14] wt-status: expand added sparse directory entries

On 6/9/2021 1:27 AM, Elijah Newren wrote:

On Mon, Jun 7, 2021 at 5:34 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

...

quoted

+test_expect_success 'reset mixed and checkout orphan' '
+       init_repos &&
+
+       test_all_match git checkout rename-out-to-in &&
+
+       # Sparse checkouts do not agree with full checkouts about
+       # how to report a directory/file conflict during a reset.
+       # This command would fail with test_all_match because the
+       # full checkout reports "T folder1/0/1" while a sparse
+       # checkout reports "D folder1/0/1". This matches because
+       # the sparse checkouts skip "adding" the other side of
+       # the conflict.
+       test_sparse_match git reset --mixed HEAD~1 &&

Ooh!  I think you found a sparse-checkout bug here.  I agree that
sparse-checkouts and full-checkouts should give different output in
this case, but I don't think the current difference is the correct
one.  Digging in a little closer, before running `git reset --mixed
HEAD~1` I see:

$ git ls-files -t | grep folder
S folder1/0/0/0
S folder1/0/1
S folder2/0/0/0
S folder2/0/1/1
S folder2/a
S folder2/larger-content

and after running git reset --mixed HEAD~1, I see:
S folder1/0/0/0
S folder1/0/1
H folder1/a
H folder1/larger-content
S folder2/0/0/0
H folder2/0/1
S folder2/a
S folder2/larger-content

meaning that the reset of the index failed.  It thinks some entries
are present in the working copy, though it didn't actually check any
of them out, leaving them to be marked as deleted.  This leaves the
sparse-checkout in a messed up state.  To correct it, I need to run
either of the following:

    git diff --diff-filter=D --name-only | xargs git update-index
--skip-worktree

or

    git sparse-checkout reapply

(Though one could ask whether sparse-checkout reapply should take a
missing file that isn't SKIP_WORKTREE and determine it's okay to just
mark it as SKIP_WORKTREE rather than treating it as dirty.  I'm not
sure the answer to that...)

I really think that `git reset --mixed ...` should have been getting
the sparsity right on its own without the manual fixup afterwards that
I needed to add.

quoted

+       test_sparse_match test-tool read-cache --table --expand &&

If both the full and the sparse checkouts do a reset --mixed, I would
think that this step should be able to use a test_all_match...at least
if reset --mixed weren't broken.

I will add this to my list when getting to 'git reset' integration
with sparse-checkout. Thanks.

quoted

+       test_sparse_match git status --porcelain=v2 &&
+       test_sparse_match git status --porcelain=v2 &&

Why is this test run twice?

quoted

+
+       # At this point, sparse-checkouts behave differently
+       # from the full-checkout.
+       test_sparse_match git checkout --orphan new-branch &&
+       test_sparse_match test-tool read-cache --table --expand &&
+       test_sparse_match git status --porcelain=v2 &&
+       test_sparse_match git status --porcelain=v2

And again, you run the status twice...why?

quoted

+'
+
+test_expect_success 'add everything with deep new file' '
+       init_repos &&
+
+       run_on_sparse git sparse-checkout set deep/deeper1/deepest &&
+
+       run_on_all touch deep/deeper1/x &&
+       test_all_match git add . &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git status --porcelain=v2

same question.

These double 'git status' calls are actually a bit subtle: there
was a bug in an earlier version that only appeared when using
'git status' twice, because the first kept the sparse index
without expanding it, and the bug actually had an incorrect
result when writing that index. Only the second 'git status'
would notice the problem. I started adding two calls to my tests,
but it is not necessary any more.

The reason to leave it out of the Git tests is that I'm testing
all of my submissions against the Scalar functional tests which
run 'git status' multiple times throughout each test situation
and that catches the problem as well. In the future, we will have
'git add' keeping the sparse index in-memory; that will also
expose this behavior sufficiently.

Thanks,
-Stolee