threads / patch / 12387

patchAdd test for git rebase --abort

Subject: [PATCH] Add test for git rebase --abort

## tl;dr

12 messages between Feb 29, 2008 and Mar 2, 2008. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Mike Hommey· Feb 29, 2008, 22:08 UTC · lore

We expect git rebase --abort to come back to the original (pre-rebase) head, independently from when it's run during a rebase.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 I wrote this test because I spotted a problem with git rebase --abort, and
 expanded the test to other cases that work, but that it is still better to
 have a test for.
 The failing test is the third. I don't have enough knowledge in git-rebase
 to write an appropriate fix, but the problem seems to be in
 move_to_original_branch, where testing head_name doesn't seem appropriate.
 t/t3407-rebase-abort.sh |   59 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)
 create mode 100755 t/t3407-rebase-abort.sh
Show changes to t/t3407-rebase-abort.sh +59 −0
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
new file mode 100755
index 0000000..abdecc9
--- /dev/null
+++ b/t/t3407-rebase-abort.sh
@@ -0,0 +1,59 @@
+#!/bin/sh
+
+test_description='git rebase --abort tests'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo a > a &&
+	git add a &&
+	git commit -m a &&
+	git branch to-rebase &&
+
+	echo b > a &&
+	git commit -a -m b &&
+	echo c > a &&
+	git commit -a -m c &&
+
+	git checkout to-rebase &&
+	echo d > a &&
+	git commit -a -m "merge should fail on this" &&
+	echo e > a &&
+	git commit -a -m "merge should fail on this, too" &&
+	git branch pre-rebase
+'
+
+test_expect_success 'rebase --abort' '
+	! git rebase master &&
+	git rebase --abort &&
+	test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase)
+'
+
+# In case previous test failed
+git reset --hard pre-rebase >&3 2>&4
+rm -rf .dotest # Should be changed whenever rebase stop using .dotest
+
+test_expect_success 'rebase --abort after --skip' '
+	! git rebase master &&
+	! git rebase --skip &&
+	test $(git rev-parse HEAD) = $(git rev-parse master) &&
+	sh -x ../../git-rebase --abort &&
+	test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase)
+'
+
+# In case previous test failed
+git reset --hard pre-rebase >&3 2>&4
+rm -rf .dotest # Should be changed whenever rebase stop using .dotest
+
+test_expect_success 'rebase --abort after --continue' '
+	! git rebase master &&
+	echo c > a &&
+	echo d >> a &&
+	git add a &&
+	! git rebase --continue &&
+	test $(git rev-parse HEAD) != $(git rev-parse master) &&
+	git rebase --abort &&
+	test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase)
+'
+
+test_done

-- 
1.5.4.3.340.g97b97.dirty
Junio C Hamano· Feb 29, 2008, 23:26 UTC · re: Mike Hommey · lore

Re: [PATCH] Add test for git rebase --abort

Mike Hommey <mh@glandium.org> writes:
>  The failing test is the third. I don't have enough knowledge in git-rebase
>  to write an appropriate fix, but the problem seems to be in
>  move_to_original_branch, where testing head_name doesn't seem appropriate.

Please mark such an "expected to succeed but fails due to suspected bug" with test_expect_failure.

> +test_expect_success 'rebase --abort' '
> +	! git rebase master &&

When making sure "git frotz" refuses gracefully (instead of segfault-and-burn), please say "test_must_fail git frotz".

> +# In case previous test failed
> +git reset --hard pre-rebase >&3 2>&4
> +rm -rf .dotest # Should be changed whenever rebase stop using .dotest

Have this kind of clean-up at the very beginning of the next test. Test writers should not have to learn about file descriptors 3 and 4.

        Side note.  As a test framework extension, we might want
        to add 4th parameter to test_expect_{success,failure}
        that specifies a clean-up to be made regardless of the
        outcome of the test.
Show 7 quoted lines
> +test_expect_success 'rebase --abort after --skip' '
> +	! git rebase master &&
> +	! git rebase --skip &&
> +	test $(git rev-parse HEAD) = $(git rev-parse master) &&
> +	sh -x ../../git-rebase --abort &&
> +	test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase)
> +'

I take that "sh -x ../../" is not for inclusion in the official release.

Johannes Schindelin· Feb 29, 2008, 23:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add test for git rebase --abort

Hi,
On Fri, 29 Feb 2008, Junio C Hamano wrote:
>         Side note.  As a test framework extension, we might want
>         to add 4th parameter to test_expect_{success,failure}
>         that specifies a clean-up to be made regardless of the
>         outcome of the test.

I saw something the other night, where the test _I_ introduced, for git-daemon, failed (with -i -v), and the daemon was _not_ killed as I expected (with trap "<bla>" 0).

Ciao, Dscho

Mike Hommey· Mar 1, 2008, 07:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add test for git rebase --abort

On Fri, Feb 29, 2008 at 03:26:01PM -0800, Junio C Hamano wrote:
Show 8 quoted lines
> Mike Hommey <mh@glandium.org> writes:
> 
> >  The failing test is the third. I don't have enough knowledge in git-rebase
> >  to write an appropriate fix, but the problem seems to be in
> >  move_to_original_branch, where testing head_name doesn't seem appropriate.
> 
> Please mark such an "expected to succeed but fails due to
> suspected bug" with test_expect_failure.

I was kind of expecting the bug would be fixed before the test be included ;)

Show 5 quoted lines
> > +test_expect_success 'rebase --abort' '
> > +	! git rebase master &&
> 
> When making sure "git frotz" refuses gracefully (instead of
> segfault-and-burn), please say "test_must_fail git frotz".
Ooooh, I just saw 74359821.
Show 23 quoted lines
> > +# In case previous test failed
> > +git reset --hard pre-rebase >&3 2>&4
> > +rm -rf .dotest # Should be changed whenever rebase stop using .dotest
> 
> Have this kind of clean-up at the very beginning of the next
> test.  Test writers should not have to learn about file
> descriptors 3 and 4.
> 
>         Side note.  As a test framework extension, we might want
>         to add 4th parameter to test_expect_{success,failure}
>         that specifies a clean-up to be made regardless of the
>         outcome of the test.
> 
> > +test_expect_success 'rebase --abort after --skip' '
> > +	! git rebase master &&
> > +	! git rebase --skip &&
> > +	test $(git rev-parse HEAD) = $(git rev-parse master) &&
> > +	sh -x ../../git-rebase --abort &&
> > +	test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase)
> > +'
> 
> I take that "sh -x ../../" is not for inclusion in the official
> release.
D'oh, I forgot to change that back.
Mike
Junio C Hamano· Mar 1, 2008, 07:41 UTC · re: Mike Hommey · lore

Re: [PATCH] Add test for git rebase --abort

Mike Hommey <mh@glandium.org> writes:
Show 7 quoted lines
> On Fri, Feb 29, 2008 at 03:26:01PM -0800, Junio C Hamano wrote:
> ...
>> Please mark such an "expected to succeed but fails due to
>> suspected bug" with test_expect_failure.
>
> I was kind of expecting the bug would be fixed before the test be
> included ;)

Your patch does not have to go through me to whoever fixes the issue. People can pick it up from the list, apply them to their tree and start working on it. The point is to be gentle to them.

Mike Hommey· Mar 1, 2008, 07:45 UTC · re: Mike Hommey · lore

Re: [PATCH] Add test for git rebase --abort

On Sat, Mar 01, 2008 at 08:36:12AM +0100, Mike Hommey wrote:
Show 12 quoted lines
> On Fri, Feb 29, 2008 at 03:26:01PM -0800, Junio C Hamano wrote:
> > Mike Hommey <mh@glandium.org> writes:
> > 
> > >  The failing test is the third. I don't have enough knowledge in git-rebase
> > >  to write an appropriate fix, but the problem seems to be in
> > >  move_to_original_branch, where testing head_name doesn't seem appropriate.
> > 
> > Please mark such an "expected to succeed but fails due to
> > suspected bug" with test_expect_failure.
> 
> I was kind of expecting the bug would be fixed before the test be
> included ;)
... and the test actually passes with v1.5.0. I'll bisect.
Mike
Mike Hommey· Mar 1, 2008, 08:15 UTC · re: Mike Hommey · lore

Re: [PATCH] Add test for git rebase --abort

On Sat, Mar 01, 2008 at 08:45:11AM +0100, Mike Hommey wrote:
Show 15 quoted lines
> On Sat, Mar 01, 2008 at 08:36:12AM +0100, Mike Hommey wrote:
> > On Fri, Feb 29, 2008 at 03:26:01PM -0800, Junio C Hamano wrote:
> > > Mike Hommey <mh@glandium.org> writes:
> > > 
> > > >  The failing test is the third. I don't have enough knowledge in git-rebase
> > > >  to write an appropriate fix, but the problem seems to be in
> > > >  move_to_original_branch, where testing head_name doesn't seem appropriate.
> > > 
> > > Please mark such an "expected to succeed but fails due to
> > > suspected bug" with test_expect_failure.
> > 
> > I was kind of expecting the bug would be fixed before the test be
> > included ;)
> 
> ... and the test actually passes with v1.5.0. I'll bisect.

... and I'm even the one to blame fb6e4e1f3f048898677f3cf177bfcaf60123bd5c is first bad commit

Mike
Junio C Hamano· Mar 1, 2008, 09:17 UTC · re: Mike Hommey · lore

Re: [PATCH] Add test for git rebase --abort

Mike Hommey <mh@glandium.org> writes:
Show 19 quoted lines
> On Sat, Mar 01, 2008 at 08:45:11AM +0100, Mike Hommey wrote:
>> On Sat, Mar 01, 2008 at 08:36:12AM +0100, Mike Hommey wrote:
>> > On Fri, Feb 29, 2008 at 03:26:01PM -0800, Junio C Hamano wrote:
>> > > Mike Hommey <mh@glandium.org> writes:
>> > > 
>> > > >  The failing test is the third. I don't have enough knowledge in git-rebase
>> > > >  to write an appropriate fix, but the problem seems to be in
>> > > >  move_to_original_branch, where testing head_name doesn't seem appropriate.
>> > > 
>> > > Please mark such an "expected to succeed but fails due to
>> > > suspected bug" with test_expect_failure.
>> > 
>> > I was kind of expecting the bug would be fixed before the test be
>> > included ;)
>> 
>> ... and the test actually passes with v1.5.0. I'll bisect.
>
> ... and I'm even the one to blame
> fb6e4e1f3f048898677f3cf177bfcaf60123bd5c is first bad commit
Heh, didn't you say you don't have enough knowledge in git-rebase? ;-)
I've applied the test with the following fixups.
 t/t3407-rebase-abort.sh |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)
Show changes to t/t3407-rebase-abort.sh +8 −11
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index abdecc9..94bdd72 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -24,33 +24,33 @@ test_expect_success setup '
 '
 
 test_expect_success 'rebase --abort' '
-	! git rebase master &&
+	test_must_fail git rebase master &&
 	git rebase --abort &&
 	test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase)
 '
 
-# In case previous test failed
-git reset --hard pre-rebase >&3 2>&4
-rm -rf .dotest # Should be changed whenever rebase stop using .dotest
+test_expect_failure 'rebase --abort after --skip' '
+	# Clean up the state from the previous one
+	git reset --hard pre-rebase
+	rm -rf .dotest
 
-test_expect_success 'rebase --abort after --skip' '
-	! git rebase master &&
-	! git rebase --skip &&
+	test_must_fail git rebase master &&
+	test_must_fail git rebase --skip &&
 	test $(git rev-parse HEAD) = $(git rev-parse master) &&
-	sh -x ../../git-rebase --abort &&
+	git rebase --abort &&
 	test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase)
 '
 
-# In case previous test failed
-git reset --hard pre-rebase >&3 2>&4
-rm -rf .dotest # Should be changed whenever rebase stop using .dotest
Mike Hommey· Mar 1, 2008, 10:32 UTC · re: Junio C Hamano · lore

[PATCH] Fix git reset --abort not restoring the right commit under some conditions

Previously, --abort would end by git resetting to ORIG_HEAD, but some commands, such as git reset --hard (which happened in git rebase --skip, but could just as well be typed by the user), modify ORIG_HEAD.

Just use the orig-head we store in $dotest instead. ---

 > > ... and I'm even the one to blame
 > > fb6e4e1f3f048898677f3cf177bfcaf60123bd5c is first bad commit
 >
 > Heh, didn't you say you don't have enough knowledge in git-rebase? ;-)
 I'm relieved, I'm not exactly to blame ;) I just exposed the bug that was
 actually already here.
 git-rebase.sh           |    5 ++---
 t/t3407-rebase-abort.sh |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)
Show changes to 2 files +3 −4

git-rebase.sh, t/t3407-rebase-abort.sh

diff --git a/git-rebase.sh b/git-rebase.sh
index bdcea0e..6b9af96 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -208,16 +208,15 @@ do
 		if test -d "$dotest"
 		then
 			move_to_original_branch
-			rm -r "$dotest"
 		elif test -d .dotest
 		then
 			dotest=.dotest
 			move_to_original_branch
-			rm -r .dotest
 		else
 			die "No rebase in progress?"
 		fi
-		git reset --hard ORIG_HEAD
+		git reset --hard $(cat $dotest/orig-head)
+		rm -r "$dotest"
 		exit
 		;;
 	--onto)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 94bdd72..3417138 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -29,7 +29,7 @@ test_expect_success 'rebase --abort' '
 	test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase)
 '
 
-test_expect_failure 'rebase --abort after --skip' '
+test_expect_success 'rebase --abort after --skip' '
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase
 	rm -rf .dotest

-- 
1.5.4.3.343.gb141c.dirty
Mike Hommey· Mar 1, 2008, 11:11 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix git reset --abort not restoring the right commit under some conditions

On Sat, Mar 01, 2008 at 11:32:14AM +0100, Mike Hommey wrote:
Show 5 quoted lines
> Previously, --abort would end by git resetting to ORIG_HEAD, but some
> commands, such as git reset --hard (which happened in git rebase --skip,
> but could just as well be typed by the user), modify ORIG_HEAD.
> 
> Just use the orig-head we store in $dotest instead.
Oops, forgot the -s.
Signed-off-by: Mike Hommey <mh@glandium.org>
Junio C Hamano· Mar 2, 2008, 02:27 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix git reset --abort not restoring the right commit under some conditions

Mike Hommey <mh@glandium.org> writes:
Show 5 quoted lines
> Previously, --abort would end by git resetting to ORIG_HEAD, but some
> commands, such as git reset --hard (which happened in git rebase --skip,
> but could just as well be typed by the user), modify ORIG_HEAD.
>
> Just use the orig-head we store in $dotest instead.
What happens if we are not using $dotest but .dotest?
Junio C Hamano· Mar 2, 2008, 03:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix git reset --abort not restoring the right commit under some conditions

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Mike Hommey <mh@glandium.org> writes:
>
>> Previously, --abort would end by git resetting to ORIG_HEAD, but some
>> commands, such as git reset --hard (which happened in git rebase --skip,
>> but could just as well be typed by the user), modify ORIG_HEAD.
>>
>> Just use the orig-head we store in $dotest instead.
>
> What happens if we are not using $dotest but .dotest?
Ah, there is an assignment to that variable in that case.  Ok.

The script really needs an overhaul someday. It got unreadable over time.

← back to recent threads