Re: rebase --autosquash does not handle fixup! of fixup!

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

Re: rebase --autosquash does not handle fixup! of fixup!

From: Thomas Rast <hidden>
Date: 2016-06-15 22:57:41

Andrew Pimlott [off-list ref] writes:
git rebase -i --autosquash does not handle a fixup! of a fixup!, such as
the history:

    aaaaaaa fix nasty bug
    ...
    bbbbbbb fixup! fix nasty bug
    ...
    ccccccc fixup! fixup! fix nasty bug

--autosquash produces:

    pick aaaaaaa fix nasty bug
    fixup bbbbbbb fixup! fix nasty bug
    ...
    pick ccccccc fixup! fixup! fix nasty bug

This defeats the workflow I was hoping to use:

    git commit -m 'fix nasty bug'
    ...
    git commit --fixup :/nasty
    ...
    git commit --fixup :/nasty

The second :/nasty resolves to the previous fixup, not the initial
commit.  I could have made the regular expression more precise, but this
would be a hassle.

Would a change to support fixup! fixup! be considered?
Sure, why not.  You could start with something like the patch below
(untested).  If that happens to work, just add a test and a good commit
message.

diff --git i/git-rebase--interactive.sh w/git-rebase--interactive.sh
index f953d8d..798ae81 100644
--- i/git-rebase--interactive.sh
+++ w/git-rebase--interactive.sh
@@ -689,7 +689,17 @@ rearrange_squash () {
 		case "$message" in
 		"squash! "*|"fixup! "*)
 			action="${message%%!*}"
-			rest="${message#*! }"
+			rest=$message
+			while : ; do
+				case "$rest" in
+				"squash! "*|"fixup! "*)
+					rest="${rest#*! }"
+					;;
+				*)
+					break
+					;;
+				esac
+			done
 			echo "$sha1 $action $rest"
 			# if it's a single word, try to resolve to a full sha1 and
 			# emit a second copy. This allows us to match on both message

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH] rebase -i: fixup fixup! fixup!

From: Andrew Pimlott <hidden>
Date: 2016-06-15 22:57:45

Excerpts from Thomas Rast's message of Tue Jun 11 11:50:07 -0700 2013:
Andrew Pimlott [off-list ref] writes:
quoted
    git commit -m 'fix nasty bug'
    ...
    git commit --fixup :/nasty
    ...
    git commit --fixup :/nasty

The second :/nasty resolves to the previous fixup, not the initial
commit.  I could have made the regular expression more precise, but this
would be a hassle.

Would a change to support fixup! fixup! be considered?
Sure, why not.  You could start with something like the patch below
(untested).  If that happens to work, just add a test and a good commit
message.
It happened to work and I added a test.  But then it occurred to me that
it might have been better to fix commit --fixup/--squash to strip the
fixup! or squash! from the referenced commit in the first place.
Anyhow, below is my patch for --autosquash, but unles someone has an
objection to doing it in commit, I'll work on that.

Andrew

Ignore subsequent "fixup! " or "squash! " after the first.  Handy in case a
git commit --fixup/--squash referred to a previous fixup/squash instead of
the original commit, for example with :/msg.

Signed-off-by: Andrew Pimlott <redacted>
---
 Documentation/git-rebase.txt |    4 +++-
 git-rebase--interactive.sh   |   13 ++++++++++-
 t/t3415-rebase-autosquash.sh |   49 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index c84854a..725cf27 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -389,7 +389,9 @@ squash/fixup series.
 	the same ..., automatically modify the todo list of rebase -i
 	so that the commit marked for squashing comes right after the
 	commit to be modified, and change the action of the moved
-	commit from `pick` to `squash` (or `fixup`).
+	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
+	"fixup! " or "squash! " after the first, in case you referred to a
+	previous fixup/squash with `git commit --fixup/--squash`.
 +
 This option is only valid when the '--interactive' option is used.
 +
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..54ed4c3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -689,7 +689,18 @@ rearrange_squash () {
 		case "$message" in
 		"squash! "*|"fixup! "*)
 			action="${message%%!*}"
-			rest="${message#*! }"
+			rest=$message
+			# ignore any squash! or fixup! after the first
+			while : ; do
+				case "$rest" in
+				"squash! "*|"fixup! "*)
+					rest="${rest#*! }"
+					;;
+				*)
+					break
+					;;
+				esac
+			done
 			echo "$sha1 $action $rest"
 			# if it's a single word, try to resolve to a full sha1 and
 			# emit a second copy. This allows us to match on both message
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index a1e86c4..1a3f40a 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -193,4 +193,53 @@ test_expect_success 'use commit --squash' '
 	test_auto_commit_flags squash 2
 '
 
+test_auto_fixup_fixup () {
+	git reset --hard base &&
+	echo 1 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! first" &&
+	echo 2 >file1 &&
+	git add -u &&
+	test_tick &&
+	git commit -m "$1! $2! first" &&
+	git tag "final-$1-$2" &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^^ &&
+	git log --oneline >actual &&
+	test_pause &&
+	if [ "$1" = "fixup" ]; then
+		test_line_count = 3 actual
+	elif [ "$1" = "squash" ]; then
+		test_line_count = 4 actual
+	else
+		false
+	fi &&
+	git diff --exit-code "final-$1-$2" &&
+	test 2 = "$(git cat-file blob HEAD^:file1)" &&
+	if [ "$1" = "fixup" ]; then
+		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	elif [ "$1" = "squash" ]; then
+		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	else
+		false
+	fi
+}
+
+test_expect_success 'fixup! fixup!' '
+	test_auto_fixup_fixup fixup fixup
+'
+
+test_expect_success 'fixup! squash!' '
+	test_auto_fixup_fixup fixup squash
+'
+
+test_expect_success 'squash! squash!' '
+	test_auto_fixup_fixup squash squash
+'
+
+test_expect_success 'squash! fixup!' '
+	test_auto_fixup_fixup squash fixup
+'
+
 test_done
-- 
1.7.10.4

Re: [PATCH] rebase -i: fixup fixup! fixup!

From: Andrew Pimlott <hidden>
Date: 2016-06-15 22:57:45

Excerpts from Andrew Pimlott's message of Fri Jun 14 12:31:57 -0700 2013:
It happened to work and I added a test.  But then it occurred to me that
it might have been better to fix commit --fixup/--squash to strip the
fixup! or squash! from the referenced commit in the first place.
Anyhow, below is my patch for --autosquash, but unles someone has an
objection to doing it in commit, I'll work on that.
Here is a patch for commit.c that does this.

Andrew

When building the commit message for --fixup/--squash, ignore a leading
fixup! or squash! on the referenced commit.  Handy in case the user referred
to an earlier squash/fixup commit instead of the original commit, for
example with :/msg.

Signed-off-by: Andrew Pimlott <redacted>
---
 builtin/commit.c  |   18 ++++++++++++++----
 t/t7500-commit.sh |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1621dfc..370df88 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -601,8 +601,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			if (!c)
 				die(_("could not lookup commit %s"), squash_message);
 			ctx.output_encoding = get_commit_output_encoding();
-			format_commit_message(c, "squash! %s\n\n", &sb,
-					      &ctx);
+			format_commit_message(c, "%s\n\n", &sb, &ctx);
+			if (!prefixcmp(sb.buf, "fixup! ")) {
+				strbuf_remove(&sb, 0, strlen("fixup! "));
+			} else if (!prefixcmp(sb.buf, "squash! ")) {
+				strbuf_remove(&sb, 0, strlen("squash! "));
+			}
+			strbuf_insert(&sb, 0, "squash! ", strlen("squash! "));
 		}
 	}
 
@@ -634,8 +639,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (!commit)
 			die(_("could not lookup commit %s"), fixup_message);
 		ctx.output_encoding = get_commit_output_encoding();
-		format_commit_message(commit, "fixup! %s\n\n",
-				      &sb, &ctx);
+		format_commit_message(commit, "%s\n\n", &sb, &ctx);
+		if (!prefixcmp(sb.buf, "fixup! ")) {
+			strbuf_remove(&sb, 0, strlen("fixup! "));
+		} else if (!prefixcmp(sb.buf, "squash! ")) {
+			strbuf_remove(&sb, 0, strlen("squash! "));
+		}
+		strbuf_insert(&sb, 0, "fixup! ", strlen("fixup! "));
 		hook_arg1 = "message";
 	} else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0)
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 436b7b6..ecdf74a 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -320,4 +320,40 @@ test_expect_success 'invalid message options when using --fixup' '
 	test_must_fail git commit --fixup HEAD~1 -F log
 '
 
+test_expect_success 'commit --fixup of existing fixup' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --fixup HEAD~1 &&
+	echo "fourth content line" >>foo &&
+	git add foo
+	git commit --fixup HEAD &&
+	commit_msg_is "fixup! target message subject line"
+'
+
+test_expect_success 'commit --fixup of existing squash' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --squash HEAD~1 &&
+	echo "fourth content line" >>foo &&
+	git add foo
+	git commit --fixup HEAD &&
+	commit_msg_is "fixup! target message subject line"
+'
+
+test_expect_success 'commit --squash of existing squash' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --squash HEAD~1 &&
+	echo "fourth content line" >>foo &&
+	git add foo
+	git commit --squash HEAD &&
+	commit_msg_is "squash! target message subject linecommit message"
+'
+
+test_expect_success 'commit --squash of existing fixup' '
+	commit_for_rebase_autosquash_setup &&
+	git commit --fixup HEAD~1 &&
+	echo "fourth content line" >>foo &&
+	git add foo
+	git commit --squash HEAD &&
+	commit_msg_is "squash! target message subject linecommit message"
+'
+
 test_done
-- 
1.7.10.4
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help