{"thread":{"id":"39121","subject":"How do I resolve conflict after popping stash without adding the file to index?","startedAt":"2015-04-20T21:02:54Z","lastAt":"2015-04-22T19:55:13Z","messageCount":14,"participants":["Dmitry Gutov","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"259707","messageId":"5535697E.2030204@yandex.ru","threadId":"39121","inReplyTo":null,"subject":"How do I resolve conflict after popping stash without adding the file to index?","fromName":"Dmitry Gutov","fromEmail":"dgutov@yandex.ru","sentAt":"2015-04-20T21:02:54Z","receivedAt":"2015-04-20T21:02:54Z","isPatch":false,"sender":{"key":"dgutov@yandex.ru","avatar":"https://gravatar.com/avatar/e29ab2de517e0d4042de95eeaddb4f555bcb3b3c1d3c3f50b8169edb266ec43d?d=mp&s=160"},"body":"Hi all,\n\nAfter the user does 'git stash pop', it may result in conflicts.\n\nHowever, in many cases they may not intend to commit the stashed changes \nright away, so staging the applied changes is often not what they intend \nto do. However, the conflict is there until you mark it as resolved.\n\nWhat's the proper thing to do there? 'git add file.ext' followed by 'git \nreset file.ext'? Or simply 'git reset file.ext'?\n\nEither will reset already-staged changes from the said file, which is an \nirreversible operation.\n\nBest regards,\nDmitry.\n"},{"id":"259708","messageId":"xmqqsibujyit.fsf@gitster.dls.corp.google.com","threadId":"39121","inReplyTo":"5535697E.2030204@yandex.ru","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-20T21:11:22Z","receivedAt":"2015-04-20T21:11:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Gutov <dgutov@yandex.ru> writes:\n\n> Either will reset already-staged changes from the said file, which is\n> an irreversible operation.\n\nBut the said file, if it had conflicted, would have had only the\nconflicted higher stage entries in the index, no?  That is, the\nfailed merge wouldn't have touched the index for the path if it\nalready had changes there in the first place.\n\nIf you want to keep them then you do not have to reset, but your\nquestion is about resolving conflict only in the working tree and\nleave the index clean, so I do not think \"git reset -- $path\" would\nnot lose anything \"irreversibly\".\n"},{"id":"259717","messageId":"553583C0.6090404@yandex.ru","threadId":"39121","inReplyTo":"xmqqsibujyit.fsf@gitster.dls.corp.google.com","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Dmitry Gutov","fromEmail":"dgutov@yandex.ru","sentAt":"2015-04-20T22:54:56Z","receivedAt":"2015-04-20T22:54:56Z","isPatch":false,"sender":{"key":"dgutov@yandex.ru","avatar":"https://gravatar.com/avatar/e29ab2de517e0d4042de95eeaddb4f555bcb3b3c1d3c3f50b8169edb266ec43d?d=mp&s=160"},"body":"On 04/21/2015 12:11 AM, Junio C Hamano wrote:\n\n> But the said file, if it had conflicted, would have had only the\n> conflicted higher stage entries in the index, no?  That is, the\n> failed merge wouldn't have touched the index for the path if it\n> already had changes there in the first place.\n\nI'm not really sure what \"higher stage entries\" are, but this scenario \nseems to be a counter-example:\n\ngit init\necho \"aaaaa\" > test\ngit add test\ngit commit -m \"first\"\necho \"aaa\" > test\ngit stash save\necho \"bbbbb\" > test\ngit add test\ngit stash pop\n\nEither that, or 'git stash pop' was a destructive operation, and ate the \nstaged changes.\n\n> If you want to keep them then you do not have to reset, but your\n> question is about resolving conflict only in the working tree and\n> leave the index clean, so I do not think \"git reset -- $path\" would\n> not lose anything \"irreversibly\".\n\nRather, I'd prefer to leave the index as-is, if it makes sense.\n\nBasically, this is about tool automation, see the context here: \nhttp://debbugs.gnu.org/cgi/bugreport.cgi?bug=20292\n"},{"id":"259763","messageId":"20150421212922.GC13230@peff.net","threadId":"39121","inReplyTo":"553583C0.6090404@yandex.ru","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-21T21:29:23Z","receivedAt":"2015-04-21T21:29:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 21, 2015 at 01:54:56AM +0300, Dmitry Gutov wrote:\n\n> I'm not really sure what \"higher stage entries\" are, but this scenario seems\n> to be a counter-example:\n> \n> git init\n> echo \"aaaaa\" > test\n> git add test\n> git commit -m \"first\"\n> echo \"aaa\" > test\n> git stash save\n> echo \"bbbbb\" > test\n> git add test\n> git stash pop\n> \n> Either that, or 'git stash pop' was a destructive operation, and ate the\n> staged changes.\n\nHmm, interestingly, if you do _not_ stage the changes (i.e., drop the\nfinal \"git add\" there), you get:\n\n  $ git stash pop\n  error: Your local changes to the following files would be overwritten by merge:\n\ttest\n  Please, commit your changes or stash them before you can merge.\n  Aborting\n\nwhich makes sense. Writing conflict markers into the file would leave\nyou in a situation where it is hard to recover the \"bbbbb\" content.\n\nBut we seem to skip that safety valve when the content has been staged,\nwhich seems questionable to me (technically we are slightly better off\nthan the protected case because \"bbbbb\" was written to a git blob\nobject, so you can recover it.  But it may be difficult to find the\ncorrect blob in the object database).\n\n-Peff\n"},{"id":"259769","messageId":"5536D099.1000103@yandex.ru","threadId":"39121","inReplyTo":"20150421212922.GC13230@peff.net","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Dmitry Gutov","fromEmail":"dgutov@yandex.ru","sentAt":"2015-04-21T22:35:05Z","receivedAt":"2015-04-21T22:35:05Z","isPatch":false,"sender":{"key":"dgutov@yandex.ru","avatar":"https://gravatar.com/avatar/e29ab2de517e0d4042de95eeaddb4f555bcb3b3c1d3c3f50b8169edb266ec43d?d=mp&s=160"},"body":"On 04/22/2015 12:29 AM, Jeff King wrote:\n\n> Hmm, interestingly, if you do _not_ stage the changes (i.e., drop the\n> final \"git add\" there), you get:\n>\n>    $ git stash pop\n>    error: Your local changes to the following files would be overwritten by merge:\n> \ttest\n>    Please, commit your changes or stash them before you can merge.\n>    Aborting\n>\n> which makes sense. Writing conflict markers into the file would leave\n> you in a situation where it is hard to recover the \"bbbbb\" content.\n\nIndeed.\n\n> But we seem to skip that safety valve when the content has been staged,\n> which seems questionable to me (technically we are slightly better off\n> than the protected case because \"bbbbb\" was written to a git blob\n> object, so you can recover it.  But it may be difficult to find the\n> correct blob in the object database).\n\nAny suggestions how to restore that content in the index \nprogrammatically? If it's non-trivial to do, maybe that is indeed a bug, \nand 'git stash pop' should abort before creating the conflict.\n"},{"id":"259773","messageId":"20150421225228.GB26322@peff.net","threadId":"39121","inReplyTo":"5536D099.1000103@yandex.ru","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-21T22:52:29Z","receivedAt":"2015-04-21T22:52:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 01:35:05AM +0300, Dmitry Gutov wrote:\n\n> >But we seem to skip that safety valve when the content has been staged,\n> >which seems questionable to me (technically we are slightly better off\n> >than the protected case because \"bbbbb\" was written to a git blob\n> >object, so you can recover it.  But it may be difficult to find the\n> >correct blob in the object database).\n> \n> Any suggestions how to restore that content in the index programmatically?\n> If it's non-trivial to do, maybe that is indeed a bug, and 'git stash pop'\n> should abort before creating the conflict.\n\nRight, I am suggesting that latter: that stash should abort if the index\nhas modified entries. The abort for modified working tree files is done\nby git-merge, which can be selective about which entries will be changed\n(since it knows which ones need written).  I haven't thought hard enough\nto say whether it should be doing the same for the index (i.e., whether\nthis is a \"merge\" problem or a \"stash\" problem).\n\n-Peff\n"},{"id":"259805","messageId":"xmqqa8y0f4cv.fsf@gitster.dls.corp.google.com","threadId":"39121","inReplyTo":"20150421225228.GB26322@peff.net","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-22T17:41:04Z","receivedAt":"2015-04-22T17:41:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Right, I am suggesting that latter: that stash should abort if the index\n> has modified entries. The abort for modified working tree files is done\n> by git-merge, which can be selective about which entries will be changed\n> (since it knows which ones need written).  I haven't thought hard enough\n> to say whether it should be doing the same for the index (i.e., whether\n> this is a \"merge\" problem or a \"stash\" problem).\n\nThis is a stash problem.  I've always thought that it insisted on\nhaving a clean index and a clean working tree, but apparently it\ndoesn't, as shown in Dmitry's example sequence.\n\nGenerally speaking, any mergy operation should be done with a clean\nindex (i.e. matches HEAD) so that any difference between the index\nand the HEAD after it stops in the middle is purely the result of\nthat mergy operation, and the commands should stop when the index is\nnot clean as a safety measure (e.g. \"git am -3\", \"git merge\") [*1*].\n\nAn especially tricky command may also insist on a clean working tree\nif it is not easy to guarantee that it will not clobber changes by\nthe user when it stops in the middle (e.g. \"git rebase\").  But this\nis an escape hatch for lazy implementations ;-) It would be ideal if\na command stops without doing anything when the set of paths it\nneeds to touch would overlap the set of paths in which user has\nchanges before the command is run (e.g. \"git merge\" works that way).\n\nI think that \"stash apply\" requires a clean working tree for the\nlatter reason, and does not require a clean index because it just\nforgot that it must do so.\n"},{"id":"259815","messageId":"20150422183540.GC3535@peff.net","threadId":"39121","inReplyTo":"xmqqa8y0f4cv.fsf@gitster.dls.corp.google.com","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T18:35:41Z","receivedAt":"2015-04-22T18:35:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 10:41:04AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Right, I am suggesting that latter: that stash should abort if the index\n> > has modified entries. The abort for modified working tree files is done\n> > by git-merge, which can be selective about which entries will be changed\n> > (since it knows which ones need written).  I haven't thought hard enough\n> > to say whether it should be doing the same for the index (i.e., whether\n> > this is a \"merge\" problem or a \"stash\" problem).\n> \n> This is a stash problem.  I've always thought that it insisted on\n> having a clean index and a clean working tree, but apparently it\n> doesn't, as shown in Dmitry's example sequence.\n\nIt did check the working tree manually, until my e0e2a9c (stash: drop\ndirty worktree check on apply, 2011-04-05). But I don't think we ever\nchecked that the index was clean with respect to HEAD.\n\n> Generally speaking, any mergy operation should be done with a clean\n> index (i.e. matches HEAD) so that any difference between the index\n> and the HEAD after it stops in the middle is purely the result of\n> that mergy operation, and the commands should stop when the index is\n> not clean as a safety measure (e.g. \"git am -3\", \"git merge\") [*1*].\n\nI guess this was the heart of my question. Should a mergy operation\nstart with a clean index (which the caller can enforce), or does it only\nneed the index entries that it is going to touch to be clean (which is\nknown only to the merging code)? The latter is more permissive, as we\nknow that we will not create conflict entries that overwrite what is\nstaged in the index. But I don't think we have good tool support for\noperating on only those entries afterwards (e.g., \"git reset --keep\"\nwould hose your staged changes along with undoing the parts modified by\nthe merge).  So probably asking for a completely clean index is the only\nsane thing.\n\n> An especially tricky command may also insist on a clean working tree\n> if it is not easy to guarantee that it will not clobber changes by\n> the user when it stops in the middle (e.g. \"git rebase\").  But this\n> is an escape hatch for lazy implementations ;-) It would be ideal if\n> a command stops without doing anything when the set of paths it\n> needs to touch would overlap the set of paths in which user has\n> changes before the command is run (e.g. \"git merge\" works that way).\n\nRight, and I think we do that appropriately in stash since e0e2a9c.\n\n> I think that \"stash apply\" requires a clean working tree for the\n> latter reason, and does not require a clean index because it just\n> forgot that it must do so.\n\nIronically, the message before e0e2a9c actually recommends staging\nchanges before applying the stash, which would lead to this exact\nsituation! So I think the most trivial patch is:\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex d4cf818..f1865c9 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -442,6 +442,7 @@ apply_stash () {\n \tassert_stash_like \"$@\"\n \n \tgit update-index -q --refresh || die \"$(gettext \"unable to refresh index\")\"\n+\tgit diff-index --cached HEAD || die \"dirty index; cannot apply stash\"\n \n \t# current index state\n \tc_tree=$(git write-tree) ||\n\nbut it makes me wonder if somebody would find it annoying that they\ncannot apply a stash into their work-in-progress (i.e., it _might_ cause\nannoyance, but most of the time it will be convenient to do so).\n\nWe also have require_clean_work_tree() in git-sh-setup.sh. We definitely\ndon't want the first half of that, but we do want the diff-index check.\nSo probably we'd want to refactor that into two separate functions, and\nonly call the require_clean_index part.\n\n-Peff\n"},{"id":"259824","messageId":"20150422192948.GA27656@peff.net","threadId":"39121","inReplyTo":"20150422183540.GC3535@peff.net","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T19:29:49Z","receivedAt":"2015-04-22T19:29:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 02:35:40PM -0400, Jeff King wrote:\n\n> Ironically, the message before e0e2a9c actually recommends staging\n> changes before applying the stash, which would lead to this exact\n> situation! So I think the most trivial patch is:\n> \n> diff --git a/git-stash.sh b/git-stash.sh\n> index d4cf818..f1865c9 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -442,6 +442,7 @@ apply_stash () {\n>  \tassert_stash_like \"$@\"\n>  \n>  \tgit update-index -q --refresh || die \"$(gettext \"unable to refresh index\")\"\n> +\tgit diff-index --cached HEAD || die \"dirty index; cannot apply stash\"\n>  \n>  \t# current index state\n>  \tc_tree=$(git write-tree) ||\n> \n> but it makes me wonder if somebody would find it annoying that they\n> cannot apply a stash into their work-in-progress (i.e., it _might_ cause\n> annoyance, but most of the time it will be convenient to do so).\n\nIt does actually fail a test in t3903, but I think that test just\nincidentally had a dirty index, and didn't care about that particular\nfeature.\n\n> We also have require_clean_work_tree() in git-sh-setup.sh. We definitely\n> don't want the first half of that, but we do want the diff-index check.\n> So probably we'd want to refactor that into two separate functions, and\n> only call the require_clean_index part.\n\nThis turned out to be more work than it was worth. Most of the effort\nin that function is about adjusting the messages to handle the cases\nwhen either or both of the working tree and index are dirty. I did pick\nup the useful bits from there, though:\n\n  - use --quiet to suppress output and so that the exit code actually\n    matters\n\n  - use \"--\" to disambiguate the ref\n\n  - I didn't pick up the `rev-parse HEAD` call. I don't think it's\n    necessary (i.e., diff-index should barf for us if it can't read\n    HEAD).\n\nHere are the patches.\n\n  [1/3]: t3903: stop hard-coding commit sha1s\n  [2/3]: t3903: avoid applying onto dirty index\n  [3/3]: stash: require a clean index to apply\n\n-Peff\n"},{"id":"259826","messageId":"20150422193052.GA27945@peff.net","threadId":"39121","inReplyTo":"20150422192948.GA27656@peff.net","subject":"[PATCH 1/3] t3903: stop hard-coding commit sha1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T19:30:52Z","receivedAt":"2015-04-22T19:30:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When testing the diff output of \"git stash list\", we look\nfor the stash's subject of \"WIP on master: $sha1\", even\nthough it's not relevant to the diff output. This makes the\ntest brittle to refactoring, as any changes to earlier tests\nmay impact the commit sha1.\n\nSince we don't care about the commit subject here, we can\nsimply ask stash not to print it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3903-stash.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 1e29962..6da4856 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -695,8 +695,8 @@ test_expect_success 'setup stash with index and worktree changes' '\n '\n \n test_expect_success 'stash list implies --first-parent -m' '\n-\tcat >expect <<-\\EOF &&\n-\tstash@{0}: WIP on master: b27a2bc subdir\n+\tcat >expect <<-EOF &&\n+\tstash@{0}\n \n \tdiff --git a/file b/file\n \tindex 257cc56..d26b33d 100644\n@@ -706,13 +706,13 @@ test_expect_success 'stash list implies --first-parent -m' '\n \t-foo\n \t+working\n \tEOF\n-\tgit stash list -p >actual &&\n+\tgit stash list --format=%gd -p >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'stash list --cc shows combined diff' '\n \tcat >expect <<-\\EOF &&\n-\tstash@{0}: WIP on master: b27a2bc subdir\n+\tstash@{0}\n \n \tdiff --cc file\n \tindex 257cc56,9015a7a..d26b33d\n@@ -723,7 +723,7 @@ test_expect_success 'stash list --cc shows combined diff' '\n \t -index\n \t++working\n \tEOF\n-\tgit stash list -p --cc >actual &&\n+\tgit stash list --format=%gd -p --cc >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.4.0.rc2.498.g02440db\n"},{"id":"259827","messageId":"20150422193058.GB27945@peff.net","threadId":"39121","inReplyTo":"20150422192948.GA27656@peff.net","subject":"[PATCH 2/3] t3903: avoid applying onto dirty index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T19:30:58Z","receivedAt":"2015-04-22T19:30:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of the tests in t3903 wants to make sure that applying a\nstash that touches only \"file\" can still happen even if there\nare working tree changes to \"other-file\". To do so, it adds\n\"other-file\" to the index (since otherwise it is an\nuntracked file, voiding the purpose of the test).\n\nBut as we are about to refactor the dirty-index handling,\nand as this test does not actually care about having a dirty\nindex (only a dirty working tree), let's bump the tracking\nof \"other-file\" into the setup phase, so we can have _just_\na dirty working tree here.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3903-stash.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 6da4856..f179c93 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -10,6 +10,8 @@ test_description='Test git stash'\n test_expect_success 'stash some dirty working directory' '\n \techo 1 > file &&\n \tgit add file &&\n+\techo unrelated >other-file &&\n+\tgit add other-file &&\n \ttest_tick &&\n \tgit commit -m initial &&\n \techo 2 > file &&\n@@ -45,8 +47,6 @@ test_expect_success 'applying bogus stash does nothing' '\n \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 \tgit stash apply &&\n \techo 3 >expect &&\n \ttest_cmp expect file\n-- \n2.4.0.rc2.498.g02440db\n"},{"id":"259828","messageId":"20150422193101.GC27945@peff.net","threadId":"39121","inReplyTo":"20150422192948.GA27656@peff.net","subject":"[PATCH 3/3] stash: require a clean index to apply","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T19:31:02Z","receivedAt":"2015-04-22T19:31:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you have staged contents in your index and run \"stash\napply\", we may hit a conflict and put new entries into the\nindex. Recovering to your original state is difficult at\nthat point, because tools like \"git reset --keep\" will blow\naway anything staged.  We can make this safer by refusing to\napply when there are staged changes.\n\nIt's possible we could provide better tooling here, as \"git\nstash apply\" should be writing only conflicts to the index\n(so we know that any stage-0 entries are potentially\nprecious). But it is the odd duck; most \"mergy\" commands\nwill update the index for cleanly merged entries, and it is\nnot worth updating our tooling to support this use case\nwhich is unlikely to be of interest (besides which, we would\nstill need to block a dirty index for \"stash apply --index\",\nsince that case _would_ be ambiguous).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-stash.sh     | 2 ++\n t/t3903-stash.sh | 7 +++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex d4cf818..cc28368 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -442,6 +442,8 @@ apply_stash () {\n \tassert_stash_like \"$@\"\n \n \tgit update-index -q --refresh || die \"$(gettext \"unable to refresh index\")\"\n+\tgit diff-index --cached --quiet --ignore-submodules HEAD -- ||\n+\t\tdie \"$(gettext \"Cannot apply stash: Your index contains uncommitted changes.\")\"\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 f179c93..0746eee 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -45,6 +45,13 @@ test_expect_success 'applying bogus stash does nothing' '\n \ttest_cmp expect file\n '\n \n+test_expect_success 'apply requires a clean index' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo changed >other-file &&\n+\tgit add other-file &&\n+\ttest_must_fail git stash apply\n+'\n+\n test_expect_success 'apply does not need clean working directory' '\n \techo 4 >other-file &&\n \tgit stash apply &&\n-- \n2.4.0.rc2.498.g02440db\n"},{"id":"259831","messageId":"xmqq8udkdk1a.fsf@gitster.dls.corp.google.com","threadId":"39121","inReplyTo":"20150422183540.GC3535@peff.net","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-22T19:45:21Z","receivedAt":"2015-04-22T19:45:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Ironically, the message before e0e2a9c actually recommends staging\n> changes before applying the stash, which would lead to this exact\n> situation!\n\nThe ancient history is hazy to me, but did we fall back to three-way\nmerge in old days (or did anything to the index for that matter), I\nwonder?  In a world \"git stash apply\" only applied the change to the\nworking tree via \"git apply\", that old recommendation would make\nperfect sense.\n\nBut obviously we do not live in such a world right now.  And because\nwe are doing \"merge-recursive\", we should insist on a clean index;\notherwise there is no way to \"undo\" its effect without losing the\nchanges by the end-user.\n\n> So I think the most trivial patch is:\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index d4cf818..f1865c9 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -442,6 +442,7 @@ apply_stash () {\n>  \tassert_stash_like \"$@\"\n>  \n>  \tgit update-index -q --refresh || die \"$(gettext \"unable to refresh index\")\"\n> +\tgit diff-index --cached HEAD || die \"dirty index; cannot apply stash\"\n\nYes, that makes sense.  The original report from Dmitry was\ntriggering the safety from one line above and \"git stash pop\" doing\nthe right thing by refusing to touch the index with unresolved mergy\noperation before doing anything, and with this additional safety, we\nwould make it even safer from people who do \"git add\" and then \"git\nstash pop\" (which is somewhat strange thing to do, given that\n\"stash\" was designed for \"stash to save away; do other things; come\nback to the original commit state that is 'reset --hard' clean;\nunstash\" sequence in the first place).\n\n>  \t# current index state\n>  \tc_tree=$(git write-tree) ||\n>\n> but it makes me wonder if somebody would find it annoying that they\n> cannot apply a stash into their work-in-progress (i.e., it _might_ cause\n> annoyance, but most of the time it will be convenient to do so).\n\nThey can always do \"git stash show -p stash@{n} | git apply\" if they\nwant to build changes incrementally X-<, but it would be annoying.\n\n> So probably we'd want to refactor that into two separate functions, and\n> only call the require_clean_index part.\n\nHmph, but what would that helper do, other than a single \"diff-index\n--quiet --cached HEAD\" call?\n"},{"id":"259835","messageId":"20150422195513.GC27656@peff.net","threadId":"39121","inReplyTo":"xmqq8udkdk1a.fsf@gitster.dls.corp.google.com","subject":"Re: How do I resolve conflict after popping stash without adding the file to index?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T19:55:13Z","receivedAt":"2015-04-22T19:55:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 12:45:21PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Ironically, the message before e0e2a9c actually recommends staging\n> > changes before applying the stash, which would lead to this exact\n> > situation!\n> \n> The ancient history is hazy to me, but did we fall back to three-way\n> merge in old days (or did anything to the index for that matter), I\n> wonder?  In a world \"git stash apply\" only applied the change to the\n> working tree via \"git apply\", that old recommendation would make\n> perfect sense.\n\nHmm, that advice came in 2a79d2f (Clarify how the user can satisfy\nstash's 'dirty state' check., 2008-09-29), at which point it looks like\nwe were already running merge-recursive. So I think it was simply bad\nadvice. ;)\n\n> But obviously we do not live in such a world right now.  And because\n> we are doing \"merge-recursive\", we should insist on a clean index;\n> otherwise there is no way to \"undo\" its effect without losing the\n> changes by the end-user.\n\nYeah, agreed.\n\n> > but it makes me wonder if somebody would find it annoying that they\n> > cannot apply a stash into their work-in-progress (i.e., it _might_ cause\n> > annoyance, but most of the time it will be convenient to do so).\n> \n> They can always do \"git stash show -p stash@{n} | git apply\" if they\n> want to build changes incrementally X-<, but it would be annoying.\n\nI think the best thing to do is introduce this safety, let it cook for a\nwhile, and see what comes up. Perhaps we could add a \"--force\" or\nsimilar, but I'd rather see if anybody ever actually runs into the\nsituation first.\n\n> > So probably we'd want to refactor that into two separate functions, and\n> > only call the require_clean_index part.\n> \n> Hmph, but what would that helper do, other than a single \"diff-index\n> --quiet --cached HEAD\" call?\n\nI was wanting to keep the error message and the flags we feed to\ndiff-index consistent. But yeah, there is little enough duplicate\nmaterial and enough added boilerplate that I do not think it is worth it\n(see the series I just posted).\n\n-Peff\n"}]}