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

[Junio C Hamano <gitster@pobox.com>]

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.