On Wed, Feb 17, 2021 at 8:01 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Matheus Tavares <matheus.bernardino@xxxxxx> writes: > > > > +test_expect_success "git add does not remove SKIP_WORKTREE entries" ' > > We use the term SKIP_WORKTREE and SPARSE interchangeably here. I > wonder if it is easier to understand if we stick to one e.g. by > saying "... does not remove 'sparse' entries" instead? I dunno. Good idea, thanks. > > + setup_sparse_entry && > > + rm sparse_entry && > > + git add sparse_entry && > > + test_sparse_entry_unchanged > > Wow. OK. Makes a reader wonder what should happen when the two > operations are replaced with "git rm sparse_entry"; let's read on. > > > +' > > + > > +test_expect_success "git add -A does not remove SKIP_WORKTREE entries" ' > > + setup_sparse_entry && > > + rm sparse_entry && > > + git add -A && > > + test_sparse_entry_unchanged > > +' > > OK. As there is nothing other than sparse_entry in the working tree > or in the index, the above two should be equivalent. I just realized that the "actual" file created by the previous test_sparse_entry_unchanged would also be added to the index here. This doesn't affect the current test or the next ones, but I guess we could use `git add -A sparse_entry` to avoid any future problems. > I wonder what should happen if the "add -A" gets replaced with "add ."; > it should behave the same way, I think. Is it worth testing that > case as well, or is it redundant? Hmm, I think it might be better to test only `add -A sparse_entry`, to avoid adding the "actual" file or others that might be introduced in future changes. > > +test_expect_success 'git add --refresh does not update SKIP_WORKTREE entries' ' > > + setup_sparse_entry && > > + test-tool chmtime -60 sparse_entry && > > + git add --refresh sparse_entry && > > + > > + # We must unset the SKIP_WORKTREE bit, otherwise > > + # git diff-files would skip examining the file > > + git update-index --no-skip-worktree sparse_entry && > > + > > + echo sparse_entry >expected && > > + git diff-files --name-only sparse_entry >actual && > > + test_cmp actual expected > > Hmph, I am not sure what we are testing at this point. One way to > make the final diff-files step show sparse_entry would be for "git > add --refresh" to be a no-op, in which case, the cached stat > information in the index would be different in mtime from the path > in the working tree. But "update-index --no-skip-worktree" may be > buggy and further change or invalidate the cached stat information > to cause diff-files to report that the path may be different. Oh, that is a problem... We could use `git ls-files --debug` and directly compare the mtime field. But the ls-files doc says that --debug format may change at any time... Any other idea? > > +' > > + > > +test_expect_failure 'git add --chmod does not update SKIP_WORKTREE entries' ' > > + setup_sparse_entry && > > + git add --chmod=+x sparse_entry && > > + test_sparse_entry_unchanged > > Hmph. Should we also check if sparse_entry in the filesystem also > is not made executable, not just the entry in the index? I think so. This is already tested at the "core" --chmod tests in t3700, but it certainly wouldn't hurt to test here too. Thanks for all the great feedback
