From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 10 Feb 2020 19:30:46 GMT
Subject: Re: [PATCH] advice: refactor advise API
Message-ID: <xmqq8slaz1cp.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <97406f9e-b8ef-b5b9-3987-cdef843b31a5@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

>> +static const char turn_off_instructions[] =
>> +N_("\n"
>> +"Turn this message off by running\n"
>> +"\"git config %s false\"");
>
> I have mixed feelings on the use of these instructions. Perhaps at
> minimum the addition of these instructions could be left to a
> separate patch than the creation of advise_ng().
>
> My biggest concern is that this adds unexpected noise to users who
> want the advice to stay. I'm calling attention to it, because this
> part isn't a simple refactor like the rest of the patch.
> ...
> I definitely tend to recommend more tests than most, but perhaps this
> unit test is overkill? You demonstrate a good test below using a real
> Git command, which should be sufficient. If the "turn this message off"
> part gets removed, then you will still have coverage of your method.
> It just won't require a test change because it would not modify behavior.
> ...

All good suggestions.  Thanks for an excellent review.

Another thing.  

advise_ng() may have been a good name for illustration but is a
horrible name for real-world use (imagine we need to revamp the API
one more time in the future---what would it be called, which has to
say that it is newer than the "next generation"?
advise_3rd_try()?).

Thanks.