Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Aug 18, 2021, 10:08 UTC
- Message-ID
- <2973f162-d2ab-12da-49ec-82ef823089f6@gmail.com>
- In-Reply-To
- <xmqqwnotaq0e.fsf@gitster.g>
On 10/08/2021 23:33, Junio C Hamano wrote:
Show 45 quoted lines
> 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.
Show 15 quoted lines
>> @@ -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
Show 13 quoted lines
>
>> 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;
>> }