Merge branch 'js/rebase-i-clean-msg-after-fixup-continue'
authorJunio C Hamano <gitster@pobox.com>
Wed, 23 May 2018 05:38:18 +0000 (14:38 +0900)
committerJunio C Hamano <gitster@pobox.com>
Wed, 23 May 2018 05:38:18 +0000 (14:38 +0900)
"git rebase -i" sometimes left intermediate "# This is a
combination of N commits" message meant for the human consumption
inside an editor in the final result in certain corner cases, which
has been fixed.

* js/rebase-i-clean-msg-after-fixup-continue:
rebase --skip: clean up commit message after a failed fixup/squash
sequencer: always commit without editing when asked for
rebase -i: Handle "combination of <n> commits" with GETTEXT_POISON
rebase -i: demonstrate bugs with fixup!/squash! commit messages

1  2 
sequencer.c
t/t3418-rebase-continue.sh
diff --combined sequencer.c
index f10033b9dcb7fcd7368bff3c556e9d4b49fbf25f,2eb5ec7227300316f5a192f54edf7818af87f827..6991bcb8a707e674a653de710e9b1e9a228df0c8
@@@ -7,7 -7,7 +7,7 @@@
  #include "sequencer.h"
  #include "tag.h"
  #include "run-command.h"
 -#include "exec_cmd.h"
 +#include "exec-cmd.h"
  #include "utf8.h"
  #include "cache-tree.h"
  #include "diff.h"
@@@ -74,13 -74,6 +74,6 @@@ static GIT_PATH_FUNC(rebase_path_messag
   * 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")
  /*
   * 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
@@@ -127,7 -125,6 +125,7 @@@ static GIT_PATH_FUNC(rebase_path_rewrit
  static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
  static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
  static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
 +static GIT_PATH_FUNC(rebase_path_signoff, "rebase-merge/signoff")
  static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
  static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
  static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
@@@ -253,6 -250,7 +251,7 @@@ int sequencer_remove_state(struct repla
        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);
@@@ -283,7 -281,7 +282,7 @@@ struct commit_message 
  
  static const char *short_commit_name(struct commit *commit)
  {
 -      return find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
 +      return find_unique_abbrev(&commit->object.oid, DEFAULT_ABBREV);
  }
  
  static int get_message(struct commit *commit, struct commit_message *out)
@@@ -500,8 -498,8 +499,8 @@@ static int do_recursive_merge(struct co
        o.show_rename_progress = 1;
  
        head_tree = parse_tree_indirect(head);
 -      next_tree = next ? next->tree : empty_tree();
 -      base_tree = base ? base->tree : empty_tree();
 +      next_tree = next ? get_commit_tree(next) : empty_tree();
 +      base_tree = base ? get_commit_tree(base) : empty_tree();
  
        for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
                parse_merge_opt(&o, *xopt);
@@@ -562,7 -560,7 +561,7 @@@ static int is_index_unchanged(void
                        return error(_("unable to update cache tree"));
  
        return !oidcmp(&active_cache_tree->oid,
 -                     &head_commit->tree->object.oid);
 +                     get_commit_tree_oid(head_commit));
  }
  
  static int write_author_script(const char *message)
@@@ -718,6 -716,8 +717,8 @@@ static int run_git_commit(const char *d
                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))
@@@ -1113,13 -1113,13 +1114,13 @@@ static int try_to_commit(struct strbuf 
                commit_list_insert(current_head, &parents);
        }
  
 -      if (write_cache_as_tree(tree.hash, 0, NULL)) {
 +      if (write_cache_as_tree(&tree, 0, NULL)) {
                res = error(_("git write-tree failed to write a tree"));
                goto out;
        }
  
        if (!(flags & ALLOW_EMPTY) && !oidcmp(current_head ?
 -                                            &current_head->tree->object.oid :
 +                                            get_commit_tree_oid(current_head) :
                                              &empty_tree_oid, &tree)) {
                res = 1; /* run 'git commit' to display error message */
                goto out;
                goto out;
        }
  
 +      reset_ident_date();
 +
        if (commit_tree_extended(msg->buf, msg->len, &tree, parents,
                                 oid, author, opts->gpg_sign, extra)) {
                res = error(_("failed to write commit object"));
@@@ -1219,12 -1217,12 +1220,12 @@@ static int is_original_commit_empty(str
                if (parse_commit(parent))
                        return error(_("could not parse parent commit %s"),
                                oid_to_hex(&parent->object.oid));
 -              ptree_oid = &parent->tree->object.oid;
 +              ptree_oid = get_commit_tree_oid(parent);
        } else {
                ptree_oid = the_hash_algo->empty_tree; /* commit is root */
        }
  
 -      return !oidcmp(ptree_oid, &commit->tree->object.oid);
 +      return !oidcmp(ptree_oid, get_commit_tree_oid(commit));
  }
  
  /*
@@@ -1331,34 -1329,23 +1332,23 @@@ static int update_squash_messages(enum 
                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 {
                                     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");
        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
  
        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;
  }
  
@@@ -1477,7 -1474,7 +1477,7 @@@ static int do_pick_commit(enum todo_com
                 * that represents the "current" state for merge-recursive
                 * to work on.
                 */
 -              if (write_cache_as_tree(head.hash, 0, NULL))
 +              if (write_cache_as_tree(&head, 0, NULL))
                        return error(_("your index file is unmerged."));
        } else {
                unborn = get_oid("HEAD", &head);
                }
        }
  
 -      if (opts->signoff)
 +      if (opts->signoff && !is_fixup(command))
                append_signoff(&msgbuf, 0, 0);
  
        if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
@@@ -1678,6 -1675,9 +1678,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,14 -2046,19 +2049,24 @@@ static int read_populate_opts(struct re
                if (file_exists(rebase_path_verbose()))
                        opts->verbose = 1;
  
 +              if (file_exists(rebase_path_signoff())) {
 +                      opts->allow_ff = 0;
 +                      opts->signoff = 1;
 +              }
 +
                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;
        }
  
@@@ -2400,10 -2405,9 +2413,9 @@@ static int error_with_patch(struct comm
  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'"),
@@@ -2769,19 -2773,16 +2781,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;
                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;
  }
  
@@@ -2818,14 -2907,16 +2915,16 @@@ int sequencer_continue(struct replay_op
        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)) {
@@@ -2884,9 -2975,7 +2983,9 @@@ int sequencer_pick_revisions(struct rep
  
                if (!get_oid(name, &oid)) {
                        if (!lookup_commit_reference_gently(&oid, 1)) {
 -                              enum object_type type = sha1_object_info(oid.hash, NULL);
 +                              enum object_type type = oid_object_info(the_repository,
 +                                                                      &oid,
 +                                                                      NULL);
                                return error(_("%s: can't cherry-pick a %s"),
                                        name, type_name(type));
                        }
@@@ -3010,7 -3099,7 +3109,7 @@@ int sequencer_make_script(FILE *out, in
        init_revisions(&revs, NULL);
        revs.verbose_header = 1;
        revs.max_parents = 1;
 -      revs.cherry_pick = 1;
 +      revs.cherry_mark = 1;
        revs.limited = 1;
        revs.reverse = 1;
        revs.right_only = 1;
                return error(_("make_script: error preparing revisions"));
  
        while ((commit = get_revision(&revs))) {
 +              int is_empty  = is_original_commit_empty(commit);
 +
 +              if (!is_empty && (commit->object.flags & PATCHSAME))
 +                      continue;
                strbuf_reset(&buf);
 -              if (!keep_empty && is_original_commit_empty(commit))
 +              if (!keep_empty && is_empty)
                        strbuf_addf(&buf, "%c ", comment_line_char);
                strbuf_addf(&buf, "%s %s ", insn,
                            oid_to_hex(&commit->object.oid));
index 9214d0bb511e24f401188790858dc1e0ca1b92fd,e9fd029160d62dbce488bfdd597bb75c39fc19e4..03bf1b8a3b3df2e44ed0f70dc176c25af9ac244a
@@@ -24,7 -24,7 +24,7 @@@ test_expect_success 'interactive rebas
        git checkout master &&
  
        FAKE_LINES="edit 1" git rebase -i HEAD^ &&
 -      test-chmtime =-60 F1 &&
 +      test-tool chmtime =-60 F1 &&
        git rebase --continue
  '
  
@@@ -36,7 -36,7 +36,7 @@@ test_expect_success 'non-interactive re
        test_must_fail git rebase --onto master master topic &&
        echo "Resolved" >F2 &&
        git add F2 &&
 -      test-chmtime =-60 F1 &&
 +      test-tool chmtime =-60 F1 &&
        git rebase --continue
  '
  
@@@ -87,6 -87,55 +87,55 @@@ test_expect_success 'rebase passes merg
        test_commit force-change &&
        git rebase --continue
  '
+ 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 squash 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 &&
+       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 &&
+       : 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' '
        rm -fr .git/rebase-* &&