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

[Junio C Hamano <gitster@pobox.com>]

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.