Junio C Hamano <gitster@xxxxxxxxx> 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
>
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
