From: Junio C Hamano Date: Wed, 25 Aug 2021 19:40:40 GMT Subject: Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit Message-ID: In-Reply-To: <496da0b17476011b4ef4dde31593afc7572246eb.1628623058.git.steadmon@google.com> Josh Steadmon 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 > --- > 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 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.