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.
