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

[Elijah Newren <newren@gmail.com>]

On Wed, Feb 17, 2021 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > We already have a couple tests for `add` with SKIP_WORKTREE entries in
> > t7012, but these only cover the most basic scenarios. As we will be
> > changing how `add` deals with sparse paths in the subsequent commits,
> > let's move these two tests to their own file and add more test cases
> > for different `add` options and situations. This also demonstrates two
> > options that don't currently respect SKIP_WORKTREE entries: `--chmod`
> > and `--renormalize`.
>
> Nice.  It makes sense to describe what we want first, like this step..
>
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > ---
> >  t/t3705-add-sparse-checkout.sh   | 92 ++++++++++++++++++++++++++++++++
> >  t/t7012-skip-worktree-writing.sh | 19 -------
> >  2 files changed, 92 insertions(+), 19 deletions(-)
> >  create mode 100755 t/t3705-add-sparse-checkout.sh
> >
> > diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
> > new file mode 100755
> > index 0000000000..5530e796b5
> > --- /dev/null
> > +++ b/t/t3705-add-sparse-checkout.sh
> > @@ -0,0 +1,92 @@
> > +#!/bin/sh
> > +
> > +test_description='git add in sparse checked out working trees'
> > +
> > +. ./test-lib.sh
> > +
> > +SPARSE_ENTRY_BLOB=""
> > +
> > +# Optionally take a string for the entry's contents
> > +setup_sparse_entry()
> > +{
>
> Style.
>
>         setup_sparse_entry () {
>
> on a single line.
>
> > +     if test -f sparse_entry
> > +     then
> > +             rm sparse_entry
> > +     fi &&
> > +     git update-index --force-remove sparse_entry &&
>
> Why not an unconditional removal on the working tree side?
>
>         rm -f sparse_entry &&
>         git update-index --force-remove sparse_entry &&
>
> Are there cases where we may have sparse_entry directory here?
>
> > +
> > +     if test "$#" -eq 1
>
> No need to quote $# (we know it is a number).
>
> > +     then
> > +             printf "$1" >sparse_entry
>
> Make sure the test writers know that they are passing a string that
> will be interpreted as a printf format.  Review the comment before
> the function and adjust it appropriately ("a string" is not what you
> want to tell them).
>
> > +     else
> > +             printf "" >sparse_entry
>
> Just
>
>                 >sparse_entry
>
> is sufficient, no?
>
> > +     fi &&
> > +     git add sparse_entry &&
> > +     git update-index --skip-worktree sparse_entry &&
> > +     SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry)
> > +}
> > +
> > +test_sparse_entry_unchanged() {
>
> Style.
>
>         test_sparse_entry_unchanged () {
>
> > +     echo "100644 $SPARSE_ENTRY_BLOB 0       sparse_entry" >expected &&
> > +     git ls-files --stage sparse_entry >actual &&
> > +     test_cmp expected actual
>
> OK.  So the expected pattern is to first "setup", do stuff that
> shouldn't affect the sparse entry in the index, and then call this
> to make sure?
>
> > +}
>
> > +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.
>
> > +     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 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?
>
> > +for opt in "" -f -u --ignore-removal
> > +do
> > +     if test -n "$opt"
> > +     then
> > +             opt=" $opt"
> > +     fi
>
> The above is cumulative, and as a consequence, "git add -u <path>"
> is not tested, but "git add -f -u <path>" is.  Intended?  How was
> the order of the options listed in "for opt in ..." chosen?
>
> > +     test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
> > +             setup_sparse_entry &&
> > +             echo modified >sparse_entry &&
> > +             git add $opt sparse_entry &&
> > +             test_sparse_entry_unchanged
> > +     '
> > +done
> > +
> > +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.
>
> > +'
> > +
> > +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?
>
> > +'
> > +
> > +test_expect_failure 'git add --renormalize does not update SKIP_WORKTREE entries' '
> > +     test_config core.autocrlf false &&
> > +     setup_sparse_entry "LINEONE\r\nLINETWO\r\n" &&
> > +     echo "sparse_entry text=auto" >.gitattributes &&
> > +     git add --renormalize sparse_entry &&
> > +     test_sparse_entry_unchanged
>
> Makes sense.
>
> What should "git diff sparse_entry" say at this point, I have to
> wonder?

It'll show nothing:

$ git show :0:sparse_entry
LINEONE
LINETWO
$ echo foobar >sparse_entry
$ cat sparse_entry
foobar
$ git diff sparse_entry
$

Likewise, `git status` will ignore SKIP_WORKTREE entries, and `reset
--hard` will fail to correct these files.  Several of these were
discussed at https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/;
Matheus is just cleaning up the first few candidates.

I thought that present-despite-SKIP_WORKTREE-setting files would be
extremely rare.  While they are somewhat rare, they show up more than
I thought.  The reason I've seen them appear for users is that they
hit Ctrl+C in the middle of progress updates for a `git
sparse-checkout {disable,add} ...` command; they decide mid-operation
that they specified the wrong set of sparsity paths or didn't really
want to unsparsify or whatever, and wrongly assume that operations are
atomic (individual files are checked out to the working copy first,
followed by an update of the index and .git/info/sparse-checkout file
at the end).

The same kind of issue exists without SKIP_WORKTREE when people use
checkout or switch to change branches and hit Ctrl+C in the middle,
but `git status` will warn users about those and `git reset --hard`
will clean them up.  Neither is true in the
present-despite-SKIP_WORKTREE files case.  That should also be fixed
up, but add & rm were easier low-hanging fruit.