git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Victoria Dye <vdye@github.com>
Date
Jan 6, 2022, 15:07 UTC
Message-ID
<ff3faad1-f5e9-c4db-712d-11f8110d1945@github.com>
In-Reply-To
<CABPp-BGCCzgNJEaZ9fgHCkW52ws-ef+00GBhy+LZ5Tx+XkhqDw@mail.gmail.com>
Elijah Newren wrote:
Show 73 quoted lines
> 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.

Show 37 quoted lines
>> +
>>  --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

Show 69 quoted lines
>>                 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.

Previous: Elijah NewrenNext: Elijah Newren
Message 10 of 39 in “Sparse index: integrate with 'clean', 'checkout-index', 'update-index'”
  1. 0/9 Sparse index: integrate with 'clean', 'checkout-index', 'update-index'Victoria Dye via GitGitGadget, Jan 4, 2022
  2. 1/9 reset: fix validation in sparse index testVictoria Dye via GitGitGadget, Jan 4, 2022
  3. 2/9 reset: reorder wildcard pathspec conditionsVictoria Dye via GitGitGadget, Jan 4, 2022
  4. 3/9 clean: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 4, 2022
  5. 4/9 checkout-index: expand sparse checkout compatibility testsVictoria Dye via GitGitGadget, Jan 4, 2022
  6. Elijah NewrenJan 5, 2022
  7. Elijah NewrenJan 7, 2022
  8. 5/9 checkout-index: add --ignore-skip-worktree-bits optionVictoria Dye via GitGitGadget, Jan 4, 2022
  9. Elijah NewrenJan 6, 2022
  10. Victoria DyeJan 6, 2022
  11. Elijah NewrenJan 7, 2022
  12. 6/9 checkout-index: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 4, 2022
  13. Elijah NewrenJan 6, 2022
  14. 7/9 update-index: add tests for sparse-checkout compatibilityVictoria Dye via GitGitGadget, Jan 4, 2022
  15. Elijah NewrenJan 8, 2022
  16. Victoria DyeJan 10, 2022
  17. Elijah NewrenJan 10, 2022
  18. Victoria DyeJan 10, 2022
  19. Elijah NewrenJan 10, 2022
  20. 8/9 update-index: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 4, 2022
  21. Elijah NewrenJan 9, 2022
  22. Victoria DyeJan 10, 2022
  23. Elijah NewrenJan 10, 2022
  24. 9/9 update-index: reduce scope of index expansion in do_reupdateVictoria Dye via GitGitGadget, Jan 4, 2022
  25. Elijah NewrenJan 9, 2022
  26. Elijah NewrenJan 9, 2022
  27. 0/9 Sparse index: integrate with 'clean', 'checkout-index', 'update-index'Victoria Dye via GitGitGadget, Jan 11, 2022
  28. 1/9 reset: fix validation in sparse index testVictoria Dye via GitGitGadget, Jan 11, 2022
  29. 2/9 reset: reorder wildcard pathspec conditionsVictoria Dye via GitGitGadget, Jan 11, 2022
  30. 4/9 checkout-index: expand sparse checkout compatibility testsVictoria Dye via GitGitGadget, Jan 11, 2022
  31. 3/9 clean: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 11, 2022
  32. 6/9 checkout-index: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 11, 2022
  33. 9/9 update-index: reduce scope of index expansion in do_reupdateVictoria Dye via GitGitGadget, Jan 11, 2022
  34. 8/9 update-index: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 11, 2022
  35. 7/9 update-index: add tests for sparse-checkout compatibilityVictoria Dye via GitGitGadget, Jan 11, 2022
  36. 5/9 checkout-index: add --ignore-skip-worktree-bits optionVictoria Dye via GitGitGadget, Jan 11, 2022
  37. Elijah NewrenJan 13, 2022
  38. Derrick StoleeJan 27, 2022
  39. Junio C HamanoJan 27, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.