Re: [PATCH v3 11/12] wt-status: expand added sparse directory entries

On Tue, May 18, 2021 at 12:05 PM Derrick Stolee [off-list ref] wrote:

On 5/18/2021 2:26 PM, Derrick Stolee wrote:

quoted

On 5/17/2021 10:27 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>

It is difficult, but possible, to get into a state where we intend to
add a directory that is outside of the sparse-checkout definition. Add a

Then we need to fix that; allowing things to be added outside the
sparse-checkout definition is a bug[1][2].  That's an invariant I
believe we should maintain everywhere; things get really confusing to
users somewhere later down the road if we don't.  Matheus worked to
fix that with 'git add'; if there are other commands that need fixing
too, then we should also fix them.

[1] https://lore.kernel.org/git/CABPp-BFhyFiKSXdLM5q5t=ZKzr6V0pY7dbheierRaOHFbMEdkg@mail.gmail.com/ (local)
[2] https://lore.kernel.org/git/CABPp-BF0ZhbSs42R3Bw_r-hbhQ71qtbXSBqXdq0djyaan=8p=A@mail.gmail.com/ (local)

quoted

test to t1092-sparse-checkout-compatibility.sh that demonstrates this
using a combination of 'git reset --mixed' and 'git checkout --orphan'.

I think `git checkout --orphan` should just throw an error if
sparse-checkout is in use.  Allowing adding paths outside the
sparse-checkout set causes too much collateral and deferred confusion
for users.

I've been trying to strike an interesting balance of creating
performance improvements without changing behavior, trying to
defer those behavior changes to an isolated instance. I think
that approach is unavoidable with the 'git add' work that I
pulled out of this series and will return to soon.

However, here I think it would be too much to start throwing
an error in this case. I think that change is a bit too much.

The thing I can try to do, instead of the current approach, is
to not allow sparse directory entries to differ between the
index and HEAD. That will satisfy this case, but also a lot of
other painful cases.

I have no idea how to actually accomplish that, but I'll start
digging.

It didn't take much digging to discover that this is likely
impossible, or rather it would be a drastic change to make this
happen.

The immediate issue is trying to prevent sparse directory entries
from existing when the contained paths don't match what exists at
HEAD. However, in the 'git checkout --orphan' case, we are using
a full index for the unpack_trees() that updates the in-memory
index according to the paths at HEAD, then updates HEAD to point
to a non-existing ref. The sparse directories are only created as
part of convert_to_sparse() within do_write_index(). At that
point, there is no HEAD provided. Trying to load it from scratch
violates the fact that HEAD is being staged to change _after_ the
index updates in a command like 'git checkout'.

So, the drastic change to make this work would be to update the
index API to require a root tree to be provided whenever writing
the index. However, that doesn't make sense, either! What do we
do when in a conflicted state?

What if a user modifies HEAD manually to point to a new ref?

Such a change would couple the index to the concept of HEAD in
an unproductive way, I think. The index data structure exists
as a separate entity that is frequently _compared_ to HEAD, and
the solution presented in this patch presents a way to keep the
comparison of a sparse-index and HEAD to be the same as if we
had a full index.

So, after looking into it, I'm back in favor of this change and
forever allowing sparse cache entries to differ from HEAD,
because there is no way to avoid it.

Doh, thanks for digging in and entertaining the idea.  I'm worried
we'll get lots of confused users over the years from not being able to
do this, but you do make some good points.

I still think `git checkout --orphan` should be an error when in a
sparse checkout -- the point of a sparse checkout is that you only
care about a subset of files, whereas checkout --orphan fundamentally
says you are throwing away history but care about each and every file
since you are staging "changes" from all of them to include in some
new commit soon.  They just seem in strong opposition to me, and it
seems likely to result in surprises for some of the users when despite
the --orphan request and them fixing up the working directory how they
like, they get some new commit that contains files that aren't in
their working tree.  (In contrast, `git switch --orphan` would
probably be fine in a sparse checkout, precisely because it really
does empty everything).  However, I do agree with you that such a
change belongs in a separate series.  So, yes, your patch is good, and
I'll raise the behavioral change later.

(Sorry for being slow to respond and still not getting to all your
good reviews of my series; I'm a bit limited in my time for git right
now...)