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

On 04/08/2021 22:28, Junio C Hamano wrote:

Josh Steadmon [off-list ref] writes:

quoted

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

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.

quoted

  		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'

quoted

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