rebase -i: Make option handling in pick_one more flexible

pick_one and pick_one_preserving_merges are wrappers around cherry-pick in rebase --interactive. They take the hash of a commit and build a cherry-pick command line that

- respects the user-supplied merge options
- disables complaints about empty commits
- tries to fast-forward the rebase head unless rebase is forced
- suppresses output unless the user requested higher verbosity
- rewrites merge commits to point to their rebased parents.

pick_one is used to implement not only pick but also squash, which amends the previous commit rather than creating a new one. When pick_one is called from squash, it receives a second argument -n. This tells pick_one to apply the changes to the index without committing them. Since the argument is almost directly passed to cherry-pick, we might want to do the same with other cherry-pick options. Currently, pick_one expects -n to be the first and only argument except for the commit hash.

Prepare pick_one for additional cherry-pick options by allowing -n to appear anywhere before the commit hash in the argument list. Loop over the argument list and pop each handled item until the commit hash is the only parameter left on the list. If an option is not supported, ignore it and issue a warning on the console. Construct a new arguments list extra_args of recognized options that shall be passed to cherry-pick on the command line.

Signed-off-by Fabian Ruch <[email protected]>

---
 git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 16 deletions(-)

git-rebase--interactive.sh +1 -0

index f267d8b..ea5514e 100644
@@ -237,8 +237,26 @@ git_sequence_editor () {
 
 pick_one () {
 	ff=--ff
+	extra_args=

git-rebase--interactive.sh +8 -1

+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	test $# -ne 1 && die "pick_one: wrong number of arguments"
+	sha1=$1
 
-	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
 	case "$force_rebase" in '') ;; ?*) ff= ;; esac
 	output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
 

git-rebase--interactive.sh +24 -13

 pick_one_preserving_merges () {
 	fast_forward=t
-	case "$1" in
-	-n)
-		fast_forward=f
-		sha1=$2
-		;;
-	*)
-		sha1=$1
-		;;
-	esac
-	sha1=$(git rev-parse $sha1)
+	no_commit=
+	extra_args=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-n)
+			fast_forward=f
+			extra_args="$extra_args -n"
+			no_commit=y
+			;;
+		-*)
+			warn "pick_one_preserving_merges: ignored option -- $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	test $# -ne 1 && die "pick_one_preserving_merges: wrong number of arguments"
+	sha1=$(git rev-parse $1)
 
 	if test -f "$state_dir"/current-commit
 	then

@@ -335,7 +364,7 @@ pick_one_preserving_merges () {
 	f)
 		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
 
-		if [ "$1" != "-n" ]
+		if test -z "$no_commit"
 		then
 			# detach HEAD to current parent
 			output git checkout $first_parent 2> /dev/null ||

@@ -344,7 +373,7 @@ pick_one_preserving_merges () {
 
 		case "$new_parents" in
 		' '*' '*)
-			test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
+			test -n "$no_commit" && die "Refusing to squash a merge: $sha1"
 
 			# redo merge
 			author_script_content=$(get_author_ident_from_commit $sha1)

@@ -365,7 +394,7 @@ pick_one_preserving_merges () {
 		*)
 			output eval git cherry-pick \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-				"$strategy_args" "$@" ||
+				"$strategy_args" $extra_args $sha1 ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac

Quoted 41 lines · Fabian Ruch

pick_one and pick_one_preserving_merges are wrappers around cherry-pick in rebase --interactive. They take the hash of a commit and build a cherry-pick command line that

- respects the user-supplied merge options
- disables complaints about empty commits
- tries to fast-forward the rebase head unless rebase is forced
- suppresses output unless the user requested higher verbosity
- rewrites merge commits to point to their rebased parents.

pick_one is used to implement not only pick but also squash, which amends the previous commit rather than creating a new one. When pick_one is called from squash, it receives a second argument -n. This tells pick_one to apply the changes to the index without committing them. Since the argument is almost directly passed to cherry-pick, we might want to do the same with other cherry-pick options. Currently, pick_one expects -n to be the first and only argument except for the commit hash.

Prepare pick_one for additional cherry-pick options by allowing -n to appear anywhere before the commit hash in the argument list. Loop over the argument list and pop each handled item until the commit hash is the only parameter left on the list. If an option is not supported, ignore it and issue a warning on the console. Construct a new arguments list extra_args of recognized options that shall be passed to cherry-pick on the command line.

Signed-off-by Fabian Ruch <[email protected]>

---
 git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 16 deletions(-)

git-rebase--interactive.sh +1 -0

index f267d8b..ea5514e 100644
@@ -237,8 +237,26 @@ git_sequence_editor () {
 
 pick_one () {
 	ff=--ff
+	extra_args=

git-rebase--interactive.sh git_sequence_editor () { · line 250

Quoted 10 lines

git-rebase--interactive.sh +10 -0

+	while test $# -gt 0
+	do
+		case "$1" in
+		-n)
+			ff=
+			extra_args="$extra_args -n"
+			;;
+		-*)
+			warn "pick_one: ignored option -- $1"
+			;;

This is an internal interface, right? I.e., user input isn't being processed here? If so, then the presence of an unrecognized option is a bug and it is preferable to "die" here rather than "warn".

The same below and in at least one later commit.

Quoted 16 lines

git-rebase--interactive.sh +8 -1

+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	test $# -ne 1 && die "pick_one: wrong number of arguments"
+	sha1=$1
 
-	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
 	case "$force_rebase" in '') ;; ?*) ff= ;; esac
 	output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
 

@@ -248,24 +266,35 @@ pick_one () {
 	fi
 

git-rebase--interactive.sh pick_one () { · line 273

Quoted 8 lines

git-rebase--interactive.sh +2 -2

 	test -d "$rewritten" &&
-		pick_one_preserving_merges "$@" && return
+		pick_one_preserving_merges $extra_args $sha1 && return
 	output eval git cherry-pick \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-			"$strategy_args" $empty_args $ff "$@"
+			"$strategy_args" $empty_args $ff $extra_args $sha1
 }

It might be confusing that extra_args is used both in pick_one and in pick_one_preserving_merges. Since these are not local variables, the call to the latter changes the value of the variable in the former. I don't know if that could be a problem now (can pick_one_preserving_merges return with a nonzero exit code?) but even if not, it is a trap for future developers. I recommend giving the two variables different names.

if test -d "$rewritten"
then
pick_one_preserving_merges "$@"
else

	output eval git cherry-pick \
			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
			"$strategy_args" $empty_args $ff "$@"

fi

Quoted 64 lines

git-rebase--interactive.sh +24 -13

 pick_one_preserving_merges () {
 	fast_forward=t
-	case "$1" in
-	-n)
-		fast_forward=f
-		sha1=$2
-		;;
-	*)
-		sha1=$1
-		;;
-	esac
-	sha1=$(git rev-parse $sha1)
+	no_commit=
+	extra_args=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-n)
+			fast_forward=f
+			extra_args="$extra_args -n"
+			no_commit=y
+			;;
+		-*)
+			warn "pick_one_preserving_merges: ignored option -- $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	test $# -ne 1 && die "pick_one_preserving_merges: wrong number of arguments"
+	sha1=$(git rev-parse $1)
 
 	if test -f "$state_dir"/current-commit
 	then

@@ -335,7 +364,7 @@ pick_one_preserving_merges () {
 	f)
 		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
 
-		if [ "$1" != "-n" ]
+		if test -z "$no_commit"
 		then
 			# detach HEAD to current parent
 			output git checkout $first_parent 2> /dev/null ||

@@ -344,7 +373,7 @@ pick_one_preserving_merges () {
 
 		case "$new_parents" in
 		' '*' '*)
-			test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
+			test -n "$no_commit" && die "Refusing to squash a merge: $sha1"
 
 			# redo merge
 			author_script_content=$(get_author_ident_from_commit $sha1)

@@ -365,7 +394,7 @@ pick_one_preserving_merges () {
 		*)
 			output eval git cherry-pick \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-				"$strategy_args" "$@" ||
+				"$strategy_args" $extra_args $sha1 ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac

Quoted 45 lines · Michael Haggerty

Quoted 44 lines · Fabian Ruch

pick_one and pick_one_preserving_merges are wrappers around cherry-pick in rebase --interactive. They take the hash of a commit and build a cherry-pick command line that

- respects the user-supplied merge options
- disables complaints about empty commits
- tries to fast-forward the rebase head unless rebase is forced
- suppresses output unless the user requested higher verbosity
- rewrites merge commits to point to their rebased parents.

pick_one is used to implement not only pick but also squash, which amends the previous commit rather than creating a new one. When pick_one is called from squash, it receives a second argument -n. This tells pick_one to apply the changes to the index without committing them. Since the argument is almost directly passed to cherry-pick, we might want to do the same with other cherry-pick options. Currently, pick_one expects -n to be the first and only argument except for the commit hash.

Prepare pick_one for additional cherry-pick options by allowing -n to appear anywhere before the commit hash in the argument list. Loop over the argument list and pop each handled item until the commit hash is the only parameter left on the list. If an option is not supported, ignore it and issue a warning on the console. Construct a new arguments list extra_args of recognized options that shall be passed to cherry-pick on the command line.

[...]

git-rebase--interactive.sh +8 -1

+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	test $# -ne 1 && die "pick_one: wrong number of arguments"
+	sha1=$1
 
-	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
 	case "$force_rebase" in '') ;; ?*) ff= ;; esac
 	output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"

@@ -248,24 +266,35 @@ pick_one () {
 	fi
 

Quoted 16 lines

git-rebase--interactive.sh pick_one () { · line 273

Quoted 8 lines

git-rebase--interactive.sh +2 -2

 	test -d "$rewritten" &&
-		pick_one_preserving_merges "$@" && return
+		pick_one_preserving_merges $extra_args $sha1 && return
 	output eval git cherry-pick \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-			"$strategy_args" $empty_args $ff "$@"
+			"$strategy_args" $empty_args $ff $extra_args $sha1
 }

It might be confusing that extra_args is used both in pick_one and in pick_one_preserving_merges. Since these are not local variables, the call to the latter changes the value of the variable in the former. I don't know if that could be a problem now (can pick_one_preserving_merges return with a nonzero exit code?) but even if not, it is a trap for future developers. I recommend giving the two variables different names.

Please correct me if I missed something. At the moment (786a89d), pick_one_preserving_merges will not and cannot return with a non-zero exit code because it doesn't explicitly return and all possibly failing last statements are guarded with a ... || die "...". Since this can only be established by carefully looking at the code, I will change the reuse of extra_args.

Now that we're at it, what I didn't understand when creating this patch was why the code doesn't explicitly say "one or the other" in the first place:

Quoted 8 lines

if test -d "$rewritten"
then
pick_one_preserving_merges "$@"
else

	output eval git cherry-pick \
			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
			"$strategy_args" $empty_args $ff "$@"

fi

At least that is how I interpreted it. After all, if pick_one_preserving_merges failed to recreate a merge commit, wouldn't it be a bug to record the changes in a single-parent commit?

Johannes, I cc'd you this email since you're the author of f09c9b8 - "Teach rebase -i about --preserve-merges". I hope you don't resent me digging out this seven-year-old patch.

git-rebase--interactive.sh git_sequence_editor () { · line 250

Quoted 13 lines

pick_one () {

git-rebase--interactive.sh +11 -0

 	ff=--ff
+	extra_args=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-n)
+			ff=
+			extra_args="$extra_args -n"
+			;;
+		-*)
+			warn "pick_one: ignored option -- $1"
+			;;

This is an internal interface, right? I.e., user input isn't being processed here? If so, then the presence of an unrecognized option is a bug and it is preferable to "die" here rather than "warn".

The same below and in at least one later commit.

Quoted 18 lines · Michael Haggerty

git-rebase--interactive.sh git_sequence_editor () { · line 250

Quoted 13 lines

pick_one () {

git-rebase--interactive.sh +11 -0

 	ff=--ff
+	extra_args=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-n)
+			ff=
+			extra_args="$extra_args -n"
+			;;
+		-*)
+			warn "pick_one: ignored option -- $1"
+			;;

This is an internal interface, right? I.e., user input isn't being processed here? If so, then the presence of an unrecognized option is a bug and it is preferable to "die" here rather than "warn".

Quoted 1 line

The same below and in at least one later commit.

And if this is purely an internal interface, then I really do not see the point of allowing -n to be anywhere other than the front. If we are planning to accept other random options to cherry-pick in later steps, but we are not yet doing so at this step, then I do not thin we want to have any loop like this before we actually start accepting and passing them to the underlying cherry-pick.

Furthermore, if the "-n" is currently used as an internal signal from the caller to pick_one() that it is executing the end-user supplied "squash" in the insn sheet, it may be a good idea to change that "-n" to something that is *NOT* a valid option to cherry-pick at this step, before we start accepting user-supplied options and relaying them to underlying cherry-pick.

One way to do so cleanly may be to _always_ add the type of pick as the first parameter to pick_one, i.e. either "pick" or "squash", and do:

        pick_one () {
                ...
                n_arg=
                case "$1" in
                pick) ;;
                squash) n_arg=-n ;;
                *)	die "BUG: pick_one $1???" ;;
                esac
                shift
                sha1=$1
                ...
                output eval git cherry-pick $n_arg \
                        ...
        }

Also I suspect that you would need to be careful *not* to allow "-n" to be given as part of the "random user-specified options" and pass that to cherry-pick in the later steps of your series [*1*], and for that you may need a loop that inspects the arguments like you had in this patch.

[Footnote]

*1* The existing callers of "pick_one -n" very well know and expect
that the step will only update the working tree and the index and it is the callers' responsibility to create a commit out of that state (either by amending or committing); similarly the existing callers of "pick_one" without "-n" very well know and expect that the step will make a commit unless there is a problem. I do not think you would consider it such a "problem to replay the change in the named commit" for the end user's insn sheet to pass a "-n".

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. If one thinks of pick entries as scheduled cherry-pick command lines, then reword becomes an alias for the command line cherry-pick --edit. The porcelain rebase--interactive defines a function do_pick for processing the pick entries on to-do lists. Teach do_pick to handle the option --edit and reimplement reword in terms of do_pick --edit. Refer to pick_one for the way options are parsed.

do_pick ought to act as a wrapper around cherry-pick. Unfortunately, it cannot just forward --edit to the cherry-pick command line. The assembled command line is executed within a command substitution for controlling the verbosity of rebase--interactive. Passing --edit would either hang the terminal or clutter the substituted command output with control sequences. Execute the reword code from do_next instead if the option --edit is specified. Adjust the fragment in two regards. Firstly, discard the additional message which is printed if an error occurs because

Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.) Could not amend commit after successfully picking 1234567... Some change

is more readable than and contains (almost) the same information as

Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.) Could not amend commit after successfully picking 1234567... Some change This is most likely due to an empty commit message, or the pre-commit hook failed. If the pre-commit hook failed, you may need to resolve the issue before you are able to reword the commit.

(It is true that a hook might not output any diagnosis but the same problem arises when using git-commit directly. git-rebase at least prints a generic message saying that it failed to commit.) Secondly, sneak in additional git-commit arguments:

- --allow-empty is missing: rebase--interactive suddenly fails if an empty commit is picked using reword instead of pick. The whether a commit is empty or not is not changed by an altered log message, so do as pick does. Add test.

- -n: Hide the fact that we are committing several times by not executing the pre-commit hook. Caveat: The altered log message is not verified because -n also skips the commit-msg hook. Either the log message verification must be included in the post-rewrite hook or git-commit must support more fine-grained control over which hooks are executed.

- -q: Hide the fact that we are committing several times by not printing the commit summary.

Signed-off-by Fabian Ruch <[email protected]>

---
 git-rebase--interactive.sh    | 52 ++++++++++++++++++++++++++++++++++++-------
 t/t3404-rebase-interactive.sh |  8 +++++++
 2 files changed, 52 insertions(+), 8 deletions(-)

git-rebase--interactive.sh +2 -0

index ea5514e..fffdfa5 100644
@@ -490,7 +490,42 @@ record_in_rewritten() {
 	esac
 }
 
+# Apply the changes introduced by the given commit to the current head.
+#

git-rebase--interactive.sh +32 -8

+#
+# -e, --edit
+#     After picking <commit>, open an editor and let the user edit the
+#     commit message. The editor contents becomes the commit message of
+#     the new head.
 do_pick () {
+	edit=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-e|--edit)
+			edit=y
+			;;
+		-*)
+			warn "do_pick: ignored option -- $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	test $# -ne 2 && die "do_pick: wrong number of arguments"
+
 	if test "$(git rev-parse HEAD)" = "$squash_onto"
 	then
 		# Set the correct commit message and author info on the

@@ -512,6 +547,14 @@ do_pick () {
 		pick_one $1 ||
 			die_with_patch $1 "Could not apply $1... $2"
 	fi
+
+	if test -n "$edit"
+	then
+		git commit --allow-empty --amend --no-post-rewrite -n -q ${gpg_sign_opt:+"$gpg_sign_opt"} || {
+			warn "Could not amend commit after successfully picking $1... $2"
+			exit_with_patch $1 1
+		}
+	fi
 }
 
 do_next () {

@@ -532,14 +575,7 @@ do_next () {
 		comment_for_reflog reword
 
 		mark_action_done
-		do_pick $sha1 "$rest"
-		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
-			warn "Could not amend commit after successfully picking $sha1... $rest"
-			warn "This is most likely due to an empty commit message, or the pre-commit hook"
-			warn "failed. If the pre-commit hook failed, you may need to resolve the issue before"
-			warn "you are able to reword the commit."
-			exit_with_patch $sha1 1
-		}
+		do_pick --edit $sha1 "$rest"
 		record_in_rewritten $sha1
 		;;
 	edit|e)

t/t3404-rebase-interactive.sh +8 -0

index 8197ed2..9931143 100755
@@ -75,6 +75,14 @@ test_expect_success 'rebase --keep-empty' '
 	test_line_count = 6 actual
 '
 
+test_expect_success 'rebase --keep-empty' '
+	git checkout emptybranch &&
+	set_fake_editor &&
+	FAKE_LINES="1 reword 2" git rebase --keep-empty -i HEAD~2 &&
+	git log --oneline >actual &&
+	test_line_count = 6 actual
+'
+
 test_expect_success 'rebase -i with the exec command' '
 	git checkout master &&
 	(

Quoted 47 lines · Fabian Ruch

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. If one thinks of pick entries as scheduled cherry-pick command lines, then reword becomes an alias for the command line cherry-pick --edit. The porcelain rebase--interactive defines a function do_pick for processing the pick entries on to-do lists. Teach do_pick to handle the option --edit and reimplement reword in terms of do_pick --edit. Refer to pick_one for the way options are parsed.

do_pick ought to act as a wrapper around cherry-pick. Unfortunately, it cannot just forward --edit to the cherry-pick command line. The assembled command line is executed within a command substitution for controlling the verbosity of rebase--interactive. Passing --edit would either hang the terminal or clutter the substituted command output with control sequences. Execute the reword code from do_next instead if the option --edit is specified. Adjust the fragment in two regards. Firstly, discard the additional message which is printed if an error occurs because

Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.) Could not amend commit after successfully picking 1234567... Some change

is more readable than and contains (almost) the same information as

Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.) Could not amend commit after successfully picking 1234567... Some change This is most likely due to an empty commit message, or the pre-commit hook failed. If the pre-commit hook failed, you may need to resolve the issue before you are able to reword the commit.

(It is true that a hook might not output any diagnosis but the same problem arises when using git-commit directly. git-rebase at least prints a generic message saying that it failed to commit.) Secondly, sneak in additional git-commit arguments:

- --allow-empty is missing: rebase--interactive suddenly fails if an empty commit is picked using reword instead of pick. The whether a commit is empty or not is not changed by an altered log message, so do as pick does. Add test.

- -n: Hide the fact that we are committing several times by not executing the pre-commit hook. Caveat: The altered log message is not verified because -n also skips the commit-msg hook. Either the log message verification must be included in the post-rewrite hook or git-commit must support more fine-grained control over which hooks are executed.

Quoted 2 lines

- -q: Hide the fact that we are committing several times by not printing the commit summary.

It is preferable that each commit makes one logical change (though it must always be a self-contained change; i.e., the code should never be broken at the end of a commit). It would be clearer if you would split this commit into one refactoring commit (moving the handling of --edit to do_pick) plus one commit for each "git commit" option change and error message change. That way,

* Each commit (and log message) becomes simpler, making it easier to review.
* The changes can be discussed separately.
* If there is an error, "git bisect" can help determine which of the changes is at fault.

Quoted 14 lines

Signed-off-by Fabian Ruch <[email protected]>

---
 git-rebase--interactive.sh    | 52 ++++++++++++++++++++++++++++++++++++-------
 t/t3404-rebase-interactive.sh |  8 +++++++
 2 files changed, 52 insertions(+), 8 deletions(-)

git-rebase--interactive.sh +0 -0

index ea5514e..fffdfa5 100644
@@ -490,7 +490,42 @@ record_in_rewritten() {
 	esac
 }
 

git-rebase--interactive.sh record_in_rewritten() { · line 504

Quoted 12 lines

git-rebase--interactive.sh +12 -0

+# Apply the changes introduced by the given commit to the current head.
+#
+# do_pick [--edit] <commit> <title>
+#
+# Wrapper around git-cherry-pick.
+#
+# <title>
+#     The commit message title of <commit>. Used for information
+#     purposes only.
+#
+# <commit>
+#     The commit to cherry-pick.

Unless there is a reason to do otherwise, please order the documentation to match the order that the do_pick arguments appear.

Quoted 77 lines

git-rebase--interactive.sh +32 -8

+#
+# -e, --edit
+#     After picking <commit>, open an editor and let the user edit the
+#     commit message. The editor contents becomes the commit message of
+#     the new head.
 do_pick () {
+	edit=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-e|--edit)
+			edit=y
+			;;
+		-*)
+			warn "do_pick: ignored option -- $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	test $# -ne 2 && die "do_pick: wrong number of arguments"
+
 	if test "$(git rev-parse HEAD)" = "$squash_onto"
 	then
 		# Set the correct commit message and author info on the

@@ -512,6 +547,14 @@ do_pick () {
 		pick_one $1 ||
 			die_with_patch $1 "Could not apply $1... $2"
 	fi
+
+	if test -n "$edit"
+	then
+		git commit --allow-empty --amend --no-post-rewrite -n -q ${gpg_sign_opt:+"$gpg_sign_opt"} || {
+			warn "Could not amend commit after successfully picking $1... $2"
+			exit_with_patch $1 1
+		}
+	fi
 }
 
 do_next () {

@@ -532,14 +575,7 @@ do_next () {
 		comment_for_reflog reword
 
 		mark_action_done
-		do_pick $sha1 "$rest"
-		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
-			warn "Could not amend commit after successfully picking $sha1... $rest"
-			warn "This is most likely due to an empty commit message, or the pre-commit hook"
-			warn "failed. If the pre-commit hook failed, you may need to resolve the issue before"
-			warn "you are able to reword the commit."
-			exit_with_patch $sha1 1
-		}
+		do_pick --edit $sha1 "$rest"
 		record_in_rewritten $sha1
 		;;
 	edit|e)

t/t3404-rebase-interactive.sh +8 -0

index 8197ed2..9931143 100755
@@ -75,6 +75,14 @@ test_expect_success 'rebase --keep-empty' '
 	test_line_count = 6 actual
 '
 
+test_expect_success 'rebase --keep-empty' '
+	git checkout emptybranch &&
+	set_fake_editor &&
+	FAKE_LINES="1 reword 2" git rebase --keep-empty -i HEAD~2 &&
+	git log --oneline >actual &&
+	test_line_count = 6 actual
+'
+
 test_expect_success 'rebase -i with the exec command' '
 	git checkout master &&
 	(

When rebase is executed with --root but no --onto is specified, rebase creates a sentinel commit which is replaced with the root commit in three steps. This combination of options results never in a fast-forward.

1. The sentinel commit is forced into the authorship of the root commit.

2. The changes introduced by the root commit are applied to the index but not committed. If this step fails for whatever reason, all commit information will be there and the user can safely run git-commit --amend after resolving the problems.

3. The new root commit is created by squashing the changes into the sentinel commit which already carries the authorship of the cherry-picked root commit.

The command line used to create the commit in the third step specifies effectless and erroneous options. Remove those.

- --allow-empty-message is erroneous: If the root's commit message is empty, the rebase shall fail like for any other commit that is on the to-do list and has an empty commit message.

Fix the bug that git-rebase does not fail when the initial commit has an empty log message but is replayed using --root is specified. Add test.

- -C is effectless: The commit being amended, which is the sentinel commit, already carries the authorship and log message of the cherry-picked root commit. The committer email and commit date fields are reset either way.

After all, if step two fails, rebase --continue won't include these flags in the git-commit command line either.

Signed-off-by Fabian Ruch <[email protected]>

---
 git-rebase--interactive.sh |  4 ++--
 t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

git-rebase--interactive.sh +2 -2

index fffdfa5..f09eeae 100644
@@ -539,8 +539,8 @@ do_pick () {
                git commit --allow-empty --allow-empty-message --amend \
                           --no-post-rewrite -n -q -C $1 &&
                        pick_one -n $1 &&
-                       git commit --allow-empty --allow-empty-message \
-                                  --amend --no-post-rewrite -n -q -C $1 \
+                       git commit --allow-empty --amend \
+                                  --no-post-rewrite -n -q \
                                   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
                        die_with_patch $1 "Could not apply $1... $2"
        else

t/t3412-rebase-root.sh +3 -0

index 0b52105..3608db4 100755
@@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '
        test_cmp expect-conflict-p out
 '
 
+test_expect_success 'stop rebase --root on empty root log message' '
+       # create a root commit with a non-empty tree so that rebase does
+       # not fail because of an empty commit, and an empty log message

t/t3412-rebase-root.sh +13 -0

+'
+
+test_expect_success 'stop rebase --root on empty child log message' '
+       # create a root commit with a non-empty tree and provide a log
+       # message so that rebase does not fail until the root commit is
+       # successfully replayed
+       echo root-commit >file &&
+       git add file &&
+       tree=$(git write-tree) &&
+       root=$(git commit-tree $tree -m root-commit) &&
+       git checkout -b no-message-child-commit $root &&
+       # create a child commit with a non-empty patch so that rebase
+       # does not fail because of an empty commit, but an empty log

t/t3412-rebase-root.sh +2 -1

+'
+
 test_done
--

2.0.0

Quoted 68 lines · Fabian Ruch

When rebase is executed with --root but no --onto is specified, rebase creates a sentinel commit which is replaced with the root commit in three steps. This combination of options results never in a fast-forward.

1. The sentinel commit is forced into the authorship of the root commit.

2. The changes introduced by the root commit are applied to the index but not committed. If this step fails for whatever reason, all commit information will be there and the user can safely run git-commit --amend after resolving the problems.

3. The new root commit is created by squashing the changes into the sentinel commit which already carries the authorship of the cherry-picked root commit.

The command line used to create the commit in the third step specifies effectless and erroneous options. Remove those.

- --allow-empty-message is erroneous: If the root's commit message is empty, the rebase shall fail like for any other commit that is on the to-do list and has an empty commit message.

Fix the bug that git-rebase does not fail when the initial commit has an empty log message but is replayed using --root is specified. Add test.

- -C is effectless: The commit being amended, which is the sentinel commit, already carries the authorship and log message of the cherry-picked root commit. The committer email and commit date fields are reset either way.

After all, if step two fails, rebase --continue won't include these flags in the git-commit command line either.

Signed-off-by Fabian Ruch <[email protected]>

---
 git-rebase--interactive.sh |  4 ++--
 t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

git-rebase--interactive.sh +2 -2

index fffdfa5..f09eeae 100644
@@ -539,8 +539,8 @@ do_pick () {
                git commit --allow-empty --allow-empty-message --amend \
                           --no-post-rewrite -n -q -C $1 &&
                        pick_one -n $1 &&
-                       git commit --allow-empty --allow-empty-message \
-                                  --amend --no-post-rewrite -n -q -C $1 \
+                       git commit --allow-empty --amend \
+                                  --no-post-rewrite -n -q \
                                   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
                        die_with_patch $1 "Could not apply $1... $2"
        else

t/t3412-rebase-root.sh +3 -0

index 0b52105..3608db4 100755
@@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '
        test_cmp expect-conflict-p out
 '
 
+test_expect_success 'stop rebase --root on empty root log message' '
+       # create a root commit with a non-empty tree so that rebase does
+       # not fail because of an empty commit, and an empty log message

t/t3412-rebase-root.sh test_expect_success 'rebase -i -p --root with conflict (second part)' ' · line 293

Quoted 10 lines

t/t3412-rebase-root.sh +10 -0

+       echo root-commit >file &&
+       git add file &&
+       tree=$(git write-tree) &&
+       root=$(git commit-tree $tree </dev/null) &&
+       git checkout -b no-message-root-commit $root &&
+       # do not ff because otherwise neither the patch nor the message
+       # are looked at and checked for emptiness
+       test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+       echo root-commit >file.expected &&
+       test_cmp file file.expected &&

It is customary, and provides nicer diagnostic output upon failure, to have the "expected" file mentioned first:

    test_cmp file.expected file &&

t/t3412-rebase-root.sh test_expect_success 'rebase -i -p --root with conflict (second part)' ' · line 294

Quoted 1 line

t/t3412-rebase-root.sh +1 -0

+       git rebase --abort

Do you want to place this under control of test_when_finished somewhere above the "git rebase" invocation to ensure cleanup if something fails before this point?

Quoted 17 lines

t/t3412-rebase-root.sh +17 -0

+'
+
+test_expect_success 'stop rebase --root on empty child log message' '
+       # create a root commit with a non-empty tree and provide a log
+       # message so that rebase does not fail until the root commit is
+       # successfully replayed
+       echo root-commit >file &&
+       git add file &&
+       tree=$(git write-tree) &&
+       root=$(git commit-tree $tree -m root-commit) &&
+       git checkout -b no-message-child-commit $root &&
+       # create a child commit with a non-empty patch so that rebase
+       # does not fail because of an empty commit, but an empty log
+       # message
+       echo child-commit >file &&
+       git add file &&
+       git commit --allow-empty-message --no-edit &&

t/t3412-rebase-root.sh test_expect_success 'rebase -i -p --root with conflict (second part)' ' · line 317

Quoted 6 lines

t/t3412-rebase-root.sh +6 -0

+       # do not ff because otherwise neither the patch nor the message
+       # are looked at and checked for emptiness
+       test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+       echo child-commit >file.expected &&
+       test_cmp file file.expected &&
+       git rebase --abort

Same two comments as for previous test.

Quoted 3 lines

t/t3412-rebase-root.sh +2 -0

+'
+
 test_done