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

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Jan 02, 2020 at 11:54:11AM -0800, Junio C Hamano 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.
> 
> 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@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 .'?"));
> 

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