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

[Derrick Stolee <stolee@gmail.com>]

On 5/17/2021 10:27 PM, Elijah Newren wrote:
> On Fri, May 14, 2021 at 11:31 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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/
> [2] https://lore.kernel.org/git/CABPp-BF0ZhbSs42R3Bw_r-hbhQ71qtbXSBqXdq0djyaan=8p=A@mail.gmail.com/
> 
>> 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@microsoft.com>
>> ---
>>  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