On 20/06/2019 04:40, Junio C Hamano wrote:
Rohit Ashiwal <rohit.ashiwal265@xxxxxxxxx> writes:+give_advice: + advise(_("have you committed already?\n" + "try \"git %s --continue\""), + action == REPLAY_REVERT ? "revert" : "cherry-pick"); + return error(_("there is nothing to skip")); +}Two comments. The places touched by patch 1/5 emitted the error followed by an advice message; this new one breaks the pattern by giving the "hint:" first and then error. Be consistent by swapping these two (and return -1, as "error() that returns -1" will no longer be the last thing executed in this function. This one, and the in_progress_advice emitted from the patch 1/5, are both bad in that they make calls to advise() without guarding it with an advice.* configuration variable.
I'm not sure we have one for cherry-pick/revert/rebase. At the moment they print advice advice for a failed pick unconditionally (the caller of `print_advice()` sets `show_hint` based on the result of the merge rather than user preference) it would be nice to fix that. Maybe that should be checking advice.resolveConflict though. I'm also not sure if that is really within the scope of this patch series.
Best Wishes Phillip
This does not allow the user to say say "I've learned this part of Git enough; do not tell me verbosely." Pick a random global variable that is defined near the top of advice.c, and learn how they are set (to true by default, allowing configuration to flip it off) and how they are used in order to prevent a call to advise() getting made. Then mimick that to guard these calls to advise(). Thanks.
