Merge branch 'js/rebase-i-with-colliding-hash'

"git rebase -i" identifies existing commits in its todo file with
their abbreviated object name, which could become ambigous as it
goes to create new commits, and has a mechanism to avoid ambiguity
in the main part of its execution.  A few other cases however were
not covered by the protection against ambiguity, which has been
corrected.

* js/rebase-i-with-colliding-hash:
  rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
  rebase -i: re-fix short SHA-1 collision
  parse_insn_line(): improve error message when parsing failed

diff --git a/rebase-interactive.c b/rebase-interactive.c
index aa18ae82b724812ea0b2939842395ac78f77bab1..1259adc8ea1afd51f062c18c972ef4acefeef496 100644 (file)
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -104,9 +104,11 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
                                    -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
                return error_errno(_("could not write '%s'"), todo_file);
 
-       if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))
-               return error(_("could not copy '%s' to '%s'."), todo_file,
-                            rebase_path_todo_backup());
+       if (initial &&
+           todo_list_write_to_file(r, todo_list, rebase_path_todo_backup(),
+                                   shortrevisions, shortonto, -1,
+                                   (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
+               return error(_("could not write '%s'."), rebase_path_todo_backup());
 
        if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
                return -2;

diff --git a/sequencer.c b/sequencer.c
index 4d31ec3296b328fd3db1f6e8a0d55bcc4bc8bf93..df6d18f9ab609c1e1e050b509b587495f7957f90 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
        saved = *end_of_object_name;
        *end_of_object_name = '\0';
        status = get_oid(bol, &commit_oid);
+       if (status < 0)
+               error(_("could not parse '%s'"), bol); /* return later */
        *end_of_object_name = saved;
 
        bol = end_of_object_name + strspn(end_of_object_name, " \t");
@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
        item->arg_len = (int)(eol - bol);
 
        if (status < 0)
-               return error(_("could not parse '%.*s'"),
-                            (int)(end_of_object_name - bol), bol);
+               return status;
 
        item->commit = lookup_commit_reference(r, &commit_oid);
-       return !item->commit;
+       return item->commit ? 0 : -1;
 }
 
 int sequencer_get_last_command(struct repository *r, enum replay_action *action)
@@ -5075,7 +5076,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 {
        const char *shortonto, *todo_file = rebase_path_todo();
        struct todo_list new_todo = TODO_LIST_INIT;
-       struct strbuf *buf = &todo_list->buf;
+       struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
        struct object_id oid = onto->object.oid;
        int res;
 
@@ -5127,6 +5128,15 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
                return -1;
        }
 
+       /* Expand the commit IDs */
+       todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
+       strbuf_swap(&new_todo.buf, &buf2);
+       strbuf_release(&buf2);
+       new_todo.total_nr -= new_todo.nr;
+       if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
+               BUG("invalid todo list after expanding IDs:\n%s",
+                   new_todo.buf.buf);
+
        if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {
                todo_list_release(&new_todo);
                return error(_("could not skip unnecessary pick commands"));

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ae6e55ce79ab67320a7dfb34efc6e677456e810d..b90ea0fe440ffff8ae876ce293b10c32262d9ad9 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1264,13 +1264,26 @@ test_expect_success SHA1 'short SHA-1 setup' '
 test_expect_success SHA1 'short SHA-1 collide' '
        test_when_finished "reset_rebase && git checkout master" &&
        git checkout collide &&
+       colliding_sha1=6bcda37 &&
+       test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
        (
                unset test_tick &&
                test_tick &&
                set_fake_editor &&
                FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \
-               FAKE_LINES="reword 1 2" git rebase -i HEAD~2
-       )
+               FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 &&
+               test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
+               grep "^pick $colliding_sha1 " \
+                       .git/rebase-merge/git-rebase-todo.tmp &&
+               grep "^pick [0-9a-f]\{40\}" \
+                       .git/rebase-merge/git-rebase-todo &&
+               grep "^pick [0-9a-f]\{40\}" \
+                       .git/rebase-merge/git-rebase-todo.backup &&
+               git rebase --continue
+       ) &&
+       collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" &&
+       collide3="$(git rev-parse collide3 | cut -c 1-4)" &&
+       test "$collide2" = "$collide3"
 '
 
 test_expect_success 'respect core.abbrev' '