Re: [PATCH v2 1/1] advice: add --no-advice global option

["James Liu" <james@jamesliu.io>]

Thanks for the feedback Dragan!

> After having a more detailed look at the Git documentation,
> I think that adding support for "--advice" at the same time
> would be the right thing to do.  That option would override
> all "advice.<hint>" options that may exist in the configuration
> (and would override the GIT_NO_ADVICE environment variable,
> if we end up supporting it), similarly to how "--paginate"
> overrides all "pager.<cmd>" in the configuration, which would
> be both consistent and rather useful in some situations.

I think this makes sense from a UX consistenty perspective, but I'm not
sure if we should increase the scope of this patch. It's also much
easier to re-enable previously silenced advice hints, so I'm unsure
whether an --advice option makes sense. We can also avoid the decision
of what to do if the user supplies --advice and --no-advice
simultaneously if we only have one option.

> > diff --git a/advice.c b/advice.c
> > index 75111191ad..f6282c3bde 100644
> > --- a/advice.c
> > +++ b/advice.c
> > @@ -2,6 +2,7 @@
> >  #include "advice.h"
> >  #include "config.h"
> >  #include "color.h"
> > +#include "environment.h"
> >  #include "gettext.h"
> >  #include "help.h"
> >  #include "string-list.h"
> > @@ -126,7 +127,12 @@ void advise(const char *advice, ...)
> > 
> >  int advice_enabled(enum advice_type type)
> >  {
> > -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
> > +	int enabled;
> > +
> > +	if (getenv(GIT_NO_ADVICE))
> > +		return 0;
>
> Huh, I was under impression that having an environment
> variable to control this behavior was frowned upon by
> Junio? [1]  To me, supporting such a variable would be
> a somewhat acceptable risk, [2] but of course it's the
> maintainer's opinion that matters most.
>
> [1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/
> [2] 
> https://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/

You're correct. I saw this pattern for a few of the other global CLI
options and followed it. I'm unsure what the best alternative for
passing this configuration down to the `advice_enabled()` function is.
I'd appreciate some guidance here.

Cheers,
James