[PATCH 00/18] rebase -i: For pure fixups, do not start log message editor

[PATCH 00/18] rebase -i: For pure fixups, do not start log message editor

[Michael Haggerty]

This patch series is a successor to mh/rebase-fixup, which causes
"rebase -i" to skip opening the log message editor when processing a
block of "fixup" commands that does not include any "squash"es.  The
idea was discussed on the mailing list [1] to general approval.

This patch series applies to "next", as it depends on earlier commits
in mh/rebase-fixup.  With this patch series, I believe that this topic
is finished.

The first several commits are trivial fixups.

The three commits starting with "Document how temporary files are
used" attempt to document how the existing git-rebase--interactive.sh
script uses temporary files, as that information was not obvious.  I
would appreciate feedback about whether I have inferred the uses
correctly.

Following are several refactorings leading to the main point of this
patch series, "For fixup commands without squashes, do not start
editor".

I also noticed a related (pre-existing) bug, namely that when there is
a conflict during the processing of a "squash" series, the user is
sometimes asked to edit an intermediate commit message.  But after
later squashes are processed, the user's edited message is discarded.
The last two patches fix this problem--whenever the user has edited a
commit message, the resulting combined commit is treated (internally)
as a new starting point for further squash/fixups.  As such, the
user-edited commit message is the first part of the suggested commit
message for the final squashed/fixedup commit.

Michael

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

Michael Haggerty (18):
  rebase -i: Make the condition for an "if" more transparent
  rebase -i: Remove dead code
  rebase -i: Inline expression
  rebase -i: Use "test -n" instead of "test ! -z"
  rebase -i: Use symbolic constant $MSG consistently
  rebase -i: Document how temporary files are used
  rebase -i: Introduce a constant AUTHOR_SCRIPT
  rebase -i: Introduce a constant AMEND
  t3404: Test the commit count in commit messages generated by "rebase
    -i"
  rebase -i: Improve consistency of commit count in generated commit
    messages
  rebase -i: Simplify commit counting for generated commit messages
  rebase -i: Extract a function "commit_message"
  rebase -i: Handle the author script all in one place in do_next
  rebase -i: Extract function do_with_author
  rebase -i: Change function make_squash_message into
    update_squash_message
  rebase -i: For fixup commands without squashes, do not start editor
  t3404: Set up more of the test repo in the "setup" step
  rebase -i: Retain user-edited commit messages after squash/fixup
    conflicts

 git-rebase--interactive.sh    |  224 ++++++++++++++++++++++++++---------------
 t/lib-rebase.sh               |    6 +-
 t/t3404-rebase-interactive.sh |  101 +++++++++++++------
 3 files changed, 219 insertions(+), 112 deletions(-)

[PATCH 01/18] rebase -i: Make the condition for an "if" more transparent

[Michael Haggerty]

Test $no_ff separately rather than testing it indirectly by gluing it
onto a comparison of two SHA1s.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2e56e64..fba8fb6 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -166,7 +166,8 @@ pick_one () {
 	parent_sha1=$(git rev-parse --verify $sha1^) ||
 		die "Could not get the parent of $sha1"
 	current_sha1=$(git rev-parse --verify HEAD)
-	if test "$no_ff$current_sha1" = "$parent_sha1"; then
+	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"
+	then
 		output git reset --hard $sha1
 		test "a$1" = a-n && output git reset --soft $current_sha1
 		sha1=$(git rev-parse --short $sha1)
-- 
1.6.6

[PATCH 02/18] rebase -i: Remove dead code

[Michael Haggerty]

This branch of the "if" is only executed if $no_ff is empty, which
only happens if $1 was not '-n'.  (This code has been dead since
1d25c8cf82eead72e11287d574ef72d3ebec0db1.)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index fba8fb6..c6174bb 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -169,7 +169,6 @@ pick_one () {
 	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"
 	then
 		output git reset --hard $sha1
-		test "a$1" = a-n && output git reset --soft $current_sha1
 		sha1=$(git rev-parse --short $sha1)
 		output warn Fast-forward to $sha1
 	else
-- 
1.6.6

[PATCH 03/18] rebase -i: Inline expression

[Michael Haggerty]

Inline expression when generating output rather than overwriting the
"sha1" local variable with a short SHA1.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c6174bb..3b65cdf 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -169,8 +169,7 @@ pick_one () {
 	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"
 	then
 		output git reset --hard $sha1
-		sha1=$(git rev-parse --short $sha1)
-		output warn Fast-forward to $sha1
+		output warn Fast-forward to $(git rev-parse --short $sha1)
 	else
 		output git cherry-pick "$@"
 	fi
-- 
1.6.6

[PATCH 04/18] rebase -i: Use "test -n" instead of "test ! -z"

[Michael Haggerty]

It is a tiny bit simpler.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3b65cdf..6dbc64e 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -158,7 +158,7 @@ pick_one () {
 	output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
 	test -d "$REWRITTEN" &&
 		pick_one_preserving_merges "$@" && return
-	if test ! -z "$REBASE_ROOT"
+	if test -n "$REBASE_ROOT"
 	then
 		output git cherry-pick "$@"
 		return
-- 
1.6.6

[PATCH 05/18] rebase -i: Use symbolic constant $MSG consistently

[Michael Haggerty]

The filename constant $MSG was previously used in some places and
written out literally in others.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6dbc64e..b7e8189 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -131,8 +131,8 @@ make_patch () {
 		echo "Root commit"
 		;;
 	esac > "$DOTEST"/patch
-	test -f "$DOTEST"/message ||
-		git cat-file commit "$1" | sed "1,/^$/d" > "$DOTEST"/message
+	test -f "$MSG" ||
+		git cat-file commit "$1" | sed "1,/^$/d" > "$MSG"
 	test -f "$DOTEST"/author-script ||
 		get_author_ident_from_commit "$1" > "$DOTEST"/author-script
 }
@@ -343,7 +343,7 @@ peek_next_command () {
 }
 
 do_next () {
-	rm -f "$DOTEST"/message "$DOTEST"/author-script \
+	rm -f "$MSG" "$DOTEST"/author-script \
 		"$DOTEST"/amend || exit
 	read command sha1 rest < "$TODO"
 	case "$command" in
@@ -611,7 +611,7 @@ first and then run 'git rebase --continue' again."
 				die "Cannot rewind the HEAD"
 			fi
 			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
-			git commit --no-verify -F "$DOTEST"/message -e || {
+			git commit --no-verify -F "$MSG" -e || {
 				test -n "$amend" && git reset --soft $amend
 				die "Could not commit staged changes."
 			}
-- 
1.6.6

[PATCH 06/18] rebase -i: Document how temporary files are used

[Michael Haggerty]

Add documentation, inferred by reverse-engineering, about how
git-rebase--interactive.sh uses many of its temporary files.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |   41 ++++++++++++++++++++++++++++++++++-------
 1 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b7e8189..77e5773 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -35,11 +35,45 @@ autosquash         move commits that begin with squash!/fixup! under -i
 require_work_tree
 
 DOTEST="$GIT_DIR/rebase-merge"
+
+# The file containing rebase commands, comments, and empty lines.
+# This file is created by "git rebase -i" then edited by the user.  As
+# the lines are processed, they are removed from the front of this
+# file and written to the tail of $DONE.
 TODO="$DOTEST"/git-rebase-todo
+
+# The rebase command lines that have already been processed.  A line
+# is moved here when it is first handled, before any associated user
+# actions.
 DONE="$DOTEST"/done
+
+# The commit message that is planned to be used for any changes that
+# need to be committed following a user interaction.
 MSG="$DOTEST"/message
+
+# The file into which is accumulated the suggested commit message for
+# squash/fixup commands.  When the first of a series of squash/fixups
+# is seen, the file is created and the commit message from the
+# previous commit and from the first squash/fixup commit are written
+# to it.  The commit message for each subsequent squash/fixup commit
+# is appended to the file as it is processed.
+#
+# The first line of the file is of the form
+#     # This is a combination of $COUNT commits.
+# where $COUNT is the number of commits whose messages have been
+# written to the file so far (including the initial "pick" commit).
+# Each time that a commit message is processed, this line is read and
+# updated.  It is deleted just before the combined commit is made.
 SQUASH_MSG="$DOTEST"/message-squash
+
+# $REWRITTEN is the name of a directory containing files for each
+# commit that is reachable by at least one merge base of $HEAD and
+# $UPSTREAM. They are not necessarily rewritten, but their children
+# might be.  This ensures that commits on merged, but otherwise
+# unrelated side branches are left alone. (Think "X" in the man page's
+# example.)
 REWRITTEN="$DOTEST"/rewritten
+
 DROPPED="$DOTEST"/dropped
 PRESERVE_MERGES=
 STRATEGY=
@@ -738,13 +772,6 @@ first and then run 'git rebase --continue' again."
 		test t = "$VERBOSE" && : > "$DOTEST"/verbose
 		if test t = "$PRESERVE_MERGES"
 		then
-			# $REWRITTEN contains files for each commit that is
-			# reachable by at least one merge base of $HEAD and
-			# $UPSTREAM. They are not necessarily rewritten, but
-			# their children might be.
-			# This ensures that commits on merged, but otherwise
-			# unrelated side branches are left alone. (Think "X"
-			# in the man page's example.)
 			if test -z "$REBASE_ROOT"
 			then
 				mkdir "$REWRITTEN" &&
-- 
1.6.6

[PATCH 07/18] rebase -i: Introduce a constant AUTHOR_SCRIPT

[Michael Haggerty]

Add a constant AUTHOR_SCRIPT, holding the filename of the
$DOTEST/author_script file, and document how this temporary file is
used.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 77e5773..6fcc00e 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -75,6 +75,12 @@ SQUASH_MSG="$DOTEST"/message-squash
 REWRITTEN="$DOTEST"/rewritten
 
 DROPPED="$DOTEST"/dropped
+
+# A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
+# GIT_AUTHOR_DATE that will be used for the commit that is currently
+# being rebased.
+AUTHOR_SCRIPT="$DOTEST"/author-script
+
 PRESERVE_MERGES=
 STRATEGY=
 ONTO=
@@ -167,8 +173,8 @@ make_patch () {
 	esac > "$DOTEST"/patch
 	test -f "$MSG" ||
 		git cat-file commit "$1" | sed "1,/^$/d" > "$MSG"
-	test -f "$DOTEST"/author-script ||
-		get_author_ident_from_commit "$1" > "$DOTEST"/author-script
+	test -f "$AUTHOR_SCRIPT" ||
+		get_author_ident_from_commit "$1" > "$AUTHOR_SCRIPT"
 }
 
 die_with_patch () {
@@ -377,8 +383,7 @@ peek_next_command () {
 }
 
 do_next () {
-	rm -f "$MSG" "$DOTEST"/author-script \
-		"$DOTEST"/amend || exit
+	rm -f "$MSG" "$AUTHOR_SCRIPT" "$DOTEST"/amend || exit
 	read command sha1 rest < "$TODO"
 	case "$command" in
 	'#'*|''|noop)
@@ -454,7 +459,7 @@ do_next () {
 			rm -f "$GIT_DIR"/MERGE_MSG || exit
 			;;
 		esac
-		echo "$author_script" > "$DOTEST"/author-script
+		echo "$author_script" > "$AUTHOR_SCRIPT"
 		if test $failed = f
 		then
 			# This is like --amend, but with a different message
@@ -631,7 +636,7 @@ do
 		then
 			: Nothing to commit -- skip this
 		else
-			. "$DOTEST"/author-script ||
+			. "$AUTHOR_SCRIPT" ||
 				die "Cannot find the author identity"
 			amend=
 			if test -f "$DOTEST"/amend
-- 
1.6.6

[PATCH 08/18] rebase -i: Introduce a constant AMEND

[Michael Haggerty]

Add a constant AMEND holding the filename of the $DOTEST/amend file,
and document how this temporary file is used.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6fcc00e..2c7f3ec 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -81,6 +81,14 @@ DROPPED="$DOTEST"/dropped
 # being rebased.
 AUTHOR_SCRIPT="$DOTEST"/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
+# --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
+# command is processed, this file is deleted.
+AMEND="$DOTEST"/amend
+
 PRESERVE_MERGES=
 STRATEGY=
 ONTO=
@@ -383,7 +391,7 @@ peek_next_command () {
 }
 
 do_next () {
-	rm -f "$MSG" "$AUTHOR_SCRIPT" "$DOTEST"/amend || exit
+	rm -f "$MSG" "$AUTHOR_SCRIPT" "$AMEND" || exit
 	read command sha1 rest < "$TODO"
 	case "$command" in
 	'#'*|''|noop)
@@ -411,7 +419,7 @@ do_next () {
 		pick_one $sha1 ||
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
 		make_patch $sha1
-		git rev-parse --verify HEAD > "$DOTEST"/amend
+		git rev-parse --verify HEAD > "$AMEND"
 		warn "Stopped at $sha1... $rest"
 		warn "You can amend the commit now, with"
 		warn
@@ -639,10 +647,10 @@ do
 			. "$AUTHOR_SCRIPT" ||
 				die "Cannot find the author identity"
 			amend=
-			if test -f "$DOTEST"/amend
+			if test -f "$AMEND"
 			then
 				amend=$(git rev-parse --verify HEAD)
-				test "$amend" = $(cat "$DOTEST"/amend) ||
+				test "$amend" = $(cat "$AMEND") ||
 				die "\
 You have uncommitted changes in your working tree. Please, commit them
 first and then run 'git rebase --continue' again."
-- 
1.6.6

[PATCH 09/18] t3404: Test the commit count in commit messages generated by "rebase -i"

[Michael Haggerty]

The first line of commit messages generated for "rebase -i"
squash/fixup commits includes a count of the number of commits that
are being combined.  Add machinery to check that this count is
correct, and add such a check to some test cases.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/lib-rebase.sh               |    6 +++++-
 t/t3404-rebase-interactive.sh |    9 +++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 0db8250..2d922ae 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -2,9 +2,10 @@
 
 # After setting the fake editor with this function, you can
 #
-# - override the commit message with $FAKE_COMMIT_MESSAGE,
+# - override the commit message with $FAKE_COMMIT_MESSAGE
 # - amend the commit message with $FAKE_COMMIT_AMEND
 # - check that non-commit messages have a certain line count with $EXPECT_COUNT
+# - check the commit count in the commit message header with $EXPECT_HEADER_COUNT
 # - rewrite a rebase -i script as directed by $FAKE_LINES.
 #   $FAKE_LINES consists of a sequence of words separated by spaces.
 #   The following word combinations are possible:
@@ -25,6 +26,9 @@ set_fake_editor () {
 	cat >> fake-editor.sh <<\EOF
 case "$1" in
 */COMMIT_EDITMSG)
+	test -z "$EXPECT_HEADER_COUNT" ||
+		test "$EXPECT_HEADER_COUNT" = $(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1") ||
+		exit
 	test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
 	test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
 	exit
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d9382e4..0335b78 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -135,7 +135,8 @@ test_expect_success 'squash' '
 	test_tick &&
 	GIT_AUTHOR_NAME="Nitfol" git commit -m "nitfol" file7 &&
 	echo "******************************" &&
-	FAKE_LINES="1 squash 2" git rebase -i --onto master HEAD~2 &&
+	FAKE_LINES="1 squash 2" EXPECT_HEADER_COUNT=two \
+		git rebase -i --onto master HEAD~2 &&
 	test B = $(cat file7) &&
 	test $(git rev-parse HEAD^) = $(git rev-parse master)
 '
@@ -230,6 +231,7 @@ test_expect_success 'verbose flag is heeded, even after --continue' '
 test_expect_success 'multi-squash only fires up editor once' '
 	base=$(git rev-parse HEAD~4) &&
 	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 squash 2 squash 3 squash 4" \
+		EXPECT_HEADER_COUNT=4 \
 		git rebase -i $base &&
 	test $base = $(git rev-parse HEAD^) &&
 	test 1 = $(git show | grep ONCE | wc -l)
@@ -239,6 +241,7 @@ test_expect_success 'multi-fixup only fires up editor once' '
 	git checkout -b multi-fixup E &&
 	base=$(git rev-parse HEAD~4) &&
 	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
+		EXPECT_HEADER_COUNT=4 \
 		git rebase -i $base &&
 	test $base = $(git rev-parse HEAD^) &&
 	test 1 = $(git show | grep ONCE | wc -l) &&
@@ -258,6 +261,7 @@ test_expect_success 'squash and fixup generate correct log messages' '
 	git checkout -b squash-fixup E &&
 	base=$(git rev-parse HEAD~4) &&
 	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fixup 2 squash 3 fixup 4" \
+		EXPECT_HEADER_COUNT=4 \
 		git rebase -i $base &&
 	git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
 	test_cmp expect-squash-fixup actual-squash-fixup &&
@@ -297,7 +301,8 @@ test_expect_success 'squash works as expected' '
 		git commit -m $n
 	done &&
 	one=$(git rev-parse HEAD~3) &&
-	FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&
+	FAKE_LINES="1 squash 3 2" EXPECT_HEADER_COUNT=two \
+		git rebase -i HEAD~3 &&
 	test $one = $(git rev-parse HEAD~2)
 '
 
-- 
1.6.6

[PATCH 10/18] rebase -i: Improve consistency of commit count in generated commit messages

[Michael Haggerty]

Use the numeral "2" instead of the word "two" when two commits are
being interactively squashed.  This makes the treatment consistent
with that for higher numbers of commits.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh    |    2 +-
 t/t3404-rebase-interactive.sh |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c7f3ec..16e1990 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -362,7 +362,7 @@ make_squash_message () {
 		}' <"$SQUASH_MSG"
 	else
 		COUNT=2
-		echo "# This is a combination of two commits."
+		echo "# This is a combination of 2 commits."
 		echo "# The first commit's message is:"
 		echo
 		git cat-file commit HEAD | sed -e '1,/^$/d'
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 0335b78..0511709 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -135,7 +135,7 @@ test_expect_success 'squash' '
 	test_tick &&
 	GIT_AUTHOR_NAME="Nitfol" git commit -m "nitfol" file7 &&
 	echo "******************************" &&
-	FAKE_LINES="1 squash 2" EXPECT_HEADER_COUNT=two \
+	FAKE_LINES="1 squash 2" EXPECT_HEADER_COUNT=2 \
 		git rebase -i --onto master HEAD~2 &&
 	test B = $(cat file7) &&
 	test $(git rev-parse HEAD^) = $(git rev-parse master)
@@ -301,7 +301,7 @@ test_expect_success 'squash works as expected' '
 		git commit -m $n
 	done &&
 	one=$(git rev-parse HEAD~3) &&
-	FAKE_LINES="1 squash 3 2" EXPECT_HEADER_COUNT=two \
+	FAKE_LINES="1 squash 3 2" EXPECT_HEADER_COUNT=2 \
 		git rebase -i HEAD~3 &&
 	test $one = $(git rev-parse HEAD~2)
 '
-- 
1.6.6

[PATCH 11/18] rebase -i: Simplify commit counting for generated commit messages

[Michael Haggerty]

Read the old count from the first line of the old commit message
rather than counting the number of commit message blocks in the file.
This is simpler, faster, and more robust (e.g., it cannot be confused
by strange commit message contents).

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 16e1990..5ed80b0 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -351,11 +351,9 @@ nth_string () {
 
 make_squash_message () {
 	if test -f "$SQUASH_MSG"; then
-		# We want to be careful about matching only the commit
-		# message comment lines generated by this function.
-		# "[snrt][tdh]" matches the nth_string endings.
-		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)[snrt][tdh] commit message.*:/\1/p" \
-			< "$SQUASH_MSG" | sed -ne '$p')+1))
+		COUNT=$(($(sed -n \
+			-e "1s/^# This is a combination of \(.*\) commits\./\1/p" \
+			-e "q" < "$SQUASH_MSG")+1))
 		echo "# This is a combination of $COUNT commits."
 		sed -e 1d -e '2,/^./{
 			/^$/d
@@ -378,9 +376,6 @@ make_squash_message () {
 		echo
 		echo "# The $(nth_string $COUNT) commit message will be skipped:"
 		echo
-		# Comment the lines of the commit message out using
-		# "#	" rather than "# " to make them less likely to
-		# confuse the sed regexp above.
 		git cat-file commit $2 | sed -e '1,/^$/d' -e 's/^/#	/'
 		;;
 	esac
-- 
1.6.6

[PATCH 12/18] rebase -i: Extract a function "commit_message"

[Michael Haggerty]

...instead of repeating the same short but slightly obscure blob of
code in several places.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5ed80b0..04a1c3a 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -120,6 +120,11 @@ output () {
 	esac
 }
 
+# Output the commit message for the specified commit.
+commit_message () {
+	git cat-file commit "$1" | sed "1,/^$/d"
+}
+
 run_pre_rebase_hook () {
 	if test -z "$OK_TO_SKIP_PRE_REBASE" &&
 	   test -x "$GIT_DIR/hooks/pre-rebase"
@@ -180,7 +185,7 @@ make_patch () {
 		;;
 	esac > "$DOTEST"/patch
 	test -f "$MSG" ||
-		git cat-file commit "$1" | sed "1,/^$/d" > "$MSG"
+		commit_message "$1" > "$MSG"
 	test -f "$AUTHOR_SCRIPT" ||
 		get_author_ident_from_commit "$1" > "$AUTHOR_SCRIPT"
 }
@@ -318,7 +323,7 @@ pick_one_preserving_merges () {
 			# redo merge
 			author_script=$(get_author_ident_from_commit $sha1)
 			eval "$author_script"
-			msg="$(git cat-file commit $sha1 | sed -e '1,/^$/d')"
+			msg="$(commit_message $sha1)"
 			# No point in merging the first parent, that's HEAD
 			new_parents=${new_parents# $first_parent}
 			if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
@@ -363,20 +368,20 @@ make_squash_message () {
 		echo "# This is a combination of 2 commits."
 		echo "# The first commit's message is:"
 		echo
-		git cat-file commit HEAD | sed -e '1,/^$/d'
+		commit_message HEAD
 	fi
 	case $1 in
 	squash)
 		echo
 		echo "# This is the $(nth_string $COUNT) commit message:"
 		echo
-		git cat-file commit $2 | sed -e '1,/^$/d'
+		commit_message $2
 		;;
 	fixup)
 		echo
 		echo "# The $(nth_string $COUNT) commit message will be skipped:"
 		echo
-		git cat-file commit $2 | sed -e '1,/^$/d' -e 's/^/#	/'
+		commit_message $2 | sed -e 's/^/#	/'
 		;;
 	esac
 }
-- 
1.6.6

[PATCH 13/18] rebase -i: Handle the author script all in one place in do_next

[Michael Haggerty]

This change has no practical effect but makes the code easier to
follow.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 04a1c3a..15b8605 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -449,6 +449,8 @@ do_next () {
 		make_squash_message $squash_style $sha1 > "$MSG"
 		failed=f
 		author_script=$(get_author_ident_from_commit HEAD)
+		echo "$author_script" > "$AUTHOR_SCRIPT"
+		eval "$author_script"
 		output git reset --soft HEAD^
 		pick_one -n $sha1 || failed=t
 		case "$(peek_next_command)" in
@@ -467,11 +469,9 @@ do_next () {
 			rm -f "$GIT_DIR"/MERGE_MSG || exit
 			;;
 		esac
-		echo "$author_script" > "$AUTHOR_SCRIPT"
 		if test $failed = f
 		then
 			# This is like --amend, but with a different message
-			eval "$author_script"
 			GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
 			GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
 			GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
-- 
1.6.6

[PATCH 14/18] rebase -i: Extract function do_with_author

[Michael Haggerty]

Call it instead of repeating similar code blocks in several places.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 15b8605..2902644 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -205,6 +205,15 @@ has_action () {
 	sane_grep '^[^#]' "$1" >/dev/null
 }
 
+# Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
+# GIT_AUTHOR_DATE exported from the current environment.
+do_with_author () {
+	GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
+	GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
+	GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
+	"$@"
+}
+
 pick_one () {
 	no_ff=
 	case "$1" in -n) sha1=$2; no_ff=t ;; *) sha1=$1 ;; esac
@@ -326,11 +335,8 @@ pick_one_preserving_merges () {
 			msg="$(commit_message $sha1)"
 			# No point in merging the first parent, that's HEAD
 			new_parents=${new_parents# $first_parent}
-			if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
-				GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
-				GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
-				output git merge $STRATEGY -m "$msg" \
-					$new_parents
+			if ! do_with_author output \
+				git merge $STRATEGY -m "$msg" $new_parents
 			then
 				printf "%s\n" "$msg" > "$GIT_DIR"/MERGE_MSG
 				die_with_patch $sha1 "Error redoing merge $sha1"
@@ -472,11 +478,9 @@ do_next () {
 		if test $failed = f
 		then
 			# This is like --amend, but with a different message
-			GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
-			GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
-			GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
-			$USE_OUTPUT git commit --no-verify \
-				$MSG_OPT "$EDIT_OR_FILE" || failed=t
+			do_with_author $USE_OUTPUT git commit --no-verify \
+				$MSG_OPT "$EDIT_OR_FILE" ||
+				failed=t
 		fi
 		if test $failed = t
 		then
@@ -657,8 +661,7 @@ first and then run 'git rebase --continue' again."
 				git reset --soft HEAD^ ||
 				die "Cannot rewind the HEAD"
 			fi
-			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
-			git commit --no-verify -F "$MSG" -e || {
+			do_with_author git commit --no-verify -F "$MSG" -e || {
 				test -n "$amend" && git reset --soft $amend
 				die "Could not commit staged changes."
 			}
-- 
1.6.6

[PATCH 15/18] rebase -i: Change function make_squash_message into update_squash_message

[Michael Haggerty]

Alter the file $SQUASH_MSG in place rather than outputting the new
message then juggling it around.  Change the function name
accordingly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2902644..5a48fbf 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -360,21 +360,26 @@ nth_string () {
 	esac
 }
 
-make_squash_message () {
+update_squash_message () {
 	if test -f "$SQUASH_MSG"; then
+		mv "$SQUASH_MSG" "$SQUASH_MSG".bak || exit
 		COUNT=$(($(sed -n \
 			-e "1s/^# This is a combination of \(.*\) commits\./\1/p" \
-			-e "q" < "$SQUASH_MSG")+1))
-		echo "# This is a combination of $COUNT commits."
-		sed -e 1d -e '2,/^./{
-			/^$/d
-		}' <"$SQUASH_MSG"
+			-e "q" < "$SQUASH_MSG".bak)+1))
+		{
+			echo "# This is a combination of $COUNT commits."
+			sed -e 1d -e '2,/^./{
+				/^$/d
+			}' <"$SQUASH_MSG".bak
+		} >$SQUASH_MSG
 	else
 		COUNT=2
-		echo "# This is a combination of 2 commits."
-		echo "# The first commit's message is:"
-		echo
-		commit_message HEAD
+		{
+			echo "# This is a combination of 2 commits."
+			echo "# The first commit's message is:"
+			echo
+			commit_message HEAD
+		} >$SQUASH_MSG
 	fi
 	case $1 in
 	squash)
@@ -389,7 +394,7 @@ make_squash_message () {
 		echo
 		commit_message $2 | sed -e 's/^/#	/'
 		;;
-	esac
+	esac >>$SQUASH_MSG
 }
 
 peek_next_command () {
@@ -452,7 +457,7 @@ do_next () {
 			die "Cannot '$squash_style' without a previous commit"
 
 		mark_action_done
-		make_squash_message $squash_style $sha1 > "$MSG"
+		update_squash_message $squash_style $sha1
 		failed=f
 		author_script=$(get_author_ident_from_commit HEAD)
 		echo "$author_script" > "$AUTHOR_SCRIPT"
@@ -462,16 +467,16 @@ do_next () {
 		case "$(peek_next_command)" in
 		squash|s|fixup|f)
 			USE_OUTPUT=output
+			cp "$SQUASH_MSG" "$MSG" || exit
 			MSG_OPT=-F
 			EDIT_OR_FILE="$MSG"
-			cp "$MSG" "$SQUASH_MSG"
 			;;
 		*)
 			USE_OUTPUT=
 			MSG_OPT=
 			EDIT_OR_FILE=-e
-			rm -f "$SQUASH_MSG" || exit
-			cp "$MSG" "$GIT_DIR"/SQUASH_MSG
+			cp "$SQUASH_MSG" "$MSG" || exit
+			mv "$SQUASH_MSG" "$GIT_DIR"/SQUASH_MSG || exit
 			rm -f "$GIT_DIR"/MERGE_MSG || exit
 			;;
 		esac
-- 
1.6.6

[PATCH 16/18] rebase -i: For fixup commands without squashes, do not start editor

[Michael Haggerty]

If the "rebase -i" commands include a series of fixup commands without
any squash commands, then commit the combined commit using the commit
message of the corresponding "pick" without starting up the
commit-message editor.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh    |   81 +++++++++++++++++++++++++++--------------
 t/t3404-rebase-interactive.sh |    7 ++--
 2 files changed, 57 insertions(+), 31 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5a48fbf..c9390cd 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -66,6 +66,13 @@ MSG="$DOTEST"/message
 # updated.  It is deleted just before the combined commit is made.
 SQUASH_MSG="$DOTEST"/message-squash
 
+# If the current series of squash/fixups has not yet included a squash
+# command, then this file exists and holds the commit message of the
+# original "pick" commit.  (If the series ends without a "squash"
+# command, then this can be used as the commit message of the combined
+# commit without opening the editor.)
+FIXUP_MSG="$DOTEST"/message-fixup
+
 # $REWRITTEN is the name of a directory containing files for each
 # commit that is reachable by at least one merge base of $HEAD and
 # $UPSTREAM. They are not necessarily rewritten, but their children
@@ -360,7 +367,7 @@ nth_string () {
 	esac
 }
 
-update_squash_message () {
+update_squash_messages () {
 	if test -f "$SQUASH_MSG"; then
 		mv "$SQUASH_MSG" "$SQUASH_MSG".bak || exit
 		COUNT=$(($(sed -n \
@@ -373,16 +380,18 @@ update_squash_message () {
 			}' <"$SQUASH_MSG".bak
 		} >$SQUASH_MSG
 	else
+		commit_message HEAD > "$FIXUP_MSG" || die "Cannot write $FIXUP_MSG"
 		COUNT=2
 		{
 			echo "# This is a combination of 2 commits."
 			echo "# The first commit's message is:"
 			echo
-			commit_message HEAD
+			cat "$FIXUP_MSG"
 		} >$SQUASH_MSG
 	fi
 	case $1 in
 	squash)
+		rm -f "$FIXUP_MSG"
 		echo
 		echo "# This is the $(nth_string $COUNT) commit message:"
 		echo
@@ -457,7 +466,7 @@ do_next () {
 			die "Cannot '$squash_style' without a previous commit"
 
 		mark_action_done
-		update_squash_message $squash_style $sha1
+		update_squash_messages $squash_style $sha1
 		failed=f
 		author_script=$(get_author_ident_from_commit HEAD)
 		echo "$author_script" > "$AUTHOR_SCRIPT"
@@ -466,34 +475,52 @@ do_next () {
 		pick_one -n $sha1 || failed=t
 		case "$(peek_next_command)" in
 		squash|s|fixup|f)
-			USE_OUTPUT=output
-			cp "$SQUASH_MSG" "$MSG" || exit
-			MSG_OPT=-F
-			EDIT_OR_FILE="$MSG"
+			# This is an intermediate commit; its message will only be
+			# used in case of trouble.  So use the long version:
+			if test $failed = f
+			then
+				do_with_author output git commit --no-verify -F "$SQUASH_MSG" ||
+					failed=t
+			fi
+			if test $failed = t
+			then
+				cp "$SQUASH_MSG" "$MSG" || exit
+				# After any kind of hiccup, prevent committing without
+				# opening the commit message editor:
+				rm -f "$FIXUP_MSG"
+				cp "$MSG" "$GIT_DIR"/MERGE_MSG || exit
+				warn
+				warn "Could not apply $sha1... $rest"
+				die_with_patch $sha1 ""
+			fi
 			;;
 		*)
-			USE_OUTPUT=
-			MSG_OPT=
-			EDIT_OR_FILE=-e
-			cp "$SQUASH_MSG" "$MSG" || exit
-			mv "$SQUASH_MSG" "$GIT_DIR"/SQUASH_MSG || exit
-			rm -f "$GIT_DIR"/MERGE_MSG || exit
+			# This is the final command of this squash/fixup group
+			if test $failed = f
+			then
+				if test -f "$FIXUP_MSG"
+				then
+					do_with_author git commit --no-verify -F "$FIXUP_MSG" ||
+						failed=t
+				else
+					cp "$SQUASH_MSG" "$GIT_DIR"/SQUASH_MSG || exit
+					rm -f "$GIT_DIR"/MERGE_MSG
+					do_with_author git commit --no-verify -e ||
+						failed=t
+				fi
+			fi
+			rm -f "$FIXUP_MSG"
+			if test $failed = t
+			then
+				mv "$SQUASH_MSG" "$MSG" || exit
+				cp "$MSG" "$GIT_DIR"/MERGE_MSG || exit
+				warn
+				warn "Could not apply $sha1... $rest"
+				die_with_patch $sha1 ""
+			fi
+			rm -f "$SQUASH_MSG"
 			;;
 		esac
-		if test $failed = f
-		then
-			# This is like --amend, but with a different message
-			do_with_author $USE_OUTPUT git commit --no-verify \
-				$MSG_OPT "$EDIT_OR_FILE" ||
-				failed=t
-		fi
-		if test $failed = t
-		then
-			cp "$MSG" "$GIT_DIR"/MERGE_MSG
-			warn
-			warn "Could not apply $sha1... $rest"
-			die_with_patch $sha1 ""
-		fi
 		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 0511709..175a86c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -237,14 +237,13 @@ test_expect_success 'multi-squash only fires up editor once' '
 	test 1 = $(git show | grep ONCE | wc -l)
 '
 
-test_expect_success 'multi-fixup only fires up editor once' '
+test_expect_success 'multi-fixup does not fire up editor' '
 	git checkout -b multi-fixup E &&
 	base=$(git rev-parse HEAD~4) &&
-	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
-		EXPECT_HEADER_COUNT=4 \
+	FAKE_COMMIT_AMEND="NEVER" FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
 		git rebase -i $base &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 1 = $(git show | grep ONCE | wc -l) &&
+	test 0 = $(git show | grep NEVER | wc -l) &&
 	git checkout to-be-rebased &&
 	git branch -D multi-fixup
 '
-- 
1.6.6

[PATCH 17/18] t3404: Set up more of the test repo in the "setup" step

[Michael Haggerty]

...and reuse these pre-created branches in tests rather than creating
duplicates.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3404-rebase-interactive.sh |   51 +++++++++++++++++++++--------------------
 1 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 175a86c..a119ce0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -14,15 +14,20 @@ that the result still makes sense.
 
 set_fake_editor
 
-# set up two branches like this:
+# Set up the repository like this:
 #
-# A - B - C - D - E     (master)
+#     one - two - three - four (conflict-branch)
+#   /
+# A - B - C - D - E            (master)
+# | \
+# |   F - G - H                (branch1)
+# |     \
+#  \      I                    (branch2)
 #   \
-#     F - G - H         (branch1)
-#       \
-#         I             (branch2)
+#     J - K - L - M            (no-conflict-branch)
 #
-# where A, B, D and G touch the same file.
+# where A, B, D and G all touch file1, and one, two, three, four all
+# touch file "conflict".
 
 test_expect_success 'setup' '
 	test_commit A file1 &&
@@ -36,9 +41,20 @@ test_expect_success 'setup' '
 	test_commit H file5 &&
 	git checkout -b branch2 F &&
 	test_commit I file6
+	git checkout -b conflict-branch A &&
+	for n in one two three four
+	do
+		test_commit $n conflict
+	done &&
+	git checkout -b no-conflict-branch A &&
+	for n in J K L M
+	do
+		test_commit $n file$n
+	done
 '
 
 test_expect_success 'no changes are a nop' '
+	git checkout branch2 &&
 	git rebase -i F &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
 	test $(git rev-parse I) = $(git rev-parse HEAD)
@@ -97,7 +113,7 @@ cat > expect2 << EOF
 D
 =======
 G
->>>>>>> 91201e5... G
+>>>>>>> 51047de... G
 EOF
 
 test_expect_success 'stop on conflicting pick' '
@@ -293,12 +309,7 @@ test_expect_success 'squash ignores blank lines' '
 '
 
 test_expect_success 'squash works as expected' '
-	for n in one two three four
-	do
-		echo $n >> file$n &&
-		git add file$n &&
-		git commit -m $n
-	done &&
+	git checkout -b squash-works no-conflict-branch &&
 	one=$(git rev-parse HEAD~3) &&
 	FAKE_LINES="1 squash 3 2" EXPECT_HEADER_COUNT=2 \
 		git rebase -i HEAD~3 &&
@@ -306,12 +317,7 @@ test_expect_success 'squash works as expected' '
 '
 
 test_expect_success 'interrupted squash works as expected' '
-	for n in one two three four
-	do
-		echo $n >> conflict &&
-		git add conflict &&
-		git commit -m $n
-	done &&
+	git checkout -b interrupted-squash conflict-branch &&
 	one=$(git rev-parse HEAD~3) &&
 	(
 		FAKE_LINES="1 squash 3 2" &&
@@ -328,12 +334,7 @@ test_expect_success 'interrupted squash works as expected' '
 '
 
 test_expect_success 'interrupted squash works as expected (case 2)' '
-	for n in one two three four
-	do
-		echo $n >> conflict &&
-		git add conflict &&
-		git commit -m $n
-	done &&
+	git checkout -b interrupted-squash2 conflict-branch &&
 	one=$(git rev-parse HEAD~3) &&
 	(
 		FAKE_LINES="3 squash 1 2" &&
-- 
1.6.6

[PATCH 18/18] rebase -i: Retain user-edited commit messages after squash/fixup conflicts

[Michael Haggerty]

When a squash/fixup fails due to a conflict, the user is required to
edit the commit message.  Previously, if further squash/fixup commands
followed the conflicting squash/fixup, this user-edited message was
discarded and a new automatically-generated commit message was
suggested.

Change the handling of conflicts within squash/fixup command series:
Whenever the user is required to intervene, consider the resulting
commit to be a new basis for the following squash/fixups and use its
commit message in later suggested combined commit messages.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh    |   66 +++++++++++++++++------------------------
 t/t3404-rebase-interactive.sh |   36 ++++++++++++++++++++++
 2 files changed, 63 insertions(+), 39 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c9390cd..e551906 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -410,6 +410,21 @@ peek_next_command () {
 	sed -n -e "/^#/d" -e "/^$/d" -e "s/ .*//p" -e "q" < "$TODO"
 }
 
+# A squash/fixup has failed.  Prepare the long version of the squash
+# commit message, then die_with_patch.  This code path requires the
+# user to edit the combined commit message for all commits that have
+# been squashed/fixedup so far.  So also erase the old squash
+# messages, effectively causing the combined commit to be used as the
+# new basis for any further squash/fixups.  Args: sha1 rest
+die_failed_squash() {
+	mv "$SQUASH_MSG" "$MSG" || exit
+	rm -f "$FIXUP_MSG"
+	cp "$MSG" "$GIT_DIR"/MERGE_MSG || exit
+	warn
+	warn "Could not apply $1... $2"
+	die_with_patch $1 ""
+}
+
 do_next () {
 	rm -f "$MSG" "$AUTHOR_SCRIPT" "$AMEND" || exit
 	read command sha1 rest < "$TODO"
@@ -467,58 +482,31 @@ do_next () {
 
 		mark_action_done
 		update_squash_messages $squash_style $sha1
-		failed=f
 		author_script=$(get_author_ident_from_commit HEAD)
 		echo "$author_script" > "$AUTHOR_SCRIPT"
 		eval "$author_script"
 		output git reset --soft HEAD^
-		pick_one -n $sha1 || failed=t
+		pick_one -n $sha1 || die_failed_squash $sha1 "$rest"
 		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 test $failed = f
-			then
-				do_with_author output git commit --no-verify -F "$SQUASH_MSG" ||
-					failed=t
-			fi
-			if test $failed = t
-			then
-				cp "$SQUASH_MSG" "$MSG" || exit
-				# After any kind of hiccup, prevent committing without
-				# opening the commit message editor:
-				rm -f "$FIXUP_MSG"
-				cp "$MSG" "$GIT_DIR"/MERGE_MSG || exit
-				warn
-				warn "Could not apply $sha1... $rest"
-				die_with_patch $sha1 ""
-			fi
+			do_with_author output git commit --no-verify -F "$SQUASH_MSG" ||
+				die_failed_squash $sha1 "$rest"
 			;;
 		*)
 			# This is the final command of this squash/fixup group
-			if test $failed = f
+			if test -f "$FIXUP_MSG"
 			then
-				if test -f "$FIXUP_MSG"
-				then
-					do_with_author git commit --no-verify -F "$FIXUP_MSG" ||
-						failed=t
-				else
-					cp "$SQUASH_MSG" "$GIT_DIR"/SQUASH_MSG || exit
-					rm -f "$GIT_DIR"/MERGE_MSG
-					do_with_author git commit --no-verify -e ||
-						failed=t
-				fi
-			fi
-			rm -f "$FIXUP_MSG"
-			if test $failed = t
-			then
-				mv "$SQUASH_MSG" "$MSG" || exit
-				cp "$MSG" "$GIT_DIR"/MERGE_MSG || exit
-				warn
-				warn "Could not apply $sha1... $rest"
-				die_with_patch $sha1 ""
+				do_with_author git commit --no-verify -F "$FIXUP_MSG" ||
+					die_failed_squash $sha1 "$rest"
+			else
+				cp "$SQUASH_MSG" "$GIT_DIR"/SQUASH_MSG || exit
+				rm -f "$GIT_DIR"/MERGE_MSG
+				do_with_author git commit --no-verify -e ||
+					die_failed_squash $sha1 "$rest"
 			fi
-			rm -f "$SQUASH_MSG"
+			rm -f "$SQUASH_MSG" "$FIXUP_MSG"
 			;;
 		esac
 		;;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index a119ce0..4e35137 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -264,6 +264,42 @@ test_expect_success 'multi-fixup does not fire up editor' '
 	git branch -D multi-fixup
 '
 
+test_expect_success 'commit message used after conflict' '
+	git checkout -b conflict-fixup conflict-branch &&
+	base=$(git rev-parse HEAD~4) &&
+	(
+		FAKE_LINES="1 fixup 3 fixup 4" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i $base
+	) &&
+	echo three > conflict &&
+	git add conflict &&
+	FAKE_COMMIT_AMEND="ONCE" EXPECT_HEADER_COUNT=2 \
+		git rebase --continue &&
+	test $base = $(git rev-parse HEAD^) &&
+	test 1 = $(git show | grep ONCE | wc -l) &&
+	git checkout to-be-rebased &&
+	git branch -D conflict-fixup
+'
+
+test_expect_success 'commit message retained after conflict' '
+	git checkout -b conflict-squash conflict-branch &&
+	base=$(git rev-parse HEAD~4) &&
+	(
+		FAKE_LINES="1 fixup 3 squash 4" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i $base
+	) &&
+	echo three > conflict &&
+	git add conflict &&
+	FAKE_COMMIT_AMEND="TWICE" EXPECT_HEADER_COUNT=2 \
+		git rebase --continue &&
+	test $base = $(git rev-parse HEAD^) &&
+	test 2 = $(git show | grep TWICE | wc -l) &&
+	git checkout to-be-rebased &&
+	git branch -D conflict-squash
+'
+
 cat > expect-squash-fixup << EOF
 B
 
-- 
1.6.6

Re: [PATCH 09/18] t3404: Test the commit count in commit messages generated by "rebase -i"

[Johannes Sixt]

Michael Haggerty schrieb:
> +	test -z "$EXPECT_HEADER_COUNT" ||
> +		test "$EXPECT_HEADER_COUNT" = $(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1") ||
> +		exit

Is an empty $EXPECT_HEADER_COUNT an error? I suppose it isn't; therefore,
you must put the second 'test' and the 'exit' in a brace group.

You should also put the $(sed) in double-quotes.

-- Hannes

Re: [PATCH 10/18] rebase -i: Improve consistency of commit count in generated commit messages

[Johannes Sixt]

Michael Haggerty schrieb:
> Use the numeral "2" instead of the word "two" when two commits are
> being interactively squashed.  This makes the treatment consistent
> with that for higher numbers of commits.

Huh? This is just code churn: consistency is not an advantage here.

Oh...wait... The next patch needs it this way. Couldn't you have justified
it with "In a subsequent change, we will extract the numeral and use it in
an expression."

-- Hannes

Re: [PATCH 15/18] rebase -i: Change function make_squash_message into update_squash_message

[Johannes Sixt]

Michael Haggerty schrieb:
> Alter the file $SQUASH_MSG in place rather than outputting the new
> message then juggling it around.  Change the function name
> accordingly.

Code churn... Oh...wait... (You get the point ;)

-- Hannes

Re: [PATCH 09/18] t3404: Test the commit count in commit messages generated by "rebase -i"

[Michael Haggerty]

Johannes Sixt wrote:
> Michael Haggerty schrieb:
>> +	test -z "$EXPECT_HEADER_COUNT" ||
>> +		test "$EXPECT_HEADER_COUNT" = $(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1") ||
>> +		exit
> 
> Is an empty $EXPECT_HEADER_COUNT an error? I suppose it isn't; therefore,
> you must put the second 'test' and the 'exit' in a brace group.

True, an empty $EXPECT_HEADER_COUNT is not an error.  But I think the
form (A || B || C) is correct anyway, because if $EXPECT_HEADER_COUNT is
empty, then A is successful, and neither B nor C are executed, and
execution falls through to the later tests.

> You should also put the $(sed) in double-quotes.

Yes, thanks.  I'll include it in the next version.

Michael

Re: [PATCH 10/18] rebase -i: Improve consistency of commit count in generated commit messages

[Michael Haggerty]

Johannes Sixt wrote:
> Michael Haggerty schrieb:
>> Use the numeral "2" instead of the word "two" when two commits are
>> being interactively squashed.  This makes the treatment consistent
>> with that for higher numbers of commits.
> 
> Huh? This is just code churn: consistency is not an advantage here.
> 
> Oh...wait... The next patch needs it this way. Couldn't you have justified
> it with "In a subsequent change, we will extract the numeral and use it in
> an expression."

In the non-git universe, consistency in the user interface *is*
considered an advantage :-)  But I will be happy to add your suggested
*second* justification for the change in v2.

Michael

Re: [PATCH 09/18] t3404: Test the commit count in commit messages generated by "rebase -i"

[Johannes Sixt]

Michael Haggerty schrieb:
> True, an empty $EXPECT_HEADER_COUNT is not an error.  But I think the
> form (A || B || C) is correct anyway...

Very true. I haven't had my morning coffee, yet...

-- Hannes

Re: [PATCH 00/18] rebase -i: For pure fixups, do not start log message editor

[Junio C Hamano]

Very nicely done and the series was a pleasure to read, even though I
didn't read some of them very deeply and might have missed some subtle
details outside the context of the patch.

Thanks.

Re: [PATCH 01/18] rebase -i: Make the condition for an "if" more transparent

[Eric Blake]

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

>  	current_sha1=$(git rev-parse --verify HEAD)
> -	if test "$no_ff$current_sha1" = "$parent_sha1"; then
> +	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"

'test cond1 -a cond2' is not portable.  Use 'test cond1 && test cond2'.

-- 
Eric Blake

Re: [PATCH 01/18] rebase -i: Make the condition for an "if" more transparent

[Junio C Hamano]

Eric Blake <ebb9@byu.net> writes:

> Michael Haggerty <mhagger <at> alum.mit.edu> writes:
>
>>  	current_sha1=$(git rev-parse --verify HEAD)
>> -	if test "$no_ff$current_sha1" = "$parent_sha1"; then
>> +	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"
>
> 'test cond1 -a cond2' is not portable.  Use 'test cond1 && test cond2'.

I avoid "test -a/-o" myself without even thinking (I am from old-school),
but at the same time I thought the progress in the world made such caution
obsolescent.

Not so.  Even though POSIX.1 lists -a/-o as options to "test", they are
marked "Obsolescent XSI" ("Strictly Conforming POSIX Applications and
Strictly Conforming XSI Applications shall not use obsolescent features").

We may want [PATCH -01/18] to clean up the existing code first.  Even
outside git-rebase--interactive.sh there are quite a few of them.

    $ git grep -n -e 'test .* -[ao] ' -- '*.sh' | wc -l
    38

Re: [PATCH 01/18] rebase -i: Make the condition for an "if" more transparent

[Paolo Bonzini]

On 01/14/2010 06:42 PM, Junio C Hamano wrote:
> Eric Blake<ebb9@byu.net>  writes:
>
>> Michael Haggerty<mhagger<at>  alum.mit.edu>  writes:
>>
>>>   	current_sha1=$(git rev-parse --verify HEAD)
>>> -	if test "$no_ff$current_sha1" = "$parent_sha1"; then
>>> +	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"
>>
>> 'test cond1 -a cond2' is not portable.  Use 'test cond1&&  test cond2'.
>
> I avoid "test -a/-o" myself without even thinking (I am from old-school),
> but at the same time I thought the progress in the world made such caution
> obsolescent.
>
> Not so.  Even though POSIX.1 lists -a/-o as options to "test", they are
> marked "Obsolescent XSI" ("Strictly Conforming POSIX Applications and
> Strictly Conforming XSI Applications shall not use obsolescent features").

The reason for this is that the precedence rules were never well 
specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not 
portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b

but this doesn't

   $ test -z "$x" -a a = b
   bash: test: too many arguments

because it groups "test -n = -a" and is left with "a = b".

Similarly, if $x is "-f", these

   $ test "$x"
   $ test "$x" || test c = d

correctly adds an implicit "-n", but this fails:

   $ test "$x" -o c = d
   bash: test: too many arguments

If anybody cleans up git's usage of test -a/-o, feel free to cut'n'paste 
the above into the commit messages.

Paolo

Re: [PATCH 06/18] rebase -i: Document how temporary files are used

[Greg Price]

Michael Haggerty wrote:
> Add documentation, inferred by reverse-engineering, about how
> git-rebase--interactive.sh uses many of its temporary files.

Brilliant!  I like the 'fixup' command and look forward to using it,
but this is probably my favorite commit in the series.  I did the same
reverse-engineering a few months ago and didn't think to write it down
and save you having to do it.  Now you have saved the next person from
doing so.

This and your refactoring commits will cause me some annoyance as I
bring the rebase -i -p series up to date, but that just means I should
hurry up and get it in shape for merge rather than continue to have to
resolve conflicts.

Greg

Re: [PATCH 01/18] rebase -i: Make the condition for an "if" more transparent

[Johannes Schindelin]

Hi,

On Thu, 14 Jan 2010, Michael Haggerty wrote:

> @@ -166,7 +166,8 @@ pick_one () {
>  	parent_sha1=$(git rev-parse --verify $sha1^) ||
>  		die "Could not get the parent of $sha1"
>  	current_sha1=$(git rev-parse --verify HEAD)
> -	if test "$no_ff$current_sha1" = "$parent_sha1"; then
> +	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"

Rather use &&, right?

Ciao,
Dscho

Re: [PATCH 00/18] rebase -i: For pure fixups, do not start log message editor

[Johannes Schindelin]

Hi,

On Thu, 14 Jan 2010, Michael Haggerty wrote:

> This patch series is a successor to mh/rebase-fixup, which causes
> "rebase -i" to skip opening the log message editor when processing a
> block of "fixup" commands that does not include any "squash"es.

I knew why I kept this patch series as the last thing I would do before 
going offline: it is real elegantly done.

Ciao,
Dscho

Re: [PATCH 01/18] rebase -i: Make the condition for an "if" more transparent

[Michael Haggerty]

Johannes Schindelin wrote:
> On Thu, 14 Jan 2010, Michael Haggerty wrote:
> 
>> @@ -166,7 +166,8 @@ pick_one () {
>>  	parent_sha1=$(git rev-parse --verify $sha1^) ||
>>  		die "Could not get the parent of $sha1"
>>  	current_sha1=$(git rev-parse --verify HEAD)
>> -	if test "$no_ff$current_sha1" = "$parent_sha1"; then
>> +	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"
> 
> Rather use &&, right?

Yes, that mistake was caused by my own ignorance about the portability
issues.  This problem was already discussed [1] and the change has been
integrated into master.

Thanks,
Michael

[1] http://marc.info/?l=git&m=126344857120877&w=2

Re: [PATCH 01/18] rebase -i: Make the condition for an "if" more transparent

[Johannes Schindelin]

Hi,

On Tue, 26 Jan 2010, Michael Haggerty wrote:

> Johannes Schindelin wrote:
> > On Thu, 14 Jan 2010, Michael Haggerty wrote:
> > 
> >> @@ -166,7 +166,8 @@ pick_one () {
> >>  	parent_sha1=$(git rev-parse --verify $sha1^) ||
> >>  		die "Could not get the parent of $sha1"
> >>  	current_sha1=$(git rev-parse --verify HEAD)
> >> -	if test "$no_ff$current_sha1" = "$parent_sha1"; then
> >> +	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"
> > 
> > Rather use &&, right?
> 
> Yes, that mistake was caused by my own ignorance about the portability
> issues.  This problem was already discussed [1] and the change has been
> integrated into master.

Thanks for the clarification, and sorry for not reviewing your series 
before Junio applied it.

Ciao,
Dscho