From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 09 Sep 2026 18:51:03 GMT
Subject: Re: [PATCH] advice: use global config for default branch name
Message-ID: <xmqqv78eqmw8.fsf@gitster.g>
In-Reply-To: <20260909155440.GA94069@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> 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?

Perfect.

> 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");

Yes, but all of these callers call advice_enabled() without _is ;-)

>
> 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.

Depends on how we do so, I guess.  Do you mean we should rewrite
advise() call above to advice_if_enabled(), even though the check
for ADVICE_FOO token appear redundant?

> 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);
>   }

Yes, checking with is-enabled primarily for the purpose of skipping
"do lots of work" is a very typical use.  I do not know why you
assume ...

>
> which _wouldn't_ want to convert to advise_if_enabled().

... this "try X" is something the users would not want to learn how
to disable, but assuming it is not, the existing code above as-is
should be what we want.

> 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.

Yes, I think we already have some callers that do so, in a pattern
where they want to skip the "do lots of work" part.  Or at least I
think I suggested the pattern in the past for somebody who wanted to
do that.