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

Hi,

On Wed, 26 Dec 2007, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

 		# 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.

Right.  My intention was to have a "|| die" at the end of the && cascade.

quoted

 		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.

Exactly.  I wanted to make sure that the working directory is not 
different from what is in the index.

quoted

 		! 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.

Any response to this or problems in the clean-up patch?

quoted

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

Looks good to me!  And I'm sorry for taking so long to respond.

Ciao,
Dscho