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

On 12/10, Elijah Newren wrote:

On Sun, Dec 9, 2018 at 12:04 PM Thomas Gummerer [off-list ref] wrote:

quoted

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

quoted

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?

quoted

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.

quoted

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

quoted

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.