[PATCH 0/3] rebase -i: always update HEAD before rewording

[PATCH 0/3] rebase -i: always update HEAD before rewording

[Phillip Wood via GitGitGadget]

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
-- 
gitgitgadget

[PATCH 1/3] rebase -i: always update HEAD before rewording

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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/xmqqlfvu4be3.fsf@gitster-ct.c.googlers.com/T/#m133009cb91cf0917bcf667300f061178be56680a

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 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(-)

diff --git a/sequencer.c b/sequencer.c
index 34ebf8ed94..085777f4a1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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());
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 461dd539ff..d2f1d5bd23 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -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 &&
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index ba8bd1b514..94f85cdf83 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -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
 	'
 }
diff --git a/t/t7505/expected-rebase-i b/t/t7505/expected-rebase-i
index c514bdbb94..93bada596e 100644
--- a/t/t7505/expected-rebase-i
+++ b/t/t7505/expected-rebase-i
@@ -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]
-- 
gitgitgadget

[PATCH 2/3] rebase -i: check for updated todo after squash and reword

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                 | 42 ++++++++++++++++++++++++-------------
 t/t3429-rebase-edit-todo.sh | 21 ++++++++++++++++++-
 2 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 085777f4a1..a13e1a7466 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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,
diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
index 76f6d306ea..8739cb60a7 100755
--- a/t/t3429-rebase-edit-todo.sh
+++ b/t/t3429-rebase-edit-todo.sh
@@ -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
-- 
gitgitgadget

[PATCH 3/3] sequencer: simplify root commit creation

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 75 +++--------------------------------------------------
 1 file changed, 4 insertions(+), 71 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a13e1a7466..758c780790 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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;
 
-- 
gitgitgadget

Re: [PATCH 3/3] sequencer: simplify root commit creation

[Eric Sunshine]

On Mon, Aug 19, 2019 at 5:18 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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

s/The/This/

> 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 <phillip.wood@dunelm.org.uk>

Re: [PATCH 3/3] sequencer: simplify root commit creation

[Phillip Wood]

On 19/08/2019 17:09, Eric Sunshine wrote:
> On Mon, Aug 19, 2019 at 5:18 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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
> 
> s/The/This/

Thanks Eric - well spotted as ever. Thanks Junio for fixing this up in pu.

Best Wishes

Phillip

>> 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 <phillip.wood@dunelm.org.uk>