On Wed, Feb 17, 2021 at 3:01 PM Junio C Hamano [off-list ref] wrote:

Matheus Tavares [off-list ref] writes:

quoted

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

quoted

Signed-off-by: Matheus Tavares <redacted>
---
 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.

quoted

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

quoted

+
+     if test "$#" -eq 1

No need to quote $# (we know it is a number).

quoted

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

quoted

+     else
+             printf "" >sparse_entry

Just

                >sparse_entry

is sufficient, no?

quoted

+     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 () {

quoted

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

quoted

+}

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+'
+
+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/ (local);
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.