It was brought to my attention that I should've elaborated in the cover letter the difference between v1 and v2 and the reason behind the change. So I'll elaborate that here: As pointed out by this review on v1 https://lore.kernel.org/git/xmqq36dgayma.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx/T/#m3adc84664e907bd8fcc13ac22c8702ae15925f8f By default git honors the user's selection of displaying hints, but in the specific case of using the editor to write a commit message ea9882bfc4 wanted to turn it off temporarily because while the editor is open and the commit is in-progress, the user can't run these hints anyway (list discussion for reference https://lore.kernel.org/git/vpq4n9tghk5.fsf@xxxxxxxxxxxx/) So it makes more sense to see the change made for this specific editor case inside an if condition rather than applying it to all the commit cases, and then flip it back once the commit fails. Please correct me if I'm wrong, but I think for the rest of the cases, as long as there's no editor with status displayed inside (hints displayed while commit in progress), there's no need to turn off the hints. Heba On Thu, Dec 19, 2019 at 10:16 PM Heba Waly via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > Display hints to the user when trying to commit without staging the modified > files first (when advice.statusHints is set to true). Change the output of > the unsuccessful commit from e.g: > > . [...] . Changes not staged for commit: . modified: builtin/commit.c . . > no changes added to commit > > to: > > . [...] . Changes not staged for commit: . (use "git add ..." to update > what will be committed) . (use "git checkout -- ..." to discard changes in > working directory) . . modified: ../builtin/commit.c . . no changes added to > commit (use "git add" and/or "git commit -a") > > In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, > 2013-09-12) the intent was to disable status hints when writing to > COMMIT_EDITMSG, but in fact the implementation disabled status messages in > more locations, e.g in case the commit wasn't successful, status hints will > still be disabled and no hints will be displayed to the user although > advice.statusHints is set to true. > > Signed-off-by: Heba Waly heba.waly@xxxxxxxxx [heba.waly@xxxxxxxxx] > > Heba Waly (1): > commit: display advice hints when commit fails > > builtin/commit.c | 18 ++++++++++++------ > t/t7500-commit-template-squash-signoff.sh | 9 +++++++++ > 2 files changed, 21 insertions(+), 6 deletions(-) > > > base-commit: 12029dc57db23baef008e77db1909367599210ee > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-495%2FHebaWaly%2Fhints-for-unsuccessful-commit-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-495/HebaWaly/hints-for-unsuccessful-commit-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/495 > > Range-diff vs v1: > > 1: f23477c5a3 ! 1: ebec237920 commit: display advice hints when commit fails > @@ -2,9 +2,9 @@ > > commit: display advice hints when commit fails > > - Display hints to the user when trying to commit without staging the modified > - files first (when advice.statusHints is set to true). Change the output of the > - unsuccessful commit from e.g: > + Display hints to the user when trying to commit without staging the > + modified files first (when advice.statusHints is set to true). Change > + the output of the unsuccessful commit from e.g: > > # [...] > # Changes not staged for commit: > @@ -19,16 +19,16 @@ > # (use "git add <file>..." to update what will be committed) > # (use "git checkout -- <file>..." to discard changes in working directory) > # > - # modified: ../builtin/commit.c > + # modified: /builtin/commit.c > # > # no changes added to commit (use "git add" and/or "git commit -a") > > - In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, > - 2013-09-12) the intent was to disable status hints when writing to > - COMMIT_EDITMSG, but in fact the implementation disabled status messages in > - more locations, e.g in case the commit wasn't successful, status hints > - will still be disabled and no hints will be displayed to the user although > - advice.statusHints is set to true. > + In ea9882bfc4 (commit: disable status hints when writing to > + COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when > + writing to COMMIT_EDITMSG, but in fact the implementation disabled > + status messages in more locations, e.g in case the commit wasn't > + successful, status hints will still be disabled and no hints will be > + displayed to the user although advice.statusHints is set to true. > > Signed-off-by: Heba Waly <heba.waly@xxxxxxxxx> > > @@ -36,13 +36,44 @@ > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ > - */ > - if (!committable && whence != FROM_MERGE && !allow_empty && > - !(amend && is_a_merge(current_head))) { > -+ s->hints = advice_status_hints; > - s->display_comment_prefix = old_display_comment_prefix; > - run_status(stdout, index_file, prefix, 0, s); > - if (amend) > + old_display_comment_prefix = s->display_comment_prefix; > + s->display_comment_prefix = 1; > + > +- /* > +- * Most hints are counter-productive when the commit has > +- * already started. > +- */ > +- s->hints = 0; > +- > + if (clean_message_contents) > + strbuf_stripspace(&sb, 0); > + > +@@ > + int saved_color_setting; > + struct ident_split ci, ai; > + > ++ /* > ++ * Most hints are counter-productive when displayed in > ++ * the commit message editor. > ++ */ > ++ s->hints = 0; > ++ > + if (whence != FROM_COMMIT) { > + if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS && > + !merge_contains_scissors) > +@@ > + saved_color_setting = s->use_color; > + s->use_color = 0; > + committable = run_status(s->fp, index_file, prefix, 1, s); > ++ if(!committable) > ++ /* > ++ Status is to be printed to stdout, so hints will be useful to the > ++ user. Reset s->hints to what the user configured > ++ */ > ++ s->hints = advice_status_hints; > + s->use_color = saved_color_setting; > + string_list_clear(&s->change, 1); > + } else { > > diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh > --- a/t/t7500-commit-template-squash-signoff.sh > @@ -56,7 +87,7 @@ > + git add file && > + git commit -m initial && > + echo "changes" >>file && > -+ test_must_fail git commit -m initial >actual && > ++ test_must_fail git commit -m update >actual && > + test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual > +' > + > > -- > gitgitgadget
