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

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

[Glen Choo]

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 did you expect to happen? (Expected behavior)

I expected to see staged changes only, since I restored only paths
outside of my sparse spec (which was t/, plus the implicit root
directory).

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

What's different between what you expected and what actually happened?

git status should not have said that the file was deleted in the
working copy

[System Info]
git version: git version 2.37.3.998.g577e59143f-goog
cpu: x86_64 no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.17.11-1rodete2-amd64 #1 SMP PREEMPT Debian
5.17.11-1rodete2 (2022-06-09) x86_64
compiler info: gnuc: 12.2
libc info: glibc: 2.33
$SHELL (typically, interactive shell): /bin/bash

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

[Victoria Dye]

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.
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).

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

albeit with other (probably related?) bugs. Given that, I think the correct
behavior would be:

1. if '--ignore-skip-worktree-bits' *is not* specified, do not restore any
   files outside of the sparse-checkout patterns.
2. if '--ignore-skip-worktree-bits' *is* specified, remove the
   'SKIP_WORKTREE' bit & check out all entries matching the pathspec to
   disk.

Fixing this should probably wait until the design doc is finalized (I've
meant to follow up on it for a while now, but I should have some time to do
that this week). In the meantime, 'checkout' will at least allow you to (for
better or for worse) avoid the "deleted" status.

Hope that helps!
- Victoria

[1] https://lore.kernel.org/git/pull.1367.v2.git.1664353951797.gitgitgadget@gmail.com/

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

[Victoria Dye]

Hi Martin!

Please make sure you respond in plaintext - it looks like your message didn't
make it to the mailing list. I've reformatted it to render nicely in this 
reply.

Martin von Zweigbergk wrote:
>> On Tue, Oct 4, 2022 at 9:34 AM Victoria Dye <vdye@github.com <mailto: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.

Conversely, that's behavior a user *wouldn't* want if they want to keep
their sparse cone intact (not to mention the performance impact of checking
out the entire worktree). I think it does more harm to those users than it
would benefit the ones that want to checkout out-of-cone files.

The use-case you're describing should be served by the
'--ignore-skip-worktree-bits' option (not the most intuitive name,
unfortunately). Luckily, there's an increasing desire to improve the naming
of sparse-related options, so the UX situation should improve in the future.

> 
>> 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.

Assuming everything was working properly, users could still choose to
restore only to the index (using the '--no-worktree' option).

> 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? 

The index will, but the working tree will be empty because all index entries
not matching 'non-existent' will have SKIP_WORKTREE applied.

> 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:

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

Right now, SKIP_WORKTREE is removed from the matched entries, but suppose
(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.

> 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? 

It's important to avoid restoring a file to the worktree when it has
SKIP_WORKTREE enabled. See af6a51875a (repo_read_index: clear SKIP_WORKTREE
bit from files present in worktree, 2022-01-14) and the corresponding
discussion in [1].

[1] https://lore.kernel.org/all/pull.1114.v2.git.1642175983.gitgitgadget@gmail.com/

> 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?
> 

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

[Martin von Zweigbergk]

On Tue, Oct 4, 2022 at 1:35 PM Victoria Dye <vdye@github.com> wrote:
>
> Hi Martin!
>
> Please make sure you respond in plaintext - it looks like your message didn't
> make it to the mailing list. I've reformatted it to render nicely in this
> reply.

Hi! Sorry about that, and thanks for fixing! I've switched to plain
text now, and I've
manually added newlines to keep lines short (unsure if I was supposed to).

>
> Martin von Zweigbergk wrote:
> >> On Tue, Oct 4, 2022 at 9:34 AM Victoria Dye <vdye@github.com <mailto: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.
>
> Conversely, that's behavior a user *wouldn't* want if they want to keep
> their sparse cone intact (not to mention the performance impact of checking
> out the entire worktree). I think it does more harm to those users than it
> would benefit the ones that want to checkout out-of-cone files.
>
> The use-case you're describing should be served by the
> '--ignore-skip-worktree-bits' option (not the most intuitive name,
> unfortunately). Luckily, there's an increasing desire to improve the naming
> of sparse-related options, so the UX situation should improve in the future.

I realized after sending my previous email that I might have a
different view of what
sparse checkout is about. To me, it seems like it should be just a performance
optimization. That's why I feel like commands should behave the same way with
or without a sparse spec (unless that proposed `--restrict` flag is passed). I
understand if that's just not feasible. Sorry about the noise in that case :)

>
> >
> >> 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.

Ah, `--worktree` is on by default even if I pass `--staged`, I see. Hmm, the
help text actually says "Specifying --staged will only restore the index."...

>
> Assuming everything was working properly, users could still choose to
> restore only to the index (using the '--no-worktree' option).
>
> > 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?
>
> The index will, but the working tree will be empty because all index entries
> not matching 'non-existent' will have SKIP_WORKTREE applied.
>
> > 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:
>
> - you are restoring to the index
> - you are *not* restoring to the worktree
> - you're restoring files with SKIP_WORKTREE applied
>
> Right now, SKIP_WORKTREE is removed from the matched entries, but suppose
> (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.

Yes, not technically changed. I was (and still am) thinking of the working copy
as logically containing all the files even if some of them are not written out
to disk. I understand if that seems like an odd way of thinking about
it. It might
help to think about how it might appear in a virtual file system where clean
files outside the sparse spec are presented from the index. Or it may be better
to not try to think about it in this weird way at all :)

>
> 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?

FWIW, that's the case I meant.

> I'd lean towards "no", but I'm interested to
> hear other contributors' thoughts.
>
> > 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?
>
> It's important to avoid restoring a file to the worktree when it has
> SKIP_WORKTREE enabled. See af6a51875a (repo_read_index: clear SKIP_WORKTREE
> bit from files present in worktree, 2022-01-14) and the corresponding
> discussion in [1].

Makes sense. That's why I said "and add the old file contents back in the
working copy" (if I understood correctly what that discussion was about).

>
> [1] https://lore.kernel.org/all/pull.1114.v2.git.1642175983.gitgitgadget@gmail.com/
>
> > 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?
> >
>
>

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

[Elijah Newren]

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

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

[Elijah Newren]

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.)

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

[Elijah Newren]

On Tue, Oct 4, 2022 at 9:53 PM Martin von Zweigbergk
<martinvonz@google.com> wrote:
>
> On Tue, Oct 4, 2022 at 1:35 PM Victoria Dye <vdye@github.com> wrote:
> >
> > Martin von Zweigbergk wrote:
> > >> On Tue, Oct 4, 2022 at 9:34 AM Victoria Dye <vdye@github.com <mailto: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.
> > >>
> > >
> > > 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.
> >
> > Conversely, that's behavior a user *wouldn't* want if they want to keep
> > their sparse cone intact (not to mention the performance impact of checking
> > out the entire worktree). I think it does more harm to those users than it
> > would benefit the ones that want to checkout out-of-cone files.
> >
> > The use-case you're describing should be served by the
> > '--ignore-skip-worktree-bits' option (not the most intuitive name,
> > unfortunately). Luckily, there's an increasing desire to improve the naming
> > of sparse-related options, so the UX situation should improve in the future.
>
> I realized after sending my previous email that I might have a
> different view of what
> sparse checkout is about. To me, it seems like it should be just a performance
> optimization. That's why I feel like commands should behave the same way with
> or without a sparse spec (unless that proposed `--restrict` flag is passed). I
> understand if that's just not feasible. Sorry about the noise in that case :)

The problem I see with that definition is that I'm not even sure what
that means.  Behaving the same way with or without a sparse
specification at the extreme means that switching branches should
populate all the files in that branch...meaning a dense checkout.
That kind of defeats the point.  So, I'm sure you don't mean that they
behave the same in all cases...but where do you draw the line?

Is it just switch/checkout & `reset --hard` that avoid reading and
writing outside the sparse specification?

Should diff & status ignore files outside the sparse specification
even if users wrote to such files?  A "performance optimization" might
suggest we should, but would users get confused?

What about merge/rebase/cherry-pick/revert?  Should those write
additional files to the working tree or avoid it?  What about if there
are conflicts outside the sparse specification?

And if extra files get written outside the sparse specification, are
there ways for this to get "cleaned up" where after resolving
conflicts or changes we can again remove the file from the working
tree?

What about `git grep PATTERN`?  That's documented to search the
tracked files in the working tree.  But should that include the files
that would have been there were it not for the "performance
optimization" of not checking them out?  (Similarly, what about `git
grep --cached PATTERN` or `git grep PATTERN REVISION`?)  I mean, if
these commands should behave the same regardless of sparse
specification, then you should search those other files, right?  But
isn't that a nasty performance penalty if the user has a
sparse/partial clone since Git will have to do many network operations
to get additional blobs in order to search them?  Is that really
wanted?

What about `git rm '*.png'` to remove all the tracked png files from
my working tree.  Should that also remove all the files that would
have been there were it not for the "performance optimization"?  Will
that result in very negative surprises for those with a "I want to
concentrate on just this subset of files" mental model?

What about `git worktree add`?  Should the sparse specification be the
same for all worktrees?  Be per-worktree?  Should it default to dense?
 To just top-level sparse?  To the same sparsity as the worktree you
were in when you created a new one?

There are more questions along these lines, but perhaps this gives a
flavor of the kinds of questions that can come up which need
underlying user mental models and usecases to help us answer how these
commands should behave.

> > But suppose you're doing something like 'git restore --staged --no-worktree
> > --ignore-skip-worktree-bits --source HEAD^'. In that case:
> >
> > - you are restoring to the index
> > - you are *not* restoring to the worktree
> > - you're restoring files with SKIP_WORKTREE applied
> >
> > Right now, SKIP_WORKTREE is removed from the matched entries, but suppose
> > (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.
>
> Yes, not technically changed. I was (and still am) thinking of the working copy
> as logically containing all the files even if some of them are not written out
> to disk.

Git started with a nearly identical definition and still sadly
includes it (though buried deep in a plumbing manual that thankfully
virtually no one will ever read):

       Skip-worktree bit can be defined in one (long) sentence: When reading
       an entry, if it is marked as skip-worktree, then Git pretends its
       working directory version is up to date and read the index version
       instead.

Such a mental model is useless to me in answering how commands should
behave in any interesting cases, and actually led to several
inconsistencies and bugs[1].  However, you didn't leave it there; you
took it a step further...

[1] https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/

> I understand if that seems like an odd way of thinking about
> it. It might
> help to think about how it might appear in a virtual file system where clean
> files outside the sparse spec are presented from the index. Or it may be better
> to not try to think about it in this weird way at all :)

Now you've provided a real usecase (virtual file system really
pretending that all files are present), which gives us some context
with which we can try to answer those questions I asked above.  I
think some of the answers might be different for this usecase than the
"behavior A" and "behavior B" in my current sparse-checkout.txt
document, so maybe we want to add this usecase to the mix.

Anyway, thanks for the interesting bug report, and the context of
another usecase to consider.

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

[Victoria Dye]

Martin von Zweigbergk wrote:>>>> 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.
> 
> Ah, `--worktree` is on by default even if I pass `--staged`, I see. Hmm, the
> help text actually says "Specifying --staged will only restore the index."...

You (and Elijah [1]) are correct. '--staged' overrides the "checkout to
worktree" default behavior of 'git restore' to only restore to the index. If
you want to checkout to the worktree _and_ the index, 'git restore --staged
--worktree' is what you'd use.

Sorry for the incorrect information! 

[1] https://lore.kernel.org/git/CABPp-BGeC3hXw-v3voniY5ZU2f6W8NXfXVvq0C03eGGhvSefgg@mail.gmail.com/

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

[Martin von Zweigbergk]

On Wed, Oct 5, 2022 at 12:51 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Tue, Oct 4, 2022 at 9:53 PM Martin von Zweigbergk
> <martinvonz@google.com> wrote:
> >
> > On Tue, Oct 4, 2022 at 1:35 PM Victoria Dye <vdye@github.com> wrote:
> > >
> > > Martin von Zweigbergk wrote:
> > > >> On Tue, Oct 4, 2022 at 9:34 AM Victoria Dye <vdye@github.com <mailto: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.
> > > >>
> > > >
> > > > 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.
> > >
> > > Conversely, that's behavior a user *wouldn't* want if they want to keep
> > > their sparse cone intact (not to mention the performance impact of checking
> > > out the entire worktree). I think it does more harm to those users than it
> > > would benefit the ones that want to checkout out-of-cone files.
> > >
> > > The use-case you're describing should be served by the
> > > '--ignore-skip-worktree-bits' option (not the most intuitive name,
> > > unfortunately). Luckily, there's an increasing desire to improve the naming
> > > of sparse-related options, so the UX situation should improve in the future.
> >
> > I realized after sending my previous email that I might have a
> > different view of what
> > sparse checkout is about. To me, it seems like it should be just a performance
> > optimization. That's why I feel like commands should behave the same way with
> > or without a sparse spec (unless that proposed `--restrict` flag is passed). I
> > understand if that's just not feasible. Sorry about the noise in that case :)
>
> The problem I see with that definition is that I'm not even sure what
> that means.  Behaving the same way with or without a sparse
> specification at the extreme means that switching branches should
> populate all the files in that branch...meaning a dense checkout.
> That kind of defeats the point.  So, I'm sure you don't mean that they
> behave the same in all cases...but where do you draw the line?

I agree with you and Stolee that there are two different cases: some
people use sparse checkouts to restrict what they see (behavior A), and
some people use it just as a performance optimization (behavior B). So I
suspect we roughly agree about what should happen if you pass
`--restrict` (or if that becomes the default so you don't actually need to
pass it). My arguments were about the `--no-restrict` case. Sorry, I
should have made that clear.

I also agree that having a way to make commands restrict to certain paths
by default is useful, and I agree that tying that set of paths to the current
worktree's sparse spec makes sense.

I'll answer the questions below for the `--no-restrict` case
(behavior B).

>
> Is it just switch/checkout & `reset --hard` that avoid reading and
> writing outside the sparse specification?
>
> Should diff & status ignore files outside the sparse specification
> even if users wrote to such files?  A "performance optimization" might
> suggest we should, but would users get confused?

I think they should be included (again, in the `--no-restrict` case).

>
> What about merge/rebase/cherry-pick/revert?  Should those write
> additional files to the working tree or avoid it?  What about if there
> are conflicts outside the sparse specification?

I think they should avoid it, but since the user will need to resolve
that conflict anyway, I can see it makes sense to write them to disk
if there are conflicts.

>
> And if extra files get written outside the sparse specification, are
> there ways for this to get "cleaned up" where after resolving
> conflicts or changes we can again remove the file from the working
> tree?

I've never really used `git sparse-checkout` (until I read your doc),
but isn't that what `git sparse-checkout reapply` is for?

>
> What about `git grep PATTERN`?  That's documented to search the
> tracked files in the working tree.  But should that include the files
> that would have been there were it not for the "performance
> optimization" of not checking them out?  (Similarly, what about `git
> grep --cached PATTERN` or `git grep PATTERN REVISION`?)  I mean, if
> these commands should behave the same regardless of sparse
> specification, then you should search those other files, right?  But
> isn't that a nasty performance penalty if the user has a
> sparse/partial clone since Git will have to do many network operations
> to get additional blobs in order to search them?  Is that really
> wanted?

I think it's consistent to search them with `--no-restrict` (but not
with `--restrict`, of course).

>
> What about `git rm '*.png'` to remove all the tracked png files from
> my working tree.  Should that also remove all the files that would
> have been there were it not for the "performance optimization"?  Will
> that result in very negative surprises for those with a "I want to
> concentrate on just this subset of files" mental model?

Same here.

>
> What about `git worktree add`?  Should the sparse specification be the
> same for all worktrees?  Be per-worktree?  Should it default to dense?
>  To just top-level sparse?  To the same sparsity as the worktree you
> were in when you created a new one?

That's an interesting case. If someone does `git worktree add` and expects
all files to be available in the working copy, they might be surprised, yes.
I think that's a much smaller risk than
`git restore --source HEAD^ --staged && git commit -m 'undo changes'` being
partial, however.

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

[Elijah Newren]

On Wed, Oct 5, 2022 at 1:00 PM Martin von Zweigbergk
<martinvonz@google.com> wrote:
>
> On Wed, Oct 5, 2022 at 12:51 AM Elijah Newren <newren@gmail.com> wrote:
> >
[...]
> I agree with you and Stolee that there are two different cases: some
> people use sparse checkouts to restrict what they see (behavior A), and
> some people use it just as a performance optimization (behavior B). So I
> suspect we roughly agree about what should happen if you pass
> `--restrict` (or if that becomes the default so you don't actually need to
> pass it). My arguments were about the `--no-restrict` case. Sorry, I
> should have made that clear.
>
> I also agree that having a way to make commands restrict to certain paths
> by default is useful, and I agree that tying that set of paths to the current
> worktree's sparse spec makes sense.
>
> I'll answer the questions below for the `--no-restrict` case
> (behavior B).

I don't think your usecase matches behavior B.  I think we should
label your VFS usecase as behavior C and define it separately.  More
on that below...

[...]
> > What about merge/rebase/cherry-pick/revert?  Should those write
> > additional files to the working tree or avoid it?  What about if there
> > are conflicts outside the sparse specification?
>
> I think they should avoid it, but since the user will need to resolve
> that conflict anyway, I can see it makes sense to write them to disk
> if there are conflicts.
>
> >
> > And if extra files get written outside the sparse specification, are
> > there ways for this to get "cleaned up" where after resolving
> > conflicts or changes we can again remove the file from the working
> > tree?
>
> I've never really used `git sparse-checkout` (until I read your doc),
> but isn't that what `git sparse-checkout reapply` is for?

While that command is available for users that want to manually clean
things up proactively, my suspicion is that it is used very rarely --
especially now that we have the present-despite-skipped class of
issues fixed.  I suspect nearly all cleaning up is actually done as an
implicit side-effect of calls to unpack_trees(), which would affect
commands such `switch`, the switch-like portion of `checkout`, `reset
--hard`, `merge`, `rebase`, and many others.

All of these commands have two types of implicit clean-up they do as
part of their operation (which could be thought of as a
post-processing step): (1) marking *unmodified* files outside the
sparsity patterns as SKIP_WORKTREE in the index and removing them from
the working tree, and (2) taking files which match the sparsity
patterns which were previously SKIP_WORKTREE and flip them to
!SKIP_WORKTREE and restore them to the working tree.  I've got a few
examples of what this clean up looks like over at:
https://lore.kernel.org/git/CABPp-BHGrxLPu_S3y2zG-U6uo0rM5TYYEREZa2A=e=d9VZb2PA@mail.gmail.com/

I have no idea how this cleanup affects the VFS usecase; it's very
focused towards "sparse checkout means many files should NOT be
present in the working tree" which may be at odds with how the VFS
stuff is intended to behave.  But it's also been part of
sparse-checkout behavior the longest; for well over a decade now.

> > What about `git grep PATTERN`?  That's documented to search the
> > tracked files in the working tree.  But should that include the files
> > that would have been there were it not for the "performance
> > optimization" of not checking them out?  (Similarly, what about `git
> > grep --cached PATTERN` or `git grep PATTERN REVISION`?)  I mean, if
> > these commands should behave the same regardless of sparse
> > specification, then you should search those other files, right?  But
> > isn't that a nasty performance penalty if the user has a
> > sparse/partial clone since Git will have to do many network operations
> > to get additional blobs in order to search them?  Is that really
> > wanted?
>
> I think it's consistent to search them with `--no-restrict` (but not
> with `--restrict`, of course).
>
> > What about `git rm '*.png'` to remove all the tracked png files from
> > my working tree.  Should that also remove all the files that would
> > have been there were it not for the "performance optimization"?  Will
> > that result in very negative surprises for those with a "I want to
> > concentrate on just this subset of files" mental model?
>
> Same here.
>
> >
> > What about `git worktree add`?  Should the sparse specification be the
> > same for all worktrees?  Be per-worktree?  Should it default to dense?
> >  To just top-level sparse?  To the same sparsity as the worktree you
> > were in when you created a new one?
>
> That's an interesting case. If someone does `git worktree add` and expects
> all files to be available in the working copy, they might be surprised, yes.
> I think that's a much smaller risk than
> `git restore --source HEAD^ --staged && git commit -m 'undo changes'` being
> partial, however.

After you described the VFS usecase, I was guessing you'd answer how
you did for most of these commands.  Most of your answers do not match
the answers I'd expect for behavior B, which seems to me to support my
suspicion that you've got a third usecase.

In particular, I think the difference between Behavior B and your
usecase hinges on the expectation for the working tree:
   Behavior B: Files outside the sparse specification are NOT present
in the working tree.
   Behavior C (your usecase): Files outside the sparse specification
ARE "present" in the working tree, but Git doesn't have to put them
there (they'll be lazily put into place by something else, and the VFS
will ensure that users don't ever notice them actually missing, so far
all intents and purposes, the files are present).

In particular, that difference is perhaps most notable with `git grep`
(without --cached or REVISION flags); such a command is supposed to
search the worktree.  For Behavior B, files outside the sparse
specification are NOT present in the working tree, and hence those
files should NOT be searched.  For your usecase, as you highlight
above, you view all files as present in the working tree (even if Git
isn't the thing writing those files to the working tree and even if
they aren't technically present until you query whether they are
there), so all those files SHOULD be searched.

This difference about the "presence" of files has other knock-on
effects too.  Under Behavior B, users get used to working on just a
subset of files.  Thus `git rm '*.jpg'` or `git restore --source HEAD^
-- '*.md'` should NOT overwrite files outside the sparse specification
(but an error should be shown if the pathspec doesn't match anything,
and that error should point out how users can affect other files
outside the sparse specification).  Under your usecase, users are
always working on the full set of files and all of them can be viewed
in their working copy (as enforced by the filesystem intercepting any
attempts to view or edit files and suddenly magically materializing
them when users look) -- so users in your usecase are not expecting to
be working on a subset of files, and thus those commands would operate
tree-wide.

Similarly, under Behavior B, `git add outside/of/cone/path` should
throw an error.  If it doesn't, some future command will silently
remove the file from the working copy, which may confuse the user;
they are getting themselves into an erroneous state.  Users are
pointed to an override flag they can use if they want to accept the
consequences.  Under your usecase, since ALL files are always
"present" (but not materialized until an attempt to access is made),
that same command would be expected to run without an override and
with no error or warning.

Related to the above, under Behavior B, `git status` should probably
report the existence of any untracked files that do not match the
sparsity patterns as an erroneous condition (because why wait until
git-add to throw an error; let the user know early).  Under your
usecase, we wouldn't.


You might think I'm describing Behavior A above, but only because
Behavior A and Behavior B overlap on worktree-related operations.  The
primary difference between Behavior A and Behavior B is with behavior
of history-related operations.  Behavior B says "the working tree is
sparse, but history is dense; if I do a query on older revisions of
history (grep/diff/log/etc.) then give me results across all paths",
whereas Behavior A says "I only care about this subset of files, both
for the working tree and for history.  Unless I override,
grep/diff/etc. on history should restrict all output to files within
the sparse specification.

One potential way to (over)simplify this would be:
    Behavior A: `--scope=sparse` for both worktree and history operations
    Behavior B: `--scope=sparse` for worktree operations,
`--scope=all` for history operations
    Behavior C: `--scope=all` for both worktree and history operations

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

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

>> 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 you misread 183fb44fd2, which says --staged is to restore
the contents in the index and --worktree is to restore the contents
in the working tree, and both of them can be used at the same time
to affect both destinations.  As gitcli(7) says, --staged here is a
synonym to --cached here and should not touch the working tree.

Having needless synonym may be a source of confusion, so we may want
to straighten out the UI a bit around here, but that is a separate
topic.

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think you misread 183fb44fd2, ...

Ah, now I see this was already resolved yesterday while I was
offline.  Sorry for the noise.