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

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

Junio C Hamano <gitster@pobox.com> writes:

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

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

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