threads / bug / 28959

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

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

## tl;dr

6 messages between Nov 17, 2011 and Nov 30, 2011.

replies: 5people: 3as markdown or json

Alexey Shumkin· Nov 17, 2011, 08:58 UTC · lore

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

Andrew Wong· Nov 28, 2011, 16:15 UTC · re: Alexey Shumkin · lore

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

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

Andrew Wong· Nov 28, 2011, 16:15 UTC · re: Andrew Wong · lore

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

"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

Junio C Hamano· Nov 29, 2011, 20:08 UTC · re: Andrew Wong · lore

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

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

Show 7 quoted lines

> "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"?

Show 17 quoted lines

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

Andrew Wong· Nov 30, 2011, 15:52 UTC · re: Junio C Hamano · lore

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

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

Andrew Wong· Nov 30, 2011, 15:52 UTC · re: Andrew Wong · lore

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

"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