Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()

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

Rubén Justo <rjusto@gmail.com> writes:

>  advice.c          | 109 +++++++++++++++++++++++++---------------------
>  t/t0018-advice.sh |   1 -
>  2 files changed, 59 insertions(+), 51 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index f6e4c2f302..8474966ce1 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
>  	return "";
>  }
>  
> +enum advice_level {
> +	ADVICE_LEVEL_DEFAULT = 0,

We may want to say "_NONE" not "_DEFAULT" to match the convention
used elsewhere, but I have no strong preference.  I do have a
preference to see that, when we explicitly say "In this enum, there
is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the
definition, the places we use a variable of this enum type, we say

	if (!variable)

and not

	if (variable == ADVICE_LEVEL_DEFAULT)

> +	ADVICE_LEVEL_DISABLED,
> +	ADVICE_LEVEL_ENABLED,
> +};
> +
>  static struct {
>  	const char *key;
> -	int enabled;
> +	enum advice_level level;
>  } advice_setting[] = {
> ...
> -	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
> +	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
> ...
> +	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan" },
>  };

It certainly becomes nicer to use the "unspecified is left to 0"
convention like this.

>  static const char turn_off_instructions[] =
> @@ -116,13 +122,13 @@ void advise(const char *advice, ...)
>  
>  int advice_enabled(enum advice_type type)
>  {
> -	switch(type) {
> -	case ADVICE_PUSH_UPDATE_REJECTED:
> -		return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
> -		       advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
> -	default:
> -		return advice_setting[type].enabled;
> -	}
> +	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;

OK.

> +	if (type == ADVICE_PUSH_UPDATE_REJECTED)
> +		return enabled &&
> +		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);

I like the textbook use of a simple recursion here ;-)

> +	return enabled;
>  }

>  void advise_if_enabled(enum advice_type type, const char *advice, ...)
> @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
>  		return;
>  
>  	va_start(params, advice);
> -	vadvise(advice, 1, advice_setting[type].key, params);
> +	vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
> +		advice_setting[type].key, params);

OK.  Once you configure this advice to be always shown, you no
longer are using the _DEFAULT, so we pass 0 as the second parameter.
Makes sense.

As I said, if we explicitly document that _DEFAULT's value is zero
with "= 0" in the enum definition, which we also rely on the
initialization of the advice_setting[] array, then it may probably
be better to write it more like so:

	vadvice(advice, !advice_setting[type].level,
		advice_setting[type].key, params);

I wonder if it makes this caller simpler to update the return value
of advice_enabled() to a tristate.  Then we can do

	int enabled = advice_enabled(type);

	if (!enabled)
		return;
	va_start(...);
	vadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...);
	va_end(...);

but it does not make that much difference.

>  	va_end(params);
>  }
>  
> @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
>  	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
>  		if (strcasecmp(k, advice_setting[i].key))
>  			continue;
> -		advice_setting[i].enabled = git_config_bool(var, value);
> +		advice_setting[i].level = git_config_bool(var, value)
> +					  ? ADVICE_LEVEL_ENABLED
> +					  : ADVICE_LEVEL_DISABLED;

OK.  We saw (var, value) in the configuration files we are parsing,
so we find [i] that corresponds to the advice key and tweak the
level.

This loop obviously is O(N*M) for the number of "advice.*"
configuration variables N and the number of advice keys M, but that
is not new to this patch---our expectation is that N will be small,
even though M will grow over time.

> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index c13057a4ca..0dcfb760a2 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
>  test_expect_success 'advice should be printed when config variable is set to true' '
>  	cat >expect <<-\EOF &&
>  	hint: This is a piece of advice
> -	hint: Disable this message with "git config advice.nestedTag false"
>  	EOF
>  	test_config advice.nestedTag true &&
>  	test-tool advise "This is a piece of advice" 2>actual &&

OK.  The commit changes behaviour for existing users who explicitly
said "I want to see this advice" by configuring advice.* key to 'true',
and it probably is a good thing, even though it may be a bit surprising.

One thing that may be missing is a documentation update.  Something
along this line to highlight that now 'true' has a bit different
meaning from before (and as a consequence, we can no longer say
"defaults to true").

 Documentation/config/advice.txt | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt
index 2737381a11..364a8268b6 100644
--- c/Documentation/config/advice.txt
+++ w/Documentation/config/advice.txt
@@ -1,7 +1,11 @@
 advice.*::
-	These variables control various optional help messages designed to
-	aid new users. All 'advice.*' variables default to 'true', and you
-	can tell Git that you do not need help by setting these to 'false':
+
+	These variables control various optional help messages
+	designed to aid new users.  When left unconfigured, Git will
+	give you the help message with an instruction on how to
+	squelch it.  When set to 'true', the help message is given,
+	but without hint on how to squelch the message.  You can
+	tell Git that you do not need help by setting these to 'false':
 +
 --
 	ambiguousFetchRefspec::