{"thread":{"id":"14741","subject":"[PATCH] Advertise the ability to abort a commit","startedAt":"2008-07-29T19:32:05Z","lastAt":"2008-07-31T11:09:26Z","messageCount":19,"participants":["Anders Melchiorsen","Junio C Hamano","Jeff King","Brian Gernhardt","Avery Pennarun","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"85505","messageId":"1217359925-30130-1-git-send-email-mail@cup.kalibalik.dk","threadId":"14741","inReplyTo":null,"subject":"[PATCH] Advertise the ability to abort a commit","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-07-29T19:32:05Z","receivedAt":"2008-07-29T19:32:05Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"This treats aborting a commit more like a feature.\n\nSigned-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>\n---\n builtin-commit.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 9a11ca0..75eeb4b 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -555,6 +555,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix)\n \t\tfprintf(fp,\n \t\t\t\"\\n\"\n \t\t\t\"# Please enter the commit message for your changes.\\n\"\n+\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n \t\t\t\"# (Comment lines starting with '#' will \");\n \t\tif (cleanup_mode == CLEANUP_ALL)\n \t\t\tfprintf(fp, \"not be included)\\n\");\n@@ -1003,7 +1004,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (sb.len < header_len || message_is_empty(&sb, header_len)) {\n \t\trollback_index_files();\n-\t\tdie(\"no commit message?  aborting commit.\");\n+\t\tdie(\"no commit message.  aborting commit.\");\n \t}\n \tstrbuf_addch(&sb, '\\0');\n \tif (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))\n-- \n1.5.6.4\n"},{"id":"85513","messageId":"1217362342-30370-1-git-send-email-mail@cup.kalibalik.dk","threadId":"14741","inReplyTo":"1217359925-30130-1-git-send-email-mail@cup.kalibalik.dk","subject":"[PATCH v2] Advertise the ability to abort a commit","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-07-29T20:12:22Z","receivedAt":"2008-07-29T20:12:22Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"This treats aborting a commit more like a feature.\n\nSigned-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>\n---\n\nNow includes updates to test file.\n\nIncidentally, this change was proposed by pasky in #git.\n\n\n builtin-commit.c  |    3 ++-\n t/t7502-commit.sh |    7 ++++---\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 9a11ca0..75eeb4b 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -555,6 +555,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix)\n \t\tfprintf(fp,\n \t\t\t\"\\n\"\n \t\t\t\"# Please enter the commit message for your changes.\\n\"\n+\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n \t\t\t\"# (Comment lines starting with '#' will \");\n \t\tif (cleanup_mode == CLEANUP_ALL)\n \t\t\tfprintf(fp, \"not be included)\\n\");\n@@ -1003,7 +1004,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (sb.len < header_len || message_is_empty(&sb, header_len)) {\n \t\trollback_index_files();\n-\t\tdie(\"no commit message?  aborting commit.\");\n+\t\tdie(\"no commit message.  aborting commit.\");\n \t}\n \tstrbuf_addch(&sb, '\\0');\n \tif (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 4f2682e..f111263 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -142,6 +142,7 @@ test_expect_success 'cleanup commit messages (strip,-F)' '\n echo \"sample\n \n # Please enter the commit message for your changes.\n+# To abort the commit, use an empty commit message.\n # (Comment lines starting with '#' will not be included)\" >expect\n \n test_expect_success 'cleanup commit messages (strip,-F,-e)' '\n@@ -149,7 +150,7 @@ test_expect_success 'cleanup commit messages (strip,-F,-e)' '\n \techo >>negative &&\n \t{ echo;echo sample;echo; } >text &&\n \tgit commit -e -F text -a &&\n-\thead -n 4 .git/COMMIT_EDITMSG >actual &&\n+\thead -n 5 .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual\n \n '\n@@ -162,7 +163,7 @@ test_expect_success 'author different from committer' '\n \n \techo >>negative &&\n \tgit commit -e -m \"sample\"\n-\thead -n 7 .git/COMMIT_EDITMSG >actual &&\n+\thead -n 8 .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -181,7 +182,7 @@ test_expect_success 'committer is automatic' '\n \t\t# must fail because there is no change\n \t\ttest_must_fail git commit -e -m \"sample\"\n \t) &&\n-\thead -n 8 .git/COMMIT_EDITMSG |\t\\\n+\thead -n 9 .git/COMMIT_EDITMSG |\t\\\n \tsed \"s/^# Committer: .*/# Committer:/\" >actual &&\n \ttest_cmp expect actual\n '\n-- \n1.5.6.4\n"},{"id":"85514","messageId":"7vfxpsct3f.fsf@gitster.siamese.dyndns.org","threadId":"14741","inReplyTo":"1217362342-30370-1-git-send-email-mail@cup.kalibalik.dk","subject":"Re: [PATCH v2] Advertise the ability to abort a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-29T20:51:00Z","receivedAt":"2008-07-29T20:51:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Melchiorsen <mail@cup.kalibalik.dk> writes:\n\n> diff --git a/builtin-commit.c b/builtin-commit.c\n> index 9a11ca0..75eeb4b 100644\n> --- a/builtin-commit.c\n> +++ b/builtin-commit.c\n> @@ -555,6 +555,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix)\n>  \t\tfprintf(fp,\n>  \t\t\t\"\\n\"\n>  \t\t\t\"# Please enter the commit message for your changes.\\n\"\n> +\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n>  \t\t\t\"# (Comment lines starting with '#' will \");\n>  \t\tif (cleanup_mode == CLEANUP_ALL)\n>  \t\t\tfprintf(fp, \"not be included)\\n\");\n\nThanks.  This sounds like a helpful message.\n\n> @@ -1003,7 +1004,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n>  \tif (sb.len < header_len || message_is_empty(&sb, header_len)) {\n>  \t\trollback_index_files();\n> -\t\tdie(\"no commit message?  aborting commit.\");\n> +\t\tdie(\"no commit message.  aborting commit.\");\n>  \t}\n>  \tstrbuf_addch(&sb, '\\0');\n>  \tif (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))\n\n\nSorry but I do not see a point in this hunk.\n\nI am somewhere between neutral to mildly negative about changing \"Abort\nwith error and do not create a commit if there is no message\" to \"Do not\ncreate a commit if there is no message, and this condition is not an\nerror\".  I further think the new message at the top is very helpful to the\nend users, with the understanding that users who changed their mind after\nrunning \"git commit\" _can_ deliberately trigger this _error condition_ to\nprevent commit from happening.  I also agree this ability to trigger an\nerror can be called a feature.\n\nThis still calls die(), which means this is still an error condition.  I\ndo not see a point in changing that question mark (which hints \"perhaps\nyou made a mistake, and that is the reason we are aborting\") to a full\nstop.  I think the current question mark is more helpful to people who did\nnot pay close attention to the new message at the top.\n\nIf the change _were_ to reword the message to more neutral sounding\n\"aborting commit due to missing log message.\", and change die() to a\nnormal exit, that would be making this not an error.  As I already said, I\nam mildly negative, but at least such a change would be internally\nconsistent.\n\nI sense that the change from question mark to full stop might be showing\nthe desire to go in that direction, but in that case your change from the\nquestion mark to full stop does not go far enough.\n"},{"id":"85519","messageId":"38467.N1gUGH5fRhE=.1217366347.squirrel@webmail.hotelhot.dk","threadId":"14741","inReplyTo":"7vfxpsct3f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Advertise the ability to abort a commit","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-07-29T21:19:07Z","receivedAt":"2008-07-29T21:19:07Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"Junio C Hamano wrote:\n> Anders Melchiorsen <mail@cup.kalibalik.dk> writes:\n>\n>> -\t\tdie(\"no commit message?  aborting commit.\");\n>> +\t\tdie(\"no commit message.  aborting commit.\");\n\n> [...]\n\n> If the change _were_ to reword the message to more neutral sounding\n> \"aborting commit due to missing log message.\", and change die() to a\n> normal exit, that would be making this not an error.  As I already said, I\n> am mildly negative, but at least such a change would be internally\n> consistent.\n>\n> I sense that the change from question mark to full stop might be showing\n> the desire to go in that direction, but in that case your change from the\n> question mark to full stop does not go far enough.\n\nI took the question mark to mean that Git was confused about an empty\nmessage. That does not seem right when Git itself proposes it.\n\nI would be happy to also change it to a normal exit. However, since you do\nnot like the change, let us just forget about that hunk.\n\n\nCheers,\nAnders.\n"},{"id":"85573","messageId":"20080730050715.GA4034@sigill.intra.peff.net","threadId":"14741","inReplyTo":"1217362342-30370-1-git-send-email-mail@cup.kalibalik.dk","subject":"Re: [PATCH v2] Advertise the ability to abort a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-30T05:07:16Z","receivedAt":"2008-07-30T05:07:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 29, 2008 at 10:12:22PM +0200, Anders Melchiorsen wrote:\n\n>  \t\t\t\"# Please enter the commit message for your changes.\\n\"\n> +\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n>  \t\t\t\"# (Comment lines starting with '#' will \");\n\nI think this is a good thing to mention, but this text has been getting\nlonger lately. Maybe we can compact it like this:\n\n  # Please enter the commit message for your changes. Lines starting\n  # with '#' will be ignored, and an empty message aborts the commit.\n\n?\n\n> -\t\tdie(\"no commit message?  aborting commit.\");\n> +\t\tdie(\"no commit message.  aborting commit.\");\n\nI don't think the change of punctuation makes a big difference here,\nbut this could probably stand to be reworded. Maybe:\n\n  Aborting commit due to empty commit message.\n\n-Peff\n"},{"id":"85574","messageId":"20080730051059.GA4497@sigill.intra.peff.net","threadId":"14741","inReplyTo":"20080730050715.GA4034@sigill.intra.peff.net","subject":"Re: [PATCH v2] Advertise the ability to abort a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-30T05:11:00Z","receivedAt":"2008-07-30T05:11:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 30, 2008 at 01:07:15AM -0400, Jeff King wrote:\n\n> > -\t\tdie(\"no commit message?  aborting commit.\");\n> > +\t\tdie(\"no commit message.  aborting commit.\");\n> \n> I don't think the change of punctuation makes a big difference here,\n> but this could probably stand to be reworded. Maybe:\n> \n>   Aborting commit due to empty commit message.\n\nUsing \"die\" also prepends \"fatal: \" which is perhaps a bit much for an\nexpected feature. So maybe:\n\n  fprintf(stderr, \"Aborting commit due to empty commit message.\\n\");\n  exit(1); /* or even some specific \"intentional abort\" exit code */\n\n-Peff\n"},{"id":"85656","messageId":"1217440391-13259-1-git-send-email-mail@cup.kalibalik.dk","threadId":"14741","inReplyTo":"20080730051059.GA4497@sigill.intra.peff.net","subject":"[PATCH v3] Advertise the ability to abort a commit","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-07-30T17:53:11Z","receivedAt":"2008-07-30T17:53:11Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"An empty commit message is now treated as a normal situation, not an error.\n\nSigned-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>\n---\n\nSo, I decided that I find it wrong to promote functionality\nthat results in an error. The error is now changed into a\nnormal exit (with status code 1.)\n\n\n builtin-commit.c  |    4 +++-\n t/t7502-commit.sh |    7 ++++---\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 9a11ca0..bc59718 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -555,6 +555,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix)\n \t\tfprintf(fp,\n \t\t\t\"\\n\"\n \t\t\t\"# Please enter the commit message for your changes.\\n\"\n+\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n \t\t\t\"# (Comment lines starting with '#' will \");\n \t\tif (cleanup_mode == CLEANUP_ALL)\n \t\t\tfprintf(fp, \"not be included)\\n\");\n@@ -1003,7 +1004,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (sb.len < header_len || message_is_empty(&sb, header_len)) {\n \t\trollback_index_files();\n-\t\tdie(\"no commit message?  aborting commit.\");\n+\t\tfprintf(stderr, \"Aborting commit due to empty commit message.\\n\");\n+\t\texit(1);\n \t}\n \tstrbuf_addch(&sb, '\\0');\n \tif (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 4f2682e..f111263 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -142,6 +142,7 @@ test_expect_success 'cleanup commit messages (strip,-F)' '\n echo \"sample\n \n # Please enter the commit message for your changes.\n+# To abort the commit, use an empty commit message.\n # (Comment lines starting with '#' will not be included)\" >expect\n \n test_expect_success 'cleanup commit messages (strip,-F,-e)' '\n@@ -149,7 +150,7 @@ test_expect_success 'cleanup commit messages (strip,-F,-e)' '\n \techo >>negative &&\n \t{ echo;echo sample;echo; } >text &&\n \tgit commit -e -F text -a &&\n-\thead -n 4 .git/COMMIT_EDITMSG >actual &&\n+\thead -n 5 .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual\n \n '\n@@ -162,7 +163,7 @@ test_expect_success 'author different from committer' '\n \n \techo >>negative &&\n \tgit commit -e -m \"sample\"\n-\thead -n 7 .git/COMMIT_EDITMSG >actual &&\n+\thead -n 8 .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -181,7 +182,7 @@ test_expect_success 'committer is automatic' '\n \t\t# must fail because there is no change\n \t\ttest_must_fail git commit -e -m \"sample\"\n \t) &&\n-\thead -n 8 .git/COMMIT_EDITMSG |\t\\\n+\thead -n 9 .git/COMMIT_EDITMSG |\t\\\n \tsed \"s/^# Committer: .*/# Committer:/\" >actual &&\n \ttest_cmp expect actual\n '\n-- \n1.5.6.4\n"},{"id":"85667","messageId":"E2809CE9-1DEB-48DA-8E42-8BEAB376FED2@silverinsanity.com","threadId":"14741","inReplyTo":"1217440391-13259-1-git-send-email-mail@cup.kalibalik.dk","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2008-07-30T19:01:02Z","receivedAt":"2008-07-30T19:01:02Z","isPatch":true,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"\nOn Jul 30, 2008, at 1:53 PM, Anders Melchiorsen wrote:\n\n> An empty commit message is now treated as a normal situation, not an  \n> error.\n>\n> Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>\n> ---\n>\n> So, I decided that I find it wrong to promote functionality\n> that results in an error. The error is now changed into a\n> normal exit (with status code 1.)\n\n'git commit' should return with an error any time it does not commit.   \nOtherwise scripts could get confused, thinking everything went fine  \nwhen nothing actually got done.  Here, the user decided something was  \nin error and canceled out, the same way using using ^C causes a non- \nzero return status.\n\n~~ Brian\n"},{"id":"85689","messageId":"32541b130807301409t2f1f3a80n44c62447c628f03a@mail.gmail.com","threadId":"14741","inReplyTo":"E2809CE9-1DEB-48DA-8E42-8BEAB376FED2@silverinsanity.com","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-07-30T21:09:13Z","receivedAt":"2008-07-30T21:09:13Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On 7/30/08, Brian Gernhardt <benji@silverinsanity.com> wrote:\n>  'git commit' should return with an error any time it does not commit.\n> Otherwise scripts could get confused, thinking everything went fine when\n> nothing actually got done.  Here, the user decided something was in error\n> and canceled out, the same way using using ^C causes a non-zero return\n> status.\n\nThe patch uses a non-zero exit code, which is an error status.  But as\nthat's the case, I'm not sure why it's described in the changelog as\ntreating it \"not as an error.\"\n\nHave fun,\n\nAvery\n"},{"id":"85690","messageId":"4D1D0F77-C2BE-4513-B664-80505957CB06@silverinsanity.com","threadId":"14741","inReplyTo":"32541b130807301409t2f1f3a80n44c62447c628f03a@mail.gmail.com","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2008-07-30T21:16:55Z","receivedAt":"2008-07-30T21:16:55Z","isPatch":true,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"\nOn Jul 30, 2008, at 5:09 PM, Avery Pennarun wrote:\n\n> On 7/30/08, Brian Gernhardt <benji@silverinsanity.com> wrote:\n>> 'git commit' should return with an error any time it does not commit.\n>> Otherwise scripts could get confused, thinking everything went fine  \n>> when\n>> nothing actually got done.  Here, the user decided something was in  \n>> error\n>> and canceled out, the same way using using ^C causes a non-zero  \n>> return\n>> status.\n>\n> The patch uses a non-zero exit code, which is an error status.  But as\n> that's the case, I'm not sure why it's described in the changelog as\n> treating it \"not as an error.\"\n\nSorry, was reading through the list too quickly.  Of course an exit  \ncode of 1 is an error.  I'll go back to hiding under my rock now.\n\n~~ Brian\n"},{"id":"85695","messageId":"87tze7qbsm.fsf@cup.kalibalik.dk","threadId":"14741","inReplyTo":"32541b130807301409t2f1f3a80n44c62447c628f03a@mail.gmail.com","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-07-30T21:53:13Z","receivedAt":"2008-07-30T21:53:13Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"\"Avery Pennarun\" <apenwarr@gmail.com> writes:\n\n> The patch uses a non-zero exit code, which is an error status. But\n> as that's the case, I'm not sure why it's described in the changelog\n> as treating it \"not as an error.\"\n\nA matter of terminology, I guess. Apologies if I used the wrong word.\n\nI figured that a non-zero return value was not necessarily an error,\nbut could also be an unusual exit. Like when calling \"git\" for help.\n\nHowever, printing out \"fatal:\" and a lowercase note is definitely an\nerror situation.\n\n\nCheers,\nAnders.\n"},{"id":"85725","messageId":"20080731055024.GA17652@sigill.intra.peff.net","threadId":"14741","inReplyTo":"1217440391-13259-1-git-send-email-mail@cup.kalibalik.dk","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T05:50:24Z","receivedAt":"2008-07-31T05:50:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 30, 2008 at 07:53:11PM +0200, Anders Melchiorsen wrote:\n\n> An empty commit message is now treated as a normal situation, not an error.\n\nAs others have commented, I think the right way to say this is probably\n\"it is not reported to the user as an error, but still exits with a\nnon-zero exit status\".\n\nAnd I think it looks better.\n\nBut:\n\n>  \t\t\t\"# Please enter the commit message for your changes.\\n\"\n> +\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n>  \t\t\t\"# (Comment lines starting with '#' will \");\n\nI still prefer a shortened version of these three lines, as I mentioned\nearlier.\n\n-Peff\n"},{"id":"85728","messageId":"7vwsj23896.fsf@gitster.siamese.dyndns.org","threadId":"14741","inReplyTo":"20080731055024.GA17652@sigill.intra.peff.net","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-31T05:58:13Z","receivedAt":"2008-07-31T05:58:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jul 30, 2008 at 07:53:11PM +0200, Anders Melchiorsen wrote:\n>\n>> An empty commit message is now treated as a normal situation, not an error.\n>\n> As others have commented, I think the right way to say this is probably\n> \"it is not reported to the user as an error, but still exits with a\n> non-zero exit status\".\n>\n> And I think it looks better.\n>\n> But:\n>\n>>  \t\t\t\"# Please enter the commit message for your changes.\\n\"\n>> +\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n>>  \t\t\t\"# (Comment lines starting with '#' will \");\n>\n> I still prefer a shortened version of these three lines, as I mentioned\n> earlier.\n\nI tend to agree; please make it so ;-)\n"},{"id":"85731","messageId":"20080731063619.GA28345@sigill.intra.peff.net","threadId":"14741","inReplyTo":"7vwsj23896.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Advertise the ability to abort a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T06:36:19Z","receivedAt":"2008-07-31T06:36:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Anders Melchiorsen <mail@cup.kalibalik.dk>\n\nWe explicitly let the user know that an empty commit message\nwill abort the commit. At the same time, we take the\nopportunity to reword the template text a bit to keep it\nmore compact.\n\nThis patch also makes the \"fatal: empty commit message?\"\nwarning a bit less scary, since this is now a \"feature\"\ninstead of an error. However, we retain the non-zero exit\nstatus to indicate to callers that nothing was committed.\n\n[jk: I compacted the text and expanded the commit message\nfrom Anders' original patch]\n\nSigned-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-commit.c  |   18 ++++++++++++------\n t/t7502-commit.sh |    4 ++--\n 2 files changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 9a11ca0..b783e6e 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -554,13 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)\n \n \t\tfprintf(fp,\n \t\t\t\"\\n\"\n-\t\t\t\"# Please enter the commit message for your changes.\\n\"\n-\t\t\t\"# (Comment lines starting with '#' will \");\n+\t\t\t\"# Please enter the commit message for your changes.\");\n \t\tif (cleanup_mode == CLEANUP_ALL)\n-\t\t\tfprintf(fp, \"not be included)\\n\");\n+\t\t\tfprintf(fp,\n+\t\t\t\t\" Lines starting\\n\"\n+\t\t\t\t\"# with '#' will be ignored, and an empty\"\n+\t\t\t\t\" message aborts the commit.\\n\");\n \t\telse /* CLEANUP_SPACE, that is. */\n-\t\t\tfprintf(fp, \"be kept.\\n\"\n-\t\t\t\t\"# You can remove them yourself if you want to)\\n\");\n+\t\t\tfprintf(fp,\n+\t\t\t\t\" Lines starting\\n\"\n+\t\t\t\t\"# with '#' will be kept; you may remove them\"\n+\t\t\t\t\" yourself if you want to.\\n\"\n+\t\t\t\t\"# An empty message aborts the commit.\\n\");\n \t\tif (only_include_assumed)\n \t\t\tfprintf(fp, \"# %s\\n\", only_include_assumed);\n \n@@ -1003,7 +1008,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (sb.len < header_len || message_is_empty(&sb, header_len)) {\n \t\trollback_index_files();\n-\t\tdie(\"no commit message?  aborting commit.\");\n+\t\tfprintf(stderr, \"Aborting commit due to empty commit message.\\n\");\n+\t\texit(1);\n \t}\n \tstrbuf_addch(&sb, '\\0');\n \tif (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 4f2682e..3eb9fae 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -141,8 +141,8 @@ test_expect_success 'cleanup commit messages (strip,-F)' '\n \n echo \"sample\n \n-# Please enter the commit message for your changes.\n-# (Comment lines starting with '#' will not be included)\" >expect\n+# Please enter the commit message for your changes. Lines starting\n+# with '#' will be ignored, and an empty message aborts the commit.\" >expect\n \n test_expect_success 'cleanup commit messages (strip,-F,-e)' '\n \n-- \n1.6.0.rc1.168.g8c00d.dirty\n"},{"id":"85738","messageId":"87sktqfr6a.fsf@cup.kalibalik.dk","threadId":"14741","inReplyTo":"20080731055024.GA17652@sigill.intra.peff.net","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-07-31T07:28:45Z","receivedAt":"2008-07-31T07:28:45Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"Jeff King <peff@peff.net> writes:\n\n> I still prefer a shortened version of these three lines, as I\n> mentioned earlier.\n\nYeah, and I obviously didn't :-). I think the line wrapped, run-on\nsentence makes it look more busy, even if it is shorter. Here is a\nfinal compromise proposal:\n\n# Enter a commit message for your changes. Use an empty one to abort.\n# (Comment lines starting with '#' will not be included)\n\n\nAs this is mostly a matter of personal opinion, I will stop here.\n\n\nCheers,\nAnders.\n"},{"id":"85739","messageId":"20080731073213.GA8017@sigill.intra.peff.net","threadId":"14741","inReplyTo":"87sktqfr6a.fsf@cup.kalibalik.dk","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T07:32:14Z","receivedAt":"2008-07-31T07:32:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2008 at 09:28:45AM +0200, Anders Melchiorsen wrote:\n\n> > I still prefer a shortened version of these three lines, as I\n> > mentioned earlier.\n> \n> Yeah, and I obviously didn't :-). I think the line wrapped, run-on\n> sentence makes it look more busy, even if it is shorter. Here is a\n> final compromise proposal:\n\nOK, I wasn't sure if I was being disagreed with or overlooked. ;)\n\n> # Enter a commit message for your changes. Use an empty one to abort.\n> # (Comment lines starting with '#' will not be included)\n\nI don't like that as well as mine, but we are well into the realm of\npersonal preference. I am fine with whatever the list (or Junio)\ndecides.\n\n-Peff\n"},{"id":"85740","messageId":"20080731073609.GA8049@sigill.intra.peff.net","threadId":"14741","inReplyTo":"7vwsj23896.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T07:36:09Z","receivedAt":"2008-07-31T07:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 30, 2008 at 10:58:13PM -0700, Junio C Hamano wrote:\n\n> >>  \t\t\t\"# Please enter the commit message for your changes.\\n\"\n> >> +\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n> >>  \t\t\t\"# (Comment lines starting with '#' will \");\n> >\n> > I still prefer a shortened version of these three lines, as I mentioned\n> > earlier.\n> \n> I tend to agree; please make it so ;-)\n\nHmm, I didn't realize you had already applied the original patch. Here\nis my previous patch, rebased on top of the current master.\n\nI like this wording, but there is perhaps some disagreement. I will let\nyou apply, tweak, or ignore as you desire. :)\n\nNote that this still has the error message change that Anders put in a\nlater patch, but is not in master. Should that be a separate patch (I\nreally didn't anticipate this much discussion for such a simple change,\nbut I think there is a rule of thumb about patch size and bike\nsheds...)?\n\n-- >8 --\nCompact commit template message\n\nWe recently let the user know explicitly that an empty\ncommit message will abort the commit. However, this adds yet\nanother line to the template; let's rephrase and re-wrap so\nthat this fits back on two lines.\n\nThis patch also makes the \"fatal: empty commit message?\"\nwarning a bit less scary, since this is now a \"feature\"\ninstead of an error. However, we retain the non-zero exit\nstatus to indicate to callers that nothing was committed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-commit.c  |   19 ++++++++++++-------\n t/t7502-commit.sh |   11 +++++------\n 2 files changed, 17 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex f7c053a..b783e6e 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -554,14 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)\n \n \t\tfprintf(fp,\n \t\t\t\"\\n\"\n-\t\t\t\"# Please enter the commit message for your changes.\\n\"\n-\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n-\t\t\t\"# (Comment lines starting with '#' will \");\n+\t\t\t\"# Please enter the commit message for your changes.\");\n \t\tif (cleanup_mode == CLEANUP_ALL)\n-\t\t\tfprintf(fp, \"not be included)\\n\");\n+\t\t\tfprintf(fp,\n+\t\t\t\t\" Lines starting\\n\"\n+\t\t\t\t\"# with '#' will be ignored, and an empty\"\n+\t\t\t\t\" message aborts the commit.\\n\");\n \t\telse /* CLEANUP_SPACE, that is. */\n-\t\t\tfprintf(fp, \"be kept.\\n\"\n-\t\t\t\t\"# You can remove them yourself if you want to)\\n\");\n+\t\t\tfprintf(fp,\n+\t\t\t\t\" Lines starting\\n\"\n+\t\t\t\t\"# with '#' will be kept; you may remove them\"\n+\t\t\t\t\" yourself if you want to.\\n\"\n+\t\t\t\t\"# An empty message aborts the commit.\\n\");\n \t\tif (only_include_assumed)\n \t\t\tfprintf(fp, \"# %s\\n\", only_include_assumed);\n \n@@ -1004,7 +1008,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (sb.len < header_len || message_is_empty(&sb, header_len)) {\n \t\trollback_index_files();\n-\t\tdie(\"no commit message?  aborting commit.\");\n+\t\tfprintf(stderr, \"Aborting commit due to empty commit message.\\n\");\n+\t\texit(1);\n \t}\n \tstrbuf_addch(&sb, '\\0');\n \tif (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex f111263..3eb9fae 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -141,16 +141,15 @@ test_expect_success 'cleanup commit messages (strip,-F)' '\n \n echo \"sample\n \n-# Please enter the commit message for your changes.\n-# To abort the commit, use an empty commit message.\n-# (Comment lines starting with '#' will not be included)\" >expect\n+# Please enter the commit message for your changes. Lines starting\n+# with '#' will be ignored, and an empty message aborts the commit.\" >expect\n \n test_expect_success 'cleanup commit messages (strip,-F,-e)' '\n \n \techo >>negative &&\n \t{ echo;echo sample;echo; } >text &&\n \tgit commit -e -F text -a &&\n-\thead -n 5 .git/COMMIT_EDITMSG >actual &&\n+\thead -n 4 .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual\n \n '\n@@ -163,7 +162,7 @@ test_expect_success 'author different from committer' '\n \n \techo >>negative &&\n \tgit commit -e -m \"sample\"\n-\thead -n 8 .git/COMMIT_EDITMSG >actual &&\n+\thead -n 7 .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -182,7 +181,7 @@ test_expect_success 'committer is automatic' '\n \t\t# must fail because there is no change\n \t\ttest_must_fail git commit -e -m \"sample\"\n \t) &&\n-\thead -n 9 .git/COMMIT_EDITMSG |\t\\\n+\thead -n 8 .git/COMMIT_EDITMSG |\t\\\n \tsed \"s/^# Committer: .*/# Committer:/\" >actual &&\n \ttest_cmp expect actual\n '\n-- \n1.6.0.rc1.169.g34ee\n"},{"id":"85756","messageId":"20080731105539.GM32184@machine.or.cz","threadId":"14741","inReplyTo":"20080731073609.GA8049@sigill.intra.peff.net","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-31T10:55:39Z","receivedAt":"2008-07-31T10:55:39Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Thu, Jul 31, 2008 at 03:36:09AM -0400, Jeff King wrote:\n>  builtin-commit.c  |   19 ++++++++++++-------\n>  t/t7502-commit.sh |   11 +++++------\n>  2 files changed, 17 insertions(+), 13 deletions(-)\n> \n> diff --git a/builtin-commit.c b/builtin-commit.c\n> index f7c053a..b783e6e 100644\n> --- a/builtin-commit.c\n> +++ b/builtin-commit.c\n> @@ -554,14 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)\n>  \n>  \t\tfprintf(fp,\n>  \t\t\t\"\\n\"\n> -\t\t\t\"# Please enter the commit message for your changes.\\n\"\n> -\t\t\t\"# To abort the commit, use an empty commit message.\\n\"\n> -\t\t\t\"# (Comment lines starting with '#' will \");\n> +\t\t\t\"# Please enter the commit message for your changes.\");\n>  \t\tif (cleanup_mode == CLEANUP_ALL)\n> -\t\t\tfprintf(fp, \"not be included)\\n\");\n> +\t\t\tfprintf(fp,\n> +\t\t\t\t\" Lines starting\\n\"\n> +\t\t\t\t\"# with '#' will be ignored, and an empty\"\n> +\t\t\t\t\" message aborts the commit.\\n\");\n>  \t\telse /* CLEANUP_SPACE, that is. */\n> -\t\t\tfprintf(fp, \"be kept.\\n\"\n> -\t\t\t\t\"# You can remove them yourself if you want to)\\n\");\n> +\t\t\tfprintf(fp,\n> +\t\t\t\t\" Lines starting\\n\"\n> +\t\t\t\t\"# with '#' will be kept; you may remove them\"\n> +\t\t\t\t\" yourself if you want to.\\n\"\n> +\t\t\t\t\"# An empty message aborts the commit.\\n\");\n>  \t\tif (only_include_assumed)\n>  \t\t\tfprintf(fp, \"# %s\\n\", only_include_assumed);\n>  \n\nThis is rather funny-looking; you print _one_ fragment of the common\nstring by a common fprintf, but then repeat _second_ fragment of the\nstill-common string in a per-case fprintf. Can't we at least split this\non the line boundary, if not do something loosely like this?\n\n\t\tfprintf(fp,\n\t\t\t\"\\n\"\n\t\t\t\"# Please enter the commit message for your \"\n\t\t\t\"changes. Lines starting\\n\"\n\t\t\t\"# with a '#' will be %s \"\n\t\t\t\"and an empty message aborts the commit\\n\",\n\t\t\tcleanup_mode == CLEANUP_ALL ? \"ignored,\"\n\t\t\t/* CLEANUP_SPACE */ : \"kept (you may remove them \"\n\t\t\t\t\"yourself if you want to)\\n#\");\n\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nAs in certain cults it is possible to kill a process if you know\nits true name.  -- Ken Thompson and Dennis M. Ritchie\n"},{"id":"85759","messageId":"20080731110926.GA23234@sigill.intra.peff.net","threadId":"14741","inReplyTo":"20080731105539.GM32184@machine.or.cz","subject":"Re: [PATCH v3] Advertise the ability to abort a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T11:09:26Z","receivedAt":"2008-07-31T11:09:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2008 at 12:55:39PM +0200, Petr Baudis wrote:\n\n> >  \t\tif (cleanup_mode == CLEANUP_ALL)\n> > -\t\t\tfprintf(fp, \"not be included)\\n\");\n> > +\t\t\tfprintf(fp,\n> > +\t\t\t\t\" Lines starting\\n\"\n> > +\t\t\t\t\"# with '#' will be ignored, and an empty\"\n> > +\t\t\t\t\" message aborts the commit.\\n\");\n> >  \t\telse /* CLEANUP_SPACE, that is. */\n> > -\t\t\tfprintf(fp, \"be kept.\\n\"\n> > -\t\t\t\t\"# You can remove them yourself if you want to)\\n\");\n> > +\t\t\tfprintf(fp,\n> > +\t\t\t\t\" Lines starting\\n\"\n> > +\t\t\t\t\"# with '#' will be kept; you may remove them\"\n> > +\t\t\t\t\" yourself if you want to.\\n\"\n> > +\t\t\t\t\"# An empty message aborts the commit.\\n\");\n> >  \t\tif (only_include_assumed)\n> >  \t\t\tfprintf(fp, \"# %s\\n\", only_include_assumed);\n> >  \n> \n> This is rather funny-looking; you print _one_ fragment of the common\n> string by a common fprintf, but then repeat _second_ fragment of the\n> still-common string in a per-case fprintf. Can't we at least split this\n> on the line boundary, if not do something loosely like this?\n\nI just broke it by sentence, thinking that followed the semantics more\nclearly (i.e., the first fprintf says one thing, then the second says\nanother; however, we must say the second one differently depending on\nthe case). I almost just split the whole paragraph by cleanup case,\nallowing each to be worded and wrapped as most appropriate.\n\n> \t\tfprintf(fp,\n> \t\t\t\"\\n\"\n> \t\t\t\"# Please enter the commit message for your \"\n> \t\t\t\"changes. Lines starting\\n\"\n> \t\t\t\"# with a '#' will be %s \"\n> \t\t\t\"and an empty message aborts the commit\\n\",\n> \t\t\tcleanup_mode == CLEANUP_ALL ? \"ignored,\"\n> \t\t\t/* CLEANUP_SPACE */ : \"kept (you may remove them \"\n> \t\t\t\t\"yourself if you want to)\\n#\");\n\nI did something like that before submitting, but decided against it\nbecause:\n\n  - I found mine more readable, since it is hard to see in yours exactly\n    where there will be a linebreak.\n\n  - I actually changed the phrasing for the second one. Since we\n    introduce another clause into the sentence in the CLEANUP_SPACE\n    case, it makes sense to start another sentence for the final point.\n\n-Peff\n"}]}