Josh Steadmon [off-list ref] writes:
+ skippedCherryPicks::
+ Shown when linkgit:git-rebase[1] skips a commit that has already
+ been cherry-picked onto the upstream branch.
Makes sense.
+ [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1},
No need to resend to only fix this, but I'll add SP after "1" to
match surrounding lines while queuing the patch.
quoted hunk
@@ -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".
quoted hunk
@@ -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.
quoted hunk
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;
}
On 10/08/2021 23:33, Junio C Hamano wrote:
Josh Steadmon [off-list ref] writes:
quoted
@@ -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.
quoted
+ 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.
quoted
@@ -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
quoted
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;
}
On 2021.08.10 15:33, Junio C Hamano wrote:
Josh Steadmon [off-list ref] writes:
quoted
+ [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1},
No need to resend to only fix this, but I'll add SP after "1" to
match surrounding lines while queuing the patch.
Ack. Fixed in V3. Do you also want me to rebase this on
ab/retire-advice-config?
quoted
@@ -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.
Switched these to warnings, squelched by "--quiet" as suggested below.
quoted
+ 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".