[PATCH 0/6] Let the sequencer handle `git rebase -i --root`

[PATCH 0/6] Let the sequencer handle `git rebase -i --root`

[Johannes Schindelin]

When I reimplemented the most performance-critical bits of the
interactive rebase in the sequencer, to speed up `git rebase -i`
particularly on Windows (even if the benefits are still quite notable on
Linux or macOS), I punted on the --root part.

I had always hoped that some other contributor (or I myself) would come
back later to address the --root part in the sequencer, too, with the
idea that this would move the last remaining complicated code from
git-rebase--interactive.sh into sequencer.c, to facilitate converting
the rest of git-rebase--interactive.sh.

When I say "the last remaining complicated code", of course I neglect
the --preserve-merges code, but as I worked hard on the --rebase-merges
patch series with the intention to eventually deprecate and maybe even
remove the --preserve-merges mode, I always implicitly assume that the
--preserve-merges code will be moved into its own shell script
(git-rebase--preserve-merges.sh, maybe?) and never be converted.

So here goes: the patches to move the handling of --root into the
sequencer. After two preparatory patches, the real conversion takes
place in the third patch. After that, we take care of the --root related
concerns that arise in conjunction with the --rebase-merges mode.

As the --rebase-merges/--root patches overlap quite a bit (not so much
in the code itself as in philosophical considerations such as "what
should happen if you try to merge a branch into a new root", or the
fact that the label/reset/merge commands make it desirable to be able to
create a new root commit in the middle of a todo list), I had to
consider in which order to contribute them. In the end, I decided to go
with --rebase-merges first, so the --root patches are based on the
--rebase-merges patch series.

I consider this patch series a critical prerequisite for Alban's Google
Summer of Code project to convert rebase -i into a builtin.


Johannes Schindelin (6):
  sequencer: extract helper to update active_cache_tree
  sequencer: learn about the special "fake root commit" handling
  rebase -i --root: let the sequencer handle even the initial part
  sequencer: allow introducing new root commits
  rebase --rebase-merges: a "merge" into a new root is a fast-forward
  rebase --rebase-merges: root commits can be cousins, too

 git-rebase--interactive.sh        |   4 +-
 sequencer.c                       | 186 ++++++++++++++++++++++++++----
 sequencer.h                       |   4 +
 t/t3404-rebase-interactive.sh     |  19 ++-
 t/t3421-rebase-topology-linear.sh |   6 +-
 t/t3430-rebase-merges.sh          |  72 ++++++++++++
 6 files changed, 256 insertions(+), 35 deletions(-)


base-commit: 673fb9cb8b5c7d57cb560b6ade45e419c8dd09fc
Based-On: recreate-merges at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git recreate-merges
Published-As: https://github.com/dscho/git/releases/tag/sequencer-and-root-commits-v1
Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-and-root-commits-v1
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 1/6] sequencer: extract helper to update active_cache_tree

[Johannes Schindelin]

This patch extracts the code from is_index_unchanged() to initialize or
update the index' cache tree (i.e. a tree object reflecting the current
index' top-level tree).

The new helper will be used in the upcoming code to support `git rebase
-i --root` via the sequencer.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e2f83942843..90c8218aa9a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -562,9 +562,23 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	return !clean;
 }
 
+static struct object_id *get_cache_tree_oid(void)
+{
+	if (!active_cache_tree)
+		active_cache_tree = cache_tree();
+
+	if (!cache_tree_fully_valid(active_cache_tree))
+		if (cache_tree_update(&the_index, 0)) {
+			error(_("unable to update cache tree"));
+			return NULL;
+		}
+
+	return &active_cache_tree->oid;
+}
+
 static int is_index_unchanged(void)
 {
-	struct object_id head_oid;
+	struct object_id head_oid, *cache_tree_oid;
 	struct commit *head_commit;
 
 	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL))
@@ -583,15 +597,10 @@ static int is_index_unchanged(void)
 	if (parse_commit(head_commit))
 		return -1;
 
-	if (!active_cache_tree)
-		active_cache_tree = cache_tree();
-
-	if (!cache_tree_fully_valid(active_cache_tree))
-		if (cache_tree_update(&the_index, 0))
-			return error(_("unable to update cache tree"));
+	if (!(cache_tree_oid = get_cache_tree_oid()))
+		return -1;
 
-	return !oidcmp(&active_cache_tree->oid,
-		       &head_commit->tree->object.oid);
+	return !oidcmp(cache_tree_oid, &head_commit->tree->object.oid);
 }
 
 static int write_author_script(const char *message)
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 2/6] sequencer: learn about the special "fake root commit" handling

[Johannes Schindelin]

When an interactive rebase wants to recreate a root commit, it
- first creates a new, empty root commit,
- checks it out,
- converts the next `pick` command so that it amends the empty root
  commit

Introduce support in the sequencer to handle such an empty root commit,
by looking for the file <GIT_DIR>/rebase-merge/squash-onto; if it exists
and contains a commit name, the sequencer will compare the HEAD to said
root commit, and if identical, a new root commit will be created.

While converting scripted code into proper, portable C, we also do away
with the old "amend with an empty commit message, then cherry-pick
without committing, then amend again" dance and replace it with code
that uses the internal API properly to do exactly what we want: create a
new root commit.

To keep the implementation simple, we always spawn `git commit` to create
new root commits.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 sequencer.h |   4 ++
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 90c8218aa9a..fc124596b53 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -125,6 +125,12 @@ static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
 static GIT_PATH_FUNC(rebase_path_rewritten_pending,
 	"rebase-merge/rewritten-pending")
 
+/*
+ * The path of the file containig the OID of the "squash onto" commit, i.e.
+ * the dummy commit used for `reset [new root]`.
+ */
+static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
+
 /*
  * The path of the file listing refs that need to be deleted after the rebase
  * finishes. This is used by the `label` command to record the need for cleanup.
@@ -470,7 +476,8 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD",
-				   to, unborn ? &null_oid : from,
+				   to, unborn && !is_rebase_i(opts) ?
+				   &null_oid : from,
 				   0, sb.buf, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
@@ -692,6 +699,42 @@ static char *get_author(const char *message)
 	return NULL;
 }
 
+static const char *read_author_ident(struct strbuf *buf)
+{
+	char *p, *p2;
+
+	if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
+		return NULL;
+
+	for (p = buf->buf; *p; p++)
+		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
+			strbuf_splice(buf, p - buf->buf, p2 - p, "'", 1);
+		else if (*p == '\'')
+			strbuf_splice(buf, p-- - buf->buf, 1, "", 0);
+
+	if (skip_prefix(buf->buf, "GIT_AUTHOR_NAME=", (const char **)&p)) {
+		strbuf_splice(buf, 0, p - buf->buf, "", 0);
+		p = strchr(buf->buf, '\n');
+		if (skip_prefix(p, "\nGIT_AUTHOR_EMAIL=", (const char **)&p2)) {
+			strbuf_splice(buf, p - buf->buf, p2 - p, " <", 2);
+			p = strchr(p, '\n');
+			if (skip_prefix(p, "\nGIT_AUTHOR_DATE=@",
+					(const char **)&p2)) {
+				strbuf_splice(buf, p - buf->buf, p2 - p,
+					      "> ", 2);
+				p = strchr(p, '\n');
+				if (p) {
+					strbuf_setlen(buf, p - buf->buf);
+					return buf->buf;
+				}
+			}
+		}
+	}
+
+	warning(_("could not parse '%s'"), rebase_path_author_script());
+	return NULL;
+}
+
 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"
@@ -711,6 +754,7 @@ N_("you have staged changes in your working tree\n"
 #define AMEND_MSG   (1<<2)
 #define CLEANUP_MSG (1<<3)
 #define VERIFY_MSG  (1<<4)
+#define CREATE_ROOT_COMMIT (1<<5)
 
 /*
  * If we are cherry-pick, and if the merge did not result in
@@ -730,6 +774,40 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	const char *value;
 
+	if (flags & CREATE_ROOT_COMMIT) {
+		struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
+		const char *author = is_rebase_i(opts) ?
+			read_author_ident(&script) : NULL;
+		struct object_id root_commit, *cache_tree_oid;
+		int res = 0;
+
+		if (!defmsg)
+			BUG("root commit without message");
+
+		if (!(cache_tree_oid = get_cache_tree_oid()))
+			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) {
+			update_ref(NULL, "CHERRY_PICK_HEAD", &root_commit, NULL,
+				   REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR);
+			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)) {
@@ -1216,7 +1294,8 @@ static int do_commit(const char *msg_file, const char *author,
 {
 	int res = 1;
 
-	if (!(flags & EDIT_MSG) && !(flags & VERIFY_MSG)) {
+	if (!(flags & EDIT_MSG) && !(flags & VERIFY_MSG) &&
+	    !(flags & CREATE_ROOT_COMMIT)) {
 		struct object_id oid;
 		struct strbuf sb = STRBUF_INIT;
 
@@ -1369,6 +1448,12 @@ static int is_fixup(enum todo_command command)
 	return command == TODO_FIXUP || command == TODO_SQUASH;
 }
 
+/* Does this command create a (non-merge) commit? */
+static int is_pick_or_similar(enum todo_command command)
+{
+	return command <= TODO_SQUASH;
+}
+
 static int update_squash_messages(enum todo_command command,
 		struct commit *commit, struct replay_opts *opts)
 {
@@ -1523,7 +1608,14 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			return error(_("your index file is unmerged."));
 	} else {
 		unborn = get_oid("HEAD", &head);
-		if (unborn)
+		/* Do we want to generate a root commit? */
+		if (is_pick_or_similar(command) && opts->have_squash_onto &&
+		    !oidcmp(&head, &opts->squash_onto)) {
+			if (is_fixup(command))
+				return error(_("cannot fixup root commit"));
+			flags |= CREATE_ROOT_COMMIT;
+			unborn = 1;
+		} else if (unborn)
 			oidcpy(&head, the_hash_algo->empty_tree);
 		if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD",
 				       NULL, 0))
@@ -2136,6 +2228,12 @@ static int read_populate_opts(struct replay_opts *opts)
 		read_strategy_opts(opts, &buf);
 		strbuf_release(&buf);
 
+		if (read_oneliner(&buf, rebase_path_squash_onto(), 0)) {
+			if (get_oid_hex(buf.buf, &opts->squash_onto) < 0)
+				return error(_("unusable squash-onto"));
+			opts->have_squash_onto = 1;
+		}
+
 		return 0;
 	}
 
diff --git a/sequencer.h b/sequencer.h
index d9570d92b11..4b2717881fa 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -44,6 +44,10 @@ struct replay_opts {
 	char **xopts;
 	size_t xopts_nr, xopts_alloc;
 
+	/* placeholder commit for -i --root */
+	struct object_id squash_onto;
+	int have_squash_onto;
+
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
 };
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 3/6] rebase -i --root: let the sequencer handle even the initial part

[Johannes Schindelin]

In this developer's earlier attempt to accelerate interactive rebases by
converting large parts from Unix shell script into portable, performant
C, the --root handling was specifically excluded (to simplify the task a
little bit; it still took over a year to get that reduced set of patches
into Git proper).

This patch ties up that loose end: now only --preserve-merges uses the
slow Unix shell script implementation to perform the interactive rebase.

As the rebase--helper reports progress to stderr (unlike the scripted
interactive rebase, which reports it to stdout, of all places), we have
to adjust a couple of tests that did not expect that for `git rebase -i
--root`.

This patch fixes -- at long last! -- the really old bug reported in
6a6bc5bdc4d (add tests for rebasing root, 2013-06-06) that rebasing with
--root *always* rewrote the root commit, even if there were no changes.

The bug still persists in --preserve-merges mode, of course, but that
mode will be deprecated as soon as the new --rebase-merges mode
stabilizes, anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh        |  4 +++-
 t/t3404-rebase-interactive.sh     | 19 +++++++++++++------
 t/t3421-rebase-topology-linear.sh |  6 +++---
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cbf44f86482..2f4941d0fc9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -894,6 +894,8 @@ init_revisions_and_shortrevisions () {
 	else
 		revisions=$onto...$orig_head
 		shortrevisions=$shorthead
+		test -z "$squash_onto" ||
+		echo "$squash_onto" >"$state_dir"/squash-onto
 	fi
 }
 
@@ -948,7 +950,7 @@ EOF
 	die "Could not skip unnecessary pick commands"
 
 	checkout_onto
-	if test -z "$rebase_root" && test ! -d "$rewritten"
+	if test ! -d "$rewritten"
 	then
 		require_clean_work_tree "rebase"
 		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 59c766540e5..c65826ddace 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1204,10 +1204,6 @@ test_expect_success 'drop' '
 	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
 '
 
-cat >expect <<EOF
-Successfully rebased and updated refs/heads/missing-commit.
-EOF
-
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 	test_config rebase.missingCommitsCheck ignore &&
 	rebase_setup_and_clean missing-commit &&
@@ -1215,7 +1211,9 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 	FAKE_LINES="1 2 3 4" \
 		git rebase -i --root 2>actual &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
-	test_i18ncmp expect actual
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual
 '
 
 cat >expect <<EOF
@@ -1227,15 +1225,24 @@ To avoid this message, use "drop" to explicitly remove a commit.
 Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 The possible behaviours are: ignore, warn, error.
 
+Rebasing (1/4)
+Rebasing (2/4)
+Rebasing (3/4)
+Rebasing (4/4)
 Successfully rebased and updated refs/heads/missing-commit.
 EOF
 
+cr_to_nl () {
+	tr '\015' '\012'
+}
+
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
 	test_config rebase.missingCommitsCheck warn &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
 	FAKE_LINES="1 2 3 4" \
-		git rebase -i --root 2>actual &&
+		git rebase -i --root 2>actual.2 &&
+	cr_to_nl <actual.2 >actual &&
 	test_i18ncmp expect actual &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p)
 '
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index e7438ad06ac..99b2aac9219 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -328,9 +328,9 @@ test_run_rebase () {
 		test_cmp_rev c HEAD
 	"
 }
-test_run_rebase failure ''
-test_run_rebase failure -m
-test_run_rebase failure -i
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
 test_run_rebase failure -p
 
 test_run_rebase () {
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 4/6] sequencer: allow introducing new root commits

[Johannes Schindelin]

In the context of the new --rebase-merges mode, which was designed
specifically to allow for changing the existing branch topology
liberally, a user may want to extract commits into a completely fresh
branch that starts with a newly-created root commit.

This is now possible by inserting the command `reset [new root]` before
`pick`ing the commit that wants to become a root commit. Example:

	reset [new root]
	pick 012345 a commit that is about to become a root commit
	pick 234567 this commit will have the previous one as parent

This does not conflict with other uses of the `reset` command because
`[new root]` is not (part of) a valid ref name: both the opening bracket
as well as the space are illegal in ref names.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c              | 40 ++++++++++++++++++++++++++++------------
 t/t3430-rebase-merges.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index fc124596b53..d10ebd62520 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2727,18 +2727,34 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
 		return -1;
 
-	/* Determine the length of the label */
-	for (i = 0; i < len; i++)
-		if (isspace(name[i]))
-			len = i;
-
-	strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
-	if (get_oid(ref_name.buf, &oid) &&
-	    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
-		error(_("could not read '%s'"), ref_name.buf);
-		rollback_lock_file(&lock);
-		strbuf_release(&ref_name);
-		return -1;
+	if (len == 10 && !strncmp("[new root]", name, len)) {
+		if (!opts->have_squash_onto) {
+			const char *hex;
+			if (commit_tree("", 0, the_hash_algo->empty_tree,
+					NULL, &opts->squash_onto,
+					NULL, NULL))
+				return error(_("writing fake root commit"));
+			opts->have_squash_onto = 1;
+			hex = oid_to_hex(&opts->squash_onto);
+			if (write_message(hex, strlen(hex),
+					  rebase_path_squash_onto(), 0))
+				return error(_("writing squash-onto"));
+		}
+		oidcpy(&oid, &opts->squash_onto);
+	} else {
+		/* Determine the length of the label */
+		for (i = 0; i < len; i++)
+			if (isspace(name[i]))
+				len = i;
+
+		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
+		if (get_oid(ref_name.buf, &oid) &&
+		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
+			error(_("could not read '%s'"), ref_name.buf);
+			rollback_lock_file(&lock);
+			strbuf_release(&ref_name);
+			return -1;
+		}
 	}
 
 	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 3d4dfdf7bec..35260862fcb 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -241,4 +241,38 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
 	test_cmp_rev HEAD $before
 '
 
+test_expect_success 'root commits' '
+	git checkout --orphan unrelated &&
+	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
+	 test_commit second-root) &&
+	test_commit third-root &&
+	cat >script-from-scratch <<-\EOF &&
+	pick third-root
+	label first-branch
+	reset [new root]
+	pick second-root
+	merge first-branch # Merge the 3rd root
+	EOF
+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	git rebase -i --force --root -r &&
+	test "Parsnip" = "$(git show -s --format=%an HEAD^)" &&
+	test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
+	test $(git rev-parse second-root:second-root.t) = \
+		$(git rev-parse HEAD^:second-root.t) &&
+	test_cmp_graph HEAD <<-\EOF &&
+	*   Merge the 3rd root
+	|\
+	| * third-root
+	* second-root
+	EOF
+
+	: fast forward if possible &&
+	before="$(git rev-parse --verify HEAD)" &&
+	test_might_fail git config --unset sequence.editor &&
+	test_tick &&
+	git rebase -i --root -r &&
+	test_cmp_rev HEAD $before
+'
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 5/6] rebase --rebase-merges: a "merge" into a new root is a fast-forward

[Johannes Schindelin]

When a user provides a todo list containing something like

	reset [new root]
	merge my-branch

let's do the same as if pulling into an orphan branch: simply
fast-forward.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c              | 12 ++++++++++++
 t/t3430-rebase-merges.sh | 13 +++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index d10ebd62520..ad5ff2709a6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2850,6 +2850,18 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 		goto leave_merge;
 	}
 
+	if (opts->have_squash_onto &&
+	    !oidcmp(&head_commit->object.oid, &opts->squash_onto)) {
+		/*
+		 * When the user tells us to "merge" something into a
+		 * "[new root]", let's simply fast-forward to the merge head.
+		 */
+		rollback_lock_file(&lock);
+		ret = fast_forward_to(&merge_commit->object.oid,
+				       &head_commit->object.oid, 0, opts);
+		goto leave_merge;
+	}
+
 	if (commit) {
 		const char *message = get_commit_buffer(commit, NULL);
 		const char *body;
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 35260862fcb..5543f1d5a34 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -275,4 +275,17 @@ test_expect_success 'root commits' '
 	test_cmp_rev HEAD $before
 '
 
+test_expect_success 'a "merge" into a root commit is a fast-forward' '
+	head=$(git rev-parse HEAD) &&
+	cat >script-from-scratch <<-EOF &&
+	reset [new root]
+	merge $head
+	EOF
+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	git rebase -i -r HEAD^ &&
+	test_cmp_rev HEAD $head
+'
+
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 6/6] rebase --rebase-merges: root commits can be cousins, too

[Johannes Schindelin]

Reported by Wink Saville: when rebasing with no-rebase-cousins, we
will want to refrain from rebasing all of them, even when they are
root commits.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c              |  3 ++-
 t/t3430-rebase-merges.sh | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index ad5ff2709a6..2bcd13e1fc6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3883,7 +3883,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		}
 
 		if (!commit)
-			fprintf(out, "%s onto\n", cmd_reset);
+			fprintf(out, "%s %s\n", cmd_reset,
+				rebase_cousins ? "onto" : "[new root]");
 		else {
 			const char *to = NULL;
 
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 5543f1d5a34..ce6de6f491e 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -287,5 +287,30 @@ test_expect_success 'a "merge" into a root commit is a fast-forward' '
 	test_cmp_rev HEAD $head
 '
 
+test_expect_success 'A root commit can be a cousin, treat it that way' '
+	git checkout --orphan khnum &&
+	test_commit yama &&
+	git checkout -b asherah master &&
+	test_commit shamkat &&
+	git merge --allow-unrelated-histories khnum &&
+	test_tick &&
+	git rebase -f -r HEAD^ &&
+	! test_cmp_rev HEAD^2 khnum &&
+	test_cmp_graph HEAD^.. <<-\EOF &&
+	*   Merge branch '\''khnum'\'' into asherah
+	|\
+	| * yama
+	o shamkat
+	EOF
+	test_tick &&
+	git rebase --rebase-merges=rebase-cousins HEAD^ &&
+	test_cmp_graph HEAD^.. <<-\EOF
+	*   Merge branch '\''khnum'\'' into asherah
+	|\
+	| * yama
+	|/
+	o shamkat
+	EOF
+'
 
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

Re: [PATCH 1/6] sequencer: extract helper to update active_cache_tree

[Stefan Beller]

Hi Johannes,

On Fri, Apr 27, 2018 at 3:30 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> This patch extracts the code from is_index_unchanged() to initialize or
> update the index' cache tree (i.e. a tree object reflecting the current
> index' top-level tree).
>
> The new helper will be used in the upcoming code to support `git rebase
> -i --root` via the sequencer.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e2f83942843..90c8218aa9a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -562,9 +562,23 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>         return !clean;
>  }
>
> +static struct object_id *get_cache_tree_oid(void)
> +{
> +       if (!active_cache_tree)
> +               active_cache_tree = cache_tree();
> +
> +       if (!cache_tree_fully_valid(active_cache_tree))
> +               if (cache_tree_update(&the_index, 0)) {
> +                       error(_("unable to update cache tree"));
> +                       return NULL;
> +               }

This move is a verbatim move of the code below, except that
we need to add braces. For some reason I fantasize of using
the comma operator in C eventually (which we do not use in
our code base AFAICT), then we could leave out the braces
and have a

    return error(...), NULL;

Anyway, this patch is
Reviewed-by: Stefan Beller <sbeller@google.com>

Re: [PATCH 2/6] sequencer: learn about the special "fake root commit" handling

[Stefan Beller]

On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When an interactive rebase wants to recreate a root commit, it
> - first creates a new, empty root commit,
> - checks it out,
> - converts the next `pick` command so that it amends the empty root
>   commit
>
> Introduce support in the sequencer to handle such an empty root commit,
> by looking for the file <GIT_DIR>/rebase-merge/squash-onto; if it exists
> and contains a commit name, the sequencer will compare the HEAD to said
> root commit, and if identical, a new root commit will be created.
>
> While converting scripted code into proper, portable C, we also do away
> with the old "amend with an empty commit message, then cherry-pick
> without committing, then amend again" dance and replace it with code
> that uses the internal API properly to do exactly what we want: create a
> new root commit.
>
> To keep the implementation simple, we always spawn `git commit` to create
> new root commits.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  sequencer.h |   4 ++
>  2 files changed, 105 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 90c8218aa9a..fc124596b53 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -125,6 +125,12 @@ static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
>  static GIT_PATH_FUNC(rebase_path_rewritten_pending,
>         "rebase-merge/rewritten-pending")
>
> +/*
> + * The path of the file containig the OID of the "squash onto" commit, i.e.
> + * the dummy commit used for `reset [new root]`.
> + */
> +static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
> +
>  /*
>   * The path of the file listing refs that need to be deleted after the rebase
>   * finishes. This is used by the `label` command to record the need for cleanup.
> @@ -470,7 +476,8 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
>         transaction = ref_transaction_begin(&err);
>         if (!transaction ||
>             ref_transaction_update(transaction, "HEAD",
> -                                  to, unborn ? &null_oid : from,
> +                                  to, unborn && !is_rebase_i(opts) ?
> +                                  &null_oid : from,
>                                    0, sb.buf, &err) ||
>             ref_transaction_commit(transaction, &err)) {
>                 ref_transaction_free(transaction);
> @@ -692,6 +699,42 @@ static char *get_author(const char *message)
>         return NULL;
>  }
>
> +static const char *read_author_ident(struct strbuf *buf)

This seems to be the counter part of write_author_script(*msg),
would it make sense to either rename this to read_author_script
or rename the counter part to write_author_ident ?

> +{
> +       char *p, *p2;
> +
> +       if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)

The 256 is a hint for read_file how to size the buffer initially.
If not given it defaults to 8k, which presumably is too much for
an author identity.



> +       for (p = buf->buf; *p; p++)
> +               if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> +                       strbuf_splice(buf, p - buf->buf, p2 - p, "'", 1);
> +               else if (*p == '\'')
> +                       strbuf_splice(buf, p-- - buf->buf, 1, "", 0);

This part could be prefixed with
    /* un-escape text: turn \\ into ' and remove single quotes. */

> +       if (skip_prefix(buf->buf, "GIT_AUTHOR_NAME=", (const char **)&p)) {
> +               strbuf_splice(buf, 0, p - buf->buf, "", 0);
> +               p = strchr(buf->buf, '\n');
> +               if (skip_prefix(p, "\nGIT_AUTHOR_EMAIL=", (const char **)&p2)) {
> +                       strbuf_splice(buf, p - buf->buf, p2 - p, " <", 2);
> +                       p = strchr(p, '\n');
> +                       if (skip_prefix(p, "\nGIT_AUTHOR_DATE=@",
> +                                       (const char **)&p2)) {
> +                               strbuf_splice(buf, p - buf->buf, p2 - p,
> +                                             "> ", 2);
> +                               p = strchr(p, '\n');
> +                               if (p) {
> +                                       strbuf_setlen(buf, p - buf->buf);
> +                                       return buf->buf;

So here we have read GIT_AUTHOR_NAME, _EMAIL
and _DATE in that order and converted it to its form
"name <email> date" in a single line.

It would be better to invert the conditions and keep
the indentation level lower by:

    if (!skip_prefix(...))
        goto warning_and_return;
    strbuf_splice(...);
    ...

I wondered if we want to factor out the conversion of
"author string in commit form" to "author information
in script form" into their own functions, and keep the reading
writing out of them. But then again we only need them in
these use cases for now, and such a refactoring can happen
later if needed.

> +       warning(_("could not parse '%s'"), rebase_path_author_script());

This function needs all three environment variables in its correct order,
which sounds a little brittle, but then again we do not expect manual
editing of that file, but expect it to be written by Git.

> @@ -1369,6 +1448,12 @@ static int is_fixup(enum todo_command command)
>         return command == TODO_FIXUP || command == TODO_SQUASH;
>  }
>
> +/* Does this command create a (non-merge) commit? */
> +static int is_pick_or_similar(enum todo_command command)
> +{
> +       return command <= TODO_SQUASH;
> +}

This code looks scary.
Fortunately the enum todo_command hints that the order matters,
such that we're probably protected from wild reordering in the future,
however this implies that the section /* commands that handle commits */
comes first and that TODO_SQUASH is the last entry of that section.
So maybe we'd want to add a /* must be last in section */ to
TODO_squash and also document that the section must be first?

Do we have other code that needs a very specific ordering
with similar further assumptions (section being first/last, a
command being first/last in their section)?

I wondered what the _or_similar means and by looking up
that enum, I would think a name like

static int handles_single_commit(enum todo_command)

might be better?

> @@ -1523,7 +1608,14 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>                         return error(_("your index file is unmerged."));
>         } else {
>                 unborn = get_oid("HEAD", &head);
> -               if (unborn)
> +               /* Do we want to generate a root commit? */
> +               if (is_pick_or_similar(command) && opts->have_squash_onto &&
> +                   !oidcmp(&head, &opts->squash_onto)) {
> +                       if (is_fixup(command))
> +                               return error(_("cannot fixup root commit"));

I would expect you also cannot squash into root commit?

Thanks,
Stefan

Re: [PATCH 3/6] rebase -i --root: let the sequencer handle even the initial part

[Stefan Beller]

On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> In this developer's earlier attempt to accelerate interactive rebases by
> converting large parts from Unix shell script into portable, performant
> C, the --root handling was specifically excluded (to simplify the task a
> little bit; it still took over a year to get that reduced set of patches
> into Git proper).
>
> This patch ties up that loose end: now only --preserve-merges uses the
> slow Unix shell script implementation to perform the interactive rebase.
>
> As the rebase--helper reports progress to stderr (unlike the scripted
> interactive rebase, which reports it to stdout, of all places), we have
> to adjust a couple of tests that did not expect that for `git rebase -i
> --root`.
>
> This patch fixes -- at long last! -- the really old bug reported in
> 6a6bc5bdc4d (add tests for rebasing root, 2013-06-06) that rebasing with
> --root *always* rewrote the root commit, even if there were no changes.
>
> The bug still persists in --preserve-merges mode, of course, but that
> mode will be deprecated as soon as the new --rebase-merges mode
> stabilizes, anyway.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-rebase--interactive.sh        |  4 +++-
>  t/t3404-rebase-interactive.sh     | 19 +++++++++++++------
>  t/t3421-rebase-topology-linear.sh |  6 +++---
>  3 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cbf44f86482..2f4941d0fc9 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -894,6 +894,8 @@ init_revisions_and_shortrevisions () {
>         else
>                 revisions=$onto...$orig_head
>                 shortrevisions=$shorthead
> +               test -z "$squash_onto" ||
> +               echo "$squash_onto" >"$state_dir"/squash-onto
>         fi
>  }
>
> @@ -948,7 +950,7 @@ EOF
>         die "Could not skip unnecessary pick commands"
>
>         checkout_onto
> -       if test -z "$rebase_root" && test ! -d "$rewritten"
> +       if test ! -d "$rewritten"

I have the impression this is the line that is really well
explained in the commit message ("migrate to rebase
helper even when there is $rebase_root set")

The rest of the patch is covered as "a couple of places
where we adjust stdout to stderr"?

Makes sense,
Stefan

Re: [PATCH 2/6] sequencer: learn about the special "fake root commit" handling

[Johannes Schindelin]

Hi Stefan,

On Sat, 28 Apr 2018, Stefan Beller wrote:

> On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > When an interactive rebase wants to recreate a root commit, it
> > - first creates a new, empty root commit,
> > - checks it out,
> > - converts the next `pick` command so that it amends the empty root
> >   commit
> >
> > Introduce support in the sequencer to handle such an empty root commit,
> > by looking for the file <GIT_DIR>/rebase-merge/squash-onto; if it exists
> > and contains a commit name, the sequencer will compare the HEAD to said
> > root commit, and if identical, a new root commit will be created.
> >
> > While converting scripted code into proper, portable C, we also do away
> > with the old "amend with an empty commit message, then cherry-pick
> > without committing, then amend again" dance and replace it with code
> > that uses the internal API properly to do exactly what we want: create a
> > new root commit.
> >
> > To keep the implementation simple, we always spawn `git commit` to create
> > new root commits.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  sequencer.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  sequencer.h |   4 ++
> >  2 files changed, 105 insertions(+), 3 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 90c8218aa9a..fc124596b53 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -125,6 +125,12 @@ static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
> >  static GIT_PATH_FUNC(rebase_path_rewritten_pending,
> >         "rebase-merge/rewritten-pending")
> >
> > +/*
> > + * The path of the file containig the OID of the "squash onto" commit, i.e.
> > + * the dummy commit used for `reset [new root]`.
> > + */
> > +static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
> > +
> >  /*
> >   * The path of the file listing refs that need to be deleted after the rebase
> >   * finishes. This is used by the `label` command to record the need for cleanup.
> > @@ -470,7 +476,8 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
> >         transaction = ref_transaction_begin(&err);
> >         if (!transaction ||
> >             ref_transaction_update(transaction, "HEAD",
> > -                                  to, unborn ? &null_oid : from,
> > +                                  to, unborn && !is_rebase_i(opts) ?
> > +                                  &null_oid : from,
> >                                    0, sb.buf, &err) ||
> >             ref_transaction_commit(transaction, &err)) {
> >                 ref_transaction_free(transaction);
> > @@ -692,6 +699,42 @@ static char *get_author(const char *message)
> >         return NULL;
> >  }
> >
> > +static const char *read_author_ident(struct strbuf *buf)
> 
> This seems to be the counter part of write_author_script(*msg),
> would it make sense to either rename this to read_author_script
> or rename the counter part to write_author_ident ?

It is not really *the* counterpart of write_author_script(). There are
many conceivable counterparts, one of them already exists and is called
read_env_script(). They serve different purposes, even if both read the
author-script file, and they parse things in a fundamentally different
way. I had already pointed out something along those lines in the review
of the patch introducing the read_env_script() and Junio did not believe
me. To make sure, it was my fault that I failed to make a compelling
enough argument. I am glad that I now have proof that I was right: just
because you read the same file, for a similar purpose, does not
necessarily imply that you can share code for those purposes. All you can
share is the name of the input file.

Having said that, *this* time round, what we need to do is actually very
similar to what builtin/am.c's read_author_script() does (even if we
cannot use it as-is: it populates part of a `struct am_state`). I'll have
to look into this more closely.

> > +{
> > +       char *p, *p2;
> > +
> > +       if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
> 
> The 256 is a hint for read_file how to size the buffer initially.
> If not given it defaults to 8k, which presumably is too much for
> an author identity.

That is a correct reading of the code's intent.

> > +       for (p = buf->buf; *p; p++)
> > +               if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> > +                       strbuf_splice(buf, p - buf->buf, p2 - p, "'", 1);
> > +               else if (*p == '\'')
> > +                       strbuf_splice(buf, p-- - buf->buf, 1, "", 0);
> 
> This part could be prefixed with
>     /* un-escape text: turn \\ into ' and remove single quotes. */

If could be prefixed that way, but it would be incorrect. We never turn \\
into '. What we do here (and I do not want to repeat in a comment what the
code does): we dequote what we previously quoted using single quotes. So
we use the fact that we know that the value is of the form 'abc', or if it
contains single quotes: 'this has '\''single'\'' quotes'.

> 
> > +       if (skip_prefix(buf->buf, "GIT_AUTHOR_NAME=", (const char **)&p)) {
> > +               strbuf_splice(buf, 0, p - buf->buf, "", 0);
> > +               p = strchr(buf->buf, '\n');
> > +               if (skip_prefix(p, "\nGIT_AUTHOR_EMAIL=", (const char **)&p2)) {
> > +                       strbuf_splice(buf, p - buf->buf, p2 - p, " <", 2);
> > +                       p = strchr(p, '\n');
> > +                       if (skip_prefix(p, "\nGIT_AUTHOR_DATE=@",
> > +                                       (const char **)&p2)) {
> > +                               strbuf_splice(buf, p - buf->buf, p2 - p,
> > +                                             "> ", 2);
> > +                               p = strchr(p, '\n');
> > +                               if (p) {
> > +                                       strbuf_setlen(buf, p - buf->buf);
> > +                                       return buf->buf;
> 
> So here we have read GIT_AUTHOR_NAME, _EMAIL
> and _DATE in that order and converted it to its form
> "name <email> date" in a single line.
> 
> It would be better to invert the conditions and keep
> the indentation level lower by:
> 
>     if (!skip_prefix(...))
>         goto warning_and_return;
>     strbuf_splice(...);
>     ...
> 
> I wondered if we want to factor out the conversion of
> "author string in commit form" to "author information
> in script form" into their own functions, and keep the reading
> writing out of them. But then again we only need them in
> these use cases for now, and such a refactoring can happen
> later if needed.

Right, but there *is* an opportunity now to share code with builtin/am.c.

> > +       warning(_("could not parse '%s'"), rebase_path_author_script());
> 
> This function needs all three environment variables in its correct order,
> which sounds a little brittle, but then again we do not expect manual
> editing of that file, but expect it to be written by Git.

It would not hurt to make it less brittle, I agree. Maybe I can do that
easily. If not, in my opinion it is not a big deal to expect that order,
for the reason you stated: we expect to have written the file ourselves.

> > @@ -1369,6 +1448,12 @@ static int is_fixup(enum todo_command command)
> >         return command == TODO_FIXUP || command == TODO_SQUASH;
> >  }
> >
> > +/* Does this command create a (non-merge) commit? */
> > +static int is_pick_or_similar(enum todo_command command)
> > +{
> > +       return command <= TODO_SQUASH;
> > +}
> 
> This code looks scary.
> Fortunately the enum todo_command hints that the order matters,
> such that we're probably protected from wild reordering in the future,
> however this implies that the section /* commands that handle commits */
> comes first and that TODO_SQUASH is the last entry of that section.
> So maybe we'd want to add a /* must be last in section */ to
> TODO_squash and also document that the section must be first?
> 
> Do we have other code that needs a very specific ordering
> with similar further assumptions (section being first/last, a
> command being first/last in their section)?

The code already does things like that, by testing `command <
TODO_COMMENT`.

But I guess your concerns would go away if I made this a big honking
switch() statement that lists *explicitly* what should be considered "pick
or similar"?

> I wondered what the _or_similar means and by looking up
> that enum, I would think a name like
> 
> static int handles_single_commit(enum todo_command)
> 
> might be better?

No, that would be incorrect. TODO_MERGE also *potentially* handles a
single commit (and one or more labels).

I really mean "is it a pick or similar", i.e. does it cherry-pick a
single, non-merge commit, possibly doing funny stuff such as `reword` on
top of it.

> > @@ -1523,7 +1608,14 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> >                         return error(_("your index file is unmerged."));
> >         } else {
> >                 unborn = get_oid("HEAD", &head);
> > -               if (unborn)
> > +               /* Do we want to generate a root commit? */
> > +               if (is_pick_or_similar(command) && opts->have_squash_onto &&
> > +                   !oidcmp(&head, &opts->squash_onto)) {
> > +                       if (is_fixup(command))
> > +                               return error(_("cannot fixup root commit"));
> 
> I would expect you also cannot squash into root commit?

In sequencer.c, `is_fixup()` really means "is it a fixup or a squash?". So
yes, you are correct that we also cannot squash into a root commit.

However, a squash is the same as a fixup with the only difference being
that the squash lets you edit the final commit message (and does not
comment out the squash commit's message, in contrast to fixup).

Is it really worth adding an ugly line break in the middle of the error()
statement just to say "cannot fixup/squash into root commit"? I'd rather
not.

Ciao,
Dscho

Re: [PATCH 3/6] rebase -i --root: let the sequencer handle even the initial part

[Johannes Schindelin]

Hi Stefan,

On Sat, 28 Apr 2018, Stefan Beller wrote:

> On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > In this developer's earlier attempt to accelerate interactive rebases by
> > converting large parts from Unix shell script into portable, performant
> > C, the --root handling was specifically excluded (to simplify the task a
> > little bit; it still took over a year to get that reduced set of patches
> > into Git proper).
> >
> > This patch ties up that loose end: now only --preserve-merges uses the
> > slow Unix shell script implementation to perform the interactive rebase.
> >
> > As the rebase--helper reports progress to stderr (unlike the scripted
> > interactive rebase, which reports it to stdout, of all places), we have
> > to adjust a couple of tests that did not expect that for `git rebase -i
> > --root`.
> >
> > This patch fixes -- at long last! -- the really old bug reported in
> > 6a6bc5bdc4d (add tests for rebasing root, 2013-06-06) that rebasing with
> > --root *always* rewrote the root commit, even if there were no changes.
> >
> > The bug still persists in --preserve-merges mode, of course, but that
> > mode will be deprecated as soon as the new --rebase-merges mode
> > stabilizes, anyway.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  git-rebase--interactive.sh        |  4 +++-
> >  t/t3404-rebase-interactive.sh     | 19 +++++++++++++------
> >  t/t3421-rebase-topology-linear.sh |  6 +++---
> >  3 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index cbf44f86482..2f4941d0fc9 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -894,6 +894,8 @@ init_revisions_and_shortrevisions () {
> >         else
> >                 revisions=$onto...$orig_head
> >                 shortrevisions=$shorthead
> > +               test -z "$squash_onto" ||
> > +               echo "$squash_onto" >"$state_dir"/squash-onto
> >         fi
> >  }
> >
> > @@ -948,7 +950,7 @@ EOF
> >         die "Could not skip unnecessary pick commands"
> >
> >         checkout_onto
> > -       if test -z "$rebase_root" && test ! -d "$rewritten"
> > +       if test ! -d "$rewritten"
> 
> I have the impression this is the line that is really well
> explained in the commit message ("migrate to rebase
> helper even when there is $rebase_root set")
> 
> The rest of the patch is covered as "a couple of places
> where we adjust stdout to stderr"?

Correct.

Thanks for reviewing!
Dscho

Re: [PATCH 2/6] sequencer: learn about the special "fake root commit" handling

[Stefan Beller]

Hi Johannes,

thanks for taking your time to explain things. It shows I am not
familiar with the rebase code, yet.

>
> Having said that, *this* time round, what we need to do is actually very
> similar to what builtin/am.c's read_author_script() does (even if we
> cannot use it as-is: it populates part of a `struct am_state`). I'll have
> to look into this more closely.

Heh, so my rambling was worth it. Thanks for looking into that.


>> This part could be prefixed with
>>     /* un-escape text: turn \\ into ' and remove single quotes. */
>
> If could be prefixed that way, but it would be incorrect. We never turn \\
> into '. What we do here (and I do not want to repeat in a comment what the
> code does): we dequote what we previously quoted using single quotes. So
> we use the fact that we know that the value is of the form 'abc', or if it
> contains single quotes: 'this has '\''single'\'' quotes'.

Is there a helper 'dequote' somewhere?

>> > +/* Does this command create a (non-merge) commit? */
>> > +static int is_pick_or_similar(enum todo_command command)
>> > +{
>> > +       return command <= TODO_SQUASH;
>> > +}
>>
>> This code looks scary.
[...]

> The code already does things like that, by testing `command <
> TODO_COMMENT`.

ok great. So that is a widely used pattern for enum todo_command,
such that rearranging their order would break a lot. (Hence people will
find out quickly when doing so).

> But I guess your concerns would go away if I made this a big honking
> switch() statement that lists *explicitly* what should be considered "pick
> or similar"?

I did not spell that out as producing lots of lines of code is not the
primary goal, but understandability is.

And it looked like it would just pick the first section, so I thought about
some different approaches, either by making the enum todo_command
a lot more complex than an enum (an array of structs?) or adding
a new parallel array, that contains additional information for each
value at the given index, e.g.

static int is_pick_or_similar(enum todo_command command)
{
    return todo_command_flags[value] & TODO_CMDS_IS_PICKING;
}

but all approaches I had were more complicated, such that the additional
verbosity would not be enough of a trade off.

I think this function was only scary as I was not familiar with the rebase
code as that employs similar patterns already.

>> > +                       if (is_fixup(command))
>> > +                               return error(_("cannot fixup root commit"));
>>
>> I would expect you also cannot squash into root commit?
>
> In sequencer.c, `is_fixup()` really means "is it a fixup or a squash?". So
> yes, you are correct that we also cannot squash into a root commit.
>
> However, a squash is the same as a fixup with the only difference being
> that the squash lets you edit the final commit message (and does not
> comment out the squash commit's message, in contrast to fixup).
>
> Is it really worth adding an ugly line break in the middle of the error()
> statement just to say "cannot fixup/squash into root commit"? I'd rather
> not.

Makes sense,

Thanks for your patience,
Stefan

[PATCH v2 0/6] Let the sequencer handle `git rebase -i --root`

[Johannes Schindelin]

When I reimplemented the most performance-critical bits of the
interactive rebase in the sequencer, to speed up `git rebase -i`
particularly on Windows (even if the benefits are still quite notable on
Linux or macOS), I punted on the --root part.

I had always hoped that some other contributor (or I myself) would come
back later to address the --root part in the sequencer, too, with the
idea that this would move the last remaining complicated code from
git-rebase--interactive.sh into sequencer.c, to facilitate converting
the rest of git-rebase--interactive.sh.

When I say "the last remaining complicated code", of course I neglect
the --preserve-merges code, but as I worked hard on the --rebase-merges
patch series with the intention to eventually deprecate and maybe even
remove the --preserve-merges mode, I always implicitly assume that the
--preserve-merges code will be moved into its own shell script
(git-rebase--preserve-merges.sh, maybe?) and never be converted.

So here goes: the patches to move the handling of --root into the
sequencer. After two preparatory patches, the real conversion takes
place in the third patch. After that, we take care of the --root related
concerns that arise in conjunction with the --rebase-merges mode.

As the --rebase-merges/--root patches overlap quite a bit (not so much
in the code itself as in philosophical considerations such as "what
should happen if you try to merge a branch into a new root", or the
fact that the label/reset/merge commands make it desirable to be able to
create a new root commit in the middle of a todo list), I had to
consider in which order to contribute them. In the end, I decided to go
with --rebase-merges first, so the --root patches are based on the
--rebase-merges patch series.

I consider this patch series a critical prerequisite for Alban's Google
Summer of Code project to convert rebase -i into a builtin.

Changes since v1:

- Expanded is_pick_or_similar() to use a clear and verbose switch()
  statement, replacing the magic "command <= TODO_SQUASH".

- Completely revamped read_author_ident(), using sq_dequote(); Sadly,
  trying to reuse builtin/am.c's read_author_script() would result in
  substantialy more lines of code, and I also failed to find an easy way
  to allow for arbitrary order of the environment variables in
  author-script.


Johannes Schindelin (6):
  sequencer: extract helper to update active_cache_tree
  sequencer: learn about the special "fake root commit" handling
  rebase -i --root: let the sequencer handle even the initial part
  sequencer: allow introducing new root commits
  rebase --rebase-merges: a "merge" into a new root is a fast-forward
  rebase --rebase-merges: root commits can be cousins, too

 git-rebase--interactive.sh        |   4 +-
 sequencer.c                       | 206 ++++++++++++++++++++++++++----
 sequencer.h                       |   4 +
 t/t3404-rebase-interactive.sh     |  19 ++-
 t/t3421-rebase-topology-linear.sh |   6 +-
 t/t3430-rebase-merges.sh          |  72 +++++++++++
 6 files changed, 276 insertions(+), 35 deletions(-)


base-commit: 673fb9cb8b5c7d57cb560b6ade45e419c8dd09fc
Based-On: recreate-merges at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git recreate-merges
Published-As: https://github.com/dscho/git/releases/tag/sequencer-and-root-commits-v2
Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-and-root-commits-v2

Branch-diff vs v1:
 1: 42db734a980 ! 1: 73398da7119 sequencer: learn about the special "fake root commit" handling
     @@ -54,40 +54,50 @@
       	return NULL;
       }
       
     ++/* Read author-script and return an ident line (author <email> timestamp) */
      +static const char *read_author_ident(struct strbuf *buf)
      +{
     -+	char *p, *p2;
     ++	const char *keys[] = {
     ++		"GIT_AUTHOR_NAME=", "GIT_AUTHOR_EMAIL=", "GIT_AUTHOR_DATE="
     ++	};
     ++	char *in, *out, *eol;
     ++	int i = 0, len;
      +
      +	if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
      +		return NULL;
      +
     -+	for (p = buf->buf; *p; p++)
     -+		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
     -+			strbuf_splice(buf, p - buf->buf, p2 - p, "'", 1);
     -+		else if (*p == '\'')
     -+			strbuf_splice(buf, p-- - buf->buf, 1, "", 0);
     -+
     -+	if (skip_prefix(buf->buf, "GIT_AUTHOR_NAME=", (const char **)&p)) {
     -+		strbuf_splice(buf, 0, p - buf->buf, "", 0);
     -+		p = strchr(buf->buf, '\n');
     -+		if (skip_prefix(p, "\nGIT_AUTHOR_EMAIL=", (const char **)&p2)) {
     -+			strbuf_splice(buf, p - buf->buf, p2 - p, " <", 2);
     -+			p = strchr(p, '\n');
     -+			if (skip_prefix(p, "\nGIT_AUTHOR_DATE=@",
     -+					(const char **)&p2)) {
     -+				strbuf_splice(buf, p - buf->buf, p2 - p,
     -+					      "> ", 2);
     -+				p = strchr(p, '\n');
     -+				if (p) {
     -+					strbuf_setlen(buf, p - buf->buf);
     -+					return buf->buf;
     -+				}
     -+			}
     ++	/* dequote values and construct ident line in-place */
     ++	for (in = out = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {
     ++		if (!skip_prefix(in, keys[i], (const char **)&in)) {
     ++			warning("could not parse '%s' (looking for '%s'",
     ++				rebase_path_author_script(), keys[i]);
     ++			return NULL;
      +		}
     -+	}
     -+
     -+	warning(_("could not parse '%s'"), rebase_path_author_script());
     -+	return NULL;
     ++
     ++		eol = strchrnul(in, '\n');
     ++		*eol = '\0';
     ++		sq_dequote(in);
     ++		len = strlen(in);
     ++
     ++		if (i > 0) /* separate values by spaces */
     ++			*(out++) = ' ';
     ++		if (i == 1) /* email needs to be surrounded by <...> */
     ++			*(out++) = '<';
     ++		memmove(out, in, len);
     ++		out += len;
     ++		if (i == 1) /* email needs to be surrounded by <...> */
     ++			*(out++) = '>';
     ++		in = eol + 1;
     ++	}
     ++
     ++	if (i < 3) {
     ++		warning("could not parse '%s' (looking for '%s')",
     ++			rebase_path_author_script(), keys[i]);
     ++		return NULL;
     ++	}
     ++
     ++	buf->len = out - buf->buf;
     ++	return buf->buf;
      +}
      +
       static const char staged_changes_advice[] =
     @@ -159,7 +169,17 @@
      +/* Does this command create a (non-merge) commit? */
      +static int is_pick_or_similar(enum todo_command command)
      +{
     -+	return command <= TODO_SQUASH;
     ++	switch (command) {
     ++	case TODO_PICK:
     ++	case TODO_REVERT:
     ++	case TODO_EDIT:
     ++	case TODO_REWORD:
     ++	case TODO_FIXUP:
     ++	case TODO_SQUASH:
     ++		return 1;
     ++	default:
     ++		return 0;
     ++	}
      +}
      +
       static int update_squash_messages(enum todo_command command,
 2: 1c8740eaa91 = 2: 2dfe8315993 rebase -i --root: let the sequencer handle even the initial part
 3: 8a7f5751412 = 3: 0ee765bbb36 sequencer: allow introducing new root commits
 4: 0b7379b576b = 4: 29d7a9265e3 rebase --rebase-merges: a "merge" into a new root is a fast-forward
 5: 270b8fdf477 = 5: f7ee1b3de12 rebase --rebase-merges: root commits can be cousins, too
-- 
2.17.0.windows.1.38.g05ca542f78d

[PATCH v2 1/6] sequencer: extract helper to update active_cache_tree

[Johannes Schindelin]

This patch extracts the code from is_index_unchanged() to initialize or
update the index' cache tree (i.e. a tree object reflecting the current
index' top-level tree).

The new helper will be used in the upcoming code to support `git rebase
-i --root` via the sequencer.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e2f83942843..90c8218aa9a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -562,9 +562,23 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	return !clean;
 }
 
+static struct object_id *get_cache_tree_oid(void)
+{
+	if (!active_cache_tree)
+		active_cache_tree = cache_tree();
+
+	if (!cache_tree_fully_valid(active_cache_tree))
+		if (cache_tree_update(&the_index, 0)) {
+			error(_("unable to update cache tree"));
+			return NULL;
+		}
+
+	return &active_cache_tree->oid;
+}
+
 static int is_index_unchanged(void)
 {
-	struct object_id head_oid;
+	struct object_id head_oid, *cache_tree_oid;
 	struct commit *head_commit;
 
 	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL))
@@ -583,15 +597,10 @@ static int is_index_unchanged(void)
 	if (parse_commit(head_commit))
 		return -1;
 
-	if (!active_cache_tree)
-		active_cache_tree = cache_tree();
-
-	if (!cache_tree_fully_valid(active_cache_tree))
-		if (cache_tree_update(&the_index, 0))
-			return error(_("unable to update cache tree"));
+	if (!(cache_tree_oid = get_cache_tree_oid()))
+		return -1;
 
-	return !oidcmp(&active_cache_tree->oid,
-		       &head_commit->tree->object.oid);
+	return !oidcmp(cache_tree_oid, &head_commit->tree->object.oid);
 }
 
 static int write_author_script(const char *message)
-- 
2.17.0.windows.1.38.g05ca542f78d

[PATCH v2 2/6] sequencer: learn about the special "fake root commit" handling

[Johannes Schindelin]

When an interactive rebase wants to recreate a root commit, it
- first creates a new, empty root commit,
- checks it out,
- converts the next `pick` command so that it amends the empty root
  commit

Introduce support in the sequencer to handle such an empty root commit,
by looking for the file <GIT_DIR>/rebase-merge/squash-onto; if it exists
and contains a commit name, the sequencer will compare the HEAD to said
root commit, and if identical, a new root commit will be created.

While converting scripted code into proper, portable C, we also do away
with the old "amend with an empty commit message, then cherry-pick
without committing, then amend again" dance and replace it with code
that uses the internal API properly to do exactly what we want: create a
new root commit.

To keep the implementation simple, we always spawn `git commit` to create
new root commits.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 sequencer.h |   4 ++
 2 files changed, 125 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 90c8218aa9a..8eddda681d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -125,6 +125,12 @@ static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
 static GIT_PATH_FUNC(rebase_path_rewritten_pending,
 	"rebase-merge/rewritten-pending")
 
+/*
+ * The path of the file containig the OID of the "squash onto" commit, i.e.
+ * the dummy commit used for `reset [new root]`.
+ */
+static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
+
 /*
  * The path of the file listing refs that need to be deleted after the rebase
  * finishes. This is used by the `label` command to record the need for cleanup.
@@ -470,7 +476,8 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD",
-				   to, unborn ? &null_oid : from,
+				   to, unborn && !is_rebase_i(opts) ?
+				   &null_oid : from,
 				   0, sb.buf, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
@@ -692,6 +699,52 @@ 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)
+{
+	const char *keys[] = {
+		"GIT_AUTHOR_NAME=", "GIT_AUTHOR_EMAIL=", "GIT_AUTHOR_DATE="
+	};
+	char *in, *out, *eol;
+	int i = 0, len;
+
+	if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
+		return NULL;
+
+	/* dequote values and construct ident line in-place */
+	for (in = out = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {
+		if (!skip_prefix(in, keys[i], (const char **)&in)) {
+			warning("could not parse '%s' (looking for '%s'",
+				rebase_path_author_script(), keys[i]);
+			return NULL;
+		}
+
+		eol = strchrnul(in, '\n');
+		*eol = '\0';
+		sq_dequote(in);
+		len = strlen(in);
+
+		if (i > 0) /* separate values by spaces */
+			*(out++) = ' ';
+		if (i == 1) /* email needs to be surrounded by <...> */
+			*(out++) = '<';
+		memmove(out, in, len);
+		out += len;
+		if (i == 1) /* email needs to be surrounded by <...> */
+			*(out++) = '>';
+		in = eol + 1;
+	}
+
+	if (i < 3) {
+		warning("could not parse '%s' (looking for '%s')",
+			rebase_path_author_script(), keys[i]);
+		return NULL;
+	}
+
+	buf->len = out - buf->buf;
+	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"
@@ -711,6 +764,7 @@ N_("you have staged changes in your working tree\n"
 #define AMEND_MSG   (1<<2)
 #define CLEANUP_MSG (1<<3)
 #define VERIFY_MSG  (1<<4)
+#define CREATE_ROOT_COMMIT (1<<5)
 
 /*
  * If we are cherry-pick, and if the merge did not result in
@@ -730,6 +784,40 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	const char *value;
 
+	if (flags & CREATE_ROOT_COMMIT) {
+		struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
+		const char *author = is_rebase_i(opts) ?
+			read_author_ident(&script) : NULL;
+		struct object_id root_commit, *cache_tree_oid;
+		int res = 0;
+
+		if (!defmsg)
+			BUG("root commit without message");
+
+		if (!(cache_tree_oid = get_cache_tree_oid()))
+			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) {
+			update_ref(NULL, "CHERRY_PICK_HEAD", &root_commit, NULL,
+				   REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR);
+			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)) {
@@ -1216,7 +1304,8 @@ static int do_commit(const char *msg_file, const char *author,
 {
 	int res = 1;
 
-	if (!(flags & EDIT_MSG) && !(flags & VERIFY_MSG)) {
+	if (!(flags & EDIT_MSG) && !(flags & VERIFY_MSG) &&
+	    !(flags & CREATE_ROOT_COMMIT)) {
 		struct object_id oid;
 		struct strbuf sb = STRBUF_INIT;
 
@@ -1369,6 +1458,22 @@ static int is_fixup(enum todo_command command)
 	return command == TODO_FIXUP || command == TODO_SQUASH;
 }
 
+/* Does this command create a (non-merge) commit? */
+static int is_pick_or_similar(enum todo_command command)
+{
+	switch (command) {
+	case TODO_PICK:
+	case TODO_REVERT:
+	case TODO_EDIT:
+	case TODO_REWORD:
+	case TODO_FIXUP:
+	case TODO_SQUASH:
+		return 1;
+	default:
+		return 0;
+	}
+}
+
 static int update_squash_messages(enum todo_command command,
 		struct commit *commit, struct replay_opts *opts)
 {
@@ -1523,7 +1628,14 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			return error(_("your index file is unmerged."));
 	} else {
 		unborn = get_oid("HEAD", &head);
-		if (unborn)
+		/* Do we want to generate a root commit? */
+		if (is_pick_or_similar(command) && opts->have_squash_onto &&
+		    !oidcmp(&head, &opts->squash_onto)) {
+			if (is_fixup(command))
+				return error(_("cannot fixup root commit"));
+			flags |= CREATE_ROOT_COMMIT;
+			unborn = 1;
+		} else if (unborn)
 			oidcpy(&head, the_hash_algo->empty_tree);
 		if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD",
 				       NULL, 0))
@@ -2136,6 +2248,12 @@ static int read_populate_opts(struct replay_opts *opts)
 		read_strategy_opts(opts, &buf);
 		strbuf_release(&buf);
 
+		if (read_oneliner(&buf, rebase_path_squash_onto(), 0)) {
+			if (get_oid_hex(buf.buf, &opts->squash_onto) < 0)
+				return error(_("unusable squash-onto"));
+			opts->have_squash_onto = 1;
+		}
+
 		return 0;
 	}
 
diff --git a/sequencer.h b/sequencer.h
index d9570d92b11..4b2717881fa 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -44,6 +44,10 @@ struct replay_opts {
 	char **xopts;
 	size_t xopts_nr, xopts_alloc;
 
+	/* placeholder commit for -i --root */
+	struct object_id squash_onto;
+	int have_squash_onto;
+
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
 };
-- 
2.17.0.windows.1.38.g05ca542f78d

[PATCH v2 3/6] rebase -i --root: let the sequencer handle even the initial part

[Johannes Schindelin]

In this developer's earlier attempt to accelerate interactive rebases by
converting large parts from Unix shell script into portable, performant
C, the --root handling was specifically excluded (to simplify the task a
little bit; it still took over a year to get that reduced set of patches
into Git proper).

This patch ties up that loose end: now only --preserve-merges uses the
slow Unix shell script implementation to perform the interactive rebase.

As the rebase--helper reports progress to stderr (unlike the scripted
interactive rebase, which reports it to stdout, of all places), we have
to adjust a couple of tests that did not expect that for `git rebase -i
--root`.

This patch fixes -- at long last! -- the really old bug reported in
6a6bc5bdc4d (add tests for rebasing root, 2013-06-06) that rebasing with
--root *always* rewrote the root commit, even if there were no changes.

The bug still persists in --preserve-merges mode, of course, but that
mode will be deprecated as soon as the new --rebase-merges mode
stabilizes, anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh        |  4 +++-
 t/t3404-rebase-interactive.sh     | 19 +++++++++++++------
 t/t3421-rebase-topology-linear.sh |  6 +++---
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cbf44f86482..2f4941d0fc9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -894,6 +894,8 @@ init_revisions_and_shortrevisions () {
 	else
 		revisions=$onto...$orig_head
 		shortrevisions=$shorthead
+		test -z "$squash_onto" ||
+		echo "$squash_onto" >"$state_dir"/squash-onto
 	fi
 }
 
@@ -948,7 +950,7 @@ EOF
 	die "Could not skip unnecessary pick commands"
 
 	checkout_onto
-	if test -z "$rebase_root" && test ! -d "$rewritten"
+	if test ! -d "$rewritten"
 	then
 		require_clean_work_tree "rebase"
 		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 59c766540e5..c65826ddace 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1204,10 +1204,6 @@ test_expect_success 'drop' '
 	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
 '
 
-cat >expect <<EOF
-Successfully rebased and updated refs/heads/missing-commit.
-EOF
-
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 	test_config rebase.missingCommitsCheck ignore &&
 	rebase_setup_and_clean missing-commit &&
@@ -1215,7 +1211,9 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 	FAKE_LINES="1 2 3 4" \
 		git rebase -i --root 2>actual &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
-	test_i18ncmp expect actual
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit" \
+		actual
 '
 
 cat >expect <<EOF
@@ -1227,15 +1225,24 @@ To avoid this message, use "drop" to explicitly remove a commit.
 Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 The possible behaviours are: ignore, warn, error.
 
+Rebasing (1/4)
+Rebasing (2/4)
+Rebasing (3/4)
+Rebasing (4/4)
 Successfully rebased and updated refs/heads/missing-commit.
 EOF
 
+cr_to_nl () {
+	tr '\015' '\012'
+}
+
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
 	test_config rebase.missingCommitsCheck warn &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
 	FAKE_LINES="1 2 3 4" \
-		git rebase -i --root 2>actual &&
+		git rebase -i --root 2>actual.2 &&
+	cr_to_nl <actual.2 >actual &&
 	test_i18ncmp expect actual &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p)
 '
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index e7438ad06ac..99b2aac9219 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -328,9 +328,9 @@ test_run_rebase () {
 		test_cmp_rev c HEAD
 	"
 }
-test_run_rebase failure ''
-test_run_rebase failure -m
-test_run_rebase failure -i
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
 test_run_rebase failure -p
 
 test_run_rebase () {
-- 
2.17.0.windows.1.38.g05ca542f78d

[PATCH v2 4/6] sequencer: allow introducing new root commits

[Johannes Schindelin]

In the context of the new --rebase-merges mode, which was designed
specifically to allow for changing the existing branch topology
liberally, a user may want to extract commits into a completely fresh
branch that starts with a newly-created root commit.

This is now possible by inserting the command `reset [new root]` before
`pick`ing the commit that wants to become a root commit. Example:

	reset [new root]
	pick 012345 a commit that is about to become a root commit
	pick 234567 this commit will have the previous one as parent

This does not conflict with other uses of the `reset` command because
`[new root]` is not (part of) a valid ref name: both the opening bracket
as well as the space are illegal in ref names.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c              | 40 ++++++++++++++++++++++++++++------------
 t/t3430-rebase-merges.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8eddda681d1..a7832399b1f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2747,18 +2747,34 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
 		return -1;
 
-	/* Determine the length of the label */
-	for (i = 0; i < len; i++)
-		if (isspace(name[i]))
-			len = i;
-
-	strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
-	if (get_oid(ref_name.buf, &oid) &&
-	    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
-		error(_("could not read '%s'"), ref_name.buf);
-		rollback_lock_file(&lock);
-		strbuf_release(&ref_name);
-		return -1;
+	if (len == 10 && !strncmp("[new root]", name, len)) {
+		if (!opts->have_squash_onto) {
+			const char *hex;
+			if (commit_tree("", 0, the_hash_algo->empty_tree,
+					NULL, &opts->squash_onto,
+					NULL, NULL))
+				return error(_("writing fake root commit"));
+			opts->have_squash_onto = 1;
+			hex = oid_to_hex(&opts->squash_onto);
+			if (write_message(hex, strlen(hex),
+					  rebase_path_squash_onto(), 0))
+				return error(_("writing squash-onto"));
+		}
+		oidcpy(&oid, &opts->squash_onto);
+	} else {
+		/* Determine the length of the label */
+		for (i = 0; i < len; i++)
+			if (isspace(name[i]))
+				len = i;
+
+		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
+		if (get_oid(ref_name.buf, &oid) &&
+		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
+			error(_("could not read '%s'"), ref_name.buf);
+			rollback_lock_file(&lock);
+			strbuf_release(&ref_name);
+			return -1;
+		}
 	}
 
 	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 3d4dfdf7bec..35260862fcb 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -241,4 +241,38 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
 	test_cmp_rev HEAD $before
 '
 
+test_expect_success 'root commits' '
+	git checkout --orphan unrelated &&
+	(GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
+	 test_commit second-root) &&
+	test_commit third-root &&
+	cat >script-from-scratch <<-\EOF &&
+	pick third-root
+	label first-branch
+	reset [new root]
+	pick second-root
+	merge first-branch # Merge the 3rd root
+	EOF
+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	git rebase -i --force --root -r &&
+	test "Parsnip" = "$(git show -s --format=%an HEAD^)" &&
+	test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
+	test $(git rev-parse second-root:second-root.t) = \
+		$(git rev-parse HEAD^:second-root.t) &&
+	test_cmp_graph HEAD <<-\EOF &&
+	*   Merge the 3rd root
+	|\
+	| * third-root
+	* second-root
+	EOF
+
+	: fast forward if possible &&
+	before="$(git rev-parse --verify HEAD)" &&
+	test_might_fail git config --unset sequence.editor &&
+	test_tick &&
+	git rebase -i --root -r &&
+	test_cmp_rev HEAD $before
+'
+
 test_done
-- 
2.17.0.windows.1.38.g05ca542f78d

[PATCH v2 5/6] rebase --rebase-merges: a "merge" into a new root is a fast-forward

[Johannes Schindelin]

When a user provides a todo list containing something like

	reset [new root]
	merge my-branch

let's do the same as if pulling into an orphan branch: simply
fast-forward.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c              | 12 ++++++++++++
 t/t3430-rebase-merges.sh | 13 +++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index a7832399b1f..65a8c493781 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2870,6 +2870,18 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 		goto leave_merge;
 	}
 
+	if (opts->have_squash_onto &&
+	    !oidcmp(&head_commit->object.oid, &opts->squash_onto)) {
+		/*
+		 * When the user tells us to "merge" something into a
+		 * "[new root]", let's simply fast-forward to the merge head.
+		 */
+		rollback_lock_file(&lock);
+		ret = fast_forward_to(&merge_commit->object.oid,
+				       &head_commit->object.oid, 0, opts);
+		goto leave_merge;
+	}
+
 	if (commit) {
 		const char *message = get_commit_buffer(commit, NULL);
 		const char *body;
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 35260862fcb..5543f1d5a34 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -275,4 +275,17 @@ test_expect_success 'root commits' '
 	test_cmp_rev HEAD $before
 '
 
+test_expect_success 'a "merge" into a root commit is a fast-forward' '
+	head=$(git rev-parse HEAD) &&
+	cat >script-from-scratch <<-EOF &&
+	reset [new root]
+	merge $head
+	EOF
+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	git rebase -i -r HEAD^ &&
+	test_cmp_rev HEAD $head
+'
+
+
 test_done
-- 
2.17.0.windows.1.38.g05ca542f78d

[PATCH v2 6/6] rebase --rebase-merges: root commits can be cousins, too

[Johannes Schindelin]

Reported by Wink Saville: when rebasing with no-rebase-cousins, we
will want to refrain from rebasing all of them, even when they are
root commits.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c              |  3 ++-
 t/t3430-rebase-merges.sh | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 65a8c493781..01e561bc20e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3903,7 +3903,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		}
 
 		if (!commit)
-			fprintf(out, "%s onto\n", cmd_reset);
+			fprintf(out, "%s %s\n", cmd_reset,
+				rebase_cousins ? "onto" : "[new root]");
 		else {
 			const char *to = NULL;
 
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 5543f1d5a34..ce6de6f491e 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -287,5 +287,30 @@ test_expect_success 'a "merge" into a root commit is a fast-forward' '
 	test_cmp_rev HEAD $head
 '
 
+test_expect_success 'A root commit can be a cousin, treat it that way' '
+	git checkout --orphan khnum &&
+	test_commit yama &&
+	git checkout -b asherah master &&
+	test_commit shamkat &&
+	git merge --allow-unrelated-histories khnum &&
+	test_tick &&
+	git rebase -f -r HEAD^ &&
+	! test_cmp_rev HEAD^2 khnum &&
+	test_cmp_graph HEAD^.. <<-\EOF &&
+	*   Merge branch '\''khnum'\'' into asherah
+	|\
+	| * yama
+	o shamkat
+	EOF
+	test_tick &&
+	git rebase --rebase-merges=rebase-cousins HEAD^ &&
+	test_cmp_graph HEAD^.. <<-\EOF
+	*   Merge branch '\''khnum'\'' into asherah
+	|\
+	| * yama
+	|/
+	o shamkat
+	EOF
+'
 
 test_done
-- 
2.17.0.windows.1.38.g05ca542f78d

Re: [PATCH v2 0/6] Let the sequencer handle `git rebase -i --root`

[Stefan Beller]

>
> Branch-diff vs v1:
>  1: 42db734a980 ! 1: 73398da7119 sequencer: learn about the special "fake root commit" handling
>      @@ -54,40 +54,50 @@
>         return NULL;
>        }
>
>      ++/* Read author-script and return an ident line (author <email> timestamp) */
>       +static const char *read_author_ident(struct strbuf *buf)


I like the new way of read_author_ident. Thanks for writing it!


>       +
>        static const char staged_changes_advice[] =
>      @@ -159,7 +169,17 @@
>       +/* Does this command create a (non-merge) commit? */
>       +static int is_pick_or_similar(enum todo_command command)
>       +{
>      -+ return command <= TODO_SQUASH;
>      ++ switch (command) {
>      ++ case TODO_PICK:
>      ++ case TODO_REVERT:
>      ++ case TODO_EDIT:
>      ++ case TODO_REWORD:
>      ++ case TODO_FIXUP:
>      ++ case TODO_SQUASH:
>      ++         return 1;
>      ++ default:
>      ++         return 0;
>      ++ }

The switch case is not as bad as I thought following the discussion on of v1.

This series is
Reviewed-by: Stefan Beller <sbeller@google.com>

Thanks!

<off topic>
During a lunch discussion I wondered if the branch diff format could lead to
another form of machine readable communication, i.e. if we want to add the
ability to read the branch diff format and apply the changes. Note how applying
this diff-diff would not create new commits, but rather amend existing commits.

Re: [PATCH v2 0/6] Let the sequencer handle `git rebase -i --root`

[Johannes Schindelin]

Hi Stefan,

On Fri, 4 May 2018, Stefan Beller wrote:

> > Branch-diff vs v1:
> >  1: 42db734a980 ! 1: 73398da7119 sequencer: learn about the special "fake root commit" handling
> >      @@ -54,40 +54,50 @@
> >         return NULL;
> >        }
> >
> >      ++/* Read author-script and return an ident line (author <email> timestamp) */
> >       +static const char *read_author_ident(struct strbuf *buf)
> 
> 
> I like the new way of read_author_ident. Thanks for writing it!

You're welcome. After sleeping a night over it, I think this function
might also benefit from a new name: extract_ident_from_author_script().
What do you think?

> >      @@ -159,7 +169,17 @@
> >       +/* Does this command create a (non-merge) commit? */
> >       +static int is_pick_or_similar(enum todo_command command)
> >       +{
> >      -+ return command <= TODO_SQUASH;
> >      ++ switch (command) {
> >      ++ case TODO_PICK:
> >      ++ case TODO_REVERT:
> >      ++ case TODO_EDIT:
> >      ++ case TODO_REWORD:
> >      ++ case TODO_FIXUP:
> >      ++ case TODO_SQUASH:
> >      ++         return 1;
> >      ++ default:
> >      ++         return 0;
> >      ++ }
> 
> The switch case is not as bad as I thought following the discussion on of v1.

Yes, it makes things explicit, and it is not too long a case-chain.

> This series is
> Reviewed-by: Stefan Beller <sbeller@google.com>

Thanks!

> <off topic>
> During a lunch discussion I wondered if the branch diff format could lead to
> another form of machine readable communication, i.e. if we want to add the
> ability to read the branch diff format and apply the changes. Note how applying
> this diff-diff would not create new commits, but rather amend existing commits.

</off topic> (which BTW is not valid XML)

I do not think that it would be a wise idea to detour even further from
Git when exchanging patch series iterations. We have a lovely way to
exchange commits, after all: `git fetch` and `git push`, and for times you
cannot agree on a central server, `git bundle`.

Ciao,
Dscho