[PATCHv2 0/2] Add a "fixup" command to "rebase --interactive"

5 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCHv2 0/2] Add a "fixup" command to "rebase --interactive"

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:47:50

Thanks, everybody, for all of the feedback.  This is the redone patch
series, which I think addresses all of your suggestions.

I would still like to implement "don't open the editor if there are
only fixups", but if it's OK I'll work on that in a separate patch
series when I have time.

Michael J Gruber wrote:
My first reaction to the subject was "Huh? What repository?". So I
suggest something like

t3404: Better document the original repository layout

as a more descriptive subject.
Good idea.  Done.

Johannes Schindelin wrote:
Alternatively, one could use the test_commit function, I guess. [...]
Yes, that's much easier.  Changed.  This makes the old first and
second patches reduce to a single one.

Johannes Schindelin wrote:
On Fri, 4 Dec 2009, Junio C Hamano wrote:
quoted
Michael Haggerty [off-list ref] writes:
quoted
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0bd3bf7..539413d 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -302,7 +302,7 @@ nth_string () {
 
 make_squash_message () {
 	if test -f "$SQUASH_MSG"; then
-		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
+		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
 			< "$SQUASH_MSG" | sed -ne '$p')+1))
This sed replacement worries me.  I don't have a time to check myself
today but do we use \(this\|or\|that\) alternates with our sed script
already elsewhere in the codebase (test scripts do not count)?

Otherwise this may suddenly be breaking a platform that has an
implementation of sed that may be substandard but so far has been
sufficient to work with git.
IIRC "|" was not correctly handled by BSD sed (used e.g. in MacOSX).

So maybe it would be best to just look for "commit message"?  I agree with
Michael that the regex should not be too loose.
Thanks for pointing this out.  I replaced the problematic part of the
regexp with "[snrt][tdh]", which isn't quite so selective but should
be portable.  (This is the same fix that Junio suggested.)

Björn Gustavsson wrote:
On Fri, Dec 4, 2009 at 3:36 PM, Michael Haggerty [off-list ref] wrote:
Nitpick: the recommended style is to leave out the full stop
at the end of the first line of the commit message.
Nit picked.

Junio C Hamano wrote:
IIRC, the end result of the bikeshedding for the name of the command was
won by Dscho's "fixup":

  http://thread.gmane.org/gmane.comp.version-control.git/127923/focus=121820
That's fine with me (the abbreviation is even the same :-) ).
Changed.

Michael

Michael Haggerty (2):
  t3404: Use test_commit to set up test repository
  Add a command "fixup" to rebase --interactive

 Documentation/git-rebase.txt  |   13 +++--
 git-rebase--interactive.sh    |   51 +++++++++++++++++----
 t/lib-rebase.sh               |    7 ++-
 t/t3404-rebase-interactive.sh |   96 +++++++++++++++++++++-------------------
 4 files changed, 103 insertions(+), 64 deletions(-)

[PATCHv2 2/2] Add a command "fixup" to rebase --interactive

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:47:50

The command is like "squash", except that it discards the commit message
of the corresponding commit.

Signed-off-by: Michael Haggerty <redacted>
---
 Documentation/git-rebase.txt  |   13 ++++++----
 git-rebase--interactive.sh    |   51 +++++++++++++++++++++++++++++++++--------
 t/lib-rebase.sh               |    7 +++--
 t/t3404-rebase-interactive.sh |   30 ++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 18 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ca5e1e8..9b648ec 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -382,9 +382,12 @@ If you just want to edit the commit message for a commit, replace the
 command "pick" with the command "reword".
 
 If you want to fold two or more commits into one, replace the command
-"pick" with "squash" for the second and subsequent commit.  If the
-commits had different authors, it will attribute the squashed commit to
-the author of the first commit.
+"pick" for the second and subsequent commits with "squash" or "fixup".
+If the commits had different authors, the folded commit will be
+attributed to the author of the first commit.  The suggested commit
+message for the folded commit is the concatenation of the commit
+messages of the first commit and of those with the "squash" command,
+but omits the commit messages of commits with the "fixup" command.
 
 'git-rebase' will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
@@ -512,8 +515,8 @@ Easy case: The changes are literally the same.::
 Hard case: The changes are not the same.::
 
 	This happens if the 'subsystem' rebase had conflicts, or used
-	`\--interactive` to omit, edit, or squash commits; or if the
-	upstream used one of `commit \--amend`, `reset`, or
+	`\--interactive` to omit, edit, squash, or fixup commits; or
+	if the upstream used one of `commit \--amend`, `reset`, or
 	`filter-branch`.
 
 
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0bd3bf7..a7de5ea 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -302,7 +302,13 @@ nth_string () {
 
 make_squash_message () {
 	if test -f "$SQUASH_MSG"; then
-		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
+		# We want to be careful about matching only the commit
+		# message comment lines generated by this function.
+		# But supposedly some sed versions don't handle "\|"
+		# correctly, so instead of "\(st\|nd\|rd\|th\)", use
+		# the less accurate "[snrt][tdh]" to match 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))
 		echo "# This is a combination of $COUNT commits."
 		sed -e 1d -e '2,/^./{
@@ -315,10 +321,26 @@ make_squash_message () {
 		echo
 		git cat-file commit HEAD | sed -e '1,/^$/d'
 	fi
-	echo
-	echo "# This is the $(nth_string $COUNT) commit message:"
-	echo
-	git cat-file commit $1 | sed -e '1,/^$/d'
+	case $1 in
+	squash)
+		echo
+		echo "# This is the $(nth_string $COUNT) commit message:"
+		echo
+		git cat-file commit $2 | sed -e '1,/^$/d'
+		;;
+	fixup)
+		echo
+		echo "# The $(nth_string $COUNT) commit message will be skipped:"
+		echo
+		# Comment the lines of the commit message out using
+		# "#" rather than "# " (a) to make them more distinct
+		# from the explanatory comments added by this function
+		# and (b) to make it less likely that the sed regexp
+		# above will be confused by a commented-out commit
+		# message.
+		git cat-file commit $2 | sed -e '1,/^$/d' -e 's/^/#/'
+		;;
+	esac
 }
 
 peek_next_command () {
@@ -367,20 +389,28 @@ do_next () {
 		warn
 		exit 0
 		;;
-	squash|s)
-		comment_for_reflog squash
+	squash|s|fixup|f)
+		case "$command" in
+		squash|s)
+			squash_style=squash
+			;;
+		fixup|f)
+			squash_style=fixup
+			;;
+		esac
+		comment_for_reflog $squash_style
 
 		test -f "$DONE" && has_action "$DONE" ||
-			die "Cannot 'squash' without a previous commit"
+			die "Cannot '$squash_style' without a previous commit"
 
 		mark_action_done
-		make_squash_message $sha1 > "$MSG"
+		make_squash_message $squash_style $sha1 > "$MSG"
 		failed=f
 		author_script=$(get_author_ident_from_commit HEAD)
 		output git reset --soft HEAD^
 		pick_one -n $sha1 || failed=t
 		case "$(peek_next_command)" in
-		squash|s)
+		squash|s|fixup|f)
 			USE_OUTPUT=output
 			MSG_OPT=-F
 			EDIT_OR_FILE="$MSG"
@@ -768,6 +798,7 @@ first and then run 'git rebase --continue' again."
 #  r, reword = use commit, but edit the commit message
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
+#  f, fixup = like "squash", but discard this commit's log message
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 62f452c..f4dda02 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -9,8 +9,9 @@
 #
 #	"[<lineno1>] [<lineno2>]..."
 #
-#   If a line number is prefixed with "squash", "edit", or "reword", the
-#   respective line's command will be replaced with the specified one.
+#   If a line number is prefixed with "squash", "fixup", "edit", or
+#   "reword", the respective line's command will be replaced with the
+#   specified one.
 
 set_fake_editor () {
 	echo "#!$SHELL_PATH" >fake-editor.sh
@@ -32,7 +33,7 @@ cat "$1".tmp
 action=pick
 for line in $FAKE_LINES; do
 	case $line in
-	squash|edit|reword)
+	squash|fixup|edit|reword)
 		action="$line";;
 	*)
 		echo sed -n "${line}s/^pick/$action/p"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 778daf4..ea26115 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -235,6 +235,36 @@ 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' '
+	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" \
+		git rebase -i $base &&
+	test $base = $(git rev-parse HEAD^) &&
+	test 1 = $(git show | grep ONCE | wc -l) &&
+	git checkout to-be-rebased &&
+	git branch -D multi-fixup
+'
+
+cat > expect-squash-fixup << EOF
+B
+
+D
+
+ONCE
+EOF
+
+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" \
+		git rebase -i $base &&
+	git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
+	test_cmp expect-squash-fixup actual-squash-fixup &&
+	git checkout to-be-rebased &&
+	git branch -D squash-fixup
+'
+
 test_expect_success 'squash works as expected' '
 	for n in one two three four
 	do
-- 
1.6.6.rc1.34.gd7b83

[PATCHv2 1/2] t3404: Use test_commit to set up test repository

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:47:50

Also adjust "expected" text to reflect the file contents generated by
test_commit, which are slightly different than those generated by the
old code.

Signed-off-by: Michael Haggerty <redacted>
---
 t/t3404-rebase-interactive.sh |   66 ++++++++++++----------------------------
 1 files changed, 20 insertions(+), 46 deletions(-)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3a37793..778daf4 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -16,53 +16,26 @@ set_fake_editor
 
 # set up two branches like this:
 #
-# A - B - C - D - E
+# A - B - C - D - E     (master)
 #   \
-#     F - G - H
+#     F - G - H         (branch1)
 #       \
-#         I
+#         I             (branch2)
 #
-# where B, D and G touch the same file.
+# where A, B, D and G touch the same file.
 
 test_expect_success 'setup' '
-	: > file1 &&
-	git add file1 &&
-	test_tick &&
-	git commit -m A &&
-	git tag A &&
-	echo 1 > file1 &&
-	test_tick &&
-	git commit -m B file1 &&
-	: > file2 &&
-	git add file2 &&
-	test_tick &&
-	git commit -m C &&
-	echo 2 > file1 &&
-	test_tick &&
-	git commit -m D file1 &&
-	: > file3 &&
-	git add file3 &&
-	test_tick &&
-	git commit -m E &&
+	test_commit A file1 &&
+	test_commit B file1 &&
+	test_commit C file2 &&
+	test_commit D file1 &&
+	test_commit E file3 &&
 	git checkout -b branch1 A &&
-	: > file4 &&
-	git add file4 &&
-	test_tick &&
-	git commit -m F &&
-	git tag F &&
-	echo 3 > file1 &&
-	test_tick &&
-	git commit -m G file1 &&
-	: > file5 &&
-	git add file5 &&
-	test_tick &&
-	git commit -m H &&
+	test_commit F file4 &&
+	test_commit G file1 &&
+	test_commit H file5 &&
 	git checkout -b branch2 F &&
-	: > file6 &&
-	git add file6 &&
-	test_tick &&
-	git commit -m I &&
-	git tag I
+	test_commit I file6
 '
 
 test_expect_success 'no changes are a nop' '
@@ -111,19 +84,20 @@ test_expect_success 'exchange two commits' '
 
 cat > expect << EOF
 diff --git a/file1 b/file1
-index e69de29..00750ed 100644
+index f70f10e..fd79235 100644
 --- a/file1
 +++ b/file1
-@@ -0,0 +1 @@
-+3
+@@ -1 +1 @@
+-A
++G
 EOF
 
 cat > expect2 << EOF
 <<<<<<< HEAD
-2
+D
 =======
-3
->>>>>>> b7ca976... G
+G
+>>>>>>> 91201e5... G
 EOF
 
 test_expect_success 'stop on conflicting pick' '
-- 
1.6.6.rc1.34.gd7b83

Re: [PATCHv2 2/2] Add a command "fixup" to rebase --interactive

From: Sverre Rabbelier <hidden>
Date: 2016-06-15 22:47:50

Heya,

On Mon, Dec 7, 2009 at 05:22, Michael Haggerty [off-list ref] wrote:
The command is like "squash", except that it discards the commit message
of the corresponding commit.
No no, wait, wasn't "fixup" supposed to let you just edit the commit
message of the commit you're fixing up? :(

-- 
Cheers,

Sverre Rabbelier

Re: [PATCHv2 2/2] Add a command "fixup" to rebase --interactive

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:47:50

Sverre Rabbelier wrote:
On Mon, Dec 7, 2009 at 05:22, Michael Haggerty [off-list ref] wrote:
quoted
The command is like "squash", except that it discards the commit message
of the corresponding commit.
No no, wait, wasn't "fixup" supposed to let you just edit the commit
message of the commit you're fixing up? :(
That command is called "reword".

Michael
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help