| Message ID | 7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | allow disabling the automatic hint in advise_if_enabled() | expand |
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. So I would probably expect a reroll of this series, but if you disagree with my comments, I would not be sad to see this series merged as-is. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > 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.
On 09-ene-2024 14:32:20, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > that configuration knob is probably misnamed, as you > pointed out). Agree. Maybe we have a better name: "showDisableInstructions". > 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. That's an interesting twist ... and intuitive it seems, as Peff also came up with a similar approach. But do we need, or want, that fine grain? Using advise_if_enabled() allows us to display a hint while automatically adding the option to disable it, _explicitly_ (so far*), to the user. But it happens that advise_if_enabled() itself has a hint to give. My goal in this series is about giving the user the possibility to disable _that_ hint (in the hint). The user choosing to disable that hint is telling us: "I know I can disable a hint, stop telling me so, please". So I don't think this opens up a new risk or problem finding how to disable a hint. $ git -c advice.showDisableInstructions=1 command_with_a_no_longer_welcomed_hint Is there a need to make that "hint in the hint" customizable _per hint_? That probably means that a new "make-it-always-for-all" may be desirable alongside the new tristate yes-no-always ... I dunno. * perhaps this multi-value possibility could be a path to explore what Taylor outlined in another message in this thread: the possibility of a "one-shot" hint.