threads / patch / 25801

patchrebase --abort: do not update branch ref

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

## tl;dr

4 messages between Nov 21, 2010 and Nov 22, 2010. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Martin von Zweigbergk· Nov 21, 2010, 11:11 UTC · lore

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(-)

Show changes to 2 files +18 −2

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

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

Martin von Zweigbergk· Nov 21, 2010, 19:20 UTC · re: Martin von Zweigbergk · lore

[PATCH] t3407: make reflog check stricter

--- 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(-)

Show changes to t/t3407-rebase-abort.sh +4 −3

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

Junio C Hamano· Nov 22, 2010, 03:11 UTC · re: Martin von Zweigbergk · lore

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

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

Show 14 quoted lines

> 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.

Show 10 quoted lines

> +		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

Martin von Zweigbergk· Nov 22, 2010, 12:23 UTC · re: Junio C Hamano · lore

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

On Sun, Nov 21, 2010 at 10:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

> 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