On 12/08/2021 18:45, Philippe Blain wrote:
Hi Josh,
[...]
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').
That would be nice, but we could always add it in the future if Josh
does not want to implement it now. At the moment the function that
creates the todo list does not know if it is going to be edited, I'm not
sure how easy it would be to pass that information down.
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...
I think feasible but not easy (or required for this change), it would
also complicate the code in a different way as I think we'd have to add
some conditionals for whether we are recreating merges or not.
Best Wishes
Phillip
Thanks for suggesting this change,
Philippe.
[1]
https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@xxxxxxxxx/
