sequencer: do not record dropped commits as rewritten

If a commit gets dropped because its changes are already upstream
then we should not record it as rewritten. As well as confusing any
post-rewrite hooks this means we end up copying the notes from the
dropped commit to the commit that was picked immediately before the
one that was dropped.

While we do not want to record the dropped commit is rewritten, if
it is the final commit in a chain of fixups then we need to flush
the list of rewritten commits. The behavior of an "edit" command
where the commit is dropped is changed so that "rebase --continue"
will not amend the previous pick. However, as the code comment notes
it will still be erroneously recorded as rewritten when the rebase
continues. That will need to be addressed separately along with not
recording skipped commits as rewritten.

The initialization of "drop_commit" is moved to ensure it is initialized
when rewording a fast-forwarded commit.

Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/sequencer.c b/sequencer.c
index ca005b969c460cf574033f5466533ad9d2001558..a85f9e8b77d2c3ce5c7aca791ecf0357ebb250c9 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -2264,6 +2264,7 @@ enum pick_result {
        PICK_RESULT_ERROR = -1,
        PICK_RESULT_OK,
        PICK_RESULT_CONFLICTS,
+       PICK_RESULT_DROPPED,
 };
 
 static enum pick_result do_pick_commit(struct repository *r,
@@ -2279,7 +2280,7 @@ static enum pick_result do_pick_commit(struct repository *r,
        const char *base_label, *next_label, *reflog_action;
        char *author = NULL;
        struct commit_message msg = { NULL, NULL, NULL, NULL };
-       int res, unborn = 0, reword = 0, allow, drop_commit;
+       int res, unborn = 0, reword = 0, allow, drop_commit = 0;
        enum todo_command command = item->command;
        struct commit *commit = item->commit;
 
@@ -2509,7 +2510,6 @@ static enum pick_result do_pick_commit(struct repository *r,
                goto leave;
        }
 
-       drop_commit = 0;
        allow = allow_empty(r, opts, commit);
        if (allow < 0) {
                res = allow;
@@ -2574,6 +2574,8 @@ leave:
                return PICK_RESULT_ERROR;
        else if (res > 0)
                return PICK_RESULT_CONFLICTS;
+       else if (drop_commit)
+               return PICK_RESULT_DROPPED;
        else
                return PICK_RESULT_OK;
 }
@@ -4994,19 +4996,31 @@ static int pick_one_commit(struct repository *r,
        } else if (item->command == TODO_EDIT) {
                struct commit *commit = item->commit;
                int res = pick_res == PICK_RESULT_CONFLICTS;
+               int to_amend = pick_res != PICK_RESULT_CONFLICTS &&
+                               pick_res != PICK_RESULT_DROPPED;
 
-               if (pick_res == PICK_RESULT_OK) {
+               /*
+                * NEEDSWORK: Do not record the commit as rewritten when
+                * continuing if it was dropped. Does it even make sense
+                * to stop if the commit was dropped?
+                */
+               if (pick_res == PICK_RESULT_OK ||
+                   pick_res == PICK_RESULT_DROPPED) {
                        if (!opts->verbose)
                                term_clear_line();
                        fprintf(stderr, _("Stopped at %s...  %.*s\n"),
                                short_commit_name(r, commit), item->arg_len, arg);
                }
-               return error_with_patch(r, commit,
-                                       arg, item->arg_len, opts, res, !res);
+               return error_with_patch(r, commit, arg, item->arg_len, opts,
+                                       res, to_amend);
        } else if (pick_res == PICK_RESULT_OK) {
                record_in_rewritten(&item->commit->object.oid,
                                    peek_command(todo_list, 1));
                return 0;
+       } else if (pick_res == PICK_RESULT_DROPPED) {
+               if (is_final_fixup(todo_list))
+                       flush_rewritten_pending();
+               return 0;
        } else if (pick_res == PICK_RESULT_CONFLICTS &&
                   is_fixup(item->command)) {
                return error_failed_squash(r, item->commit, opts,

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index f0e7fcf649aebc70a08a3a8c7bd324a4cd18e751..1d09886ea35141340dc7cb7230ba29e36926612a 100755 (executable)
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -276,6 +276,18 @@ test_expect_success 'rebase --apply can copy notes' '
        test "a note" = "$(git notes show HEAD)"
 '
 
+test_expect_success 'rebase drops notes of dropped commits' '
+       git checkout n1 &&
+       echo n3 >n3.t &&
+       echo n4 >n4.t &&
+       git add n3.t n4.t &&
+       git commit -m n34 &&
+       git rebase HEAD n3 &&
+       test_commit_message HEAD -m n2 &&
+       test_must_fail git notes list HEAD >actual &&
+       test_must_be_empty actual
+'
+
 test_expect_success 'rebase commit with an ancient timestamp' '
        git reset --hard &&
 

diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index ad7f8c6f00202c5ec844108b14a2c1301c185223..51991956d1d28d7a5a66841da1a06bb3c0e82800 100755 (executable)
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -310,4 +310,27 @@ test_expect_success 'git rebase -i (exec)' '
        verify_hook_input
 '
 
+test_expect_success 'rebase with commits that become empty' '
+       cat >todo <<-\EOF &&
+       pick H
+       pick E
+       fixup I
+       fixup H
+       pick G
+       pick I
+       EOF
+       (
+               set_replace_editor todo &&
+               git rebase -i --empty=drop A A
+       ) &&
+       echo rebase >expected.args &&
+       cat >expected.data <<-EOF &&
+       $(git rev-parse H) $(git rev-parse HEAD~2)
+       $(git rev-parse E) $(git rev-parse HEAD~1)
+       $(git rev-parse I) $(git rev-parse HEAD~1)
+       $(git rev-parse G) $(git rev-parse HEAD)
+       EOF
+       verify_hook_input
+'
+
 test_done