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

[Junio C Hamano <gitster@pobox.com>]

"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.

>
> 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".

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().

Thanks.