Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()
- From
Jeff King <peff@peff.net>
- Date
- Jan 13, 2024, 07:38 UTC
- Message-ID
- <20240113073828.GB657764@coredump.intra.peff.net>
- In-Reply-To
- <xmqqsf326vpn.fsf@gitster.g>
On Fri, Jan 12, 2024 at 02:19:32PM -0800, Junio C Hamano wrote:
Show 28 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. 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)This may be getting into the realm of bikeshedding, but... ;)
For a tri-state we often use "-1" for "not specified". That has the nice side effect in this case that "if (level)" shows the advice (that works because "unspecified" and "explicitly true" both show the advice. And then "if (level < 0)" is used for just the hint. But maybe that is too clever/fragile.
Of course that means that all of the initializers have to use "-1" explicitly. So zero-initialization is sometimes nice, too.
-Peff