# [PATCH] rebase --abort: do not update branch ref 4 messages from 2010-11-21 to 2010-11-22. Participants: Martin von Zweigbergk, Junio C Hamano. Thread: https://gitlist.dev/t/25801 ## Martin von Zweigbergk, 2010-11-21 11:11 Subject: [PATCH] rebase --abort: do not update branch ref Message-ID: <1290337881-26449-1-git-send-email-martin.von.zweigbergk@gmail.com> URL: https://gitlist.dev/e/1290337881-26449-1-git-send-email-martin.von.zweigbergk%40gmail.com ``` 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 --- 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 ``` ## Martin von Zweigbergk, 2010-11-21 19:20 Subject: [PATCH] t3407: make reflog check stricter Message-ID: <1290367234-8206-1-git-send-email-martin.von.zweigbergk@gmail.com> URL: https://gitlist.dev/e/1290367234-8206-1-git-send-email-martin.von.zweigbergk%40gmail.com In-Reply-To: <1290337881-26449-1-git-send-email-martin.von.zweigbergk@gmail.com> ``` --- 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 ``` ## Junio C Hamano, 2010-11-22 03:11 Subject: Re: [PATCH] rebase --abort: do not update branch ref Message-ID: <7v1v6evyq5.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1v6evyq5.fsf%40alter.siamese.dyndns.org In-Reply-To: <1290337881-26449-1-git-send-email-martin.von.zweigbergk@gmail.com> ``` Martin von Zweigbergk 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 ``` ## Martin von Zweigbergk, 2010-11-22 12:23 Subject: Re: [PATCH] rebase --abort: do not update branch ref Message-ID: URL: https://gitlist.dev/e/AANLkTikwZTZhP5biBEsf62g%2BGTkZN7NwX%3DzeF2PKhmwO%40mail.gmail.com In-Reply-To: <7v1v6evyq5.fsf@alter.siamese.dyndns.org> ``` On Sun, Nov 21, 2010 at 10:11 PM, Junio C Hamano wrote: > Martin von Zweigbergk 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 ```