| Message ID | 496da0b17476011b4ef4dde31593afc7572246eb.1628623058.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] sequencer: advise if skipping cherry-picked commit | expand |
Josh Steadmon <steadmon@google.com> writes: > + skippedCherryPicks:: > + Shown when linkgit:git-rebase[1] skips a commit that has already > + been cherry-picked onto the upstream branch. Makes sense. > + [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1}, No need to resend to only fix this, but I'll add SP after "1" to match surrounding lines while queuing the patch. > @@ -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; > > @@ -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")); I agree with the change in this hunk that advanced users may want to squelch this "what to do" hint. I am not sure about the earlier hunk that reports when some commits have actually been skipped. When --no-reapply-cherry-picks is in effect, the user is expecting that some commits are cherry-picks among other (hopefully the majority of) commits, and even those users who do not want to be taught how to use the command would want to learn the fact that some commits were skipped (and which ones). Using two separate advice configuration variables feel way overkill for this. I wonder if the previous hunk should use warning(), i.e. > + if (!is_empty && (commit->object.flags & PATCHSAME)) { > + warning(_("skipped previously applied commit %s"), > + short_commit_name(commit)); > + skipped_commit = 1; > continue; > + } possibly squelched by "git rebase --quiet". > @@ -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; > + } Likewise. I wonder why we have two so-much-similar codepaths that we need to touch, though. > 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; > }
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". > > 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'. > 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'). 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... Thanks for suggesting this change, Philippe. [1] https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/
Philippe Blain <levraiphilippeblain@gmail.com> writes: > 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. It shouldn't be too hard to teach "format-patch --cherry-pick" to report when commits are actually dropped. There isn't necessarily a need to give help to users what to do to apply the same change twice, but even for those who _know_ how to do so, they need to know if commits are dropped in the first place. And such a notice is useful for any user of the --cherry-pick feature of format-patch, not just as an implementation detail of "git rebase".
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@gmail.com/
On 10/08/2021 23:33, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: >> @@ -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; >> >> @@ -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")); > > I agree with the change in this hunk that advanced users may want to > squelch this "what to do" hint. > > I am not sure about the earlier hunk that reports when some commits > have actually been skipped. When --no-reapply-cherry-picks is in > effect, the user is expecting that some commits are cherry-picks > among other (hopefully the majority of) commits, and even those > users who do not want to be taught how to use the command would want > to learn the fact that some commits were skipped (and which ones). > > Using two separate advice configuration variables feel way overkill > for this. I wonder if the previous hunk should use warning(), i.e. > >> + if (!is_empty && (commit->object.flags & PATCHSAME)) { >> + warning(_("skipped previously applied commit %s"), >> + short_commit_name(commit)); >> + skipped_commit = 1; >> continue; >> + } > > possibly squelched by "git rebase --quiet". I think that would be nicer. Using advise_if_enabled() in the second hunk makes sense but I think a printing "warning" rather than a "hint" in the first hunk. >> @@ -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; >> + } > > Likewise. I wonder why we have two so-much-similar codepaths that > we need to touch, though. --recreate-merges implemented the main loop for its todo list generation in a new function as it is much more involved that for the linear case. Best Wishes Phillip > >> 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; >> }
Hi Phillip, Le 2021-08-18 à 06:02, Phillip Wood a écrit : > 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. Well, I'm not sure we need to ? If we change the 'pick' to 'drop', instead of not writing these lines to the todo list, the cherry-picked commits will get dropped either way, no? Unless I'm missing something - I think you are way more well-versed in the sequencer code than me :) >> >> 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. Yes, I agree it's not required for this change. Cheers, Philippe.
Hi Philippe On 18/08/2021 23:45, Philippe Blain wrote: > Hi Phillip, > [...] >>> 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. > > Well, I'm not sure we need to ? If we change the 'pick' to 'drop', > instead of > not writing these lines to the todo list, the cherry-picked commits will > get dropped > either way, no? Unless I'm missing something - I think you are way more > well-versed in > the sequencer code than me :) I think I read your suggestion as meaning we would not print the warning if the user was going to edit the list - hence the need for the distinction. I've just had a closer look at the code and I think it would be fairly easy to tell sequencer_make_script() if the todo list is going to be edited, there is only one caller in builtin/rebase.c which could easily pass a flag for this. Thinking about the 'print warning' vs 'add drop command' issue if the user is using a terminal based editor such as vim or nano then they will barely have time to see the warnings being printed let alone read them before the editor is opened and hides them so having some indication in the todo list would probably be a good idea. Best Wishes Phillip >>> >>> 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. > > Yes, I agree it's not required for this change. > > Cheers, > > Philippe.
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 advice > in this case so that users are aware of what's happening. > > 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(-) [jc: no need for immediate action from Josh] With <patch-v4-2.4-3869bda3b39-20210823T103719Z-avarab@gmail.com> it would make more sense to stop using the "global variable registered with advice_config[] array" part of the patch, especially because we only use advise_if_enabled() already in this patch. > + 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; > + } > @@ -139,6 +141,7 @@ static struct { > [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 }, > [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 }, > [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, > + [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1}, > }; This needs to be moved to keep the list sorted. When I queued the topic to migrate to advise-settings from advice-config, I made some adjustment for this topic (and luckily there is no other topic in flight that touch advice.c). Please sanity check the result when today's integration result is pushed out, perhaps in 5 hours or so. Thanks.
On 2021.08.10 15:33, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > + [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1}, > > No need to resend to only fix this, but I'll add SP after "1" to > match surrounding lines while queuing the patch. Ack. Fixed in V3. Do you also want me to rebase this on ab/retire-advice-config? > > @@ -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; > > > > @@ -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")); > > I agree with the change in this hunk that advanced users may want to > squelch this "what to do" hint. > > I am not sure about the earlier hunk that reports when some commits > have actually been skipped. When --no-reapply-cherry-picks is in > effect, the user is expecting that some commits are cherry-picks > among other (hopefully the majority of) commits, and even those > users who do not want to be taught how to use the command would want > to learn the fact that some commits were skipped (and which ones). > > Using two separate advice configuration variables feel way overkill > for this. I wonder if the previous hunk should use warning(), i.e. Switched these to warnings, squelched by "--quiet" as suggested below. > > + if (!is_empty && (commit->object.flags & PATCHSAME)) { > > + warning(_("skipped previously applied commit %s"), > > + short_commit_name(commit)); > > + skipped_commit = 1; > > continue; > > + } > > possibly squelched by "git rebase --quiet". >
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/
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index 8b2849ff7b..063eec2511 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -44,6 +44,9 @@ advice.*:: Shown when linkgit:git-push[1] rejects a forced update of a branch when its remote-tracking ref has updates that we do not have locally. + skippedCherryPicks:: + Shown when linkgit:git-rebase[1] skips a commit that has already + been cherry-picked onto the upstream branch. statusAheadBehind:: Shown when linkgit:git-status[1] computes the ahead/behind counts for a local ref compared to its remote tracking ref, diff --git a/advice.c b/advice.c index 0b9c89c48a..7768319b4a 100644 --- a/advice.c +++ b/advice.c @@ -34,6 +34,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1; int advice_submodule_alternate_error_strategy_die = 1; int advice_add_ignored_file = 1; int advice_add_empty_pathspec = 1; +int advice_skipped_cherry_picks = 1; static int advice_use_color = -1; static char advice_colors[][COLOR_MAXLEN] = { @@ -96,6 +97,7 @@ static struct { { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die }, { "addIgnoredFile", &advice_add_ignored_file }, { "addEmptyPathspec", &advice_add_empty_pathspec }, + { "skippedCherryPicks", &advice_skipped_cherry_picks }, /* make this an alias for backward compatibility */ { "pushNonFastForward", &advice_push_update_rejected } @@ -139,6 +141,7 @@ static struct { [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 }, [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 }, [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, + [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1}, }; static const char turn_off_instructions[] = diff --git a/advice.h b/advice.h index 9f8ffc7354..d705bf164c 100644 --- a/advice.h +++ b/advice.h @@ -75,6 +75,7 @@ extern int advice_add_empty_pathspec; ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE, ADVICE_UPDATE_SPARSE_PATH, ADVICE_WAITING_FOR_EDITOR, + ADVICE_SKIPPED_CHERRY_PICKS, }; 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; @@ -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; }
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. 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(-)