Re: [PATCH v7 15/16] wt-status: expand added sparse directory entries

On 7/12/2021 3:32 PM, Elijah Newren wrote:

On Mon, Jul 12, 2021 at 6:56 AM Derrick Stolee [off-list ref] wrote:

quoted

On 7/8/2021 9:03 PM, Elijah Newren wrote:

quoted

On Mon, Jun 28, 2021 at 7:05 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

...

quoted

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.

The same issue I highlighted last time is still present.  If you
insert an "exit 1" right here, then run
    ./t1092-sparse-checkout-compatibility.sh --ver --imm -x
until it stops, then
    cd t/trash directory.t1092-sparse-checkout-compatibility/sparse-checkout
    git ls-files -t | grep folder  # Note the files that are sparse
    git reset --mixed HEAD~1
    git ls-files -t | grep folder  # Note the files that are sparse --
there are some that aren't that should be
    git sparse-checkout reapply
    git ls-files -t | grep folder  # Note the files that are sparse

Granted, this is a bug with sparse-checkout without sparse-index, so
not something new to your series.  But since you are using comparisons
between regular sparse-checkouts and sparse-index to verify
correctness, this seems problematic to me.

I'll add it to the pile, but I want to continue having this series
focus on making the sparse-index work quickly without a change in
behavior from a normal index. Changing the behavior of the sparse-
checkout feature should be a separate series.

Hmm..perhaps there's some middle ground?  I appreciate that you want
to have this series focus on making the sparse-index work without
worrying about behavioral changes to sparse-checkout.  I'm concerned,
though, that testcases tend to be treated as documentation of intended
behavior, even when the tests are testing something else.  These tests
are clearly triggering buggy behavior, and I think your comments and
subsequent command may be affected by it.  I don't want to leave
future folks (even ourselves) to have to try to explain away why the
behavior expected in this test should not be expected.

Perhaps we can just add a comment that this testcase is triggering a
bug in both sparse-checkout and sparse-index but we're only checking
that the two match, and that once the bug is fix, the testcase itself
may need tweaking?

 
I can get behind that approach: document the bug, but comment that it
_is_ a bug and should be changed in the future.

Thanks,
-Stolee