On 12/10, Elijah Newren wrote: > On Sun, Dec 9, 2018 at 12:04 PM Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote: > > > > Here's the series I mentioned a couple of times on the list already, > > introducing a no-overlay mode in 'git checkout'. The inspiration for > > this came from Junios message in [*1*]. > > > > Basically the idea is to also delete files when the match <pathspec> > > in 'git checkout <tree-ish> -- <pathspec>' in the current tree, but > > don't match <pathspec> in <tree-ish>. The rest of the cases are > > already properly taken care of by 'git checkout'. > > Yes, but I'd put it a little differently: > > """ > Basically, the idea is when the user run "git checkout --no-overlay > <tree-ish> -- <pathspec>" that the given pathspecs should exactly > match <tree-ish> after the operation completes. This means that we > also want to delete files that match <pathspec> if those paths are not > found in <tree-ish>. > """ > > ...and maybe even toss in some comments about the fact that this is > the way git checkout should have always behaved, it just traditionally > hasn't. (You could also work in comments about how with this new mode > the user can run git diff afterward with the given commit-ish and > pathspecs and get back an empty diff, as expected, which wasn't true > before. But maybe I'm belaboring the point.) > > > > The final step in the series is to actually make use of this in 'git > > stash', which simplifies the code there a bit. I am however happy to > > hold off on this step until the stash-in-C series is merged, so we > > don't delay that further. > > > > In addition to the no-overlay mode, we also add a --cached mode, which > > works only on the index, thus similar to 'git reset <tree-ish> -- <pathspec>'. > > If you're adding a --cached mode to make it only work on the index, > should there be a similar mode to allow it to only work on the working > tree? (I'm not as concerned with that here, but I really think the > new restore-files command by default should only operate on the > working tree, and then have options to affect the index either in > addition or instead of the working tree.) Yeah I think that would be nice to have, though I'm not sure what we would name it in 'git checkout'. Maybe just having it in 'git restore-files' is good enough? > > Actually deprecating 'git reset <tree-ish> -- <pathspec>' should come > > later, probably not before Duy's restore-files command lands, as 'git > > checkout --no-overlay <tree-ish> -- <pathspec>' is a bit cumbersome to > > type compared to 'git reset <tree-ish> -- <pathspec>'. > > Makes sense. > > > My hope is also that the no-overlay mode could become the new default > > in the restore-files command Duy is currently working on. > > Absolutely, yes. I don't want another broken command. :-) > > > > No documentation yet, as I wanted to get this out for review first. > > I'm not familiar with most of the code I touched here, so there may > > well be much better ways to implement some of this, that I wasn't able > > to figure out. I'd be very happy with some feedback around that. > > > > Another thing I'm not sure about is how to deal with conflicts. In > > the cached mode this patch series is not dealing with it at all, as > > 'git checkout -- <pathspec>' when pathspec matches a file with > > conflicts doesn't update the index. For the no-overlay mode, the file > > is removed if the corresponding stage is not found in the index. I'm > > however not sure this is the right thing to do in all cases? > > Here's how I'd go about analyzing that... > > If the user passes a <tree-ish>, then the answer about what to do is > pretty obvious; the <tree-ish> didn't have conflicts, so conflicted > paths in the index that match the pathspec should be overwritten with > whatever version of those paths existed in <tree-ish> (possibly > implying deletion of some paths). > > Also, as you point out, --cached means only modify the index and not > the working tree; so if they specify both --cached and provide no > tree, then they've specified a no-op. > > So it's only interesting when you have conflicts in the index and > specify --no-overlay without a <tree-ish> or --cached. This boils > down to "how do we update the working tree to match the index, when > the index is conflicted?" A couple points to consider: > * This is somewhat of an edge case > * In the normal case --no-overlay is only different from --overlay > behavior for directories; it'd be nice if that extended to all cases I'm not sure I follow what you mean here. How is --no-overlay different from --overlay with respect to directories? It's only different with respect to deletions, no? > * How does this command behave without a <tree-ish> when > --no-overlay is specified and a directory is given for a <pathspec> > and there aren't any conflicts? Are we being consistent with that > behavior? > > > However, I think it turns out that the answer is much simpler than all > that initial analysis or what you say you've implemented. Here's why: > > If <pathspec> is a file which is present in both the working tree and > the index and it has conflicts, then "git checkout -- <pathspec>" will > currently throw an error: I think what was missing from my original description, that actually makes it slightly more interesting from what you describe below is the '--ours' and '--theirs' flags in 'git checkout', with which one can check out a version of the file in the working tree. This is where it gets more interesting. I think I got the right solution for that in patch 5, with deleting the file if it's deleted in "their" version and we pass --theirs to 'git checkout', and analogous for --ours. I was just wondering if there were any further edge cases that I can't think of right no. > $ git checkout -- subdir/counting > error: path 'subdir/counting' is unmerged > > In fact, even if every entry in subdir/ is a path that is in both the > index and the working tree (so that --no-overlay and --overlay ought > to behave the same), if any one of the files in subdir is conflicted, > attempting to checkout the subdir will abort with this same error > message and no paths will be updated at all: > > $ git checkout -- subdir > error: path 'subdir/counting' is unmerged > > as such, the answer with what to do with --no-overlay mode is pretty > clear: if the <pathspec> matches _any_ path that is conflicted, simply > throw an error and abort the operation without making any changes at > all.
