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

[Derrick Stolee <stolee@gmail.com>]

On 6/9/2021 1:27 AM, Elijah Newren wrote:
> On Mon, Jun 7, 2021 at 5:34 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
...
>> +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.
> 
>> +       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.

>> +       test_sparse_match git status --porcelain=v2 &&
>> +       test_sparse_match git status --porcelain=v2 &&
> 
> Why is this test run twice?
> 
>> +
>> +       # 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?
> 
>> +'
>> +
>> +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