| Message ID | 4d83766ab3425a5f4b361df2ac505d07fefd7899.1628109852.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] sequencer: warn on skipping previously seen commit | expand |
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. 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. > + 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.*". Thanks.
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. >
Phillip Wood <phillip.wood123@gmail.com> writes: >>> + 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. Yes, but aborting the current rebase and redoing from scratch should not be a brain surgery. Even if you had already resolved conflicts in earlier steps, the work will be replayed automatically for you by the rerere mechanism.
diff --git a/sequencer.c b/sequencer.c index 7f07cd00f3..8888031c7b 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,12 @@ 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)) { + warning(_("skipped previously seen commit %s"), + short_commit_name(commit)); + skipped_commit = 1; continue; + } if (is_empty && !keep_empty) continue; @@ -5214,6 +5219,8 @@ 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) + warning(_("use --reapply-cherry-picks to include skipped commits")); /* * Second phase: @@ -5334,6 +5341,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 +5377,12 @@ 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)) { + warning(_("skipped previously seen commit %s"), + short_commit_name(commit)); + skipped_commit = 1; continue; + } if (is_empty && !keep_empty) continue; strbuf_addf(out, "%s %s ", insn, @@ -5380,6 +5392,8 @@ 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) + warning(_("use --reapply-cherry-picks to include skipped commits")); return 0; }
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? sequencer.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)