# [PATCH] rebase -i: restore autostash on abort 7 messages from 2016-06-28 to 2016-06-29. Participants: Patrick Steinhardt, Junio C Hamano, Matthieu Moy, Johannes Schindelin. Thread: https://gitlist.dev/t/42735 ## Patrick Steinhardt, 2016-06-28 17:57 Subject: [PATCH] rebase -i: restore autostash on abort Message-ID: <20160628175704.26095-1-ps@pks.im> URL: https://gitlist.dev/e/20160628175704.26095-1-ps%40pks.im ``` When we abort an interactive rebase we do so by calling `die_abort`, which cleans up after us by removing the rebase state directory. If the user has requested to use the autostash feature, though, the state directory may also contain a reference to the autostash, which will now be deleted. Fix the issue by trying to re-apply the autostash in `die_abort`. This will also handle the case where the autostash does not apply cleanly anymore by recording it in a user-visible stash. Reported-by: Daniel Hahler Signed-off-by: Patrick Steinhardt --- git-rebase--interactive.sh | 1 + t/t3420-rebase-autostash.sh | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 05f22e4..4f499d2 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -212,6 +212,7 @@ exit_with_patch () { } die_abort () { + apply_autostash rm -rf "$state_dir" die "$1" } diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index 944154b..2e1171e 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -192,4 +192,15 @@ test_expect_success 'abort rebase -i with --autostash' ' test_cmp expected file0 ' +test_expect_success 'restore autostash on editor failure' ' + test_when_finished "git reset --hard" && + echo uncommited-content >file0 && + ( + test_set_editor "false" && + test_must_fail git rebase -i --autostash HEAD^ + ) && + echo uncommited-content >expected && + test_cmp expected file0 +' + test_done -- 2.9.0 ``` ## Junio C Hamano, 2016-06-28 19:02 Subject: Re: [PATCH] rebase -i: restore autostash on abort Message-ID: URL: https://gitlist.dev/e/xmqqa8i587cx.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160628175704.26095-1-ps@pks.im> ``` Patrick Steinhardt writes: > When we abort an interactive rebase we do so by calling > `die_abort`, which cleans up after us by removing the rebase > state directory. If the user has requested to use the autostash > feature, though, the state directory may also contain a reference > to the autostash, which will now be deleted. > > Fix the issue by trying to re-apply the autostash in `die_abort`. > This will also handle the case where the autostash does not apply > cleanly anymore by recording it in a user-visible stash. I do not do autostash myself, but it is a good thing to try not to lose information ;-) > +test_expect_success 'restore autostash on editor failure' ' > + test_when_finished "git reset --hard" && > + echo uncommited-content >file0 && > + ( > + test_set_editor "false" && > + test_must_fail git rebase -i --autostash HEAD^ > + ) && > + echo uncommited-content >expected && While making sure this case works is crucial, it is not an interesting failure mode, is it? Can we also have "does not apply cleanly anymore" case, too? > + test_cmp expected file0 > +' > + > test_done ``` ## Matthieu Moy, 2016-06-28 20:42 Subject: Re: [PATCH] rebase -i: restore autostash on abort Message-ID: URL: https://gitlist.dev/e/vpqvb0tqc46.fsf%40anie.imag.fr In-Reply-To: ``` Junio C Hamano writes: > Patrick Steinhardt writes: > >> +test_expect_success 'restore autostash on editor failure' ' >> + test_when_finished "git reset --hard" && >> + echo uncommited-content >file0 && >> + ( >> + test_set_editor "false" && >> + test_must_fail git rebase -i --autostash HEAD^ >> + ) && >> + echo uncommited-content >expected && > > While making sure this case works is crucial, it is not an > interesting failure mode, is it? Can we also have "does not apply > cleanly anymore" case, too? It is "interesting" if you mean "matches real-life use-case", as it corresponds to the case where the user killed the editor (as reported by Daniel Hahler indeed, "Abort with ":cq", which will make Vim exit non-zero"). If you mean "likely to trigger nasty bugs", then indeed testing the case when apply_autostash fails is interesting: for example, calling die_abort when "stash apply" fails is tempting, but would lead to infinite recursion (it doesn't seem to be the case, but a test would be nice). Setting the editor to something that modifies uncommited-content before 'false' should do the trick. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Junio C Hamano, 2016-06-28 21:13 Subject: Re: [PATCH] rebase -i: restore autostash on abort Message-ID: URL: https://gitlist.dev/e/xmqqshvx6mpe.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Matthieu Moy writes: > It is "interesting" if you mean "matches real-life use-case", as it > corresponds to the case where the user killed the editor (as reported by > Daniel Hahler indeed, "Abort with ":cq", which will make Vim exit > non-zero"). Yes. It is an interesting failure mode in that sense. But breakage of such a basic mode is something an end-user is likely to notice immediately, so in that sense, having such a test alone is not all that interesting. > If you mean "likely to trigger nasty bugs", then indeed testing the case > when apply_autostash fails is interesting: for example, calling > die_abort when "stash apply" fails is tempting, but would lead to > infinite recursion (it doesn't seem to be the case, but a test would be > nice). Setting the editor to something that modifies uncommited-content > before 'false' should do the trick. ``` ## Patrick Steinhardt, 2016-06-29 06:21 Subject: [PATCH v2] rebase -i: restore autostash on abort Message-ID: <20160629062127.8946-1-ps@pks.im> URL: https://gitlist.dev/e/20160629062127.8946-1-ps%40pks.im In-Reply-To: <20160628175704.26095-1-ps@pks.im> ``` When we abort an interactive rebase we do so by calling `die_abort`, which cleans up after us by removing the rebase state directory. If the user has requested to use the autostash feature, though, the state directory may also contain a reference to the autostash, which will now be deleted. Fix the issue by trying to re-apply the autostash in `die_abort`. This will also handle the case where the autostash does not apply cleanly anymore by recording it in a user-visible stash. Reported-by: Daniel Hahler Signed-off-by: Patrick Steinhardt --- The original bug-report can be found at [1], the previous version of this patch at [2]. I've included a second test which now also tests how we fare when we actually fail to restore the autostash after the editor has been aborted. Another minor change is fixing of a typo inherited from the previous test "abort rebase -i with --autostash" (commited -> committed). I don't think there's any benefit in fixing the typo in the existing test so I'll just keep it there. [1]: http://article.gmane.org/gmane.comp.version-control.git/297404 [2]: http://article.gmane.org/gmane.comp.version-control.git/298481 git-rebase--interactive.sh | 1 + t/t3420-rebase-autostash.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 05f22e4..4f499d2 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -212,6 +212,7 @@ exit_with_patch () { } die_abort () { + apply_autostash rm -rf "$state_dir" die "$1" } diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index 944154b..532ff5c 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -192,4 +192,35 @@ test_expect_success 'abort rebase -i with --autostash' ' test_cmp expected file0 ' +test_expect_success 'restore autostash on editor failure' ' + test_when_finished "git reset --hard" && + echo uncommitted-content >file0 && + ( + test_set_editor "false" && + test_must_fail git rebase -i --autostash HEAD^ + ) && + echo uncommitted-content >expected && + test_cmp expected file0 +' + +test_expect_success 'autostash is saved on editor failure with conflict' ' + test_when_finished "git reset --hard" && + echo uncommitted-content >file0 && + ( + write_script abort-editor.sh <<-\EOF && + echo conflicting-content >file0 + exit 1 + EOF + test_set_editor "$(pwd)/abort-editor.sh" && + test_must_fail git rebase -i --autostash HEAD^ && + rm -f abort-editor.sh + ) && + echo conflicting-content >expected && + test_cmp expected file0 && + git checkout file0 && + git stash pop && + echo uncommitted-content >expected && + test_cmp expected file0 +' + test_done -- 2.9.0 ``` ## Patrick Steinhardt, 2016-06-29 06:28 Subject: Re: [PATCH] rebase -i: restore autostash on abort Message-ID: <20160629062818.GA416@pks-pc.localdomain> URL: https://gitlist.dev/e/20160629062818.GA416%40pks-pc.localdomain In-Reply-To: ``` On Tue, Jun 28, 2016 at 02:13:49PM -0700, Junio C Hamano wrote: > Matthieu Moy writes: > > > It is "interesting" if you mean "matches real-life use-case", as it > > corresponds to the case where the user killed the editor (as reported by > > Daniel Hahler indeed, "Abort with ":cq", which will make Vim exit > > non-zero"). > > Yes. It is an interesting failure mode in that sense. But breakage > of such a basic mode is something an end-user is likely to notice > immediately, so in that sense, having such a test alone is not all > that interesting. Well, given that the bug has been lingering since autostashing has been implemented it seems users didn't catch the breakage as fast ;) I guess it's just a little-used feature _or_ the breakage is too obscure to regularly happen. At least I have never cause my editor to return an error when editing the ISN-sheet. But still, I agree that a test for conflicting autostashes is beneficial, even though the scenario is even more unlikely (the user's editor has to return an error code as well as that a stashed file needs to be modified while editing the ISN-sheet). I've just sent out a second version of the patch. Thank you both for your input. > > If you mean "likely to trigger nasty bugs", then indeed testing the case > > when apply_autostash fails is interesting: for example, calling > > die_abort when "stash apply" fails is tempting, but would lead to > > infinite recursion (it doesn't seem to be the case, but a test would be > > nice). Setting the editor to something that modifies uncommited-content > > before 'false' should do the trick. ``` ## Johannes Schindelin, 2016-06-29 14:02 Subject: Re: [PATCH v2] rebase -i: restore autostash on abort Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1606291600300.12947%40virtualbox In-Reply-To: <20160629062127.8946-1-ps@pks.im> ``` Hi Patrick, On Wed, 29 Jun 2016, Patrick Steinhardt wrote: > When we abort an interactive rebase we do so by calling > `die_abort`, which cleans up after us by removing the rebase > state directory. If the user has requested to use the autostash > feature, though, the state directory may also contain a reference > to the autostash, which will now be deleted. > > Fix the issue by trying to re-apply the autostash in `die_abort`. > This will also handle the case where the autostash does not apply > cleanly anymore by recording it in a user-visible stash. I like it. Thanks for including the tests, they were very helpful to verify that my rebase--helper work is unaffected: the patch touches only those parts that are not moved into the rebase--helper. Thanks, Johannes ```