rebase -i: always update HEAD before rewording
PATCH 1/3 rebase -i: always update HEAD before rewording
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
If the user runs git log while rewording a commit it is confusing if sometimes we're amending the commit that's being reworded and at other times we're creating a new commit depending on whether we could fast-forward or not[1]. Fix this inconsistency by always committing the picked commit and then running 'git commit --amend' to do the reword.
The first commit is performed by the sequencer without forking git commit and does not impact on the speed of rebase. In a test rewording 100 commits with
GIT_EDITOR=true GIT_SEQUENCE_EDITOR='sed -i s/pick/reword/' \ ../bin-wrappers/git rebase -i --root
and taking the best of three runs the current master took 957ms and with this patch it took 961ms.
This change fixes rewording the new root commit when rearranging commits with --root.
Note that the new code no longer updates CHERRY_PICK_HEAD after creating a root commit - I'm not sure why the old code was that creating that ref after a successful commit, everywhere else it is removed after a successful commit.
[1] https://public-inbox.org/git/[email protected]/T/#m133009cb91cf0917bcf667300f061178be56680a
--- sequencer.c | 19 +++++++++++-------- t/t3404-rebase-interactive.sh | 16 +++++++++++++--- t/t7505-prepare-commit-msg-hook.sh | 8 +------- t/t7505/expected-rebase-i | 3 ++- 4 files changed, 27 insertions(+), 19 deletions(-)
sequencer.c +11 -8
@@ -986,12 +986,10 @@ static int run_git_commit(struct repository *r,strbuf_release (&msg);strbuf_release (&script); -if (!res) { -update_ref (NULL ,"CHERRY_PICK_HEAD" , &root_commit,NULL , -REF_NO_DEREF ,UPDATE_REFS_MSG_ON_ERR ); +if (!res) res =update_ref (NULL ,"HEAD" , &root_commit,NULL ,0 ,UPDATE_REFS_MSG_ON_ERR ); - } +return res <0 ?error (_ ("writing root commit" )) :0 ; }
@@ -1785,7 +1783,7 @@ static int do_pick_commit(struct repository *r,char *author =NULL ;struct commit_message msg = {NULL ,NULL ,NULL ,NULL };struct strbuf msgbuf =STRBUF_INIT ; -int res, unborn =0 , allow; +int res, unborn =0 , reword =0 , allow;if (opts->no_commit) {/*
@@ -1855,7 +1853,7 @@ static int do_pick_commit(struct repository *r, opts);if (res || command !=TODO_REWORD ) goto leave; - flags |=EDIT_MSG |AMEND_MSG |VERIFY_MSG ; + reword =1 ; msg_file =NULL ; goto fast_forward_edit; }
@@ -1913,7 +1911,7 @@ static int do_pick_commit(struct repository *r, }if (command ==TODO_REWORD ) - flags |=EDIT_MSG |VERIFY_MSG ; + reword =1 ;else if (is_fixup (command)) {if (update_squash_messages (r, command, commit, opts))return -1 ;
@@ -1997,13 +1995,18 @@ static int do_pick_commit(struct repository *r, }else if (allow) flags |=ALLOW_EMPTY ;if (!opts->no_commit) { -fast_forward_edit:if (author || command ==TODO_REVERT || (flags &AMEND_MSG )) res =do_commit (r, msg_file, author, opts, flags);else res =error (_ ("unable to parse commit author" )); +if (!res && reword) +fast_forward_edit: + res =run_git_commit (r,NULL , opts,EDIT_MSG | +VERIFY_MSG |AMEND_MSG | + (flags &ALLOW_EMPTY )); } +if (!res && final_fixup) {unlink (rebase_path_fixup_msg ());unlink (rebase_path_squash_msg ());
t/t3404-rebase-interactive.sh +13 -3
@@ -1016,9 +1016,9 @@ test_expect_success 'rebase -i --root fixup root commit' ' test 0 = $(git cat-file commit HEAD | grep -c ^parent\ ) ' -test_expect_success 'rebase -i --root reword root commit' ' +test_expect_success 'rebase -i --root reword original root commit' ' test_when_finished "test_might_fail git rebase --abort" && - git checkout -b reword-root-branch master && + git checkout -b reword-original-root-branch master && set_fake_editor && FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \ git rebase -i --root &&
@@ -1026,6 +1026,16 @@ test_expect_success 'rebase -i --root reword root commit' ' test -z "$(git show -s --format=%p HEAD^)" ' +test_expect_success 'rebase -i --root reword new root commit' ' + test_when_finished "test_might_fail git rebase --abort" && + git checkout -b reword-now-root-branch master && + set_fake_editor && + FAKE_LINES="reword 3 1" FAKE_COMMIT_MESSAGE="C changed" \ + git rebase -i --root && + git show HEAD^ | grep "C changed" && + test -z "$(git show -s --format=%p HEAD^)" +' + test_expect_success 'rebase -i --root when root has untracked file conflict' ' test_when_finished "reset_rebase" && git checkout -b failing-root-pick A &&
@@ -1054,7 +1064,7 @@ test_expect_success 'rebase -i --root reword root when root has untracked file c ' test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-interactive rebase' ' - git checkout reword-root-branch && + git checkout reword-original-root-branch && git reset --hard && git checkout conflict-branch && set_fake_editor &&
t/t7505-prepare-commit-msg-hook.sh +1 -7
@@ -241,13 +241,7 @@ test_rebase () { git add b && git rebase --continue ) && - if test "$mode" = -p # reword amended after pick - then - n=18 - else - n=17 - fi && - git log --pretty=%s -g -n$n HEAD@{1} >actual && + git log --pretty=%s -g -n18 HEAD@{1} >actual && test_cmp "$TEST_DIRECTORY/t7505/expected-rebase${mode:--i}" actual ' }
t/t7505/expected-rebase-i +2 -1
@@ -7,7 +7,8 @@ message (no editor) [edit rebase-10] message [fixup rebase-9] message (no editor) [fixup rebase-8] message (no editor) [squash rebase-7] -message [reword rebase-6] +HEAD [reword rebase-6] +message (no editor) [reword rebase-6] message [squash rebase-5] message (no editor) [fixup rebase-4] message (no editor) [pick rebase-3]
Signature · 1 line
gitgitgadget
cover rebase -i: always update HEAD before rewording
This series contains a couple of patches to make the C version of rebase --interactive behave more like the scripted version. The third patch is not strictly related to the first two but is included here to avoid merge conflicts.
Phillip Wood (3):
rebase -i: always update HEAD before rewording rebase -i: check for updated todo after squash and reword
sequencer: simplify root commit creation
sequencer.c | 130 +++++++++-------------------- t/t3404-rebase-interactive.sh | 16 +++- t/t3429-rebase-edit-todo.sh | 21 ++++- t/t7505-prepare-commit-msg-hook.sh | 8 +- t/t7505/expected-rebase-i | 3 +- 5 files changed, 75 insertions(+), 103 deletions(-)
base-commit: ff66981f4593aec0f3b3eeace0eacb7dbe44fd8c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-312%2Fphillipwood%2Fwip%2Frebase-reword-update-head-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-312/phillipwood/wip/rebase-reword-update-head-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/312
Signature · 1 line
gitgitgadget
PATCH 2/3 rebase -i: check for updated todo after squash and reword
From: Phillip Wood <[email protected]>
While a rebase is stopped for the user to edit a commit message it can be convenient for them to also edit the todo list. The scripted version of rebase supported this but the C version does not. We already check to see if the todo list has been updated by an exec command so extend this to rewords and squashes. It only costs a single stat call to do this so it should not affect the speed of the rebase (especially as it has just stopped for the user to edit a message)
Note that for squashes the editor may be opened on a different pick to the squash itself as we edit the message at the end of a chain fixups and squashes.
--- sequencer.c | 42 ++++++++++++++++++++++++------------- t/t3429-rebase-edit-todo.sh | 21 ++++++++++++++++++- 2 files changed, 47 insertions(+), 16 deletions(-)
sequencer.c +27 -15
@@ -1773,7 +1773,7 @@ static int do_pick_commit(struct repository *r,enum todo_command command, struct commit *commit, struct replay_opts *opts, - int final_fixup) + int final_fixup, int *check_todo) { unsigned int flags = opts->edit ? EDIT_MSG : 0;const char *msg_file = opts->edit ?NULL :git_path_merge_msg (r);
@@ -1999,11 +1999,14 @@ static int do_pick_commit(struct repository *r, res =do_commit (r, msg_file, author, opts, flags); else res =error (_ ("unable to parse commit author" )); -if (!res && reword) + *check_todo = !!(flags &EDIT_MSG ); +if (!res && reword) { fast_forward_edit: res =run_git_commit (r,NULL , opts,EDIT_MSG |VERIFY_MSG |AMEND_MSG | (flags &ALLOW_EMPTY )); + *check_todo =1 ; + } }
@@ -3821,6 +3824,7 @@ static int pick_commits(struct repository *r,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); +int check_todo =0 ;if (save_todo (todo_list, opts))return -1 ;
@@ -3859,7 +3863,8 @@ static int pick_commits(struct repository *r, command_to_string(item->command), NULL), 1); res =do_pick_commit (r, item->command, item->commit, - opts,is_final_fixup (todo_list)); + opts,is_final_fixup (todo_list), + &check_todo);if (is_rebase_i (opts) && res <0 ) {/* Reschedule */ advise(_(rescheduled_advice),
@@ -3916,7 +3921,6 @@ static int pick_commits(struct repository *r, }else if (item->command ==TODO_EXEC ) {char *end_of_arg = (char *)(arg + item->arg_len);int saved = *end_of_arg; -struct stat st;if (!opts->verbose)term_clear_line ();
@@ -3927,17 +3931,8 @@ static int pick_commits(struct repository *r,if (res) {if (opts->reschedule_failed_exec) reschedule =1 ; - }else if (stat (get_todo_path (opts), &st)) - res =error_errno (_ ("could not stat '%s'" ), -get_todo_path (opts)); -else if (match_stat_data (&todo_list->stat, &st)) { -/* Reread the todo file if it has changed. */ -todo_list_release (todo_list); -if (read_populate_todo (r, todo_list, opts)) - res =-1 ;/* message was printed */ -/* `current` will be incremented below */ - todo_list->current =-1 ; } + check_todo =1 ; }else if (item->command ==TODO_LABEL ) {if ((res =do_label (r, arg, item->arg_len))) reschedule =1 ;
@@ -3973,6 +3968,20 @@ static int pick_commits(struct repository *r, item->commit, arg, item->arg_len, opts, res, 0); + }else if (check_todo && !res) { +struct stat st; + +if (stat (get_todo_path (opts), &st)) { + res =error_errno (_ ("could not stat '%s'" ), +get_todo_path (opts)); + }else if (match_stat_data (&todo_list->stat, &st)) { +/* Reread the todo file if it has changed. */ +todo_list_release (todo_list); +if (read_populate_todo (r, todo_list, opts)) + res =-1 ;/* message was printed */ +/* `current` will be incremented below */ + todo_list->current =-1 ; + } } todo_list->current++;
@@ -4299,9 +4308,12 @@ static int single_pick(struct repository *r,struct commit *cmit, struct replay_opts *opts) { + int check_todo; +setenv (GIT_REFLOG_ACTION ,action_name (opts),0 );return do_pick_commit (r, opts->action ==REPLAY_PICK ? -TODO_PICK :TODO_REVERT , cmit, opts,0 ); +TODO_PICK :TODO_REVERT , cmit, opts,0 , + &check_todo); } int sequencer_pick_revisions(struct repository *r,
t/t3429-rebase-edit-todo.sh +20 -1
@@ -3,9 +3,15 @@ test_description='rebase should reread the todo file if an exec modifies it' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-rebase.sh + +test_expect_success 'setup' ' + test_commit first file && + test_commit second file && + test_commit third file +' test_expect_success 'rebase exec modifies rebase-todo' ' - test_commit initial && todo=.git/rebase-merge/git-rebase-todo && git rebase HEAD -x "echo exec touch F >>$todo" && test -e F
@@ -33,4 +39,17 @@ test_expect_success SHA1 'loose object cache vs re-reading todo list' ' git rebase HEAD -x "./append-todo.sh 5 6" ' +test_expect_success 'todo is re-read after reword and squash' ' + write_script reword-editor.sh <<-\EOS && + GIT_SEQUENCE_EDITOR="echo \"exec echo $(cat file) >>actual\" >>" \ + git rebase --edit-todo + EOS + + test_write_lines first third >expected && + set_fake_editor && + GIT_SEQUENCE_EDITOR="$EDITOR" FAKE_LINES="reword 1 squash 2 fixup 3" \ + GIT_EDITOR=./reword-editor.sh git rebase -i --root third && + test_cmp expected actual +' + test_done
Signature · 1 line
gitgitgadget
PATCH 3/3 sequencer: simplify root commit creation
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Adapt try_to_commit() to create a new root commit rather than special casing this in run_git_commit(). The significantly reduces the amount of special case code for creating the root commit and reduces the number of commit code paths we have to worry about.
--- sequencer.c | 75 +++-------------------------------------------------- 1 file changed, 4 insertions(+), 71 deletions(-)
sequencer.c +4 -71
@@ -869,34 +869,6 @@ static char *get_author(const char *message)return NULL ; } -/* Read author-script and return an ident line (author <email> timestamp) */ -static const char *read_author_ident (struct strbuf *buf) -{ -struct strbuf out =STRBUF_INIT ; -char *name, *email, *date; - -if (read_author_script (rebase_path_author_script (), - &name, &email, &date,0 )) -return NULL ; - -/* validate date since fmt_ident() will die() on bad value */ -if (parse_date (date, &out)){ -warning (_ ("invalid date format '%s' in '%s'" ), - date,rebase_path_author_script ()); -strbuf_release (&out); -return NULL ; - } - -strbuf_reset (&out); -strbuf_addstr (&out,fmt_ident (name, email,WANT_AUTHOR_IDENT , date,0 )); -strbuf_swap (buf, &out); -strbuf_release (&out); -free (name); -free (email); -free (date); -return buf->buf; -} -static const char staged_changes_advice[] =N_ ("you have staged changes in your working tree\n" "If these changes are meant to be squashed into the previous commit, run:\n"
@@ -954,45 +926,6 @@ static int run_git_commit(struct repository *r, {struct child_process cmd =CHILD_PROCESS_INIT ; -if ((flags &CREATE_ROOT_COMMIT ) && !(flags &AMEND_MSG )) { -struct strbuf msg =STRBUF_INIT , script =STRBUF_INIT ; -const char *author =NULL ; -struct object_id root_commit, *cache_tree_oid; -int res =0 ; - -if (is_rebase_i (opts)) { - author =read_author_ident (&script); -if (!author) { -strbuf_release (&script); -return -1 ; - } - } - -if (!defmsg) -BUG ("root commit without message" ); - -if (!(cache_tree_oid =get_cache_tree_oid (r->index))) - res =-1 ; - -if (!res) - res =strbuf_read_file (&msg, defmsg,0 ); - -if (res <=0 ) - res =error_errno (_ ("could not read '%s'" ), defmsg); -else - res =commit_tree (msg.buf, msg.len, cache_tree_oid, -NULL , &root_commit, author, - opts->gpg_sign); - -strbuf_release (&msg); -strbuf_release (&script); -if (!res) - res =update_ref (NULL ,"HEAD" , &root_commit,NULL ,0 , -UPDATE_REFS_MSG_ON_ERR ); - -return res <0 ?error (_ ("writing root commit" )) :0 ; - } - cmd.git_cmd =1 ;if (is_rebase_i (opts) &&read_env_script (&cmd.env_array)) {
@@ -1376,7 +1309,7 @@ static int try_to_commit(struct repository *r, struct object_id *oid) {struct object_id tree; -struct commit *current_head; +struct commit *current_head =NULL ;struct commit_list *parents =NULL ;struct commit_extra_header *extra =NULL ;struct strbuf err =STRBUF_INIT ;
@@ -1411,7 +1344,8 @@ static int try_to_commit(struct repository *r, } parents =copy_commit_list (current_head->parents); extra =read_commit_extra_headers (current_head, exclude_gpgsig); - }else if (current_head) { + }else if (current_head && + (!(flags &CREATE_ROOT_COMMIT ) || (flags &AMEND_MSG ))) {commit_list_insert (current_head, &parents); }
@@ -1488,8 +1422,7 @@ static int do_commit(struct repository *r, {int res =1 ; -if (!(flags &EDIT_MSG ) && !(flags &VERIFY_MSG ) && - !(flags &CREATE_ROOT_COMMIT )) { +if (!(flags &EDIT_MSG ) && !(flags &VERIFY_MSG )) {struct object_id oid;struct strbuf sb =STRBUF_INIT ;
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] sequencer: simplify root commit creation
Quoted 2 lines · Phillip Wood via GitGitGadget
Adapt
try_to_commit()to create a new root commit rather than special casing this inrun_git_commit(). The significantly reduces the amount of
s/The/This/
Quoted 4 lines
special case code for creating the root commit and reduces the number of commit code paths we have to worry about.
Signed-off-by Phillip Wood <[email protected]>
Re: [PATCH 3/3] sequencer: simplify root commit creation
Quoted 6 lines · Eric Sunshine
Quoted 2 lines · Phillip Wood via GitGitGadget
Adapt
try_to_commit()to create a new root commit rather than special casing this inrun_git_commit(). The significantly reduces the amount ofs/The/This/
Thanks Eric - well spotted as ever. Thanks Junio for fixing this up in pu.
Best Wishes
Phillip
Quoted 4 lines
Quoted 4 lines
special case code for creating the root commit and reduces the number of commit code paths we have to worry about.
Signed-off-by Phillip Wood <[email protected]>