Re: [PATCH] rebase -i: handle fixup of root commit correctly

[Johannes Sixt <j6t@kdbg.org>]

Am 24.07.2012 14:17, schrieb Chris Webb:
> There is a bug with git rebase -i --root when a fixup or squash line is
> applied to the new root. We attempt to amend the commit onto which they
> apply with git reset --soft HEAD^ followed by a normal commit. Unlike a
> real commit --amend, this sequence will fail against a root commit as it
> has no parent.
>
> Fix rebase -i to use commit --amend for fixup and squash instead, and
> add a test for the case of a fixup of the root commit.
>
> Signed-off-by: Chris Webb<chris@arachsys.com>
> ---
>
> Sorry, I should have spotted this issue when I did the original root-rebase
> series. I've checked that this patch doesn't break any of the existing
> tests, as well as satisfying the newly introduced check for the root-fixup
> case.
>
>   git-rebase--interactive.sh    | 25 +++++++++++++------------
>   t/t3404-rebase-interactive.sh |  8 ++++++++
>   2 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index bef7bc0..0d2056f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -493,25 +493,28 @@ do_next () {
>   		author_script_content=$(get_author_ident_from_commit HEAD)
>   		echo "$author_script_content">  "$author_script"
>   		eval "$author_script_content"
> -		output git reset --soft HEAD^
> -		pick_one -n $sha1 || die_failed_squash $sha1 "$rest"
> +		if ! pick_one -n $sha1
> +		then
> +			git rev-parse --verify HEAD>"$amend"
> +			die_failed_squash $sha1 "$rest"
> +		fi
>   		case "$(peek_next_command)" in
>   		squash|s|fixup|f)
>   			# This is an intermediate commit; its message will only be
>   			# used in case of trouble.  So use the long version:
> -			do_with_author output git commit --no-verify -F "$squash_msg" ||
> +			do_with_author output git commit --amend --no-verify -F "$squash_msg" ||
>   				die_failed_squash $sha1 "$rest"

This new sequence looks *VERY* suspicious. It makes a HUGE difference in 
what is left behind if the cherry-pick fails. Did you think about what 
happens when the cherry-pick fails in a squash+squash+fixup+fixup sequence 
(or any combination thereof) and then the rebase is continued (after a 
manual resolution)?

>   			;;
>   		*)
>   			# This is the final command of this squash/fixup group
>   			if test -f "$fixup_msg"
>   			then
> -				do_with_author git commit --no-verify -F "$fixup_msg" ||
> +				do_with_author git commit --amend --no-verify -F "$fixup_msg" ||
>   					die_failed_squash $sha1 "$rest"
>   			else
>   				cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
>   				rm -f "$GIT_DIR"/MERGE_MSG
> -				do_with_author git commit --no-verify -e ||
> +				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e ||
>   					die_failed_squash $sha1 "$rest"
>   			fi
>   			rm -f "$squash_msg" "$fixup_msg"
> @@ -748,7 +751,6 @@ In both case, once you're done, continue with:
>   		fi
>   		. "$author_script" ||
>   			die "Error trying to find the author identity to amend commit"
> -		current_head=
>   		if test -f "$amend"
>   		then
>   			current_head=$(git rev-parse --verify HEAD)
> @@ -756,13 +758,12 @@ In both case, once you're done, continue with:
>   			die "\
>   You have uncommitted changes in your working tree. Please, commit them
>   first and then run 'git rebase --continue' again."
> -			git reset --soft HEAD^ ||
> -			die "Cannot rewind the HEAD"
> +			do_with_author git commit --amend --no-verify -F "$msg" -e ||
> +				die "Could not commit staged changes."
> +		else
> +			do_with_author git commit --no-verify -F "$msg" -e ||
> +				die "Could not commit staged changes."
>   		fi
> -		do_with_author git commit --no-verify -F "$msg" -e || {
> -			test -n "$current_head"&&  git reset --soft $current_head
> -			die "Could not commit staged changes."
> -		}
>   	fi
>
>   	record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8078db6..3f75d32 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -903,4 +903,12 @@ test_expect_success 'rebase -i --root temporary sentinel commit' '
>   	git rebase --abort
>   '
>
> +test_expect_success 'rebase -i --root fixup root commit' '
> +	git checkout B&&
> +	FAKE_LINES="1 fixup 2" git rebase -i --root&&
> +	test A = $(git cat-file commit HEAD | sed -ne \$p)&&
> +	test B = $(git show HEAD:file1)&&
> +	test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
> +'
> +
>   test_done

-- Hannes