* [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() @ 2024-01-09 15:25 Rubén Justo 2024-01-09 15:29 ` [PATCH 1/3] t/test-tool: usage description Rubén Justo ` (4 more replies) 0 siblings, 5 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-09 15:25 UTC (permalink / raw) To: Git List 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. Rubén Justo (3): t/test-tool: usage description t/test-tool: handle -c <name>=<value> arguments advice: allow disabling the automatic hint in advise_if_enabled() advice.c | 3 ++- advice.h | 3 ++- t/helper/test-tool.c | 15 +++++++++++++-- t/t0018-advice.sh | 8 ++++++++ 4 files changed, 25 insertions(+), 4 deletions(-) -- 2.42.0 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/3] t/test-tool: usage description 2024-01-09 15:25 [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() Rubén Justo @ 2024-01-09 15:29 ` Rubén Justo 2024-01-09 18:19 ` Junio C Hamano 2024-01-09 15:29 ` [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments Rubén Justo ` (3 subsequent siblings) 4 siblings, 1 reply; 36+ messages in thread From: Rubén Justo @ 2024-01-09 15:29 UTC (permalink / raw) To: Git List Even though this is an internal tool, let's keep the usage description correct and well organized. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- t/helper/test-tool.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 37ba996539..d9f57c20db 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -5,7 +5,7 @@ #include "parse-options.h" static const char * const test_tool_usage[] = { - "test-tool [-C <directory>] <command [<arguments>...]]", + "test-tool [-C <directory>] <command> [<arguments>...]]", NULL }; @@ -100,7 +100,7 @@ static NORETURN void die_usage(void) { size_t i; - fprintf(stderr, "usage: test-tool <toolname> [args]\n"); + fprintf(stderr, "usage: %s\n", test_tool_usage[0]); for (i = 0; i < ARRAY_SIZE(cmds); i++) fprintf(stderr, " %s\n", cmds[i].name); exit(128); -- 2.42.0 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 1/3] t/test-tool: usage description 2024-01-09 15:29 ` [PATCH 1/3] t/test-tool: usage description Rubén Justo @ 2024-01-09 18:19 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2024-01-09 18:19 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List > Subject: Re: [PATCH 1/3] t/test-tool: usage description Good eyes to spot the missing close-angle-bracket. I'll add some missing verb, e.g. "fix usage string", while queuing. I would not bother replacing the fprintf() format string in the same patch. Hits from $ git grep '"usage:' t/helper indicates that far less than half (3 among 12) reuses usage_str[] for this purpose. Making these "usage:" strings come from a unified API (perhaps parse_options() family of functions have something more appropriate than ad-hoc use of fprintf()? I didn't check) might be a welcome change but that is clearly outside the scope of the mark-up fix, and I do not see touching only this one that still uses fprintf() advances toward such a goal. t/helper/test-chmtime.c: fprintf(stderr, "usage: %s %s\n", argv[0], usage_str); t/helper/test-delta.c: fprintf(stderr, "usage: %s\n", usage_str); t/helper/test-windows-named-pipe.c: fprintf(stderr, "usage: %s %s\n", argv[0], usage_string); t/helper/test-advise.c: die("usage: %s <advice>", argv[0]); t/helper/test-csprng.c: fprintf(stderr, "usage: %s [<size>]\n", argv[0]); t/helper/test-genrandom.c: fprintf(stderr, "usage: %s <seed_string> [<size>]\n", argv[0]); t/helper/test-genzeros.c: fprintf(stderr, "usage: %s [<count>]\n", argv[0]); t/helper/test-hash-speed.c: die("usage: test-tool hash-speed algo_name"); t/helper/test-mergesort.c: fprintf(stderr, "usage: test-tool mergesort generate <distribution> <mode> <n> <m>\n"); t/helper/test-strcmp-offset.c: die("usage: %s <string1> <string2>", argv[0]); t/helper/test-tool.c: fprintf(stderr, "usage: test-tool <toolname> [args]\n"); > Even though this is an internal tool, let's keep the usage description > correct and well organized. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > t/helper/test-tool.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > index 37ba996539..d9f57c20db 100644 > --- a/t/helper/test-tool.c > +++ b/t/helper/test-tool.c > @@ -5,7 +5,7 @@ > #include "parse-options.h" > > static const char * const test_tool_usage[] = { > - "test-tool [-C <directory>] <command [<arguments>...]]", > + "test-tool [-C <directory>] <command> [<arguments>...]]", > NULL > }; > > @@ -100,7 +100,7 @@ static NORETURN void die_usage(void) > { > size_t i; > > - fprintf(stderr, "usage: test-tool <toolname> [args]\n"); > + fprintf(stderr, "usage: %s\n", test_tool_usage[0]); > for (i = 0; i < ARRAY_SIZE(cmds); i++) > fprintf(stderr, " %s\n", cmds[i].name); > exit(128); ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments 2024-01-09 15:25 [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() Rubén Justo 2024-01-09 15:29 ` [PATCH 1/3] t/test-tool: usage description Rubén Justo @ 2024-01-09 15:29 ` Rubén Justo 2024-01-09 18:19 ` Junio C Hamano 2024-01-09 18:20 ` Taylor Blau 2024-01-09 15:30 ` [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() Rubén Justo ` (2 subsequent siblings) 4 siblings, 2 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-09 15:29 UTC (permalink / raw) To: Git List Soon we're going to need to pass configuration values to a command in test-tool. Let's teach test-tool to take config values via command line arguments. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- t/helper/test-tool.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index d9f57c20db..7eba4ec9ab 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -3,9 +3,10 @@ #include "test-tool-utils.h" #include "trace2.h" #include "parse-options.h" +#include "config.h" static const char * const test_tool_usage[] = { - "test-tool [-C <directory>] <command> [<arguments>...]]", + "test-tool [-C <directory>] [-c <name>=<value>] <command> [<arguments>...]", NULL }; @@ -106,6 +107,13 @@ static NORETURN void die_usage(void) exit(128); } +static int parse_config_option(const struct option *opt, const char *arg, + int unset) +{ + git_config_push_parameter(arg); + return 0; +} + int cmd_main(int argc, const char **argv) { int i; @@ -113,6 +121,9 @@ int cmd_main(int argc, const char **argv) struct option options[] = { OPT_STRING('C', NULL, &working_directory, "directory", "change the working directory"), + OPT_CALLBACK('c', NULL, NULL, "<name>=<value>", + "pass a configuration parameter to the command", + parse_config_option), OPT_END() }; -- 2.42.0 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments 2024-01-09 15:29 ` [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments Rubén Justo @ 2024-01-09 18:19 ` Junio C Hamano 2024-01-09 18:20 ` Taylor Blau 1 sibling, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2024-01-09 18:19 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List Rubén Justo <rjusto@gmail.com> writes: > Soon we're going to need to pass configuration values to a command in > test-tool. > > Let's teach test-tool to take config values via command line arguments. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > t/helper/test-tool.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) Nice. > > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > index d9f57c20db..7eba4ec9ab 100644 > --- a/t/helper/test-tool.c > +++ b/t/helper/test-tool.c > @@ -3,9 +3,10 @@ > #include "test-tool-utils.h" > #include "trace2.h" > #include "parse-options.h" > +#include "config.h" > > static const char * const test_tool_usage[] = { > - "test-tool [-C <directory>] <command> [<arguments>...]]", > + "test-tool [-C <directory>] [-c <name>=<value>] <command> [<arguments>...]", > NULL > }; > > @@ -106,6 +107,13 @@ static NORETURN void die_usage(void) > exit(128); > } > > +static int parse_config_option(const struct option *opt, const char *arg, > + int unset) > +{ > + git_config_push_parameter(arg); > + return 0; > +} > + > int cmd_main(int argc, const char **argv) > { > int i; > @@ -113,6 +121,9 @@ int cmd_main(int argc, const char **argv) > struct option options[] = { > OPT_STRING('C', NULL, &working_directory, "directory", > "change the working directory"), > + OPT_CALLBACK('c', NULL, NULL, "<name>=<value>", > + "pass a configuration parameter to the command", > + parse_config_option), > OPT_END() > }; ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments 2024-01-09 15:29 ` [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments Rubén Justo 2024-01-09 18:19 ` Junio C Hamano @ 2024-01-09 18:20 ` Taylor Blau 1 sibling, 0 replies; 36+ messages in thread From: Taylor Blau @ 2024-01-09 18:20 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List On Tue, Jan 09, 2024 at 04:29:57PM +0100, Rubén Justo wrote: > Soon we're going to need to pass configuration values to a command in > test-tool. > > Let's teach test-tool to take config values via command line arguments. I wasn't expecting a step like this to appear in this series. I don't have strong feelings about it, especially since test-tool helpers already understand $GIT_DIR/config when they rely on library code which implicitly reads configuration. But it does seem odd to have test-tool invocations that intimately depend on a particular set of configuration values. At the very least, this step seems to encourage passing finely tuned configuration values to test-tool helpers, which I am not sure is a good idea. Your patch message suggests that this will be useful in the following patch, which makes sense. But I wonder if it would be easier to avoid the test-tool entirely and call some Git command in a state that we expect to generate advice. Then we can test its output with various values of advice.adviceOff. > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > t/helper/test-tool.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) The patch itself looks reasonable, though. Thanks, Taylor ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-09 15:25 [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() Rubén Justo 2024-01-09 15:29 ` [PATCH 1/3] t/test-tool: usage description Rubén Justo 2024-01-09 15:29 ` [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments Rubén Justo @ 2024-01-09 15:30 ` Rubén Justo 2024-01-09 18:23 ` Junio C Hamano ` (2 more replies) 2024-01-09 18:28 ` [PATCH 0/3] " Taylor Blau 2024-01-12 10:05 ` [PATCH] advice: " Rubén Justo 4 siblings, 3 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-09 15:30 UTC (permalink / raw) To: Git List 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 +' + test_done -- 2.42.0 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-09 15:30 ` [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() Rubén Justo @ 2024-01-09 18:23 ` Junio C Hamano 2024-01-09 18:27 ` Taylor Blau 2024-01-10 11:02 ` Jeff King 2 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2024-01-09 18:23 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-09 15:30 ` [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() Rubén Justo 2024-01-09 18:23 ` Junio C Hamano @ 2024-01-09 18:27 ` Taylor Blau 2024-01-09 19:57 ` Junio C Hamano 2024-01-10 12:11 ` Rubén Justo 2024-01-10 11:02 ` Jeff King 2 siblings, 2 replies; 36+ messages in thread From: Taylor Blau @ 2024-01-09 18:27 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-09 18:27 ` Taylor Blau @ 2024-01-09 19:57 ` Junio C Hamano 2024-01-10 12:11 ` Rubén Justo 1 sibling, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2024-01-09 19:57 UTC (permalink / raw) To: Taylor Blau; +Cc: Rubén Justo, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-09 18:27 ` Taylor Blau 2024-01-09 19:57 ` Junio C Hamano @ 2024-01-10 12:11 ` Rubén Justo 1 sibling, 0 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-10 12:11 UTC (permalink / raw) To: Taylor Blau; +Cc: Git List, Junio C Hamano, Jeff King 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-09 15:30 ` [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() Rubén Justo 2024-01-09 18:23 ` Junio C Hamano 2024-01-09 18:27 ` Taylor Blau @ 2024-01-10 11:02 ` Jeff King 2024-01-10 11:39 ` Rubén Justo ` (2 more replies) 2 siblings, 3 replies; 36+ messages in thread From: Jeff King @ 2024-01-10 11:02 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-10 11:02 ` Jeff King @ 2024-01-10 11:39 ` Rubén Justo 2024-01-10 14:18 ` Dragan Simic 2024-01-10 16:14 ` Junio C Hamano 2 siblings, 0 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-10 11:39 UTC (permalink / raw) To: Jeff King; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-10 11:02 ` Jeff King 2024-01-10 11:39 ` Rubén Justo @ 2024-01-10 14:18 ` Dragan Simic 2024-01-10 14:32 ` Rubén Justo 2024-01-10 16:14 ` Junio C Hamano 2 siblings, 1 reply; 36+ messages in thread From: Dragan Simic @ 2024-01-10 14:18 UTC (permalink / raw) To: Jeff King; +Cc: Rubén Justo, Git List 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-10 14:18 ` Dragan Simic @ 2024-01-10 14:32 ` Rubén Justo 2024-01-10 14:44 ` Dragan Simic 0 siblings, 1 reply; 36+ messages in thread From: Rubén Justo @ 2024-01-10 14:32 UTC (permalink / raw) To: Dragan Simic, Jeff King; +Cc: Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-10 14:32 ` Rubén Justo @ 2024-01-10 14:44 ` Dragan Simic 2024-01-10 16:22 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Dragan Simic @ 2024-01-10 14:44 UTC (permalink / raw) To: Rubén Justo; +Cc: Jeff King, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-10 14:44 ` Dragan Simic @ 2024-01-10 16:22 ` Junio C Hamano 2024-01-10 17:45 ` Dragan Simic 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2024-01-10 16:22 UTC (permalink / raw) To: Dragan Simic; +Cc: Rubén Justo, Jeff King, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-10 16:22 ` Junio C Hamano @ 2024-01-10 17:45 ` Dragan Simic 2024-01-11 8:04 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Dragan Simic @ 2024-01-10 17:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Rubén Justo, Jeff King, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-10 17:45 ` Dragan Simic @ 2024-01-11 8:04 ` Jeff King 2024-01-18 6:15 ` Dragan Simic 0 siblings, 1 reply; 36+ messages in thread From: Jeff King @ 2024-01-11 8:04 UTC (permalink / raw) To: Dragan Simic; +Cc: Junio C Hamano, Rubén Justo, Git List 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-11 8:04 ` Jeff King @ 2024-01-18 6:15 ` Dragan Simic 2024-01-18 18:26 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Dragan Simic @ 2024-01-18 6:15 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Rubén Justo, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-18 6:15 ` Dragan Simic @ 2024-01-18 18:26 ` Junio C Hamano 2024-01-18 18:53 ` Dragan Simic 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2024-01-18 18:26 UTC (permalink / raw) To: Dragan Simic; +Cc: Jeff King, Rubén Justo, Git List 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? ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-18 18:26 ` Junio C Hamano @ 2024-01-18 18:53 ` Dragan Simic 2024-01-18 20:19 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Dragan Simic @ 2024-01-18 18:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Rubén Justo, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-18 18:53 ` Dragan Simic @ 2024-01-18 20:19 ` Junio C Hamano 2024-01-18 20:50 ` Dragan Simic 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2024-01-18 20:19 UTC (permalink / raw) To: Dragan Simic; +Cc: Jeff King, Rubén Justo, Git List 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? ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-18 20:19 ` Junio C Hamano @ 2024-01-18 20:50 ` Dragan Simic 2024-01-20 11:31 ` Rubén Justo 0 siblings, 1 reply; 36+ messages in thread From: Dragan Simic @ 2024-01-18 20:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Rubén Justo, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-18 20:50 ` Dragan Simic @ 2024-01-20 11:31 ` Rubén Justo 2024-01-20 15:31 ` Dragan Simic 0 siblings, 1 reply; 36+ messages in thread From: Rubén Justo @ 2024-01-20 11:31 UTC (permalink / raw) To: Dragan Simic, Junio C Hamano; +Cc: Jeff King, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-20 11:31 ` Rubén Justo @ 2024-01-20 15:31 ` Dragan Simic 0 siblings, 0 replies; 36+ messages in thread From: Dragan Simic @ 2024-01-20 15:31 UTC (permalink / raw) To: Rubén Justo; +Cc: Junio C Hamano, Jeff King, Git List 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! ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-10 11:02 ` Jeff King 2024-01-10 11:39 ` Rubén Justo 2024-01-10 14:18 ` Dragan Simic @ 2024-01-10 16:14 ` Junio C Hamano 2 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2024-01-10 16:14 UTC (permalink / raw) To: Jeff King; +Cc: Rubén Justo, Git List 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)". ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() 2024-01-09 15:25 [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() Rubén Justo ` (2 preceding siblings ...) 2024-01-09 15:30 ` [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() Rubén Justo @ 2024-01-09 18:28 ` Taylor Blau 2024-01-09 22:32 ` Junio C Hamano 2024-01-12 10:05 ` [PATCH] advice: " Rubén Justo 4 siblings, 1 reply; 36+ messages in thread From: Taylor Blau @ 2024-01-09 18:28 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() 2024-01-09 18:28 ` [PATCH 0/3] " Taylor Blau @ 2024-01-09 22:32 ` Junio C Hamano 2024-01-10 12:40 ` Rubén Justo 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2024-01-09 22:32 UTC (permalink / raw) To: Taylor Blau; +Cc: Rubén Justo, Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() 2024-01-09 22:32 ` Junio C Hamano @ 2024-01-10 12:40 ` Rubén Justo 0 siblings, 0 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-10 12:40 UTC (permalink / raw) To: Junio C Hamano, Taylor Blau, Jeff King; +Cc: Git List 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-09 15:25 [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() Rubén Justo ` (3 preceding siblings ...) 2024-01-09 18:28 ` [PATCH 0/3] " Taylor Blau @ 2024-01-12 10:05 ` Rubén Justo 2024-01-12 22:19 ` Junio C Hamano 2024-01-15 14:28 ` [PATCH v2] " Rubén Justo 4 siblings, 2 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-12 10:05 UTC (permalink / raw) To: Git List, Junio C Hamano, Taylor Blau, Jeff King, Dragan Simic Using advise_if_enabled() to display an advice will automatically include instructions on how to disable the advice, alongside the main advice: hint: use --reapply-cherry-picks to include skipped commits hint: Disable this message with "git config advice.skippedCherryPicks false" To do so, we provide a knob which can be used to disable the advice. But also to tell us the opposite: to show the advice. Let's not include the deactivation instructions for an advice if the user explicitly sets its visibility. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- I hope this patch captures most of the comments from the previous iteration, mostly: - Allow to disable the disabling instructions, per advice. - No new test is needed, therefore the first two commits from the previous iteration are not needed here. Thanks. advice.c | 109 +++++++++++++++++++++++++--------------------- t/t0018-advice.sh | 1 - 2 files changed, 59 insertions(+), 51 deletions(-) diff --git a/advice.c b/advice.c index f6e4c2f302..8474966ce1 100644 --- a/advice.c +++ b/advice.c @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix) return ""; } +enum advice_level { + ADVICE_LEVEL_DEFAULT = 0, + ADVICE_LEVEL_DISABLED, + ADVICE_LEVEL_ENABLED, +}; + static struct { const char *key; - int enabled; + enum advice_level level; } advice_setting[] = { - [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 }, - [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec", 1 }, - [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile", 1 }, - [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec", 1 }, - [ADVICE_AM_WORK_DIR] = { "amWorkDir", 1 }, - [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName", 1 }, - [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 }, - [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 }, - [ADVICE_DIVERGING] = { "diverging", 1 }, - [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 }, - [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch", 1 }, - [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 }, - [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 }, - [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity", 1 }, - [ADVICE_NESTED_TAG] = { "nestedTag", 1 }, - [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning", 1 }, - [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists", 1 }, - [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst", 1 }, - [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce", 1 }, - [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent", 1 }, - [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching", 1 }, - [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate", 1 }, - [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName", 1 }, - [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected", 1 }, - [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward", 1 }, /* backwards compatibility */ - [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh", 1 }, - [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict", 1 }, - [ADVICE_RM_HINTS] = { "rmHints", 1 }, - [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse", 1 }, - [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure", 1 }, - [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1 }, - [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning", 1 }, - [ADVICE_STATUS_HINTS] = { "statusHints", 1 }, - [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 }, - [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated", 1 }, - [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 }, - [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 }, - [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 }, - [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, - [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 }, + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" }, + [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec" }, + [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile" }, + [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec" }, + [ADVICE_AM_WORK_DIR] = { "amWorkDir" }, + [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName" }, + [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge" }, + [ADVICE_DETACHED_HEAD] = { "detachedHead" }, + [ADVICE_DIVERGING] = { "diverging" }, + [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates" }, + [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch" }, + [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated" }, + [ADVICE_IGNORED_HOOK] = { "ignoredHook" }, + [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity" }, + [ADVICE_NESTED_TAG] = { "nestedTag" }, + [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning" }, + [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists" }, + [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst" }, + [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce" }, + [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent" }, + [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching" }, + [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate" }, + [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" }, + [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" }, + [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */ + [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" }, + [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" }, + [ADVICE_RM_HINTS] = { "rmHints" }, + [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" }, + [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" }, + [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" }, + [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning" }, + [ADVICE_STATUS_HINTS] = { "statusHints" }, + [ADVICE_STATUS_U_OPTION] = { "statusUoption" }, + [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated" }, + [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" }, + [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead" }, + [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath" }, + [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" }, + [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" }, }; static const char turn_off_instructions[] = @@ -116,13 +122,13 @@ void advise(const char *advice, ...) int advice_enabled(enum advice_type type) { - switch(type) { - case ADVICE_PUSH_UPDATE_REJECTED: - return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled && - advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled; - default: - return advice_setting[type].enabled; - } + int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED; + + if (type == ADVICE_PUSH_UPDATE_REJECTED) + return enabled && + advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS); + + return enabled; } void advise_if_enabled(enum advice_type type, const char *advice, ...) @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...) return; va_start(params, advice); - vadvise(advice, 1, advice_setting[type].key, params); + vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT, + advice_setting[type].key, params); va_end(params); } @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value) for (i = 0; i < ARRAY_SIZE(advice_setting); i++) { if (strcasecmp(k, advice_setting[i].key)) continue; - advice_setting[i].enabled = git_config_bool(var, value); + advice_setting[i].level = git_config_bool(var, value) + ? ADVICE_LEVEL_ENABLED + : ADVICE_LEVEL_DISABLED; return 0; } diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh index c13057a4ca..0dcfb760a2 100755 --- a/t/t0018-advice.sh +++ b/t/t0018-advice.sh @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' ' test_expect_success 'advice should be printed when config variable is set to true' ' cat >expect <<-\EOF && hint: This is a piece of advice - hint: Disable this message with "git config advice.nestedTag false" EOF test_config advice.nestedTag true && test-tool advise "This is a piece of advice" 2>actual && base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d -- 2.42.0 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-12 10:05 ` [PATCH] advice: " Rubén Justo @ 2024-01-12 22:19 ` Junio C Hamano 2024-01-13 7:38 ` Jeff King 2024-01-15 11:24 ` Rubén Justo 2024-01-15 14:28 ` [PATCH v2] " Rubén Justo 1 sibling, 2 replies; 36+ messages in thread From: Junio C Hamano @ 2024-01-12 22:19 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Taylor Blau, Jeff King, Dragan Simic Rubén Justo <rjusto@gmail.com> writes: > advice.c | 109 +++++++++++++++++++++++++--------------------- > t/t0018-advice.sh | 1 - > 2 files changed, 59 insertions(+), 51 deletions(-) > > diff --git a/advice.c b/advice.c > index f6e4c2f302..8474966ce1 100644 > --- a/advice.c > +++ b/advice.c > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix) > return ""; > } > > +enum advice_level { > + ADVICE_LEVEL_DEFAULT = 0, We may want to say "_NONE" not "_DEFAULT" to match the convention used elsewhere, but I have no strong preference. I do have a preference to see that, when we explicitly say "In this enum, there is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the definition, the places we use a variable of this enum type, we say if (!variable) and not if (variable == ADVICE_LEVEL_DEFAULT) > + ADVICE_LEVEL_DISABLED, > + ADVICE_LEVEL_ENABLED, > +}; > + > static struct { > const char *key; > - int enabled; > + enum advice_level level; > } advice_setting[] = { > ... > - [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 }, > + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" }, > ... > + [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" }, > }; It certainly becomes nicer to use the "unspecified is left to 0" convention like this. > static const char turn_off_instructions[] = > @@ -116,13 +122,13 @@ void advise(const char *advice, ...) > > int advice_enabled(enum advice_type type) > { > - switch(type) { > - case ADVICE_PUSH_UPDATE_REJECTED: > - return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled && > - advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled; > - default: > - return advice_setting[type].enabled; > - } > + int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED; OK. > + if (type == ADVICE_PUSH_UPDATE_REJECTED) > + return enabled && > + advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS); I like the textbook use of a simple recursion here ;-) > + return enabled; > } > void advise_if_enabled(enum advice_type type, const char *advice, ...) > @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...) > return; > > va_start(params, advice); > - vadvise(advice, 1, advice_setting[type].key, params); > + vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT, > + advice_setting[type].key, params); OK. Once you configure this advice to be always shown, you no longer are using the _DEFAULT, so we pass 0 as the second parameter. Makes sense. As I said, if we explicitly document that _DEFAULT's value is zero with "= 0" in the enum definition, which we also rely on the initialization of the advice_setting[] array, then it may probably be better to write it more like so: vadvice(advice, !advice_setting[type].level, advice_setting[type].key, params); I wonder if it makes this caller simpler to update the return value of advice_enabled() to a tristate. Then we can do int enabled = advice_enabled(type); if (!enabled) return; va_start(...); vadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...); va_end(...); but it does not make that much difference. > va_end(params); > } > > @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value) > for (i = 0; i < ARRAY_SIZE(advice_setting); i++) { > if (strcasecmp(k, advice_setting[i].key)) > continue; > - advice_setting[i].enabled = git_config_bool(var, value); > + advice_setting[i].level = git_config_bool(var, value) > + ? ADVICE_LEVEL_ENABLED > + : ADVICE_LEVEL_DISABLED; OK. We saw (var, value) in the configuration files we are parsing, so we find [i] that corresponds to the advice key and tweak the level. This loop obviously is O(N*M) for the number of "advice.*" configuration variables N and the number of advice keys M, but that is not new to this patch---our expectation is that N will be small, even though M will grow over time. > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh > index c13057a4ca..0dcfb760a2 100755 > --- a/t/t0018-advice.sh > +++ b/t/t0018-advice.sh > @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' ' > test_expect_success 'advice should be printed when config variable is set to true' ' > cat >expect <<-\EOF && > hint: This is a piece of advice > - hint: Disable this message with "git config advice.nestedTag false" > EOF > test_config advice.nestedTag true && > test-tool advise "This is a piece of advice" 2>actual && OK. The commit changes behaviour for existing users who explicitly said "I want to see this advice" by configuring advice.* key to 'true', and it probably is a good thing, even though it may be a bit surprising. One thing that may be missing is a documentation update. Something along this line to highlight that now 'true' has a bit different meaning from before (and as a consequence, we can no longer say "defaults to true"). Documentation/config/advice.txt | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt index 2737381a11..364a8268b6 100644 --- c/Documentation/config/advice.txt +++ w/Documentation/config/advice.txt @@ -1,7 +1,11 @@ advice.*:: - These variables control various optional help messages designed to - aid new users. All 'advice.*' variables default to 'true', and you - can tell Git that you do not need help by setting these to 'false': + + These variables control various optional help messages + designed to aid new users. When left unconfigured, Git will + give you the help message with an instruction on how to + squelch it. When set to 'true', the help message is given, + but without hint on how to squelch the message. You can + tell Git that you do not need help by setting these to 'false': + -- ambiguousFetchRefspec:: ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-12 22:19 ` Junio C Hamano @ 2024-01-13 7:38 ` Jeff King 2024-01-16 4:56 ` Junio C Hamano 2024-01-15 11:24 ` Rubén Justo 1 sibling, 1 reply; 36+ messages in thread From: Jeff King @ 2024-01-13 7:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: Rubén Justo, Git List, Taylor Blau, Dragan Simic On Fri, Jan 12, 2024 at 02:19:32PM -0800, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > advice.c | 109 +++++++++++++++++++++++++--------------------- > > t/t0018-advice.sh | 1 - > > 2 files changed, 59 insertions(+), 51 deletions(-) > > > > diff --git a/advice.c b/advice.c > > index f6e4c2f302..8474966ce1 100644 > > --- a/advice.c > > +++ b/advice.c > > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix) > > return ""; > > } > > > > +enum advice_level { > > + ADVICE_LEVEL_DEFAULT = 0, > > We may want to say "_NONE" not "_DEFAULT" to match the convention > used elsewhere, but I have no strong preference. I do have a > preference to see that, when we explicitly say "In this enum, there > is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the > definition, the places we use a variable of this enum type, we say > > if (!variable) > > and not > > if (variable == ADVICE_LEVEL_DEFAULT) This may be getting into the realm of bikeshedding, but... ;) For a tri-state we often use "-1" for "not specified". That has the nice side effect in this case that "if (level)" shows the advice (that works because "unspecified" and "explicitly true" both show the advice. And then "if (level < 0)" is used for just the hint. But maybe that is too clever/fragile. Of course that means that all of the initializers have to use "-1" explicitly. So zero-initialization is sometimes nice, too. -Peff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-13 7:38 ` Jeff King @ 2024-01-16 4:56 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2024-01-16 4:56 UTC (permalink / raw) To: Jeff King; +Cc: Rubén Justo, Git List, Taylor Blau, Dragan Simic Jeff King <peff@peff.net> writes: > For a tri-state we often use "-1" for "not specified". That has the nice > side effect in this case that "if (level)" shows the advice (that works > because "unspecified" and "explicitly true" both show the advice. And > then "if (level < 0)" is used for just the hint. But maybe that is too > clever/fragile. > > Of course that means that all of the initializers have to use "-1" > explicitly. So zero-initialization is sometimes nice, too. ;-) 100% agreed. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-12 22:19 ` Junio C Hamano 2024-01-13 7:38 ` Jeff King @ 2024-01-15 11:24 ` Rubén Justo 1 sibling, 0 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-15 11:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Taylor Blau, Jeff King, Dragan Simic On 12-ene-2024 14:19:32, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > advice.c | 109 +++++++++++++++++++++++++--------------------- > > t/t0018-advice.sh | 1 - > > 2 files changed, 59 insertions(+), 51 deletions(-) > > > > diff --git a/advice.c b/advice.c > > index f6e4c2f302..8474966ce1 100644 > > --- a/advice.c > > +++ b/advice.c > > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix) > > return ""; > > } > > > > +enum advice_level { > > + ADVICE_LEVEL_DEFAULT = 0, > > We may want to say "_NONE" not "_DEFAULT" to match the convention > used elsewhere, but I have no strong preference. The "_DEFAULT" name is rooted in the idea of having a default but configurable value. I'm not developing that idea in this series because I'm not sure if it's desirable. But, I'll leave a sketch here: diff --git a/advice.c b/advice.c index 8474966ce1..1bb427a8d8 100644 --- a/advice.c +++ b/advice.c @@ -39,6 +39,8 @@ enum advice_level { ADVICE_LEVEL_ENABLED, }; +static enum advice_level advice_level_default; + static struct { const char *key; enum advice_level level; @@ -122,7 +124,9 @@ void advise(const char *advice, ...) int advice_enabled(enum advice_type type) { - int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED; + int enabled = advice_setting[type].level + ? advice_setting[type].level != ADVICE_LEVEL_DISABLED + : advice_level_default != ADVICE_LEVEL_DISABLED; if (type == ADVICE_PUSH_UPDATE_REJECTED) return enabled && @@ -139,7 +143,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...) return; va_start(params, advice); - vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT, + vadvise(advice, !advice_setting[type].level && !advice_level_default, advice_setting[type].key, params); va_end(params); } @@ -166,6 +170,13 @@ int git_default_advice_config(const char *var, const char *value) if (!skip_prefix(var, "advice.", &k)) return 0; + if (!strcmp(var, "advice.default")) { + advice_level_default = git_config_bool(var, value) + ? ADVICE_LEVEL_ENABLED + : ADVICE_LEVEL_DISABLED; + return 0; + } + for (i = 0; i < ARRAY_SIZE(advice_setting); i++) { if (strcasecmp(k, advice_setting[i].key)) continue; The way it is now, "_NONE" is perfectly fine. I'll use it. > I do have a > preference to see that, when we explicitly say "In this enum, there > is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the > definition, the places we use a variable of this enum type, we say > > if (!variable) OK > > and not > > if (variable == ADVICE_LEVEL_DEFAULT) > > > + ADVICE_LEVEL_DISABLED, > > + ADVICE_LEVEL_ENABLED, > > +}; > > + > > static struct { > > const char *key; > > - int enabled; > > + enum advice_level level; > > } advice_setting[] = { > > ... > > - [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 }, > > + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" }, > > ... > > + [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" }, > > }; > > It certainly becomes nicer to use the "unspecified is left to 0" > convention like this. Indeed, that's my feeling too. > > > static const char turn_off_instructions[] = > > @@ -116,13 +122,13 @@ void advise(const char *advice, ...) > > > > int advice_enabled(enum advice_type type) > > { > > - switch(type) { > > - case ADVICE_PUSH_UPDATE_REJECTED: > > - return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled && > > - advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled; > > - default: > > - return advice_setting[type].enabled; > > - } > > + int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED; > > OK. > > > + if (type == ADVICE_PUSH_UPDATE_REJECTED) > > + return enabled && > > + advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS); > > I like the textbook use of a simple recursion here ;-) > > > + return enabled; > > } > > > void advise_if_enabled(enum advice_type type, const char *advice, ...) > > @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...) > > return; > > > > va_start(params, advice); > > - vadvise(advice, 1, advice_setting[type].key, params); > > + vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT, > > + advice_setting[type].key, params); > > OK. Once you configure this advice to be always shown, you no > longer are using the _DEFAULT, so we pass 0 as the second parameter. > Makes sense. > > As I said, if we explicitly document that _DEFAULT's value is zero > with "= 0" in the enum definition, which we also rely on the > initialization of the advice_setting[] array, then it may probably > be better to write it more like so: > > vadvice(advice, !advice_setting[type].level, > advice_setting[type].key, params); OK > > I wonder if it makes this caller simpler to update the return value > of advice_enabled() to a tristate. Then we can do > > int enabled = advice_enabled(type); > > if (!enabled) > return; > va_start(...); > vadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...); > va_end(...); > > but it does not make that much difference. > > > va_end(params); > > } > > > > @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value) > > for (i = 0; i < ARRAY_SIZE(advice_setting); i++) { > > if (strcasecmp(k, advice_setting[i].key)) > > continue; > > - advice_setting[i].enabled = git_config_bool(var, value); > > + advice_setting[i].level = git_config_bool(var, value) > > + ? ADVICE_LEVEL_ENABLED > > + : ADVICE_LEVEL_DISABLED; > > OK. We saw (var, value) in the configuration files we are parsing, > so we find [i] that corresponds to the advice key and tweak the > level. > > This loop obviously is O(N*M) for the number of "advice.*" > configuration variables N and the number of advice keys M, but that > is not new to this patch---our expectation is that N will be small, > even though M will grow over time. I wonder if we can drop entirely this loop. Maybe a hashmap can be helpful here. Of course, this is tangential to this series. > > > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh > > index c13057a4ca..0dcfb760a2 100755 > > --- a/t/t0018-advice.sh > > +++ b/t/t0018-advice.sh > > @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' ' > > test_expect_success 'advice should be printed when config variable is set to true' ' > > cat >expect <<-\EOF && > > hint: This is a piece of advice > > - hint: Disable this message with "git config advice.nestedTag false" > > EOF > > test_config advice.nestedTag true && > > test-tool advise "This is a piece of advice" 2>actual && > > OK. The commit changes behaviour for existing users who explicitly > said "I want to see this advice" by configuring advice.* key to 'true', Technically, when the user sets any value. Maybe in the future we transform the knob to have more than two states, beyond yes/no. At that point, current rationality would still apply. > and it probably is a good thing, even though it may be a bit surprising. > > One thing that may be missing is a documentation update. Something > along this line to highlight that now 'true' has a bit different > meaning from before (and as a consequence, we can no longer say > "defaults to true"). > > Documentation/config/advice.txt | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt > index 2737381a11..364a8268b6 100644 > --- c/Documentation/config/advice.txt > +++ w/Documentation/config/advice.txt > @@ -1,7 +1,11 @@ > advice.*:: > - These variables control various optional help messages designed to > - aid new users. All 'advice.*' variables default to 'true', and you > - can tell Git that you do not need help by setting these to 'false': > + > + These variables control various optional help messages > + designed to aid new users. When left unconfigured, Git will > + give you the help message with an instruction on how to > + squelch it. When set to 'true', the help message is given, > + but without hint on how to squelch the message. You can > + tell Git that you do not need help by setting these to 'false': > + > -- > ambiguousFetchRefspec:: OK. I'll add this. Thanks. ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2] advice: allow disabling the automatic hint in advise_if_enabled() 2024-01-12 10:05 ` [PATCH] advice: " Rubén Justo 2024-01-12 22:19 ` Junio C Hamano @ 2024-01-15 14:28 ` Rubén Justo 1 sibling, 0 replies; 36+ messages in thread From: Rubén Justo @ 2024-01-15 14:28 UTC (permalink / raw) To: Git List, Junio C Hamano, Taylor Blau, Jeff King, Dragan Simic Using advise_if_enabled() to display an advice will automatically include instructions on how to disable the advice, alongside the main advice: hint: use --reapply-cherry-picks to include skipped commits hint: Disable this message with "git config advice.skippedCherryPicks false" To do so, we provide a knob which can be used to disable the advice. But also to tell us the opposite: to show the advice. Let's not include the deactivation instructions for an advice if the user explicitly sets its visibility. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- This must have been v3, but previous iteration was erroneously sent as a v1. Sorry. Range-diff against v1: 1: d280195c7b ! 1: 0bee5d1bba advice: allow disabling the automatic hint in advise_if_enabled() @@ Commit message Signed-off-by: Rubén Justo <rjusto@gmail.com> + ## Documentation/config/advice.txt ## +@@ + advice.*:: + These variables control various optional help messages designed to +- aid new users. All 'advice.*' variables default to 'true', and you +- can tell Git that you do not need help by setting these to 'false': ++ aid new users. When left unconfigured, Git will give the message ++ alongside instructions on how to squelch it. You can tell Git ++ that you do not need the help message by setting these to 'false': + + + -- + addEmbeddedRepo:: + ## advice.c ## @@ advice.c: static const char *advise_get_color(enum color_advice ix) return ""; } +enum advice_level { -+ ADVICE_LEVEL_DEFAULT = 0, ++ ADVICE_LEVEL_NONE = 0, + ADVICE_LEVEL_DISABLED, + ADVICE_LEVEL_ENABLED, +}; @@ advice.c: void advise_if_enabled(enum advice_type type, const char *advice, ...) va_start(params, advice); - vadvise(advice, 1, advice_setting[type].key, params); -+ vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT, -+ advice_setting[type].key, params); ++ vadvise(advice, !advice_setting[type].level, advice_setting[type].key, ++ params); va_end(params); } Documentation/config/advice.txt | 5 +- advice.c | 109 +++++++++++++++++--------------- t/t0018-advice.sh | 1 - 3 files changed, 62 insertions(+), 53 deletions(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index 25c0917524..c7ea70f2e2 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -1,7 +1,8 @@ advice.*:: These variables control various optional help messages designed to - aid new users. All 'advice.*' variables default to 'true', and you - can tell Git that you do not need help by setting these to 'false': + aid new users. When left unconfigured, Git will give the message + alongside instructions on how to squelch it. You can tell Git + that you do not need the help message by setting these to 'false': + -- addEmbeddedRepo:: diff --git a/advice.c b/advice.c index f6e4c2f302..6e9098ff08 100644 --- a/advice.c +++ b/advice.c @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix) return ""; } +enum advice_level { + ADVICE_LEVEL_NONE = 0, + ADVICE_LEVEL_DISABLED, + ADVICE_LEVEL_ENABLED, +}; + static struct { const char *key; - int enabled; + enum advice_level level; } advice_setting[] = { - [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 }, - [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec", 1 }, - [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile", 1 }, - [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec", 1 }, - [ADVICE_AM_WORK_DIR] = { "amWorkDir", 1 }, - [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName", 1 }, - [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 }, - [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 }, - [ADVICE_DIVERGING] = { "diverging", 1 }, - [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 }, - [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch", 1 }, - [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 }, - [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 }, - [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity", 1 }, - [ADVICE_NESTED_TAG] = { "nestedTag", 1 }, - [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning", 1 }, - [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists", 1 }, - [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst", 1 }, - [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce", 1 }, - [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent", 1 }, - [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching", 1 }, - [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate", 1 }, - [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName", 1 }, - [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected", 1 }, - [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward", 1 }, /* backwards compatibility */ - [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh", 1 }, - [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict", 1 }, - [ADVICE_RM_HINTS] = { "rmHints", 1 }, - [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse", 1 }, - [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure", 1 }, - [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1 }, - [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning", 1 }, - [ADVICE_STATUS_HINTS] = { "statusHints", 1 }, - [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 }, - [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated", 1 }, - [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 }, - [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 }, - [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 }, - [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, - [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 }, + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" }, + [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec" }, + [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile" }, + [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec" }, + [ADVICE_AM_WORK_DIR] = { "amWorkDir" }, + [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName" }, + [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge" }, + [ADVICE_DETACHED_HEAD] = { "detachedHead" }, + [ADVICE_DIVERGING] = { "diverging" }, + [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates" }, + [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch" }, + [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated" }, + [ADVICE_IGNORED_HOOK] = { "ignoredHook" }, + [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity" }, + [ADVICE_NESTED_TAG] = { "nestedTag" }, + [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning" }, + [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists" }, + [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst" }, + [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce" }, + [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent" }, + [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching" }, + [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate" }, + [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" }, + [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" }, + [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */ + [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" }, + [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" }, + [ADVICE_RM_HINTS] = { "rmHints" }, + [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" }, + [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" }, + [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" }, + [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning" }, + [ADVICE_STATUS_HINTS] = { "statusHints" }, + [ADVICE_STATUS_U_OPTION] = { "statusUoption" }, + [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated" }, + [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" }, + [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead" }, + [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath" }, + [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" }, + [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" }, }; static const char turn_off_instructions[] = @@ -116,13 +122,13 @@ void advise(const char *advice, ...) int advice_enabled(enum advice_type type) { - switch(type) { - case ADVICE_PUSH_UPDATE_REJECTED: - return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled && - advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled; - default: - return advice_setting[type].enabled; - } + int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED; + + if (type == ADVICE_PUSH_UPDATE_REJECTED) + return enabled && + advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS); + + return enabled; } void advise_if_enabled(enum advice_type type, const char *advice, ...) @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...) return; va_start(params, advice); - vadvise(advice, 1, advice_setting[type].key, params); + vadvise(advice, !advice_setting[type].level, advice_setting[type].key, + params); va_end(params); } @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value) for (i = 0; i < ARRAY_SIZE(advice_setting); i++) { if (strcasecmp(k, advice_setting[i].key)) continue; - advice_setting[i].enabled = git_config_bool(var, value); + advice_setting[i].level = git_config_bool(var, value) + ? ADVICE_LEVEL_ENABLED + : ADVICE_LEVEL_DISABLED; return 0; } diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh index c13057a4ca..0dcfb760a2 100755 --- a/t/t0018-advice.sh +++ b/t/t0018-advice.sh @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' ' test_expect_success 'advice should be printed when config variable is set to true' ' cat >expect <<-\EOF && hint: This is a piece of advice - hint: Disable this message with "git config advice.nestedTag false" EOF test_config advice.nestedTag true && test-tool advise "This is a piece of advice" 2>actual && base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d -- 2.42.0 ^ permalink raw reply related [flat|nested] 36+ messages in thread
end of thread, other threads:[~2024-01-20 15:32 UTC | newest] Thread overview: 36+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-09 15:25 [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled() Rubén Justo 2024-01-09 15:29 ` [PATCH 1/3] t/test-tool: usage description Rubén Justo 2024-01-09 18:19 ` Junio C Hamano 2024-01-09 15:29 ` [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments Rubén Justo 2024-01-09 18:19 ` Junio C Hamano 2024-01-09 18:20 ` Taylor Blau 2024-01-09 15:30 ` [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled() Rubén Justo 2024-01-09 18:23 ` Junio C Hamano 2024-01-09 18:27 ` Taylor Blau 2024-01-09 19:57 ` Junio C Hamano 2024-01-10 12:11 ` Rubén Justo 2024-01-10 11:02 ` Jeff King 2024-01-10 11:39 ` Rubén Justo 2024-01-10 14:18 ` Dragan Simic 2024-01-10 14:32 ` Rubén Justo 2024-01-10 14:44 ` Dragan Simic 2024-01-10 16:22 ` Junio C Hamano 2024-01-10 17:45 ` Dragan Simic 2024-01-11 8:04 ` Jeff King 2024-01-18 6:15 ` Dragan Simic 2024-01-18 18:26 ` Junio C Hamano 2024-01-18 18:53 ` Dragan Simic 2024-01-18 20:19 ` Junio C Hamano 2024-01-18 20:50 ` Dragan Simic 2024-01-20 11:31 ` Rubén Justo 2024-01-20 15:31 ` Dragan Simic 2024-01-10 16:14 ` Junio C Hamano 2024-01-09 18:28 ` [PATCH 0/3] " Taylor Blau 2024-01-09 22:32 ` Junio C Hamano 2024-01-10 12:40 ` Rubén Justo 2024-01-12 10:05 ` [PATCH] advice: " Rubén Justo 2024-01-12 22:19 ` Junio C Hamano 2024-01-13 7:38 ` Jeff King 2024-01-16 4:56 ` Junio C Hamano 2024-01-15 11:24 ` Rubén Justo 2024-01-15 14:28 ` [PATCH v2] " Rubén Justo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).