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

From: Rubén Justo <rjusto@gmail.com>
Date: Jan 10, 2024, 12:11 UTC
Message-ID: <19bdce2f-a854-48a6-b865-32700971c34d@gmail.com>
In-Reply-To: <ZZ2QGYBBmK8cSYBD@nand.local>

On 09-ene-2024 13:27:37, Taylor Blau wrote:

Show 22 quoted lines

> > +	[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 ;-)

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

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.