[PATCH 1/2] stash: fix accidental apply of non-existent stashes

[PATCH 1/2] stash: fix accidental apply of non-existent stashes

[Jeff King]

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

diff --git a/git-stash.sh b/git-stash.sh
index a305fb1..a5b1dc3 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -264,7 +264,7 @@ parse_flags_and_rev()
 	b_tree=
 	i_tree=
 
-	REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
+	REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
 
 	FLAGS=
 	for opt
@@ -310,16 +310,6 @@ parse_flags_and_rev()
 	IS_STASH_LIKE=t &&
 	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" &&
 	IS_STASH_REF=t
-
-	if test "${REV}" != "${REV%{*\}}"
-	then
-		# maintainers: it would be better if git rev-parse indicated
-		# this condition with a non-zero status code but as of 1.7.2.1 it
-		# it did not. So, we use non-empty stderr output as a proxy for the
-		# condition of interest.
-		test -z "$(git rev-parse "$REV" 2>&1 >/dev/null)" || die "$REV does not exist in the stash log"
-	fi
-
 }
 
 is_stash_like()
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index f62aaf5..11077f0 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -37,6 +37,12 @@ test_expect_success 'parents of stash' '
 	test_cmp output expect
 '
 
+test_expect_success 'applying bogus stash does nothing' '
+	test_must_fail git stash apply stash@{1} &&
+	echo 1 >expect &&
+	test_cmp expect file
+'
+
 test_expect_success 'apply needs clean working directory' '
 	echo 4 > other-file &&
 	git add other-file &&
-- 
1.7.4.3.13.g0b769.dirty

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

[Jeff King]

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

diff --git a/git-stash.sh b/git-stash.sh
index a5b1dc3..07ac323 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -334,9 +334,7 @@ apply_stash () {
 
 	assert_stash_like "$@"
 
-	git update-index -q --refresh &&
-	git diff-files --quiet --ignore-submodules ||
-		die 'Cannot apply to a dirty working tree, please stage your changes'
+	git update-index -q --refresh || die 'unable to refresh index'
 
 	# current index state
 	c_tree=$(git write-tree) ||
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 11077f0..966bc46 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -43,14 +43,26 @@ test_expect_success 'applying bogus stash does nothing' '
 	test_cmp expect file
 '
 
-test_expect_success 'apply needs clean working directory' '
+test_expect_success 'apply does not need clean working directory' '
 	echo 4 > other-file &&
 	git add other-file &&
 	echo 5 > other-file &&
-	test_must_fail git stash apply
+	git stash apply &&
+	echo 3 >expect &&
+	test_cmp expect file
+'
+
+test_expect_success 'apply does not clobber working directory changes' '
+	git reset --hard &&
+	echo 4 >file &&
+	test_must_fail git stash apply &&
+	echo 4 >expect &&
+	test_cmp expect file
 '
 
 test_expect_success 'apply stashed changes' '
+	git reset --hard &&
+	echo 5 >other-file &&
 	git add other-file &&
 	test_tick &&
 	git commit -m other-file &&
-- 
1.7.4.3.13.g0b769.dirty

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

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

[Jeff King]

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

> Jeff King <peff@peff.net> 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.

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Junio C Hamano]

Jeff King <peff@github.com> writes:

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

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.

Re: [PATCH 1/2] stash: fix accidental apply of non-existent stashes

[Jon Seymour]

On Wed, Apr 6, 2011 at 7:20 AM, Jeff King <peff@peff.net> wrote:
> Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Jon Seymour <jon.seymour@gmail.com>

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

jon.

> ---
>  git-stash.sh     |   12 +-----------
>  t/t3903-stash.sh |    6 ++++++
>  2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index a305fb1..a5b1dc3 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -264,7 +264,7 @@ parse_flags_and_rev()
>        b_tree=
>        i_tree=
>
> -       REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
> +       REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
>
>        FLAGS=
>        for opt
> @@ -310,16 +310,6 @@ parse_flags_and_rev()
>        IS_STASH_LIKE=t &&
>        test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" &&
>        IS_STASH_REF=t
> -
> -       if test "${REV}" != "${REV%{*\}}"
> -       then
> -               # maintainers: it would be better if git rev-parse indicated
> -               # this condition with a non-zero status code but as of 1.7.2.1 it
> -               # it did not. So, we use non-empty stderr output as a proxy for the
> -               # condition of interest.
> -               test -z "$(git rev-parse "$REV" 2>&1 >/dev/null)" || die "$REV does not exist in the stash log"
> -       fi
> -
>  }
>
>  is_stash_like()
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index f62aaf5..11077f0 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -37,6 +37,12 @@ test_expect_success 'parents of stash' '
>        test_cmp output expect
>  '
>
> +test_expect_success 'applying bogus stash does nothing' '
> +       test_must_fail git stash apply stash@{1} &&
> +       echo 1 >expect &&
> +       test_cmp expect file
> +'
> +
>  test_expect_success 'apply needs clean working directory' '
>        echo 4 > other-file &&
>        git add other-file &&
> --
> 1.7.4.3.13.g0b769.dirty
>
>

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

[Junio C Hamano]

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

>> So perhaps there is no broken case at all, and it was just a matter of
>> being overly conservative from the beginning.
>
> 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.

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