Re: [PATCH 1/1] commit: display advice hints when commit fails

[Heba Waly <heba.waly@gmail.com>]

On Wed, Dec 18, 2019 at 11:45 AM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Tue, Dec 17, 2019 at 09:17:22AM +0000, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > 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 <file>..." to update what will be committed)
> >   #   (use "git checkout -- <file>..." 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@gmail.com>
> > ---
> >  builtin/commit.c                          | 1 +
> >  t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index 2db2ad0de4..4439666465 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> >        */
> >       if (!committable && whence != FROM_MERGE && !allow_empty &&
> >           !(amend && is_a_merge(current_head))) {
> > +             s->hints = advice_status_hints;
>
> Hm. This looks like it turns hints back on specifically for this case,
> but might not fix other places where ea9882bfc4 turned them off.
>
> I think the intent of that commit was to not put hints into the editor,
> so does it make sense to instead wrap this guy:
>
>   /*
>    * Most hints are counter-productive when the commit has
>    * already started.
>    */
>   s->hints = 0;
>
> in "if (use_editor)"?
>

That's a good idea, I tried it and it seems to be working fine.

> I didn't try it on my end. Maybe it won't help much, because we think
> we're going to use the editor right up until we realize it's not
> committable?
>
> I wonder which other cases that commit got rid of hints for by accident.
>

The number of cases is quite overwhelming because of all the options that
can be passed to the commit command, but hopefully after wrapping it in
an if condition as you suggested we'll be more certain of the affected cases.
Will send an update shortly.

>  - Emily
>
> >               s->display_comment_prefix = old_display_comment_prefix;
> >               run_status(stdout, index_file, prefix, 0, s);
> >               if (amend)
> > diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> > index 46a5cd4b73..3d76e8ebbd 100755
> > --- a/t/t7500-commit-template-squash-signoff.sh
> > +++ b/t/t7500-commit-template-squash-signoff.sh
> > @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
> >       )
> >  '
> >
> > +test_expect_success 'commit without staging files fails and displays hints' '
> > +     echo "initial" >>file &&
> > +     git add file &&
> > +     git commit -m initial &&
> > +     echo "changes" >>file &&
> > +     test_must_fail git commit -m initial >actual &&
> > +     test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
> > +'
> > +
> >  test_done
> > --
> > gitgitgadget

Thanks,

Heba