Re: [PATCH] Reallow git-rebase --interactive --continue if commit is unnecessary

[Junio C Hamano <gitster@pobox.com>]

Will do, but the code looks quite bad (not entirely your fault).

Line by line comment to show my puzzlement.

 		# commit if necessary

Ok, the user has prepared the index for us, and we are going to do some
tests and conditionally create commit.

 		git rev-parse --verify HEAD > /dev/null &&

Do we have HEAD commit?  Why check this --- we do not want to rebase
from the beginning of time?  No, that's not it.  If this fails, there is
something seriously wrong.  This is not about "will we make a commit?"
check at all.  This is a basic sanity check and if it fails we must
abort, not just skip.

 		git update-index --refresh &&
 		git diff-files --quiet &&

Is the work tree clean with respect to the index?  Why check this --- we
want to skip the commit if work tree is dirty?  Or is this trying to
enforce the invariant that during the rebase the work tree and index and
HEAD should all match?  If the latter, failure from this again is a
reason to abort.

 		! git diff-index --cached --quiet HEAD -- &&

Do we have something to commit?  This needs to be checked so that we can
skip a commit that results in emptyness, so using this as a check to see
if we should commit makes sense.

 		. "$DOTEST"/author-script && {
 			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
 		} &&

Find GIT_AUTHOR_* variables and if we are amending rewind the HEAD.  The
failure from this is a grave problem and reason to abort, isn't it?

 		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
		git commit --no-verify -F "$DOTEST"/message -e

Then we go on to create commit.  As you said, failure from this is a
grave error.

If my commentary above is right, how many bugs did we find in these 10
lines?

Grumpy I am...

---
 git-rebase--interactive.sh |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 090c3e5..7aa4278 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -363,17 +363,26 @@ do
 
 		test -d "$DOTEST" || die "No interactive rebase running"
 
-		# commit if necessary
-		git rev-parse --verify HEAD > /dev/null &&
-		git update-index --refresh &&
-		git diff-files --quiet &&
-		! git diff-index --cached --quiet HEAD -- &&
-		. "$DOTEST"/author-script && {
-			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
-		} &&
-		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
-		if ! git commit --no-verify -F "$DOTEST"/message -e
+		# Sanity check
+		git rev-parse --verify HEAD >/dev/null ||
+			die "Cannot read HEAD"
+		git update-index --refresh && git diff-files --quiet ||
+			die "Working tree is dirty"
+
+		# do we have anything to commit?
+		if git diff-index --cached --quiet HEAD --
 		then
+			: Nothing to commit -- skip this
+		else
+			. "$DOTEST"/author-script ||
+				die "Cannot find the author identity"
+			if test -f "$DOTEST"/amend
+			then
+				git reset --soft HEAD^ ||
+				die "Cannot rewind the HEAD"
+			fi
+			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
+			git commit --no-verify -F "$DOTEST"/message -e ||
 			die "Could not commit staged changes."
 		fi