{"thread":{"id":"42735","subject":"[PATCH] rebase -i: restore autostash on abort","startedAt":"2016-06-28T17:57:12Z","lastAt":"2016-06-29T14:03:39Z","messageCount":7,"participants":["Patrick Steinhardt","Junio C Hamano","Matthieu Moy","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"290405","messageId":"20160628175704.26095-1-ps@pks.im","threadId":"42735","inReplyTo":null,"subject":"[PATCH] rebase -i: restore autostash on abort","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-06-28T17:57:04Z","receivedAt":"2016-06-28T17:57:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When we abort an interactive rebase we do so by calling\n`die_abort`, which cleans up after us by removing the rebase\nstate directory. If the user has requested to use the autostash\nfeature, though, the state directory may also contain a reference\nto the autostash, which will now be deleted.\n\nFix the issue by trying to re-apply the autostash in `die_abort`.\nThis will also handle the case where the autostash does not apply\ncleanly anymore by recording it in a user-visible stash.\n\nReported-by: Daniel Hahler <git@thequod.de>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n git-rebase--interactive.sh  |  1 +\n t/t3420-rebase-autostash.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 05f22e4..4f499d2 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -212,6 +212,7 @@ exit_with_patch () {\n }\n \n die_abort () {\n+\tapply_autostash\n \trm -rf \"$state_dir\"\n \tdie \"$1\"\n }\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 944154b..2e1171e 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -192,4 +192,15 @@ test_expect_success 'abort rebase -i with --autostash' '\n \ttest_cmp expected file0\n '\n \n+test_expect_success 'restore autostash on editor failure' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo uncommited-content >file0 &&\n+\t(\n+\t\ttest_set_editor \"false\" &&\n+\t\ttest_must_fail git rebase -i --autostash HEAD^\n+\t) &&\n+\techo uncommited-content >expected &&\n+\ttest_cmp expected file0\n+'\n+\n test_done\n-- \n2.9.0\n\n"},{"id":"290425","messageId":"xmqqa8i587cx.fsf@gitster.mtv.corp.google.com","threadId":"42735","inReplyTo":"20160628175704.26095-1-ps@pks.im","subject":"Re: [PATCH] rebase -i: restore autostash on abort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-28T19:02:22Z","receivedAt":"2016-06-28T19:02:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When we abort an interactive rebase we do so by calling\n> `die_abort`, which cleans up after us by removing the rebase\n> state directory. If the user has requested to use the autostash\n> feature, though, the state directory may also contain a reference\n> to the autostash, which will now be deleted.\n>\n> Fix the issue by trying to re-apply the autostash in `die_abort`.\n> This will also handle the case where the autostash does not apply\n> cleanly anymore by recording it in a user-visible stash.\n\nI do not do autostash myself, but it is a good thing to try not to\nlose information ;-)\n\n> +test_expect_success 'restore autostash on editor failure' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\techo uncommited-content >file0 &&\n> +\t(\n> +\t\ttest_set_editor \"false\" &&\n> +\t\ttest_must_fail git rebase -i --autostash HEAD^\n> +\t) &&\n> +\techo uncommited-content >expected &&\n\nWhile making sure this case works is crucial, it is not an\ninteresting failure mode, is it?  Can we also have \"does not apply\ncleanly anymore\" case, too?\n\n\n> +\ttest_cmp expected file0\n> +'\n> +\n>  test_done\n"},{"id":"290431","messageId":"vpqvb0tqc46.fsf@anie.imag.fr","threadId":"42735","inReplyTo":"xmqqa8i587cx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase -i: restore autostash on abort","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-28T20:42:17Z","receivedAt":"2016-06-28T20:42:41Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>> +test_expect_success 'restore autostash on editor failure' '\n>> +\ttest_when_finished \"git reset --hard\" &&\n>> +\techo uncommited-content >file0 &&\n>> +\t(\n>> +\t\ttest_set_editor \"false\" &&\n>> +\t\ttest_must_fail git rebase -i --autostash HEAD^\n>> +\t) &&\n>> +\techo uncommited-content >expected &&\n>\n> While making sure this case works is crucial, it is not an\n> interesting failure mode, is it? Can we also have \"does not apply\n> cleanly anymore\" case, too?\n\nIt is \"interesting\" if you mean \"matches real-life use-case\", as it\ncorresponds to the case where the user killed the editor (as reported by\nDaniel Hahler indeed, \"Abort with \":cq\", which will make Vim exit\nnon-zero\").\n\nIf you mean \"likely to trigger nasty bugs\", then indeed testing the case\nwhen apply_autostash fails is interesting: for example, calling\ndie_abort when \"stash apply\" fails is tempting, but would lead to\ninfinite recursion (it doesn't seem to be the case, but a test would be\nnice). Setting the editor to something that modifies uncommited-content\nbefore 'false' should do the trick.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"290437","messageId":"xmqqshvx6mpe.fsf@gitster.mtv.corp.google.com","threadId":"42735","inReplyTo":"vpqvb0tqc46.fsf@anie.imag.fr","subject":"Re: [PATCH] rebase -i: restore autostash on abort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-28T21:13:49Z","receivedAt":"2016-06-28T21:13:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> It is \"interesting\" if you mean \"matches real-life use-case\", as it\n> corresponds to the case where the user killed the editor (as reported by\n> Daniel Hahler indeed, \"Abort with \":cq\", which will make Vim exit\n> non-zero\").\n\nYes.  It is an interesting failure mode in that sense.  But breakage\nof such a basic mode is something an end-user is likely to notice\nimmediately, so in that sense, having such a test alone is not all\nthat interesting.\n\n> If you mean \"likely to trigger nasty bugs\", then indeed testing the case\n> when apply_autostash fails is interesting: for example, calling\n> die_abort when \"stash apply\" fails is tempting, but would lead to\n> infinite recursion (it doesn't seem to be the case, but a test would be\n> nice). Setting the editor to something that modifies uncommited-content\n> before 'false' should do the trick.\n"},{"id":"290457","messageId":"20160629062127.8946-1-ps@pks.im","threadId":"42735","inReplyTo":"20160628175704.26095-1-ps@pks.im","subject":"[PATCH v2] rebase -i: restore autostash on abort","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-06-29T06:21:27Z","receivedAt":"2016-06-29T06:21:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When we abort an interactive rebase we do so by calling\n`die_abort`, which cleans up after us by removing the rebase\nstate directory. If the user has requested to use the autostash\nfeature, though, the state directory may also contain a reference\nto the autostash, which will now be deleted.\n\nFix the issue by trying to re-apply the autostash in `die_abort`.\nThis will also handle the case where the autostash does not apply\ncleanly anymore by recording it in a user-visible stash.\n\nReported-by: Daniel Hahler <git@thequod.de>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nThe original bug-report can be found at [1], the previous version\nof this patch at [2].\n\nI've included a second test which now also tests how we fare when\nwe actually fail to restore the autostash after the editor has\nbeen aborted.\n\nAnother minor change is fixing of a typo inherited from the\nprevious test \"abort rebase -i with --autostash\" (commited ->\ncommitted). I don't think there's any benefit in fixing the typo\nin the existing test so I'll just keep it there.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/297404\n[2]: http://article.gmane.org/gmane.comp.version-control.git/298481\n\n git-rebase--interactive.sh  |  1 +\n t/t3420-rebase-autostash.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 05f22e4..4f499d2 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -212,6 +212,7 @@ exit_with_patch () {\n }\n \n die_abort () {\n+\tapply_autostash\n \trm -rf \"$state_dir\"\n \tdie \"$1\"\n }\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 944154b..532ff5c 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -192,4 +192,35 @@ test_expect_success 'abort rebase -i with --autostash' '\n \ttest_cmp expected file0\n '\n \n+test_expect_success 'restore autostash on editor failure' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo uncommitted-content >file0 &&\n+\t(\n+\t\ttest_set_editor \"false\" &&\n+\t\ttest_must_fail git rebase -i --autostash HEAD^\n+\t) &&\n+\techo uncommitted-content >expected &&\n+\ttest_cmp expected file0\n+'\n+\n+test_expect_success 'autostash is saved on editor failure with conflict' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo uncommitted-content >file0 &&\n+\t(\n+\t\twrite_script abort-editor.sh <<-\\EOF &&\n+\t\t\techo conflicting-content >file0\n+\t\t\texit 1\n+\t\tEOF\n+\t\ttest_set_editor \"$(pwd)/abort-editor.sh\" &&\n+\t\ttest_must_fail git rebase -i --autostash HEAD^ &&\n+\t\trm -f abort-editor.sh\n+\t) &&\n+\techo conflicting-content >expected &&\n+\ttest_cmp expected file0 &&\n+\tgit checkout file0 &&\n+\tgit stash pop &&\n+\techo uncommitted-content >expected &&\n+\ttest_cmp expected file0\n+'\n+\n test_done\n-- \n2.9.0\n\n"},{"id":"290458","messageId":"20160629062818.GA416@pks-pc.localdomain","threadId":"42735","inReplyTo":"xmqqshvx6mpe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase -i: restore autostash on abort","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-06-29T06:28:18Z","receivedAt":"2016-06-29T06:27:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 28, 2016 at 02:13:49PM -0700, Junio C Hamano wrote:\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n> \n> > It is \"interesting\" if you mean \"matches real-life use-case\", as it\n> > corresponds to the case where the user killed the editor (as reported by\n> > Daniel Hahler indeed, \"Abort with \":cq\", which will make Vim exit\n> > non-zero\").\n> \n> Yes.  It is an interesting failure mode in that sense.  But breakage\n> of such a basic mode is something an end-user is likely to notice\n> immediately, so in that sense, having such a test alone is not all\n> that interesting.\n\nWell, given that the bug has been lingering since autostashing\nhas been implemented it seems users didn't catch the breakage as\nfast ;) I guess it's just a little-used feature _or_ the breakage\nis too obscure to regularly happen. At least I have never cause\nmy editor to return an error when editing the ISN-sheet.\n\nBut still, I agree that a test for conflicting autostashes is\nbeneficial, even though the scenario is even more unlikely (the\nuser's editor has to return an error code as well as that a\nstashed file needs to be modified while editing the ISN-sheet).\nI've just sent out a second version of the patch.\n\nThank you both for your input.\n\n> > If you mean \"likely to trigger nasty bugs\", then indeed testing the case\n> > when apply_autostash fails is interesting: for example, calling\n> > die_abort when \"stash apply\" fails is tempting, but would lead to\n> > infinite recursion (it doesn't seem to be the case, but a test would be\n> > nice). Setting the editor to something that modifies uncommited-content\n> > before 'false' should do the trick.\n"},{"id":"290481","messageId":"alpine.DEB.2.20.1606291600300.12947@virtualbox","threadId":"42735","inReplyTo":"20160629062127.8946-1-ps@pks.im","subject":"Re: [PATCH v2] rebase -i: restore autostash on abort","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-06-29T14:02:11Z","receivedAt":"2016-06-29T14:03:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Wed, 29 Jun 2016, Patrick Steinhardt wrote:\n\n> When we abort an interactive rebase we do so by calling\n> `die_abort`, which cleans up after us by removing the rebase\n> state directory. If the user has requested to use the autostash\n> feature, though, the state directory may also contain a reference\n> to the autostash, which will now be deleted.\n> \n> Fix the issue by trying to re-apply the autostash in `die_abort`.\n> This will also handle the case where the autostash does not apply\n> cleanly anymore by recording it in a user-visible stash.\n\nI like it. Thanks for including the tests, they were very helpful to\nverify that my rebase--helper work is unaffected: the patch touches only\nthose parts that are not moved into the rebase--helper.\n\nThanks,\nJohannes\n"}]}