[PATCH 2/2] Catch and handle git-commit failures in git-rebase --interactive

Shawn O. Pearce· Dec 19, 2007, 06:45 UTC · lore

If git-commit fails for any reason then git-rebase needs to stop and not plow through the rest of the series. Its unlikely that a future git-commit will succeed if the current attempt failed.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 Comments welcome.  This is on top of my 1/2 patch but we could
 drop my 1/2 and rewrite this to not use --no-verify, but handle
 the git-commit error correctly.

 However that would force users to fix whitespace errors in later
 patches in a series if they use -i, even though non-i wouldn't
 require that sort of fix-up.  So I think we should do both patches
 in the series.

 This has also bitten me a few times. :-\

 git-rebase--interactive.sh |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

Show changes to git-rebase--interactive.sh +9 −8

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b846ea8..0f7d939 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -289,22 +289,22 @@ do_next () {
 		output git reset --soft HEAD^
 		pick_one -n $sha1 || failed=t
 		echo "$author_script" > "$DOTEST"/author-script
-		case $failed in
-		f)
+		if test $failed = f
+		then
 			# This is like --amend, but with a different message
 			eval "$author_script"
 			GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
 			GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
 			GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
-			$USE_OUTPUT git commit --no-verify -F "$MSG" $EDIT_COMMIT
-			;;
-		t)
+			$USE_OUTPUT git commit --no-verify -F "$MSG" $EDIT_COMMIT || failed=t
+		fi
+		if test $failed = t
+		then
 			cp "$MSG" "$GIT_DIR"/MERGE_MSG
 			warn
 			warn "Could not apply $sha1... $rest"
 			die_with_patch $sha1 ""
-			;;
-		esac
+		fi
 		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
@@ -372,7 +372,8 @@ do
 			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
 		} &&
 		export GIT_AUTHOR_NAME GIT_AUTHOR_NAME GIT_AUTHOR_DATE &&
-		git commit --no-verify -F "$DOTEST"/message -e
+		git commit --no-verify -F "$DOTEST"/message -e ||
+			die "Could not commit staged changes."
 
 		require_clean_work_tree
 		do_rest

-- 
1.5.4.rc0.1155.g12ed9

Junio C Hamano· Dec 19, 2007, 07:03 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 2/2] Catch and handle git-commit failures in git-rebase --interactive

"Shawn O. Pearce" <spearce@spearce.org> writes:

Show 8 quoted lines

>  Comments welcome.  This is on top of my 1/2 patch but we could
>  drop my 1/2 and rewrite this to not use --no-verify, but handle
>  the git-commit error correctly.
>
>  However that would force users to fix whitespace errors in later
>  patches in a series if they use -i, even though non-i wouldn't
>  require that sort of fix-up.  So I think we should do both patches
>  in the series.

I agree with both patches.

When one wants to use rebase to fix-up whitespace breakage in patches in bulk, you can set apply.whitespace to "fix". One bad side effect of this is that if you usually have apply.whitespace set to "fix" (because you need to accept a lot of patches but your contributers tend to give crappy patches), you need to temporarily change the configuration while rebasing if you do not want to preserve intentional whitespace breakages (e.g. ones in test scripts). This can be argued either a feature or a misfeature.

But rebase -i breaking and squashing upon commit failure (including pre-commit safety) cannot be called either feature nor misfeature --- it is an outright bug.

Johannes Schindelin· Dec 19, 2007, 12:21 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 2/2] Catch and handle git-commit failures in git-rebase --interactive

Hi,

On Wed, 19 Dec 2007, Shawn O. Pearce wrote:

>  This has also bitten me a few times. :-\

Sorry to hear that.  Both patches look sensible to me.

Ciao, Dscho

patch, 2 partsCatch and handle git-commit failures in git-rebase --interactive