git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Rubén Justo <rjusto@gmail.com>
Date
Jan 15, 2024, 11:24 UTC
Message-ID
<7cb6cae4-3de7-4c2f-8e4c-ea653b6b802f@gmail.com>
In-Reply-To
<xmqqsf326vpn.fsf@gitster.g>
On 12-ene-2024 14:19:32, Junio C Hamano wrote:
Show 19 quoted lines
> 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.
Previous: Junio C HamanoNext: Rubén Justo
Message 35 of 36 in “allow disabling the automatic hint in advise_if_enabled()”
  1. 0/3 allow disabling the automatic hint in advise_if_enabled()Rubén Justo, Jan 9, 2024
  2. 1/3 t/test-tool: usage descriptionRubén Justo, Jan 9, 2024
  3. Junio C HamanoJan 9, 2024
  4. 2/3 t/test-tool: handle -c <name>=<value> argumentsRubén Justo, Jan 9, 2024
  5. Junio C HamanoJan 9, 2024
  6. Taylor BlauJan 9, 2024
  7. 3/3 advice: allow disabling the automatic hint in advise_if_enabled()Rubén Justo, Jan 9, 2024
  8. Junio C HamanoJan 9, 2024
  9. Taylor BlauJan 9, 2024
  10. Junio C HamanoJan 9, 2024
  11. Rubén JustoJan 10, 2024
  12. Jeff KingJan 10, 2024
  13. Rubén JustoJan 10, 2024
  14. Dragan SimicJan 10, 2024
  15. Rubén JustoJan 10, 2024
  16. Dragan SimicJan 10, 2024
  17. Junio C HamanoJan 10, 2024
  18. Dragan SimicJan 10, 2024
  19. Jeff KingJan 11, 2024
  20. Dragan SimicJan 18, 2024
  21. Junio C HamanoJan 18, 2024
  22. Dragan SimicJan 18, 2024
  23. Junio C HamanoJan 18, 2024
  24. Dragan SimicJan 18, 2024
  25. Rubén JustoJan 20, 2024
  26. Dragan SimicJan 20, 2024
  27. Junio C HamanoJan 10, 2024
  28. Taylor BlauJan 9, 2024
  29. Junio C HamanoJan 9, 2024
  30. Rubén JustoJan 10, 2024
  31. advice: allow disabling the automatic hint in advise_if_enabled()Rubén Justo, Jan 12, 2024
  32. Junio C HamanoJan 12, 2024
  33. Jeff KingJan 13, 2024
  34. Junio C HamanoJan 16, 2024
  35. Rubén JustoJan 15, 2024
  36. advice: allow disabling the automatic hint in advise_if_enabled()Rubén Justo, Jan 15, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.