Re: [PATCH v5 7/8] reset: make --mixed sparse-aware

[Elijah Newren <newren@gmail.com>]

Sorry, one more thing...

On Wed, Oct 27, 2021 at 7:39 AM 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
> 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;
> +
> +                               /*
> +                                * 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))
> +                       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 5664ff8f039..44d5e11c762 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -781,11 +781,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\* &&
> +

You've added two testcases where a wildcard results in no index
expansion; should there also be a test where a wildcard results in
index expansion for completeness?