patch, 2 partsstash: fix accidental apply of non-existent stashes

Once upon a time, "git rev-parse ref@{9999999}" did not generate an error. Therefore when we got an invalid stash reference in "stash apply", we could end up not noticing until quite late. Commit b0f0ecd (detached-stash: work around git rev-parse failure to detect bad log refs, 2010-08-21) handled this by checking for the "Log for stash has only %d entries" warning on stderr when we validated the ref.

A few days later, e6eedc3 (rev-parse: exit with non-zero status if ref@{n} is not valid., 2010-08-24) fixed the original issue. That made the extra stderr test superfluous, but also introduced a new bug. Now the early call to:

  git rev-parse --symbolic "$@"

fails, but we don't notice the exit code. Worse, its empty output means we think the user didn't provide us a ref, and we try to apply stash@{0}.

This patch checks the rev-parse exit code and fails early in the revision parsing process. We can also get rid of the stderr test; as a bonus, this means that "stash apply" can now run under GIT_TRACE=1 properly.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-stash.sh     |   12 +-----------
 t/t3903-stash.sh |    6 ++++++
 2 files changed, 7 insertions(+), 11 deletions(-)

Before we apply a stash, we make sure there are no changes in the worktree that are not in the index. This check dates back to the original git-stash.sh, and is presumably intended to prevent changes in the working tree from being accidentally lost during the merge.

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.

So we can simply drop the check entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
As with any time we loosen a safety valve, I am worried that I'm missing
a case where it is still doing something useful. Thus the RFC tag on
this patch.

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.

 git-stash.sh     |    4 +---
 t/t3903-stash.sh |   16 ++++++++++++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

Jeff King <peff@peff.net> writes:

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

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

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.

Yeah, that sounds sensible to me.

-Peff

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

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:

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

Jeff King <peff@github.com> writes:

Perhaps.

If we are going to treat this as another mergy operation, we should at least still make sure that the index is clean (i.e. "diff --cached" is empty), I think.

Junio C Hamano <gitster@pobox.com> writes:

Please disregard this.  

If the user is doing "stash apply" without --index, it is Ok for the index to be different from HEAD. On the other hand, if the "stash apply" is used with --index, even if the result conflicted in the working tree and then resolution gets marked with "git add" for conflicted paths, it is Ok to have a path that is unrelated to the "stash apply" already added to the index.

So I don't think there is no point to insist that the index is clean. I was just confused and was thinking about the "git merge" (where we must have a clean index, as we are going to commit the index as the result of the merge with conflict resolution).

On Wed, Apr 6, 2011 at 7:20 AM, Jeff King <peff@peff.net> wrote:

Acked-by: Jon Seymour <jon.seymour@gmail.com>

I've also submitted a patch that fixes the test that should have caught this.

jon.