Re: [PATCH 06/27] checkout-index: ensure full index
- From
Elijah Newren <newren@gmail.com>
- Date
- Mar 17, 2021, 17:50 UTC
- Message-ID
- <CABPp-BF=-2qD9YodpnhYP49EnKEV=MOFDMUnB-agd-d69jVeQA@mail.gmail.com>
- In-Reply-To
- <bfa0164cc3c167e383cdb5405526202432ae624e.1615929436.git.gitgitgadget@gmail.com>
On Tue, Mar 16, 2021 at 2:17 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 25 quoted lines
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Before we iterate over all cache entries, ensure that the index is not
> sparse. This loop in checkout_all() might be safe to iterate over a
> sparse index, but let's put this protection here until it can be
> carefully tested.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> builtin/checkout-index.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 023e49e271c2..ba31a036f193 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -119,6 +119,7 @@ static void checkout_all(const char *prefix, int prefix_length)
> int i, errs = 0;
> struct cache_entry *last_ce = NULL;
>
> + ensure_full_index(&the_index);
> for (i = 0; i < active_nr ; i++) {
> struct cache_entry *ce = active_cache[i];
> if (ce_stage(ce) != checkout_stageWith the caveat in the commit message, this change looks okay, but checkout-index may be buggy regardless of the presence of ensure_full_index(). If ensure_full_index() really is needed here because it needs to operate on all SKIP_WORKTREE paths and not just leading directories, that's because it's writing all those SKIP_WORKTREE entries to the working tree. When it writes them to the working tree, is it clearing the SKIP_WORKTREE bit? If not, we're in a bit of a pickle...
Might be nice to add a /* TODO: audit if this is needed; if it is, we may have other bugs... */ or something like that. But then again, perhaps you're considering all uses of ensure_full_index() to be need-to-be-reaudited codepaths? If so, and we determine we really do need one and want to keep it indefinitely, will we mark those with a comment about why it's considered correct?
I just want a way to know what still needs to be audited and what doesn't without doing a lot of history spelunking...