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

["Rubén Justo" <rjusto@gmail.com>]

On 12-ene-2024 14:19:32, Junio C Hamano wrote:
> 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.

The "_DEFAULT" name is rooted in the idea of having a default but
configurable value.  I'm not developing that idea in this series because
I'm not sure if it's desirable.  But, I'll leave a sketch here:

diff --git a/advice.c b/advice.c
index 8474966ce1..1bb427a8d8 100644
--- a/advice.c
+++ b/advice.c
@@ -39,6 +39,8 @@ enum advice_level {
 	ADVICE_LEVEL_ENABLED,
 };
 
+static enum advice_level advice_level_default;
+
 static struct {
 	const char *key;
 	enum advice_level level;
@@ -122,7 +124,9 @@ void advise(const char *advice, ...)
 
 int advice_enabled(enum advice_type type)
 {
-	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+	int enabled = advice_setting[type].level
+		      ? advice_setting[type].level != ADVICE_LEVEL_DISABLED
+		      : advice_level_default != ADVICE_LEVEL_DISABLED;
 
 	if (type == ADVICE_PUSH_UPDATE_REJECTED)
 		return enabled &&
@@ -139,7 +143,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
 		return;
 
 	va_start(params, advice);
-	vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
+	vadvise(advice, !advice_setting[type].level && !advice_level_default,
 		advice_setting[type].key, params);
 	va_end(params);
 }
@@ -166,6 +170,13 @@ int git_default_advice_config(const char *var, const char *value)
 	if (!skip_prefix(var, "advice.", &k))
 		return 0;
 
+	if (!strcmp(var, "advice.default")) {
+		advice_level_default = git_config_bool(var, value)
+				       ? ADVICE_LEVEL_ENABLED
+				       : ADVICE_LEVEL_DISABLED;
+		return 0;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
 		if (strcasecmp(k, advice_setting[i].key))
 			continue;

The way it is now, "_NONE" is perfectly fine.  I'll use it.

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

OK

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

Indeed, that's my feeling too.

> 
> >  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);

OK

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

I wonder if we can drop entirely this loop.  Maybe a hashmap can be
helpful here.  Of course, this is tangential to this series.

> 
> > 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',

Technically, when the user sets any value.

Maybe in the future we transform the knob to have more than two states,
beyond yes/no.  At that point, current rationality would still apply.

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

OK.  I'll add this.

Thanks.