{"thread":{"id":"52471","subject":"[PATCH 0/1] [Outreachy] commit: display advice hints when commit fails","startedAt":"2019-12-17T09:17:28Z","lastAt":"2020-01-02T19:56:43Z","messageCount":27,"participants":["Heba Waly via GitGitGadget","Junio C Hamano","Emily Shaffer","Jonathan Tan","Heba Waly","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"388337","messageId":"pull.495.git.1576574242.gitgitgadget@gmail.com","threadId":"52471","inReplyTo":null,"subject":"[PATCH 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Heba Waly via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-17T09:17:21Z","receivedAt":"2019-12-17T09:17:28Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"Display hints to the user when trying to commit without staging the modified\nfiles first (when advice.statusHints is set to true). Change the output of\nthe unsuccessful commit from e.g:\n\n[...]\n=====\n\nChanges not staged for commit:\n==============================\n\nmodified: builtin/commit.c\n==========================\n\n #\n\nno changes added to commit\n==========================\n\nto:\n\n[...]\n=====\n\nChanges not staged for commit:\n==============================\n\n(use \"git add ...\" to update what will be committed)\n====================================================\n\n(use \"git checkout -- ...\" to discard changes in working directory)\n===================================================================\n\n #\n\nmodified: ../builtin/commit.c\n=============================\n\n #\n\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n=================================================================\n\nIn ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n2013-09-12) the intent was to disable status hints when writing to\nCOMMIT_EDITMSG, but in fact the implementation disabled status messages in\nmore locations, e.g in case the commit wasn't successful, status hints will\nstill be disabled and no hints will be displayed to the user although\nadvice.statusHints is set to true.\n\nSigned-off-by: Heba Waly heba.waly@gmail.com [heba.waly@gmail.com]\n\nHeba Waly (1):\n commit: display advice hints when commit fails\n\n builtin/commit.c | 1 +\n t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n 2 files changed, 10 insertions(+)\n\n\nbase-commit: ad05a3d8e5a6a06443836b5e40434262d992889a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-495%2FHebaWaly%2Fhints-for-unsuccessful-commit-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-495/HebaWaly/hints-for-unsuccessful-commit-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/495\n-- \ngitgitgadget\n"},{"id":"388338","messageId":"f23477c5a32e5d638310024194040146026972b8.1576574242.git.gitgitgadget@gmail.com","threadId":"52471","inReplyTo":"pull.495.git.1576574242.gitgitgadget@gmail.com","subject":"[PATCH 1/1] commit: display advice hints when commit fails","fromName":"Heba Waly via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-17T09:17:22Z","receivedAt":"2019-12-17T09:17:29Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"From: Heba Waly \n\nDisplay hints to the user when trying to commit without staging the modified\nfiles first (when advice.statusHints is set to true). Change the output of the\nunsuccessful commit from e.g:\n\n # [...]\n # Changes not staged for commit:\n # modified: builtin/commit.c\n #\n # no changes added to commit\n\nto:\n\n # [...]\n # Changes not staged for commit:\n # (use \"git add ...\" to update what will be committed)\n # (use \"git checkout -- ...\" to discard changes in working directory)\n #\n # modified: ../builtin/commit.c\n #\n # no changes added to commit (use \"git add\" and/or \"git commit -a\")\n\nIn ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n2013-09-12) the intent was to disable status hints when writing to\nCOMMIT_EDITMSG, but in fact the implementation disabled status messages in\nmore locations, e.g in case the commit wasn't successful, status hints\nwill still be disabled and no hints will be displayed to the user although\nadvice.statusHints is set to true.\n\nSigned-off-by: Heba Waly \n---\n builtin/commit.c | 1 +\n t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2db2ad0de4..4439666465 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t */\n \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n \t !(amend && is_a_merge(current_head))) {\n+\t\ts->hints = advice_status_hints;\n \t\ts->display_comment_prefix = old_display_comment_prefix;\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 46a5cd4b73..3d76e8ebbd 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '\n \t)\n '\n \n+test_expect_success 'commit without staging files fails and displays hints' '\n+\techo \"initial\" >>file &&\n+\tgit add file &&\n+\tgit commit -m initial &&\n+\techo \"changes\" >>file &&\n+\ttest_must_fail git commit -m initial >actual &&\n+\ttest_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"388408","messageId":"xmqq5ziefuby.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"f23477c5a32e5d638310024194040146026972b8.1576574242.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-17T22:41:53Z","receivedAt":"2019-12-17T22:42:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Heba Waly via GitGitGadget\" writes:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2db2ad0de4..4439666465 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t */\n> \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n> \t !(amend && is_a_merge(current_head))) {\n> +\t\ts->hints = advice_status_hints;\n> \t\ts->display_comment_prefix = old_display_comment_prefix;\n> \t\trun_status(stdout, index_file, prefix, 0, s);\n> \t\tif (amend)\n\nIt almost tempts me to say why this is not done inside run_status(),\nwhich has other callers, but I think the answer is because we do not\nwant these hints when we are actually committing (iow, the ongoing\ncommit must be aborted before the user can actually say \"git add\"\netc. that are suggested). \n\nSo the change makes sense to me.\n\nWill queue.\n\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 46a5cd4b73..3d76e8ebbd 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '\n> \t)\n> '\n> \n> +test_expect_success 'commit without staging files fails and displays hints' '\n> +\techo \"initial\" >>file &&\n> +\tgit add file &&\n> +\tgit commit -m initial &&\n> +\techo \"changes\" >>file &&\n> +\ttest_must_fail git commit -m initial >actual &&\n> +\ttest_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n> +'\n> +\n> test_done\n"},{"id":"388409","messageId":"20191217224541.GA230678@google.com","threadId":"52471","inReplyTo":"f23477c5a32e5d638310024194040146026972b8.1576574242.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] commit: display advice hints when commit fails","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-17T22:45:41Z","receivedAt":"2019-12-17T22:45:48Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Dec 17, 2019 at 09:17:22AM +0000, Heba Waly via GitGitGadget wrote:\n> From: Heba Waly \n> \n> Display hints to the user when trying to commit without staging the modified\n> files first (when advice.statusHints is set to true). Change the output of the\n> unsuccessful commit from e.g:\n> \n> # [...]\n> # Changes not staged for commit:\n> # modified: builtin/commit.c\n> #\n> # no changes added to commit\n> \n> to:\n> \n> # [...]\n> # Changes not staged for commit:\n> # (use \"git add ...\" to update what will be committed)\n> # (use \"git checkout -- ...\" to discard changes in working directory)\n> #\n> # modified: ../builtin/commit.c\n> #\n> # no changes added to commit (use \"git add\" and/or \"git commit -a\")\n> \n> In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n> 2013-09-12) the intent was to disable status hints when writing to\n> COMMIT_EDITMSG, but in fact the implementation disabled status messages in\n> more locations, e.g in case the commit wasn't successful, status hints\n> will still be disabled and no hints will be displayed to the user although\n> advice.statusHints is set to true.\n> \n> Signed-off-by: Heba Waly \n> ---\n> builtin/commit.c | 1 +\n> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n> 2 files changed, 10 insertions(+)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2db2ad0de4..4439666465 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t */\n> \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n> \t !(amend && is_a_merge(current_head))) {\n> +\t\ts->hints = advice_status_hints;\n\nHm. This looks like it turns hints back on specifically for this case,\nbut might not fix other places where ea9882bfc4 turned them off.\n\nI think the intent of that commit was to not put hints into the editor,\nso does it make sense to instead wrap this guy:\n\n /* \n * Most hints are counter-productive when the commit has \n * already started. \n */ \n s->hints = 0; \n\nin \"if (use_editor)\"?\n\nI didn't try it on my end. Maybe it won't help much, because we think\nwe're going to use the editor right up until we realize it's not\ncommittable?\n\nI wonder which other cases that commit got rid of hints for by accident.\n\n - Emily\n\n> \t\ts->display_comment_prefix = old_display_comment_prefix;\n> \t\trun_status(stdout, index_file, prefix, 0, s);\n> \t\tif (amend)\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 46a5cd4b73..3d76e8ebbd 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '\n> \t)\n> '\n> \n> +test_expect_success 'commit without staging files fails and displays hints' '\n> +\techo \"initial\" >>file &&\n> +\tgit add file &&\n> +\tgit commit -m initial &&\n> +\techo \"changes\" >>file &&\n> +\ttest_must_fail git commit -m initial >actual &&\n> +\ttest_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n> +'\n> +\n> test_done\n> -- \n> gitgitgadget\n"},{"id":"388417","messageId":"20191218031338.203382-1-jonathantanmy@google.com","threadId":"52471","inReplyTo":"f23477c5a32e5d638310024194040146026972b8.1576574242.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] commit: display advice hints when commit fails","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-12-18T03:13:38Z","receivedAt":"2019-12-18T03:24:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> From: Heba Waly \n> \n> Display hints to the user when trying to commit without staging the modified\n> files first (when advice.statusHints is set to true). Change the output of the\n> unsuccessful commit from e.g:\n\nWrap your commit messages at 72 characters.\n\n> # [...]\n> # Changes not staged for commit:\n> # modified: builtin/commit.c\n> #\n> # no changes added to commit\n> \n> to:\n> \n> # [...]\n> # Changes not staged for commit:\n> # (use \"git add ...\" to update what will be committed)\n> # (use \"git checkout -- ...\" to discard changes in working directory)\n> #\n> # modified: ../builtin/commit.c\n\nFor tidiness, can this line also be \"builtin/commit.c\" (that is, without\nthe \"../\" at the beginning) to match what's before \"to:\"?\n\n> In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n> 2013-09-12) the intent was to disable status hints when writing to\n> COMMIT_EDITMSG, but in fact the implementation disabled status messages in\n> more locations, e.g in case the commit wasn't successful, status hints\n> will still be disabled and no hints will be displayed to the user although\n> advice.statusHints is set to true.\n> \n> Signed-off-by: Heba Waly \n> ---\n> builtin/commit.c | 1 +\n> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n\nI wondered if there was a better place to put the test, but I couldn't\nfind one, so this is fine.\n\n> @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t */\n> \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n> \t !(amend && is_a_merge(current_head))) {\n> +\t\ts->hints = advice_status_hints;\n> \t\ts->display_comment_prefix = old_display_comment_prefix;\n> \t\trun_status(stdout, index_file, prefix, 0, s);\n> \t\tif (amend)\n\nI checked that this undoing of \"s->hints = 0\" is safe, because s is no\nlonger used in this function nor in the calling function cmd_commit()\n(which is the one that declared s locally).\n\nStill probably worth a comment, though. For example:\n\n This status is to be printed to stdout, so hints will be useful to the\n user. Reset s->hints to what the user configured.\n\nThe corresponding comment on \"s->hints = 0\" might need to be tweaked,\ntoo, but I can't think of anything at the moment.\n\n> +test_expect_success 'commit without staging files fails and displays hints' '\n> +\techo \"initial\" >>file &&\n> +\tgit add file &&\n> +\tgit commit -m initial &&\n> +\techo \"changes\" >>file &&\n> +\ttest_must_fail git commit -m initial >actual &&\n\nUse another commit message for this, since this is no longer \"initial\".\n(Maybe \"after initial\" or something like that.)\n"},{"id":"388452","messageId":"xmqqo8w5ec14.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"20191218031338.203382-1-jonathantanmy@google.com","subject":"Re: [PATCH 1/1] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-18T18:14:47Z","receivedAt":"2019-12-18T18:14:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> From: Heba Waly \n>> \n>> Display hints to the user when trying to commit without staging the modified\n>> files first (when advice.statusHints is set to true). Change the output of the\n>> unsuccessful commit from e.g:\n>\n> Wrap your commit messages at 72 characters.\n>\n>> # [...]\n>> # Changes not staged for commit:\n>> # modified: builtin/commit.c\n>> #\n>> # no changes added to commit\n>> \n>> to:\n>> \n>> # [...]\n>> # Changes not staged for commit:\n>> # (use \"git add ...\" to update what will be committed)\n>> # (use \"git checkout -- ...\" to discard changes in working directory)\n>> #\n>> # modified: ../builtin/commit.c\n>\n> For tidiness, can this line also be \"builtin/commit.c\" (that is, without\n> the \"../\" at the beginning) to match what's before \"to:\"?\n>\n>> In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n>> 2013-09-12) the intent was to disable status hints when writing to\n>> COMMIT_EDITMSG, but in fact the implementation disabled status messages in\n>> more locations, e.g in case the commit wasn't successful, status hints\n>> will still be disabled and no hints will be displayed to the user although\n>> advice.statusHints is set to true.\n>> \n>> Signed-off-by: Heba Waly \n>> ---\n>> builtin/commit.c | 1 +\n>> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n>\n> I wondered if there was a better place to put the test, but I couldn't\n> find one, so this is fine.\n>\n>> @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>> \t */\n>> \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n>> \t !(amend && is_a_merge(current_head))) {\n>> +\t\ts->hints = advice_status_hints;\n>> \t\ts->display_comment_prefix = old_display_comment_prefix;\n>> \t\trun_status(stdout, index_file, prefix, 0, s);\n>> \t\tif (amend)\n>\n> I checked that this undoing of \"s->hints = 0\" is safe, because s is no\n> longer used in this function nor in the calling function cmd_commit()\n> (which is the one that declared s locally).\n>\n> Still probably worth a comment, though. For example:\n>\n> This status is to be printed to stdout, so hints will be useful to the\n> user. Reset s->hints to what the user configured.\n>\n> The corresponding comment on \"s->hints = 0\" might need to be tweaked,\n> too, but I can't think of anything at the moment.\n>\n>> +test_expect_success 'commit without staging files fails and displays hints' '\n>> +\techo \"initial\" >>file &&\n>> +\tgit add file &&\n>> +\tgit commit -m initial &&\n>> +\techo \"changes\" >>file &&\n>> +\ttest_must_fail git commit -m initial >actual &&\n>\n> Use another commit message for this, since this is no longer \"initial\".\n> (Maybe \"after initial\" or something like that.)\n\nThanks for a careful review.\n"},{"id":"388501","messageId":"CACg5j25np9drh7ofpRJs2nY57Yq2NtxDYc2xjGX2tW-0SmwejQ@mail.gmail.com","threadId":"52471","inReplyTo":"20191217224541.GA230678@google.com","subject":"Re: [PATCH 1/1] commit: display advice hints when commit fails","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2019-12-19T03:47:29Z","receivedAt":"2019-12-19T03:47:43Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"On Wed, Dec 18, 2019 at 11:45 AM Emily Shaffer wrote:\n>\n> On Tue, Dec 17, 2019 at 09:17:22AM +0000, Heba Waly via GitGitGadget wrote:\n> > From: Heba Waly \n> >\n> > Display hints to the user when trying to commit without staging the modified\n> > files first (when advice.statusHints is set to true). Change the output of the\n> > unsuccessful commit from e.g:\n> >\n> > # [...]\n> > # Changes not staged for commit:\n> > # modified: builtin/commit.c\n> > #\n> > # no changes added to commit\n> >\n> > to:\n> >\n> > # [...]\n> > # Changes not staged for commit:\n> > # (use \"git add ...\" to update what will be committed)\n> > # (use \"git checkout -- ...\" to discard changes in working directory)\n> > #\n> > # modified: ../builtin/commit.c\n> > #\n> > # no changes added to commit (use \"git add\" and/or \"git commit -a\")\n> >\n> > In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n> > 2013-09-12) the intent was to disable status hints when writing to\n> > COMMIT_EDITMSG, but in fact the implementation disabled status messages in\n> > more locations, e.g in case the commit wasn't successful, status hints\n> > will still be disabled and no hints will be displayed to the user although\n> > advice.statusHints is set to true.\n> >\n> > Signed-off-by: Heba Waly \n> > ---\n> > builtin/commit.c | 1 +\n> > t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n> > 2 files changed, 10 insertions(+)\n> >\n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 2db2ad0de4..4439666465 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> > */\n> > if (!committable && whence != FROM_MERGE && !allow_empty &&\n> > !(amend && is_a_merge(current_head))) {\n> > + s->hints = advice_status_hints;\n>\n> Hm. This looks like it turns hints back on specifically for this case,\n> but might not fix other places where ea9882bfc4 turned them off.\n>\n> I think the intent of that commit was to not put hints into the editor,\n> so does it make sense to instead wrap this guy:\n>\n> /*\n> * Most hints are counter-productive when the commit has\n> * already started.\n> */\n> s->hints = 0;\n>\n> in \"if (use_editor)\"?\n>\n\nThat's a good idea, I tried it and it seems to be working fine.\n\n> I didn't try it on my end. Maybe it won't help much, because we think\n> we're going to use the editor right up until we realize it's not\n> committable?\n>\n> I wonder which other cases that commit got rid of hints for by accident.\n>\n\nThe number of cases is quite overwhelming because of all the options that\ncan be passed to the commit command, but hopefully after wrapping it in\nan if condition as you suggested we'll be more certain of the affected cases.\nWill send an update shortly.\n\n> - Emily\n>\n> > s->display_comment_prefix = old_display_comment_prefix;\n> > run_status(stdout, index_file, prefix, 0, s);\n> > if (amend)\n> > diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> > index 46a5cd4b73..3d76e8ebbd 100755\n> > --- a/t/t7500-commit-template-squash-signoff.sh\n> > +++ b/t/t7500-commit-template-squash-signoff.sh\n> > @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '\n> > )\n> > '\n> >\n> > +test_expect_success 'commit without staging files fails and displays hints' '\n> > + echo \"initial\" >>file &&\n> > + git add file &&\n> > + git commit -m initial &&\n> > + echo \"changes\" >>file &&\n> > + test_must_fail git commit -m initial >actual &&\n> > + test_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n> > +'\n> > +\n> > test_done\n> > --\n> > gitgitgadget\n\nThanks,\n\nHeba\n"},{"id":"388502","messageId":"CACg5j258jHGjzxEeUS7OccHsmk=XVj9+bvZM9TvcdZ0+musnUg@mail.gmail.com","threadId":"52471","inReplyTo":"20191218031338.203382-1-jonathantanmy@google.com","subject":"Re: [PATCH 1/1] commit: display advice hints when commit fails","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2019-12-19T03:48:41Z","receivedAt":"2019-12-19T03:48:55Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"On Wed, Dec 18, 2019 at 4:13 PM Jonathan Tan wrote:\n>\n> > From: Heba Waly \n> >\n> > Display hints to the user when trying to commit without staging the modified\n> > files first (when advice.statusHints is set to true). Change the output of the\n> > unsuccessful commit from e.g:\n>\n> Wrap your commit messages at 72 characters.\n>\nOK\n\n> > # [...]\n> > # Changes not staged for commit:\n> > # modified: builtin/commit.c\n> > #\n> > # no changes added to commit\n> >\n> > to:\n> >\n> > # [...]\n> > # Changes not staged for commit:\n> > # (use \"git add ...\" to update what will be committed)\n> > # (use \"git checkout -- ...\" to discard changes in working directory)\n> > #\n> > # modified: ../builtin/commit.c\n>\n> For tidiness, can this line also be \"builtin/commit.c\" (that is, without\n> the \"../\" at the beginning) to match what's before \"to:\"?\n>\n\nSure.\n\n> > In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n> > 2013-09-12) the intent was to disable status hints when writing to\n> > COMMIT_EDITMSG, but in fact the implementation disabled status messages in\n> > more locations, e.g in case the commit wasn't successful, status hints\n> > will still be disabled and no hints will be displayed to the user although\n> > advice.statusHints is set to true.\n> >\n> > Signed-off-by: Heba Waly \n> > ---\n> > builtin/commit.c | 1 +\n> > t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n>\n> I wondered if there was a better place to put the test, but I couldn't\n> find one, so this is fine.\n>\n> > @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> > */\n> > if (!committable && whence != FROM_MERGE && !allow_empty &&\n> > !(amend && is_a_merge(current_head))) {\n> > + s->hints = advice_status_hints;\n> > s->display_comment_prefix = old_display_comment_prefix;\n> > run_status(stdout, index_file, prefix, 0, s);\n> > if (amend)\n>\n> I checked that this undoing of \"s->hints = 0\" is safe, because s is no\n> longer used in this function nor in the calling function cmd_commit()\n> (which is the one that declared s locally).\n>\n> Still probably worth a comment, though. For example:\n>\n> This status is to be printed to stdout, so hints will be useful to the\n> user. Reset s->hints to what the user configured.\n>\n\nOk.\n\n> The corresponding comment on \"s->hints = 0\" might need to be tweaked,\n> too, but I can't think of anything at the moment.\n>\n> > +test_expect_success 'commit without staging files fails and displays hints' '\n> > + echo \"initial\" >>file &&\n> > + git add file &&\n> > + git commit -m initial &&\n> > + echo \"changes\" >>file &&\n> > + test_must_fail git commit -m initial >actual &&\n>\n> Use another commit message for this, since this is no longer \"initial\".\n> (Maybe \"after initial\" or something like that.)\n\nMakes sense.\n\nThank you Jonathan.\n\nHeba\n\n\nHeba\n"},{"id":"388507","messageId":"pull.495.v2.git.1576746982.gitgitgadget@gmail.com","threadId":"52471","inReplyTo":"pull.495.git.1576574242.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Heba Waly via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-19T09:16:21Z","receivedAt":"2019-12-19T09:16:28Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"Display hints to the user when trying to commit without staging the modified\nfiles first (when advice.statusHints is set to true). Change the output of\nthe unsuccessful commit from e.g:\n\n . [...] . Changes not staged for commit: . modified: builtin/commit.c . .\nno changes added to commit\n\nto:\n\n . [...] . Changes not staged for commit: . (use \"git add ...\" to update\nwhat will be committed) . (use \"git checkout -- ...\" to discard changes in\nworking directory) . . modified: ../builtin/commit.c . . no changes added to\ncommit (use \"git add\" and/or \"git commit -a\")\n\nIn ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n2013-09-12) the intent was to disable status hints when writing to\nCOMMIT_EDITMSG, but in fact the implementation disabled status messages in\nmore locations, e.g in case the commit wasn't successful, status hints will\nstill be disabled and no hints will be displayed to the user although\nadvice.statusHints is set to true.\n\nSigned-off-by: Heba Waly heba.waly@gmail.com [heba.waly@gmail.com]\n\nHeba Waly (1):\n commit: display advice hints when commit fails\n\n builtin/commit.c | 18 ++++++++++++------\n t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\n\nbase-commit: 12029dc57db23baef008e77db1909367599210ee\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-495%2FHebaWaly%2Fhints-for-unsuccessful-commit-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-495/HebaWaly/hints-for-unsuccessful-commit-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/495\n\nRange-diff vs v1:\n\n 1: f23477c5a3 ! 1: ebec237920 commit: display advice hints when commit fails\n @@ -2,9 +2,9 @@\n \n commit: display advice hints when commit fails\n \n - Display hints to the user when trying to commit without staging the modified\n - files first (when advice.statusHints is set to true). Change the output of the\n - unsuccessful commit from e.g:\n + Display hints to the user when trying to commit without staging the\n + modified files first (when advice.statusHints is set to true). Change\n + the output of the unsuccessful commit from e.g:\n \n # [...]\n # Changes not staged for commit:\n @@ -19,16 +19,16 @@\n # (use \"git add ...\" to update what will be committed)\n # (use \"git checkout -- ...\" to discard changes in working directory)\n #\n - # modified: ../builtin/commit.c\n + # modified: /builtin/commit.c\n #\n # no changes added to commit (use \"git add\" and/or \"git commit -a\")\n \n - In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n - 2013-09-12) the intent was to disable status hints when writing to\n - COMMIT_EDITMSG, but in fact the implementation disabled status messages in\n - more locations, e.g in case the commit wasn't successful, status hints\n - will still be disabled and no hints will be displayed to the user although\n - advice.statusHints is set to true.\n + In ea9882bfc4 (commit: disable status hints when writing to\n + COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when\n + writing to COMMIT_EDITMSG, but in fact the implementation disabled\n + status messages in more locations, e.g in case the commit wasn't\n + successful, status hints will still be disabled and no hints will be\n + displayed to the user although advice.statusHints is set to true.\n \n Signed-off-by: Heba Waly \n \n @@ -36,13 +36,44 @@\n --- a/builtin/commit.c\n +++ b/builtin/commit.c\n @@\n - \t */\n - \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n - \t !(amend && is_a_merge(current_head))) {\n -+\t\ts->hints = advice_status_hints;\n - \t\ts->display_comment_prefix = old_display_comment_prefix;\n - \t\trun_status(stdout, index_file, prefix, 0, s);\n - \t\tif (amend)\n + \told_display_comment_prefix = s->display_comment_prefix;\n + \ts->display_comment_prefix = 1;\n + \n +-\t/*\n +-\t * Most hints are counter-productive when the commit has\n +-\t * already started.\n +-\t */\n +-\ts->hints = 0;\n +-\n + \tif (clean_message_contents)\n + \t\tstrbuf_stripspace(&sb, 0);\n + \n +@@\n + \t\tint saved_color_setting;\n + \t\tstruct ident_split ci, ai;\n + \n ++\t\t/*\n ++\t\t * Most hints are counter-productive when displayed in\n ++\t\t * the commit message editor.\n ++\t\t */\n ++\t\ts->hints = 0;\n ++\n + \t\tif (whence != FROM_COMMIT) {\n + \t\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&\n + \t\t\t\t!merge_contains_scissors)\n +@@\n + \t\tsaved_color_setting = s->use_color;\n + \t\ts->use_color = 0;\n + \t\tcommittable = run_status(s->fp, index_file, prefix, 1, s);\n ++\t\tif(!committable)\n ++\t\t\t/*\n ++\t\t\t Status is to be printed to stdout, so hints will be useful to the\n ++\t\t\t user. Reset s->hints to what the user configured\n ++\t\t\t */\n ++\t\t\ts->hints = advice_status_hints;\n + \t\ts->use_color = saved_color_setting;\n + \t\tstring_list_clear(&s->change, 1);\n + \t} else {\n \n diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n --- a/t/t7500-commit-template-squash-signoff.sh\n @@ -56,7 +87,7 @@\n +\tgit add file &&\n +\tgit commit -m initial &&\n +\techo \"changes\" >>file &&\n -+\ttest_must_fail git commit -m initial >actual &&\n ++\ttest_must_fail git commit -m update >actual &&\n +\ttest_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n +'\n +\n\n-- \ngitgitgadget\n"},{"id":"388508","messageId":"ebec2379207681152c6e5196a1418aca03da113a.1576746982.git.gitgitgadget@gmail.com","threadId":"52471","inReplyTo":"pull.495.v2.git.1576746982.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Heba Waly via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-19T09:16:22Z","receivedAt":"2019-12-19T09:16:28Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"From: Heba Waly \n\nDisplay hints to the user when trying to commit without staging the\nmodified files first (when advice.statusHints is set to true). Change\nthe output of the unsuccessful commit from e.g:\n\n # [...]\n # Changes not staged for commit:\n # modified: builtin/commit.c\n #\n # no changes added to commit\n\nto:\n\n # [...]\n # Changes not staged for commit:\n # (use \"git add ...\" to update what will be committed)\n # (use \"git checkout -- ...\" to discard changes in working directory)\n #\n # modified: /builtin/commit.c\n #\n # no changes added to commit (use \"git add\" and/or \"git commit -a\")\n\nIn ea9882bfc4 (commit: disable status hints when writing to\nCOMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when\nwriting to COMMIT_EDITMSG, but in fact the implementation disabled\nstatus messages in more locations, e.g in case the commit wasn't\nsuccessful, status hints will still be disabled and no hints will be\ndisplayed to the user although advice.statusHints is set to true.\n\nSigned-off-by: Heba Waly \n---\n builtin/commit.c | 18 ++++++++++++------\n t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e48c1fd90a..868c0d7819 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -811,12 +811,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \told_display_comment_prefix = s->display_comment_prefix;\n \ts->display_comment_prefix = 1;\n \n-\t/*\n-\t * Most hints are counter-productive when the commit has\n-\t * already started.\n-\t */\n-\ts->hints = 0;\n-\n \tif (clean_message_contents)\n \t\tstrbuf_stripspace(&sb, 0);\n \n@@ -837,6 +831,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tint saved_color_setting;\n \t\tstruct ident_split ci, ai;\n \n+\t\t/*\n+\t\t * Most hints are counter-productive when displayed in\n+\t\t * the commit message editor.\n+\t\t */\n+\t\ts->hints = 0;\n+\n \t\tif (whence != FROM_COMMIT) {\n \t\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&\n \t\t\t\t!merge_contains_scissors)\n@@ -912,6 +912,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tsaved_color_setting = s->use_color;\n \t\ts->use_color = 0;\n \t\tcommittable = run_status(s->fp, index_file, prefix, 1, s);\n+\t\tif(!committable)\n+\t\t\t/*\n+\t\t\t Status is to be printed to stdout, so hints will be useful to the\n+\t\t\t user. Reset s->hints to what the user configured\n+\t\t\t */\n+\t\t\ts->hints = advice_status_hints;\n \t\ts->use_color = saved_color_setting;\n \t\tstring_list_clear(&s->change, 1);\n \t} else {\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 46a5cd4b73..a8179e4074 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '\n \t)\n '\n \n+test_expect_success 'commit without staging files fails and displays hints' '\n+\techo \"initial\" >>file &&\n+\tgit add file &&\n+\tgit commit -m initial &&\n+\techo \"changes\" >>file &&\n+\ttest_must_fail git commit -m update >actual &&\n+\ttest_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"388556","messageId":"xmqqlfr8b28v.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"pull.495.v2.git.1576746982.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-19T18:26:40Z","receivedAt":"2019-12-19T18:26:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Heba Waly via GitGitGadget\" writes:\n\n> @@ -19,16 +19,16 @@\n> # (use \"git add ...\" to update what will be committed)\n> # (use \"git checkout -- ...\" to discard changes in working directory)\n> #\n> - # modified: ../builtin/commit.c\n> + # modified: /builtin/commit.c\n\nReally?\n"},{"id":"388557","messageId":"20191219185427.GA227872@google.com","threadId":"52471","inReplyTo":"xmqqlfr8b28v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-19T18:54:27Z","receivedAt":"2019-12-19T18:58:41Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Dec 19, 2019 at 10:26:40AM -0800, Junio C Hamano wrote:\n> \"Heba Waly via GitGitGadget\" writes:\n> \n> > @@ -19,16 +19,16 @@\n> > # (use \"git add ...\" to update what will be committed)\n> > # (use \"git checkout -- ...\" to discard changes in working directory)\n> > #\n> > - # modified: ../builtin/commit.c\n> > + # modified: /builtin/commit.c\n> \n> Really?\n\nIt's hard to know what this cryptic comment means.. :)\n\nThis was a recommended change:\nhttps://lore.kernel.org/git/20191218031338.203382-1-jonathantanmy@google.com\n\nSince other changes were being made at the same time, I personally don't\nmind a little nit fix in the commit message.\n\nOr, do you mean that \"now it looks like the file is at the filesystem\nroot, which is wrong\"? It is indeed wrong now when it wasn't before. But\nI, for one, can't tell what you mean by just the one word.\n\n - Emily\n"},{"id":"388558","messageId":"xmqqfthgb01m.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"ebec2379207681152c6e5196a1418aca03da113a.1576746982.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-19T19:14:13Z","receivedAt":"2019-12-19T19:14:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Heba Waly via GitGitGadget\" writes:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e48c1fd90a..868c0d7819 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -811,12 +811,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \told_display_comment_prefix = s->display_comment_prefix;\n> \ts->display_comment_prefix = 1;\n> \n> -\t/*\n> -\t * Most hints are counter-productive when the commit has\n> -\t * already started.\n> -\t */\n> -\ts->hints = 0;\n> -\n\nHmm.\n\n> @@ -837,6 +831,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\tint saved_color_setting;\n> \t\tstruct ident_split ci, ai;\n> \n> +\t\t/*\n> +\t\t * Most hints are counter-productive when displayed in\n> +\t\t * the commit message editor.\n> +\t\t */\n> +\t\ts->hints = 0;\n> +\n\nWe no longer drop s->hints when we are not using editor and not\nincluding status (i.e. the \"else\" side) because these lines are\nmoved inside \"if\". As this change is not about that \"no editor\"\nside, I am not 100% convinced that this is a good change.\n\n> @@ -912,6 +912,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\tsaved_color_setting = s->use_color;\n> \t\ts->use_color = 0;\n> \t\tcommittable = run_status(s->fp, index_file, prefix, 1, s);\n> +\t\tif(!committable)\n\nStyle: SP between \"if\" and \"(\".\n\n> +\t\t\t/*\n> +\t\t\t Status is to be printed to stdout, so hints will be useful to the\n> +\t\t\t user. Reset s->hints to what the user configured\n> +\t\t\t */\n> +\t\t\ts->hints = advice_status_hints;\n\nThe \"if\" side has been changed to flip s->hints to the configured\nadvice hints value when !committable here. The \"else\" side\n(i.e. when we are not using editor and not including status) does\nnot do anything to s->hints after finding out if committable after\nthis change. Because \"s->hints = 0\" was moved to \"if\" with this\npatch, the \"else\" side no longer drops s->hints at all.\n\nSo the final run_status() called when the attempt to commit is\nrejected will feed s->hints that is not cleared with this change.\n\nIs that intended? Is the updated behaviour checked with a test?\n\n> \t\ts->use_color = saved_color_setting;\n> \t\tstring_list_clear(&s->change, 1);\n> \t} else {\n\nThis fix was about \"we do not want to unconditionally drop the\nadvice messages when we reject the attempt to commit and show the\noutput like 'git status'\", wasn't it? The earlier single-liner fix\nin v1 that flips s->hints just before calling run_status() before\nrejecting the attempt to commit was a lot easier to reason about, as\nthe fix was very focused and to the point. Why are we seeing this\nmany (seemingly unrelated) changes?\n\nPuzzled...\n"},{"id":"388559","messageId":"xmqqbls4aznl.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"xmqqfthgb01m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-19T19:22:38Z","receivedAt":"2019-12-19T19:22:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This fix was about \"we do not want to unconditionally drop the\n> advice messages when we reject the attempt to commit and show the\n> output like 'git status'\", wasn't it? The earlier single-liner fix\n> in v1 that flips s->hints just before calling run_status() before\n> rejecting the attempt to commit was a lot easier to reason about, as\n> the fix was very focused and to the point. Why are we seeing this\n> many (seemingly unrelated) changes?\n\nIn any case, here is what I tentatively have in my tree (with heavy\nrewrite to the proposed log message).\n\n-- >8 --\nFrom: Heba Waly \nDate: Tue, 17 Dec 2019 09:17:22 +0000\nSubject: [PATCH] commit: honor advice.statusHints when rejecting an empty\n commit\n\nIn ea9882bfc4 (commit: disable status hints when writing to\nCOMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints\nwhen writing to COMMIT_EDITMSG, because giving the hints in the \"git\nstatus\" like output in the commit message template are too late to\nbe useful (they say things like \"'git add' to stage\", but that is\nonly possible after aborting the current \"git commit\" session).\n\nBut there is one case that the hints can be useful: When the current\nattempt to commit is rejected because no change is recorded in the\nindex. The message is given and \"git commit\" errors out, so the\nhints can immediately be followed by the user. Teach the codepath\nto honor the configuration variable.\n\nSigned-off-by: Heba Waly \nSigned-off-by: Junio C Hamano \n---\n builtin/commit.c | 1 +\n t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e588bc6ad3..0078faf117 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -944,6 +944,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t */\n \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n \t !(amend && is_a_merge(current_head))) {\n+\t\ts->hints = advice_status_hints;\n \t\ts->display_comment_prefix = old_display_comment_prefix;\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 46a5cd4b73..a8179e4074 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '\n \t)\n '\n \n+test_expect_success 'commit without staging files fails and displays hints' '\n+\techo \"initial\" >>file &&\n+\tgit add file &&\n+\tgit commit -m initial &&\n+\techo \"changes\" >>file &&\n+\ttest_must_fail git commit -m update >actual &&\n+\ttest_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n+'\n+\n test_done\n-- \n2.24.1-732-ga9f9d4909c\n\n"},{"id":"388560","messageId":"xmqq7e2sazlh.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"20191219185427.GA227872@google.com","subject":"Re: [PATCH v2 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-19T19:23:54Z","receivedAt":"2019-12-19T19:24:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> On Thu, Dec 19, 2019 at 10:26:40AM -0800, Junio C Hamano wrote:\n>> \"Heba Waly via GitGitGadget\" writes:\n>> \n>> > @@ -19,16 +19,16 @@\n>> > # (use \"git add ...\" to update what will be committed)\n>> > # (use \"git checkout -- ...\" to discard changes in working directory)\n>> > #\n>> > - # modified: ../builtin/commit.c\n>> > + # modified: /builtin/commit.c\n>> \n>> Really?\n>\n> It's hard to know what this cryptic comment means.. :)\n>\n> This was a recommended change:\n> https://lore.kernel.org/git/20191218031338.203382-1-jonathantanmy@google.com\n>\n> Since other changes were being made at the same time, I personally don't\n> mind a little nit fix in the commit message.\n>\n> Or, do you mean that \"now it looks like the file is at the filesystem\n> root, which is wrong\"? It is indeed wrong now when it wasn't before. But\n> I, for one, can't tell what you mean by just the one word.\n\nThat is exactly the point. I am not in the business of spoon\nfeeding answers. I want my contributors to *think*.\n"},{"id":"388561","messageId":"xmqq36dgayma.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"xmqq7e2sazlh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-19T19:45:01Z","receivedAt":"2019-12-19T19:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Emily Shaffer writes:\n>\n>> On Thu, Dec 19, 2019 at 10:26:40AM -0800, Junio C Hamano wrote:\n>>> \"Heba Waly via GitGitGadget\" writes:\n>>> \n>>> > @@ -19,16 +19,16 @@\n>>> > # (use \"git add ...\" to update what will be committed)\n>>> > # (use \"git checkout -- ...\" to discard changes in working directory)\n>>> > #\n>>> > - # modified: ../builtin/commit.c\n>>> > + # modified: /builtin/commit.c\n>>> \n>>> Really?\n>>\n>> It's hard to know what this cryptic comment means.. :)\n>>\n>> This was a recommended change:\n>> https://lore.kernel.org/git/20191218031338.203382-1-jonathantanmy@google.com\n>>\n>> Since other changes were being made at the same time, I personally don't\n>> mind a little nit fix in the commit message.\n>>\n>> Or, do you mean that \"now it looks like the file is at the filesystem\n>> root, which is wrong\"? It is indeed wrong now when it wasn't before. But\n>> I, for one, can't tell what you mean by just the one word.\n>\n> That is exactly the point. I am not in the business of spoon\n> feeding answers. I want my contributors to *think*.\n\nAnd I am not being unnecessarily cryptic. If anybody thought a bit\nabout what the topic was about, looking at it it would immediately\nbe obvious that the sample output shown there is totally bogus, due\nto the leading slash.\n\nAny contributor working on this topic should be competent enough to\nrealize/notice it once it is pointed out---even though lack of\nproof-reading before sending may cause such a mistake by\ncarelessness. And that is what I wanted to convey by deliberately\na short response.\n\n\n"},{"id":"388562","messageId":"CAPig+cQZBXOZeYDJRH+9YLobTOP1_UndV_Snk+S0_LL1=h-aag@mail.gmail.com","threadId":"52471","inReplyTo":"xmqqbls4aznl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-19T19:47:49Z","receivedAt":"2019-12-19T19:48:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 19, 2019 at 2:22 PM Junio C Hamano wrote:\n> In any case, here is what I tentatively have in my tree (with heavy\n> rewrite to the proposed log message).\n>\n> +test_expect_success 'commit without staging files fails and displays hints' '\n> + echo \"initial\" >>file &&\n\nThe use of '>>' here rather than '>' feels wrong, especially when\n\"initial\" is used for both the file body and the commit message,\ncausing a reader of the test to wonder if this test somehow depends\nupon earlier tests.\n\n> + git add file &&\n> + git commit -m initial &&\n> + echo \"changes\" >>file &&\n> + test_must_fail git commit -m update >actual &&\n> + test_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n> +'\n"},{"id":"388564","messageId":"xmqqy2v89jgp.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"CAPig+cQZBXOZeYDJRH+9YLobTOP1_UndV_Snk+S0_LL1=h-aag@mail.gmail.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-19T19:57:42Z","receivedAt":"2019-12-19T19:57:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Thu, Dec 19, 2019 at 2:22 PM Junio C Hamano wrote:\n>> In any case, here is what I tentatively have in my tree (with heavy\n>> rewrite to the proposed log message).\n>>\n>> +test_expect_success 'commit without staging files fails and displays hints' '\n>> + echo \"initial\" >>file &&\n>\n> The use of '>>' here rather than '>' feels wrong, especially when\n> \"initial\" is used for both the file body and the commit message,\n> causing a reader of the test to wonder if this test somehow depends\n> upon earlier tests.\n\nYeah, makes sense. This was verbatim from v1 but I think starting\nthe file from scratch like you suggest makes it clearer what is\ngoing on.\n\n\n>\n>> + git add file &&\n>> + git commit -m initial &&\n>> + echo \"changes\" >>file &&\n>> + test_must_fail git commit -m update >actual &&\n>> + test_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n>> +'\n"},{"id":"388607","messageId":"20191220023125.GD227872@google.com","threadId":"52471","inReplyTo":"xmqqbls4aznl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-20T02:31:25Z","receivedAt":"2019-12-20T02:31:35Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Dec 19, 2019 at 11:22:38AM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > This fix was about \"we do not want to unconditionally drop the\n> > advice messages when we reject the attempt to commit and show the\n> > output like 'git status'\", wasn't it? The earlier single-liner fix\n> > in v1 that flips s->hints just before calling run_status() before\n> > rejecting the attempt to commit was a lot easier to reason about, as\n> > the fix was very focused and to the point. Why are we seeing this\n> > many (seemingly unrelated) changes?\n> \n> In any case, here is what I tentatively have in my tree (with heavy\n> rewrite to the proposed log message).\n\nHm. I'm surprised to see this feedback come in the form of a local\nchange when making the topic branch, rather than in a reply to the v1\npatch. What's the reasoning? (Or is this scissors patch intended to be\nthe feedback?)\n\nI ask because out of all of us, it seems the Outreachy interns can\nbenefit the most from advice on how and why to write their commit\nmessages - that is, part of the point of an internship is to learn best\npractices and cultural norms in addition to coding practice. (Plus, I\nfind being asked to rewrite a commit message tends to force me to\nunderstand my own change even better than before.)\n\nI'll go ahead and look through the changes to the commit message so I\ncan learn what you're looking for too :)\n\n> \n> -- >8 --\n> From: Heba Waly \n> Date: Tue, 17 Dec 2019 09:17:22 +0000\n> Subject: [PATCH] commit: honor advice.statusHints when rejecting an empty\n> commit\n> \n> In ea9882bfc4 (commit: disable status hints when writing to\n> COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints\n> when writing to COMMIT_EDITMSG, because giving the hints in the \"git\n> status\" like output in the commit message template are too late to\n> be useful (they say things like \"'git add' to stage\", but that is\n> only possible after aborting the current \"git commit\" session).\n\nMore context on why the previous change was made - \"by the time the\neditor was open, it was too late to apply hints anyways\". Sure.\n\n> \n> But there is one case that the hints can be useful: When the current\n> attempt to commit is rejected because no change is recorded in the\n> index. The message is given and \"git commit\" errors out, so the\n> hints can immediately be followed by the user. Teach the codepath\n> to honor the configuration variable.\n\nExpanding the \"but\" to supply the specific story this commit touches,\nincluding \"what happens instead\" and \"how are we gonna fix it\".\n\nAnd the copy-paste of the output before and the output now is different.\nFor me, I don't particularly see why we'd want to be rid of it - it sort\nof feels like \"a picture is worth a thousand words\" to include the\nactual use case in the commit message. Is there style guidance\nsuggesting not to do that that I missed?\n\n - Emily\n\n> \n> Signed-off-by: Heba Waly \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/commit.c | 1 +\n> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n> 2 files changed, 10 insertions(+)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e588bc6ad3..0078faf117 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -944,6 +944,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t */\n> \tif (!committable && whence != FROM_MERGE && !allow_empty &&\n> \t !(amend && is_a_merge(current_head))) {\n> +\t\ts->hints = advice_status_hints;\n> \t\ts->display_comment_prefix = old_display_comment_prefix;\n> \t\trun_status(stdout, index_file, prefix, 0, s);\n> \t\tif (amend)\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 46a5cd4b73..a8179e4074 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '\n> \t)\n> '\n> \n> +test_expect_success 'commit without staging files fails and displays hints' '\n> +\techo \"initial\" >>file &&\n> +\tgit add file &&\n> +\tgit commit -m initial &&\n> +\techo \"changes\" >>file &&\n> +\ttest_must_fail git commit -m update >actual &&\n> +\ttest_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n> +'\n> +\n> test_done\n> -- \n> 2.24.1-732-ga9f9d4909c\n> \n"},{"id":"388655","messageId":"xmqqbls2alsb.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"20191220023125.GD227872@google.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-20T18:34:28Z","receivedAt":"2019-12-20T18:34:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Hm. I'm surprised to see this feedback come in the form of a local\n> change when making the topic branch, rather than in a reply to the v1\n> patch. What's the reasoning? (Or is this scissors patch intended to be\n> the feedback?)\n\nYou haven't seen a suggestion in the form of counter-proposal?\n\n> I ask because out of all of us, it seems the Outreachy interns can\n> benefit the most from advice on how and why to write their commit\n> messages - that is, part of the point of an internship is to learn best\n> practices and cultural norms in addition to coding practice. (Plus, I\n> find being asked to rewrite a commit message tends to force me to\n> understand my own change even better than before.)\n\nIt's something Mentors can help doing (I do not necessarily have\ntime for that myself), and you're welcome to use the \"tenatively\nqueued\" version as an example.\n\n> I'll go ahead and look through the changes to the commit message so I\n> can learn what you're looking for too :)\n\nNice.\n\nOne thing you missed in your review of the \"tentatively queued\"\nversion is the reversal of the order of presentation. Instead of\nstarting with \"I decided to do this\" without explanation, give the\npicture of status quo to set the stage, explain what issue exists in\nthe current behaviour, and then describe what approach was chosen to\nsolve the issue.\n\n> For me, I don't particularly see why we'd want to be rid of it - it sort\n> of feels like \"a picture is worth a thousand words\" to include the\n> actual use case in the commit message.\n\nOutput coming from commands and/or options that are used only in a\nbit more advanced workflow and the ones that are rarely seen, I do\nagree that showing example is a good way to illustrate exactly what\nyou are talking about.\n\nOn the other hand, for behaviour of basic local commands like \"git\nadd\", \"git commit\", \"git diff\", ..., I do not necessarily agree, as\nthese should be obvious and clear to all the intended audiences,\nwhich would be \"anybody who has used Git for say more than two\nweeks.\n"},{"id":"388680","messageId":"20191220213926.GA1876@google.com","threadId":"52471","inReplyTo":"xmqqbls2alsb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-20T21:39:26Z","receivedAt":"2019-12-20T21:39:34Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Dec 20, 2019 at 10:34:28AM -0800, Junio C Hamano wrote:\n> Emily Shaffer writes:\n> \n> > Hm. I'm surprised to see this feedback come in the form of a local\n> > change when making the topic branch, rather than in a reply to the v1\n> > patch. What's the reasoning? (Or is this scissors patch intended to be\n> > the feedback?)\n> \n> You haven't seen a suggestion in the form of counter-proposal?\n\nI actually have only seen the scissors-patch as a \"yes, and\" in\npractice. I think this is a sign I should be doing more reviews ;)\n\n> \n> > I ask because out of all of us, it seems the Outreachy interns can\n> > benefit the most from advice on how and why to write their commit\n> > messages - that is, part of the point of an internship is to learn best\n> > practices and cultural norms in addition to coding practice. (Plus, I\n> > find being asked to rewrite a commit message tends to force me to\n> > understand my own change even better than before.)\n> \n> It's something Mentors can help doing (I do not necessarily have\n> time for that myself), and you're welcome to use the \"tenatively\n> queued\" version as an example.\n> \n> > I'll go ahead and look through the changes to the commit message so I\n> > can learn what you're looking for too :)\n> \n> Nice.\n> \n> One thing you missed in your review of the \"tentatively queued\"\n> version is the reversal of the order of presentation. Instead of\n> starting with \"I decided to do this\" without explanation, give the\n> picture of status quo to set the stage, explain what issue exists in\n> the current behaviour, and then describe what approach was chosen to\n> solve the issue.\n\nThanks for explaining this - that's a good point for me to take home.\n\n> \n> > For me, I don't particularly see why we'd want to be rid of it - it sort\n> > of feels like \"a picture is worth a thousand words\" to include the\n> > actual use case in the commit message.\n> \n> Output coming from commands and/or options that are used only in a\n> bit more advanced workflow and the ones that are rarely seen, I do\n> agree that showing example is a good way to illustrate exactly what\n> you are talking about.\n> \n> On the other hand, for behaviour of basic local commands like \"git\n> add\", \"git commit\", \"git diff\", ..., I do not necessarily agree, as\n> these should be obvious and clear to all the intended audiences,\n> which would be \"anybody who has used Git for say more than two\n> weeks.\n\nHm, I see. Thanks for clarifying.\n\n - Emily\n"},{"id":"388703","messageId":"CACg5j249Ttouiua7iSfMGi7ZaKOanb0TCbMFmEjVtzKsPur4PA@mail.gmail.com","threadId":"52471","inReplyTo":"xmqq36dgayma.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2019-12-21T04:37:16Z","receivedAt":"2019-12-21T04:37:31Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"On Fri, Dec 20, 2019 at 8:45 AM Junio C Hamano wrote:\n>\n> That is exactly the point. I am not in the business of spoon\n> feeding answers.\n\nWhy would pointing out a typo clearly to the author be considered as\nspoon feeding answers?\nRather than simply reviewing and pointing out mistakes in a clear and\na respectful way?\n\n> I want my contributors to *think*.\n\nNobody is against *thinking*, and I'm sure an author who found an\nissue and implemented a fix\nhad his/her share of thinking to get the job done.\nSo I hope a typo is not standing in the way of acknowledging the\nauthor's effort.\n\n> Any contributor working on this topic should be competent enough to\n> realize/notice it once it is pointed out---even though lack of\n> proof-reading before sending may cause such a mistake by\n> carelessness.\n\nI have no reason not to believe that this is crossing the line.\nNo matter how much a reviewer disagrees with the proposed changes, I'd\nappreciate\nkeeping a mutually respectful discussion during the review process.\n\nHeba\n"},{"id":"388704","messageId":"CACg5j254YMCsWrLPxGXitFa-PKqyyxwdkn2Wmq-2r1FgLFN0Mg@mail.gmail.com","threadId":"52471","inReplyTo":"pull.495.v2.git.1576746982.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2019-12-21T05:02:56Z","receivedAt":"2019-12-21T05:03:12Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"It was brought to my attention that I should've elaborated in the\ncover letter the difference between v1 and v2 and the reason behind\nthe change.\nSo I'll elaborate that here:\n\nAs pointed out by this review on v1\nhttps://lore.kernel.org/git/xmqq36dgayma.fsf@gitster-ct.c.googlers.com/T/#m3adc84664e907bd8fcc13ac22c8702ae15925f8f\n\nBy default git honors the user's selection of displaying hints, but in\nthe specific case of using the editor to write\na commit message ea9882bfc4 wanted to turn it off temporarily because\nwhile the editor is open and the commit\nis in-progress, the user can't run these hints anyway\n(list discussion for reference\nhttps://lore.kernel.org/git/vpq4n9tghk5.fsf@anie.imag.fr/)\n\nSo it makes more sense to see the change made for this specific editor\ncase inside an if condition rather than\napplying it to all the commit cases, and then flip it back once the\ncommit fails.\n\nPlease correct me if I'm wrong, but I think for the rest of the cases,\nas long as there's no editor with status\ndisplayed inside (hints displayed while commit in progress), there's\nno need to turn off the hints.\n\nHeba\n\n\nOn Thu, Dec 19, 2019 at 10:16 PM Heba Waly via GitGitGadget\n wrote:\n>\n> Display hints to the user when trying to commit without staging the modified\n> files first (when advice.statusHints is set to true). Change the output of\n> the unsuccessful commit from e.g:\n>\n> . [...] . Changes not staged for commit: . modified: builtin/commit.c . .\n> no changes added to commit\n>\n> to:\n>\n> . [...] . Changes not staged for commit: . (use \"git add ...\" to update\n> what will be committed) . (use \"git checkout -- ...\" to discard changes in\n> working directory) . . modified: ../builtin/commit.c . . no changes added to\n> commit (use \"git add\" and/or \"git commit -a\")\n>\n> In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n> 2013-09-12) the intent was to disable status hints when writing to\n> COMMIT_EDITMSG, but in fact the implementation disabled status messages in\n> more locations, e.g in case the commit wasn't successful, status hints will\n> still be disabled and no hints will be displayed to the user although\n> advice.statusHints is set to true.\n>\n> Signed-off-by: Heba Waly heba.waly@gmail.com [heba.waly@gmail.com]\n>\n> Heba Waly (1):\n> commit: display advice hints when commit fails\n>\n> builtin/commit.c | 18 ++++++++++++------\n> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n> 2 files changed, 21 insertions(+), 6 deletions(-)\n>\n>\n> base-commit: 12029dc57db23baef008e77db1909367599210ee\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-495%2FHebaWaly%2Fhints-for-unsuccessful-commit-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-495/HebaWaly/hints-for-unsuccessful-commit-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/495\n>\n> Range-diff vs v1:\n>\n> 1: f23477c5a3 ! 1: ebec237920 commit: display advice hints when commit fails\n> @@ -2,9 +2,9 @@\n>\n> commit: display advice hints when commit fails\n>\n> - Display hints to the user when trying to commit without staging the modified\n> - files first (when advice.statusHints is set to true). Change the output of the\n> - unsuccessful commit from e.g:\n> + Display hints to the user when trying to commit without staging the\n> + modified files first (when advice.statusHints is set to true). Change\n> + the output of the unsuccessful commit from e.g:\n>\n> # [...]\n> # Changes not staged for commit:\n> @@ -19,16 +19,16 @@\n> # (use \"git add ...\" to update what will be committed)\n> # (use \"git checkout -- ...\" to discard changes in working directory)\n> #\n> - # modified: ../builtin/commit.c\n> + # modified: /builtin/commit.c\n> #\n> # no changes added to commit (use \"git add\" and/or \"git commit -a\")\n>\n> - In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,\n> - 2013-09-12) the intent was to disable status hints when writing to\n> - COMMIT_EDITMSG, but in fact the implementation disabled status messages in\n> - more locations, e.g in case the commit wasn't successful, status hints\n> - will still be disabled and no hints will be displayed to the user although\n> - advice.statusHints is set to true.\n> + In ea9882bfc4 (commit: disable status hints when writing to\n> + COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when\n> + writing to COMMIT_EDITMSG, but in fact the implementation disabled\n> + status messages in more locations, e.g in case the commit wasn't\n> + successful, status hints will still be disabled and no hints will be\n> + displayed to the user although advice.statusHints is set to true.\n>\n> Signed-off-by: Heba Waly \n>\n> @@ -36,13 +36,44 @@\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@\n> - */\n> - if (!committable && whence != FROM_MERGE && !allow_empty &&\n> - !(amend && is_a_merge(current_head))) {\n> -+ s->hints = advice_status_hints;\n> - s->display_comment_prefix = old_display_comment_prefix;\n> - run_status(stdout, index_file, prefix, 0, s);\n> - if (amend)\n> + old_display_comment_prefix = s->display_comment_prefix;\n> + s->display_comment_prefix = 1;\n> +\n> +- /*\n> +- * Most hints are counter-productive when the commit has\n> +- * already started.\n> +- */\n> +- s->hints = 0;\n> +-\n> + if (clean_message_contents)\n> + strbuf_stripspace(&sb, 0);\n> +\n> +@@\n> + int saved_color_setting;\n> + struct ident_split ci, ai;\n> +\n> ++ /*\n> ++ * Most hints are counter-productive when displayed in\n> ++ * the commit message editor.\n> ++ */\n> ++ s->hints = 0;\n> ++\n> + if (whence != FROM_COMMIT) {\n> + if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&\n> + !merge_contains_scissors)\n> +@@\n> + saved_color_setting = s->use_color;\n> + s->use_color = 0;\n> + committable = run_status(s->fp, index_file, prefix, 1, s);\n> ++ if(!committable)\n> ++ /*\n> ++ Status is to be printed to stdout, so hints will be useful to the\n> ++ user. Reset s->hints to what the user configured\n> ++ */\n> ++ s->hints = advice_status_hints;\n> + s->use_color = saved_color_setting;\n> + string_list_clear(&s->change, 1);\n> + } else {\n>\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> @@ -56,7 +87,7 @@\n> + git add file &&\n> + git commit -m initial &&\n> + echo \"changes\" >>file &&\n> -+ test_must_fail git commit -m initial >actual &&\n> ++ test_must_fail git commit -m update >actual &&\n> + test_i18ngrep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n> +'\n> +\n>\n> --\n> gitgitgadget\n"},{"id":"388770","messageId":"xmqqa77l6xpo.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"CACg5j249Ttouiua7iSfMGi7ZaKOanb0TCbMFmEjVtzKsPur4PA@mail.gmail.com","subject":"Re: [PATCH v2 0/1] [Outreachy] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-21T23:54:59Z","receivedAt":"2019-12-21T23:55:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heba Waly writes:\n\n>> Any contributor working on this topic should be competent enough to\n>> realize/notice it once it is pointed out---even though lack of\n>> proof-reading before sending may cause such a mistake by\n>> carelessness.\n>\n> I have no reason not to believe that this is crossing the line.\n> No matter how much a reviewer disagrees with the proposed changes, I'd\n> appreciate\n> keeping a mutually respectful discussion during the review process.\n\nI do not see any lack of respect in saying that I believed you are\ncompetent enough that a short \"Huh?\" answer was sufficient.\n"},{"id":"389119","messageId":"20191231000420.32396-1-jonathantanmy@google.com","threadId":"52471","inReplyTo":"xmqqbls4aznl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-12-31T00:04:20Z","receivedAt":"2019-12-31T00:04:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > This fix was about \"we do not want to unconditionally drop the\n> > advice messages when we reject the attempt to commit and show the\n> > output like 'git status'\", wasn't it? The earlier single-liner fix\n> > in v1 that flips s->hints just before calling run_status() before\n> > rejecting the attempt to commit was a lot easier to reason about, as\n> > the fix was very focused and to the point. Why are we seeing this\n> > many (seemingly unrelated) changes?\n> \n> In any case, here is what I tentatively have in my tree (with heavy\n> rewrite to the proposed log message).\n\nJunio, what are your plans over what you have in your tree? If you'd\nlike to hear Heba's opinion on it, then she can chime in; if you'd like\na review, then I think it's good to go in.\n\nI think the main area of discussion is whether we should go with Heba's\nattempt to address Emily's comment [1]:\n\n> I think the intent of that commit was to not put hints into the editor,\n> so does it make sense to instead wrap this guy:\n> \n> /* \n> * Most hints are counter-productive when the commit has \n> * already started. \n> */ \n> s->hints = 0; \n> \n> in \"if (use_editor)\"?\n> \n> I didn't try it on my end. Maybe it won't help much, because we think\n> we're going to use the editor right up until we realize it's not\n> committable?\n\nAnd I think the answer to that is \"s\" is used throughout the function in\nvarious ways (in particular, used to print statuses both to stdout and\nto the message template) so any wrapping or corralling of scope would\njust make things more complicated. In particular, the way Heba did it in\nv2 is more unclear - at the time of setting s->hints = 0, it's done\nwithin a \"if (use_editor && include_status)\" block, but (as far as I can\ntell) the commit message template might also be used when there is no\neditor - for example, as input to a hook. And more importantly, when\ns->hints is reset to the config, we don't know at that point that the\nnext status is going to stdout. So I think it's better just to use the\nv1 way.\n\nThe second area of discussion I see is in the commit message. Commit\nmessages have to balance brevity and comprehensiveness, and this can be\na subjective matter, but I think Junio's strikes a good balance.\n\n[1] https://lore.kernel.org/git/20191217224541.GA230678@google.com/\n"},{"id":"389137","messageId":"xmqq7e2cfh68.fsf@gitster-ct.c.googlers.com","threadId":"52471","inReplyTo":"20191231000420.32396-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-31T19:06:55Z","receivedAt":"2019-12-31T19:07:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> In any case, here is what I tentatively have in my tree (with heavy\n>> rewrite to the proposed log message).\n>\n> Junio, what are your plans over what you have in your tree? If you'd\n> like to hear Heba's opinion on it, then she can chime in; if you'd like\n> a review, then I think it's good to go in.\n\nOn hold until anything like those happens ;-) \n\nA random reviewer mentioning something on a patch (either in a\nline-by-line critique form or \"how about doing it this way instead\"\ncounterproposal form) without getting followed up by others\n(including the original author) is a stall review thread, and it\ndoes not change the equation if the random reviewer happens to be me.\n\n>> I didn't try it on my end. Maybe it won't help much, because we think\n>> we're going to use the editor right up until we realize it's not\n>> committable?\n>\n> And I think the answer to that is \"s\" is used throughout the function in\n> various ways (in particular, used to print statuses both to stdout and\n> to the message template) so any wrapping or corralling of scope would\n> just make things more complicated. In particular, the way Heba did it in\n> v2 is more unclear - at the time of setting s->hints = 0, it's done\n\nYou mean \"less clear\" (just double checking if I got the negation right)?\n\n> within a \"if (use_editor && include_status)\" block, but (as far as I can\n> tell) the commit message template might also be used when there is no\n> editor - for example, as input to a hook. And more importantly, when\n> s->hints is reset to the config, we don't know at that point that the\n> next status is going to stdout. So I think it's better just to use the\n> v1 way.\n\nYeah, thanks for going back to compare v1 and v2, and I agree with\nyour assessment.\n\n> The second area of discussion I see is in the commit message. Commit\n> messages have to balance brevity and comprehensiveness, and this can be\n> a subjective matter, but I think Junio's strikes a good balance.\n\nAs one side of the comparison is my own, I won't be a good judge on\nthis, but yes I tried to strick a good balance as much as possible.\n\nI think I've merged it to 'next' yesterday, but it does not mean\nthat much as we are in -rc and it is not such an urgent \"oops we\nbroke it in this cycle, let's fix it\" issue. If we see a v3 that\nimproves it, I do not mind at all reverting what I merged to 'next'\nand use the updated one instead (either way, it will be in 'master'\nduring the next cycle at the earliest).\n\nThanks.\n"},{"id":"389172","messageId":"20200102195637.176142-1-jonathantanmy@google.com","threadId":"52471","inReplyTo":"xmqq7e2cfh68.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] commit: display advice hints when commit fails","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-02T19:56:37Z","receivedAt":"2020-01-02T19:56:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Junio, what are your plans over what you have in your tree? If you'd\n> > like to hear Heba's opinion on it, then she can chime in; if you'd like\n> > a review, then I think it's good to go in.\n> \n> On hold until anything like those happens ;-) \n> \n> A random reviewer mentioning something on a patch (either in a\n> line-by-line critique form or \"how about doing it this way instead\"\n> counterproposal form) without getting followed up by others\n> (including the original author) is a stall review thread, and it\n> does not change the equation if the random reviewer happens to be me.\n\nOK :-)\n\n> > And I think the answer to that is \"s\" is used throughout the function in\n> > various ways (in particular, used to print statuses both to stdout and\n> > to the message template) so any wrapping or corralling of scope would\n> > just make things more complicated. In particular, the way Heba did it in\n> > v2 is more unclear - at the time of setting s->hints = 0, it's done\n> \n> You mean \"less clear\" (just double checking if I got the negation right)?\n\nYes, less clear - v2 is less clear than v1.\n\n> I think I've merged it to 'next' yesterday, but it does not mean\n> that much as we are in -rc and it is not such an urgent \"oops we\n> broke it in this cycle, let's fix it\" issue. If we see a v3 that\n> improves it, I do not mind at all reverting what I merged to 'next'\n> and use the updated one instead (either way, it will be in 'master'\n> during the next cycle at the earliest).\n\nSounds good - thanks.\n"}]}