From: Heba Waly <heba.waly@gmail.com>
Date: Wed, 29 Jan 2020 02:04:17 GMT
Subject: Re: [PATCH v2 1/1] add: use advise function to display hints
Message-ID: <CACg5j25fi0nSMe2nH+iJ184ouk2VsrBWSZmEv-C_PrY8qfq4Sw@mail.gmail.com>
In-Reply-To: <20200128000047.176372-1-jonathantanmy@google.com>

On Tue, Jan 28, 2020 at 1:00 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Use the advise function in advice.c to display hints to the users, as
> > it provides a neat and a standard format for hint messages, i.e: the
> > text is colored in yellow and the line starts by the word "hint:".
> >
> > Also this will enable us to control the messages using advice.*
> > configuration variables.
>
> Firstly, sorry for getting back to this so late.
>
> As written, this gives me the impression that advise() is what enables
> us to control the messages using configuration variables, but that's not
> true - that's done by a separate mechanism in advise.c and .h.
> Paraphrasing what Junio wrote [1], the commit message might be better
> written as:
>
>   In the "add" command, use the advice API instead of fprintf() for the
>   hint shown when nothing was added. Thus, this hint message follows the
>   standard hint message format, and its visibility is made configurable.
>
> (Note that I mentioned the "add" command and called it the advice API
> instead of the advise() function.)
>

That makes sense.

> (Feel free to use this or write your own.)
>
> [1] https://lore.kernel.org/git/xmqqpng1eisc.fsf@gitster-ct.c.googlers.com/
>
> > diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> > index c325167b90..a649805369 100755
> > --- a/t/t3700-add.sh
> > +++ b/t/t3700-add.sh
> > @@ -326,7 +326,7 @@ test_expect_success 'git add --dry-run of an existing file output' "
> >  cat >expect.err <<\EOF
> >  The following paths are ignored by one of your .gitignore files:
> >  ignored-file
> > -Use -f if you really want to add them.
> > +hint: Use -f if you really want to add them.
> >  EOF
> >  cat >expect.out <<\EOF
> >  add 'track-this'
>
> Also add a test that checks what happens if advice.addNothing is set.
> (It seems that we generally don't test what happens when advice is
> suppressed. If we consider solely this patch, I'm on the fence of the
> usefulness of this test, but if we plan to refactor the advise()
> function to take care of checking the config variable itself, for
> example, then we will need such a test anyway, so I think we might as
> well include at least one such advice test now.)

I'm tempted to say let's worry about it when refactoring advise(),
maybe then we'll
find a more suitable place for this test. as it'll be advice-related,
not caller-related.

Thanks,
Heba