Re: [PATCH v3 6/8] read-tree: narrow scope of index expansion for '--prefix'

From: Glen Choo <chooglen@google.com>
Date: Mar 4, 2022, 18:47 UTC
Message-ID: <kl6lilst4kt3.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <5353fb06-09e4-77bf-554c-1cd750158730@github.com>

Victoria Dye <vdye@github.com> writes:

Show 6 quoted lines

> There's an (admittedly subtle if you aren't familiar with sparse index)
> distinction between a "sparse directory" index entry and "a directory
> outside the sparse-checkout cone with SKIP_WORKTREE enabled on its entries".
> Ultimately, that's what necessitates the two checks but, as in [1], I want
> to use this as an opportunity to shed some light on what 'unpack_trees(...)'
> does.

Thanks! This explanation was really helpful.

Show 13 quoted lines

> Using our example above, suppose 'baz/' is partially expanded in the index,
> with the following index contents:
>
> H bar/f1
> S baz/deep/
> S baz/f2
> H foo
> H foo1
>
> If we use the prefix 'baz/' here, we actually traverse the trees properly:
> 'baz/deep/' and 'baz/f2' will be found and merged - no index expansion
> needed! But if we only checked '!path_in_cone_mode_sparse_checkout(...)', we
> would have expanded the index because 'baz/' is outside the sparse cone.

In particular, I didn't consider that a directory outside of the sparse-checkout cone could be partially expanded. This seems to be the crux of it, which is that even if the path is outside of the sparse-checkout clone, we can still get correct behavior (without expanding the index) if its entries are expanded...

Show 7 quoted lines

> This presents a problem because index expansion is *extremely* expensive -
> we should avoid it whenever possible. That's where checking
> 'index_name_pos(...)' comes in: if the directory is in the index as a sparse
> directory, the position is '>= 0' and 'ensure_full_index(...)' is called; if
> the directory is inside an existing sparse directory, the position will be
> '< 0' but the index will be expanded implicitly. In every other case, we
> avoid expanding the index and proceed with the merge as normal.

and because of this, we don't always need to expand the index when the path is outside of the cone, so my suggested patch expands the index in too many cases.

What I also didn't consider is that index_name_pos() doesn't _always_ expand the index, it only expands the index when the directory is inside a sparse directory entry.

So the side-effect of index_name_pos() is actually _exactly_ what we want. Granted, it would be clearer if we had a function that did _only_ 'expand if path is inside a sparse directory entry', but I suppose it's overkill.

(Purely optional suggestion) I wonder if we could add a test that can distinguish between 'always expand if --prefix is outside of the cone' vs 'expand only if path is outside of cone AND inside a sparse directory entry'. The scenario you described sounds perfect as a test case, though I don't know how feasible it is to set up.