rebase -i: Implement `reword` and `squash` in terms of `do_pick`

Hi,

This patch series is part of the undertaking to add command line options to to-do list commands. The goal is to be able to write something similar to

pick --reset-author --signoff a Some changes
pick b Some more changes
squash --no-edit c Ack Some more changes.

The first submission to the mailing list adds options parsing to do_pick and reimplements the current set of to-do list commands in terms[1] of a parametrized do_pick. It neither adds new commands to to-do lists nor exposes the command line options feature to the user. Still it makes the implementation of do_next slightly more straight-forward and the function do_pick behave more like an internal git-cherry-pick.

The patches try to accomplish one thing at a time but are not standalone in the sense that each of them constitutes a frame only in which a subsequent patch can express its one thing. For instance, only committing once and not dying in do_pick are things specific to implementing squash in terms of do_pick but presented as separate patches so that their correctness can be discussed independently.

The last patch contains some notes about when to run the post-rewrite hook, something I could not figure out myself.

Thanks for your time,
Fabian

[1] pick, reword, squash, fixup

Fabian Ruch (7):
rebase -i: Make option handling in pick_one more flexible rebase -i: Teach do_pick the option --edit rebase -i: Stop on root commits with empty log messages rebase -i: Commit only once when rewriting picks rebase -i: Do not die in do_pick
rebase -i: Prepare for squash in terms of do_pick --amend rebase -i: Teach do_pick the options --amend and --file

 git-rebase--interactive.sh    | 194 +++++++++++++++++++++++++++++++-----------
 t/t3404-rebase-interactive.sh |   8 ++
 t/t3412-rebase-root.sh        |  39 +++++++++
 3 files changed, 193 insertions(+), 48 deletions(-)

2.0.0

Hi,

this reroll applies the comments from Eric, Junio and Michael. In particular,

* It turned out that pick_one does not need option handling at all and the option-like argument -n determines whether pick_one or do_pick creates the replay commit. The latter happens if the task wants to rewrite the commit being picked (f.i., for the purpose of editing the log message or resetting the authorship).

do_pick still seems to require a flexible parsing of options, i.e. a relatively expensive loop, since it receives the whitelisted user-supplied options. Unsupported and unknown options are treated as an "unknown command" error.

* The do_pick options are documented in the same order they are listed in the function signature. Moreover, it is mentioned which options rewrite commits being picked.

* The test cases output differing actual values as changes to the expected values and not the other way around. Moreover, the failed rebases are not cleaned up until the respective test succeeds.

Two stages (and two sub-stages) can be identified in the rerolled patch series:

1. Route primary to-do list commands through do_pick

a. Implement reword in terms of do_pick (5/19) b. Implement squash in terms of do_pick (14/19)

2. Add user options to main commands

Enable options --signoff, --reset-author for pick, reword (19/19)

The last stage was added in this reroll. It enables the parsing of line options for to-do list commands, which is still restricted to options without arguments because it is unclear how spaces can be parsed as characters rather than separators where needed. For instance, if we were to support

pick --author="A U Thor" fa1afe1 Some changes

then read(1) would hand us the tokens --author="A, U and Thor" instead of --author=A U Thor, which we would want to relay to do_pick. Interpreting the shell quoting would help. However, eval(1) seems to disqualify itself as an interpreter because the to-do list entry could potentially contain any shell command line. This could be both a security and a usability issue. For this reason, the patch series still hasn't graduated from being RFC.

Fabian

Fabian Ruch (19):
rebase -i: Failed reword prints redundant error message rebase -i: reword complains about empty commit despite --keep-empty rebase -i: reword executes pre-commit hook on interim commit rebase -i: Teach do_pick the option --edit
rebase -i: Implement reword in terms of do_pick
rebase -i: Stop on root commits with empty log messages rebase -i: The replay of root commits is not shown with --verbose rebase -i: Root commits are replayed with an unnecessary option rebase -i: Commit only once when rewriting picks
rebase -i: Do not die in do_pick
rebase -i: Teach do_pick the option --amend
rebase -i: Teach do_pick the option --file
rebase -i: Prepare for squash in terms of do_pick --amend rebase -i: Implement squash in terms of do_pick
rebase -i: Explicitly distinguish replay commands and exec tasks rebase -i: Parse to-do list command line options
rebase -i: Teach do_pick the option --reset-author
rebase -i: Teach do_pick the option --signoff
rebase -i: Enable options --signoff, --reset-author for pick, reword

 git-rebase--interactive.sh    | 277 ++++++++++++++++++++++++++++++++++--------
 t/t3404-rebase-interactive.sh |   8 ++
 t/t3412-rebase-root.sh        |  39 ++++++
 3 files changed, 273 insertions(+), 51 deletions(-)

2.0.0

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. If the edited log message is empty or is found ill-formatted by one of the commit hooks, git-rebase--interactive prints three error messages to the console.

1. The git-commit output, which contains all the output from hook scripts.
2. A rebase diagnosis saying at which task on the to-do list it got stuck.
3. Generic presumptions about what could have triggered the error.

The third message contains redundant information and does not add any enlightenment either, which makes the output unnecessarily longish and different from the other command's output. For instance, this is what the output looks like if the log message is empty (contains duplicate Signed-off-by lines).

(1.) Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.) (2.) Could not amend commit after successfully picking fa1afe1... Some change (3.) 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.

Discard the third message.

It is true that a failed hook script might not output any diagnosis but then the generic message is not of much help either. Since this lack of information affects the built-in git commands for commit, merge and cherry-pick first, the solution would be to keep track of the failed hooks in their output so that the user knows which of her hooks require improvement.

---
 git-rebase--interactive.sh | 3 ---
 1 file changed, 3 deletions(-)

index 7e1eda0..e733d7f 100644
@@ -506,9 +506,6 @@ do_next () {
 		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
 		}
 		record_in_rewritten $sha1

2.0.0

I think the message originally came from 0becb3e4 (rebase -i:
interrupt rebase when "commit --amend" failed during "reword", 2011-11-30); it seems that the primary point of the change was to make sure it exits and the warning message may not have been well thought out, but before discarding the result of somebody else's work, it may not be a bad idea to ask just in case you may have overlooked something (Andrew Wong Cc'ed).

Since "git commit" already prints out error messages for failing due to empty commit message, the third message is really about giving hints in the case where pre-commit fails. We could probably assume that pre-commit would also print out error messages. So I'd be fine with removing the third message. But I wonder if we need to explain that the user needs to run "git rebase --continue" to resume the rebase?

Hi Andrew,

thanks for your review and sorry that I forgot to cc the bug fix to you.

That is still taken care of by exit_with_patch here. When called in the error case, it prints

You can amend the commit now, with

git commit --amend

Once you are satisfied with your changes, run

git rebase --continue

to standard error. I might have overlooked this in one of the later patches though.

Fabian

Oh, that's right. Ok, go ahead and remove the third warning then. Thanks!

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. If --keep-empty is passed as option to git-rebase--interactive, empty commits ought to be replayed without complaints. However, if the users chooses to reword an empty commit by changing the respective to-do list entry from

pick fa1afe1 Empty commit

to

reword fa1afe1 Empty commit

, then git-rebase--interactive suddenly fails to replay the empty commit. This is especially counterintuitive because reword is thought of as a pick that alters the log message in some way but nothing more and the unchanged to-do list entry would not fail.

Handle reword by cherry-picking the named commit and editing the log message using

    git commit --allow-empty --amend

instead of

    git commit --amend.

Add test.

---
 git-rebase--interactive.sh    | 2 +-
 t/t3404-rebase-interactive.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

index e733d7f..689400e 100644
@@ -504,7 +504,7 @@ do_next () {
 
 		mark_action_done
 		do_pick $sha1 "$rest"
-		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
+		git commit --allow-empty --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
 			warn "Could not amend commit after successfully picking $sha1... $rest"
 			exit_with_patch $sha1 1
 		}

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

2.0.0

I had to read the title and then the log twice before understanding that the above is not "change the complaint message" (i.e. "reword complaints" spelled incorrectly) but is a description of the current behaviour (i.e. "the command complains when 'reword' is used on an empty commit") that is not accompanied by an evaluation ("ok, it complains; are you saying it is a good thing or a bad thing?") or an action ("if it is a bad thing, what are you doing about it?").

Perhaps

rebase -i: allow rewording an empty commit

or something?

I thought "...despite --keep-empty" would already imply that "reword complains about empty commit" is not supposed to happen, the action would have been obvious. However, I understand that --keep-empty is first of all concerned with which commits of $upstream...$orig_head end up on the initial to-do list and the git-rebase manual page doesn't mention that it picks commits using the --allow-empty option. It is simply a necessity of a script not to complain about something it put on the to-do list itself.

The subject reads now

rebase -i: allow rewording an empty commit without complaints

trying to convey that this is not a new feature but a bug fix.

Fabian

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. This happens in two steps. Firstly, the named commit is cherry-picked. Secondly, the commit created in the first step is amended using an unchanged index to edit the log message. The pre-commit hook is meant to verify the changes introduced by a commit (for instance, catching inserted trailing white space). Since the committed tree is not changed between the two steps, do not execute the pre-commit hook in the second step.

Specify the git-commit option --no-verify to disable the pre-commit hook when editing the log message. Because --no-verify also skips the commit-msg hook, execute the hook from within git-rebase--interactive after the commit is created. Fortunately, the commit message is still available in $GIT_DIR/COMMIT_EDITMSG after git-commit terminates. Caveat: In case the commit-msg hook finds the new log message ill-formatted, the user is only notified of the failed commit-msg hook but the log message is used for the commit anyway. git-commit ought to offer more fine-grained control over which hooks are executed.

---
 git-rebase--interactive.sh | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

index 689400e..b50770d 100644
@@ -504,10 +504,19 @@ do_next () {
 
 		mark_action_done
 		do_pick $sha1 "$rest"
-		git commit --allow-empty --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
-			warn "Could not amend commit after successfully picking $sha1... $rest"
-			exit_with_patch $sha1 1
-		}
+		# TODO: Work around the fact that git-commit lets us
+		# disable either both the pre-commit and the commit-msg
+		# hook or none. Disable the pre-commit hook because the
+		# tree is left unchanged but run the commit-msg hook
+		# from here because the log message is altered.
+		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
+			if test -x "$GIT_DIR"/hooks/commit-msg
+			then
+				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
+			fi || {
+				warn "Could not amend commit after successfully picking $sha1... $rest"
+				exit_with_patch $sha1 1
+			}
 		record_in_rewritten $sha1
 		;;
 	edit|e)

2.0.0

It is not like the second step is built as a child commit of the result from the first step, recording the same tree without any change. Why is it a good thing not to run the pre-commit hook (or other hooks for that matter)? After all, the result of the second step is what is recorded in the final history; it just feels wrong not to test that one, even if it were a good idea to test only once.

Hi Junio,

if I understand correctly, the tree of the (amended) commit isn't tested at all by git-rebase--interactive because git-cherry-pick, and therefore pick_one, commits with both the pre-commit and commit-msg hook disabled (see the "git commit -n" command line being built in sequencer.c#run_git_commit since revision 9fa4db5).

The "commit --amend" we are concerned with here amends using an untouched tree so that the history ought to record exactly the same trees as prior to "commit --amend". If git-cherry-pick was testing the tree, then it would be unnecessary to test the tree a second time. Since git-cherry-pick is not testing as of yet, testing and possibly rejecting a tree in the second step would actually be wrong. I totally neglected to look for a test case when I submitted this patch, so I'd like to supply one late now:

(This addition to t3404-rebase--interactive.sh is based on the test case "rebase a commit violating pre-commit"; "test_commit --no-verify" will be implemented the obvious way.)

In both cases, it's correct to disable the pre-commit hook when editing the log message and it just makes sense to test changes where you make them. Unfortunately, it is not obvious that "git commit --amend" merely edits the log message rather than committing a new tree.

For what it's worth, if we were to create an empty child commit and squash it into the parent instead of amending immediately, then git-rebase--interactive would disable tree verification in the second step as well. This behaviour was introduced by commit c5b09fe. Although the change seems to have been triggered by something completely different back then, the correctness criterion remains the same, i.e. recording previously committed trees.

Best regards,
Fabian

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is used to implement the to-do list command pick. To cater for the different pick behaviours (like reword), do_pick accepts several options not only from the git-cherry-pick but also the git-commit interface. Add the common option --edit to let the user edit the log message of the named commit.

Loop over $@ to parse the do_pick arguments. Assign the local variable edit if one of the options is --edit so that the remainder of do_pick can easily check whether the client code asked to edit the commit message. If one of the options is unknown, mention it on the console and die. Break the loop on the first non-option and do some sanity checking to ensure that there exactly two non-options, which are interpreted by the remainder as <commit> and <title> like before.

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.

---
 git-rebase--interactive.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

index b50770d..e06d9b6 100644
@@ -461,7 +461,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.
+#
+# -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. This creates a fresh commit.
+#
+# <commit>
+#     The commit to cherry-pick.
+#
+# <title>
+#     The commit message title of <commit>. Used for information
+#     purposes only.
 do_pick () {
+	edit=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-e|--edit)
+			edit=y
+			;;
+		-*)
+			die "do_pick: unrecognized 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

@@ -483,6 +518,23 @@ do_pick () {
 		pick_one $1 ||
 			die_with_patch $1 "Could not apply $1... $2"
 	fi
+
+	if test -n "$edit"
+	then
+		# TODO: Work around the fact that git-commit lets us
+		# disable either both the pre-commit and the commit-msg
+		# hook or none. Disable the pre-commit hook because the
+		# tree is left unchanged but run the commit-msg hook
+		# from here because the log message is altered.
+		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
+			if test -x "$GIT_DIR"/hooks/commit-msg
+			then
+				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
+			fi || {
+				warn "Could not amend commit after successfully picking $1... $2"
+				exit_with_patch $1 1
+			}
+	fi
 }
 
 do_next () {

2.0.0

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. Reimplement reword in terms of do_pick --edit.

If the user picks a commit using the to-do list line

reword fa1afe1 Some change

execute the command do_pick --edit fa1afe1 "Some change" which carries out exactly the same steps as the case arm for reword in do_next so far.

---
 git-rebase--interactive.sh | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

index e06d9b6..4c875d5 100644
@@ -555,20 +555,7 @@ do_next () {
 		comment_for_reflog reword
 
 		mark_action_done
-		do_pick $sha1 "$rest"
-		# TODO: Work around the fact that git-commit lets us
-		# disable either both the pre-commit and the commit-msg
-		# hook or none. Disable the pre-commit hook because the
-		# tree is left unchanged but run the commit-msg hook
-		# from here because the log message is altered.
-		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
-			if test -x "$GIT_DIR"/hooks/commit-msg
-			then
-				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
-			fi || {
-				warn "Could not amend commit after successfully picking $sha1... $rest"
-				exit_with_patch $sha1 1
-			}
+		do_pick --edit $sha1 "$rest"
 		record_in_rewritten $sha1
 		;;
 	edit|e)

2.0.0

Hi Matthieu,

thanks for taking a look at this patch series. I might have caused some confusion by starting with version v1 again after removing the RFC tag. The current reroll[1] is tagged with PATCH v1, not PATCH RFC v2.

I'm sorry if this is the reason why your reply appears on this sub-thread.

Your concerns below are of course noted.

Fabian

[1] http://article.gmane.org/gmane.comp.version-control.git/254361

It is not required that a patch series solves all of the problems in the universe. If these patches implements some useful features robustly, and if there is no reason to expect that future enhancements will require the user interface to be changed in a backwards-compatible way, then there is no reason that this patch series has to be held as an RFC hostage to some hypothetical future.

Michael

Michael Haggerty
[email protected]

The command line used to recreate root commits specifies the erroneous option --allow-empty-message. If the root commit has an empty log message, the replay of this commit should fail and the rebase should be interrupted like for any other commit that is on the to-do list and has an empty commit message. Remove the option.

The option might have been introduced by copy-and-paste of the first part of the command line which initializes the authorship of the sentinel commit. Indeed, the sentinel commit has an empty log message and this should not trigger a failure, which is why the option --allow-empty-message is correctly specified here.

Add test.

---
 git-rebase--interactive.sh |  2 +-
 t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

index 4c875d5..0af96f2 100644
@@ -510,7 +510,7 @@ 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 \
+			git commit --allow-empty \
 				   --amend --no-post-rewrite -n -q -C $1 \
 				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
 			die_with_patch $1 "Could not apply $1... $2"

index 0b52105..9867705 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
+	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_when_finished git rebase --abort &&
+	test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+	echo root-commit >file.expected &&
+	test_cmp file.expected file
+'
+
+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_when_finished git rebase --abort &&
+	test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+	echo child-commit >file.expected &&
+	test_cmp file.expected file
+'
+
 test_done

2.0.0

The first "commit --amend" uses -C "$1" to give the amended result not just the authorship but also the log message taken from "$1". If we are allowing a commit without any message to be used as "$1", I think --allow-empty-message needs to be there. If "$1" requires the option here, why doesn't the second one, that records the updated tree with the metainformation taken from the same commit "$1" can successfully commit without the option?

Puzzled...

Hi Junio,

(I realize now that the emptiness of the sentinel log message is irrelevant to the success of the first "commit --amend" since we are amending using -C. I'll rewrite the second paragraph of the patch description.)

The first "commit --amend" requires --allow-empty-message because we do not want to fail without the authorship or log message of $1 being in place. It's not a matter of allowing or disallowing empty log messages yet.

git-rebase--interactive can come across an empty log message in three different ways, which are, depicted as to-do list tasks, the following.

1) pick --ff $1
2) pick --no-ff $1
3) reword $1

This patch is concerned with consistency in the second case. git-rebase--root does not handle the first case yet and the third case is handled somewhere else in the script independent of the first two.

The --root option handling was added to the script as a special case later in the revision history. It's that option handling which introduced the inconsistency that non-fast-forwards of commits with empty log messages succeed if they are root commits but have always failed otherwise.

Your reply suggests that git-rebase--interactive was wrong from the beginning and that the replay of commits without any message should be allowed. This would reconcile the first case with the second. In fact, since neither of them alters the changes introduced by $1 or its log message, it might be incorrect to complain about a missing message in the first place.

Do you want me to replace this patch with a patch

rebase -i: Always allow picking of commits with empty log messages

that makes git-rebase--interactive cherry-pick commits using --allow-empty-message? The script would still abort an empty reword with the new patch and the user could then still force the empty log message with "git commit --amend --allow-empty-message".

Fabian

Now you explained your line of thought more clearly and I have thought about it a bit more, I think I agree with the conclusion of the above.

An alternative may be to teach a new option --allow-empty-message to rebase to allow people to replay/reproduce an existing history with commits without any message, and uniformly tighten to refuse replaying a commit without message by default. But I am not sure if that is a good direction to go. Doesn't "rebase" without "-i" by default replay a commit without message faithfully? It might be debatable to allow a commit that we would normally would flag as suspicious (i.e. no change relative to its parent, or no log message) when replaying with "reword" or "edit", but when replaying with "pick", "rebase -i" should behave just like "rebase" without interactive.

Thanks.

I do not "want" any particular behaviour change. I wanted you to clarify what changed behaviour you are trying to implement and why, and that was why I was asking these questions.

The command line used to recreate root commits specifies the erroneous option -q which suppresses the commit summary message. However, git-rebase--interactive tends to tell the user about the commits it creates, if she wishes (cf. command line option --verbose). The code parts handling non-root commits or squash commits all output commit summary messages. Do not make the replay of root commits an exception. Remove the option.

It is OK to suppress the commit summary when git-commit is used to initialize the authorship of the sentinel commit because the existence of this additional commit is a detail of git-rebase--interactive's implementation. The option -q was probably introduced as a copy-and-paste error stemming from that part of the root commit handling code.

---
 git-rebase--interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0af96f2..ff04d5d 100644
@@ -511,7 +511,7 @@ do_pick () {
 			   --no-post-rewrite -n -q -C $1 &&
 			pick_one -n $1 &&
 			git commit --allow-empty \
-				   --amend --no-post-rewrite -n -q -C $1 \
+				   --amend --no-post-rewrite -n -C $1 \
 				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
 			die_with_patch $1 "Could not apply $1... $2"
 	else

2.0.0

This one I can buy; there is no reason to drop "-q" from both (which would give us the same thing twice and the one before the "pick_one -n" runs is not the final one anyway) and the later one that records the updated tree would be the one to report what it did.

Hi Chris,

you're the original author of the code touched by this patch. Is the second -q option really a simple copy-and-paste of the first or am I overlooking something here? I'd like to confirm this as, in retrospect, I feel a bit uncertain about the hasty claim in the log message.

Kind regards,
Fabian

It was a while ago and I don't remember the details of this patch I'm afraid, but your rationale here looks sensible to me. I tend to find git too noisy by default rather than too quiet, so it wouldn't surprise me if I never thought to try the --verbose version.

Cheers,

Chris.

The command line used to recreate root commits specifies the effectless option -C. It is used to reuse commit message and authorship from the named commit but the commit being amended here, which is the sentinel commit, already carries the authorship and log message of the processed commit. Note that the committer email and commit date fields do not match the root commit either way. Remove the option. However, -C (other than -c) does not invoke the editor and the --amend option invokes it by default. Disable editor invocation again by specifying --no-edit.

---
 git-rebase--interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ff04d5d..17836d5 100644
@@ -511,7 +511,7 @@ do_pick () {
 			   --no-post-rewrite -n -q -C $1 &&
 			pick_one -n $1 &&
 			git commit --allow-empty \
-				   --amend --no-post-rewrite -n -C $1 \
+				   --amend --no-post-rewrite -n --no-edit \
 				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
 			die_with_patch $1 "Could not apply $1... $2"
 	else

2.0.0

I'd say this is a no-value change, in the sense that it can be written either way for the same effect and if the original were written with "--amend --no-edit" then there would be no reason to change it to "-C $1" because such a change would also be equally a no-value change.

What exactly are we gaining? Performance? Correctness?

Hi Junio,

I submitted this change separately from the next ("rebase -i: Commit only once when rewriting picks") for the following reasons, at least I thought they were.

It makes the next patch easier to understand because the finalising command line "git commit --allow-empty --amend --no-post-rewrite -n --no-edit" seems to be simply moved to the end of do_pick. Substituting --no-edit for -C there would make that log message overly long and the paragraph saying why this substitution is correct would be distracting (it's a little unfortunate that there is this special case in the first place). While the resulting history stays the same of course, it might be confusing to someone reading the code that the log message gets replaced with itself.

I know the last point is rather weak but aren't the two patches and log messages easier to read?

Fabian

And the reason why the same "-C $1" is not used and "--no-edit" is used in the final version that is moved to the end of do_pick is...?

Hi Junio,

The finalising command line is executed by do_pick to rewrite $1 when it is being replayed.

For instance, one purpose of rewriting is amending the current head with the changes introduced by $1. In this case, we don't want the log message of $1 to be the final log message. Another purpose of rewriting is using a text file or a string as log message, which would be defeated by "-C $1" as well.

It's true that, by the time the next patch is introduced, do_pick doesn't yet implement either rewriting mechanism (they are both used to thread "squash" through do_pick later on) but these considerations suggest that "-C $1" might not be what we want to use to disable the log message editor.

Fabian

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.

git-commit lets the user edit the commit log message by default. We do not want that for the rewriting git-commit command line because the default behaviour of git-cherry-pick is exactly the opposite. Pass --no-edit when rewriting a picked commit. An explicit --edit passed to do_pick (for instance, when reword is executed) enables the editor launch again.

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.

---
 git-rebase--interactive.sh | 55 +++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 22 deletions(-)

index 17836d5..46b2db1 100644
@@ -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

@@ -479,12 +480,15 @@ record_in_rewritten() {
 #     The commit message title of <commit>. Used for information
 #     purposes only.
 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
 			;;
 		-*)
 			die "do_pick: unrecognized option -- $1"

@@ -499,6 +503,10 @@ 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

@@ -509,31 +517,34 @@ 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 --no-edit \
-				   ${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
-		# TODO: Work around the fact that git-commit lets us
-		# disable either both the pre-commit and the commit-msg
-		# hook or none. Disable the pre-commit hook because the
-		# tree is left unchanged but run the commit-msg hook
-		# from here because the log message is altered.
-		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
-			if test -x "$GIT_DIR"/hooks/commit-msg
-			then
-				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
-			fi || {
-				warn "Could not amend commit after successfully picking $1... $2"
-				exit_with_patch $1 1
-			}
+		git commit --allow-empty --no-post-rewrite -n --no-edit \
+			   ${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
+
+	# TODO: Work around the fact that git-commit lets us
+	# disable either both the pre-commit and the commit-msg
+	# hook or none. Disable the pre-commit hook because the
+	# tree is left unchanged but run the commit-msg hook
+	# from here because the log message is altered.
+	if test -n "$rewrite_edit"
+	then
+		if test -x "$GIT_DIR"/hooks/commit-msg
+		then
+			"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
+		fi ||
+			die_with_patch $1 "Could not rewrite commit after successfully picking $1... $2"
 	fi
 }
 

2.0.0

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. The git-rebase--interactive script is not terminated. Moreover, if do_pick is called while a squash or fixup is in effect, die_with_patch will discard $squash_msg as commit message. Lastly, after a die in do_pick do_next has no chance to reschedule tasks that failed before changes could be applied.

Indicate an error in do_pick using return statements and properly kill the script at the call sites. Although possible in principle, the issued error messages are no more indicating whether do_pick failed while applying or while committing the changes. This reduces code complexity at the call site and does not matter from a user's point of view because a glance at the index reveals whether there are conflicts or not and in-depth troubleshooting is still possible using the --verbose option.

Remove the commit message title argument from do_pick's interface, which has become unused.

---
 git-rebase--interactive.sh | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

index 46b2db1..0070b3e 100644
@@ -464,7 +464,7 @@ 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.
 #

@@ -476,9 +476,11 @@ record_in_rewritten() {
 # <commit>
 #     The commit to cherry-pick.
 #
-# <title>
-#     The commit message title of <commit>. Used for information
-#     purposes only.
+# 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=

@@ -491,7 +493,8 @@ do_pick () {
 			rewrite_edit=y
 			;;
 		-*)
-			die "do_pick: unrecognized option -- $1"
+			warn "do_pick: unrecognized option -- $1"
+			return 2
 			;;
 		*)
 			break

@@ -499,7 +502,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

@@ -517,11 +524,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"

@@ -529,8 +534,7 @@ do_pick () {
 		git commit --allow-empty --no-post-rewrite -n --no-edit \
 			   ${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
 
 	# TODO: Work around the fact that git-commit lets us

@@ -543,8 +547,7 @@ do_pick () {
 		if test -x "$GIT_DIR"/hooks/commit-msg
 		then
 			"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
-		fi ||
-			die_with_patch $1 "Could not rewrite commit after successfully picking $1... $2"
+		fi || return 3
 	fi
 }
 

@@ -559,21 +562,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

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is used to implement the to-do list commands pick, reword and edit. To cater for the different pick behaviours (like squash), do_pick accepts several options not only from the git-cherry-pick but also the git-commit interface.

Add the option --amend from the git-commit interface to the options pool of do_pick. It creates a new commit for the changes introduced by the picked commit and the previous one. The previous commit is then replaced with the new commit. If no other options are specified, the log message of the previous commit is used.

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 git-rebase--interactive 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.

---
 git-rebase--interactive.sh | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

index 0070b3e..046d358 100644
@@ -464,10 +464,16 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--edit] <commit>
+# do_pick [--amend] [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #
+# --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._
+#
 # -e, --edit
 #     After picking <commit>, open an editor and let the user edit the
 #     commit message. The editor contents becomes the commit message of

@@ -488,6 +494,16 @@ do_pick () {
 	while test $# -gt 0
 	do
 		case "$1" in
+		--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

2.0.0

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is used to implement the to-do list command pick, reword and edit. To cater for the different pick behaviours (like squash), do_pick accepts several options not only from the git-cherry-pick but also the git-commit interface.

Add the option --file from the git-commit interface to the options pool of do_pick. It expects an argument itself which is interpreted as a file path and takes the commit message from the given file. If --file is passed to do_pick, assign the given file path to the local variable rewrite_message and relay the option

--file "$rewrite_message"

to the git-commit command line which creates the commit.

---
 git-rebase--interactive.sh | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

index 046d358..47e3edf 100644
@@ -464,7 +464,7 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--amend] [--edit] <commit>
+# do_pick [--amend] [--file <file>] [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #

@@ -474,6 +474,12 @@ record_in_rewritten() {
 #
 #     _This is not a git-cherry-pick option._
 #
+# -F <file>, --file <file>
+#     Take the commit message from the given file. This creates a fresh
+#     commit.
+#
+#     _This is not a git-cherry-pick option._
+#
 # -e, --edit
 #     After picking <commit>, open an editor and let the user edit the
 #     commit message. The editor contents becomes the commit message of

@@ -491,6 +497,7 @@ do_pick () {
 	rewrite=
 	rewrite_amend=
 	rewrite_edit=
+	rewrite_message=
 	while test $# -gt 0
 	do
 		case "$1" in

@@ -504,6 +511,16 @@ do_pick () {
 			rewrite_amend=y
 			git rev-parse --verify HEAD >"$amend"
 			;;
+		-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
+			;;
 		-e|--edit)
 			rewrite=y
 			rewrite_edit=y

@@ -550,6 +567,7 @@ do_pick () {
 		git commit --allow-empty --no-post-rewrite -n --no-edit \
 			   ${rewrite_amend:+--amend} \
 			   ${rewrite_edit:+--edit} \
+			   ${rewrite_message:+--file "$rewrite_message"} \
 			   ${gpg_sign_opt:+"$gpg_sign_opt"} || return 3
 	fi
 

2.0.0

Rewrite squash and fixup handling in do_next using the sequence

pick_one
commit

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

Do not call rm -f "$GIT_DIR"/MERGE_MSG since it has no effect on the state after git-rebase--interactive terminates. The option -F makes git-commit ignore MERGE_MSG for the log message. If git-commit succeeds, MERGE_MSG is removed, and if it fails, MERGE_MSG is overwritten by the error sequence die_failed_squash. In summary, removing MERGE_MSG neither influences the squash commit message nor the file state after git-commit returns.

Moreover, pick_one ignores $GIT_DIR/SQUASH_MSG and does not touch $squash_msg so that it is correct to execute pick_one immediately before git-commit.

Might be squashed into the subsequent commit.

---
 git-rebase--interactive.sh | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

index 47e3edf..37800be 100644
@@ -633,15 +633,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"

@@ -650,12 +650,21 @@ 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

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 git-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, reimplement squash in terms of do_pick --amend. Introduce $squash_msg as the commit message via the --file option. When the last commit of a squash series is processed, the user is asked to review the log message. Pass --edit as additional option in this case. The only difference in the options passed to git-commit and do_pick is the omitted --no-verify. However, do_pick does not execute the verification hooks anyway because it solely replays commits and assumes that they have been verified before.

---
 git-rebase--interactive.sh | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

index 37800be..008f3a0 100644
@@ -637,37 +637,19 @@ 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
-				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

There are two kinds of to-do list commands available. One kind replays a commit (pick, reword, edit, squash and fixup that is) and the other executes a shell command (exec). We will call the first kind replay commands.

The two kinds of tasks are scheduled using different line formats. Replay commands expect a commit hash argument following the command name and exec concatenates all arguments to assemble a command line.

Adhere to the distinction of formats by not trying to parse the sha1 field unless we are dealing with a replay command. Move the replay command handling code to a new function do_replay which assumes the first argument to be a commit hash and make no more such assumptions in do_next.

---
 git-rebase--interactive.sh | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

index 008f3a0..9de7441 100644
@@ -585,13 +585,12 @@ do_pick () {
 	fi
 }
 
-do_next () {
-	rm -f "$msg" "$author_script" "$amend" || exit
-	read -r command sha1 rest < "$todo"
+do_replay () {
+	command=$1
+	sha1=$2
+	rest=$3
+
 	case "$command" in
-	"$comment_char"*|''|noop)
-		mark_action_done
-		;;
 	pick|p)
 		comment_for_reflog pick
 

@@ -656,6 +655,28 @@ do_next () {
 		esac
 		record_in_rewritten $sha1
 		;;
+	*)
+		read -r command <"$todo"
+		warn "Unknown command: $command"
+		fixtodo="Please fix this using 'git rebase --edit-todo'."
+		if git rev-parse --verify -q "$sha1" >/dev/null
+		then
+			die_with_patch $sha1 "$fixtodo"
+		else
+			die "$fixtodo"
+		fi
+		;;
+	esac
+}
+
+do_next () {
+	rm -f "$msg" "$author_script" "$amend" || exit
+	read -r command sha1 rest <"$todo"
+
+	case "$command" in
+	"$comment_char"*|''|noop)
+		mark_action_done
+		;;
 	x|"exec")
 		read -r command rest < "$todo"
 		mark_action_done

@@ -695,14 +716,7 @@ do_next () {
 		fi
 		;;
 	*)
-		warn "Unknown command: $command $sha1 $rest"
-		fixtodo="Please fix this using 'git rebase --edit-todo'."
-		if git rev-parse --verify -q "$sha1" >/dev/null
-		then
-			die_with_patch $sha1 "$fixtodo"
-		else
-			die "$fixtodo"
-		fi
+		do_replay $command $sha1 "$rest"
 		;;
 	esac
 	test -s "$todo" && return

2.0.0

In do_next(), we used read the first line of the insn sheet into three variables, assuming the common case of handling one of the replay commands, and (somewhat wastefully) re-read the same line into two variables when we realize that the command was "exec".

After you split do_replay() out of do_next() with this patch, we seem to do exactly the same thing.

What exactly is the problem this change wants to fix?

Puzzled...

Read in to-do list lines as

command args

instead of

command sha1 rest

so that to-do list command lines can specify additional arguments apart from the commit hash and the log message title, which become the non-options in args. Loop over args, put all options (an argument beginning with a dash) in opts, stop the loop on the first non-option and assign it to sha1. The loop does not know the options it parses so that options that take an argument themselves are not supported at the moment. Neither are options that contain spaces because the shell expansion of args in do_next interprets white space characters as argument separator, that is a command line like

pick --author "A U Thor" fa1afe1 Some change

is parsed as the pick command

    pick --author

and the commit hash

    "A

which obviously results in an unknown revision error. For the sake of completeness, in the example above the message title variable rest is assigned the string 'U Thor" fa1afe1 Some change' (without the single quotes).

Print an error message for unknown or unsupported command line options, which means an error for all specified options at the moment. Cleanly break the do_next loop by assigning the special value 'unknown' to the local variable command, which triggers the unknown command case in do_cmd.

The to-do list is also parsed when the commit hashes are translated between long and short format before and after the to-do list is edited. Apply the same procedure as in do_cmd with the exception that we only care about where the options stop and the commit hash begins. Do not reject any options when transforming the commit hashes.

---
 git-rebase--interactive.sh | 49 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 8 deletions(-)

index 9de7441..2119d00 100644
@@ -587,8 +587,26 @@ do_pick () {
 
 do_replay () {
 	command=$1
-	sha1=$2
-	rest=$3
+	shift
+
+	opts=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-*)
+			warn "Unknown option: $1"
+			command=unknown
+			;;
+		*)
+			break
+			;;
+		esac
+		opts="$opts $(git rev-parse --sq-quote "$1")"
+		shift
+	done
+	sha1=$1
+	shift
+	rest=$*
 
 	case "$command" in
 	pick|p)

@@ -671,7 +689,7 @@ do_replay () {
 
 do_next () {
 	rm -f "$msg" "$author_script" "$amend" || exit
-	read -r command sha1 rest <"$todo"
+	read -r command args <"$todo"
 
 	case "$command" in
 	"$comment_char"*|''|noop)

@@ -716,7 +734,7 @@ do_next () {
 		fi
 		;;
 	*)
-		do_replay $command $sha1 "$rest"
+		do_replay $command $args
 		;;
 	esac
 	test -s "$todo" && return

@@ -796,19 +814,34 @@ skip_unnecessary_picks () {
 }
 
 transform_todo_ids () {
-	while read -r command rest
+	while read -r command args
 	do
 		case "$command" in
 		"$comment_char"* | exec)
 			# Be careful for oddball commands like 'exec'
 			# that do not have a SHA-1 at the beginning of $rest.
+			newargs=\ $args
 			;;
 		*)
-			sha1=$(git rev-parse --verify --quiet "$@" ${rest%% *}) &&
-			rest="$sha1 ${rest#* }"
+			newargs=
+			sha1=
+			for arg in $args
+			do
+				case "$arg" in
+				-*)
+					newargs="$newargs $arg"
+					;;
+				*)
+					test -z "$sha1" &&
+						sha1=$(git rev-parse --verify --quiet "$@" $arg) &&
+						arg=$sha1
+					newargs="$newargs $arg"
+					;;
+				esac
+			done
 			;;
 		esac
-		printf '%s\n' "$command${rest:+ }$rest"
+		printf '%s\n' "$command$newargs"
 	done <"$todo" >"$todo.new" &&
 	mv -f "$todo.new" "$todo"
 }

2.0.0

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is used to implement many of the to-do list commands. Eventually, the complete do_pick interface will be exposed to the user in some form or another and those commands will become simple aliases for the do_pick options now used to implement them.

Add the git-commit option --reset-author to the options pool of do_pick. It rewrites the author date and name of the picked commit to match the committer date and name.

If --reset-author is passed to do_pick, set the rewrite flag and relay the option to the git-commit command line which creates the final commit. If --amend is not passed as well, the fresh authorship effect is achieved by the mere fact that we are creating a new commit.

---
 git-rebase--interactive.sh | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

index 2119d00..a9fcb76 100644
@@ -464,10 +464,18 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--amend] [--file <file>] [--edit] <commit>
+# do_pick [--reset-author] [--amend] [--file <file>] [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #
+# --reset-author
+#     Pretend the changes were made for the first time. Declare that the
+#     authorship of the resulting commit now belongs to the committer.
+#     This also renews the author timestamp. This creates a fresh
+#     commit.
+#
+#     _This is not a git-cherry-pick option._
+#
 # --amend
 #     After picking <commit>, replace the current head commit with a new
 #     commit that also introduces the changes of <commit>.

@@ -501,6 +509,10 @@ do_pick () {
 	while test $# -gt 0
 	do
 		case "$1" in
+		--reset-author)
+			rewrite=y
+			rewrite_author=y
+			;;
 		--amend)
 			if test "$(git rev-parse HEAD)" = "$squash_onto" || ! git rev-parse --verify HEAD
 			then

@@ -562,12 +574,21 @@ do_pick () {
 		pick_one ${rewrite:+-n} $1 || return 1
 	fi
 
+	if test -n "$rewrite_author" && test -z "$rewrite_amend"
+	then
+		# keep rewrite flag to create a new commit, rewrite
+		# without --reset-author though because it can only be
+		# used with -C, -c or --amend
+		rewrite_author=
+	fi
+
 	if test -n "$rewrite"
 	then
 		git commit --allow-empty --no-post-rewrite -n --no-edit \
 			   ${rewrite_amend:+--amend} \
 			   ${rewrite_edit:+--edit} \
 			   ${rewrite_message:+--file "$rewrite_message"} \
+			   ${rewrite_author:+--reset-author} \
 			   ${gpg_sign_opt:+"$gpg_sign_opt"} || return 3
 	fi
 

2.0.0

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is currently used to implement most of the to-do list commands and offers additional options that will eventually find their way onto to-do lists.

To extend the repertoire of available options, add the git-commit and git-cherry-pick option --signoff to the do_pick interface. It appends a Signed-off-by: line using the committer identity to the log message of the picked commit.

---
 git-rebase--interactive.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

index a9fcb76..bb258bb 100644
@@ -464,10 +464,15 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--reset-author] [--amend] [--file <file>] [--edit] <commit>
+# do_pick [--signoff] [--reset-author] [--amend] [--file <file>]
+#         [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #
+# -s, --signoff
+#     Insert a Signed-off-by: line using the committer identity at the
+#     end of the commit log message. This creates a fresh commit.
+#
 # --reset-author
 #     Pretend the changes were made for the first time. Declare that the
 #     authorship of the resulting commit now belongs to the committer.

@@ -509,6 +514,10 @@ do_pick () {
 	while test $# -gt 0
 	do
 		case "$1" in
+		-s|--signoff)
+			rewrite=y
+			rewrite_signoff=y
+			;;
 		--reset-author)
 			rewrite=y
 			rewrite_author=y

@@ -585,6 +594,7 @@ do_pick () {
 	if test -n "$rewrite"
 	then
 		git commit --allow-empty --no-post-rewrite -n --no-edit \
+			   ${rewrite_signoff:+--signoff} \
 			   ${rewrite_amend:+--amend} \
 			   ${rewrite_edit:+--edit} \
 			   ${rewrite_message:+--file "$rewrite_message"} \

2.0.0

pick and reword are atomic to-do list commands in the sense that they open a new task which is closed after the respective command is completed. squash and fixup are not atomic. They create a new task which is not completed until the last squash or fixup is processed.

Lift the general unknown option blockade for the pick and reword commands. If do_cmd comes across one of the options --signoff and --reset-author while parsing a to-do entry and the scheduled command is either pick or reword, relay the option to do_pick.

The do_pick options --gpg-sign and --file are not yet supported because do_cmd cannot handle option arguments and options with spaces at the moment. It is true that edit is one of the atomic commands but it displays hash information when the rebase is stopped and some options rewrite the picked commit which alters that information. squash and fixup still do not accept user options as the interplay of --reset-author and the author script are yet to be determined.

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

index bb258bb..c34a9a7 100644
@@ -624,6 +624,16 @@ do_replay () {
 	while test $# -gt 0
 	do
 		case "$1" in
+		--signoff|--reset-author)
+			case "$command" in
+			pick|reword)
+				;;
+			*)
+				warn "Unsupported option: $1"
+				command=unknown
+				;;
+			esac
+			;;
 		-*)
 			warn "Unknown option: $1"
 			command=unknown

@@ -644,21 +654,24 @@ do_replay () {
 		comment_for_reflog pick
 
 		mark_action_done
-		do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1... $rest"
+		eval do_pick $opts $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 || die_with_patch $sha1 "Could not apply $sha1... $rest"
+		eval do_pick --edit $opts $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 || die_with_patch $sha1 "Could not apply $sha1... $rest"
+		eval do_pick $opts $sha1 \
+			|| die_with_patch $sha1 "Could not apply $sha1... $rest"
 		warn "Stopped at $sha1... $rest"
 		exit_with_patch $sha1 0
 		;;

2.0.0

After "rebase -i:", some begin with lowercase and many begin with capital, which makes the short-log output look distracting.

The ones that begin with lower-case letters are the ones that begin with the command name "reword". All first lines are typed in lower case now.

Sorry for the noise.

Fabian

You could spell it 'reword' (with the quotes), which also disambiguates the command from the verb.

Thomas Rast
[email protected]

Hi,

this is a reroll of the patch series that enables rudimentary support of line options for git-rebase's to-do list commands and reimplements the well-known commands reword and squash in terms of a parameterised do_pick.

I tried to address all the issues raised by Junio in this reroll. Here is a short summary.

- Test that reword behaves like pick regarding the pre-commit hook and executes commit-msg for the new log message.

- Instead of disallowing empty log messages for root commits allow empty log messages for all unchanged commits.

- The patch "root commits are replayed with an unnecessary option" does not change the behaviour of git-rebase but it makes the subsequent patch easier to read.

The hidden agenda is to have one git-commit command line in do_pick that takes care of rewriting commits as they are being replayed, be they parentless or not. The advantage of having one rewrite command instead of one for root commits and one for non-root commits is that changes to the rewrite mechanism entail less code changes. As more rewriting mechanisms are being added to do_pick, it becomes important that they compose well with each other to avoid having a series of amending commits. The -C option conflicts with other ways of substituting the log message, like files and strings, which is why it seems not to compose so well.

- The patch "explicitly distinguish replay commands and exec tasks" does not change behaviour either but is again a separated refactoring patch.

The distinction of the exec command and the replay commands, which all share the same line format command args sha1 rest, allows us to employ the positional parameters feature of the shell without introducing another indentation level in do_pick.

Thanks for your time,
Fabian

Fabian Ruch (19):
rebase -i: failed reword prints redundant error message rebase -i: allow rewording an empty commit without complaints rebase -i: reword executes pre-commit hook on interim commit rebase -i: teach do_pick the option --edit
rebase -i: implement reword in terms of do_pick
rebase -i: allow replaying commits with empty log messages rebase -i: log the replay of root commits
rebase -i: root commits are replayed with an unnecessary option rebase -i: commit only once when rewriting picks
rebase -i: do not die in do_pick
rebase -i: teach do_pick the option --amend
rebase -i: teach do_pick the option --file
rebase -i: prepare for squash in terms of do_pick --amend rebase -i: implement squash in terms of do_pick
rebase -i: explicitly distinguish replay commands and exec tasks rebase -i: parse to-do list command line options
rebase -i: teach do_pick the option --reset-author
rebase -i: teach do_pick the option --signoff
rebase -i: enable options --signoff, --reset-author for pick, reword

 git-rebase--interactive.sh    | 284 ++++++++++++++++++++++++++++++++++--------
 t/t3404-rebase-interactive.sh |  70 +++++++++++
 t/t3412-rebase-root.sh        |  19 +++
 t/test-lib-functions.sh       |   6 +-
 4 files changed, 325 insertions(+), 54 deletions(-)

2.0.1

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. If the edited log message is empty or is found ill-formatted by one of the commit hooks, git-rebase--interactive prints three error messages to the console.

1. The git-commit output, which contains all the output from hook scripts.
2. A rebase diagnosis saying at which task on the to-do list it got stuck.
3. Generic presumptions about what could have triggered the error.

The third message contains redundant information and does not add any enlightenment either, which makes the output unnecessarily longish and different from the other command's output. For instance, this is what the output looks like if the log message is empty (contains duplicate Signed-off-by lines).

(1.) Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.) (2.) Could not amend commit after successfully picking fa1afe1... Some change (3.) 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.

Discard the third message.

It is true that a failed hook script might not output any diagnosis but then the generic message is not of much help either. Since this lack of information affects the built-in git commands for commit, merge and cherry-pick first, the solution would be to keep track of the failed hooks in their output so that the user knows which of her hooks require improvement.

---
 git-rebase--interactive.sh | 3 ---
 1 file changed, 3 deletions(-)

index 7e1eda0..e733d7f 100644
@@ -506,9 +506,6 @@ do_next () {
 		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
 		}
 		record_in_rewritten $sha1

2.0.1

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. If --keep-empty is passed as option to git-rebase--interactive, empty commits ought to be replayed without complaints. However, if the users chooses to reword an empty commit by changing the respective to-do list entry from

pick fa1afe1 Empty commit

to

reword fa1afe1 Empty commit

, then git-rebase--interactive suddenly fails to replay the empty commit. This is especially counterintuitive because reword is thought of as a pick that alters the log message in some way but nothing more and the unchanged to-do list entry would not fail.

Handle reword by cherry-picking the named commit and editing the log message using

    git commit --allow-empty --amend

instead of

    git commit --amend.

Add test.

---
 git-rebase--interactive.sh    | 2 +-
 t/t3404-rebase-interactive.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

index e733d7f..689400e 100644
@@ -504,7 +504,7 @@ do_next () {
 
 		mark_action_done
 		do_pick $sha1 "$rest"
-		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
+		git commit --allow-empty --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
 			warn "Could not amend commit after successfully picking $sha1... $rest"
 			exit_with_patch $sha1 1
 		}

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

2.0.1

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. This happens in two steps. Firstly, the named commit is cherry-picked. Secondly, the commit created in the first step is amended using an unchanged index to edit the log message. The pre-commit hook is meant to verify the changes introduced by a commit (for instance, catching inserted trailing white space). Since the committed tree is not changed between the two steps and we do not verify rebased patches, do not execute the pre-commit hook in the second step.

Specify the git-commit option --no-verify to disable the pre-commit hook when editing the log message. Because --no-verify also skips the commit-msg hook, execute the hook from within git-rebase--interactive after the commit is created. Fortunately, the commit message is still available in $GIT_DIR/COMMIT_EDITMSG after git-commit terminates. Caveat: In case the commit-msg hook finds the new log message ill-formatted, the user is only notified of the failed commit-msg hook but the log message is used for the commit anyway. git-commit ought to offer more fine-grained control over which hooks are executed.

Teach test_commit the --no-verify option and add test.

---
 git-rebase--interactive.sh    | 17 +++++++++++++----
 t/t3404-rebase-interactive.sh | 38 ++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh       |  6 +++++-
 3 files changed, 56 insertions(+), 5 deletions(-)

index 689400e..b50770d 100644
@@ -504,10 +504,19 @@ do_next () {
 
 		mark_action_done
 		do_pick $sha1 "$rest"
-		git commit --allow-empty --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
-			warn "Could not amend commit after successfully picking $sha1... $rest"
-			exit_with_patch $sha1 1
-		}
+		# TODO: Work around the fact that git-commit lets us
+		# disable either both the pre-commit and the commit-msg
+		# hook or none. Disable the pre-commit hook because the
+		# tree is left unchanged but run the commit-msg hook
+		# from here because the log message is altered.
+		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
+			if test -x "$GIT_DIR"/hooks/commit-msg
+			then
+				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
+			fi || {
+				warn "Could not amend commit after successfully picking $sha1... $rest"
+				exit_with_patch $sha1 1
+			}
 		record_in_rewritten $sha1
 		;;
 	edit|e)

index 9931143..2da4b9c 100755
@@ -577,6 +577,44 @@ test_expect_success 'rebase a commit violating pre-commit' '
 
 '
 
+test_expect_success 'reword a commit violating pre-commit' '
+	test_when_finished rm -r .git/hooks &&
+	mkdir -p .git/hooks &&
+	PRE_COMMIT=.git/hooks/pre-commit &&
+	cat >"$PRE_COMMIT" <<EOF
+#!/bin/sh
+echo running pre-commit: exit 1
+exit 1
+EOF
+	chmod +x "$PRE_COMMIT" &&
+	test_must_fail test_commit pre-commit-violated &&
+	test_commit --no-verify pre-commit-violated &&
+	test_when_finished reset_rebase &&
+	set_fake_editor &&
+	FAKE_LINES="pick 1" git rebase -i HEAD^ &&
+	FAKE_LINES="pick 1" git rebase -i --no-ff HEAD^ &&
+	FAKE_LINES="reword 1" git rebase -i HEAD^
+'
+
+test_expect_success 'reword a commit violating commit-msg' '
+	test_when_finished rm -r .git/hooks &&
+	mkdir -p .git/hooks &&
+	COMMIT_MSG=.git/hooks/commit-msg &&
+	cat >"$COMMIT_MSG" <<EOF
+#!/bin/sh
+echo running commit-msg: exit 1
+exit 1
+EOF
+	chmod +x "$COMMIT_MSG" &&
+	test_must_fail test_commit commit-msg-violated &&
+	test_commit --no-verify commit-msg-violated &&
+	test_when_finished reset_rebase &&
+	set_fake_editor &&
+	FAKE_LINES="pick 1" git rebase -i HEAD^ &&
+	FAKE_LINES="pick 1" git rebase -i --no-ff HEAD^ &&
+	test_must_fail env FAKE_LINES="reword 1" git rebase -i HEAD^
+'
+
 test_expect_success 'rebase with a file named HEAD in worktree' '
 
 	rm -fr .git/hooks &&

index 0377d3e..db65653 100644
@@ -155,6 +155,7 @@ test_pause () {
 test_commit () {
 	notick= &&
 	signoff= &&
+	noverify= &&
 	while test $# != 0
 	do
 		case "$1" in

@@ -164,6 +165,9 @@ test_commit () {
 		--signoff)
 			signoff="$1"
 			;;
+		--no-verify)
+			noverify="$1"
+			;;
 		*)
 			break
 			;;

@@ -177,7 +181,7 @@ test_commit () {
 	then
 		test_tick
 	fi &&
-	git commit $signoff -m "$1" &&
+	git commit $signoff $noverify -m "$1" &&
 	git tag "${4:-$1}"
 }
 

2.0.1

Thanks for a nice explanation of the tradeoff. Have you looked at adding an option to git-commit? We already have --no-post-rewrite. I think you would just need:

index 5ed6036..f7af220 100644
@@ -102,6 +102,7 @@ static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
 static char *sign_commit;
+static int no_pre_commit;
 
 /*
  * The default commit message cleanup mode will remove the lines

@@ -661,7 +662,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+	if (!no_verify && !no_pre_commit &&
+	    run_commit_hook(use_editor, index_file, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {

@@ -1604,6 +1606,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			 N_("terminate entries with NUL")),
 		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
 		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
+		OPT_BOOL(0, "no-pre-commit", &no_pre_commit, N_("bypass pre-commit hook")),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
 		/* end commit contents options */
 

though I would also not be opposed to some more uniform hook selection mechanism (e.g., "--no-verify=pre-commit" or something).

-Peff

Hi,

While the --no-verify= mechanism doesn't add a new option to the git-commit interface but lets one refine the --no-verify option, users might find it weird to have an argument to name disabled hooks but then not be able to disable all hooks that way.

To have that uniform hook selection without duplicating code, we might want to have something like --bypass-hook= right away. This would cover --no-post-rewrite as well and add support for disabling prepare-commit-msg and post-commit, whose execution cannot be controlled via the git-commit interface at the moment.

Since the hook selection wouldn't have to change, the options parsing code seems to be simpler (--bypass-hook= would have to support several occurrences with different arguments which could be implemented as an OPT_CALLBACK?) and git-commit decided to have --no- options for hook selection so far, I would lean towards your patch from above.

I know you didn't say anything otherwise, I just wanted to expand a little. You find an amended version of your patch below that documents --no-verify as a synonym for --no-pre-commit and --no-commit-msg, and adds tests.

Right, I think that is only worth doing if we add it uniformly for all hooks. I wonder if it should be a "git" option, not one to specific commands. Like:

git --disable-hook=pre-commit commit ...

and then the run_hook code can just respect the disabled list.

I think a name like "--disable-hook" or your "--bypass-hook" is better than "--no-verify" here, too. Not all hooks are about verifying (I also think "disable" is better than "bypass" for that reason, too).

You'd probably implement --disable-hook with OPT_STRING_LIST (or just do it by hand if it's the git.c option parser). And then the --no-post-rewrite arguments remain for historical compatibility, and can be implemented as synonyms for "--disable-hook=post-rewrite", etc.

I think people have also asked for the ability to override hooks, too, though I do not remember the exact details. Instead of --disable-hook, we could have an option for setting specific hooks (and setting them to nothing to disable would just be one possibility).

This is getting bigger in scope, though. I was trying not to derail you too much from your GSoC project, but see if we could just fix this one hacky corner easily.

I could go either way. Just because they are motivated by git-rebase does not mean other callers might not find them useful (after all, git commands are often meant to be scripted). As long as we promise to support them in future versions as we do with normal options, I do not think there is any problem in advertising them.

That being said, "git commit -h" is already getting pretty long. It might be worth cutting some seldom-used options from that list just to make it more palatable to normal users.

-Peff

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is used to implement the to-do list command pick. To cater for the different pick behaviours (like reword), do_pick accepts several options not only from the git-cherry-pick but also the git-commit interface. Add the common option --edit to let the user edit the log message of the named commit.

Loop over $@ to parse the do_pick arguments. Assign the local variable edit if one of the options is --edit so that the remainder of do_pick can easily check whether the client code asked to edit the commit message. If one of the options is unknown, mention it on the console and die. Break the loop on the first non-option and do some sanity checking to ensure that there exactly two non-options, which are interpreted by the remainder as <commit> and <title> like before.

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.

---
 git-rebase--interactive.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

index b50770d..e06d9b6 100644
@@ -461,7 +461,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.
+#
+# -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. This creates a fresh commit.
+#
+# <commit>
+#     The commit to cherry-pick.
+#
+# <title>
+#     The commit message title of <commit>. Used for information
+#     purposes only.
 do_pick () {
+	edit=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-e|--edit)
+			edit=y
+			;;
+		-*)
+			die "do_pick: unrecognized 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

@@ -483,6 +518,23 @@ do_pick () {
 		pick_one $1 ||
 			die_with_patch $1 "Could not apply $1... $2"
 	fi
+
+	if test -n "$edit"
+	then
+		# TODO: Work around the fact that git-commit lets us
+		# disable either both the pre-commit and the commit-msg
+		# hook or none. Disable the pre-commit hook because the
+		# tree is left unchanged but run the commit-msg hook
+		# from here because the log message is altered.
+		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
+			if test -x "$GIT_DIR"/hooks/commit-msg
+			then
+				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
+			fi || {
+				warn "Could not amend commit after successfully picking $1... $2"
+				exit_with_patch $1 1
+			}
+	fi
 }
 
 do_next () {

2.0.1

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. Reimplement reword in terms of do_pick --edit.

If the user picks a commit using the to-do list line

reword fa1afe1 Some change

execute the command do_pick --edit fa1afe1 "Some change" which carries out exactly the same steps as the case arm for reword in do_next so far.

---
 git-rebase--interactive.sh | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

index e06d9b6..4c875d5 100644
@@ -555,20 +555,7 @@ do_next () {
 		comment_for_reflog reword
 
 		mark_action_done
-		do_pick $sha1 "$rest"
-		# TODO: Work around the fact that git-commit lets us
-		# disable either both the pre-commit and the commit-msg
-		# hook or none. Disable the pre-commit hook because the
-		# tree is left unchanged but run the commit-msg hook
-		# from here because the log message is altered.
-		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
-			if test -x "$GIT_DIR"/hooks/commit-msg
-			then
-				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
-			fi || {
-				warn "Could not amend commit after successfully picking $sha1... $rest"
-				exit_with_patch $sha1 1
-			}
+		do_pick --edit $sha1 "$rest"
 		record_in_rewritten $sha1
 		;;
 	edit|e)

2.0.1

git-rebase--interactive handles empty log messages inconsistently between enabled and disabled fast-forwards. By default, commits with empty log messages are rebased successfully like in non-interactive mode. In contrast, the --no-ff option aborts the replay of such commits.

In line with not verifying rebased commits and behaving like git-rebase for pick lines, use the --allow-empty-message option to replay commits. Root commits are replayed by recreating them in do_pick using git-commit and commits with parents are replayed using git-cherry-pick in pick_one. Apply the option, understood by both git-commit and git-cherry-pick, at the respective sites.

In case of reword and squash/fixup continue to abort the rebase if the resulting commit would have no commit message. The rationale behind this default is that patches and their log messages should be verified at least once. For unchanged commits this is assumed to have happened according to the author's standards when she created the commits the first time. While the empty log message can always be kept in place by editing and resuming the aborted rebase, a debatable alternative could be to teach git-rebase--interactive the option --allow-empty-message for disabling complaints about empty log messages in changed commits. The to-do list command edit is handled just like pick for this matter, because git-rebase--interactive replays the named commit without changes before the rebase is interrupted and the user can make her changes to the replayed commit.

Add tests.

---
 git-rebase--interactive.sh    |  4 ++--
 t/t3404-rebase-interactive.sh | 24 ++++++++++++++++++++++++
 t/t3412-rebase-root.sh        | 19 +++++++++++++++++++
 3 files changed, 45 insertions(+), 2 deletions(-)

index 4c875d5..6e2c429 100644
@@ -249,7 +249,7 @@ pick_one () {
 
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output eval git cherry-pick \
+	output eval git cherry-pick --allow-empty-message \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 			"$strategy_args" $empty_args $ff "$@"
 }

@@ -363,7 +363,7 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output eval git cherry-pick \
+			output eval git cherry-pick --allow-empty-message \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 				"$strategy_args" "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"

index 2da4b9c..2e8fa27 100755
@@ -1085,4 +1085,28 @@ test_expect_success 'short SHA-1 collide' '
 	)
 '
 
+test_expect_success 'rebase commits with empty commit log messages' '
+	git checkout -b no-msg-commit &&
+	test_commit no-msg-commit-1 &&
+	git commit --amend --allow-empty-message -m " " &&
+	test_commit no-msg-commit-2 &&
+	git commit --amend --allow-empty-message -m " " &&
+	EDITOR=true git rebase -i HEAD^ &&
+	EDITOR=true git rebase -i --no-ff HEAD^
+'
+
+test_expect_success 'reword commit with empty commit log message' '
+	git checkout no-msg-commit &&
+	test_when_finished reset_rebase &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="reword 1" git rebase -i HEAD^
+'
+
+test_expect_success 'squash commits with empty commit log messages' '
+	git checkout no-msg-commit &&
+	test_when_finished reset_rebase &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 squash 2" git rebase -i HEAD^^
+'
+
 test_done

index 0b52105..798c9f1 100755
@@ -278,4 +278,23 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '
 	test_cmp expect-conflict-p out
 '
 
+test_expect_success 'rebase --root with empty root log message' '
+	git checkout --orphan no-msg-root-commit &&
+	test_commit no-msg-root-commit &&
+	git commit --amend -m " " --allow-empty-message &&
+	git rebase --root &&
+	test_path_is_file no-msg-root-commit.t &&
+	git rebase --root --no-ff &&
+	test_path_is_file no-msg-root-commit.t
+'
+
+test_expect_success 'rebase --root with empty child log message' '
+	test_commit no-msg-child-commit &&
+	git commit --amend -m " " --allow-empty-message &&
+	git rebase --root &&
+	test_path_is_file no-msg-child-commit.t &&
+	git rebase --root --no-ff &&
+	test_path_is_file no-msg-child-commit.t
+'
+
 test_done

2.0.1

The command line used to recreate root commits specifies the option -q which suppresses the commit summary message. However, git-rebase--interactive tends to tell the user about the commits it creates in the final history, if she wishes (cf. command line option --verbose). The code parts handling non-root commits and squash commits all output commit summary messages. Do not make the replay of root commits an exception. Remove the option to make the report of the rebased history complete.

It is OK that the commit summary is still suppressed when git-commit is used to initialize the authorship of the sentinel commit because this additional commit is an implementation detail hidden from the final history. The removed -q option was probably introduced as a copy-and-paste error stemming from that part of the root commit handling code.

---
 git-rebase--interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 6e2c429..576e0b1 100644
@@ -511,7 +511,7 @@ do_pick () {
 			   --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 \
+				   --amend --no-post-rewrite -n -C $1 \
 				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
 			die_with_patch $1 "Could not apply $1... $2"
 	else

2.0.1

I'm confused. This implies that we should be seeing summaries for other commits, but not root commits, and this patch is bring them into harmony. But if I have a repo like this:

git init -q repo &&
cd repo &&
for i in one two; do

    echo $i >file &&
    git add file &&
    git commit -q -m $i

done

then using stock git gives me this:

  $ GIT_EDITOR=true git rebase -i --root 2>&1 | perl -pe 's/\r/\\r\n/g'
  Rebasing (1/2)\r
  Rebasing (2/2)\r
  Successfully rebased and updated refs/heads/master.

but with your patch, I get:

  $ GIT_EDITOR=true git.compile rebase -i --root 2>&1 | perl -pe 's/\r/\\r\n/g'
  Rebasing (1/2)\r
  [detached HEAD 60834b3] one
   Date: Fri Aug 1 20:00:05 2014 -0400
   1 file changed, 1 insertion(+)
   create mode 100644 file
  Rebasing (2/2)\r
  Successfully rebased and updated refs/heads/master.

Am I misunderstanding the purpose of the patch?

-Peff

Hi,

Thanks for laying out the differences in the user visible output. With stock git we are seeing summaries for other commits, but not root commits, _with the --verbose option_. It's the fault of my patch to show the summary even in non-verbose mode. This is now fixed by wrapping the relevant command in 'output', a shell function defined in git-rebase.sh as follows:

The problem that git-rebase--interactive has is that the redirection of stdin to a variable (or a file) does not work reliably with commands that invoke the log message editor, that is 'reword' and 'squash' produce their output both in verbose and non-verbose mode. I wouldn't know how to fix this but

1) invoking $GIT_EDITOR from git-rebase--interactive.sh, but to make this right there should be a built-in command for shell scripts and an interface for git-commit that prepare the editor contents like git-commit does now, or

2) forcing $GIT_EDITOR and git-commit to print on distinct file descriptors, which would involve temporarily wrapping the $GIT_EDITOR call in a shell script that redirects stdin to some other file descriptor similar to what t/test-lib.sh does, or

3) passing the --quiet option in non-verbose mode and omitting it in verbose mode, which would cover the '$status != 0' above for if a command fails, it should indicate its error status despite being asked to be silent.

Options 2) and 3) seem attainable within this patch series and 3) sounds like the cleanest option but I'm uncertain if I'm missing something here. The only command line that is wrapped in 'output' and that doesn't support a --quiet option seems to be a 'warn' line which could simply be skipped in non-verbose mode. (Johannes Schindelin is cc'd as the original author of git-rebase--interactive.sh and 'output' in particular).

Fabian

Ah, OK. That makes a lot more sense, then.

Hmm. In the test scripts, we send stdout and stderr for sub-commands to fds 3 and 4 respectively. And then we either point those at /dev/null or to >&1 and >&2, depending on whether $verbose mode was specified.

I don't think that will work here, though. You literally have one "git commit" command to run, and you want its stderr/stdout to go somewhere different than the $GIT_EDITOR it will invoke. Your (2) makes some sense to me. Something like:

  GIT_EDITOR="$(shellquote "$(git var GIT_EDITOR)") >&3 2>&4" \
    git commit ... 3>&1 4>&2 >wherever 2>&1

Or we could just point it at /dev/tty, though I guess that may open another can of worms (systems without /dev/tty, what happens when you do not have a terminal, etc).

Yeah, that is probably the cleanest option if it works. I would just worry that it is not as complete. It works for "git commit", but are there are other commands wrapped in the verbose output that would want the same treatment (that might not know about --quiet)? Your paragraph below says it would not be that big a deal, so as long as we don't plan to add anything in the future that could not handle the requirement, that may be enough.

-Peff

The command line used to recreate root commits specifies the effectless option -C. It makes git-commit reuse commit message and authorship of the named commit. However, the commit being amended here, which is the sentinel commit, already carries the authorship and log message of the commit being replayed. Remove the option.

Since -C (in contrast to -c) does not invoke the editor and the --amend option invokes it by default, disable editor invocation again by specifying --no-edit.

---
 git-rebase--interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 576e0b1..46f436f 100644
@@ -511,7 +511,7 @@ do_pick () {
 			   --no-post-rewrite -n -q -C $1 &&
 			pick_one -n $1 &&
 			git commit --allow-empty --allow-empty-message \
-				   --amend --no-post-rewrite -n -C $1 \
+				   --amend --no-post-rewrite -n --no-edit \
 				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
 			die_with_patch $1 "Could not apply $1... $2"
 	else

2.0.1

I found this description a little backwards. The "-C" does have an effect, as you noticed in the second paragraph.

I think the reasoning is more like:

The command line used to recreate root commits uses "-C" to suppress the commit editor. This is unnecessarily confusing, though, because that suppression is a secondary effect of the option. The main purpose of "-C" is to pull the metadata from another commit, but here we know that this is a noop, since we are amending a commit just created from the same data.

At the time, commit did not yet know "--no-edit", and this was a reasonable way to get the desired behavior. We can switch it to use "--no-edit" to make the intended effect more obvious.

-Peff

Hi Jeff,

Thanks again, I shamelessly copied your formulation but squeezed in an "undocumented" because --no-edit had just been implemented (commit ca1ba2010), though was then still missing from the git-commit manpage.

Fabian

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.

git-commit lets the user edit the commit log message by default. We do not want that for the rewriting git-commit command line because the default behaviour of git-rebase is exactly the opposite. Pass --no-edit when rewriting a picked commit. An explicit --edit passed to do_pick (for instance, when reword is executed) enables the editor launch again. Similarly, pass --allow-empty-message unless the log message is edited.

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.

---
 git-rebase--interactive.sh | 58 ++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 22 deletions(-)

index 46f436f..96c24e8 100644
@@ -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

@@ -479,12 +480,17 @@ record_in_rewritten() {
 #     The commit message title of <commit>. Used for information
 #     purposes only.
 do_pick () {
-	edit=
+	allow_empty_message=y
+	rewrite=
+	rewrite_amend=
+	rewrite_edit=
 	while test $# -gt 0
 	do
 		case "$1" in
 		-e|--edit)
-			edit=y
+			rewrite=y
+			rewrite_edit=y
+			allow_empty_message=
 			;;
 		-*)
 			die "do_pick: unrecognized option -- $1"

@@ -499,6 +505,10 @@ 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

@@ -509,31 +519,35 @@ 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 --allow-empty-message \
-				   --amend --no-post-rewrite -n --no-edit \
-				   ${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
-		# TODO: Work around the fact that git-commit lets us
-		# disable either both the pre-commit and the commit-msg
-		# hook or none. Disable the pre-commit hook because the
-		# tree is left unchanged but run the commit-msg hook
-		# from here because the log message is altered.
-		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
-			if test -x "$GIT_DIR"/hooks/commit-msg
-			then
-				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
-			fi || {
-				warn "Could not amend commit after successfully picking $1... $2"
-				exit_with_patch $1 1
-			}
+		git commit --allow-empty --no-post-rewrite -n --no-edit \
+			   ${allow_empty_message:+--allow-empty-message} \
+			   ${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
+
+	# TODO: Work around the fact that git-commit lets us
+	# disable either both the pre-commit and the commit-msg
+	# hook or none. Disable the pre-commit hook because the
+	# tree is left unchanged but run the commit-msg hook
+	# from here because the log message is altered.
+	if test -n "$rewrite_edit"
+	then
+		if test -x "$GIT_DIR"/hooks/commit-msg
+		then
+			"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
+		fi ||
+			die_with_patch $1 "Could not rewrite commit after successfully picking $1... $2"
 	fi
 }
 

2.0.1

Hmm. So does this mean the user will actually see a different state during such a conflict?

E.g., if I have instructions like:

pick A
squash B
squash C

and there is a conflict picking C, then what state do I see? Right now I see a commit with the A+B squash prepared. But your description sounds to me like we would avoid the squash for "B", and the user would see a different state.

However, I couldn't trigger that behavior in a few experiments. Am I misunderstanding, or is there some case where the user-visible state will be different?

-Peff

Hi Jeff,

The squash state will not be different. squash sequences are still taken care of one line after the other: committing A, committing A+B, committing A+B+C. If there is a conflict picking C, HEAD will point to A+B and the index will record the conflicting changes.

The user-visible state will be different for rewords. For instance, let's consider

pick A
reword B.

The verbose log used to show two commits for B (with ff disabled), one after picking and one after editing. Now the log will show a single commit in connection with 'reword B' which might be less confusing.

Thanks for raising your eyebrows. I noticed now that the last paragraph is plainly wrong. The described amend situation did not arise "if the pick conflicted" but "if the edited commit did not verify". There will be no "after the conflicts are resolved" but the user can either commit manually and circumvent log message verification if she knows what she's doing, or provide a corrected log message in the editor launched by 'git rebase --continue'. The _incomplete_ 'git commit --amend' tip which used to be displayed after a failed verification hook could become unnecessary and this would possibly spare us including correct GPG sign options for instance.

However, this patch is mostly motivated by the unification of how commits are rewritten. Before, rewords and squashes went about this differently, now both fail with an uncommitted index if there are conflicts or the log message is ill-formatted.

The log message must be corrected and the following bug, which was noticed after PATCH v2, must be fixed.

cat >.git/hooks/commit-msg <<-EOF
#!/bin/sh
exit 1
EOF
chmod +x .git/hooks/commit-msg
test_must_fail env FAKE_LINES="reword 1" git rebase -i test_must_fail git rebase --continue
# the last command succeeds because --continue does not verify

Fabian

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. The git-rebase--interactive script is not terminated. Moreover, if do_pick is called while a squash or fixup is in effect, die_with_patch will discard $squash_msg as commit message. Lastly, after a die in do_pick do_next has no chance to reschedule tasks that failed before changes could be applied.

Indicate an error in do_pick using return statements and properly kill the script at the call sites. Although possible in principle, the issued error messages are no more indicating whether do_pick failed while applying or while committing the changes. This reduces code complexity at the call site and does not matter from a user's point of view because a glance at the index reveals whether there are conflicts or not and in-depth troubleshooting is still possible using the --verbose option.

Remove the commit message title argument from do_pick's interface, which has become unused.

---
 git-rebase--interactive.sh | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

index 96c24e8..9f08f1b 100644
@@ -464,7 +464,7 @@ 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.
 #

@@ -476,9 +476,11 @@ record_in_rewritten() {
 # <commit>
 #     The commit to cherry-pick.
 #
-# <title>
-#     The commit message title of <commit>. Used for information
-#     purposes only.
+# 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 () {
 	allow_empty_message=y
 	rewrite=

@@ -493,7 +495,8 @@ do_pick () {
 			allow_empty_message=
 			;;
 		-*)
-			die "do_pick: unrecognized option -- $1"
+			warn "do_pick: unrecognized option -- $1"
+			return 2
 			;;
 		*)
 			break

@@ -501,7 +504,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

@@ -519,11 +526,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"

@@ -532,8 +537,7 @@ do_pick () {
 			   ${allow_empty_message:+--allow-empty-message} \
 			   ${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
 
 	# TODO: Work around the fact that git-commit lets us

@@ -546,8 +550,7 @@ do_pick () {
 		if test -x "$GIT_DIR"/hooks/commit-msg
 		then
 			"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
-		fi ||
-			die_with_patch $1 "Could not rewrite commit after successfully picking $1... $2"
+		fi || return 3
 	fi
 }
 

@@ -562,21 +565,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.1

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is used to implement the to-do list commands pick, reword and edit. To cater for the different pick behaviours (like squash), do_pick accepts several options not only from the git-cherry-pick but also the git-commit interface.

Add the option --amend from the git-commit interface to the options pool of do_pick. It creates a new commit for the changes introduced by the picked commit and the previous one. The previous commit is then replaced with the new commit. If no other options are specified, the log message of the previous commit is used.

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 git-rebase--interactive 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.

---
 git-rebase--interactive.sh | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

index 9f08f1b..22a8f7b 100644
@@ -464,10 +464,16 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--edit] <commit>
+# do_pick [--amend] [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #
+# --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._
+#
 # -e, --edit
 #     After picking <commit>, open an editor and let the user edit the
 #     commit message. The editor contents becomes the commit message of

@@ -489,6 +495,16 @@ do_pick () {
 	while test $# -gt 0
 	do
 		case "$1" in
+		--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

2.0.1

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is used to implement the to-do list command pick, reword and edit. To cater for the different pick behaviours (like squash), do_pick accepts several options not only from the git-cherry-pick but also the git-commit interface.

Add the option --file from the git-commit interface to the options pool of do_pick. It expects an argument itself which is interpreted as a file path and takes the commit message from the given file. If --file is passed to do_pick, assign the given file path to the local variable rewrite_message and relay the option

--file "$rewrite_message"

to the git-commit command line which creates the commit.

---
 git-rebase--interactive.sh | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

index 22a8f7b..5df1086 100644
@@ -464,7 +464,7 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--amend] [--edit] <commit>
+# do_pick [--amend] [--file <file>] [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #

@@ -474,6 +474,12 @@ record_in_rewritten() {
 #
 #     _This is not a git-cherry-pick option._
 #
+# -F <file>, --file <file>
+#     Take the commit message from the given file. This creates a fresh
+#     commit.
+#
+#     _This is not a git-cherry-pick option._
+#
 # -e, --edit
 #     After picking <commit>, open an editor and let the user edit the
 #     commit message. The editor contents becomes the commit message of

@@ -492,6 +498,7 @@ do_pick () {
 	rewrite=
 	rewrite_amend=
 	rewrite_edit=
+	rewrite_message=
 	while test $# -gt 0
 	do
 		case "$1" in

@@ -505,6 +512,16 @@ do_pick () {
 			rewrite_amend=y
 			git rev-parse --verify HEAD >"$amend"
 			;;
+		-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
+			;;
 		-e|--edit)
 			rewrite=y
 			rewrite_edit=y

@@ -553,6 +570,7 @@ do_pick () {
 			   ${allow_empty_message:+--allow-empty-message} \
 			   ${rewrite_amend:+--amend} \
 			   ${rewrite_edit:+--edit} \
+			   ${rewrite_message:+--file "$rewrite_message"} \
 			   ${gpg_sign_opt:+"$gpg_sign_opt"} || return 3
 	fi
 

2.0.1

Rewrite squash and fixup handling in do_next using the sequence

pick_one
commit

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

Do not call rm -f "$GIT_DIR"/MERGE_MSG since it has no effect on the state after git-rebase--interactive terminates. The option -F makes git-commit ignore MERGE_MSG for the log message. If git-commit succeeds, MERGE_MSG is removed, and if it fails, MERGE_MSG is overwritten by the error sequence die_failed_squash. In summary, removing MERGE_MSG neither influences the squash commit message nor the file state after git-commit returns.

Moreover, pick_one ignores $GIT_DIR/SQUASH_MSG and does not touch $squash_msg so that it is correct to execute pick_one immediately before git-commit.

Might be squashed into the subsequent commit.

---
 git-rebase--interactive.sh | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

index 5df1086..d85e55d 100644
@@ -636,15 +636,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"

@@ -653,12 +653,21 @@ 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.1

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 git-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, reimplement squash in terms of do_pick --amend. Introduce $squash_msg as the commit message via the --file option. When the last commit of a squash series is processed, the user is asked to review the log message. Pass --edit as additional option in this case. The only difference in the options passed to git-commit and do_pick is the omitted --no-verify. However, do_pick does not execute the verification hooks anyway because it solely replays commits and assumes that they have been verified before.

---
 git-rebase--interactive.sh | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

index d85e55d..9307fa4 100644
@@ -640,37 +640,19 @@ 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
-				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.1

There are two kinds of to-do list commands available. One kind replays a commit (pick, reword, edit, squash and fixup that is) and the other executes a shell command (exec). We will call the first kind replay commands.

The two kinds of tasks are scheduled using different line formats. Replay commands expect a commit hash argument following the command name and exec concatenates all arguments to assemble a command line.

Adhere to the distinction of formats by not trying to parse the sha1 field unless we are dealing with a replay command. Move the replay command handling code to a new function do_replay which assumes the first argument to be a commit hash and make no more such assumptions in do_next.

---
 git-rebase--interactive.sh | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

index 9307fa4..8dde8e6 100644
@@ -588,13 +588,12 @@ do_pick () {
 	fi
 }
 
-do_next () {
-	rm -f "$msg" "$author_script" "$amend" || exit
-	read -r command sha1 rest < "$todo"
+do_replay () {
+	command=$1
+	sha1=$2
+	rest=$3
+
 	case "$command" in
-	"$comment_char"*|''|noop)
-		mark_action_done
-		;;
 	pick|p)
 		comment_for_reflog pick
 

@@ -659,6 +658,28 @@ do_next () {
 		esac
 		record_in_rewritten $sha1
 		;;
+	*)
+		read -r command <"$todo"
+		warn "Unknown command: $command"
+		fixtodo="Please fix this using 'git rebase --edit-todo'."
+		if git rev-parse --verify -q "$sha1" >/dev/null
+		then
+			die_with_patch $sha1 "$fixtodo"
+		else
+			die "$fixtodo"
+		fi
+		;;
+	esac
+}
+
+do_next () {
+	rm -f "$msg" "$author_script" "$amend" || exit
+	read -r command sha1 rest <"$todo"
+
+	case "$command" in
+	"$comment_char"*|''|noop)
+		mark_action_done
+		;;
 	x|"exec")
 		read -r command rest < "$todo"
 		mark_action_done

@@ -698,14 +719,7 @@ do_next () {
 		fi
 		;;
 	*)
-		warn "Unknown command: $command $sha1 $rest"
-		fixtodo="Please fix this using 'git rebase --edit-todo'."
-		if git rev-parse --verify -q "$sha1" >/dev/null
-		then
-			die_with_patch $sha1 "$fixtodo"
-		else
-			die "$fixtodo"
-		fi
+		do_replay $command $sha1 "$rest"
 		;;
 	esac
 	test -s "$todo" && return

2.0.1

Read in to-do list lines as

command args

instead of

command sha1 rest

so that to-do list command lines can specify additional arguments apart from the commit hash and the log message title, which become the non-options in args. Loop over args, put all options (an argument beginning with a dash) in opts, stop the loop on the first non-option and assign it to sha1. The loop does not know the options it parses so that options that take an argument themselves are not supported at the moment. Neither are options that contain spaces because the shell expansion of args in do_next interprets white space characters as argument separator, that is a command line like

pick --author "A U Thor" fa1afe1 Some change

is parsed as the pick command

    pick --author

and the commit hash

    "A

which obviously results in an unknown revision error. For the sake of completeness, in the example above the message title variable rest is assigned the string 'U Thor" fa1afe1 Some change' (without the single quotes).

Print an error message for unknown or unsupported command line options, which means an error for all specified options at the moment. Cleanly break the do_next loop by assigning the special value 'unknown' to the local variable command, which triggers the unknown command case in do_cmd.

The to-do list is also parsed when the commit hashes are translated between long and short format before and after the to-do list is edited. Apply the same procedure as in do_cmd with the exception that we only care about where the options stop and the commit hash begins. Do not reject any options when transforming the commit hashes.

---
 git-rebase--interactive.sh | 49 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 8 deletions(-)

index 8dde8e6..ff4ba7f 100644
@@ -590,8 +590,26 @@ do_pick () {
 
 do_replay () {
 	command=$1
-	sha1=$2
-	rest=$3
+	shift
+
+	opts=
+	while test $# -gt 0
+	do
+		case "$1" in
+		-*)
+			warn "Unknown option: $1"
+			command=unknown
+			;;
+		*)
+			break
+			;;
+		esac
+		opts="$opts $(git rev-parse --sq-quote "$1")"
+		shift
+	done
+	sha1=$1
+	shift
+	rest=$*
 
 	case "$command" in
 	pick|p)

@@ -674,7 +692,7 @@ do_replay () {
 
 do_next () {
 	rm -f "$msg" "$author_script" "$amend" || exit
-	read -r command sha1 rest <"$todo"
+	read -r command args <"$todo"
 
 	case "$command" in
 	"$comment_char"*|''|noop)

@@ -719,7 +737,7 @@ do_next () {
 		fi
 		;;
 	*)
-		do_replay $command $sha1 "$rest"
+		do_replay $command $args
 		;;
 	esac
 	test -s "$todo" && return

@@ -799,19 +817,34 @@ skip_unnecessary_picks () {
 }
 
 transform_todo_ids () {
-	while read -r command rest
+	while read -r command args
 	do
 		case "$command" in
 		"$comment_char"* | exec)
 			# Be careful for oddball commands like 'exec'
 			# that do not have a SHA-1 at the beginning of $rest.
+			newargs=\ $args
 			;;
 		*)
-			sha1=$(git rev-parse --verify --quiet "$@" ${rest%% *}) &&
-			rest="$sha1 ${rest#* }"
+			newargs=
+			sha1=
+			for arg in $args
+			do
+				case "$arg" in
+				-*)
+					newargs="$newargs $arg"
+					;;
+				*)
+					test -z "$sha1" &&
+						sha1=$(git rev-parse --verify --quiet "$@" $arg) &&
+						arg=$sha1
+					newargs="$newargs $arg"
+					;;
+				esac
+			done
 			;;
 		esac
-		printf '%s\n' "$command${rest:+ }$rest"
+		printf '%s\n' "$command$newargs"
 	done <"$todo" >"$todo.new" &&
 	mv -f "$todo.new" "$todo"
 }

2.0.1

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is used to implement many of the to-do list commands. Eventually, the complete do_pick interface will be exposed to the user in some form or another and those commands will become simple aliases for the do_pick options now used to implement them.

Add the git-commit option --reset-author to the options pool of do_pick. It rewrites the author date and name of the picked commit to match the committer date and name.

If --reset-author is passed to do_pick, set the rewrite flag and relay the option to the git-commit command line which creates the final commit. If --amend is not passed as well, the fresh authorship effect is achieved by the mere fact that we are creating a new commit.

---
 git-rebase--interactive.sh | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

index ff4ba7f..3886a80 100644
@@ -464,10 +464,18 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--amend] [--file <file>] [--edit] <commit>
+# do_pick [--reset-author] [--amend] [--file <file>] [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #
+# --reset-author
+#     Pretend the changes were made for the first time. Declare that the
+#     authorship of the resulting commit now belongs to the committer.
+#     This also renews the author timestamp. This creates a fresh
+#     commit.
+#
+#     _This is not a git-cherry-pick option._
+#
 # --amend
 #     After picking <commit>, replace the current head commit with a new
 #     commit that also introduces the changes of <commit>.

@@ -502,6 +510,10 @@ do_pick () {
 	while test $# -gt 0
 	do
 		case "$1" in
+		--reset-author)
+			rewrite=y
+			rewrite_author=y
+			;;
 		--amend)
 			if test "$(git rev-parse HEAD)" = "$squash_onto" || ! git rev-parse --verify HEAD
 			then

@@ -564,6 +576,14 @@ do_pick () {
 		pick_one ${rewrite:+-n} $1 || return 1
 	fi
 
+	if test -n "$rewrite_author" && test -z "$rewrite_amend"
+	then
+		# keep rewrite flag to create a new commit, rewrite
+		# without --reset-author though because it can only be
+		# used with -C, -c or --amend
+		rewrite_author=
+	fi
+
 	if test -n "$rewrite"
 	then
 		git commit --allow-empty --no-post-rewrite -n --no-edit \

@@ -571,6 +591,7 @@ do_pick () {
 			   ${rewrite_amend:+--amend} \
 			   ${rewrite_edit:+--edit} \
 			   ${rewrite_message:+--file "$rewrite_message"} \
+			   ${rewrite_author:+--reset-author} \
 			   ${gpg_sign_opt:+"$gpg_sign_opt"} || return 3
 	fi
 

2.0.1

do_pick is the git-cherry-pick wrapper in git-rebase--interactive that is currently used to implement most of the to-do list commands and offers additional options that will eventually find their way onto to-do lists.

To extend the repertoire of available options, add the git-commit and git-cherry-pick option --signoff to the do_pick interface. It appends a Signed-off-by: line using the committer identity to the log message of the picked commit.

---
 git-rebase--interactive.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

index 3886a80..9324ed3 100644
@@ -464,10 +464,15 @@ record_in_rewritten() {
 
 # Apply the changes introduced by the given commit to the current head.
 #
-# do_pick [--reset-author] [--amend] [--file <file>] [--edit] <commit>
+# do_pick [--signoff] [--reset-author] [--amend] [--file <file>]
+#         [--edit] <commit>
 #
 # Wrapper around git-cherry-pick.
 #
+# -s, --signoff
+#     Insert a Signed-off-by: line using the committer identity at the
+#     end of the commit log message. This creates a fresh commit.
+#
 # --reset-author
 #     Pretend the changes were made for the first time. Declare that the
 #     authorship of the resulting commit now belongs to the committer.

@@ -510,6 +515,10 @@ do_pick () {
 	while test $# -gt 0
 	do
 		case "$1" in
+		-s|--signoff)
+			rewrite=y
+			rewrite_signoff=y
+			;;
 		--reset-author)
 			rewrite=y
 			rewrite_author=y

@@ -588,6 +597,7 @@ do_pick () {
 	then
 		git commit --allow-empty --no-post-rewrite -n --no-edit \
 			   ${allow_empty_message:+--allow-empty-message} \
+			   ${rewrite_signoff:+--signoff} \
 			   ${rewrite_amend:+--amend} \
 			   ${rewrite_edit:+--edit} \
 			   ${rewrite_message:+--file "$rewrite_message"} \

2.0.1

pick and reword are atomic to-do list commands in the sense that they open a new task which is closed after the respective command is completed. squash and fixup are not atomic. They create a new task which is not completed until the last squash or fixup is processed.

Lift the general unknown option blockade for the pick and reword commands. If do_cmd comes across one of the options --signoff and --reset-author while parsing a to-do entry and the scheduled command is either pick or reword, relay the option to do_pick.

The do_pick options --gpg-sign and --file are not yet supported because do_cmd cannot handle option arguments and options with spaces at the moment. It is true that edit is one of the atomic commands but it displays hash information when the rebase is stopped and some options rewrite the picked commit which alters that information. squash and fixup still do not accept user options as the interplay of --reset-author and the author script are yet to be determined.

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

index 9324ed3..32fd047 100644
@@ -627,6 +627,16 @@ do_replay () {
 	while test $# -gt 0
 	do
 		case "$1" in
+		--signoff|--reset-author)
+			case "$command" in
+			pick|reword)
+				;;
+			*)
+				warn "Unsupported option: $1"
+				command=unknown
+				;;
+			esac
+			;;
 		-*)
 			warn "Unknown option: $1"
 			command=unknown

@@ -647,21 +657,24 @@ do_replay () {
 		comment_for_reflog pick
 
 		mark_action_done
-		do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1... $rest"
+		eval do_pick $opts $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 || die_with_patch $sha1 "Could not apply $sha1... $rest"
+		eval do_pick --edit $opts $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 || die_with_patch $sha1 "Could not apply $sha1... $rest"
+		eval do_pick $opts $sha1 \
+			|| die_with_patch $sha1 "Could not apply $sha1... $rest"
 		warn "Stopped at $sha1... $rest"
 		exit_with_patch $sha1 0
 		;;

2.0.1

I just finished reading over the whole series (which is my first real exposure to it). With the exception of the comments I already sent, it looks pretty reasonable to me.

Thanks for splitting it and writing good commit messages; that made it relatively easy to follow what was going on.

-Peff

Hi Peff,

Thanks a lot for taking the time.

Your review revealed more shortcomings of the patch series. Now that the replay of root commits is logged, the log is dumped on the console even in non-verbose mode. And, I must admit that the fact that --no-edit hasn't been a (documented) feature of git-commit for all time didn't struck me at all as a possible reason for using -C, which is a little embarrassing.

I will include more details in separate replies to your comments later.

Fabian

Hi List,

this is the second reroll of the patch series fixing corner-case bugs regarding empty commits, commits with no log message and root commits, providing a uniform implementation of the to-do list commands using the do_pick interface.

This reroll includes the following list of changes to PATCH v1 from last week, mostly induced by Jeff's comments. However, I didn't get around the most recent discussion from tonight yet.

- new tests: allow replaying commits with empty log messages
- coverage of all to-do list commands
- new patch: allow squashing empty commits without complaints
- do not complain about an empty squash commit unless it is the final one and --keep-empty is not specified on the command line
- new tests: allow rewording empty commits without complaints
- coverage of all to-do list commands
- new patch: hide interactive command messages in verbose mode
- make it possible to launch an editor inside 'output'
- new patch: allow disabling pre-commit and commit-msg separately
- add options --no-pre-commit and --no-commit-msg to git-commit
- redefine --no-verify as synonym for the above two
- new patch: squash skips commit-msg hook
- run commit-msg hook for reworded _and_ squashed commits
- a change to 'test_commit' options and 'fake_editor' debug output

Thanks for your time and reviews,
Fabian

Fabian Ruch (23):
rebase -i: allow replaying commits with empty log messages rebase -i: allow squashing empty commits without complaints rebase -i: allow rewording empty commits without complaints rebase -i: hide interactive command messages in verbose mode rebase -i: failed reword prints redundant error message
commit: allow disabling pre-commit and commit-msg separately rebase -i: squash skips commit-msg hook
rebase -i: reword executes pre-commit hook on interim commit rebase -i: teach do_pick the option --edit
rebase -i: implement reword in terms of do_pick
rebase -i: log the replay of root commits
rebase -i: root commits are replayed with an unnecessary option rebase -i: commit only once when rewriting picks
rebase -i: do not die in do_pick
rebase -i: teach do_pick the option --amend
rebase -i: teach do_pick the option --file
rebase -i: prepare for squash in terms of do_pick --amend rebase -i: implement squash in terms of do_pick
rebase -i: explicitly distinguish replay commands and exec tasks rebase -i: parse to-do list command line options
rebase -i: teach do_pick the option --reset-author
rebase -i: teach do_pick the option --signoff
rebase -i: enable options --signoff, --reset-author for pick, reword

 Documentation/git-commit.txt  |   8 +-
 builtin/commit.c              |  32 ++++-
 git-rebase--interactive.sh    | 288 ++++++++++++++++++++++++++++++++++--------
 git-rebase.sh                 |  12 +-
 t/lib-rebase.sh               |   8 +-
 t/t3404-rebase-interactive.sh | 234 ++++++++++++++++++++++++++++++++--
 t/t3406-rebase-message.sh     |  18 +++
 t/t3412-rebase-root.sh        |  16 +++
 t/t7503-pre-commit-hook.sh    |  65 ++++++++--
 t/t7504-commit-msg-hook.sh    |  85 ++++++++++---
 t/test-lib-functions.sh       |  23 +++-
 11 files changed, 680 insertions(+), 109 deletions(-)

2.0.1

git-rebase--interactive handles empty log messages inconsistently between enabled and disabled fast-forwards. By default, commits with empty log messages are rebased successfully like in non-interactive mode. In contrast, the --no-ff option aborts the replay of such commits.

In line with not verifying rebased commits and behaving like git-rebase for pick lines, use the --allow-empty-message option to replay commits. Root commits are replayed by recreating them in do_pick using git-commit and all other commits are replayed using git-cherry-pick in pick_one. Apply the option, understood by both git-commit and git-cherry-pick, at the respective sites.

In case of reword and squash, continue to abort the rebase if the _resulting_ commit would have no commit message. The rationale behind this default is that patches and their log messages should be verified at least once. For unchanged commits this is assumed to have happened according to the author's standards when she created the commits for the first time. While the empty log message can always be kept in place by editing and resuming the aborted rebase, a debatable alternative could be to teach git-rebase--interactive the option --allow-empty-message for disabling complaints about empty log messages even in changed commits.

The fixup case is different again because it throws away the second commit's log message and uses the first log message for the changed commit. Do not abort the rebase if that message is empty either since it is assumed to have been verified already.

The remaining to-do list command edit is handled just like pick for this matter, because git-rebase--interactive replays the named commit without changes before the rebase is interrupted and the user can make her changes to the replayed commit.

Add tests. In particular, design the squash-specific test case such that it involves interim commits and fixup steps. Interim commits should not trigger failures themselves and fixup steps should not let git-rebase--interactive forget that it is still dealing with a squash result.

---
 git-rebase--interactive.sh    | 10 ++++++----
 t/t3404-rebase-interactive.sh | 38 ++++++++++++++++++++++++++++++++++++++
 t/t3412-rebase-root.sh        | 16 ++++++++++++++++
 3 files changed, 60 insertions(+), 4 deletions(-)

index b64dd28..3222bf6 100644
@@ -249,7 +249,7 @@ pick_one () {
 
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output eval git cherry-pick \
+	output eval git cherry-pick --allow-empty-message \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 			"$strategy_args" $empty_args $ff "$@"
 }

@@ -363,7 +363,7 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output eval git cherry-pick \
+			output eval git cherry-pick --allow-empty-message \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 				"$strategy_args" "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"

@@ -549,7 +549,8 @@ 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:
-			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
+			do_with_author output git commit --allow-empty-message \
+				--amend --no-verify -F "$squash_msg" \
 				${gpg_sign_opt:+"$gpg_sign_opt"} ||
 				die_failed_squash $sha1 "$rest"
 			;;

@@ -557,7 +558,8 @@ do_next () {
 			# This is the final command of this squash/fixup group
 			if test -f "$fixup_msg"
 			then
-				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
+				do_with_author git commit --allow-empty-message \
+					--amend --no-verify -F "$fixup_msg" \
 					${gpg_sign_opt:+"$gpg_sign_opt"} ||
 					die_failed_squash $sha1 "$rest"
 			else

index 8197ed2..9c71835 100755
@@ -1039,4 +1039,42 @@ test_expect_success 'short SHA-1 collide' '
 	)
 '
 
+test_expect_success 'setup commits with empty commit log messages' '
+	git checkout -b empty-log-messages master &&
+	test_commit no-msg-commit-1 &&
+	git commit --amend --allow-empty-message -F - </dev/null &&
+	test_commit no-msg-commit-2 &&
+	git commit --amend --allow-empty-message -F - </dev/null &&
+	test_commit no-msg-commit-3 &&
+	git commit --amend --allow-empty-message -F - </dev/null
+'
+
+test_expect_success 'rebase commits with empty commit log messages' '
+	git checkout -b rebase-empty-log-messages empty-log-messages &&
+	set_fake_editor &&
+	test_expect_code 0 env FAKE_LINES="1" git rebase -i master &&
+	test_expect_code 0 env FAKE_LINES="1" git rebase -i --no-ff master
+'
+
+test_expect_success 'reword commits with empty commit log messages' '
+	git checkout -b reword-empty-log-messages empty-log-messages &&
+	test_when_finished reset_rebase &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="reword 1" git rebase -i master
+'
+
+test_expect_success 'squash commits with empty commit log messages' '
+	git checkout -b squash-empty-log-messages empty-log-messages &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 squash 2 fixup 3" git rebase -i master &&
+	git commit --allow-empty-message --amend &&
+	git rebase --continue
+'
+
+test_expect_success 'fixup commits with empty commit log messages' '
+	git checkout -b fixup-empty-log-messages empty-log-messages &&
+	set_fake_editor &&
+	env FAKE_LINES="1 fixup 2" git rebase -i master
+'
+
 test_done

index 0b52105..7add7a1 100755
@@ -278,4 +278,20 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '
 	test_cmp expect-conflict-p out
 '
 
+test_expect_success 'rebase --root root with empty log message' '
+	git checkout --orphan empty-log-messages-root master &&
+	test_commit no-msg-root-commit &&
+	git commit --amend --allow-empty-message -F - </dev/null &&
+	test_expect_code 0 git rebase --root &&
+	test_expect_code 0 git rebase --root --no-ff
+'
+
+test_expect_success 'rebase --root commits with empty log messages' '
+	git checkout -b empty-log-messages master &&
+	test_commit no-msg-commit &&
+	git commit --amend --allow-empty-message -F - </dev/null &&
+	test_expect_code 0 git rebase --root &&
+	test_expect_code 0 git rebase --root --no-ff
+'
+
 test_done

2.0.1

The to-do list commands squash and fixup apply the changes introduced by the named commit to the tree but instead of creating a new commit on top of the current head it replaces the previous commit with a new commit that records the updated tree. If the result is an empty commit git-rebase stops with the error message

You asked to amend the most recent commit, but doing so would make it empty. You can repeat your command with --allow-empty, or you can remove the commit entirely with "git reset HEAD^".

This message is not very helpful because neither does git-rebase support an option --allow-empty nor does the messages say how to resume the rebase. Firstly, change the error message to

The squash result is empty and --keep-empty was not specified.

You can remove the squash commit now with

git reset HEAD^

Once you are down, run

     git rebase --continue

If the user wishes to squash a sequence of commits into one commit, f. i.

pick A
squash Revert "A"
squash A'

, it does not matter for the end result that the first squash result, or any sub-sequence in general, is going to be empty. The squash message is not affected at all by which commits are created and only the commit created by the last line in the sequence will end up in the final history. Secondly, print the error message only if the whole squash sequence produced an empty commit.

Lastly, since an empty squash commit is not a failure to rewrite the history as planned, issue the message above as a mere warning and interrupt the rebase with the return value zero. The interruption should be considered as a notification with the chance to undo it on the spot. Specifying the --keep-empty option tells git-rebase to keep empty squash commits in the rebased history without notification.

Add tests.

---
Hi,

Peter Krefting is cc'd as the author of the bug report "Confusing error message in rebase when commit becomes empty" discussed on the mailing list in June. Phil Hord and Jeff King both participated in the problem discussion which ended with two proposals by Jeff.

This patch chooses the second alternative. Either way seems OK. The crucial consensus of the discussion was to silently throw away empty interim commits.

Fabian

 git-rebase--interactive.sh    | 20 +++++++++++---
 t/t3404-rebase-interactive.sh | 62 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)

index 3222bf6..8820eac 100644
@@ -549,7 +549,7 @@ 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:
-			do_with_author output git commit --allow-empty-message \
+			do_with_author output git commit --allow-empty-message --allow-empty \
 				--amend --no-verify -F "$squash_msg" \
 				${gpg_sign_opt:+"$gpg_sign_opt"} ||
 				die_failed_squash $sha1 "$rest"

@@ -558,18 +558,32 @@ do_next () {
 			# This is the final command of this squash/fixup group
 			if test -f "$fixup_msg"
 			then
-				do_with_author git commit --allow-empty-message \
+				do_with_author git commit --allow-empty-message --allow-empty \
 					--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
-				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
+				do_with_author git commit --allow-empty --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
 					${gpg_sign_opt:+"$gpg_sign_opt"} ||
 					die_failed_squash $sha1 "$rest"
 			fi
 			rm -f "$squash_msg" "$fixup_msg"
+			if test -z "$keep_empty" && is_empty_commit HEAD
+			then
+				echo "$sha1" >"$state_dir"/stopped-sha
+				warn "The squash result is empty and --keep-empty was not specified."
+				warn
+				warn "You can remove the squash commit now with"
+				warn
+				warn "  git reset HEAD^"
+				warn
+				warn "Once you are down, run"
+				warn
+				warn "  git rebase --continue"
+				exit 0
+			fi
 			;;
 		esac
 		record_in_rewritten $sha1

2.0.1

Fabian Ruch:

Yes, the important part is that giving good advice is better than giving bad advice. Thank you for taking your time to fix this.

I haven't reviewed the changes themselves, but I am happy with the underlying idea.

\\// Peter - http://www.softwolves.pp.se/

I guess you meant: s/down/done

Same issue with the actually message in the code (below).

Hi Eric,

Fixed.

Thanks for the thorough reading.

Fabian

Thanks for working on this.

s/down/done

I like the direction of this rewording, but it seems to hide the instructions for the user who wants to keep the empty commit.

I'm not sure how to improve it. Maybe this:

The squash result is empty and --keep-empty was not specified.

You may remove the squash commit now with

git reset HEAD^

or keep it by doing nothing extra.

Continue the rebase with

git rebase --continue

This patch does _not_ silently throw away empty commits. I wonder if such behavior could be triggered with something like --no-keep-empty. Maybe that belongs in another patch.

The to-do list command reword replays a commit like pick but lets the user also edit the commit's log message. If --keep-empty is passed as option to git-rebase--interactive, empty commits ought to be replayed without complaints. However, if the users chooses to reword an empty commit by changing the respective to-do list entry from

pick fa1afe1 Empty commit

to

reword fa1afe1 Empty commit

, then git-rebase--interactive suddenly fails to replay the empty commit. This is especially counterintuitive because reword is thought of as a pick that alters the log message in some way but nothing more and the unchanged to-do list entry would not fail.

Handle reword by cherry-picking the named commit and editing the log message using

    git commit --allow-empty --amend

instead of

    git commit --amend.

Add test.

---
 git-rebase--interactive.sh    |  2 +-
 t/t3404-rebase-interactive.sh | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

index 8820eac..89ef5e2 100644
@@ -504,7 +504,7 @@ do_next () {
 
 		mark_action_done
 		do_pick $sha1 "$rest"
-		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
+		git commit --allow-empty --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"

index a95cb2a..3e64280 100755
@@ -75,6 +75,30 @@ test_expect_success 'rebase --keep-empty' '
 	test_line_count = 6 actual
 '
 
+test_expect_success 'rebase --keep-empty (reword)' '
+	git checkout -b reword-emptybranch 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 --keep-empty (fixup)' '
+	git checkout -b fixup-emptybranch emptybranch &&
+	set_fake_editor &&
+	FAKE_LINES="1 fixup 2" git rebase --keep-empty -i HEAD~2 &&
+	git log --oneline >actual &&
+	test_line_count = 5 actual
+'
+
+test_expect_success 'rebase --keep-empty (squash)' '
+	git checkout -b squash-emptybranch emptybranch &&
+	set_fake_editor &&
+	FAKE_LINES="1 squash 2" git rebase --keep-empty -i HEAD~2 &&
+	git log --oneline >actual &&
+	test_line_count = 5 actual
+'
+
 test_expect_success 'rebase -i with the exec command' '
 	git checkout master &&
 	(

2.0.1

git-rebase--interactive prints summary messages of the commits it creates in the final history only if the --verbose option is specified by the user and suppresses them otherwise. This behaviour is implemented by wrapping git-commit calls in a shell function named output which redirects stderr to stdout, captures stdout in a shell variable and ignores its contents unless the command exits with an error status.

The command lines used to implement the to-do list commands reword and squash print diagnostic messages even in non-verbose mode. The reason for this inconsistency is that both commands launch the log message editor which usually requires a working terminal attached to stdin. Temporarily redirect the editor output to a third file descriptor in order to ship it around the capture stream. Wrap the respective git-commit command lines in output.

fake_editor prints the to-do list before and after applying the FAKE_LINES rewrite rules to it. Redirect this debug output to stderr so that it does not interfere with the git-rebase status output. Add test.

---
 git-rebase--interactive.sh    |  9 +++++----
 git-rebase.sh                 | 12 ++++++++++--
 t/lib-rebase.sh               |  8 ++++----
 t/t3404-rebase-interactive.sh | 18 ++++++------------
 t/t3406-rebase-message.sh     | 18 ++++++++++++++++++
 5 files changed, 43 insertions(+), 22 deletions(-)

index 89ef5e2..5dfdf13 100644
@@ -504,7 +504,7 @@ do_next () {
 
 		mark_action_done
 		do_pick $sha1 "$rest"
-		git commit --allow-empty --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
+		output git commit --allow-empty --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"

@@ -558,14 +558,14 @@ do_next () {
 			# This is the final command of this squash/fixup group
 			if test -f "$fixup_msg"
 			then
-				do_with_author git commit --allow-empty-message --allow-empty \
+				do_with_author output git commit --allow-empty-message --allow-empty \
 					--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
-				do_with_author git commit --allow-empty --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
+				do_with_author output git commit --allow-empty --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
 					${gpg_sign_opt:+"$gpg_sign_opt"} ||
 					die_failed_squash $sha1 "$rest"
 			fi

@@ -923,6 +923,8 @@ EOF
 	;;
 esac
 
+mkdir -p "$state_dir" || die "Could not create temporary $state_dir"
+
 git var GIT_COMMITTER_IDENT >/dev/null ||
 	die "You need to set your committer info first"
 

@@ -938,7 +940,6 @@ then
 fi
 
 orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?"
-mkdir -p "$state_dir" || die "Could not create temporary $state_dir"
 
 : > "$state_dir"/interactive || die "Could not mark as interactive"
 write_basic_state

index 55da9db..f90541e 100755
@@ -131,9 +131,17 @@ write_basic_state () {
 output () {
 	case "$verbose" in
 	'')
-		output=$("$@" 2>&1 )
+		cat >"$state_dir"/editor.sh <<EOF
+#!/bin/sh
+$(git var GIT_EDITOR) "\$@" >&3
+EOF
+		chmod +x "$state_dir"/editor.sh
+		(
+			export GIT_EDITOR=\""$state_dir"/editor.sh\"
+			"$@" 3>&1 >"$state_dir"/output 2>&1
+		)
 		status=$?
-		test $status != 0 && printf "%s\n" "$output"
+		test $status != 0 && cat "$state_dir"/output
 		return $status
 		;;
 	*)

index 6bd2522..0cd1193 100644
@@ -41,8 +41,8 @@ set_fake_editor () {
 	test -z "$FAKE_LINES" && exit
 	grep -v '^#' < "$1" > "$1".tmp
 	rm -f "$1"
-	echo 'rebase -i script before editing:'
-	cat "$1".tmp
+	echo 'rebase -i script before editing:' >&2
+	cat "$1".tmp >&2
 	action=pick
 	for line in $FAKE_LINES; do
 		case $line in

@@ -59,8 +59,8 @@ set_fake_editor () {
 			action=pick;;
 		esac
 	done
-	echo 'rebase -i script after editing:'
-	cat "$1"
+	echo 'rebase -i script after editing:' >&2
+	cat "$1" >&2
 	EOF
 
 	test_set_editor "$(pwd)/fake-editor.sh"

index 3e64280..c6578c3 100755
@@ -873,9 +873,8 @@ test_expect_success 'running "git rebase -i --exec git show HEAD"' '
 	(
 		FAKE_LINES="1 exec_git_show_HEAD 2 exec_git_show_HEAD" &&
 		export FAKE_LINES &&
-		git rebase -i HEAD~2 >expect
+		git rebase -i HEAD~2 >expected
 	) &&
-	sed -e "1,9d" expect >expected &&
 	test_cmp expected actual
 '
 

@@ -887,9 +886,8 @@ test_expect_success 'running "git rebase --exec git show HEAD -i"' '
 	(
 		FAKE_LINES="1 exec_git_show_HEAD 2 exec_git_show_HEAD" &&
 		export FAKE_LINES &&
-		git rebase -i HEAD~2 >expect
+		git rebase -i HEAD~2 >expected
 	) &&
-	sed -e "1,9d" expect >expected &&
 	test_cmp expected actual
 '
 

@@ -901,9 +899,8 @@ test_expect_success 'running "git rebase -ix git show HEAD"' '
 	(
 		FAKE_LINES="1 exec_git_show_HEAD 2 exec_git_show_HEAD" &&
 		export FAKE_LINES &&
-		git rebase -i HEAD~2 >expect
+		git rebase -i HEAD~2 >expected
 	) &&
-	sed -e "1,9d" expect >expected &&
 	test_cmp expected actual
 '
 

@@ -915,9 +912,8 @@ test_expect_success 'rebase -ix with several <CMD>' '
 	(
 		FAKE_LINES="1 exec_git_show_HEAD;_pwd 2 exec_git_show_HEAD;_pwd" &&
 		export FAKE_LINES &&
-		git rebase -i HEAD~2 >expect
+		git rebase -i HEAD~2 >expected
 	) &&
-	sed -e "1,9d" expect >expected &&
 	test_cmp expected actual
 '
 

@@ -930,9 +926,8 @@ test_expect_success 'rebase -ix with several instances of --exec' '
 		FAKE_LINES="1 exec_git_show_HEAD exec_pwd 2
 				exec_git_show_HEAD exec_pwd" &&
 		export FAKE_LINES &&
-		git rebase -i HEAD~2 >expect
+		git rebase -i HEAD~2 >expected
 	) &&
-	sed -e "1,11d" expect >expected &&
 	test_cmp expected actual
 '
 

@@ -956,9 +951,8 @@ test_expect_success 'rebase -ix with --autosquash' '
 		git checkout -b autosquash_expected &&
 		FAKE_LINES="1 fixup 3 fixup 4 exec_git_show_HEAD 2 exec_git_show_HEAD" &&
 		export FAKE_LINES &&
-		git rebase -i HEAD~4 >expect
+		git rebase -i HEAD~4 >expected
 	) &&
-	sed -e "1,13d" expect >expected &&
 	test_cmp expected actual
 '
 

index 0392e36..d7003a9 100755
@@ -4,6 +4,8 @@ test_description='messages from rebase operation'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 test_expect_success 'setup' '
 	test_commit O fileO &&
 	test_commit X fileX &&

@@ -84,4 +86,20 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
 	test_i18ngrep "invalid-ref" err
 '
 
+test_expect_success 'commit summary is suppressed in non-verbose mode' '
+	git checkout --detach Y &&
+	cat >expected.out <<-EOF &&
+	Rebasing (1/5)\r
+	Rebasing (2/5)\r
+	Rebasing (3/5)\r
+	Rebasing (4/5)\r
+	Rebasing (5/5)\r
+	EOF
+	set_fake_editor &&
+	FAKE_LINES="reword 1 fixup 2 fixup 3 4 squash 5" \
+	git rebase -i --root >actual.out.tmp &&
+	sed -e "s/\r/\\\\r\n/g" <actual.out.tmp >actual.out &&
+	test_cmp expected.out actual.out
+'
+
 test_done