| Message ID | d6099d78-43c6-4709-9121-11f84228cf91@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | allow disabling the automatic hint in advise_if_enabled() | expand |
Rubén Justo <rjusto@gmail.com> writes: > 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 can 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. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > advice.c | 3 ++- > advice.h | 3 ++- > t/t0018-advice.sh | 8 ++++++++ > 3 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/advice.c b/advice.c > index 50c79443ba..fa203f8806 100644 > --- a/advice.c > +++ b/advice.c > @@ -79,6 +79,7 @@ static struct { > [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 }, > [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, > [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 }, > + [ADVICE_ADVICE_OFF] = { "adviceOff", 1 }, > }; > > static const char turn_off_instructions[] = > @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions, > > strbuf_vaddf(&buf, advice, params); > > - if (display_instructions) > + if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF)) > strbuf_addf(&buf, turn_off_instructions, key); > > for (cp = buf.buf; *cp; cp = np) { > diff --git a/advice.h b/advice.h > index 2affbe1426..1f2eef034e 100644 > --- a/advice.h > +++ b/advice.h > @@ -10,7 +10,7 @@ struct string_list; > * Add the new config variable to Documentation/config/advice.txt. > * Call advise_if_enabled to print your advice. > */ > - enum advice_type { > +enum advice_type { > ADVICE_ADD_EMBEDDED_REPO, > ADVICE_ADD_EMPTY_PATHSPEC, > ADVICE_ADD_IGNORED_FILE, > @@ -50,6 +50,7 @@ struct string_list; > ADVICE_WAITING_FOR_EDITOR, > ADVICE_SKIPPED_CHERRY_PICKS, > ADVICE_WORKTREE_ADD_ORPHAN, > + ADVICE_ADVICE_OFF, > }; > > int git_default_advice_config(const char *var, const char *value); > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh > index c13057a4ca..0b6a8b4a10 100755 > --- a/t/t0018-advice.sh > +++ b/t/t0018-advice.sh > @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to > test_must_be_empty actual > ' > > +test_expect_success 'advice without the instructions to disable it' ' > + cat >expect <<-\EOF && > + hint: This is a piece of advice > + EOF > + test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual && > + test_cmp expect actual > +' This is testing the right thing but in a "showing off a shiny new toy" way. We want to make sure we will catch regressions in the future by testing with a bit more conditions perturbed. For example, with the new "-c var=val" mechanism, we could * set advice.nestedtag to off (which would disable the whole advice) * set advice.adviceoff to on (which should be the same as not setting it explicitly at all). to test different combinations that we were unable to test before [2/3] invented the mechanism. Thanks.
On Tue, Jan 09, 2024 at 04:30:16PM +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 can become distracting or noisy over time, while the user may > still want to receive the main advice. Presumably for more trivial pieces of advice, a user may want to immediately disable those hints in the future more quickly after first receiving the advice, in which case this feature may not be as useful for them. But for trickier pieces of advice, I agree completely with your reasoning and think that something like this makes sense. > Let's have a switch to allow disabling this automatic advice. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > advice.c | 3 ++- > advice.h | 3 ++- > t/t0018-advice.sh | 8 ++++++++ > 3 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/advice.c b/advice.c > index 50c79443ba..fa203f8806 100644 > --- a/advice.c > +++ b/advice.c > @@ -79,6 +79,7 @@ static struct { > [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 }, > [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, > [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 }, > + [ADVICE_ADVICE_OFF] = { "adviceOff", 1 }, The name seems to imply that setting `advice.adviceOff=true` would cause Git to suppress the turn-off instructions. But... > }; > > static const char turn_off_instructions[] = > @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions, > > strbuf_vaddf(&buf, advice, params); > > - if (display_instructions) > + if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF)) > strbuf_addf(&buf, turn_off_instructions, key); ...it looks like the opposite is true. I guess the "adviceOff" part of this new configuration option suggests "show me advice on how to turn off advice of xyz kind in the future". Perhaps a clearer alternative might be "advice.showDisableInstructions" or something? I don't know, coming up with a direct/clear name of this configuration is challenging for me. > > for (cp = buf.buf; *cp; cp = np) { > diff --git a/advice.h b/advice.h > index 2affbe1426..1f2eef034e 100644 > --- a/advice.h > +++ b/advice.h > @@ -10,7 +10,7 @@ struct string_list; > * Add the new config variable to Documentation/config/advice.txt. > * Call advise_if_enabled to print your advice. > */ > - enum advice_type { > +enum advice_type { > ADVICE_ADD_EMBEDDED_REPO, > ADVICE_ADD_EMPTY_PATHSPEC, > ADVICE_ADD_IGNORED_FILE, > @@ -50,6 +50,7 @@ struct string_list; > ADVICE_WAITING_FOR_EDITOR, > ADVICE_SKIPPED_CHERRY_PICKS, > ADVICE_WORKTREE_ADD_ORPHAN, > + ADVICE_ADVICE_OFF, > }; > > int git_default_advice_config(const char *var, const char *value); > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh > index c13057a4ca..0b6a8b4a10 100755 > --- a/t/t0018-advice.sh > +++ b/t/t0018-advice.sh > @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to > test_must_be_empty actual > ' > > +test_expect_success 'advice without the instructions to disable it' ' > + cat >expect <<-\EOF && > + hint: This is a piece of advice > + EOF > + test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual && > + test_cmp expect actual > +' Looking at this test, I wonder why we don't imitate the existing style of: test_config advice.adviceOff false && test-tool advise "This is a piece of advice" 2>actual && test_cmp expect actual instead of teaching the test-tool helpers how to interpret `-c` arguments. Doing so would allow us to drop the first couple of patches in this series and simplify things a bit. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > Looking at this test, I wonder why we don't imitate the existing style > of: > > test_config advice.adviceOff false && > test-tool advise "This is a piece of advice" 2>actual && > test_cmp expect actual > > instead of teaching the test-tool helpers how to interpret `-c` > arguments. Doing so would allow us to drop the first couple of patches > in this series and simplify things a bit. Thanks for a dose of sanity. I too got a bit too excited by a shiny new toy ;-) Reusing the existing mechanism does make sense.
On Tue, Jan 09, 2024 at 04:30:16PM +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 can 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. If I'm reading your patch correctly, this is a single option that controls the extra line for _all_ advice messages. But I'd have expected this to be something you'd want to set on a per-message basis. Here's my thinking. The original idea for advice messages was that they might be verbose and annoying, but if you had one that showed up a lot you'd choose to shut it up individually. But you wouldn't do so for _all_ messages, because you might benefit from seeing others (including new ones that get added). The "Disable this..." part was added later to help you easily know which config option to tweak. The expectation was that you'd fall into one of two categories: 1. You don't see the message often enough to care, so you do nothing. 2. You do find it annoying, so you disable this instance. Your series proposes a third state: 3. You find the actual hint useful, but the verbosity of "how to shut it up" is too much for you. That make sense to me, along with being able to partially shut-up a message. But wouldn't you still need the "how to shut up" hint for _other_ messages, since it's customized for each situation? E.g., suppose that after getting annoyed at advice.skippedCherryPicks, you run "git config advice.adviseOff false". But now you run into another hint, like: $ git foo hint: you can use --empty-commits to deal with isn't as good as --xyzzy and you want to disable it entirely. Which advice.* config option does so? You're stuck trying to find it in the manpage (which is tedious but also kind of tricky since you're now guessing which name goes with which message). You probably do want: hint: Disable this message with "git config advice.xyzzy false" in that case (at which point you may decide to silence it completely or partially). Which implies to me that the value of advice.* should be a tri-state to match the cases above: true, false, or a "minimal" / "quiet" mode (there might be a better name). And then you'd do: git config advice.skippedCherryPicks minimal (or whatever it is called) to get the mode you want, without affecting advice.xyzzy. > advice.c | 3 ++- > advice.h | 3 ++- > t/t0018-advice.sh | 8 ++++++++ > 3 files changed, 12 insertions(+), 2 deletions(-) Speaking of manpages, we'd presumably need an update to Documentation/config/advice.txt. :) -Peff
On 10-ene-2024 06:02:26, Jeff King wrote: > On Tue, Jan 09, 2024 at 04:30:16PM +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 can 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. > > If I'm reading your patch correctly, this is a single option that > controls the extra line for _all_ advice messages. But I'd have expected > this to be something you'd want to set on a per-message basis. Here's my > thinking. > > The original idea for advice messages was that they might be verbose and > annoying, but if you had one that showed up a lot you'd choose to shut > it up individually. But you wouldn't do so for _all_ messages, because > you might benefit from seeing others (including new ones that get > added). The "Disable this..." part was added later to help you easily > know which config option to tweak. > > The expectation was that you'd fall into one of two categories: > > 1. You don't see the message often enough to care, so you do nothing. > > 2. You do find it annoying, so you disable this instance. > > Your series proposes a third state: > > 3. You find the actual hint useful, but the verbosity of "how to shut > it up" is too much for you. > > That make sense to me, along with being able to partially shut-up a > message. But wouldn't you still need the "how to shut up" hint for > _other_ messages, since it's customized for each situation? > > E.g., suppose that after getting annoyed at advice.skippedCherryPicks, > you run "git config advice.adviseOff false". > > But now you run into another hint, like: > > $ git foo > hint: you can use --empty-commits to deal with isn't as good as --xyzzy > > and you want to disable it entirely. Which advice.* config option does > so? You're stuck trying to find it in the manpage (which is tedious but > also kind of tricky since you're now guessing which name goes with which > message). You probably do want: > > hint: Disable this message with "git config advice.xyzzy false" > > in that case (at which point you may decide to silence it completely or > partially). > > Which implies to me that the value of advice.* should be a tri-state to > match the cases above: true, false, or a "minimal" / "quiet" mode (there > might be a better name). And then you'd do: > > git config advice.skippedCherryPicks minimal > > (or whatever it is called) to get the mode you want, without affecting > advice.xyzzy. Your reasoning is sensible, but I'm not sure if we want that level of granularity. My guts doesn't feel it, though I'm not opposed. In the message before yours in this thread, Junio proposed a similar approach, and I've been thinking about it. Let me answer to your comments from that message too. > > > advice.c | 3 ++- > > advice.h | 3 ++- > > t/t0018-advice.sh | 8 ++++++++ > > 3 files changed, 12 insertions(+), 2 deletions(-) > > Speaking of manpages, we'd presumably need an update to > Documentation/config/advice.txt. :) This has made me jump first to this message in the thread ... I missed this. Thank you! > > -Peff
On 09-ene-2024 13:27:37, Taylor Blau wrote: > > + [ADVICE_ADVICE_OFF] = { "adviceOff", 1 }, > > The name seems to imply that setting `advice.adviceOff=true` would cause > Git to suppress the turn-off instructions. But... > > > }; > > > > static const char turn_off_instructions[] = > > @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions, > > > > strbuf_vaddf(&buf, advice, params); > > > > - if (display_instructions) > > + if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF)) > > strbuf_addf(&buf, turn_off_instructions, key); > > ...it looks like the opposite is true. I guess the "adviceOff" part of > this new configuration option suggests "show me advice on how to turn > off advice of xyz kind in the future". > > Perhaps a clearer alternative might be "advice.showDisableInstructions" > or something? Yeah. We can then use ADVICE_SHOW_DISABLE_INSTRUCTIONS, which is also a better name than the current ADVICE_ADVICE_OFF. Thanks. I'll reroll also with a description of it in Documentation/config/advice.txt, as Jeff has pointed out. > I don't know, coming up with a direct/clear name of this > configuration is challenging for me. Well ... I'm not going to show the previous names I've been considering for this ;-) > > > > > for (cp = buf.buf; *cp; cp = np) { > > diff --git a/advice.h b/advice.h > > index 2affbe1426..1f2eef034e 100644 > > --- a/advice.h > > +++ b/advice.h > > @@ -10,7 +10,7 @@ struct string_list; > > * Add the new config variable to Documentation/config/advice.txt. > > * Call advise_if_enabled to print your advice. > > */ > > - enum advice_type { > > +enum advice_type { > > ADVICE_ADD_EMBEDDED_REPO, > > ADVICE_ADD_EMPTY_PATHSPEC, > > ADVICE_ADD_IGNORED_FILE, > > @@ -50,6 +50,7 @@ struct string_list; > > ADVICE_WAITING_FOR_EDITOR, > > ADVICE_SKIPPED_CHERRY_PICKS, > > ADVICE_WORKTREE_ADD_ORPHAN, > > + ADVICE_ADVICE_OFF, > > }; > > > > int git_default_advice_config(const char *var, const char *value); > > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh > > index c13057a4ca..0b6a8b4a10 100755 > > --- a/t/t0018-advice.sh > > +++ b/t/t0018-advice.sh > > @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to > > test_must_be_empty actual > > ' > > > > +test_expect_success 'advice without the instructions to disable it' ' > > + cat >expect <<-\EOF && > > + hint: This is a piece of advice > > + EOF > > + test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual && > > + test_cmp expect actual > > +' > > Looking at this test, I wonder why we don't imitate the existing style > of: > > test_config advice.adviceOff false && > test-tool advise "This is a piece of advice" 2>actual && > test_cmp expect actual As a reference, implicitly, that is: git config advice.adviceOff false && test_when_finished "git config --unset-all advice.adviceOff" && test-tool advise "This is a piece of advice" 2>actual && test_cmp expect actual I think the proposed test is better and understandable enough. As a matter of fact, when I was thinking how to write the test I was expecting to have a working "-c" in test-tool, as if we have a (not expected) "git-advise". So ... > > instead of teaching the test-tool helpers how to interpret `-c` > arguments. Doing so would allow us to drop the first couple of patches > in this series and simplify things a bit. ... IMHO the first two patches allows us to write simpler and more intuitive tests based on test-tool. I hope this argument persuades you, but I am not against your proposal. > > Thanks, > Taylor Thank you for reviewing this series.
On 2024-01-10 12:02, Jeff King wrote: > On Tue, Jan 09, 2024 at 04:30:16PM +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 can 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. > > If I'm reading your patch correctly, this is a single option that > controls the extra line for _all_ advice messages. But I'd have > expected > this to be something you'd want to set on a per-message basis. Here's > my > thinking. > > The original idea for advice messages was that they might be verbose > and > annoying, but if you had one that showed up a lot you'd choose to shut > it up individually. But you wouldn't do so for _all_ messages, because > you might benefit from seeing others (including new ones that get > added). The "Disable this..." part was added later to help you easily > know which config option to tweak. Just to chime in and support this behavior of the advice messages. Basically, you don't want to have them all disabled at the same time, but to have per-message enable/disable granularity. I'd guess that some of the messages are quite usable even to highly experienced users, and encountering newly added messages is also very useful. Thus, having them all disabled wouldn't be the best idea. > The expectation was that you'd fall into one of two categories: > > 1. You don't see the message often enough to care, so you do nothing. > > 2. You do find it annoying, so you disable this instance. > > Your series proposes a third state: > > 3. You find the actual hint useful, but the verbosity of "how to shut > it up" is too much for you. > > That make sense to me, along with being able to partially shut-up a > message. But wouldn't you still need the "how to shut up" hint for > _other_ messages, since it's customized for each situation? > > E.g., suppose that after getting annoyed at advice.skippedCherryPicks, > you run "git config advice.adviseOff false". > > But now you run into another hint, like: > > $ git foo > hint: you can use --empty-commits to deal with isn't as good as > --xyzzy > > and you want to disable it entirely. Which advice.* config option does > so? You're stuck trying to find it in the manpage (which is tedious but > also kind of tricky since you're now guessing which name goes with > which > message). You probably do want: > > hint: Disable this message with "git config advice.xyzzy false" > > in that case (at which point you may decide to silence it completely or > partially). > > Which implies to me that the value of advice.* should be a tri-state to > match the cases above: true, false, or a "minimal" / "quiet" mode > (there > might be a better name). And then you'd do: > > git config advice.skippedCherryPicks minimal > > (or whatever it is called) to get the mode you want, without affecting > advice.xyzzy. > >> advice.c | 3 ++- >> advice.h | 3 ++- >> t/t0018-advice.sh | 8 ++++++++ >> 3 files changed, 12 insertions(+), 2 deletions(-) > > Speaking of manpages, we'd presumably need an update to > Documentation/config/advice.txt. :) > > -Peff
On 10-ene-2024 15:18:17, Dragan Simic wrote: > On 2024-01-10 12:02, Jeff King wrote: > Just to chime in and support this behavior of the advice messages. > Basically, you don't want to have them all disabled at the same time, but to > have per-message enable/disable granularity. I'd guess that some of the > messages are quite usable even to highly experienced users, and encountering > newly added messages is also very useful. Thus, having them all disabled > wouldn't be the best idea. Totally agree. This series is about disabling _the part in the advice about how to disable the advice_, but not the proper advice. Maybe the name ADVICE_ADVICE_OFF has caused confusion. Sorry if so.
On 2024-01-10 15:32, Rubén Justo wrote: > On 10-ene-2024 15:18:17, Dragan Simic wrote: >> On 2024-01-10 12:02, Jeff King wrote: > >> Just to chime in and support this behavior of the advice messages. >> Basically, you don't want to have them all disabled at the same time, >> but to >> have per-message enable/disable granularity. I'd guess that some of >> the >> messages are quite usable even to highly experienced users, and >> encountering >> newly added messages is also very useful. Thus, having them all >> disabled >> wouldn't be the best idea. > > Totally agree. > > This series is about disabling _the part in the advice about how to > disable the advice_, but not the proper advice. > > Maybe the name ADVICE_ADVICE_OFF has caused confusion. Sorry if so. No worries. Regarding disabling the advices for disabling the advice messages, maybe it would be better to have only one configuration knob for that purpose, e.g. "core.verboseAdvice", as a boolean knob. That way, fishing for the right knob for some advice message wouldn't turn itself into an issue, and the users would be able to turn the additional "advices about advices" on and off easily, to be able to see what knobs actually turn off the advice messages that have become annoying enough to them.
Jeff King <peff@peff.net> writes: > If I'm reading your patch correctly, this is a single option that > controls the extra line for _all_ advice messages. But I'd have expected > this to be something you'd want to set on a per-message basis. Here's my > thinking. > > The original idea for advice messages was that they might be verbose and > annoying, but if you had one that showed up a lot you'd choose to shut > it up individually. But you wouldn't do so for _all_ messages, because > you might benefit from seeing others (including new ones that get > added). The "Disable this..." part was added later to help you easily > know which config option to tweak. > > The expectation was that you'd fall into one of two categories: > > 1. You don't see the message often enough to care, so you do nothing. > > 2. You do find it annoying, so you disable this instance. > > Your series proposes a third state: > > 3. You find the actual hint useful, but the verbosity of "how to shut > it up" is too much for you. > > That make sense to me, along with being able to partially shut-up a > message. But wouldn't you still need the "how to shut up" hint for > _other_ messages, since it's customized for each situation? Thanks for saying what I wanted to say in my one of the messages much clearly than I could. The above is exactly why I would be more sympathetic to "advice.foo = (yes/no/always)".
Dragan Simic <dsimic@manjaro.org> writes: > No worries. Regarding disabling the advices for disabling the advice > messages, maybe it would be better to have only one configuration knob > for that purpose, e.g. "core.verboseAdvice", as a boolean knob. I am not sure if you understood Peff's example that illustrates why it is a bad thing, as ... > That > way, fishing for the right knob for some advice message wouldn't turn > itself into an issue, ... this is exactly what a single core.verboseAdvice knob that squelches the "how to disable this particular advice message" part from the message is problematic. Before you see and familialize yourself with all advice messages, you may see one piece of advice X and find it useful to keep, feeling no need to turn it off. If you use that single knob to squelch the part to tell you how to turn advice X off. But what happens when you hit another unrelated advice Y then? Because your core.verboseAdvice is a single big red button, the message does not say which advice.* variable to tweak for this particular advice message Y.
On 2024-01-10 17:22, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> No worries. Regarding disabling the advices for disabling the advice >> messages, maybe it would be better to have only one configuration knob >> for that purpose, e.g. "core.verboseAdvice", as a boolean knob. > > I am not sure if you understood Peff's example that illustrates why > it is a bad thing, as ... > >> That >> way, fishing for the right knob for some advice message wouldn't turn >> itself into an issue, > > ... this is exactly what a single core.verboseAdvice knob that > squelches the "how to disable this particular advice message" part > from the message is problematic. Before you see and familialize > yourself with all advice messages, you may see one piece of advice X > and find it useful to keep, feeling no need to turn it off. If you > use that single knob to squelch the part to tell you how to turn > advice X off. But what happens when you hit another unrelated > advice Y then? Because your core.verboseAdvice is a single big red > button, the message does not say which advice.* variable to tweak > for this particular advice message Y. Makes sense, but please allow me to explain how I envisioned the whole thing with the single, big core.verboseAdvice configuration knob: 1) You use git and find some advice useful, so you decide to keep it displayed. However, the additional advice about turning the advice off becomes annoying a bit, or better said, it becomes redundant because the main advice stays. 2) As a result, you simply set core.verboseAdvice to false and voila, the redundant additional advice disappears. Seems perfect! Of course, it isn't perfect, as the next point will clearly show. 3) You keep using git, and some advice becomes no longer needed, maybe even one of the advices that you previously used to find useful, or you find some newly added advice a bit annoying and, as a result, not needed. But, what do you do, where's that helpful additional advice? 4) As a careful git user that remembers important things, you go back to your git configuration file and set core.verboseAdvice to true, and the additional advices are back, telling you how to disable the unwanted advice. 5) After you disable the unwanted advice, you set core.verboseAdvice back to false and keep it that way until the next redundant advice pops up. However, I do see that this approach has its downsides, for example the need for the unwanted advice to be displayed again together with the additional advice, by executing the appropriate git commands, after the above-described point #4. Let's see what it would look like with the granular, per-advice on/off knobs: 1) You use git and find some advice useful, so you decide to keep it displayed. However, the additional advice about turning the advice off becomes annoying a bit, or better said, it becomes redundant because the main advice stays. 2) As a result, you follow the additional advice and set the specific knob to false, and voila, the redundant additional advice disappears. Of course, it once again isn't perfect, as the next point will clearly show. 3) You keep using git, and one of the advices that you previously used to find useful becomes no longer needed. But, what do you do, where's that helpful additional advice telling you how to turn the advice off? 4) Now you need to dig though the manual pages, or to re-enable the additional advices in your git configuration file, perhaps all of them at once, while keeping a backup of your original settings, to restore it later. Then, you again need to wait until the original advice gets displayed. 5) The additional advice is finally back, after some time passes or after specifically reproducing the exact git commands, telling you how to disable the unwanted advice. Of course, you follow the advice and set the right knob in your git configuration file. 5) After you disable the unwanted advice, you restore the git configuration backup that you made earlier (you did that, right?), taking care not to override the newly made changes that disabled the unwanted advice. Quite frankly, the second approach, although more granular, seems much more complicated and more error-prone to me. Of course, please let me know if I'm missing something obvious.
On Wed, Jan 10, 2024 at 06:45:49PM +0100, Dragan Simic wrote: > 4) As a careful git user that remembers important things, you go back > to your git configuration file and set core.verboseAdvice to true, and > the additional advices are back, telling you how to disable the > unwanted advice. > > 5) After you disable the unwanted advice, you set core.verboseAdvice > back to false and keep it that way until the next redundant advice > pops up. > > However, I do see that this approach has its downsides, for example > the need for the unwanted advice to be displayed again together with > the additional advice, by executing the appropriate git commands, > after the above-described point #4. Right, the need to re-trigger the advice is the problematic part here, I think. In some cases it is easy. But in others, especially commands which mutate the repo state (like the empty-commit rebase that started this thread), you may need to do a lot of work to recreate the situation. > Let's see what it would look like with the granular, per-advice on/off > knobs: > > 1) You use git and find some advice useful, so you decide to keep it > displayed. However, the additional advice about turning the advice > off becomes annoying a bit, or better said, it becomes redundant > because the main advice stays. > > 2) As a result, you follow the additional advice and set the specific > knob to false, and voila, the redundant additional advice disappears. > Of course, it once again isn't perfect, as the next point will clearly > show. > > 3) You keep using git, and one of the advices that you previously used > to find useful becomes no longer needed. But, what do you do, where's > that helpful additional advice telling you how to turn the advice off? > > 4) Now you need to dig though the manual pages, or to re-enable the > additional advices in your git configuration file, perhaps all of them > at once, while keeping a backup of your original settings, to restore > it later. Then, you again need to wait until the original advice gets > displayed. These steps (3) and (4) seem unlikely to me. These are by definition messages you have seen before and decided to configure specifically (to "always", and not just "off"). So you probably only have a handful (if even more than one) of them in your config file. Whereas for the case I am worried about, you are exposed to a _new_ message that you haven't seen before (and is maybe even new to Git!), from the much-larger pool of "all advice messages Git knows about". It's possible we could implement both mechanisms and let people choose which one they want, depending on their preferences. It's not very much code. I just hesitate to make things more complex than they need to be. -Peff
On 2024-01-11 09:04, Jeff King wrote: > On Wed, Jan 10, 2024 at 06:45:49PM +0100, Dragan Simic wrote: > >> 4) As a careful git user that remembers important things, you go back >> to your git configuration file and set core.verboseAdvice to true, and >> the additional advices are back, telling you how to disable the >> unwanted advice. >> >> 5) After you disable the unwanted advice, you set core.verboseAdvice >> back to false and keep it that way until the next redundant advice >> pops up. >> >> However, I do see that this approach has its downsides, for example >> the need for the unwanted advice to be displayed again together with >> the additional advice, by executing the appropriate git commands, >> after the above-described point #4. > > Right, the need to re-trigger the advice is the problematic part here, > I > think. In some cases it is easy. But in others, especially commands > which mutate the repo state (like the empty-commit rebase that started > this thread), you may need to do a lot of work to recreate the > situation. I apologize for my delayed response. Yes, recreating some situations may simply require an unacceptable amount of work and time, making it pretty much infeasible in practice. >> Let's see what it would look like with the granular, per-advice on/off >> knobs: >> >> 1) You use git and find some advice useful, so you decide to keep it >> displayed. However, the additional advice about turning the advice >> off becomes annoying a bit, or better said, it becomes redundant >> because the main advice stays. >> >> 2) As a result, you follow the additional advice and set the specific >> knob to false, and voila, the redundant additional advice disappears. >> Of course, it once again isn't perfect, as the next point will clearly >> show. >> >> 3) You keep using git, and one of the advices that you previously used >> to find useful becomes no longer needed. But, what do you do, where's >> that helpful additional advice telling you how to turn the advice off? >> >> 4) Now you need to dig though the manual pages, or to re-enable the >> additional advices in your git configuration file, perhaps all of them >> at once, while keeping a backup of your original settings, to restore >> it later. Then, you again need to wait until the original advice gets >> displayed. > > These steps (3) and (4) seem unlikely to me. These are by definition > messages you have seen before and decided to configure specifically (to > "always", and not just "off"). So you probably only have a handful (if > even more than one) of them in your config file. Yes, the number of such messages shouldn't, in general, get out of hand over time. Though, different users may do it differently. > Whereas for the case I am worried about, you are exposed to a _new_ > message that you haven't seen before (and is maybe even new to Git!), > from the much-larger pool of "all advice messages Git knows about". > > It's possible we could implement both mechanisms and let people choose > which one they want, depending on their preferences. It's not very much > code. I just hesitate to make things more complex than they need to be. Perhaps having both options available could be a good thing. Though, adding quite a few knobs may end up confusing the users, so we should make sure to document it well.
Dragan Simic <dsimic@manjaro.org> writes: > Perhaps having both options available could be a good thing. Though, > adding quite a few knobs may end up confusing the users, so we should > make sure to document it well. I agree there is a need for documentation, but we are not increasing the number of knobs, are we?
On 2024-01-18 19:26, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> Perhaps having both options available could be a good thing. Though, >> adding quite a few knobs may end up confusing the users, so we should >> make sure to document it well. > > I agree there is a need for documentation, but we are not increasing > the number of knobs, are we? Perhaps there would be one more knob if we end up deciding to implement support for both approaches, as previously discussed.
Dragan Simic <dsimic@manjaro.org> writes: > On 2024-01-18 19:26, Junio C Hamano wrote: >> Dragan Simic <dsimic@manjaro.org> writes: >> >>> Perhaps having both options available could be a good thing. Though, >>> adding quite a few knobs may end up confusing the users, so we should >>> make sure to document it well. >> I agree there is a need for documentation, but we are not increasing >> the number of knobs, are we? > > Perhaps there would be one more knob if we end up deciding to implement > support for both approaches, as previously discussed. But that would be just one knob (which I personally do not quite see the need for), not "quite a few knobs" as you said, no?
On 2024-01-18 21:19, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: >> On 2024-01-18 19:26, Junio C Hamano wrote: >>> Dragan Simic <dsimic@manjaro.org> writes: >>> >>>> Perhaps having both options available could be a good thing. >>>> Though, >>>> adding quite a few knobs may end up confusing the users, so we >>>> should >>>> make sure to document it well. >>> >>> I agree there is a need for documentation, but we are not increasing >>> the number of knobs, are we? >> >> Perhaps there would be one more knob if we end up deciding to >> implement >> support for both approaches, as previously discussed. > > But that would be just one knob (which I personally do not quite see > the need for), not "quite a few knobs" as you said, no? I'm sorry for being imprecise. Regarding the need for that additional "global" knob, I think it can't hurt. Some people might even find it quite useful, and the good thing is that it wouldn't make the internal logic significantly more complicated.
On 18-ene-2024 21:50:23, Dragan Simic wrote: > On 2024-01-18 21:19, Junio C Hamano wrote: > > Dragan Simic <dsimic@manjaro.org> writes: > > > On 2024-01-18 19:26, Junio C Hamano wrote: > > > > Dragan Simic <dsimic@manjaro.org> writes: > > > > > > > > > Perhaps having both options available could be a good thing. > > > > > Though, > > > > > adding quite a few knobs may end up confusing the users, so > > > > > we should > > > > > make sure to document it well. > > > > > > > > I agree there is a need for documentation, but we are not increasing > > > > the number of knobs, are we? > > > > > > Perhaps there would be one more knob if we end up deciding to > > > implement > > > support for both approaches, as previously discussed. > > > > But that would be just one knob (which I personally do not quite see > > the need for), not "quite a few knobs" as you said, no? > > I'm sorry for being imprecise. Hi Dragan My original motivation for starting this series has been fulfilled: Give the user an option to tell us not to include the disabling instructions alongside the advice. The current consensus is: if the user explicitly enables visibility for an advice, we can stop including the instructions on how to disable its visibility. As reference, in this thread two "global" options have been reviewed: - To take the disabling instructions as an advice in itself and as such, as with the rest, we can have a knob to disable it. Affecting all advice messages. - A new knob to allow the user to set the default visibility for those advice messages without an explicit visibility set. And so we can take on globally what we now take on locally; if there is not an explicit visibility value but there is an explicit "default" value, we can stop showing the instruction on how to disable it. > Regarding the need for that additional > "global" knob, I think it can't hurt. Some people might even find it > quite useful I don't quite understand what "global" knob you are referring to. But I share with you the feeling that a "global" knob might be useful. The current iteration for this series looks like it will be merged to 'next' soon. In my opinion, it is a good stop for this series, and maybe we can explore that 'global' knob in a new one. Thank you for your interest in making this series better.
On 2024-01-20 12:31, Rubén Justo wrote: > On 18-ene-2024 21:50:23, Dragan Simic wrote: >> On 2024-01-18 21:19, Junio C Hamano wrote: >> > Dragan Simic <dsimic@manjaro.org> writes: >> > > On 2024-01-18 19:26, Junio C Hamano wrote: >> > > > Dragan Simic <dsimic@manjaro.org> writes: >> > > > >> > > > > Perhaps having both options available could be a good thing. >> > > > > Though, >> > > > > adding quite a few knobs may end up confusing the users, so >> > > > > we should >> > > > > make sure to document it well. >> > > > >> > > > I agree there is a need for documentation, but we are not increasing >> > > > the number of knobs, are we? >> > > >> > > Perhaps there would be one more knob if we end up deciding to >> > > implement >> > > support for both approaches, as previously discussed. >> > >> > But that would be just one knob (which I personally do not quite see >> > the need for), not "quite a few knobs" as you said, no? >> >> I'm sorry for being imprecise. > > Hi Dragan Hello Rubén! > My original motivation for starting this series has been fulfilled: > Give the user an option to tell us not to include the disabling > instructions alongside the advice. I like the whole idea, because if someone finds some hint usable and decides to keep it displayed, displaying the additional hint about disabling the primary hint (i.e. the advice) simply becomes redundant. > The current consensus is: if the user explicitly enables visibility for > an advice, we can stop including the instructions on how to disable its > visibility. Totally agreed, simple and effective. > As reference, in this thread two "global" options have been reviewed: > > - To take the disabling instructions as an advice in itself and as > such, as with the rest, we can have a knob to disable it. Affecting > all advice messages. > > - A new knob to allow the user to set the default visibility for those > advice messages without an explicit visibility set. And so we can > take on globally what we now take on locally; if there is not an > explicit visibility value but there is an explicit "default" value, > we can stop showing the instruction on how to disable it. > >> Regarding the need for that additional "global" knob, I think it can't >> hurt. Some people might even find it quite useful > > I don't quite understand what "global" knob you are referring to. But > I > share with you the feeling that a "global" knob might be useful. The additional "global knob", at least the way I see it, would enable all advice messages, overriding all other configuration options that may be present. It would be like a "big switch" that makes all advices displayed, for the case in which someone decides to get rid of the hint that used to be useful to them so the advice was disabled, but the hint is no longer found to be useful to them. In such cases, having no advice displayed would mean that the user is unable to know easily what knob disables the no-longer-favored hint. The reason for "forcing" all advices to be displayed would be to allow the advices to be displayed without the need to "fish" for the right knob to be turned in the configuration file. Though, it wouldn't be perfect from the usability standpoint, because recreating the right conditions for displaying some hint and the associated advice may rather easily be practically infeasible, as already discussed in this thread. Of course, going through the man pages to find the right knob is always the best option for those who have the time, but having a "global knob", to help the users a bit, if possible, in general shouldn't hurt. I hope it all makes more sense now. Please, let me know if further clarification is required. Additionally, the way I envisioned it could also be combined with the first option for the "global knob" that you listed above, as an additional option for it to "force" the advices to be displayed, in addition to the ability to disable all advices. > The current iteration for this series looks like it will be merged to > 'next' soon. In my opinion, it is a good stop for this series, and > maybe we can explore that 'global' knob in a new one. I agree, the "global knob" can be added later, if we decide so. > Thank you for your interest in making this series better. Thank you for your work on the patches!
diff --git a/advice.c b/advice.c index 50c79443ba..fa203f8806 100644 --- a/advice.c +++ b/advice.c @@ -79,6 +79,7 @@ static struct { [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 }, [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 }, + [ADVICE_ADVICE_OFF] = { "adviceOff", 1 }, }; static const char turn_off_instructions[] = @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions, strbuf_vaddf(&buf, advice, params); - if (display_instructions) + if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF)) strbuf_addf(&buf, turn_off_instructions, key); for (cp = buf.buf; *cp; cp = np) { diff --git a/advice.h b/advice.h index 2affbe1426..1f2eef034e 100644 --- a/advice.h +++ b/advice.h @@ -10,7 +10,7 @@ struct string_list; * Add the new config variable to Documentation/config/advice.txt. * Call advise_if_enabled to print your advice. */ - enum advice_type { +enum advice_type { ADVICE_ADD_EMBEDDED_REPO, ADVICE_ADD_EMPTY_PATHSPEC, ADVICE_ADD_IGNORED_FILE, @@ -50,6 +50,7 @@ struct string_list; ADVICE_WAITING_FOR_EDITOR, ADVICE_SKIPPED_CHERRY_PICKS, ADVICE_WORKTREE_ADD_ORPHAN, + ADVICE_ADVICE_OFF, }; int git_default_advice_config(const char *var, const char *value); diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh index c13057a4ca..0b6a8b4a10 100755 --- a/t/t0018-advice.sh +++ b/t/t0018-advice.sh @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to test_must_be_empty actual ' +test_expect_success 'advice without the instructions to disable it' ' + cat >expect <<-\EOF && + hint: This is a piece of advice + EOF + test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual && + test_cmp expect actual +' + test_done
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 can 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. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- advice.c | 3 ++- advice.h | 3 ++- t/t0018-advice.sh | 8 ++++++++ 3 files changed, 12 insertions(+), 2 deletions(-)