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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx/
