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

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

[Genki Sky]

--> 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 <sky@genki.is>
---
 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

[Johannes Schindelin]

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

> 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

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

[Junio C Hamano]

Genki Sky <sky@genki.is> writes:

> --> 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 <sky@genki.is>
> ---

Do you have our project history so that you can try running "git
log" to realize that the above does not quite match how people write
their log messages?  If not, please obtain one and do so ;-)

 - We discourage log messages from not explaining what *it* needs to
   explain itself and only referring to external resources.  The
   first part of the above is a typical anti-pattern.  The only
   thing readers can gather from "Motivation" part without refering
   to outside resources is it is a moral follow-up of 4bee95847
   whatever "this work" is doing, without being told what approach
   "this approach" means, etc.  URLs are good as supporting info,
   but there must be something they are meant to support readable in
   the log message itself.

 - Also we do not organize our log messages as "-->" bulletted
   chapters.  For this particular commit, once the first part
   becomes self-sufficient, I think it is sufficient to drop these
   bulletted headlines and have two paragraphs (first describing the
   problem being solved, then describing how the patch realizes that
   solution).

I think the changes to the code are sensible.  As Dscho said, I
found the new test script somewhat iffy.  Does it have to be a
completely new test script (as opposed to an additional test or two
to an existing tests for rebase that checks a similar feature like
keep-empty)?  Would it make it simpler to piggy back on an existing
one?

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

[Genki Sky]

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 <sky@genki.is>
---

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

[Johannes Schindelin]

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 <sky@genki.is>
> ---
> 
> 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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Will do.  You do not have to apologize for not "testing" it; nobody
expects _you_ to test every change you come across on the list, and
other people (including the CI) are testing without advertising ;-).