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

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