From: Jeff King <peff@peff.net>
Date: Wed, 09 Sep 2026 19:51:32 GMT
Subject: Re: [PATCH] advice: use global config for default branch name
Message-ID: <20260909195132.GA182066@coredump.intra.peff.net>
In-Reply-To: <xmqqv78eqmw8.fsf@gitster.g>

On Wed, Sep 09, 2026 at 11:51:03AM -0700, Junio C Hamano wrote:

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

I mean we could mechanically rewrite:

  if (advice_enabled(ADVICE_FOO))
	advise(...);

to:

  advise_if_enabled(ADVICE_FOO, ...);

So the check wouldn't be redundant, but rather folded into the helper
function. The code becomes shorter, and the user-visible behavior
changes to produce the extra "turn-off" message.

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

I meant only that they would not want the same mechanical conversion
above, because that would lose the ability to avoid the extra work.

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

I think we do the same thing with trace_want() in a few spots.

-Peff