{"thread":{"id":"26999","subject":"[PATCH 1/2] stash: fix accidental apply of non-existent stashes","startedAt":"2011-04-05T21:20:25Z","lastAt":"2011-04-06T23:01:31Z","messageCount":9,"participants":["Jeff King","Junio C Hamano","Jon Seymour"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"165214","messageId":"20110405212025.GA3579@sigill.intra.peff.net","threadId":"26999","inReplyTo":null,"subject":"[PATCH 1/2] stash: fix accidental apply of non-existent stashes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-05T21:20:25Z","receivedAt":"2011-04-05T21:20:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, \"git rev-parse ref@{9999999}\" did not\ngenerate an error. Therefore when we got an invalid stash\nreference in \"stash apply\", we could end up not noticing\nuntil quite late.  Commit b0f0ecd (detached-stash: work\naround git rev-parse failure to detect bad log refs,\n2010-08-21) handled this by checking for the \"Log for stash\nhas only %d entries\" warning on stderr when we validated the\nref.\n\nA few days later, e6eedc3 (rev-parse: exit with non-zero\nstatus if ref@{n} is not valid., 2010-08-24) fixed the\noriginal issue. That made the extra stderr test superfluous,\nbut also introduced a new bug. Now the early call to:\n\n  git rev-parse --symbolic \"$@\"\n\nfails, but we don't notice the exit code. Worse, its empty\noutput means we think the user didn't provide us a ref, and\nwe try to apply stash@{0}.\n\nThis patch checks the rev-parse exit code and fails early in\nthe revision parsing process. We can also get rid of the\nstderr test; as a bonus, this means that \"stash apply\" can\nnow run under GIT_TRACE=1 properly.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-stash.sh     |   12 +-----------\n t/t3903-stash.sh |    6 ++++++\n 2 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex a305fb1..a5b1dc3 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -264,7 +264,7 @@ parse_flags_and_rev()\n \tb_tree=\n \ti_tree=\n \n-\tREV=$(git rev-parse --no-flags --symbolic \"$@\" 2>/dev/null)\n+\tREV=$(git rev-parse --no-flags --symbolic \"$@\") || exit 1\n \n \tFLAGS=\n \tfor opt\n@@ -310,16 +310,6 @@ parse_flags_and_rev()\n \tIS_STASH_LIKE=t &&\n \ttest \"$ref_stash\" = \"$(git rev-parse --symbolic-full-name \"${REV%@*}\")\" &&\n \tIS_STASH_REF=t\n-\n-\tif test \"${REV}\" != \"${REV%{*\\}}\"\n-\tthen\n-\t\t# maintainers: it would be better if git rev-parse indicated\n-\t\t# this condition with a non-zero status code but as of 1.7.2.1 it\n-\t\t# it did not. So, we use non-empty stderr output as a proxy for the\n-\t\t# condition of interest.\n-\t\ttest -z \"$(git rev-parse \"$REV\" 2>&1 >/dev/null)\" || die \"$REV does not exist in the stash log\"\n-\tfi\n-\n }\n \n is_stash_like()\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex f62aaf5..11077f0 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -37,6 +37,12 @@ test_expect_success 'parents of stash' '\n \ttest_cmp output expect\n '\n \n+test_expect_success 'applying bogus stash does nothing' '\n+\ttest_must_fail git stash apply stash@{1} &&\n+\techo 1 >expect &&\n+\ttest_cmp expect file\n+'\n+\n test_expect_success 'apply needs clean working directory' '\n \techo 4 > other-file &&\n \tgit add other-file &&\n-- \n1.7.4.3.13.g0b769.dirty\n"},{"id":"165216","messageId":"20110405212314.GA3613@sigill.intra.peff.net","threadId":"26999","inReplyTo":"20110405212025.GA3579@sigill.intra.peff.net","subject":"[RFC/PATCH 2/2] stash: drop dirty worktree check on apply","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-05T21:23:15Z","receivedAt":"2011-04-05T21:23:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Before we apply a stash, we make sure there are no changes\nin the worktree that are not in the index. This check dates\nback to the original git-stash.sh, and is presumably\nintended to prevent changes in the working tree from being\naccidentally lost during the merge.\n\nHowever, this check has two problems:\n\n  1. It is overly restrictive. If my stash changes only file\n     \"foo\", but \"bar\" is dirty in the working tree, it will\n     prevent us from applying the stash.\n\n  2. It is redundant. We don't touch the working tree at all\n     until we actually call merge-recursive. But it has its\n     own (much more accurate) checks to avoid losing working\n     tree data, and will abort the merge with a nicer\n     message telling us which paths were problems.\n\nSo we can simply drop the check entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs with any time we loosen a safety valve, I am worried that I'm missing\na case where it is still doing something useful. Thus the RFC tag on\nthis patch.\n\nI'm not sure if the check was perhaps even required when git-stash was\nwritten, and has simply since become useless as merge-recursive became\nmore careful.\n\n git-stash.sh     |    4 +---\n t/t3903-stash.sh |   16 ++++++++++++++--\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex a5b1dc3..07ac323 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -334,9 +334,7 @@ apply_stash () {\n \n \tassert_stash_like \"$@\"\n \n-\tgit update-index -q --refresh &&\n-\tgit diff-files --quiet --ignore-submodules ||\n-\t\tdie 'Cannot apply to a dirty working tree, please stage your changes'\n+\tgit update-index -q --refresh || die 'unable to refresh index'\n \n \t# current index state\n \tc_tree=$(git write-tree) ||\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 11077f0..966bc46 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -43,14 +43,26 @@ test_expect_success 'applying bogus stash does nothing' '\n \ttest_cmp expect file\n '\n \n-test_expect_success 'apply needs clean working directory' '\n+test_expect_success 'apply does not need clean working directory' '\n \techo 4 > other-file &&\n \tgit add other-file &&\n \techo 5 > other-file &&\n-\ttest_must_fail git stash apply\n+\tgit stash apply &&\n+\techo 3 >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'apply does not clobber working directory changes' '\n+\tgit reset --hard &&\n+\techo 4 >file &&\n+\ttest_must_fail git stash apply &&\n+\techo 4 >expect &&\n+\ttest_cmp expect file\n '\n \n test_expect_success 'apply stashed changes' '\n+\tgit reset --hard &&\n+\techo 5 >other-file &&\n \tgit add other-file &&\n \ttest_tick &&\n \tgit commit -m other-file &&\n-- \n1.7.4.3.13.g0b769.dirty\n"},{"id":"165221","messageId":"7vvcysl5pz.fsf@alter.siamese.dyndns.org","threadId":"26999","inReplyTo":"20110405212314.GA3613@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 2/2] stash: drop dirty worktree check on apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-05T21:59:36Z","receivedAt":"2011-04-05T21:59:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> However, this check has two problems:\n>\n>   1. It is overly restrictive. If my stash changes only file\n>      \"foo\", but \"bar\" is dirty in the working tree, it will\n>      prevent us from applying the stash.\n>\n>   2. It is redundant. We don't touch the working tree at all\n>      until we actually call merge-recursive. But it has its\n>      own (much more accurate) checks to avoid losing working\n>      tree data, and will abort the merge with a nicer\n>      message telling us which paths were problems.\n\nI _think_ the reason we originally insisted on clean working tree was that\nwhile merge-resolve has always had an acurate check, merge-recursive's\ncheck was not very good, especially when renames are involved.  So\nprobably this part of your comment ...\n\n> I'm not sure if the check was perhaps even required when git-stash was\n> written, and has simply since become useless as merge-recursive became\n> more careful.\n\n... may need to be used to rewrite bullet 2. above.\n\nThis is a tangent, but I notice that the additional bolted-on codepath for\nthe --index option has this:\n\n    git diff-tree --binary $s^2^..$s^2 | git apply --cached\n\nIt might want to do -B -M to match what \"git merge-recursive\" does.\n"},{"id":"165225","messageId":"20110405221827.GA4123@sigill.intra.peff.net","threadId":"26999","inReplyTo":"7vvcysl5pz.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 2/2] stash: drop dirty worktree check on apply","fromName":"Jeff King","fromEmail":"peff@github.com","sentAt":"2011-04-05T22:18:28Z","receivedAt":"2011-04-05T22:18:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 05, 2011 at 02:59:36PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > However, this check has two problems:\n> >\n> >   1. It is overly restrictive. If my stash changes only file\n> >      \"foo\", but \"bar\" is dirty in the working tree, it will\n> >      prevent us from applying the stash.\n> >\n> >   2. It is redundant. We don't touch the working tree at all\n> >      until we actually call merge-recursive. But it has its\n> >      own (much more accurate) checks to avoid losing working\n> >      tree data, and will abort the merge with a nicer\n> >      message telling us which paths were problems.\n> \n> I _think_ the reason we originally insisted on clean working tree was that\n> while merge-resolve has always had an acurate check, merge-recursive's\n> check was not very good, especially when renames are involved.  So\n> probably this part of your comment ...\n> \n> > I'm not sure if the check was perhaps even required when git-stash was\n> > written, and has simply since become useless as merge-recursive became\n> > more careful.\n> \n> ... may need to be used to rewrite bullet 2. above.\n\nThat makes sense to me. I'd be a lot more comfortable if I could find\nthe actual place where merge-recursive got more accurate. I'll see if\nit's simple to bisect.\n\n> This is a tangent, but I notice that the additional bolted-on codepath for\n> the --index option has this:\n> \n>     git diff-tree --binary $s^2^..$s^2 | git apply --cached\n> \n> It might want to do -B -M to match what \"git merge-recursive\" does.\n\nYeah, that sounds sensible to me.\n\n-Peff\n"},{"id":"165230","messageId":"20110405225038.GB7358@sigill.intra.peff.net","threadId":"26999","inReplyTo":"20110405221827.GA4123@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 2/2] stash: drop dirty worktree check on apply","fromName":"Jeff King","fromEmail":"peff@github.com","sentAt":"2011-04-05T22:50:38Z","receivedAt":"2011-04-05T22:50:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 05, 2011 at 06:18:28PM -0400, Jeff King wrote:\n\n> > I _think_ the reason we originally insisted on clean working tree was that\n> > while merge-resolve has always had an acurate check, merge-recursive's\n> > check was not very good, especially when renames are involved.  So\n> > probably this part of your comment ...\n> > \n> > > I'm not sure if the check was perhaps even required when git-stash was\n> > > written, and has simply since become useless as merge-recursive became\n> > > more careful.\n> > \n> > ... may need to be used to rewrite bullet 2. above.\n> \n> That makes sense to me. I'd be a lot more comfortable if I could find\n> the actual place where merge-recursive got more accurate. I'll see if\n> it's simple to bisect.\n\nHmm, no such luck. In v1.5.0, before stash even existed, \"git merge\"\nwill properly fail on this case (though the error message isn't as\npretty):\n\n-- >8 --\n#!/bin/sh\n\nrm -rf repo\n\nmkdir repo &&\ncd repo &&\ngit init-db &&\necho base >file &&\ngit add file &&\ngit commit -m base &&\necho master >>file &&\ngit commit -a -m master &&\ngit checkout -b other HEAD^\necho other >>file &&\ngit commit -a -m other &&\necho more >>file &&\ngit merge master\n-- 8< --\n\nwhich leads me to believe there is a more complex case that\nmerge-recursive wasn't handling at the time, and which may or may not be\nhandled better today. What that case would be, I have no clue.\n\n-Peff\n"},{"id":"165232","messageId":"20110405230245.GA7631@sigill.intra.peff.net","threadId":"26999","inReplyTo":"20110405225038.GB7358@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 2/2] stash: drop dirty worktree check on apply","fromName":"Jeff King","fromEmail":"peff@github.com","sentAt":"2011-04-05T23:02:45Z","receivedAt":"2011-04-05T23:02:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 05, 2011 at 06:50:38PM -0400, Jeff King wrote:\n\n> > > I _think_ the reason we originally insisted on clean working tree was that\n> > > while merge-resolve has always had an acurate check, merge-recursive's\n> > > check was not very good, especially when renames are involved.  So\n> > > probably this part of your comment ...\n> [...]\n> > That makes sense to me. I'd be a lot more comfortable if I could find\n> > the actual place where merge-recursive got more accurate. I'll see if\n> > it's simple to bisect.\n> \n> Hmm, no such luck. In v1.5.0, before stash even existed, \"git merge\"\n> will properly fail on this case (though the error message isn't as\n> pretty):\n> [...]\n> which leads me to believe there is a more complex case that\n> merge-recursive wasn't handling at the time, and which may or may not be\n> handled better today. What that case would be, I have no clue.\n\nHmm. I think that code is due to your comment on the original git-stash\n(then \"git-save\") here:\n\n  http://article.gmane.org/gmane.comp.version-control.git/50749\n\n  > +function restore_save () {\n  > +     save=$(git rev-parse --verify --default saved \"$1\")\n  > +     h_tree=$(git rev-parse --verify $save:base)\n  > +     i_tree=$(git rev-parse --verify $save:indx)\n  > +     w_tree=$(git rev-parse --verify $save:work)\n  > +\n  > +     git-merge-recursive $h_tree -- HEAD^{tree} $w_tree\n  > +}\n\n  The same \"robustness\" comments for the save_work function apply\n  here.  You probably do not want to restore on a dirty tree; the\n  intended use case is \"stash away, pull, then restore\", so I\n  think it is Ok to assume that you will only be restoring on a\n  clean state (and it would make the implementation simpler).\n\nSo perhaps there is no broken case at all, and it was just a matter of\nbeing overly conservative from the beginning.\n\n-Peff\n"},{"id":"165233","messageId":"7v7hb8l23q.fsf@alter.siamese.dyndns.org","threadId":"26999","inReplyTo":"20110405230245.GA7631@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 2/2] stash: drop dirty worktree check on apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-05T23:17:45Z","receivedAt":"2011-04-05T23:17:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@github.com> writes:\n\n>   The same \"robustness\" comments for the save_work function apply\n>   here.  You probably do not want to restore on a dirty tree; the\n>   intended use case is \"stash away, pull, then restore\", so I\n>   think it is Ok to assume that you will only be restoring on a\n>   clean state (and it would make the implementation simpler).\n>\n> So perhaps there is no broken case at all, and it was just a matter of\n> being overly conservative from the beginning.\n\nPerhaps.\n\nIf we are going to treat this as another mergy operation, we should at\nleast still make sure that the index is clean (i.e. \"diff --cached\" is\nempty), I think.\n"},{"id":"165236","messageId":"BANLkTik4sJwvBsnS855YEV=1akpGqYaspA@mail.gmail.com","threadId":"26999","inReplyTo":"20110405212025.GA3579@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] stash: fix accidental apply of non-existent stashes","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2011-04-05T23:23:12Z","receivedAt":"2011-04-05T23:23:12Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"On Wed, Apr 6, 2011 at 7:20 AM, Jeff King <peff@peff.net> wrote:\n> Signed-off-by: Jeff King <peff@peff.net>\nAcked-by: Jon Seymour <jon.seymour@gmail.com>\n\nI've also submitted a patch that fixes the test that should have caught this.\n\njon.\n\n> ---\n>  git-stash.sh     |   12 +-----------\n>  t/t3903-stash.sh |    6 ++++++\n>  2 files changed, 7 insertions(+), 11 deletions(-)\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index a305fb1..a5b1dc3 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -264,7 +264,7 @@ parse_flags_and_rev()\n>        b_tree=\n>        i_tree=\n>\n> -       REV=$(git rev-parse --no-flags --symbolic \"$@\" 2>/dev/null)\n> +       REV=$(git rev-parse --no-flags --symbolic \"$@\") || exit 1\n>\n>        FLAGS=\n>        for opt\n> @@ -310,16 +310,6 @@ parse_flags_and_rev()\n>        IS_STASH_LIKE=t &&\n>        test \"$ref_stash\" = \"$(git rev-parse --symbolic-full-name \"${REV%@*}\")\" &&\n>        IS_STASH_REF=t\n> -\n> -       if test \"${REV}\" != \"${REV%{*\\}}\"\n> -       then\n> -               # maintainers: it would be better if git rev-parse indicated\n> -               # this condition with a non-zero status code but as of 1.7.2.1 it\n> -               # it did not. So, we use non-empty stderr output as a proxy for the\n> -               # condition of interest.\n> -               test -z \"$(git rev-parse \"$REV\" 2>&1 >/dev/null)\" || die \"$REV does not exist in the stash log\"\n> -       fi\n> -\n>  }\n>\n>  is_stash_like()\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index f62aaf5..11077f0 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -37,6 +37,12 @@ test_expect_success 'parents of stash' '\n>        test_cmp output expect\n>  '\n>\n> +test_expect_success 'applying bogus stash does nothing' '\n> +       test_must_fail git stash apply stash@{1} &&\n> +       echo 1 >expect &&\n> +       test_cmp expect file\n> +'\n> +\n>  test_expect_success 'apply needs clean working directory' '\n>        echo 4 > other-file &&\n>        git add other-file &&\n> --\n> 1.7.4.3.13.g0b769.dirty\n>\n>\n"},{"id":"165321","messageId":"7vfwpvgf1w.fsf@alter.siamese.dyndns.org","threadId":"26999","inReplyTo":"7v7hb8l23q.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 2/2] stash: drop dirty worktree check on apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-06T23:01:31Z","receivedAt":"2011-04-06T23:01:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> So perhaps there is no broken case at all, and it was just a matter of\n>> being overly conservative from the beginning.\n>\n> Perhaps.\n>\n> If we are going to treat this as another mergy operation, we should at\n> least still make sure that the index is clean (i.e. \"diff --cached\" is\n> empty), I think.\n\nPlease disregard this.  \n\nIf the user is doing \"stash apply\" without --index, it is Ok for the index\nto be different from HEAD.  On the other hand, if the \"stash apply\" is\nused with --index, even if the result conflicted in the working tree and\nthen resolution gets marked with \"git add\" for conflicted paths, it is Ok\nto have a path that is unrelated to the \"stash apply\" already added to the\nindex.\n\nSo I don't think there is no point to insist that the index is clean. I\nwas just confused and was thinking about the \"git merge\" (where we must\nhave a clean index, as we are going to commit the index as the result of\nthe merge with conflict resolution).\n"}]}