Re: [PATCH v3 11/12] wt-status: expand added sparse directory entries

On 5/17/2021 10:27 PM, Elijah Newren wrote:

On Fri, May 14, 2021 at 11:31 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

From: Derrick Stolee <redacted>

It is difficult, but possible, to get into a state where we intend to
add a directory that is outside of the sparse-checkout definition. Add a

Then we need to fix that; allowing things to be added outside the
sparse-checkout definition is a bug[1][2].  That's an invariant I
believe we should maintain everywhere; things get really confusing to
users somewhere later down the road if we don't.  Matheus worked to
fix that with 'git add'; if there are other commands that need fixing
too, then we should also fix them.

[1] https://lore.kernel.org/git/CABPp-BFhyFiKSXdLM5q5t=ZKzr6V0pY7dbheierRaOHFbMEdkg@mail.gmail.com/ (local)
[2] https://lore.kernel.org/git/CABPp-BF0ZhbSs42R3Bw_r-hbhQ71qtbXSBqXdq0djyaan=8p=A@mail.gmail.com/ (local)

quoted

test to t1092-sparse-checkout-compatibility.sh that demonstrates this
using a combination of 'git reset --mixed' and 'git checkout --orphan'.

I think `git checkout --orphan` should just throw an error if
sparse-checkout is in use.  Allowing adding paths outside the
sparse-checkout set causes too much collateral and deferred confusion
for users.

I've been trying to strike an interesting balance of creating
performance improvements without changing behavior, trying to
defer those behavior changes to an isolated instance. I think
that approach is unavoidable with the 'git add' work that I
pulled out of this series and will return to soon.

However, here I think it would be too much to start throwing
an error in this case. I think that change is a bit too much.

The thing I can try to do, instead of the current approach, is
to not allow sparse directory entries to differ between the
index and HEAD. That will satisfy this case, but also a lot of
other painful cases.

I have no idea how to actually accomplish that, but I'll start
digging.

quoted

This test failed before because the output of 'git status
--porcelain=v2' would not match on the lines for folder1/:

* The sparse-checkout repo (with a full index) would output each path
  name that is intended to be added.

* The sparse-index repo would only output that "folder1/" is staged for
  addition.

The status should report the full list of files to be added, and so this
sparse-directory entry should be expanded to a full list when reaching
it inside the wt_status_collect_changes_initial() method. Use
read_tree_at() to assist.

Having a sparse directory entry whose object_id in the index does not
match HEAD should be an error.

I can get behind this understanding.

 Have a CE_SKIP_WORKTREE non-directory
whose object_id in the index does not match HEAD should also be an
error.

I'm less convinced here. At minimum, I'm not willing to stake
a firm claim and change the behavior around this statement in
the current series.

 I don't think we should complicate the code to try to handle
violations of those assumptions.  I do think we should add checks to
enforce that constraint (or BUG() if it's violated).

A BUG() is likely too strict, because existing Git clients can
get users into this state, and then they upgrade and are suddenly
in a BUG() state. We should perhaps do our best effort to avoid
this case and handle it as appropriately as possible.

And yeah, that also means 'git sparse-checkout add/set' would need to
error out if paths are requested to be sparsified despite being
different from HEAD.

This would be a reasonable thing, assuming the established
behavior is changed.

quoted

Somehow, this loop over the cache entries was not guarded by
ensure_full_index() as intended.

Signed-off-by: Derrick Stolee <redacted>
---
 t/t1092-sparse-checkout-compatibility.sh | 28 +++++++++++++
 wt-status.c                              | 50 ++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 59faf7381093..cd3669d36b53 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -492,4 +492,32 @@ test_expect_success 'sparse-index is not expanded' '
        test_region ! index ensure_full_index trace2.txt
 '

+test_expect_success 'reset mixed and checkout orphan' '
+       init_repos &&
+
+       test_all_match git checkout rename-out-to-in &&
+       test_all_match git reset --mixed HEAD~1 &&
+       test_sparse_match test-tool read-cache --table --expand &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git status --porcelain=v2 &&
+
+       # 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
+'
+
+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
+'>

This was a really nice catch that you got this particular testcase.
While I disagree with the fix, I do have to say nice work on the catch
and the implementation otherwise.

This test exists almost verbatim in the Scalar and VFS For Git
functional tests. I have no idea what context caused it to be
necessary.

I can understand your aversion to the solution I presented here.
Preventing sparse directory entries that differ from the tree at
HEAD for that path should be more robust to future integrations.

Thanks,
-Stolee