Re: [PATCH v2 4/7] reset: integrate with sparse index

[Elijah Newren <newren@gmail.com>]

On Tue, Oct 5, 2021 at 6:21 AM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Victoria Dye <vdye@github.com>
>
> `reset --soft` does not modify the index, so no compatibility changes are
> needed for it to function without expanding the index. For all other reset
> modes (`--mixed`, `--hard`, `--keep`, `--merge`), the full index is
> explicitly expanded with `ensure_full_index` to maintain current behavior.

"to maintain current behavior"?  You are changing code here, which
suggests some kind of behavior is changing, but that description seems
to be claiming the opposite.  Is it some kind of preventative change
to add ensure_full_index calls in an additional place, with a later
patch in the series intending to remove the other one(s), so you're
making sure that later changes won't cause unwanted behavioral
changes?  Or was something else meant here?

If the above wasn't what you meant, but you're adding
ensure_full_index calls, does that suggest that we had some important
code paths that were not protected by such calls?  I thought Stolee
said we had them all covered (at least to the best of our knowledge),
so I'm curious if we just discovered we missed some.  If so, are there
other codepaths like this one where we missed protective
ensure_full_index calls?

> Additionally, the `read_cache()` check verifying an uncorrupted index is
> moved after argument parsing and preparing the repo settings. The index is
> not used by the preceding argument handling, but `read_cache()` does need to
> be run after enabling sparse index for the command and before resetting.

This seems to be discussing what code changes are being made, but not
why.  I'm guessing at the reasoning, but is it something along the
lines of:

"""
Also, make sure to read_cache() after setting
command_requires_full_index = 0, so that we don't unnecessarily expand
the index as part of our early index-corruption check.
"""

?

>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/reset.c | 10 +++++++---
>  cache-tree.c    |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 3b75d3b2f20..e1f2a2bb2c4 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -184,6 +184,7 @@ static int read_from_tree(const struct pathspec *pathspec,
>         opt.flags.override_submodule_config = 1;
>         opt.repo = the_repository;
>
> +       ensure_full_index(&the_index);
>         if (do_diff_cache(tree_oid, &opt))
>                 return 1;
>         diffcore_std(&opt);
> @@ -261,9 +262,6 @@ static void parse_args(struct pathspec *pathspec,
>         }
>         *rev_ret = rev;
>
> -       if (read_cache() < 0)
> -               die(_("index file corrupt"));
> -
>         parse_pathspec(pathspec, 0,
>                        PATHSPEC_PREFER_FULL |
>                        (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
> @@ -409,6 +407,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>         if (intent_to_add && reset_type != MIXED)
>                 die(_("-N can only be used with --mixed"));
>
> +       prepare_repo_settings(the_repository);
> +       the_repository->settings.command_requires_full_index = 0;
> +
> +       if (read_cache() < 0)
> +               die(_("index file corrupt"));
> +
>         /* Soft reset does not touch the index file nor the working tree
>          * at all, but requires them in a good order.  Other resets reset
>          * the index file to the tree object we are switching to. */
> diff --git a/cache-tree.c b/cache-tree.c
> index 90919f9e345..9be19c85b66 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -776,6 +776,7 @@ void prime_cache_tree(struct repository *r,
>         cache_tree_free(&istate->cache_tree);
>         istate->cache_tree = cache_tree();
>
> +       ensure_full_index(istate);
>         prime_cache_tree_rec(r, istate->cache_tree, tree);
>         istate->cache_changed |= CACHE_TREE_CHANGED;
>         trace2_region_leave("cache-tree", "prime_cache_tree", the_repository);
> --
> gitgitgadget
>