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.) > 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 * 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: $ 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.
