On Fri, Mar 10, 2023 at 1:23 PM Victoria Dye <vdye@xxxxxxxxxx> wrote: Hi Victoria! Sorry for the late reply! I was having midterm exams. Thank you for the feedback on the patch . I appreciate your time giving me the advice for improvement. > > In order to have staged changes outside of > > the sparse-checkout cone, create a 'newdirectory/testfile' and > > add it to the index, while leaving it outside of > > the sparse-checkout definition.Test 'newdirectory/testfile' > nit: missing space after "definition." Will do! > I'd be interested in seeing an additional test case for a pathspec with > wildcards or other "magic" [1], e.g. 'git diff-files "deep/*"'. In past > sparse index integrations, there has occasionally been a need for special > handling of those types of pathspecs [2][3], so it would be good for the > test to cover cases like that. Will do! > From the comment you've added here, it looks like the state you want to test > is "file outside sparse checkout definition exists on disk". However, since > one of the goals of this test is to verify sparse index behavior once its > integrated with the 'diff-files' command, whether the "outside of sparse- > checkout definition" file belongs to a sparse directory entry is an > important (and fairly nuanced) factor to consider. > > The main difference between a "regular" sparse-checkout and a sparse > index-enabled sparse-checkout is the existence of "sparse directory" > entries: index entries with 'SKIP_WORKTREE' that represent directories and > their contents. In the context of these tests, the thing we really want to > verify is that the sparse index-enabled case produces the same results as > the full index when an operation needs to get some information out of a > sparse directory. > > Coming back to this test, the 'newdirectory/testfile' you create, while > outside the sparse-checkout definition, never actually belongs to a sparse > directory because 'newdirectory' is never collapsed - in fact, I don't > think it even gets the SKIP_WORKTREE bit in the index. To ensure you have a > sparse directory & SKIP_WORKTREE, you'd need to run 'git sparse-checkout > reapply' after removing 'newdirectory/testfile' from disk - which doesn't > help if you want to test what happens when the file exists on disk! Thanks for the explanation here! I learn a lot. > In fact, because of built-in safeguards around on-disk files and > sparse-checkout, there isn't really a way in Git to materialize a file > without also expanding its sparse directory and removing SKIP_WORKTREE. If > you want to preserve a sparse directory, you should write the contents of an > existing file inside that sparse directory to disk manually: > > run_on_sparse mkdir folder1 && > run_on_sparse cp a folder1/a && # `folder1/a` is identical to `a` in the base commit > > Git's index will not be touched by doing this, meaning the next command you > invoke (in this case, 'diff-files') will need to reconcile the file on disk > with what it sees in the index. > > > + test_sparse_match git diff-files && > > + test_must_be_empty sparse-checkout-out && > > + test_must_be_empty sparse-checkout-err && > > + test_sparse_match git diff-files newdirectory/testfile && > > + test_must_be_empty sparse-checkout-out && > > + test_must_be_empty sparse-checkout-err && > > These checks should be 'test_all_match' rather than 'test_sparse_match'. > Since (through other tests) we're confident that 'git diff-files' on an > unmodified, non-sparse-checkout repository will give us an empty result, you > wouldn't need the additional 'test_must_be_empty' checks. > > A bit of a "spoiler": when I tested this out locally, I found that the diff > was *not* empty for the sparse-checkout cases, until I ran 'git status' > (which strips the 'SKIP_WORKTREE' bit from the file and writes it to the > index). That's not the desired behavior, so there's a bug in the > sparse-checkout logic used in 'diff-files' that needs to be fixed (my first > guess would be that 'clear_skip_worktree_from_present_files()' is not being > applied when the index is read). Yeah. After I use run_on_sparse mkdir folder1 && run_on_sparse cp a folder1/a && # `folder1/a` is identical to `a` in the base commit diff was *not* empty for the sparse-checkout cases when I look into builtin/diff-files.c 'repo_read_index_preload' call 'repo_read_index' which call 'clear_skip_worktree_from_present_files()' to apply when the index is read. I got stuck in here and do not have the idea to investigate it. Any tips would be helpful! > > + run_on_sparse ../edit-contents newdirectory/testfile && > > + test_sparse_match git diff-files && > > + test_cmp expect sparse-checkout-out && > > + test_sparse_match git diff-files newdirectory/testfile && > > + test_cmp expect sparse-checkout-out > > Similarly, you should use 'run_on_all' to modify the file & 'test_all_match' > to verify that they all match here. It would demonstrate that we don't > expect any "special" behavior from sparse-checkout, meaning you can probably > avoid checking the result verbatim. > Finally, as with the earlier test, it'd be nice to show that the result is > the same with a wildcard pathspec, e.g. 'git diff-files "folder*/a"'. Will do! ---------------------------------------------------------------------------- Thanks Shuqi
