Sparse index: integrate with 'clean', 'checkout-index', 'update-index'

From: Victoria Dye <[email protected]>

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 <[email protected]>

---
 t/perf/p2000-sparse-operations.sh        |  1 +
 t/t1092-sparse-checkout-compatibility.sh | 54 ++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

t/perf/p2000-sparse-operations.sh +1 -0

index cb777c74a24..54f8602f3c1 100755
@@ -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

t/t1092-sparse-checkout-compatibility.sh +10 -0

index 05587361452..db7ad41109b 100755
@@ -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 &&

t/t1092-sparse-checkout-compatibility.sh +12 -0

+       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

t/t1092-sparse-checkout-compatibility.sh +2 -1

+'
+
 test_expect_success 'clean' '
        init_repos &&
 
--

gitgitgadget

Quoted 3 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'cherry-pick with conflicts' ' · line 804

Quoted 5 lines

+'

t/t1092-sparse-checkout-compatibility.sh +4 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'cherry-pick with conflicts' ' · line 809

Quoted 5 lines

t/t1092-sparse-checkout-compatibility.sh +5 -0

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

From: Victoria Dye <[email protected]>

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 <[email protected]>

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

Documentation/git-checkout-index.txt +4 -2

index 4d33e7be0f5..2815f3d4b19 100644
@@ -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::

Documentation/git-checkout-index.txt +1 -0

+
 --stdin::
        Instead of taking list of paths from the command line,
        read list of paths from the standard input.  Paths are

builtin/checkout-index.c +2 -1

index e21620d964e..2053a80103a 100644
@@ -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;

builtin/checkout-index.c +6 -1

                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,

t/t1092-sparse-checkout-compatibility.sh +0 -3

index db7ad41109b..fad61d96107 100755
@@ -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.

Quoted 11 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

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

Quoted 46 lines

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 <[email protected]>

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

Documentation/git-checkout-index.txt +4 -2

index 4d33e7be0f5..2815f3d4b19 100644
@@ -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.
 

Documentation/git-checkout-index.txt OPTIONS · line 67

Quoted 4 lines

Documentation/git-checkout-index.txt +4 -0

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

Quoted 20 lines

Documentation/git-checkout-index.txt +1 -0

+
 --stdin::
        Instead of taking list of paths from the command line,
        read list of paths from the standard input.  Paths are

builtin/checkout-index.c +1 -0

index e21620d964e..2053a80103a 100644
@@ -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;
 }
 

builtin/checkout-index.c static int checkout_all(const char *prefix, int prefix_length) · line 130

Quoted 11 lines

builtin/checkout-index.c +3 -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.

Quoted 49 lines

builtin/checkout-index.c +6 -1

                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,

t/t1092-sparse-checkout-compatibility.sh +0 -3

index db7ad41109b..fad61d96107 100755
@@ -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.

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'checkout-index with folders' ' · line 809

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +5 -2

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

Quoted 5 lines · Elijah Newren

Quoted 3 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

Quoted 5 lines

Quoted 4 lines

+--ignore-skip-worktree-bits::

Documentation/git-checkout-index.txt +3 -0

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

Quoted 4 lines

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.

Quoted 20 lines

Quoted 20 lines

Documentation/git-checkout-index.txt +1 -0

+
 --stdin::
        Instead of taking list of paths from the command line,
        read list of paths from the standard input.  Paths are

builtin/checkout-index.c +1 -0

index e21620d964e..2053a80103a 100644
@@ -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;
 }
 

Quoted 17 lines

builtin/checkout-index.c static int checkout_all(const char *prefix, int prefix_length) · line 130

Quoted 11 lines

builtin/checkout-index.c +3 -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/[email protected]/
[2] https://git-scm.com/docs/git-checkout-index#Documentation/git-checkout-index.txt-Toupdateandrefreshonlythefilesalreadycheckedout

Quoted 68 lines

Quoted 49 lines

builtin/checkout-index.c +6 -1

                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,

t/t1092-sparse-checkout-compatibility.sh +0 -3

index db7ad41109b..fad61d96107 100755
@@ -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.

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'checkout-index with folders' ' · line 809

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +5 -2

-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

Quoted 1 line

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.

From: Victoria Dye <[email protected]>

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 <[email protected]>

---
 builtin/checkout-index.c                 | 28 ++++++++++++++++++++++--
 t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++-
 2 files changed, 36 insertions(+), 3 deletions(-)

builtin/checkout-index.c +11 -2

index 2053a80103a..9c5657ccf22 100644
@@ -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);
+

builtin/checkout-index.c +5 -0

+               }
+
                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");
        }

t/t1092-sparse-checkout-compatibility.sh +10 -2

index fad61d96107..6ecf1f2bf8e 100755
@@ -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

From: Victoria Dye <[email protected]>

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 <[email protected]>

Signed-off-by Victoria Dye <[email protected]>

---
 t/perf/p2000-sparse-operations.sh        |   1 +
 t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
 2 files changed, 126 insertions(+)

t/perf/p2000-sparse-operations.sh +1 -0

index 54f8602f3c1..7dbed330160 100755
@@ -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

t/t1092-sparse-checkout-compatibility.sh +1 -0

index 6ecf1f2bf8e..6804ab23a27 100755
@@ -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' '

t/t1092-sparse-checkout-compatibility.sh +7 -0

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

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh +2 -1

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

gitgitgadget

Quoted 5 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

Introduce tests for a variety of git update-index use cases, including performance scenarios.

Makes sense.

Quoted 3 lines

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.

Quoted 27 lines

Co-authored-by Derrick Stolee <[email protected]>

Signed-off-by Victoria Dye <[email protected]>

---
 t/perf/p2000-sparse-operations.sh        |   1 +
 t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
 2 files changed, 126 insertions(+)

t/perf/p2000-sparse-operations.sh +1 -0

index 54f8602f3c1..7dbed330160 100755
@@ -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

t/t1092-sparse-checkout-compatibility.sh +1 -0

index 6ecf1f2bf8e..6804ab23a27 100755
@@ -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' '

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 643

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 656

Quoted 13 lines

t/t1092-sparse-checkout-compatibility.sh +13 -0

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

Quoted 11 lines

t/t1092-sparse-checkout-compatibility.sh +11 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 673

Quoted 6 lines

t/t1092-sparse-checkout-compatibility.sh +6 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 682

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +9 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 685

Quoted 2 lines

t/t1092-sparse-checkout-compatibility.sh +2 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 688

Quoted 5 lines

t/t1092-sparse-checkout-compatibility.sh +4 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 693

Quoted 4 lines

t/t1092-sparse-checkout-compatibility.sh +4 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 703

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 712

Quoted 9 lines

t/t1092-sparse-checkout-compatibility.sh +9 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 715

Quoted 3 lines

t/t1092-sparse-checkout-compatibility.sh +3 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 723

Quoted 8 lines

t/t1092-sparse-checkout-compatibility.sh +8 -0

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

Quoted 7 lines

t/t1092-sparse-checkout-compatibility.sh +7 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 735

Quoted 5 lines

t/t1092-sparse-checkout-compatibility.sh +5 -0

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

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 755

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

Quoted 7 lines

t/t1092-sparse-checkout-compatibility.sh +2 -1

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

gitgitgadget

Quoted 98 lines · Elijah Newren

Quoted 5 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

Introduce tests for a variety of git update-index use cases, including performance scenarios.

Makes sense.

Quoted 3 lines

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.

Quoted 27 lines

Co-authored-by Derrick Stolee <[email protected]>

Signed-off-by Victoria Dye <[email protected]>

---
 t/perf/p2000-sparse-operations.sh        |   1 +
 t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
 2 files changed, 126 insertions(+)

t/perf/p2000-sparse-operations.sh +1 -0

index 54f8602f3c1..7dbed330160 100755
@@ -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

t/t1092-sparse-checkout-compatibility.sh +1 -0

index 6ecf1f2bf8e..6804ab23a27 100755
@@ -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' '

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 643

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 656

Quoted 13 lines

t/t1092-sparse-checkout-compatibility.sh +13 -0

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

Quoted 11 lines

t/t1092-sparse-checkout-compatibility.sh +11 -0

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

Quoted 9 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 673

Quoted 6 lines

t/t1092-sparse-checkout-compatibility.sh +6 -0

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

Quoted 10 lines

$ 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"

Quoted 5 lines

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.

Quoted 29 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 682

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +9 -0

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

Quoted 9 lines

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/[email protected]/
[2] https://lore.kernel.org/git/163b42dfa21c306dc1dc573c5edfc8bda5c99fd0.1572432578.git.gitgitgadget@gmail.com/

Quoted 6 lines

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 685

Quoted 2 lines

t/t1092-sparse-checkout-compatibility.sh +2 -0

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

Quoted 9 lines

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.

Quoted 19 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 688

Quoted 6 lines

t/t1092-sparse-checkout-compatibility.sh +4 -0

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

Quoted 5 lines

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/[email protected]/
(copied from earlier in this message)
[3] https://lore.kernel.org/git/[email protected]/

Quoted 38 lines

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 693

Quoted 4 lines

t/t1092-sparse-checkout-compatibility.sh +4 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 703

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 712

Quoted 9 lines

t/t1092-sparse-checkout-compatibility.sh +9 -0

+
+       # 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 &&

Quoted 3 lines

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.

Quoted 6 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 715

Quoted 3 lines

t/t1092-sparse-checkout-compatibility.sh +3 -0

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

Quoted 12 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 723

Quoted 8 lines

t/t1092-sparse-checkout-compatibility.sh +8 -0

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

Quoted 7 lines

Quoted 7 lines

t/t1092-sparse-checkout-compatibility.sh +7 -0

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

Quoted 9 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 735

Quoted 5 lines

t/t1092-sparse-checkout-compatibility.sh +5 -0

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

Quoted 21 lines

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 755

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

+       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

Quoted 8 lines

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.

Quoted 7 lines

Quoted 7 lines

t/t1092-sparse-checkout-compatibility.sh +2 -1

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

gitgitgadget

Quoted 110 lines · Victoria Dye

Quoted 98 lines · Elijah Newren

Quoted 5 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

Introduce tests for a variety of git update-index use cases, including performance scenarios.

Makes sense.

Quoted 3 lines

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.

Quoted 27 lines

Co-authored-by Derrick Stolee <[email protected]>

Signed-off-by Victoria Dye <[email protected]>

---
 t/perf/p2000-sparse-operations.sh        |   1 +
 t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
 2 files changed, 126 insertions(+)

t/perf/p2000-sparse-operations.sh +1 -0

index 54f8602f3c1..7dbed330160 100755
@@ -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

t/t1092-sparse-checkout-compatibility.sh +1 -0

index 6ecf1f2bf8e..6804ab23a27 100755
@@ -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' '

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 643

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 656

Quoted 13 lines

t/t1092-sparse-checkout-compatibility.sh +13 -0

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

Quoted 11 lines

t/t1092-sparse-checkout-compatibility.sh +11 -0

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

Quoted 9 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 673

Quoted 6 lines

t/t1092-sparse-checkout-compatibility.sh +6 -0

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

Quoted 7 lines

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.

Quoted 4 lines

Quoted 4 lines

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.

Quoted 19 lines

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

Quoted 10 lines

$ 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"

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 39 lines

Quoted 29 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 682

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +9 -0

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

Quoted 9 lines

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.

Quoted 2 lines

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

Quoted 15 lines

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.

Quoted 2 lines

[1] https://lore.kernel.org/git/[email protected]/
[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.)

Quoted 31 lines

Quoted 6 lines

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 685

Quoted 2 lines

t/t1092-sparse-checkout-compatibility.sh +2 -0

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

Quoted 9 lines

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

Quoted 2 lines

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.

Quoted 25 lines

Quoted 19 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 688

Quoted 6 lines

t/t1092-sparse-checkout-compatibility.sh +4 -0

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

Quoted 5 lines

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?

Quoted 8 lines

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/[email protected]/
(copied from earlier in this message)
[3] https://lore.kernel.org/git/[email protected]/

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.

Quoted 128 lines

Quoted 38 lines

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 693

Quoted 4 lines

t/t1092-sparse-checkout-compatibility.sh +4 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 703

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 712

Quoted 9 lines

t/t1092-sparse-checkout-compatibility.sh +9 -0

+
+       # 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 &&

Quoted 3 lines

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.

Quoted 6 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 715

Quoted 3 lines

t/t1092-sparse-checkout-compatibility.sh +3 -0

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

Quoted 12 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 723

Quoted 8 lines

t/t1092-sparse-checkout-compatibility.sh +8 -0

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

Quoted 7 lines

Quoted 7 lines

t/t1092-sparse-checkout-compatibility.sh +7 -0

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

Quoted 9 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 735

Quoted 5 lines

t/t1092-sparse-checkout-compatibility.sh +5 -0

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

Quoted 21 lines

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

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

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'reset with wildcard pathspec' ' · line 755

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +10 -0

+       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

Quoted 8 lines

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.

Quoted 7 lines

Quoted 7 lines

t/t1092-sparse-checkout-compatibility.sh +2 -1

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

gitgitgadget

Quoted 8 lines · Elijah Newren

Quoted 7 lines · Victoria Dye

Quoted 5 lines · Elijah Newren

Quoted 3 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

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:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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)

Quoted 10 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

From: Victoria Dye <[email protected]>

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 <[email protected]>

---
 builtin/update-index.c                   |  3 +++
 read-cache.c                             | 10 +++++++---
 t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++++
 3 files changed, 22 insertions(+), 3 deletions(-)

builtin/update-index.c +3 -0

index 187203e8bb5..605cc693bbd 100644
@@ -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)

read-cache.c +7 -3

index cbe73f14e5e..b4600e954b6 100644
@@ -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)

t/t1092-sparse-checkout-compatibility.sh +1 -0

index 6804ab23a27..bc0741c970d 100755
@@ -1216,6 +1216,18 @@ test_expect_success 'sparse index is not expanded: blame' '
        done
 '
 
+test_expect_success 'sparse index is not expanded: update-index' '

t/t1092-sparse-checkout-compatibility.sh +1 -1

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

Quoted 72 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

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 <[email protected]>

---
 builtin/update-index.c                   |  3 +++
 read-cache.c                             | 10 +++++++---
 t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++++
 3 files changed, 22 insertions(+), 3 deletions(-)

builtin/update-index.c +3 -0

index 187203e8bb5..605cc693bbd 100644
@@ -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)

read-cache.c +7 -3

index cbe73f14e5e..b4600e954b6 100644
@@ -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)

t/t1092-sparse-checkout-compatibility.sh +0 -0

index 6804ab23a27..bc0741c970d 100755
@@ -1216,6 +1216,18 @@ test_expect_success 'sparse index is not expanded: blame' '
        done
 '
 

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'sparse index is not expanded: blame' ' · line 1229

Quoted 11 lines

t/t1092-sparse-checkout-compatibility.sh +11 -0

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

Quoted 7 lines

t/t1092-sparse-checkout-compatibility.sh +1 -1

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

Quoted 74 lines · Elijah Newren

Quoted 72 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

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 <[email protected]>

---
 builtin/update-index.c                   |  3 +++
 read-cache.c                             | 10 +++++++---
 t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++++
 3 files changed, 22 insertions(+), 3 deletions(-)

builtin/update-index.c +3 -0

index 187203e8bb5..605cc693bbd 100644
@@ -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)

read-cache.c +7 -3

index cbe73f14e5e..b4600e954b6 100644
@@ -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)

t/t1092-sparse-checkout-compatibility.sh +0 -0

index 6804ab23a27..bc0741c970d 100755
@@ -1216,6 +1216,18 @@ test_expect_success 'sparse index is not expanded: blame' '
        done
 '
 

Quoted 16 lines

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'sparse index is not expanded: blame' ' · line 1229

Quoted 11 lines

t/t1092-sparse-checkout-compatibility.sh +11 -0

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

Quoted 7 lines

Quoted 7 lines

t/t1092-sparse-checkout-compatibility.sh +1 -1

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

From: Victoria Dye <[email protected]>

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 <[email protected]>

---
 builtin/update-index.c                   | 14 +++++++++++---
 t/t1092-sparse-checkout-compatibility.sh |  5 ++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

builtin/update-index.c +11 -3

index 605cc693bbd..52ecc714d99 100644
@@ -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.

t/t1092-sparse-checkout-compatibility.sh +4 -2

index bc0741c970d..0863c9747c4 100755
@@ -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

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)