{"thread":{"id":"41945","subject":"[PATCH] commit: --amend -m '' silently fails to wipe message","startedAt":"2016-04-06T17:15:03Z","lastAt":"2016-04-07T20:28:19Z","messageCount":9,"participants":["Adam Dinwoodie","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"282778","messageId":"20160406171503.GA2345@dinwoodie.org","threadId":"41945","inReplyTo":null,"subject":"[PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2016-04-06T17:15:03Z","receivedAt":"2016-04-06T17:15:03Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"`git commit --amend -m ''` seems to be an unambiguous request to blank a\ncommit message, but it actually leaves the commit message as-is. That's\nthe case regardless of whether `--allow-empty-message` is specified, and\ndoesn't so much as drop a non-zero return code.\n\nAdd failing tests to show this behaviour.\n\nSigned-off-by: Adam Dinwoodie \n---\n\nI've had to guess at the correct file to add these tests to; t7500\ncovers the mainline --allow-empty-message cases, while t7501 doesn't\n(currently) cover --allow-empty-message but does cover --amend. I've\nmade an educated guess about the correct file, but moving the new tests\nto the other file should be reasonably trivial.\n\n t/t7501-commit.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 63e0427..a7e9322 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -200,6 +200,26 @@ test_expect_success '--amend --edit of empty message' '\n \ttest_cmp expect msg\n '\n \n+test_expect_failure '--amend to set message to empty' '\n+\techo batá >file &&\n+\tgit add file &&\n+\tgit commit -m \"unamended\" &&\n+\tgit commit --amend --allow-empty-message -m \"\" &&\n+\tgit diff-tree -s --format=%s HEAD >msg &&\n+\techo \"\" >expect &&\n+\ttest_cmp expect msg\n+'\n+\n+test_expect_failure '--amend to set empty message needs --allow-empty-message' '\n+\techo conga >file &&\n+\tgit add file &&\n+\tgit commit -m \"unamended\" &&\n+\ttest_must_fail git commit --amend -m \"\" &&\n+\tgit diff-tree -s --format=%s HEAD >msg &&\n+\techo \"unamended\" >expect &&\n+\ttest_cmp expect msg\n+'\n+\n test_expect_success '-m --edit' '\n \techo amended >expect &&\n \tgit commit --allow-empty -m buffer &&\n-- \n2.7.4\n"},{"id":"282828","messageId":"20160407044219.GA29710@sigill.intra.peff.net","threadId":"41945","inReplyTo":"20160406171503.GA2345@dinwoodie.org","subject":"Re: [PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-07T04:42:19Z","receivedAt":"2016-04-07T04:42:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 06, 2016 at 06:15:03PM +0100, Adam Dinwoodie wrote:\n\n> `git commit --amend -m ''` seems to be an unambiguous request to blank a\n> commit message, but it actually leaves the commit message as-is. That's\n> the case regardless of whether `--allow-empty-message` is specified, and\n> doesn't so much as drop a non-zero return code.\n> \n> Add failing tests to show this behaviour.\n\nHmm. Is it just that we check \"message.len\", which cannot tell the\ndifference between \"-m was not given\" and \"-m was given the empty\nstring\"?\n\nIOW, would this fix it?\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 109742e..3cdc44e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t}\n \t}\n \n-\tif (message.len) {\n+\tif (have_option_m) {\n \t\tstrbuf_addbuf(&sb, &message);\n \t\thook_arg1 = \"message\";\n \t} else if (logfile && !strcmp(logfile, \"-\")) {\n\nIt passes the test suite and your two new tests, but I didn't look too\ndeeply to see if there are other fallouts. I don't think anybody should\nbe using it to counteract a previous \"-m\" or anything like that; we have\n\"--no-message\" for that.\n\n-Peff\n\nPS Is there a previous thread? I see a couple people cc'd, including me,\n but I don't remember a previous discussion. Did I just forget it?\n"},{"id":"282829","messageId":"20160407044837.GA28859@sigill.intra.peff.net","threadId":"41945","inReplyTo":"20160407044219.GA29710@sigill.intra.peff.net","subject":"Re: [PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-07T04:48:37Z","receivedAt":"2016-04-07T04:48:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 07, 2016 at 12:42:19AM -0400, Jeff King wrote:\n\n> On Wed, Apr 06, 2016 at 06:15:03PM +0100, Adam Dinwoodie wrote:\n> \n> > `git commit --amend -m ''` seems to be an unambiguous request to blank a\n> > commit message, but it actually leaves the commit message as-is. That's\n> > the case regardless of whether `--allow-empty-message` is specified, and\n> > doesn't so much as drop a non-zero return code.\n> > \n> > Add failing tests to show this behaviour.\n> \n> Hmm. Is it just that we check \"message.len\", which cannot tell the\n> difference between \"-m was not given\" and \"-m was given the empty\n> string\"?\n> \n> IOW, would this fix it?\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 109742e..3cdc44e 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\t}\n> \t}\n> \n> -\tif (message.len) {\n> +\tif (have_option_m) {\n> \t\tstrbuf_addbuf(&sb, &message);\n> \t\thook_arg1 = \"message\";\n> \t} else if (logfile && !strcmp(logfile, \"-\")) {\n\nI guessed that this might have come from the conversion of \"message\"\nform a pointer (which could be NULL) into a strbuf. And indeed, it looks\nlike f956853 (builtin-commit: resurrect behavior for multiple -m\noptions, 2007-11-11) did that.\n\nThere are a few other checks for \"message.len\" which probably should be\nusing \"have_option_m\". E.g.:\n\n $ git commit -F /dev/null -m foo\n fatal: Option -m cannot be combined with -c/-C/-F/--fixup.\n\n $ git commit -F /dev/null -m ''\n On branch master\n nothing to commit, working directory clean\n\n-Peff\n"},{"id":"282834","messageId":"20160407095006.GB2345@dinwoodie.org","threadId":"41945","inReplyTo":"20160407044219.GA29710@sigill.intra.peff.net","subject":"Re: [PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2016-04-07T09:50:06Z","receivedAt":"2016-04-07T09:50:06Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Thu, Apr 07, 2016 at 12:42:19AM -0400, Jeff King wrote:\n> On Wed, Apr 06, 2016 at 06:15:03PM +0100, Adam Dinwoodie wrote:\n> > `git commit --amend -m ''` seems to be an unambiguous request to blank a\n> > commit message, but it actually leaves the commit message as-is. That's\n> > the case regardless of whether `--allow-empty-message` is specified, and\n> > doesn't so much as drop a non-zero return code.\n> > \n> > Add failing tests to show this behaviour.\n> \n> Hmm. Is it just that we check \"message.len\", which cannot tell the\n> difference between \"-m was not given\" and \"-m was given the empty\n> string\"?\n> \n> IOW, would this fix it?\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 109742e..3cdc44e 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\t}\n> \t}\n> \n> -\tif (message.len) {\n> +\tif (have_option_m) {\n> \t\tstrbuf_addbuf(&sb, &message);\n> \t\thook_arg1 = \"message\";\n> \t} else if (logfile && !strcmp(logfile, \"-\")) {\n> \n> It passes the test suite and your two new tests, but I didn't look too\n> deeply to see if there are other fallouts. I don't think anybody should\n> be using it to counteract a previous \"-m\" or anything like that; we have\n> \"--no-message\" for that.\n\nThat makes sense as a fix to me, but I'm not going to claim to be even\nslightly familiar with the code here.\n\n> PS Is there a previous thread? I see a couple people cc'd, including me,\n> but I don't remember a previous discussion. Did I just forget it?\n\nNo previous thread: I noticed the odd behaviour, and figured I'd report\nit. The best way to report a problem seemed to be providing a test\nshowing the behaviour, and the SubmittingPatches doc said I should CC\nfolk who looked they're involved in the area...\n\nAdam\n"},{"id":"282865","messageId":"xmqqshyxz3ss.fsf@gitster.mtv.corp.google.com","threadId":"41945","inReplyTo":"20160407044837.GA28859@sigill.intra.peff.net","subject":"Re: [PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T18:12:19Z","receivedAt":"2016-04-07T18:12:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I guessed that this might have come from the conversion of \"message\"\n> form a pointer (which could be NULL) into a strbuf. And indeed, it looks\n> like f956853 (builtin-commit: resurrect behavior for multiple -m\n> options, 2007-11-11) did that.\n\nYikes. That is a quite ancient breakage.\n\nThe funny thing is that we did address the same breakage in 25206778\n(commit: don't start editor if empty message is given with -m,\n2013-05-25), but didn't notice that there are other breakages of the\nsame nature.\n\nI think all message.len check can and should be have_option_m\ninstead.\n\n - The one in the first hunk is a fix for the issue that \"-m ''\" is\n ignored and we read from use_message etc., which is the original\n issue in this thread.\n\n - The second one is a fix for your \"git commit -m '' -F f\" example\n that does not error out.\n\n - The last one is a fix for \"git -c commit.template=f commit -m ''\"\n that still uses the template file 'f'.\n\n builtin/commit.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 98e1527..391126e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t}\n \t}\n \n-\tif (message.len) {\n+\tif (have_option_m) {\n \t\tstrbuf_addbuf(&sb, &message);\n \t\thook_arg1 = \"message\";\n \t} else if (logfile && !strcmp(logfile, \"-\")) {\n@@ -1172,9 +1172,9 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\tf++;\n \tif (f > 1)\n \t\tdie(_(\"Only one of -c/-C/-F/--fixup can be used.\"));\n-\tif (message.len && f > 0)\n+\tif (have_option_m && f > 0)\n \t\tdie((_(\"Option -m cannot be combined with -c/-C/-F/--fixup.\")));\n-\tif (f || message.len)\n+\tif (f || have_option_m)\n \t\ttemplate_file = NULL;\n \tif (edit_message)\n \t\tuse_message = edit_message;\n"},{"id":"282873","messageId":"20160407190210.GA4478@sigill.intra.peff.net","threadId":"41945","inReplyTo":"xmqqshyxz3ss.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-07T19:02:11Z","receivedAt":"2016-04-07T19:02:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 07, 2016 at 11:12:19AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I guessed that this might have come from the conversion of \"message\"\n> > form a pointer (which could be NULL) into a strbuf. And indeed, it looks\n> > like f956853 (builtin-commit: resurrect behavior for multiple -m\n> > options, 2007-11-11) did that.\n> \n> Yikes. That is a quite ancient breakage.\n> \n> The funny thing is that we did address the same breakage in 25206778\n> (commit: don't start editor if empty message is given with -m,\n> 2013-05-25), but didn't notice that there are other breakages of the\n> same nature.\n> \n> I think all message.len check can and should be have_option_m\n> instead.\n> \n> - The one in the first hunk is a fix for the issue that \"-m ''\" is\n> ignored and we read from use_message etc., which is the original\n> issue in this thread.\n> \n> - The second one is a fix for your \"git commit -m '' -F f\" example\n> that does not error out.\n> \n> - The last one is a fix for \"git -c commit.template=f commit -m ''\"\n> that still uses the template file 'f'.\n\nYes, FWIW, those were the sites and reasons I identified last night.\nYour patch looks like the right thing to me.\n\n-Peff\n"},{"id":"282874","messageId":"20160407190249.GB4478@sigill.intra.peff.net","threadId":"41945","inReplyTo":"20160407095006.GB2345@dinwoodie.org","subject":"Re: [PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-07T19:02:50Z","receivedAt":"2016-04-07T19:02:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 07, 2016 at 10:50:06AM +0100, Adam Dinwoodie wrote:\n\n> > PS Is there a previous thread? I see a couple people cc'd, including me,\n> > but I don't remember a previous discussion. Did I just forget it?\n> \n> No previous thread: I noticed the odd behaviour, and figured I'd report\n> it. The best way to report a problem seemed to be providing a test\n> showing the behaviour, and the SubmittingPatches doc said I should CC\n> folk who looked they're involved in the area...\n\nAh, yeah, that's definitely a good thing to do. I just didn't realize I\nwas ever involved with \"-m\". :)\n\n-Peff\n"},{"id":"282902","messageId":"xmqq1t6hyyz9.fsf@gitster.mtv.corp.google.com","threadId":"41945","inReplyTo":"20160407190210.GA4478@sigill.intra.peff.net","subject":"Re: [PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T19:56:26Z","receivedAt":"2016-04-07T19:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, FWIW, those were the sites and reasons I identified last night.\n> Your patch looks like the right thing to me.\n\nThanks, let's do this then. I'd already anticipated your sign-off ;-).\n\n-- >8 --\nFrom: Jeff King \nSubject: commit: do not ignore an empty message given by -m ''\n\nWhen f9568530 (builtin-commit: resurrect behavior for multiple -m\noptions, 2007-11-11) converted a \"char *message\" to \"struct strbuf\nmessage\" to hold the messages given with the \"-m\" option, it\nincorrectly changed the checks \"did we get a message with the -m\noption?\" to \"is message.len 0?\". Later, we noticed one breakage\nfrom this change and corrected it with 25206778 (commit: don't start\neditor if empty message is given with -m, 2013-05-25).\n\nHowever, \"we got a message with -m, even though an empty one, so we\nshouldn't be launching an editor\" was not the only breakage.\n\n * \"git commit --amend -m '' --allow-empty\", even though it looks\n strange, is a valid request to amend the commit to have no\n message at all. Due to the misdetection of the presence of -m on\n the command line, we ended up keeping the log messsage from the\n original commit.\n\n * \"git commit -m \"$msg\" -F file\" should be rejected whether $msg is\n an empty string or not, but due to the same bug, was not rejected\n when $msg is empty.\n\n * \"git -c template=file -m \"$msg\"\" should ignore the template even\n when $msg is empty, but it didn't and instead used the contents\n from the template file.\n\nCorrect these by checking have_option_m, which the earlier 25206778\nintroduced to fix the same bug.\n\nReported-by: Adam Dinwoodie \nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/commit.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 98e1527..391126e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t}\n \t}\n \n-\tif (message.len) {\n+\tif (have_option_m) {\n \t\tstrbuf_addbuf(&sb, &message);\n \t\thook_arg1 = \"message\";\n \t} else if (logfile && !strcmp(logfile, \"-\")) {\n@@ -1172,9 +1172,9 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\tf++;\n \tif (f > 1)\n \t\tdie(_(\"Only one of -c/-C/-F/--fixup can be used.\"));\n-\tif (message.len && f > 0)\n+\tif (have_option_m && f > 0)\n \t\tdie((_(\"Option -m cannot be combined with -c/-C/-F/--fixup.\")));\n-\tif (f || message.len)\n+\tif (f || have_option_m)\n \t\ttemplate_file = NULL;\n \tif (edit_message)\n \t\tuse_message = edit_message;\n"},{"id":"282907","messageId":"20160407202818.GB7705@sigill.intra.peff.net","threadId":"41945","inReplyTo":"xmqq1t6hyyz9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: --amend -m '' silently fails to wipe message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-07T20:28:19Z","receivedAt":"2016-04-07T20:28:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 07, 2016 at 12:56:26PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yes, FWIW, those were the sites and reasons I identified last night.\n> > Your patch looks like the right thing to me.\n> \n> Thanks, let's do this then. I'd already anticipated your sign-off ;-).\n> \n> -- >8 --\n> From: Jeff King \n> Subject: commit: do not ignore an empty message given by -m ''\n> [...]\n\nI don't know if I deserve authorship, but I am happy to take the blame. :)\n\nThank you for picking this up. I'm still traveling through the weekend,\nso I'll be a bit slow on turnaround in general.\n\n> builtin/commit.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n\nI think this should pass Adam's tests, along with some new ones if we\ncared to write them, but I'm not sure if it's worth testing such\nspecific items (i.e., it is not likely to break in exactly the same way\nagain).\n\nI will leave it to you whether you think it's worth squashing them in.\n\n-Peff\n"}]}