Possible bug in "git rebase" (non-interactive) with regards to post-rewrite

Possible bug in "git rebase" (non-interactive) with regards to post-rewrite

[Mihai Rusu]

Hi

I have found a possible bug in Git. When running "git rebase"
(non-interactively, ie not "git rebase -i") on code that would
conflict on the last commit that is being rebased and if that last
commit is being skipped (git rebase --skip) then after the rebase is
done the "post-rewrite" hook is not called by "git rebase". If I get a
conflict and "git rebase --skip" any other commit or if I use "git
rebase -i" and "git rebase --skip" the last commit when it conflicts
then it calls post-rewrite just fine. Because this hook is normally
called only once, at the end of a non-aborted rebase the fact that
"git rebase" does not call it when the last commit conflicts and is
skipped means the script is not called at all for that rebase
operation thus breaking the code that depends on it.

Please advise, thank you.

-- 
Mihai Rusu

Re: Possible bug in "git rebase" (non-interactive) with regards to post-rewrite

[Junio C Hamano]

Mihai Rusu <dizzy@google.com> writes:

> I have found a possible bug in Git. When running "git rebase"
> (non-interactively, ie not "git rebase -i") on code that would
> conflict on the last commit that is being rebased and if that last
> commit is being skipped (git rebase --skip) then after the rebase is
> done the "post-rewrite" hook is not called by "git rebase". If I get a
> conflict and "git rebase --skip" any other commit or if I use "git
> rebase -i" and "git rebase --skip" the last commit when it conflicts
> then it calls post-rewrite just fine. Because this hook is normally
> called only once, at the end of a non-aborted rebase the fact that
> "git rebase" does not call it when the last commit conflicts and is
> skipped means the script is not called at all for that rebase
> operation thus breaking the code that depends on it.
>
> Please advise, thank you.

Perhaps this?  Totally untested...

 git-am.sh |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index cf1f64b..6cdd591 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -577,13 +577,6 @@ then
 	resume=
 fi
 
-if test "$this" -gt "$last"
-then
-	say Nothing to do.
-	rm -fr "$dotest"
-	exit
-fi
-
 while test "$this" -le "$last"
 do
 	msgnum=`printf "%0${prec}d" $this`

Re: Possible bug in "git rebase" (non-interactive) with regards to post-rewrite

[Junio C Hamano]

When "rebase --skip" is used to skip the last patch in the series, the
code to wrap up the rewrite by copying the notes from old to new commits
and also by running the post-rewrite hook was bypassed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-am.sh                    |    7 -------
 t/t5407-post-rewrite-hook.sh |   18 +++++++++++++++++-
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index de116a2..69474e5 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -554,13 +554,6 @@ then
 	resume=
 fi
 
-if test "$this" -gt "$last"
-then
-	say Nothing to do.
-	rm -fr "$dotest"
-	exit
-fi
-
 while test "$this" -le "$last"
 do
 	msgnum=`printf "%0${prec}d" $this`
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 552da65..baa670c 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -10,7 +10,11 @@ test_expect_success 'setup' '
 	test_commit A foo A &&
 	test_commit B foo B &&
 	test_commit C foo C &&
-	test_commit D foo D
+	test_commit D foo D &&
+	git checkout A^0 &&
+	test_commit E bar E &&
+	test_commit F foo F &&
+	git checkout master
 '
 
 mkdir .git/hooks
@@ -79,6 +83,18 @@ EOF
 	verify_hook_input
 '
 
+test_expect_success 'git rebase --skip the last one' '
+	git reset --hard F &&
+	clear_hook_input &&
+	test_must_fail git rebase --onto D A &&
+	git rebase --skip &&
+	echo rebase >expected.args &&
+	cat >expected.data <<EOF &&
+$(git rev-parse E) $(git rev-parse HEAD)
+EOF
+	verify_hook_input
+'
+
 test_expect_success 'git rebase -m' '
 	git reset --hard D &&
 	clear_hook_input &&

Re: Possible bug in "git rebase" (non-interactive) with regards to post-rewrite

[Thomas Rast]

Junio C Hamano wrote:
> When "rebase --skip" is used to skip the last patch in the series, the
> code to wrap up the rewrite by copying the notes from old to new commits
> and also by running the post-rewrite hook was bypassed.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
[...]
> -if test "$this" -gt "$last"
> -then
> -	say Nothing to do.
> -	rm -fr "$dotest"
> -	exit
> -fi
> -
>  while test "$this" -le "$last"
>  do
>  	msgnum=`printf "%0${prec}d" $this`

Ack, thanks for patching this.

(I think it's saner anyway not to say "nothing to do" when the user
would expect "we're all done here"...)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch