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

Commit Message

Matheus Tavares Feb. 17, 2021, 9:02 p.m. UTC

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

Signed-off-by: Matheus Tavares <[e-mail]>
---
 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

Comments

Junio C Hamano Feb. 17, 2021, 11:01 p.m. UTC | #1

Matheus Tavares <[e-mail]> 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 <[e-mail]>
> ---
>  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?

> +'
> +
> +test_done

Thanks.

Eric Sunshine Feb. 17, 2021, 11:22 p.m. UTC | #2

On Wed, Feb 17, 2021 at 6:04 PM Junio C Hamano <[e-mail]> wrote:
> Matheus Tavares <[e-mail]> writes:
> > +for opt in "" -f -u --ignore-removal
> > +do
> > +     if test -n "$opt"
> > +     then
> > +             opt=" $opt"
> > +     fi
> > +     test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
>
> 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?

I may be misreading, but I don't think this is cumulative (though it's
easy to mistake it as such due to the way it inserts a space before
$opt). My interpretation is that `opt` gets overwritten with a new
value on each iteration, and it is inserting the space merely to make
the test title print nicely. A more idiomatic way to do this would
have been:

    for opt in "" -f -u --ignore-removal
    do
        test_expect_success " git add${opt:+ $opt} does ..." '

Junio C Hamano Feb. 17, 2021, 11:34 p.m. UTC | #3

Eric Sunshine <[e-mail]> writes:

> On Wed, Feb 17, 2021 at 6:04 PM Junio C Hamano <[e-mail]> wrote:
>> Matheus Tavares <[e-mail]> writes:
>> > +for opt in "" -f -u --ignore-removal
>> > +do
>> > +     if test -n "$opt"
>> > +     then
>> > +             opt=" $opt"
>> > +     fi
>> > +     test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
>>
>> 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?
>
> I may be misreading, but I don't think this is cumulative (though it's
> easy to mistake it as such due to the way it inserts a space before
> $opt).

Ah, yes, I misread it.

> ... A more idiomatic way to do this would
> have been:
>
>     for opt in "" -f -u --ignore-removal
>     do
>         test_expect_success " git add${opt:+ $opt} does ..." '

Yes, indeed.

Matheus Tavares Feb. 18, 2021, 3:07 a.m. UTC | #4

On Wed, Feb 17, 2021 at 8:01 PM Junio C Hamano <[e-mail]> wrote:
>
> Matheus Tavares <[e-mail]> 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

Matheus Tavares Feb. 18, 2021, 3:11 a.m. UTC | #5

On Wed, Feb 17, 2021 at 8:22 PM Eric Sunshine <[e-mail]> wrote:
>
> On Wed, Feb 17, 2021 at 6:04 PM Junio C Hamano <[e-mail]> wrote:
> > Matheus Tavares <[e-mail]> writes:
> > > +for opt in "" -f -u --ignore-removal
> > > +do
> > > +     if test -n "$opt"
> > > +     then
> > > +             opt=" $opt"
> > > +     fi
> > > +     test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
> >
> ... A more idiomatic way to do this would
> have been:
>
>     for opt in "" -f -u --ignore-removal
>     do
>         test_expect_success " git add${opt:+ $opt} does ..." '

That's much better, thanks :)

Matheus Tavares Feb. 18, 2021, 2:38 p.m. UTC | #6

On Thu, Feb 18, 2021 at 12:07 AM Matheus Tavares Bernardino <[e-mail]> wrote:
>
> On Wed, Feb 17, 2021 at 8:01 PM Junio C Hamano <[e-mail]> wrote:
> >
> > Matheus Tavares <[e-mail]> writes:
> > >
> > > +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?

Or maybe we could use a test helper for this? Something like:

-- >8 --
#include "test-tool.h"
#include "cache.h"

int cmd__cached_mtime(int argc, const char **argv)
{
	int i;

	setup_git_directory();
	if (read_cache() < 0)
		die("could not read the index");

	for (i = 1; i < argc; i++) {
		const struct stat_data *sd;
		int pos = cache_name_pos(argv[i], strlen(argv[i]));

		if (pos < 0) {
			pos = -pos-1;
			if (pos < active_nr && !strcmp(active_cache[pos]->name, argv[i]))
				die("'%s' is unmerged", argv[i]);
			else
				die("'%s' is not in the index", argv[i]);
		}

		sd = &active_cache[pos]->ce_stat_data;
		printf("%s %u:%u\n", argv[i], sd->sd_mtime.sec, sd->sd_mtime.nsec);
	}

	discard_cache();
	return 0;
}
-- >8 --

Then, the test would become:

	setup_sparse_entry &&
	test-tool cached_mtime sparse_entry >before &&
	test-tool chmtime -60 sparse_entry &&
	git add --refresh sparse_entry &&
	test-tool cached_mtime sparse_entry >after &&
	test_cmp before after

What do you think?

Junio C Hamano Feb. 18, 2021, 7:02 p.m. UTC | #7

Matheus Tavares Bernardino <[e-mail]> writes:

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

Rewriting 'git add -A' to 'git add -A sparse_entry' may not be wrong
but it will invite "does -A somehow misbehave without pathspec?" and
other puzzlements.

If adding 'actual' or 'expect' do not matter, I think it would be OK
to just add it, but if it bothers you, we can prepare an .gitignore
and list them early in the test with a comment that says "we will do
many 'git add .' and the like and do not want to be affected by what
the test needs to use to verify the result".

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

The option is to help us developers; if somebody wants to change it
and breaks your tests, they are responsible for rewriting their
change in such a way to keep your tests working or adjust your tests
to their new output format.

Junio C Hamano Feb. 18, 2021, 7:05 p.m. UTC | #8

Matheus Tavares <[e-mail]> writes:

> Then, the test would become:
>
> 	setup_sparse_entry &&
> 	test-tool cached_mtime sparse_entry >before &&
> 	test-tool chmtime -60 sparse_entry &&
> 	git add --refresh sparse_entry &&
> 	test-tool cached_mtime sparse_entry >after &&
> 	test_cmp before after
>
> What do you think?

I do not see much point in introducing a duplicated "ls-files --debug"
that gives only a subset of its output.  Even if we add test-tool,
we would need to reserve the right to change its output format any time,
so I am not sure what we'd be gaining by avoiding the use of
existing one.

Thanks.

Elijah Newren Feb. 22, 2021, 6:53 p.m. UTC | #9

On Wed, Feb 17, 2021 at 3:01 PM Junio C Hamano <[e-mail]> wrote:
>
> Matheus Tavares <[e-mail]> 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 <[e-mail]>
> > ---
> >  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/[e-mail]/;
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.

Patch

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()
+{
+	if test -f sparse_entry
+	then
+		rm sparse_entry
+	fi &&
+	git update-index --force-remove sparse_entry &&
+
+	if test "$#" -eq 1
+	then
+		printf "$1" >sparse_entry
+	else
+		printf "" >sparse_entry
+	fi &&
+	git add sparse_entry &&
+	git update-index --skip-worktree sparse_entry &&
+	SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry)
+}
+
+test_sparse_entry_unchanged() {
+	echo "100644 $SPARSE_ENTRY_BLOB 0	sparse_entry" >expected &&
+	git ls-files --stage sparse_entry >actual &&
+	test_cmp expected actual
+}
+
+test_expect_success "git add does not remove SKIP_WORKTREE entries" '
+	setup_sparse_entry &&
+	rm sparse_entry &&
+	git add sparse_entry &&
+	test_sparse_entry_unchanged
+'
+
+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
+'
+
+for opt in "" -f -u --ignore-removal
+do
+	if test -n "$opt"
+	then
+		opt=" $opt"
+	fi
+
+	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
+'
+
+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
+'
+
+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
+'
+
+test_done
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index e5c6a038fb..217207c1ce 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -60,13 +60,6 @@  setup_absent() {
 	git update-index --skip-worktree 1
 }
 
-test_absent() {
-	echo "100644 $EMPTY_BLOB 0	1" > expected &&
-	git ls-files --stage 1 > result &&
-	test_cmp expected result &&
-	test ! -f 1
-}
-
 setup_dirty() {
 	git update-index --force-remove 1 &&
 	echo dirty > 1 &&
@@ -100,18 +93,6 @@  test_expect_success 'index setup' '
 	test_cmp expected result
 '
 
-test_expect_success 'git-add ignores worktree content' '
-	setup_absent &&
-	git add 1 &&
-	test_absent
-'
-
-test_expect_success 'git-add ignores worktree content' '
-	setup_dirty &&
-	git add 1 &&
-	test_dirty
-'
-
 test_expect_success 'git-rm fails if worktree is dirty' '
 	setup_dirty &&
 	test_must_fail git rm 1 &&