git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <stolee@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Victoria Dye <vdye@github.com>
Subject: Re: [PATCH 6/9] checkout-index: integrate with sparse index
Date: Wed, 5 Jan 2022 17:59:45 -0800	[thread overview]
Message-ID: <CABPp-BFT5Rd9pHGAfK7ymNWXs5AGRu5N+dmdrytSoRdkdKvpRQ@mail.gmail.com> (raw)
In-Reply-To: <18c00fc9dd373bd5cfb527cb7d672a5a1b3b0588.1641317820.git.gitgitgadget@gmail.com>

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

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

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

Patch looks good to me.

  reply	other threads:[~2022-01-06  2:00 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-04 17:36 [PATCH 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index' Victoria Dye via GitGitGadget
2022-01-04 17:36 ` [PATCH 1/9] reset: fix validation in sparse index test Victoria Dye via GitGitGadget
2022-01-04 17:36 ` [PATCH 2/9] reset: reorder wildcard pathspec conditions Victoria Dye via GitGitGadget
2022-01-04 17:36 ` [PATCH 3/9] clean: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-04 17:36 ` [PATCH 4/9] checkout-index: expand sparse checkout compatibility tests Victoria Dye via GitGitGadget
2022-01-05 21:04   ` Elijah Newren
2022-01-07 16:21     ` Elijah Newren
2022-01-04 17:36 ` [PATCH 5/9] checkout-index: add --ignore-skip-worktree-bits option Victoria Dye via GitGitGadget
2022-01-06  1:52   ` Elijah Newren
2022-01-06 15:07     ` Victoria Dye
2022-01-07 16:35       ` Elijah Newren
2022-01-04 17:36 ` [PATCH 6/9] checkout-index: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-06  1:59   ` Elijah Newren [this message]
2022-01-04 17:36 ` [PATCH 7/9] update-index: add tests for sparse-checkout compatibility Victoria Dye via GitGitGadget
2022-01-08 23:57   ` Elijah Newren
2022-01-10 15:47     ` Victoria Dye
2022-01-10 17:11       ` Elijah Newren
2022-01-10 18:01         ` Victoria Dye
2022-01-10 20:03           ` Elijah Newren
2022-01-04 17:36 ` [PATCH 8/9] update-index: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-09  1:49   ` Elijah Newren
2022-01-10 14:10     ` Victoria Dye
2022-01-10 15:52       ` Elijah Newren
2022-01-04 17:37 ` [PATCH 9/9] update-index: reduce scope of index expansion in do_reupdate Victoria Dye via GitGitGadget
2022-01-09  4:24   ` Elijah Newren
2022-01-09  4:41 ` [PATCH 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index' Elijah Newren
2022-01-11 18:04 ` [PATCH v2 " Victoria Dye via GitGitGadget
2022-01-11 18:04   ` [PATCH v2 1/9] reset: fix validation in sparse index test Victoria Dye via GitGitGadget
2022-01-11 18:04   ` [PATCH v2 2/9] reset: reorder wildcard pathspec conditions Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 3/9] clean: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 4/9] checkout-index: expand sparse checkout compatibility tests Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 5/9] checkout-index: add --ignore-skip-worktree-bits option Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 6/9] checkout-index: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 7/9] update-index: add tests for sparse-checkout compatibility Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 8/9] update-index: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 9/9] update-index: reduce scope of index expansion in do_reupdate Victoria Dye via GitGitGadget
2022-01-13  3:02   ` [PATCH v2 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index' Elijah Newren
2022-01-27 16:36     ` Derrick Stolee
2022-01-27 20:04       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABPp-BFT5Rd9pHGAfK7ymNWXs5AGRu5N+dmdrytSoRdkdKvpRQ@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=stolee@gmail.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).