Re: [PATCH v3 02/12] sparse-index: include EXTENDED flag when expanding

On Tue, May 18, 2021 at 7:57 AM Derrick Stolee [off-list ref] wrote:

On 5/17/2021 9:33 PM, Elijah Newren wrote:

quoted

On Fri, May 14, 2021 at 11:31 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

From: Derrick Stolee <redacted>

When creating a full index from a sparse one, we create cache entries
for every blob within a given sparse directory entry. These are
correctly marked with the CE_SKIP_WORKTREE flag, but they must also be
marked with the CE_EXTENDED flag to ensure that the skip-worktree bit is
correctly written to disk in the case that the index is not converted
back down to a sparse-index.

This seems odd to me.  When sparse-index is not involved and we are
just doing simple sparse checkouts, do we mark CE_SKIP_WORKTREE
entries with CE_EXTENDED?  I can't find any code that does so.

Is it possible that the setting of CE_EXTENDED is just a workaround
that happens to force the index to be written in cases where the logic
is otherwise thinking it can get away without one?  Or is there
something I'm missing about why the CE_EXTENDED flag is actually
needed here?

This is happening within the context of ensure_full_index(), so we
are creating new cache entries and want to mimic what they would
look like on-disk. Something within do_write_index() discovers that
since CE_SKIP_WORKTREE is set, then also CE_EXTENDED should be set
in order to ensure that the on-disk representation has enough room
for the CE_SKIP_WORKTREE bit.

Yeah, I think it's this part:

        /* reduce extended entries if possible */
        cache[i]->ce_flags &= ~CE_EXTENDED;
        if (cache[i]->ce_flags & CE_EXTENDED_FLAGS) {
            extended++;
            cache[i]->ce_flags |= CE_EXTENDED;
        }

I suppose this might not have a meaningful purpose other than when
I compare a full index against an expanded sparse-index and check
if their flags match.

Ah, you're just setting this flag in advance of do_write_index() being
called so that you can compare in memory values and check they match
without doing a write-to-disk-and-read-back cycle.  Makes sense, but
it'd be nice to see this in the commit message.