threads / patch / 42735

patchrebase -i: restore autostash on abort

Subject: [PATCH] rebase -i: restore autostash on abort

## tl;dr

7 messages between Jun 28, 2016 and Jun 29, 2016. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Patrick Steinhardt· Jun 28, 2016, 17:57 UTC · lore

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 <git@thequod.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 git-rebase--interactive.sh  |  1 +
 t/t3420-rebase-autostash.sh | 11 +++++++++++
 2 files changed, 12 insertions(+)
Show changes to 2 files +12 −0

git-rebase--interactive.sh, t/t3420-rebase-autostash.sh

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· Jun 28, 2016, 19:02 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] rebase -i: restore autostash on abort

Patrick Steinhardt <ps@pks.im> writes:
Show 9 quoted lines
> 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 ;-)

Show 8 quoted lines
> +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· Jun 28, 2016, 20:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: restore autostash on abort

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Patrick Steinhardt <ps@pks.im> 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· Jun 28, 2016, 21:13 UTC · re: Matthieu Moy · lore

Re: [PATCH] rebase -i: restore autostash on abort

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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.

Show 6 quoted lines
> 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· Jun 29, 2016, 06:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: restore autostash on abort

On Tue, Jun 28, 2016 at 02:13:49PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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.
Show 6 quoted lines
> > 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· Jun 29, 2016, 06:21 UTC · re: Patrick Steinhardt · lore

[PATCH v2] 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 <git@thequod.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

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(+)
Show changes to 2 files +32 −0

git-rebase--interactive.sh, t/t3420-rebase-autostash.sh

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
Johannes Schindelin· Jun 29, 2016, 14:02 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2] rebase -i: restore autostash on abort

Hi Patrick,
On Wed, 29 Jun 2016, Patrick Steinhardt wrote:
Show 9 quoted lines
> 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

← back to recent threads