[PATCH] rebase: add --allow-empty-message option

Subsystems: documentation, the rest

STALE3126d

4 messages, 2 authors, 2018-02-06 · open the first message on its own page

[PATCH] rebase: add --allow-empty-message option

From: Genki Sky <hidden>
Date: 2018-02-02 06:29:12 

--> Motivation

commit 4bee95847 ("cherry-pick: add --allow-empty-message option", 2012-08-02)
started doing this work, but it was never completed. For more discussion
on why this approach was chosen, see the thread beginning here:

  https://public-inbox.org/git/20120801111658.GA21272@arachsys.com/

https://stackoverflow.com/q/8542304 also shows this as a desirable
feature, and that the workaround is non-trivial to get right.

--> Implementation

Add a new --allow-empty-message flag. Propagate it to all calls of 'git
commit', 'git cherry-pick', and 'git rebase--helper' within the rebase
scripts.

Signed-off-by: Genki Sky <redacted>
---
 Documentation/git-rebase.txt    |  5 +++
 builtin/rebase--helper.c        |  2 ++
 git-rebase--am.sh               |  1 +
 git-rebase--interactive.sh      | 25 +++++++------
 git-rebase--merge.sh            |  3 +-
 git-rebase.sh                   |  5 +++
 t/t3430-rebase-empty-message.sh | 79 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 109 insertions(+), 11 deletions(-)
 create mode 100755 t/t3430-rebase-empty-message.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8a861c1e0..d713951b8 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -244,6 +244,11 @@ leave out at most one of A and B, in which case it defaults to HEAD.
 	Keep the commits that do not change anything from its
 	parents in the result.

+--allow-empty-message::
+	By default, rebasing commits with an empty message will fail.
+	This option overrides that behavior, allowing commits with empty
+	messages to be rebased.
+
 --skip::
 	Restart the rebasing process by skipping the current patch.

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7daee544b..2090114e9 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -22,6 +22,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
 		OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
+		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
+			N_("allow commits with empty messages")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 14c50782e..0f7805179 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -46,6 +46,7 @@ then
 	# makes this easy
 	git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
 		$allow_rerere_autoupdate --right-only "$revisions" \
+		$allow_empty_message \
 		${restrict_revision+^$restrict_revision}
 	ret=$?
 else
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d47bd2959..81c5b4287 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -281,7 +281,7 @@ pick_one () {

 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output eval git cherry-pick $allow_rerere_autoupdate \
+	output eval git cherry-pick $allow_rerere_autoupdate $allow_empty_message \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 			"$strategy_args" $empty_args $ff "$@"

@@ -406,6 +406,7 @@ pick_one_preserving_merges () {
 			;;
 		*)
 			output eval git cherry-pick $allow_rerere_autoupdate \
+				$allow_empty_message \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 				"$strategy_args" "$@" ||
 				die_with_patch $sha1 "$(eval_gettext "Could not pick \$sha1")"
@@ -559,7 +560,8 @@ do_next () {

 		mark_action_done
 		do_pick $sha1 "$rest"
-		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
+		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} \
+			$allow_empty_message || {
 			warn "$(eval_gettext "\
 Could not amend commit after successfully picking \$sha1... \$rest
 This is most likely due to an empty commit message, or the pre-commit hook
@@ -607,7 +609,7 @@ you are able to reword the commit.")"
 			# This is an intermediate commit; its message will only be
 			# used in case of trouble.  So use the long version:
 			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
-				${gpg_sign_opt:+"$gpg_sign_opt"} ||
+				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 				die_failed_squash $sha1 "$rest"
 			;;
 		*)
@@ -615,13 +617,13 @@ you are able to reword the commit.")"
 			if test -f "$fixup_msg"
 			then
 				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
-					${gpg_sign_opt:+"$gpg_sign_opt"} ||
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 					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 --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
-					${gpg_sign_opt:+"$gpg_sign_opt"} ||
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 					die_failed_squash $sha1 "$rest"
 			fi
 			rm -f "$squash_msg" "$fixup_msg"
@@ -754,7 +756,8 @@ case "$action" in
 continue)
 	if test ! -d "$rewritten"
 	then
-		exec git rebase--helper ${force_rebase:+--no-ff} --continue
+		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+			--continue
 	fi
 	# do we have anything to commit?
 	if git diff-index --cached --quiet HEAD --
@@ -794,11 +797,11 @@ In both cases, once you're done, continue with:
 You have uncommitted changes in your working tree. Please commit them
 first and then run 'git rebase --continue' again.")"
 			do_with_author git commit --amend --no-verify -F "$msg" -e \
-				${gpg_sign_opt:+"$gpg_sign_opt"} ||
+				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 				die "$(gettext "Could not commit staged changes.")"
 		else
 			do_with_author git commit --no-verify -F "$msg" -e \
-				${gpg_sign_opt:+"$gpg_sign_opt"} ||
+				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 				die "$(gettext "Could not commit staged changes.")"
 		fi
 	fi
@@ -817,7 +820,8 @@ skip)

 	if test ! -d "$rewritten"
 	then
-		exec git rebase--helper ${force_rebase:+--no-ff} --continue
+		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+			--continue
 	fi
 	do_rest
 	return 0
@@ -1016,7 +1020,8 @@ checkout_onto
 if test -z "$rebase_root" && test ! -d "$rewritten"
 then
 	require_clean_work_tree "rebase"
-	exec git rebase--helper ${force_rebase:+--no-ff} --continue
+	exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+		--continue
 fi
 do_rest

diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index 06a4723d4..1831e589b 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -27,7 +27,8 @@ continue_merge () {
 	cmt=$(cat "$state_dir/current")
 	if ! git diff-index --quiet --ignore-submodules HEAD --
 	then
-		if ! git commit ${gpg_sign_opt:+"$gpg_sign_opt"} --no-verify -C "$cmt"
+		if ! git commit ${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message \
+			--no-verify -C "$cmt"
 		then
 			echo "Commit failed, please do not call \"git commit\""
 			echo "directly, but instead do one of the following: "
diff --git a/git-rebase.sh b/git-rebase.sh
index fd72a35c6..b353c33d4 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -24,6 +24,7 @@ m,merge!           use merging strategies to rebase
 i,interactive!     let the user edit the list of commits to rebase
 x,exec=!           add exec lines after each commit of the editable list
 k,keep-empty	   preserve empty commits during rebase
+allow-empty-message allow rebasing commits with empty messages
 f,force-rebase!    force rebase even if branch is up to date
 X,strategy-option=! pass the argument through to the merge strategy
 stat!              display a diffstat of what changed upstream
@@ -89,6 +90,7 @@ action=
 preserve_merges=
 autosquash=
 keep_empty=
+allow_empty_message=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
@@ -262,6 +264,9 @@ do
 	--keep-empty)
 		keep_empty=yes
 		;;
+	--allow-empty-message)
+		allow_empty_message=--allow-empty-message
+		;;
 	--preserve-merges)
 		preserve_merges=t
 		test -z "$interactive_rebase" && interactive_rebase=implied
diff --git a/t/t3430-rebase-empty-message.sh b/t/t3430-rebase-empty-message.sh
new file mode 100755
index 000000000..74af73f3c
--- /dev/null
+++ b/t/t3430-rebase-empty-message.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+
+test_description='rebase: option to allow empty commit messages'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success 'setup: three regular commits' '
+	test_commit A &&
+	test_commit B &&
+	test_commit C
+'
+
+test_expect_success 'rebase -i "reword" should fail to create an empty commit message' '
+	set_fake_editor &&
+	test_must_fail env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="1 reword 2" \
+		git rebase -i HEAD~2
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	set_fake_editor &&
+	FAKE_COMMIT_MESSAGE=" " FAKE_LINES="1 reword 2" \
+		git rebase -i --allow-empty-message HEAD~2
+'
+
+test_expect_success 'rebase -i "fixup" should fail to fixup an empty commit message' '
+	test_commit D &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i HEAD~2
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	FAKE_LINES="1 fixup 2" git rebase -i --allow-empty-message HEAD~2
+'
+
+# The test expects the following rebase to fail. It will only fail if it
+# actually has to cmd_commit() a new [empty message] commit. However, this
+# rebase makes no actual changes. So if the date does not change in time, it is
+# possible for it to simply take the original commits exactly as they are.
+# So, we test_tick() just to be safe.
+test_expect_success 'rebase --root should fail on an empty commit message' '
+	test_tick &&
+	test_must_fail git rebase --root
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	git rebase --root --allow-empty-message
+'
+
+test_expect_success 'setup: multiple branches' '
+	git checkout -b branch-keep-empty HEAD^1 &&
+	echo E >E &&
+	git add E &&
+	git commit --allow-empty-message -m "" &&
+	git branch branch-merge
+'
+
+test_expect_success 'rebase --keep-empty should fail on an empty commit message' '
+	test_must_fail git rebase --keep-empty master branch-keep-empty
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git cherry-pick --abort &&
+	git rebase --keep-empty --allow-empty-message master branch-keep-empty
+'
+
+test_expect_success 'rebase -m should fail on an empty commit message' '
+	test_must_fail git rebase -m master branch-merge
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	git rebase -m --allow-empty-message master branch-merge
+'
+
+test_done
--
2.16.1.73.g13f648520

Re: [PATCH] rebase: add --allow-empty-message option

From: Johannes Schindelin <hidden>
Date: 2018-02-02 20:54:13 

Hi,

On Fri, 2 Feb 2018, Genki Sky wrote:

--> Motivation
I won't comment on the motivation (leaving that up to the Git maintainer),
but on the patch.

The patch on the shell scripts and the C code looks straight-forward
enough, if a little repetitive (but that is hardly your fault).
quoted hunk
diff --git a/t/t3430-rebase-empty-message.sh b/t/t3430-rebase-empty-message.sh
new file mode 100755
index 000000000..74af73f3c
--- /dev/null
+++ b/t/t3430-rebase-empty-message.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+
+test_description='rebase: option to allow empty commit messages'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success 'setup: three regular commits' '
+	test_commit A &&
+	test_commit B &&
+	test_commit C
+'
None of these commits have empty commit messages, which is a little
curious for a 'setup' test case.

+test_expect_success 'rebase -i "reword" should fail to create an empty commit message' '
+	set_fake_editor &&
+	test_must_fail env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="1 reword 2" \
+		git rebase -i HEAD~2
+'
Why not make this more focused via

	... FAKE_LINES="reword 1" git rebase -i HEAD^

The effect will be the same because the first pick will be skipped as an
unnecessary pick anyway.

+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
This should be part of the previous test case:

	test_when_finished "test_might_fail git rebase --abort" &&

Also, I think this test case should be folded into the previous test case
(which would make that test_when_finished suggestion moot).

+	set_fake_editor &&
+	FAKE_COMMIT_MESSAGE=" " FAKE_LINES="1 reword 2" \
+		git rebase -i --allow-empty-message HEAD~2
+'
+
+test_expect_success 'rebase -i "fixup" should fail to fixup an empty commit message' '
+	test_commit D &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i HEAD~2
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	FAKE_LINES="1 fixup 2" git rebase -i --allow-empty-message HEAD~2
+'
+
+# The test expects the following rebase to fail. It will only fail if it
+# actually has to cmd_commit() a new [empty message] commit. However, this
+# rebase makes no actual changes.
Don't you want to use `--force-rebase` here?

So if the date does not change in time, it is
+# possible for it to simply take the original commits exactly as they are.
+# So, we test_tick() just to be safe.
+test_expect_success 'rebase --root should fail on an empty commit message' '
+	test_tick &&
+	test_must_fail git rebase --root
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	git rebase --root --allow-empty-message
+'
+
+test_expect_success 'setup: multiple branches' '
+	git checkout -b branch-keep-empty HEAD^1 &&
+	echo E >E &&
+	git add E &&
+	git commit --allow-empty-message -m "" &&
+	git branch branch-merge
+'
+
+test_expect_success 'rebase --keep-empty should fail on an empty commit message' '
+	test_must_fail git rebase --keep-empty master branch-keep-empty
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git cherry-pick --abort &&
+	git rebase --keep-empty --allow-empty-message master branch-keep-empty
+'
I do not really see why we have to test --keep-empty here. The code paths
overlap with what was tested previously.

+test_expect_success 'rebase -m should fail on an empty commit message' '
+	test_must_fail git rebase -m master branch-merge
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	git rebase -m --allow-empty-message master branch-merge
+'
+
+test_done
In general, I would much rather fold the test cases that verify the
behavior with --allow-empty-message into the same test case as verifying
the behavior without --allow-empty-message.

One of my aims in the Git project is to avoid test bloat. I know that
several other active contributors could not care less, and even the Git
maintainer seems to be oblivious to the danger of making a test suite so
unsuitable (both in terms of run-time and in terms of being able to
understand regressions) as to make it less useful. I certainly had painful
discussions on this very mailing list about that, and I still don't feel
heard nor understood.

While your patch certainly is clear enough to make it really easy to
understand regressions, I find that it errs on the side of over-testing.

And this is not an academic consideration. The test suite takes an insane
70-90 minutes *on a fast* Windows machine, even skipping all of the
git-svn tests. That's insane. (And the fact that Git is optimized for
Linux and runs the test suite much faster there is only a very feeble
excuse, if you want my opinion.)

The reason? It's death by a thousand only partially necessary tests.

In that light, I would really like to make at least new tests a lot more
focused, and I would really like it if newly-added tests would be
considerate of the time they take to run, and not only on Linux.

Thank you,
Johannes

[PATCH v2] rebase: add --allow-empty-message option

From: Genki Sky <hidden>
Date: 2018-02-04 20:08:21 

This option allows commits with empty commit messages to be rebased,
matching the same option in git-commit and git-cherry-pick. While empty
log messages are frowned upon, sometimes one finds them in older
repositories (e.g. translated from another VCS [0]), or have other
reasons for desiring them. The option is available in git-commit and
git-cherry-pick, so it is natural to make other git tools play nicely
with them. Adding this as an option allows the default to be "give the
user a chance to fix", while not interrupting the user's workflow
otherwise [1].

  [0]: https://stackoverflow.com/q/8542304
  [1]: https://public-inbox.org/git/7vd33afqjh.fsf@alter.siamese.dyndns.org/

To implement this, add a new --allow-empty-message flag. Then propagate
it to all calls of 'git commit', 'git cherry-pick', and 'git rebase--helper'
within the rebase scripts.

Signed-off-by: Genki Sky <redacted>
---

Notes:

  Thanks for the initial feedback, here are the changes from [v1]:
  - Made my commit message include the main motivations inline.
  - Moved new tests to t/t3405-rebase-malformed.sh, which has the
    relevant test description: "rebase should handle arbitrary git
    message".
  - Accordingly re-used existing test setup.
  - Minimized tests to just one for git-rebase--interactive.sh and one
    for git-rebase--merge.sh. First, one test per file keeps things
    focused while getting most benefit (other code within same file is
    likely to be noticed by modifiers). And, while git-rebase--am.sh
    does have one cherry-pick, it is only for a special case with
    --keep-empty. So git-rebase--am.sh otherwise doesn't have this
    empty-message issue.

  In general, there was a concern of over-testing, and over-checking
  implementation details. So, this time, I erred on the conservative
  side.

  [v1]: https://public-inbox.org/git/9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is/t/

 Documentation/git-rebase.txt |  5 +++++
 builtin/rebase--helper.c     |  2 ++
 git-rebase--am.sh            |  1 +
 git-rebase--interactive.sh   | 25 +++++++++++++++----------
 git-rebase--merge.sh         |  3 ++-
 git-rebase.sh                |  5 +++++
 t/t3405-rebase-malformed.sh  | 23 +++++++++++++++++++++++
 7 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8a861c1e0..d713951b8 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -244,6 +244,11 @@ leave out at most one of A and B, in which case it defaults to HEAD.
 	Keep the commits that do not change anything from its
 	parents in the result.

+--allow-empty-message::
+	By default, rebasing commits with an empty message will fail.
+	This option overrides that behavior, allowing commits with empty
+	messages to be rebased.
+
 --skip::
 	Restart the rebasing process by skipping the current patch.

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7daee544b..2090114e9 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -22,6 +22,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
 		OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
+		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
+			N_("allow commits with empty messages")),
 		OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
 				CONTINUE),
 		OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 14c50782e..0f7805179 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -46,6 +46,7 @@ then
 	# makes this easy
 	git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
 		$allow_rerere_autoupdate --right-only "$revisions" \
+		$allow_empty_message \
 		${restrict_revision+^$restrict_revision}
 	ret=$?
 else
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d47bd2959..81c5b4287 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -281,7 +281,7 @@ pick_one () {

 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output eval git cherry-pick $allow_rerere_autoupdate \
+	output eval git cherry-pick $allow_rerere_autoupdate $allow_empty_message \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 			"$strategy_args" $empty_args $ff "$@"

@@ -406,6 +406,7 @@ pick_one_preserving_merges () {
 			;;
 		*)
 			output eval git cherry-pick $allow_rerere_autoupdate \
+				$allow_empty_message \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 				"$strategy_args" "$@" ||
 				die_with_patch $sha1 "$(eval_gettext "Could not pick \$sha1")"
@@ -559,7 +560,8 @@ do_next () {

 		mark_action_done
 		do_pick $sha1 "$rest"
-		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
+		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} \
+			$allow_empty_message || {
 			warn "$(eval_gettext "\
 Could not amend commit after successfully picking \$sha1... \$rest
 This is most likely due to an empty commit message, or the pre-commit hook
@@ -607,7 +609,7 @@ you are able to reword the commit.")"
 			# This is an intermediate commit; its message will only be
 			# used in case of trouble.  So use the long version:
 			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
-				${gpg_sign_opt:+"$gpg_sign_opt"} ||
+				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 				die_failed_squash $sha1 "$rest"
 			;;
 		*)
@@ -615,13 +617,13 @@ you are able to reword the commit.")"
 			if test -f "$fixup_msg"
 			then
 				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
-					${gpg_sign_opt:+"$gpg_sign_opt"} ||
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 					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 --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
-					${gpg_sign_opt:+"$gpg_sign_opt"} ||
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 					die_failed_squash $sha1 "$rest"
 			fi
 			rm -f "$squash_msg" "$fixup_msg"
@@ -754,7 +756,8 @@ case "$action" in
 continue)
 	if test ! -d "$rewritten"
 	then
-		exec git rebase--helper ${force_rebase:+--no-ff} --continue
+		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+			--continue
 	fi
 	# do we have anything to commit?
 	if git diff-index --cached --quiet HEAD --
@@ -794,11 +797,11 @@ In both cases, once you're done, continue with:
 You have uncommitted changes in your working tree. Please commit them
 first and then run 'git rebase --continue' again.")"
 			do_with_author git commit --amend --no-verify -F "$msg" -e \
-				${gpg_sign_opt:+"$gpg_sign_opt"} ||
+				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 				die "$(gettext "Could not commit staged changes.")"
 		else
 			do_with_author git commit --no-verify -F "$msg" -e \
-				${gpg_sign_opt:+"$gpg_sign_opt"} ||
+				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
 				die "$(gettext "Could not commit staged changes.")"
 		fi
 	fi
@@ -817,7 +820,8 @@ skip)

 	if test ! -d "$rewritten"
 	then
-		exec git rebase--helper ${force_rebase:+--no-ff} --continue
+		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+			--continue
 	fi
 	do_rest
 	return 0
@@ -1016,7 +1020,8 @@ checkout_onto
 if test -z "$rebase_root" && test ! -d "$rewritten"
 then
 	require_clean_work_tree "rebase"
-	exec git rebase--helper ${force_rebase:+--no-ff} --continue
+	exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+		--continue
 fi
 do_rest

diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index 06a4723d4..1831e589b 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -27,7 +27,8 @@ continue_merge () {
 	cmt=$(cat "$state_dir/current")
 	if ! git diff-index --quiet --ignore-submodules HEAD --
 	then
-		if ! git commit ${gpg_sign_opt:+"$gpg_sign_opt"} --no-verify -C "$cmt"
+		if ! git commit ${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message \
+			--no-verify -C "$cmt"
 		then
 			echo "Commit failed, please do not call \"git commit\""
 			echo "directly, but instead do one of the following: "
diff --git a/git-rebase.sh b/git-rebase.sh
index fd72a35c6..b353c33d4 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -24,6 +24,7 @@ m,merge!           use merging strategies to rebase
 i,interactive!     let the user edit the list of commits to rebase
 x,exec=!           add exec lines after each commit of the editable list
 k,keep-empty	   preserve empty commits during rebase
+allow-empty-message allow rebasing commits with empty messages
 f,force-rebase!    force rebase even if branch is up to date
 X,strategy-option=! pass the argument through to the merge strategy
 stat!              display a diffstat of what changed upstream
@@ -89,6 +90,7 @@ action=
 preserve_merges=
 autosquash=
 keep_empty=
+allow_empty_message=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
@@ -262,6 +264,9 @@ do
 	--keep-empty)
 		keep_empty=yes
 		;;
+	--allow-empty-message)
+		allow_empty_message=--allow-empty-message
+		;;
 	--preserve-merges)
 		preserve_merges=t
 		test -z "$interactive_rebase" && interactive_rebase=implied
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
index ff8c360cd..cb7c6de84 100755
--- a/t/t3405-rebase-malformed.sh
+++ b/t/t3405-rebase-malformed.sh
@@ -3,6 +3,7 @@
 test_description='rebase should handle arbitrary git message'

 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh

 cat >F <<\EOF
 This is an example of a commit log message
@@ -25,6 +26,7 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m "Initial commit" &&
 	git branch diff-in-message &&
+	git branch empty-message-merge &&

 	git checkout -b multi-line-subject &&
 	cat F >file2 &&
@@ -45,6 +47,11 @@ test_expect_success setup '

 	git cat-file commit HEAD | sed -e "1,/^\$/d" >G0 &&

+	git checkout empty-message-merge &&
+	echo file3 >file3 &&
+	git add file3 &&
+	git commit --allow-empty-message -m "" &&
+
 	git checkout master &&

 	echo One >file1 &&
@@ -69,4 +76,20 @@ test_expect_success 'rebase commit with diff in message' '
 	test_cmp G G0
 '

+test_expect_success 'rebase -m commit with empty message' '
+	test_must_fail git rebase -m master empty-message-merge &&
+	git rebase --abort &&
+	git rebase -m --allow-empty-message master empty-message-merge
+'
+
+test_expect_success 'rebase -i commit with empty message' '
+	git checkout diff-in-message &&
+	set_fake_editor &&
+	test_must_fail env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
+		git rebase -i HEAD^ &&
+	git rebase --abort &&
+	FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
+		git rebase -i --allow-empty-message HEAD^
+'
+
 test_done
--
2.16.1.73.g3d9791bdf

Re: [PATCH v2] rebase: add --allow-empty-message option

From: Johannes Schindelin <hidden>
Date: 2018-02-06 14:54:00 

Hi Genki,

On Sun, 4 Feb 2018, Genki Sky wrote:

This option allows commits with empty commit messages to be rebased,
matching the same option in git-commit and git-cherry-pick. While empty
log messages are frowned upon, sometimes one finds them in older
repositories (e.g. translated from another VCS [0]), or have other
reasons for desiring them. The option is available in git-commit and
git-cherry-pick, so it is natural to make other git tools play nicely
with them. Adding this as an option allows the default to be "give the
user a chance to fix", while not interrupting the user's workflow
otherwise [1].

  [0]: https://stackoverflow.com/q/8542304
  [1]: https://public-inbox.org/git/7vd33afqjh.fsf@alter.siamese.dyndns.org/

To implement this, add a new --allow-empty-message flag. Then propagate
it to all calls of 'git commit', 'git cherry-pick', and 'git rebase--helper'
within the rebase scripts.

Signed-off-by: Genki Sky <redacted>
---

Notes:

  Thanks for the initial feedback, here are the changes from [v1]:
  - Made my commit message include the main motivations inline.
  - Moved new tests to t/t3405-rebase-malformed.sh, which has the
    relevant test description: "rebase should handle arbitrary git
    message".
  - Accordingly re-used existing test setup.
  - Minimized tests to just one for git-rebase--interactive.sh and one
    for git-rebase--merge.sh. First, one test per file keeps things
    focused while getting most benefit (other code within same file is
    likely to be noticed by modifiers). And, while git-rebase--am.sh
    does have one cherry-pick, it is only for a special case with
    --keep-empty. So git-rebase--am.sh otherwise doesn't have this
    empty-message issue.

  In general, there was a concern of over-testing, and over-checking
  implementation details. So, this time, I erred on the conservative
  side.

  [v1]: https://public-inbox.org/git/9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is/t/
Very nice. I looked over the patch (sorry, I have too little time to test
this thoroughly, but then, it is the custom on this here mailing list to
just review the patch as per the mail) and it looks very good to me.

Junio, if you like, please add a "Reviewed-by:" line for me.

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