From d5bc6f292ab0a1715ff021f5854ac5a81c2b88b0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 27 Apr 2018 22:48:16 +0200 Subject: [PATCH 1/4] rebase -i: demonstrate bugs with fixup!/squash! commit messages When multiple fixup/squash commands are processed and the last one causes merge conflicts and is skipped, we leave the "This is a combination of ..." comments in the commit message. Noticed by Eric Sunshine. This regression test also demonstrates that we rely on the localized version of # This is a combination of commits to contain the in ASCII, which breaks under GETTEXT_POISON. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- t/t3418-rebase-continue.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 7c91a85f43..69cb079dd1 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -88,6 +88,28 @@ test_expect_success 'rebase passes merge strategy options correctly' ' git rebase --continue ' +test_expect_failure '--skip after failed fixup cleans commit message' ' + test_when_finished "test_might_fail git rebase --abort" && + git checkout -b with-conflicting-fixup && + test_commit wants-fixup && + test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 && + test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 && + test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 && + test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \ + git rebase -i HEAD~4 && + + : now there is a conflict, and comments in the commit message && + git show HEAD >out && + grep "fixup! wants-fixup" out && + + : skip and continue && + git rebase --skip && + + : now the comments in the commit message should have been cleaned up && + git show HEAD >out && + ! grep "fixup! wants-fixup" out +' + test_expect_success 'setup rerere database' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F3-on-topic-branch && From e12a7ef5972423c822834f9c514a54e32ea99e65 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 27 Apr 2018 22:48:21 +0200 Subject: [PATCH 2/4] rebase -i: Handle "combination of commits" with GETTEXT_POISON We previously relied on the localized versions of # This is a combination of commits (which we write into the commit messages during fixup/squash chains) to contain encoded in ASCII. This is not true in general, and certainly not true when compiled with GETTEXT_POISON=TryToKillMe, as demonstrated by the regression test we just introduced in t3418. So let's decouple keeping track of the count from the (localized) commit messages by introducing a new file called 'current-fixups' that keeps track of the current fixup/squash chain. This file contains a bit more than just the count (it contains a list of "fixup "/"squash " lines). This is done on purpose, as it will come in handy for a fix for the bug where `git rebase --skip` on a final fixup/squash will leave the commit message in limbo. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 78 ++++++++++++++++++++++++++++++----------------------- sequencer.h | 6 ++++- 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/sequencer.c b/sequencer.c index f9d1001dee..3ce45fce80 100644 --- a/sequencer.c +++ b/sequencer.c @@ -74,13 +74,6 @@ static GIT_PATH_FUNC(rebase_path_message, "rebase-merge/message") * previous commit and from the first squash/fixup commit are written * to it. The commit message for each subsequent squash/fixup commit * is appended to the file as it is processed. - * - * The first line of the file is of the form - * # This is a combination of $count commits. - * where $count is the number of commits whose messages have been - * written to the file so far (including the initial "pick" commit). - * Each time that a commit message is processed, this line is read and - * updated. It is deleted just before the combined commit is made. */ static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash") /* @@ -91,6 +84,11 @@ static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash") * commit without opening the editor.) */ static GIT_PATH_FUNC(rebase_path_fixup_msg, "rebase-merge/message-fixup") +/* + * This file contains the list fixup/squash commands that have been + * accumulated into message-fixup or message-squash so far. + */ +static GIT_PATH_FUNC(rebase_path_current_fixups, "rebase-merge/current-fixups") /* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and * GIT_AUTHOR_DATE that will be used for the commit that is currently @@ -252,6 +250,7 @@ int sequencer_remove_state(struct replay_opts *opts) for (i = 0; i < opts->xopts_nr; i++) free(opts->xopts[i]); free(opts->xopts); + strbuf_release(&opts->current_fixups); strbuf_addstr(&dir, get_dir(opts)); remove_dir_recursively(&dir, 0); @@ -1328,34 +1327,23 @@ static int update_squash_messages(enum todo_command command, struct commit *commit, struct replay_opts *opts) { struct strbuf buf = STRBUF_INIT; - int count, res; + int res; const char *message, *body; - if (file_exists(rebase_path_squash_msg())) { + if (opts->current_fixup_count > 0) { struct strbuf header = STRBUF_INIT; - char *eol, *p; + char *eol; - if (strbuf_read_file(&buf, rebase_path_squash_msg(), 2048) <= 0) + if (strbuf_read_file(&buf, rebase_path_squash_msg(), 9) <= 0) return error(_("could not read '%s'"), rebase_path_squash_msg()); - p = buf.buf + 1; - eol = strchrnul(buf.buf, '\n'); - if (buf.buf[0] != comment_line_char || - (p += strcspn(p, "0123456789\n")) == eol) - return error(_("unexpected 1st line of squash message:" - "\n\n\t%.*s"), - (int)(eol - buf.buf), buf.buf); - count = strtol(p, NULL, 10); - - if (count < 1) - return error(_("invalid 1st line of squash message:\n" - "\n\t%.*s"), - (int)(eol - buf.buf), buf.buf); + eol = buf.buf[0] != comment_line_char ? + buf.buf : strchrnul(buf.buf, '\n'); strbuf_addf(&header, "%c ", comment_line_char); - strbuf_addf(&header, - _("This is a combination of %d commits."), ++count); + strbuf_addf(&header, _("This is a combination of %d commits."), + opts->current_fixup_count + 2); strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len); strbuf_release(&header); } else { @@ -1378,10 +1366,8 @@ static int update_squash_messages(enum todo_command command, rebase_path_fixup_msg()); } - count = 2; strbuf_addf(&buf, "%c ", comment_line_char); - strbuf_addf(&buf, _("This is a combination of %d commits."), - count); + strbuf_addf(&buf, _("This is a combination of %d commits."), 2); strbuf_addf(&buf, "\n%c ", comment_line_char); strbuf_addstr(&buf, _("This is the 1st commit message:")); strbuf_addstr(&buf, "\n\n"); @@ -1398,13 +1384,14 @@ static int update_squash_messages(enum todo_command command, if (command == TODO_SQUASH) { unlink(rebase_path_fixup_msg()); strbuf_addf(&buf, "\n%c ", comment_line_char); - strbuf_addf(&buf, _("This is the commit message #%d:"), count); + strbuf_addf(&buf, _("This is the commit message #%d:"), + ++opts->current_fixup_count); strbuf_addstr(&buf, "\n\n"); strbuf_addstr(&buf, body); } else if (command == TODO_FIXUP) { strbuf_addf(&buf, "\n%c ", comment_line_char); strbuf_addf(&buf, _("The commit message #%d will be skipped:"), - count); + ++opts->current_fixup_count); strbuf_addstr(&buf, "\n\n"); strbuf_add_commented_lines(&buf, body, strlen(body)); } else @@ -1413,6 +1400,17 @@ static int update_squash_messages(enum todo_command command, res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0); strbuf_release(&buf); + + if (!res) { + strbuf_addf(&opts->current_fixups, "%s%s %s", + opts->current_fixups.len ? "\n" : "", + command_to_string(command), + oid_to_hex(&commit->object.oid)); + res = write_message(opts->current_fixups.buf, + opts->current_fixups.len, + rebase_path_current_fixups(), 0); + } + return res; } @@ -1675,6 +1673,9 @@ fast_forward_edit: if (!res && final_fixup) { unlink(rebase_path_fixup_msg()); unlink(rebase_path_squash_msg()); + unlink(rebase_path_current_fixups()); + strbuf_reset(&opts->current_fixups); + opts->current_fixup_count = 0; } leave: @@ -2046,6 +2047,16 @@ static int read_populate_opts(struct replay_opts *opts) read_strategy_opts(opts, &buf); strbuf_release(&buf); + if (read_oneliner(&opts->current_fixups, + rebase_path_current_fixups(), 1)) { + const char *p = opts->current_fixups.buf; + opts->current_fixup_count = 1; + while ((p = strchr(p, '\n'))) { + opts->current_fixup_count++; + p++; + } + } + return 0; } @@ -2392,10 +2403,9 @@ static int error_with_patch(struct commit *commit, static int error_failed_squash(struct commit *commit, struct replay_opts *opts, int subject_len, const char *subject) { - if (rename(rebase_path_squash_msg(), rebase_path_message())) - return error(_("could not rename '%s' to '%s'"), + if (copy_file(rebase_path_message(), rebase_path_squash_msg(), 0666)) + return error(_("could not copy '%s' to '%s'"), rebase_path_squash_msg(), rebase_path_message()); - unlink(rebase_path_fixup_msg()); unlink(git_path_merge_msg()); if (copy_file(git_path_merge_msg(), rebase_path_message(), 0666)) return error(_("could not copy '%s' to '%s'"), diff --git a/sequencer.h b/sequencer.h index e45b178dfc..1898158c52 100644 --- a/sequencer.h +++ b/sequencer.h @@ -44,10 +44,14 @@ struct replay_opts { char **xopts; size_t xopts_nr, xopts_alloc; + /* Used by fixup/squash */ + struct strbuf current_fixups; + int current_fixup_count; + /* Only used by REPLAY_NONE */ struct rev_info *revs; }; -#define REPLAY_OPTS_INIT { -1 } +#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT } /* Call this to setup defaults before parsing command line options */ void sequencer_init_config(struct replay_opts *opts); From dc4b5bc3534bf4d8f362173d905d294048c48f79 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 27 Apr 2018 22:48:28 +0200 Subject: [PATCH 3/4] sequencer: always commit without editing when asked for Previously, we only called run_git_commit() without EDIT_MSG when we also passed in a default message. However, an upcoming caller will want to commit without EDIT_MSG and *without* a default message: to clean up fixup/squash comments in HEAD's commit message. Let's prepare for that. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sequencer.c b/sequencer.c index 3ce45fce80..6b2f01f6d6 100644 --- a/sequencer.c +++ b/sequencer.c @@ -716,6 +716,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign); if (defmsg) argv_array_pushl(&cmd.args, "-F", defmsg, NULL); + else if (!(flags & EDIT_MSG)) + argv_array_pushl(&cmd.args, "-C", "HEAD", NULL); if ((flags & CLEANUP_MSG)) argv_array_push(&cmd.args, "--cleanup=strip"); if ((flags & EDIT_MSG)) From 15ef69314d54945471603223bef00091673b5725 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 27 Apr 2018 22:48:30 +0200 Subject: [PATCH 4/4] rebase --skip: clean up commit message after a failed fixup/squash During a series of fixup/squash commands, the interactive rebase builds up a commit message with comments. This will be presented to the user in the editor if at least one of those commands was a `squash`. In any case, the commit message will be cleaned up eventually, removing all those intermediate comments, in the final step of such a fixup/squash chain. However, if the last fixup/squash command in such a chain fails with merge conflicts, and if the user then decides to skip it (or resolve it to a clean worktree and then continue the rebase), the current code fails to clean up the commit message. This commit fixes that behavior. The fix is quite a bit more involved than meets the eye because it is not only about the question whether we are `git rebase --skip`ing a fixup or squash. It is also about removing the skipped fixup/squash's commit message from the accumulated commit message. And it is also about the question whether we should let the user edit the final commit message or not ("Was there a squash in the chain *that was not skipped*?"). For example, in this case we will want to fix the commit message, but not open it in an editor: pick <- succeeds fixup <- succeeds squash <- fails, will be skipped This is where the newly-introduced `current-fixups` file comes in real handy. A quick look and we can determine whether there was a non-skipped squash. We only need to make sure to keep it up to date with respect to skipped fixup/squash commands. As a bonus, we can even avoid committing unnecessarily, e.g. when there was only one fixup, and it failed, and was skipped. To fix only the bug where the final commit message was not cleaned up properly, but without fixing the rest, would have been more complicated than fixing it all in one go, hence this commit lumps together more than a single concern. For the same reason, this commit also adds a bit more to the existing test case for the regression we just fixed. The diff is best viewed with --color-moved. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 113 ++++++++++++++++++++++++++++++++----- t/t3418-rebase-continue.sh | 35 ++++++++++-- 2 files changed, 131 insertions(+), 17 deletions(-) diff --git a/sequencer.c b/sequencer.c index 6b2f01f6d6..2eb5ec7227 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2773,19 +2773,16 @@ static int continue_single_pick(void) return run_command_v_opt(argv, RUN_GIT_CMD); } -static int commit_staged_changes(struct replay_opts *opts) +static int commit_staged_changes(struct replay_opts *opts, + struct todo_list *todo_list) { unsigned int flags = ALLOW_EMPTY | EDIT_MSG; + unsigned int final_fixup = 0, is_clean; if (has_unstaged_changes(1)) return error(_("cannot rebase: You have unstaged changes.")); - if (!has_uncommitted_changes(0)) { - const char *cherry_pick_head = git_path_cherry_pick_head(); - if (file_exists(cherry_pick_head) && unlink(cherry_pick_head)) - return error(_("could not remove CHERRY_PICK_HEAD")); - return 0; - } + is_clean = !has_uncommitted_changes(0); if (file_exists(rebase_path_amend())) { struct strbuf rev = STRBUF_INIT; @@ -2798,19 +2795,107 @@ static int commit_staged_changes(struct replay_opts *opts) if (get_oid_hex(rev.buf, &to_amend)) return error(_("invalid contents: '%s'"), rebase_path_amend()); - if (oidcmp(&head, &to_amend)) + if (!is_clean && oidcmp(&head, &to_amend)) return error(_("\nYou have uncommitted changes in your " "working tree. Please, commit them\n" "first and then run 'git rebase " "--continue' again.")); + /* + * When skipping a failed fixup/squash, we need to edit the + * commit message, the current fixup list and count, and if it + * was the last fixup/squash in the chain, we need to clean up + * the commit message and if there was a squash, let the user + * edit it. + */ + if (is_clean && !oidcmp(&head, &to_amend) && + opts->current_fixup_count > 0 && + file_exists(rebase_path_stopped_sha())) { + const char *p = opts->current_fixups.buf; + int len = opts->current_fixups.len; + + opts->current_fixup_count--; + if (!len) + BUG("Incorrect current_fixups:\n%s", p); + while (len && p[len - 1] != '\n') + len--; + strbuf_setlen(&opts->current_fixups, len); + if (write_message(p, len, rebase_path_current_fixups(), + 0) < 0) + return error(_("could not write file: '%s'"), + rebase_path_current_fixups()); + + /* + * If a fixup/squash in a fixup/squash chain failed, the + * commit message is already correct, no need to commit + * it again. + * + * Only if it is the final command in the fixup/squash + * chain, and only if the chain is longer than a single + * fixup/squash command (which was just skipped), do we + * actually need to re-commit with a cleaned up commit + * message. + */ + if (opts->current_fixup_count > 0 && + !is_fixup(peek_command(todo_list, 0))) { + final_fixup = 1; + /* + * If there was not a single "squash" in the + * chain, we only need to clean up the commit + * message, no need to bother the user with + * opening the commit message in the editor. + */ + if (!starts_with(p, "squash ") && + !strstr(p, "\nsquash ")) + flags = (flags & ~EDIT_MSG) | CLEANUP_MSG; + } else if (is_fixup(peek_command(todo_list, 0))) { + /* + * We need to update the squash message to skip + * the latest commit message. + */ + struct commit *commit; + const char *path = rebase_path_squash_msg(); + + if (parse_head(&commit) || + !(p = get_commit_buffer(commit, NULL)) || + write_message(p, strlen(p), path, 0)) { + unuse_commit_buffer(commit, p); + return error(_("could not write file: " + "'%s'"), path); + } + unuse_commit_buffer(commit, p); + } + } strbuf_release(&rev); flags |= AMEND_MSG; } - if (run_git_commit(rebase_path_message(), opts, flags)) + if (is_clean) { + const char *cherry_pick_head = git_path_cherry_pick_head(); + + if (file_exists(cherry_pick_head) && unlink(cherry_pick_head)) + return error(_("could not remove CHERRY_PICK_HEAD")); + if (!final_fixup) + return 0; + } + + if (run_git_commit(final_fixup ? NULL : rebase_path_message(), + opts, flags)) return error(_("could not commit staged changes.")); unlink(rebase_path_amend()); + if (final_fixup) { + unlink(rebase_path_fixup_msg()); + unlink(rebase_path_squash_msg()); + } + if (opts->current_fixup_count > 0) { + /* + * Whether final fixup or not, we just cleaned up the commit + * message... + */ + unlink(rebase_path_current_fixups()); + strbuf_reset(&opts->current_fixups); + opts->current_fixup_count = 0; + } return 0; } @@ -2822,14 +2907,16 @@ int sequencer_continue(struct replay_opts *opts) if (read_and_refresh_cache(opts)) return -1; + if (read_populate_opts(opts)) + return -1; if (is_rebase_i(opts)) { - if (commit_staged_changes(opts)) + if ((res = read_populate_todo(&todo_list, opts))) + goto release_todo_list; + if (commit_staged_changes(opts, &todo_list)) return -1; } else if (!file_exists(get_todo_path(opts))) return continue_single_pick(); - if (read_populate_opts(opts)) - return -1; - if ((res = read_populate_todo(&todo_list, opts))) + else if ((res = read_populate_todo(&todo_list, opts))) goto release_todo_list; if (!is_rebase_i(opts)) { diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 69cb079dd1..e9fd029160 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -88,14 +88,14 @@ test_expect_success 'rebase passes merge strategy options correctly' ' git rebase --continue ' -test_expect_failure '--skip after failed fixup cleans commit message' ' +test_expect_success '--skip after failed fixup cleans commit message' ' test_when_finished "test_might_fail git rebase --abort" && git checkout -b with-conflicting-fixup && test_commit wants-fixup && test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 && test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 && test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 && - test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \ + test_must_fail env FAKE_LINES="1 fixup 2 squash 4" \ git rebase -i HEAD~4 && : now there is a conflict, and comments in the commit message && @@ -103,11 +103,38 @@ test_expect_failure '--skip after failed fixup cleans commit message' ' grep "fixup! wants-fixup" out && : skip and continue && - git rebase --skip && + echo "cp \"\$1\" .git/copy.txt" | write_script copy-editor.sh && + (test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) && + + : the user should not have had to edit the commit message && + test_path_is_missing .git/copy.txt && : now the comments in the commit message should have been cleaned up && git show HEAD >out && - ! grep "fixup! wants-fixup" out + ! grep "fixup! wants-fixup" out && + + : now, let us ensure that "squash" is handled correctly && + git reset --hard wants-fixup-3 && + test_must_fail env FAKE_LINES="1 squash 4 squash 2 squash 4" \ + git rebase -i HEAD~4 && + + : the first squash failed, but there are two more in the chain && + (test_set_editor "$PWD/copy-editor.sh" && + test_must_fail git rebase --skip) && + + : not the final squash, no need to edit the commit message && + test_path_is_missing .git/copy.txt && + + : The first squash was skipped, therefore: && + git show HEAD >out && + test_i18ngrep "# This is a combination of 2 commits" out && + + (test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) && + git show HEAD >out && + test_i18ngrep ! "# This is a combination" out && + + : Final squash failed, but there was still a squash && + test_i18ngrep "# This is a combination of 2 commits" .git/copy.txt ' test_expect_success 'setup rerere database' '