Re: Possible regression for sparse-checkout in git 2.27.0

[Elijah Newren <newren@gmail.com>]

Hi,

On Wed, Jun 3, 2020 at 8:36 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/3/2020 12:37 AM, Elijah Newren wrote:
> > I think it'd be more natural to run
>
> >   git clone --filter=blob:none --sparse
> > https://github.com/r-spacex/launch-timeline.git
>
> > in place of the combination of
>
> >   git clone --filter=blob:none --no-checkout
> > https://github.com/r-spacex/launch-timeline.git
> >   git sparse-checkout init --cone
>
> > since the --sparse flag was added just for this kind of case -- to do
> > a clone but start with only a few things checked out.  It's easier, is
> > the route we're moving towards, and as a bonus also happens to work.
>
> Just one warning: the --sparse option in "git clone" does not currently
> enable core.sparseCheckoutCone, so running "git sparse-checkout init --cone"
> afterwards is a good idea, or else your "git sparse-checkout (set|add)"
> commands will not behave the way you expect.
>
> (I think that I will propose a change in behavior to make it do so during
> this release cycle.)
>
> > A bit of a side note, or a few of them, but this command of yours is broken:
> >   git sparse-checkout set README.md
> > because --cone mode means you are specifying *directories* that should
> > be checked out.  Currently, this gives no error, it instead silently
> > drops you back to non-cone mode, which seems bad to me.
> > sparse-checkout should provide some kind of error -- or at very least
> > a warning -- when you make that mistake.
>
> > Now let's talk about the commit in question that changed behavior
> > here.  The point of sparse-checkout is never to switch branches or
> > checkout new commits; all it does is update which paths are in the
> > current working directory.  A related point to this is it should never
> > add or remove entries from the index and shouldn't change any hashes
> > of files in the index.  It used to violate this, at first via an
> > implementation that was literally invoking `git read-tree -mu HEAD` in
> > a subprocess, and then later using internal code equivalent to
> > invoking that command in a subprocess.  But by violating the
> > leave-index-entries-alone mandate, it left folks who were in the
> > middle of a rebase and wanted to update their sparse-checkout to
> > include some more directories in their working tree in a precarious
> > spot -- if they didn't update, then they didn't have the files
> > necessary to build, and if they did forcibly update via `git read-tree
> > -mu HEAD` then their staged changes would all get wiped out.  I spent
> > some quality time helping users recover their files and teaching them
> > about the git storage model.
> >
> > So that brings us back to your original question.  When you said
> > --no-checkout, it means that there is no commit checked out and the
> > index is empty.  update_sparsity() is correctly toggling the
> > SKIP_WORKTREE bits for the existing index entries that don't match the
> > sparsity patterns, and it is correctly calling check_updates().
> > check_updates() is correctly checking for files currently in the index
> > which have toggled to being needed in the current worktree so that it
> > can issue downloads related to promisor packs.  The problem is just
> > that there aren't any index entries to begin with, so there are no
> > SKIP_WORKTREE bits to update, and thus no files that need to be
> > downloaded.
> >
> > It seems a bit risky to make sparse-checkout start doing
> > checkout/switch behavior and adding entries to the index.  There's a
> > couple ways forward.  One, we could decide this is a special edge or
> > corner case where we allow it: if the index is completely empty, then
> > there's no data to lose and thus we could make `git sparse-checkout
> > init [--cone]` in that one case use the old 'read-tree -mu HEAD'
> > logic.  Alternatively, we could just require users to run 'git reset
> > --hard' at the end of your script.
> >
> > Stolee: Thoughts?
>
> I agree that using "--sparse" instead of "--no-checkout" is the
> best way forward for now, but I'll classify that as a workaround
> and not necessarily the end of the conversation.

Agreed.

> In general, the commit in question is doing something extremely
> valuable for common situations, like rebase that you mention.
> I also think that this change in behavior is warranted by the
> clear warning placed at the top of the docs [1]:
>
>         THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE
>         BEHAVIOR OF OTHER COMMANDS IN THE PRESENCE OF SPARSE-
>         CHECKOUTS, WILL LIKELY CHANGE IN THE FUTURE.
>
> [1] https://git-scm.com/docs/git-sparse-checkout#_description
>
> Of course, that's just us covering our butts as we push the
> feature forward. It doesn't stop users (especially those that
> are bravely using the feature) from feeling pain that might
> be avoidable.
>
> To wrap up, it's unfortunate that the behavior changed. If it
> is easy to detect and handle this special case, then it _may_
> be worth doing to avoid disrupting users. Elijah: could you
> spend at most an hour trying to see how difficult this would be?

I actually spent...um...I don't know, but quite a bit longer than an
hour on this today (er, um, yesterday now).  I kept feeling that we
would be painting ourselves into a bad corner, or violating some
important invariant, and was also worried for a bit that this was just
a canary pointing out other important bugs that were ready to spring
on us once partial clones and sparse-checkouts were used more heavily
together.  So I dug around a fair amount.  jrnieder's post today[1]
was very timely; it gave me a good way to frame what bothered me so
much.  It seems to explain why I tend to hate working in dir.c, but
don't mind merge-recursive.c quite as bad: merge-recursive.c is a bit
of a mess but it has intended invariants that can be discovered if you
dig far enough (which means, it should be fixable or at least
replaceable); dir.c by contrast is mostly just a pile of empirical
behavior bolted on as we went and attempting to modify or replace or
duplicate it is going to always be a big uphill battle.  I'm worried
about moving sparse-checkout in the direction of a pile of bolted on
behaviors and losing sight of high-level design principles.  In
particular, I think that restoring the old behavior would move us in
that direction, and make us more inconsistent with other aspects of
git.  However, that said, the new behavior is not good either; it's
just buggy in a new way (though it's limited to this unborn index
special case).

I've got a patch that I'll post soon; it's a simple two-line change,
accompanied by an 87-line commit message -- because it took a while to
explain the history, the intended invariants, how those invariants
were messed up in different ways historically, how we can restore the
desired properties that'll make it easier to reason about the code,
and how the new behavior after the patch also makes us more consistent
with other behavior already found in git.  I'll submit it as soon as
it passes testsuites on the relevant machines.


[1] https://lore.kernel.org/git/20200603212336.GD253041@google.com/