git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Sixt <j6t@kdbg.org>
Date
Jul 31, 2012, 09:14 UTC
Message-ID
<5017A1E4.1070800@kdbg.org>
In-Reply-To
<20120724121703.GG26014@arachsys.com>
Am 24.07.2012 14:17, schrieb Chris Webb:
Show 43 quoted lines
> 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)?

Show 60 quoted lines
>   			;;
>   		*)
>   			# 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
Previous: Junio C HamanoNext: Chris Webb
Message 3 of 7 in “rebase -i: handle fixup of root commit correctly”
  1. rebase -i: handle fixup of root commit correctlyChris Webb, Jul 24, 2012
  2. Junio C HamanoJul 24, 2012
  3. Johannes SixtJul 31, 2012
  4. Chris WebbJul 31, 2012
  5. Chris WebbJul 31, 2012
  6. Johannes SixtJul 31, 2012
  7. Chris WebbJul 31, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.