[PATCH 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index'

[PATCH 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index'

[Victoria Dye via GitGitGadget]

This series continues the work to integrate commands with the sparse index,
adding integrations with 'git clean', 'git checkout-index', and 'git
update-index'. These three commands, while useful in their own right, are
updated mainly because they're used in 'git stash'. A future series will
integrate sparse index with 'stash' directly, but its subcommands must be
integrated to avoid the performance cost of each one expanding and
collapsing the index.

The series is broken up into 4 parts:

 * Patches 1-2 are minor fixups to the 'git reset' sparse index integration
   in response to discussion [1] that came after the series was ready for
   merge to 'next'.
 * Patch 3 integrates 'git clean' with the sparse index.
 * Patches 4-6 integrate 'git checkout-index' with the sparse index and
   introduce a new '--ignore-skip-worktree-bits' option for use with 'git
   checkout-index --all'.
   * This involves changing the behavior of '--all' to respect
     'skip-worktree' by default (i.e., it won't check out 'skip-worktree'
     files). The '--ignore-skip-worktree-bits' option can be specified to
     force checkout of 'skip-worktree' files, if desired.
 * Patches 7-9 integrate 'git update-index' with the sparse index.
   * Note that, although this integrates the sparse index with
     '--cacheinfo', sparse directories still cannot be updated using that
     option (see the prior discussion [2] for more details on why)

Thanks!

 * Victoria

[1]
https://lore.kernel.org/git/CABPp-BG0iDHf268UAnRyA=0y0T69YTc+bLMdxCmSbrL8s=9ziA@mail.gmail.com/

[2]
https://lore.kernel.org/git/a075091c-d0d4-db5d-fa21-c9d6c90c343e@gmail.com/

Victoria Dye (9):
  reset: fix validation in sparse index test
  reset: reorder wildcard pathspec conditions
  clean: integrate with sparse index
  checkout-index: expand sparse checkout compatibility tests
  checkout-index: add --ignore-skip-worktree-bits option
  checkout-index: integrate with sparse index
  update-index: add tests for sparse-checkout compatibility
  update-index: integrate with sparse index
  update-index: reduce scope of index expansion in do_reupdate

 Documentation/git-checkout-index.txt     |  11 +-
 builtin/checkout-index.c                 |  40 +++-
 builtin/clean.c                          |   3 +
 builtin/reset.c                          |  12 +-
 builtin/update-index.c                   |  17 +-
 read-cache.c                             |  10 +-
 t/perf/p2000-sparse-operations.sh        |   2 +
 t/t1092-sparse-checkout-compatibility.sh | 230 ++++++++++++++++++++++-
 8 files changed, 306 insertions(+), 19 deletions(-)


base-commit: dcc0cd074f0c639a0df20461a301af6d45bd582e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1109%2Fvdye%2Fsparse%2Fupdate-index_checkout-index_clean-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1109/vdye/sparse/update-index_checkout-index_clean-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1109
-- 
gitgitgadget

[PATCH 1/9] reset: fix validation in sparse index test

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update t1092 test 'reset with pathspecs outside sparse definition' to verify
index contents. The use of `rev-parse` verifies the contents of HEAD, not
the index, providing no real validation of the reset results. Conversely,
`ls-files` reports the contents of the index (OIDs, flags, filenames), which
are then compared across checkouts to ensure compatible index states.

Fixes 741a2c9ffa (reset: expand test coverage for sparse checkouts,
2021-09-27).

Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 49f70a65692..d5167e7ed69 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -596,13 +596,11 @@ test_expect_success 'reset with pathspecs outside sparse definition' '
 
 	test_sparse_match git reset update-folder1 -- folder1 &&
 	git -C full-checkout reset update-folder1 -- folder1 &&
-	test_sparse_match git status --porcelain=v2 &&
-	test_all_match git rev-parse HEAD:folder1 &&
+	test_all_match git ls-files -s -- folder1 &&
 
 	test_sparse_match git reset update-folder2 -- folder2/a &&
 	git -C full-checkout reset update-folder2 -- folder2/a &&
-	test_sparse_match git status --porcelain=v2 &&
-	test_all_match git rev-parse HEAD:folder2/a
+	test_all_match git ls-files -s -- folder2/a
 '
 
 test_expect_success 'reset with wildcard pathspec' '
-- 
gitgitgadget

[PATCH 2/9] reset: reorder wildcard pathspec conditions

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Rearrange conditions in method determining whether index expansion is
necessary when a pathspec is specified for `git reset`, placing less
expensive condition first. Additionally, add details & examples to related
code comments to help with readability.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/reset.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index b1ff699b43a..79b40385b99 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -204,10 +204,16 @@ static int pathspec_needs_expanded_index(const struct pathspec *pathspec)
 			/*
 			 * Special case: if the pattern is a path inside the cone
 			 * followed by only wildcards, the pattern cannot match
-			 * partial sparse directories, so we don't expand the index.
+			 * partial sparse directories, so we know we don't need to
+			 * expand the index.
+			 *
+			 * Examples:
+			 * - in-cone/foo***: doesn't need expanded index
+			 * - not-in-cone/bar*: may need expanded index
+			 * - **.c: may need expanded index
 			 */
-			if (path_in_cone_mode_sparse_checkout(item.original, &the_index) &&
-			    strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len)
+			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
+			    path_in_cone_mode_sparse_checkout(item.original, &the_index))
 				continue;
 
 			for (pos = 0; pos < active_nr; pos++) {
-- 
gitgitgadget

[PATCH 3/9] clean: integrate with sparse index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Remove full index requirement for `git clean` and test to ensure the index
is not expanded in `git clean`. Add to existing test for `git clean` to
verify cleanup of untracked files in sparse directories is consistent
between sparse index and non-sparse index checkouts.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/clean.c                          |  3 +++
 t/t1092-sparse-checkout-compatibility.sh | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/builtin/clean.c b/builtin/clean.c
index 98a2860409b..5628fc7103e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -983,6 +983,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		dir.flags |= DIR_KEEP_UNTRACKED_CONTENTS;
 	}
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index d5167e7ed69..05587361452 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -764,23 +764,42 @@ test_expect_success 'clean' '
 	test_all_match git commit -m "ignore bogus files" &&
 
 	run_on_sparse mkdir folder1 &&
+	run_on_all mkdir -p deep/untracked-deep &&
 	run_on_all touch folder1/bogus &&
+	run_on_all touch folder1/untracked &&
+	run_on_all touch deep/untracked-deep/bogus &&
+	run_on_all touch deep/untracked-deep/untracked &&
 
 	test_all_match git status --porcelain=v2 &&
 	test_all_match git clean -f &&
 	test_all_match git status --porcelain=v2 &&
 	test_sparse_match ls &&
 	test_sparse_match ls folder1 &&
+	run_on_all test_path_exists folder1/bogus &&
+	run_on_all test_path_is_missing folder1/untracked &&
+	run_on_all test_path_exists deep/untracked-deep/bogus &&
+	run_on_all test_path_exists deep/untracked-deep/untracked &&
+
+	test_all_match git clean -fd &&
+	test_all_match git status --porcelain=v2 &&
+	test_sparse_match ls &&
+	test_sparse_match ls folder1 &&
+	run_on_all test_path_exists folder1/bogus &&
+	run_on_all test_path_exists deep/untracked-deep/bogus &&
+	run_on_all test_path_is_missing deep/untracked-deep/untracked &&
 
 	test_all_match git clean -xf &&
 	test_all_match git status --porcelain=v2 &&
 	test_sparse_match ls &&
 	test_sparse_match ls folder1 &&
+	run_on_all test_path_is_missing folder1/bogus &&
+	run_on_all test_path_exists deep/untracked-deep/bogus &&
 
 	test_all_match git clean -xdf &&
 	test_all_match git status --porcelain=v2 &&
 	test_sparse_match ls &&
 	test_sparse_match ls folder1 &&
+	run_on_all test_path_is_missing deep/untracked-deep/bogus &&
 
 	test_sparse_match test_path_is_dir folder1
 '
@@ -920,6 +939,8 @@ test_expect_success 'sparse-index is not expanded' '
 	# Wildcard identifies only full sparse directories, no index expansion
 	ensure_not_expanded reset deepest -- folder\* &&
 
+	ensure_not_expanded clean -fd &&
+
 	ensure_not_expanded checkout -f update-deep &&
 	test_config -C sparse-index pull.twohead ort &&
 	(
-- 
gitgitgadget

[PATCH 4/9] checkout-index: expand sparse checkout compatibility tests

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Add tests to cover `checkout-index`, with a focus on cases interesting in a
sparse checkout (e.g., files specified outside sparse checkout definition).
New tests are intended to serve as a baseline for expected behavior and
performance when integrating `checkout-index` with the sparse index.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/perf/p2000-sparse-operations.sh        |  1 +
 t/t1092-sparse-checkout-compatibility.sh | 54 ++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index cb777c74a24..54f8602f3c1 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -117,5 +117,6 @@ test_perf_on_all git diff
 test_perf_on_all git diff --cached
 test_perf_on_all git blame $SPARSE_CONE/a
 test_perf_on_all git blame $SPARSE_CONE/f3/a
+test_perf_on_all git checkout-index -f --all
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 05587361452..db7ad41109b 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -755,6 +755,60 @@ test_expect_success 'cherry-pick with conflicts' '
 	test_all_match test_must_fail git cherry-pick to-cherry-pick
 '
 
+test_expect_success 'checkout-index inside sparse definition' '
+	init_repos &&
+
+	run_on_all rm -f deep/a &&
+	test_all_match git checkout-index -- deep/a &&
+	test_all_match git status --porcelain=v2 &&
+
+	echo test >>new-a &&
+	run_on_all cp ../new-a a &&
+	test_all_match test_must_fail git checkout-index -- a &&
+	test_all_match git checkout-index -f -- a &&
+	test_all_match git status --porcelain=v2
+'
+
+test_expect_success 'checkout-index outside sparse definition' '
+	init_repos &&
+
+	# File does not exist on disk yet for sparse checkouts, so checkout-index
+	# succeeds without -f
+	test_sparse_match git checkout-index -- folder1/a &&
+	test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
+	test_cmp sparse-checkout/folder1/a full-checkout/folder1/a &&
+
+	run_on_sparse rm -rf folder1 &&
+	echo test >new-a &&
+	run_on_sparse mkdir -p folder1 &&
+	run_on_all cp ../new-a folder1/a &&
+
+	test_all_match test_must_fail git checkout-index -- folder1/a &&
+	test_all_match git checkout-index -f -- folder1/a &&
+	test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
+	test_cmp sparse-checkout/folder1/a full-checkout/folder1/a
+'
+
+test_expect_success 'checkout-index with folders' '
+	init_repos &&
+
+	# Inside checkout definition
+	test_all_match test_must_fail git checkout-index -f -- deep/ &&
+
+	# Outside checkout definition
+	test_all_match test_must_fail git checkout-index -f -- folder1/
+'
+
+# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
+# files (even those outside the sparse definition) on disk. However, these files
+# don't appear in the percentage of tracked files in git status.
+test_expect_failure 'checkout-index --all' '
+	init_repos &&
+
+	test_all_match git checkout-index --all &&
+	test_sparse_match test_path_is_missing folder1
+'
+
 test_expect_success 'clean' '
 	init_repos &&
 
-- 
gitgitgadget

Re: [PATCH 4/9] checkout-index: expand sparse checkout compatibility tests

[Elijah Newren]

On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Victoria Dye <vdye@github.com>
>
> Add tests to cover `checkout-index`, with a focus on cases interesting in a
> sparse checkout (e.g., files specified outside sparse checkout definition).
> New tests are intended to serve as a baseline for expected behavior and
> performance when integrating `checkout-index` with the sparse index.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  t/perf/p2000-sparse-operations.sh        |  1 +
>  t/t1092-sparse-checkout-compatibility.sh | 54 ++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
>
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index cb777c74a24..54f8602f3c1 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -117,5 +117,6 @@ test_perf_on_all git diff
>  test_perf_on_all git diff --cached
>  test_perf_on_all git blame $SPARSE_CONE/a
>  test_perf_on_all git blame $SPARSE_CONE/f3/a
> +test_perf_on_all git checkout-index -f --all
>
>  test_done
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 05587361452..db7ad41109b 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -755,6 +755,60 @@ test_expect_success 'cherry-pick with conflicts' '
>         test_all_match test_must_fail git cherry-pick to-cherry-pick
>  '
>
> +test_expect_success 'checkout-index inside sparse definition' '
> +       init_repos &&
> +
> +       run_on_all rm -f deep/a &&
> +       test_all_match git checkout-index -- deep/a &&
> +       test_all_match git status --porcelain=v2 &&
> +
> +       echo test >>new-a &&
> +       run_on_all cp ../new-a a &&
> +       test_all_match test_must_fail git checkout-index -- a &&
> +       test_all_match git checkout-index -f -- a &&
> +       test_all_match git status --porcelain=v2
> +'
> +
> +test_expect_success 'checkout-index outside sparse definition' '
> +       init_repos &&
> +
> +       # File does not exist on disk yet for sparse checkouts, so checkout-index
> +       # succeeds without -f
> +       test_sparse_match git checkout-index -- folder1/a &&

Whoa, um, what is `git checkout-index -- folder1/a` even supposed to
mean with the sparse-index and cone mode?  That's an individual file,
so should folder1 and everything under it have the SKIP_WORKTREE bit
cleared?  Should folder1 just stop being a sparse directory, with all
siblings of folder1/a retaining the SKIP_WORKTREE bit (so folder1/0
could become a sparse directory) while folder1/a loses the
SKIP_WORKTREE bit, kind of violating cone mode but perhaps just until
the next sparse-checkout reapply?

The test sadly doesn't look at the SKIP_WORKTREE bits at all, which
feels like an important oversight (unless you meant to test this
later?).  Especially since my manual testing shows that this test
leaves the files as SKIP_WORKTREE despite writing the files to the
working tree.  I think the present-despite-skip-worktree problem I've
discussed elsewhere is growing in importance rather than shrinking;
you don't need to address that problem with this series but I raise it
mostly so we're aware to at least test it.  I'll keep reading to see
if you mention this or fix it up in subsequent patches.

> +       test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
> +       test_cmp sparse-checkout/folder1/a full-checkout/folder1/a &&
> +
> +       run_on_sparse rm -rf folder1 &&
> +       echo test >new-a &&
> +       run_on_sparse mkdir -p folder1 &&
> +       run_on_all cp ../new-a folder1/a &&
> +
> +       test_all_match test_must_fail git checkout-index -- folder1/a &&
> +       test_all_match git checkout-index -f -- folder1/a &&
> +       test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
> +       test_cmp sparse-checkout/folder1/a full-checkout/folder1/a
> +'
> +
> +test_expect_success 'checkout-index with folders' '
> +       init_repos &&
> +
> +       # Inside checkout definition
> +       test_all_match test_must_fail git checkout-index -f -- deep/ &&
> +
> +       # Outside checkout definition
> +       test_all_match test_must_fail git checkout-index -f -- folder1/

Passing a directory to checkout-index?  I'm glad they all immediately
fail, but is the plan just to make sure that this continues?  I'm not
sure what could make sense otherwise.  You've piqued my curiosity;
I'll keep reading the series to see.

> +'
> +
> +# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
> +# files (even those outside the sparse definition) on disk. However, these files
> +# don't appear in the percentage of tracked files in git status.

Ah, so you _are_ noticing the present-despite-SKIP_WORKTREE files.  I
think it might be nice to test for these a bit earlier, but it's nice
to see some test here for them.

I think that present-despite-SKIP_WORKTREE files is an erroneous
condition, one we should avoid triggering, and one we should help
users clean up from.  For checkout-index, the fact that it currently
makes more of these files is buggy.  It should either (1) clear the
SKIP_WORKTREE bit when it writes the files to the working tree, or (2)
avoid writing the files to the working tree.

And if we choose (1), there's already a --no-create option we could
piggy-back on to allow folks to not write the SKIP_WORKTREE files.

> +test_expect_failure 'checkout-index --all' '
> +       init_repos &&
> +
> +       test_all_match git checkout-index --all &&
> +       test_sparse_match test_path_is_missing folder1

Ah, it looks like you're choosing (2).  That may be fine, but an
interesting anecdote:

While attempting to adopt sparse checkouts at $DAYJOB (and
particularly using cone mode), we found the code structure just didn't
quite work.  We needed some directories to be ignored for sparse
checkouts to be meaningful at all, but we had some files that were
siblings to those directories that were needed for builds to function.
We came up with a hack to "add a few files back", using "git
checkout-index -- $FILENAME".  We expected that hack to write the
listed file(s) to the working tree -- though I think we also had logic
to then run the stuff we needed and then delete these temporary files.
We used this hack starting in Feb 2020, and eventually restructured
the code to not need this hack in Feb 2021.  I'll have to mull over
whether your choice of option (2) might cause us some problems if
someone (a) uses a new git version to (b) access an old version of our
code and (c) really wants to work with a sparse checkout (since the
"checkout-index" stuff was part of the build logic checked into the
code).  I think your change here is fine (because not using sparse
checkouts is an option, we told folks it was experimental, and those
are old versions that are only getting security fixes in special
circumstances), but let me think about it for a bit...

> +'
> +
>  test_expect_success 'clean' '
>         init_repos &&
>
> --
> gitgitgadget

Re: [PATCH 4/9] checkout-index: expand sparse checkout compatibility tests

[Elijah Newren]

On Wed, Jan 5, 2022 at 1:04 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Victoria Dye <vdye@github.com>
> >
...
> > +'
> > +
> > +# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
> > +# files (even those outside the sparse definition) on disk. However, these files
> > +# don't appear in the percentage of tracked files in git status.
>
> Ah, so you _are_ noticing the present-despite-SKIP_WORKTREE files.  I
> think it might be nice to test for these a bit earlier, but it's nice
> to see some test here for them.
>
> I think that present-despite-SKIP_WORKTREE files is an erroneous
> condition, one we should avoid triggering, and one we should help
> users clean up from.  For checkout-index, the fact that it currently
> makes more of these files is buggy.  It should either (1) clear the
> SKIP_WORKTREE bit when it writes the files to the working tree, or (2)
> avoid writing the files to the working tree.
>
> And if we choose (1), there's already a --no-create option we could
> piggy-back on to allow folks to not write the SKIP_WORKTREE files.
>
> > +test_expect_failure 'checkout-index --all' '
> > +       init_repos &&
> > +
> > +       test_all_match git checkout-index --all &&
> > +       test_sparse_match test_path_is_missing folder1
>
> Ah, it looks like you're choosing (2).  That may be fine, but an
> interesting anecdote:
>
> While attempting to adopt sparse checkouts at $DAYJOB (and
> particularly using cone mode), we found the code structure just didn't
> quite work.  We needed some directories to be ignored for sparse
> checkouts to be meaningful at all, but we had some files that were
> siblings to those directories that were needed for builds to function.
> We came up with a hack to "add a few files back", using "git
> checkout-index -- $FILENAME".  We expected that hack to write the
> listed file(s) to the working tree -- though I think we also had logic
> to then run the stuff we needed and then delete these temporary files.
> We used this hack starting in Feb 2020, and eventually restructured
> the code to not need this hack in Feb 2021.  I'll have to mull over
> whether your choice of option (2) might cause us some problems if
> someone (a) uses a new git version to (b) access an old version of our
> code and (c) really wants to work with a sparse checkout (since the
> "checkout-index" stuff was part of the build logic checked into the
> code).  I think your change here is fine (because not using sparse
> checkouts is an option, we told folks it was experimental, and those
> are old versions that are only getting security fixes in special
> circumstances), but let me think about it for a bit...

I brought this up a day or two ago with my co-workers and discussed
the combination of conditions needed to trigger any problems.  Their
response was "meh, it's pretty unlikely that anyone ever hits it, the
feature was labeled as experimental anyway, and there are multiple
easy workarounds for the user to choose from".  So, no need to worry
about this angle.

[PATCH 5/9] checkout-index: add --ignore-skip-worktree-bits option

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update `checkout-index --all` to no longer refresh files that have the
`skip-worktree` bit set. The newly-added `--ignore-skip-worktree-bits`
option, when used with `--all`, maintains the old behavior and checks out
all files regardless of `skip-worktree`.

The ability to toggle whether files should be checked-out based on
`skip-worktree` already exists in `git checkout` and `git restore` (both of
which have an `--ignore-skip-worktree-bits` option). Adding the option to
`checkout-index` (and changing the corresponding default behavior to respect
the `skip-worktree` bit) is especially helpful for sparse-checkout: it
prevents inadvertent creation of *all* files outside the sparse definition
on disk and eliminates the need to expand a sparse index by default when
using the `--all` option.

Internal usage of `checkout-index` in `git stash` and `git filter-branch` do
not make explicit use of files with `skip-worktree` enabled, so
`--ignore-skip-worktree-bits` is not added to them.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-checkout-index.txt     | 11 +++++++++--
 builtin/checkout-index.c                 | 12 ++++++++++--
 t/t1092-sparse-checkout-compatibility.sh | 10 +++++-----
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-checkout-index.txt b/Documentation/git-checkout-index.txt
index 4d33e7be0f5..2815f3d4b19 100644
--- a/Documentation/git-checkout-index.txt
+++ b/Documentation/git-checkout-index.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git checkout-index' [-u] [-q] [-a] [-f] [-n] [--prefix=<string>]
 		   [--stage=<number>|all]
 		   [--temp]
+		   [--ignore-skip-worktree-bits]
 		   [-z] [--stdin]
 		   [--] [<file>...]
 
@@ -37,8 +38,9 @@ OPTIONS
 
 -a::
 --all::
-	checks out all files in the index.  Cannot be used
-	together with explicit filenames.
+	checks out all files in the index except for those with the
+	skip-worktree bit set (see `--ignore-skip-worktree-bits`).
+	Cannot be used together with explicit filenames.
 
 -n::
 --no-create::
@@ -59,6 +61,11 @@ OPTIONS
 	write the content to temporary files.  The temporary name
 	associations will be written to stdout.
 
+--ignore-skip-worktree-bits::
+	Check out all files, including those with the skip-worktree bit
+	set. Note: may only be used with `--all`; skip-worktree is
+	ignored when explicit filenames are specified.
+
 --stdin::
 	Instead of taking list of paths from the command line,
 	read list of paths from the standard input.  Paths are
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index e21620d964e..2053a80103a 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -7,6 +7,7 @@
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "builtin.h"
 #include "config.h"
+#include "dir.h"
 #include "lockfile.h"
 #include "quote.h"
 #include "cache-tree.h"
@@ -116,7 +117,7 @@ static int checkout_file(const char *name, const char *prefix)
 	return -1;
 }
 
-static int checkout_all(const char *prefix, int prefix_length)
+static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_worktree)
 {
 	int i, errs = 0;
 	struct cache_entry *last_ce = NULL;
@@ -125,6 +126,8 @@ static int checkout_all(const char *prefix, int prefix_length)
 	ensure_full_index(&the_index);
 	for (i = 0; i < active_nr ; i++) {
 		struct cache_entry *ce = active_cache[i];
+		if (!ignore_skip_worktree && ce_skip_worktree(ce))
+			continue;
 		if (ce_stage(ce) != checkout_stage
 		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
 			continue;
@@ -176,6 +179,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	int i;
 	struct lock_file lock_file = LOCK_INIT;
 	int all = 0;
+	int ignore_skip_worktree = 0;
 	int read_from_stdin = 0;
 	int prefix_length;
 	int force = 0, quiet = 0, not_new = 0;
@@ -185,6 +189,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	struct option builtin_checkout_index_options[] = {
 		OPT_BOOL('a', "all", &all,
 			N_("check out all files in the index")),
+		OPT_BOOL(0, "ignore-skip-worktree-bits", &ignore_skip_worktree,
+			N_("do not skip files with skip-worktree set")),
 		OPT__FORCE(&force, N_("force overwrite of existing files"), 0),
 		OPT__QUIET(&quiet,
 			N_("no warning for existing files and files not in index")),
@@ -247,6 +253,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 
 		if (all)
 			die("git checkout-index: don't mix '--all' and explicit filenames");
+		if (ignore_skip_worktree)
+			die("git checkout-index: don't mix '--ignore-skip-worktree-bits' and explicit filenames");
 		if (read_from_stdin)
 			die("git checkout-index: don't mix '--stdin' and explicit filenames");
 		p = prefix_path(prefix, prefix_length, arg);
@@ -280,7 +288,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (all)
-		err |= checkout_all(prefix, prefix_length);
+		err |= checkout_all(prefix, prefix_length, ignore_skip_worktree);
 
 	if (pc_workers > 1)
 		err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index db7ad41109b..fad61d96107 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -799,14 +799,14 @@ test_expect_success 'checkout-index with folders' '
 	test_all_match test_must_fail git checkout-index -f -- folder1/
 '
 
-# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
-# files (even those outside the sparse definition) on disk. However, these files
-# don't appear in the percentage of tracked files in git status.
-test_expect_failure 'checkout-index --all' '
+test_expect_success 'checkout-index --all' '
 	init_repos &&
 
 	test_all_match git checkout-index --all &&
-	test_sparse_match test_path_is_missing folder1
+	test_sparse_match test_path_is_missing folder1 &&
+
+	test_all_match git checkout-index --ignore-skip-worktree-bits --all &&
+	test_all_match test_path_exists folder1
 '
 
 test_expect_success 'clean' '
-- 
gitgitgadget

Re: [PATCH 5/9] checkout-index: add --ignore-skip-worktree-bits option

[Elijah Newren]

On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Victoria Dye <vdye@github.com>
>
> Update `checkout-index --all` to no longer refresh files that have the
> `skip-worktree` bit set. The newly-added `--ignore-skip-worktree-bits`
> option, when used with `--all`, maintains the old behavior and checks out
> all files regardless of `skip-worktree`.
>
> The ability to toggle whether files should be checked-out based on
> `skip-worktree` already exists in `git checkout` and `git restore` (both of
> which have an `--ignore-skip-worktree-bits` option).

I learned something new.

And ick, what a name.  Why not --ignore-sparsity or something?  Oh well...

> Adding the option to
> `checkout-index` (and changing the corresponding default behavior to respect
> the `skip-worktree` bit) is especially helpful for sparse-checkout: it
> prevents inadvertent creation of *all* files outside the sparse definition
> on disk and eliminates the need to expand a sparse index by default when
> using the `--all` option.
>
> Internal usage of `checkout-index` in `git stash` and `git filter-branch` do
> not make explicit use of files with `skip-worktree` enabled, so
> `--ignore-skip-worktree-bits` is not added to them.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  Documentation/git-checkout-index.txt     | 11 +++++++++--
>  builtin/checkout-index.c                 | 12 ++++++++++--
>  t/t1092-sparse-checkout-compatibility.sh | 10 +++++-----
>  3 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-checkout-index.txt b/Documentation/git-checkout-index.txt
> index 4d33e7be0f5..2815f3d4b19 100644
> --- a/Documentation/git-checkout-index.txt
> +++ b/Documentation/git-checkout-index.txt
> @@ -12,6 +12,7 @@ SYNOPSIS
>  'git checkout-index' [-u] [-q] [-a] [-f] [-n] [--prefix=<string>]
>                    [--stage=<number>|all]
>                    [--temp]
> +                  [--ignore-skip-worktree-bits]
>                    [-z] [--stdin]
>                    [--] [<file>...]
>
> @@ -37,8 +38,9 @@ OPTIONS
>
>  -a::
>  --all::
> -       checks out all files in the index.  Cannot be used
> -       together with explicit filenames.
> +       checks out all files in the index except for those with the
> +       skip-worktree bit set (see `--ignore-skip-worktree-bits`).
> +       Cannot be used together with explicit filenames.
>
>  -n::
>  --no-create::
> @@ -59,6 +61,11 @@ OPTIONS
>         write the content to temporary files.  The temporary name
>         associations will be written to stdout.
>
> +--ignore-skip-worktree-bits::
> +       Check out all files, including those with the skip-worktree bit
> +       set. Note: may only be used with `--all`; skip-worktree is
> +       ignored when explicit filenames are specified.

Why this restriction?  What if the user ran
   git checkout-index -- '*.c'
That's not an explicit filename, but a glob.

> +
>  --stdin::
>         Instead of taking list of paths from the command line,
>         read list of paths from the standard input.  Paths are
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index e21620d964e..2053a80103a 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -7,6 +7,7 @@
>  #define USE_THE_INDEX_COMPATIBILITY_MACROS
>  #include "builtin.h"
>  #include "config.h"
> +#include "dir.h"
>  #include "lockfile.h"
>  #include "quote.h"
>  #include "cache-tree.h"
> @@ -116,7 +117,7 @@ static int checkout_file(const char *name, const char *prefix)
>         return -1;
>  }
>
> -static int checkout_all(const char *prefix, int prefix_length)
> +static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_worktree)
>  {
>         int i, errs = 0;
>         struct cache_entry *last_ce = NULL;
> @@ -125,6 +126,8 @@ static int checkout_all(const char *prefix, int prefix_length)
>         ensure_full_index(&the_index);
>         for (i = 0; i < active_nr ; i++) {
>                 struct cache_entry *ce = active_cache[i];
> +               if (!ignore_skip_worktree && ce_skip_worktree(ce))
> +                       continue;

So here I see you let it fall through to the code below that will
write the file to the working tree...but it doesn't clear the
SKIP_WORKTREE bit in the index when it does so, which I think is a
bug.

>                 if (ce_stage(ce) != checkout_stage
>                     && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
>                         continue;
> @@ -176,6 +179,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>         int i;
>         struct lock_file lock_file = LOCK_INIT;
>         int all = 0;
> +       int ignore_skip_worktree = 0;
>         int read_from_stdin = 0;
>         int prefix_length;
>         int force = 0, quiet = 0, not_new = 0;
> @@ -185,6 +189,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>         struct option builtin_checkout_index_options[] = {
>                 OPT_BOOL('a', "all", &all,
>                         N_("check out all files in the index")),
> +               OPT_BOOL(0, "ignore-skip-worktree-bits", &ignore_skip_worktree,
> +                       N_("do not skip files with skip-worktree set")),
>                 OPT__FORCE(&force, N_("force overwrite of existing files"), 0),
>                 OPT__QUIET(&quiet,
>                         N_("no warning for existing files and files not in index")),
> @@ -247,6 +253,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>
>                 if (all)
>                         die("git checkout-index: don't mix '--all' and explicit filenames");
> +               if (ignore_skip_worktree)
> +                       die("git checkout-index: don't mix '--ignore-skip-worktree-bits' and explicit filenames");
>                 if (read_from_stdin)
>                         die("git checkout-index: don't mix '--stdin' and explicit filenames");
>                 p = prefix_path(prefix, prefix_length, arg);
> @@ -280,7 +288,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>         }
>
>         if (all)
> -               err |= checkout_all(prefix, prefix_length);
> +               err |= checkout_all(prefix, prefix_length, ignore_skip_worktree);
>
>         if (pc_workers > 1)
>                 err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index db7ad41109b..fad61d96107 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -799,14 +799,14 @@ test_expect_success 'checkout-index with folders' '
>         test_all_match test_must_fail git checkout-index -f -- folder1/
>  '
>
> -# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
> -# files (even those outside the sparse definition) on disk. However, these files
> -# don't appear in the percentage of tracked files in git status.
> -test_expect_failure 'checkout-index --all' '
> +test_expect_success 'checkout-index --all' '
>         init_repos &&
>
>         test_all_match git checkout-index --all &&
> -       test_sparse_match test_path_is_missing folder1
> +       test_sparse_match test_path_is_missing folder1 &&
> +
> +       test_all_match git checkout-index --ignore-skip-worktree-bits --all &&
> +       test_all_match test_path_exists folder1

I added an 'exit 1' here, ran the test and then checked:

$ cd trash\ directory.t1092-sparse-checkout-compatibility/sparse-checkout/
$ git ls-files -t | grep folder1/
S folder1/0/0/0
S folder1/0/1
S folder1/a

So there's some more work to do on this patch.

Re: [PATCH 5/9] checkout-index: add --ignore-skip-worktree-bits option

[Victoria Dye]

Elijah Newren wrote:
> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Victoria Dye <vdye@github.com>
>>
>> Update `checkout-index --all` to no longer refresh files that have the
>> `skip-worktree` bit set. The newly-added `--ignore-skip-worktree-bits`
>> option, when used with `--all`, maintains the old behavior and checks out
>> all files regardless of `skip-worktree`.
>>
>> The ability to toggle whether files should be checked-out based on
>> `skip-worktree` already exists in `git checkout` and `git restore` (both of
>> which have an `--ignore-skip-worktree-bits` option).
> 
> I learned something new.
> 
> And ick, what a name.  Why not --ignore-sparsity or something?  Oh well...
> 
>> Adding the option to
>> `checkout-index` (and changing the corresponding default behavior to respect
>> the `skip-worktree` bit) is especially helpful for sparse-checkout: it
>> prevents inadvertent creation of *all* files outside the sparse definition
>> on disk and eliminates the need to expand a sparse index by default when
>> using the `--all` option.
>>
>> Internal usage of `checkout-index` in `git stash` and `git filter-branch` do
>> not make explicit use of files with `skip-worktree` enabled, so
>> `--ignore-skip-worktree-bits` is not added to them.
>>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  Documentation/git-checkout-index.txt     | 11 +++++++++--
>>  builtin/checkout-index.c                 | 12 ++++++++++--
>>  t/t1092-sparse-checkout-compatibility.sh | 10 +++++-----
>>  3 files changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/git-checkout-index.txt b/Documentation/git-checkout-index.txt
>> index 4d33e7be0f5..2815f3d4b19 100644
>> --- a/Documentation/git-checkout-index.txt
>> +++ b/Documentation/git-checkout-index.txt
>> @@ -12,6 +12,7 @@ SYNOPSIS
>>  'git checkout-index' [-u] [-q] [-a] [-f] [-n] [--prefix=<string>]
>>                    [--stage=<number>|all]
>>                    [--temp]
>> +                  [--ignore-skip-worktree-bits]
>>                    [-z] [--stdin]
>>                    [--] [<file>...]
>>
>> @@ -37,8 +38,9 @@ OPTIONS
>>
>>  -a::
>>  --all::
>> -       checks out all files in the index.  Cannot be used
>> -       together with explicit filenames.
>> +       checks out all files in the index except for those with the
>> +       skip-worktree bit set (see `--ignore-skip-worktree-bits`).
>> +       Cannot be used together with explicit filenames.
>>
>>  -n::
>>  --no-create::
>> @@ -59,6 +61,11 @@ OPTIONS
>>         write the content to temporary files.  The temporary name
>>         associations will be written to stdout.
>>
>> +--ignore-skip-worktree-bits::
>> +       Check out all files, including those with the skip-worktree bit
>> +       set. Note: may only be used with `--all`; skip-worktree is
>> +       ignored when explicit filenames are specified.
> 
> Why this restriction?  What if the user ran
>    git checkout-index -- '*.c'
> That's not an explicit filename, but a glob.
> 

`checkout-index` doesn't accept globs/pathspecs, so every provided argument
must correspond exactly to an entry in the index. 

I originally restricted '--ignore-skip-worktree-bits' to only work with
'--all' because I wanted changes to the current behavior of `checkout-index`
in a sparse checkout to be as minimal as possible. However, if this is more
"unexpected inconsistency" than "I'm glad checkout-index with filenames
still works the way it did before", I'm happy to change it. 

>> +
>>  --stdin::
>>         Instead of taking list of paths from the command line,
>>         read list of paths from the standard input.  Paths are
>> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
>> index e21620d964e..2053a80103a 100644
>> --- a/builtin/checkout-index.c
>> +++ b/builtin/checkout-index.c
>> @@ -7,6 +7,7 @@
>>  #define USE_THE_INDEX_COMPATIBILITY_MACROS
>>  #include "builtin.h"
>>  #include "config.h"
>> +#include "dir.h"
>>  #include "lockfile.h"
>>  #include "quote.h"
>>  #include "cache-tree.h"
>> @@ -116,7 +117,7 @@ static int checkout_file(const char *name, const char *prefix)
>>         return -1;
>>  }
>>
>> -static int checkout_all(const char *prefix, int prefix_length)
>> +static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_worktree)
>>  {
>>         int i, errs = 0;
>>         struct cache_entry *last_ce = NULL;
>> @@ -125,6 +126,8 @@ static int checkout_all(const char *prefix, int prefix_length)
>>         ensure_full_index(&the_index);
>>         for (i = 0; i < active_nr ; i++) {
>>                 struct cache_entry *ce = active_cache[i];
>> +               if (!ignore_skip_worktree && ce_skip_worktree(ce))
>> +                       continue;
> 
> So here I see you let it fall through to the code below that will
> write the file to the working tree...but it doesn't clear the
> SKIP_WORKTREE bit in the index when it does so, which I think is a
> bug.
> 

I disagree, mainly because updating a flag seems inconsistent with how
`checkout-index` otherwise works. Specifically, `checkout-index` creates or
replaces a file on disk (not even necessarily in the git working directory)
based on the file's state in the index, but doesn't modify the index in the
process. The only exception is '-u', which is effectively a shortcut for
running `git update-index --refresh` [1]. 

If a user wants to to checkout a file *and* update `skip-worktree`, I think
explicitly using `update-index` would be a more appropriate way to do that
(similar to the example [2] in the `checkout-index` documentation):

        $ git checkout-index outside-cone/file
        $ git update-index --no-skip-worktree outside-cone/file

[1] https://lore.kernel.org/git/7vis1kvqac.fsf@assigned-by-dhcp.cox.net/
[2] https://git-scm.com/docs/git-checkout-index#Documentation/git-checkout-index.txt-Toupdateandrefreshonlythefilesalreadycheckedout

>>                 if (ce_stage(ce) != checkout_stage
>>                     && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
>>                         continue;
>> @@ -176,6 +179,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>>         int i;
>>         struct lock_file lock_file = LOCK_INIT;
>>         int all = 0;
>> +       int ignore_skip_worktree = 0;
>>         int read_from_stdin = 0;
>>         int prefix_length;
>>         int force = 0, quiet = 0, not_new = 0;
>> @@ -185,6 +189,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>>         struct option builtin_checkout_index_options[] = {
>>                 OPT_BOOL('a', "all", &all,
>>                         N_("check out all files in the index")),
>> +               OPT_BOOL(0, "ignore-skip-worktree-bits", &ignore_skip_worktree,
>> +                       N_("do not skip files with skip-worktree set")),
>>                 OPT__FORCE(&force, N_("force overwrite of existing files"), 0),
>>                 OPT__QUIET(&quiet,
>>                         N_("no warning for existing files and files not in index")),
>> @@ -247,6 +253,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>>
>>                 if (all)
>>                         die("git checkout-index: don't mix '--all' and explicit filenames");
>> +               if (ignore_skip_worktree)
>> +                       die("git checkout-index: don't mix '--ignore-skip-worktree-bits' and explicit filenames");
>>                 if (read_from_stdin)
>>                         die("git checkout-index: don't mix '--stdin' and explicit filenames");
>>                 p = prefix_path(prefix, prefix_length, arg);
>> @@ -280,7 +288,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>>         }
>>
>>         if (all)
>> -               err |= checkout_all(prefix, prefix_length);
>> +               err |= checkout_all(prefix, prefix_length, ignore_skip_worktree);
>>
>>         if (pc_workers > 1)
>>                 err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>> index db7ad41109b..fad61d96107 100755
>> --- a/t/t1092-sparse-checkout-compatibility.sh
>> +++ b/t/t1092-sparse-checkout-compatibility.sh
>> @@ -799,14 +799,14 @@ test_expect_success 'checkout-index with folders' '
>>         test_all_match test_must_fail git checkout-index -f -- folder1/
>>  '
>>
>> -# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
>> -# files (even those outside the sparse definition) on disk. However, these files
>> -# don't appear in the percentage of tracked files in git status.
>> -test_expect_failure 'checkout-index --all' '
>> +test_expect_success 'checkout-index --all' '
>>         init_repos &&
>>
>>         test_all_match git checkout-index --all &&
>> -       test_sparse_match test_path_is_missing folder1
>> +       test_sparse_match test_path_is_missing folder1 &&
>> +
>> +       test_all_match git checkout-index --ignore-skip-worktree-bits --all &&
>> +       test_all_match test_path_exists folder1
> 
> I added an 'exit 1' here, ran the test and then checked:
> 
> $ cd trash\ directory.t1092-sparse-checkout-compatibility/sparse-checkout/
> $ git ls-files -t | grep folder1/
> S folder1/0/0/0
> S folder1/0/1
> S folder1/a
> 
> So there's some more work to do on this patch.

Unless I'm misreading your comment, this is exactly the behavior I would
expect in this test: all files (even those with `skip-worktree` set, per
'--ignore-skip-worktree-bits') are created on-disk, with `skip-worktree`
unmodified.

Re: [PATCH 5/9] checkout-index: add --ignore-skip-worktree-bits option

[Elijah Newren]

On Thu, Jan 6, 2022 at 7:07 AM Victoria Dye <vdye@github.com> wrote:
>
> Elijah Newren wrote:
> > On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Victoria Dye <vdye@github.com>
> >>
...
> >> +--ignore-skip-worktree-bits::
> >> +       Check out all files, including those with the skip-worktree bit
> >> +       set. Note: may only be used with `--all`; skip-worktree is
> >> +       ignored when explicit filenames are specified.
> >
> > Why this restriction?  What if the user ran
> >    git checkout-index -- '*.c'
> > That's not an explicit filename, but a glob.
> >
>
> `checkout-index` doesn't accept globs/pathspecs, so every provided argument
> must correspond exactly to an entry in the index.

Ah, my mistake; thanks for pointing that out.  I learned something else new.

> I originally restricted '--ignore-skip-worktree-bits' to only work with
> '--all' because I wanted changes to the current behavior of `checkout-index`
> in a sparse checkout to be as minimal as possible. However, if this is more
> "unexpected inconsistency" than "I'm glad checkout-index with filenames
> still works the way it did before", I'm happy to change it.

That's good context.  I'm not sure I have a real strong opinion on
this side of things, but digging around I did note that this behavior
is somewhat inconsistent with checkout/restore:

$ git checkout HEAD sparse-dir/filename
error: pathspec 'sparse-dir/filename' did not match any file(s) known to git

$ git ls-files -t sparse-dir/filename
S sparse-dir/filename

(The error message in checkout/restore might be worth fixing up, since
it's actually a lie as shown by the `ls-files -t` command below it.
But that's obviously outside the scope of this series.)

So we should probably either require the --ignore-skip-worktree-bits
flag even with individual paths (which might also slightly reduce my
objections to the other items below), or explain in the commit message
why checkout and checkout-index treat paths outside the sparsity
specification differently.

> >> +
> >>  --stdin::
> >>         Instead of taking list of paths from the command line,
> >>         read list of paths from the standard input.  Paths are
> >> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> >> index e21620d964e..2053a80103a 100644
> >> --- a/builtin/checkout-index.c
> >> +++ b/builtin/checkout-index.c
> >> @@ -7,6 +7,7 @@
> >>  #define USE_THE_INDEX_COMPATIBILITY_MACROS
> >>  #include "builtin.h"
> >>  #include "config.h"
> >> +#include "dir.h"
> >>  #include "lockfile.h"
> >>  #include "quote.h"
> >>  #include "cache-tree.h"
> >> @@ -116,7 +117,7 @@ static int checkout_file(const char *name, const char *prefix)
> >>         return -1;
> >>  }
> >>
> >> -static int checkout_all(const char *prefix, int prefix_length)
> >> +static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_worktree)
> >>  {
> >>         int i, errs = 0;
> >>         struct cache_entry *last_ce = NULL;
> >> @@ -125,6 +126,8 @@ static int checkout_all(const char *prefix, int prefix_length)
> >>         ensure_full_index(&the_index);
> >>         for (i = 0; i < active_nr ; i++) {
> >>                 struct cache_entry *ce = active_cache[i];
> >> +               if (!ignore_skip_worktree && ce_skip_worktree(ce))
> >> +                       continue;
> >
> > So here I see you let it fall through to the code below that will
> > write the file to the working tree...but it doesn't clear the
> > SKIP_WORKTREE bit in the index when it does so, which I think is a
> > bug.
> >
>
> I disagree, mainly because updating a flag seems inconsistent with how
> `checkout-index` otherwise works. Specifically, `checkout-index` creates or
> replaces a file on disk (not even necessarily in the git working directory)
> based on the file's state in the index, but doesn't modify the index in the
> process. The only exception is '-u', which is effectively a shortcut for
> running `git update-index --refresh` [1].
>
> If a user wants to to checkout a file *and* update `skip-worktree`, I think
> explicitly using `update-index` would be a more appropriate way to do that
> (similar to the example [2] in the `checkout-index` documentation):
>
>         $ git checkout-index outside-cone/file
>         $ git update-index --no-skip-worktree outside-cone/file

I understand the desire to make a minimal change and only do
operations the command previously did, but I'm also worried about
introducing or exacerbating inconsistencies for users.  Matheus'
patches to grep stalled for nearly a year, in part because of
complications of how to handle sparse-checkouts appropriately in all
cases[1][2] (with trying to sanely figure out how to sanely handle
present-despite-SKIP_WORKTREE files being one of the complications).
His rm/add follow-ups also took months because of those kinds of
issues[3].  We've had to add ugly logic to merge-ort to attempt to
handle present-despite-SKIP_WORKTREE files[4], and basically just been
forced to give up in merge-recursive knowing full well that we'll
sometimes silently discard user modifications. Despite stash
essentially being a merge, it needed extra code (beyond what was in
merge-ort and merge-recursive) to manually tweak SKIP_WORKTREE bits in
order to avoid a few different bugs that'd result in an early abort
with a partial stash application[5].  But beyond implementation
complexity for other commands to deal with these problematic files, it
will also cause problems for users:

Since present-despite-SKIP_WORKTREE files are not reported by status,
diff, grep, or anything, and `git sparse-checkout reapply` will NOT
attempt to remove or notify the user about them, and commands like
switch/checkout/pull/merge/rebase etc. will not update them (unless
there are conflicts in that path), the contents of these files will
remain stagnant and likely reflect a version of the file from weeks
ago on a different branch altogether.  When users then either change
their sparsity patterns or disable sparse-checkouts, now they see
files with "all kinds of changes" (because it was the version of a
file from a different branch or commit altogether).  What are users
supposed to do with that?  They have to do something, or else
switch/checkout/pull will now start erroring out, and the issue that
cause the problem for the users is totally disconnected from the
command that eventually gives them errors, potentially by days or
weeks.  It's nearly impossible to help users debug why this happened
with such a disconnect.

However...I've been complaining about these kinds of problems for well
over a year, because the only solution I knew of was to try to
minimize each way we could get into this broken state (of having
present-despite-SKIP_WORKTREE files) and ask folks to test and modify
commands to do something semi-sane when we unfortunately find
ourselves in this broken state anyway.  I've gone round and round with
Matheus trying to figure them out.  And back and forth many times with
Stolee.  And now...it feels like there'll be no end.  So, I decided to
bite the bullet and attempt to see just how hard it would be to
actually detect/notify/correct this broken state for users.  I think I
may have a clever solution.  As long as others don't feel it's too
heavy handed, it could just obviate all these discussions and all the
special case code and simply the tests significantly and drop my
objections to your patch.  I'll submit the series later today.

[1] See https://lore.kernel.org/git/5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br/#t
and the dates on the thread; also Matheus and I had several
conversations off-list trying to resolve the issues over that time
[2] ...it finally kind of got unstuck after
https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/
[3] See e.g. https://lore.kernel.org/git/CABPp-BHwNoVnooqDFPAsZxBT9aR5Dwk5D9sDRCvYSb8akxAJgA@mail.gmail.com/#t
and quotes like "The core functionality of sparse-checkout has always
been only partially implemented", a statement I still believe is true
today.
[4] See commit 66b209b86a ("merge-ort: implement CE_SKIP_WORKTREE
handling with conflicted entries", 2021-03-20)
[5] See commit ba359fd507 ("stash: fix stash application in
sparse-checkouts", 2020-12-01)

> [1] https://lore.kernel.org/git/7vis1kvqac.fsf@assigned-by-dhcp.cox.net/
> [2] https://git-scm.com/docs/git-checkout-index#Documentation/git-checkout-index.txt-Toupdateandrefreshonlythefilesalreadycheckedout
>
> >>                 if (ce_stage(ce) != checkout_stage
> >>                     && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
> >>                         continue;
> >> @@ -176,6 +179,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> >>         int i;
> >>         struct lock_file lock_file = LOCK_INIT;
> >>         int all = 0;
> >> +       int ignore_skip_worktree = 0;
> >>         int read_from_stdin = 0;
> >>         int prefix_length;
> >>         int force = 0, quiet = 0, not_new = 0;
> >> @@ -185,6 +189,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> >>         struct option builtin_checkout_index_options[] = {
> >>                 OPT_BOOL('a', "all", &all,
> >>                         N_("check out all files in the index")),
> >> +               OPT_BOOL(0, "ignore-skip-worktree-bits", &ignore_skip_worktree,
> >> +                       N_("do not skip files with skip-worktree set")),
> >>                 OPT__FORCE(&force, N_("force overwrite of existing files"), 0),
> >>                 OPT__QUIET(&quiet,
> >>                         N_("no warning for existing files and files not in index")),
> >> @@ -247,6 +253,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> >>
> >>                 if (all)
> >>                         die("git checkout-index: don't mix '--all' and explicit filenames");
> >> +               if (ignore_skip_worktree)
> >> +                       die("git checkout-index: don't mix '--ignore-skip-worktree-bits' and explicit filenames");
> >>                 if (read_from_stdin)
> >>                         die("git checkout-index: don't mix '--stdin' and explicit filenames");
> >>                 p = prefix_path(prefix, prefix_length, arg);
> >> @@ -280,7 +288,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> >>         }
> >>
> >>         if (all)
> >> -               err |= checkout_all(prefix, prefix_length);
> >> +               err |= checkout_all(prefix, prefix_length, ignore_skip_worktree);
> >>
> >>         if (pc_workers > 1)
> >>                 err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
> >> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> >> index db7ad41109b..fad61d96107 100755
> >> --- a/t/t1092-sparse-checkout-compatibility.sh
> >> +++ b/t/t1092-sparse-checkout-compatibility.sh
> >> @@ -799,14 +799,14 @@ test_expect_success 'checkout-index with folders' '
> >>         test_all_match test_must_fail git checkout-index -f -- folder1/
> >>  '
> >>
> >> -# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
> >> -# files (even those outside the sparse definition) on disk. However, these files
> >> -# don't appear in the percentage of tracked files in git status.
> >> -test_expect_failure 'checkout-index --all' '
> >> +test_expect_success 'checkout-index --all' '
> >>         init_repos &&
> >>
> >>         test_all_match git checkout-index --all &&
> >> -       test_sparse_match test_path_is_missing folder1
> >> +       test_sparse_match test_path_is_missing folder1 &&
> >> +
> >> +       test_all_match git checkout-index --ignore-skip-worktree-bits --all &&
> >> +       test_all_match test_path_exists folder1
> >
> > I added an 'exit 1' here, ran the test and then checked:
> >
> > $ cd trash\ directory.t1092-sparse-checkout-compatibility/sparse-checkout/
> > $ git ls-files -t | grep folder1/
> > S folder1/0/0/0
> > S folder1/0/1
> > S folder1/a
> >
> > So there's some more work to do on this patch.
>
> Unless I'm misreading your comment, this is exactly the behavior I would
> expect in this test: all files (even those with `skip-worktree` set, per
> '--ignore-skip-worktree-bits') are created on-disk, with `skip-worktree`
> unmodified.

If folks agree with the series I'll submit later today to detect/fix
present-despite-SKIP_WORKTREE files, then I'll drop my objection to
this behavior.  But if folks object to that other series, then I am
concerned this behavior gives users a ticking time-bomb without
sufficient warning.  So if that other series isn't acceptable to
folks, then I may come back here and argue that either you or I need
to go back in to checkout-index and make it clear the SKIP_WORKTREE
bit for any files it writes.

[PATCH 6/9] checkout-index: integrate with sparse index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Add repository settings to allow usage of the sparse index.

When using the `--all` option, sparse directories are ignored by default due
to the `skip-worktree` flag, so there is no need to expand the index. If
`--ignore-skip-worktree-bits` is specified, the index is expanded in order
to check out all files.

When checking out individual files, existing behavior in a full index is to
exit with an error if a directory is specified (as the directory name will
not match an index entry). However, it is possible in a sparse index to
match a directory name to a sparse directory index entry, but checking out
that sparse directory still results in an error on checkout. To reduce some
potential confusion for users, `checkout_file(...)` explicitly exits with an
informative error if provided with a sparse directory name. The test
corresponding to this scenario verifies the error message, which now differs
between sparse index and non-sparse index checkouts.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/checkout-index.c                 | 28 ++++++++++++++++++++++--
 t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++-
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 2053a80103a..9c5657ccf22 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -66,6 +66,7 @@ static int checkout_file(const char *name, const char *prefix)
 	int namelen = strlen(name);
 	int pos = cache_name_pos(name, namelen);
 	int has_same_name = 0;
+	int is_file = 0;
 	int did_checkout = 0;
 	int errs = 0;
 
@@ -79,6 +80,9 @@ static int checkout_file(const char *name, const char *prefix)
 			break;
 		has_same_name = 1;
 		pos++;
+		if (S_ISSPARSEDIR(ce->ce_mode))
+			break;
+		is_file = 1;
 		if (ce_stage(ce) != checkout_stage
 		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
 			continue;
@@ -107,6 +111,8 @@ static int checkout_file(const char *name, const char *prefix)
 		fprintf(stderr, "git checkout-index: %s ", name);
 		if (!has_same_name)
 			fprintf(stderr, "is not in the cache");
+		else if (!is_file)
+			fprintf(stderr, "is a sparse directory");
 		else if (checkout_stage)
 			fprintf(stderr, "does not exist at stage %d",
 				checkout_stage);
@@ -122,10 +128,25 @@ static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_w
 	int i, errs = 0;
 	struct cache_entry *last_ce = NULL;
 
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(&the_index);
 	for (i = 0; i < active_nr ; i++) {
 		struct cache_entry *ce = active_cache[i];
+
+		if (S_ISSPARSEDIR(ce->ce_mode)) {
+			if (!ce_skip_worktree(ce))
+				BUG("sparse directory '%s' does not have skip-worktree set", ce->name);
+
+			/*
+			 * If the current entry is a sparse directory and skip-worktree
+			 * entries are being checked out, expand the index and continue
+			 * the loop on the current index position (now pointing to the
+			 * first entry inside the expanded sparse directory).
+			 */
+			if (ignore_skip_worktree) {
+				ensure_full_index(&the_index);
+				ce = active_cache[i];
+			}
+		}
+
 		if (!ignore_skip_worktree && ce_skip_worktree(ce))
 			continue;
 		if (ce_stage(ce) != checkout_stage
@@ -218,6 +239,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	prefix_length = prefix ? strlen(prefix) : 0;
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	if (read_cache() < 0) {
 		die("invalid cache");
 	}
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index fad61d96107..6ecf1f2bf8e 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -796,7 +796,14 @@ test_expect_success 'checkout-index with folders' '
 	test_all_match test_must_fail git checkout-index -f -- deep/ &&
 
 	# Outside checkout definition
-	test_all_match test_must_fail git checkout-index -f -- folder1/
+	# Note: although all tests fail (as expected), the messaging differs. For
+	# non-sparse index checkouts, the error is that the "file" does not appear
+	# in the index; for sparse checkouts, the error is explicitly that the
+	# entry is a sparse directory.
+	run_on_all test_must_fail git checkout-index -f -- folder1/ &&
+	test_cmp full-checkout-err sparse-checkout-err &&
+	! test_cmp full-checkout-err sparse-index-err &&
+	grep "is a sparse directory" sparse-index-err
 '
 
 test_expect_success 'checkout-index --all' '
@@ -965,6 +972,8 @@ test_expect_success 'sparse-index is not expanded' '
 	echo >>sparse-index/untracked.txt &&
 	ensure_not_expanded add . &&
 
+	ensure_not_expanded checkout-index -f a &&
+	ensure_not_expanded checkout-index -f --all &&
 	for ref in update-deep update-folder1 update-folder2 update-deep
 	do
 		echo >>sparse-index/README.md &&
-- 
gitgitgadget

Re: [PATCH 6/9] checkout-index: integrate with sparse index

[Elijah Newren]

On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Victoria Dye <vdye@github.com>
>
> Add repository settings to allow usage of the sparse index.
>
> When using the `--all` option, sparse directories are ignored by default due
> to the `skip-worktree` flag, so there is no need to expand the index. If
> `--ignore-skip-worktree-bits` is specified, the index is expanded in order
> to check out all files.
>
> When checking out individual files, existing behavior in a full index is to
> exit with an error if a directory is specified (as the directory name will
> not match an index entry). However, it is possible in a sparse index to
> match a directory name to a sparse directory index entry, but checking out
> that sparse directory still results in an error on checkout. To reduce some
> potential confusion for users, `checkout_file(...)` explicitly exits with an
> informative error if provided with a sparse directory name. The test
> corresponding to this scenario verifies the error message, which now differs
> between sparse index and non-sparse index checkouts.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/checkout-index.c                 | 28 ++++++++++++++++++++++--
>  t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++-
>  2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 2053a80103a..9c5657ccf22 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -66,6 +66,7 @@ static int checkout_file(const char *name, const char *prefix)
>         int namelen = strlen(name);
>         int pos = cache_name_pos(name, namelen);
>         int has_same_name = 0;
> +       int is_file = 0;
>         int did_checkout = 0;
>         int errs = 0;
>
> @@ -79,6 +80,9 @@ static int checkout_file(const char *name, const char *prefix)
>                         break;
>                 has_same_name = 1;
>                 pos++;
> +               if (S_ISSPARSEDIR(ce->ce_mode))
> +                       break;
> +               is_file = 1;
>                 if (ce_stage(ce) != checkout_stage
>                     && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
>                         continue;
> @@ -107,6 +111,8 @@ static int checkout_file(const char *name, const char *prefix)
>                 fprintf(stderr, "git checkout-index: %s ", name);
>                 if (!has_same_name)
>                         fprintf(stderr, "is not in the cache");
> +               else if (!is_file)
> +                       fprintf(stderr, "is a sparse directory");
>                 else if (checkout_stage)
>                         fprintf(stderr, "does not exist at stage %d",
>                                 checkout_stage);
> @@ -122,10 +128,25 @@ static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_w
>         int i, errs = 0;
>         struct cache_entry *last_ce = NULL;
>
> -       /* TODO: audit for interaction with sparse-index. */
> -       ensure_full_index(&the_index);
>         for (i = 0; i < active_nr ; i++) {
>                 struct cache_entry *ce = active_cache[i];
> +
> +               if (S_ISSPARSEDIR(ce->ce_mode)) {
> +                       if (!ce_skip_worktree(ce))
> +                               BUG("sparse directory '%s' does not have skip-worktree set", ce->name);
> +
> +                       /*
> +                        * If the current entry is a sparse directory and skip-worktree
> +                        * entries are being checked out, expand the index and continue
> +                        * the loop on the current index position (now pointing to the
> +                        * first entry inside the expanded sparse directory).
> +                        */
> +                       if (ignore_skip_worktree) {
> +                               ensure_full_index(&the_index);
> +                               ce = active_cache[i];
> +                       }

So while iterating through the index, we reach an entry and decide to
expand the index.  This would be unsafe if our iterator became
invalid, but the only way that would happen is if there was a sparse
directory entry earlier in the index, and by construction of this loop
we expand upon the first sparse directory entry we see.  Since you
reassign  ce = active_cache[i] immediately after expanding the index
and the sparse directory's sub-entries have to go to that same spot,
you're actually at the next valid item to iterate over.  Slightly
tricky, but makes sense.

> +               }
> +
>                 if (!ignore_skip_worktree && ce_skip_worktree(ce))
>                         continue;
>                 if (ce_stage(ce) != checkout_stage
> @@ -218,6 +239,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>         git_config(git_default_config, NULL);
>         prefix_length = prefix ? strlen(prefix) : 0;
>
> +       prepare_repo_settings(the_repository);
> +       the_repository->settings.command_requires_full_index = 0;
> +
>         if (read_cache() < 0) {
>                 die("invalid cache");
>         }
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index fad61d96107..6ecf1f2bf8e 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -796,7 +796,14 @@ test_expect_success 'checkout-index with folders' '
>         test_all_match test_must_fail git checkout-index -f -- deep/ &&
>
>         # Outside checkout definition
> -       test_all_match test_must_fail git checkout-index -f -- folder1/
> +       # Note: although all tests fail (as expected), the messaging differs. For
> +       # non-sparse index checkouts, the error is that the "file" does not appear
> +       # in the index; for sparse checkouts, the error is explicitly that the
> +       # entry is a sparse directory.
> +       run_on_all test_must_fail git checkout-index -f -- folder1/ &&
> +       test_cmp full-checkout-err sparse-checkout-err &&
> +       ! test_cmp full-checkout-err sparse-index-err &&
> +       grep "is a sparse directory" sparse-index-err
>  '
>
>  test_expect_success 'checkout-index --all' '
> @@ -965,6 +972,8 @@ test_expect_success 'sparse-index is not expanded' '
>         echo >>sparse-index/untracked.txt &&
>         ensure_not_expanded add . &&
>
> +       ensure_not_expanded checkout-index -f a &&
> +       ensure_not_expanded checkout-index -f --all &&
>         for ref in update-deep update-folder1 update-folder2 update-deep
>         do
>                 echo >>sparse-index/README.md &&
> --
> gitgitgadget

Patch looks good to me.

[PATCH 7/9] update-index: add tests for sparse-checkout compatibility

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Introduce tests for a variety of `git update-index` use cases, including
performance scenarios. Tests for `update-index add/remove` are specifically
focused on how `git stash` uses `git update-index` as a subcommand to
prepare for sparse index integration with `stash` in a future series.

Co-authored-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/perf/p2000-sparse-operations.sh        |   1 +
 t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 54f8602f3c1..7dbed330160 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -118,5 +118,6 @@ test_perf_on_all git diff --cached
 test_perf_on_all git blame $SPARSE_CONE/a
 test_perf_on_all git blame $SPARSE_CONE/f3/a
 test_perf_on_all git checkout-index -f --all
+test_perf_on_all git update-index --add --remove
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 6ecf1f2bf8e..6804ab23a27 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -630,6 +630,131 @@ test_expect_success 'reset with wildcard pathspec' '
 	test_all_match git ls-files -s -- folder1
 '
 
+test_expect_success 'update-index modify outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	# Create & modify folder1/a
+	run_on_sparse mkdir -p folder1 &&
+	run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
+	run_on_all ../edit-contents folder1/a &&
+
+	# If file has skip-worktree enabled, update-index does not modify the
+	# index entry
+	test_sparse_match git update-index folder1/a &&
+	test_sparse_match git status --porcelain=v2 &&
+	test_must_be_empty sparse-checkout-out &&
+
+	# When skip-worktree is disabled (even on files outside sparse cone), file
+	# is updated in the index
+	test_sparse_match git update-index --no-skip-worktree folder1/a &&
+	test_all_match git status --porcelain=v2 &&
+	test_all_match git update-index folder1/a &&
+	test_all_match git status --porcelain=v2
+'
+
+test_expect_success 'update-index --add outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	# Create folder1, add new file
+	run_on_sparse mkdir -p folder1 &&
+	run_on_all ../edit-contents folder1/b &&
+
+	# Similar to `git add`, the untracked out-of-cone file is added to the index
+	# identically across sparse and non-sparse checkouts
+	test_all_match git update-index --add folder1/b &&
+	test_all_match git status --porcelain=v2
+'
+
+test_expect_success 'update-index --remove outside sparse definition' '
+	init_repos &&
+
+	# When `--ignore-skip-worktree-entries` is specified, out-of-cone files are
+	# not removed from the index if they do not exist on disk
+	test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
+	test_all_match git status --porcelain=v2 &&
+
+	# When the flag is _not_ specified, out-of-cone, not-on-disk files are
+	# removed from the index
+	rm full-checkout/folder1/a &&
+	test_all_match git update-index --remove folder1/a &&
+	test_all_match git status --porcelain=v2 &&
+
+	# NOTE: --force-remove supercedes --ignore-skip-worktree-entries, removing
+	# a skip-worktree file from the index (and disk) when both are specified
+	test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
+	test_all_match git status --porcelain=v2
+'
+
+test_expect_success 'update-index with directories' '
+	init_repos &&
+
+	# update-index will exit silently when provided with a directory name
+	# containing a trailing slash
+	test_all_match git update-index deep/ folder1/ &&
+	grep "Ignoring path deep/" sparse-checkout-err &&
+	grep "Ignoring path folder1/" sparse-checkout-err &&
+
+	# When update-index is given a directory name WITHOUT a trailing slash, it will
+	# behave in different ways depending on the status of the directory on disk:
+	# * if it exists, the command exits with an error ("add individual files instead")
+	# * if it does NOT exist (e.g., in a sparse-checkout), it is assumed to be a
+	#   file and either triggers an error ("does not exist  and --remove not passed")
+	#   or is ignored completely (when using --remove)
+	test_all_match test_must_fail git update-index deep &&
+	run_on_all test_must_fail git update-indexe folder1 &&
+	test_must_fail git -C full-checkout update-index --remove folder1 &&
+	test_sparse_match git update-index --remove folder1 &&
+	test_all_match git status --porcelain=v2
+'
+
+test_expect_success 'update-index --again file outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	test_all_match git checkout -b test-reupdate &&
+
+	# Update HEAD without modifying the index to introduce a difference in
+	# folder1/a
+	test_sparse_match git reset --soft update-folder1 &&
+
+	# Because folder1/a differs in the index vs HEAD,
+	# `git update-index --remove --again` will effectively perform
+	# `git update-index --remove folder1/a` and remove the folder1/a
+	test_sparse_match git update-index --remove --again &&
+	test_sparse_match git status --porcelain=v2
+'
+
+test_expect_success 'update-index --cacheinfo' '
+	init_repos &&
+
+	deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
+	folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) &&
+	folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) &&
+
+	test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a &&
+	test_all_match git status --porcelain=v2 &&
+
+	# Cannot add sparse directory, even in sparse index case
+	test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
+
+	# Sparse match only - because folder1/a is outside the sparse checkout
+	# definition (and thus not on-disk), it will appear "deleted" in
+	# unstaged changes.
+	test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
+	test_sparse_match git status --porcelain=v2
+'
+
 test_expect_success 'merge, cherry-pick, and rebase' '
 	init_repos &&
 
-- 
gitgitgadget

Re: [PATCH 7/9] update-index: add tests for sparse-checkout compatibility

[Elijah Newren]

On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Victoria Dye <vdye@github.com>
>
> Introduce tests for a variety of `git update-index` use cases, including
> performance scenarios.

Makes sense.

> Tests for `update-index add/remove` are specifically
> focused on how `git stash` uses `git update-index` as a subcommand to
> prepare for sparse index integration with `stash` in a future series.

This is possibly a tangent, but I'd rather that if we were trying to
fix `git stash`, that we instead would do so by making it stop forking
subprocesses and having it call internal API instead.  See for
example, a4031f6dc0 ("Merge branch 'en/stash-apply-sparse-checkout'
into maint", 2021-02-05) which did this.  The fact that it forks so
many subprocesses is a bug, and comes from the fact that stash is a
partial conversion from shell to C.  I think the subprocess forking is
part of the problem that causes issues for sparse-checkouts, as I
spelled out in patches 2 & 3 of the series I mentioned above.

However, that doesn't affect this series.

> Co-authored-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  t/perf/p2000-sparse-operations.sh        |   1 +
>  t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
>
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 54f8602f3c1..7dbed330160 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -118,5 +118,6 @@ test_perf_on_all git diff --cached
>  test_perf_on_all git blame $SPARSE_CONE/a
>  test_perf_on_all git blame $SPARSE_CONE/f3/a
>  test_perf_on_all git checkout-index -f --all
> +test_perf_on_all git update-index --add --remove
>
>  test_done
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 6ecf1f2bf8e..6804ab23a27 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -630,6 +630,131 @@ test_expect_success 'reset with wildcard pathspec' '
>         test_all_match git ls-files -s -- folder1
>  '
>
> +test_expect_success 'update-index modify outside sparse definition' '
> +       init_repos &&
> +
> +       write_script edit-contents <<-\EOF &&
> +       echo text >>$1
> +       EOF
> +
> +       # Create & modify folder1/a
> +       run_on_sparse mkdir -p folder1 &&
> +       run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
> +       run_on_all ../edit-contents folder1/a &&

As I've mentioned to Stolee, I'd rather these were explicitly marked
as intentionally setting up an erroneous condition.

However, that might not matter if my other series gets accepted (the
one I promised to send out yesterday, but then spent all day
responding to emails instead).  Hopefully I'll send it soon.

> +
> +       # If file has skip-worktree enabled, update-index does not modify the
> +       # index entry
> +       test_sparse_match git update-index folder1/a &&
> +       test_sparse_match git status --porcelain=v2 &&
> +       test_must_be_empty sparse-checkout-out &&
> +
> +       # When skip-worktree is disabled (even on files outside sparse cone), file
> +       # is updated in the index
> +       test_sparse_match git update-index --no-skip-worktree folder1/a &&
> +       test_all_match git status --porcelain=v2 &&
> +       test_all_match git update-index folder1/a &&
> +       test_all_match git status --porcelain=v2

These make sense.

> +'
> +
> +test_expect_success 'update-index --add outside sparse definition' '
> +       init_repos &&
> +
> +       write_script edit-contents <<-\EOF &&
> +       echo text >>$1
> +       EOF
> +
> +       # Create folder1, add new file
> +       run_on_sparse mkdir -p folder1 &&
> +       run_on_all ../edit-contents folder1/b &&
> +
> +       # Similar to `git add`, the untracked out-of-cone file is added to the index
> +       # identically across sparse and non-sparse checkouts
> +       test_all_match git update-index --add folder1/b &&
> +       test_all_match git status --porcelain=v2

The comment is not correct:

$ git add out-of-cone/file
The following paths and/or pathspecs matched paths that exist
outside of your sparse-checkout definition, so will not be
updated in the index:
out-of-cone/file
hint: If you intend to update such entries, try one of the following:
hint: * Use the --sparse option.
hint: * Disable or modify the sparsity rules.
hint: Disable this message with "git config advice.updateSparsePath false"

I might buy that `git update-index` is lower level and should be
considered the same as `git add --sparse`, but the comment should
mention that and try to sell why update-index should be equivalent to
that instead of to `git add`.

> +'
> +
> +test_expect_success 'update-index --remove outside sparse definition' '
> +       init_repos &&

> +
> +       # When `--ignore-skip-worktree-entries` is specified, out-of-cone files are
> +       # not removed from the index if they do not exist on disk
> +       test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
> +       test_all_match git status --porcelain=v2 &&

The file is present despite being marked to be missing, we're ignoring
the intention of the marking, and we ask for it to be removed, so we
don't remove it?

The levels of negation are _very_ confusing.  It took me a while to
unravel this.  I think the logic is something like this
  * folder1/a is marked as SKIP_WORKTREE, meaning it's not supposed to
be in the worktree.
  * and it's not.
  * We are stating that we're ignoring SKIP_WORKTREE, though, so this
looks like a regular file that has been deleted.
So, what would `git update-index --remove $FILE` do for a normal $FILE
deleted from the working copy?  According to the docs:

    --remove
           If a specified file is in the index but is missing then it’s
           removed. Default behavior is to ignore removed file.

So, the docs say it would remove it.  But you don't.


After digging around and looking at the testcase below, if I had to
guess what happened, I would say that you figured out what the
SKIP_WORKTREE behavior was, and assumed that was correct, and added a
flag that allowed you to request the opposite behavior.
Unfortunately, I think the pre-existing behavior is buggy.

Of course, there's lots of negation here.  Did I get something
backwards by chance?

> +
> +       # When the flag is _not_ specified ...

In my head I'm translating this as:

SKIP_WORKTREE = !worktree
--ignore-skip-worktree-entries = !!worktree
"flag is _not_ specified" = !!!worktree ?

I'm not sure I would do anything to change it, but just pointing out
it can be a little hard for others to come up to speed.

>             ...     , out-of-cone, not-on-disk files are
> +       # removed from the index
> +       rm full-checkout/folder1/a &&
> +       test_all_match git update-index --remove folder1/a &&
> +       test_all_match git status --porcelain=v2 &&

Documentation/git-update-index.txt defines SKIP_WORKTREE as follows:

"Skip-worktree bit can be defined in one (long) sentence: When reading
an entry, if it is marked as skip-worktree, then Git pretends its
working directory version is up to date and read the index version
instead."

If Git is pretending the file is up-to-date, and `git update-index
--remove $UP_TO_DATE_FILE` is typically a no-op because the --remove
flag doesn't do anything when a file is present in the working copy,
then why is this the expected behavior?

I know it's the traditional behavior of update-index, but
SKIP_WORKTREE support in Git has traditionally been filled with holes.
So, was this behavior by design (despite contradicting the
documentation), or by accident?

(To be fair, I think the definition given in the manual for
SKIP_WORKTREE is horrible for other reasons, so I don't like leaning
on it.  But I used different logic above in the
--ignore-skip-worktree-entries case to arrive at the same conclusion
that the --remove behavior of update-index seems to be backwards.
Unless I missed a negation in both cases somewhere?  There are so many
floating around...)

> +       # NOTE: --force-remove supercedes --ignore-skip-worktree-entries, removing
> +       # a skip-worktree file from the index (and disk) when both are specified
> +       test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
> +       test_all_match git status --porcelain=v2

Makes sense.

> +'
> +
> +test_expect_success 'update-index with directories' '
> +       init_repos &&
> +
> +       # update-index will exit silently when provided with a directory name
> +       # containing a trailing slash
> +       test_all_match git update-index deep/ folder1/ &&
> +       grep "Ignoring path deep/" sparse-checkout-err &&
> +       grep "Ignoring path folder1/" sparse-checkout-err &&

Is this desired behavior or just current behavior?

> +
> +       # When update-index is given a directory name WITHOUT a trailing slash, it will
> +       # behave in different ways depending on the status of the directory on disk:
> +       # * if it exists, the command exits with an error ("add individual files instead")
> +       # * if it does NOT exist (e.g., in a sparse-checkout), it is assumed to be a
> +       #   file and either triggers an error ("does not exist  and --remove not passed")
> +       #   or is ignored completely (when using --remove)
> +       test_all_match test_must_fail git update-index deep &&
> +       run_on_all test_must_fail git update-indexe folder1 &&

This one will fail for the wrong reason, though -- `update-indexe` is
not a valid subcommand.  (extra 'e' at the end)

> +       test_must_fail git -C full-checkout update-index --remove folder1 &&
> +       test_sparse_match git update-index --remove folder1 &&
> +       test_all_match git status --porcelain=v2

Otherwise these seem reasonable.

> +'
> +
> +test_expect_success 'update-index --again file outside sparse definition' '
> +       init_repos &&
> +
> +       write_script edit-contents <<-\EOF &&
> +       echo text >>$1
> +       EOF

Copy and paste and forget to remove?  edit-contents doesn't seem to be
used in this test.

> +
> +       test_all_match git checkout -b test-reupdate &&
> +
> +       # Update HEAD without modifying the index to introduce a difference in
> +       # folder1/a
> +       test_sparse_match git reset --soft update-folder1 &&
> +
> +       # Because folder1/a differs in the index vs HEAD,
> +       # `git update-index --remove --again` will effectively perform
> +       # `git update-index --remove folder1/a` and remove the folder1/a
> +       test_sparse_match git update-index --remove --again &&
> +       test_sparse_match git status --porcelain=v2

This might need a --ignore-skip-worktree-entries, as per the
discussion above.  Otherwise, this test makes sense.

> +'
> +
> +test_expect_success 'update-index --cacheinfo' '
> +       init_repos &&
> +
> +       deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
> +       folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) &&
> +       folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) &&
> +
> +       test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a &&
> +       test_all_match git status --porcelain=v2 &&
> +
> +       # Cannot add sparse directory, even in sparse index case
> +       test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
> +
> +       # Sparse match only - because folder1/a is outside the sparse checkout
> +       # definition (and thus not on-disk), it will appear "deleted" in
> +       # unstaged changes.
> +       test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
> +       test_sparse_match git status --porcelain=v2

Makes sense, because the update-index command removes the existing
cache entry and adds a new one without the SKIP_WORKTREE bit.  But it
might be worth mentioning that in the commit message.  Also, you could
follow this up with `git update-index --skip-worktree folder1/a`, and
then do a test_all_match git status --porcelain=v2, to show that when
the SKIP_WORKTREE bit is restored back to the file, then it again does
as expected despite not being on-disk.

> +'
> +
>  test_expect_success 'merge, cherry-pick, and rebase' '
>         init_repos &&
>
> --
> gitgitgadget

Re: [PATCH 7/9] update-index: add tests for sparse-checkout compatibility

[Victoria Dye]

Elijah Newren wrote:
> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Victoria Dye <vdye@github.com>
>>
>> Introduce tests for a variety of `git update-index` use cases, including
>> performance scenarios.
> 
> Makes sense.
> 
>> Tests for `update-index add/remove` are specifically
>> focused on how `git stash` uses `git update-index` as a subcommand to
>> prepare for sparse index integration with `stash` in a future series.
> 
> This is possibly a tangent, but I'd rather that if we were trying to
> fix `git stash`, that we instead would do so by making it stop forking
> subprocesses and having it call internal API instead.  See for
> example, a4031f6dc0 ("Merge branch 'en/stash-apply-sparse-checkout'
> into maint", 2021-02-05) which did this.  The fact that it forks so
> many subprocesses is a bug, and comes from the fact that stash is a
> partial conversion from shell to C.  I think the subprocess forking is
> part of the problem that causes issues for sparse-checkouts, as I
> spelled out in patches 2 & 3 of the series I mentioned above.
> 
> However, that doesn't affect this series.
> 
>> Co-authored-by: Derrick Stolee <dstolee@microsoft.com>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  t/perf/p2000-sparse-operations.sh        |   1 +
>>  t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
>>  2 files changed, 126 insertions(+)
>>
>> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
>> index 54f8602f3c1..7dbed330160 100755
>> --- a/t/perf/p2000-sparse-operations.sh
>> +++ b/t/perf/p2000-sparse-operations.sh
>> @@ -118,5 +118,6 @@ test_perf_on_all git diff --cached
>>  test_perf_on_all git blame $SPARSE_CONE/a
>>  test_perf_on_all git blame $SPARSE_CONE/f3/a
>>  test_perf_on_all git checkout-index -f --all
>> +test_perf_on_all git update-index --add --remove
>>
>>  test_done
>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>> index 6ecf1f2bf8e..6804ab23a27 100755
>> --- a/t/t1092-sparse-checkout-compatibility.sh
>> +++ b/t/t1092-sparse-checkout-compatibility.sh
>> @@ -630,6 +630,131 @@ test_expect_success 'reset with wildcard pathspec' '
>>         test_all_match git ls-files -s -- folder1
>>  '
>>
>> +test_expect_success 'update-index modify outside sparse definition' '
>> +       init_repos &&
>> +
>> +       write_script edit-contents <<-\EOF &&
>> +       echo text >>$1
>> +       EOF
>> +
>> +       # Create & modify folder1/a
>> +       run_on_sparse mkdir -p folder1 &&
>> +       run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
>> +       run_on_all ../edit-contents folder1/a &&
> 
> As I've mentioned to Stolee, I'd rather these were explicitly marked
> as intentionally setting up an erroneous condition.
> 
> However, that might not matter if my other series gets accepted (the
> one I promised to send out yesterday, but then spent all day
> responding to emails instead).  Hopefully I'll send it soon.
> 
>> +
>> +       # If file has skip-worktree enabled, update-index does not modify the
>> +       # index entry
>> +       test_sparse_match git update-index folder1/a &&
>> +       test_sparse_match git status --porcelain=v2 &&
>> +       test_must_be_empty sparse-checkout-out &&
>> +
>> +       # When skip-worktree is disabled (even on files outside sparse cone), file
>> +       # is updated in the index
>> +       test_sparse_match git update-index --no-skip-worktree folder1/a &&
>> +       test_all_match git status --porcelain=v2 &&
>> +       test_all_match git update-index folder1/a &&
>> +       test_all_match git status --porcelain=v2
> 
> These make sense.
> 
>> +'
>> +
>> +test_expect_success 'update-index --add outside sparse definition' '
>> +       init_repos &&
>> +
>> +       write_script edit-contents <<-\EOF &&
>> +       echo text >>$1
>> +       EOF
>> +
>> +       # Create folder1, add new file
>> +       run_on_sparse mkdir -p folder1 &&
>> +       run_on_all ../edit-contents folder1/b &&
>> +
>> +       # Similar to `git add`, the untracked out-of-cone file is added to the index
>> +       # identically across sparse and non-sparse checkouts
>> +       test_all_match git update-index --add folder1/b &&
>> +       test_all_match git status --porcelain=v2
> 
> The comment is not correct:
> 

It is correct, but for *untracked* out-of-cone files only. Those files don't
have a `skip-worktree` bit because they're not in the index in the first
place. The comment is intended to highlight the fact that `update-index`
(like `git add`, `git status`, etc.) "decides" whether to operate on a file
in a sparse-checkout based on `skip-worktree`, *not* the sparse patterns.

Seeing as the comparison to `git add` makes things more confusing, I'll
rephrase the test comment.

> $ git add out-of-cone/file
> The following paths and/or pathspecs matched paths that exist
> outside of your sparse-checkout definition, so will not be
> updated in the index:
> out-of-cone/file
> hint: If you intend to update such entries, try one of the following:
> hint: * Use the --sparse option.
> hint: * Disable or modify the sparsity rules.
> hint: Disable this message with "git config advice.updateSparsePath false"
> 
> I might buy that `git update-index` is lower level and should be
> considered the same as `git add --sparse`, but the comment should
> mention that and try to sell why update-index should be equivalent to
> that instead of to `git add`.
> 

Tracked, out-of-cone files aren't affected by `--add` (the flag allows
`update-index` to add untracked files), and `update-index out-of-cone/tracked`
would ignore the file, so I believe the behavior of `update-index` is
currently more consistent with `git add` than `git add --sparse`.

>> +'
>> +
>> +test_expect_success 'update-index --remove outside sparse definition' '
>> +       init_repos &&
> 
>> +
>> +       # When `--ignore-skip-worktree-entries` is specified, out-of-cone files are
>> +       # not removed from the index if they do not exist on disk
>> +       test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
>> +       test_all_match git status --porcelain=v2 &&
> 
> The file is present despite being marked to be missing, we're ignoring
> the intention of the marking, and we ask for it to be removed, so we
> don't remove it?
> 
> The levels of negation are _very_ confusing.  It took me a while to
> unravel this.  I think the logic is something like this
>   * folder1/a is marked as SKIP_WORKTREE, meaning it's not supposed to
> be in the worktree.
>   * and it's not.
>   * We are stating that we're ignoring SKIP_WORKTREE, though, so this
> looks like a regular file that has been deleted.
> So, what would `git update-index --remove $FILE` do for a normal $FILE
> deleted from the working copy?  According to the docs:
> 
>     --remove
>            If a specified file is in the index but is missing then it’s
>            removed. Default behavior is to ignore removed file.
> 
> So, the docs say it would remove it.  But you don't.
> 
> 
> After digging around and looking at the testcase below, if I had to
> guess what happened, I would say that you figured out what the
> SKIP_WORKTREE behavior was, and assumed that was correct, and added a
> flag that allowed you to request the opposite behavior.
> Unfortunately, I think the pre-existing behavior is buggy.
> 

I understand why you find it buggy, but I am not making baseless assumptions
about the correctness (or lack thereof) of the current behavior. This
specific "gap" in `update-index --remove` has been discussed in the past [1]
and was acknowledged as non-ideal and a candidate for change in the future.
At the time, the introduction of `--ignore-skip-worktree-entries` [2] was a
"safe" way to ignore `skip-worktree` without changing the default behavior.

Personally, I think updating the default behavior of `--remove` (and
corresponding deprecation of `--[no-]ignore-skip-worktree-entries`) is
probably the right way to go. However, I'd like to avoid including it in
this series because it deviates pretty substantially from the goal
"integrate with sparse index", and as a result has the potential to
overshadow/derail the rest of the series. If you're alright with (slightly)
deferring changes to the behavior of `--remove`, I can submit a separate
series for it once this one has stabilized.

[1] https://lore.kernel.org/git/xmqq36fda3i8.fsf@gitster-ct.c.googlers.com/
[2] https://lore.kernel.org/git/163b42dfa21c306dc1dc573c5edfc8bda5c99fd0.1572432578.git.gitgitgadget@gmail.com/

> Of course, there's lots of negation here.  Did I get something
> backwards by chance?
> 
>> +
>> +       # When the flag is _not_ specified ...
> 
> In my head I'm translating this as:
> 
> SKIP_WORKTREE = !worktree
> --ignore-skip-worktree-entries = !!worktree
> "flag is _not_ specified" = !!!worktree ?
> 
> I'm not sure I would do anything to change it, but just pointing out
> it can be a little hard for others to come up to speed.
> 

Most of the confusion likely comes from the non-standard behavior of
`--remove`, but I think I can distill it into (somewhat) straightforward
statements about `update-index`:

1. When using the command *without* either `--ignore-skip-worktree-entries`
   OR `--remove`, `skip-worktree` entries provided to the command are
   ignored.
2. When using the command *with* `--remove` and *without*
   `--ignore-skip-worktree-entries`, `skip-worktree` entries are *not*
   ignored, and are removed from the index.
3. When both `--remove` and `--ignore-skip-worktree-entries` are specified,
   `skip-worktree` entries are again ignored.
4. `--ignore-skip-worktree-entries` has no effect without `--remove` also
   specified

The goal of this test, then, is to exercise conditions 2 & 3 and directly
show their effect on `skip-worktree` entries.

>>             ...     , out-of-cone, not-on-disk files are
>> +       # removed from the index
>> +       rm full-checkout/folder1/a &&
>> +       test_all_match git update-index --remove folder1/a &&
>> +       test_all_match git status --porcelain=v2 &&
> 
> Documentation/git-update-index.txt defines SKIP_WORKTREE as follows:
> 
> "Skip-worktree bit can be defined in one (long) sentence: When reading
> an entry, if it is marked as skip-worktree, then Git pretends its
> working directory version is up to date and read the index version
> instead."
> 
> If Git is pretending the file is up-to-date, and `git update-index
> --remove $UP_TO_DATE_FILE` is typically a no-op because the --remove
> flag doesn't do anything when a file is present in the working copy,
> then why is this the expected behavior?
> 
> I know it's the traditional behavior of update-index, but
> SKIP_WORKTREE support in Git has traditionally been filled with holes.
> So, was this behavior by design (despite contradicting the
> documentation), or by accident?
> 

As far as I can tell, it appears to have been intentional in the original
`skip-worktree` implementation [3], but given Junio & Johannes' discussion
on the `--ignore-skip-worktree-entries` patch [1], the sentiment now would
probably lean towards having `--remove` ignore `skip-worktree`.

[1] https://lore.kernel.org/git/xmqq36fda3i8.fsf@gitster-ct.c.googlers.com/
    (copied from earlier in this message)
[3] https://lore.kernel.org/git/1250776033-12395-5-git-send-email-pclouds@gmail.com/


> (To be fair, I think the definition given in the manual for
> SKIP_WORKTREE is horrible for other reasons, so I don't like leaning
> on it.  But I used different logic above in the
> --ignore-skip-worktree-entries case to arrive at the same conclusion
> that the --remove behavior of update-index seems to be backwards.
> Unless I missed a negation in both cases somewhere?  There are so many
> floating around...)
> 
>> +       # NOTE: --force-remove supercedes --ignore-skip-worktree-entries, removing
>> +       # a skip-worktree file from the index (and disk) when both are specified
>> +       test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
>> +       test_all_match git status --porcelain=v2
> 
> Makes sense.
> 
>> +'
>> +
>> +test_expect_success 'update-index with directories' '
>> +       init_repos &&
>> +
>> +       # update-index will exit silently when provided with a directory name
>> +       # containing a trailing slash
>> +       test_all_match git update-index deep/ folder1/ &&
>> +       grep "Ignoring path deep/" sparse-checkout-err &&
>> +       grep "Ignoring path folder1/" sparse-checkout-err &&
> 
> Is this desired behavior or just current behavior?
> 
>> +
>> +       # When update-index is given a directory name WITHOUT a trailing slash, it will
>> +       # behave in different ways depending on the status of the directory on disk:
>> +       # * if it exists, the command exits with an error ("add individual files instead")
>> +       # * if it does NOT exist (e.g., in a sparse-checkout), it is assumed to be a
>> +       #   file and either triggers an error ("does not exist  and --remove not passed")
>> +       #   or is ignored completely (when using --remove)
>> +       test_all_match test_must_fail git update-index deep &&
>> +       run_on_all test_must_fail git update-indexe folder1 &&
> 
> This one will fail for the wrong reason, though -- `update-indexe` is
> not a valid subcommand.  (extra 'e' at the end)
> 

Thanks for catching that! I'll update in my next re-roll.

>> +       test_must_fail git -C full-checkout update-index --remove folder1 &&
>> +       test_sparse_match git update-index --remove folder1 &&
>> +       test_all_match git status --porcelain=v2
> 
> Otherwise these seem reasonable.
> 
>> +'
>> +
>> +test_expect_success 'update-index --again file outside sparse definition' '
>> +       init_repos &&
>> +
>> +       write_script edit-contents <<-\EOF &&
>> +       echo text >>$1
>> +       EOF
> 
> Copy and paste and forget to remove?  edit-contents doesn't seem to be
> used in this test.
> 

Will remove.

>> +
>> +       test_all_match git checkout -b test-reupdate &&
>> +
>> +       # Update HEAD without modifying the index to introduce a difference in
>> +       # folder1/a
>> +       test_sparse_match git reset --soft update-folder1 &&
>> +
>> +       # Because folder1/a differs in the index vs HEAD,
>> +       # `git update-index --remove --again` will effectively perform
>> +       # `git update-index --remove folder1/a` and remove the folder1/a
>> +       test_sparse_match git update-index --remove --again &&
>> +       test_sparse_match git status --porcelain=v2
> 
> This might need a --ignore-skip-worktree-entries, as per the
> discussion above.  Otherwise, this test makes sense.
> 

The `--ignore-skip-worktree-entries` option is explicitly omitted because
this test needs `update-index` to modify a `skip-worktree` entry. However,
given the debate around what `--remove` should do, I'll update the scenario
to not use `--remove` or any variation of it.

>> +'
>> +
>> +test_expect_success 'update-index --cacheinfo' '
>> +       init_repos &&
>> +
>> +       deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
>> +       folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) &&
>> +       folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) &&
>> +
>> +       test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a &&
>> +       test_all_match git status --porcelain=v2 &&
>> +
>> +       # Cannot add sparse directory, even in sparse index case
>> +       test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
>> +
>> +       # Sparse match only - because folder1/a is outside the sparse checkout
>> +       # definition (and thus not on-disk), it will appear "deleted" in
>> +       # unstaged changes.
>> +       test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
>> +       test_sparse_match git status --porcelain=v2
> 
> Makes sense, because the update-index command removes the existing
> cache entry and adds a new one without the SKIP_WORKTREE bit.  But it
> might be worth mentioning that in the commit message.  Also, you could
> follow this up with `git update-index --skip-worktree folder1/a`, and
> then do a test_all_match git status --porcelain=v2, to show that when
> the SKIP_WORKTREE bit is restored back to the file, then it again does
> as expected despite not being on-disk.
> 

I really like this - it helps clarify how `update-index` can be used to
correctly add a sparse-checkout entry to the index with plumbing commands.
I'll include it in V2.

>> +'
>> +
>>  test_expect_success 'merge, cherry-pick, and rebase' '
>>         init_repos &&
>>
>> --
>> gitgitgadget

Re: [PATCH 7/9] update-index: add tests for sparse-checkout compatibility

[Elijah Newren]

On Mon, Jan 10, 2022 at 7:47 AM Victoria Dye <vdye@github.com> wrote:
>
> Elijah Newren wrote:
> > On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Victoria Dye <vdye@github.com>
> >>
> >> Introduce tests for a variety of `git update-index` use cases, including
> >> performance scenarios.
> >
> > Makes sense.
> >
> >> Tests for `update-index add/remove` are specifically
> >> focused on how `git stash` uses `git update-index` as a subcommand to
> >> prepare for sparse index integration with `stash` in a future series.
> >
> > This is possibly a tangent, but I'd rather that if we were trying to
> > fix `git stash`, that we instead would do so by making it stop forking
> > subprocesses and having it call internal API instead.  See for
> > example, a4031f6dc0 ("Merge branch 'en/stash-apply-sparse-checkout'
> > into maint", 2021-02-05) which did this.  The fact that it forks so
> > many subprocesses is a bug, and comes from the fact that stash is a
> > partial conversion from shell to C.  I think the subprocess forking is
> > part of the problem that causes issues for sparse-checkouts, as I
> > spelled out in patches 2 & 3 of the series I mentioned above.
> >
> > However, that doesn't affect this series.
> >
> >> Co-authored-by: Derrick Stolee <dstolee@microsoft.com>
> >> Signed-off-by: Victoria Dye <vdye@github.com>
> >> ---
> >>  t/perf/p2000-sparse-operations.sh        |   1 +
> >>  t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
> >>  2 files changed, 126 insertions(+)
> >>
> >> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> >> index 54f8602f3c1..7dbed330160 100755
> >> --- a/t/perf/p2000-sparse-operations.sh
> >> +++ b/t/perf/p2000-sparse-operations.sh
> >> @@ -118,5 +118,6 @@ test_perf_on_all git diff --cached
> >>  test_perf_on_all git blame $SPARSE_CONE/a
> >>  test_perf_on_all git blame $SPARSE_CONE/f3/a
> >>  test_perf_on_all git checkout-index -f --all
> >> +test_perf_on_all git update-index --add --remove
> >>
> >>  test_done
> >> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> >> index 6ecf1f2bf8e..6804ab23a27 100755
> >> --- a/t/t1092-sparse-checkout-compatibility.sh
> >> +++ b/t/t1092-sparse-checkout-compatibility.sh
> >> @@ -630,6 +630,131 @@ test_expect_success 'reset with wildcard pathspec' '
> >>         test_all_match git ls-files -s -- folder1
> >>  '
> >>
> >> +test_expect_success 'update-index modify outside sparse definition' '
> >> +       init_repos &&
> >> +
> >> +       write_script edit-contents <<-\EOF &&
> >> +       echo text >>$1
> >> +       EOF
> >> +
> >> +       # Create & modify folder1/a
> >> +       run_on_sparse mkdir -p folder1 &&
> >> +       run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
> >> +       run_on_all ../edit-contents folder1/a &&
> >
> > As I've mentioned to Stolee, I'd rather these were explicitly marked
> > as intentionally setting up an erroneous condition.
> >
> > However, that might not matter if my other series gets accepted (the
> > one I promised to send out yesterday, but then spent all day
> > responding to emails instead).  Hopefully I'll send it soon.
> >
> >> +
> >> +       # If file has skip-worktree enabled, update-index does not modify the
> >> +       # index entry
> >> +       test_sparse_match git update-index folder1/a &&
> >> +       test_sparse_match git status --porcelain=v2 &&
> >> +       test_must_be_empty sparse-checkout-out &&
> >> +
> >> +       # When skip-worktree is disabled (even on files outside sparse cone), file
> >> +       # is updated in the index
> >> +       test_sparse_match git update-index --no-skip-worktree folder1/a &&
> >> +       test_all_match git status --porcelain=v2 &&
> >> +       test_all_match git update-index folder1/a &&
> >> +       test_all_match git status --porcelain=v2
> >
> > These make sense.
> >
> >> +'
> >> +
> >> +test_expect_success 'update-index --add outside sparse definition' '
> >> +       init_repos &&
> >> +
> >> +       write_script edit-contents <<-\EOF &&
> >> +       echo text >>$1
> >> +       EOF
> >> +
> >> +       # Create folder1, add new file
> >> +       run_on_sparse mkdir -p folder1 &&
> >> +       run_on_all ../edit-contents folder1/b &&
> >> +
> >> +       # Similar to `git add`, the untracked out-of-cone file is added to the index
> >> +       # identically across sparse and non-sparse checkouts
> >> +       test_all_match git update-index --add folder1/b &&
> >> +       test_all_match git status --porcelain=v2
> >
> > The comment is not correct:
> >
>
> It is correct, but for *untracked* out-of-cone files only. Those files don't
> have a `skip-worktree` bit because they're not in the index in the first
> place.

> The comment is intended to highlight the fact that `update-index`
> (like `git add`, `git status`, etc.) "decides" whether to operate on a file
> in a sparse-checkout based on `skip-worktree`, *not* the sparse patterns.

git-update-index may not pay attention to the sparsity patterns for
untracked files, but `git add` does.  Let me demonstrate.  First, I
set up a simple repo where the following is true:

$ git ls-files -t
H in-cone/foo.c
S out-of-cone/tracked
$ git status --porcelain
?? out-of-cone/initially-untracked

Now, let's compare `git add` and `git add --sparse`.  First, without --sparse:

$ git add out-of-cone/initially-untracked
The following paths and/or pathspecs matched paths that exist
outside of your sparse-checkout definition, so will not be
updated in the index:
out-of-cone/initially-untracked
hint: If you intend to update such entries, try one of the following:
hint: * Use the --sparse option.
hint: * Disable or modify the sparsity rules.
hint: Disable this message with "git config advice.updateSparsePath false"
$ git ls-files -t
H in-cone/foo.c
S out-of-cone/tracked

So, `git add $UNTRACKED` did not add the file.  In contrast:

$ git add --sparse out-of-cone/initially-untracked
$ git ls-files -t
H in-cone/foo.c
H out-of-cone/initially-untracked
S out-of-cone/tracked

So, `git add --sparse $UNTRACKED` did add it.

> Seeing as the comparison to `git add` makes things more confusing, I'll
> rephrase the test comment.
>
> > $ git add out-of-cone/file
> > The following paths and/or pathspecs matched paths that exist
> > outside of your sparse-checkout definition, so will not be
> > updated in the index:
> > out-of-cone/file
> > hint: If you intend to update such entries, try one of the following:
> > hint: * Use the --sparse option.
> > hint: * Disable or modify the sparsity rules.
> > hint: Disable this message with "git config advice.updateSparsePath false"
> >
> > I might buy that `git update-index` is lower level and should be
> > considered the same as `git add --sparse`, but the comment should
> > mention that and try to sell why update-index should be equivalent to
> > that instead of to `git add`.
> >
>
> Tracked, out-of-cone files aren't affected by `--add` (the flag allows
> `update-index` to add untracked files), and `update-index out-of-cone/tracked`
> would ignore the file.

Yes, I believe you're explaining update-index behavior correctly.

> so I believe the behavior of `update-index` is
> currently more consistent with `git add` than `git add --sparse`.

But not quite `git add`'s.  Just to be clear, let's add update-index
to the above comparison I did between add and add --sparse.  First,
let's go back to the initial setup point:

$ git ls-files -t
H in-cone/foo.c
S out-of-cone/tracked
$ git status --porcelain
?? out-of-cone/initially-untracked

Now, let's try update-index:

$ git update-index --add out-of-cone/initially-untracked
$ git ls-files -t
H in-cone/foo.c
H out-of-cone/initially-untracked
S out-of-cone/tracked

So, in other words, `git update-index --add $UNTRACKED` matches the
behavior of `git add --sparse $UNTRACKED`, not the behavior of `git
add $UNTRACKED`.

> >> +'
> >> +
> >> +test_expect_success 'update-index --remove outside sparse definition' '
> >> +       init_repos &&
> >
> >> +
> >> +       # When `--ignore-skip-worktree-entries` is specified, out-of-cone files are
> >> +       # not removed from the index if they do not exist on disk
> >> +       test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
> >> +       test_all_match git status --porcelain=v2 &&
> >
> > The file is present despite being marked to be missing, we're ignoring
> > the intention of the marking, and we ask for it to be removed, so we
> > don't remove it?
> >
> > The levels of negation are _very_ confusing.  It took me a while to
> > unravel this.  I think the logic is something like this
> >   * folder1/a is marked as SKIP_WORKTREE, meaning it's not supposed to
> > be in the worktree.
> >   * and it's not.
> >   * We are stating that we're ignoring SKIP_WORKTREE, though, so this
> > looks like a regular file that has been deleted.
> > So, what would `git update-index --remove $FILE` do for a normal $FILE
> > deleted from the working copy?  According to the docs:
> >
> >     --remove
> >            If a specified file is in the index but is missing then it’s
> >            removed. Default behavior is to ignore removed file.
> >
> > So, the docs say it would remove it.  But you don't.
> >
> >
> > After digging around and looking at the testcase below, if I had to
> > guess what happened, I would say that you figured out what the
> > SKIP_WORKTREE behavior was, and assumed that was correct, and added a
> > flag that allowed you to request the opposite behavior.
> > Unfortunately, I think the pre-existing behavior is buggy.
> >
>
> I understand why you find it buggy, but I am not making baseless assumptions
> about the correctness (or lack thereof) of the current behavior.

To be clear, the fact that the behavior was there for a decade would
typically be basis enough for an assumption (in my opinion), and I
wouldn't have faulted folks for making it.  I might well have done so
myself.  My reasoning was just that I was getting confused by the
negations and trying to understand the testcase, and when I started to
unravel it, I found what looked like a possible inconsistency.

Anyway, it's clear here you've actually dug a lot deeper and know the
history here.  In contrast, I was making assumptions about the history
(and ones that weren't correct, though I'd argue my assumptions
weren't baseless)...

> This
> specific "gap" in `update-index --remove` has been discussed in the past [1]
> and was acknowledged as non-ideal and a candidate for change in the future.
> At the time, the introduction of `--ignore-skip-worktree-entries` [2] was a
> "safe" way to ignore `skip-worktree` without changing the default behavior.
>
> Personally, I think updating the default behavior of `--remove` (and
> corresponding deprecation of `--[no-]ignore-skip-worktree-entries`) is
> probably the right way to go. However, I'd like to avoid including it in
> this series because it deviates pretty substantially from the goal
> "integrate with sparse index", and as a result has the potential to
> overshadow/derail the rest of the series. If you're alright with (slightly)
> deferring changes to the behavior of `--remove`, I can submit a separate
> series for it once this one has stabilized.
>
> [1] https://lore.kernel.org/git/xmqq36fda3i8.fsf@gitster-ct.c.googlers.com/
> [2] https://lore.kernel.org/git/163b42dfa21c306dc1dc573c5edfc8bda5c99fd0.1572432578.git.gitgitgadget@gmail.com/

Oh, wow.

Yeah, fixing it in a later series seems fine.  However, could you add
a comment to these testcases that --remove has behavior that violates
the definition of `SKIP_WORKTREE`, and that we might want to fix that
in the future but for now you are just testing the pre-existing
behavior for coverage?

(There is a possibility we can't fix it for some reason when we dig
in.  In that case, we should update the documentation for `--remove`
to call out this special case.  But again, that can be for a later
series.)

> > Of course, there's lots of negation here.  Did I get something
> > backwards by chance?
> >
> >> +
> >> +       # When the flag is _not_ specified ...
> >
> > In my head I'm translating this as:
> >
> > SKIP_WORKTREE = !worktree
> > --ignore-skip-worktree-entries = !!worktree
> > "flag is _not_ specified" = !!!worktree ?
> >
> > I'm not sure I would do anything to change it, but just pointing out
> > it can be a little hard for others to come up to speed.
> >
>
> Most of the confusion likely comes from the non-standard behavior of
> `--remove`, but I think I can distill it into (somewhat) straightforward
> statements about `update-index`:
>
> 1. When using the command *without* either `--ignore-skip-worktree-entries`
>    OR `--remove`, `skip-worktree` entries provided to the command are
>    ignored.
> 2. When using the command *with* `--remove` and *without*
>    `--ignore-skip-worktree-entries`, `skip-worktree` entries are *not*
>    ignored, and are removed from the index.
> 3. When both `--remove` and `--ignore-skip-worktree-entries` are specified,
>    `skip-worktree` entries are again ignored.
> 4. `--ignore-skip-worktree-entries` has no effect without `--remove` also
>    specified
>
> The goal of this test, then, is to exercise conditions 2 & 3 and directly
> show their effect on `skip-worktree` entries.

Yeah, it makes sense to have good test coverage.  +1.

>
> >>             ...     , out-of-cone, not-on-disk files are
> >> +       # removed from the index
> >> +       rm full-checkout/folder1/a &&
> >> +       test_all_match git update-index --remove folder1/a &&
> >> +       test_all_match git status --porcelain=v2 &&
> >
> > Documentation/git-update-index.txt defines SKIP_WORKTREE as follows:
> >
> > "Skip-worktree bit can be defined in one (long) sentence: When reading
> > an entry, if it is marked as skip-worktree, then Git pretends its
> > working directory version is up to date and read the index version
> > instead."
> >
> > If Git is pretending the file is up-to-date, and `git update-index
> > --remove $UP_TO_DATE_FILE` is typically a no-op because the --remove
> > flag doesn't do anything when a file is present in the working copy,
> > then why is this the expected behavior?
> >
> > I know it's the traditional behavior of update-index, but
> > SKIP_WORKTREE support in Git has traditionally been filled with holes.
> > So, was this behavior by design (despite contradicting the
> > documentation), or by accident?
> >
>
> As far as I can tell, it appears to have been intentional in the original
> `skip-worktree` implementation [3], but given Junio & Johannes' discussion
> on the `--ignore-skip-worktree-entries` patch [1], the sentiment now would
> probably lean towards having `--remove` ignore `skip-worktree`.
>
> [1] https://lore.kernel.org/git/xmqq36fda3i8.fsf@gitster-ct.c.googlers.com/
>     (copied from earlier in this message)
> [3] https://lore.kernel.org/git/1250776033-12395-5-git-send-email-pclouds@gmail.com/

Allow me to comment on this history...

I can see sometimes wanting to make a command line option a special
case that doesn't follow the general documented rules.  Not sure I
believe it in this specific case, but I can see it as being a
possibility in general.  But when you're going to make an option not
follow the otherwise documented behavior, then that option's
documentation should have a special callout about how it diverges
and/or why it's a special case.  So, this seems like a double-layered
problem to me (not only choosing the wrong behavior, but leaving the
documentation to claim it does something else).  It looks like Dscho
partially tried to fix it, but I would have preferred that Dscho's
documentation comment he added to `--ignore-skip-worktree-entries` be
added to `--remove`; it's odd that folks wanting to learn about
`--remove` behavior need to read the documentation for
`--ignore-skip-worktree-entries` (even if they aren't using it) in
order to understand `--remove`'s behavior.

> > (To be fair, I think the definition given in the manual for
> > SKIP_WORKTREE is horrible for other reasons, so I don't like leaning
> > on it.  But I used different logic above in the
> > --ignore-skip-worktree-entries case to arrive at the same conclusion
> > that the --remove behavior of update-index seems to be backwards.
> > Unless I missed a negation in both cases somewhere?  There are so many
> > floating around...)
> >
> >> +       # NOTE: --force-remove supercedes --ignore-skip-worktree-entries, removing
> >> +       # a skip-worktree file from the index (and disk) when both are specified
> >> +       test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
> >> +       test_all_match git status --porcelain=v2
> >
> > Makes sense.
> >
> >> +'
> >> +
> >> +test_expect_success 'update-index with directories' '
> >> +       init_repos &&
> >> +
> >> +       # update-index will exit silently when provided with a directory name
> >> +       # containing a trailing slash
> >> +       test_all_match git update-index deep/ folder1/ &&
> >> +       grep "Ignoring path deep/" sparse-checkout-err &&
> >> +       grep "Ignoring path folder1/" sparse-checkout-err &&
> >
> > Is this desired behavior or just current behavior?
> >
> >> +
> >> +       # When update-index is given a directory name WITHOUT a trailing slash, it will
> >> +       # behave in different ways depending on the status of the directory on disk:
> >> +       # * if it exists, the command exits with an error ("add individual files instead")
> >> +       # * if it does NOT exist (e.g., in a sparse-checkout), it is assumed to be a
> >> +       #   file and either triggers an error ("does not exist  and --remove not passed")
> >> +       #   or is ignored completely (when using --remove)
> >> +       test_all_match test_must_fail git update-index deep &&
> >> +       run_on_all test_must_fail git update-indexe folder1 &&
> >
> > This one will fail for the wrong reason, though -- `update-indexe` is
> > not a valid subcommand.  (extra 'e' at the end)
> >
>
> Thanks for catching that! I'll update in my next re-roll.
>
> >> +       test_must_fail git -C full-checkout update-index --remove folder1 &&
> >> +       test_sparse_match git update-index --remove folder1 &&
> >> +       test_all_match git status --porcelain=v2
> >
> > Otherwise these seem reasonable.
> >
> >> +'
> >> +
> >> +test_expect_success 'update-index --again file outside sparse definition' '
> >> +       init_repos &&
> >> +
> >> +       write_script edit-contents <<-\EOF &&
> >> +       echo text >>$1
> >> +       EOF
> >
> > Copy and paste and forget to remove?  edit-contents doesn't seem to be
> > used in this test.
> >
>
> Will remove.
>
> >> +
> >> +       test_all_match git checkout -b test-reupdate &&
> >> +
> >> +       # Update HEAD without modifying the index to introduce a difference in
> >> +       # folder1/a
> >> +       test_sparse_match git reset --soft update-folder1 &&
> >> +
> >> +       # Because folder1/a differs in the index vs HEAD,
> >> +       # `git update-index --remove --again` will effectively perform
> >> +       # `git update-index --remove folder1/a` and remove the folder1/a
> >> +       test_sparse_match git update-index --remove --again &&
> >> +       test_sparse_match git status --porcelain=v2
> >
> > This might need a --ignore-skip-worktree-entries, as per the
> > discussion above.  Otherwise, this test makes sense.
> >
>
> The `--ignore-skip-worktree-entries` option is explicitly omitted because
> this test needs `update-index` to modify a `skip-worktree` entry. However,
> given the debate around what `--remove` should do, I'll update the scenario
> to not use `--remove` or any variation of it.
>
> >> +'
> >> +
> >> +test_expect_success 'update-index --cacheinfo' '
> >> +       init_repos &&
> >> +
> >> +       deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
> >> +       folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) &&
> >> +       folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) &&
> >> +
> >> +       test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a &&
> >> +       test_all_match git status --porcelain=v2 &&
> >> +
> >> +       # Cannot add sparse directory, even in sparse index case
> >> +       test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
> >> +
> >> +       # Sparse match only - because folder1/a is outside the sparse checkout
> >> +       # definition (and thus not on-disk), it will appear "deleted" in
> >> +       # unstaged changes.
> >> +       test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
> >> +       test_sparse_match git status --porcelain=v2
> >
> > Makes sense, because the update-index command removes the existing
> > cache entry and adds a new one without the SKIP_WORKTREE bit.  But it
> > might be worth mentioning that in the commit message.  Also, you could
> > follow this up with `git update-index --skip-worktree folder1/a`, and
> > then do a test_all_match git status --porcelain=v2, to show that when
> > the SKIP_WORKTREE bit is restored back to the file, then it again does
> > as expected despite not being on-disk.
> >
>
> I really like this - it helps clarify how `update-index` can be used to
> correctly add a sparse-checkout entry to the index with plumbing commands.
> I'll include it in V2.
>
> >> +'
> >> +
> >>  test_expect_success 'merge, cherry-pick, and rebase' '
> >>         init_repos &&
> >>
> >> --
> >> gitgitgadget

Re: [PATCH 7/9] update-index: add tests for sparse-checkout compatibility

[Victoria Dye]

Elijah Newren wrote:
> On Mon, Jan 10, 2022 at 7:47 AM Victoria Dye <vdye@github.com> wrote:
>>
>> Elijah Newren wrote:
>>> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
>>> <gitgitgadget@gmail.com> wrote:
>>>>
>>>> From: Victoria Dye <vdye@github.com>
>>>>
>>>> Introduce tests for a variety of `git update-index` use cases, including
>>>> performance scenarios.
>>>
>>> Makes sense.
>>>
>>>> Tests for `update-index add/remove` are specifically
>>>> focused on how `git stash` uses `git update-index` as a subcommand to
>>>> prepare for sparse index integration with `stash` in a future series.
>>>
>>> This is possibly a tangent, but I'd rather that if we were trying to
>>> fix `git stash`, that we instead would do so by making it stop forking
>>> subprocesses and having it call internal API instead.  See for
>>> example, a4031f6dc0 ("Merge branch 'en/stash-apply-sparse-checkout'
>>> into maint", 2021-02-05) which did this.  The fact that it forks so
>>> many subprocesses is a bug, and comes from the fact that stash is a
>>> partial conversion from shell to C.  I think the subprocess forking is
>>> part of the problem that causes issues for sparse-checkouts, as I
>>> spelled out in patches 2 & 3 of the series I mentioned above.
>>>
>>> However, that doesn't affect this series.
>>>
>>>> Co-authored-by: Derrick Stolee <dstolee@microsoft.com>
>>>> Signed-off-by: Victoria Dye <vdye@github.com>
>>>> ---
>>>>  t/perf/p2000-sparse-operations.sh        |   1 +
>>>>  t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
>>>>  2 files changed, 126 insertions(+)
>>>>
>>>> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
>>>> index 54f8602f3c1..7dbed330160 100755
>>>> --- a/t/perf/p2000-sparse-operations.sh
>>>> +++ b/t/perf/p2000-sparse-operations.sh
>>>> @@ -118,5 +118,6 @@ test_perf_on_all git diff --cached
>>>>  test_perf_on_all git blame $SPARSE_CONE/a
>>>>  test_perf_on_all git blame $SPARSE_CONE/f3/a
>>>>  test_perf_on_all git checkout-index -f --all
>>>> +test_perf_on_all git update-index --add --remove
>>>>
>>>>  test_done
>>>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>>>> index 6ecf1f2bf8e..6804ab23a27 100755
>>>> --- a/t/t1092-sparse-checkout-compatibility.sh
>>>> +++ b/t/t1092-sparse-checkout-compatibility.sh
>>>> @@ -630,6 +630,131 @@ test_expect_success 'reset with wildcard pathspec' '
>>>>         test_all_match git ls-files -s -- folder1
>>>>  '
>>>>
>>>> +test_expect_success 'update-index modify outside sparse definition' '
>>>> +       init_repos &&
>>>> +
>>>> +       write_script edit-contents <<-\EOF &&
>>>> +       echo text >>$1
>>>> +       EOF
>>>> +
>>>> +       # Create & modify folder1/a
>>>> +       run_on_sparse mkdir -p folder1 &&
>>>> +       run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
>>>> +       run_on_all ../edit-contents folder1/a &&
>>>
>>> As I've mentioned to Stolee, I'd rather these were explicitly marked
>>> as intentionally setting up an erroneous condition.
>>>
>>> However, that might not matter if my other series gets accepted (the
>>> one I promised to send out yesterday, but then spent all day
>>> responding to emails instead).  Hopefully I'll send it soon.
>>>
>>>> +
>>>> +       # If file has skip-worktree enabled, update-index does not modify the
>>>> +       # index entry
>>>> +       test_sparse_match git update-index folder1/a &&
>>>> +       test_sparse_match git status --porcelain=v2 &&
>>>> +       test_must_be_empty sparse-checkout-out &&
>>>> +
>>>> +       # When skip-worktree is disabled (even on files outside sparse cone), file
>>>> +       # is updated in the index
>>>> +       test_sparse_match git update-index --no-skip-worktree folder1/a &&
>>>> +       test_all_match git status --porcelain=v2 &&
>>>> +       test_all_match git update-index folder1/a &&
>>>> +       test_all_match git status --porcelain=v2
>>>
>>> These make sense.
>>>
>>>> +'
>>>> +
>>>> +test_expect_success 'update-index --add outside sparse definition' '
>>>> +       init_repos &&
>>>> +
>>>> +       write_script edit-contents <<-\EOF &&
>>>> +       echo text >>$1
>>>> +       EOF
>>>> +
>>>> +       # Create folder1, add new file
>>>> +       run_on_sparse mkdir -p folder1 &&
>>>> +       run_on_all ../edit-contents folder1/b &&
>>>> +
>>>> +       # Similar to `git add`, the untracked out-of-cone file is added to the index
>>>> +       # identically across sparse and non-sparse checkouts
>>>> +       test_all_match git update-index --add folder1/b &&
>>>> +       test_all_match git status --porcelain=v2
>>>
>>> The comment is not correct:
>>>
>>
>> It is correct, but for *untracked* out-of-cone files only. Those files don't
>> have a `skip-worktree` bit because they're not in the index in the first
>> place.
> 
>> The comment is intended to highlight the fact that `update-index`
>> (like `git add`, `git status`, etc.) "decides" whether to operate on a file
>> in a sparse-checkout based on `skip-worktree`, *not* the sparse patterns.
> 
> git-update-index may not pay attention to the sparsity patterns for
> untracked files, but `git add` does.  Let me demonstrate.  First, I
> set up a simple repo where the following is true:
> 
> $ git ls-files -t
> H in-cone/foo.c
> S out-of-cone/tracked
> $ git status --porcelain
> ?? out-of-cone/initially-untracked
> 
> Now, let's compare `git add` and `git add --sparse`.  First, without --sparse:
> 
> $ git add out-of-cone/initially-untracked
> The following paths and/or pathspecs matched paths that exist
> outside of your sparse-checkout definition, so will not be
> updated in the index:
> out-of-cone/initially-untracked
> hint: If you intend to update such entries, try one of the following:
> hint: * Use the --sparse option.
> hint: * Disable or modify the sparsity rules.
> hint: Disable this message with "git config advice.updateSparsePath false"
> $ git ls-files -t
> H in-cone/foo.c
> S out-of-cone/tracked
> 
> So, `git add $UNTRACKED` did not add the file.  In contrast:
> 
> $ git add --sparse out-of-cone/initially-untracked
> $ git ls-files -t
> H in-cone/foo.c
> H out-of-cone/initially-untracked
> S out-of-cone/tracked
> 
> So, `git add --sparse $UNTRACKED` did add it.
> 

Sorry about that - when I wrote the first version of this series in the
`microsoft/git` fork, `git add` hadn't been updated to reject out-of-cone
untracked files as it is in [1]. It's my mistake for not double-checking
that it was still the case, apologies for wasting your time on re-explaining
this.

In any case, I'll update the test comment and commit message per your
suggestion:

>>> I might buy that `git update-index` is lower level and should be
>>> considered the same as `git add --sparse`, but the comment should
>>> mention that and try to sell why update-index should be equivalent to
>>> that instead of to `git add`.

I'm leaning only slightly towards the current behavior (and will update the
comment accordingly), but I'm happy to change it if the reasoning isn't as
strong as that of another approach.

[1] 105e8b014b (add: fail when adding an untracked sparse file, 2021-09-24)

>> Seeing as the comparison to `git add` makes things more confusing, I'll
>> rephrase the test comment.
>>
>>> $ git add out-of-cone/file
>>> The following paths and/or pathspecs matched paths that exist
>>> outside of your sparse-checkout definition, so will not be
>>> updated in the index:
>>> out-of-cone/file
>>> hint: If you intend to update such entries, try one of the following:
>>> hint: * Use the --sparse option.
>>> hint: * Disable or modify the sparsity rules.
>>> hint: Disable this message with "git config advice.updateSparsePath false"
>>>
>>> I might buy that `git update-index` is lower level and should be
>>> considered the same as `git add --sparse`, but the comment should
>>> mention that and try to sell why update-index should be equivalent to
>>> that instead of to `git add`.
>>>
>>
>> Tracked, out-of-cone files aren't affected by `--add` (the flag allows
>> `update-index` to add untracked files), and `update-index out-of-cone/tracked`
>> would ignore the file.
> 
> Yes, I believe you're explaining update-index behavior correctly.
> 
>> so I believe the behavior of `update-index` is
>> currently more consistent with `git add` than `git add --sparse`.
> 
> But not quite `git add`'s.  Just to be clear, let's add update-index
> to the above comparison I did between add and add --sparse.  First,
> let's go back to the initial setup point:
> 
> $ git ls-files -t
> H in-cone/foo.c
> S out-of-cone/tracked
> $ git status --porcelain
> ?? out-of-cone/initially-untracked
> 
> Now, let's try update-index:
> 
> $ git update-index --add out-of-cone/initially-untracked
> $ git ls-files -t
> H in-cone/foo.c
> H out-of-cone/initially-untracked
> S out-of-cone/tracked
> 
> So, in other words, `git update-index --add $UNTRACKED` matches the
> behavior of `git add --sparse $UNTRACKED`, not the behavior of `git
> add $UNTRACKED`.
> 
>>>> +'
>>>> +
>>>> +test_expect_success 'update-index --remove outside sparse definition' '
>>>> +       init_repos &&
>>>
>>>> +
>>>> +       # When `--ignore-skip-worktree-entries` is specified, out-of-cone files are
>>>> +       # not removed from the index if they do not exist on disk
>>>> +       test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
>>>> +       test_all_match git status --porcelain=v2 &&
>>>
>>> The file is present despite being marked to be missing, we're ignoring
>>> the intention of the marking, and we ask for it to be removed, so we
>>> don't remove it?
>>>
>>> The levels of negation are _very_ confusing.  It took me a while to
>>> unravel this.  I think the logic is something like this
>>>   * folder1/a is marked as SKIP_WORKTREE, meaning it's not supposed to
>>> be in the worktree.
>>>   * and it's not.
>>>   * We are stating that we're ignoring SKIP_WORKTREE, though, so this
>>> looks like a regular file that has been deleted.
>>> So, what would `git update-index --remove $FILE` do for a normal $FILE
>>> deleted from the working copy?  According to the docs:
>>>
>>>     --remove
>>>            If a specified file is in the index but is missing then it’s
>>>            removed. Default behavior is to ignore removed file.
>>>
>>> So, the docs say it would remove it.  But you don't.
>>>
>>>
>>> After digging around and looking at the testcase below, if I had to
>>> guess what happened, I would say that you figured out what the
>>> SKIP_WORKTREE behavior was, and assumed that was correct, and added a
>>> flag that allowed you to request the opposite behavior.
>>> Unfortunately, I think the pre-existing behavior is buggy.
>>>
>>
>> I understand why you find it buggy, but I am not making baseless assumptions
>> about the correctness (or lack thereof) of the current behavior.
> 
> To be clear, the fact that the behavior was there for a decade would
> typically be basis enough for an assumption (in my opinion), and I
> wouldn't have faulted folks for making it.  I might well have done so
> myself.  My reasoning was just that I was getting confused by the
> negations and trying to understand the testcase, and when I started to
> unravel it, I found what looked like a possible inconsistency.
> 
> Anyway, it's clear here you've actually dug a lot deeper and know the
> history here.  In contrast, I was making assumptions about the history
> (and ones that weren't correct, though I'd argue my assumptions
> weren't baseless)...
> 

Your assumptions were completely valid, I apologize if my response came off
as implying otherwise. I'll try to use the comments on the tests to clarify
their behavior as much as possible, hopefully reducing some confusion around
all the multiple-negative flags & options.

>> This
>> specific "gap" in `update-index --remove` has been discussed in the past [1]
>> and was acknowledged as non-ideal and a candidate for change in the future.
>> At the time, the introduction of `--ignore-skip-worktree-entries` [2] was a
>> "safe" way to ignore `skip-worktree` without changing the default behavior.
>>
>> Personally, I think updating the default behavior of `--remove` (and
>> corresponding deprecation of `--[no-]ignore-skip-worktree-entries`) is
>> probably the right way to go. However, I'd like to avoid including it in
>> this series because it deviates pretty substantially from the goal
>> "integrate with sparse index", and as a result has the potential to
>> overshadow/derail the rest of the series. If you're alright with (slightly)
>> deferring changes to the behavior of `--remove`, I can submit a separate
>> series for it once this one has stabilized.
>>
>> [1] https://lore.kernel.org/git/xmqq36fda3i8.fsf@gitster-ct.c.googlers.com/
>> [2] https://lore.kernel.org/git/163b42dfa21c306dc1dc573c5edfc8bda5c99fd0.1572432578.git.gitgitgadget@gmail.com/
> 
> Oh, wow.
> 
> Yeah, fixing it in a later series seems fine.  However, could you add
> a comment to these testcases that --remove has behavior that violates
> the definition of `SKIP_WORKTREE`, and that we might want to fix that
> in the future but for now you are just testing the pre-existing
> behavior for coverage?
> 

Will do!

> (There is a possibility we can't fix it for some reason when we dig
> in.  In that case, we should update the documentation for `--remove`
> to call out this special case.  But again, that can be for a later
> series.)
> 
>>> Of course, there's lots of negation here.  Did I get something
>>> backwards by chance?
>>>
>>>> +
>>>> +       # When the flag is _not_ specified ...
>>>
>>> In my head I'm translating this as:
>>>
>>> SKIP_WORKTREE = !worktree
>>> --ignore-skip-worktree-entries = !!worktree
>>> "flag is _not_ specified" = !!!worktree ?
>>>
>>> I'm not sure I would do anything to change it, but just pointing out
>>> it can be a little hard for others to come up to speed.
>>>
>>
>> Most of the confusion likely comes from the non-standard behavior of
>> `--remove`, but I think I can distill it into (somewhat) straightforward
>> statements about `update-index`:
>>
>> 1. When using the command *without* either `--ignore-skip-worktree-entries`
>>    OR `--remove`, `skip-worktree` entries provided to the command are
>>    ignored.
>> 2. When using the command *with* `--remove` and *without*
>>    `--ignore-skip-worktree-entries`, `skip-worktree` entries are *not*
>>    ignored, and are removed from the index.
>> 3. When both `--remove` and `--ignore-skip-worktree-entries` are specified,
>>    `skip-worktree` entries are again ignored.
>> 4. `--ignore-skip-worktree-entries` has no effect without `--remove` also
>>    specified
>>
>> The goal of this test, then, is to exercise conditions 2 & 3 and directly
>> show their effect on `skip-worktree` entries.
> 
> Yeah, it makes sense to have good test coverage.  +1.
> 
>>
>>>>             ...     , out-of-cone, not-on-disk files are
>>>> +       # removed from the index
>>>> +       rm full-checkout/folder1/a &&
>>>> +       test_all_match git update-index --remove folder1/a &&
>>>> +       test_all_match git status --porcelain=v2 &&
>>>
>>> Documentation/git-update-index.txt defines SKIP_WORKTREE as follows:
>>>
>>> "Skip-worktree bit can be defined in one (long) sentence: When reading
>>> an entry, if it is marked as skip-worktree, then Git pretends its
>>> working directory version is up to date and read the index version
>>> instead."
>>>
>>> If Git is pretending the file is up-to-date, and `git update-index
>>> --remove $UP_TO_DATE_FILE` is typically a no-op because the --remove
>>> flag doesn't do anything when a file is present in the working copy,
>>> then why is this the expected behavior?
>>>
>>> I know it's the traditional behavior of update-index, but
>>> SKIP_WORKTREE support in Git has traditionally been filled with holes.
>>> So, was this behavior by design (despite contradicting the
>>> documentation), or by accident?
>>>
>>
>> As far as I can tell, it appears to have been intentional in the original
>> `skip-worktree` implementation [3], but given Junio & Johannes' discussion
>> on the `--ignore-skip-worktree-entries` patch [1], the sentiment now would
>> probably lean towards having `--remove` ignore `skip-worktree`.
>>
>> [1] https://lore.kernel.org/git/xmqq36fda3i8.fsf@gitster-ct.c.googlers.com/
>>     (copied from earlier in this message)
>> [3] https://lore.kernel.org/git/1250776033-12395-5-git-send-email-pclouds@gmail.com/
> 
> Allow me to comment on this history...
> 
> I can see sometimes wanting to make a command line option a special
> case that doesn't follow the general documented rules.  Not sure I
> believe it in this specific case, but I can see it as being a
> possibility in general.  But when you're going to make an option not
> follow the otherwise documented behavior, then that option's
> documentation should have a special callout about how it diverges
> and/or why it's a special case.  So, this seems like a double-layered
> problem to me (not only choosing the wrong behavior, but leaving the
> documentation to claim it does something else).  It looks like Dscho
> partially tried to fix it, but I would have preferred that Dscho's
> documentation comment he added to `--ignore-skip-worktree-entries` be
> added to `--remove`; it's odd that folks wanting to learn about
> `--remove` behavior need to read the documentation for
> `--ignore-skip-worktree-entries` (even if they aren't using it) in
> order to understand `--remove`'s behavior.
> 
>>> (To be fair, I think the definition given in the manual for
>>> SKIP_WORKTREE is horrible for other reasons, so I don't like leaning
>>> on it.  But I used different logic above in the
>>> --ignore-skip-worktree-entries case to arrive at the same conclusion
>>> that the --remove behavior of update-index seems to be backwards.
>>> Unless I missed a negation in both cases somewhere?  There are so many
>>> floating around...)
>>>
>>>> +       # NOTE: --force-remove supercedes --ignore-skip-worktree-entries, removing
>>>> +       # a skip-worktree file from the index (and disk) when both are specified
>>>> +       test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
>>>> +       test_all_match git status --porcelain=v2
>>>
>>> Makes sense.
>>>
>>>> +'
>>>> +
>>>> +test_expect_success 'update-index with directories' '
>>>> +       init_repos &&
>>>> +
>>>> +       # update-index will exit silently when provided with a directory name
>>>> +       # containing a trailing slash
>>>> +       test_all_match git update-index deep/ folder1/ &&
>>>> +       grep "Ignoring path deep/" sparse-checkout-err &&
>>>> +       grep "Ignoring path folder1/" sparse-checkout-err &&
>>>
>>> Is this desired behavior or just current behavior?
>>>
>>>> +
>>>> +       # When update-index is given a directory name WITHOUT a trailing slash, it will
>>>> +       # behave in different ways depending on the status of the directory on disk:
>>>> +       # * if it exists, the command exits with an error ("add individual files instead")
>>>> +       # * if it does NOT exist (e.g., in a sparse-checkout), it is assumed to be a
>>>> +       #   file and either triggers an error ("does not exist  and --remove not passed")
>>>> +       #   or is ignored completely (when using --remove)
>>>> +       test_all_match test_must_fail git update-index deep &&
>>>> +       run_on_all test_must_fail git update-indexe folder1 &&
>>>
>>> This one will fail for the wrong reason, though -- `update-indexe` is
>>> not a valid subcommand.  (extra 'e' at the end)
>>>
>>
>> Thanks for catching that! I'll update in my next re-roll.
>>
>>>> +       test_must_fail git -C full-checkout update-index --remove folder1 &&
>>>> +       test_sparse_match git update-index --remove folder1 &&
>>>> +       test_all_match git status --porcelain=v2
>>>
>>> Otherwise these seem reasonable.
>>>
>>>> +'
>>>> +
>>>> +test_expect_success 'update-index --again file outside sparse definition' '
>>>> +       init_repos &&
>>>> +
>>>> +       write_script edit-contents <<-\EOF &&
>>>> +       echo text >>$1
>>>> +       EOF
>>>
>>> Copy and paste and forget to remove?  edit-contents doesn't seem to be
>>> used in this test.
>>>
>>
>> Will remove.
>>
>>>> +
>>>> +       test_all_match git checkout -b test-reupdate &&
>>>> +
>>>> +       # Update HEAD without modifying the index to introduce a difference in
>>>> +       # folder1/a
>>>> +       test_sparse_match git reset --soft update-folder1 &&
>>>> +
>>>> +       # Because folder1/a differs in the index vs HEAD,
>>>> +       # `git update-index --remove --again` will effectively perform
>>>> +       # `git update-index --remove folder1/a` and remove the folder1/a
>>>> +       test_sparse_match git update-index --remove --again &&
>>>> +       test_sparse_match git status --porcelain=v2
>>>
>>> This might need a --ignore-skip-worktree-entries, as per the
>>> discussion above.  Otherwise, this test makes sense.
>>>
>>
>> The `--ignore-skip-worktree-entries` option is explicitly omitted because
>> this test needs `update-index` to modify a `skip-worktree` entry. However,
>> given the debate around what `--remove` should do, I'll update the scenario
>> to not use `--remove` or any variation of it.
>>
>>>> +'
>>>> +
>>>> +test_expect_success 'update-index --cacheinfo' '
>>>> +       init_repos &&
>>>> +
>>>> +       deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
>>>> +       folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) &&
>>>> +       folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) &&
>>>> +
>>>> +       test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a &&
>>>> +       test_all_match git status --porcelain=v2 &&
>>>> +
>>>> +       # Cannot add sparse directory, even in sparse index case
>>>> +       test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
>>>> +
>>>> +       # Sparse match only - because folder1/a is outside the sparse checkout
>>>> +       # definition (and thus not on-disk), it will appear "deleted" in
>>>> +       # unstaged changes.
>>>> +       test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
>>>> +       test_sparse_match git status --porcelain=v2
>>>
>>> Makes sense, because the update-index command removes the existing
>>> cache entry and adds a new one without the SKIP_WORKTREE bit.  But it
>>> might be worth mentioning that in the commit message.  Also, you could
>>> follow this up with `git update-index --skip-worktree folder1/a`, and
>>> then do a test_all_match git status --porcelain=v2, to show that when
>>> the SKIP_WORKTREE bit is restored back to the file, then it again does
>>> as expected despite not being on-disk.
>>>
>>
>> I really like this - it helps clarify how `update-index` can be used to
>> correctly add a sparse-checkout entry to the index with plumbing commands.
>> I'll include it in V2.
>>
>>>> +'
>>>> +
>>>>  test_expect_success 'merge, cherry-pick, and rebase' '
>>>>         init_repos &&
>>>>
>>>> --
>>>> gitgitgadget

Re: [PATCH 7/9] update-index: add tests for sparse-checkout compatibility

[Elijah Newren]

On Mon, Jan 10, 2022 at 10:01 AM Victoria Dye <vdye@github.com> wrote:
>
> Elijah Newren wrote:
> > On Mon, Jan 10, 2022 at 7:47 AM Victoria Dye <vdye@github.com> wrote:
> >>
> >> Elijah Newren wrote:
> >>> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> >>> <gitgitgadget@gmail.com> wrote:
> >>>>
> >>>> From: Victoria Dye <vdye@github.com>
> >>>>
...
>
> Sorry about that - when I wrote the first version of this series in the
> `microsoft/git` fork, `git add` hadn't been updated to reject out-of-cone
> untracked files as it is in [1]. It's my mistake for not double-checking
> that it was still the case, apologies for wasting your time on re-explaining
> this.

No worries; there's lots of moving parts in the sparse world.

> In any case, I'll update the test comment and commit message per your
> suggestion:
>
> >>> I might buy that `git update-index` is lower level and should be
> >>> considered the same as `git add --sparse`, but the comment should
> >>> mention that and try to sell why update-index should be equivalent to
> >>> that instead of to `git add`.
>
> I'm leaning only slightly towards the current behavior (and will update the
> comment accordingly), but I'm happy to change it if the reasoning isn't as
> strong as that of another approach.
>
> [1] 105e8b014b (add: fail when adding an untracked sparse file, 2021-09-24)

I'm also leaning slightly towards keeping the current behavior and
just updating the comment.

...
> >> I understand why you find it buggy, but I am not making baseless assumptions
> >> about the correctness (or lack thereof) of the current behavior.
> >
> > To be clear, the fact that the behavior was there for a decade would
> > typically be basis enough for an assumption (in my opinion), and I
> > wouldn't have faulted folks for making it.  I might well have done so
> > myself.  My reasoning was just that I was getting confused by the
> > negations and trying to understand the testcase, and when I started to
> > unravel it, I found what looked like a possible inconsistency.
> >
> > Anyway, it's clear here you've actually dug a lot deeper and know the
> > history here.  In contrast, I was making assumptions about the history
> > (and ones that weren't correct, though I'd argue my assumptions
> > weren't baseless)...
> >
>
> Your assumptions were completely valid, I apologize if my response came off
> as implying otherwise. I'll try to use the comments on the tests to clarify
> their behavior as much as possible, hopefully reducing some confusion around
> all the multiple-negative flags & options.

Oh, no, not at all.  I was just worried that my earlier response might
have come across poorly and could be read as criticizing assumptions I
(incorrectly) _thought_ you were making.  I wasn't sure if that was
actually implied in your wording, but just to be careful I was just
trying to specify that I think the level of effort of your submissions
is totally appropriate -- even if you had been making the assumptions
I thought you had been.  And I was pointing out that I, after all, had
made and will continue to make assumptions too.  The point of review
is getting a second pair of eyes, because they can help catch issues;
and finding those issues often includes double-checking unspoken
assumptions.

Anyway, I think we're on the same page.  Thanks for your hard work
here; looking forward to seeing your reroll!

[PATCH 8/9] update-index: integrate with sparse index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Enable usage of the sparse index with `update-index`. Most variations of
`update-index` work without explicitly expanding the index or making any
other updates in or outside of `update-index.c`.

The one usage requiring additional changes is `--cacheinfo`; if a file
inside a sparse directory was specified, the index would not be expanded
until after the cache tree is invalidated, leading to a mismatch between the
index and cache tree. This scenario is handled by rearranging
`add_index_entry_with_check`, allowing `index_name_stage_pos` to expand the
index *before* attempting to invalidate the relevant cache tree path,
avoiding cache tree/index corruption.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/update-index.c                   |  3 +++
 read-cache.c                             | 10 +++++++---
 t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 187203e8bb5..605cc693bbd 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1077,6 +1077,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
+	prepare_repo_settings(r);
+	the_repository->settings.command_requires_full_index = 0;
+
 	/* we will diagnose later if it turns out that we need to update it */
 	newfd = hold_locked_index(&lock_file, 0);
 	if (newfd < 0)
diff --git a/read-cache.c b/read-cache.c
index cbe73f14e5e..b4600e954b6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1339,9 +1339,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
 	int new_only = option & ADD_CACHE_NEW_ONLY;
 
-	if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
-		cache_tree_invalidate_path(istate, ce->name);
-
 	/*
 	 * If this entry's path sorts after the last entry in the index,
 	 * we can avoid searching for it.
@@ -1352,6 +1349,13 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	else
 		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), EXPAND_SPARSE);
 
+	/*
+	 * Cache tree path should be invalidated only after index_name_stage_pos,
+	 * in case it expands a sparse index.
+	 */
+	if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
+		cache_tree_invalidate_path(istate, ce->name);
+
 	/* existing match? Just replace it. */
 	if (pos >= 0) {
 		if (!new_only)
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 6804ab23a27..bc0741c970d 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1216,6 +1216,18 @@ test_expect_success 'sparse index is not expanded: blame' '
 	done
 '
 
+test_expect_success 'sparse index is not expanded: update-index' '
+	init_repos &&
+
+	echo "test" >sparse-index/README.md &&
+	echo "test2" >sparse-index/a &&
+	rm -f sparse-index/deep/a &&
+
+	ensure_not_expanded update-index --add README.md &&
+	ensure_not_expanded update-index a &&
+	ensure_not_expanded update-index --remove deep/a
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

Re: [PATCH 8/9] update-index: integrate with sparse index

[Elijah Newren]

On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Victoria Dye <vdye@github.com>
>
> Enable usage of the sparse index with `update-index`. Most variations of
> `update-index` work without explicitly expanding the index or making any
> other updates in or outside of `update-index.c`.
>
> The one usage requiring additional changes is `--cacheinfo`; if a file
> inside a sparse directory was specified, the index would not be expanded
> until after the cache tree is invalidated, leading to a mismatch between the
> index and cache tree. This scenario is handled by rearranging
> `add_index_entry_with_check`, allowing `index_name_stage_pos` to expand the
> index *before* attempting to invalidate the relevant cache tree path,
> avoiding cache tree/index corruption.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/update-index.c                   |  3 +++
>  read-cache.c                             | 10 +++++++---
>  t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++++
>  3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 187203e8bb5..605cc693bbd 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1077,6 +1077,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>
>         git_config(git_default_config, NULL);
>
> +       prepare_repo_settings(r);
> +       the_repository->settings.command_requires_full_index = 0;
> +
>         /* we will diagnose later if it turns out that we need to update it */
>         newfd = hold_locked_index(&lock_file, 0);
>         if (newfd < 0)
> diff --git a/read-cache.c b/read-cache.c
> index cbe73f14e5e..b4600e954b6 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1339,9 +1339,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
>         int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
>         int new_only = option & ADD_CACHE_NEW_ONLY;
>
> -       if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
> -               cache_tree_invalidate_path(istate, ce->name);
> -
>         /*
>          * If this entry's path sorts after the last entry in the index,
>          * we can avoid searching for it.
> @@ -1352,6 +1349,13 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
>         else
>                 pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), EXPAND_SPARSE);
>
> +       /*
> +        * Cache tree path should be invalidated only after index_name_stage_pos,
> +        * in case it expands a sparse index.
> +        */
> +       if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
> +               cache_tree_invalidate_path(istate, ce->name);
> +
>         /* existing match? Just replace it. */
>         if (pos >= 0) {
>                 if (!new_only)
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 6804ab23a27..bc0741c970d 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1216,6 +1216,18 @@ test_expect_success 'sparse index is not expanded: blame' '
>         done
>  '
>
> +test_expect_success 'sparse index is not expanded: update-index' '
> +       init_repos &&
> +
> +       echo "test" >sparse-index/README.md &&
> +       echo "test2" >sparse-index/a &&
> +       rm -f sparse-index/deep/a &&
> +
> +       ensure_not_expanded update-index --add README.md &&
> +       ensure_not_expanded update-index a &&
> +       ensure_not_expanded update-index --remove deep/a
> +'

The commit message said this change was about --cacheinfo, but this
test doesn't use that option.  I'm confused; was this a bad patch
splitting by chance?

> +
>  # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>  # in this scenario, but it shouldn't.
>  test_expect_success 'reset mixed and checkout orphan' '
> --
> gitgitgadget
>

Re: [PATCH 8/9] update-index: integrate with sparse index

[Victoria Dye]

Elijah Newren wrote:
> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Victoria Dye <vdye@github.com>
>>
>> Enable usage of the sparse index with `update-index`. Most variations of
>> `update-index` work without explicitly expanding the index or making any
>> other updates in or outside of `update-index.c`.
>>
>> The one usage requiring additional changes is `--cacheinfo`; if a file
>> inside a sparse directory was specified, the index would not be expanded
>> until after the cache tree is invalidated, leading to a mismatch between the
>> index and cache tree. This scenario is handled by rearranging
>> `add_index_entry_with_check`, allowing `index_name_stage_pos` to expand the
>> index *before* attempting to invalidate the relevant cache tree path,
>> avoiding cache tree/index corruption.
>>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  builtin/update-index.c                   |  3 +++
>>  read-cache.c                             | 10 +++++++---
>>  t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++++
>>  3 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index 187203e8bb5..605cc693bbd 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1077,6 +1077,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>>
>>         git_config(git_default_config, NULL);
>>
>> +       prepare_repo_settings(r);
>> +       the_repository->settings.command_requires_full_index = 0;
>> +
>>         /* we will diagnose later if it turns out that we need to update it */
>>         newfd = hold_locked_index(&lock_file, 0);
>>         if (newfd < 0)
>> diff --git a/read-cache.c b/read-cache.c
>> index cbe73f14e5e..b4600e954b6 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1339,9 +1339,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
>>         int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
>>         int new_only = option & ADD_CACHE_NEW_ONLY;
>>
>> -       if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
>> -               cache_tree_invalidate_path(istate, ce->name);
>> -
>>         /*
>>          * If this entry's path sorts after the last entry in the index,
>>          * we can avoid searching for it.
>> @@ -1352,6 +1349,13 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
>>         else
>>                 pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), EXPAND_SPARSE);
>>
>> +       /*
>> +        * Cache tree path should be invalidated only after index_name_stage_pos,
>> +        * in case it expands a sparse index.
>> +        */
>> +       if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
>> +               cache_tree_invalidate_path(istate, ce->name);
>> +
>>         /* existing match? Just replace it. */
>>         if (pos >= 0) {
>>                 if (!new_only)
>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>> index 6804ab23a27..bc0741c970d 100755
>> --- a/t/t1092-sparse-checkout-compatibility.sh
>> +++ b/t/t1092-sparse-checkout-compatibility.sh
>> @@ -1216,6 +1216,18 @@ test_expect_success 'sparse index is not expanded: blame' '
>>         done
>>  '
>>
>> +test_expect_success 'sparse index is not expanded: update-index' '
>> +       init_repos &&
>> +
>> +       echo "test" >sparse-index/README.md &&
>> +       echo "test2" >sparse-index/a &&
>> +       rm -f sparse-index/deep/a &&
>> +
>> +       ensure_not_expanded update-index --add README.md &&
>> +       ensure_not_expanded update-index a &&
>> +       ensure_not_expanded update-index --remove deep/a
>> +'
> 
> The commit message said this change was about --cacheinfo, but this
> test doesn't use that option.  I'm confused; was this a bad patch
> splitting by chance?
> 

It was not - the commit message title is "update-index: integrate with
sparse index" and the message starts by saying that this patch enables use
of the sparse index for *all* of `update-index` (where "[m]ost variations of
`update-index` work without...making any other updates in or outside of
`update-index.c`").

It goes on to say that the `--cache-info` option is an exception to the
above statement (that most variations work without updates outside
`update-index.c`) because it requires a slight change to
`add_index_entry_with_check(...)` to avoid index corruption. That change is
also in this patch, but it's not the main "focus".

The test added here is intended to broadly test `update-index` with a sparse
index. I'll add a case for `--cacheinfo` in my next re-roll but, for
context, the reason I didn't originally is because I focused on the (as far
as I could tell) most commonly-used variations of `update-index`.

>> +
>>  # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>>  # in this scenario, but it shouldn't.
>>  test_expect_success 'reset mixed and checkout orphan' '
>> --
>> gitgitgadget
>>

Re: [PATCH 8/9] update-index: integrate with sparse index

[Elijah Newren]

On Mon, Jan 10, 2022 at 6:10 AM Victoria Dye <vdye@github.com> wrote:
>
> Elijah Newren wrote:
> > On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Victoria Dye <vdye@github.com>
> >>
> >> Enable usage of the sparse index with `update-index`. Most variations of
> >> `update-index` work without explicitly expanding the index or making any
> >> other updates in or outside of `update-index.c`.
> >>
> >> The one usage requiring additional changes is `--cacheinfo`; if a file
> >> inside a sparse directory was specified, the index would not be expanded
> >> until after the cache tree is invalidated, leading to a mismatch between the
> >> index and cache tree. This scenario is handled by rearranging
> >> `add_index_entry_with_check`, allowing `index_name_stage_pos` to expand the
> >> index *before* attempting to invalidate the relevant cache tree path,
> >> avoiding cache tree/index corruption.
> >>
> >> Signed-off-by: Victoria Dye <vdye@github.com>
> >> ---
> >>  builtin/update-index.c                   |  3 +++
> >>  read-cache.c                             | 10 +++++++---
> >>  t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++++
> >>  3 files changed, 22 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/builtin/update-index.c b/builtin/update-index.c
> >> index 187203e8bb5..605cc693bbd 100644
> >> --- a/builtin/update-index.c
> >> +++ b/builtin/update-index.c
> >> @@ -1077,6 +1077,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> >>
> >>         git_config(git_default_config, NULL);
> >>
> >> +       prepare_repo_settings(r);
> >> +       the_repository->settings.command_requires_full_index = 0;
> >> +
> >>         /* we will diagnose later if it turns out that we need to update it */
> >>         newfd = hold_locked_index(&lock_file, 0);
> >>         if (newfd < 0)
> >> diff --git a/read-cache.c b/read-cache.c
> >> index cbe73f14e5e..b4600e954b6 100644
> >> --- a/read-cache.c
> >> +++ b/read-cache.c
> >> @@ -1339,9 +1339,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
> >>         int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
> >>         int new_only = option & ADD_CACHE_NEW_ONLY;
> >>
> >> -       if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
> >> -               cache_tree_invalidate_path(istate, ce->name);
> >> -
> >>         /*
> >>          * If this entry's path sorts after the last entry in the index,
> >>          * we can avoid searching for it.
> >> @@ -1352,6 +1349,13 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
> >>         else
> >>                 pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), EXPAND_SPARSE);
> >>
> >> +       /*
> >> +        * Cache tree path should be invalidated only after index_name_stage_pos,
> >> +        * in case it expands a sparse index.
> >> +        */
> >> +       if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
> >> +               cache_tree_invalidate_path(istate, ce->name);
> >> +
> >>         /* existing match? Just replace it. */
> >>         if (pos >= 0) {
> >>                 if (!new_only)
> >> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> >> index 6804ab23a27..bc0741c970d 100755
> >> --- a/t/t1092-sparse-checkout-compatibility.sh
> >> +++ b/t/t1092-sparse-checkout-compatibility.sh
> >> @@ -1216,6 +1216,18 @@ test_expect_success 'sparse index is not expanded: blame' '
> >>         done
> >>  '
> >>
> >> +test_expect_success 'sparse index is not expanded: update-index' '
> >> +       init_repos &&
> >> +
> >> +       echo "test" >sparse-index/README.md &&
> >> +       echo "test2" >sparse-index/a &&
> >> +       rm -f sparse-index/deep/a &&
> >> +
> >> +       ensure_not_expanded update-index --add README.md &&
> >> +       ensure_not_expanded update-index a &&
> >> +       ensure_not_expanded update-index --remove deep/a
> >> +'
> >
> > The commit message said this change was about --cacheinfo, but this
> > test doesn't use that option.  I'm confused; was this a bad patch
> > splitting by chance?
> >
>
> It was not - the commit message title is "update-index: integrate with
> sparse index" and the message starts by saying that this patch enables use
> of the sparse index for *all* of `update-index` (where "[m]ost variations of
> `update-index` work without...making any other updates in or outside of
> `update-index.c`").
>
> It goes on to say that the `--cache-info` option is an exception to the
> above statement (that most variations work without updates outside
> `update-index.c`) because it requires a slight change to
> `add_index_entry_with_check(...)` to avoid index corruption. That change is
> also in this patch, but it's not the main "focus".
>
> The test added here is intended to broadly test `update-index` with a sparse
> index. I'll add a case for `--cacheinfo` in my next re-roll but, for
> context, the reason I didn't originally is because I focused on the (as far
> as I could tell) most commonly-used variations of `update-index`.

Ah, I see.  I didn't read closely enough and then saw the majority of
the commit message and code changes were about --cacheinfo, but the
testcase wasn't.  Sorry about that.

> >> +
> >>  # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
> >>  # in this scenario, but it shouldn't.
> >>  test_expect_success 'reset mixed and checkout orphan' '
> >> --
> >> gitgitgadget
> >>

[PATCH 9/9] update-index: reduce scope of index expansion in do_reupdate

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Expand the full index (and redo reupdate operation) only if a sparse
directory in the index differs from HEAD. Only the index entries that differ
between the index and HEAD are updated when performing `git update-index
--again`, so unmodified sparse directories are safely skipped. The index
does need to be expanded when sparse directories contain changes, though,
because `update_one(...)` will not operate on sparse directory index
entries.

Because the index should only be expanded if a sparse directory is modified,
add a test ensuring the index is not expanded when differences only exist
within the sparse cone.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/update-index.c                   | 14 +++++++++++---
 t/t1092-sparse-checkout-compatibility.sh |  5 ++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 605cc693bbd..52ecc714d99 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -606,7 +606,7 @@ static struct cache_entry *read_one_ent(const char *which,
 			error("%s: not in %s branch.", path, which);
 		return NULL;
 	}
-	if (mode == S_IFDIR) {
+	if (!the_index.sparse_index && mode == S_IFDIR) {
 		if (which)
 			error("%s: not a blob in %s branch.", path, which);
 		return NULL;
@@ -743,8 +743,6 @@ static int do_reupdate(int ac, const char **av,
 		 */
 		has_head = 0;
  redo:
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(&the_index);
 	for (pos = 0; pos < active_nr; pos++) {
 		const struct cache_entry *ce = active_cache[pos];
 		struct cache_entry *old = NULL;
@@ -761,6 +759,16 @@ static int do_reupdate(int ac, const char **av,
 			discard_cache_entry(old);
 			continue; /* unchanged */
 		}
+
+		/* At this point, we know the contents of the sparse directory are
+		 * modified with respect to HEAD, so we expand the index and restart
+		 * to process each path individually
+		 */
+		if (S_ISSPARSEDIR(ce->ce_mode)) {
+			ensure_full_index(&the_index);
+			goto redo;
+		}
+
 		/* Be careful.  The working tree may not have the
 		 * path anymore, in which case, under 'allow_remove',
 		 * or worse yet 'allow_replace', active_nr may decrease.
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index bc0741c970d..0863c9747c4 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1225,7 +1225,10 @@ test_expect_success 'sparse index is not expanded: update-index' '
 
 	ensure_not_expanded update-index --add README.md &&
 	ensure_not_expanded update-index a &&
-	ensure_not_expanded update-index --remove deep/a
+	ensure_not_expanded update-index --remove deep/a &&
+
+	ensure_not_expanded reset --soft update-deep &&
+	ensure_not_expanded update-index --add --remove --again
 '
 
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
-- 
gitgitgadget

Re: [PATCH 9/9] update-index: reduce scope of index expansion in do_reupdate

[Elijah Newren]

On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Victoria Dye <vdye@github.com>
>
> Expand the full index (and redo reupdate operation) only if a sparse
> directory in the index differs from HEAD.

This was a bit hard to parse and follow for me.  Perhaps:

Avoid pre-emptively expanding to a full index in update-index's do_reupdate().
However, if a sparse directory in the index differs from HEAD, we will need to
then expand the index and restart the operation.

> Only the index entries that differ
> between the index and HEAD are updated when performing `git update-index
> --again`, so unmodified sparse directories are safely skipped. The index
> does need to be expanded when sparse directories contain changes, though,
> because `update_one(...)` will not operate on sparse directory index
> entries.
>
> Because the index should only be expanded if a sparse directory is modified,
> add a test ensuring the index is not expanded when differences only exist
> within the sparse cone.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/update-index.c                   | 14 +++++++++++---
>  t/t1092-sparse-checkout-compatibility.sh |  5 ++++-
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 605cc693bbd..52ecc714d99 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -606,7 +606,7 @@ static struct cache_entry *read_one_ent(const char *which,
>                         error("%s: not in %s branch.", path, which);
>                 return NULL;
>         }
> -       if (mode == S_IFDIR) {
> +       if (!the_index.sparse_index && mode == S_IFDIR) {
>                 if (which)
>                         error("%s: not a blob in %s branch.", path, which);
>                 return NULL;
> @@ -743,8 +743,6 @@ static int do_reupdate(int ac, const char **av,
>                  */
>                 has_head = 0;
>   redo:
> -       /* TODO: audit for interaction with sparse-index. */
> -       ensure_full_index(&the_index);
>         for (pos = 0; pos < active_nr; pos++) {
>                 const struct cache_entry *ce = active_cache[pos];
>                 struct cache_entry *old = NULL;
> @@ -761,6 +759,16 @@ static int do_reupdate(int ac, const char **av,
>                         discard_cache_entry(old);
>                         continue; /* unchanged */
>                 }
> +
> +               /* At this point, we know the contents of the sparse directory are
> +                * modified with respect to HEAD, so we expand the index and restart
> +                * to process each path individually
> +                */
> +               if (S_ISSPARSEDIR(ce->ce_mode)) {
> +                       ensure_full_index(&the_index);
> +                       goto redo;
> +               }
> +
>                 /* Be careful.  The working tree may not have the
>                  * path anymore, in which case, under 'allow_remove',
>                  * or worse yet 'allow_replace', active_nr may decrease.
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index bc0741c970d..0863c9747c4 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1225,7 +1225,10 @@ test_expect_success 'sparse index is not expanded: update-index' '
>
>         ensure_not_expanded update-index --add README.md &&
>         ensure_not_expanded update-index a &&
> -       ensure_not_expanded update-index --remove deep/a
> +       ensure_not_expanded update-index --remove deep/a &&
> +
> +       ensure_not_expanded reset --soft update-deep &&
> +       ensure_not_expanded update-index --add --remove --again
>  '
>
>  # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
> --
> gitgitgadget

I think the rest makes sense.  Thanks for working on these!

Re: [PATCH 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index'

[Elijah Newren]

On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This series continues the work to integrate commands with the sparse index,
> adding integrations with 'git clean', 'git checkout-index', and 'git
> update-index'. These three commands, while useful in their own right, are
> updated mainly because they're used in 'git stash'. A future series will
> integrate sparse index with 'stash' directly, but its subcommands must be
> integrated to avoid the performance cost of each one expanding and
> collapsing the index.

Thanks for working on these; it's very nice to see others continuing
to work on sparse-checkout issues, since it still feels somewhat
incomplete to me.  This work is perhaps less glamorous since you're
starting to work on commands less frequently used by users, but I
still feel it's very important.  Thanks for your diligence.

> The series is broken up into 4 parts:
>
>  * Patches 1-2 are minor fixups to the 'git reset' sparse index integration
>    in response to discussion [1] that came after the series was ready for
>    merge to 'next'.

These look good.

>  * Patch 3 integrates 'git clean' with the sparse index.

Yeah, since clean tends to work with untracked files instead of
tracked, and SKIP_WORKTREE is all about tracked files, I'd expect the
conversion would be simple.  Nice to see verification that it was.

>  * Patches 4-6 integrate 'git checkout-index' with the sparse index and
>    introduce a new '--ignore-skip-worktree-bits' option for use with 'git
>    checkout-index --all'.
>    * This involves changing the behavior of '--all' to respect
>      'skip-worktree' by default (i.e., it won't check out 'skip-worktree'
>      files). The '--ignore-skip-worktree-bits' option can be specified to
>      force checkout of 'skip-worktree' files, if desired.

I think the basic idea makes sense, and the patches are certainly
moving in the right direction.

If others agree with the series I'm about to submit, then patch 4
looks fine as-is and there's just a minor item about
--ignore-skip-worktree-bits without --all for checkout-index in Patch
5 (either further behavior changes, or a more detailed commit
message).  If others disagree with that other series, then I think
both patches 4 & 5 should do more work to avoid problems with
present-despite-SKIP_WORKTREE files.

However, the issues raised in Patch 7 suggest that Patch 5 might need
some reworking (independent of the other series I'm about to submit).

Patch 6 looks good either way.

>  * Patches 7-9 integrate 'git update-index' with the sparse index.
>    * Note that, although this integrates the sparse index with
>      '--cacheinfo', sparse directories still cannot be updated using that
>      option (see the prior discussion [2] for more details on why)

Patch 9 might benefit from a small commit message rewording.

I either misunderstood Patch 8, or it has a disconnect of some kind
between the commit message and the testcase.

I've left various comments on patch 7; it'll probably need updates and
likely points to further changes needed in patch 5.


Anyway, I tend to concentrate on perceived issues when reviewing
rather than on what looks good, but despite the fact that I think
there are some further improvements needed, overall I think this looks
like some nice work.  Keep it up!

[PATCH v2 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index'

[Victoria Dye via GitGitGadget]

This series continues the work to integrate commands with the sparse index,
adding integrations with 'git clean', 'git checkout-index', and 'git
update-index'. These three commands, while useful in their own right, are
updated mainly because they're used in 'git stash'. A future series will
integrate sparse index with 'stash' directly, but its subcommands must be
integrated to avoid the performance cost of each one expanding and
collapsing the index.

The series is broken up into 4 parts:

 * Patches 1-2 are minor fixups to the 'git reset' sparse index integration
   in response to discussion [1] that came after the series was ready for
   merge to 'next'.
 * Patch 3 integrates 'git clean' with the sparse index.
 * Patches 4-6 integrate 'git checkout-index' with the sparse index and
   introduce a new '--ignore-skip-worktree-bits' option.
   * This involves changing the behavior of 'checkout-index' to respect
     'skip-worktree' by default (i.e., it won't check out 'skip-worktree'
     files). The '--ignore-skip-worktree-bits' option can be specified to
     force checkout of 'skip-worktree' files, if desired.
 * Patches 7-9 integrate 'git update-index' with the sparse index.
   * Note that, although this integrates the sparse index with
     '--cacheinfo', sparse directories still cannot be updated using that
     option (see the prior discussion [2] for more details on why)


Changes since V1
================

 * Changed 'checkout-index' to fail by default when given filenames of files
   with 'skip-worktree' enabled
   * These files can still be forcibly checked-out by using the
     '--ignore-skip-worktree-bits' option
   * Added/updated corresponding t1092 tests
 * Updated t1092 'update-index' tests
   * Mentioned where/why 'skip-worktree' files were manually created on-disk
     for testing purposes
   * Provided explanation as to what '--remove' does, and how it relates to
     '--ignore-skip-worktree-entries'; restructured corresponding test
   * Fixed typo 'update-indexe' -> 'update-index'
   * Removed unused 'edit-contents'
   * Changed '--again' test to not use '--remove' to avoid confusion over
     how/why it updates 'skip-worktree' entries
   * Added "set skip-worktree" step to '--cacheinfo' test to illustrate how
     it could be used to add a new outside-of-cone file and remain generally
     compliant with a sparse-checkout definition
   * Added '--cacheinfo' test to "ensure not expanded"
   * Moved t1092 test 'sparse index is not expanded: update-index' to avoid
     merge conflict
 * Updated p2000 test for 'update-index': added file argument
   * Without any file arguments, 'update-index' was effectively a no-op
 * Clarified reasoning behind changing/not changing behavior of update-index
   in sparse-checkouts

Thanks!

 * Victoria

[1]
https://lore.kernel.org/git/CABPp-BG0iDHf268UAnRyA=0y0T69YTc+bLMdxCmSbrL8s=9ziA@mail.gmail.com/

[2]
https://lore.kernel.org/git/a075091c-d0d4-db5d-fa21-c9d6c90c343e@gmail.com/

Victoria Dye (9):
  reset: fix validation in sparse index test
  reset: reorder wildcard pathspec conditions
  clean: integrate with sparse index
  checkout-index: expand sparse checkout compatibility tests
  checkout-index: add --ignore-skip-worktree-bits option
  checkout-index: integrate with sparse index
  update-index: add tests for sparse-checkout compatibility
  update-index: integrate with sparse index
  update-index: reduce scope of index expansion in do_reupdate

 Documentation/git-checkout-index.txt     |  10 +-
 builtin/checkout-index.c                 |  41 +++-
 builtin/clean.c                          |   3 +
 builtin/reset.c                          |  12 +-
 builtin/update-index.c                   |  17 +-
 read-cache.c                             |  10 +-
 t/perf/p2000-sparse-operations.sh        |   2 +
 t/t1092-sparse-checkout-compatibility.sh | 282 ++++++++++++++++++++++-
 8 files changed, 360 insertions(+), 17 deletions(-)


base-commit: dcc0cd074f0c639a0df20461a301af6d45bd582e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1109%2Fvdye%2Fsparse%2Fupdate-index_checkout-index_clean-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1109/vdye/sparse/update-index_checkout-index_clean-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1109

Range-diff vs v1:

  1:  eefb6ab4c61 =  1:  eefb6ab4c61 reset: fix validation in sparse index test
  2:  0194d894c2f =  2:  0194d894c2f reset: reorder wildcard pathspec conditions
  3:  52aec13d18e =  3:  52aec13d18e clean: integrate with sparse index
  4:  e6a8671f6be !  4:  d964507fdcc checkout-index: expand sparse checkout compatibility tests
     @@ Commit message
      
          Add tests to cover `checkout-index`, with a focus on cases interesting in a
          sparse checkout (e.g., files specified outside sparse checkout definition).
     -    New tests are intended to serve as a baseline for expected behavior and
     -    performance when integrating `checkout-index` with the sparse index.
     +
     +    New tests are intended to serve as a baseline for existing and/or expected
     +    behavior and performance when integrating `checkout-index` with the sparse
     +    index. Note that the test 'checkout-index --all' is marked as
     +    'test_expect_failure', indicating that `update-index --all` will be modified
     +    in a subsequent patch to behave as the test expects.
      
          Signed-off-by: Victoria Dye <vdye@github.com>
      
  5:  ec9a751e8dc !  5:  601888606d1 checkout-index: add --ignore-skip-worktree-bits option
     @@ Metadata
       ## Commit message ##
          checkout-index: add --ignore-skip-worktree-bits option
      
     -    Update `checkout-index --all` to no longer refresh files that have the
     -    `skip-worktree` bit set. The newly-added `--ignore-skip-worktree-bits`
     -    option, when used with `--all`, maintains the old behavior and checks out
     -    all files regardless of `skip-worktree`.
     +    Update `checkout-index` to no longer refresh files that have the
     +    `skip-worktree` bit set, exiting with an error if `skip-worktree` filenames
     +    are directly provided to `checkout-index`. The newly-added
     +    `--ignore-skip-worktree-bits` option provides a mechanism to replicate the
     +    old behavior, checking out *all* files specified (even those with
     +    `skip-worktree` enabled).
      
          The ability to toggle whether files should be checked-out based on
          `skip-worktree` already exists in `git checkout` and `git restore` (both of
     -    which have an `--ignore-skip-worktree-bits` option). Adding the option to
     -    `checkout-index` (and changing the corresponding default behavior to respect
     -    the `skip-worktree` bit) is especially helpful for sparse-checkout: it
     -    prevents inadvertent creation of *all* files outside the sparse definition
     -    on disk and eliminates the need to expand a sparse index by default when
     -    using the `--all` option.
     +    which have an `--ignore-skip-worktree-bits` option). The change to, by
     +    default, ignore `skip-worktree` files is especially helpful for
     +    sparse-checkout; it prevents inadvertent creation of files outside the
     +    sparse definition on disk and eliminates the need to expand a sparse index
     +    when using the `--all` option.
      
          Internal usage of `checkout-index` in `git stash` and `git filter-branch` do
          not make explicit use of files with `skip-worktree` enabled, so
          `--ignore-skip-worktree-bits` is not added to them.
      
     +    Helped-by: Elijah Newren <newren@gmail.com>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## Documentation/git-checkout-index.txt ##
     @@ Documentation/git-checkout-index.txt: OPTIONS
       
      +--ignore-skip-worktree-bits::
      +	Check out all files, including those with the skip-worktree bit
     -+	set. Note: may only be used with `--all`; skip-worktree is
     -+	ignored when explicit filenames are specified.
     ++	set.
      +
       --stdin::
       	Instead of taking list of paths from the command line,
     @@ builtin/checkout-index.c
       #include "lockfile.h"
       #include "quote.h"
       #include "cache-tree.h"
     +@@
     + #define CHECKOUT_ALL 4
     + static int nul_term_line;
     + static int checkout_stage; /* default to checkout stage0 */
     ++static int ignore_skip_worktree; /* default to 0 */
     + static int to_tempfile;
     + static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
     + 
      @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
     - 	return -1;
     - }
     + 	int namelen = strlen(name);
     + 	int pos = cache_name_pos(name, namelen);
     + 	int has_same_name = 0;
     ++	int is_skipped = 1;
     + 	int did_checkout = 0;
     + 	int errs = 0;
       
     --static int checkout_all(const char *prefix, int prefix_length)
     -+static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_worktree)
     - {
     - 	int i, errs = 0;
     - 	struct cache_entry *last_ce = NULL;
     +@@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
     + 			break;
     + 		has_same_name = 1;
     + 		pos++;
     ++		if (!ignore_skip_worktree && ce_skip_worktree(ce))
     ++			break;
     ++		is_skipped = 0;
     + 		if (ce_stage(ce) != checkout_stage
     + 		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
     + 			continue;
     +@@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
     + 		fprintf(stderr, "git checkout-index: %s ", name);
     + 		if (!has_same_name)
     + 			fprintf(stderr, "is not in the cache");
     ++		else if (is_skipped)
     ++			fprintf(stderr, "has skip-worktree enabled; "
     ++					"use '--ignore-skip-worktree-bits' to checkout");
     + 		else if (checkout_stage)
     + 			fprintf(stderr, "does not exist at stage %d",
     + 				checkout_stage);
      @@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix_length)
       	ensure_full_index(&the_index);
       	for (i = 0; i < active_nr ; i++) {
     @@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix
       		if (ce_stage(ce) != checkout_stage
       		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
       			continue;
     -@@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
     - 	int i;
     - 	struct lock_file lock_file = LOCK_INIT;
     - 	int all = 0;
     -+	int ignore_skip_worktree = 0;
     - 	int read_from_stdin = 0;
     - 	int prefix_length;
     - 	int force = 0, quiet = 0, not_new = 0;
      @@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
       	struct option builtin_checkout_index_options[] = {
       		OPT_BOOL('a', "all", &all,
     @@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, co
       		OPT__FORCE(&force, N_("force overwrite of existing files"), 0),
       		OPT__QUIET(&quiet,
       			N_("no warning for existing files and files not in index")),
     -@@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
     +
     + ## t/t1092-sparse-checkout-compatibility.sh ##
     +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout-index inside sparse definition' '
     + test_expect_success 'checkout-index outside sparse definition' '
     + 	init_repos &&
       
     - 		if (all)
     - 			die("git checkout-index: don't mix '--all' and explicit filenames");
     -+		if (ignore_skip_worktree)
     -+			die("git checkout-index: don't mix '--ignore-skip-worktree-bits' and explicit filenames");
     - 		if (read_from_stdin)
     - 			die("git checkout-index: don't mix '--stdin' and explicit filenames");
     - 		p = prefix_path(prefix, prefix_length, arg);
     -@@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
     - 	}
     +-	# File does not exist on disk yet for sparse checkouts, so checkout-index
     +-	# succeeds without -f
     +-	test_sparse_match git checkout-index -- folder1/a &&
     ++	# Without --ignore-skip-worktree-bits, outside-of-cone files will trigger
     ++	# an error
     ++	test_sparse_match test_must_fail git checkout-index -- folder1/a &&
     ++	test_i18ngrep "folder1/a has skip-worktree enabled" sparse-checkout-err &&
     ++	test_path_is_missing folder1/a &&
     ++
     ++	# With --ignore-skip-worktree-bits, outside-of-cone files are checked out
     ++	test_sparse_match git checkout-index --ignore-skip-worktree-bits -- folder1/a &&
     + 	test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
     + 	test_cmp sparse-checkout/folder1/a full-checkout/folder1/a &&
       
     - 	if (all)
     --		err |= checkout_all(prefix, prefix_length);
     -+		err |= checkout_all(prefix, prefix_length, ignore_skip_worktree);
     +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout-index outside sparse definition' '
     + 	run_on_sparse mkdir -p folder1 &&
     + 	run_on_all cp ../new-a folder1/a &&
       
     - 	if (pc_workers > 1)
     - 		err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
     -
     - ## t/t1092-sparse-checkout-compatibility.sh ##
     +-	test_all_match test_must_fail git checkout-index -- folder1/a &&
     +-	test_all_match git checkout-index -f -- folder1/a &&
     ++	test_all_match test_must_fail git checkout-index --ignore-skip-worktree-bits -- folder1/a &&
     ++	test_all_match git checkout-index -f --ignore-skip-worktree-bits -- folder1/a &&
     + 	test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
     + 	test_cmp sparse-checkout/folder1/a full-checkout/folder1/a
     + '
      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout-index with folders' '
       	test_all_match test_must_fail git checkout-index -f -- folder1/
       '
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout-index wi
      -	test_sparse_match test_path_is_missing folder1
      +	test_sparse_match test_path_is_missing folder1 &&
      +
     ++	# --ignore-skip-worktree-bits will cause `skip-worktree` files to be
     ++	# checked out, causing the outside-of-cone `folder1` to exist on-disk
      +	test_all_match git checkout-index --ignore-skip-worktree-bits --all &&
      +	test_all_match test_path_exists folder1
       '
  6:  18c00fc9dd3 !  6:  b4b9086dcdc checkout-index: integrate with sparse index
     @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char
       	int pos = cache_name_pos(name, namelen);
       	int has_same_name = 0;
      +	int is_file = 0;
     + 	int is_skipped = 1;
       	int did_checkout = 0;
       	int errs = 0;
     - 
      @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
       			break;
       		has_same_name = 1;
     @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char
      +		if (S_ISSPARSEDIR(ce->ce_mode))
      +			break;
      +		is_file = 1;
     - 		if (ce_stage(ce) != checkout_stage
     - 		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
     - 			continue;
     + 		if (!ignore_skip_worktree && ce_skip_worktree(ce))
     + 			break;
     + 		is_skipped = 0;
      @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
       		fprintf(stderr, "git checkout-index: %s ", name);
       		if (!has_same_name)
       			fprintf(stderr, "is not in the cache");
      +		else if (!is_file)
      +			fprintf(stderr, "is a sparse directory");
     - 		else if (checkout_stage)
     - 			fprintf(stderr, "does not exist at stage %d",
     - 				checkout_stage);
     -@@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_w
     + 		else if (is_skipped)
     + 			fprintf(stderr, "has skip-worktree enabled; "
     + 					"use '--ignore-skip-worktree-bits' to checkout");
     +@@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix_length)
       	int i, errs = 0;
       	struct cache_entry *last_ce = NULL;
       
  7:  3b734f89c0f !  7:  ff32952a21c update-index: add tests for sparse-checkout compatibility
     @@ Commit message
          update-index: add tests for sparse-checkout compatibility
      
          Introduce tests for a variety of `git update-index` use cases, including
     -    performance scenarios. Tests for `update-index add/remove` are specifically
     -    focused on how `git stash` uses `git update-index` as a subcommand to
     -    prepare for sparse index integration with `stash` in a future series.
     +    performance scenarios. Tests are intended to exercise `update-index` with
     +    options that change the commands interaction with the index (e.g.,
     +    `--again`) and with files/directories inside and outside a sparse checkout
     +    cone.
     +
     +    Of note is that these tests clearly establish the behavior of `git
     +    update-index --add` with untracked, outside-of-cone files. Unlike `git add`,
     +    which fails with an error when provided with such files, `update-index`
     +    succeeds in adding them to the index. Additionally, the `skip-worktree` flag
     +    is *not* automatically added to the new entry. Although this is pre-existing
     +    behavior, there are a couple of reasons to avoid changing it in favor of
     +    consistency with e.g. `git add`:
     +
     +    * `update-index` is low-level command for modifying the index; while it can
     +      perform operations similar to those of `add`, it traditionally has fewer
     +      "guardrails" preventing a user from doing something they may not want to
     +      do (in this case, adding an outside-of-cone, non-`skip-worktree` file to
     +      the index)
     +    * `update-index` typically only exits with an error code if it is incapable
     +      of performing an operation (e.g., if an internal function call fails);
     +      adding a new file outside the sparse checkout definition is still a valid
     +      operation, albeit an inadvisable one
     +    * `update-index` does not implicitly set flags (e.g., `skip-worktree`) when
     +      creating new index entries with `--add`; if flags need to be updated,
     +      options like `--[no-]skip-worktree` allow a user to intentionally set them
     +
     +    All this to say that, while there are valid reasons to consider changing the
     +    treatment of outside-of-cone files in `update-index`, there are also
     +    sufficient reasons for leaving it as-is.
      
          Co-authored-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Victoria Dye <vdye@github.com>
     @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git diff --cached
       test_perf_on_all git blame $SPARSE_CONE/a
       test_perf_on_all git blame $SPARSE_CONE/f3/a
       test_perf_on_all git checkout-index -f --all
     -+test_perf_on_all git update-index --add --remove
     ++test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
       
       test_done
      
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
      +	EOF
      +
      +	# Create & modify folder1/a
     ++	# Note that this setup is a manual way of reaching the erroneous
     ++	# condition in which a `skip-worktree` enabled, outside-of-cone file
     ++	# exists on disk. It is used here to ensure `update-index` is stable
     ++	# and behaves predictably if such a condition occurs.
      +	run_on_sparse mkdir -p folder1 &&
      +	run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
      +	run_on_all ../edit-contents folder1/a &&
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
      +	run_on_sparse mkdir -p folder1 &&
      +	run_on_all ../edit-contents folder1/b &&
      +
     -+	# Similar to `git add`, the untracked out-of-cone file is added to the index
     -+	# identically across sparse and non-sparse checkouts
     ++	# The *untracked* out-of-cone file is added to the index because it does
     ++	# not have a `skip-worktree` bit to signal that it should be ignored
     ++	# (unlike in `git add`, which will fail due to the file being outside
     ++	# the sparse checkout definition).
      +	test_all_match git update-index --add folder1/b &&
      +	test_all_match git status --porcelain=v2
      +'
      +
     ++# NEEDSWORK: `--remove`, unlike the rest of `update-index`, does not ignore
     ++# `skip-worktree` entries by default and will remove them from the index.
     ++# The `--ignore-skip-worktree-entries` flag must be used in conjunction with
     ++# `--remove` to ignore the `skip-worktree` entries and prevent their removal
     ++# from the index.
      +test_expect_success 'update-index --remove outside sparse definition' '
      +	init_repos &&
      +
     -+	# When `--ignore-skip-worktree-entries` is specified, out-of-cone files are
     -+	# not removed from the index if they do not exist on disk
     ++	# When --ignore-skip-worktree-entries is _not_ specified:
     ++	# out-of-cone, not-on-disk files are removed from the index
     ++	test_sparse_match git update-index --remove folder1/a &&
     ++	cat >expect <<-EOF &&
     ++	D	folder1/a
     ++	EOF
     ++	test_sparse_match git diff --cached --name-status &&
     ++	test_cmp expect sparse-checkout-out &&
     ++
     ++	# Reset the state
     ++	test_all_match git reset --hard &&
     ++
     ++	# When --ignore-skip-worktree-entries is specified, out-of-cone
     ++	# (skip-worktree) files are ignored
      +	test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
     -+	test_all_match git status --porcelain=v2 &&
     ++	test_sparse_match git diff --cached --name-status &&
     ++	test_must_be_empty sparse-checkout-out &&
      +
     -+	# When the flag is _not_ specified, out-of-cone, not-on-disk files are
     -+	# removed from the index
     -+	rm full-checkout/folder1/a &&
     -+	test_all_match git update-index --remove folder1/a &&
     -+	test_all_match git status --porcelain=v2 &&
     ++	# Reset the state
     ++	test_all_match git reset --hard &&
      +
     -+	# NOTE: --force-remove supercedes --ignore-skip-worktree-entries, removing
     ++	# --force-remove supercedes --ignore-skip-worktree-entries, removing
      +	# a skip-worktree file from the index (and disk) when both are specified
     -+	test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
     -+	test_all_match git status --porcelain=v2
     ++	# with --remove
     ++	test_sparse_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
     ++	cat >expect <<-EOF &&
     ++	D	folder1/a
     ++	EOF
     ++	test_sparse_match git diff --cached --name-status &&
     ++	test_cmp expect sparse-checkout-out
      +'
      +
      +test_expect_success 'update-index with directories' '
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
      +	#   file and either triggers an error ("does not exist  and --remove not passed")
      +	#   or is ignored completely (when using --remove)
      +	test_all_match test_must_fail git update-index deep &&
     -+	run_on_all test_must_fail git update-indexe folder1 &&
     ++	run_on_all test_must_fail git update-index folder1 &&
      +	test_must_fail git -C full-checkout update-index --remove folder1 &&
      +	test_sparse_match git update-index --remove folder1 &&
      +	test_all_match git status --porcelain=v2
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
      +test_expect_success 'update-index --again file outside sparse definition' '
      +	init_repos &&
      +
     -+	write_script edit-contents <<-\EOF &&
     -+	echo text >>$1
     -+	EOF
     -+
      +	test_all_match git checkout -b test-reupdate &&
      +
      +	# Update HEAD without modifying the index to introduce a difference in
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
      +	test_sparse_match git reset --soft update-folder1 &&
      +
      +	# Because folder1/a differs in the index vs HEAD,
     -+	# `git update-index --remove --again` will effectively perform
     -+	# `git update-index --remove folder1/a` and remove the folder1/a
     -+	test_sparse_match git update-index --remove --again &&
     -+	test_sparse_match git status --porcelain=v2
     ++	# `git update-index --no-skip-worktree --again` will effectively perform
     ++	# `git update-index --no-skip-worktree folder1/a` and remove the skip-worktree
     ++	# flag from folder1/a
     ++	test_sparse_match git update-index --no-skip-worktree --again &&
     ++	test_sparse_match git status --porcelain=v2 &&
     ++
     ++	cat >expect <<-EOF &&
     ++	D	folder1/a
     ++	EOF
     ++	test_sparse_match git diff --name-status &&
     ++	test_cmp expect sparse-checkout-out
      +'
      +
      +test_expect_success 'update-index --cacheinfo' '
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
      +	# Cannot add sparse directory, even in sparse index case
      +	test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
      +
     -+	# Sparse match only - because folder1/a is outside the sparse checkout
     -+	# definition (and thus not on-disk), it will appear "deleted" in
     -+	# unstaged changes.
     -+	test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
     -+	test_sparse_match git status --porcelain=v2
     ++	# Sparse match only: the new outside-of-cone entry is added *without* skip-worktree,
     ++	# so `git status` reports it as "deleted" in the worktree
     ++	test_sparse_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
     ++	test_sparse_match git status --porcelain=v2 &&
     ++	cat >expect <<-EOF &&
     ++	MD folder1/a
     ++	EOF
     ++	test_sparse_match git status --short -- folder1/a &&
     ++	test_cmp expect sparse-checkout-out &&
     ++
     ++	# To return folder1/a to "normal" for a sparse checkout (ignored &
     ++	# outside-of-cone), add the skip-worktree flag.
     ++	test_sparse_match git update-index --skip-worktree folder1/a &&
     ++	cat >expect <<-EOF &&
     ++	S folder1/a
     ++	EOF
     ++	test_sparse_match git ls-files -t -- folder1/a &&
     ++	test_cmp expect sparse-checkout-out
      +'
      +
       test_expect_success 'merge, cherry-pick, and rebase' '
  8:  c5b98e36516 !  8:  9ddc51a47d5 update-index: integrate with sparse index
     @@ Metadata
       ## Commit message ##
          update-index: integrate with sparse index
      
     -    Enable usage of the sparse index with `update-index`. Most variations of
     +    Enable use of the sparse index with `update-index`. Most variations of
          `update-index` work without explicitly expanding the index or making any
          other updates in or outside of `update-index.c`.
      
     @@ read-cache.c: static int add_index_entry_with_check(struct index_state *istate,
       		if (!new_only)
      
       ## t/t1092-sparse-checkout-compatibility.sh ##
     -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is not expanded: blame' '
     - 	done
     +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is not expanded: diff' '
     + 	ensure_not_expanded diff --cached
       '
       
      +test_expect_success 'sparse index is not expanded: update-index' '
      +	init_repos &&
      +
     ++	deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
     ++	ensure_not_expanded update-index --cacheinfo 100644 $deep_a_oid deep/a &&
     ++
      +	echo "test" >sparse-index/README.md &&
      +	echo "test2" >sparse-index/a &&
      +	rm -f sparse-index/deep/a &&
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
      +	ensure_not_expanded update-index --remove deep/a
      +'
      +
     - # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
     - # in this scenario, but it shouldn't.
     - test_expect_success 'reset mixed and checkout orphan' '
     + test_expect_success 'sparse index is not expanded: blame' '
     + 	init_repos &&
     + 
  9:  de7fc143562 !  9:  80697e9259e update-index: reduce scope of index expansion in do_reupdate
     @@ Metadata
       ## Commit message ##
          update-index: reduce scope of index expansion in do_reupdate
      
     -    Expand the full index (and redo reupdate operation) only if a sparse
     -    directory in the index differs from HEAD. Only the index entries that differ
     -    between the index and HEAD are updated when performing `git update-index
     -    --again`, so unmodified sparse directories are safely skipped. The index
     -    does need to be expanded when sparse directories contain changes, though,
     -    because `update_one(...)` will not operate on sparse directory index
     -    entries.
     +    Replace unconditional index expansion in 'do_reupdate()' with one scoped to
     +    only where a full index is needed. A full index is only required in
     +    'do_reupdate()' when a sparse directory in the index differs from HEAD; in
     +    that case, the index is expanded and the operation restarted.
      
          Because the index should only be expanded if a sparse directory is modified,
          add a test ensuring the index is not expanded when differences only exist
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
      +	ensure_not_expanded update-index --add --remove --again
       '
       
     - # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
     + test_expect_success 'sparse index is not expanded: blame' '

-- 
gitgitgadget

[PATCH v2 1/9] reset: fix validation in sparse index test

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update t1092 test 'reset with pathspecs outside sparse definition' to verify
index contents. The use of `rev-parse` verifies the contents of HEAD, not
the index, providing no real validation of the reset results. Conversely,
`ls-files` reports the contents of the index (OIDs, flags, filenames), which
are then compared across checkouts to ensure compatible index states.

Fixes 741a2c9ffa (reset: expand test coverage for sparse checkouts,
2021-09-27).

Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 49f70a65692..d5167e7ed69 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -596,13 +596,11 @@ test_expect_success 'reset with pathspecs outside sparse definition' '
 
 	test_sparse_match git reset update-folder1 -- folder1 &&
 	git -C full-checkout reset update-folder1 -- folder1 &&
-	test_sparse_match git status --porcelain=v2 &&
-	test_all_match git rev-parse HEAD:folder1 &&
+	test_all_match git ls-files -s -- folder1 &&
 
 	test_sparse_match git reset update-folder2 -- folder2/a &&
 	git -C full-checkout reset update-folder2 -- folder2/a &&
-	test_sparse_match git status --porcelain=v2 &&
-	test_all_match git rev-parse HEAD:folder2/a
+	test_all_match git ls-files -s -- folder2/a
 '
 
 test_expect_success 'reset with wildcard pathspec' '
-- 
gitgitgadget

[PATCH v2 2/9] reset: reorder wildcard pathspec conditions

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Rearrange conditions in method determining whether index expansion is
necessary when a pathspec is specified for `git reset`, placing less
expensive condition first. Additionally, add details & examples to related
code comments to help with readability.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/reset.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index b1ff699b43a..79b40385b99 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -204,10 +204,16 @@ static int pathspec_needs_expanded_index(const struct pathspec *pathspec)
 			/*
 			 * Special case: if the pattern is a path inside the cone
 			 * followed by only wildcards, the pattern cannot match
-			 * partial sparse directories, so we don't expand the index.
+			 * partial sparse directories, so we know we don't need to
+			 * expand the index.
+			 *
+			 * Examples:
+			 * - in-cone/foo***: doesn't need expanded index
+			 * - not-in-cone/bar*: may need expanded index
+			 * - **.c: may need expanded index
 			 */
-			if (path_in_cone_mode_sparse_checkout(item.original, &the_index) &&
-			    strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len)
+			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
+			    path_in_cone_mode_sparse_checkout(item.original, &the_index))
 				continue;
 
 			for (pos = 0; pos < active_nr; pos++) {
-- 
gitgitgadget

[PATCH v2 3/9] clean: integrate with sparse index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Remove full index requirement for `git clean` and test to ensure the index
is not expanded in `git clean`. Add to existing test for `git clean` to
verify cleanup of untracked files in sparse directories is consistent
between sparse index and non-sparse index checkouts.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/clean.c                          |  3 +++
 t/t1092-sparse-checkout-compatibility.sh | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/builtin/clean.c b/builtin/clean.c
index 98a2860409b..5628fc7103e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -983,6 +983,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		dir.flags |= DIR_KEEP_UNTRACKED_CONTENTS;
 	}
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index d5167e7ed69..05587361452 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -764,23 +764,42 @@ test_expect_success 'clean' '
 	test_all_match git commit -m "ignore bogus files" &&
 
 	run_on_sparse mkdir folder1 &&
+	run_on_all mkdir -p deep/untracked-deep &&
 	run_on_all touch folder1/bogus &&
+	run_on_all touch folder1/untracked &&
+	run_on_all touch deep/untracked-deep/bogus &&
+	run_on_all touch deep/untracked-deep/untracked &&
 
 	test_all_match git status --porcelain=v2 &&
 	test_all_match git clean -f &&
 	test_all_match git status --porcelain=v2 &&
 	test_sparse_match ls &&
 	test_sparse_match ls folder1 &&
+	run_on_all test_path_exists folder1/bogus &&
+	run_on_all test_path_is_missing folder1/untracked &&
+	run_on_all test_path_exists deep/untracked-deep/bogus &&
+	run_on_all test_path_exists deep/untracked-deep/untracked &&
+
+	test_all_match git clean -fd &&
+	test_all_match git status --porcelain=v2 &&
+	test_sparse_match ls &&
+	test_sparse_match ls folder1 &&
+	run_on_all test_path_exists folder1/bogus &&
+	run_on_all test_path_exists deep/untracked-deep/bogus &&
+	run_on_all test_path_is_missing deep/untracked-deep/untracked &&
 
 	test_all_match git clean -xf &&
 	test_all_match git status --porcelain=v2 &&
 	test_sparse_match ls &&
 	test_sparse_match ls folder1 &&
+	run_on_all test_path_is_missing folder1/bogus &&
+	run_on_all test_path_exists deep/untracked-deep/bogus &&
 
 	test_all_match git clean -xdf &&
 	test_all_match git status --porcelain=v2 &&
 	test_sparse_match ls &&
 	test_sparse_match ls folder1 &&
+	run_on_all test_path_is_missing deep/untracked-deep/bogus &&
 
 	test_sparse_match test_path_is_dir folder1
 '
@@ -920,6 +939,8 @@ test_expect_success 'sparse-index is not expanded' '
 	# Wildcard identifies only full sparse directories, no index expansion
 	ensure_not_expanded reset deepest -- folder\* &&
 
+	ensure_not_expanded clean -fd &&
+
 	ensure_not_expanded checkout -f update-deep &&
 	test_config -C sparse-index pull.twohead ort &&
 	(
-- 
gitgitgadget

[PATCH v2 4/9] checkout-index: expand sparse checkout compatibility tests

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Add tests to cover `checkout-index`, with a focus on cases interesting in a
sparse checkout (e.g., files specified outside sparse checkout definition).

New tests are intended to serve as a baseline for existing and/or expected
behavior and performance when integrating `checkout-index` with the sparse
index. Note that the test 'checkout-index --all' is marked as
'test_expect_failure', indicating that `update-index --all` will be modified
in a subsequent patch to behave as the test expects.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/perf/p2000-sparse-operations.sh        |  1 +
 t/t1092-sparse-checkout-compatibility.sh | 54 ++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index cb777c74a24..54f8602f3c1 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -117,5 +117,6 @@ test_perf_on_all git diff
 test_perf_on_all git diff --cached
 test_perf_on_all git blame $SPARSE_CONE/a
 test_perf_on_all git blame $SPARSE_CONE/f3/a
+test_perf_on_all git checkout-index -f --all
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 05587361452..db7ad41109b 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -755,6 +755,60 @@ test_expect_success 'cherry-pick with conflicts' '
 	test_all_match test_must_fail git cherry-pick to-cherry-pick
 '
 
+test_expect_success 'checkout-index inside sparse definition' '
+	init_repos &&
+
+	run_on_all rm -f deep/a &&
+	test_all_match git checkout-index -- deep/a &&
+	test_all_match git status --porcelain=v2 &&
+
+	echo test >>new-a &&
+	run_on_all cp ../new-a a &&
+	test_all_match test_must_fail git checkout-index -- a &&
+	test_all_match git checkout-index -f -- a &&
+	test_all_match git status --porcelain=v2
+'
+
+test_expect_success 'checkout-index outside sparse definition' '
+	init_repos &&
+
+	# File does not exist on disk yet for sparse checkouts, so checkout-index
+	# succeeds without -f
+	test_sparse_match git checkout-index -- folder1/a &&
+	test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
+	test_cmp sparse-checkout/folder1/a full-checkout/folder1/a &&
+
+	run_on_sparse rm -rf folder1 &&
+	echo test >new-a &&
+	run_on_sparse mkdir -p folder1 &&
+	run_on_all cp ../new-a folder1/a &&
+
+	test_all_match test_must_fail git checkout-index -- folder1/a &&
+	test_all_match git checkout-index -f -- folder1/a &&
+	test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
+	test_cmp sparse-checkout/folder1/a full-checkout/folder1/a
+'
+
+test_expect_success 'checkout-index with folders' '
+	init_repos &&
+
+	# Inside checkout definition
+	test_all_match test_must_fail git checkout-index -f -- deep/ &&
+
+	# Outside checkout definition
+	test_all_match test_must_fail git checkout-index -f -- folder1/
+'
+
+# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
+# files (even those outside the sparse definition) on disk. However, these files
+# don't appear in the percentage of tracked files in git status.
+test_expect_failure 'checkout-index --all' '
+	init_repos &&
+
+	test_all_match git checkout-index --all &&
+	test_sparse_match test_path_is_missing folder1
+'
+
 test_expect_success 'clean' '
 	init_repos &&
 
-- 
gitgitgadget

[PATCH v2 5/9] checkout-index: add --ignore-skip-worktree-bits option

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update `checkout-index` to no longer refresh files that have the
`skip-worktree` bit set, exiting with an error if `skip-worktree` filenames
are directly provided to `checkout-index`. The newly-added
`--ignore-skip-worktree-bits` option provides a mechanism to replicate the
old behavior, checking out *all* files specified (even those with
`skip-worktree` enabled).

The ability to toggle whether files should be checked-out based on
`skip-worktree` already exists in `git checkout` and `git restore` (both of
which have an `--ignore-skip-worktree-bits` option). The change to, by
default, ignore `skip-worktree` files is especially helpful for
sparse-checkout; it prevents inadvertent creation of files outside the
sparse definition on disk and eliminates the need to expand a sparse index
when using the `--all` option.

Internal usage of `checkout-index` in `git stash` and `git filter-branch` do
not make explicit use of files with `skip-worktree` enabled, so
`--ignore-skip-worktree-bits` is not added to them.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-checkout-index.txt     | 10 +++++++--
 builtin/checkout-index.c                 | 13 ++++++++++++
 t/t1092-sparse-checkout-compatibility.sh | 27 +++++++++++++++---------
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-checkout-index.txt b/Documentation/git-checkout-index.txt
index 4d33e7be0f5..01dbd5cbf54 100644
--- a/Documentation/git-checkout-index.txt
+++ b/Documentation/git-checkout-index.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git checkout-index' [-u] [-q] [-a] [-f] [-n] [--prefix=<string>]
 		   [--stage=<number>|all]
 		   [--temp]
+		   [--ignore-skip-worktree-bits]
 		   [-z] [--stdin]
 		   [--] [<file>...]
 
@@ -37,8 +38,9 @@ OPTIONS
 
 -a::
 --all::
-	checks out all files in the index.  Cannot be used
-	together with explicit filenames.
+	checks out all files in the index except for those with the
+	skip-worktree bit set (see `--ignore-skip-worktree-bits`).
+	Cannot be used together with explicit filenames.
 
 -n::
 --no-create::
@@ -59,6 +61,10 @@ OPTIONS
 	write the content to temporary files.  The temporary name
 	associations will be written to stdout.
 
+--ignore-skip-worktree-bits::
+	Check out all files, including those with the skip-worktree bit
+	set.
+
 --stdin::
 	Instead of taking list of paths from the command line,
 	read list of paths from the standard input.  Paths are
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index e21620d964e..615a118e2f5 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -7,6 +7,7 @@
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "builtin.h"
 #include "config.h"
+#include "dir.h"
 #include "lockfile.h"
 #include "quote.h"
 #include "cache-tree.h"
@@ -17,6 +18,7 @@
 #define CHECKOUT_ALL 4
 static int nul_term_line;
 static int checkout_stage; /* default to checkout stage0 */
+static int ignore_skip_worktree; /* default to 0 */
 static int to_tempfile;
 static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
 
@@ -65,6 +67,7 @@ static int checkout_file(const char *name, const char *prefix)
 	int namelen = strlen(name);
 	int pos = cache_name_pos(name, namelen);
 	int has_same_name = 0;
+	int is_skipped = 1;
 	int did_checkout = 0;
 	int errs = 0;
 
@@ -78,6 +81,9 @@ static int checkout_file(const char *name, const char *prefix)
 			break;
 		has_same_name = 1;
 		pos++;
+		if (!ignore_skip_worktree && ce_skip_worktree(ce))
+			break;
+		is_skipped = 0;
 		if (ce_stage(ce) != checkout_stage
 		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
 			continue;
@@ -106,6 +112,9 @@ static int checkout_file(const char *name, const char *prefix)
 		fprintf(stderr, "git checkout-index: %s ", name);
 		if (!has_same_name)
 			fprintf(stderr, "is not in the cache");
+		else if (is_skipped)
+			fprintf(stderr, "has skip-worktree enabled; "
+					"use '--ignore-skip-worktree-bits' to checkout");
 		else if (checkout_stage)
 			fprintf(stderr, "does not exist at stage %d",
 				checkout_stage);
@@ -125,6 +134,8 @@ static int checkout_all(const char *prefix, int prefix_length)
 	ensure_full_index(&the_index);
 	for (i = 0; i < active_nr ; i++) {
 		struct cache_entry *ce = active_cache[i];
+		if (!ignore_skip_worktree && ce_skip_worktree(ce))
+			continue;
 		if (ce_stage(ce) != checkout_stage
 		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
 			continue;
@@ -185,6 +196,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	struct option builtin_checkout_index_options[] = {
 		OPT_BOOL('a', "all", &all,
 			N_("check out all files in the index")),
+		OPT_BOOL(0, "ignore-skip-worktree-bits", &ignore_skip_worktree,
+			N_("do not skip files with skip-worktree set")),
 		OPT__FORCE(&force, N_("force overwrite of existing files"), 0),
 		OPT__QUIET(&quiet,
 			N_("no warning for existing files and files not in index")),
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index db7ad41109b..434ef0433c0 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -772,9 +772,14 @@ test_expect_success 'checkout-index inside sparse definition' '
 test_expect_success 'checkout-index outside sparse definition' '
 	init_repos &&
 
-	# File does not exist on disk yet for sparse checkouts, so checkout-index
-	# succeeds without -f
-	test_sparse_match git checkout-index -- folder1/a &&
+	# Without --ignore-skip-worktree-bits, outside-of-cone files will trigger
+	# an error
+	test_sparse_match test_must_fail git checkout-index -- folder1/a &&
+	test_i18ngrep "folder1/a has skip-worktree enabled" sparse-checkout-err &&
+	test_path_is_missing folder1/a &&
+
+	# With --ignore-skip-worktree-bits, outside-of-cone files are checked out
+	test_sparse_match git checkout-index --ignore-skip-worktree-bits -- folder1/a &&
 	test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
 	test_cmp sparse-checkout/folder1/a full-checkout/folder1/a &&
 
@@ -783,8 +788,8 @@ test_expect_success 'checkout-index outside sparse definition' '
 	run_on_sparse mkdir -p folder1 &&
 	run_on_all cp ../new-a folder1/a &&
 
-	test_all_match test_must_fail git checkout-index -- folder1/a &&
-	test_all_match git checkout-index -f -- folder1/a &&
+	test_all_match test_must_fail git checkout-index --ignore-skip-worktree-bits -- folder1/a &&
+	test_all_match git checkout-index -f --ignore-skip-worktree-bits -- folder1/a &&
 	test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
 	test_cmp sparse-checkout/folder1/a full-checkout/folder1/a
 '
@@ -799,14 +804,16 @@ test_expect_success 'checkout-index with folders' '
 	test_all_match test_must_fail git checkout-index -f -- folder1/
 '
 
-# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all
-# files (even those outside the sparse definition) on disk. However, these files
-# don't appear in the percentage of tracked files in git status.
-test_expect_failure 'checkout-index --all' '
+test_expect_success 'checkout-index --all' '
 	init_repos &&
 
 	test_all_match git checkout-index --all &&
-	test_sparse_match test_path_is_missing folder1
+	test_sparse_match test_path_is_missing folder1 &&
+
+	# --ignore-skip-worktree-bits will cause `skip-worktree` files to be
+	# checked out, causing the outside-of-cone `folder1` to exist on-disk
+	test_all_match git checkout-index --ignore-skip-worktree-bits --all &&
+	test_all_match test_path_exists folder1
 '
 
 test_expect_success 'clean' '
-- 
gitgitgadget

[PATCH v2 6/9] checkout-index: integrate with sparse index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Add repository settings to allow usage of the sparse index.

When using the `--all` option, sparse directories are ignored by default due
to the `skip-worktree` flag, so there is no need to expand the index. If
`--ignore-skip-worktree-bits` is specified, the index is expanded in order
to check out all files.

When checking out individual files, existing behavior in a full index is to
exit with an error if a directory is specified (as the directory name will
not match an index entry). However, it is possible in a sparse index to
match a directory name to a sparse directory index entry, but checking out
that sparse directory still results in an error on checkout. To reduce some
potential confusion for users, `checkout_file(...)` explicitly exits with an
informative error if provided with a sparse directory name. The test
corresponding to this scenario verifies the error message, which now differs
between sparse index and non-sparse index checkouts.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/checkout-index.c                 | 28 ++++++++++++++++++++++--
 t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++-
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 615a118e2f5..97e06e8c52c 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -67,6 +67,7 @@ static int checkout_file(const char *name, const char *prefix)
 	int namelen = strlen(name);
 	int pos = cache_name_pos(name, namelen);
 	int has_same_name = 0;
+	int is_file = 0;
 	int is_skipped = 1;
 	int did_checkout = 0;
 	int errs = 0;
@@ -81,6 +82,9 @@ static int checkout_file(const char *name, const char *prefix)
 			break;
 		has_same_name = 1;
 		pos++;
+		if (S_ISSPARSEDIR(ce->ce_mode))
+			break;
+		is_file = 1;
 		if (!ignore_skip_worktree && ce_skip_worktree(ce))
 			break;
 		is_skipped = 0;
@@ -112,6 +116,8 @@ static int checkout_file(const char *name, const char *prefix)
 		fprintf(stderr, "git checkout-index: %s ", name);
 		if (!has_same_name)
 			fprintf(stderr, "is not in the cache");
+		else if (!is_file)
+			fprintf(stderr, "is a sparse directory");
 		else if (is_skipped)
 			fprintf(stderr, "has skip-worktree enabled; "
 					"use '--ignore-skip-worktree-bits' to checkout");
@@ -130,10 +136,25 @@ static int checkout_all(const char *prefix, int prefix_length)
 	int i, errs = 0;
 	struct cache_entry *last_ce = NULL;
 
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(&the_index);
 	for (i = 0; i < active_nr ; i++) {
 		struct cache_entry *ce = active_cache[i];
+
+		if (S_ISSPARSEDIR(ce->ce_mode)) {
+			if (!ce_skip_worktree(ce))
+				BUG("sparse directory '%s' does not have skip-worktree set", ce->name);
+
+			/*
+			 * If the current entry is a sparse directory and skip-worktree
+			 * entries are being checked out, expand the index and continue
+			 * the loop on the current index position (now pointing to the
+			 * first entry inside the expanded sparse directory).
+			 */
+			if (ignore_skip_worktree) {
+				ensure_full_index(&the_index);
+				ce = active_cache[i];
+			}
+		}
+
 		if (!ignore_skip_worktree && ce_skip_worktree(ce))
 			continue;
 		if (ce_stage(ce) != checkout_stage
@@ -225,6 +246,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	prefix_length = prefix ? strlen(prefix) : 0;
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	if (read_cache() < 0) {
 		die("invalid cache");
 	}
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 434ef0433c0..0c72c854d84 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -801,7 +801,14 @@ test_expect_success 'checkout-index with folders' '
 	test_all_match test_must_fail git checkout-index -f -- deep/ &&
 
 	# Outside checkout definition
-	test_all_match test_must_fail git checkout-index -f -- folder1/
+	# Note: although all tests fail (as expected), the messaging differs. For
+	# non-sparse index checkouts, the error is that the "file" does not appear
+	# in the index; for sparse checkouts, the error is explicitly that the
+	# entry is a sparse directory.
+	run_on_all test_must_fail git checkout-index -f -- folder1/ &&
+	test_cmp full-checkout-err sparse-checkout-err &&
+	! test_cmp full-checkout-err sparse-index-err &&
+	grep "is a sparse directory" sparse-index-err
 '
 
 test_expect_success 'checkout-index --all' '
@@ -972,6 +979,8 @@ test_expect_success 'sparse-index is not expanded' '
 	echo >>sparse-index/untracked.txt &&
 	ensure_not_expanded add . &&
 
+	ensure_not_expanded checkout-index -f a &&
+	ensure_not_expanded checkout-index -f --all &&
 	for ref in update-deep update-folder1 update-folder2 update-deep
 	do
 		echo >>sparse-index/README.md &&
-- 
gitgitgadget

[PATCH v2 7/9] update-index: add tests for sparse-checkout compatibility

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Introduce tests for a variety of `git update-index` use cases, including
performance scenarios. Tests are intended to exercise `update-index` with
options that change the commands interaction with the index (e.g.,
`--again`) and with files/directories inside and outside a sparse checkout
cone.

Of note is that these tests clearly establish the behavior of `git
update-index --add` with untracked, outside-of-cone files. Unlike `git add`,
which fails with an error when provided with such files, `update-index`
succeeds in adding them to the index. Additionally, the `skip-worktree` flag
is *not* automatically added to the new entry. Although this is pre-existing
behavior, there are a couple of reasons to avoid changing it in favor of
consistency with e.g. `git add`:

* `update-index` is low-level command for modifying the index; while it can
  perform operations similar to those of `add`, it traditionally has fewer
  "guardrails" preventing a user from doing something they may not want to
  do (in this case, adding an outside-of-cone, non-`skip-worktree` file to
  the index)
* `update-index` typically only exits with an error code if it is incapable
  of performing an operation (e.g., if an internal function call fails);
  adding a new file outside the sparse checkout definition is still a valid
  operation, albeit an inadvisable one
* `update-index` does not implicitly set flags (e.g., `skip-worktree`) when
  creating new index entries with `--add`; if flags need to be updated,
  options like `--[no-]skip-worktree` allow a user to intentionally set them

All this to say that, while there are valid reasons to consider changing the
treatment of outside-of-cone files in `update-index`, there are also
sufficient reasons for leaving it as-is.

Co-authored-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 t/perf/p2000-sparse-operations.sh        |   1 +
 t/t1092-sparse-checkout-compatibility.sh | 167 +++++++++++++++++++++++
 2 files changed, 168 insertions(+)

diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 54f8602f3c1..2a7106b9495 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -118,5 +118,6 @@ test_perf_on_all git diff --cached
 test_perf_on_all git blame $SPARSE_CONE/a
 test_perf_on_all git blame $SPARSE_CONE/f3/a
 test_perf_on_all git checkout-index -f --all
+test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c72c854d84..91f849f541e 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -630,6 +630,173 @@ test_expect_success 'reset with wildcard pathspec' '
 	test_all_match git ls-files -s -- folder1
 '
 
+test_expect_success 'update-index modify outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	# Create & modify folder1/a
+	# Note that this setup is a manual way of reaching the erroneous
+	# condition in which a `skip-worktree` enabled, outside-of-cone file
+	# exists on disk. It is used here to ensure `update-index` is stable
+	# and behaves predictably if such a condition occurs.
+	run_on_sparse mkdir -p folder1 &&
+	run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
+	run_on_all ../edit-contents folder1/a &&
+
+	# If file has skip-worktree enabled, update-index does not modify the
+	# index entry
+	test_sparse_match git update-index folder1/a &&
+	test_sparse_match git status --porcelain=v2 &&
+	test_must_be_empty sparse-checkout-out &&
+
+	# When skip-worktree is disabled (even on files outside sparse cone), file
+	# is updated in the index
+	test_sparse_match git update-index --no-skip-worktree folder1/a &&
+	test_all_match git status --porcelain=v2 &&
+	test_all_match git update-index folder1/a &&
+	test_all_match git status --porcelain=v2
+'
+
+test_expect_success 'update-index --add outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	# Create folder1, add new file
+	run_on_sparse mkdir -p folder1 &&
+	run_on_all ../edit-contents folder1/b &&
+
+	# The *untracked* out-of-cone file is added to the index because it does
+	# not have a `skip-worktree` bit to signal that it should be ignored
+	# (unlike in `git add`, which will fail due to the file being outside
+	# the sparse checkout definition).
+	test_all_match git update-index --add folder1/b &&
+	test_all_match git status --porcelain=v2
+'
+
+# NEEDSWORK: `--remove`, unlike the rest of `update-index`, does not ignore
+# `skip-worktree` entries by default and will remove them from the index.
+# The `--ignore-skip-worktree-entries` flag must be used in conjunction with
+# `--remove` to ignore the `skip-worktree` entries and prevent their removal
+# from the index.
+test_expect_success 'update-index --remove outside sparse definition' '
+	init_repos &&
+
+	# When --ignore-skip-worktree-entries is _not_ specified:
+	# out-of-cone, not-on-disk files are removed from the index
+	test_sparse_match git update-index --remove folder1/a &&
+	cat >expect <<-EOF &&
+	D	folder1/a
+	EOF
+	test_sparse_match git diff --cached --name-status &&
+	test_cmp expect sparse-checkout-out &&
+
+	# Reset the state
+	test_all_match git reset --hard &&
+
+	# When --ignore-skip-worktree-entries is specified, out-of-cone
+	# (skip-worktree) files are ignored
+	test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
+	test_sparse_match git diff --cached --name-status &&
+	test_must_be_empty sparse-checkout-out &&
+
+	# Reset the state
+	test_all_match git reset --hard &&
+
+	# --force-remove supercedes --ignore-skip-worktree-entries, removing
+	# a skip-worktree file from the index (and disk) when both are specified
+	# with --remove
+	test_sparse_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
+	cat >expect <<-EOF &&
+	D	folder1/a
+	EOF
+	test_sparse_match git diff --cached --name-status &&
+	test_cmp expect sparse-checkout-out
+'
+
+test_expect_success 'update-index with directories' '
+	init_repos &&
+
+	# update-index will exit silently when provided with a directory name
+	# containing a trailing slash
+	test_all_match git update-index deep/ folder1/ &&
+	grep "Ignoring path deep/" sparse-checkout-err &&
+	grep "Ignoring path folder1/" sparse-checkout-err &&
+
+	# When update-index is given a directory name WITHOUT a trailing slash, it will
+	# behave in different ways depending on the status of the directory on disk:
+	# * if it exists, the command exits with an error ("add individual files instead")
+	# * if it does NOT exist (e.g., in a sparse-checkout), it is assumed to be a
+	#   file and either triggers an error ("does not exist  and --remove not passed")
+	#   or is ignored completely (when using --remove)
+	test_all_match test_must_fail git update-index deep &&
+	run_on_all test_must_fail git update-index folder1 &&
+	test_must_fail git -C full-checkout update-index --remove folder1 &&
+	test_sparse_match git update-index --remove folder1 &&
+	test_all_match git status --porcelain=v2
+'
+
+test_expect_success 'update-index --again file outside sparse definition' '
+	init_repos &&
+
+	test_all_match git checkout -b test-reupdate &&
+
+	# Update HEAD without modifying the index to introduce a difference in
+	# folder1/a
+	test_sparse_match git reset --soft update-folder1 &&
+
+	# Because folder1/a differs in the index vs HEAD,
+	# `git update-index --no-skip-worktree --again` will effectively perform
+	# `git update-index --no-skip-worktree folder1/a` and remove the skip-worktree
+	# flag from folder1/a
+	test_sparse_match git update-index --no-skip-worktree --again &&
+	test_sparse_match git status --porcelain=v2 &&
+
+	cat >expect <<-EOF &&
+	D	folder1/a
+	EOF
+	test_sparse_match git diff --name-status &&
+	test_cmp expect sparse-checkout-out
+'
+
+test_expect_success 'update-index --cacheinfo' '
+	init_repos &&
+
+	deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
+	folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) &&
+	folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) &&
+
+	test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a &&
+	test_all_match git status --porcelain=v2 &&
+
+	# Cannot add sparse directory, even in sparse index case
+	test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
+
+	# Sparse match only: the new outside-of-cone entry is added *without* skip-worktree,
+	# so `git status` reports it as "deleted" in the worktree
+	test_sparse_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
+	test_sparse_match git status --porcelain=v2 &&
+	cat >expect <<-EOF &&
+	MD folder1/a
+	EOF
+	test_sparse_match git status --short -- folder1/a &&
+	test_cmp expect sparse-checkout-out &&
+
+	# To return folder1/a to "normal" for a sparse checkout (ignored &
+	# outside-of-cone), add the skip-worktree flag.
+	test_sparse_match git update-index --skip-worktree folder1/a &&
+	cat >expect <<-EOF &&
+	S folder1/a
+	EOF
+	test_sparse_match git ls-files -t -- folder1/a &&
+	test_cmp expect sparse-checkout-out
+'
+
 test_expect_success 'merge, cherry-pick, and rebase' '
 	init_repos &&
 
-- 
gitgitgadget

[PATCH v2 8/9] update-index: integrate with sparse index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Enable use of the sparse index with `update-index`. Most variations of
`update-index` work without explicitly expanding the index or making any
other updates in or outside of `update-index.c`.

The one usage requiring additional changes is `--cacheinfo`; if a file
inside a sparse directory was specified, the index would not be expanded
until after the cache tree is invalidated, leading to a mismatch between the
index and cache tree. This scenario is handled by rearranging
`add_index_entry_with_check`, allowing `index_name_stage_pos` to expand the
index *before* attempting to invalidate the relevant cache tree path,
avoiding cache tree/index corruption.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/update-index.c                   |  3 +++
 read-cache.c                             | 10 +++++++---
 t/t1092-sparse-checkout-compatibility.sh | 15 +++++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 187203e8bb5..605cc693bbd 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1077,6 +1077,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
+	prepare_repo_settings(r);
+	the_repository->settings.command_requires_full_index = 0;
+
 	/* we will diagnose later if it turns out that we need to update it */
 	newfd = hold_locked_index(&lock_file, 0);
 	if (newfd < 0)
diff --git a/read-cache.c b/read-cache.c
index cbe73f14e5e..b4600e954b6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1339,9 +1339,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
 	int new_only = option & ADD_CACHE_NEW_ONLY;
 
-	if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
-		cache_tree_invalidate_path(istate, ce->name);
-
 	/*
 	 * If this entry's path sorts after the last entry in the index,
 	 * we can avoid searching for it.
@@ -1352,6 +1349,13 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	else
 		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), EXPAND_SPARSE);
 
+	/*
+	 * Cache tree path should be invalidated only after index_name_stage_pos,
+	 * in case it expands a sparse index.
+	 */
+	if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
+		cache_tree_invalidate_path(istate, ce->name);
+
 	/* existing match? Just replace it. */
 	if (pos >= 0) {
 		if (!new_only)
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 91f849f541e..fceaba7101d 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1253,6 +1253,21 @@ test_expect_success 'sparse index is not expanded: diff' '
 	ensure_not_expanded diff --cached
 '
 
+test_expect_success 'sparse index is not expanded: update-index' '
+	init_repos &&
+
+	deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
+	ensure_not_expanded update-index --cacheinfo 100644 $deep_a_oid deep/a &&
+
+	echo "test" >sparse-index/README.md &&
+	echo "test2" >sparse-index/a &&
+	rm -f sparse-index/deep/a &&
+
+	ensure_not_expanded update-index --add README.md &&
+	ensure_not_expanded update-index a &&
+	ensure_not_expanded update-index --remove deep/a
+'
+
 test_expect_success 'sparse index is not expanded: blame' '
 	init_repos &&
 
-- 
gitgitgadget

[PATCH v2 9/9] update-index: reduce scope of index expansion in do_reupdate

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Replace unconditional index expansion in 'do_reupdate()' with one scoped to
only where a full index is needed. A full index is only required in
'do_reupdate()' when a sparse directory in the index differs from HEAD; in
that case, the index is expanded and the operation restarted.

Because the index should only be expanded if a sparse directory is modified,
add a test ensuring the index is not expanded when differences only exist
within the sparse cone.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/update-index.c                   | 14 +++++++++++---
 t/t1092-sparse-checkout-compatibility.sh |  5 ++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 605cc693bbd..52ecc714d99 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -606,7 +606,7 @@ static struct cache_entry *read_one_ent(const char *which,
 			error("%s: not in %s branch.", path, which);
 		return NULL;
 	}
-	if (mode == S_IFDIR) {
+	if (!the_index.sparse_index && mode == S_IFDIR) {
 		if (which)
 			error("%s: not a blob in %s branch.", path, which);
 		return NULL;
@@ -743,8 +743,6 @@ static int do_reupdate(int ac, const char **av,
 		 */
 		has_head = 0;
  redo:
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(&the_index);
 	for (pos = 0; pos < active_nr; pos++) {
 		const struct cache_entry *ce = active_cache[pos];
 		struct cache_entry *old = NULL;
@@ -761,6 +759,16 @@ static int do_reupdate(int ac, const char **av,
 			discard_cache_entry(old);
 			continue; /* unchanged */
 		}
+
+		/* At this point, we know the contents of the sparse directory are
+		 * modified with respect to HEAD, so we expand the index and restart
+		 * to process each path individually
+		 */
+		if (S_ISSPARSEDIR(ce->ce_mode)) {
+			ensure_full_index(&the_index);
+			goto redo;
+		}
+
 		/* Be careful.  The working tree may not have the
 		 * path anymore, in which case, under 'allow_remove',
 		 * or worse yet 'allow_replace', active_nr may decrease.
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index fceaba7101d..53f84881de7 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1265,7 +1265,10 @@ test_expect_success 'sparse index is not expanded: update-index' '
 
 	ensure_not_expanded update-index --add README.md &&
 	ensure_not_expanded update-index a &&
-	ensure_not_expanded update-index --remove deep/a
+	ensure_not_expanded update-index --remove deep/a &&
+
+	ensure_not_expanded reset --soft update-deep &&
+	ensure_not_expanded update-index --add --remove --again
 '
 
 test_expect_success 'sparse index is not expanded: blame' '
-- 
gitgitgadget

Re: [PATCH v2 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index'

[Elijah Newren]

On Tue, Jan 11, 2022 at 10:05 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This series continues the work to integrate commands with the sparse index,
> adding integrations with 'git clean', 'git checkout-index', and 'git
> update-index'. These three commands, while useful in their own right, are
> updated mainly because they're used in 'git stash'. A future series will
> integrate sparse index with 'stash' directly, but its subcommands must be
> integrated to avoid the performance cost of each one expanding and
> collapsing the index.
>
> The series is broken up into 4 parts:
>
>  * Patches 1-2 are minor fixups to the 'git reset' sparse index integration
>    in response to discussion [1] that came after the series was ready for
>    merge to 'next'.
>  * Patch 3 integrates 'git clean' with the sparse index.
>  * Patches 4-6 integrate 'git checkout-index' with the sparse index and
>    introduce a new '--ignore-skip-worktree-bits' option.
>    * This involves changing the behavior of 'checkout-index' to respect
>      'skip-worktree' by default (i.e., it won't check out 'skip-worktree'
>      files). The '--ignore-skip-worktree-bits' option can be specified to
>      force checkout of 'skip-worktree' files, if desired.
>  * Patches 7-9 integrate 'git update-index' with the sparse index.
>    * Note that, although this integrates the sparse index with
>      '--cacheinfo', sparse directories still cannot be updated using that
>      option (see the prior discussion [2] for more details on why)
>
>
> Changes since V1
> ================
>
>  * Changed 'checkout-index' to fail by default when given filenames of files
>    with 'skip-worktree' enabled
>    * These files can still be forcibly checked-out by using the
>      '--ignore-skip-worktree-bits' option
>    * Added/updated corresponding t1092 tests
>  * Updated t1092 'update-index' tests
>    * Mentioned where/why 'skip-worktree' files were manually created on-disk
>      for testing purposes
>    * Provided explanation as to what '--remove' does, and how it relates to
>      '--ignore-skip-worktree-entries'; restructured corresponding test
>    * Fixed typo 'update-indexe' -> 'update-index'
>    * Removed unused 'edit-contents'
>    * Changed '--again' test to not use '--remove' to avoid confusion over
>      how/why it updates 'skip-worktree' entries
>    * Added "set skip-worktree" step to '--cacheinfo' test to illustrate how
>      it could be used to add a new outside-of-cone file and remain generally
>      compliant with a sparse-checkout definition
>    * Added '--cacheinfo' test to "ensure not expanded"
>    * Moved t1092 test 'sparse index is not expanded: update-index' to avoid
>      merge conflict
>  * Updated p2000 test for 'update-index': added file argument
>    * Without any file arguments, 'update-index' was effectively a no-op
>  * Clarified reasoning behind changing/not changing behavior of update-index
>    in sparse-checkouts

Nicely done!  You've addressed all my (voluminous) feedback from v1;
this round looks good to me.

Reviewed-by: Elijah Newren <newren@gmail.com>

>
> Thanks!
>
>  * Victoria
>
> [1]
> https://lore.kernel.org/git/CABPp-BG0iDHf268UAnRyA=0y0T69YTc+bLMdxCmSbrL8s=9ziA@mail.gmail.com/
>
> [2]
> https://lore.kernel.org/git/a075091c-d0d4-db5d-fa21-c9d6c90c343e@gmail.com/
>
> Victoria Dye (9):
>   reset: fix validation in sparse index test
>   reset: reorder wildcard pathspec conditions
>   clean: integrate with sparse index
>   checkout-index: expand sparse checkout compatibility tests
>   checkout-index: add --ignore-skip-worktree-bits option
>   checkout-index: integrate with sparse index
>   update-index: add tests for sparse-checkout compatibility
>   update-index: integrate with sparse index
>   update-index: reduce scope of index expansion in do_reupdate
>
>  Documentation/git-checkout-index.txt     |  10 +-
>  builtin/checkout-index.c                 |  41 +++-
>  builtin/clean.c                          |   3 +
>  builtin/reset.c                          |  12 +-
>  builtin/update-index.c                   |  17 +-
>  read-cache.c                             |  10 +-
>  t/perf/p2000-sparse-operations.sh        |   2 +
>  t/t1092-sparse-checkout-compatibility.sh | 282 ++++++++++++++++++++++-
>  8 files changed, 360 insertions(+), 17 deletions(-)
>
>
> base-commit: dcc0cd074f0c639a0df20461a301af6d45bd582e
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1109%2Fvdye%2Fsparse%2Fupdate-index_checkout-index_clean-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1109/vdye/sparse/update-index_checkout-index_clean-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1109
>
> Range-diff vs v1:
>
>   1:  eefb6ab4c61 =  1:  eefb6ab4c61 reset: fix validation in sparse index test
>   2:  0194d894c2f =  2:  0194d894c2f reset: reorder wildcard pathspec conditions
>   3:  52aec13d18e =  3:  52aec13d18e clean: integrate with sparse index
>   4:  e6a8671f6be !  4:  d964507fdcc checkout-index: expand sparse checkout compatibility tests
>      @@ Commit message
>
>           Add tests to cover `checkout-index`, with a focus on cases interesting in a
>           sparse checkout (e.g., files specified outside sparse checkout definition).
>      -    New tests are intended to serve as a baseline for expected behavior and
>      -    performance when integrating `checkout-index` with the sparse index.
>      +
>      +    New tests are intended to serve as a baseline for existing and/or expected
>      +    behavior and performance when integrating `checkout-index` with the sparse
>      +    index. Note that the test 'checkout-index --all' is marked as
>      +    'test_expect_failure', indicating that `update-index --all` will be modified
>      +    in a subsequent patch to behave as the test expects.
>
>           Signed-off-by: Victoria Dye <vdye@github.com>
>
>   5:  ec9a751e8dc !  5:  601888606d1 checkout-index: add --ignore-skip-worktree-bits option
>      @@ Metadata
>        ## Commit message ##
>           checkout-index: add --ignore-skip-worktree-bits option
>
>      -    Update `checkout-index --all` to no longer refresh files that have the
>      -    `skip-worktree` bit set. The newly-added `--ignore-skip-worktree-bits`
>      -    option, when used with `--all`, maintains the old behavior and checks out
>      -    all files regardless of `skip-worktree`.
>      +    Update `checkout-index` to no longer refresh files that have the
>      +    `skip-worktree` bit set, exiting with an error if `skip-worktree` filenames
>      +    are directly provided to `checkout-index`. The newly-added
>      +    `--ignore-skip-worktree-bits` option provides a mechanism to replicate the
>      +    old behavior, checking out *all* files specified (even those with
>      +    `skip-worktree` enabled).
>
>           The ability to toggle whether files should be checked-out based on
>           `skip-worktree` already exists in `git checkout` and `git restore` (both of
>      -    which have an `--ignore-skip-worktree-bits` option). Adding the option to
>      -    `checkout-index` (and changing the corresponding default behavior to respect
>      -    the `skip-worktree` bit) is especially helpful for sparse-checkout: it
>      -    prevents inadvertent creation of *all* files outside the sparse definition
>      -    on disk and eliminates the need to expand a sparse index by default when
>      -    using the `--all` option.
>      +    which have an `--ignore-skip-worktree-bits` option). The change to, by
>      +    default, ignore `skip-worktree` files is especially helpful for
>      +    sparse-checkout; it prevents inadvertent creation of files outside the
>      +    sparse definition on disk and eliminates the need to expand a sparse index
>      +    when using the `--all` option.
>
>           Internal usage of `checkout-index` in `git stash` and `git filter-branch` do
>           not make explicit use of files with `skip-worktree` enabled, so
>           `--ignore-skip-worktree-bits` is not added to them.
>
>      +    Helped-by: Elijah Newren <newren@gmail.com>
>           Signed-off-by: Victoria Dye <vdye@github.com>
>
>        ## Documentation/git-checkout-index.txt ##
>      @@ Documentation/git-checkout-index.txt: OPTIONS
>
>       +--ignore-skip-worktree-bits::
>       + Check out all files, including those with the skip-worktree bit
>      -+ set. Note: may only be used with `--all`; skip-worktree is
>      -+ ignored when explicit filenames are specified.
>      ++ set.
>       +
>        --stdin::
>         Instead of taking list of paths from the command line,
>      @@ builtin/checkout-index.c
>        #include "lockfile.h"
>        #include "quote.h"
>        #include "cache-tree.h"
>      +@@
>      + #define CHECKOUT_ALL 4
>      + static int nul_term_line;
>      + static int checkout_stage; /* default to checkout stage0 */
>      ++static int ignore_skip_worktree; /* default to 0 */
>      + static int to_tempfile;
>      + static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
>      +
>       @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
>      -  return -1;
>      - }
>      +  int namelen = strlen(name);
>      +  int pos = cache_name_pos(name, namelen);
>      +  int has_same_name = 0;
>      ++ int is_skipped = 1;
>      +  int did_checkout = 0;
>      +  int errs = 0;
>
>      --static int checkout_all(const char *prefix, int prefix_length)
>      -+static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_worktree)
>      - {
>      -  int i, errs = 0;
>      -  struct cache_entry *last_ce = NULL;
>      +@@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
>      +                  break;
>      +          has_same_name = 1;
>      +          pos++;
>      ++         if (!ignore_skip_worktree && ce_skip_worktree(ce))
>      ++                 break;
>      ++         is_skipped = 0;
>      +          if (ce_stage(ce) != checkout_stage
>      +              && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
>      +                  continue;
>      +@@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
>      +          fprintf(stderr, "git checkout-index: %s ", name);
>      +          if (!has_same_name)
>      +                  fprintf(stderr, "is not in the cache");
>      ++         else if (is_skipped)
>      ++                 fprintf(stderr, "has skip-worktree enabled; "
>      ++                                 "use '--ignore-skip-worktree-bits' to checkout");
>      +          else if (checkout_stage)
>      +                  fprintf(stderr, "does not exist at stage %d",
>      +                          checkout_stage);
>       @@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix_length)
>         ensure_full_index(&the_index);
>         for (i = 0; i < active_nr ; i++) {
>      @@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix
>                 if (ce_stage(ce) != checkout_stage
>                     && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
>                         continue;
>      -@@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>      -  int i;
>      -  struct lock_file lock_file = LOCK_INIT;
>      -  int all = 0;
>      -+ int ignore_skip_worktree = 0;
>      -  int read_from_stdin = 0;
>      -  int prefix_length;
>      -  int force = 0, quiet = 0, not_new = 0;
>       @@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>         struct option builtin_checkout_index_options[] = {
>                 OPT_BOOL('a', "all", &all,
>      @@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, co
>                 OPT__FORCE(&force, N_("force overwrite of existing files"), 0),
>                 OPT__QUIET(&quiet,
>                         N_("no warning for existing files and files not in index")),
>      -@@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>      +
>      + ## t/t1092-sparse-checkout-compatibility.sh ##
>      +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout-index inside sparse definition' '
>      + test_expect_success 'checkout-index outside sparse definition' '
>      +  init_repos &&
>
>      -          if (all)
>      -                  die("git checkout-index: don't mix '--all' and explicit filenames");
>      -+         if (ignore_skip_worktree)
>      -+                 die("git checkout-index: don't mix '--ignore-skip-worktree-bits' and explicit filenames");
>      -          if (read_from_stdin)
>      -                  die("git checkout-index: don't mix '--stdin' and explicit filenames");
>      -          p = prefix_path(prefix, prefix_length, arg);
>      -@@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>      -  }
>      +- # File does not exist on disk yet for sparse checkouts, so checkout-index
>      +- # succeeds without -f
>      +- test_sparse_match git checkout-index -- folder1/a &&
>      ++ # Without --ignore-skip-worktree-bits, outside-of-cone files will trigger
>      ++ # an error
>      ++ test_sparse_match test_must_fail git checkout-index -- folder1/a &&
>      ++ test_i18ngrep "folder1/a has skip-worktree enabled" sparse-checkout-err &&
>      ++ test_path_is_missing folder1/a &&
>      ++
>      ++ # With --ignore-skip-worktree-bits, outside-of-cone files are checked out
>      ++ test_sparse_match git checkout-index --ignore-skip-worktree-bits -- folder1/a &&
>      +  test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
>      +  test_cmp sparse-checkout/folder1/a full-checkout/folder1/a &&
>
>      -  if (all)
>      --         err |= checkout_all(prefix, prefix_length);
>      -+         err |= checkout_all(prefix, prefix_length, ignore_skip_worktree);
>      +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout-index outside sparse definition' '
>      +  run_on_sparse mkdir -p folder1 &&
>      +  run_on_all cp ../new-a folder1/a &&
>
>      -  if (pc_workers > 1)
>      -          err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
>      -
>      - ## t/t1092-sparse-checkout-compatibility.sh ##
>      +- test_all_match test_must_fail git checkout-index -- folder1/a &&
>      +- test_all_match git checkout-index -f -- folder1/a &&
>      ++ test_all_match test_must_fail git checkout-index --ignore-skip-worktree-bits -- folder1/a &&
>      ++ test_all_match git checkout-index -f --ignore-skip-worktree-bits -- folder1/a &&
>      +  test_cmp sparse-checkout/folder1/a sparse-index/folder1/a &&
>      +  test_cmp sparse-checkout/folder1/a full-checkout/folder1/a
>      + '
>       @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout-index with folders' '
>         test_all_match test_must_fail git checkout-index -f -- folder1/
>        '
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout-index wi
>       - test_sparse_match test_path_is_missing folder1
>       + test_sparse_match test_path_is_missing folder1 &&
>       +
>      ++ # --ignore-skip-worktree-bits will cause `skip-worktree` files to be
>      ++ # checked out, causing the outside-of-cone `folder1` to exist on-disk
>       + test_all_match git checkout-index --ignore-skip-worktree-bits --all &&
>       + test_all_match test_path_exists folder1
>        '
>   6:  18c00fc9dd3 !  6:  b4b9086dcdc checkout-index: integrate with sparse index
>      @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char
>         int pos = cache_name_pos(name, namelen);
>         int has_same_name = 0;
>       + int is_file = 0;
>      +  int is_skipped = 1;
>         int did_checkout = 0;
>         int errs = 0;
>      -
>       @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
>                         break;
>                 has_same_name = 1;
>      @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char
>       +         if (S_ISSPARSEDIR(ce->ce_mode))
>       +                 break;
>       +         is_file = 1;
>      -          if (ce_stage(ce) != checkout_stage
>      -              && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
>      -                  continue;
>      +          if (!ignore_skip_worktree && ce_skip_worktree(ce))
>      +                  break;
>      +          is_skipped = 0;
>       @@ builtin/checkout-index.c: static int checkout_file(const char *name, const char *prefix)
>                 fprintf(stderr, "git checkout-index: %s ", name);
>                 if (!has_same_name)
>                         fprintf(stderr, "is not in the cache");
>       +         else if (!is_file)
>       +                 fprintf(stderr, "is a sparse directory");
>      -          else if (checkout_stage)
>      -                  fprintf(stderr, "does not exist at stage %d",
>      -                          checkout_stage);
>      -@@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix_length, int ignore_skip_w
>      +          else if (is_skipped)
>      +                  fprintf(stderr, "has skip-worktree enabled; "
>      +                                  "use '--ignore-skip-worktree-bits' to checkout");
>      +@@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix_length)
>         int i, errs = 0;
>         struct cache_entry *last_ce = NULL;
>
>   7:  3b734f89c0f !  7:  ff32952a21c update-index: add tests for sparse-checkout compatibility
>      @@ Commit message
>           update-index: add tests for sparse-checkout compatibility
>
>           Introduce tests for a variety of `git update-index` use cases, including
>      -    performance scenarios. Tests for `update-index add/remove` are specifically
>      -    focused on how `git stash` uses `git update-index` as a subcommand to
>      -    prepare for sparse index integration with `stash` in a future series.
>      +    performance scenarios. Tests are intended to exercise `update-index` with
>      +    options that change the commands interaction with the index (e.g.,
>      +    `--again`) and with files/directories inside and outside a sparse checkout
>      +    cone.
>      +
>      +    Of note is that these tests clearly establish the behavior of `git
>      +    update-index --add` with untracked, outside-of-cone files. Unlike `git add`,
>      +    which fails with an error when provided with such files, `update-index`
>      +    succeeds in adding them to the index. Additionally, the `skip-worktree` flag
>      +    is *not* automatically added to the new entry. Although this is pre-existing
>      +    behavior, there are a couple of reasons to avoid changing it in favor of
>      +    consistency with e.g. `git add`:
>      +
>      +    * `update-index` is low-level command for modifying the index; while it can
>      +      perform operations similar to those of `add`, it traditionally has fewer
>      +      "guardrails" preventing a user from doing something they may not want to
>      +      do (in this case, adding an outside-of-cone, non-`skip-worktree` file to
>      +      the index)
>      +    * `update-index` typically only exits with an error code if it is incapable
>      +      of performing an operation (e.g., if an internal function call fails);
>      +      adding a new file outside the sparse checkout definition is still a valid
>      +      operation, albeit an inadvisable one
>      +    * `update-index` does not implicitly set flags (e.g., `skip-worktree`) when
>      +      creating new index entries with `--add`; if flags need to be updated,
>      +      options like `--[no-]skip-worktree` allow a user to intentionally set them
>      +
>      +    All this to say that, while there are valid reasons to consider changing the
>      +    treatment of outside-of-cone files in `update-index`, there are also
>      +    sufficient reasons for leaving it as-is.
>
>           Co-authored-by: Derrick Stolee <dstolee@microsoft.com>
>           Signed-off-by: Victoria Dye <vdye@github.com>
>      @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git diff --cached
>        test_perf_on_all git blame $SPARSE_CONE/a
>        test_perf_on_all git blame $SPARSE_CONE/f3/a
>        test_perf_on_all git checkout-index -f --all
>      -+test_perf_on_all git update-index --add --remove
>      ++test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
>
>        test_done
>
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
>       + EOF
>       +
>       + # Create & modify folder1/a
>      ++ # Note that this setup is a manual way of reaching the erroneous
>      ++ # condition in which a `skip-worktree` enabled, outside-of-cone file
>      ++ # exists on disk. It is used here to ensure `update-index` is stable
>      ++ # and behaves predictably if such a condition occurs.
>       + run_on_sparse mkdir -p folder1 &&
>       + run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
>       + run_on_all ../edit-contents folder1/a &&
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
>       + run_on_sparse mkdir -p folder1 &&
>       + run_on_all ../edit-contents folder1/b &&
>       +
>      -+ # Similar to `git add`, the untracked out-of-cone file is added to the index
>      -+ # identically across sparse and non-sparse checkouts
>      ++ # The *untracked* out-of-cone file is added to the index because it does
>      ++ # not have a `skip-worktree` bit to signal that it should be ignored
>      ++ # (unlike in `git add`, which will fail due to the file being outside
>      ++ # the sparse checkout definition).
>       + test_all_match git update-index --add folder1/b &&
>       + test_all_match git status --porcelain=v2
>       +'
>       +
>      ++# NEEDSWORK: `--remove`, unlike the rest of `update-index`, does not ignore
>      ++# `skip-worktree` entries by default and will remove them from the index.
>      ++# The `--ignore-skip-worktree-entries` flag must be used in conjunction with
>      ++# `--remove` to ignore the `skip-worktree` entries and prevent their removal
>      ++# from the index.
>       +test_expect_success 'update-index --remove outside sparse definition' '
>       + init_repos &&
>       +
>      -+ # When `--ignore-skip-worktree-entries` is specified, out-of-cone files are
>      -+ # not removed from the index if they do not exist on disk
>      ++ # When --ignore-skip-worktree-entries is _not_ specified:
>      ++ # out-of-cone, not-on-disk files are removed from the index
>      ++ test_sparse_match git update-index --remove folder1/a &&
>      ++ cat >expect <<-EOF &&
>      ++ D       folder1/a
>      ++ EOF
>      ++ test_sparse_match git diff --cached --name-status &&
>      ++ test_cmp expect sparse-checkout-out &&
>      ++
>      ++ # Reset the state
>      ++ test_all_match git reset --hard &&
>      ++
>      ++ # When --ignore-skip-worktree-entries is specified, out-of-cone
>      ++ # (skip-worktree) files are ignored
>       + test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
>      -+ test_all_match git status --porcelain=v2 &&
>      ++ test_sparse_match git diff --cached --name-status &&
>      ++ test_must_be_empty sparse-checkout-out &&
>       +
>      -+ # When the flag is _not_ specified, out-of-cone, not-on-disk files are
>      -+ # removed from the index
>      -+ rm full-checkout/folder1/a &&
>      -+ test_all_match git update-index --remove folder1/a &&
>      -+ test_all_match git status --porcelain=v2 &&
>      ++ # Reset the state
>      ++ test_all_match git reset --hard &&
>       +
>      -+ # NOTE: --force-remove supercedes --ignore-skip-worktree-entries, removing
>      ++ # --force-remove supercedes --ignore-skip-worktree-entries, removing
>       + # a skip-worktree file from the index (and disk) when both are specified
>      -+ test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
>      -+ test_all_match git status --porcelain=v2
>      ++ # with --remove
>      ++ test_sparse_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
>      ++ cat >expect <<-EOF &&
>      ++ D       folder1/a
>      ++ EOF
>      ++ test_sparse_match git diff --cached --name-status &&
>      ++ test_cmp expect sparse-checkout-out
>       +'
>       +
>       +test_expect_success 'update-index with directories' '
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
>       + #   file and either triggers an error ("does not exist  and --remove not passed")
>       + #   or is ignored completely (when using --remove)
>       + test_all_match test_must_fail git update-index deep &&
>      -+ run_on_all test_must_fail git update-indexe folder1 &&
>      ++ run_on_all test_must_fail git update-index folder1 &&
>       + test_must_fail git -C full-checkout update-index --remove folder1 &&
>       + test_sparse_match git update-index --remove folder1 &&
>       + test_all_match git status --porcelain=v2
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
>       +test_expect_success 'update-index --again file outside sparse definition' '
>       + init_repos &&
>       +
>      -+ write_script edit-contents <<-\EOF &&
>      -+ echo text >>$1
>      -+ EOF
>      -+
>       + test_all_match git checkout -b test-reupdate &&
>       +
>       + # Update HEAD without modifying the index to introduce a difference in
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
>       + test_sparse_match git reset --soft update-folder1 &&
>       +
>       + # Because folder1/a differs in the index vs HEAD,
>      -+ # `git update-index --remove --again` will effectively perform
>      -+ # `git update-index --remove folder1/a` and remove the folder1/a
>      -+ test_sparse_match git update-index --remove --again &&
>      -+ test_sparse_match git status --porcelain=v2
>      ++ # `git update-index --no-skip-worktree --again` will effectively perform
>      ++ # `git update-index --no-skip-worktree folder1/a` and remove the skip-worktree
>      ++ # flag from folder1/a
>      ++ test_sparse_match git update-index --no-skip-worktree --again &&
>      ++ test_sparse_match git status --porcelain=v2 &&
>      ++
>      ++ cat >expect <<-EOF &&
>      ++ D       folder1/a
>      ++ EOF
>      ++ test_sparse_match git diff --name-status &&
>      ++ test_cmp expect sparse-checkout-out
>       +'
>       +
>       +test_expect_success 'update-index --cacheinfo' '
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca
>       + # Cannot add sparse directory, even in sparse index case
>       + test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
>       +
>      -+ # Sparse match only - because folder1/a is outside the sparse checkout
>      -+ # definition (and thus not on-disk), it will appear "deleted" in
>      -+ # unstaged changes.
>      -+ test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
>      -+ test_sparse_match git status --porcelain=v2
>      ++ # Sparse match only: the new outside-of-cone entry is added *without* skip-worktree,
>      ++ # so `git status` reports it as "deleted" in the worktree
>      ++ test_sparse_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
>      ++ test_sparse_match git status --porcelain=v2 &&
>      ++ cat >expect <<-EOF &&
>      ++ MD folder1/a
>      ++ EOF
>      ++ test_sparse_match git status --short -- folder1/a &&
>      ++ test_cmp expect sparse-checkout-out &&
>      ++
>      ++ # To return folder1/a to "normal" for a sparse checkout (ignored &
>      ++ # outside-of-cone), add the skip-worktree flag.
>      ++ test_sparse_match git update-index --skip-worktree folder1/a &&
>      ++ cat >expect <<-EOF &&
>      ++ S folder1/a
>      ++ EOF
>      ++ test_sparse_match git ls-files -t -- folder1/a &&
>      ++ test_cmp expect sparse-checkout-out
>       +'
>       +
>        test_expect_success 'merge, cherry-pick, and rebase' '
>   8:  c5b98e36516 !  8:  9ddc51a47d5 update-index: integrate with sparse index
>      @@ Metadata
>        ## Commit message ##
>           update-index: integrate with sparse index
>
>      -    Enable usage of the sparse index with `update-index`. Most variations of
>      +    Enable use of the sparse index with `update-index`. Most variations of
>           `update-index` work without explicitly expanding the index or making any
>           other updates in or outside of `update-index.c`.
>
>      @@ read-cache.c: static int add_index_entry_with_check(struct index_state *istate,
>                 if (!new_only)
>
>        ## t/t1092-sparse-checkout-compatibility.sh ##
>      -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is not expanded: blame' '
>      -  done
>      +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is not expanded: diff' '
>      +  ensure_not_expanded diff --cached
>        '
>
>       +test_expect_success 'sparse index is not expanded: update-index' '
>       + init_repos &&
>       +
>      ++ deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
>      ++ ensure_not_expanded update-index --cacheinfo 100644 $deep_a_oid deep/a &&
>      ++
>       + echo "test" >sparse-index/README.md &&
>       + echo "test2" >sparse-index/a &&
>       + rm -f sparse-index/deep/a &&
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
>       + ensure_not_expanded update-index --remove deep/a
>       +'
>       +
>      - # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>      - # in this scenario, but it shouldn't.
>      - test_expect_success 'reset mixed and checkout orphan' '
>      + test_expect_success 'sparse index is not expanded: blame' '
>      +  init_repos &&
>      +
>   9:  de7fc143562 !  9:  80697e9259e update-index: reduce scope of index expansion in do_reupdate
>      @@ Metadata
>        ## Commit message ##
>           update-index: reduce scope of index expansion in do_reupdate
>
>      -    Expand the full index (and redo reupdate operation) only if a sparse
>      -    directory in the index differs from HEAD. Only the index entries that differ
>      -    between the index and HEAD are updated when performing `git update-index
>      -    --again`, so unmodified sparse directories are safely skipped. The index
>      -    does need to be expanded when sparse directories contain changes, though,
>      -    because `update_one(...)` will not operate on sparse directory index
>      -    entries.
>      +    Replace unconditional index expansion in 'do_reupdate()' with one scoped to
>      +    only where a full index is needed. A full index is only required in
>      +    'do_reupdate()' when a sparse directory in the index differs from HEAD; in
>      +    that case, the index is expanded and the operation restarted.
>
>           Because the index should only be expanded if a sparse directory is modified,
>           add a test ensuring the index is not expanded when differences only exist
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
>       + ensure_not_expanded update-index --add --remove --again
>        '
>
>      - # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>      + test_expect_success 'sparse index is not expanded: blame' '
>
> --
> gitgitgadget

Re: [PATCH v2 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index'

[Derrick Stolee]

n 1/12/2022 10:02 PM, Elijah Newren wrote:
> On Tue, Jan 11, 2022 at 10:05 AM Victoria Dye via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> This series continues the work to integrate commands with the sparse index,
>> adding integrations with 'git clean', 'git checkout-index', and 'git
>> update-index'. These three commands, while useful in their own right, are
>> updated mainly because they're used in 'git stash'. A future series will
>> integrate sparse index with 'stash' directly, but its subcommands must be
>> integrated to avoid the performance cost of each one expanding and
>> collapsing the index.
>>
>> The series is broken up into 4 parts:
>>
>>  * Patches 1-2 are minor fixups to the 'git reset' sparse index integration
>>    in response to discussion [1] that came after the series was ready for
>>    merge to 'next'.
>>  * Patch 3 integrates 'git clean' with the sparse index.
>>  * Patches 4-6 integrate 'git checkout-index' with the sparse index and
>>    introduce a new '--ignore-skip-worktree-bits' option.
>>    * This involves changing the behavior of 'checkout-index' to respect
>>      'skip-worktree' by default (i.e., it won't check out 'skip-worktree'
>>      files). The '--ignore-skip-worktree-bits' option can be specified to
>>      force checkout of 'skip-worktree' files, if desired.
>>  * Patches 7-9 integrate 'git update-index' with the sparse index.
>>    * Note that, although this integrates the sparse index with
>>      '--cacheinfo', sparse directories still cannot be updated using that
>>      option (see the prior discussion [2] for more details on why)
>>
>>
>> Changes since V1
>> ================
>>
>>  * Changed 'checkout-index' to fail by default when given filenames of files
>>    with 'skip-worktree' enabled
>>    * These files can still be forcibly checked-out by using the
>>      '--ignore-skip-worktree-bits' option
>>    * Added/updated corresponding t1092 tests
>>  * Updated t1092 'update-index' tests
>>    * Mentioned where/why 'skip-worktree' files were manually created on-disk
>>      for testing purposes
>>    * Provided explanation as to what '--remove' does, and how it relates to
>>      '--ignore-skip-worktree-entries'; restructured corresponding test
>>    * Fixed typo 'update-indexe' -> 'update-index'
>>    * Removed unused 'edit-contents'
>>    * Changed '--again' test to not use '--remove' to avoid confusion over
>>      how/why it updates 'skip-worktree' entries
>>    * Added "set skip-worktree" step to '--cacheinfo' test to illustrate how
>>      it could be used to add a new outside-of-cone file and remain generally
>>      compliant with a sparse-checkout definition
>>    * Added '--cacheinfo' test to "ensure not expanded"
>>    * Moved t1092 test 'sparse index is not expanded: update-index' to avoid
>>      merge conflict
>>  * Updated p2000 test for 'update-index': added file argument
>>    * Without any file arguments, 'update-index' was effectively a no-op
>>  * Clarified reasoning behind changing/not changing behavior of update-index
>>    in sparse-checkouts
> 
> Nicely done!  You've addressed all my (voluminous) feedback from v1;
> this round looks good to me.
> 
> Reviewed-by: Elijah Newren <newren@gmail.com>

Thank you for that review, Elijah. I took this opportunity to
reread the series as well as the range-diff and I agree that
this version is ready to go.

Thanks,
-Stolee

Re: [PATCH v2 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index'

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> this version is ready to go.

Thanks, all.  I marked the topic as "Will merge to 'next'" in the
draft of "What's cooking" report.