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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 2, 2020, 19:54 UTC
Message-ID: <xmqqpng1eisc.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <90608636bf184de76f91e4e04d9e796a021775a0.1577934241.git.gitgitgadget@gmail.com>

"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

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

Show 28 quoted lines

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