On 3/15/2022 12:07 PM, Victoria Dye wrote: > Shaoxuan Yuan wrote: >> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> >> --- >> builtin/mv.c | 3 +++ >> t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++ >> 2 files changed, 37 insertions(+) >> > > Hi Shaoxuan! Hello! > I'll answer your question "are the tests on the right track?" [1] inline > with the tests here. > In 't1092', I've tried to write test cases around some of the > characteristics relevant to sparse checkout/sparse index. For example: > > - files inside vs. outside of sparse cone (e.g., 'deep/a' vs 'folder1/a') > - "normal" directories vs. sparse directories (e.g., 'deep/' vs. 'folder1/') > - directories inside a sparse directory vs. "toplevel" sparse directories > (e.g., 'folder1/0/' vs. 'folder1/') > - options that follow different code paths, especially if those code paths > interact with the index differently (e.g., 'git reset --hard' vs 'git > reset --mixed') > - (probably not relevant for 'git mv') files with vs. without staged changes > in the index > > I've found that exercising these characteristics provides good baseline > coverage for a sparse index integration, not leaving any major gaps. I'll > also typically add cases specific to any workarounds I need to add to a > command (like for 'git read-tree --prefix' [2]). This, and other advice that Victoria mentions, are really good points to keep in mind. > My recommendations: > > - add tests covering outside-of-sparse-cone 'mv' arguments > - add tests covering 'mv' attempting to move directories (in-cone and > sparse) > - add some "test_must_fail" tests to see what happens when you do something > "wrong", e.g. to try to overwrite a file without '-f' (I've found some > really interesting issues in the past where you expect something to fail > and it doesn't) > - add 'git status --porcelain=v2' checks to confirm that the 'mv' worked the > same across the different checkouts > - remove multiples of test cases that test the same general behavior (e.g., > 'git mv <in-cone file> <in-cone file>' only needs to be done once) > - double-check whether '-v' and '-k' have the ability to affect > full-checkout/sparse-checkout/sparse-index differently - if not, you > probably don't need to test them > > Thanks for working on this, and I hope this helps! You mention in your cover letter that the ensure_not_expanded tests are not added yet (same with performance tests). Now that you've gotten feedback on this version of the patch, I might recommend the organization you might want for a full series: 1. Add these 'mv' tests to t1092 _without_ the code change. These tests should work when the index is expanded, and making the code change to not expand the index shouldn't change the behavior. 2. Add the performance test so we have a baseline to measure how well 'mv' does in the normal case (and how it is slower when expanding the index). 3. Make the code change and add the ensure_not_expanded test, since the functionality from the tests added in (1) will not change and we can report the results from the perf tests added in (2). The only thing to test is the new, internal behavior that the index is not expanded when doing these actions. (Keep in mind that we expect the index to be expanded for out-of-cone moves, but it's the in-cone moves that we expect to not expand.) Thanks! -Stolee
