Re: [RFC PATCH] sequencer: warn on skipping previously seen commit

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

On 04/08/2021 22:28, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
>> Silently skipping commits when rebasing with --no-reapply-cherry-picks
>> (currently the default behavior) can cause user confusion. Issue a
>> warning in this case so that users are aware of what's happening.
>>
>> Signed-off-by: Josh Steadmon <steadmon@google.com>
>> ---
>>
>> We've had some complaints at $JOB where users were confused when
>> rebasing branches that contained commits that were previously
>> cherry-picked into their master branch. How do folks feel about adding a
>> warning in this case?
> 
> I'd unconditionally in support if this were done under --verbose
> option, but it becomes iffy if this is done unconditionally.

Perhaps we could skip the warning if the user is going to edit the todo 
list as they should see that the skipped commits have been commented 
out. I'm not sure about requiring --verbose - that might mean the users 
who would benefit most end up missing warning. As you say below I think 
it depends how often it appears in practice.

> This is because I do not expect everybody will stay to be ignorant
> of the behaviour of the tool they use every day, and I'd fear that
> we'd start hearing "yeah, I know the command would skip to avoid
> duplicated changes, why waste lines to tell me that?" complaints.
> 
> Having said that, I _hope_ that in a project with good hygiene, such
> a multiple cherry-picking would not be so common and an exception,
> and if my _hope_ proves to be true, then I am OK with giving this
> warning unconditionally.  The user may know what the command does
> when it sees a duplicated change, but the warning becomes about the
> presence of such duplicated changes, which would be a rare event
> that is worth notifying about.
> 
>>   		is_empty = is_original_commit_empty(commit);
>> -		if (!is_empty && (commit->object.flags & PATCHSAME))
>> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +			warning(_("skipped previously seen commit %s"),
> 
> I am debating myself if s/seen/applied/ should be suggested here.
> 
> The existing text in the manual page says "a patch already accepted
> upstream with a different commit message or timestamp will be
> skipped", and "accepted" is a verb that would apply only in a
> certain workflow, which is OK in the manual page that give more
> context, but not here.  But 'seen' feels a bit too weak to me.

Yes, I think 'applied' or 'cherry-picked' would be better than 'seen'

>> +	if (skipped_commit)
>> +		warning(_("use --reapply-cherry-picks to include skipped commits"));
> 
> I'd be hesitant to endorse doing this kind of "here is how to use
> this command" unconditionally.  Perhaps under --verbose, or hide it
> under "advise.*".

and use advise() rather than warning(). I'm guess this might be helpful 
but it wont help them get their commits back as there is no way to stop 
the rebase from dropping them at that point.

Best Wishes

Phillip

> Thanks.
>