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

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 9, 2024, 18:27 UTC
Message-ID: <ZZ2QGYBBmK8cSYBD@nand.local>
In-Reply-To: <d6099d78-43c6-4709-9121-11f84228cf91@gmail.com>

On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:

Show 9 quoted lines

> 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.

Show 18 quoted lines

> 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...

Show 10 quoted lines

>  };
>
>  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.

Show 38 quoted lines

>
>  	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