Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2024, 22:32 UTC
Message-ID: <xmqq8r4yf897.fsf@gitster.g>
In-Reply-To: <ZZ2QafUf/JxXYZU/@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 21 quoted lines

> On Tue, Jan 09, 2024 at 04:25:32PM +0100, Rubén Justo wrote:
>> Using advise_if_enabled() to display an advice will automatically
>> include instructions on how to disable the advice, along with the main
>> advice:
>>
>> 	hint: use --reapply-cherry-picks to include skipped commits
>> 	hint: Disable this message with "git config advice.skippedCherryPicks false"
>>
>> This may become distracting or "noisy" over time, while the user may
>> still want to receive the main advice.
>>
>> Let's have a switch to allow disabling this automatic advice.
>
> I reviewed this and had a couple of notes, mostly focused on what to
> call the new configuration option, and if we should be modifying the
> test-tool helpers to accept arbitrary configuration via command-line
> options.
>
> I think that we could reasonably drop the first two patches by
> imitating the existing style of t0018 more closely, but I don't feel all
> that strongly about it.

Even though I do not have a fundamental objection against test-tool learning "-c key=val", I do not see a strong need for the first two steps, either.

I actually have more problems with the primary change of this series (in addition to that configuration knob is probably misnamed, as you pointed out). How to disable the advice is an integral part of the end-user experience about the conditional advice system. The idea is to show it repeatedly, perhaps loud enough to be called "noisy", so that the user learns to follow the suggestion given by the hint. Until then, the user is expected to gradually learn to follow the suggestion more and more, seeing the advice message less and less often. If we rob the "how to disable THIS PARTICULAR message" and gave only this line:

>> 	hint: use --reapply-cherry-picks to include skipped commits

it would become impossible to find how to disable it, once the user get comfortable enough to pass --reapply-cherry-picks when it is appropriate to do so. I do not think there is no quick way other than grepping in the source to find that the user needs to disable the skippedCherryPicks message (no, you can look at the output from "git config --help" and find "skippedCherryPicks" if you know that is the symbol to be found, but there is nothing to link the above hint message to that particular help page entry).

I would understand if the proposed change were to change the "advice.<key>" configuration variable from a Boolean to a tristate (yes = default, no = disabled, always = give advice without instruction), though. IOW, the message might look like so:

    hint: use --reapply-cherry-picks to include skipped commits
    hint: setting advice.skippedCherryPicks to 'false' disables this message
    hint: and setting it to 'always' removes this additional instruction.

Then, those who find the hint always useful (because the particular mistake the hint is given against is something the user commits very rarely and the convenience of being reminded when it happens, which is rare, outweigh the burden of learning what is suggested) may want to set it to 'always' to accept that new choice.

But not the way the new feature is proposed.

Thanks.