From 36ac861a305720c54f759da814647d5987beb10b Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 6 Sep 2023 15:22:45 +0000 Subject: rebase -i: move unlink() calls At the start of each iteration the loop that picks commits removes the state files from the previous pick. However some of these files are only written if there are conflicts in which case we exit the loop before the end of the loop body. Therefore they only need to be removed when the rebase continues, not at the start of each iteration. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index d2c7698c48..5073ec5902 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4639,6 +4639,10 @@ static int pick_commits(struct repository *r, if (read_and_refresh_cache(r, opts)) return -1; + unlink(rebase_path_message()); + unlink(rebase_path_stopped_sha()); + unlink(rebase_path_amend()); + while (todo_list->current < todo_list->nr) { struct todo_item *item = todo_list->items + todo_list->current; const char *arg = todo_item_get_arg(todo_list, item); @@ -4662,10 +4666,7 @@ static int pick_commits(struct repository *r, todo_list->total_nr, opts->verbose ? "\n" : "\r"); } - unlink(rebase_path_message()); unlink(rebase_path_author_script()); - unlink(rebase_path_stopped_sha()); - unlink(rebase_path_amend()); unlink(git_path_merge_head(r)); unlink(git_path_auto_merge(r)); delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF); -- cgit 1.2.3-korg From 206a78d710853eaed97ab50ba336789e6b3499c7 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 6 Sep 2023 15:22:46 +0000 Subject: rebase -i: remove patch file after conflict resolution When a rebase stops for the user to resolve conflicts it writes a patch for the conflicting commit to .git/rebase-merge/patch. This file has been written since the introduction of "git-rebase-interactive.sh" in 1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25). I assume the idea was to enable the user inspect the conflicting commit in the same way as they could for the patch based rebase. This file should be deleted when the rebase continues as if the rebase stops for a failed "exec" command or a "break" command it is confusing to the user if there is a stale patch lying around from an unrelated command. As the path is now used in two different places rebase_path_patch() is added and used to obtain the path for the patch. To construct the path write_patch() previously used get_dir() which returns different paths depending on whether we're rebasing or cherry-picking/reverting. As this function is only called when rebasing it is safe to use a hard coded string for the directory instead. An assertion is added to make sure we don't starting calling this function when cherry-picking in the future. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 16 ++++++++++++---- t/t3418-rebase-continue.sh | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index 5073ec5902..61c12d4f2b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -132,6 +132,11 @@ static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend") * the commit object name of the corresponding patch. */ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha") +/* + * When we stop for the user to resolve conflicts this file contains + * the patch of the commit that is being picked. + */ +static GIT_PATH_FUNC(rebase_path_patch, "rebase-merge/patch") /* * For the post-rewrite hook, we make a list of rewritten commits and * their new sha1s. The rewritten-pending list keeps the sha1s of @@ -3485,12 +3490,14 @@ static int make_patch(struct repository *r, char hex[GIT_MAX_HEXSZ + 1]; int res = 0; + if (!is_rebase_i(opts)) + BUG("make_patch should only be called when rebasing"); + oid_to_hex_r(hex, &commit->object.oid); if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0) return -1; res |= write_rebase_head(&commit->object.oid); - strbuf_addf(&buf, "%s/patch", get_dir(opts)); memset(&log_tree_opt, 0, sizeof(log_tree_opt)); repo_init_revisions(r, &log_tree_opt, NULL); log_tree_opt.abbrev = 0; @@ -3498,15 +3505,15 @@ static int make_patch(struct repository *r, log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH; log_tree_opt.disable_stdin = 1; log_tree_opt.no_commit_id = 1; - log_tree_opt.diffopt.file = fopen(buf.buf, "w"); + log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w"); log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER; if (!log_tree_opt.diffopt.file) - res |= error_errno(_("could not open '%s'"), buf.buf); + res |= error_errno(_("could not open '%s'"), + rebase_path_patch()); else { res |= log_tree_commit(&log_tree_opt, commit); fclose(log_tree_opt.diffopt.file); } - strbuf_reset(&buf); strbuf_addf(&buf, "%s/message", get_dir(opts)); if (!file_exists(buf.buf)) { @@ -4642,6 +4649,7 @@ static int pick_commits(struct repository *r, unlink(rebase_path_message()); unlink(rebase_path_stopped_sha()); unlink(rebase_path_amend()); + unlink(rebase_path_patch()); while (todo_list->current < todo_list->nr) { struct todo_item *item = todo_list->items + todo_list->current; diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 2d0789e554..261e7cd754 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -244,6 +244,24 @@ test_expect_success 'the todo command "break" works' ' test_path_is_file execed ' +test_expect_success 'patch file is removed before break command' ' + test_when_finished "git rebase --abort" && + cat >todo <<-\EOF && + pick commit-new-file-F2-on-topic-branch + break + EOF + + ( + set_replace_editor todo && + test_must_fail git rebase -i --onto commit-new-file-F2 HEAD + ) && + test_path_is_file .git/rebase-merge/patch && + echo 22>F2 && + git add F2 && + git rebase --continue && + test_path_is_missing .git/rebase-merge/patch +' + test_expect_success '--reschedule-failed-exec' ' test_when_finished "git rebase --abort" && test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ && -- cgit 1.2.3-korg From 9f67899b41e5a95ce31af38c6b6d600433546d9e Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 6 Sep 2023 15:22:47 +0000 Subject: sequencer: use rebase_path_message() Rather than constructing the path in a struct strbuf use the ready made function to get the path name instead. This was the last remaining use of the strbuf so remove it as well. As with the previous patch we now use a hard coded string rather than git_dir() when constructing the path. This is safe for the same reason (make_patch() is only called when rebasing) and is protected by the assertion added in the previous patch. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index 61c12d4f2b..cf45597d4f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3484,7 +3484,6 @@ static int make_patch(struct repository *r, struct commit *commit, struct replay_opts *opts) { - struct strbuf buf = STRBUF_INIT; struct rev_info log_tree_opt; const char *subject; char hex[GIT_MAX_HEXSZ + 1]; @@ -3515,18 +3514,16 @@ static int make_patch(struct repository *r, fclose(log_tree_opt.diffopt.file); } - strbuf_addf(&buf, "%s/message", get_dir(opts)); - if (!file_exists(buf.buf)) { + if (!file_exists(rebase_path_message())) { const char *encoding = get_commit_output_encoding(); const char *commit_buffer = repo_logmsg_reencode(r, commit, NULL, encoding); find_commit_subject(commit_buffer, &subject); - res |= write_message(subject, strlen(subject), buf.buf, 1); + res |= write_message(subject, strlen(subject), rebase_path_message(), 1); repo_unuse_commit_buffer(r, commit, commit_buffer); } - strbuf_release(&buf); release_revisions(&log_tree_opt); return res; -- cgit 1.2.3-korg From f2b5f41eda52416d29fade2f823d3b5bd7aa2205 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 6 Sep 2023 15:22:48 +0000 Subject: sequencer: factor out part of pick_commits() This simplifies the next commit. If a pick fails we now return the error at the end of the loop body rather than returning early, a successful "edit" command continues to return early. There are three things to check to ensure that removing the early return for an error does not change the behavior of the code: (1) We could enter the block guarded by "if (reschedule)". This block is not entered because "reschedlue" is always zero when picking a commit. (2) We could enter the block guarded by "else if (is_rebase_i(opts) && check_todo && !res)". This block is not entered when returning an error because "res" is non-zero in that case. (3) todo_list->current could be incremented before returning. That is avoided by moving the increment which is of course a potential change in behavior itself. The move is safe because none of the callers look at todo_list after this function returns. Moving the increment makes it clear we only want to advance the current item if the command was successful. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 132 ++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 71 insertions(+), 61 deletions(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index cf45597d4f..5b598413e3 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4628,6 +4628,72 @@ N_("Could not execute the todo command\n" " git rebase --edit-todo\n" " git rebase --continue\n"); +static int pick_one_commit(struct repository *r, + struct todo_list *todo_list, + struct replay_opts *opts, + int *check_todo) +{ + int res; + struct todo_item *item = todo_list->items + todo_list->current; + const char *arg = todo_item_get_arg(todo_list, item); + if (is_rebase_i(opts)) + opts->reflog_message = reflog_message( + opts, command_to_string(item->command), NULL); + + res = do_pick_commit(r, item, opts, is_final_fixup(todo_list), + check_todo); + if (is_rebase_i(opts) && res < 0) { + /* Reschedule */ + advise(_(rescheduled_advice), + get_item_line_length(todo_list, todo_list->current), + get_item_line(todo_list, todo_list->current)); + todo_list->current--; + if (save_todo(todo_list, opts)) + return -1; + } + if (item->command == TODO_EDIT) { + struct commit *commit = item->commit; + if (!res) { + if (!opts->verbose) + term_clear_line(); + fprintf(stderr, _("Stopped at %s... %.*s\n"), + short_commit_name(commit), item->arg_len, arg); + } + return error_with_patch(r, commit, + arg, item->arg_len, opts, res, !res); + } + if (is_rebase_i(opts) && !res) + 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 error_failed_squash(r, item->commit, opts, + item->arg_len, arg); + } else if (res && is_rebase_i(opts) && item->commit) { + int to_amend = 0; + struct object_id oid; + + /* + * If we are rewording and have either + * fast-forwarded already, or are about to + * create a new root commit, we want to amend, + * otherwise we do not. + */ + if (item->command == TODO_REWORD && + !repo_get_oid(r, "HEAD", &oid) && + (oideq(&item->commit->object.oid, &oid) || + (opts->have_squash_onto && + 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 res; +} + static int pick_commits(struct repository *r, struct todo_list *todo_list, struct replay_opts *opts) @@ -4683,66 +4749,9 @@ static int pick_commits(struct repository *r, } } if (item->command <= TODO_SQUASH) { - if (is_rebase_i(opts)) - opts->reflog_message = reflog_message(opts, - command_to_string(item->command), NULL); - - res = do_pick_commit(r, item, opts, - is_final_fixup(todo_list), - &check_todo); - if (is_rebase_i(opts) && res < 0) { - /* Reschedule */ - advise(_(rescheduled_advice), - get_item_line_length(todo_list, - todo_list->current), - get_item_line(todo_list, - todo_list->current)); - todo_list->current--; - if (save_todo(todo_list, opts)) - return -1; - } - if (item->command == TODO_EDIT) { - struct commit *commit = item->commit; - if (!res) { - if (!opts->verbose) - term_clear_line(); - fprintf(stderr, - _("Stopped at %s... %.*s\n"), - short_commit_name(commit), - item->arg_len, arg); - } - return error_with_patch(r, commit, - arg, item->arg_len, opts, res, !res); - } - if (is_rebase_i(opts) && !res) - 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 error_failed_squash(r, item->commit, opts, - item->arg_len, arg); - } else if (res && is_rebase_i(opts) && item->commit) { - int to_amend = 0; - struct object_id oid; - - /* - * If we are rewording and have either - * fast-forwarded already, or are about to - * create a new root commit, we want to amend, - * otherwise we do not. - */ - if (item->command == TODO_REWORD && - !repo_get_oid(r, "HEAD", &oid) && - (oideq(&item->commit->object.oid, &oid) || - (opts->have_squash_onto && - oideq(&opts->squash_onto, &oid)))) - to_amend = 1; - - return res | error_with_patch(r, item->commit, - arg, item->arg_len, opts, - res, to_amend); - } + res = pick_one_commit(r, todo_list, opts, &check_todo); + if (!res && item->command == TODO_EDIT) + return 0; } else if (item->command == TODO_EXEC) { char *end_of_arg = (char *)(arg + item->arg_len); int saved = *end_of_arg; @@ -4803,9 +4812,10 @@ static int pick_commits(struct repository *r, return -1; } - todo_list->current++; if (res) return res; + + todo_list->current++; } if (is_rebase_i(opts)) { -- cgit 1.2.3-korg From e032abd5a0bec40a700527f71073f3ba21fce7d6 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 6 Sep 2023 15:22:49 +0000 Subject: rebase: fix rewritten list for failed pick git rebase keeps a list that maps the OID of each commit before it was rebased to the OID of the equivalent commit after the rebase. This list is used to drive the "post-rewrite" hook that is called at the end of a successful rebase. When a rebase stops for the user to resolve merge conflicts the OID of the commit being picked is written to ".git/rebase-merge/stopped-sha". Then when the rebase is continued that OID is added to the list of rewritten commits. Unfortunately if a commit cannot be picked because it would overwrite an untracked file we still write the "stopped-sha1" file. This means that when the rebase is continued the commit is added into the list of rewritten commits even though it has not been picked yet. Fix this by not calling error_with_patch() for failed commands. The pick has failed so there is nothing to commit and therefore we do not want to set up the state files for committing staged changes when the rebase continues. This change means we no-longer write a patch for the failed command or display the error message printed by error_with_patch(). As the command has failed the patch isn't really useful and in any case the user can inspect the commit associated with the failed command by inspecting REBASE_HEAD. Unless the user has disabled it we already print an advice message that is more helpful than the message from error_with_patch() which the user will still see. Even if the advice is disabled the user will see the messages from the merge machinery detailing the problem. The code to add a failed command back into the todo list is duplicated between pick_one_commit() and the loop in pick_commits(). Both sites print advice about the command being rescheduled, decrement the current item and save the todo list. To avoid duplicating this code pick_one_commit() is modified to set a flag to indicate that the command should be rescheduled in the main loop. This simplifies things as only the remaining copy of the code needs to be modified to set REBASE_HEAD rather than calling error_with_patch(). Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 19 +++++++---------- t/t3404-rebase-interactive.sh | 6 ++++++ t/t3430-rebase-merges.sh | 4 ++-- t/t5407-post-rewrite-hook.sh | 48 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 14 deletions(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index 5b598413e3..575464358c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4141,6 +4141,7 @@ static int do_merge(struct repository *r, if (ret < 0) { error(_("could not even attempt to merge '%.*s'"), merge_arg_len, arg); + unlink(git_path_merge_msg(r)); goto leave_merge; } /* @@ -4631,7 +4632,7 @@ N_("Could not execute the todo command\n" static int pick_one_commit(struct repository *r, struct todo_list *todo_list, struct replay_opts *opts, - int *check_todo) + int *check_todo, int* reschedule) { int res; struct todo_item *item = todo_list->items + todo_list->current; @@ -4644,12 +4645,8 @@ static int pick_one_commit(struct repository *r, check_todo); if (is_rebase_i(opts) && res < 0) { /* Reschedule */ - advise(_(rescheduled_advice), - get_item_line_length(todo_list, todo_list->current), - get_item_line(todo_list, todo_list->current)); - todo_list->current--; - if (save_todo(todo_list, opts)) - return -1; + *reschedule = 1; + return -1; } if (item->command == TODO_EDIT) { struct commit *commit = item->commit; @@ -4749,7 +4746,8 @@ static int pick_commits(struct repository *r, } } if (item->command <= TODO_SQUASH) { - res = pick_one_commit(r, todo_list, opts, &check_todo); + res = pick_one_commit(r, todo_list, opts, &check_todo, + &reschedule); if (!res && item->command == TODO_EDIT) return 0; } else if (item->command == TODO_EXEC) { @@ -4803,10 +4801,7 @@ static int pick_commits(struct repository *r, if (save_todo(todo_list, opts)) return -1; if (item->commit) - return error_with_patch(r, - item->commit, - arg, item->arg_len, - opts, res, 0); + write_rebase_head(&item->commit->object.oid); } else if (is_rebase_i(opts) && check_todo && !res && reread_todo_if_changed(r, todo_list, opts)) { return -1; diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index ff0afad63e..6d3788c588 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1287,7 +1287,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' >file6 && test_must_fail git rebase --continue && test_cmp_rev HEAD F && + test_cmp_rev REBASE_HEAD I && rm file6 && + test_path_is_missing .git/rebase-merge/patch && git rebase --continue && test_cmp_rev HEAD I ' @@ -1305,7 +1307,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)' >file6 && test_must_fail git rebase --continue && test_cmp_rev HEAD F && + test_cmp_rev REBASE_HEAD I && rm file6 && + test_path_is_missing .git/rebase-merge/patch && git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = I && git reset --hard original-branch2 @@ -1323,7 +1327,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' >file6 && test_must_fail git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = F && + test_cmp_rev REBASE_HEAD I && rm file6 && + test_path_is_missing .git/rebase-merge/patch && git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = I ' diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index f03599c63b..5514738abd 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' ' test_config sequence.editor \""$PWD"/replace-editor.sh\" && test_tick && test_must_fail git rebase -ir HEAD && + test_cmp_rev REBASE_HEAD H^0 && grep "^merge -C .* G$" .git/rebase-merge/done && grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && - test_path_is_file .git/rebase-merge/patch && + test_path_is_missing .git/rebase-merge/patch && : fail because of merge conflict && - rm G.t .git/rebase-merge/patch && git reset --hard conflicting-G && test_must_fail git rebase --continue && ! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh index 5f3ff051ca..ad7f8c6f00 100755 --- a/t/t5407-post-rewrite-hook.sh +++ b/t/t5407-post-rewrite-hook.sh @@ -17,6 +17,12 @@ test_expect_success 'setup' ' git checkout A^0 && test_commit E bar E && test_commit F foo F && + git checkout B && + git merge E && + git tag merge-E && + test_commit G G && + test_commit H H && + test_commit I I && git checkout main && test_hook --setup post-rewrite <<-EOF @@ -173,6 +179,48 @@ test_fail_interactive_rebase () { ) } +test_expect_success 'git rebase with failed pick' ' + clear_hook_input && + cat >todo <<-\EOF && + exec >bar + merge -C merge-E E + exec >G + pick G + exec >H 2>I + pick H + fixup I + EOF + + ( + set_replace_editor todo && + test_must_fail git rebase -i D D 2>err + ) && + grep "would be overwritten" err && + rm bar && + + test_must_fail git rebase --continue 2>err && + grep "would be overwritten" err && + rm G && + + test_must_fail git rebase --continue 2>err && + grep "would be overwritten" err && + rm H && + + test_must_fail git rebase --continue 2>err && + grep "would be overwritten" err && + rm I && + + git rebase --continue && + echo rebase >expected.args && + cat >expected.data <<-EOF && + $(git rev-parse merge-E) $(git rev-parse HEAD~2) + $(git rev-parse G) $(git rev-parse HEAD~1) + $(git rev-parse H) $(git rev-parse HEAD) + $(git rev-parse I) $(git rev-parse HEAD) + EOF + verify_hook_input +' + test_expect_success 'git rebase -i (unchanged)' ' git reset --hard D && clear_hook_input && -- cgit 1.2.3-korg From 405509cbd66b2149fcce6e71c0eb0704c241a826 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 6 Sep 2023 15:22:50 +0000 Subject: rebase --continue: refuse to commit after failed command If a commit cannot be picked because it would overwrite an untracked file then "git rebase --continue" should refuse to commit any staged changes as the commit was not picked. This is implemented by refusing to commit if the message file is missing. The message file is chosen for this check because it is only written when "git rebase" stops for the user to resolve merge conflicts. Existing commands that refuse to commit staged changes when continuing such as a failed "exec" rely on checking for the absence of the author script in run_git_commit(). This prevents the staged changes from being committed but prints error: could not open '.git/rebase-merge/author-script' for reading before the message about not being able to commit. This is confusing to users and so checking for the message file instead improves the user experience. The existing test for refusing to commit after a failed exec is updated to check that we do not print the error message about a missing author script anymore. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 5 +++++ t/t3404-rebase-interactive.sh | 18 +++++++++++++++++- t/t3430-rebase-merges.sh | 4 ++++ 3 files changed, 26 insertions(+), 1 deletion(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index 575464358c..f9ef301d63 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4963,6 +4963,11 @@ static int commit_staged_changes(struct repository *r, is_clean = !has_uncommitted_changes(r, 0); + if (!is_clean && !file_exists(rebase_path_message())) { + const char *gpg_opt = gpg_sign_opt_quoted(opts); + + return error(_(staged_changes_advice), gpg_opt, gpg_opt); + } if (file_exists(rebase_path_amend())) { struct strbuf rev = STRBUF_INIT; struct object_id head, to_amend; diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 6d3788c588..a8ad398956 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -604,7 +604,8 @@ test_expect_success 'clean error after failed "exec"' ' echo "edited again" > file7 && git add file7 && test_must_fail git rebase --continue 2>error && - test_i18ngrep "you have staged changes in your working tree" error + test_i18ngrep "you have staged changes in your working tree" error && + test_i18ngrep ! "could not open.*for reading" error ' test_expect_success 'rebase a detached HEAD' ' @@ -1290,6 +1291,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' test_cmp_rev REBASE_HEAD I && rm file6 && test_path_is_missing .git/rebase-merge/patch && + echo changed >file1 && + git add file1 && + test_must_fail git rebase --continue 2>err && + grep "error: you have staged changes in your working tree" err && + git reset --hard HEAD && git rebase --continue && test_cmp_rev HEAD I ' @@ -1310,6 +1316,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)' test_cmp_rev REBASE_HEAD I && rm file6 && test_path_is_missing .git/rebase-merge/patch && + echo changed >file1 && + git add file1 && + test_must_fail git rebase --continue 2>err && + grep "error: you have staged changes in your working tree" err && + git reset --hard HEAD && git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = I && git reset --hard original-branch2 @@ -1330,6 +1341,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' test_cmp_rev REBASE_HEAD I && rm file6 && test_path_is_missing .git/rebase-merge/patch && + echo changed >file1 && + git add file1 && + test_must_fail git rebase --continue 2>err && + grep "error: you have staged changes in your working tree" err && + git reset --hard HEAD && git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = I ' diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 5514738abd..bf75080281 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -169,6 +169,10 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' ' grep "^merge -C .* G$" .git/rebase-merge/done && grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && test_path_is_missing .git/rebase-merge/patch && + echo changed >file1 && + git add file1 && + test_must_fail git rebase --continue 2>err && + grep "error: you have staged changes in your working tree" err && : fail because of merge conflict && git reset --hard conflicting-G && -- cgit 1.2.3-korg From 203573b024610d7e50cc83a6d06867880ed8cc4f Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 6 Sep 2023 15:22:51 +0000 Subject: rebase -i: fix adding failed command to the todo list When rebasing commands are moved from the todo list in "git-rebase-todo" to the "done" file (which is used by "git status" to show the recently executed commands) just before they are executed. This means that if a command fails because it would overwrite an untracked file it has to be added back into the todo list before the rebase stops for the user to fix the problem. Unfortunately when a failed command is added back into the todo list the command preceding it is erroneously appended to the "done" file. This means that when rebase stops after "pick B" fails the "done" file contains pick A pick B pick A instead of pick A pick B This happens because save_todo() updates the "done" file with the previous command whenever "git-rebase-todo" is updated. When we add the failed pick back into "git-rebase-todo" we do not want to update "done". Fix this by adding a "reschedule" parameter to save_todo() which prevents the "done" file from being updated when adding a failed command back into the "git-rebase-todo" file. A couple of the existing tests are modified to improve their coverage as none of them trigger this bug or check the "done" file. Reported-by: Stefan Haller Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 12 ++++++------ t/t3404-rebase-interactive.sh | 29 ++++++++++++++++++----------- t/t3430-rebase-merges.sh | 22 ++++++++++++++++------ 3 files changed, 40 insertions(+), 23 deletions(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index f9ef301d63..f014c60cc8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3380,7 +3380,8 @@ give_advice: return -1; } -static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts, + int reschedule) { struct lock_file todo_lock = LOCK_INIT; const char *todo_path = get_todo_path(opts); @@ -3390,7 +3391,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) * rebase -i writes "git-rebase-todo" without the currently executing * command, appending it to "done" instead. */ - if (is_rebase_i(opts)) + if (is_rebase_i(opts) && !reschedule) next++; fd = hold_lock_file_for_update(&todo_lock, todo_path, 0); @@ -3403,7 +3404,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) if (commit_lock_file(&todo_lock) < 0) return error(_("failed to finalize '%s'"), todo_path); - if (is_rebase_i(opts) && next > 0) { + if (is_rebase_i(opts) && !reschedule && next > 0) { const char *done = rebase_path_done(); int fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666); int ret = 0; @@ -4716,7 +4717,7 @@ static int pick_commits(struct repository *r, const char *arg = todo_item_get_arg(todo_list, item); int check_todo = 0; - if (save_todo(todo_list, opts)) + if (save_todo(todo_list, opts, reschedule)) return -1; if (is_rebase_i(opts)) { if (item->command != TODO_COMMENT) { @@ -4797,8 +4798,7 @@ static int pick_commits(struct repository *r, get_item_line_length(todo_list, todo_list->current), get_item_line(todo_list, todo_list->current)); - todo_list->current--; - if (save_todo(todo_list, opts)) + if (save_todo(todo_list, opts, reschedule)) return -1; if (item->commit) write_rebase_head(&item->commit->object.oid); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index a8ad398956..71da9c465a 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1277,19 +1277,24 @@ test_expect_success 'todo count' ' ' test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' - git checkout --force branch2 && + git checkout --force A && git clean -f && + cat >todo <<-EOF && + exec >file2 + pick $(git rev-parse B) B + pick $(git rev-parse C) C + pick $(git rev-parse D) D + exec cat .git/rebase-merge/done >actual + EOF ( - set_fake_editor && - FAKE_LINES="edit 1 2" git rebase -i A + set_replace_editor todo && + test_must_fail git rebase -i A ) && - test_cmp_rev HEAD F && - test_path_is_missing file6 && - >file6 && - test_must_fail git rebase --continue && - test_cmp_rev HEAD F && - test_cmp_rev REBASE_HEAD I && - rm file6 && + test_cmp_rev HEAD B && + test_cmp_rev REBASE_HEAD C && + head -n3 todo >expect && + test_cmp expect .git/rebase-merge/done && + rm file2 && test_path_is_missing .git/rebase-merge/patch && echo changed >file1 && git add file1 && @@ -1297,7 +1302,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' grep "error: you have staged changes in your working tree" err && git reset --hard HEAD && git rebase --continue && - test_cmp_rev HEAD I + test_cmp_rev HEAD D && + tail -n3 todo >>expect && + test_cmp expect actual ' test_expect_success 'rebase -i commits that overwrite untracked files (squash)' ' diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index bf75080281..2668be6a4b 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -128,14 +128,24 @@ test_expect_success 'generate correct todo list' ' ' test_expect_success '`reset` refuses to overwrite untracked files' ' - git checkout -b refuse-to-reset && + git checkout B && test_commit dont-overwrite-untracked && - git checkout @{-1} && - : >dont-overwrite-untracked.t && - echo "reset refs/tags/dont-overwrite-untracked" >script-from-scratch && + cat >script-from-scratch <<-EOF && + exec >dont-overwrite-untracked.t + pick $(git rev-parse B) B + reset refs/tags/dont-overwrite-untracked + pick $(git rev-parse C) C + exec cat .git/rebase-merge/done >actual + EOF test_config sequence.editor \""$PWD"/replace-editor.sh\" && - test_must_fail git rebase -ir HEAD && - git rebase --abort + test_must_fail git rebase -ir A && + test_cmp_rev HEAD B && + head -n3 script-from-scratch >expect && + test_cmp expect .git/rebase-merge/done && + rm dont-overwrite-untracked.t && + git rebase --continue && + tail -n3 script-from-scratch >>expect && + test_cmp expect actual ' test_expect_success '`reset` rejects trees' ' -- cgit 1.2.3-korg