[RFC PATCH] sequencer: warn on skipping previously seen commit

Silently skipping commits when rebasing with --no-reapply-cherry-picks
(currently the default behavior) can cause user confusion. Issue a
warning in this case so that users are aware of what's happening.

Signed-off-by: Josh Steadmon <redacted>
---

We've had some complaints at $JOB where users were confused when
rebasing branches that contained commits that were previously
cherry-picked into their master branch. How do folks feel about adding a
warning in this case?

 sequencer.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7f07cd00f3..8888031c7b 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
 	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
+	int skipped_commit = 0;
 	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
 	struct strbuf label = STRBUF_INIT;
 	struct commit_list *commits = NULL, **tail = &commits, *iter;

@@ -5149,8 +5150,12 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		oidset_insert(&interesting, &commit->object.oid);
 
 		is_empty = is_original_commit_empty(commit);
-		if (!is_empty && (commit->object.flags & PATCHSAME))
+		if (!is_empty && (commit->object.flags & PATCHSAME)) {
+			warning(_("skipped previously seen commit %s"),
+				short_commit_name(commit));
+			skipped_commit = 1;
 			continue;
+		}
 		if (is_empty && !keep_empty)
 			continue;
 

@@ -5214,6 +5219,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		oidcpy(&entry->entry.oid, &commit->object.oid);
 		oidmap_put(&commit2todo, entry);
 	}
+	if (skipped_commit)
+		warning(_("use --reapply-cherry-picks to include skipped commits"));
 
 	/*
 	 * Second phase:

@@ -5334,6 +5341,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
 	int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;
+	int skipped_commit = 0;
 
 	repo_init_revisions(r, &revs, NULL);
 	revs.verbose_header = 1;

@@ -5369,8 +5377,12 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 	while ((commit = get_revision(&revs))) {
 		int is_empty = is_original_commit_empty(commit);
 
-		if (!is_empty && (commit->object.flags & PATCHSAME))
+		if (!is_empty && (commit->object.flags & PATCHSAME)) {
+			warning(_("skipped previously seen commit %s"),
+				short_commit_name(commit));
+			skipped_commit = 1;
 			continue;
+		}
 		if (is_empty && !keep_empty)
 			continue;
 		strbuf_addf(out, "%s %s ", insn,

@@ -5380,6 +5392,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 			strbuf_addf(out, " %c empty", comment_line_char);
 		strbuf_addch(out, '\n');
 	}
+	if (skipped_commit)
+		warning(_("use --reapply-cherry-picks to include skipped commits"));
 	return 0;
 }
 

-- 
2.32.0.554.ge1b32706d8-goog

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 <redacted>
---
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(-)

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;
 }
 

-- 
2.32.0.605.g8dce9f2422-goog

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 <redacted>
---
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'.

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.

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/

On 12/08/2021 18:45, Philippe Blain wrote:

Hi Josh,
[...]

quoted

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.

quoted

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

Hi Phillip,

Le 2021-08-18 à 06:02, Phillip Wood a écrit :

On 12/08/2021 18:45, Philippe Blain wrote:

quoted

Hi Josh,
[...]

quoted

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 :)

quoted

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,
[...]

quoted

quoted

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

quoted

quoted

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.

On 2021.08.12 13:45, Philippe Blain wrote:

Hi Josh,

Le 2021-08-10 à 15:20, Josh Steadmon a écrit :

quoted

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.

quoted

Signed-off-by: Josh Steadmon <redacted>
---
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.

quoted

  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.

quoted

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

Silently skipping commits when rebasing with --no-reapply-cherry-picks
(currently the default behavior) can cause user confusion. Issue
warnings when this happens, as well as advice on how to preserve the
skipped commits.

These warnings and advice are displayed only when using the (default)
"merge" rebase backend.

Update the git-rebase docs to mention the warnings and advice.

Signed-off-by: Josh Steadmon <redacted>
---
I dealt with the smaller suggestions from the last review round, but I
hope to eventually send out a V4 that also includes some of the larger
changes, such as extending this to git-format-patch. But I don't expect
to have time for that in the near future, so I figured it's better to
get V3 out in case we are OK with merging without those additional
features.

Junio, please let me know if you'd like me to rebase this on top of
ab/retire-advice-config.

Changes in V3:
* Switch back to warning() for individual skipped commits
* Suppress warnings when `--quiet` is given
* Fixed formatting of advice_setting entries in advice.c
* Mention the advice / warnings in git-rebase doc

Changes in V2:
* use advise_if_enabled() instead of warning()
* s/seen/applied/ in the advice text

Range-diff against v2:
1:  496da0b174 ! 1:  691c660422 sequencer: advise if skipping cherry-picked commit
    @@ Commit message
         sequencer: advise if skipping cherry-picked commit
     
         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.
    +    (currently the default behavior) can cause user confusion. Issue
    +    warnings when this happens, as well as advice on how to preserve the
    +    skipped commits.
    +
    +    These warnings and advice are displayed only when using the (default)
    +    "merge" rebase backend.
    +
    +    Update the git-rebase docs to mention the warnings and advice.
     
     
    @@ Documentation/config/advice.txt: advice.*::
      		Shown when linkgit:git-status[1] computes the ahead/behind
      		counts for a local ref compared to its remote tracking ref,
     
    + ## Documentation/git-rebase.txt ##
    +@@ Documentation/git-rebase.txt: remain the checked-out branch.
    + 
    + If the upstream branch already contains a change you have made (e.g.,
    + because you mailed a patch which was applied upstream), then that commit
    +-will be skipped. For example, running `git rebase master` on the
    +-following history (in which `A'` and `A` introduce the same set of changes,
    +-but have different committer information):
    ++will be skipped and warnings will be issued (if the `merge` backend is
    ++used).  For example, running `git rebase master` on the following
    ++history (in which `A'` and `A` introduce the same set of changes, but
    ++have different committer information):
    + 
    + ------------
    +           A---B---C topic
    +@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
    + By default (or if `--no-reapply-cherry-picks` is given), these commits
    + will be automatically dropped.  Because this necessitates reading all
    + upstream commits, this can be expensive in repos with a large number
    +-of upstream commits that need to be read.
    ++of upstream commits that need to be read.  When using the `merge`
    ++backend, warnings will be issued for each dropped commit (unless
    ++`--quiet` is given). Advice will also be issued unless
    ++`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).
    + +
    + `--reapply-cherry-picks` allows rebase to forgo reading all upstream
    + commits, potentially improving performance.
    +
      ## advice.c ##
     @@ advice.c: int advice_checkout_ambiguous_remote_branch_name = 1;
      int advice_submodule_alternate_error_strategy_die = 1;
    @@ advice.c: static struct {
      	/* make this an alias for backward compatibility */
      	{ "pushNonFastForward", &advice_push_update_rejected }
     @@ advice.c: 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[] =
    + 	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
    + 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },
    + 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure", 1 },
    ++	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1 },
    + 	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning", 1 },
    + 	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
    + 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
     
      ## advice.h ##
     @@ advice.h: extern int advice_add_empty_pathspec;
    @@ advice.h: extern int advice_add_empty_pathspec;
      
      int git_default_advice_config(const char *var, const char *value);
     
    + ## builtin/rebase.c ##
    +@@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts,
    + 	flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
    + 	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
    + 	flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
    ++	flags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;
    + 
    + 	switch (command) {
    + 	case ACTION_NONE: {
    +
      ## sequencer.c ##
     @@ sequencer.c: static int make_script_with_merges(struct pretty_print_context *pp,
      	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
    @@ sequencer.c: static int make_script_with_merges(struct pretty_print_context *pp,
      		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"),
    ++			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
    ++				warning(_("skipped previously applied commit %s"),
     +					short_commit_name(commit));
     +			skipped_commit = 1;
      			continue;
    @@ sequencer.c: int sequencer_make_script(struct repository *r, struct strbuf *out,
      
     -		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));
    ++			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
    ++				warning(_("skipped previously applied commit %s"),
    ++					short_commit_name(commit));
     +			skipped_commit = 1;
      			continue;
     +		}
    @@ sequencer.c: int sequencer_make_script(struct repository *r, struct strbuf *out,
      	return 0;
      }
      
    +
    + ## sequencer.h ##
    +@@ sequencer.h: int sequencer_remove_state(struct replay_opts *opts);
    +  */
    + #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
    + #define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7)
    ++#define TODO_LIST_WARN_SKIPPED_CHERRY_PICKS (1U << 8)
    + 
    + int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
    + 			  const char **argv, unsigned flags);

 Documentation/config/advice.txt |  3 +++
 Documentation/git-rebase.txt    | 12 ++++++++----
 advice.c                        |  3 +++
 advice.h                        |  1 +
 builtin/rebase.c                |  1 +
 sequencer.c                     | 22 ++++++++++++++++++++--
 sequencer.h                     |  1 +
 7 files changed, 37 insertions(+), 6 deletions(-)

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/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 55af6fd24e..3978814912 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -79,9 +79,10 @@ remain the checked-out branch.
 
 If the upstream branch already contains a change you have made (e.g.,
 because you mailed a patch which was applied upstream), then that commit
-will be skipped. For example, running `git rebase master` on the
-following history (in which `A'` and `A` introduce the same set of changes,
-but have different committer information):
+will be skipped and warnings will be issued (if the `merge` backend is
+used).  For example, running `git rebase master` on the following
+history (in which `A'` and `A` introduce the same set of changes, but
+have different committer information):
 
 ------------
           A---B---C topic

@@ -312,7 +313,10 @@ See also INCOMPATIBLE OPTIONS below.
 By default (or if `--no-reapply-cherry-picks` is given), these commits
 will be automatically dropped.  Because this necessitates reading all
 upstream commits, this can be expensive in repos with a large number
-of upstream commits that need to be read.
+of upstream commits that need to be read.  When using the `merge`
+backend, warnings will be issued for each dropped commit (unless
+`--quiet` is given). Advice will also be issued unless
+`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.

diff --git a/advice.c b/advice.c
index 0b9c89c48a..ba6e703280 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 }

@@ -133,6 +135,7 @@ static struct {
 	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },
 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure", 1 },
+	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1 },
 	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning", 1 },
 	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },

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/builtin/rebase.c b/builtin/rebase.c
index 12f093121d..aa9a795f46 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -405,6 +405,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 	flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
 	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 	flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
+	flags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;
 
 	switch (command) {
 	case ACTION_NONE: {

diff --git a/sequencer.c b/sequencer.c
index 7f07cd00f3..8b036925e4 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)) {
+			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
+				warning(_("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)) {
+			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
+				warning(_("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;
 }
 

diff --git a/sequencer.h b/sequencer.h
index d57d8ea23d..2088344cb3 100644
--- a/sequencer.h
+++ b/sequencer.h

@@ -156,6 +156,7 @@ int sequencer_remove_state(struct replay_opts *opts);
  */
 #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
 #define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7)
+#define TODO_LIST_WARN_SKIPPED_CHERRY_PICKS (1U << 8)
 
 int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 			  const char **argv, unsigned flags);

-- 
2.33.0.259.gc128427fd7-goog