Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit
From: Junio C Hamano <hidden>
Date: 2021-08-25 19:40:52
Josh Steadmon [off-list ref] 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 <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(-)
[jc: no need for immediate action from Josh] With [ref] 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.
quoted hunk
+ 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; + }
quoted hunk
@@ -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.