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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2021, 19:40 UTC
Message-ID: <xmqqfsuxia87.fsf@gitster.g>
In-Reply-To: <496da0b17476011b4ef4dde31593afc7572246eb.1628623058.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 15 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

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