Hi Johannes, sorry it's taken me a while to look at this. I think it
mostly makes sense to me, the code is well documented. I've got one
comment below
On 27/04/18 21:48, Johannes Schindelin wrote:
>
> 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 <[email protected]>
> ---
> 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 56166b0d6c7..cec180714ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2779,19 +2779,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;
> @@ -2804,19 +2801,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);
> + }
I think it should probably recreate the fixup message as well. If there
is a sequence
pick commit
fixup a
fixup b
fixup c
and 'fixup b' gets skipped then when 'fixup c' is applied the user will
be prompted to edit the message unless rebase_path_fixup_msg() exists.
Best Wishes
Phillip
> + 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;
> }
>
> @@ -2828,14 +2913,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 3874f187246..03bf1b8a3b3 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' '
>
