On Tue, Jan 28, 2020 at 1:00 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote: > > > From: Heba Waly <heba.waly@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxx/ > > > 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
