Merge branch 'pw/rebase-skip-commit-message-fix'

"git rebase -i" with a series of squash/fixup, when one of the
steps stopped in conflicts and ended up getting skipped, did not
handle the accumulated commit log messages, which has been
corrected.

* pw/rebase-skip-commit-message-fix:
  rebase --skip: fix commit message clean up when skipping squash

Author: gitster

sequencer.c

@@ -5048,19 +5048,31 @@ static int commit_staged_changes(struct repository *r,
 				 * We need to update the squash message to skip
 				 * the latest commit message.
 				 */
+				int res = 0;
 				struct commit *commit;
+				const char *msg;
 				const char *path = rebase_path_squash_msg();
 				const char *encoding = get_commit_output_encoding();
 
-				if (parse_head(r, &commit) ||
-				    !(p = repo_logmsg_reencode(r, commit, NULL, encoding)) ||
-				    write_message(p, strlen(p), path, 0)) {
-					repo_unuse_commit_buffer(r, commit, p);
-					return error(_("could not write file: "
+				if (parse_head(r, &commit))
+					return error(_("could not parse HEAD"));
+
+				p = repo_logmsg_reencode(r, commit, NULL, encoding);
+				if (!p)  {
+					res = error(_("could not parse commit %s"),
+						    oid_to_hex(&commit->object.oid));
+					goto unuse_commit_buffer;
+				}
+				find_commit_subject(p, &msg);
+				if (write_message(msg, strlen(msg), path, 0)) {
+					res = error(_("could not write file: "
 						       "'%s'"), path);
+					goto unuse_commit_buffer;
 				}
-				repo_unuse_commit_buffer(r,
-							 commit, p);
+			unuse_commit_buffer:
+				repo_unuse_commit_buffer(r, commit, p);
+				if (res)
+					return res;
 			}
 		}
 

t/t3418-rebase-continue.sh

@@ -115,15 +115,23 @@ test_expect_success '--skip after failed fixup cleans commit message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout -b with-conflicting-fixup &&
 	test_commit wants-fixup &&
-	test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
-	test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
-	test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
+	test_commit "fixup 1" wants-fixup.t 1 wants-fixup-1 &&
+	test_commit "fixup 2" wants-fixup.t 2 wants-fixup-2 &&
+	test_commit "fixup 3" wants-fixup.t 3 wants-fixup-3 &&
 	test_must_fail env FAKE_LINES="1 fixup 2 squash 4" \
 		git rebase -i HEAD~4 &&
 
 	: now there is a conflict, and comments in the commit message &&
-	git show HEAD >out &&
-	grep "fixup! wants-fixup" out &&
+	test_commit_message HEAD <<-\EOF &&
+	# This is a combination of 2 commits.
+	# This is the 1st commit message:
+
+	wants-fixup
+
+	# The commit message #2 will be skipped:
+
+	# fixup 1
+	EOF
 
 	: skip and continue &&
 	echo "cp \"\$1\" .git/copy.txt" | write_script copy-editor.sh &&
@@ -133,33 +141,49 @@ test_expect_success '--skip after failed fixup cleans commit message' '
 	test_path_is_missing .git/copy.txt &&
 
 	: now the comments in the commit message should have been cleaned up &&
-	git show HEAD >out &&
-	! grep "fixup! wants-fixup" out &&
+	test_commit_message HEAD -m wants-fixup &&
 
 	: now, let us ensure that "squash" is handled correctly &&
 	git reset --hard wants-fixup-3 &&
-	test_must_fail env FAKE_LINES="1 squash 4 squash 2 squash 4" \
+	test_must_fail env FAKE_LINES="1 squash 2 squash 1 squash 3 squash 1" \
 		git rebase -i HEAD~4 &&
 
-	: the first squash failed, but there are two more in the chain &&
+	: the second squash failed, but there are two more in the chain &&
 	(test_set_editor "$PWD/copy-editor.sh" &&
 	 test_must_fail git rebase --skip) &&
 
 	: not the final squash, no need to edit the commit message &&
 	test_path_is_missing .git/copy.txt &&
 
-	: The first squash was skipped, therefore: &&
-	git show HEAD >out &&
-	test_i18ngrep "# This is a combination of 2 commits" out &&
-	test_i18ngrep "# This is the commit message #2:" out &&
+	: The first and third squashes succeeded, therefore: &&
+	test_commit_message HEAD <<-\EOF &&
+	# This is a combination of 3 commits.
+	# This is the 1st commit message:
+
+	wants-fixup
+
+	# This is the commit message #2:
+
+	fixup 1
+
+	# This is the commit message #3:
+
+	fixup 2
+	EOF
 
 	(test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) &&
-	git show HEAD >out &&
-	test_i18ngrep ! "# This is a combination" out &&
+	test_commit_message HEAD <<-\EOF &&
+	wants-fixup
+
+	fixup 1
+
+	fixup 2
+	EOF
 
 	: Final squash failed, but there was still a squash &&
-	test_i18ngrep "# This is a combination of 2 commits" .git/copy.txt &&
-	test_i18ngrep "# This is the commit message #2:" .git/copy.txt
+	head -n1 .git/copy.txt >first-line &&
+	test_i18ngrep "# This is a combination of 3 commits" first-line &&
+	test_i18ngrep "# This is the commit message #3:" .git/copy.txt
 '
 
 test_expect_success 'setup rerere database' '

t/t3437-rebase-fixup-options.sh

@@ -21,21 +21,6 @@ TEST_PASSES_SANITIZE_LEAK=true
 
 EMPTY=""
 
-# test_commit_message <rev> -m <msg>
-# test_commit_message <rev> <path>
-# Verify that the commit message of <rev> matches
-# <msg> or the content of <path>.
-test_commit_message () {
-	git show --no-patch --pretty=format:%B "$1" >actual &&
-	case "$2" in
-	-m)
-		echo "$3" >expect &&
-		test_cmp expect actual ;;
-	*)
-		test_cmp "$2" actual ;;
-	esac
-}
-
 get_author () {
 	rev="$1" &&
 	git log -1 --pretty=format:"%an %ae %at" "$rev"

t/test-lib-functions.sh

@@ -1291,6 +1291,39 @@ test_cmp_rev () {
 	fi
 }
 
+# Tests that a commit message matches the expected text
+#
+# Usage: test_commit_message <rev> [-m <msg> | <file>]
+#
+# When using "-m" <msg> will have a line feed appended. If the second
+# argument is omitted then the expected message is read from stdin.
+
+test_commit_message () {
+	local msg_file=expect.msg
+
+	case $# in
+	3)
+		if test "$2" = "-m"
+		then
+			printf "%s\n" "$3" >"$msg_file"
+		else
+			BUG "Usage: test_commit_message <rev> [-m <message> | <file>]"
+		fi
+		;;
+	2)
+		msg_file="$2"
+		;;
+	1)
+		cat >"$msg_file"
+		;;
+	*)
+		BUG "Usage: test_commit_message <rev> [-m <message> | <file>]"
+		;;
+	esac
+	git show --no-patch --pretty=format:%B "$1" -- >actual.msg &&
+	test_cmp "$msg_file" actual.msg
+}
+
 # Compare paths respecting core.ignoreCase
 test_cmp_fspath () {
 	if test "x$1" = "x$2"