[RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

[RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

[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 <bafain@gmail.com>
---
 git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f267d8b..ea5514e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -237,8 +237,26 @@ git_sequence_editor () {
 
 pick_one () {
 	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"
+			;;
+		*)
+			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
 
 	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
 }
 
 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
-- 
2.0.0

[RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit

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

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

Signed-off-by: Fabian Ruch <bafain@gmail.com>
---
 git-rebase--interactive.sh    | 52 ++++++++++++++++++++++++++++++++++++-------
 t/t3404-rebase-interactive.sh |  8 +++++++
 2 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ea5514e..fffdfa5 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -490,7 +490,42 @@ record_in_rewritten() {
 	esac
 }
 
+# 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.
+#
+# -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)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8197ed2..9931143 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -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 &&
 	(
-- 
2.0.0

[RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages

[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 <bafain@gmail.com>
---
 git-rebase--interactive.sh |  4 ++--
 t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index fffdfa5..f09eeae 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -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
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 0b52105..3608db4 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -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
+	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 &&
+	git rebase --abort
+'
+
+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 &&
+	# 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
+'
+
 test_done
-- 
2.0.0

[RFC PATCH 4/7] rebase -i: Commit only once when rewriting picks

[Fabian Ruch]

The options passed to `do_pick` determine whether the picked commit will
be rewritten or not. If the commit gets rewritten, because the user
requested to edit the commit message for instance, let `pick_one` merely
apply the changes introduced by the commit and do not commit the
resulting tree yet. If the commit is replayed as is, leave it to
`pick_one` to recreate the commit (possibly by fast-forwarding the
head). This makes it easier to combine git-commit options like `--edit`
and `--amend` in `do_pick` because git-cherry-pick does not support
`--amend`.

In the case of `--edit`, do not `exit_with_patch` but assign `rewrite`
to pick the changes with `-n`. If the pick conflicts, no commit is
created which we would have to amend when continuing the rebase. To
complete the pick after the conflicts are resolved the user just resumes
with `git rebase --continue`.

If `rebase--interactive` is used to rebase a complete branch onto some
head, `rebase` creates a sentinel commit that requires special treatment
by `do_pick`. Do not finalize the pick here either because its commit
message can be altered as for any other pick. Since the orphaned root
commit gets a temporary parent, it is always rewritten. Safely use the
rewrite infrastructure of `do_pick` to create the final commit.

Signed-off-by: Fabian Ruch <bafain@gmail.com>
---
 git-rebase--interactive.sh | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f09eeae..f903599 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -63,7 +63,8 @@ msgnum="$state_dir"/msgnum
 author_script="$state_dir"/author-script
 
 # When an "edit" rebase command is being processed, the SHA1 of the
-# commit to be edited is recorded in this file.  When "git rebase
+# commit to be edited is recorded in this file.  The same happens when
+# rewriting a commit fails, for instance "reword".  When "git rebase
 # --continue" is executed, if there are any staged changes then they
 # will be amended to the HEAD commit, but only provided the HEAD
 # commit is still the commit to be edited.  When any other rebase
@@ -508,12 +509,15 @@ record_in_rewritten() {
 #     commit message. The editor contents becomes the commit message of
 #     the new head.
 do_pick () {
-	edit=
+	rewrite=
+	rewrite_amend=
+	rewrite_edit=
 	while test $# -gt 0
 	do
 		case "$1" in
 		-e|--edit)
-			edit=y
+			rewrite=y
+			rewrite_edit=y
 			;;
 		-*)
 			warn "do_pick: ignored option -- $1"
@@ -528,6 +532,9 @@ do_pick () {
 
 	if test "$(git rev-parse HEAD)" = "$squash_onto"
 	then
+		rewrite=y
+		rewrite_amend=y
+		git rev-parse --verify HEAD >"$amend"
 		# Set the correct commit message and author info on the
 		# sentinel root before cherry-picking the original changes
 		# without committing (-n).  Finally, update the sentinel again
@@ -538,22 +545,20 @@ do_pick () {
 		# rebase --continue.
 		git commit --allow-empty --allow-empty-message --amend \
 			   --no-post-rewrite -n -q -C $1 &&
-			pick_one -n $1 &&
-			git commit --allow-empty --amend \
-				   --no-post-rewrite -n -q \
-				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
+			pick_one -n $1 ||
 			die_with_patch $1 "Could not apply $1... $2"
 	else
-		pick_one $1 ||
+		pick_one ${rewrite:+-n} $1 ||
 			die_with_patch $1 "Could not apply $1... $2"
 	fi
 
-	if test -n "$edit"
+	if test -n "$rewrite"
 	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
-		}
+		git commit --allow-empty --no-post-rewrite -n -q \
+			   ${rewrite_amend:+--amend} \
+			   ${rewrite_edit:+--edit} \
+			   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
+			die_with_patch $1 "Could not rewrite commit after successfully picking $1... $2"
 	fi
 }
 
-- 
2.0.0

[RFC PATCH 5/7] rebase -i: Do not die in do_pick

[Fabian Ruch]

Since `do_pick` might be executed in a sub-shell (a modified author
environment for instance), calling `die` in `do_pick` has no effect but
exiting the sub-shell with a failure exit status. Moreover, if `do_pick`
is called while a squash or fixup is happening, `die_with_patch` will
discard `$squash_msg` as commit message. Indicate an error in `do_pick`
using return statements and properly kill the script at the call sites.
Remove unused commit message title argument from `do_pick`'s signature.

Signed-off-by: Fabian Ruch <bafain@gmail.com>
---
 git-rebase--interactive.sh | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f903599..e4992dc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -493,14 +493,10 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--edit] <commit> <title>
+# do_pick [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #
-# <title>
-#     The commit message title of <commit>. Used for information
-#     purposes only.
-#
 # <commit>
 #     The commit to cherry-pick.
 #
@@ -508,6 +504,12 @@ record_in_rewritten() {
 #     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.
+#
+# The return value is 1 if applying the changes resulted in a conflict
+# and 2 if the specified arguments were incorrect. If the changes could
+# be applied successfully but creating the commit failed, a value
+# greater than 2 is returned. No commit is created in either case and
+# the index is left with the (conflicting) changes in place.
 do_pick () {
 	rewrite=
 	rewrite_amend=
@@ -528,7 +530,11 @@ do_pick () {
 		esac
 		shift
 	done
-	test $# -ne 2 && die "do_pick: wrong number of arguments"
+	if test $# -ne 1
+	then
+		warn "do_pick: wrong number of arguments"
+		return 2
+	fi
 
 	if test "$(git rev-parse HEAD)" = "$squash_onto"
 	then
@@ -545,11 +551,9 @@ do_pick () {
 		# rebase --continue.
 		git commit --allow-empty --allow-empty-message --amend \
 			   --no-post-rewrite -n -q -C $1 &&
-			pick_one -n $1 ||
-			die_with_patch $1 "Could not apply $1... $2"
+			pick_one -n $1 || return 1
 	else
-		pick_one ${rewrite:+-n} $1 ||
-			die_with_patch $1 "Could not apply $1... $2"
+		pick_one ${rewrite:+-n} $1 || return 1
 	fi
 
 	if test -n "$rewrite"
@@ -557,8 +561,7 @@ do_pick () {
 		git commit --allow-empty --no-post-rewrite -n -q \
 			   ${rewrite_amend:+--amend} \
 			   ${rewrite_edit:+--edit} \
-			   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
-			die_with_patch $1 "Could not rewrite commit after successfully picking $1... $2"
+			   ${gpg_sign_opt:+"$gpg_sign_opt"} || return 3
 	fi
 }
 
@@ -573,21 +576,21 @@ do_next () {
 		comment_for_reflog pick
 
 		mark_action_done
-		do_pick $sha1 "$rest"
+		do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1... $rest"
 		record_in_rewritten $sha1
 		;;
 	reword|r)
 		comment_for_reflog reword
 
 		mark_action_done
-		do_pick --edit $sha1 "$rest"
+		do_pick --edit $sha1 || die_with_patch $sha1 "Could not apply $sha1... $rest"
 		record_in_rewritten $sha1
 		;;
 	edit|e)
 		comment_for_reflog edit
 
 		mark_action_done
-		do_pick $sha1 "$rest"
+		do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1... $rest"
 		warn "Stopped at $sha1... $rest"
 		exit_with_patch $sha1 0
 		;;
-- 
2.0.0

[RFC PATCH 6/7] rebase -i: Prepare for squash in terms of do_pick --amend

[Fabian Ruch]

Rewrite `squash` and `fixup` handling in `do_next` using the atomic
sequence

    pick_one
    commit

in order to test the correctness of a single `do_squash` or parametrized
`do_pick` and make the subsequent patch reimplementing `squash` in terms
of such a single function more readable.

Might be squashed into the subsequent commit.

Signed-off-by: Fabian Ruch <bafain@gmail.com>
---
 git-rebase--interactive.sh | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e4992dc..ada520d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -613,15 +613,15 @@ do_next () {
 		author_script_content=$(get_author_ident_from_commit HEAD)
 		echo "$author_script_content" > "$author_script"
 		eval "$author_script_content"
-		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:
+			if ! pick_one -n $sha1
+			then
+				git rev-parse --verify HEAD >"$amend"
+				die_failed_squash $sha1 "Could not apply $sha1... $rest"
+			fi
 			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
 				${gpg_sign_opt:+"$gpg_sign_opt"} ||
 				die_failed_squash $sha1 "$rest"
@@ -630,12 +630,22 @@ do_next () {
 			# This is the final command of this squash/fixup group
 			if test -f "$fixup_msg"
 			then
+				if ! pick_one -n $sha1
+				then
+					git rev-parse --verify HEAD >"$amend"
+					die_failed_squash $sha1 "Could not apply $sha1... $rest"
+				fi
 				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
 					${gpg_sign_opt:+"$gpg_sign_opt"} ||
 					die_failed_squash $sha1 "$rest"
 			else
 				cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
 				rm -f "$GIT_DIR"/MERGE_MSG
+				if ! pick_one -n $sha1
+				then
+					git rev-parse --verify HEAD >"$amend"
+					die_failed_squash $sha1 "Could not apply $sha1... $rest"
+				fi
 				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
 					${gpg_sign_opt:+"$gpg_sign_opt"} ||
 					die_failed_squash $sha1 "$rest"
-- 
2.0.0

[RFC PATCH 7/7] rebase -i: Teach do_pick the options --amend and --file

[Fabian Ruch]

The to-do list command `squash` and its close relative `fixup` replay
the changes of a commit like `pick` but do not recreate the commit.
Instead they replace the previous commit with a new commit that also
introduces the changes of the squashed commit. This is roughly like
cherry-picking without committing and using git-commit to amend the
previous commit.

The to-do list

    pick   a Some changes
    squash b Some more changes

gets translated into the sequence of git commands

    git cherry-pick a
    git cherry-pick -n b
    git commit --amend

and if `cherry-pick` supported `--amend` this would look even more like
the to-do list it is based on

    git cherry-pick a
    git cherry-pick --amend b.

Since `do_pick` takes care of `pick` entries and the above suggests
`squash` as an alias for `pick --amend`, teach `do_pick` to handle the
option `--amend` and reimplement `squash` in terms of `do_pick --amend`.
Also teach it the option `--file` which is used to specify `$squash_msg`
as commit message.

Both `--amend` and `--file` are commit rewriting options. If they are
encountered during options parsing, assign `rewrite` and pass `--amend`
(`--file` respectively) to the rewrite command. Be careful when
`--amend` is used to pick a root commit because HEAD might point to the
sentinel commit but there is still nothing to amend. Be sure to
initialize `$amend` so that commits are squashed even when `rebase` is
interrupted for resolving conflicts. It is not a mistake to do the
initialization regardless of any conflicts because `$amend` is always
cleared before the next to-do item is processed.

Signed-off-by: Fabian Ruch <bafain@gmail.com>
---

Notes:
    A question about when to enable the post-rewrite hook.
    
    `rebase` collects the hashes of all processed commits using
    `record_in_rewritten` and runs the post-rewrite script after the rebase
    is complete. Two points seem to confuse me.
    
     1) For a `pick` the hash is `record_in_rewritten` regardless of whether
        the hash changed or not (the commit was recreated or the head was
        fast-forwarded). Ok, the hook can figure that out. Is this behaviour
        intended?
    
     2) For a `reword` the amend disables the post-rewrite hook but for a
        `squash` (or `fixup`) the hook is executed each time the squash
        commit is amended. Does not this result in the hook being executed
        twice for each scheduled `squash` command? Once for the amend and
        once for the rebase. The hook most likely does not figure that out.
    
    This patch never executes the post-rewrite hook when processing the
    to-do list. The execution after the rebase is finished is still
    conducted. I am uncertain whether this is correct. The tests seem to
    succeed with both implementations.

 git-rebase--interactive.sh | 65 ++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 25 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ada520d..5ddc59d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -493,7 +493,7 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--edit] <commit>
+# do_pick [--file <file>] [--amend] [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #
@@ -505,6 +505,17 @@ record_in_rewritten() {
 #     commit message. The editor contents becomes the commit message of
 #     the new head.
 #
+# --amend
+#     After picking <commit>, replace the current head commit with a new
+#     commit that also introduces the changes of <commit>.
+#
+#     _This is not a git-cherry-pick option._
+#
+# -F <file>, --file <file>
+#     Take the commit message from the given file.
+#
+#     _This is not a git-cherry-pick option._
+#
 # The return value is 1 if applying the changes resulted in a conflict
 # and 2 if the specified arguments were incorrect. If the changes could
 # be applied successfully but creating the commit failed, a value
@@ -514,9 +525,30 @@ do_pick () {
 	rewrite=
 	rewrite_amend=
 	rewrite_edit=
+	rewrite_message=
 	while test $# -gt 0
 	do
 		case "$1" in
+		-F|--file)
+			if test $# -eq 0
+			then
+				warn "do_pick: option --file specified but no <file> given"
+				return 2
+			fi
+			rewrite=y
+			rewrite_message=$2
+			shift
+			;;
+		--amend)
+			if test "$(git rev-parse HEAD)" = "$squash_onto" || ! git rev-parse --verify HEAD
+			then
+				warn "do_pick: nothing to amend"
+				return 2
+			fi
+			rewrite=y
+			rewrite_amend=y
+			git rev-parse --verify HEAD >"$amend"
+			;;
 		-e|--edit)
 			rewrite=y
 			rewrite_edit=y
@@ -561,6 +593,7 @@ do_pick () {
 		git commit --allow-empty --no-post-rewrite -n -q \
 			   ${rewrite_amend:+--amend} \
 			   ${rewrite_edit:+--edit} \
+			   ${rewrite_message:+--file "$rewrite_message"} \
 			   ${gpg_sign_opt:+"$gpg_sign_opt"} || return 3
 	fi
 }
@@ -617,38 +650,20 @@ do_next () {
 		squash|s|fixup|f)
 			# This is an intermediate commit; its message will only be
 			# used in case of trouble.  So use the long version:
-			if ! pick_one -n $sha1
-			then
-				git rev-parse --verify HEAD >"$amend"
-				die_failed_squash $sha1 "Could not apply $sha1... $rest"
-			fi
-			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
-				${gpg_sign_opt:+"$gpg_sign_opt"} ||
-				die_failed_squash $sha1 "$rest"
+			do_with_author do_pick --amend -F "$squash_msg" $sha1 \
+				|| die_failed_squash $sha1 "Could not apply $sha1... $rest"
 			;;
 		*)
 			# This is the final command of this squash/fixup group
 			if test -f "$fixup_msg"
 			then
-				if ! pick_one -n $sha1
-				then
-					git rev-parse --verify HEAD >"$amend"
-					die_failed_squash $sha1 "Could not apply $sha1... $rest"
-				fi
-				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
-					${gpg_sign_opt:+"$gpg_sign_opt"} ||
-					die_failed_squash $sha1 "$rest"
+				do_with_author do_pick --amend -F "$fixup_msg" $sha1 \
+					|| die_failed_squash $sha1 "Could not apply $sha1... $rest"
 			else
 				cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
 				rm -f "$GIT_DIR"/MERGE_MSG
-				if ! pick_one -n $sha1
-				then
-					git rev-parse --verify HEAD >"$amend"
-					die_failed_squash $sha1 "Could not apply $sha1... $rest"
-				fi
-				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
-					${gpg_sign_opt:+"$gpg_sign_opt"} ||
-					die_failed_squash $sha1 "$rest"
+				do_with_author do_pick --amend -F "$GIT_DIR"/SQUASH_MSG -e $sha1 \
+					|| die_failed_squash $sha1 "Could not apply $sha1... $rest"
 			fi
 			rm -f "$squash_msg" "$fixup_msg"
 			;;
-- 
2.0.0

Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

[Michael Haggerty]

On 06/19/2014 05:28 AM, Fabian Ruch wrote:
> `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 <bafain@gmail.com>
> ---
>  git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f267d8b..ea5514e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -237,8 +237,26 @@ git_sequence_editor () {
>  
>  pick_one () {
>  	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.

> +		*)
> +			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
>  
>  	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.

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


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit

[Michael Haggerty]

On 06/19/2014 05:28 AM, Fabian Ruch wrote:
> 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.

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.

> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
>  git-rebase--interactive.sh    | 52 ++++++++++++++++++++++++++++++++++++-------
>  t/t3404-rebase-interactive.sh |  8 +++++++
>  2 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ea5514e..fffdfa5 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -490,7 +490,42 @@ record_in_rewritten() {
>  	esac
>  }
>  
> +# 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.

> +#
> +# -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)
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8197ed2..9931143 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -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 &&
>  	(
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>>  pick_one () {
>>  	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.

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

Re: [RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages

[Eric Sunshine]

On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch <bafain@gmail.com> wrote:
> 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 <bafain@gmail.com>
> ---
>  git-rebase--interactive.sh |  4 ++--
>  t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index fffdfa5..f09eeae 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -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
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 0b52105..3608db4 100755
> --- a/t/t3412-rebase-root.sh
> +++ b/t/t3412-rebase-root.sh
> @@ -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
> +       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 &&

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

> +'
> +
> +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 &&
> +       # 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.

> +'
> +
>  test_done
> --
> 2.0.0

Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

[Fabian Ruch]

Hi Michael,

On 06/20/2014 03:40 PM, Michael Haggerty wrote:
> On 06/19/2014 05:28 AM, Fabian Ruch wrote:
>> `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 <bafain@gmail.com>
>> ---
>>  git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 45 insertions(+), 16 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index f267d8b..ea5514e 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -237,8 +237,26 @@ git_sequence_editor () {
>>  
>>  pick_one () {
>>  	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.

Ok.

>> +		*)
>> +			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
>>  
>>  	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:

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

   Fabian

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

Re: [RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit

[Fabian Ruch]

Hi Michael,

On 06/20/2014 03:41 PM, Michael Haggerty wrote:
> On 06/19/2014 05:28 AM, Fabian Ruch wrote:
>> 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.
> 
> 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.

Hmph, I neglected that totally here. I'm sorry. If it's all right, I
will reply with the five separate commits (refactoring, error message,
--allow-empty, -n, -q) to this email. The whole patch series is still
RFC and the combination of the five will be exactly this one, so that
shouldn't confuse or burden anyone.

>> Signed-off-by: Fabian Ruch <bafain@gmail.com>
>> ---
>>  git-rebase--interactive.sh    | 52 ++++++++++++++++++++++++++++++++++++-------
>>  t/t3404-rebase-interactive.sh |  8 +++++++
>>  2 files changed, 52 insertions(+), 8 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index ea5514e..fffdfa5 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -490,7 +490,42 @@ record_in_rewritten() {
>>  	esac
>>  }
>>  
>> +# 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.

Ok.

The reason was to document the non-option arguments first and I ended up
documenting the arguments in reverse order to simply not abandon all
order. Having a look at several man-pages of git commands (cherry-pick,
commit, am, rebase), I wasn't able to extract a common pattern of order
of argument documentation.

   Fabian

>> +#
>> +# -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)
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 8197ed2..9931143 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -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 &&
>>  	(

Re: [RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages

[Fabian Ruch]

Hi Eric,

On 06/21/2014 02:33 AM, Eric Sunshine wrote:
> On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch <bafain@gmail.com> wrote:
>> 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 <bafain@gmail.com>
>> ---
>>  git-rebase--interactive.sh |  4 ++--
>>  t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index fffdfa5..f09eeae 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -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
>> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
>> index 0b52105..3608db4 100755
>> --- a/t/t3412-rebase-root.sh
>> +++ b/t/t3412-rebase-root.sh
>> @@ -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
>> +       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 &&

Ok.

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

Thanks a lot for the hint, I will place the test_when_finished directly
above the rebase -i command line because it is not relevant before, the
exact position shouldn't matter though. I didn't know that test_run_
skips the cleanup code if -i (for immediate mode) is passed to the test
driver and the test case fails.

   Fabian

>> +'
>> +
>> +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 &&
>> +       # 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.

Ok.

>> +'
>> +
>>  test_done

Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

[Fabian Ruch]

Hi Junio,

On 06/20/2014 09:53 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>  pick_one () {
>>>  	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.
> 
> 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.

Ok, until we require pick_one to accept options apart from -n, this
patch is postponed, for the presence of a single option is checked
easiest without the loop. It might be the case that rewriting replayed
commits in do_pick is the better alternative anyway and that it will
never be required to relay user-specified options beyond do_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.

I really like the idea of being explicit about how pick_one shall replay
the named commit and not using the cherry-pick option name for the
squash case. However, pick_one will never receive random user-specified
options. do_pick is the interface function which handles the pick
arguments. If any user-specified options are relayed to pick_one and
cherry-pick, they will be validated by do_pick first (using a loop like
above).

   Fabian

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

Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

[Johannes Schindelin]

Hi,

On Sun, 22 Jun 2014, Fabian Ruch wrote:

> On 06/20/2014 03:40 PM, Michael Haggerty wrote:
> > On 06/19/2014 05:28 AM, Fabian Ruch wrote:
> >> `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.
> >>
> >> [...]
> >> +		*)
> >> +			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
> >>  
> >>  	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.

I agree that this is a good change, but I would like to point out that it
would be a good change even if it pick_one_preserving_merged could return
with a non-zero exit code, even if it could be established easier that it
cannot return with a non-zero exit code: it is plainly confusing. (My
fault, of course.)

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

I do not resent you. Nor do I resent that the seven-year-old change is
coming to light again. But I have to admit that my recollection as to the
rationale for the way the code was written is very faint indeed.

Also, my commit messages were substantially inferior back then, so I guess
that the commit message of f09c9b8 does not quite help to jog my memory.

Looking at that revision of git-rebase--interactive.sh, however, I see
three call sites of pick_one, and I would suspect that I wanted to
maintain DRY code. Having said that, today I would replace

	@@ -68,6 +71,8 @@ die_abort () {
	 pick_one () {
		case "$1" in -n) sha1=$2 ;; *) sha1=$1 ;; esac
		git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
	+       test -d "$REWRITTEN" &&
	+               pick_one_preserving_merges "$@" && return
		parent_sha1=$(git rev-parse --verify $sha1^ 2>/dev/null)
		current_sha1=$(git rev-parse --verify HEAD)
		if [ $current_sha1 = $parent_sha1 ]; then

by

	@@ -68,6 +71,8 @@ die_abort () {
	 pick_one () {
		case "$1" in -n) sha1=$2 ;; *) sha1=$1 ;; esac
		git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
	+       if test -d "$REWRITTEN"
	+       then
	+               pick_one_preserving_merges "$@"
	+               return
	+       fi
		parent_sha1=$(git rev-parse --verify $sha1^ 2>/dev/null)
		current_sha1=$(git rev-parse --verify HEAD)
		if [ $current_sha1 = $parent_sha1 ]; then

instead.

Hope this clarifies things!
Dscho