Re: [GSoC][PATCH v5 4/5] cherry-pick/revert: add --skip option

[Phillip Wood <phillip.wood123@gmail.com>]

On 20/06/2019 04:40, Junio C Hamano wrote:
> Rohit Ashiwal <rohit.ashiwal265@gmail.com> 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.
>