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@xxxxxxxxxx <mailto:vdye@xxxxxxxxxx>> 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@xxxxxxxxxx: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@xxxxxxxxx/ > 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? >
