Re: [RFC PATCH 3/7] t3705: add tests for `git add` in sparse checkouts

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Feb 18, 2021, 03:07 UTC
Message-ID: <CAHd-oW6b0n7ezA-C8NuUOkHUSfM8TFLmkJtwwSRv7FRtbOWjsA@mail.gmail.com>
In-Reply-To: <xmqqpn0yi9ii.fsf@gitster.g>

On Wed, Feb 17, 2021 at 8:01 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines

>
> Matheus Tavares <matheus.bernardino@usp.br> 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.

Show 19 quoted lines

> > +     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.

Show 20 quoted lines

> > +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?

Show 9 quoted lines

> > +'
> > +
> > +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