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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 2, 2020, 22:47 UTC
Message-ID: <xmqqzhf5cw69.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqqpng1eisc.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

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

A side note.

Right now, the advise() API is a bit awkweard to use correctly. When introducing a new advice message, you would

 * come up with advice.frotz configuration variable

 * define and declare advice_frotz global variable that defaults to
   true

 * sprinkle calls like this:

	if (advice_frotz)
		advise(_("helpful message about frotz"));

I am wondering about two things:

 (1) if we can update the API so that the above can be reduced to
     just adding calls like:

	advise_ng("frotz", _("helpful message about frotz"));

 (2) if such a simplified advise_ng API is a good idea to begin
     with.

There are a few advantages the current API has, but it cuts both ways.

 - Any new advice toggle MUST be registered to the
   advice.c::advice_config[] table.  This table can later be
   extended in the future to allow a list of the toggles to be
   produced at runtime.

   This can be seen as an easy mechanism to force programmers to
   keep the list up to date.  Or it can also be seen as the source
   of extra work.

 - advise() calls can be made without being guarded by any advice.*
   configuration variable.  In the overly simplified advise_ng() API
   shown above, we cannot expresss a pattern like this:

	if (advice_frotz) {
		... make expensive computation to
		... come up with values that need to be shown
		... in the advise() message
		char *result = expensive_computation(...);

		advise(_("message %s about frotz", result));
		free(result);
	}

   without adding another helper function. e.g.

	if (advise_ng_enabled("frotz")) {
		char *result = expensive_computation(...);

		/*
                 * advise_ng("frotz", _("message %s about frotz", result));
                 * is fine as well, but slightly less efficient as
                 * it would involve another call to *_enabled(), so use
		 * the unconditional form of the call
		 */
		advise_ng_raw(_("message %s about frotz", result));

		free(result);
	}