Re: [PATCH 1/1] add: use advise function to display hints

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

On Fri, Jan 3, 2020 at 8:54 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Use the advise function in advice.c to display hints to the users, as
> > it provides a neat and a standard format for hint messages, i.e: the
> > text is colored in yellow and the line starts by the word "hint:".
>
> Use of advise() function is good for giving hints not just due to
> its yellow coloring (which by the way I find not very readable,
> perhaps because I use black ink on white paper).  One good thing in
> using the advise() API is that the messages can also be squelched
> with advice.* configuration variables.
>

Got it, thanks.

> And these two hints in "git add" are good chandidates to make
> customizable (perhaps with "advice.addNothing"), so I tend to agree
> with you that it makes sense to move these two messages to advise().
> Unfortunately this patch goes only halfway and stops (see below).
>
> If there are many other places that calls to advise() are made
> without getting guarded by the toggles defined in advice.c, we
> should fix them, I think.
>

Ok, we can address that in a separate patch.

> >
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> >  builtin/add.c  | 4 ++--
> >  t/t3700-add.sh | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index 4c38aff419..eebf8d772b 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -390,7 +390,7 @@ static int add_files(struct dir_struct *dir, int flags)
> >               fprintf(stderr, _(ignore_error));
> >               for (i = 0; i < dir->ignored_nr; i++)
> >                       fprintf(stderr, "%s\n", dir->ignored[i]->name);
> > -             fprintf(stderr, _("Use -f if you really want to add them.\n"));
> > +             advise(_("Use -f if you really want to add them.\n"));
> >               exit_status = 1;
> >       }
> >
> > @@ -480,7 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >
> >       if (require_pathspec && pathspec.nr == 0) {
> >               fprintf(stderr, _("Nothing specified, nothing added.\n"));
> > -             fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
> > +             advise( _("Maybe you wanted to say 'git add .'?\n"));
> >               return 0;
> >       }
>
> The final code for the above part would look like:
>
>                 if (advice_add_nothing)
>                         advise(_("Use -f if you really want to add them."));
>                 ...
>                 if (advice_add_nothing)
>                         advise( _("Maybe you wanted to say 'git add .'?"));
>
> and then you would
>
>  * add defn of advice_add_nothing to advice.h
>  * add decl of the same, initialized to 1(true), to advice.c
>  * map "addNothing" to &advice_add_nothing in advice.c::advice_config[]
>
> to complete the other half of this patch, if the config we choose to
> use is named "advice.addNothing".
>

Understood.


> By the way, notice that the single-liner advise() messages do not
> end with LF?  This is another difference between printf() family and
> advise().  advise() cuts its message at LF and prefixes each piece
> with "hint:" but after the final LF there is nothing but NUL, which
> means the final LF is optional.
>
> The warning()/error()/die() family is different from advise() in
> that they do not chop the incoming message at LF.  This behaviour is
> less i18n friendly, and it would be nice to eventually change them
> to behave similarly to advise().
>

Thank you for the extra tip.

> Thanks.
>
>

Heba