Re: [PATCH 0/8] introduce no-overlay and cached mode in git checkout

[Thomas Gummerer <t.gummerer@gmail.com>]

On 12/10, Elijah Newren wrote:
> On Sun, Dec 9, 2018 at 12:04 PM Thomas Gummerer <t.gummerer@gmail.com> 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.