BUG. git rebase -i successfully continues (and also skips rewording) when pre-commit hook fails (exits with non-zero code)

BUG. git rebase -i successfully continues (and also skips rewording) when pre-commit hook fails (exits with non-zero code)

[Alexey Shumkin]

For a project I have a pre-commit hook that monitors
whether files in a folder (scripts of DB) changed or not
and fails if another special file (DB version) did not changed, too.

So, I did some commits and then I decided to change the order of them.
Of course, I used a lovely "git rebase -i" command. I changed the order
of the commits, then rebasing went ok. But I noticed that my pre-commit
hook output failure message (one of the commits did not meet
above-mentioned condition). It's not too bad but ugly. But when I
decided to correct a message of that specific commit I ran
"git rebase -i" again, marked that commit for rewording, rewording did
not start (because pre-commit hook failed, obviously) and rebasing went
on (commit had an unchanged message) and successfully finished. That is
not what I expected.
I guess if any of hooks fail (which usually fail the commit), rebasing
have to be interrupted (as when there are conflicts)

Here is a sample to reproduce the error
git init .
echo content > file
git add -fv file
git commit -a -m 'first commit'
echo line 2 >> file
git commit -a -m 'secont commit' # note a typo ;)
echo '#!/bin/bash
echo commit failed
exit 1' > .git/hooks/pre-commit
chmod +x .git/hooks/pre-commit
echo fail >> file
git commit -a -m 'failed commit' # to show that pre-commit hook fails
# and outputs "commit failed"
git reset --hard
git rebase -i HEAD^
# mark commit for rewording and exit an editor

note following output after all this:
>commit fail/1)
>Successfully rebased and updated refs/heads/master

Re: BUG. git rebase -i successfully continues (and also skips rewording) when pre-commit hook fails (exits with non-zero code)

[Andrew Wong]

I actually have a patch to fix this sitting in my repo for a while. Thanks for bringing this issue up again.

Andrew Wong (1):
  rebase -i: interrupt rebase when "commit --amend" failed during
    "reword"

 git-rebase--interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

-- 
1.7.8.rc3.32.gb2fac

[PATCH] rebase -i: interrupt rebase when "commit --amend" failed during "reword"

[Andrew Wong]

"commit --amend" could fail in cases like the user empties the commit
message, or pre-commit failed.  When it fails, rebase should be
interrupted, rather than ignoring the error and continue on rebasing.
This gives users a way to gracefully interrupt a "reword" if they
decided they actually want to do an "edit", or even "rebase --abort".

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 git-rebase--interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 804001b..669f378 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -408,7 +408,8 @@ do_next () {
 		mark_action_done
 		pick_one $sha1 ||
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
-		git commit --amend --no-post-rewrite
+		git commit --amend --no-post-rewrite ||
+			die_with_patch $sha1 "Cannot amend commit after successfully picking $sha1... $rest"
 		record_in_rewritten $sha1
 		;;
 	edit|e)
-- 
1.7.8.rc3.32.gb2fac

Re: [PATCH] rebase -i: interrupt rebase when "commit --amend" failed during "reword"

[Junio C Hamano]

Andrew Wong <andrew.kw.w@gmail.com> writes:

> "commit --amend" could fail in cases like the user empties the commit
> message, or pre-commit failed.  When it fails, rebase should be
> interrupted, rather than ignoring the error and continue on rebasing.
> This gives users a way to gracefully interrupt a "reword" if they
> decided they actually want to do an "edit", or even "rebase --abort".
>
> Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>

Makes sense, especially if "commit" itself failed due to some unknown
reason or a refusal from the pre-commit hook. Even though a user could
have been using the "empty the commit log message and the original is
kept" as a trick to recover from a botched rewording attempt and this
change will regress for such use cases, I have a feeling that it does
not matter.

Is there anything we should be saying more than "fatal: Cannot amend" to
help users when this new "die" triggers? What is the recommended recovery
procedure? Run "git commit --amend" after doing whatever is needed to fix
the tree (e.g. if pre-commit refused because of a coding style violation,
it may involve fixing the tree being committed; if it refused because of a
typo in the log message, the tree itself may be OK and nothing needs to be
done) and then "git rebase --continue"?

>  git-rebase--interactive.sh |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 804001b..669f378 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -408,7 +408,8 @@ do_next () {
>  		mark_action_done
>  		pick_one $sha1 ||
>  			die_with_patch $sha1 "Could not apply $sha1... $rest"
> -		git commit --amend --no-post-rewrite
> +		git commit --amend --no-post-rewrite ||
> +			die_with_patch $sha1 "Cannot amend commit after successfully picking $sha1... $rest"
>  		record_in_rewritten $sha1
>  		;;
>  	edit|e)

Re: [PATCH] rebase -i: interrupt rebase when "commit --amend" failed during "reword"

[Andrew Wong]

On 11-11-29 3:08 PM, Junio C Hamano wrote:
> Is there anything we should be saying more than "fatal: Cannot amend" to
> help users when this new "die" triggers? 

Ah, yes, that would be helpful.

The situation is actually very similar to an "edit", where a pick is successful
but requires user intervention. So I'm planning to refactor the behavior and
message from "edit" into a function called "exit_with_patch". Then call the
function from "reword" as well. Though it bothers me a bit that I have to pass
in an exit code as well, since we want the exit status for "reword" to indicate
a failure, but "edit" needs to indicate a success. Is this acceptable? Or
should I just not bother with refactoring?


Andrew Wong (1):
  rebase -i: interrupt rebase when "commit --amend" failed during
    "reword"

 git-rebase--interactive.sh |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

-- 
1.7.8.rc3.32.gb0399.dirty

[PATCH] rebase -i: interrupt rebase when "commit --amend" failed during "reword"

[Andrew Wong]

"commit --amend" could fail in cases like the user empties the commit
message, or pre-commit failed.  When it fails, rebase should be
interrupted and alert the user, rather than ignoring the error and
continue on rebasing.  This also gives users a way to gracefully
interrupt a "reword" if they decided they actually want to do an "edit",
or even "rebase --abort".

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 git-rebase--interactive.sh |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 804001b..5812222 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -143,6 +143,21 @@ die_with_patch () {
 	die "$2"
 }
 
+exit_with_patch () {
+	echo "$1" > "$state_dir"/stopped-sha
+	make_patch $1
+	git rev-parse --verify HEAD > "$amend"
+	warn "You can amend the commit now, with"
+	warn
+	warn "	git commit --amend"
+	warn
+	warn "Once you are satisfied with your changes, run"
+	warn
+	warn "	git rebase --continue"
+	warn
+	exit $2
+}
+
 die_abort () {
 	rm -rf "$state_dir"
 	die "$1"
@@ -408,7 +423,13 @@ do_next () {
 		mark_action_done
 		pick_one $sha1 ||
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
-		git commit --amend --no-post-rewrite
+		git commit --amend --no-post-rewrite || {
+			warn "Could not amend commit after successfully picking $sha1... $rest"
+			warn "This is most likely due to an empty commit message, or the pre-commit hook"
+			warn "failed. If the pre-commit hook failed, you may need to resolve the issue before"
+			warn "you are able to reword the commit."
+			exit_with_patch $sha1 1
+		}
 		record_in_rewritten $sha1
 		;;
 	edit|e)
@@ -417,19 +438,8 @@ do_next () {
 		mark_action_done
 		pick_one $sha1 ||
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
-		echo "$sha1" > "$state_dir"/stopped-sha
-		make_patch $sha1
-		git rev-parse --verify HEAD > "$amend"
 		warn "Stopped at $sha1... $rest"
-		warn "You can amend the commit now, with"
-		warn
-		warn "	git commit --amend"
-		warn
-		warn "Once you are satisfied with your changes, run"
-		warn
-		warn "	git rebase --continue"
-		warn
-		exit 0
+		exit_with_patch $sha1 0
 		;;
 	squash|s|fixup|f)
 		case "$command" in
-- 
1.7.8.rc3.32.gb0399.dirty