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>,
	"Taylor Blau" <me@ttaylorr.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Victoria Dye" <vdye@github.com>
Subject: Re: [PATCH v3 7/8] reset: make --mixed sparse-aware
Date: Sat, 20 Nov 2021 14:02:30 -0800	[thread overview]
Message-ID: <CABPp-BEcWutcLJWJvO-DC5B59EO4focgCJG1OWYg080j8ti44Q@mail.gmail.com> (raw)
In-Reply-To: <6ef8e4e31d39386e3a6fa1e6c24acd2c005fbd54.1633641339.git.gitgitgadget@gmail.com>

On Thu, Oct 7, 2021 at 2:15 PM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Victoria Dye <vdye@github.com>
>
> Remove the `ensure_full_index` guard on `read_from_tree` and update `git
> reset --mixed` to ensure it can use sparse directory index entries wherever
> possible. Sparse directory entries are reset use `diff_tree_oid`, which

I am having trouble parsing this second sentence.  Was this meant to
be 'Sparse directory entries _which_ are reset use...'?

> requires `change` and `add_remove` functions to process the internal
> contents of the sparse directory. The `recursive` diff option handles cases
> in which `reset --mixed` must diff/merge files that are nested multiple
> levels deep in a sparse directory.
>
> The use of pathspecs with `git reset --mixed` introduces scenarios in which
> internal contents of sparse directories may be matched by the pathspec. In
> order to reset *all* files in the repo that may match the pathspec, the
> following conditions on the pathspec require index expansion before
> performing the reset:
>
> * "magic" pathspecs
> * wildcard pathspecs that do not match only in-cone files or entire sparse
>   directories
> * literal pathspecs matching something outside the sparse checkout
>   definition
>
> Helped-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/reset.c                          | 78 +++++++++++++++++++++++-
>  t/t1092-sparse-checkout-compatibility.sh | 17 ++++++
>  2 files changed, 93 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 0ac0de7dc97..60517e7e1d6 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -148,7 +148,9 @@ static void update_index_from_diff(struct diff_queue_struct *q,
>                  * If the file 1) corresponds to an existing index entry with
>                  * skip-worktree set, or 2) does not exist in the index but is
>                  * outside the sparse checkout definition, add a skip-worktree bit
> -                * to the new index entry.
> +                * to the new index entry. Note that a sparse index will be expanded
> +                * if this entry is outside the sparse cone - this is necessary
> +                * to properly construct the reset sparse directory.
>                  */
>                 pos = cache_name_pos(one->path, strlen(one->path));
>                 if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) ||
> @@ -166,6 +168,73 @@ static void update_index_from_diff(struct diff_queue_struct *q,
>         }
>  }
>
> +static int pathspec_needs_expanded_index(const struct pathspec *pathspec)
> +{
> +       unsigned int i, pos;
> +       int res = 0;
> +       char *skip_worktree_seen = NULL;
> +
> +       /*
> +        * When using a magic pathspec, assume for the sake of simplicity that
> +        * the index needs to be expanded to match all matchable files.
> +        */
> +       if (pathspec->magic)
> +               return 1;
> +
> +       for (i = 0; i < pathspec->nr; i++) {
> +               struct pathspec_item item = pathspec->items[i];
> +
> +               /*
> +                * If the pathspec item has a wildcard, the index should be expanded
> +                * if the pathspec has the possibility of matching a subset of entries inside
> +                * of a sparse directory (but not the entire directory).
> +                *
> +                * If the pathspec item is a literal path, the index only needs to be expanded
> +                * if a) the pathspec isn't in the sparse checkout cone (to make sure we don't
> +                * expand for in-cone files) and b) it doesn't match any sparse directories
> +                * (since we can reset whole sparse directories without expanding them).
> +                */
> +               if (item.nowildcard_len < item.len) {
> +                       for (pos = 0; pos < active_nr; pos++) {
> +                               struct cache_entry *ce = active_cache[pos];
> +
> +                               if (!S_ISSPARSEDIR(ce->ce_mode))
> +                                       continue;

This double loop over all pathspecs and over all index entries reminds
me of the original non-cone mode sparsity patterns.  Stolee introduced
cone mode patterns specifically to avoid the expensiveness of such
double loops (cf.
https://lore.kernel.org/git/19d664a5dada87a9a8dcf18d7548582275593f10.1566313865.git.gitgitgadget@gmail.com/).
Can one of the functions he added allow us to avoid this double loop,
or are there complications that don't allow this (e.g. the actually
SKIP_WORKTREE paths don't quite match the requested sparsity paths in
some cases, or here we are faced with just a leading path of multiple
index entries)?

> +                               /*
> +                                * If the pre-wildcard length is longer than the sparse
> +                                * directory name and the sparse directory is the first
> +                                * component of the pathspec, need to expand the index.
> +                                */
> +                               if (item.nowildcard_len > ce_namelen(ce) &&
> +                                   !strncmp(item.original, ce->name, ce_namelen(ce))) {
> +                                       res = 1;
> +                                       break;
> +                               }
> +
> +                               /*
> +                                * If the pre-wildcard length is shorter than the sparse
> +                                * directory and the pathspec does not match the whole
> +                                * directory, need to expand the index.
> +                                */
> +                               if (!strncmp(item.original, ce->name, item.nowildcard_len) &&
> +                                   wildmatch(item.original, ce->name, 0)) {
> +                                       res = 1;
> +                                       break;
> +                               }
> +                       }
> +               } else if (!path_in_cone_mode_sparse_checkout(item.original, &the_index) &&
> +                          !matches_skip_worktree(pathspec, i, &skip_worktree_seen))

Oh, so you can at least generally avoid the double loop.  That's good.
So is this just a case of wildcards are special and there isn't a way,
even in cone-mode, to avoid the double loop?

(Given that I'm so tardy in reviewing this, even if the answer is that
the double loop is avoidable, or if we just don't know, I'd be totally
fine with a 'TODO: consider whether this double loop could be avoided
in cone mode using some kind of variant of
path_in_cone_mode_sparse_checkout()')

> +                       res = 1;
> +
> +               if (res > 0)
> +                       break;
> +       }
> +
> +       free(skip_worktree_seen);
> +       return res;
> +}
> +
>  static int read_from_tree(const struct pathspec *pathspec,
>                           struct object_id *tree_oid,
>                           int intent_to_add)
> @@ -178,9 +247,14 @@ static int read_from_tree(const struct pathspec *pathspec,
>         opt.format_callback = update_index_from_diff;
>         opt.format_callback_data = &intent_to_add;
>         opt.flags.override_submodule_config = 1;
> +       opt.flags.recursive = 1;
>         opt.repo = the_repository;
> +       opt.change = diff_change;
> +       opt.add_remove = diff_addremove;
> +
> +       if (pathspec->nr && the_index.sparse_index && pathspec_needs_expanded_index(pathspec))
> +               ensure_full_index(&the_index);
>
> -       ensure_full_index(&the_index);
>         if (do_diff_cache(tree_oid, &opt))
>                 return 1;
>         diffcore_std(&opt);
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 4ac93874cb2..c9343ff5b9c 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -774,11 +774,28 @@ test_expect_success 'sparse-index is not expanded' '
>                 ensure_not_expanded reset --hard $ref || return 1
>         done &&
>
> +       ensure_not_expanded reset --mixed base &&
>         ensure_not_expanded reset --hard update-deep &&
>         ensure_not_expanded reset --keep base &&
>         ensure_not_expanded reset --merge update-deep &&
>         ensure_not_expanded reset --hard &&
>
> +       ensure_not_expanded reset base -- deep/a &&
> +       ensure_not_expanded reset base -- nonexistent-file &&
> +       ensure_not_expanded reset deepest -- deep &&
> +
> +       # Although folder1 is outside the sparse definition, it exists as a
> +       # directory entry in the index, so the pathspec will not force the
> +       # index to be expanded.
> +       ensure_not_expanded reset deepest -- folder1 &&
> +       ensure_not_expanded reset deepest -- folder1/ &&
> +
> +       # Wildcard identifies only in-cone files, no index expansion
> +       ensure_not_expanded reset deepest -- deep/\* &&
> +
> +       # Wildcard identifies only full sparse directories, no index expansion
> +       ensure_not_expanded reset deepest -- folder\* &&
> +
>         ensure_not_expanded checkout -f update-deep &&
>         test_config -C sparse-index pull.twohead ort &&
>         (
> --
> gitgitgadget

  reply	other threads:[~2021-11-20 22:02 UTC|newest]

Thread overview: 114+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-30 14:50 [PATCH 0/7] Sparse Index: integrate with reset Victoria Dye via GitGitGadget
2021-09-30 14:50 ` [PATCH 1/7] reset: behave correctly with sparse-checkout Kevin Willford via GitGitGadget
2021-09-30 18:34   ` Junio C Hamano
2021-10-01 14:55     ` Victoria Dye
2021-09-30 14:50 ` [PATCH 2/7] sparse-index: update command for expand/collapse test Victoria Dye via GitGitGadget
2021-09-30 19:17   ` Taylor Blau
2021-09-30 20:11     ` Victoria Dye
2021-09-30 21:32       ` Junio C Hamano
2021-09-30 22:59         ` Victoria Dye
2021-10-01  0:04           ` Junio C Hamano
2021-10-04 13:47             ` Victoria Dye
2021-10-01  9:14   ` Bagas Sanjaya
2021-09-30 14:50 ` [PATCH 3/7] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-09-30 14:50 ` [PATCH 4/7] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-09-30 14:50 ` [PATCH 5/7] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-09-30 14:51 ` [PATCH 6/7] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-10-01 15:03   ` Victoria Dye
2021-09-30 14:51 ` [PATCH 7/7] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-10-05 13:20 ` [PATCH v2 0/7] Sparse Index: integrate with reset Victoria Dye via GitGitGadget
2021-10-05 13:20   ` [PATCH v2 1/7] reset: behave correctly with sparse-checkout Kevin Willford via GitGitGadget
2021-10-05 19:30     ` Junio C Hamano
2021-10-05 21:59       ` Victoria Dye
2021-10-06 12:44         ` Junio C Hamano
2021-10-06  1:46     ` Elijah Newren
2021-10-06 20:09       ` Victoria Dye
2021-10-06 10:31     ` Bagas Sanjaya
2021-10-05 13:20   ` [PATCH v2 2/7] update-index: add --force-full-index option for expand/collapse test Victoria Dye via GitGitGadget
2021-10-06  2:00     ` Elijah Newren
2021-10-06 20:40       ` Victoria Dye
2021-10-08  3:42         ` Elijah Newren
2021-10-08 17:11           ` Junio C Hamano
2021-10-06 10:33     ` Bagas Sanjaya
2021-10-05 13:20   ` [PATCH v2 3/7] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-10-06  2:04     ` Elijah Newren
2021-10-05 13:20   ` [PATCH v2 4/7] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-10-06  2:15     ` Elijah Newren
2021-10-06 17:48       ` Junio C Hamano
2021-10-05 13:20   ` [PATCH v2 5/7] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-10-06  3:43     ` Elijah Newren
2021-10-06 20:56       ` Victoria Dye
2021-10-06 10:34     ` Bagas Sanjaya
2021-10-05 13:20   ` [PATCH v2 6/7] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-10-06  4:43     ` Elijah Newren
2021-10-07 14:34       ` Victoria Dye
2021-10-05 13:20   ` [PATCH v2 7/7] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-10-06 10:37     ` Bagas Sanjaya
2021-10-05 15:34   ` [PATCH v2 0/7] Sparse Index: integrate with reset Ævar Arnfjörð Bjarmason
2021-10-05 20:44     ` Victoria Dye
2021-10-06  5:46   ` Elijah Newren
2021-10-07 21:15   ` [PATCH v3 0/8] " Victoria Dye via GitGitGadget
2021-10-07 21:15     ` [PATCH v3 1/8] reset: rename is_missing to !is_in_reset_tree Victoria Dye via GitGitGadget
2021-10-07 21:15     ` [PATCH v3 2/8] reset: preserve skip-worktree bit in mixed reset Kevin Willford via GitGitGadget
2021-10-08  9:04       ` Junio C Hamano
2021-10-07 21:15     ` [PATCH v3 3/8] update-index: add --force-full-index option for expand/collapse test Victoria Dye via GitGitGadget
2021-10-08  2:50       ` Bagas Sanjaya
2021-10-08  5:24       ` Junio C Hamano
2021-10-08 15:47         ` Victoria Dye
2021-10-08 17:19           ` Junio C Hamano
2021-10-11 14:12             ` Derrick Stolee
2021-10-11 15:05               ` Victoria Dye
2021-10-11 15:24               ` Junio C Hamano
2021-10-07 21:15     ` [PATCH v3 4/8] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-10-07 21:15     ` [PATCH v3 5/8] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-10-07 21:15     ` [PATCH v3 6/8] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-10-08 11:09       ` Phillip Wood
2021-10-08 17:14         ` Victoria Dye
2021-10-08 18:31           ` Junio C Hamano
2021-10-09 11:18             ` Phillip Wood
2021-10-10 22:03               ` Junio C Hamano
2021-10-11 15:55                 ` Victoria Dye
2021-10-11 16:16                   ` Junio C Hamano
2021-10-12 10:16                 ` Phillip Wood
2021-10-12 19:15                   ` Junio C Hamano
2021-10-12 10:17           ` Phillip Wood
2021-10-07 21:15     ` [PATCH v3 7/8] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-11-20 22:02       ` Elijah Newren [this message]
2021-11-22 16:46         ` Victoria Dye
2021-10-07 21:15     ` [PATCH v3 8/8] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-10-11 20:30     ` [PATCH v4 0/8] Sparse Index: integrate with reset Victoria Dye via GitGitGadget
2021-10-11 20:30       ` [PATCH v4 1/8] reset: rename is_missing to !is_in_reset_tree Victoria Dye via GitGitGadget
2021-10-11 20:30       ` [PATCH v4 2/8] reset: preserve skip-worktree bit in mixed reset Victoria Dye via GitGitGadget
2021-10-22  4:19         ` Emily Shaffer
2021-10-25 15:59           ` Victoria Dye
2021-10-11 20:30       ` [PATCH v4 3/8] sparse-index: update command for expand/collapse test Victoria Dye via GitGitGadget
2021-10-11 20:30       ` [PATCH v4 4/8] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-10-11 20:30       ` [PATCH v4 5/8] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-10-11 20:30       ` [PATCH v4 6/8] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-10-11 20:30       ` [PATCH v4 7/8] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-10-11 20:30       ` [PATCH v4 8/8] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-10-27 14:39       ` [PATCH v5 0/8] Sparse Index: integrate with reset Victoria Dye via GitGitGadget
2021-10-27 14:39         ` [PATCH v5 1/8] reset: rename is_missing to !is_in_reset_tree Victoria Dye via GitGitGadget
2021-10-27 14:39         ` [PATCH v5 2/8] reset: preserve skip-worktree bit in mixed reset Victoria Dye via GitGitGadget
2021-10-27 14:39         ` [PATCH v5 3/8] sparse-index: update command for expand/collapse test Victoria Dye via GitGitGadget
2021-10-27 14:39         ` [PATCH v5 4/8] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-10-27 14:39         ` [PATCH v5 5/8] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-10-27 14:39         ` [PATCH v5 6/8] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-10-27 14:39         ` [PATCH v5 7/8] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-11-20 22:05           ` Elijah Newren
2021-11-22 16:54             ` Victoria Dye
2021-10-27 14:39         ` [PATCH v5 8/8] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-11-20 22:13         ` [PATCH v5 0/8] Sparse Index: integrate with reset Elijah Newren
2021-11-29 15:52         ` [PATCH v6 " Victoria Dye via GitGitGadget
2021-11-29 15:52           ` [PATCH v6 1/8] reset: rename is_missing to !is_in_reset_tree Victoria Dye via GitGitGadget
2021-11-29 15:52           ` [PATCH v6 2/8] reset: preserve skip-worktree bit in mixed reset Victoria Dye via GitGitGadget
2021-11-29 15:52           ` [PATCH v6 3/8] sparse-index: update command for expand/collapse test Victoria Dye via GitGitGadget
2021-11-29 15:52           ` [PATCH v6 4/8] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-11-29 15:52           ` [PATCH v6 5/8] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-11-29 15:52           ` [PATCH v6 6/8] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-11-29 15:52           ` [PATCH v6 7/8] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-11-29 15:52           ` [PATCH v6 8/8] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-11-29 18:37           ` [PATCH v6 0/8] Sparse Index: integrate with reset Elijah Newren
2021-11-29 19:44             ` Victoria Dye
2021-11-30  3:59               ` Elijah Newren
2021-12-01 20:26               ` Ævar Arnfjörð Bjarmason

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-BEcWutcLJWJvO-DC5B59EO4focgCJG1OWYg080j8ti44Q@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=avarab@gmail.com \
    --cc=bagasdotme@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.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).