Re: [RFC/PATCH 2/2] stash: drop dirty worktree check on apply

Jeff King [off-list ref] writes:

However, this check has two problems:

  1. It is overly restrictive. If my stash changes only file
     "foo", but "bar" is dirty in the working tree, it will
     prevent us from applying the stash.

  2. It is redundant. We don't touch the working tree at all
     until we actually call merge-recursive. But it has its
     own (much more accurate) checks to avoid losing working
     tree data, and will abort the merge with a nicer
     message telling us which paths were problems.

I _think_ the reason we originally insisted on clean working tree was that
while merge-resolve has always had an acurate check, merge-recursive's
check was not very good, especially when renames are involved.  So
probably this part of your comment ...

I'm not sure if the check was perhaps even required when git-stash was
written, and has simply since become useless as merge-recursive became
more careful.

... may need to be used to rewrite bullet 2. above.

This is a tangent, but I notice that the additional bolted-on codepath for
the --index option has this:

    git diff-tree --binary $s^2^..$s^2 | git apply --cached

It might want to do -B -M to match what "git merge-recursive" does.

On Tue, Apr 05, 2011 at 02:59:36PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

However, this check has two problems:

  1. It is overly restrictive. If my stash changes only file
     "foo", but "bar" is dirty in the working tree, it will
     prevent us from applying the stash.

  2. It is redundant. We don't touch the working tree at all
     until we actually call merge-recursive. But it has its
     own (much more accurate) checks to avoid losing working
     tree data, and will abort the merge with a nicer
     message telling us which paths were problems.

I _think_ the reason we originally insisted on clean working tree was that
while merge-resolve has always had an acurate check, merge-recursive's
check was not very good, especially when renames are involved.  So
probably this part of your comment ...

quoted

I'm not sure if the check was perhaps even required when git-stash was
written, and has simply since become useless as merge-recursive became
more careful.

... may need to be used to rewrite bullet 2. above.

That makes sense to me. I'd be a lot more comfortable if I could find
the actual place where merge-recursive got more accurate. I'll see if
it's simple to bisect.

This is a tangent, but I notice that the additional bolted-on codepath for
the --index option has this:

    git diff-tree --binary $s^2^..$s^2 | git apply --cached

It might want to do -B -M to match what "git merge-recursive" does.

Yeah, that sounds sensible to me.

-Peff

On Tue, Apr 05, 2011 at 06:18:28PM -0400, Jeff King wrote:

quoted

I _think_ the reason we originally insisted on clean working tree was that
while merge-resolve has always had an acurate check, merge-recursive's
check was not very good, especially when renames are involved.  So
probably this part of your comment ...

quoted

I'm not sure if the check was perhaps even required when git-stash was
written, and has simply since become useless as merge-recursive became
more careful.

... may need to be used to rewrite bullet 2. above.

That makes sense to me. I'd be a lot more comfortable if I could find
the actual place where merge-recursive got more accurate. I'll see if
it's simple to bisect.

Hmm, no such luck. In v1.5.0, before stash even existed, "git merge"
will properly fail on this case (though the error message isn't as
pretty):

-- >8 --
#!/bin/sh

rm -rf repo

mkdir repo &&
cd repo &&
git init-db &&
echo base >file &&
git add file &&
git commit -m base &&
echo master >>file &&
git commit -a -m master &&
git checkout -b other HEAD^
echo other >>file &&
git commit -a -m other &&
echo more >>file &&
git merge master
-- 8< --

which leads me to believe there is a more complex case that
merge-recursive wasn't handling at the time, and which may or may not be
handled better today. What that case would be, I have no clue.

-Peff

On Tue, Apr 05, 2011 at 06:50:38PM -0400, Jeff King wrote:

quoted

quoted

I _think_ the reason we originally insisted on clean working tree was that
while merge-resolve has always had an acurate check, merge-recursive's
check was not very good, especially when renames are involved.  So
probably this part of your comment ...

[...]

quoted

That makes sense to me. I'd be a lot more comfortable if I could find
the actual place where merge-recursive got more accurate. I'll see if
it's simple to bisect.

Hmm, no such luck. In v1.5.0, before stash even existed, "git merge"
will properly fail on this case (though the error message isn't as
pretty):
[...]
which leads me to believe there is a more complex case that
merge-recursive wasn't handling at the time, and which may or may not be
handled better today. What that case would be, I have no clue.

Hmm. I think that code is due to your comment on the original git-stash
(then "git-save") here:

  http://article.gmane.org/gmane.comp.version-control.git/50749

  > +function restore_save () {
  > +     save=$(git rev-parse --verify --default saved "$1")
  > +     h_tree=$(git rev-parse --verify $save:base)
  > +     i_tree=$(git rev-parse --verify $save:indx)
  > +     w_tree=$(git rev-parse --verify $save:work)
  > +
  > +     git-merge-recursive $h_tree -- HEAD^{tree} $w_tree
  > +}

  The same "robustness" comments for the save_work function apply
  here.  You probably do not want to restore on a dirty tree; the
  intended use case is "stash away, pull, then restore", so I
  think it is Ok to assume that you will only be restoring on a
  clean state (and it would make the implementation simpler).

So perhaps there is no broken case at all, and it was just a matter of
being overly conservative from the beginning.

-Peff