Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 12, 2024, 22:19 UTC
- Message-ID
- <xmqqsf326vpn.fsf@gitster.g>
- In-Reply-To
- <c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
Show 14 quoted lines
> 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)
Show 15 quoted lines
> + 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.
Show 13 quoted lines
> 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; > }
Show 8 quoted lines
> 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.
Show 11 quoted lines
> 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.
Show 12 quoted lines
> 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::