[PATCH] rebase --abort: do not update branch ref

[PATCH] rebase --abort: do not update branch ref

[Martin von Zweigbergk]

If a non-interactive rebase of a ref fails at commit X and is aborted by
the user, the ref will be updated twice. First to point at X (with the
reflog message "rebase finished: $head_name onto $onto"), and then back
to $orig_head. It should not be updated at all.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
This patch unfortunately duplicates some code from
move_to_original_branch, but since I'm refactoring the rebase code quite
a bit, I don't want to change too much in this patch. I will see what I
can do to reduce the duplication after some of the refactoring.

 git-rebase.sh           |    9 +++++++--
 t/t3407-rebase-abort.sh |   11 +++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index ec08f9c..3d194b1 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -278,12 +278,17 @@ do
 		if test -d "$dotest"
 		then
 			GIT_QUIET=$(cat "$dotest/quiet")
-			move_to_original_branch
 		else
 			dotest="$GIT_DIR"/rebase-apply
 			GIT_QUIET=$(cat "$dotest/quiet")
-			move_to_original_branch
 		fi
+		head_name="$(cat "$dotest"/head-name)" &&
+		case "$head_name" in
+		refs/*)
+			git symbolic-ref HEAD $head_name ||
+			die "Could not move back to $head_name"
+			;;
+		esac
 		git reset --hard $(cat "$dotest/orig-head")
 		rm -r "$dotest"
 		exit
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index fbb3f2e..f3250c3 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -72,6 +72,17 @@ testrebase() {
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
 		test ! -d "$dotest"
 	'
+
+	test_expect_success "rebase$type --abort does not update reflog" '
+		cd "$work_dir" &&
+		# Clean up the state from the previous one
+		git reset --hard pre-rebase &&
+		reflog_entries_before=$(git reflog show to-rebase | wc -l) &&
+		test_must_fail git rebase$type master &&
+		git rebase --abort &&
+		reflog_entries_after=$(git reflog show to-rebase | wc -l) &&
+		test $reflog_entries_before -eq $reflog_entries_after
+	'
 }
 
 testrebase "" .git/rebase-apply
-- 
1.7.3.2.190.gfb4ae

[PATCH] t3407: make reflog check stricter

[Martin von Zweigbergk]

---
This is hopefully an improvement to the test case. Please squash with
previous patch if you agree.

 t/t3407-rebase-abort.sh |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index f3250c3..e573dc8 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -77,11 +77,12 @@ testrebase() {
 		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
-		reflog_entries_before=$(git reflog show to-rebase | wc -l) &&
+		git reflog show to-rebase > reflog_before &&
 		test_must_fail git rebase$type master &&
 		git rebase --abort &&
-		reflog_entries_after=$(git reflog show to-rebase | wc -l) &&
-		test $reflog_entries_before -eq $reflog_entries_after
+		git reflog show to-rebase > reflog_after &&
+		test_cmp reflog_before reflog_after &&
+		rm reflog_before reflog_after
 	'
 }
 
-- 
1.7.3.2.190.gfb4ae

Re: [PATCH] rebase --abort: do not update branch ref

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> diff --git a/git-rebase.sh b/git-rebase.sh
> index ec08f9c..3d194b1 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -278,12 +278,17 @@ do
>  		if test -d "$dotest"
>  		then
>  			GIT_QUIET=$(cat "$dotest/quiet")
> -			move_to_original_branch
>  		else
>  			dotest="$GIT_DIR"/rebase-apply
>  			GIT_QUIET=$(cat "$dotest/quiet")
> -			move_to_original_branch
>  		fi

Micronit.  It appears that GIT_QUIET is set to the same value in either
case, so perhaps you would also want to move it outside of the if block,
i.e.

	test -d "$dotest" || dotest="$GIT_DIR/rebase-apply"
        GIT_QUIET=$(cat "$dotest/quiet)
        ... your rewrite to move_to_original_branch here ...

no?  Staring at it further, I wonder who pays attention to GIT_QUIET in
this codepath that will soon exit, though.

> +		head_name="$(cat "$dotest"/head-name)" &&
> +		case "$head_name" in
> +		refs/*)
> +			git symbolic-ref HEAD $head_name ||
> +			die "Could not move back to $head_name"
> +			;;
> +		esac
>  		git reset --hard $(cat "$dotest/orig-head")
>  		rm -r "$dotest"
>  		exit

Re: [PATCH] rebase --abort: do not update branch ref

[Martin von Zweigbergk]

On Sun, Nov 21, 2010 at 10:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index ec08f9c..3d194b1 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>> @@ -278,12 +278,17 @@ do
>>               if test -d "$dotest"
>>               then
>>                       GIT_QUIET=$(cat "$dotest/quiet")
>> -                     move_to_original_branch
>>               else
>>                       dotest="$GIT_DIR"/rebase-apply
>>                       GIT_QUIET=$(cat "$dotest/quiet")
>> -                     move_to_original_branch
>>               fi
>
> Micronit.  It appears that GIT_QUIET is set to the same value in either
> case, so perhaps you would also want to move it outside of the if block,
> i.e.
>
>        test -d "$dotest" || dotest="$GIT_DIR/rebase-apply"
>        GIT_QUIET=$(cat "$dotest/quiet)
>        ... your rewrite to move_to_original_branch here ...
>
> no?  Staring at it further, I wonder who pays attention to GIT_QUIET in
> this codepath that will soon exit, though.

Yes, I just didn't care to change it in this patch, because I'm changing
this code in the refactor I'm currently working on. OTOH, I don't mind
if you do squash that in either. It might make sense since we don't yet
know if my refactoring series will accepted.

Btw, I'm sure you noticed it, but also I sent a little fix for the test
included in this patch. I have never used the In-Reply-To header, so I'm
not sure I used it right (judging by how the threaded Gmane view
presents it, I did not).

Regards,
Martin