Re: Bug report: `git restore --source --staged` deals poorly with sparse-checkout

[Elijah Newren <newren@gmail.com>]

On Tue, Oct 4, 2022 at 1:35 PM Victoria Dye <vdye@github.com> wrote:
>
[...]
> >> Thanks for reporting this! There are a few confusing things going on with
> >> 'restore' here.
> >>
> >> First is that the out-of-cone was even restored in the first place.

Yep, agreed.  I'll add this example to the Known bugs section.

[...]
> >> Theoretically, 'restore' (like 'checkout') should be limited to pathspecs
> >> inside the sparse-checkout patterns (per the documentation of
> >> '--ignore-skip-worktree-bits'), but 'Documentation' does not match them.
> >> Then, there's a difference between 'restore' and 'checkout' that doesn't
> >> seem intentional; both remove the 'SKIP_WORKTREE' flag from the file, but
> >> only 'checkout' creates the file on-disk (therefore avoiding the "deleted"
> >> status).
> >
> > Restoring only into the index (as I think `git restore --staged` is supposed
> > to do) is weird.
>
> 'git restore --staged' is intended to restore to both the worktree and index
> (per 183fb44fd2 (restore: add --worktree and --staged, 2019-04-25)). The bug
> you've identified is that it's not restoring to the worktree.

I think that commit message is easy to mis-read.  --staged means
restore to the index, and does not restore to the working tree unless
the user also specifies --worktree.  Duy considered adding a
`--no-worktree` option but decided against it.

(In constrast, `checkout` always restores to both places and gives no
way to do just one.  Defaulting to just the worktree and letting the
user pick was one of the two big warts that `restore` fixed relative
to `checkout` -- the other being the --no-overlay mode.)

[...]
> > If we now do `git restore --staged --source HEAD^` and that
> > command doesn't remove the `SKIP_WORKTREE` flag on any paths, that logically
> > means that we have modified the working copy, and I think `git
> > sparse-checkout disable` would agree with me.
>
> If you aren't using '--ignore-skip-worktree-bits', the entries with
> SKIP_WORKTREE shouldn't be touched in the first place. If you *do* specify
> it, by virtue of restoring to the working tree, SKIP_WORKTREE must be
> removed.
>
> But suppose you're doing something like 'git restore --staged --no-worktree
> --ignore-skip-worktree-bits --source HEAD^'. In that case:

That'd be `git restore --staged --source=HEAD^
--ignore-skip-worktree-bits -- <paths>`, but I think I understand the
point you're conveying...

> - you are restoring to the index
> - you are *not* restoring to the worktree
> - you're restoring files with SKIP_WORKTREE applied

minor nit: you may also be restoring files that do not exist in HEAD
or the index.  So, I'd rather say here "you're restoring files outside
the sparse specification" to handle those too.

> Right now, SKIP_WORKTREE is removed from the matched entries, but suppose

Right, any code outside of unpack-trees.c that tweaks index entries
tends to replace existing ones with new ones without carefully setting
or copying various flags (like SKIP_WORKTREE) over.  I suspect we just
have another case of that here.  sparse-checkouts are forcing us to
audit all these paths...

> (per your comment) it wasn't. That wouldn't mean that we've "modified the
> working copy"; the working tree is defined with respect to the index, and if
> the index entry says "I don't care about the worktree", then there are no
> differences to reflect.
>
> This raises an interesting question about the current behavior, though: if
> you restore a SKIP_WORKTREE entry with '--staged' and '--no-worktree',
> should we remove SKIP_WORKTREE? I'd lean towards "no", but I'm interested to
> hear other contributors' thoughts.

My current $0.02...

I'd say that, without the --ignore-skip-worktree-bits override, we
should behave like `git-add` and `git-rm` and not operate on paths
outside the sparse specification.  If the paths the user specified
don't match anything in the sparse specification, we should throw an
error and tell the user of the flag to use to override.

With the override...I'd agree that we should only update the index and
still keep (or appropriately set) the SKIP_WORKTREE bits.  Yes, it has
some weirdness (as Martin points out), but that's part of the reason
of requiring the --scope=all override, much as we do with `git-add`
and `git-rm`.  (The override for add/rm is currently named `--sparse`
so it's not quite the same, but we're planning on renaming all these
so that'll make it clearer that it's the same situation for them all.)