diff --git a/sequencer.c b/sequencer.c index 0fe8fed6c3..d849437236 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2260,10 +2260,17 @@ static const char *reflog_message(struct replay_opts *opts, return buf.buf; } -static int do_pick_commit(struct repository *r, - struct todo_item *item, - struct replay_opts *opts, - int final_fixup, int *check_todo) +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, + struct todo_item *item, + struct replay_opts *opts, + int final_fixup, int *check_todo) { struct replay_ctx *ctx = opts->ctx; unsigned int flags = should_edit(opts) ? EDIT_MSG : 0; @@ -2273,7 +2280,7 @@ static int 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; @@ -2453,14 +2460,25 @@ static int do_pick_commit(struct repository *r, struct commit_list *common = NULL; struct commit_list *remotes = NULL; - res = write_message(ctx->message.buf, ctx->message.len, - git_path_merge_msg(r), 0); + if (write_message(ctx->message.buf, ctx->message.len, + git_path_merge_msg(r), 0)) { + res = -1; + goto leave; + } commit_list_insert(base, &common); commit_list_insert(next, &remotes); - res |= try_merge_command(r, opts->strategy, - opts->xopts.nr, opts->xopts.v, + res = try_merge_command(r, opts->strategy, + opts->xopts.nr, opts->xopts.v, common, oid_to_hex(&head), remotes); + /* + * If there were conflicts, try_merge_command() returns 1, + * any other no-zero return code means that either the merge + * command could not be run, or it failed to merge. + */ + if (res && res != 1) + res = -1; + commit_list_free(common); commit_list_free(remotes); } @@ -2492,7 +2510,6 @@ static int do_pick_commit(struct repository *r, goto leave; } - drop_commit = 0; allow = allow_empty(r, opts, commit); if (allow < 0) { res = allow; @@ -2531,6 +2548,12 @@ fast_forward_edit: res = run_git_commit(NULL, reflog_action, opts, flags); *check_todo = 1; } + /* + * If "git commit" failed to run then res == -1, but we don't + * want reschedule the last command because the picking the + * commit was successful. + */ + res = !!res; } @@ -2547,7 +2570,14 @@ leave: free(author); update_abort_safety_file(); - return res; + if (res < 0) + 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; } static int prepare_revs(struct replay_opts *opts) @@ -3872,7 +3902,7 @@ static int error_failed_squash(struct repository *r, return error(_("could not copy '%s' to '%s'"), rebase_path_message(), git_path_merge_msg(r)); - return error_with_patch(r, commit, subject, subject_len, opts, 1, 0); + return error_with_patch(r, commit, subject, subject_len, opts, 1, 1); } static int do_exec(struct repository *r, const char *command_line, int quiet) @@ -4644,21 +4674,6 @@ static int do_update_refs(struct repository *r, int quiet) return res; } -static int is_final_fixup(struct todo_list *todo_list) -{ - int i = todo_list->current; - - if (!is_fixup(todo_list->items[i].command)) - return 0; - - while (++i < todo_list->nr) - if (is_fixup(todo_list->items[i].command)) - return 0; - else if (!is_noop(todo_list->items[i].command)) - break; - return 1; -} - static enum todo_command peek_command(struct todo_list *todo_list, int offset) { int i; @@ -4942,6 +4957,21 @@ static int reread_todo_if_changed(struct repository *r, return 0; } +static int is_final_fixup(struct todo_list *todo_list) +{ + int i = todo_list->current; + + if (!is_fixup(todo_list->items[i].command)) + return 0; + + while (++i < todo_list->nr) + if (is_fixup(todo_list->items[i].command)) + return 0; + else if (!is_noop(todo_list->items[i].command)) + break; + return 1; +} + static const char rescheduled_advice[] = N_("Could not execute the todo command\n" "\n" @@ -4958,37 +4988,59 @@ static int pick_one_commit(struct repository *r, struct replay_opts *opts, int *check_todo, int* reschedule) { - int res; + enum pick_result pick_res; struct todo_item *item = todo_list->items + todo_list->current; const char *arg = todo_item_get_arg(todo_list, item); - res = do_pick_commit(r, item, opts, is_final_fixup(todo_list), - check_todo); - if (is_rebase_i(opts) && res < 0) { + pick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list), + check_todo); + if (!is_rebase_i(opts)) + switch (pick_res) { + case PICK_RESULT_ERROR: + return -1; + case PICK_RESULT_CONFLICTS: + return 1; + default: + return 0; + } + + if (pick_res == PICK_RESULT_ERROR) { /* Reschedule */ *reschedule = 1; return -1; - } - if (item->command == TODO_EDIT) { + } else if (item->command == TODO_EDIT) { struct commit *commit = item->commit; - if (!res) { + int res = pick_res == PICK_RESULT_CONFLICTS; + int to_amend = pick_res != PICK_RESULT_CONFLICTS && + pick_res != PICK_RESULT_DROPPED; + + /* + * 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); - } - if (is_rebase_i(opts) && !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)); - if (res && is_fixup(item->command)) { - if (res == 1) - intend_to_amend(); + 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, item->arg_len, arg); - } else if (res && is_rebase_i(opts) && item->commit) { + } else if (pick_res == PICK_RESULT_CONFLICTS) { int to_amend = 0; struct object_id oid; @@ -5005,11 +5057,11 @@ static int pick_one_commit(struct repository *r, oideq(&opts->squash_onto, &oid)))) to_amend = 1; - return res | error_with_patch(r, item->commit, - arg, item->arg_len, opts, - res, to_amend); + return error_with_patch(r, item->commit, arg, item->arg_len, + opts, 1, to_amend); } - return res; + + BUG("Unhandled return value from do_pick_commit()"); } static int pick_commits(struct repository *r, @@ -5545,7 +5597,15 @@ static int single_pick(struct repository *r, TODO_PICK : TODO_REVERT; item.commit = cmit; - return do_pick_commit(r, &item, opts, 0, &check_todo); + switch (do_pick_commit(r, &item, opts, 0, &check_todo)) { + case PICK_RESULT_ERROR: + return -1; + case PICK_RESULT_CONFLICTS: + return 1; + default: + return 0; + } + } int sequencer_pick_revisions(struct repository *r, diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index c0c00fbb7b..1d09886ea3 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -270,12 +270,24 @@ test_expect_success 'rebase can copy notes' ' test "a note" = "$(git notes show HEAD)" ' -test_expect_success 'rebase -m can copy notes' ' +test_expect_success 'rebase --apply can copy notes' ' git reset --hard n3 && - git rebase -m --onto n1 n2 && + git rebase --apply --onto n1 n2 && 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/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 58b3bb0c27..297b84e60d 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1251,6 +1251,17 @@ test_expect_success 'interrupted rebase -i with --strategy and -X' ' test $(cat file1) = Z ' +test_expect_success 'failing pick with --strategy is rescheduled' ' + test_when_finished "rm -rf bin; test_might_fail git rebase --abort" && + mkdir bin && + echo exit 2 | write_script bin/git-merge-fail && + git log -1 --format="pick %H # %s" HEAD >expect && + test_must_fail env PATH="$PWD/bin:$PATH" \ + git rebase --no-ff --strategy fail HEAD^ && + test_cmp expect .git/rebase-merge/git-rebase-todo && + test_cmp expect .git/rebase-merge/done +' + test_expect_success 'rebase -i error on commits with \ in message' ' current_head=$(git rev-parse HEAD) && test_when_finished "git rebase --abort; git reset --hard $current_head; rm -f error" && diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh index ad7f8c6f00..51991956d1 100755 --- 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