{"thread":{"id":"36502","subject":"[PATCH] commit: do not complain of empty messages from -C","startedAt":"2014-04-25T23:11:15Z","lastAt":"2014-04-25T23:11:15Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"239727","messageId":"20140425231115.GA3855@sigill.intra.peff.net","threadId":"36502","inReplyTo":null,"subject":"[PATCH] commit: do not complain of empty messages from -C","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-25T23:11:15Z","receivedAt":"2014-04-25T23:11:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we pick another commit's message, we die() immediately\nif we find that it's empty and we are not going to run an\neditor (i.e., when running \"-C\" instead of \"-c\"). However,\nthis check is redundant and harmful.\n\nIt's redundant because we will already notice the empty\nmessage later, after we would have run the editor, and die\nthere (just as we would for a regular, not \"-C\" case, where\nthe user provided an empty message in the editor).\n\nIt's harmful for a few reasons:\n\n 1. It does not respect --allow-empty-message. As a result,\n a \"git rebase -i\" cannot \"pick\" such a commit. So you\n cannot even go back in time to fix it with a \"reword\"\n or \"edit\" instruction.\n\n 2. It does not take into account other ways besides the\n editor to modify the message. For example, \"git commit\n -C empty-commit -m foo\" could take the author\n information from empty-commit, but add a message to it.\n There's more to do to make that work correctly (and\n right now we explicitly forbid \"-C with -m\"), but this\n removes one roadblock.\n\n 3. The existing check is not enough to prevent segfaults.\n We try to find the \"\\n\\n\" header/body boundary in the\n commit. If it is at the end of the string (i.e., no\n body), _or_ if we cannot find it at all (i.e., a\n truncated commit object), we consider the message\n empty. With \"-C\", that's OK; we die in either case. But\n with \"-c\", we continue on, and in the case of a\n truncated commit may end up dereferencing NULL+2.\n\nSigned-off-by: Jeff King \n---\nI care most about the \"rebase -i\" thing, especially because it is the\nprimary method for fixing old mistakes. The segfault fix is a nice\nbonus.\n\nThe \"git commit -C empty -m foo\" thing might be nice, but I don't plan\nto work on it further. The semantics would need to be figured out (does\nit append or replace?), and you can always just use \"-c\" to fire up an\nactual editor and write the new content there.\n\n builtin/commit.c | 5 ++---\n t/t7500-commit.sh | 11 ++++++++++-\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 9cfef6c..65c069d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -650,9 +650,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t} else if (use_message) {\n \t\tchar *buffer;\n \t\tbuffer = strstr(use_message_buffer, \"\\n\\n\");\n-\t\tif (!use_editor && (!buffer || buffer[2] == '\\0'))\n-\t\t\tdie(_(\"commit has empty message\"));\n-\t\tstrbuf_add(&sb, buffer + 2, strlen(buffer + 2));\n+\t\tif (buffer)\n+\t\t\tstrbuf_add(&sb, buffer + 2, strlen(buffer + 2));\n \t\thook_arg1 = \"commit\";\n \t\thook_arg2 = use_message;\n \t} else if (fixup_message) {\ndiff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\nindex bdc1f29..116885a 100755\n--- a/t/t7500-commit.sh\n+++ b/t/t7500-commit.sh\n@@ -223,7 +223,8 @@ test_expect_success 'Commit without message is allowed with --allow-empty-messag\n \tgit add foo &&\n \t>empty &&\n \tgit commit --allow-empty-message >foo &&\n+\tgit add foo &&\n+\ttest_must_fail git commit -C empty-message-commit &&\n+\tgit commit -C empty-message-commit --allow-empty-message &&\n+\tcommit_msg_is \"\"\n+'\n+\n commit_for_rebase_autosquash_setup () {\n \techo \"first content line\" >>foo &&\n \tgit add foo &&\n-- \n1.9.1.656.ge8a0637\n"}]}