Re: [PATCH v2 1/1] add: use advise function to display hints

[Jonathan Tan <jonathantanmy@google.com>]

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

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