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

[Victoria Dye <vdye@github.com>]

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

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

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

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

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

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

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

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

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

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