Re: [PATCH] advice: refactor advise API

[Taylor Blau <me@ttaylorr.com>]

On Mon, Feb 10, 2020 at 11:30:46AM -0800, Junio C Hamano wrote:
> 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()?).

What about calling this new API 'advise()'? The first patch could call
it 'advise_ng' or whatever other temporary name we feel comfortable
using, and then each subsequent patch would update callers of 'advise()'
to use 'advise_ng()'. Once those patches have been applied, and no other
callers of 'advise()' exist, a final patch can be applied on top to
rename 'advise_ng()' to 'advise()', and update the names of all of the
callers.

This makes for a rather noisy final patch, but the intermediate states
are much clearer, and it would make this series rather self-contained.

On the other hand, having a version of 'advise_ng()' on master makes
this topic more incremental, meaning that we can pick it up and put it
down at ease and have more self-contained projects.

I don't really have a preference between the two approaches, but if we
go with the latter, I do think we need something better than
'advise_ng'. Maybe 'advise_warn'? I don't know.

> Thanks.

Thanks,
Taylor