{"thread":{"id":"29769","subject":"[PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","startedAt":"2012-02-28T08:57:05Z","lastAt":"2012-02-28T21:12:49Z","messageCount":13,"participants":["Thomas Rast","Jeff King","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"185592","messageId":"8529824c8569a8a0b4c4caf3a562750925758e74.1330419275.git.trast@student.ethz.ch","threadId":"29769","inReplyTo":null,"subject":"[PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-02-28T08:57:05Z","receivedAt":"2012-02-28T08:57:05Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Because --amend (-c foo) internally load the message from HEAD (foo,\nresp.) using the same code paths as -C, they erroneously refuse to\nwork at all when the message of HEAD (foo) is empty.\n\nRemove the corresponding check under --amend and -c.\n\nNone of this behavior was ever tested (not even for -C empty_message),\nso we add a whole batch of new tests.\n\nSigned-off-by: Thomas Rast \n---\n\nNoticed while helping \"starlays\" on IRC. It's possible to work around\nthe bug by first giving HEAD a non-empty commit message from the\ncommand line, as in\n\n git commit --amend -mfoo\n git commit --amend\n\nI haven't really checked, but from an irresponsibly quick glance it\nlooks like this bug has always been there in the C version (that is,\nsince f5bbc322).\n\n\n builtin/commit.c | 2 +-\n t/t7501-commit.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 67 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 3714582..45a57af 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -690,7 +690,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\thook_arg1 = \"message\";\n \t} else if (use_message) {\n \t\tbuffer = strstr(use_message_buffer, \"\\n\\n\");\n-\t\tif (!buffer || buffer[2] == '\\0')\n+\t\tif (!amend && !edit_message && (!buffer || buffer[2] == '\\0'))\n \t\t\tdie(_(\"commit has empty message\"));\n \t\tstrbuf_add(&sb, buffer + 2, strlen(buffer + 2));\n \t\thook_arg1 = \"commit\";\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 8bb3833..6ab7712 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -473,4 +473,70 @@ test_expect_success 'amend can copy notes' '\n \n '\n \n+test_expect_success 'amend on empty commit message' '\n+\n+\techo bar > bar &&\n+\tgit add bar &&\n+\ttest_tick &&\n+\tgit commit --allow-empty-message -m \"\" &&\n+\tgit tag empty_message &&\n+\tgit commit --amend -mnonempty &&\n+\tgit cat-file commit HEAD | grep nonempty\n+\n+'\n+\n+test_expect_success 'amend with editor on empty commit message' '\n+\n+\tgit reset --hard empty_message &&\n+\tcat >editor <<-\\EOF &&\n+\t#!/bin/sh\n+\techo nonempty_one >\"$1\"\n+\tEOF\n+\tchmod 755 editor &&\n+\tEDITOR=./editor git commit --amend &&\n+\tgit cat-file commit HEAD | grep nonempty_one\n+\n+'\n+\n+test_expect_success '--amend -C empty_message fails' '\n+\n+\ttest_commit nonempty &&\n+\ttest_must_fail git commit --amend -C empty_message\n+\n+'\n+\n+test_expect_success '-C empty_message fails' '\n+\n+\techo 1 > bar &&\n+\tgit add bar &&\n+\ttest_must_fail git commit --amend -C empty_message\n+\n+'\n+\n+test_expect_success '--amend -c empty_message works' '\n+\n+\tcat >editor <<-\\EOF &&\n+\t#!/bin/sh\n+\techo nonempty_two >\"$1\"\n+\tEOF\n+\tchmod 755 editor &&\n+\tEDITOR=./editor git commit --amend -c empty_message &&\n+\tgit cat-file commit HEAD | grep nonempty_two\n+\n+'\n+\n+test_expect_success '-c empty_message works' '\n+\n+\techo 2 > bar &&\n+\tgit add bar &&\n+\tcat >editor <<-\\EOF &&\n+\t#!/bin/sh\n+\techo nonempty_three >\"$1\"\n+\tEOF\n+\tchmod 755 editor &&\n+\tEDITOR=./editor git commit -c empty_message &&\n+\tgit cat-file commit HEAD | grep nonempty_three\n+\n+'\n+\n test_done\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"185593","messageId":"20120228090540.GB5757@sigill.intra.peff.net","threadId":"29769","inReplyTo":"8529824c8569a8a0b4c4caf3a562750925758e74.1330419275.git.trast@student.ethz.ch","subject":"Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-28T09:05:40Z","receivedAt":"2012-02-28T09:05:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2012 at 09:57:05AM +0100, Thomas Rast wrote:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 3714582..45a57af 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -690,7 +690,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\thook_arg1 = \"message\";\n> \t} else if (use_message) {\n> \t\tbuffer = strstr(use_message_buffer, \"\\n\\n\");\n> -\t\tif (!buffer || buffer[2] == '\\0')\n> +\t\tif (!amend && !edit_message && (!buffer || buffer[2] == '\\0'))\n> \t\t\tdie(_(\"commit has empty message\"));\n\nHmm. So \"buffer\" used to never be NULL (because we would die if it is),\nand now we might not die if we are doing an amend, no? And the next line\nis:\n\n> \t\tstrbuf_add(&sb, buffer + 2, strlen(buffer + 2));\n\nDoesn't this need to handle the case of NULL buffer (i.e., when it does\nnot already have \"\\n\\n\" in it)?\n\n-Peff\n"},{"id":"185595","messageId":"20120228091422.GC5757@sigill.intra.peff.net","threadId":"29769","inReplyTo":"20120228090540.GB5757@sigill.intra.peff.net","subject":"Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-28T09:14:22Z","receivedAt":"2012-02-28T09:14:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2012 at 04:05:40AM -0500, Jeff King wrote:\n\n> > \t} else if (use_message) {\n> > \t\tbuffer = strstr(use_message_buffer, \"\\n\\n\");\n> > -\t\tif (!buffer || buffer[2] == '\\0')\n> > +\t\tif (!amend && !edit_message && (!buffer || buffer[2] == '\\0'))\n> > \t\t\tdie(_(\"commit has empty message\"));\n> \n> Hmm. So \"buffer\" used to never be NULL (because we would die if it is),\n> and now we might not die if we are doing an amend, no? And the next line\n> is:\n> \n> > \t\tstrbuf_add(&sb, buffer + 2, strlen(buffer + 2));\n> \n> Doesn't this need to handle the case of NULL buffer (i.e., when it does\n> not already have \"\\n\\n\" in it)?\n\nI wrote that after looking at just your patch. Looking at\nbuiltin/commit.c, I think use_message_buffer will always be a re-encoded\ncommit object. So that strstr should _never_ fail unless the commit\nobject is corrupt. So the right thing is probably:\n\n buffer = strstr(use_message_buffer, \"\\n\\n\");\n if (!buffer)\n die(_(\"commit object has invalid format\"));\n if (!amend && !edit_message && buffer[2] == '\\0))\n die(_(\"commit has empty message\"));\n\n-Peff\n"},{"id":"185596","messageId":"87haybco1j.fsf@thomas.inf.ethz.ch","threadId":"29769","inReplyTo":"20120228091422.GC5757@sigill.intra.peff.net","subject":"Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-28T09:20:08Z","receivedAt":"2012-02-28T09:20:08Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 28, 2012 at 04:05:40AM -0500, Jeff King wrote:\n>\n>> > \t} else if (use_message) {\n>> > \t\tbuffer = strstr(use_message_buffer, \"\\n\\n\");\n>> > -\t\tif (!buffer || buffer[2] == '\\0')\n>> > +\t\tif (!amend && !edit_message && (!buffer || buffer[2] == '\\0'))\n>> > \t\t\tdie(_(\"commit has empty message\"));\n>> \n>> Hmm. So \"buffer\" used to never be NULL (because we would die if it is),\n>> and now we might not die if we are doing an amend, no? And the next line\n>> is:\n>> \n>> > \t\tstrbuf_add(&sb, buffer + 2, strlen(buffer + 2));\n>> \n>> Doesn't this need to handle the case of NULL buffer (i.e., when it does\n>> not already have \"\\n\\n\" in it)?\n>\n> I wrote that after looking at just your patch. Looking at\n> builtin/commit.c, I think use_message_buffer will always be a re-encoded\n> commit object. So that strstr should _never_ fail unless the commit\n> object is corrupt. So the right thing is probably:\n>\n> buffer = strstr(use_message_buffer, \"\\n\\n\");\n> if (!buffer)\n> die(_(\"commit object has invalid format\"));\n> if (!amend && !edit_message && buffer[2] == '\\0))\n> die(_(\"commit has empty message\"));\n\nInteresting. After I got your mail, I started poking around, and it\nturns out we're in a funny situation here. I did this:\n\n $ git cat-file commit HEAD\n tree 205f6b799e7d5c2524468ca006a0131aa57ecce7\n parent c4938d8e6d23e3a8fe10e6466ecd827662c14846\n author Thomas Rast 1330417798 +0100\n committer Thomas Rast 1330417798 +0100\n\n $ git cat-file commit HEAD | grep -v '^$' | git hash-object -w -t commit --stdin\n f68fcc1996173a9e04bd45d42abbb7c85c79546d\n $ git reset --hard f68fcc1996173a9e04bd45d42abbb7c85c79546d\n\nSo now I'm at a commit which does not have that \\n\\n. And poking around\ngives a very confusing picture:\n\n $ git fsck\n Checking object directories: 100% (256/256), done.\n $ git show\n Segmentation fault\n $ git log\n Segmentation fault\n $ git format-patch -1\n 0001-.txt\n $ cat 0001-.txt # no output!\n $ git bundle create test.bdl HEAD\n error: rev-list died of signal 11\n error: rev-list died\n $ git rev-list HEAD\n f68fcc1996173a9e04bd45d42abbb7c85c79546d\n c4938d8e6d23e3a8fe10e6466ecd827662c14846\n $ git rev-list --pretty=oneline HEAD\n Segmentation fault\n\nSo either there's a lot to be fixed, or fsck needs to catch this.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"185598","messageId":"010901fbfffe0f806bb19d556ebc1e512a4697f4.1330425111.git.trast@student.ethz.ch","threadId":"29769","inReplyTo":"20120228091422.GC5757@sigill.intra.peff.net","subject":"[PATCH v2] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-02-28T10:36:59Z","receivedAt":"2012-02-28T10:36:59Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Because --amend (-c foo) internally load the message from HEAD (foo,\nresp.) using the same code paths as -C, they erroneously refuse to\nwork at all when the message of HEAD (foo) is empty.\n\nRemove the corresponding check under --amend and -c.\n\nNone of this behavior was ever tested (not even for -C empty_message),\nso we add a whole batch of new tests.\n\nReported-by: Lazar Florentin \nHelped-by: Jeff King \nSigned-off-by: Thomas Rast \n---\n\nLike the last version, plus Peff's guard for the invalid commit\nformat. I also received Lazar (starlay) 's identity for the\nattribution.\n\n\n builtin/commit.c | 4 +++-\n t/t7501-commit.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 3714582..5e9a832 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -690,7 +690,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\thook_arg1 = \"message\";\n \t} else if (use_message) {\n \t\tbuffer = strstr(use_message_buffer, \"\\n\\n\");\n-\t\tif (!buffer || buffer[2] == '\\0')\n+\t\tif (!buffer)\n+\t\t\tdie(_(\"commit object has invalid format\"));\n+\t\tif (!amend && !edit_message && buffer[2] == '\\0')\n \t\t\tdie(_(\"commit has empty message\"));\n \t\tstrbuf_add(&sb, buffer + 2, strlen(buffer + 2));\n \t\thook_arg1 = \"commit\";\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 8bb3833..6ab7712 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -473,4 +473,70 @@ test_expect_success 'amend can copy notes' '\n \n '\n \n+test_expect_success 'amend on empty commit message' '\n+\n+\techo bar > bar &&\n+\tgit add bar &&\n+\ttest_tick &&\n+\tgit commit --allow-empty-message -m \"\" &&\n+\tgit tag empty_message &&\n+\tgit commit --amend -mnonempty &&\n+\tgit cat-file commit HEAD | grep nonempty\n+\n+'\n+\n+test_expect_success 'amend with editor on empty commit message' '\n+\n+\tgit reset --hard empty_message &&\n+\tcat >editor <<-\\EOF &&\n+\t#!/bin/sh\n+\techo nonempty_one >\"$1\"\n+\tEOF\n+\tchmod 755 editor &&\n+\tEDITOR=./editor git commit --amend &&\n+\tgit cat-file commit HEAD | grep nonempty_one\n+\n+'\n+\n+test_expect_success '--amend -C empty_message fails' '\n+\n+\ttest_commit nonempty &&\n+\ttest_must_fail git commit --amend -C empty_message\n+\n+'\n+\n+test_expect_success '-C empty_message fails' '\n+\n+\techo 1 > bar &&\n+\tgit add bar &&\n+\ttest_must_fail git commit --amend -C empty_message\n+\n+'\n+\n+test_expect_success '--amend -c empty_message works' '\n+\n+\tcat >editor <<-\\EOF &&\n+\t#!/bin/sh\n+\techo nonempty_two >\"$1\"\n+\tEOF\n+\tchmod 755 editor &&\n+\tEDITOR=./editor git commit --amend -c empty_message &&\n+\tgit cat-file commit HEAD | grep nonempty_two\n+\n+'\n+\n+test_expect_success '-c empty_message works' '\n+\n+\techo 2 > bar &&\n+\tgit add bar &&\n+\tcat >editor <<-\\EOF &&\n+\t#!/bin/sh\n+\techo nonempty_three >\"$1\"\n+\tEOF\n+\tchmod 755 editor &&\n+\tEDITOR=./editor git commit -c empty_message &&\n+\tgit cat-file commit HEAD | grep nonempty_three\n+\n+'\n+\n test_done\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"185599","messageId":"5234ba4babd28d9430750d227d629b4d4386b131.1330425111.git.trast@student.ethz.ch","threadId":"29769","inReplyTo":"010901fbfffe0f806bb19d556ebc1e512a4697f4.1330425111.git.trast@student.ethz.ch","subject":"[RFC PATCH 2a] pretty: detect missing \\n\\n in commit message","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-02-28T10:37:00Z","receivedAt":"2012-02-28T10:37:00Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"get_header()'s exit condition is finding the \\n\\n that separates the\ncommit header from its message. If such a double newline is not\npresent, it segfaults. Catch this case and die().\n\nSigned-off-by: Thomas Rast \n---\n\nThis would be the minimal fix to the pretty machinery so that 'git\nrev-list --pretty=something HEAD' works when there are such broken\ncommits.\n\nIf 2b goes in, there isn't really any point as we would never get this\nfar on such a commit.\n\n\n pretty.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 8688b8f..b7f097d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -440,7 +440,10 @@ static char *get_header(const struct commit *commit, const char *key)\n \tconst char *line = commit->buffer;\n \n \tfor (;;) {\n-\t\tconst char *eol = strchr(line, '\\n'), *next;\n+\t\tconst char *eol, *next;\n+\t\tif (!line)\n+\t\t\tdie (_(\"malformed commit object: no separating \\\\n\\\\n?\"));\n+\t\teol = strchr(line, '\\n');\n \n \t\tif (line == eol)\n \t\t\treturn NULL;\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"185600","messageId":"3f4c4b0656305bec08e87ad56ee3253b010a5a9d.1330425111.git.trast@student.ethz.ch","threadId":"29769","inReplyTo":"010901fbfffe0f806bb19d556ebc1e512a4697f4.1330425111.git.trast@student.ethz.ch","subject":"[RFC PATCH 2b] parse_commit: refuse to load commit without \\n\\n separator","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-02-28T10:37:01Z","receivedAt":"2012-02-28T10:37:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"A commit object consists of a series of headers, terminated by a blank\nline (\\n\\n), followed by the message.\n\nRefuse to parse a commit which does not contain this \\n\\n. This also\nlets git-fsck detect the breakage.\n\nSigned-off-by: Thomas Rast \n---\n\nThis is the more drastic approach compared to 2a. Since the code in\npretty.c was broken given such input, I would favor this. It does,\nhowever, remove the possibility of fixing up such a broken commit with\nthe tools git provides (other than resorting to git-hash-object and\ngit-replace).\n\n\n commit.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/commit.c b/commit.c\nindex 4b39c19..502675c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -265,6 +265,9 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \tif (get_sha1_hex(bufptr + 5, parent) < 0)\n \t\treturn error(\"bad tree pointer in commit %s\",\n \t\t\t sha1_to_hex(item->object.sha1));\n+\tif (!memmem(bufptr, size, \"\\n\\n\", 2))\n+\t\treturn error(\"commit object %s is missing \\\\n\\\\n separator\",\n+\t\t\t sha1_to_hex(item->object.sha1));\n \titem->tree = lookup_tree(parent);\n \tbufptr += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n \tpptr = &item->parents;\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"185617","messageId":"CACsJy8Cj477QnMz5ZD=i7FcFV15__zP9S-kt81pmZ2=opPBi5w@mail.gmail.com","threadId":"29769","inReplyTo":"5234ba4babd28d9430750d227d629b4d4386b131.1330425111.git.trast@student.ethz.ch","subject":"Re: [RFC PATCH 2a] pretty: detect missing \\n\\n in commit message","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-28T12:56:30Z","receivedAt":"2012-02-28T12:56:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 28, 2012 at 5:37 PM, Thomas Rast wrote:\n> get_header()'s exit condition is finding the \\n\\n that separates the\n> commit header from its message.  If such a double newline is not\n> present, it segfaults.  Catch this case and die().\n\nI'd prefer a gentler approach: if there's no \\n\\n, accept that this\ncommit has no body. I think I encountered such commits in the test\nsuite when I tried to convert commit encoding partially and made the\nassumption that \\n\\n must exist.\n-- \nDuy\n"},{"id":"185634","messageId":"7vr4xe27sq.fsf@alter.siamese.dyndns.org","threadId":"29769","inReplyTo":"87haybco1j.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T17:21:09Z","receivedAt":"2012-02-28T17:21:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> So either there's a lot to be fixed, or fsck needs to catch this.\n\nYour experiment with hash-object aside (that is like saying \"I can write\ngarbage with a disk editor, and now OS cannot read from that directory\"),\nif somebody manages to create a commit without any body, it is clear that\nthe user wanted to record no body. I think all code that tries to run\nstrstr(\"\\n\\n\") and increment the resulting pointer by two to find the\nbeginning of the body should behave as if it found one and the result\npointed at a NUL. Rejecting with fsck does not help anybody, as it\nhappens after the fact.\n"},{"id":"185636","messageId":"7vmx8227ld.fsf@alter.siamese.dyndns.org","threadId":"29769","inReplyTo":"010901fbfffe0f806bb19d556ebc1e512a4697f4.1330425111.git.trast@student.ethz.ch","subject":"Re: [PATCH v2] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T17:25:34Z","receivedAt":"2012-02-28T17:25:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 3714582..5e9a832 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -690,7 +690,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\thook_arg1 = \"message\";\n> \t} else if (use_message) {\n> \t\tbuffer = strstr(use_message_buffer, \"\\n\\n\");\n> -\t\tif (!buffer || buffer[2] == '\\0')\n> +\t\tif (!buffer)\n> +\t\t\tdie(_(\"commit object has invalid format\"));\n\nIn line with my previous comment, I think this should be more like:\n\n\tif (!buffer) {\n\t\tstatic char v_o_i_d[] = \"\\n\\n\";\n \tbuffer = v_o_i_d;\n warning(_(\"commit lacks end-of-header. A broken Git emulation?\"\");\n\t}\n\nThe warning is of course optional.\n"},{"id":"185637","messageId":"7vipiq27i9.fsf@alter.siamese.dyndns.org","threadId":"29769","inReplyTo":"5234ba4babd28d9430750d227d629b4d4386b131.1330425111.git.trast@student.ethz.ch","subject":"Re: [RFC PATCH 2a] pretty: detect missing \\n\\n in commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T17:27:26Z","receivedAt":"2012-02-28T17:27:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> get_header()'s exit condition is finding the \\n\\n that separates the\n> commit header from its message. If such a double newline is not\n> present, it segfaults. Catch this case and die().\n>\n> Signed-off-by: Thomas Rast \n> ---\n>\n> This would be the minimal fix to the pretty machinery so that 'git\n> rev-list --pretty=something HEAD' works when there are such broken\n> commits.\n>\n> If 2b goes in, there isn't really any point as we would never get this\n> far on such a commit.\n>\n>\n> pretty.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/pretty.c b/pretty.c\n> index 8688b8f..b7f097d 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -440,7 +440,10 @@ static char *get_header(const struct commit *commit, const char *key)\n> \tconst char *line = commit->buffer;\n> \n> \tfor (;;) {\n> -\t\tconst char *eol = strchr(line, '\\n'), *next;\n> +\t\tconst char *eol, *next;\n> +\t\tif (!line)\n> +\t\t\tdie (_(\"malformed commit object: no separating \\\\n\\\\n?\"));\n> +\t\teol = strchr(line, '\\n');\n\nThe same comment applies here.\n\nYou can just return NULL in this case, I suppose?\n\n> \t\tif (line == eol)\n> \t\t\treturn NULL;\n"},{"id":"185668","messageId":"20120228195931.GE11260@sigill.intra.peff.net","threadId":"29769","inReplyTo":"7vr4xe27sq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-28T19:59:32Z","receivedAt":"2012-02-28T19:59:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2012 at 09:21:09AM -0800, Junio C Hamano wrote:\n\n> Thomas Rast writes:\n> \n> > So either there's a lot to be fixed, or fsck needs to catch this.\n> \n> Your experiment with hash-object aside (that is like saying \"I can write\n> garbage with a disk editor, and now OS cannot read from that directory\"),\n\nYes, but the difference between \"OS cannot read from that directory\" and\n\"OS segfaults\" might be worth noticing. :)\n\n> if somebody manages to create a commit without any body, it is clear that\n> the user wanted to record no body. I think all code that tries to run\n> strstr(\"\\n\\n\") and increment the resulting pointer by two to find the\n> beginning of the body should behave as if it found one and the result\n> pointed at a NUL. Rejecting with fsck does not help anybody, as it\n> happens after the fact.\n\nYeah, I agree that treating it like an empty body is reasonable\n(possibly with a warning). But given that nobody has actually seen this\nin the wild, maybe it is simpler to mark it with fsck, and to just die()\nwhen we see it. That would hopefully alert the author of the broken tool\nearly, before the tools is made public. If it turns out that such\ncommits do end up in the wild, then we can relax the behavior then.\n\n-Peff\n"},{"id":"185673","messageId":"7vpqcyy84u.fsf@alter.siamese.dyndns.org","threadId":"29769","inReplyTo":"20120228195931.GE11260@sigill.intra.peff.net","subject":"Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T21:12:49Z","receivedAt":"2012-02-28T21:12:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I agree that treating it like an empty body is reasonable\n> (possibly with a warning). But given that nobody has actually seen this\n> in the wild, maybe it is simpler to mark it with fsck, and to just die()\n> when we see it. That would hopefully alert the author of the broken tool\n> early, before the tools is made public. If it turns out that such\n> commits do end up in the wild, then we can relax the behavior then.\n\nYeah, it is not like we would want to encourage a commit with empty body,\nbe it preceded with \"\\n\\n\" or just a \"\\n\", in the first place.\n\nWe would need to locate all the places that expect that strstr(\"\\n\\n\")\nwill find something, and add die(\"commit made with a broken git\") at these\nplaces anyway, in addition to the change to fsck. Given that, I suspect\nthat the extra amount of the work needed to tweak the code to tolerate\nsuch a commit and keep going might not be so big, and going that route\nwould avoid punishing the users of broken versions of git (or broken\nimitations of git, for that matter), so...\n"}]}