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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 20, 2019, 09:57 UTC
Message-ID: <76359a86-0eb1-dd87-c36f-7bd2df8b0ae5@gmail.com>
In-Reply-To: <xmqqr27oq5z1.fsf@gitster-ct.c.googlers.com>

On 20/06/2019 04:40, Junio C Hamano wrote:

Show 20 quoted lines

> 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

Show 12 quoted lines

> 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.
>