{"thread":{"id":"47745","subject":"[PATCH] rebase: add --allow-empty-message option","startedAt":"2018-02-02T06:29:12Z","lastAt":"2018-02-07T19:26:38Z","messageCount":6,"participants":["Genki Sky","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"338078","messageId":"9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is","threadId":"47745","inReplyTo":null,"subject":"[PATCH] rebase: add --allow-empty-message option","fromName":"Genki Sky","fromEmail":"sky@genki.is","sentAt":"2018-02-02T06:20:49Z","receivedAt":"2018-02-02T06:29:12Z","isPatch":true,"sender":{"key":"sky@genki.is","avatar":null},"body":"--> Motivation\n\ncommit 4bee95847 (\"cherry-pick: add --allow-empty-message option\", 2012-08-02)\nstarted doing this work, but it was never completed. For more discussion\non why this approach was chosen, see the thread beginning here:\n\n  https://public-inbox.org/git/20120801111658.GA21272@arachsys.com/\n\nhttps://stackoverflow.com/q/8542304 also shows this as a desirable\nfeature, and that the workaround is non-trivial to get right.\n\n--> Implementation\n\nAdd a new --allow-empty-message flag. Propagate it to all calls of 'git\ncommit', 'git cherry-pick', and 'git rebase--helper' within the rebase\nscripts.\n\nSigned-off-by: Genki Sky <sky@genki.is>\n---\n Documentation/git-rebase.txt    |  5 +++\n builtin/rebase--helper.c        |  2 ++\n git-rebase--am.sh               |  1 +\n git-rebase--interactive.sh      | 25 +++++++------\n git-rebase--merge.sh            |  3 +-\n git-rebase.sh                   |  5 +++\n t/t3430-rebase-empty-message.sh | 79 +++++++++++++++++++++++++++++++++++++++++\n 7 files changed, 109 insertions(+), 11 deletions(-)\n create mode 100755 t/t3430-rebase-empty-message.sh\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 8a861c1e0..d713951b8 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -244,6 +244,11 @@ leave out at most one of A and B, in which case it defaults to HEAD.\n \tKeep the commits that do not change anything from its\n \tparents in the result.\n\n+--allow-empty-message::\n+\tBy default, rebasing commits with an empty message will fail.\n+\tThis option overrides that behavior, allowing commits with empty\n+\tmessages to be rebased.\n+\n --skip::\n \tRestart the rebasing process by skipping the current patch.\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex 7daee544b..2090114e9 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -22,6 +22,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n \t\tOPT_BOOL(0, \"keep-empty\", &keep_empty, N_(\"keep empty commits\")),\n+\t\tOPT_BOOL(0, \"allow-empty-message\", &opts.allow_empty_message,\n+\t\t\tN_(\"allow commits with empty messages\")),\n \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n \t\t\t\tCONTINUE),\n \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex 14c50782e..0f7805179 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -46,6 +46,7 @@ then\n \t# makes this easy\n \tgit cherry-pick ${gpg_sign_opt:+\"$gpg_sign_opt\"} --allow-empty \\\n \t\t$allow_rerere_autoupdate --right-only \"$revisions\" \\\n+\t\t$allow_empty_message \\\n \t\t${restrict_revision+^$restrict_revision}\n \tret=$?\n else\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex d47bd2959..81c5b4287 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -281,7 +281,7 @@ pick_one () {\n\n \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput eval git cherry-pick $allow_rerere_autoupdate \\\n+\toutput eval git cherry-pick $allow_rerere_autoupdate $allow_empty_message \\\n \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n \t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n\n@@ -406,6 +406,7 @@ pick_one_preserving_merges () {\n \t\t\t;;\n \t\t*)\n \t\t\toutput eval git cherry-pick $allow_rerere_autoupdate \\\n+\t\t\t\t$allow_empty_message \\\n \t\t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n \t\t\t\t\"$strategy_args\" \"$@\" ||\n \t\t\t\tdie_with_patch $sha1 \"$(eval_gettext \"Could not pick \\$sha1\")\"\n@@ -559,7 +560,8 @@ do_next () {\n\n \t\tmark_action_done\n \t\tdo_pick $sha1 \"$rest\"\n-\t\tgit commit --amend --no-post-rewrite ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n+\t\tgit commit --amend --no-post-rewrite ${gpg_sign_opt:+\"$gpg_sign_opt\"} \\\n+\t\t\t$allow_empty_message || {\n \t\t\twarn \"$(eval_gettext \"\\\n Could not amend commit after successfully picking \\$sha1... \\$rest\n This is most likely due to an empty commit message, or the pre-commit hook\n@@ -607,7 +609,7 @@ you are able to reword the commit.\")\"\n \t\t\t# This is an intermediate commit; its message will only be\n \t\t\t# used in case of trouble.  So use the long version:\n \t\t\tdo_with_author output git commit --amend --no-verify -F \"$squash_msg\" \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\tdie_failed_squash $sha1 \"$rest\"\n \t\t\t;;\n \t\t*)\n@@ -615,13 +617,13 @@ you are able to reword the commit.\")\"\n \t\t\tif test -f \"$fixup_msg\"\n \t\t\tthen\n \t\t\t\tdo_with_author git commit --amend --no-verify -F \"$fixup_msg\" \\\n-\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n \t\t\telse\n \t\t\t\tcp \"$squash_msg\" \"$GIT_DIR\"/SQUASH_MSG || exit\n \t\t\t\trm -f \"$GIT_DIR\"/MERGE_MSG\n \t\t\t\tdo_with_author git commit --amend --no-verify -F \"$GIT_DIR\"/SQUASH_MSG -e \\\n-\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n \t\t\tfi\n \t\t\trm -f \"$squash_msg\" \"$fixup_msg\"\n@@ -754,7 +756,8 @@ case \"$action\" in\n continue)\n \tif test ! -d \"$rewritten\"\n \tthen\n-\t\texec git rebase--helper ${force_rebase:+--no-ff} --continue\n+\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t\t--continue\n \tfi\n \t# do we have anything to commit?\n \tif git diff-index --cached --quiet HEAD --\n@@ -794,11 +797,11 @@ In both cases, once you're done, continue with:\n You have uncommitted changes in your working tree. Please commit them\n first and then run 'git rebase --continue' again.\")\"\n \t\t\tdo_with_author git commit --amend --no-verify -F \"$msg\" -e \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\tdie \"$(gettext \"Could not commit staged changes.\")\"\n \t\telse\n \t\t\tdo_with_author git commit --no-verify -F \"$msg\" -e \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\tdie \"$(gettext \"Could not commit staged changes.\")\"\n \t\tfi\n \tfi\n@@ -817,7 +820,8 @@ skip)\n\n \tif test ! -d \"$rewritten\"\n \tthen\n-\t\texec git rebase--helper ${force_rebase:+--no-ff} --continue\n+\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t\t--continue\n \tfi\n \tdo_rest\n \treturn 0\n@@ -1016,7 +1020,8 @@ checkout_onto\n if test -z \"$rebase_root\" && test ! -d \"$rewritten\"\n then\n \trequire_clean_work_tree \"rebase\"\n-\texec git rebase--helper ${force_rebase:+--no-ff} --continue\n+\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t--continue\n fi\n do_rest\n\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex 06a4723d4..1831e589b 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -27,7 +27,8 @@ continue_merge () {\n \tcmt=$(cat \"$state_dir/current\")\n \tif ! git diff-index --quiet --ignore-submodules HEAD --\n \tthen\n-\t\tif ! git commit ${gpg_sign_opt:+\"$gpg_sign_opt\"} --no-verify -C \"$cmt\"\n+\t\tif ! git commit ${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message \\\n+\t\t\t--no-verify -C \"$cmt\"\n \t\tthen\n \t\t\techo \"Commit failed, please do not call \\\"git commit\\\"\"\n \t\t\techo \"directly, but instead do one of the following: \"\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex fd72a35c6..b353c33d4 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -24,6 +24,7 @@ m,merge!           use merging strategies to rebase\n i,interactive!     let the user edit the list of commits to rebase\n x,exec=!           add exec lines after each commit of the editable list\n k,keep-empty\t   preserve empty commits during rebase\n+allow-empty-message allow rebasing commits with empty messages\n f,force-rebase!    force rebase even if branch is up to date\n X,strategy-option=! pass the argument through to the merge strategy\n stat!              display a diffstat of what changed upstream\n@@ -89,6 +90,7 @@ action=\n preserve_merges=\n autosquash=\n keep_empty=\n+allow_empty_message=\n test \"$(git config --bool rebase.autosquash)\" = \"true\" && autosquash=t\n case \"$(git config --bool commit.gpgsign)\" in\n true)\tgpg_sign_opt=-S ;;\n@@ -262,6 +264,9 @@ do\n \t--keep-empty)\n \t\tkeep_empty=yes\n \t\t;;\n+\t--allow-empty-message)\n+\t\tallow_empty_message=--allow-empty-message\n+\t\t;;\n \t--preserve-merges)\n \t\tpreserve_merges=t\n \t\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\ndiff --git a/t/t3430-rebase-empty-message.sh b/t/t3430-rebase-empty-message.sh\nnew file mode 100755\nindex 000000000..74af73f3c\n--- /dev/null\n+++ b/t/t3430-rebase-empty-message.sh\n@@ -0,0 +1,79 @@\n+#!/bin/sh\n+\n+test_description='rebase: option to allow empty commit messages'\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n+\n+test_expect_success 'setup: three regular commits' '\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\ttest_commit C\n+'\n+\n+test_expect_success 'rebase -i \"reword\" should fail to create an empty commit message' '\n+\tset_fake_editor &&\n+\ttest_must_fail env FAKE_COMMIT_MESSAGE=\" \" FAKE_LINES=\"1 reword 2\" \\\n+\t\tgit rebase -i HEAD~2\n+'\n+\n+test_expect_success '... but should succeed with --allow-empty-message' '\n+\tgit rebase --abort &&\n+\tset_fake_editor &&\n+\tFAKE_COMMIT_MESSAGE=\" \" FAKE_LINES=\"1 reword 2\" \\\n+\t\tgit rebase -i --allow-empty-message HEAD~2\n+'\n+\n+test_expect_success 'rebase -i \"fixup\" should fail to fixup an empty commit message' '\n+\ttest_commit D &&\n+\tset_fake_editor &&\n+\ttest_must_fail env FAKE_LINES=\"1 fixup 2\" git rebase -i HEAD~2\n+'\n+\n+test_expect_success '... but should succeed with --allow-empty-message' '\n+\tgit rebase --abort &&\n+\tFAKE_LINES=\"1 fixup 2\" git rebase -i --allow-empty-message HEAD~2\n+'\n+\n+# The test expects the following rebase to fail. It will only fail if it\n+# actually has to cmd_commit() a new [empty message] commit. However, this\n+# rebase makes no actual changes. So if the date does not change in time, it is\n+# possible for it to simply take the original commits exactly as they are.\n+# So, we test_tick() just to be safe.\n+test_expect_success 'rebase --root should fail on an empty commit message' '\n+\ttest_tick &&\n+\ttest_must_fail git rebase --root\n+'\n+\n+test_expect_success '... but should succeed with --allow-empty-message' '\n+\tgit rebase --abort &&\n+\tgit rebase --root --allow-empty-message\n+'\n+\n+test_expect_success 'setup: multiple branches' '\n+\tgit checkout -b branch-keep-empty HEAD^1 &&\n+\techo E >E &&\n+\tgit add E &&\n+\tgit commit --allow-empty-message -m \"\" &&\n+\tgit branch branch-merge\n+'\n+\n+test_expect_success 'rebase --keep-empty should fail on an empty commit message' '\n+\ttest_must_fail git rebase --keep-empty master branch-keep-empty\n+'\n+\n+test_expect_success '... but should succeed with --allow-empty-message' '\n+\tgit cherry-pick --abort &&\n+\tgit rebase --keep-empty --allow-empty-message master branch-keep-empty\n+'\n+\n+test_expect_success 'rebase -m should fail on an empty commit message' '\n+\ttest_must_fail git rebase -m master branch-merge\n+'\n+\n+test_expect_success '... but should succeed with --allow-empty-message' '\n+\tgit rebase --abort &&\n+\tgit rebase -m --allow-empty-message master branch-merge\n+'\n+\n+test_done\n--\n2.16.1.73.g13f648520\n\n"},{"id":"338118","messageId":"nycvar.QRO.7.76.6.1802022054290.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"47745","inReplyTo":"9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is","subject":"Re: [PATCH] rebase: add --allow-empty-message option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-02-02T20:52:39Z","receivedAt":"2018-02-02T20:54:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 2 Feb 2018, Genki Sky wrote:\n\n> --> Motivation\n\nI won't comment on the motivation (leaving that up to the Git maintainer),\nbut on the patch.\n\nThe patch on the shell scripts and the C code looks straight-forward\nenough, if a little repetitive (but that is hardly your fault).\n\n> diff --git a/t/t3430-rebase-empty-message.sh b/t/t3430-rebase-empty-message.sh\n> new file mode 100755\n> index 000000000..74af73f3c\n> --- /dev/null\n> +++ b/t/t3430-rebase-empty-message.sh\n> @@ -0,0 +1,79 @@\n> +#!/bin/sh\n> +\n> +test_description='rebase: option to allow empty commit messages'\n> +\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/lib-rebase.sh\n> +\n> +test_expect_success 'setup: three regular commits' '\n> +\ttest_commit A &&\n> +\ttest_commit B &&\n> +\ttest_commit C\n> +'\n\nNone of these commits have empty commit messages, which is a little\ncurious for a 'setup' test case.\n\n> +test_expect_success 'rebase -i \"reword\" should fail to create an empty commit message' '\n> +\tset_fake_editor &&\n> +\ttest_must_fail env FAKE_COMMIT_MESSAGE=\" \" FAKE_LINES=\"1 reword 2\" \\\n> +\t\tgit rebase -i HEAD~2\n> +'\n\nWhy not make this more focused via\n\n\t... FAKE_LINES=\"reword 1\" git rebase -i HEAD^\n\nThe effect will be the same because the first pick will be skipped as an\nunnecessary pick anyway.\n\n> +test_expect_success '... but should succeed with --allow-empty-message' '\n> +\tgit rebase --abort &&\n\nThis should be part of the previous test case:\n\n\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n\nAlso, I think this test case should be folded into the previous test case\n(which would make that test_when_finished suggestion moot).\n\n> +\tset_fake_editor &&\n> +\tFAKE_COMMIT_MESSAGE=\" \" FAKE_LINES=\"1 reword 2\" \\\n> +\t\tgit rebase -i --allow-empty-message HEAD~2\n> +'\n> +\n> +test_expect_success 'rebase -i \"fixup\" should fail to fixup an empty commit message' '\n> +\ttest_commit D &&\n> +\tset_fake_editor &&\n> +\ttest_must_fail env FAKE_LINES=\"1 fixup 2\" git rebase -i HEAD~2\n> +'\n> +\n> +test_expect_success '... but should succeed with --allow-empty-message' '\n> +\tgit rebase --abort &&\n> +\tFAKE_LINES=\"1 fixup 2\" git rebase -i --allow-empty-message HEAD~2\n> +'\n> +\n> +# The test expects the following rebase to fail. It will only fail if it\n> +# actually has to cmd_commit() a new [empty message] commit. However, this\n> +# rebase makes no actual changes.\n\nDon't you want to use `--force-rebase` here?\n\n> So if the date does not change in time, it is\n> +# possible for it to simply take the original commits exactly as they are.\n> +# So, we test_tick() just to be safe.\n> +test_expect_success 'rebase --root should fail on an empty commit message' '\n> +\ttest_tick &&\n> +\ttest_must_fail git rebase --root\n> +'\n> +\n> +test_expect_success '... but should succeed with --allow-empty-message' '\n> +\tgit rebase --abort &&\n> +\tgit rebase --root --allow-empty-message\n> +'\n> +\n> +test_expect_success 'setup: multiple branches' '\n> +\tgit checkout -b branch-keep-empty HEAD^1 &&\n> +\techo E >E &&\n> +\tgit add E &&\n> +\tgit commit --allow-empty-message -m \"\" &&\n> +\tgit branch branch-merge\n> +'\n> +\n> +test_expect_success 'rebase --keep-empty should fail on an empty commit message' '\n> +\ttest_must_fail git rebase --keep-empty master branch-keep-empty\n> +'\n> +\n> +test_expect_success '... but should succeed with --allow-empty-message' '\n> +\tgit cherry-pick --abort &&\n> +\tgit rebase --keep-empty --allow-empty-message master branch-keep-empty\n> +'\n\nI do not really see why we have to test --keep-empty here. The code paths\noverlap with what was tested previously.\n\n> +test_expect_success 'rebase -m should fail on an empty commit message' '\n> +\ttest_must_fail git rebase -m master branch-merge\n> +'\n> +\n> +test_expect_success '... but should succeed with --allow-empty-message' '\n> +\tgit rebase --abort &&\n> +\tgit rebase -m --allow-empty-message master branch-merge\n> +'\n> +\n> +test_done\n\nIn general, I would much rather fold the test cases that verify the\nbehavior with --allow-empty-message into the same test case as verifying\nthe behavior without --allow-empty-message.\n\nOne of my aims in the Git project is to avoid test bloat. I know that\nseveral other active contributors could not care less, and even the Git\nmaintainer seems to be oblivious to the danger of making a test suite so\nunsuitable (both in terms of run-time and in terms of being able to\nunderstand regressions) as to make it less useful. I certainly had painful\ndiscussions on this very mailing list about that, and I still don't feel\nheard nor understood.\n\nWhile your patch certainly is clear enough to make it really easy to\nunderstand regressions, I find that it errs on the side of over-testing.\n\nAnd this is not an academic consideration. The test suite takes an insane\n70-90 minutes *on a fast* Windows machine, even skipping all of the\ngit-svn tests. That's insane. (And the fact that Git is optimized for\nLinux and runs the test suite much faster there is only a very feeble\nexcuse, if you want my opinion.)\n\nThe reason? It's death by a thousand only partially necessary tests.\n\nIn that light, I would really like to make at least new tests a lot more\nfocused, and I would really like it if newly-added tests would be\nconsiderate of the time they take to run, and not only on Linux.\n\nThank you,\nJohannes\n"},{"id":"338131","messageId":"xmqqy3kbas2h.fsf@gitster-ct.c.googlers.com","threadId":"47745","inReplyTo":"9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is","subject":"Re: [PATCH] rebase: add --allow-empty-message option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-02T23:29:42Z","receivedAt":"2018-02-02T23:29:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Genki Sky <sky@genki.is> writes:\n\n> --> Motivation\n>\n> commit 4bee95847 (\"cherry-pick: add --allow-empty-message option\", 2012-08-02)\n> started doing this work, but it was never completed. For more discussion\n> on why this approach was chosen, see the thread beginning here:\n>\n>   https://public-inbox.org/git/20120801111658.GA21272@arachsys.com/\n>\n> https://stackoverflow.com/q/8542304 also shows this as a desirable\n> feature, and that the workaround is non-trivial to get right.\n>\n> --> Implementation\n>\n> Add a new --allow-empty-message flag. Propagate it to all calls of 'git\n> commit', 'git cherry-pick', and 'git rebase--helper' within the rebase\n> scripts.\n>\n> Signed-off-by: Genki Sky <sky@genki.is>\n> ---\n\nDo you have our project history so that you can try running \"git\nlog\" to realize that the above does not quite match how people write\ntheir log messages?  If not, please obtain one and do so ;-)\n\n - We discourage log messages from not explaining what *it* needs to\n   explain itself and only referring to external resources.  The\n   first part of the above is a typical anti-pattern.  The only\n   thing readers can gather from \"Motivation\" part without refering\n   to outside resources is it is a moral follow-up of 4bee95847\n   whatever \"this work\" is doing, without being told what approach\n   \"this approach\" means, etc.  URLs are good as supporting info,\n   but there must be something they are meant to support readable in\n   the log message itself.\n\n - Also we do not organize our log messages as \"-->\" bulletted\n   chapters.  For this particular commit, once the first part\n   becomes self-sufficient, I think it is sufficient to drop these\n   bulletted headlines and have two paragraphs (first describing the\n   problem being solved, then describing how the patch realizes that\n   solution).\n\nI think the changes to the code are sensible.  As Dscho said, I\nfound the new test script somewhat iffy.  Does it have to be a\ncompletely new test script (as opposed to an additional test or two\nto an existing tests for rebase that checks a similar feature like\nkeep-empty)?  Would it make it simpler to piggy back on an existing\none?\n\n"},{"id":"338175","messageId":"05bedbb20d5f02a9fe1a74996038f7ca42e35829.1517774326.git.sky@genki.is","threadId":"47745","inReplyTo":"9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is","subject":"[PATCH v2] rebase: add --allow-empty-message option","fromName":"Genki Sky","fromEmail":"sky@genki.is","sentAt":"2018-02-04T20:08:13Z","receivedAt":"2018-02-04T20:08:21Z","isPatch":true,"sender":{"key":"sky@genki.is","avatar":null},"body":"This option allows commits with empty commit messages to be rebased,\nmatching the same option in git-commit and git-cherry-pick. While empty\nlog messages are frowned upon, sometimes one finds them in older\nrepositories (e.g. translated from another VCS [0]), or have other\nreasons for desiring them. The option is available in git-commit and\ngit-cherry-pick, so it is natural to make other git tools play nicely\nwith them. Adding this as an option allows the default to be \"give the\nuser a chance to fix\", while not interrupting the user's workflow\notherwise [1].\n\n  [0]: https://stackoverflow.com/q/8542304\n  [1]: https://public-inbox.org/git/7vd33afqjh.fsf@alter.siamese.dyndns.org/\n\nTo implement this, add a new --allow-empty-message flag. Then propagate\nit to all calls of 'git commit', 'git cherry-pick', and 'git rebase--helper'\nwithin the rebase scripts.\n\nSigned-off-by: Genki Sky <sky@genki.is>\n---\n\nNotes:\n\n  Thanks for the initial feedback, here are the changes from [v1]:\n  - Made my commit message include the main motivations inline.\n  - Moved new tests to t/t3405-rebase-malformed.sh, which has the\n    relevant test description: \"rebase should handle arbitrary git\n    message\".\n  - Accordingly re-used existing test setup.\n  - Minimized tests to just one for git-rebase--interactive.sh and one\n    for git-rebase--merge.sh. First, one test per file keeps things\n    focused while getting most benefit (other code within same file is\n    likely to be noticed by modifiers). And, while git-rebase--am.sh\n    does have one cherry-pick, it is only for a special case with\n    --keep-empty. So git-rebase--am.sh otherwise doesn't have this\n    empty-message issue.\n\n  In general, there was a concern of over-testing, and over-checking\n  implementation details. So, this time, I erred on the conservative\n  side.\n\n  [v1]: https://public-inbox.org/git/9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is/t/\n\n Documentation/git-rebase.txt |  5 +++++\n builtin/rebase--helper.c     |  2 ++\n git-rebase--am.sh            |  1 +\n git-rebase--interactive.sh   | 25 +++++++++++++++----------\n git-rebase--merge.sh         |  3 ++-\n git-rebase.sh                |  5 +++++\n t/t3405-rebase-malformed.sh  | 23 +++++++++++++++++++++++\n 7 files changed, 53 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 8a861c1e0..d713951b8 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -244,6 +244,11 @@ leave out at most one of A and B, in which case it defaults to HEAD.\n \tKeep the commits that do not change anything from its\n \tparents in the result.\n\n+--allow-empty-message::\n+\tBy default, rebasing commits with an empty message will fail.\n+\tThis option overrides that behavior, allowing commits with empty\n+\tmessages to be rebased.\n+\n --skip::\n \tRestart the rebasing process by skipping the current patch.\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex 7daee544b..2090114e9 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -22,6 +22,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n \t\tOPT_BOOL(0, \"keep-empty\", &keep_empty, N_(\"keep empty commits\")),\n+\t\tOPT_BOOL(0, \"allow-empty-message\", &opts.allow_empty_message,\n+\t\t\tN_(\"allow commits with empty messages\")),\n \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n \t\t\t\tCONTINUE),\n \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex 14c50782e..0f7805179 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -46,6 +46,7 @@ then\n \t# makes this easy\n \tgit cherry-pick ${gpg_sign_opt:+\"$gpg_sign_opt\"} --allow-empty \\\n \t\t$allow_rerere_autoupdate --right-only \"$revisions\" \\\n+\t\t$allow_empty_message \\\n \t\t${restrict_revision+^$restrict_revision}\n \tret=$?\n else\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex d47bd2959..81c5b4287 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -281,7 +281,7 @@ pick_one () {\n\n \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput eval git cherry-pick $allow_rerere_autoupdate \\\n+\toutput eval git cherry-pick $allow_rerere_autoupdate $allow_empty_message \\\n \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n \t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n\n@@ -406,6 +406,7 @@ pick_one_preserving_merges () {\n \t\t\t;;\n \t\t*)\n \t\t\toutput eval git cherry-pick $allow_rerere_autoupdate \\\n+\t\t\t\t$allow_empty_message \\\n \t\t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n \t\t\t\t\"$strategy_args\" \"$@\" ||\n \t\t\t\tdie_with_patch $sha1 \"$(eval_gettext \"Could not pick \\$sha1\")\"\n@@ -559,7 +560,8 @@ do_next () {\n\n \t\tmark_action_done\n \t\tdo_pick $sha1 \"$rest\"\n-\t\tgit commit --amend --no-post-rewrite ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n+\t\tgit commit --amend --no-post-rewrite ${gpg_sign_opt:+\"$gpg_sign_opt\"} \\\n+\t\t\t$allow_empty_message || {\n \t\t\twarn \"$(eval_gettext \"\\\n Could not amend commit after successfully picking \\$sha1... \\$rest\n This is most likely due to an empty commit message, or the pre-commit hook\n@@ -607,7 +609,7 @@ you are able to reword the commit.\")\"\n \t\t\t# This is an intermediate commit; its message will only be\n \t\t\t# used in case of trouble.  So use the long version:\n \t\t\tdo_with_author output git commit --amend --no-verify -F \"$squash_msg\" \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\tdie_failed_squash $sha1 \"$rest\"\n \t\t\t;;\n \t\t*)\n@@ -615,13 +617,13 @@ you are able to reword the commit.\")\"\n \t\t\tif test -f \"$fixup_msg\"\n \t\t\tthen\n \t\t\t\tdo_with_author git commit --amend --no-verify -F \"$fixup_msg\" \\\n-\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n \t\t\telse\n \t\t\t\tcp \"$squash_msg\" \"$GIT_DIR\"/SQUASH_MSG || exit\n \t\t\t\trm -f \"$GIT_DIR\"/MERGE_MSG\n \t\t\t\tdo_with_author git commit --amend --no-verify -F \"$GIT_DIR\"/SQUASH_MSG -e \\\n-\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n \t\t\tfi\n \t\t\trm -f \"$squash_msg\" \"$fixup_msg\"\n@@ -754,7 +756,8 @@ case \"$action\" in\n continue)\n \tif test ! -d \"$rewritten\"\n \tthen\n-\t\texec git rebase--helper ${force_rebase:+--no-ff} --continue\n+\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t\t--continue\n \tfi\n \t# do we have anything to commit?\n \tif git diff-index --cached --quiet HEAD --\n@@ -794,11 +797,11 @@ In both cases, once you're done, continue with:\n You have uncommitted changes in your working tree. Please commit them\n first and then run 'git rebase --continue' again.\")\"\n \t\t\tdo_with_author git commit --amend --no-verify -F \"$msg\" -e \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\tdie \"$(gettext \"Could not commit staged changes.\")\"\n \t\telse\n \t\t\tdo_with_author git commit --no-verify -F \"$msg\" -e \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n \t\t\t\tdie \"$(gettext \"Could not commit staged changes.\")\"\n \t\tfi\n \tfi\n@@ -817,7 +820,8 @@ skip)\n\n \tif test ! -d \"$rewritten\"\n \tthen\n-\t\texec git rebase--helper ${force_rebase:+--no-ff} --continue\n+\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t\t--continue\n \tfi\n \tdo_rest\n \treturn 0\n@@ -1016,7 +1020,8 @@ checkout_onto\n if test -z \"$rebase_root\" && test ! -d \"$rewritten\"\n then\n \trequire_clean_work_tree \"rebase\"\n-\texec git rebase--helper ${force_rebase:+--no-ff} --continue\n+\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t--continue\n fi\n do_rest\n\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex 06a4723d4..1831e589b 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -27,7 +27,8 @@ continue_merge () {\n \tcmt=$(cat \"$state_dir/current\")\n \tif ! git diff-index --quiet --ignore-submodules HEAD --\n \tthen\n-\t\tif ! git commit ${gpg_sign_opt:+\"$gpg_sign_opt\"} --no-verify -C \"$cmt\"\n+\t\tif ! git commit ${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message \\\n+\t\t\t--no-verify -C \"$cmt\"\n \t\tthen\n \t\t\techo \"Commit failed, please do not call \\\"git commit\\\"\"\n \t\t\techo \"directly, but instead do one of the following: \"\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex fd72a35c6..b353c33d4 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -24,6 +24,7 @@ m,merge!           use merging strategies to rebase\n i,interactive!     let the user edit the list of commits to rebase\n x,exec=!           add exec lines after each commit of the editable list\n k,keep-empty\t   preserve empty commits during rebase\n+allow-empty-message allow rebasing commits with empty messages\n f,force-rebase!    force rebase even if branch is up to date\n X,strategy-option=! pass the argument through to the merge strategy\n stat!              display a diffstat of what changed upstream\n@@ -89,6 +90,7 @@ action=\n preserve_merges=\n autosquash=\n keep_empty=\n+allow_empty_message=\n test \"$(git config --bool rebase.autosquash)\" = \"true\" && autosquash=t\n case \"$(git config --bool commit.gpgsign)\" in\n true)\tgpg_sign_opt=-S ;;\n@@ -262,6 +264,9 @@ do\n \t--keep-empty)\n \t\tkeep_empty=yes\n \t\t;;\n+\t--allow-empty-message)\n+\t\tallow_empty_message=--allow-empty-message\n+\t\t;;\n \t--preserve-merges)\n \t\tpreserve_merges=t\n \t\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\ndiff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh\nindex ff8c360cd..cb7c6de84 100755\n--- a/t/t3405-rebase-malformed.sh\n+++ b/t/t3405-rebase-malformed.sh\n@@ -3,6 +3,7 @@\n test_description='rebase should handle arbitrary git message'\n\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n\n cat >F <<\\EOF\n This is an example of a commit log message\n@@ -25,6 +26,7 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m \"Initial commit\" &&\n \tgit branch diff-in-message &&\n+\tgit branch empty-message-merge &&\n\n \tgit checkout -b multi-line-subject &&\n \tcat F >file2 &&\n@@ -45,6 +47,11 @@ test_expect_success setup '\n\n \tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >G0 &&\n\n+\tgit checkout empty-message-merge &&\n+\techo file3 >file3 &&\n+\tgit add file3 &&\n+\tgit commit --allow-empty-message -m \"\" &&\n+\n \tgit checkout master &&\n\n \techo One >file1 &&\n@@ -69,4 +76,20 @@ test_expect_success 'rebase commit with diff in message' '\n \ttest_cmp G G0\n '\n\n+test_expect_success 'rebase -m commit with empty message' '\n+\ttest_must_fail git rebase -m master empty-message-merge &&\n+\tgit rebase --abort &&\n+\tgit rebase -m --allow-empty-message master empty-message-merge\n+'\n+\n+test_expect_success 'rebase -i commit with empty message' '\n+\tgit checkout diff-in-message &&\n+\tset_fake_editor &&\n+\ttest_must_fail env FAKE_COMMIT_MESSAGE=\" \" FAKE_LINES=\"reword 1\" \\\n+\t\tgit rebase -i HEAD^ &&\n+\tgit rebase --abort &&\n+\tFAKE_COMMIT_MESSAGE=\" \" FAKE_LINES=\"reword 1\" \\\n+\t\tgit rebase -i --allow-empty-message HEAD^\n+'\n+\n test_done\n--\n2.16.1.73.g3d9791bdf\n\n"},{"id":"338516","messageId":"nycvar.QRO.7.76.6.1802061552090.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"47745","inReplyTo":"05bedbb20d5f02a9fe1a74996038f7ca42e35829.1517774326.git.sky@genki.is","subject":"Re: [PATCH v2] rebase: add --allow-empty-message option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-02-06T14:53:46Z","receivedAt":"2018-02-06T14:54:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Genki,\n\nOn Sun, 4 Feb 2018, Genki Sky wrote:\n\n> This option allows commits with empty commit messages to be rebased,\n> matching the same option in git-commit and git-cherry-pick. While empty\n> log messages are frowned upon, sometimes one finds them in older\n> repositories (e.g. translated from another VCS [0]), or have other\n> reasons for desiring them. The option is available in git-commit and\n> git-cherry-pick, so it is natural to make other git tools play nicely\n> with them. Adding this as an option allows the default to be \"give the\n> user a chance to fix\", while not interrupting the user's workflow\n> otherwise [1].\n> \n>   [0]: https://stackoverflow.com/q/8542304\n>   [1]: https://public-inbox.org/git/7vd33afqjh.fsf@alter.siamese.dyndns.org/\n> \n> To implement this, add a new --allow-empty-message flag. Then propagate\n> it to all calls of 'git commit', 'git cherry-pick', and 'git rebase--helper'\n> within the rebase scripts.\n> \n> Signed-off-by: Genki Sky <sky@genki.is>\n> ---\n> \n> Notes:\n> \n>   Thanks for the initial feedback, here are the changes from [v1]:\n>   - Made my commit message include the main motivations inline.\n>   - Moved new tests to t/t3405-rebase-malformed.sh, which has the\n>     relevant test description: \"rebase should handle arbitrary git\n>     message\".\n>   - Accordingly re-used existing test setup.\n>   - Minimized tests to just one for git-rebase--interactive.sh and one\n>     for git-rebase--merge.sh. First, one test per file keeps things\n>     focused while getting most benefit (other code within same file is\n>     likely to be noticed by modifiers). And, while git-rebase--am.sh\n>     does have one cherry-pick, it is only for a special case with\n>     --keep-empty. So git-rebase--am.sh otherwise doesn't have this\n>     empty-message issue.\n> \n>   In general, there was a concern of over-testing, and over-checking\n>   implementation details. So, this time, I erred on the conservative\n>   side.\n> \n>   [v1]: https://public-inbox.org/git/9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is/t/\n\nVery nice. I looked over the patch (sorry, I have too little time to test\nthis thoroughly, but then, it is the custom on this here mailing list to\njust review the patch as per the mail) and it looks very good to me.\n\nJunio, if you like, please add a \"Reviewed-by:\" line for me.\n\nThanks!\nJohannes\n"},{"id":"338655","messageId":"xmqqmv0k7g9l.fsf@gitster-ct.c.googlers.com","threadId":"47745","inReplyTo":"nycvar.QRO.7.76.6.1802061552090.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH v2] rebase: add --allow-empty-message option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-07T19:26:30Z","receivedAt":"2018-02-07T19:26:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Very nice. I looked over the patch (sorry, I have too little time to test\n> this thoroughly, but then, it is the custom on this here mailing list to\n> just review the patch as per the mail) and it looks very good to me.\n>\n> Junio, if you like, please add a \"Reviewed-by:\" line for me.\n\nWill do.  You do not have to apologize for not \"testing\" it; nobody\nexpects _you_ to test every change you come across on the list, and\nother people (including the CI) are testing without advertising ;-).\n\n\n\n\n"}]}