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

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

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---

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

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

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

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

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> Silently skipping commits when rebasing with --no-reapply-cherry-picks
> (currently the default behavior) can cause user confusion. Issue a
> warning in this case so that users are aware of what's happening.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>
> We've had some complaints at $JOB where users were confused when
> rebasing branches that contained commits that were previously
> cherry-picked into their master branch. How do folks feel about adding a
> warning in this case?

I'd unconditionally in support if this were done under --verbose
option, but it becomes iffy if this is done unconditionally.

This is because I do not expect everybody will stay to be ignorant
of the behaviour of the tool they use every day, and I'd fear that
we'd start hearing "yeah, I know the command would skip to avoid
duplicated changes, why waste lines to tell me that?" complaints.

Having said that, I _hope_ that in a project with good hygiene, such
a multiple cherry-picking would not be so common and an exception,
and if my _hope_ proves to be true, then I am OK with giving this
warning unconditionally.  The user may know what the command does
when it sees a duplicated change, but the warning becomes about the
presence of such duplicated changes, which would be a rare event
that is worth notifying about.

>  		is_empty = is_original_commit_empty(commit);
> -		if (!is_empty && (commit->object.flags & PATCHSAME))
> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> +			warning(_("skipped previously seen commit %s"),

I am debating myself if s/seen/applied/ should be suggested here.

The existing text in the manual page says "a patch already accepted
upstream with a different commit message or timestamp will be
skipped", and "accepted" is a verb that would apply only in a
certain workflow, which is OK in the manual page that give more
context, but not here.  But 'seen' feels a bit too weak to me.

> +	if (skipped_commit)
> +		warning(_("use --reapply-cherry-picks to include skipped commits"));

I'd be hesitant to endorse doing this kind of "here is how to use
this command" unconditionally.  Perhaps under --verbose, or hide it
under "advise.*".

Thanks.

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

[Phillip Wood]

On 04/08/2021 22:28, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
>> Silently skipping commits when rebasing with --no-reapply-cherry-picks
>> (currently the default behavior) can cause user confusion. Issue a
>> warning in this case so that users are aware of what's happening.
>>
>> Signed-off-by: Josh Steadmon <steadmon@google.com>
>> ---
>>
>> We've had some complaints at $JOB where users were confused when
>> rebasing branches that contained commits that were previously
>> cherry-picked into their master branch. How do folks feel about adding a
>> warning in this case?
> 
> I'd unconditionally in support if this were done under --verbose
> option, but it becomes iffy if this is done unconditionally.

Perhaps we could skip the warning if the user is going to edit the todo 
list as they should see that the skipped commits have been commented 
out. I'm not sure about requiring --verbose - that might mean the users 
who would benefit most end up missing warning. As you say below I think 
it depends how often it appears in practice.

> This is because I do not expect everybody will stay to be ignorant
> of the behaviour of the tool they use every day, and I'd fear that
> we'd start hearing "yeah, I know the command would skip to avoid
> duplicated changes, why waste lines to tell me that?" complaints.
> 
> Having said that, I _hope_ that in a project with good hygiene, such
> a multiple cherry-picking would not be so common and an exception,
> and if my _hope_ proves to be true, then I am OK with giving this
> warning unconditionally.  The user may know what the command does
> when it sees a duplicated change, but the warning becomes about the
> presence of such duplicated changes, which would be a rare event
> that is worth notifying about.
> 
>>   		is_empty = is_original_commit_empty(commit);
>> -		if (!is_empty && (commit->object.flags & PATCHSAME))
>> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +			warning(_("skipped previously seen commit %s"),
> 
> I am debating myself if s/seen/applied/ should be suggested here.
> 
> The existing text in the manual page says "a patch already accepted
> upstream with a different commit message or timestamp will be
> skipped", and "accepted" is a verb that would apply only in a
> certain workflow, which is OK in the manual page that give more
> context, but not here.  But 'seen' feels a bit too weak to me.

Yes, I think 'applied' or 'cherry-picked' would be better than 'seen'

>> +	if (skipped_commit)
>> +		warning(_("use --reapply-cherry-picks to include skipped commits"));
> 
> I'd be hesitant to endorse doing this kind of "here is how to use
> this command" unconditionally.  Perhaps under --verbose, or hide it
> under "advise.*".

and use advise() rather than warning(). I'm guess this might be helpful 
but it wont help them get their commits back as there is no way to stop 
the rebase from dropping them at that point.

Best Wishes

Phillip

> Thanks.
> 

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>> +	if (skipped_commit)
>>> +		warning(_("use --reapply-cherry-picks to include skipped commits"));
>> I'd be hesitant to endorse doing this kind of "here is how to use
>> this command" unconditionally.  Perhaps under --verbose, or hide it
>> under "advise.*".
>
> and use advise() rather than warning(). I'm guess this might be
> helpful but it wont help them get their commits back as there is no
> way to stop the rebase from dropping them at that point.

Yes, but aborting the current rebase and redoing from scratch should
not be a brain surgery.  Even if you had already resolved conflicts
in earlier steps, the work will be replayed automatically for you by
the rerere mechanism.

[PATCH v2] sequencer: advise if skipping cherry-picked commit

[Josh Steadmon]

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

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Changes in V2:
* use advise_if_enabled() instead of warning()
* s/seen/applied/ in the advice text

 Documentation/config/advice.txt |  3 +++
 advice.c                        |  3 +++
 advice.h                        |  1 +
 sequencer.c                     | 22 ++++++++++++++++++++--
 4 files changed, 27 insertions(+), 2 deletions(-)

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

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> +	skippedCherryPicks::
> +		Shown when linkgit:git-rebase[1] skips a commit that has already
> +		been cherry-picked onto the upstream branch.

Makes sense.

> +	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1},

No need to resend to only fix this, but I'll add SP after "1" to
match surrounding lines while queuing the patch.

> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  		oidset_insert(&interesting, &commit->object.oid);
>  
>  		is_empty = is_original_commit_empty(commit);
> -		if (!is_empty && (commit->object.flags & PATCHSAME))
> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +					_("skipped previously applied commit %s"),
> +					short_commit_name(commit));
> +			skipped_commit = 1;
>  			continue;
> +		}
>  		if (is_empty && !keep_empty)
>  			continue;
>  
> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  		oidcpy(&entry->entry.oid, &commit->object.oid);
>  		oidmap_put(&commit2todo, entry);
>  	}
> +	if (skipped_commit)
> +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +				  _("use --reapply-cherry-picks to include skipped commits"));

I agree with the change in this hunk that advanced users may want to
squelch this "what to do" hint.

I am not sure about the earlier hunk that reports when some commits
have actually been skipped.  When --no-reapply-cherry-picks is in
effect, the user is expecting that some commits are cherry-picks
among other (hopefully the majority of) commits, and even those
users who do not want to be taught how to use the command would want
to learn the fact that some commits were skipped (and which ones).

Using two separate advice configuration variables feel way overkill
for this.  I wonder if the previous hunk should use warning(), i.e.

> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> +			warning(_("skipped previously applied commit %s"),
> +				short_commit_name(commit));
> +			skipped_commit = 1;
>  			continue;
> +		}

possibly squelched by "git rebase --quiet".

> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>  	while ((commit = get_revision(&revs))) {
>  		int is_empty = is_original_commit_empty(commit);
>  
> -		if (!is_empty && (commit->object.flags & PATCHSAME))
> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +					  _("skipped previously applied commit %s"),
> +					  short_commit_name(commit));
> +			skipped_commit = 1;
>  			continue;
> +		}

Likewise.  I wonder why we have two so-much-similar codepaths that
we need to touch, though.

>  		if (is_empty && !keep_empty)
>  			continue;
>  		strbuf_addf(out, "%s %s ", insn,
> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>  			strbuf_addf(out, " %c empty", comment_line_char);
>  		strbuf_addch(out, '\n');
>  	}
> +	if (skipped_commit)
> +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +				  _("use --reapply-cherry-picks to include skipped commits"));
>  	return 0;
>  }

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Phillip Wood]

On 10/08/2021 23:33, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>>   		oidset_insert(&interesting, &commit->object.oid);
>>   
>>   		is_empty = is_original_commit_empty(commit);
>> -		if (!is_empty && (commit->object.flags & PATCHSAME))
>> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +					_("skipped previously applied commit %s"),
>> +					short_commit_name(commit));
>> +			skipped_commit = 1;
>>   			continue;
>> +		}
>>   		if (is_empty && !keep_empty)
>>   			continue;
>>   
>> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>>   		oidcpy(&entry->entry.oid, &commit->object.oid);
>>   		oidmap_put(&commit2todo, entry);
>>   	}
>> +	if (skipped_commit)
>> +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +				  _("use --reapply-cherry-picks to include skipped commits"));
> 
> I agree with the change in this hunk that advanced users may want to
> squelch this "what to do" hint.
> 
> I am not sure about the earlier hunk that reports when some commits
> have actually been skipped.  When --no-reapply-cherry-picks is in
> effect, the user is expecting that some commits are cherry-picks
> among other (hopefully the majority of) commits, and even those
> users who do not want to be taught how to use the command would want
> to learn the fact that some commits were skipped (and which ones).
> 
> Using two separate advice configuration variables feel way overkill
> for this.  I wonder if the previous hunk should use warning(), i.e.
> 
>> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +			warning(_("skipped previously applied commit %s"),
>> +				short_commit_name(commit));
>> +			skipped_commit = 1;
>>   			continue;
>> +		}
> 
> possibly squelched by "git rebase --quiet".

I think that would be nicer. Using advise_if_enabled() in the second 
hunk makes sense but I think a printing "warning" rather than a "hint" 
in the first hunk.

>> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>>   	while ((commit = get_revision(&revs))) {
>>   		int is_empty = is_original_commit_empty(commit);
>>   
>> -		if (!is_empty && (commit->object.flags & PATCHSAME))
>> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +					  _("skipped previously applied commit %s"),
>> +					  short_commit_name(commit));
>> +			skipped_commit = 1;
>>   			continue;
>> +		}
> 
> Likewise.  I wonder why we have two so-much-similar codepaths that
> we need to touch, though.

--recreate-merges implemented the main loop for its todo list generation 
in a new function as it is much more involved that for the linear case.

Best Wishes

Phillip

> 
>>   		if (is_empty && !keep_empty)
>>   			continue;
>>   		strbuf_addf(out, "%s %s ", insn,
>> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>>   			strbuf_addf(out, " %c empty", comment_line_char);
>>   		strbuf_addch(out, '\n');
>>   	}
>> +	if (skipped_commit)
>> +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +				  _("use --reapply-cherry-picks to include skipped commits"));
>>   	return 0;
>>   }

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Josh Steadmon]

On 2021.08.10 15:33, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > +	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1},
> 
> No need to resend to only fix this, but I'll add SP after "1" to
> match surrounding lines while queuing the patch.

Ack. Fixed in V3. Do you also want me to rebase this on
ab/retire-advice-config?

> > @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >  		oidset_insert(&interesting, &commit->object.oid);
> >  
> >  		is_empty = is_original_commit_empty(commit);
> > -		if (!is_empty && (commit->object.flags & PATCHSAME))
> > +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> > +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +					_("skipped previously applied commit %s"),
> > +					short_commit_name(commit));
> > +			skipped_commit = 1;
> >  			continue;
> > +		}
> >  		if (is_empty && !keep_empty)
> >  			continue;
> >  
> > @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >  		oidcpy(&entry->entry.oid, &commit->object.oid);
> >  		oidmap_put(&commit2todo, entry);
> >  	}
> > +	if (skipped_commit)
> > +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +				  _("use --reapply-cherry-picks to include skipped commits"));
> 
> I agree with the change in this hunk that advanced users may want to
> squelch this "what to do" hint.
> 
> I am not sure about the earlier hunk that reports when some commits
> have actually been skipped.  When --no-reapply-cherry-picks is in
> effect, the user is expecting that some commits are cherry-picks
> among other (hopefully the majority of) commits, and even those
> users who do not want to be taught how to use the command would want
> to learn the fact that some commits were skipped (and which ones).
> 
> Using two separate advice configuration variables feel way overkill
> for this.  I wonder if the previous hunk should use warning(), i.e.

Switched these to warnings, squelched by "--quiet" as suggested below.


> > +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> > +			warning(_("skipped previously applied commit %s"),
> > +				short_commit_name(commit));
> > +			skipped_commit = 1;
> >  			continue;
> > +		}
> 
> possibly squelched by "git rebase --quiet".
> 

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Philippe Blain]

Hi Josh,

Le 2021-08-10 à 15:20, Josh Steadmon a écrit :
> Silently skipping commits when rebasing with --no-reapply-cherry-picks
> (currently the default behavior) can cause user confusion. Issue advice
> in this case so that users are aware of what's happening.

I think this is an excellent idea. It can be very surprising, especially
for 'git rebase' beginners/intermediate users who might not have read the
man page.

Since your proposed changes are in sequencer.c, this will only affect
the default "merge" rebase backend, and not the older 'apply' backend. I think
it might be worth mentioning this in the commit message.

Note that it might be considerably more work to also add the warning
for the 'apply' backend, since rebase.c::run_am generates the patches
using 'git format-patch --cherry-pick --right-only $upstream..HEAD' and
so cherry-picks are dropped early in the process. I think that this not that big
of a deal since the default backend is now "merge".

> 
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> Changes in V2:
> * use advise_if_enabled() instead of warning()
> * s/seen/applied/ in the advice text
> 
>   Documentation/config/advice.txt |  3 +++
>   advice.c                        |  3 +++
>   advice.h                        |  1 +
>   sequencer.c                     | 22 ++++++++++++++++++++--
>   4 files changed, 27 insertions(+), 2 deletions(-)

I would suggest mentioning the new behaviour and the new
advice.skippedCherryPicks config in git-rebase.txt, say in the paragraph
starting with "If the upstream branch already contains" in the Description section
and in the description of '--reapply-cherry-picks'.

>   int git_default_advice_config(const char *var, const char *value);
> diff --git a/sequencer.c b/sequencer.c
> index 7f07cd00f3..1235f61c9d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
>   	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
>   	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
> +	int skipped_commit = 0;
>   	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
>   	struct strbuf label = STRBUF_INIT;
>   	struct commit_list *commits = NULL, **tail = &commits, *iter;
> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   		oidset_insert(&interesting, &commit->object.oid);
>   
>   		is_empty = is_original_commit_empty(commit);
> -		if (!is_empty && (commit->object.flags & PATCHSAME))
> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +					_("skipped previously applied commit %s"),
> +					short_commit_name(commit));
> +			skipped_commit = 1;
>   			continue;
> +		}
>   		if (is_empty && !keep_empty)
>   			continue;

For interactive rebase, an alternate implementation, that I suggested in [1] last summer, would be to keep
the cherry-picks in the todo list, but mark them as 'drop' and add a comment at the
end of their line, like '# already applied' or something like this, similar
to how empty commits have '# empty' appended. I think that for interactive rebase, I
would prefer this, since it is easier for the user to notice it and change the 'drop'
to 'pick' right away if they realise they do not want to drop those commits (easier
than seeing the warning, realising they did not want to drop them, aborting the rebase
and redoing it with '--reapply-cherry-picks').

For non-interactive rebase adding a warning/advice like your patch does seems to
be a good solution.

>   
> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   		oidcpy(&entry->entry.oid, &commit->object.oid);
>   		oidmap_put(&commit2todo, entry);
>   	}
> +	if (skipped_commit)
> +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +				  _("use --reapply-cherry-picks to include skipped commits"));
>   
>   	/*
>   	 * Second phase:
> @@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>   	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
>   	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
>   	int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;
> +	int skipped_commit = 0;
>   
>   	repo_init_revisions(r, &revs, NULL);
>   	revs.verbose_header = 1;
> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>   	while ((commit = get_revision(&revs))) {
>   		int is_empty = is_original_commit_empty(commit);
>   
> -		if (!is_empty && (commit->object.flags & PATCHSAME))
> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +					  _("skipped previously applied commit %s"),
> +					  short_commit_name(commit));
> +			skipped_commit = 1;
>   			continue;
> +		}
>   		if (is_empty && !keep_empty)
>   			continue;
>   		strbuf_addf(out, "%s %s ", insn,
> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>   			strbuf_addf(out, " %c empty", comment_line_char);
>   		strbuf_addch(out, '\n');
>   	}
> +	if (skipped_commit)
> +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +				  _("use --reapply-cherry-picks to include skipped commits"));
>   	return 0;
>   }
>   
> 

Like Junio remarked, it is a little unfortunate that some logic is duplicated between
'sequencer_make_script' and 'make_script_with_merges', such that your patch has to do
the same thing at two different code locations. Maybe a preparatory cleanup could add
a new function that takes care of the duplicated logic and call if from both ? I'm
just thinking out loud here, I did not analyze in details if this would be easy/feasible...

Thanks for suggesting this change,

Philippe.


[1] https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Since your proposed changes are in sequencer.c, this will only affect
> the default "merge" rebase backend, and not the older 'apply' backend. I think
> it might be worth mentioning this in the commit message.

It shouldn't be too hard to teach "format-patch --cherry-pick" to
report when commits are actually dropped.  There isn't necessarily a
need to give help to users what to do to apply the same change
twice, but even for those who _know_ how to do so, they need to know
if commits are dropped in the first place.  And such a notice is useful
for any user of the --cherry-pick feature of format-patch, not just
as an implementation detail of "git rebase".

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Phillip Wood]

On 12/08/2021 18:45, Philippe Blain wrote:
> Hi Josh,
> [...]
>> diff --git a/sequencer.c b/sequencer.c
>> index 7f07cd00f3..1235f61c9d 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct 
>> pretty_print_context *pp,
>>       int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
>>       int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
>>       int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
>> +    int skipped_commit = 0;
>>       struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
>>       struct strbuf label = STRBUF_INIT;
>>       struct commit_list *commits = NULL, **tail = &commits, *iter;
>> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct 
>> pretty_print_context *pp,
>>           oidset_insert(&interesting, &commit->object.oid);
>>           is_empty = is_original_commit_empty(commit);
>> -        if (!is_empty && (commit->object.flags & PATCHSAME))
>> +        if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +            advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +                    _("skipped previously applied commit %s"),
>> +                    short_commit_name(commit));
>> +            skipped_commit = 1;
>>               continue;
>> +        }
>>           if (is_empty && !keep_empty)
>>               continue;
> 
> For interactive rebase, an alternate implementation, that I suggested in 
> [1] last summer, would be to keep
> the cherry-picks in the todo list, but mark them as 'drop' and add a 
> comment at the
> end of their line, like '# already applied' or something like this, similar
> to how empty commits have '# empty' appended. I think that for 
> interactive rebase, I
> would prefer this, since it is easier for the user to notice it and 
> change the 'drop'
> to 'pick' right away if they realise they do not want to drop those 
> commits (easier
> than seeing the warning, realising they did not want to drop them, 
> aborting the rebase
> and redoing it with '--reapply-cherry-picks').

That would be nice, but we could always add it in the future if Josh 
does not want to implement it now. At the moment the function that 
creates the todo list does not know if it is going to be edited, I'm not 
sure how easy it would be to pass that information down.

> For non-interactive rebase adding a warning/advice like your patch does 
> seems to
> be a good solution.
> 
>> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct 
>> pretty_print_context *pp,
>>           oidcpy(&entry->entry.oid, &commit->object.oid);
>>           oidmap_put(&commit2todo, entry);
>>       }
>> +    if (skipped_commit)
>> +        advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +                  _("use --reapply-cherry-picks to include skipped 
>> commits"));
>>       /*
>>        * Second phase:
>> @@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, 
>> struct strbuf *out, int argc,
>>       const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : 
>> "pick";
>>       int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
>>       int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;
>> +    int skipped_commit = 0;
>>       repo_init_revisions(r, &revs, NULL);
>>       revs.verbose_header = 1;
>> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, 
>> struct strbuf *out, int argc,
>>       while ((commit = get_revision(&revs))) {
>>           int is_empty = is_original_commit_empty(commit);
>> -        if (!is_empty && (commit->object.flags & PATCHSAME))
>> +        if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +            advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +                      _("skipped previously applied commit %s"),
>> +                      short_commit_name(commit));
>> +            skipped_commit = 1;
>>               continue;
>> +        }
>>           if (is_empty && !keep_empty)
>>               continue;
>>           strbuf_addf(out, "%s %s ", insn,
>> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, 
>> struct strbuf *out, int argc,
>>               strbuf_addf(out, " %c empty", comment_line_char);
>>           strbuf_addch(out, '\n');
>>       }
>> +    if (skipped_commit)
>> +        advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +                  _("use --reapply-cherry-picks to include skipped 
>> commits"));
>>       return 0;
>>   }
>>
> 
> Like Junio remarked, it is a little unfortunate that some logic is 
> duplicated between
> 'sequencer_make_script' and 'make_script_with_merges', such that your 
> patch has to do
> the same thing at two different code locations. Maybe a preparatory 
> cleanup could add
> a new function that takes care of the duplicated logic and call if from 
> both ? I'm
> just thinking out loud here, I did not analyze in details if this would 
> be easy/feasible...

I think feasible but not easy (or required for this change), it would 
also complicate the code in a different way as I think we'd have to add 
some conditionals for whether we are recreating merges or not.

Best Wishes

Phillip


> Thanks for suggesting this change,
> 
> Philippe.
> 
> 
> [1] 
> https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Philippe Blain]

Hi Phillip,

Le 2021-08-18 à 06:02, Phillip Wood a écrit :
> On 12/08/2021 18:45, Philippe Blain wrote:
>> Hi Josh,
>> [...]
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 7f07cd00f3..1235f61c9d 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>>>       int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
>>>       int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
>>>       int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
>>> +    int skipped_commit = 0;
>>>       struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
>>>       struct strbuf label = STRBUF_INIT;
>>>       struct commit_list *commits = NULL, **tail = &commits, *iter;
>>> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>>>           oidset_insert(&interesting, &commit->object.oid);
>>>           is_empty = is_original_commit_empty(commit);
>>> -        if (!is_empty && (commit->object.flags & PATCHSAME))
>>> +        if (!is_empty && (commit->object.flags & PATCHSAME)) {
>>> +            advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>>> +                    _("skipped previously applied commit %s"),
>>> +                    short_commit_name(commit));
>>> +            skipped_commit = 1;
>>>               continue;
>>> +        }
>>>           if (is_empty && !keep_empty)
>>>               continue;
>>
>> For interactive rebase, an alternate implementation, that I suggested in [1] last summer, would be to keep
>> the cherry-picks in the todo list, but mark them as 'drop' and add a comment at the
>> end of their line, like '# already applied' or something like this, similar
>> to how empty commits have '# empty' appended. I think that for interactive rebase, I
>> would prefer this, since it is easier for the user to notice it and change the 'drop'
>> to 'pick' right away if they realise they do not want to drop those commits (easier
>> than seeing the warning, realising they did not want to drop them, aborting the rebase
>> and redoing it with '--reapply-cherry-picks').
> 
> That would be nice, but we could always add it in the future if Josh does not want to implement it now. At the moment the function that creates the todo list does not know if it is going to be edited, I'm not sure how easy it would be to pass that information down.

Well, I'm not sure we need to ? If we change the 'pick' to 'drop', instead of
not writing these lines to the todo list, the cherry-picked commits will get dropped
either way, no? Unless I'm missing something - I think you are way more well-versed in
the sequencer code than me :)

>>
>> Like Junio remarked, it is a little unfortunate that some logic is duplicated between
>> 'sequencer_make_script' and 'make_script_with_merges', such that your patch has to do
>> the same thing at two different code locations. Maybe a preparatory cleanup could add
>> a new function that takes care of the duplicated logic and call if from both ? I'm
>> just thinking out loud here, I did not analyze in details if this would be easy/feasible...
> 
> I think feasible but not easy (or required for this change), it would also complicate the code in a different way as I think we'd have to add some conditionals for whether we are recreating merges or not.

Yes, I agree it's not required for this change.

Cheers,

Philippe.

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Phillip Wood]

Hi Philippe
On 18/08/2021 23:45, Philippe Blain wrote:
> Hi Phillip,
> [...]
>>> For interactive rebase, an alternate implementation, that I suggested 
>>> in [1] last summer, would be to keep
>>> the cherry-picks in the todo list, but mark them as 'drop' and add a 
>>> comment at the
>>> end of their line, like '# already applied' or something like this, 
>>> similar
>>> to how empty commits have '# empty' appended. I think that for 
>>> interactive rebase, I
>>> would prefer this, since it is easier for the user to notice it and 
>>> change the 'drop'
>>> to 'pick' right away if they realise they do not want to drop those 
>>> commits (easier
>>> than seeing the warning, realising they did not want to drop them, 
>>> aborting the rebase
>>> and redoing it with '--reapply-cherry-picks').
>>
>> That would be nice, but we could always add it in the future if Josh 
>> does not want to implement it now. At the moment the function that 
>> creates the todo list does not know if it is going to be edited, I'm 
>> not sure how easy it would be to pass that information down.
> 
> Well, I'm not sure we need to ? If we change the 'pick' to 'drop', 
> instead of
> not writing these lines to the todo list, the cherry-picked commits will 
> get dropped
> either way, no? Unless I'm missing something - I think you are way more 
> well-versed in
> the sequencer code than me :)

I think I read your suggestion as meaning we would not print the warning 
if the user was going to edit the list - hence the need for the 
distinction. I've just had a closer look at the code and I think it 
would be fairly easy to tell sequencer_make_script() if the todo list is 
going to be edited, there is only one caller in builtin/rebase.c which 
could easily pass a flag for this. Thinking about the 'print warning' vs 
'add drop command' issue if the user is using a terminal based editor 
such as vim or nano then they will barely have time to see the warnings 
being printed let alone read them before the editor is opened and hides 
them so having some indication in the todo list would probably be a good 
idea.

Best Wishes

Phillip

>>>
>>> Like Junio remarked, it is a little unfortunate that some logic is 
>>> duplicated between
>>> 'sequencer_make_script' and 'make_script_with_merges', such that your 
>>> patch has to do
>>> the same thing at two different code locations. Maybe a preparatory 
>>> cleanup could add
>>> a new function that takes care of the duplicated logic and call if 
>>> from both ? I'm
>>> just thinking out loud here, I did not analyze in details if this 
>>> would be easy/feasible...
>>
>> I think feasible but not easy (or required for this change), it would 
>> also complicate the code in a different way as I think we'd have to 
>> add some conditionals for whether we are recreating merges or not.
> 
> Yes, I agree it's not required for this change.
> 
> Cheers,
> 
> Philippe.

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Josh Steadmon]

On 2021.08.12 13:45, Philippe Blain wrote:
> Hi Josh,
> 
> Le 2021-08-10 à 15:20, Josh Steadmon a écrit :
> > Silently skipping commits when rebasing with --no-reapply-cherry-picks
> > (currently the default behavior) can cause user confusion. Issue advice
> > in this case so that users are aware of what's happening.
> 
> I think this is an excellent idea. It can be very surprising, especially
> for 'git rebase' beginners/intermediate users who might not have read the
> man page.
> 
> Since your proposed changes are in sequencer.c, this will only affect
> the default "merge" rebase backend, and not the older 'apply' backend. I think
> it might be worth mentioning this in the commit message.
> 
> Note that it might be considerably more work to also add the warning
> for the 'apply' backend, since rebase.c::run_am generates the patches
> using 'git format-patch --cherry-pick --right-only $upstream..HEAD' and
> so cherry-picks are dropped early in the process. I think that this not that big
> of a deal since the default backend is now "merge".

Ah good point, thank you for the catch. I have noted this in the V3
commit message.

> > 
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> > Changes in V2:
> > * use advise_if_enabled() instead of warning()
> > * s/seen/applied/ in the advice text
> > 
> >   Documentation/config/advice.txt |  3 +++
> >   advice.c                        |  3 +++
> >   advice.h                        |  1 +
> >   sequencer.c                     | 22 ++++++++++++++++++++--
> >   4 files changed, 27 insertions(+), 2 deletions(-)
> 
> I would suggest mentioning the new behaviour and the new
> advice.skippedCherryPicks config in git-rebase.txt, say in the paragraph
> starting with "If the upstream branch already contains" in the Description section
> and in the description of '--reapply-cherry-picks'.

Done in V3.


> >   int git_default_advice_config(const char *var, const char *value);
> > diff --git a/sequencer.c b/sequencer.c
> > index 7f07cd00f3..1235f61c9d 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >   	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
> >   	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
> >   	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
> > +	int skipped_commit = 0;
> >   	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
> >   	struct strbuf label = STRBUF_INIT;
> >   	struct commit_list *commits = NULL, **tail = &commits, *iter;
> > @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >   		oidset_insert(&interesting, &commit->object.oid);
> >   		is_empty = is_original_commit_empty(commit);
> > -		if (!is_empty && (commit->object.flags & PATCHSAME))
> > +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> > +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +					_("skipped previously applied commit %s"),
> > +					short_commit_name(commit));
> > +			skipped_commit = 1;
> >   			continue;
> > +		}
> >   		if (is_empty && !keep_empty)
> >   			continue;
> 
> For interactive rebase, an alternate implementation, that I suggested in [1] last summer, would be to keep
> the cherry-picks in the todo list, but mark them as 'drop' and add a comment at the
> end of their line, like '# already applied' or something like this, similar
> to how empty commits have '# empty' appended. I think that for interactive rebase, I
> would prefer this, since it is easier for the user to notice it and change the 'drop'
> to 'pick' right away if they realise they do not want to drop those commits (easier
> than seeing the warning, realising they did not want to drop them, aborting the rebase
> and redoing it with '--reapply-cherry-picks').

I haven't had time to do this in V3, but I can look into it for V4.

> For non-interactive rebase adding a warning/advice like your patch does seems to
> be a good solution.
> 
> > @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >   		oidcpy(&entry->entry.oid, &commit->object.oid);
> >   		oidmap_put(&commit2todo, entry);
> >   	}
> > +	if (skipped_commit)
> > +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +				  _("use --reapply-cherry-picks to include skipped commits"));
> >   	/*
> >   	 * Second phase:
> > @@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
> >   	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
> >   	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
> >   	int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;
> > +	int skipped_commit = 0;
> >   	repo_init_revisions(r, &revs, NULL);
> >   	revs.verbose_header = 1;
> > @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
> >   	while ((commit = get_revision(&revs))) {
> >   		int is_empty = is_original_commit_empty(commit);
> > -		if (!is_empty && (commit->object.flags & PATCHSAME))
> > +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> > +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +					  _("skipped previously applied commit %s"),
> > +					  short_commit_name(commit));
> > +			skipped_commit = 1;
> >   			continue;
> > +		}
> >   		if (is_empty && !keep_empty)
> >   			continue;
> >   		strbuf_addf(out, "%s %s ", insn,
> > @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
> >   			strbuf_addf(out, " %c empty", comment_line_char);
> >   		strbuf_addch(out, '\n');
> >   	}
> > +	if (skipped_commit)
> > +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> > +				  _("use --reapply-cherry-picks to include skipped commits"));
> >   	return 0;
> >   }
> > 
> 
> Like Junio remarked, it is a little unfortunate that some logic is duplicated between
> 'sequencer_make_script' and 'make_script_with_merges', such that your patch has to do
> the same thing at two different code locations. Maybe a preparatory cleanup could add
> a new function that takes care of the duplicated logic and call if from both ? I'm
> just thinking out loud here, I did not analyze in details if this would be easy/feasible...

Will look into this for V4 as well.

> Thanks for suggesting this change,
> 
> Philippe.
> 
> 
> [1] https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/

Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> Silently skipping commits when rebasing with --no-reapply-cherry-picks
> (currently the default behavior) can cause user confusion. Issue advice
> in this case so that users are aware of what's happening.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> Changes in V2:
> * use advise_if_enabled() instead of warning()
> * s/seen/applied/ in the advice text
>
>  Documentation/config/advice.txt |  3 +++
>  advice.c                        |  3 +++
>  advice.h                        |  1 +
>  sequencer.c                     | 22 ++++++++++++++++++++--
>  4 files changed, 27 insertions(+), 2 deletions(-)

[jc: no need for immediate action from Josh]

With <patch-v4-2.4-3869bda3b39-20210823T103719Z-avarab@gmail.com> 
it would make more sense to stop using the "global variable
registered with advice_config[] array" part of the patch, especially
because we only use advise_if_enabled() already in this patch.

> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
> +					  _("skipped previously applied commit %s"),
> +					  short_commit_name(commit));
> +			skipped_commit = 1;
>  			continue;
> +		}

> @@ -139,6 +141,7 @@ static struct {
>  	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
>  	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
>  	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
> +	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1},
>  };

This needs to be moved to keep the list sorted.

When I queued the topic to migrate to advise-settings from
advice-config, I made some adjustment for this topic (and luckily
there is no other topic in flight that touch advice.c).  Please
sanity check the result when today's integration result is pushed
out, perhaps in 5 hours or so.

Thanks.

[PATCH v3] sequencer: advise if skipping cherry-picked commit

[Josh Steadmon]

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 <steadmon@google.com>
---
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

Re: [PATCH v3] sequencer: advise if skipping cherry-picked commit

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> Silently skipping commits when rebasing with --no-reapply-cherry-picks
> (currently the default behavior) can cause user confusion. Issue
> 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 <steadmon@google.com>
> ---

Looks sensible.

Will queue.  Let's merge it down to 'next' in a few days and leave
the follow-on work to later.

Thanks.

Re: [PATCH v3] sequencer: advise if skipping cherry-picked commit

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Josh Steadmon <steadmon@google.com> writes:
>
>> Silently skipping commits when rebasing with --no-reapply-cherry-picks
>> (currently the default behavior) can cause user confusion. Issue
>> 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 <steadmon@google.com>
>> ---
>
> Looks sensible.
>
> Will queue.  Let's merge it down to 'next' in a few days and leave
> the follow-on work to later.

... except for that the advice_skipped_cherry_picks variable should
either be file-scope static or declared in advice.h; otherwise "make
sparse" would fail.

It only matters until ab/retire-advice-config topic graduates, so
I'll see if I can apply a little tweak locally and make things work.

Thanks.