On Thu, Jan 02, 2020 at 11:54:11AM -0800, Junio C Hamano wrote:
> "Heba Waly via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Heba Waly <heba.waly@xxxxxxxxx>
> >
> > 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.
>
> 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.
Maybe this is my C++ habits not dying when they should :) but to me,
this begs the question, "why doesn't advise() check the toggles for me?"
Are advice messages 1:1 with advice settings? Is there a reason that
advise() doesn't look up its corresponding config for itself?
- Emily
>
> >
> > Signed-off-by: Heba Waly <heba.waly@xxxxxxxxx>
> > ---
> > 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 .'?"));
>
Hm, I guess this answers my question above about them being 1:1. But I
suppose it doesn't necessarily preclude advise() from associating a
single config with multiple advice messages.
- Emily
