threads / patch / 22217

patch, 18 partsrebase -i: For pure fixups, do not start log message editor

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

## tl;dr

34 messages between Jan 14, 2010 and Jan 26, 2010. Diffs are folded; open one to read it.

replies: 33people: 7as markdown or json

Michael Haggerty· Jan 14, 2010, 05:54 UTC · lore

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +2 −1

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

Eric Blake· Jan 14, 2010, 15:42 UTC · re: Michael Haggerty · lore

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

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

Junio C Hamano· Jan 14, 2010, 17:42 UTC · re: Eric Blake · lore

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

Eric Blake <ebb9@byu.net> writes:

Show 7 quoted lines

> 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

Paolo Bonzini· Jan 15, 2010, 07:20 UTC · re: Junio C Hamano · lore

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

On 01/14/2010 06:42 PM, Junio C Hamano wrote:

Show 17 quoted lines

> 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

Johannes Schindelin· Jan 25, 2010, 18:28 UTC · re: Michael Haggerty · lore

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

Hi,

On Thu, 14 Jan 2010, Michael Haggerty wrote:

Show 6 quoted lines

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

Michael Haggerty· Jan 26, 2010, 12:38 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:

Show 10 quoted lines

> 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

Johannes Schindelin· Jan 26, 2010, 13:11 UTC · re: Michael Haggerty · lore

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

Hi,

On Tue, 26 Jan 2010, Michael Haggerty wrote:

Show 15 quoted lines

> 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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +0 −1

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +1 −2

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +1 −1

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +4 −4

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +34 −7

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

Greg Price· Jan 25, 2010, 03:38 UTC · re: Michael Haggerty · lore

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

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +11 −6

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +12 −4

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to 2 files +12 −3

t/lib-rebase.sh, t/t3404-rebase-interactive.sh

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

Johannes Sixt· Jan 14, 2010, 07:44 UTC · re: Michael Haggerty · lore

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

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

Michael Haggerty· Jan 14, 2010, 08:59 UTC · re: Johannes Sixt · lore

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

Johannes Sixt wrote:

Show 7 quoted lines

> 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

Johannes Sixt· Jan 14, 2010, 09:16 UTC · re: Michael Haggerty · lore

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

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to 2 files +3 −3

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

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

Johannes Sixt· Jan 14, 2010, 07:54 UTC · re: Michael Haggerty · lore

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

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

Michael Haggerty· Jan 14, 2010, 09:05 UTC · re: Johannes Sixt · lore

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

Johannes Sixt wrote:

Show 10 quoted lines

> 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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +3 −8

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +10 −5

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +2 −2

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +15 −12

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to git-rebase--interactive.sh +20 −15

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

Johannes Sixt· Jan 14, 2010, 08:20 UTC · re: Michael Haggerty · lore

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

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to 2 files +57 −31

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to t/t3404-rebase-interactive.sh +26 −25

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

Michael Haggerty· Jan 14, 2010, 05:54 UTC · re: Michael Haggerty · lore

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

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

Show changes to 2 files +63 −39

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

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

Junio C Hamano· Jan 14, 2010, 09:17 UTC · re: Michael Haggerty · lore

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

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.

Johannes Schindelin· Jan 25, 2010, 18:38 UTC · re: Michael Haggerty · lore

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

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