On 5/17/2021 10:27 PM, Elijah Newren wrote: > On Fri, May 14, 2021 at 11:31 AM Derrick Stolee via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> >> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxxxx/ > [2] https://lore.kernel.org/git/CABPp-BF0ZhbSs42R3Bw_r-hbhQ71qtbXSBqXdq0djyaan=8p=A@xxxxxxxxxxxxxx/ > >> 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. >> 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. >> Somehow, this loop over the cache entries was not guarded by >> ensure_full_index() as intended. >> >> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> --- >> 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
