From: Jeff King <peff@peff.net>
Date: Wed, 09 Sep 2026 15:54:40 GMT
Subject: Re: [PATCH] advice: use global config for default branch name
Message-ID: <20260909155440.GA94069@coredump.intra.peff.net>
In-Reply-To: <xmqqik4fwoz5.fsf@gitster.g>

On Tue, Sep 08, 2026 at 11:57:18AM -0700, Junio C Hamano wrote:

> Vsevolod Myalitsin <ub4nal@mail.ru> writes:
> 
> > Yes, I agree that passing the "advice_setting" itself is cleaner and
> > more future-proof. I will change "vadvise()" to take a pointer to the
> > corresponding "advice_setting" instead.
> 
> One minor glitch is that there is an ad-hoc vadvise() call in
> advise() that is not tied to any particular entry in the
> advise_setting[] table.  I think we'd need to give a name to the
> advice_setting struct type, instanciate an ad-hoc instance on stack,
> and pass it down the callchain, perhaps like so:

Isn't this a natural fit for NULL? That ad-hoc call wants to pass the
notion that there is no matching advice config (or at least not that it
knows about). And then vadvise() can check:

  if (conf && !conf->level)
	...show instructions...

which seems natural to me.

As a side note, I think this is revealing some existing shortcomings in
the callers.  Most of the calls to advise() are doing something like:

  if (advice_is_enabled(ADVICE_FOO))
	advise("ask your doctor about foo");

Those won't get the "turn this off with advice.foo instructions". Only:

  advise_if_enabled(ADVICE_FOO, "ask your doctor about foo");

will. So there are many missed opportunities for offering the turn-off
instructions. Nobody seems to have complained, which makes me wonder if
the turn-off instructions would be annoyingly chatty if we printed them
all the time. Most of those calls predate the addition if the turn-off
instructions and advise_if_enabled(), which was added in 2020. I wonder
how people would feel if we converted them all and started printing the
turn-off instructions everywhere.

Anyway, UI philosophizing aside, another obvious pattern for advise()
is:

  if (advice_is_enabled(ADVICE_FOO)) {
	/* do lots of work */
	advise("try %s", results_of_work);
  }

which _wouldn't_ want to convert to advise_if_enabled(). If that wants
the turn-off message, we'd want to be able to pass the advice enum to
advise(), like:

  advise(ADVICE_FOO, "try %s", results_of_work);

at which point we might need a way to pass the NULL advice marker
somehow (for those cases which really aren't tied to a config value,
though arguably that is an anti-pattern in itself).

I guess the caller could just do:

  advise_if_enabled(ADVICE_FOO, ...);

inside the block. We know that it's enabled, but it's not like the check
is expensive.

-Peff