| Message ID | 8aefce6254c0bcbbbca909a62d033c74c90f980b.1631453010.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior | expand |
On Sun, Sep 12, 2021 at 6:23 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > The tests in t3705-add-sparse-checkout.sh check to see how 'git add' > behaves with paths outside the sparse-checkout definition. These > currently check to see if a given warning is present but not that the > index is not updated with the sparse entries. Add a new > 'test_sparse_entry_unstaged' helper to be sure 'git add' is behaving > correctly. > > We need to modify setup_sparse_entry to actually commit the sparse_entry > file so it exists at HEAD but is not already staged in the index. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > t/t3705-add-sparse-checkout.sh | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh > index 2b1fd0d0eef..af81b4b6846 100755 > --- a/t/t3705-add-sparse-checkout.sh > +++ b/t/t3705-add-sparse-checkout.sh > @@ -19,6 +19,7 @@ setup_sparse_entry () { > fi && > git add sparse_entry && > git update-index --skip-worktree sparse_entry && > + git commit --allow-empty -m "ensure sparse_entry exists at HEAD" && > SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry) > } > > @@ -36,6 +37,11 @@ setup_gitignore () { > EOF > } > > +test_sparse_entry_unstaged () { > + git status --porcelain >actual && > + ! grep "^M sparse_entry\$" actual Is there a reason this is ^M rather than ^D? Granted, both would be bugs so I wouldn't expect either to appear, but the point of the check is looking for likely errors. Wouldn't the more likely error case for a file not present in the working tree be that we stage the deletion of the file? > +} > + > test_expect_success 'setup' " > cat >sparse_error_header <<-EOF && > The following pathspecs didn't match any eligible path, but they do match index > @@ -55,6 +61,7 @@ test_expect_success 'git add does not remove sparse entries' ' > setup_sparse_entry && > rm sparse_entry && > test_must_fail git add sparse_entry 2>stderr && > + test_sparse_entry_unstaged && > test_cmp error_and_hint stderr && > test_sparse_entry_unchanged > ' > @@ -73,6 +80,7 @@ test_expect_success 'git add . does not remove sparse entries' ' > rm sparse_entry && > setup_gitignore && > test_must_fail git add . 2>stderr && > + test_sparse_entry_unstaged && > > cat sparse_error_header >expect && > echo . >>expect && > @@ -88,6 +96,7 @@ do > setup_sparse_entry && > echo modified >sparse_entry && > test_must_fail git add $opt sparse_entry 2>stderr && > + test_sparse_entry_unstaged && > test_cmp error_and_hint stderr && > test_sparse_entry_unchanged > ' > @@ -98,6 +107,7 @@ test_expect_success 'git add --refresh does not update sparse entries' ' > git ls-files --debug sparse_entry | grep mtime >before && > test-tool chmtime -60 sparse_entry && > test_must_fail git add --refresh sparse_entry 2>stderr && > + test_sparse_entry_unstaged && > test_cmp error_and_hint stderr && > git ls-files --debug sparse_entry | grep mtime >after && > test_cmp before after > @@ -106,6 +116,7 @@ test_expect_success 'git add --refresh does not update sparse entries' ' > test_expect_success 'git add --chmod does not update sparse entries' ' > setup_sparse_entry && > test_must_fail git add --chmod=+x sparse_entry 2>stderr && > + test_sparse_entry_unstaged && > test_cmp error_and_hint stderr && > test_sparse_entry_unchanged && > ! test -x sparse_entry > @@ -116,6 +127,7 @@ test_expect_success 'git add --renormalize does not update sparse entries' ' > setup_sparse_entry "LINEONE\r\nLINETWO\r\n" && > echo "sparse_entry text=auto" >.gitattributes && > test_must_fail git add --renormalize sparse_entry 2>stderr && > + test_sparse_entry_unstaged && > test_cmp error_and_hint stderr && > test_sparse_entry_unchanged > ' > @@ -124,6 +136,7 @@ test_expect_success 'git add --dry-run --ignore-missing warn on sparse path' ' > setup_sparse_entry && > rm sparse_entry && > test_must_fail git add --dry-run --ignore-missing sparse_entry 2>stderr && > + test_sparse_entry_unstaged && > test_cmp error_and_hint stderr && > test_sparse_entry_unchanged > ' > @@ -148,6 +161,7 @@ test_expect_success 'do not warn when pathspec matches dense entries' ' > test_expect_success 'add obeys advice.updateSparsePath' ' > setup_sparse_entry && > test_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr && > + test_sparse_entry_unstaged && > test_cmp sparse_entry_error stderr > > ' > -- > gitgitgadget Looks fine otherwise.
On 9/15/2021 1:22 AM, Elijah Newren wrote: > On Sun, Sep 12, 2021 at 6:23 AM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Derrick Stolee <dstolee@microsoft.com> >> >> The tests in t3705-add-sparse-checkout.sh check to see how 'git add' >> behaves with paths outside the sparse-checkout definition. These >> currently check to see if a given warning is present but not that the >> index is not updated with the sparse entries. Add a new >> 'test_sparse_entry_unstaged' helper to be sure 'git add' is behaving >> correctly. >> >> We need to modify setup_sparse_entry to actually commit the sparse_entry >> file so it exists at HEAD but is not already staged in the index. >> >> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> >> --- >> t/t3705-add-sparse-checkout.sh | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh >> index 2b1fd0d0eef..af81b4b6846 100755 >> --- a/t/t3705-add-sparse-checkout.sh >> +++ b/t/t3705-add-sparse-checkout.sh >> @@ -19,6 +19,7 @@ setup_sparse_entry () { >> fi && >> git add sparse_entry && >> git update-index --skip-worktree sparse_entry && >> + git commit --allow-empty -m "ensure sparse_entry exists at HEAD" && >> SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry) >> } >> >> @@ -36,6 +37,11 @@ setup_gitignore () { >> EOF >> } >> >> +test_sparse_entry_unstaged () { >> + git status --porcelain >actual && >> + ! grep "^M sparse_entry\$" actual > > Is there a reason this is ^M rather than ^D? Granted, both would be > bugs so I wouldn't expect either to appear, but the point of the check > is looking for likely errors. Wouldn't the more likely error case for > a file not present in the working tree be that we stage the deletion > of the file? You are right that we should be checking for deletions or adds _as well_ as modifications. The test_sparse_entry_unstaged helper is used in a variety of situations that typically would trigger a modification, but at least one instance in this test is a possible deletion. > >> +} >> + >> test_expect_success 'setup' " >> cat >sparse_error_header <<-EOF && >> The following pathspecs didn't match any eligible path, but they do match index >> @@ -55,6 +61,7 @@ test_expect_success 'git add does not remove sparse entries' ' >> setup_sparse_entry && >> rm sparse_entry && >> test_must_fail git add sparse_entry 2>stderr && >> + test_sparse_entry_unstaged && Here, sparse_entry could be staged as a deletion. >> test_cmp error_and_hint stderr && >> test_sparse_entry_unchanged >> ' >> @@ -73,6 +80,7 @@ test_expect_success 'git add . does not remove sparse entries' ' >> rm sparse_entry && >> setup_gitignore && >> test_must_fail git add . 2>stderr && >> + test_sparse_entry_unstaged && Deletion here. >> cat sparse_error_header >expect && >> echo . >>expect && >> @@ -88,6 +96,7 @@ do >> setup_sparse_entry && >> echo modified >sparse_entry && >> test_must_fail git add $opt sparse_entry 2>stderr && >> + test_sparse_entry_unstaged && But here would be modified. >> test_cmp error_and_hint stderr && >> test_sparse_entry_unchanged >> ' >> @@ -98,6 +107,7 @@ test_expect_success 'git add --refresh does not update sparse entries' ' >> git ls-files --debug sparse_entry | grep mtime >before && >> test-tool chmtime -60 sparse_entry && >> test_must_fail git add --refresh sparse_entry 2>stderr && >> + test_sparse_entry_unstaged && Same here. >> test_cmp error_and_hint stderr && >> git ls-files --debug sparse_entry | grep mtime >after && >> test_cmp before after >> @@ -106,6 +116,7 @@ test_expect_success 'git add --refresh does not update sparse entries' ' >> test_expect_success 'git add --chmod does not update sparse entries' ' >> setup_sparse_entry && >> test_must_fail git add --chmod=+x sparse_entry 2>stderr && >> + test_sparse_entry_unstaged && Here it would be modified with a mode change. Using the pattern "^[MDARCU][M ] sparse_entry\$" seems to work while also covering these other cases. Thanks, -Stolee
On Sun, Sep 12, 2021 at 10:23 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > The tests in t3705-add-sparse-checkout.sh check to see how 'git add' > behaves with paths outside the sparse-checkout definition. These > currently check to see if a given warning is present but not that the > index is not updated with the sparse entries. Hmm, I probably missed something, but don't we already check that with the `test_sparse_entry_unchanged` helper? The only test case that we don't call it is 'git add --refresh does not update sparse entries', but we explicitly compare the cached 'mtime' from before and after `git add` there.
On 9/15/2021 12:32 PM, Matheus Tavares wrote: > On Sun, Sep 12, 2021 at 10:23 AM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Derrick Stolee <dstolee@microsoft.com> >> >> The tests in t3705-add-sparse-checkout.sh check to see how 'git add' >> behaves with paths outside the sparse-checkout definition. These >> currently check to see if a given warning is present but not that the >> index is not updated with the sparse entries. > > Hmm, I probably missed something, but don't we already check that with > the `test_sparse_entry_unchanged` helper? The only test case that we > don't call it is 'git add --refresh does not update sparse entries', > but we explicitly compare the cached 'mtime' from before and after > `git add` there. test_sparse_entry_unchanged does a bit more by actually requiring that we fully know the mode and OID of the object in the index. Since some of the tests modify sparse_entry and then update the index using the --sparse option (including a mode change with --chmod=x), it seems more robust to avoid an exact match from ls-files. Thanks, -Stolee
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh index 2b1fd0d0eef..af81b4b6846 100755 --- a/t/t3705-add-sparse-checkout.sh +++ b/t/t3705-add-sparse-checkout.sh @@ -19,6 +19,7 @@ setup_sparse_entry () { fi && git add sparse_entry && git update-index --skip-worktree sparse_entry && + git commit --allow-empty -m "ensure sparse_entry exists at HEAD" && SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry) } @@ -36,6 +37,11 @@ setup_gitignore () { EOF } +test_sparse_entry_unstaged () { + git status --porcelain >actual && + ! grep "^M sparse_entry\$" actual +} + test_expect_success 'setup' " cat >sparse_error_header <<-EOF && The following pathspecs didn't match any eligible path, but they do match index @@ -55,6 +61,7 @@ test_expect_success 'git add does not remove sparse entries' ' setup_sparse_entry && rm sparse_entry && test_must_fail git add sparse_entry 2>stderr && + test_sparse_entry_unstaged && test_cmp error_and_hint stderr && test_sparse_entry_unchanged ' @@ -73,6 +80,7 @@ test_expect_success 'git add . does not remove sparse entries' ' rm sparse_entry && setup_gitignore && test_must_fail git add . 2>stderr && + test_sparse_entry_unstaged && cat sparse_error_header >expect && echo . >>expect && @@ -88,6 +96,7 @@ do setup_sparse_entry && echo modified >sparse_entry && test_must_fail git add $opt sparse_entry 2>stderr && + test_sparse_entry_unstaged && test_cmp error_and_hint stderr && test_sparse_entry_unchanged ' @@ -98,6 +107,7 @@ test_expect_success 'git add --refresh does not update sparse entries' ' git ls-files --debug sparse_entry | grep mtime >before && test-tool chmtime -60 sparse_entry && test_must_fail git add --refresh sparse_entry 2>stderr && + test_sparse_entry_unstaged && test_cmp error_and_hint stderr && git ls-files --debug sparse_entry | grep mtime >after && test_cmp before after @@ -106,6 +116,7 @@ test_expect_success 'git add --refresh does not update sparse entries' ' test_expect_success 'git add --chmod does not update sparse entries' ' setup_sparse_entry && test_must_fail git add --chmod=+x sparse_entry 2>stderr && + test_sparse_entry_unstaged && test_cmp error_and_hint stderr && test_sparse_entry_unchanged && ! test -x sparse_entry @@ -116,6 +127,7 @@ test_expect_success 'git add --renormalize does not update sparse entries' ' setup_sparse_entry "LINEONE\r\nLINETWO\r\n" && echo "sparse_entry text=auto" >.gitattributes && test_must_fail git add --renormalize sparse_entry 2>stderr && + test_sparse_entry_unstaged && test_cmp error_and_hint stderr && test_sparse_entry_unchanged ' @@ -124,6 +136,7 @@ test_expect_success 'git add --dry-run --ignore-missing warn on sparse path' ' setup_sparse_entry && rm sparse_entry && test_must_fail git add --dry-run --ignore-missing sparse_entry 2>stderr && + test_sparse_entry_unstaged && test_cmp error_and_hint stderr && test_sparse_entry_unchanged ' @@ -148,6 +161,7 @@ test_expect_success 'do not warn when pathspec matches dense entries' ' test_expect_success 'add obeys advice.updateSparsePath' ' setup_sparse_entry && test_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr && + test_sparse_entry_unstaged && test_cmp sparse_entry_error stderr '