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@xxxxxxxxxxx/
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
