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

[Elijah Newren <newren@gmail.com>]

Hi Marin!

On Tue, Oct 4, 2022 at 10:52 AM Martin von Zweigbergk
<martinvonz@google.com> wrote:
>
> On Tue, Oct 4, 2022 at 9:34 AM Victoria Dye <vdye@github.com> wrote:
>>
>> Glen Choo wrote:
>> > Filing a `git bugreport` on behalf of a user at $DAYJOB. I'm also pretty
>> > surprised by this behavior, perhaps someone who knows more could shed
>> > some light?
>> >
>> > What did you do before the bug happened? (Steps to reproduce your issue)
>> >
>> >   git clone git@github.com:git/git.git . &&
>> >   git sparse-checkout set t &&
>> >   git restore --source v2.38.0-rc1 --staged Documentation &&
>> >   git status
>> > ...>
>> > What happened instead? (Actual behavior)
>> >
>> > I saw a staged modification (Documentation/cmd-list.perl) and the same
>> > file reported as deleted in the working copy. Specifically,
>> >
>> >   $ git status
>> >
>> >   On branch master
>> >   Your branch is up to date with 'origin/master'.
>> >
>> >   You are in a sparse checkout with 64% of tracked files present.
>> >
>> >   Changes to be committed:
>> >     (use "git restore --staged <file>..." to unstage)
>> >           modified:   Documentation/cmd-list.perl
>> >
>> >   Changes not staged for commit:
>> >     (use "git add/rm <file>..." to update what will be committed)
>> >     (use "git restore <file>..." to discard changes in working directory)
>> >           deleted:    Documentation/cmd-list.perl
>> >
>>
>> 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.
>
>
> I was actually happy that the out-of-cone paths were restored. I ran that command as an experiment while reading Elijah's doc because I was curious what would happen. The reason I think it should restore out-of-cone paths is so you can do `git restore --staged --source <some commit> && git commit -m "restore to old commit"` without caring about the sparse spec.

I think that could lead to something that would be very dangerous or
highly confusing to other users.  In particular, if they run

  `git restore --staged --source <some commit> -- '*.rs'`

and git changes not only all the Rust files inside the sparsity
specification (i.e. the files they are interested in), but all the
ones outside too, then they'll be rather unhappy.  So, I think if the
paths you specify aren't within the sparse specification, we should
throw an error, much like we already do with `git add` and `git rm`
when in a sparse checkout.  And if you don't care about the sparse
spec despite having set up a sparse checkout, you can always specify
that (with --no-restrict or --scope=all or whatever).

>> 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. Let's say we do a clean checkout of a commit with tree A (i.e. the root tree's hash is A). If we do `git sparse-checkout set non-existent`, the index and the working copy still logically contain state A, right? 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. That's different from how `git restore --staged` without sparse-checkout would have worked (it would not have updated the working copy). So from that perspective, it might make sense to remove the `SKIP_WORKTREE` and add the old file contents back in the working (i.e. from state A in this example), and maybe that's why the commands do that? Actually, `git checkout HEAD^ .` would update both the index and the working copy to match HEAD^, so that shouldn't have to remove the `SKIP_WORKTREE`, maybe?

Yes, you've flagged this correctly as an issue, but I think only
touching files within the sparse specification is much safer and
throwing an error telling the user what flag to add if they specified
a path outside the sparse specification would be better.

Now, if they do provide the override...then your question becomes
valid.  My inclination there is that they provided --staged without
--worktree and provided an override so although they'll get weird
results (much as they also would with git-add or git-rm when they
override) we just follow what they said and only update the index and
leave the file as SKIP_WORKTREE.

> I barely ever use Git, so take all that with a grain of salt.
>
>>
>> Elijah's WIP design doc [1] describes 'restore' as one of:
>>
>> > commands that restore files to the working tree that match sparsity
>> > patterns, and remove unmodified files that don't match those patterns
>
>
> I *think* that only applies to `git restore` without `--staged`.

Yeah, you brought up a really good example here.  I think `restore`
and `checkout -- <paths>`  should probably be better grouped with
add/rm/mv