On Mon, Feb 10, 2020 at 11:42:53AM -0800, Taylor Blau wrote:
> On Mon, Feb 10, 2020 at 11:30:46AM -0800, Junio C Hamano wrote:
> > Derrick Stolee <stolee@xxxxxxxxx> 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.
I think this is the precise strategy called out in the patch
description.
https://lore.kernel.org/git/pull.548.git.1581311049547.gitgitgadget@xxxxxxxxx
> 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.
I like that this opens up the possibility of advise_err(), advise_die(),
whatever to meet Peff's suggestion.
- Emily
