sequencer: advise if skipping cherry-picked commit

Silently skipping commits when rebasing with --no-reapply-cherry-picks
(currently the default behavior) can cause user confusion. Issue
warnings when this happens, as well as advice on how to preserve the
skipped commits.

These warnings and advice are displayed only when using the (default)
"merge" rebase backend.

Update the git-rebase docs to mention the warnings and advice.

Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 8b2849ff7b3f5c014dd64504a519eaf78daa516a..063eec2511d37edd3c00037dec3ce9298ce20506 100644 (file)
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -44,6 +44,9 @@ advice.*::
                Shown when linkgit:git-push[1] rejects a forced update of
                a branch when its remote-tracking ref has updates that we
                do not have locally.
+       skippedCherryPicks::
+               Shown when linkgit:git-rebase[1] skips a commit that has already
+               been cherry-picked onto the upstream branch.
        statusAheadBehind::
                Shown when linkgit:git-status[1] computes the ahead/behind
                counts for a local ref compared to its remote tracking ref,

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 55af6fd24e27cd13ff93f7188dbd403e0cc7aeb6..397881491226d45fd432e89d0e0c03e1f539ecf2 100644 (file)
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -79,9 +79,10 @@ remain the checked-out branch.
 
 If the upstream branch already contains a change you have made (e.g.,
 because you mailed a patch which was applied upstream), then that commit
-will be skipped. For example, running `git rebase master` on the
-following history (in which `A'` and `A` introduce the same set of changes,
-but have different committer information):
+will be skipped and warnings will be issued (if the `merge` backend is
+used).  For example, running `git rebase master` on the following
+history (in which `A'` and `A` introduce the same set of changes, but
+have different committer information):
 
 ------------
           A---B---C topic
@@ -312,7 +313,10 @@ See also INCOMPATIBLE OPTIONS below.
 By default (or if `--no-reapply-cherry-picks` is given), these commits
 will be automatically dropped.  Because this necessitates reading all
 upstream commits, this can be expensive in repos with a large number
-of upstream commits that need to be read.
+of upstream commits that need to be read.  When using the `merge`
+backend, warnings will be issued for each dropped commit (unless
+`--quiet` is given). Advice will also be issued unless
+`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.

diff --git a/advice.c b/advice.c
index 0b9c89c48ab996da0e7938ac0667a3ecad925efc..ba6e703280d7ebc4f12e971b749109778cbc920d 100644 (file)
--- a/advice.c
+++ b/advice.c
@@ -34,6 +34,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
+int advice_skipped_cherry_picks = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -96,6 +97,7 @@ static struct {
        { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
        { "addIgnoredFile", &advice_add_ignored_file },
        { "addEmptyPathspec", &advice_add_empty_pathspec },
+       { "skippedCherryPicks", &advice_skipped_cherry_picks },
 
        /* make this an alias for backward compatibility */
        { "pushNonFastForward", &advice_push_update_rejected }
@@ -133,6 +135,7 @@ static struct {
        [ADVICE_RM_HINTS]                               = { "rmHints", 1 },
        [ADVICE_SEQUENCER_IN_USE]                       = { "sequencerInUse", 1 },
        [ADVICE_SET_UPSTREAM_FAILURE]                   = { "setUpstreamFailure", 1 },
+       [ADVICE_SKIPPED_CHERRY_PICKS]                   = { "skippedCherryPicks", 1 },
        [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { "statusAheadBehindWarning", 1 },
        [ADVICE_STATUS_HINTS]                           = { "statusHints", 1 },
        [ADVICE_STATUS_U_OPTION]                        = { "statusUoption", 1 },

diff --git a/advice.h b/advice.h
index 9f8ffc73546b3939ad5ca9496b187c7aeb60242e..4b4f5747205565ddb44d76b27443ef129b025269 100644 (file)
--- a/advice.h
+++ b/advice.h
@@ -35,6 +35,7 @@ extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
+extern int advice_skipped_cherry_picks;
 
 /*
  * To add a new advice, you need to:
@@ -75,6 +76,7 @@ extern int advice_add_empty_pathspec;
        ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
        ADVICE_UPDATE_SPARSE_PATH,
        ADVICE_WAITING_FOR_EDITOR,
+       ADVICE_SKIPPED_CHERRY_PICKS,
 };
 
 int git_default_advice_config(const char *var, const char *value);

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 33e09619005b2aa706ffb33c0f31f2378eba1fe7..db2342c732decb09bc3c7832844dca61d978bc34 100644 (file)
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -405,6 +405,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
        flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
        flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
        flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
+       flags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;
 
        switch (command) {
        case ACTION_NONE: {

diff --git a/sequencer.c b/sequencer.c
index 7f07cd00f3f20ab3dc0c508653ac28bdd8e8b8c1..8b036925e4293c7042673c34dc4fcedf23663c7f 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -5099,6 +5099,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
        int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
        int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
        int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
+       int skipped_commit = 0;
        struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
        struct strbuf label = STRBUF_INIT;
        struct commit_list *commits = NULL, **tail = &commits, *iter;
@@ -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)) {
+                       if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
+                               warning(_("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"));
 
        /*
         * Second phase:
@@ -5334,6 +5343,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
        const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
        int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
        int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;
+       int skipped_commit = 0;
 
        repo_init_revisions(r, &revs, NULL);
        revs.verbose_header = 1;
@@ -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)) {
+                       if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
+                               warning(_("skipped previously applied commit %s"),
+                                       short_commit_name(commit));
+                       skipped_commit = 1;
                        continue;
+               }
                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;
 }
 

diff --git a/sequencer.h b/sequencer.h
index d57d8ea23d7a225e10788859cab4bebf5cfc1346..2088344cb370be514a78b0a6f5e9a1b0aab38777 100644 (file)
--- a/sequencer.h
+++ b/sequencer.h
@@ -156,6 +156,7 @@ int sequencer_remove_state(struct replay_opts *opts);
  */
 #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
 #define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7)
+#define TODO_LIST_WARN_SKIPPED_CHERRY_PICKS (1U << 8)
 
 int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
                          const char **argv, unsigned flags);