On Tue, Mar 16, 2021 at 2:17 PM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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_stage
With 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...
