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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jan 28, 2020, 00:00 UTC
Message-ID: <20200128000047.176372-1-jonathantanmy@google.com>
In-Reply-To: <9f9febd3f4f7f82178fceac98fcc91cb28a1b3b9.1578438752.git.gitgitgadget@gmail.com>

Show 8 quoted lines

> 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/

Show 13 quoted lines

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