threads / patch / 48372

patch, 6 partsLet the sequencer handle `git rebase -i --root`

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

## tl;dr

22 messages between Apr 27, 2018 and May 5, 2018. Diffs are folded; open one to read it.

replies: 21people: 2as markdown or json

Johannes Schindelin· Apr 27, 2018, 22:29 UTC · lore

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
Johannes Schindelin· Apr 27, 2018, 22:30 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to sequencer.c +18 −9
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
Stefan Beller· Apr 28, 2018, 15:28 UTC · re: Johannes Schindelin · lore

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

Hi Johannes,

On Fri, Apr 27, 2018 at 3:30 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 30 quoted lines
> 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>
Johannes Schindelin· Apr 27, 2018, 22:31 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +105 −3

sequencer.c, sequencer.h

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
Stefan Beller· Apr 28, 2018, 16:11 UTC · re: Johannes Schindelin · lore

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

On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 58 quoted lines
> 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.

Show 5 quoted lines
> +       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. */
Show 14 quoted lines
> +       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.

Show 9 quoted lines
> @@ -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?
Show 10 quoted lines
> @@ -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

Johannes Schindelin· Apr 29, 2018, 12:33 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Sat, 28 Apr 2018, Stefan Beller wrote:
Show 64 quoted lines
> 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.

Show 8 quoted lines
> > +{
> > +       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.
Show 8 quoted lines
> > +       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'.

Show 34 quoted lines
> 
> > +       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.
Show 5 quoted lines
> > +       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.

Show 21 quoted lines
> > @@ -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"?

Show 6 quoted lines
> 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.

Show 12 quoted lines
> > @@ -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

Stefan Beller· Apr 29, 2018, 21:44 UTC · re: Johannes Schindelin · lore

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

Hi Johannes,

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

Show 5 quoted lines
>
> 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.
Show 8 quoted lines
>> 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?
Show 7 quoted lines
>> > +/* 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.

Show 15 quoted lines
>> > +                       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

Johannes Schindelin· Apr 27, 2018, 22:31 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 3 files +19 −10

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh, t/t3421-rebase-topology-linear.sh

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
Stefan Beller· Apr 28, 2018, 16:19 UTC · re: Johannes Schindelin · lore

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

On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 48 quoted lines
> 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

Johannes Schindelin· Apr 29, 2018, 12:34 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Sat, 28 Apr 2018, Stefan Beller wrote:
Show 57 quoted lines
> 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

Johannes Schindelin· Apr 27, 2018, 22:31 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +62 −12

sequencer.c, t/t3430-rebase-merges.sh

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
Johannes Schindelin· Apr 27, 2018, 22:31 UTC · re: Johannes Schindelin · lore

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

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(+)
Show changes to 2 files +25 −0

sequencer.c, t/t3430-rebase-merges.sh

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
Johannes Schindelin· Apr 27, 2018, 22:31 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +27 −1

sequencer.c, t/t3430-rebase-merges.sh

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
Johannes Schindelin· May 3, 2018, 23:01 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· May 3, 2018, 23:01 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to sequencer.c +18 −9
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
Johannes Schindelin· May 3, 2018, 23:01 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 3 files +19 −10

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh, t/t3421-rebase-topology-linear.sh

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
Johannes Schindelin· May 3, 2018, 23:01 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +125 −3

sequencer.c, sequencer.h

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
Johannes Schindelin· May 3, 2018, 23:01 UTC · re: Johannes Schindelin · lore

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

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(+)
Show changes to 2 files +25 −0

sequencer.c, t/t3430-rebase-merges.sh

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
Johannes Schindelin· May 3, 2018, 23:01 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +27 −1

sequencer.c, t/t3430-rebase-merges.sh

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
Johannes Schindelin· May 3, 2018, 23:01 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +62 −12

sequencer.c, t/t3430-rebase-merges.sh

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
Stefan Beller· May 4, 2018, 19:55 UTC · re: Johannes Schindelin · lore

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

Show 9 quoted lines
>
> 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!
Show 18 quoted lines
>       +
>        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.

Johannes Schindelin· May 5, 2018, 19:24 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Fri, 4 May 2018, Stefan Beller wrote:
Show 11 quoted lines
> > 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?

Show 18 quoted lines
> >      @@ -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!
Show 5 quoted lines
> <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

← back to recent threads