Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Josh Steadmon <steadmon@google.com>]

On 2021.08.12 13:45, Philippe Blain wrote:
> Hi Josh,
> 
> Le 2021-08-10 à 15:20, Josh Steadmon a écrit :
> > Silently skipping commits when rebasing with --no-reapply-cherry-picks
> > (currently the default behavior) can cause user confusion. Issue advice
> > in this case so that users are aware of what's happening.
> 
> I think this is an excellent idea. It can be very surprising, especially
> for 'git rebase' beginners/intermediate users who might not have read the
> man page.
> 
> Since your proposed changes are in sequencer.c, this will only affect
> the default "merge" rebase backend, and not the older 'apply' backend. I think
> it might be worth mentioning this in the commit message.
> 
> Note that it might be considerably more work to also add the warning
> for the 'apply' backend, since rebase.c::run_am generates the patches
> using 'git format-patch --cherry-pick --right-only $upstream..HEAD' and
> so cherry-picks are dropped early in the process. I think that this not that big
> of a deal since the default backend is now "merge".

Ah good point, thank you for the catch. I have noted this in the V3
commit message.

> > 
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> > Changes in V2:
> > * use advise_if_enabled() instead of warning()
> > * s/seen/applied/ in the advice text
> > 
> >   Documentation/config/advice.txt |  3 +++
> >   advice.c                        |  3 +++
> >   advice.h                        |  1 +
> >   sequencer.c                     | 22 ++++++++++++++++++++--
> >   4 files changed, 27 insertions(+), 2 deletions(-)
> 
> I would suggest mentioning the new behaviour and the new
> advice.skippedCherryPicks config in git-rebase.txt, say in the paragraph
> starting with "If the upstream branch already contains" in the Description section
> and in the description of '--reapply-cherry-picks'.

Done in V3.


> >   int git_default_advice_config(const char *var, const char *value);
> > diff --git a/sequencer.c b/sequencer.c
> > index 7f07cd00f3..1235f61c9d 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >   	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
> >   	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
> >   	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
> > +	int skipped_commit = 0;
> >   	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
> >   	struct strbuf label = STRBUF_INIT;
> >   	struct commit_list *commits = NULL, **tail = &commits, *iter;
> > @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >   		oidset_insert(&interesting, &commit->object.oid);
> >   		is_empty = is_original_commit_empty(commit);
> > -		if (!is_empty && (commit->object.flags & PATCHSAME))
> > +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> > +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +					_("skipped previously applied commit %s"),
> > +					short_commit_name(commit));
> > +			skipped_commit = 1;
> >   			continue;
> > +		}
> >   		if (is_empty && !keep_empty)
> >   			continue;
> 
> For interactive rebase, an alternate implementation, that I suggested in [1] last summer, would be to keep
> the cherry-picks in the todo list, but mark them as 'drop' and add a comment at the
> end of their line, like '# already applied' or something like this, similar
> to how empty commits have '# empty' appended. I think that for interactive rebase, I
> would prefer this, since it is easier for the user to notice it and change the 'drop'
> to 'pick' right away if they realise they do not want to drop those commits (easier
> than seeing the warning, realising they did not want to drop them, aborting the rebase
> and redoing it with '--reapply-cherry-picks').

I haven't had time to do this in V3, but I can look into it for V4.

> For non-interactive rebase adding a warning/advice like your patch does seems to
> be a good solution.
> 
> > @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >   		oidcpy(&entry->entry.oid, &commit->object.oid);
> >   		oidmap_put(&commit2todo, entry);
> >   	}
> > +	if (skipped_commit)
> > +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +				  _("use --reapply-cherry-picks to include skipped commits"));
> >   	/*
> >   	 * Second phase:
> > @@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
> >   	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
> >   	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
> >   	int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;
> > +	int skipped_commit = 0;
> >   	repo_init_revisions(r, &revs, NULL);
> >   	revs.verbose_header = 1;
> > @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
> >   	while ((commit = get_revision(&revs))) {
> >   		int is_empty = is_original_commit_empty(commit);
> > -		if (!is_empty && (commit->object.flags & PATCHSAME))
> > +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> > +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +					  _("skipped previously applied commit %s"),
> > +					  short_commit_name(commit));
> > +			skipped_commit = 1;
> >   			continue;
> > +		}
> >   		if (is_empty && !keep_empty)
> >   			continue;
> >   		strbuf_addf(out, "%s %s ", insn,
> > @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
> >   			strbuf_addf(out, " %c empty", comment_line_char);
> >   		strbuf_addch(out, '\n');
> >   	}
> > +	if (skipped_commit)
> > +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +				  _("use --reapply-cherry-picks to include skipped commits"));
> >   	return 0;
> >   }
> > 
> 
> Like Junio remarked, it is a little unfortunate that some logic is duplicated between
> 'sequencer_make_script' and 'make_script_with_merges', such that your patch has to do
> the same thing at two different code locations. Maybe a preparatory cleanup could add
> a new function that takes care of the duplicated logic and call if from both ? I'm
> just thinking out loud here, I did not analyze in details if this would be easy/feasible...

Will look into this for V4 as well.

> Thanks for suggesting this change,
> 
> Philippe.
> 
> 
> [1] https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/