Re: [PATCH v3 02/12] sparse-index: include EXTENDED flag when expanding
- From
Elijah Newren <newren@gmail.com>
- Date
- May 18, 2021, 17:48 UTC
- Message-ID
- <CABPp-BHXNLTcQC=XmPoaVoy8xLKpru3yuJU1DfJVpOf_tJy8Yg@mail.gmail.com>
- In-Reply-To
- <ddeda8ec-bc21-1f62-5d0e-839be74f1ec8@gmail.com>
On Tue, May 18, 2021 at 7:57 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 30 quoted lines
> > On 5/17/2021 9:33 PM, Elijah Newren wrote: > > On Fri, May 14, 2021 at 11:31 AM Derrick Stolee via GitGitGadget > > <gitgitgadget@gmail.com> wrote: > >> > >> From: Derrick Stolee <dstolee@microsoft.com> > >> > >> 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.