On 4/21/2021 11:14 AM, Matheus Tavares Bernardino wrote: > Hi, Stolee > > You already said you will make changes in this test to make sure > git-add's sparse warning is kept on a sparse index (BTW thanks for > that :), but I just wanted to give a couple suggestions that came to > my mind while reading the patch. I appreciate the suggestions! More tests always help me from making mistakes, and you are definitely more of a 'git add' expert than me. >> + test_must_fail git -C sparse-checkout add folder1/a && >> + test_must_fail git -C sparse-index add folder1/a && > > To make sure the output is the same, could we collapse these two lines into: > > test_sparse_match test_must_fail git add folder1/a ? This is elegant. I'm sad I didn't think of it earlier. > And additionally, I think we could repeat this check with `add > --refresh` and also after removing `folder1/a`. The reason I'm saying > this is because the check currently succeeds when `folder1/a` is in > the working tree (maybe because `fill_directory()` ends up expanding > the sparse index in this case?), but not under the two other > circumstances I mentioned (as we've discussed in [1]). > > [1]: https://lore.kernel.org/git/CAHd-oW7vCKC-XRM=rX37+jQn_XDzjtar9nNHKQ-4OHSZ=2=KFA@xxxxxxxxxxxxxx/ Can do! >> + git -C full-checkout checkout HEAD -- folder1/a && >> + test_sparse_match git status --porcelain=v2 && > > Hmm, shouldn't this be `test_all_match`? IIUC, we've resetted > `folder1/a` on the full repo to make sure the status report is the > same across all repos, right? Yes! Thanks, -Stolee
