[PATCH] rebase -i: restore autostash on abort
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase -i: restore autostash on abort
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase -i: restore autostash on abort
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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase -i: restore autostash on abort
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase -i: restore autostash on abort
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. signature.asc Description: PGP signature
