patch, 9 partsSparse index: integrate with 'clean', 'checkout-index', 'update-index'

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

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

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

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

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

>
> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Victoria Dye <vdye@github.com>
> >

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

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

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

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

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

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

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

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

>
> Elijah Newren wrote:
> > On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Victoria Dye <vdye@github.com>
> >>

> >> +--ignore-skip-worktree-bits::
> >> +       Check out all files, including those with the skip-worktree bit
> >> +       set. Note: may only be used with `--all`; skip-worktree is
> >> +       ignored when explicit filenames are specified.
> >
> > Why this restriction?  What if the user ran
> >    git checkout-index -- '*.c'
> > That's not an explicit filename, but a glob.
> >
>
> `checkout-index` doesn't accept globs/pathspecs, so every provided argument
> must correspond exactly to an entry in the index.

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

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

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

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

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

>
> From: Victoria Dye <vdye@github.com>
>
> Introduce tests for a variety of `git update-index` use cases, including
> performance scenarios.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

>
> Elijah Newren wrote:
> > On Mon, Jan 10, 2022 at 7:47 AM Victoria Dye <vdye@github.com> wrote:
> >>
> >> Elijah Newren wrote:
> >>> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> >>> <gitgitgadget@gmail.com> wrote:
> >>>>
> >>>> From: Victoria Dye <vdye@github.com>
> >>>>

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

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

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

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

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

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

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

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

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

>
> From: Victoria Dye <vdye@github.com>
>
> Expand the full index (and redo reupdate operation) only if a sparse
> directory in the index differs from HEAD.

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

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

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

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

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

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