{"thread":{"id":"56216","subject":"[RFC PATCH] sequencer: warn on skipping previously seen commit","startedAt":"2021-08-04T20:53:49Z","lastAt":"2021-08-30T23:40:13Z","messageCount":18,"participants":["Josh Steadmon","Junio C Hamano","Phillip Wood","Philippe Blain"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"431979","messageId":"4d83766ab3425a5f4b361df2ac505d07fefd7899.1628109852.git.steadmon@google.com","threadId":"56216","inReplyTo":null,"subject":"[RFC PATCH] sequencer: warn on skipping previously seen commit","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2021-08-04T20:53:45Z","receivedAt":"2021-08-04T20:53:49Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Silently skipping commits when rebasing with --no-reapply-cherry-picks\n(currently the default behavior) can cause user confusion. Issue a\nwarning in this case so that users are aware of what's happening.\n\nSigned-off-by: Josh Steadmon \n---\n\nWe've had some complaints at $JOB where users were confused when\nrebasing branches that contained commits that were previously\ncherry-picked into their master branch. How do folks feel about adding a\nwarning in this case?\n\n sequencer.c | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7f07cd00f3..8888031c7b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n \tint rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;\n \tint root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;\n+\tint skipped_commit = 0;\n \tstruct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;\n \tstruct strbuf label = STRBUF_INIT;\n \tstruct commit_list *commits = NULL, **tail = &commits, *iter;\n@@ -5149,8 +5150,12 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\toidset_insert(&interesting, &commit->object.oid);\n \n \t\tis_empty = is_original_commit_empty(commit);\n-\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n+\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n+\t\t\twarning(_(\"skipped previously seen commit %s\"),\n+\t\t\t\tshort_commit_name(commit));\n+\t\t\tskipped_commit = 1;\n \t\t\tcontinue;\n+\t\t}\n \t\tif (is_empty && !keep_empty)\n \t\t\tcontinue;\n \n@@ -5214,6 +5219,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n \t\toidmap_put(&commit2todo, entry);\n \t}\n+\tif (skipped_commit)\n+\t\twarning(_(\"use --reapply-cherry-picks to include skipped commits\"));\n \n \t/*\n \t * Second phase:\n@@ -5334,6 +5341,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n \tint reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;\n+\tint skipped_commit = 0;\n \n \trepo_init_revisions(r, &revs, NULL);\n \trevs.verbose_header = 1;\n@@ -5369,8 +5377,12 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \twhile ((commit = get_revision(&revs))) {\n \t\tint is_empty = is_original_commit_empty(commit);\n \n-\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n+\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n+\t\t\twarning(_(\"skipped previously seen commit %s\"),\n+\t\t\t\tshort_commit_name(commit));\n+\t\t\tskipped_commit = 1;\n \t\t\tcontinue;\n+\t\t}\n \t\tif (is_empty && !keep_empty)\n \t\t\tcontinue;\n \t\tstrbuf_addf(out, \"%s %s \", insn,\n@@ -5380,6 +5392,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \t\t\tstrbuf_addf(out, \" %c empty\", comment_line_char);\n \t\tstrbuf_addch(out, '\\n');\n \t}\n+\tif (skipped_commit)\n+\t\twarning(_(\"use --reapply-cherry-picks to include skipped commits\"));\n \treturn 0;\n }\n \n-- \n2.32.0.554.ge1b32706d8-goog\n\n"},{"id":"431984","messageId":"xmqqsfzonbm1.fsf@gitster.g","threadId":"56216","inReplyTo":"4d83766ab3425a5f4b361df2ac505d07fefd7899.1628109852.git.steadmon@google.com","subject":"Re: [RFC PATCH] sequencer: warn on skipping previously seen commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-04T21:28:22Z","receivedAt":"2021-08-04T21:28:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Silently skipping commits when rebasing with --no-reapply-cherry-picks\n> (currently the default behavior) can cause user confusion. Issue a\n> warning in this case so that users are aware of what's happening.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n>\n> We've had some complaints at $JOB where users were confused when\n> rebasing branches that contained commits that were previously\n> cherry-picked into their master branch. How do folks feel about adding a\n> warning in this case?\n\nI'd unconditionally in support if this were done under --verbose\noption, but it becomes iffy if this is done unconditionally.\n\nThis is because I do not expect everybody will stay to be ignorant\nof the behaviour of the tool they use every day, and I'd fear that\nwe'd start hearing \"yeah, I know the command would skip to avoid\nduplicated changes, why waste lines to tell me that?\" complaints.\n\nHaving said that, I _hope_ that in a project with good hygiene, such\na multiple cherry-picking would not be so common and an exception,\nand if my _hope_ proves to be true, then I am OK with giving this\nwarning unconditionally. The user may know what the command does\nwhen it sees a duplicated change, but the warning becomes about the\npresence of such duplicated changes, which would be a rare event\nthat is worth notifying about.\n\n> \t\tis_empty = is_original_commit_empty(commit);\n> -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> +\t\t\twarning(_(\"skipped previously seen commit %s\"),\n\nI am debating myself if s/seen/applied/ should be suggested here.\n\nThe existing text in the manual page says \"a patch already accepted\nupstream with a different commit message or timestamp will be\nskipped\", and \"accepted\" is a verb that would apply only in a\ncertain workflow, which is OK in the manual page that give more\ncontext, but not here. But 'seen' feels a bit too weak to me.\n\n> +\tif (skipped_commit)\n> +\t\twarning(_(\"use --reapply-cherry-picks to include skipped commits\"));\n\nI'd be hesitant to endorse doing this kind of \"here is how to use\nthis command\" unconditionally. Perhaps under --verbose, or hide it\nunder \"advise.*\".\n\nThanks.\n"},{"id":"432077","messageId":"d3f6eb6c-f2e4-b00c-3b4c-bf1e04c846b4@gmail.com","threadId":"56216","inReplyTo":"xmqqsfzonbm1.fsf@gitster.g","subject":"Re: [RFC PATCH] sequencer: warn on skipping previously seen commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-05T10:13:40Z","receivedAt":"2021-08-05T10:13:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/08/2021 22:28, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n>> Silently skipping commits when rebasing with --no-reapply-cherry-picks\n>> (currently the default behavior) can cause user confusion. Issue a\n>> warning in this case so that users are aware of what's happening.\n>>\n>> Signed-off-by: Josh Steadmon \n>> ---\n>>\n>> We've had some complaints at $JOB where users were confused when\n>> rebasing branches that contained commits that were previously\n>> cherry-picked into their master branch. How do folks feel about adding a\n>> warning in this case?\n> \n> I'd unconditionally in support if this were done under --verbose\n> option, but it becomes iffy if this is done unconditionally.\n\nPerhaps we could skip the warning if the user is going to edit the todo \nlist as they should see that the skipped commits have been commented \nout. I'm not sure about requiring --verbose - that might mean the users \nwho would benefit most end up missing warning. As you say below I think \nit depends how often it appears in practice.\n\n> This is because I do not expect everybody will stay to be ignorant\n> of the behaviour of the tool they use every day, and I'd fear that\n> we'd start hearing \"yeah, I know the command would skip to avoid\n> duplicated changes, why waste lines to tell me that?\" complaints.\n> \n> Having said that, I _hope_ that in a project with good hygiene, such\n> a multiple cherry-picking would not be so common and an exception,\n> and if my _hope_ proves to be true, then I am OK with giving this\n> warning unconditionally. The user may know what the command does\n> when it sees a duplicated change, but the warning becomes about the\n> presence of such duplicated changes, which would be a rare event\n> that is worth notifying about.\n> \n>> \t\tis_empty = is_original_commit_empty(commit);\n>> -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n>> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n>> +\t\t\twarning(_(\"skipped previously seen commit %s\"),\n> \n> I am debating myself if s/seen/applied/ should be suggested here.\n> \n> The existing text in the manual page says \"a patch already accepted\n> upstream with a different commit message or timestamp will be\n> skipped\", and \"accepted\" is a verb that would apply only in a\n> certain workflow, which is OK in the manual page that give more\n> context, but not here. But 'seen' feels a bit too weak to me.\n\nYes, I think 'applied' or 'cherry-picked' would be better than 'seen'\n\n>> +\tif (skipped_commit)\n>> +\t\twarning(_(\"use --reapply-cherry-picks to include skipped commits\"));\n> \n> I'd be hesitant to endorse doing this kind of \"here is how to use\n> this command\" unconditionally. Perhaps under --verbose, or hide it\n> under \"advise.*\".\n\nand use advise() rather than warning(). I'm guess this might be helpful \nbut it wont help them get their commits back as there is no way to stop \nthe rebase from dropping them at that point.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> \n\n"},{"id":"432120","messageId":"xmqqpmurlur9.fsf@gitster.g","threadId":"56216","inReplyTo":"d3f6eb6c-f2e4-b00c-3b4c-bf1e04c846b4@gmail.com","subject":"Re: [RFC PATCH] sequencer: warn on skipping previously seen commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-05T16:30:02Z","receivedAt":"2021-08-05T16:30:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> +\tif (skipped_commit)\n>>> +\t\twarning(_(\"use --reapply-cherry-picks to include skipped commits\"));\n>> I'd be hesitant to endorse doing this kind of \"here is how to use\n>> this command\" unconditionally. Perhaps under --verbose, or hide it\n>> under \"advise.*\".\n>\n> and use advise() rather than warning(). I'm guess this might be\n> helpful but it wont help them get their commits back as there is no\n> way to stop the rebase from dropping them at that point.\n\nYes, but aborting the current rebase and redoing from scratch should\nnot be a brain surgery. Even if you had already resolved conflicts\nin earlier steps, the work will be replayed automatically for you by\nthe rerere mechanism.\n"},{"id":"432405","messageId":"496da0b17476011b4ef4dde31593afc7572246eb.1628623058.git.steadmon@google.com","threadId":"56216","inReplyTo":"4d83766ab3425a5f4b361df2ac505d07fefd7899.1628109852.git.steadmon@google.com","subject":"[PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2021-08-10T19:20:38Z","receivedAt":"2021-08-10T19:20:43Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Silently skipping commits when rebasing with --no-reapply-cherry-picks\n(currently the default behavior) can cause user confusion. Issue advice\nin this case so that users are aware of what's happening.\n\nSigned-off-by: Josh Steadmon \n---\nChanges in V2:\n* use advise_if_enabled() instead of warning()\n* s/seen/applied/ in the advice text\n\n Documentation/config/advice.txt | 3 +++\n advice.c | 3 +++\n advice.h | 1 +\n sequencer.c | 22 ++++++++++++++++++++--\n 4 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 8b2849ff7b..063eec2511 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -44,6 +44,9 @@ advice.*::\n \t\tShown when linkgit:git-push[1] rejects a forced update of\n \t\ta branch when its remote-tracking ref has updates that we\n \t\tdo not have locally.\n+\tskippedCherryPicks::\n+\t\tShown when linkgit:git-rebase[1] skips a commit that has already\n+\t\tbeen cherry-picked onto the upstream branch.\n \tstatusAheadBehind::\n \t\tShown when linkgit:git-status[1] computes the ahead/behind\n \t\tcounts for a local ref compared to its remote tracking ref,\ndiff --git a/advice.c b/advice.c\nindex 0b9c89c48a..7768319b4a 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -34,6 +34,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;\n int advice_submodule_alternate_error_strategy_die = 1;\n int advice_add_ignored_file = 1;\n int advice_add_empty_pathspec = 1;\n+int advice_skipped_cherry_picks = 1;\n \n static int advice_use_color = -1;\n static char advice_colors[][COLOR_MAXLEN] = {\n@@ -96,6 +97,7 @@ static struct {\n \t{ \"submoduleAlternateErrorStrategyDie\", &advice_submodule_alternate_error_strategy_die },\n \t{ \"addIgnoredFile\", &advice_add_ignored_file },\n \t{ \"addEmptyPathspec\", &advice_add_empty_pathspec },\n+\t{ \"skippedCherryPicks\", &advice_skipped_cherry_picks },\n \n \t/* make this an alias for backward compatibility */\n \t{ \"pushNonFastForward\", &advice_push_update_rejected }\n@@ -139,6 +141,7 @@ static struct {\n \t[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { \"submoduleAlternateErrorStrategyDie\", 1 },\n \t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\", 1 },\n \t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\", 1 },\n+\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1},\n };\n \n static const char turn_off_instructions[] =\ndiff --git a/advice.h b/advice.h\nindex 9f8ffc7354..d705bf164c 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -75,6 +75,7 @@ extern int advice_add_empty_pathspec;\n \tADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,\n \tADVICE_UPDATE_SPARSE_PATH,\n \tADVICE_WAITING_FOR_EDITOR,\n+\tADVICE_SKIPPED_CHERRY_PICKS,\n };\n \n int git_default_advice_config(const char *var, const char *value);\ndiff --git a/sequencer.c b/sequencer.c\nindex 7f07cd00f3..1235f61c9d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n \tint rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;\n \tint root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;\n+\tint skipped_commit = 0;\n \tstruct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;\n \tstruct strbuf label = STRBUF_INIT;\n \tstruct commit_list *commits = NULL, **tail = &commits, *iter;\n@@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\toidset_insert(&interesting, &commit->object.oid);\n \n \t\tis_empty = is_original_commit_empty(commit);\n-\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n+\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n+\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n+\t\t\t\t\t_(\"skipped previously applied commit %s\"),\n+\t\t\t\t\tshort_commit_name(commit));\n+\t\t\tskipped_commit = 1;\n \t\t\tcontinue;\n+\t\t}\n \t\tif (is_empty && !keep_empty)\n \t\t\tcontinue;\n \n@@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n \t\toidmap_put(&commit2todo, entry);\n \t}\n+\tif (skipped_commit)\n+\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n+\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n \n \t/*\n \t * Second phase:\n@@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n \tint reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;\n+\tint skipped_commit = 0;\n \n \trepo_init_revisions(r, &revs, NULL);\n \trevs.verbose_header = 1;\n@@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \twhile ((commit = get_revision(&revs))) {\n \t\tint is_empty = is_original_commit_empty(commit);\n \n-\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n+\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n+\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n+\t\t\t\t\t _(\"skipped previously applied commit %s\"),\n+\t\t\t\t\t short_commit_name(commit));\n+\t\t\tskipped_commit = 1;\n \t\t\tcontinue;\n+\t\t}\n \t\tif (is_empty && !keep_empty)\n \t\t\tcontinue;\n \t\tstrbuf_addf(out, \"%s %s \", insn,\n@@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \t\t\tstrbuf_addf(out, \" %c empty\", comment_line_char);\n \t\tstrbuf_addch(out, '\\n');\n \t}\n+\tif (skipped_commit)\n+\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n+\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n \treturn 0;\n }\n \n-- \n2.32.0.605.g8dce9f2422-goog\n\n"},{"id":"432424","messageId":"xmqqwnotaq0e.fsf@gitster.g","threadId":"56216","inReplyTo":"496da0b17476011b4ef4dde31593afc7572246eb.1628623058.git.steadmon@google.com","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-10T22:33:53Z","receivedAt":"2021-08-10T22:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> +\tskippedCherryPicks::\n> +\t\tShown when linkgit:git-rebase[1] skips a commit that has already\n> +\t\tbeen cherry-picked onto the upstream branch.\n\nMakes sense.\n\n> +\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1},\n\nNo need to resend to only fix this, but I'll add SP after \"1\" to\nmatch surrounding lines while queuing the patch.\n\n> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \t\toidset_insert(&interesting, &commit->object.oid);\n> \n> \t\tis_empty = is_original_commit_empty(commit);\n> -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t\t_(\"skipped previously applied commit %s\"),\n> +\t\t\t\t\tshort_commit_name(commit));\n> +\t\t\tskipped_commit = 1;\n> \t\t\tcontinue;\n> +\t\t}\n> \t\tif (is_empty && !keep_empty)\n> \t\t\tcontinue;\n> \n> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n> \t\toidmap_put(&commit2todo, entry);\n> \t}\n> +\tif (skipped_commit)\n> +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n\nI agree with the change in this hunk that advanced users may want to\nsquelch this \"what to do\" hint.\n\nI am not sure about the earlier hunk that reports when some commits\nhave actually been skipped. When --no-reapply-cherry-picks is in\neffect, the user is expecting that some commits are cherry-picks\namong other (hopefully the majority of) commits, and even those\nusers who do not want to be taught how to use the command would want\nto learn the fact that some commits were skipped (and which ones).\n\nUsing two separate advice configuration variables feel way overkill\nfor this. I wonder if the previous hunk should use warning(), i.e.\n\n> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> +\t\t\twarning(_(\"skipped previously applied commit %s\"),\n> +\t\t\t\tshort_commit_name(commit));\n> +\t\t\tskipped_commit = 1;\n> \t\t\tcontinue;\n> +\t\t}\n\npossibly squelched by \"git rebase --quiet\".\n\n> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> \twhile ((commit = get_revision(&revs))) {\n> \t\tint is_empty = is_original_commit_empty(commit);\n> \n> -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t\t _(\"skipped previously applied commit %s\"),\n> +\t\t\t\t\t short_commit_name(commit));\n> +\t\t\tskipped_commit = 1;\n> \t\t\tcontinue;\n> +\t\t}\n\nLikewise. I wonder why we have two so-much-similar codepaths that\nwe need to touch, though.\n\n> \t\tif (is_empty && !keep_empty)\n> \t\t\tcontinue;\n> \t\tstrbuf_addf(out, \"%s %s \", insn,\n> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> \t\t\tstrbuf_addf(out, \" %c empty\", comment_line_char);\n> \t\tstrbuf_addch(out, '\\n');\n> \t}\n> +\tif (skipped_commit)\n> +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n> \treturn 0;\n> }\n"},{"id":"432588","messageId":"c185a396-c498-b2ef-1c86-cec7d5751f3d@gmail.com","threadId":"56216","inReplyTo":"496da0b17476011b4ef4dde31593afc7572246eb.1628623058.git.steadmon@google.com","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-08-12T17:45:58Z","receivedAt":"2021-08-12T17:46:04Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Josh,\n\nLe 2021-08-10 à 15:20, Josh Steadmon a écrit :\n> Silently skipping commits when rebasing with --no-reapply-cherry-picks\n> (currently the default behavior) can cause user confusion. Issue advice\n> in this case so that users are aware of what's happening.\n\nI think this is an excellent idea. It can be very surprising, especially\nfor 'git rebase' beginners/intermediate users who might not have read the\nman page.\n\nSince your proposed changes are in sequencer.c, this will only affect\nthe default \"merge\" rebase backend, and not the older 'apply' backend. I think\nit might be worth mentioning this in the commit message.\n\nNote that it might be considerably more work to also add the warning\nfor the 'apply' backend, since rebase.c::run_am generates the patches\nusing 'git format-patch --cherry-pick --right-only $upstream..HEAD' and\nso cherry-picks are dropped early in the process. I think that this not that big\nof a deal since the default backend is now \"merge\".\n\n> \n> Signed-off-by: Josh Steadmon \n> ---\n> Changes in V2:\n> * use advise_if_enabled() instead of warning()\n> * s/seen/applied/ in the advice text\n> \n> Documentation/config/advice.txt | 3 +++\n> advice.c | 3 +++\n> advice.h | 1 +\n> sequencer.c | 22 ++++++++++++++++++++--\n> 4 files changed, 27 insertions(+), 2 deletions(-)\n\nI would suggest mentioning the new behaviour and the new\nadvice.skippedCherryPicks config in git-rebase.txt, say in the paragraph\nstarting with \"If the upstream branch already contains\" in the Description section\nand in the description of '--reapply-cherry-picks'.\n\n> int git_default_advice_config(const char *var, const char *value);\n> diff --git a/sequencer.c b/sequencer.c\n> index 7f07cd00f3..1235f61c9d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n> \tint rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;\n> \tint root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;\n> +\tint skipped_commit = 0;\n> \tstruct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;\n> \tstruct strbuf label = STRBUF_INIT;\n> \tstruct commit_list *commits = NULL, **tail = &commits, *iter;\n> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \t\toidset_insert(&interesting, &commit->object.oid);\n> \n> \t\tis_empty = is_original_commit_empty(commit);\n> -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t\t_(\"skipped previously applied commit %s\"),\n> +\t\t\t\t\tshort_commit_name(commit));\n> +\t\t\tskipped_commit = 1;\n> \t\t\tcontinue;\n> +\t\t}\n> \t\tif (is_empty && !keep_empty)\n> \t\t\tcontinue;\n\nFor interactive rebase, an alternate implementation, that I suggested in [1] last summer, would be to keep\nthe cherry-picks in the todo list, but mark them as 'drop' and add a comment at the\nend of their line, like '# already applied' or something like this, similar\nto how empty commits have '# empty' appended. I think that for interactive rebase, I\nwould prefer this, since it is easier for the user to notice it and change the 'drop'\nto 'pick' right away if they realise they do not want to drop those commits (easier\nthan seeing the warning, realising they did not want to drop them, aborting the rebase\nand redoing it with '--reapply-cherry-picks').\n\nFor non-interactive rebase adding a warning/advice like your patch does seems to\nbe a good solution.\n\n> \n> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n> \t\toidmap_put(&commit2todo, entry);\n> \t}\n> +\tif (skipped_commit)\n> +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n> \n> \t/*\n> \t * Second phase:\n> @@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n> \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n> \tint reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;\n> +\tint skipped_commit = 0;\n> \n> \trepo_init_revisions(r, &revs, NULL);\n> \trevs.verbose_header = 1;\n> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> \twhile ((commit = get_revision(&revs))) {\n> \t\tint is_empty = is_original_commit_empty(commit);\n> \n> -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t\t _(\"skipped previously applied commit %s\"),\n> +\t\t\t\t\t short_commit_name(commit));\n> +\t\t\tskipped_commit = 1;\n> \t\t\tcontinue;\n> +\t\t}\n> \t\tif (is_empty && !keep_empty)\n> \t\t\tcontinue;\n> \t\tstrbuf_addf(out, \"%s %s \", insn,\n> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> \t\t\tstrbuf_addf(out, \" %c empty\", comment_line_char);\n> \t\tstrbuf_addch(out, '\\n');\n> \t}\n> +\tif (skipped_commit)\n> +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n> \treturn 0;\n> }\n> \n> \n\nLike Junio remarked, it is a little unfortunate that some logic is duplicated between\n'sequencer_make_script' and 'make_script_with_merges', such that your patch has to do\nthe same thing at two different code locations. Maybe a preparatory cleanup could add\na new function that takes care of the duplicated logic and call if from both ? I'm\njust thinking out loud here, I did not analyze in details if this would be easy/feasible...\n\nThanks for suggesting this change,\n\nPhilippe.\n\n\n[1] https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/\n"},{"id":"432592","messageId":"xmqqtuju328n.fsf@gitster.g","threadId":"56216","inReplyTo":"c185a396-c498-b2ef-1c86-cec7d5751f3d@gmail.com","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-12T19:13:44Z","receivedAt":"2021-08-12T19:13:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n> Since your proposed changes are in sequencer.c, this will only affect\n> the default \"merge\" rebase backend, and not the older 'apply' backend. I think\n> it might be worth mentioning this in the commit message.\n\nIt shouldn't be too hard to teach \"format-patch --cherry-pick\" to\nreport when commits are actually dropped. There isn't necessarily a\nneed to give help to users what to do to apply the same change\ntwice, but even for those who _know_ how to do so, they need to know\nif commits are dropped in the first place. And such a notice is useful\nfor any user of the --cherry-pick feature of format-patch, not just\nas an implementation detail of \"git rebase\".\n\n\n\n"},{"id":"433056","messageId":"e363df27-a99e-1a43-f493-ed90de7b6363@gmail.com","threadId":"56216","inReplyTo":"c185a396-c498-b2ef-1c86-cec7d5751f3d@gmail.com","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-18T10:02:12Z","receivedAt":"2021-08-18T10:02:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/08/2021 18:45, Philippe Blain wrote:\n> Hi Josh,\n> [...]\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 7f07cd00f3..1235f61c9d 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct \n>> pretty_print_context *pp,\n>>       int keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n>>       int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;\n>>       int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;\n>> +    int skipped_commit = 0;\n>>       struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;\n>>       struct strbuf label = STRBUF_INIT;\n>>       struct commit_list *commits = NULL, **tail = &commits, *iter;\n>> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct \n>> pretty_print_context *pp,\n>>           oidset_insert(&interesting, &commit->object.oid);\n>>           is_empty = is_original_commit_empty(commit);\n>> -        if (!is_empty && (commit->object.flags & PATCHSAME))\n>> +        if (!is_empty && (commit->object.flags & PATCHSAME)) {\n>> +            advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>> +                    _(\"skipped previously applied commit %s\"),\n>> +                    short_commit_name(commit));\n>> +            skipped_commit = 1;\n>>               continue;\n>> +        }\n>>           if (is_empty && !keep_empty)\n>>               continue;\n> \n> For interactive rebase, an alternate implementation, that I suggested in \n> [1] last summer, would be to keep\n> the cherry-picks in the todo list, but mark them as 'drop' and add a \n> comment at the\n> end of their line, like '# already applied' or something like this, similar\n> to how empty commits have '# empty' appended. I think that for \n> interactive rebase, I\n> would prefer this, since it is easier for the user to notice it and \n> change the 'drop'\n> to 'pick' right away if they realise they do not want to drop those \n> commits (easier\n> than seeing the warning, realising they did not want to drop them, \n> aborting the rebase\n> and redoing it with '--reapply-cherry-picks').\n\nThat would be nice, but we could always add it in the future if Josh \ndoes not want to implement it now. At the moment the function that \ncreates the todo list does not know if it is going to be edited, I'm not \nsure how easy it would be to pass that information down.\n\n> For non-interactive rebase adding a warning/advice like your patch does \n> seems to\n> be a good solution.\n> \n>> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct \n>> pretty_print_context *pp,\n>>           oidcpy(&entry->entry.oid, &commit->object.oid);\n>>           oidmap_put(&commit2todo, entry);\n>>       }\n>> +    if (skipped_commit)\n>> +        advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>> +                  _(\"use --reapply-cherry-picks to include skipped \n>> commits\"));\n>>       /*\n>>        * Second phase:\n>> @@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, \n>> struct strbuf *out, int argc,\n>>       const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \n>> \"pick\";\n>>       int rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n>>       int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;\n>> +    int skipped_commit = 0;\n>>       repo_init_revisions(r, &revs, NULL);\n>>       revs.verbose_header = 1;\n>> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, \n>> struct strbuf *out, int argc,\n>>       while ((commit = get_revision(&revs))) {\n>>           int is_empty = is_original_commit_empty(commit);\n>> -        if (!is_empty && (commit->object.flags & PATCHSAME))\n>> +        if (!is_empty && (commit->object.flags & PATCHSAME)) {\n>> +            advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>> +                      _(\"skipped previously applied commit %s\"),\n>> +                      short_commit_name(commit));\n>> +            skipped_commit = 1;\n>>               continue;\n>> +        }\n>>           if (is_empty && !keep_empty)\n>>               continue;\n>>           strbuf_addf(out, \"%s %s \", insn,\n>> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, \n>> struct strbuf *out, int argc,\n>>               strbuf_addf(out, \" %c empty\", comment_line_char);\n>>           strbuf_addch(out, '\\n');\n>>       }\n>> +    if (skipped_commit)\n>> +        advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>> +                  _(\"use --reapply-cherry-picks to include skipped \n>> commits\"));\n>>       return 0;\n>>   }\n>>\n> \n> Like Junio remarked, it is a little unfortunate that some logic is \n> duplicated between\n> 'sequencer_make_script' and 'make_script_with_merges', such that your \n> patch has to do\n> the same thing at two different code locations. Maybe a preparatory \n> cleanup could add\n> a new function that takes care of the duplicated logic and call if from \n> both ? I'm\n> just thinking out loud here, I did not analyze in details if this would \n> be easy/feasible...\n\nI think feasible but not easy (or required for this change), it would \nalso complicate the code in a different way as I think we'd have to add \nsome conditionals for whether we are recreating merges or not.\n\nBest Wishes\n\nPhillip\n\n\n> Thanks for suggesting this change,\n> \n> Philippe.\n> \n> \n> [1] \n> https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/\n\n"},{"id":"433057","messageId":"2973f162-d2ab-12da-49ec-82ef823089f6@gmail.com","threadId":"56216","inReplyTo":"xmqqwnotaq0e.fsf@gitster.g","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-18T10:08:58Z","receivedAt":"2021-08-18T10:09:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/08/2021 23:33, Junio C Hamano wrote:\n> Josh Steadmon writes:\n>> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n>> \t\toidset_insert(&interesting, &commit->object.oid);\n>> \n>> \t\tis_empty = is_original_commit_empty(commit);\n>> -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n>> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n>> +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>> +\t\t\t\t\t_(\"skipped previously applied commit %s\"),\n>> +\t\t\t\t\tshort_commit_name(commit));\n>> +\t\t\tskipped_commit = 1;\n>> \t\t\tcontinue;\n>> +\t\t}\n>> \t\tif (is_empty && !keep_empty)\n>> \t\t\tcontinue;\n>> \n>> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n>> \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n>> \t\toidmap_put(&commit2todo, entry);\n>> \t}\n>> +\tif (skipped_commit)\n>> +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>> +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n> \n> I agree with the change in this hunk that advanced users may want to\n> squelch this \"what to do\" hint.\n> \n> I am not sure about the earlier hunk that reports when some commits\n> have actually been skipped. When --no-reapply-cherry-picks is in\n> effect, the user is expecting that some commits are cherry-picks\n> among other (hopefully the majority of) commits, and even those\n> users who do not want to be taught how to use the command would want\n> to learn the fact that some commits were skipped (and which ones).\n> \n> Using two separate advice configuration variables feel way overkill\n> for this. I wonder if the previous hunk should use warning(), i.e.\n> \n>> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n>> +\t\t\twarning(_(\"skipped previously applied commit %s\"),\n>> +\t\t\t\tshort_commit_name(commit));\n>> +\t\t\tskipped_commit = 1;\n>> \t\t\tcontinue;\n>> +\t\t}\n> \n> possibly squelched by \"git rebase --quiet\".\n\nI think that would be nicer. Using advise_if_enabled() in the second \nhunk makes sense but I think a printing \"warning\" rather than a \"hint\" \nin the first hunk.\n\n>> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n>> \twhile ((commit = get_revision(&revs))) {\n>> \t\tint is_empty = is_original_commit_empty(commit);\n>> \n>> -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n>> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n>> +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>> +\t\t\t\t\t _(\"skipped previously applied commit %s\"),\n>> +\t\t\t\t\t short_commit_name(commit));\n>> +\t\t\tskipped_commit = 1;\n>> \t\t\tcontinue;\n>> +\t\t}\n> \n> Likewise. I wonder why we have two so-much-similar codepaths that\n> we need to touch, though.\n\n--recreate-merges implemented the main loop for its todo list generation \nin a new function as it is much more involved that for the linear case.\n\nBest Wishes\n\nPhillip\n\n> \n>> \t\tif (is_empty && !keep_empty)\n>> \t\t\tcontinue;\n>> \t\tstrbuf_addf(out, \"%s %s \", insn,\n>> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n>> \t\t\tstrbuf_addf(out, \" %c empty\", comment_line_char);\n>> \t\tstrbuf_addch(out, '\\n');\n>> \t}\n>> +\tif (skipped_commit)\n>> +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>> +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n>> \treturn 0;\n>> }\n\n"},{"id":"433093","messageId":"cc044079-c40d-ab79-8afa-8a1dfa66f279@gmail.com","threadId":"56216","inReplyTo":"e363df27-a99e-1a43-f493-ed90de7b6363@gmail.com","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-08-18T22:45:43Z","receivedAt":"2021-08-18T22:45:48Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Phillip,\n\nLe 2021-08-18 à 06:02, Phillip Wood a écrit :\n> On 12/08/2021 18:45, Philippe Blain wrote:\n>> Hi Josh,\n>> [...]\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index 7f07cd00f3..1235f61c9d 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n>>>       int keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n>>>       int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;\n>>>       int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;\n>>> +    int skipped_commit = 0;\n>>>       struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;\n>>>       struct strbuf label = STRBUF_INIT;\n>>>       struct commit_list *commits = NULL, **tail = &commits, *iter;\n>>> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n>>>           oidset_insert(&interesting, &commit->object.oid);\n>>>           is_empty = is_original_commit_empty(commit);\n>>> -        if (!is_empty && (commit->object.flags & PATCHSAME))\n>>> +        if (!is_empty && (commit->object.flags & PATCHSAME)) {\n>>> +            advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n>>> +                    _(\"skipped previously applied commit %s\"),\n>>> +                    short_commit_name(commit));\n>>> +            skipped_commit = 1;\n>>>               continue;\n>>> +        }\n>>>           if (is_empty && !keep_empty)\n>>>               continue;\n>>\n>> For interactive rebase, an alternate implementation, that I suggested in [1] last summer, would be to keep\n>> the cherry-picks in the todo list, but mark them as 'drop' and add a comment at the\n>> end of their line, like '# already applied' or something like this, similar\n>> to how empty commits have '# empty' appended. I think that for interactive rebase, I\n>> would prefer this, since it is easier for the user to notice it and change the 'drop'\n>> to 'pick' right away if they realise they do not want to drop those commits (easier\n>> than seeing the warning, realising they did not want to drop them, aborting the rebase\n>> and redoing it with '--reapply-cherry-picks').\n> \n> 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.\n\nWell, I'm not sure we need to ? If we change the 'pick' to 'drop', instead of\nnot writing these lines to the todo list, the cherry-picked commits will get dropped\neither way, no? Unless I'm missing something - I think you are way more well-versed in\nthe sequencer code than me :)\n\n>>\n>> Like Junio remarked, it is a little unfortunate that some logic is duplicated between\n>> 'sequencer_make_script' and 'make_script_with_merges', such that your patch has to do\n>> the same thing at two different code locations. Maybe a preparatory cleanup could add\n>> a new function that takes care of the duplicated logic and call if from both ? I'm\n>> just thinking out loud here, I did not analyze in details if this would be easy/feasible...\n> \n> 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.\n\nYes, I agree it's not required for this change.\n\nCheers,\n\nPhilippe.\n"},{"id":"433123","messageId":"b1a1b548-4fe2-9043-a19c-db9c04be7bb7@gmail.com","threadId":"56216","inReplyTo":"cc044079-c40d-ab79-8afa-8a1dfa66f279@gmail.com","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-19T10:04:00Z","receivedAt":"2021-08-19T10:04:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Philippe\nOn 18/08/2021 23:45, Philippe Blain wrote:\n> Hi Phillip,\n> [...]\n>>> For interactive rebase, an alternate implementation, that I suggested \n>>> in [1] last summer, would be to keep\n>>> the cherry-picks in the todo list, but mark them as 'drop' and add a \n>>> comment at the\n>>> end of their line, like '# already applied' or something like this, \n>>> similar\n>>> to how empty commits have '# empty' appended. I think that for \n>>> interactive rebase, I\n>>> would prefer this, since it is easier for the user to notice it and \n>>> change the 'drop'\n>>> to 'pick' right away if they realise they do not want to drop those \n>>> commits (easier\n>>> than seeing the warning, realising they did not want to drop them, \n>>> aborting the rebase\n>>> and redoing it with '--reapply-cherry-picks').\n>>\n>> That would be nice, but we could always add it in the future if Josh \n>> does not want to implement it now. At the moment the function that \n>> creates the todo list does not know if it is going to be edited, I'm \n>> not sure how easy it would be to pass that information down.\n> \n> Well, I'm not sure we need to ? If we change the 'pick' to 'drop', \n> instead of\n> not writing these lines to the todo list, the cherry-picked commits will \n> get dropped\n> either way, no? Unless I'm missing something - I think you are way more \n> well-versed in\n> the sequencer code than me :)\n\nI think I read your suggestion as meaning we would not print the warning \nif the user was going to edit the list - hence the need for the \ndistinction. I've just had a closer look at the code and I think it \nwould be fairly easy to tell sequencer_make_script() if the todo list is \ngoing to be edited, there is only one caller in builtin/rebase.c which \ncould easily pass a flag for this. Thinking about the 'print warning' vs \n'add drop command' issue if the user is using a terminal based editor \nsuch as vim or nano then they will barely have time to see the warnings \nbeing printed let alone read them before the editor is opened and hides \nthem so having some indication in the todo list would probably be a good \nidea.\n\nBest Wishes\n\nPhillip\n\n>>>\n>>> Like Junio remarked, it is a little unfortunate that some logic is \n>>> duplicated between\n>>> 'sequencer_make_script' and 'make_script_with_merges', such that your \n>>> patch has to do\n>>> the same thing at two different code locations. Maybe a preparatory \n>>> cleanup could add\n>>> a new function that takes care of the duplicated logic and call if \n>>> from both ? I'm\n>>> just thinking out loud here, I did not analyze in details if this \n>>> would be easy/feasible...\n>>\n>> I think feasible but not easy (or required for this change), it would \n>> also complicate the code in a different way as I think we'd have to \n>> add some conditionals for whether we are recreating merges or not.\n> \n> Yes, I agree it's not required for this change.\n> \n> Cheers,\n> \n> Philippe.\n\n"},{"id":"433764","messageId":"xmqqfsuxia87.fsf@gitster.g","threadId":"56216","inReplyTo":"496da0b17476011b4ef4dde31593afc7572246eb.1628623058.git.steadmon@google.com","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-25T19:40:40Z","receivedAt":"2021-08-25T19:40:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Silently skipping commits when rebasing with --no-reapply-cherry-picks\n> (currently the default behavior) can cause user confusion. Issue advice\n> in this case so that users are aware of what's happening.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> Changes in V2:\n> * use advise_if_enabled() instead of warning()\n> * s/seen/applied/ in the advice text\n>\n> Documentation/config/advice.txt | 3 +++\n> advice.c | 3 +++\n> advice.h | 1 +\n> sequencer.c | 22 ++++++++++++++++++++--\n> 4 files changed, 27 insertions(+), 2 deletions(-)\n\n[jc: no need for immediate action from Josh]\n\nWith \nit would make more sense to stop using the \"global variable\nregistered with advice_config[] array\" part of the patch, especially\nbecause we only use advise_if_enabled() already in this patch.\n\n> +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> +\t\t\t\t\t _(\"skipped previously applied commit %s\"),\n> +\t\t\t\t\t short_commit_name(commit));\n> +\t\t\tskipped_commit = 1;\n> \t\t\tcontinue;\n> +\t\t}\n\n> @@ -139,6 +141,7 @@ static struct {\n> \t[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { \"submoduleAlternateErrorStrategyDie\", 1 },\n> \t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\", 1 },\n> \t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\", 1 },\n> +\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1},\n> };\n\nThis needs to be moved to keep the list sorted.\n\nWhen I queued the topic to migrate to advise-settings from\nadvice-config, I made some adjustment for this topic (and luckily\nthere is no other topic in flight that touch advice.c). Please\nsanity check the result when today's integration result is pushed\nout, perhaps in 5 hours or so.\n\nThanks.\n"},{"id":"434176","messageId":"YS1La6YYAnJk4Sg8@google.com","threadId":"56216","inReplyTo":"xmqqwnotaq0e.fsf@gitster.g","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2021-08-30T21:19:39Z","receivedAt":"2021-08-30T21:19:48Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2021.08.10 15:33, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > +\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1},\n> \n> No need to resend to only fix this, but I'll add SP after \"1\" to\n> match surrounding lines while queuing the patch.\n\nAck. Fixed in V3. Do you also want me to rebase this on\nab/retire-advice-config?\n\n> > @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> > \t\toidset_insert(&interesting, &commit->object.oid);\n> > \n> > \t\tis_empty = is_original_commit_empty(commit);\n> > -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> > +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> > +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> > +\t\t\t\t\t_(\"skipped previously applied commit %s\"),\n> > +\t\t\t\t\tshort_commit_name(commit));\n> > +\t\t\tskipped_commit = 1;\n> > \t\t\tcontinue;\n> > +\t\t}\n> > \t\tif (is_empty && !keep_empty)\n> > \t\t\tcontinue;\n> > \n> > @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> > \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n> > \t\toidmap_put(&commit2todo, entry);\n> > \t}\n> > +\tif (skipped_commit)\n> > +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> > +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n> \n> I agree with the change in this hunk that advanced users may want to\n> squelch this \"what to do\" hint.\n> \n> I am not sure about the earlier hunk that reports when some commits\n> have actually been skipped. When --no-reapply-cherry-picks is in\n> effect, the user is expecting that some commits are cherry-picks\n> among other (hopefully the majority of) commits, and even those\n> users who do not want to be taught how to use the command would want\n> to learn the fact that some commits were skipped (and which ones).\n> \n> Using two separate advice configuration variables feel way overkill\n> for this. I wonder if the previous hunk should use warning(), i.e.\n\nSwitched these to warnings, squelched by \"--quiet\" as suggested below.\n\n\n> > +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> > +\t\t\twarning(_(\"skipped previously applied commit %s\"),\n> > +\t\t\t\tshort_commit_name(commit));\n> > +\t\t\tskipped_commit = 1;\n> > \t\t\tcontinue;\n> > +\t\t}\n> \n> possibly squelched by \"git rebase --quiet\".\n> \n"},{"id":"434177","messageId":"YS1LyjjOgER2R1fv@google.com","threadId":"56216","inReplyTo":"c185a396-c498-b2ef-1c86-cec7d5751f3d@gmail.com","subject":"Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2021-08-30T21:21:14Z","receivedAt":"2021-08-30T21:21:23Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2021.08.12 13:45, Philippe Blain wrote:\n> Hi Josh,\n> \n> Le 2021-08-10 à 15:20, Josh Steadmon a écrit :\n> > Silently skipping commits when rebasing with --no-reapply-cherry-picks\n> > (currently the default behavior) can cause user confusion. Issue advice\n> > in this case so that users are aware of what's happening.\n> \n> I think this is an excellent idea. It can be very surprising, especially\n> for 'git rebase' beginners/intermediate users who might not have read the\n> man page.\n> \n> Since your proposed changes are in sequencer.c, this will only affect\n> the default \"merge\" rebase backend, and not the older 'apply' backend. I think\n> it might be worth mentioning this in the commit message.\n> \n> Note that it might be considerably more work to also add the warning\n> for the 'apply' backend, since rebase.c::run_am generates the patches\n> using 'git format-patch --cherry-pick --right-only $upstream..HEAD' and\n> so cherry-picks are dropped early in the process. I think that this not that big\n> of a deal since the default backend is now \"merge\".\n\nAh good point, thank you for the catch. I have noted this in the V3\ncommit message.\n\n> > \n> > Signed-off-by: Josh Steadmon \n> > ---\n> > Changes in V2:\n> > * use advise_if_enabled() instead of warning()\n> > * s/seen/applied/ in the advice text\n> > \n> > Documentation/config/advice.txt | 3 +++\n> > advice.c | 3 +++\n> > advice.h | 1 +\n> > sequencer.c | 22 ++++++++++++++++++++--\n> > 4 files changed, 27 insertions(+), 2 deletions(-)\n> \n> I would suggest mentioning the new behaviour and the new\n> advice.skippedCherryPicks config in git-rebase.txt, say in the paragraph\n> starting with \"If the upstream branch already contains\" in the Description section\n> and in the description of '--reapply-cherry-picks'.\n\nDone in V3.\n\n\n> > int git_default_advice_config(const char *var, const char *value);\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 7f07cd00f3..1235f61c9d 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> > \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n> > \tint rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;\n> > \tint root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;\n> > +\tint skipped_commit = 0;\n> > \tstruct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;\n> > \tstruct strbuf label = STRBUF_INIT;\n> > \tstruct commit_list *commits = NULL, **tail = &commits, *iter;\n> > @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> > \t\toidset_insert(&interesting, &commit->object.oid);\n> > \t\tis_empty = is_original_commit_empty(commit);\n> > -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> > +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> > +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> > +\t\t\t\t\t_(\"skipped previously applied commit %s\"),\n> > +\t\t\t\t\tshort_commit_name(commit));\n> > +\t\t\tskipped_commit = 1;\n> > \t\t\tcontinue;\n> > +\t\t}\n> > \t\tif (is_empty && !keep_empty)\n> > \t\t\tcontinue;\n> \n> For interactive rebase, an alternate implementation, that I suggested in [1] last summer, would be to keep\n> the cherry-picks in the todo list, but mark them as 'drop' and add a comment at the\n> end of their line, like '# already applied' or something like this, similar\n> to how empty commits have '# empty' appended. I think that for interactive rebase, I\n> would prefer this, since it is easier for the user to notice it and change the 'drop'\n> to 'pick' right away if they realise they do not want to drop those commits (easier\n> than seeing the warning, realising they did not want to drop them, aborting the rebase\n> and redoing it with '--reapply-cherry-picks').\n\nI haven't had time to do this in V3, but I can look into it for V4.\n\n> For non-interactive rebase adding a warning/advice like your patch does seems to\n> be a good solution.\n> \n> > @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> > \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n> > \t\toidmap_put(&commit2todo, entry);\n> > \t}\n> > +\tif (skipped_commit)\n> > +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> > +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n> > \t/*\n> > \t * Second phase:\n> > @@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> > \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n> > \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n> > \tint reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;\n> > +\tint skipped_commit = 0;\n> > \trepo_init_revisions(r, &revs, NULL);\n> > \trevs.verbose_header = 1;\n> > @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> > \twhile ((commit = get_revision(&revs))) {\n> > \t\tint is_empty = is_original_commit_empty(commit);\n> > -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> > +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n> > +\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> > +\t\t\t\t\t _(\"skipped previously applied commit %s\"),\n> > +\t\t\t\t\t short_commit_name(commit));\n> > +\t\t\tskipped_commit = 1;\n> > \t\t\tcontinue;\n> > +\t\t}\n> > \t\tif (is_empty && !keep_empty)\n> > \t\t\tcontinue;\n> > \t\tstrbuf_addf(out, \"%s %s \", insn,\n> > @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> > \t\t\tstrbuf_addf(out, \" %c empty\", comment_line_char);\n> > \t\tstrbuf_addch(out, '\\n');\n> > \t}\n> > +\tif (skipped_commit)\n> > +\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n> > +\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n> > \treturn 0;\n> > }\n> > \n> \n> Like Junio remarked, it is a little unfortunate that some logic is duplicated between\n> 'sequencer_make_script' and 'make_script_with_merges', such that your patch has to do\n> the same thing at two different code locations. Maybe a preparatory cleanup could add\n> a new function that takes care of the duplicated logic and call if from both ? I'm\n> just thinking out loud here, I did not analyze in details if this would be easy/feasible...\n\nWill look into this for V4 as well.\n\n> Thanks for suggesting this change,\n> \n> Philippe.\n> \n> \n> [1] https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/\n"},{"id":"434197","messageId":"691c660422676c92180feb1c2a88049afe7536b9.1630359271.git.steadmon@google.com","threadId":"56216","inReplyTo":"4d83766ab3425a5f4b361df2ac505d07fefd7899.1628109852.git.steadmon@google.com","subject":"[PATCH v3] sequencer: advise if skipping cherry-picked commit","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2021-08-30T21:46:02Z","receivedAt":"2021-08-30T21:46:07Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Silently skipping commits when rebasing with --no-reapply-cherry-picks\n(currently the default behavior) can cause user confusion. Issue\nwarnings when this happens, as well as advice on how to preserve the\nskipped commits.\n\nThese warnings and advice are displayed only when using the (default)\n\"merge\" rebase backend.\n\nUpdate the git-rebase docs to mention the warnings and advice.\n\nSigned-off-by: Josh Steadmon \n---\nI dealt with the smaller suggestions from the last review round, but I\nhope to eventually send out a V4 that also includes some of the larger\nchanges, such as extending this to git-format-patch. But I don't expect\nto have time for that in the near future, so I figured it's better to\nget V3 out in case we are OK with merging without those additional\nfeatures.\n\nJunio, please let me know if you'd like me to rebase this on top of\nab/retire-advice-config.\n\nChanges in V3:\n* Switch back to warning() for individual skipped commits\n* Suppress warnings when `--quiet` is given\n* Fixed formatting of advice_setting entries in advice.c\n* Mention the advice / warnings in git-rebase doc\n\nChanges in V2:\n* use advise_if_enabled() instead of warning()\n* s/seen/applied/ in the advice text\n\nRange-diff against v2:\n1: 496da0b174 ! 1: 691c660422 sequencer: advise if skipping cherry-picked commit\n @@ Commit message\n sequencer: advise if skipping cherry-picked commit\n \n Silently skipping commits when rebasing with --no-reapply-cherry-picks\n - (currently the default behavior) can cause user confusion. Issue advice\n - in this case so that users are aware of what's happening.\n + (currently the default behavior) can cause user confusion. Issue\n + warnings when this happens, as well as advice on how to preserve the\n + skipped commits.\n +\n + These warnings and advice are displayed only when using the (default)\n + \"merge\" rebase backend.\n +\n + Update the git-rebase docs to mention the warnings and advice.\n \n \n @@ Documentation/config/advice.txt: advice.*::\n \t\tShown when linkgit:git-status[1] computes the ahead/behind\n \t\tcounts for a local ref compared to its remote tracking ref,\n \n + ## Documentation/git-rebase.txt ##\n +@@ Documentation/git-rebase.txt: remain the checked-out branch.\n + \n + If the upstream branch already contains a change you have made (e.g.,\n + because you mailed a patch which was applied upstream), then that commit\n +-will be skipped. For example, running `git rebase master` on the\n +-following history (in which `A'` and `A` introduce the same set of changes,\n +-but have different committer information):\n ++will be skipped and warnings will be issued (if the `merge` backend is\n ++used). For example, running `git rebase master` on the following\n ++history (in which `A'` and `A` introduce the same set of changes, but\n ++have different committer information):\n + \n + ------------\n + A---B---C topic\n +@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.\n + By default (or if `--no-reapply-cherry-picks` is given), these commits\n + will be automatically dropped. Because this necessitates reading all\n + upstream commits, this can be expensive in repos with a large number\n +-of upstream commits that need to be read.\n ++of upstream commits that need to be read. When using the `merge`\n ++backend, warnings will be issued for each dropped commit (unless\n ++`--quiet` is given). Advice will also be issued unless\n ++`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).\n + +\n + `--reapply-cherry-picks` allows rebase to forgo reading all upstream\n + commits, potentially improving performance.\n +\n ## advice.c ##\n @@ advice.c: int advice_checkout_ambiguous_remote_branch_name = 1;\n int advice_submodule_alternate_error_strategy_die = 1;\n @@ advice.c: static struct {\n \t/* make this an alias for backward compatibility */\n \t{ \"pushNonFastForward\", &advice_push_update_rejected }\n @@ advice.c: static struct {\n - \t[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { \"submoduleAlternateErrorStrategyDie\", 1 },\n - \t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\", 1 },\n - \t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\", 1 },\n -+\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1},\n - };\n - \n - static const char turn_off_instructions[] =\n + \t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\", 1 },\n + \t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\", 1 },\n + \t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\", 1 },\n ++\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1 },\n + \t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\", 1 },\n + \t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\", 1 },\n + \t[ADVICE_STATUS_U_OPTION]\t\t\t= { \"statusUoption\", 1 },\n \n ## advice.h ##\n @@ advice.h: extern int advice_add_empty_pathspec;\n @@ advice.h: extern int advice_add_empty_pathspec;\n \n int git_default_advice_config(const char *var, const char *value);\n \n + ## builtin/rebase.c ##\n +@@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts,\n + \tflags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;\n + \tflags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;\n + \tflags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;\n ++\tflags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;\n + \n + \tswitch (command) {\n + \tcase ACTION_NONE: {\n +\n ## sequencer.c ##\n @@ sequencer.c: static int make_script_with_merges(struct pretty_print_context *pp,\n \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n @@ sequencer.c: static int make_script_with_merges(struct pretty_print_context *pp,\n \t\tis_empty = is_original_commit_empty(commit);\n -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n -+\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n -+\t\t\t\t\t_(\"skipped previously applied commit %s\"),\n ++\t\t\tif (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)\n ++\t\t\t\twarning(_(\"skipped previously applied commit %s\"),\n +\t\t\t\t\tshort_commit_name(commit));\n +\t\t\tskipped_commit = 1;\n \t\t\tcontinue;\n @@ sequencer.c: int sequencer_make_script(struct repository *r, struct strbuf *out,\n \n -\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n +\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n -+\t\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n -+\t\t\t\t\t _(\"skipped previously applied commit %s\"),\n -+\t\t\t\t\t short_commit_name(commit));\n ++\t\t\tif (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)\n ++\t\t\t\twarning(_(\"skipped previously applied commit %s\"),\n ++\t\t\t\t\tshort_commit_name(commit));\n +\t\t\tskipped_commit = 1;\n \t\t\tcontinue;\n +\t\t}\n @@ sequencer.c: int sequencer_make_script(struct repository *r, struct strbuf *out,\n \treturn 0;\n }\n \n +\n + ## sequencer.h ##\n +@@ sequencer.h: int sequencer_remove_state(struct replay_opts *opts);\n + */\n + #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)\n + #define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7)\n ++#define TODO_LIST_WARN_SKIPPED_CHERRY_PICKS (1U << 8)\n + \n + int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n + \t\t\t const char **argv, unsigned flags);\n\n Documentation/config/advice.txt | 3 +++\n Documentation/git-rebase.txt | 12 ++++++++----\n advice.c | 3 +++\n advice.h | 1 +\n builtin/rebase.c | 1 +\n sequencer.c | 22 ++++++++++++++++++++--\n sequencer.h | 1 +\n 7 files changed, 37 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 8b2849ff7b..063eec2511 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -44,6 +44,9 @@ advice.*::\n \t\tShown when linkgit:git-push[1] rejects a forced update of\n \t\ta branch when its remote-tracking ref has updates that we\n \t\tdo not have locally.\n+\tskippedCherryPicks::\n+\t\tShown when linkgit:git-rebase[1] skips a commit that has already\n+\t\tbeen cherry-picked onto the upstream branch.\n \tstatusAheadBehind::\n \t\tShown when linkgit:git-status[1] computes the ahead/behind\n \t\tcounts for a local ref compared to its remote tracking ref,\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 55af6fd24e..3978814912 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -79,9 +79,10 @@ remain the checked-out branch.\n \n If the upstream branch already contains a change you have made (e.g.,\n because you mailed a patch which was applied upstream), then that commit\n-will be skipped. For example, running `git rebase master` on the\n-following history (in which `A'` and `A` introduce the same set of changes,\n-but have different committer information):\n+will be skipped and warnings will be issued (if the `merge` backend is\n+used). For example, running `git rebase master` on the following\n+history (in which `A'` and `A` introduce the same set of changes, but\n+have different committer information):\n \n ------------\n A---B---C topic\n@@ -312,7 +313,10 @@ See also INCOMPATIBLE OPTIONS below.\n By default (or if `--no-reapply-cherry-picks` is given), these commits\n will be automatically dropped. Because this necessitates reading all\n upstream commits, this can be expensive in repos with a large number\n-of upstream commits that need to be read.\n+of upstream commits that need to be read. When using the `merge`\n+backend, warnings will be issued for each dropped commit (unless\n+`--quiet` is given). Advice will also be issued unless\n+`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).\n +\n `--reapply-cherry-picks` allows rebase to forgo reading all upstream\n commits, potentially improving performance.\ndiff --git a/advice.c b/advice.c\nindex 0b9c89c48a..ba6e703280 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -34,6 +34,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;\n int advice_submodule_alternate_error_strategy_die = 1;\n int advice_add_ignored_file = 1;\n int advice_add_empty_pathspec = 1;\n+int advice_skipped_cherry_picks = 1;\n \n static int advice_use_color = -1;\n static char advice_colors[][COLOR_MAXLEN] = {\n@@ -96,6 +97,7 @@ static struct {\n \t{ \"submoduleAlternateErrorStrategyDie\", &advice_submodule_alternate_error_strategy_die },\n \t{ \"addIgnoredFile\", &advice_add_ignored_file },\n \t{ \"addEmptyPathspec\", &advice_add_empty_pathspec },\n+\t{ \"skippedCherryPicks\", &advice_skipped_cherry_picks },\n \n \t/* make this an alias for backward compatibility */\n \t{ \"pushNonFastForward\", &advice_push_update_rejected }\n@@ -133,6 +135,7 @@ static struct {\n \t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\", 1 },\n \t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\", 1 },\n \t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\", 1 },\n+\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1 },\n \t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\", 1 },\n \t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\", 1 },\n \t[ADVICE_STATUS_U_OPTION]\t\t\t= { \"statusUoption\", 1 },\ndiff --git a/advice.h b/advice.h\nindex 9f8ffc7354..d705bf164c 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -75,6 +75,7 @@ extern int advice_add_empty_pathspec;\n \tADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,\n \tADVICE_UPDATE_SPARSE_PATH,\n \tADVICE_WAITING_FOR_EDITOR,\n+\tADVICE_SKIPPED_CHERRY_PICKS,\n };\n \n int git_default_advice_config(const char *var, const char *value);\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 12f093121d..aa9a795f46 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -405,6 +405,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,\n \tflags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;\n \tflags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;\n \tflags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;\n+\tflags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;\n \n \tswitch (command) {\n \tcase ACTION_NONE: {\ndiff --git a/sequencer.c b/sequencer.c\nindex 7f07cd00f3..8b036925e4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n \tint rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;\n \tint root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;\n+\tint skipped_commit = 0;\n \tstruct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;\n \tstruct strbuf label = STRBUF_INIT;\n \tstruct commit_list *commits = NULL, **tail = &commits, *iter;\n@@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\toidset_insert(&interesting, &commit->object.oid);\n \n \t\tis_empty = is_original_commit_empty(commit);\n-\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n+\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n+\t\t\tif (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)\n+\t\t\t\twarning(_(\"skipped previously applied commit %s\"),\n+\t\t\t\t\tshort_commit_name(commit));\n+\t\t\tskipped_commit = 1;\n \t\t\tcontinue;\n+\t\t}\n \t\tif (is_empty && !keep_empty)\n \t\t\tcontinue;\n \n@@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n \t\toidmap_put(&commit2todo, entry);\n \t}\n+\tif (skipped_commit)\n+\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n+\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n \n \t/*\n \t * Second phase:\n@@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n \tint reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;\n+\tint skipped_commit = 0;\n \n \trepo_init_revisions(r, &revs, NULL);\n \trevs.verbose_header = 1;\n@@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \twhile ((commit = get_revision(&revs))) {\n \t\tint is_empty = is_original_commit_empty(commit);\n \n-\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n+\t\tif (!is_empty && (commit->object.flags & PATCHSAME)) {\n+\t\t\tif (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)\n+\t\t\t\twarning(_(\"skipped previously applied commit %s\"),\n+\t\t\t\t\tshort_commit_name(commit));\n+\t\t\tskipped_commit = 1;\n \t\t\tcontinue;\n+\t\t}\n \t\tif (is_empty && !keep_empty)\n \t\t\tcontinue;\n \t\tstrbuf_addf(out, \"%s %s \", insn,\n@@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \t\t\tstrbuf_addf(out, \" %c empty\", comment_line_char);\n \t\tstrbuf_addch(out, '\\n');\n \t}\n+\tif (skipped_commit)\n+\t\tadvise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,\n+\t\t\t\t _(\"use --reapply-cherry-picks to include skipped commits\"));\n \treturn 0;\n }\n \ndiff --git a/sequencer.h b/sequencer.h\nindex d57d8ea23d..2088344cb3 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -156,6 +156,7 @@ int sequencer_remove_state(struct replay_opts *opts);\n */\n #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)\n #define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7)\n+#define TODO_LIST_WARN_SKIPPED_CHERRY_PICKS (1U << 8)\n \n int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \t\t\t const char **argv, unsigned flags);\n-- \n2.33.0.259.gc128427fd7-goog\n\n"},{"id":"434200","messageId":"xmqqr1ea1sm7.fsf@gitster.g","threadId":"56216","inReplyTo":"691c660422676c92180feb1c2a88049afe7536b9.1630359271.git.steadmon@google.com","subject":"Re: [PATCH v3] sequencer: advise if skipping cherry-picked commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T22:21:20Z","receivedAt":"2021-08-30T22:21:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Silently skipping commits when rebasing with --no-reapply-cherry-picks\n> (currently the default behavior) can cause user confusion. Issue\n> warnings when this happens, as well as advice on how to preserve the\n> skipped commits.\n>\n> These warnings and advice are displayed only when using the (default)\n> \"merge\" rebase backend.\n>\n> Update the git-rebase docs to mention the warnings and advice.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n\nLooks sensible.\n\nWill queue. Let's merge it down to 'next' in a few days and leave\nthe follow-on work to later.\n\nThanks.\n"},{"id":"434204","messageId":"xmqqy28izell.fsf@gitster.g","threadId":"56216","inReplyTo":"xmqqr1ea1sm7.fsf@gitster.g","subject":"Re: [PATCH v3] sequencer: advise if skipping cherry-picked commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T23:40:06Z","receivedAt":"2021-08-30T23:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Josh Steadmon writes:\n>\n>> Silently skipping commits when rebasing with --no-reapply-cherry-picks\n>> (currently the default behavior) can cause user confusion. Issue\n>> warnings when this happens, as well as advice on how to preserve the\n>> skipped commits.\n>>\n>> These warnings and advice are displayed only when using the (default)\n>> \"merge\" rebase backend.\n>>\n>> Update the git-rebase docs to mention the warnings and advice.\n>>\n>> Signed-off-by: Josh Steadmon \n>> ---\n>\n> Looks sensible.\n>\n> Will queue. Let's merge it down to 'next' in a few days and leave\n> the follow-on work to later.\n\n... except for that the advice_skipped_cherry_picks variable should\neither be file-scope static or declared in advice.h; otherwise \"make\nsparse\" would fail.\n\nIt only matters until ab/retire-advice-config topic graduates, so\nI'll see if I can apply a little tweak locally and make things work.\n\nThanks.\n"}]}