patch, 8 partsrebase -i: offer to recreate merge commits

> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
>         label <name>
>         reset <name>
>
> As a convenience shortcut, also to improve readability of the generated
> todo list, a third command is introduced: bud. It simply resets to the
> "onto" revision, i.e. the commit onto which we currently rebase.
>

> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> >         label <name>
> >         reset <name>
> >
> > As a convenience shortcut, also to improve readability of the generated
> > todo list, a third command is introduced: bud. It simply resets to the
> > "onto" revision, i.e. the commit onto which we currently rebase.
> >
> 
> The code looks good, but I'm a little wary of adding bud which
> hard-codes a specific label. I suppose it does grant a bit of
> readability to the resulting script... ? It doesn't seem that
> important compared to use using "reset onto"? At least when
> documenting this it should be made clear that the "onto" label is
> special.

> Hi Jake,
>
> On Thu, 18 Jan 2018, Jacob Keller wrote:
>
>> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> > This commit implements the commands to label, and to reset to, given
>> > revisions. The syntax is:
>> >
>> >         label <name>
>> >         reset <name>
>> >
>> > As a convenience shortcut, also to improve readability of the generated
>> > todo list, a third command is introduced: bud. It simply resets to the
>> > "onto" revision, i.e. the commit onto which we currently rebase.
>> >
>>
>> The code looks good, but I'm a little wary of adding bud which
>> hard-codes a specific label. I suppose it does grant a bit of
>> readability to the resulting script... ? It doesn't seem that
>> important compared to use using "reset onto"? At least when
>> documenting this it should be made clear that the "onto" label is
>> special.
>
> Indeed, `bud` helped me more in the earlier times of the Git garden shears
> when I had to *introduce* branch structure into the long list of Git for
> Windows' patches.
>
> If there are no voices in favor of `bud` other than mine, I will remove
> support for it in the next iteration.
>
> Ciao,
> Dscho

> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> This commit implements the commands to label, and to reset to, given
>> revisions. The syntax is:
>>
>>         label <name>
>>         reset <name>
>>
>> As a convenience shortcut, also to improve readability of the generated
>> todo list, a third command is introduced: bud. It simply resets to the
>> "onto" revision, i.e. the commit onto which we currently rebase.
>>
>
> The code looks good, but I'm a little wary of adding bud which
> hard-codes a specific label. I suppose it does grant a bit of
> readability to the resulting script... ? It doesn't seem that
> important compared to use using "reset onto"? At least when
> documenting this it should be made clear that the "onto" label is
> special.
>
> Thanks,
> Jake.

> From: "Jacob Keller" <jacob.keller@gmail.com>
>
>> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>>>
>>> This commit implements the commands to label, and to reset to, given
>>> revisions. The syntax is:
>>>
>>>         label <name>
>>>         reset <name>
>>>
>>> As a convenience shortcut, also to improve readability of the generated
>>> todo list, a third command is introduced: bud. It simply resets to the
>>> "onto" revision, i.e. the commit onto which we currently rebase.
>>>
>>
>> The code looks good, but I'm a little wary of adding bud which
>> hard-codes a specific label. I suppose it does grant a bit of
>> readability to the resulting script... ? It doesn't seem that
>> important compared to use using "reset onto"? At least when
>> documenting this it should be made clear that the "onto" label is
>> special.
>>
>> Thanks,
>> Jake.
>
>
> I'd agree.
>
> The special 'onto' label should be fully documented, and the commit message
> should indicate which patch actually defines it (and all its corner cases
> and fall backs if --onto isn't explicitly given..)

>
> Likewise the choice of 'bud' should be explained with some nice phraseology
> indicating that we are growing the new flowering from the bud, otherwise the
> word is a bit too short and sudden for easy explanation.
>
> Philip

> From: "Jacob Keller" <jacob.keller@gmail.com>
> > On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> > <johannes.schindelin@gmx.de> wrote:
> > > This commit implements the commands to label, and to reset to, given
> > > revisions. The syntax is:
> > >
> > >         label <name>
> > >         reset <name>
> > >
> > > As a convenience shortcut, also to improve readability of the generated
> > > todo list, a third command is introduced: bud. It simply resets to the
> > > "onto" revision, i.e. the commit onto which we currently rebase.
> > >
> >
> > The code looks good, but I'm a little wary of adding bud which
> > hard-codes a specific label. I suppose it does grant a bit of
> > readability to the resulting script... ? It doesn't seem that
> > important compared to use using "reset onto"? At least when
> > documenting this it should be made clear that the "onto" label is
> > special.
> >
> > Thanks,
> > Jake.
> 
> I'd agree.
> 
> The special 'onto' label should be fully documented, and the commit message
> should indicate which patch actually defines it (and all its corner cases and
> fall backs if --onto isn't explicitly given..)

> The code looks good, but I'm a little wary of adding bud which
> hard-codes a specific label. I suppose it does grant a bit of
> readability to the resulting script... ? It doesn't seem that
> important compared to use using "reset onto"? At least when
> documenting this it should be made clear that the "onto" label is
> special.

> Jacob Keller <jacob.keller@gmail.com> writes:
> 
> > The code looks good, but I'm a little wary of adding bud which
> > hard-codes a specific label. I suppose it does grant a bit of
> > readability to the resulting script... ? It doesn't seem that
> > important compared to use using "reset onto"? At least when
> > documenting this it should be made clear that the "onto" label is
> > special.
> 
> I do not think we would mind "bud" too much in the end result, but
> the change in 1/8 is made harder to read than necessary with it.  It
> is the only thing that needs "a single-letter command name may now
> not have any argument after it" change to the parser among the three
> things being added here, and it also needs to be added to the list
> of special commands without arguments.
> 
> It would have been easier to reason about if addition of "bud" was
> in its own patch done after label and reset are added.  And if done
> as a separate step, perhaps it would have been easier to realize
> that it would be a more future-proof solution for handling the
> "special" ness of BUD to add a new "unsigned flags" word to
> todo_command_info[] structure and using a bit that says "this does
> not take an arg" than to hardcode "noop and bud are the commands
> without args" in the code.  That hardcode was good enough when there
> was only one thing in that special case.  Now it has two.

> In a similar way, the code to special case label and reset just like
> exec may also want to become more table driven, perhaps using
> another bit in the same new flags word to say "this does not refer
> to commit".  I think that can become [v2 1/N] while addition of "bud"
> can be [v2 2/N] (after all, "bud" just does the same do_reset() with
> hardcoded argument, so "label/reset" must come first).

> [...]
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
>         label <name>
>         reset <name>
> [...]
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -1919,6 +1936,139 @@ static int do_exec(const char *command_line)
> +static int safe_append(const char *filename, const char *fmt, ...)
> +{
> +       [...]
> +       if (commit_lock_file(&lock) < 0) {
> +               rollback_lock_file(&lock);
> +               return error(_("failed to finalize '%s'."), filename);

> +       }
> +
> +       return 0;
> +}
> +
> +static int do_reset(const char *name, int len)
> +{
> +       [...]
> +       if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> +               return -1;
> +
> +       for (i = 0; i < len; i++)
> +               if (isspace(name[i]))
> +                       len = i;

> +               rollback_lock_file(&lock);
> +               free((void *)desc.buffer);
> +               strbuf_release(&ref_name);
> +               return -1;
> +       }

>> +static int do_reset(const char *name, int len)
>> +{
>> +       [...]
>> +       if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
>> +               return -1;
>> +
>> +       for (i = 0; i < len; i++)
>> +               if (isspace(name[i]))
>> +                       len = i;
>
> What is the purpose of this loop? I could imagine that it's trying to
> strip all whitespace from the end of 'name', however, to do that it
> would iterate backward, not forward. (Or perhaps it's trying to
> truncate at the first space, but then it would need to invert the
> condition or use 'break'.) Am I missing something obvious?

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> +static int do_reset(const char *name, int len)
> >> +{
> >> +       [...]
> >> +       if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> >> +               return -1;
> >> +
> >> +       for (i = 0; i < len; i++)
> >> +               if (isspace(name[i]))
> >> +                       len = i;
> >
> > What is the purpose of this loop? I could imagine that it's trying to
> > strip all whitespace from the end of 'name', however, to do that it
> > would iterate backward, not forward. (Or perhaps it's trying to
> > truncate at the first space, but then it would need to invert the
> > condition or use 'break'.) Am I missing something obvious?
> 
> I must be missing the same thing.  Given that the callers of
> do_reset(), other than the "bug" thing that passes the hard coded
> "onto", uses item->arg/item->arg_len which includes everything after
> the insn word on the line in the todo list, I do suspect that the
> intention is to stop at the first whitespace char to avoid creating
> a ref with whitespace in it, i.e. it is a bug that can be fixed with
> s/len = i/break/.
> 
> The code probably should further check the resulting string with
> check_ref_format() to detect strange chars and char sequences that
> make the resulting refname invalid.  For example, you would not want
> to allow a label with two consecutive periods in it.

> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > [...]
> > +static int do_reset(const char *name, int len)
> > +{
> > +       [...]
> > +       if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > +               return -1;
> > +
> > +       for (i = 0; i < len; i++)
> > +               if (isspace(name[i]))
> > +                       len = i;
> 
> What is the purpose of this loop? I could imagine that it's trying to
> strip all whitespace from the end of 'name', however, to do that it
> would iterate backward, not forward. (Or perhaps it's trying to
> truncate at the first space, but then it would need to invert the
> condition or use 'break'.) Am I missing something obvious?

> On Fri, 19 Jan 2018, Eric Sunshine wrote:
>> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> > +static int do_reset(const char *name, int len)
>> > +{
>> > +       for (i = 0; i < len; i++)
>> > +               if (isspace(name[i]))
>> > +                       len = i;
>>
>> What is the purpose of this loop? I could imagine that it's trying to
>> strip all whitespace from the end of 'name', however, to do that it
>> would iterate backward, not forward. (Or perhaps it's trying to
>> truncate at the first space, but then it would need to invert the
>> condition or use 'break'.) Am I missing something obvious?
>
> Yes, you are missing something obvious. The idea of the `reset` command is
> that it not only has a label, but also the oneline of the original commit:
>
>         reset branch-point sequencer: prepare for cleanup
>
> In this instance, `branch-point` is the label. And for convenience of the
> person editing, it also has the oneline.

> In the Git garden shears, I separated the two arguments via `#`:
>
>         reset branch-point # sequencer: prepare for cleanup
>
> I guess that is actually more readable, so I will introduce that into this
> patch series, too.

> 
> In the upcoming commits, we will teach the sequencer to recreate merges.
> This will be done in a very different way from the unfortunate design of
> `git rebase --preserve-merges` (which does not allow for reordering
> commits, or changing the branch topology).
> 
> The main idea is to introduce new todo list commands, to support
> labeling the current revision with a given name, resetting the current
> revision to a previous state, merging labeled revisions.

> This idea was developed in Git for Windows' Git garden shears (that are
> used to maintain the "thicket of branches" on top of upstream Git), and
> this patch is part of the effort to make it available to a wider
> audience, as well as to make the entire process more robust (by
> implementing it in a safe and portable language rather than a Unix shell
> script).
> 
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
> 
> 	label <name>
> 	reset <name>

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-rebase--interactive.sh |   3 +
>  sequencer.c                | 181 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 180 insertions(+), 4 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index d47bd29593a..3d2cd19d65a 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -162,6 +162,9 @@ s, squash = use commit, but meld into previous commit
>  f, fixup = like \"squash\", but discard this commit's log message
>  x, exec = run command (the rest of the line) using shell
>  d, drop = remove commit
> +l, label = label current HEAD with a name
> +t, reset = reset HEAD to a label
> +b, bud = reset HEAD to the revision labeled 'onto'
>  
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 4d3f60594cb..91cc55a002f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -21,6 +21,8 @@
>  #include "log-tree.h"
>  #include "wt-status.h"
>  #include "hashmap.h"
> +#include "unpack-trees.h"
> +#include "worktree.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
>  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 listing refs that need to be deleted after the rebase
> + * finishes. This is used by the `merge` command.
> + */
> +static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
> +
>  /*
>   * The following files are written by git-rebase just after parsing the
>   * command-line (and are only consumed, not modified, by the sequencer).
> @@ -767,6 +776,9 @@ enum todo_command {
>  	TODO_SQUASH,
>  	/* commands that do something else than handling a single commit */
>  	TODO_EXEC,
> +	TODO_LABEL,
> +	TODO_RESET,
> +	TODO_BUD,
>  	/* commands that do nothing but are counted for reporting progress */
>  	TODO_NOOP,
>  	TODO_DROP,
> @@ -785,6 +797,9 @@ static struct {
>  	{ 'f', "fixup" },
>  	{ 's', "squash" },
>  	{ 'x', "exec" },
> +	{ 'l', "label" },
> +	{ 't', "reset" },
> +	{ 'b', "bud" },
>  	{ 0,   "noop" },
>  	{ 'd', "drop" },
>  	{ 0,   NULL }
> @@ -1253,7 +1268,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>  		if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
>  			item->command = i;
>  			break;
> -		} else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
> +		} else if ((bol + 1 == eol || bol[1] == ' ') &&
> +			   *bol == todo_command_info[i].c) {
>  			bol++;
>  			item->command = i;
>  			break;
> @@ -1265,7 +1281,7 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>  	padding = strspn(bol, " \t");
>  	bol += padding;
>  
> -	if (item->command == TODO_NOOP) {
> +	if (item->command == TODO_NOOP || item->command == TODO_BUD) {
>  		if (bol != eol)
>  			return error(_("%s does not accept arguments: '%s'"),
>  				     command_to_string(item->command), bol);
> @@ -1279,7 +1295,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>  		return error(_("missing arguments for %s"),
>  			     command_to_string(item->command));
>  
> -	if (item->command == TODO_EXEC) {
> +	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
> +	    item->command == TODO_RESET) {
>  		item->commit = NULL;
>  		item->arg = bol;
>  		item->arg_len = (int)(eol - bol);
> @@ -1919,6 +1936,139 @@ static int do_exec(const char *command_line)
>  	return status;
>  }
>  
> +static int safe_append(const char *filename, const char *fmt, ...)
> +{
> +	va_list ap;
> +	struct lock_file lock = LOCK_INIT;
> +	int fd = hold_lock_file_for_update(&lock, filename, 0);
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	if (fd < 0)
> +		return error_errno(_("could not lock '%s'"), filename);
> +
> +	if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT)
> +		return error_errno(_("could not read '%s'"), filename);
> +	strbuf_complete(&buf, '\n');
> +	va_start(ap, fmt);
> +	strbuf_vaddf(&buf, fmt, ap);
> +	va_end(ap);
> +
> +	if (write_in_full(fd, buf.buf, buf.len) < 0) {
> +		rollback_lock_file(&lock);
> +		return error_errno(_("could not write to '%s'"), filename);
> +	}
> +	if (commit_lock_file(&lock) < 0) {
> +		rollback_lock_file(&lock);
> +		return error(_("failed to finalize '%s'."), filename);
> +	}
> +
> +	return 0;
> +}
> +
> +static int do_label(const char *name, int len)
> +{
> +	struct ref_store *refs = get_main_ref_store();
> +	struct ref_transaction *transaction;
> +	struct strbuf ref_name = STRBUF_INIT, err = STRBUF_INIT;
> +	struct strbuf msg = STRBUF_INIT;
> +	int ret = 0;
> +	struct object_id head_oid;
> +
> +	strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
> +	strbuf_addf(&msg, "label '%.*s'", len, name);

> +
> +	transaction = ref_store_transaction_begin(refs, &err);
> +	if (!transaction ||
> +	    get_oid("HEAD", &head_oid) ||
> +	    ref_transaction_update(transaction, ref_name.buf, &head_oid, NULL,
> +				   0, msg.buf, &err) < 0 ||
> +	    ref_transaction_commit(transaction, &err)) {
> +		error("%s", err.buf);

> +		ret = -1;
> +	}
> +	ref_transaction_free(transaction);
> +	strbuf_release(&err);
> +	strbuf_release(&msg);
> +
> +	if (!ret)
> +		ret = safe_append(rebase_path_refs_to_delete(),
> +				  "%s\n", ref_name.buf);
> +	strbuf_release(&ref_name);
> +
> +	return ret;
> +}
> +
> +static int do_reset(const char *name, int len)
> +{
> +	struct strbuf ref_name = STRBUF_INIT;
> +	struct object_id oid;
> +	struct lock_file lock = LOCK_INIT;
> +	struct tree_desc desc;
> +	struct tree *tree;
> +	struct unpack_trees_options opts;
> +	int ret = 0, i;
> +
> +	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> +		return -1;
> +
> +	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(&opts, 0, sizeof(opts));
> +	opts.head_idx = 1;
> +	opts.src_index = &the_index;
> +	opts.dst_index = &the_index;
> +	opts.fn = oneway_merge;
> +	opts.merge = 1;
> +	opts.update = 1;
> +	opts.reset = 1;
> +
> +	read_cache_unmerged();
> +	if (!fill_tree_descriptor(&desc, &oid)) {
> +		error(_("Failed to find tree of %s."), oid_to_hex(&oid));
> +		rollback_lock_file(&lock);
> +		free((void *)desc.buffer);
> +		strbuf_release(&ref_name);
> +		return -1;
> +	}
> +
> +	if (unpack_trees(1, &desc, &opts)) {
> +		rollback_lock_file(&lock);
> +		free((void *)desc.buffer);
> +		strbuf_release(&ref_name);
> +		return -1;
> +	}
> +
> +	tree = parse_tree_indirect(&oid);
> +	prime_cache_tree(&the_index, tree);
> +
> +	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> +		ret = error(_("could not write index"));
> +	free((void *)desc.buffer);
> +
> +	if (!ret) {
> +		struct strbuf msg = STRBUF_INIT;
> +
> +		strbuf_addf(&msg, "(rebase -i) reset '%.*s'", len, name);
> +		ret = update_ref(msg.buf, "HEAD", &oid, NULL, 0,
> +				 UPDATE_REFS_MSG_ON_ERR);
> +		strbuf_release(&msg);
> +	}
> +
> +	strbuf_release(&ref_name);
> +	return ret;
> +}
> +
>  static int is_final_fixup(struct todo_list *todo_list)
>  {
>  	int i = todo_list->current;
> @@ -2102,7 +2252,13 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  				/* `current` will be incremented below */
>  				todo_list->current = -1;
>  			}
> -		} else if (!is_noop(item->command))
> +		} else if (item->command == TODO_LABEL)
> +			res = do_label(item->arg, item->arg_len);
> +		else if (item->command == TODO_RESET)
> +			res = do_reset(item->arg, item->arg_len);
> +		else if (item->command == TODO_BUD)
> +			res = do_reset("onto", 4);
> +		else if (!is_noop(item->command))
>  			return error(_("unknown command %d"), item->command);
>  
>  		todo_list->current++;
> @@ -2207,6 +2363,23 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  		}
>  		apply_autostash(opts);
>  
> +		strbuf_reset(&buf);
> +		if (strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0)
> +		    > 0) {
> +			char *p = buf.buf;
> +			while (*p) {
> +				char *eol = strchr(p, '\n');
> +				if (eol)
> +					*eol = '\0';
> +				if (delete_ref("(rebase -i) cleanup",
> +					       p, NULL, 0) < 0)
> +					warning(_("could not delete '%s'"), p);
> +				if (!eol)
> +					break;
> +				p = eol + 1;
> +			}
> +		}
> +
>  		fprintf(stderr, "Successfully rebased and updated %s.\n",
>  			head_ref.buf);
>  
> 

> 
> On 18/01/18 15:35, Johannes Schindelin wrote:
>>
>> Internally, the `label <name>` command creates the ref
>> `refs/rewritten/<name>`. This makes it possible to work with the labeled
>> revisions interactively, or in a scripted fashion (e.g. via the todo
>> list command `exec`).
> 
> If a user has two work trees and runs a rebase in each with the same
> label name, they'll clobber each other. I'd suggest storing them under
> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
> tries to rebase a second worktree with the same detached HEAD as an
> existing rebase then refuse to start.
> 

> On 19/01/18 12:24, Phillip Wood wrote:
>>
>> On 18/01/18 15:35, Johannes Schindelin wrote:
>>>
>>> Internally, the `label <name>` command creates the ref
>>> `refs/rewritten/<name>`. This makes it possible to work with the labeled
>>> revisions interactively, or in a scripted fashion (e.g. via the todo
>>> list command `exec`).
>>
>> If a user has two work trees and runs a rebase in each with the same
>> label name, they'll clobber each other. I'd suggest storing them under
>> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
>> tries to rebase a second worktree with the same detached HEAD as an
>> existing rebase then refuse to start.
>>
>
> Ah this isn't a concern after all as patch 5 makes refs/rewritten local
> to the worktree. Perhaps you could move that part of patch 5 here or add
> a note to the commit message that it will become worktree local later in
> the series
>
> Best Wishes
>
> Phillip

> On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
> > On 19/01/18 12:24, Phillip Wood wrote:
> >>
> >> On 18/01/18 15:35, Johannes Schindelin wrote:
> >>>
> >>> Internally, the `label <name>` command creates the ref
> >>> `refs/rewritten/<name>`. This makes it possible to work with the labeled
> >>> revisions interactively, or in a scripted fashion (e.g. via the todo
> >>> list command `exec`).
> >>
> >> If a user has two work trees and runs a rebase in each with the same
> >> label name, they'll clobber each other. I'd suggest storing them under
> >> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
> >> tries to rebase a second worktree with the same detached HEAD as an
> >> existing rebase then refuse to start.
> >>
> >
> > Ah this isn't a concern after all as patch 5 makes refs/rewritten local
> > to the worktree. Perhaps you could move that part of patch 5 here or add
> > a note to the commit message that it will become worktree local later in
> > the series
> >
> > Best Wishes
> >
> > Phillip
> 
> I'd rather it be included here as well.

> On Fri, 19 Jan 2018, Jacob Keller wrote:
> 
> > On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood
> > <phillip.wood@talktalk.net> wrote:
> > > On 19/01/18 12:24, Phillip Wood wrote:
> > >>
> > >> On 18/01/18 15:35, Johannes Schindelin wrote:
> > >>>
> > >>> Internally, the `label <name>` command creates the ref
> > >>> `refs/rewritten/<name>`. This makes it possible to work with the labeled
> > >>> revisions interactively, or in a scripted fashion (e.g. via the todo
> > >>> list command `exec`).
> > >>
> > >> If a user has two work trees and runs a rebase in each with the same
> > >> label name, they'll clobber each other. I'd suggest storing them under
> > >> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
> > >> tries to rebase a second worktree with the same detached HEAD as an
> > >> existing rebase then refuse to start.
> > >>
> > >
> > > Ah this isn't a concern after all as patch 5 makes refs/rewritten local
> > > to the worktree. Perhaps you could move that part of patch 5 here or add
> > > a note to the commit message that it will become worktree local later in
> > > the series
> > >
> > > Best Wishes
> > >
> > > Phillip
> > 
> > I'd rather it be included here as well.
> 
> But it would have been really easy to overlook in here. I really want this
> to be a separate commit, also to have a chance to get this done
> *differently* if somebody comes up with a splendid idea how to do that
> (because hard-coding feels quite dirty).

> 
> On 18/01/18 15:35, Johannes Schindelin wrote:
> 
> > This idea was developed in Git for Windows' Git garden shears (that
> > are used to maintain the "thicket of branches" on top of upstream
> > Git), and this patch is part of the effort to make it available to a
> > wider audience, as well as to make the entire process more robust (by
> > implementing it in a safe and portable language rather than a Unix
> > shell script).
> > 
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> > 
> > 	label <name>
> > 	reset <name>
> 
> If I've understood the code below correctly then reset will clobber
> untracked files, this is the opposite behaviour to what happens when
> tries to checkout <onto> at the start of a rebase - then it will fail if
> untracked files would be overwritten.

> > Internally, the `label <name>` command creates the ref
> > `refs/rewritten/<name>`. This makes it possible to work with the labeled
> > revisions interactively, or in a scripted fashion (e.g. via the todo
> > list command `exec`).
> 
> If a user has two work trees and runs a rebase in each with the same
> label name, they'll clobber each other. I'd suggest storing them under
> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
> tries to rebase a second worktree with the same detached HEAD as an
> existing rebase then refuse to start.

> > +static int do_label(const char *name, int len)
> > +{
> > +	struct ref_store *refs = get_main_ref_store();
> > +	struct ref_transaction *transaction;
> > +	struct strbuf ref_name = STRBUF_INIT, err = STRBUF_INIT;
> > +	struct strbuf msg = STRBUF_INIT;
> > +	int ret = 0;
> > +	struct object_id head_oid;
> > +
> > +	strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
> > +	strbuf_addf(&msg, "label '%.*s'", len, name);
> 
> The other reflog messages below have a (rebase -i) prefix

> > +	transaction = ref_store_transaction_begin(refs, &err);
> > +	if (!transaction ||
> > +	    get_oid("HEAD", &head_oid) ||
> > +	    ref_transaction_update(transaction, ref_name.buf, &head_oid, NULL,
> > +				   0, msg.buf, &err) < 0 ||
> > +	    ref_transaction_commit(transaction, &err)) {
> > +		error("%s", err.buf);
> 
> if get_oid() fails then err is empty so there wont be an message after
> the 'error: '

> > +static int do_reset(const char *name, int len)
> > +{
> > +	struct strbuf ref_name = STRBUF_INIT;
> > +	struct object_id oid;
> > +	struct lock_file lock = LOCK_INIT;
> > +	struct tree_desc desc;
> > +	struct tree *tree;
> > +	struct unpack_trees_options opts;
> > +	int ret = 0, i;
> > +
> > +	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > +		return -1;
> > +
> > +	for (i = 0; i < len; i++)
> > +		if (isspace(name[i]))
> > +			len = i;
> 
> If name starts with any white space then I think this effectively
> truncates name to a bunch of white space which doesn't sound right. I'm
> not sure how this is being called, but it might be better to clean up
> name when the to-do list is parsed instead.

> This patch is part of the effort to reimplement `--preserve-merges` with
> a substantially improved design, a design that has been developed in the
> Git for Windows project to maintain the dozens of Windows-specific patch
> series on top of upstream Git.
>
> The previous patch implemented the `label`, `bud` and `reset` commands
> to label commits and to reset to a labeled commits. This patch adds the
> `merge` command, with the following syntax:
>
>         merge <commit> <rev> <oneline>
>
> The <commit> parameter in this instance is the *original* merge commit,
> whose author and message will be used for the to-be-created merge
> commit.
>
> The <rev> parameter refers to the (possibly rewritten) revision to
> merge. Let's see an example of a todo list:
>
>         label onto
>
>         # Branch abc
>         bud
>         pick deadbeef Hello, world!
>         label abc
>
>         bud
>         pick cafecafe And now for something completely different
>         merge baaabaaa abc Merge the branch 'abc' into master
>
> To support creating *new* merges, i.e. without copying the commit
> message from an existing commit, use the special value `-` as <commit>
> parameter (in which case the text after the <rev> parameter is used as
> commit message):
>
>         merge - abc This will be the actual commit message of the merge
>
> This comes in handy when splitting a branch into two or more branches.
>

> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > This patch is part of the effort to reimplement `--preserve-merges` with
> > a substantially improved design, a design that has been developed in the
> > Git for Windows project to maintain the dozens of Windows-specific patch
> > series on top of upstream Git.
> >
> > The previous patch implemented the `label`, `bud` and `reset` commands
> > to label commits and to reset to a labeled commits. This patch adds the
> > `merge` command, with the following syntax:
> >
> >         merge <commit> <rev> <oneline>
> >
> > The <commit> parameter in this instance is the *original* merge commit,
> > whose author and message will be used for the to-be-created merge
> > commit.
> >
> > The <rev> parameter refers to the (possibly rewritten) revision to
> > merge. Let's see an example of a todo list:
> >
> >         label onto
> >
> >         # Branch abc
> >         bud
> >         pick deadbeef Hello, world!
> >         label abc
> >
> >         bud
> >         pick cafecafe And now for something completely different
> >         merge baaabaaa abc Merge the branch 'abc' into master
> >
> > To support creating *new* merges, i.e. without copying the commit
> > message from an existing commit, use the special value `-` as <commit>
> > parameter (in which case the text after the <rev> parameter is used as
> > commit message):
> >
> >         merge - abc This will be the actual commit message of the merge
> >
> > This comes in handy when splitting a branch into two or more branches.
> >
> 
> Would it be possible to open the editor with the supplied text when
> there's no commit?  The text after <rev> must be oneline only..

> It's difficult to reword merges because of the nature of rebase
> interactive, you can't just re-run the rebase command and use
> "reword".
> 
> I suppose you could cheat by putting in an "edit" command that let you
> create an empty commit with a message...

>> Would it be possible to open the editor with the supplied text when
>> there's no commit?  The text after <rev> must be oneline only..
>
> I actually want to avoid that because my main use case is fire-and-forget,
> i.e. I want to edit only the todo list and then (barring any merge
> conflicts) I do not want to edit anything anymore.
>

>> It's difficult to reword merges because of the nature of rebase
>> interactive, you can't just re-run the rebase command and use
>> "reword".
>>
>> I suppose you could cheat by putting in an "edit" command that let you
>> create an empty commit with a message...
>
> Or you could "cheat" by adding `exec git commit --amend`...
>
> Seriously again, I have no good idea how to provide an equivalent to the
> `reword` verb that would work on merge commits...
>

> [...]
> Note: this patch only adds support for recursive merges, to keep things
> simple. Support for octopus merges will be added later in this patch
> series, support for merges using strategies other than the recursive
> merge is left for future contributions.

> The design of the `merge` command as introduced by this patch only
> supports creating new merge commits with exactly two parents, i.e. it
> adds no support for octopus merges.
>
> We will introduce support for octopus merges in a later commit.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2069,6 +2077,132 @@ static int do_reset(const char *name, int len)
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> +                   struct replay_opts *opts)
> +{
> +       [...]
> +               if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
> +                       error_errno(_("Could not write '%s'"),

> 
> This patch is part of the effort to reimplement `--preserve-merges` with
> a substantially improved design, a design that has been developed in the
> Git for Windows project to maintain the dozens of Windows-specific patch
> series on top of upstream Git.
> 
> The previous patch implemented the `label`, `bud` and `reset` commands
> to label commits and to reset to a labeled commits. This patch adds the
> `merge` command, with the following syntax:
> 
> 	merge <commit> <rev> <oneline>

> The <commit> parameter in this instance is the *original* merge commit,
> whose author and message will be used for the to-be-created merge
> commit.
> 
> The <rev> parameter refers to the (possibly rewritten) revision to
> merge. Let's see an example of a todo list:
> 
> 	label onto
> 
> 	# Branch abc
> 	bud
> 	pick deadbeef Hello, world!
> 	label abc
> 
> 	bud
> 	pick cafecafe And now for something completely different
> 	merge baaabaaa abc Merge the branch 'abc' into master
> 
> To support creating *new* merges, i.e. without copying the commit
> message from an existing commit, use the special value `-` as <commit>
> parameter (in which case the text after the <rev> parameter is used as
> commit message):
> 
> 	merge - abc This will be the actual commit message of the merge
> 
> This comes in handy when splitting a branch into two or more branches.
> 
> Note: this patch only adds support for recursive merges, to keep things
> simple. Support for octopus merges will be added later in this patch
> series, support for merges using strategies other than the recursive
> merge is left for future contributions.
> 
> The design of the `merge` command as introduced by this patch only
> supports creating new merge commits with exactly two parents, i.e. it
> adds no support for octopus merges.
> 
> We will introduce support for octopus merges in a later commit.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-rebase--interactive.sh |   1 +
>  sequencer.c                | 146 +++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 143 insertions(+), 4 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 3d2cd19d65a..5bf1ea3781f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -165,6 +165,7 @@ d, drop = remove commit
>  l, label = label current HEAD with a name
>  t, reset = reset HEAD to a label
>  b, bud = reset HEAD to the revision labeled 'onto'
> +m, merge = create a merge commit using a given commit's message
>  
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 91cc55a002f..567cfcbbe8b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -779,6 +779,7 @@ enum todo_command {
>  	TODO_LABEL,
>  	TODO_RESET,
>  	TODO_BUD,
> +	TODO_MERGE,
>  	/* commands that do nothing but are counted for reporting progress */
>  	TODO_NOOP,
>  	TODO_DROP,
> @@ -800,6 +801,7 @@ static struct {
>  	{ 'l', "label" },
>  	{ 't', "reset" },
>  	{ 'b', "bud" },
> +	{ 'm', "merge" },
>  	{ 0,   "noop" },
>  	{ 'd', "drop" },
>  	{ 0,   NULL }
> @@ -1304,14 +1306,20 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>  	}
>  
>  	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> +	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> +	item->arg_len = (int)(eol - item->arg);
> +
>  	saved = *end_of_object_name;
> +	if (item->command == TODO_MERGE && *bol == '-' &&
> +	    bol + 1 == end_of_object_name) {
> +		item->commit = NULL;
> +		return 0;
> +	}
> +
>  	*end_of_object_name = '\0';
>  	status = get_oid(bol, &commit_oid);
>  	*end_of_object_name = saved;
>  
> -	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> -	item->arg_len = (int)(eol - item->arg);
> -
>  	if (status < 0)
>  		return -1;
>  
> @@ -2069,6 +2077,132 @@ static int do_reset(const char *name, int len)
>  	return ret;
>  }
>  
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> +		    struct replay_opts *opts)
> +{
> +	int merge_arg_len;
> +	struct strbuf ref_name = STRBUF_INIT;
> +	struct commit *head_commit, *merge_commit, *i;
> +	struct commit_list *common, *j, *reversed = NULL;
> +	struct merge_options o;
> +	int ret;
> +	static struct lock_file lock;
> +
> +	for (merge_arg_len = 0; merge_arg_len < arg_len; merge_arg_len++)
> +		if (isspace(arg[merge_arg_len]))
> +			break;
> +
> +	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> +		return -1;
> +
> +	if (commit) {
> +		const char *message = get_commit_buffer(commit, NULL);
> +		const char *body;
> +		int len;
> +
> +		if (!message) {
> +			rollback_lock_file(&lock);
> +			return error(_("could not get commit message of '%s'"),
> +				     oid_to_hex(&commit->object.oid));
> +		}
> +		write_author_script(message);
> +		find_commit_subject(message, &body);
> +		len = strlen(body);
> +		if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
> +			error_errno(_("Could not write '%s'"),
> +				    git_path_merge_msg());
> +			unuse_commit_buffer(commit, message);
> +			rollback_lock_file(&lock);
> +			return -1;
> +		}
> +		unuse_commit_buffer(commit, message);
> +	} else {
> +		const char *p = arg + merge_arg_len;
> +		struct strbuf buf = STRBUF_INIT;
> +		int len;
> +
> +		strbuf_addf(&buf, "author %s", git_author_info(0));
> +		write_author_script(buf.buf);
> +		strbuf_reset(&buf);
> +
> +		p += strspn(p, " \t");
> +		if (*p)
> +			len = strlen(p);
> +		else {
> +			strbuf_addf(&buf, "Merge branch '%.*s'",
> +				    merge_arg_len, arg);
> +			p = buf.buf;
> +			len = buf.len;
> +		}
> +
> +		if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> +			error_errno(_("Could not write '%s'"),
> +				    git_path_merge_msg());
> +			strbuf_release(&buf);
> +			rollback_lock_file(&lock);
> +			return -1;
> +		}
> +		strbuf_release(&buf);
> +	}
> +
> +	head_commit = lookup_commit_reference_by_name("HEAD");
> +	if (!head_commit) {
> +		rollback_lock_file(&lock);
> +		return error(_("Cannot merge without a current revision"));
> +	}
> +
> +	strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
> +	merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> +	if (!merge_commit) {
> +		/* fall back to non-rewritten ref or commit */
> +		strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0);
> +		merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> +	}
> +	if (!merge_commit) {
> +		error(_("could not resolve '%s'"), ref_name.buf);
> +		strbuf_release(&ref_name);
> +		rollback_lock_file(&lock);
> +		return -1;
> +	}
> +	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
> +		      git_path_merge_head(), 0);
> +	write_message("no-ff", 5, git_path_merge_mode(), 0);
> +
> +	common = get_merge_bases(head_commit, merge_commit);
> +	for (j = common; j; j = j->next)
> +		commit_list_insert(j->item, &reversed);
> +	free_commit_list(common);
> +
> +	read_cache();
> +	init_merge_options(&o);
> +	o.branch1 = "HEAD";
> +	o.branch2 = ref_name.buf;
> +	o.buffer_output = 2;
> +
> +	ret = merge_recursive(&o, head_commit, merge_commit, reversed, &i);
> +	if (ret <= 0)
> +		fputs(o.obuf.buf, stdout);
> +	strbuf_release(&o.obuf);
> +	if (ret < 0) {
> +		strbuf_release(&ref_name);
> +		rollback_lock_file(&lock);
> +		return error(_("conflicts while merging '%.*s'"),
> +			     merge_arg_len, arg);
> +	}
> +
> +	if (active_cache_changed &&
> +	    write_locked_index(&the_index, &lock, COMMIT_LOCK)) {
> +		strbuf_release(&ref_name);
> +		return error(_("merge: Unable to write new index file"));
> +	}
> +	rollback_lock_file(&lock);
> +
> +	ret = run_git_commit(git_path_merge_msg(), opts, 0);
> +	strbuf_release(&ref_name);
> +
> +	return ret;
> +}
> +
>  static int is_final_fixup(struct todo_list *todo_list)
>  {
>  	int i = todo_list->current;
> @@ -2258,6 +2392,9 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  			res = do_reset(item->arg, item->arg_len);
>  		else if (item->command == TODO_BUD)
>  			res = do_reset("onto", 4);
> +		else if (item->command == TODO_MERGE)
> +			res = do_merge(item->commit,
> +				       item->arg, item->arg_len, opts);
>  		else if (!is_noop(item->command))
>  			return error(_("unknown command %d"), item->command);
>  
> @@ -2757,7 +2894,8 @@ int transform_todos(unsigned flags)
>  					  oid_to_hex(&item->commit->object.oid);
>  
>  			strbuf_addf(&buf, " %s", oid);
> -		}
> +		} else if (item->command == TODO_MERGE)
> +			strbuf_addstr(&buf, " -");
>  		/* add all the rest */
>  		if (!item->arg_len)
>  			strbuf_addch(&buf, '\n');
> 

> On 18/01/18 15:35, Johannes Schindelin wrote:
>>
>> This patch is part of the effort to reimplement `--preserve-merges` with
>> a substantially improved design, a design that has been developed in the
>> Git for Windows project to maintain the dozens of Windows-specific patch
>> series on top of upstream Git.
>>
>> The previous patch implemented the `label`, `bud` and `reset` commands
>> to label commits and to reset to a labeled commits. This patch adds the
>> `merge` command, with the following syntax:
>>
>>       merge <commit> <rev> <oneline>
>
> I'm concerned that this will be confusing for users. All of the other
> rebase commands replay the changes in the commit hash immediately
> following the command name. This command instead uses the first commit
> to specify the message which is different to both 'git merge' and the
> existing rebase commands. I wonder if it would be clearer to have 'merge
> -C <commit> <rev> ...' instead so it's clear which argument specifies
> the message and which the remote head to merge. It would also allow for
> 'merge -c <commit> <rev> ...' in the future for rewording an existing
> merge message and also avoid the slightly odd 'merge - <rev> ...'. Where
> it's creating new merges I'm not sure it's a good idea to encourage
> people to only have oneline commit messages by making it harder to edit
> them, perhaps it could take another argument to mean open the editor or
> not, though as Jake said I guess it's not that common.

>
> One thought that just struck me - if a merge or reset command specifies
> an invalid label is it rescheduled so that it's still in the to-do list
> when the user edits it after rebase stops?
>
> In the future it might be nice if the label, reset and merge commands
> were validated when the to-do list is parsed so that the user gets
> immediate feedback if they try to create a label that is not a valid ref
> name or that they have a typo in a name given to reset or merge rather
> than the rebase stopping later.
>

> On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
> > On 18/01/18 15:35, Johannes Schindelin wrote:
> >>
> >> This patch is part of the effort to reimplement `--preserve-merges` with
> >> a substantially improved design, a design that has been developed in the
> >> Git for Windows project to maintain the dozens of Windows-specific patch
> >> series on top of upstream Git.
> >>
> >> The previous patch implemented the `label`, `bud` and `reset` commands
> >> to label commits and to reset to a labeled commits. This patch adds the
> >> `merge` command, with the following syntax:
> >>
> >>       merge <commit> <rev> <oneline>
> >
> > I'm concerned that this will be confusing for users. All of the other
> > rebase commands replay the changes in the commit hash immediately
> > following the command name. This command instead uses the first commit
> > to specify the message which is different to both 'git merge' and the
> > existing rebase commands. I wonder if it would be clearer to have 'merge
> > -C <commit> <rev> ...' instead so it's clear which argument specifies
> > the message and which the remote head to merge. It would also allow for
> > 'merge -c <commit> <rev> ...' in the future for rewording an existing
> > merge message and also avoid the slightly odd 'merge - <rev> ...'. Where
> > it's creating new merges I'm not sure it's a good idea to encourage
> > people to only have oneline commit messages by making it harder to edit
> > them, perhaps it could take another argument to mean open the editor or
> > not, though as Jake said I guess it's not that common.
> 
> I actually like the idea of re-using commit message options like -C,
> -c,  and -m, so we could do:
> 
> merge -C <commit> ... to take message from commit

> merge -c <commit> ...  to take the message from commit and open editor to edit
> merge -m "<message>" ... to take the message from the quoted test
> merge ... to merge and open commit editor with default message
> 
> This also, I think, allows us to not need to put the oneline on the
> end, meaning we wouldn't have to quote the parent commit arguments
> since we could use option semantics?

> > In the future it might be nice if the label, reset and merge commands
> > were validated when the to-do list is parsed so that the user gets
> > immediate feedback if they try to create a label that is not a valid
> > ref name or that they have a typo in a name given to reset or merge
> > rather than the rebase stopping later.

> On Sat, 20 Jan 2018, Jacob Keller wrote:
> 
> > On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
> > > On 18/01/18 15:35, Johannes Schindelin wrote:
> > >>
> > >> This patch adds the `merge` command, with the following syntax:
> > >>
> > >>       merge <commit> <rev> <oneline>
> > >
> > > I'm concerned that this will be confusing for users. All of the other
> > > rebase commands replay the changes in the commit hash immediately
> > > following the command name. This command instead uses the first
> > > commit to specify the message which is different to both 'git merge'
> > > and the existing rebase commands. I wonder if it would be clearer to
> > > have 'merge -C <commit> <rev> ...' instead so it's clear which
> > > argument specifies the message and which the remote head to merge.
> > > It would also allow for 'merge -c <commit> <rev> ...' in the future
> > > for rewording an existing merge message and also avoid the slightly
> > > odd 'merge - <rev> ...'. Where it's creating new merges I'm not sure
> > > it's a good idea to encourage people to only have oneline commit
> > > messages by making it harder to edit them, perhaps it could take
> > > another argument to mean open the editor or not, though as Jake said
> > > I guess it's not that common.
> > 
> > I actually like the idea of re-using commit message options like -C,
> > -c,  and -m, so we could do:
> > 
> > merge -C <commit> ... to take message from commit
> 
> That is exactly how the Git garden shears do it.
> 
> I found it not very readable. That is why I wanted to get away from it in
> --recreate-merges.

> Hi Jake & Phillip,
> 
> On Mon, 29 Jan 2018, Johannes Schindelin wrote:
> 
>> On Sat, 20 Jan 2018, Jacob Keller wrote:
>>
>>> On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
>>>> On 18/01/18 15:35, Johannes Schindelin wrote:
>>>>>
>>>>> This patch adds the `merge` command, with the following syntax:
>>>>>
>>>>>       merge <commit> <rev> <oneline>
>>>>
>>>> I'm concerned that this will be confusing for users. All of the other
>>>> rebase commands replay the changes in the commit hash immediately
>>>> following the command name. This command instead uses the first
>>>> commit to specify the message which is different to both 'git merge'
>>>> and the existing rebase commands. I wonder if it would be clearer to
>>>> have 'merge -C <commit> <rev> ...' instead so it's clear which
>>>> argument specifies the message and which the remote head to merge.
>>>> It would also allow for 'merge -c <commit> <rev> ...' in the future
>>>> for rewording an existing merge message and also avoid the slightly
>>>> odd 'merge - <rev> ...'. Where it's creating new merges I'm not sure
>>>> it's a good idea to encourage people to only have oneline commit
>>>> messages by making it harder to edit them, perhaps it could take
>>>> another argument to mean open the editor or not, though as Jake said
>>>> I guess it's not that common.
>>>
>>> I actually like the idea of re-using commit message options like -C,
>>> -c,  and -m, so we could do:
>>>
>>> merge -C <commit> ... to take message from commit
>>
>> That is exactly how the Git garden shears do it.
>>
>> I found it not very readable. That is why I wanted to get away from it in
>> --recreate-merges.
> 
> I made up my mind. Even if it is not very readable, it is still better
> than the `merge A B` where the order of A and B magically determines their
> respective roles.
> 
>>> merge -c <commit> ...  to take the message from commit and open editor to edit
>>> merge -m "<message>" ... to take the message from the quoted test
>>> merge ... to merge and open commit editor with default message
> 
> I will probably implement -c, but not -m, and will handle the absence of
> the -C and -c options to construct a default merge message which can then
> be edited.

> The -m option just opens such a can of worms with dequoting, that's why I
> do not want to do that.
> 
> BTW I am still trying to figure out how to present the oneline of the
> commit to merge (which is sometimes really helpful because the label might
> be less than meaningful) while *still* allowing for octopus merges.
> 
> So far, what I have is this:
> 
> 	merge <original> <to-merge> <oneline>
> 
> and for octopus:
> 
> 	merge <original> "<to-merge> <to-merge2>..." <oneline>...
> 
> I think with the -C syntax, it would become something like
> 
> 	merge -C <original> <to-merge> # <oneline>
> 
> and
> 
> 	merge -C <original> <to-merge> <to-merge2>...
> 	# Merging: <oneline>
> 	# Merging: <oneline2>
> 	# ...
> 
> The only qualm I have about this is that `#` really *is* a valid ref name.
> (Seriously, it is...). So that would mean that I'd have to disallow `#`
> as a label specificially.
> 
> Thoughts?

> Hi Jake & Phillip,
>
> On Mon, 29 Jan 2018, Johannes Schindelin wrote:
>
>> On Sat, 20 Jan 2018, Jacob Keller wrote:
>>
>> > On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
>> > > On 18/01/18 15:35, Johannes Schindelin wrote:
>> > >>
>> > >> This patch adds the `merge` command, with the following syntax:
>> > >>
>> > >>       merge <commit> <rev> <oneline>
>> > >
>> > > I'm concerned that this will be confusing for users. All of the other
>> > > rebase commands replay the changes in the commit hash immediately
>> > > following the command name. This command instead uses the first
>> > > commit to specify the message which is different to both 'git merge'
>> > > and the existing rebase commands. I wonder if it would be clearer to
>> > > have 'merge -C <commit> <rev> ...' instead so it's clear which
>> > > argument specifies the message and which the remote head to merge.
>> > > It would also allow for 'merge -c <commit> <rev> ...' in the future
>> > > for rewording an existing merge message and also avoid the slightly
>> > > odd 'merge - <rev> ...'. Where it's creating new merges I'm not sure
>> > > it's a good idea to encourage people to only have oneline commit
>> > > messages by making it harder to edit them, perhaps it could take
>> > > another argument to mean open the editor or not, though as Jake said
>> > > I guess it's not that common.
>> >
>> > I actually like the idea of re-using commit message options like -C,
>> > -c,  and -m, so we could do:
>> >
>> > merge -C <commit> ... to take message from commit
>>
>> That is exactly how the Git garden shears do it.
>>
>> I found it not very readable. That is why I wanted to get away from it in
>> --recreate-merges.
>
> I made up my mind. Even if it is not very readable, it is still better
> than the `merge A B` where the order of A and B magically determines their
> respective roles.
>
>> > merge -c <commit> ...  to take the message from commit and open editor to edit
>> > merge -m "<message>" ... to take the message from the quoted test
>> > merge ... to merge and open commit editor with default message
>
> I will probably implement -c, but not -m, and will handle the absence of
> the -C and -c options to construct a default merge message which can then
> be edited.
>
> The -m option just opens such a can of worms with dequoting, that's why I
> do not want to do that.
>

> BTW I am still trying to figure out how to present the oneline of the
> commit to merge (which is sometimes really helpful because the label might
> be less than meaningful) while *still* allowing for octopus merges.
>
> So far, what I have is this:
>
>         merge <original> <to-merge> <oneline>
>
> and for octopus:
>
>         merge <original> "<to-merge> <to-merge2>..." <oneline>...
>
> I think with the -C syntax, it would become something like
>
>         merge -C <original> <to-merge> # <oneline>
>

> and
>
>         merge -C <original> <to-merge> <to-merge2>...
>         # Merging: <oneline>
>         # Merging: <oneline2>
>         # ...
>

> The only qualm I have about this is that `#` really *is* a valid ref name.
> (Seriously, it is...). So that would mean that I'd have to disallow `#`
> as a label specifically.
>
> Thoughts?
>

>  	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> +	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> +	item->arg_len = (int)(eol - item->arg);
> +
>  	saved = *end_of_object_name;
> +	if (item->command == TODO_MERGE && *bol == '-' &&
> +	    bol + 1 == end_of_object_name) {
> +		item->commit = NULL;
> +		return 0;
> +	}
> +
>  	*end_of_object_name = '\0';
>  	status = get_oid(bol, &commit_oid);
>  	*end_of_object_name = saved;
>  
> -	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> -	item->arg_len = (int)(eol - item->arg);
> -

> @@ -2069,6 +2077,132 @@ static int do_reset(const char *name, int len)
>  	return ret;
>  }
>  
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> +		    struct replay_opts *opts)
> +{
> +	int merge_arg_len;
> +	struct strbuf ref_name = STRBUF_INIT;
> +	struct commit *head_commit, *merge_commit, *i;
> +	struct commit_list *common, *j, *reversed = NULL;
> +	struct merge_options o;
> +	int ret;
> +	static struct lock_file lock;
> +
> +	for (merge_arg_len = 0; merge_arg_len < arg_len; merge_arg_len++)
> +		if (isspace(arg[merge_arg_len]))
> +			break;

> +	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> +		return -1;
> +
> +	if (commit) {
> +		const char *message = get_commit_buffer(commit, NULL);
> +		const char *body;
> +		int len;
> +
> +		if (!message) {
> +			rollback_lock_file(&lock);
> +			return error(_("could not get commit message of '%s'"),
> +				     oid_to_hex(&commit->object.oid));
> +		}
> +		write_author_script(message);
> +		find_commit_subject(message, &body);
> +		len = strlen(body);
> +		if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
> +			error_errno(_("Could not write '%s'"),
> +				    git_path_merge_msg());
> +			unuse_commit_buffer(commit, message);
> +			rollback_lock_file(&lock);
> +			return -1;
> +		}
> +		unuse_commit_buffer(commit, message);
> +	} else {
> +		const char *p = arg + merge_arg_len;
> +		struct strbuf buf = STRBUF_INIT;
> +		int len;
> +
> +		strbuf_addf(&buf, "author %s", git_author_info(0));
> +		write_author_script(buf.buf);
> +		strbuf_reset(&buf);
> +
> +		p += strspn(p, " \t");

> +		if (*p)
> +			len = strlen(p);
> +		else {
> +			strbuf_addf(&buf, "Merge branch '%.*s'",
> +				    merge_arg_len, arg);
> +			p = buf.buf;
> +			len = buf.len;
> +		}

> +		if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> +			error_errno(_("Could not write '%s'"),
> +				    git_path_merge_msg());
> +			strbuf_release(&buf);
> +			rollback_lock_file(&lock);
> +			return -1;
> +		}
> +		strbuf_release(&buf);
> +	}

> +	head_commit = lookup_commit_reference_by_name("HEAD");
> +	if (!head_commit) {
> +		rollback_lock_file(&lock);
> +		return error(_("Cannot merge without a current revision"));
> +	}

> +	strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
> +	merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> +	if (!merge_commit) {
> +		/* fall back to non-rewritten ref or commit */
> +		strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0);
> +		merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> +	}

> +	if (!merge_commit) {
> +		error(_("could not resolve '%s'"), ref_name.buf);
> +		strbuf_release(&ref_name);
> +		rollback_lock_file(&lock);
> +		return -1;
> +	}
> +	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
> +		      git_path_merge_head(), 0);
> +	write_message("no-ff", 5, git_path_merge_mode(), 0);

> +	read_cache();
> +	init_merge_options(&o);
> +	o.branch1 = "HEAD";
> +	o.branch2 = ref_name.buf;
> +	o.buffer_output = 2;
> +
> +	ret = merge_recursive(&o, head_commit, merge_commit, reversed, &i);
> +	if (ret <= 0)
> +		fputs(o.obuf.buf, stdout);
> +...

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> >  	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> > +	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> > +	item->arg_len = (int)(eol - item->arg);
> > +
> >  	saved = *end_of_object_name;
> > +	if (item->command == TODO_MERGE && *bol == '-' &&
> > +	    bol + 1 == end_of_object_name) {
> > +		item->commit = NULL;
> > +		return 0;
> > +	}
> > +
> >  	*end_of_object_name = '\0';
> >  	status = get_oid(bol, &commit_oid);
> >  	*end_of_object_name = saved;
> >  
> > -	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> > -	item->arg_len = (int)(eol - item->arg);
> > -
> 
> Assigning to "saved" before the added "if we are doing merge and see
> '-', do this special thing" is not only unnecessary, but makes the
> logic in the non-special case harder to read.  The four things
> "saved = *eol; *eol = 0; do_thing_using(bol); *eol = saved;" is a
> single logical unit; keep them together.

> > +		if (*p)
> > +			len = strlen(p);
> > +		else {
> > +			strbuf_addf(&buf, "Merge branch '%.*s'",
> > +				    merge_arg_len, arg);
> > +			p = buf.buf;
> > +			len = buf.len;
> > +		}
> 
> So... "arg" received by this function can be a single non-whitespace
> token, which is taken as the name of the branch being merged (in
> this else clause).  Or it can also be followed by a single liner
> message for the merge commit.  Presumably, this is for creating a
> new merge (i.e. "commit==NULL" case), and preparing a proper log
> message in the todo list is unrealistic, so this would be a
> reasonable compromise.  Those users who want to write proper log
> message could presumably follow such "merge" insn with a "x git
> commit --amend" or something, I presume, if they really wanted to.

> > +		if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> > +			error_errno(_("Could not write '%s'"),
> > +				    git_path_merge_msg());
> > +			strbuf_release(&buf);
> > +			rollback_lock_file(&lock);
> > +			return -1;
> > +		}
> > +		strbuf_release(&buf);
> > +	}
> 
> OK.  Now we have prepared the MERGE_MSG file and are ready to commit.
> 
> > +	head_commit = lookup_commit_reference_by_name("HEAD");
> > +	if (!head_commit) {
> > +		rollback_lock_file(&lock);
> > +		return error(_("Cannot merge without a current revision"));
> > +	}
> 
> Hmph, I would have expected to see this a lot earlier, before
> dealing with the log message.  Leftover MERGE_MSG file after an
> error will cause unexpected fallout to the end-user experience
> (including what is shown by the shell prompt scripts), but if we do
> this before the MERGE_MSG thing, we do not have to worry about
> error codepath having to remove it.

> > +	strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
> > +	merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> > +	if (!merge_commit) {
> > +		/* fall back to non-rewritten ref or commit */
> > +		strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0);
> > +		merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> > +	}
> 
> OK, so "abc" in the example in the log message is looked up first as
> a label and then we take a fallback to interpret as an object name.

> > +	if (!merge_commit) {
> > +		error(_("could not resolve '%s'"), ref_name.buf);
> > +		strbuf_release(&ref_name);
> > +		rollback_lock_file(&lock);
> > +		return -1;
> > +	}
> > +	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
> > +		      git_path_merge_head(), 0);
> > +	write_message("no-ff", 5, git_path_merge_mode(), 0);
> 
> These two calls gave me a "Huh?" moment; write_message() sounds like
> it is allowed to be later updated with frills suitable for *_MSG
> files we place in .git/ directory (iow, it is in principle OK if
> commented out instructions common to these files are added to the
> output by the function), but these want exact bytes passed in the
> result, for which wrapper.c::write_file() is more appropriate.

> > +	common = get_merge_bases(head_commit, merge_commit);
> > +	for (j = common; j; j = j->next)
> > +		commit_list_insert(j->item, &reversed);
> > +	free_commit_list(common);
> 
> I know this is copy&pasted code from "builtin/merge.c", but is there
> a reason to reverse the common ancestor list here?

> 
> Just like with regular `pick` commands, if we are trying to recreate a
> merge commit, we now test whether the parents of said commit match HEAD
> and the commits to be merged, and fast-forward if possible.
> 
> This is not only faster, but also avoids unnecessary proliferation of
> new objects.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 567cfcbbe8b..a96255426e7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2085,7 +2085,7 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
>  	struct commit *head_commit, *merge_commit, *i;
>  	struct commit_list *common, *j, *reversed = NULL;
>  	struct merge_options o;
> -	int ret;
> +	int can_fast_forward, ret;
>  	static struct lock_file lock;
>  
>  	for (merge_arg_len = 0; merge_arg_len < arg_len; merge_arg_len++)
> @@ -2151,6 +2151,14 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
>  		return error(_("Cannot merge without a current revision"));
>  	}
>  
> +	/*
> +	 * If HEAD is not identical to the parent of the original merge commit,
> +	 * we cannot fast-forward.
> +	 */
> +	can_fast_forward = commit && commit->parents &&
> +		!oidcmp(&commit->parents->item->object.oid,
> +			&head_commit->object.oid);
> +
>  	strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
>  	merge_commit = lookup_commit_reference_by_name(ref_name.buf);
>  	if (!merge_commit) {
> @@ -2164,6 +2172,17 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
>  		rollback_lock_file(&lock);
>  		return -1;
>  	}
> +
> +	if (can_fast_forward && commit->parents->next &&
> +	    !commit->parents->next->next &&
> +	    !oidcmp(&commit->parents->next->item->object.oid,
> +		    &merge_commit->object.oid)) {
> +		strbuf_release(&ref_name);
> +		rollback_lock_file(&lock);
> +		return fast_forward_to(&commit->object.oid,
> +				       &head_commit->object.oid, 0, opts);
> +	}
> +
>  	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
>  		      git_path_merge_head(), 0);
>  	write_message("no-ff", 5, git_path_merge_mode(), 0);
> 

> On 18/01/18 15:35, Johannes Schindelin wrote:
>> 
>> Just like with regular `pick` commands, if we are trying to recreate a
>> merge commit, we now test whether the parents of said commit match HEAD
>> and the commits to be merged, and fast-forward if possible.
>> 
>> This is not only faster, but also avoids unnecessary proliferation of
>> new objects.
>
> I might have missed something but shouldn't this be checking opts->allow_ff?

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> On 18/01/18 15:35, Johannes Schindelin wrote:
>>>
>>> Just like with regular `pick` commands, if we are trying to recreate a
>>> merge commit, we now test whether the parents of said commit match HEAD
>>> and the commits to be merged, and fast-forward if possible.
>>>
>>> This is not only faster, but also avoids unnecessary proliferation of
>>> new objects.
>>
>> I might have missed something but shouldn't this be checking opts->allow_ff?
> 
> Because the whole point of this mechanism is to recreate the
> topology faithfully to the original, even if the original was a
> redundant merge (which has a side parent that is an ancestor or a
> descendant of the first parent), we should just point at the
> original merge when the condition allows it, regardless of
> opts->allow_ff.

> I think it is a different matter if an insn to create a new merge
> (i.e. "merge - <parent> <message>", not "merge <commit> <parent>")
> should honor opts->allow_ff; because it is not about recreating an
> existing history but is a way to create what did not exist before,
> I think it is sensible if allow_ff option is honored.

> On 18/01/18 15:35, Johannes Schindelin wrote:
> > 
> > Just like with regular `pick` commands, if we are trying to recreate a
> > merge commit, we now test whether the parents of said commit match HEAD
> > and the commits to be merged, and fast-forward if possible.
> > 
> > This is not only faster, but also avoids unnecessary proliferation of
> > new objects.
> 
> I might have missed something but shouldn't this be checking
> opts->allow_ff?

> +	/*
> +	 * If HEAD is not identical to the parent of the original merge commit,
> +	 * we cannot fast-forward.
> +	 */
> +	can_fast_forward = commit && commit->parents &&
> +		!oidcmp(&commit->parents->item->object.oid,
> +			&head_commit->object.oid);
> +

>  	strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
>  	merge_commit = lookup_commit_reference_by_name(ref_name.buf);
>  	if (!merge_commit) {
> @@ -2164,6 +2172,17 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
>  		rollback_lock_file(&lock);
>  		return -1;
>  	}
> +
> +	if (can_fast_forward && commit->parents->next &&
> +	    !commit->parents->next->next &&
> +	    !oidcmp(&commit->parents->next->item->object.oid,
> +		    &merge_commit->object.oid)) {

> +		strbuf_release(&ref_name);
> +		rollback_lock_file(&lock);
> +		return fast_forward_to(&commit->object.oid,
> +				       &head_commit->object.oid, 0, opts);
> +	}
> +
>  	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
>  		      git_path_merge_head(), 0);
>  	write_message("no-ff", 5, git_path_merge_mode(), 0);

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +	/*
> > +	 * If HEAD is not identical to the parent of the original merge commit,
> > +	 * we cannot fast-forward.
> > +	 */
> > +	can_fast_forward = commit && commit->parents &&
> > +		!oidcmp(&commit->parents->item->object.oid,
> > +			&head_commit->object.oid);
> > +
> 
> I think this expression and assignment should better be done much
> later.  Are you going to update commit, commit->parents, etc. that
> are involved in the computation in the meantime???

> > @@ -2164,6 +2172,17 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
> >  		rollback_lock_file(&lock);
> >  		return -1;
> >  	}
> > +
> > +	if (can_fast_forward && commit->parents->next &&
> > +	    !commit->parents->next->next &&
> > +	    !oidcmp(&commit->parents->next->item->object.oid,
> > +		    &merge_commit->object.oid)) {
> 
> ... Namely, here.

> Because the earlier one is computing "are we
> replaying exactly the same commit on top of exactly the same
> state?", which is merely one half of "can we fast-forward", and
> storing it in a variable whose name is over-promising way before it
> becomes necessary.  The other half of "can we fast-forward?" logic
> is the remainder of the if() condition we see above.  IOW, when
> fully spelled, this code can fast-forward when we are replaying a
> commit on top of exactly the same first-parent and the commit being
> replayed is a single parent merge.
> 
> We may even want to get rid of can_fast_forward variable.

> The sequencer just learned a new commands intended to recreate branch
> structure (similar in spirit to --preserve-merges, but with a
> substantially less-broken design).
>
> Let's allow the rebase--helper to generate todo lists making use of
> these commands, triggered by the new --recreate-merges option. For a
> commit topology like this:
>
> A - B - C
>   \   /
>     D

>
> the generated todo list would look like this:
>
> # branch D
> pick 0123 A
> label branch-point
> pick 1234 D
> label D
>
> reset branch-point
> pick 2345 B
> merge 3456 D C
>
> To keep things simple, we first only implement support for merge commits
> with exactly two parents, leaving support for octopus merges to a later
> patch in this patch series.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/rebase--helper.c |   4 +-
> sequencer.c              | 343 
> ++++++++++++++++++++++++++++++++++++++++++++++-
> sequencer.h              |   1 +
> 3 files changed, 345 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index 7daee544b7b..a34ab5c0655 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] 
> = {
> int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> {
>  struct replay_opts opts = REPLAY_OPTS_INIT;
> - unsigned flags = 0, keep_empty = 0;
> + unsigned flags = 0, keep_empty = 0, recreate_merges = 0;
>  int abbreviate_commands = 0;
>  enum {
>  CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
> @@ -22,6 +22,7 @@ int cmd_rebase__helper(int argc, const char **argv, 
> const char *prefix)
>  struct option options[] = {
>  OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
>  OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
> + OPT_BOOL(0, "recreate-merges", &recreate_merges, N_("recreate merge 
> commits")),
>  OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
>  CONTINUE),
>  OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
> @@ -55,6 +56,7 @@ int cmd_rebase__helper(int argc, const char **argv, 
> const char *prefix)
>
>  flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
>  flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
> + flags |= recreate_merges ? TODO_LIST_RECREATE_MERGES : 0;
>  flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
>
>  if (command == CONTINUE && argc == 1)
> diff --git a/sequencer.c b/sequencer.c
> index a96255426e7..1bef16647b4 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -23,6 +23,8 @@
> #include "hashmap.h"
> #include "unpack-trees.h"
> #include "worktree.h"
> +#include "oidmap.h"
> +#include "oidset.h"
>
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int 
> ignore_footer, unsigned flag)
>  strbuf_release(&sob);
> }
>
> +struct labels_entry {
> + struct hashmap_entry entry;
> + char label[FLEX_ARRAY];
> +};
> +
> +static int labels_cmp(const void *fndata, const struct labels_entry *a,
> +       const struct labels_entry *b, const void *key)
> +{
> + return key ? strcmp(a->label, key) : strcmp(a->label, b->label);
> +}
> +
> +struct string_entry {
> + struct oidmap_entry entry;
> + char string[FLEX_ARRAY];
> +};
> +
> +struct label_state {
> + struct oidmap commit2label;
> + struct hashmap labels;
> + struct strbuf buf;
> +};
> +
> +static const char *label_oid(struct object_id *oid, const char *label,
> +      struct label_state *state)
> +{
> + struct labels_entry *labels_entry;
> + struct string_entry *string_entry;
> + struct object_id dummy;
> + size_t len;
> + int i;
> +
> + string_entry = oidmap_get(&state->commit2label, oid);
> + if (string_entry)
> + return string_entry->string;
> +
> + /*
> + * For "uninteresting" commits, i.e. commits that are not to be
> + * rebased, and which can therefore not be labeled, we use a unique
> + * abbreviation of the commit name. This is slightly more complicated
> + * than calling find_unique_abbrev() because we also need to make
> + * sure that the abbreviation does not conflict with any other
> + * label.
> + *
> + * We disallow "interesting" commits to be labeled by a string that
> + * is a valid full-length hash, to ensure that we always can find an
> + * abbreviation for any uninteresting commit's names that does not
> + * clash with any other label.
> + */
> + if (!label) {
> + char *p;
> +
> + strbuf_reset(&state->buf);
> + strbuf_grow(&state->buf, GIT_SHA1_HEXSZ);
> + label = p = state->buf.buf;
> +
> + find_unique_abbrev_r(p, oid->hash, default_abbrev);
> +
> + /*
> + * We may need to extend the abbreviated hash so that there is
> + * no conflicting label.
> + */
> + if (hashmap_get_from_hash(&state->labels, strihash(p), p)) {
> + size_t i = strlen(p) + 1;
> +
> + oid_to_hex_r(p, oid);
> + for (; i < GIT_SHA1_HEXSZ; i++) {
> + char save = p[i];
> + p[i] = '\0';
> + if (!hashmap_get_from_hash(&state->labels,
> +    strihash(p), p))
> + break;
> + p[i] = save;
> + }
> + }
> + } else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
> +     !get_oid_hex(label, &dummy)) ||
> +    hashmap_get_from_hash(&state->labels,
> + strihash(label), label)) {
> + /*
> + * If the label already exists, or if the label is a valid full
> + * OID, we append a dash and a number to make it unique.
> + */
> + struct strbuf *buf = &state->buf;
> +
> + strbuf_reset(buf);
> + strbuf_add(buf, label, len);
> +
> + for (i = 2; ; i++) {
> + strbuf_setlen(buf, len);
> + strbuf_addf(buf, "-%d", i);
> + if (!hashmap_get_from_hash(&state->labels,
> +    strihash(buf->buf),
> +    buf->buf))
> + break;
> + }
> +
> + label = buf->buf;
> + }
> +
> + FLEX_ALLOC_STR(labels_entry, label, label);
> + hashmap_entry_init(labels_entry, strihash(label));
> + hashmap_add(&state->labels, labels_entry);
> +
> + FLEX_ALLOC_STR(string_entry, string, label);
> + oidcpy(&string_entry->entry.oid, oid);
> + oidmap_put(&state->commit2label, string_entry);
> +
> + return string_entry->string;
> +}
> +
> +static int make_script_with_merges(struct pretty_print_context *pp,
> +    struct rev_info *revs, FILE *out,
> +    unsigned flags)
> +{
> + int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
> + struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
> + struct strbuf label = STRBUF_INIT;
> + struct commit_list *commits = NULL, **tail = &commits, *iter;
> + struct commit_list *tips = NULL, **tips_tail = &tips;
> + struct commit *commit;
> + struct oidmap commit2todo = OIDMAP_INIT;
> + struct string_entry *entry;
> + struct oidset interesting = OIDSET_INIT, child_seen = OIDSET_INIT,
> + shown = OIDSET_INIT;
> + struct label_state state = { OIDMAP_INIT, { NULL }, STRBUF_INIT };
> +
> + int abbr = flags & TODO_LIST_ABBREVIATE_CMDS;
> + const char *p = abbr ? "p" : "pick", *l = abbr ? "l" : "label",
> + *t = abbr ? "t" : "reset", *b = abbr ? "b" : "bud",
> + *m = abbr ? "m" : "merge";
> +
> + oidmap_init(&commit2todo, 0);
> + oidmap_init(&state.commit2label, 0);
> + hashmap_init(&state.labels, (hashmap_cmp_fn) labels_cmp, NULL, 0);
> + strbuf_init(&state.buf, 32);
> +
> + if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
> + struct object_id *oid = &revs->cmdline.rev[0].item->oid;
> + FLEX_ALLOC_STR(entry, string, "onto");

> + oidcpy(&entry->entry.oid, oid);
> + oidmap_put(&state.commit2label, entry);
> + }
> +
> + /*
> + * First phase:
> + * - get onelines for all commits
> + * - gather all branch tips (i.e. 2nd or later parents of merges)
> + * - label all branch tips
> + */
> + while ((commit = get_revision(revs))) {
> + struct commit_list *to_merge;
> + int is_octopus;
> + const char *p1, *p2;
> + struct object_id *oid;
> +
> + tail = &commit_list_insert(commit, tail)->next;
> + oidset_insert(&interesting, &commit->object.oid);
> +
> + if ((commit->object.flags & PATCHSAME))
> + continue;
> +
> + strbuf_reset(&oneline);
> + pretty_print_commit(pp, commit, &oneline);
> +
> + to_merge = commit->parents ? commit->parents->next : NULL;
> + if (!to_merge) {
> + /* non-merge commit: easy case */
> + strbuf_reset(&buf);
> + if (!keep_empty && is_original_commit_empty(commit))
> + strbuf_addf(&buf, "%c ", comment_line_char);
> + strbuf_addf(&buf, "%s %s %s", p,
> +     oid_to_hex(&commit->object.oid),
> +     oneline.buf);
> +
> + FLEX_ALLOC_STR(entry, string, buf.buf);
> + oidcpy(&entry->entry.oid, &commit->object.oid);
> + oidmap_put(&commit2todo, entry);
> +
> + continue;
> + }
> +
> + is_octopus = to_merge && to_merge->next;
> +
> + if (is_octopus)
> + BUG("Octopus merges not yet supported");
> +
> + /* Create a label */
> + strbuf_reset(&label);
> + if (skip_prefix(oneline.buf, "Merge ", &p1) &&
> +     (p1 = strchr(p1, '\'')) &&
> +     (p2 = strchr(++p1, '\'')))
> + strbuf_add(&label, p1, p2 - p1);
> + else if (skip_prefix(oneline.buf, "Merge pull request ",
> +      &p1) &&
> + (p1 = strstr(p1, " from ")))
> + strbuf_addstr(&label, p1 + strlen(" from "));
> + else
> + strbuf_addbuf(&label, &oneline);
> +
> + for (p1 = label.buf; *p1; p1++)
> + if (isspace(*p1))
> + *(char *)p1 = '-';
> +
> + strbuf_reset(&buf);
> + strbuf_addf(&buf, "%s %s", m, oid_to_hex(&commit->object.oid));
> +
> + /* label the tip of merged branch */
> + oid = &to_merge->item->object.oid;
> + strbuf_addch(&buf, ' ');
> +
> + if (!oidset_contains(&interesting, oid))
> + strbuf_addstr(&buf, label_oid(oid, NULL, &state));
> + else {
> + tips_tail = &commit_list_insert(to_merge->item,
> + tips_tail)->next;
> +
> + strbuf_addstr(&buf, label_oid(oid, label.buf, &state));
> + }
> + strbuf_addf(&buf, " %s", oneline.buf);
> +
> + FLEX_ALLOC_STR(entry, string, buf.buf);
> + oidcpy(&entry->entry.oid, &commit->object.oid);
> + oidmap_put(&commit2todo, entry);
> + }
> +
> + /*
> + * Second phase:
> + * - label branch points
> + * - add HEAD to the branch tips
> + */
> + for (iter = commits; iter; iter = iter->next) {
> + struct commit_list *parent = iter->item->parents;
> + for (; parent; parent = parent->next) {
> + struct object_id *oid = &parent->item->object.oid;
> + if (!oidset_contains(&interesting, oid))
> + continue;
> + if (!oidset_contains(&child_seen, oid))
> + oidset_insert(&child_seen, oid);
> + else
> + label_oid(oid, "branch-point", &state);
> + }
> +
> + /* Add HEAD as implict "tip of branch" */
> + if (!iter->next)
> + tips_tail = &commit_list_insert(iter->item,
> + tips_tail)->next;
> + }
> +
> + /*
> + * Third phase: output the todo list. This is a bit tricky, as we
> + * want to avoid jumping back and forth between revisions. To
> + * accomplish that goal, we walk backwards from the branch tips,
> + * gathering commits not yet shown, reversing the list on the fly,
> + * then outputting that list (labeling revisions as needed).
> + */
> + fprintf(out, "%s onto\n", l);
> + for (iter = tips; iter; iter = iter->next) {
> + struct commit_list *list = NULL, *iter2;
> +
> + commit = iter->item;
> + if (oidset_contains(&shown, &commit->object.oid))
> + continue;
> + entry = oidmap_get(&state.commit2label, &commit->object.oid);
> +
> + if (entry)
> + fprintf(out, "\n# Branch %s\n", entry->string);
> + else
> + fprintf(out, "\n");
> +
> + while (oidset_contains(&interesting, &commit->object.oid) &&
> +        !oidset_contains(&shown, &commit->object.oid)) {
> + commit_list_insert(commit, &list);
> + if (!commit->parents) {
> + commit = NULL;
> + break;
> + }
> + commit = commit->parents->item;
> + }
> +
> + if (!commit)
> + fprintf(out, "%s\n", b);
> + else {
> + const char *to = NULL;
> +
> + entry = oidmap_get(&state.commit2label,
> +    &commit->object.oid);
> + if (entry)
> + to = entry->string;
> +
> + if (!to || !strcmp("onto", to))
> + fprintf(out, "%s\n", b);
> + else {
> + strbuf_reset(&oneline);
> + pretty_print_commit(pp, commit, &oneline);
> + fprintf(out, "%s %s %s\n",
> + t, to, oneline.buf);
> + }
> + }
> +
> + for (iter2 = list; iter2; iter2 = iter2->next) {
> + struct object_id *oid = &iter2->item->object.oid;
> + entry = oidmap_get(&commit2todo, oid);
> + /* only show if not already upstream */
> + if (entry)
> + fprintf(out, "%s\n", entry->string);
> + entry = oidmap_get(&state.commit2label, oid);
> + if (entry)
> + fprintf(out, "%s %s\n", l, entry->string);
> + oidset_insert(&shown, oid);
> + }
> +
> + free_commit_list(list);
> + }
> +
> + free_commit_list(commits);
> + free_commit_list(tips);
> +
> + strbuf_release(&label);
> + strbuf_release(&oneline);
> + strbuf_release(&buf);
> +
> + oidmap_free(&commit2todo, 1);
> + oidmap_free(&state.commit2label, 1);
> + hashmap_free(&state.labels, 1);
> + strbuf_release(&state.buf);
> +
> + return 0;
> +}
> +
> int sequencer_make_script(FILE *out, int argc, const char **argv,
>    unsigned flags)
> {
> @@ -2795,11 +3126,16 @@ int sequencer_make_script(FILE *out, int argc, 
> const char **argv,
>  struct commit *commit;
>  int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
>  const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
> + int recreate_merges = flags & TODO_LIST_RECREATE_MERGES;
>
>  init_revisions(&revs, NULL);
>  revs.verbose_header = 1;
> - revs.max_parents = 1;
> - revs.cherry_pick = 1;
> + if (recreate_merges)
> + revs.cherry_mark = 1;
> + else {
> + revs.max_parents = 1;
> + revs.cherry_pick = 1;
> + }
>  revs.limited = 1;
>  revs.reverse = 1;
>  revs.right_only = 1;
> @@ -2823,6 +3159,9 @@ int sequencer_make_script(FILE *out, int argc, const 
> char **argv,
>  if (prepare_revision_walk(&revs) < 0)
>  return error(_("make_script: error preparing revisions"));
>
> + if (recreate_merges)
> + return make_script_with_merges(&pp, &revs, out, flags);
> +
>  while ((commit = get_revision(&revs))) {
>  strbuf_reset(&buf);
>  if (!keep_empty && is_original_commit_empty(commit))
> diff --git a/sequencer.h b/sequencer.h
> index 81f6d7d393f..11d1ac925ef 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -48,6 +48,7 @@ int sequencer_remove_state(struct replay_opts *opts);
> #define TODO_LIST_KEEP_EMPTY (1U << 0)
> #define TODO_LIST_SHORTEN_IDS (1U << 1)
> #define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
> +#define TODO_LIST_RECREATE_MERGES (1U << 3)
> int sequencer_make_script(FILE *out, int argc, const char **argv,
>    unsigned flags);
>
> -- 
> 2.15.1.windows.2.1430.ga56c4f9e2a9
>
> 

> structure (similar in spirit to --preserve-merges, but with a
> substantially less-broken design).
> [...]
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> +static const char *label_oid(struct object_id *oid, const char *label,
> +                            struct label_state *state)
> +{
> +       [...]
> +       } else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
> +                   !get_oid_hex(label, &dummy)) ||
> +                  hashmap_get_from_hash(&state->labels,
> +                                        strihash(label), label)) {
> +               /*
> +                * If the label already exists, or if the label is a valid full
> +                * OID, we append a dash and a number to make it unique.
> +                */
> +               [...]
> +               for (i = 2; ; i++) {

> +                       strbuf_setlen(buf, len);
> +                       strbuf_addf(buf, "-%d", i);
> +                       if (!hashmap_get_from_hash(&state->labels,
> +                                                  strihash(buf->buf),
> +                                                  buf->buf))
> +                               break;
> +               }
> +
> +static int make_script_with_merges(struct pretty_print_context *pp,
> +                                  struct rev_info *revs, FILE *out,
> +                                  unsigned flags)
> +{
> +       [...]
> +               is_octopus = to_merge && to_merge->next;
> +
> +               if (is_octopus)
> +                       BUG("Octopus merges not yet supported");

>> +               is_octopus = to_merge && to_merge->next;
>> +
>> +               if (is_octopus)
>> +                       BUG("Octopus merges not yet supported");
>
> Is this a situation which the end-user can trigger by specifying a
> merge with more than two parents? If so, shouldn't this be just a
> normal error message rather than a (developer) bug message? Or, am I
> misunderstanding?

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> +               is_octopus = to_merge && to_merge->next;
> >> +
> >> +               if (is_octopus)
> >> +                       BUG("Octopus merges not yet supported");
> >
> > Is this a situation which the end-user can trigger by specifying a
> > merge with more than two parents? If so, shouldn't this be just a
> > normal error message rather than a (developer) bug message? Or, am I
> > misunderstanding?
> 
> BUG() is "we wrote code carefully so that this should not trigger;
> we do not _expect_ the code to reach here".  This one is expected to
> trigger, and I agree with you that it should be die(), if the series
> is meant to be released to the general public in the current form
> (i.e. until the limitation is lifted so that it can handle an
> octopus).
> 
> If the callers are made more careful to check if there is an octopus
> involved and reject the request early, then seeing an octopus in
> this location in a loop will become a BUG().

> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> 
> > structure (similar in spirit to --preserve-merges, but with a
> > substantially less-broken design).
> > [...]
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> > +static const char *label_oid(struct object_id *oid, const char *label,
> > +                            struct label_state *state)
> > +{
> > +       [...]
> > +       } else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
> > +                   !get_oid_hex(label, &dummy)) ||
> > +                  hashmap_get_from_hash(&state->labels,
> > +                                        strihash(label), label)) {
> > +               /*
> > +                * If the label already exists, or if the label is a valid full
> > +                * OID, we append a dash and a number to make it unique.
> > +                */
> > +               [...]
> > +               for (i = 2; ; i++) {
> 
> Why '2'? Is there some non-obvious significance to this value?

> > +static int make_script_with_merges(struct pretty_print_context *pp,
> > +                                  struct rev_info *revs, FILE *out,
> > +                                  unsigned flags)
> > +{
> > +       [...]
> > +               is_octopus = to_merge && to_merge->next;
> > +
> > +               if (is_octopus)
> > +                       BUG("Octopus merges not yet supported");
> 
> Is this a situation which the end-user can trigger by specifying a
> merge with more than two parents? If so, shouldn't this be just a
> normal error message rather than a (developer) bug message? Or, am I
> misunderstanding?

> structure (similar in spirit to --preserve-merges, but with a
> substantially less-broken design).
> ...
> @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>  	strbuf_release(&sob);
>  }
>  
> +struct labels_entry {
> +	struct hashmap_entry entry;
> +	char label[FLEX_ARRAY];
> +};
> +
> +static int labels_cmp(const void *fndata, const struct labels_entry *a,
> +		      const struct labels_entry *b, const void *key)
> +{
> +	return key ? strcmp(a->label, key) : strcmp(a->label, b->label);
> +}

> +struct string_entry {
> +	struct oidmap_entry entry;
> +	char string[FLEX_ARRAY];
> +};
> +
> +struct label_state {
> +	struct oidmap commit2label;
> +	struct hashmap labels;
> +	struct strbuf buf;
> +};
> +
> +static const char *label_oid(struct object_id *oid, const char *label,
> +			     struct label_state *state)
> +{
> +	struct labels_entry *labels_entry;
> +	struct string_entry *string_entry;
> +	struct object_id dummy;
> +	size_t len;
> +	int i;
> +
> +	string_entry = oidmap_get(&state->commit2label, oid);
> +	if (string_entry)
> +		return string_entry->string;
> +
> +	/*
> +	 * For "uninteresting" commits, i.e. commits that are not to be
> +	 * rebased, and which can therefore not be labeled, we use a unique
> +	 * abbreviation of the commit name. This is slightly more complicated
> +	 * than calling find_unique_abbrev() because we also need to make
> +	 * sure that the abbreviation does not conflict with any other
> +	 * label.
> +	 *
> +	 * We disallow "interesting" commits to be labeled by a string that
> +	 * is a valid full-length hash, to ensure that we always can find an
> +	 * abbreviation for any uninteresting commit's names that does not
> +	 * clash with any other label.
> +	 */
> +	if (!label) {
> +		char *p;
> +
> +		strbuf_reset(&state->buf);
> +		strbuf_grow(&state->buf, GIT_SHA1_HEXSZ);
> +		label = p = state->buf.buf;
> +
> +		find_unique_abbrev_r(p, oid->hash, default_abbrev);
> +
> +		/*
> +		 * We may need to extend the abbreviated hash so that there is
> +		 * no conflicting label.
> +		 */
> +		if (hashmap_get_from_hash(&state->labels, strihash(p), p)) {
> +			size_t i = strlen(p) + 1;
> +
> +			oid_to_hex_r(p, oid);
> +			for (; i < GIT_SHA1_HEXSZ; i++) {
> +				char save = p[i];
> +				p[i] = '\0';
> +				if (!hashmap_get_from_hash(&state->labels,
> +							   strihash(p), p))
> +					break;
> +				p[i] = save;
> +			}
> +		}

> +static int make_script_with_merges(struct pretty_print_context *pp,
> +				   struct rev_info *revs, FILE *out,
> +				   unsigned flags)
> +{
> + ...
> +	int abbr = flags & TODO_LIST_ABBREVIATE_CMDS;
> +	const char *p = abbr ? "p" : "pick", *l = abbr ? "l" : "label",
> +		 *t = abbr ? "t" : "reset", *b = abbr ? "b" : "bud",
> +		 *m = abbr ? "m" : "merge";

> +
> +	oidmap_init(&commit2todo, 0);
> +	oidmap_init(&state.commit2label, 0);
> +	hashmap_init(&state.labels, (hashmap_cmp_fn) labels_cmp, NULL, 0);
> +	strbuf_init(&state.buf, 32);
> +
> +	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
> +		struct object_id *oid = &revs->cmdline.rev[0].item->oid;
> +		FLEX_ALLOC_STR(entry, string, "onto");
> +		oidcpy(&entry->entry.oid, oid);
> +		oidmap_put(&state.commit2label, entry);
> +	}
> +
> +	/*
> +	 * First phase:
> +	 * - get onelines for all commits
> +	 * - gather all branch tips (i.e. 2nd or later parents of merges)
> +	 * - label all branch tips
> +	 */

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > structure (similar in spirit to --preserve-merges, but with a
> > substantially less-broken design).
> > ...
> > @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> >  	strbuf_release(&sob);
> >  }
> >  
> > +struct labels_entry {
> > +	struct hashmap_entry entry;
> > +	char label[FLEX_ARRAY];
> > +};
> > +
> > +static int labels_cmp(const void *fndata, const struct labels_entry *a,
> > +		      const struct labels_entry *b, const void *key)
> > +{
> > +	return key ? strcmp(a->label, key) : strcmp(a->label, b->label);
> > +}
> 
> label_oid() accesses state->labels hash using strihash() as the hash
> function, but the final comparison between the entries in the same
> hash buckets are done with case sensitivity.  It is unclear to me if
> that is what was intended, and why.

> > +struct string_entry {
> > +	struct oidmap_entry entry;
> > +	char string[FLEX_ARRAY];
> > +};
> > +
> > +struct label_state {
> > +	struct oidmap commit2label;
> > +	struct hashmap labels;
> > +	struct strbuf buf;
> > +};
> > +
> > +static const char *label_oid(struct object_id *oid, const char *label,
> > +			     struct label_state *state)
> > +{
> > +	struct labels_entry *labels_entry;
> > +	struct string_entry *string_entry;
> > +	struct object_id dummy;
> > +	size_t len;
> > +	int i;
> > +
> > +	string_entry = oidmap_get(&state->commit2label, oid);
> > +	if (string_entry)
> > +		return string_entry->string;
> > +
> > +	/*
> > +	 * For "uninteresting" commits, i.e. commits that are not to be
> > +	 * rebased, and which can therefore not be labeled, we use a unique
> > +	 * abbreviation of the commit name. This is slightly more complicated
> > +	 * than calling find_unique_abbrev() because we also need to make
> > +	 * sure that the abbreviation does not conflict with any other
> > +	 * label.
> > +	 *
> > +	 * We disallow "interesting" commits to be labeled by a string that
> > +	 * is a valid full-length hash, to ensure that we always can find an
> > +	 * abbreviation for any uninteresting commit's names that does not
> > +	 * clash with any other label.
> > +	 */
> > +	if (!label) {
> > +		char *p;
> > +
> > +		strbuf_reset(&state->buf);
> > +		strbuf_grow(&state->buf, GIT_SHA1_HEXSZ);
> > +		label = p = state->buf.buf;
> > +
> > +		find_unique_abbrev_r(p, oid->hash, default_abbrev);
> > +
> > +		/*
> > +		 * We may need to extend the abbreviated hash so that there is
> > +		 * no conflicting label.
> > +		 */
> > +		if (hashmap_get_from_hash(&state->labels, strihash(p), p)) {
> > +			size_t i = strlen(p) + 1;
> > +
> > +			oid_to_hex_r(p, oid);
> > +			for (; i < GIT_SHA1_HEXSZ; i++) {
> > +				char save = p[i];
> > +				p[i] = '\0';
> > +				if (!hashmap_get_from_hash(&state->labels,
> > +							   strihash(p), p))
> > +					break;
> > +				p[i] = save;
> > +			}
> > +		}
> 
> If oid->hash required full 40-hex to disambiguate, then
> find-unique-abbrev would give 40-hex and we'd want the same "-<num>"
> suffix technique employed below to make it consistently unique.  I
> wonder if organizing the function this way ...
> 
> 	if (!label)
> 		label = oid-to-hex(oid);
> 
> 	if (label already exists or full oid) {
> 		make it unambiguous;
> 	}

> > +static int make_script_with_merges(struct pretty_print_context *pp,
> > +				   struct rev_info *revs, FILE *out,
> > +				   unsigned flags)
> > +{
> > + ...
> > +	int abbr = flags & TODO_LIST_ABBREVIATE_CMDS;
> > +	const char *p = abbr ? "p" : "pick", *l = abbr ? "l" : "label",
> > +		 *t = abbr ? "t" : "reset", *b = abbr ? "b" : "bud",
> > +		 *m = abbr ? "m" : "merge";
> 
> It would be easier to understand if these short named variables are
> reserved only for temporary use, not as constants.  It is not too
> much to spell 
> 
> 	fprintf(out, "%s onto\n", cmd_label);
> 
> than
> 
> 	fprintf(out, "%s onto\n", l);
> 
> and would save readers from head-scratching, wondering where the
> last assignment to variable "l" is.

> > +	oidmap_init(&commit2todo, 0);
> > +	oidmap_init(&state.commit2label, 0);
> > +	hashmap_init(&state.labels, (hashmap_cmp_fn) labels_cmp, NULL, 0);
> > +	strbuf_init(&state.buf, 32);
> > +
> > +	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
> > +		struct object_id *oid = &revs->cmdline.rev[0].item->oid;
> > +		FLEX_ALLOC_STR(entry, string, "onto");
> > +		oidcpy(&entry->entry.oid, oid);
> > +		oidmap_put(&state.commit2label, entry);
> > +	}
> > +
> > +	/*
> > +	 * First phase:
> > +	 * - get onelines for all commits
> > +	 * - gather all branch tips (i.e. 2nd or later parents of merges)
> > +	 * - label all branch tips
> > +	 */
> 
> When an early part of a branch is merged and then the remaining part
> of the same branch is merged again, "branch tip" and "2nd or later
> parents of merges" would become different concepts.  The 2nd parent
> of an early merge is not among the branch tips.
> 
> For the purpose of the "recreate the topology" algorithm, I am
> imagining that you would need not just the tips but all the 2nd and
> subsequent parents of merges, and my quick skimming tells me that
> the following code grabs them correctly.

> [...]
> With this patch, the goodness of the Git garden shears comes to `git
> rebase -i` itself. Passing the `--recreate-merges` option will generate
> a todo list that can be understood readily, and where it is obvious
> how to reorder commits. New branches can be introduced by inserting
> `label` commands and calling `merge - <label> <oneline>`. And once this
> mode has become stable and universally accepted, we can deprecate the
> design mistake that was `--preserve-merges`.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> @@ -900,6 +900,7 @@ fi
>  if test t != "$preserve_merges"
>  then
>         git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> +               ${recreate_merges:+--recreate-merges} \

>                 $revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
>         die "$(gettext "Could not generate todo list")"
> diff --git a/git-rebase.sh b/git-rebase.sh
> @@ -262,6 +264,10 @@ do
> +       --recreate-merges)
> +               recreate_merges=t
> +               test -z "$interactive_rebase" && interactive_rebase=implied
> +               ;;
>         --preserve-merges)
>                 preserve_merges=t
>                 test -z "$interactive_rebase" && interactive_rebase=implied

> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > [...]
> > With this patch, the goodness of the Git garden shears comes to `git
> > rebase -i` itself. Passing the `--recreate-merges` option will generate
> > a todo list that can be understood readily, and where it is obvious
> > how to reorder commits. New branches can be introduced by inserting
> > `label` commands and calling `merge - <label> <oneline>`. And once this
> > mode has become stable and universally accepted, we can deprecate the
> > design mistake that was `--preserve-merges`.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > @@ -900,6 +900,7 @@ fi
> >  if test t != "$preserve_merges"
> >  then
> >         git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> > +               ${recreate_merges:+--recreate-merges} \
> 
> If the user specifies both --preserve-merges and --recreate-merges, it
> looks like --preserve-merges will take precedence.
> 
> Should git-rebase.sh have a mutual-exclusion check and error out if
> both are specified?

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 8a861c1e0d6..1d061373288 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -368,6 +368,11 @@ The commit list format can be changed by setting the configuration option
>  rebase.instructionFormat.  A customized instruction format will automatically
>  have the long commit hash prepended to the format.
>  
> +--recreate-merges::
> +	Recreate merge commits instead of flattening the history by replaying
> +	merges. Merge conflict resolutions or manual amendments to merge
> +	commits are not preserved.
> +

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> > index 8a861c1e0d6..1d061373288 100644
> > --- a/Documentation/git-rebase.txt
> > +++ b/Documentation/git-rebase.txt
> > @@ -368,6 +368,11 @@ The commit list format can be changed by setting the configuration option
> >  rebase.instructionFormat.  A customized instruction format will automatically
> >  have the long commit hash prepended to the format.
> >  
> > +--recreate-merges::
> > +	Recreate merge commits instead of flattening the history by replaying
> > +	merges. Merge conflict resolutions or manual amendments to merge
> > +	commits are not preserved.
> > +
> 
> It is sensible to postpone tackling "evil merges" in this initial
> iteration of the series, and "manual amendments ... not preserved"
> is a reasonable thing to document.  But do we want to say a bit more
> about conflicting merges?  "conflict resolutions ... not preserved"
> sounds as if it does not stop and instead record the result with
> conflict markers without even letting rerere to kick in, which
> certainly is not the impression you wanted to give to the readers.
> 
> I am imagining that it will stop and give control back to the end
> user just like a conflicted "pick" would, and allow "rebase
> --continue" to record resolution from the working tree, and just
> like conflicted "pick", it would allow rerere() to help end users
> recall previous resolution.

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> [...]
>
>> +--recreate-merges::
>> +     Recreate merge commits instead of flattening the history by replaying
>> +     merges. Merge conflict resolutions or manual amendments to merge
>> +     commits are not preserved.
>
> I wonder why you guys still hold on replaying "merge-the-operation"
> instead of replaying "merge-the-result"? The latter, the merge commit
> itself, no matter how exactly it was created in the first place, is the
> most valuable thing git keeps about the merge, and you silently drop it
> entirely! OTOH, git keeps almost no information about
> "merge-the-operation", so it's virtually impossible to reliably replay
> the operation automatically, and yet you try to.
>

> IMHO that was severe mistake in the original --preserve-merges, and you
> bring with you to this new --recreate-merges... It's sad. Even more sad
> as solution is already known for years:
>
>     bc00341838a8faddcd101da9e746902994eef38a
>     Author: Johannes Sixt <j6t@kdbg.org>
>     Date:   Sun Jun 16 15:50:42 2013 +0200
>
>         rebase -p --first-parent: redo merge by cherry-picking first-parent change
>
> and it works like a charm.
>
> -- Sergey
>

> On Tue, Feb 6, 2018 at 10:16 PM, Sergey Organov <sorganov@gmail.com> wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>> [...]
>>
>>> +--recreate-merges::
>>> +     Recreate merge commits instead of flattening the history by replaying
>>> +     merges. Merge conflict resolutions or manual amendments to merge
>>> +     commits are not preserved.
>>
>> I wonder why you guys still hold on replaying "merge-the-operation"
>> instead of replaying "merge-the-result"? The latter, the merge commit
>> itself, no matter how exactly it was created in the first place, is the
>> most valuable thing git keeps about the merge, and you silently drop it
>> entirely! OTOH, git keeps almost no information about
>> "merge-the-operation", so it's virtually impossible to reliably replay
>> the operation automatically, and yet you try to.
>>
>
> I'm not sure I follow what you mean here?
>
> You mean that you'd want this to actually attempt to re-create the
> original merge including conflict resolutions by taking the contents
> of the result?

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> [...]
> 
>> +--recreate-merges::
>> +	Recreate merge commits instead of flattening the history by replaying
>> +	merges. Merge conflict resolutions or manual amendments to merge
>> +	commits are not preserved.
> 
> I wonder why you guys still hold on replaying "merge-the-operation"
> instead of replaying "merge-the-result"? The latter, the merge commit
> itself, no matter how exactly it was created in the first place, is the
> most valuable thing git keeps about the merge, and you silently drop it
> entirely! OTOH, git keeps almost no information about
> "merge-the-operation", so it's virtually impossible to reliably replay
> the operation automatically, and yet you try to.

> IMHO that was severe mistake in the original --preserve-merges, and you
> bring with you to this new --recreate-merges... It's sad. Even more sad
> as solution is already known for years:
> 
>      bc00341838a8faddcd101da9e746902994eef38a
>      Author: Johannes Sixt <j6t@kdbg.org>
>      Date:   Sun Jun 16 15:50:42 2013 +0200
>      
>          rebase -p --first-parent: redo merge by cherry-picking first-parent change
> 
> and it works like a charm.
> 
> -- Sergey

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> [...]
> 
> > +--recreate-merges::
> > +	Recreate merge commits instead of flattening the history by replaying
> > +	merges. Merge conflict resolutions or manual amendments to merge
> > +	commits are not preserved.
> 
> I wonder why you guys still hold on replaying "merge-the-operation"
> instead of replaying "merge-the-result"?

> Even more sad as solution is already known for years:
> 
>     bc00341838a8faddcd101da9e746902994eef38a
>     Author: Johannes Sixt <j6t@kdbg.org>
>     Date:   Sun Jun 16 15:50:42 2013 +0200
>     
>         rebase -p --first-parent: redo merge by cherry-picking first-parent change
> 
> and it works like a charm.

>
> Hi,
>
> [...]
>
> And guess what happens if you drop that `pick` line in your todo list

>> So no, I do not think that --recreate-merges --first-parent is a good
> idea
>> at all. Unless you try to do that non-interactively only, *and
> disallow it
>> in interactive mode*.

> Øyvind Rønningstad <ronningstad@gmail.com> writes:
> 
> >> So no, I do not think that --recreate-merges --first-parent is a good
> > idea
> >> at all. Unless you try to do that non-interactively only, *and
> > disallow it
> >> in interactive mode*.
> 
> Correct.  If the original side branch has commits A, B and C, you
> are rebuilding the topic to have only A and C but not B and then
> recreate the merge of that rebuilt topic, then you absolutely do not
> want "cherry-pick -m1" of the original merge when recreating the
> merge, as that would resurrect the effect of having B.  The same
> argument applies if you rebuilt the topic with A and C and then a
> new commit D.  "cherry-pick -m1" of the original would do a wrong
> thing.
> 
> When there is no such fixing up, "cherry-pick -m1" is the right
> thing to do, though, so it probably makes sense to pick merges that
> way when the side topic being merged consists of the same commits as
> the original.

> Just to give you one concrete example: when I recently rebased some
> patches (no reording or dropping involved here!) and one of the picks
> failed with merge conflicts, I realized that that particular commit
> introduced incorrect formatting and fixed that right away (verifying that
> no other commits introduced incorrect formatting, of course).
>
> With your new cute idea to magically cherry-pick -m1, this change would
> have been magically dropped from the subsequent merge commits!

> On Wed, 7 Feb 2018, Sergey Organov wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> > +--recreate-merges::
>> > +	Recreate merge commits instead of flattening the history by replaying
>> > +	merges. Merge conflict resolutions or manual amendments to merge
>> > +	commits are not preserved.
>> 
>> I wonder why you guys still hold on replaying "merge-the-operation"
>> instead of replaying "merge-the-result"?
>
> This misses the point of rebasing: you want to replay the changes.

>
>> The latter, the merge commit itself, no matter how exactly it was
>> created in the first place, is the most valuable thing git keeps about
>> the merge, and you silently drop it entirely!
>
> You miss another very crucial point.

> Let me explain the scenario which comes up plenty of times in my work with
> Git for Windows. We have a thicket of some 70 branches on top of git.git's
> latest release. These branches often include fixup! and squash! commits
> and even more complicated constructs that rebase cannot handle at all at
> the moment, such as reorder-before! and reorder-after! (for commits that
> really need to go into a different branch).

> Even if you do not have such a complicated setup, it is quite possible
> that you need to include a commit in your development that needs to be
> dropped before contributing your work. Think e.g. removing the `-O2` flag
> when compiling with GCC because GDB gets utterly confused with executables
> compiled with `-O2` while single-stepping. This could be an initial commit
> called `TO-DROP` or some such.
>
> And guess what happens if you drop that `pick` line in your todo list and
> then the `merge` command simply tries to re-create the original merge
> commit's changes?
>
> Exactly. The merge will become an evil merge, and will introduce that very
> much not-wanted and therefore-dropped changes.

>> OTOH, git keeps almost no information about "merge-the-operation", so
>> it's virtually impossible to reliably replay the operation
>> automatically, and yet you try to.
>
> That is true. However, the intended use case is not to allow you to
> recreate funny merges. Its use case is to allow you to recreate
> merges.

>
>> IMHO that was severe mistake in the original --preserve-merges, and you
>> bring with you to this new --recreate-merges... It's sad.
>
> Please refrain from drawing this discussion into an emotional direction.
> That is definitely not helpful.

>> Even more sad as solution is already known for years:
>> 
>>     bc00341838a8faddcd101da9e746902994eef38a
>>     Author: Johannes Sixt <j6t@kdbg.org>
>>     Date:   Sun Jun 16 15:50:42 2013 +0200
>>     
>>         rebase -p --first-parent: redo merge by cherry-picking first-parent change
>> 
>> and it works like a charm.
>
> It might work for you, as you probably used --preserve-merges, and dealt
> with the fact that you could neither drop nor reorder commits.
>
> So --preserve-merges --first-parent is probably what you were looking
> for.

> And I totally could see myself being vexed again about options that worked
> perfectly well (just like --preserve-merges) being completely messed up by
> allowing it to be combined with options *that they cannot work with* (just
> like --preserve-merges --interactive, a *huge* mistake causing so many
> annoying "bug" reports: I *never intended it that way because I knew it
> would not work as users expect*).

> So no, I do not think that --recreate-merges --first-parent is a good idea
> at all. Unless you try to do that non-interactively only, *and disallow it
> in interactive mode*. Because the entire point of the interactive rebase
> is to allow reordering and dropping commits, in --recreate-merges even
> moving, introducing and dropping merge commits. The --first-parent option
> flies in the face of this idea.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Let me explain the scenario which comes up plenty of times in my work with
>> Git for Windows. We have a thicket of some 70 branches on top of git.git's
>> latest release. These branches often include fixup! and squash! commits
>> and even more complicated constructs that rebase cannot handle at all at
>> the moment, such as reorder-before! and reorder-after! (for commits that
>> really need to go into a different branch).
> 
> I sympathize, but a solution that breaks even in simple cases can't be
> used reliably to solve more complex problems, sorry. Being so deep
> into your problems, I think you maybe just aren't seeing forest for the
> trees [1].

> Am 09.02.2018 um 07:11 schrieb Sergey Organov:
>>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>>
>>> Let me explain the scenario which comes up plenty of times in my work
>>> with
>>> Git for Windows. We have a thicket of some 70 branches on top of
>>> git.git's
>>> latest release. These branches often include fixup! and squash! commits
>>> and even more complicated constructs that rebase cannot handle at all at
>>> the moment, such as reorder-before! and reorder-after! (for commits that
>>> really need to go into a different branch).
>>
>>
>> I sympathize, but a solution that breaks even in simple cases can't be
>> used reliably to solve more complex problems, sorry. Being so deep
>> into your problems, I think you maybe just aren't seeing forest for the
>> trees [1].
>
>
> Hold your horses! Dscho has a point here. --preserve-merges --first-parent
> works only as long as you don't tamper with the side branches. If you make
> changes in the side branches during the same rebase operation, this
> --first-parent mode would undo that change. (And, yes, its result would be
> called an "evil merge", and that scary name _should_ frighten you!)
>
> -- Hannes

> Am 09.02.2018 um 07:11 schrieb Sergey Organov:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>> Let me explain the scenario which comes up plenty of times in my work with
>>> Git for Windows. We have a thicket of some 70 branches on top of git.git's
>>> latest release. These branches often include fixup! and squash! commits
>>> and even more complicated constructs that rebase cannot handle at all at
>>> the moment, such as reorder-before! and reorder-after! (for commits that
>>> really need to go into a different branch).
>>
>> I sympathize, but a solution that breaks even in simple cases can't be
>> used reliably to solve more complex problems, sorry. Being so deep
>> into your problems, I think you maybe just aren't seeing forest for the
>> trees [1].
>
> Hold your horses! Dscho has a point here. --preserve-merges
> --first-parent works only as long as you don't tamper with the side
> branches. If you make changes in the side branches during the same
> rebase operation, this --first-parent mode would undo that change.

> With this patch, the goodness of the Git garden shears comes to `git
> rebase -i` itself. Passing the `--recreate-merges` option will generate
> a todo list that can be understood readily, and where it is obvious
> how to reorder commits. New branches can be introduced by inserting
> `label` commands and calling `merge - <label> <oneline>`. And once this
> mode has become stable and universally accepted, we can deprecate the
> design mistake that was `--preserve-merges`.

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> [...]
> 
> > With this patch, the goodness of the Git garden shears comes to `git
> > rebase -i` itself. Passing the `--recreate-merges` option will generate
> > a todo list that can be understood readily, and where it is obvious
> > how to reorder commits. New branches can be introduced by inserting
> > `label` commands and calling `merge - <label> <oneline>`. And once this
> > mode has become stable and universally accepted, we can deprecate the
> > design mistake that was `--preserve-merges`.
> 
> This doesn't explain why you introduced this new --recreate-merges. Why
> didn't you rather fix --preserve-merges to generate and use new todo
> list format?

> Then, if new option indeed required, please look at the resulting manual:
> 
> --recreate-merges::
> 	Recreate merge commits instead of flattening the history by replaying
> 	merges. Merge conflict resolutions or manual amendments to merge
> 	commits are not preserved.
> 
> -p::
> --preserve-merges::
> 	Recreate merge commits instead of flattening the history by replaying
> 	commits a merge commit introduces. Merge conflict resolutions or manual
> 	amendments to merge commits are not preserved.

> Don't you think more explanations are needed there in the manual on
> why do we have 2 separate options with almost the same yet subtly
> different description? Is this subtle difference even important? How?
> 
> I also have trouble making sense of "Recreate merge commits instead of
> flattening the history by replaying merges." Is it "<Recreate merge
> commits by replaying merges> instead of <flattening the history>" or is it
> rather "<Recreate merge commits> instead of <flattening the history by
> replaying merges>?

>> I also have trouble making sense of "Recreate merge commits instead of
>> flattening the history by replaying merges." Is it "<Recreate merge
>> commits by replaying merges> instead of <flattening the history>" or is it
>> rather "<Recreate merge commits> instead of <flattening the history by
>> replaying merges>?

> Thanks for explanations, and could you please answer this one:
> 
> [...]
> 
> >> I also have trouble making sense of "Recreate merge commits instead of
> >> flattening the history by replaying merges." Is it "<Recreate merge
> >> commits by replaying merges> instead of <flattening the history>" or is it
> >> rather "<Recreate merge commits> instead of <flattening the history by
> >> replaying merges>?

> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>
>> Thanks for explanations, and could you please answer this one:
>> 
>> [...]
>> 
>> >> I also have trouble making sense of "Recreate merge commits instead of
>> >> flattening the history by replaying merges." Is it "<Recreate merge
>> >> commits by replaying merges> instead of <flattening the history>" or is it
>> >> rather "<Recreate merge commits> instead of <flattening the history by
>> >> replaying merges>?
>
> I thought I had answered that one.

>
> Flattening the history is what happens in regular rebase (i.e. without
> --recreate-merges and without --preserve-merges).
>
> The idea to recreate merges is of course to *not* flatten the history.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The wording is poor either way, but you are also not a native speaker so
> > we have to rely on, say, Eric to help us out here.
> 
> Likely, but why didn't you keep original wording from --preserve-merges?
> Do you feel it's somehow poor either?

> Hi,
>
> On Tue, 13 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > The wording is poor either way, but you are also not a native speaker so
>> > we have to rely on, say, Eric to help us out here.
>> 
>> Likely, but why didn't you keep original wording from --preserve-merges?
>> Do you feel it's somehow poor either?
>
> Yes, I felt it is poor, especially when --recreate-merges is present, that
> is indeed why I changed it.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Tue, 13 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >> > The wording is poor either way, but you are also not a native speaker so
> >> > we have to rely on, say, Eric to help us out here.
> >> 
> >> Likely, but why didn't you keep original wording from --preserve-merges?
> >> Do you feel it's somehow poor either?
> >
> > Yes, I felt it is poor, especially when --recreate-merges is present, that
> > is indeed why I changed it.
> 
> So, how about this (yeah, I noticed the option now got arguments, but
> please, tweak this to the new implementation yourself):
> 
> --recreate-merges::
> 	Recreate merge commits instead of flattening the history. Merge
> 	conflict resolutions or manual amendments to merge commits are
> 	not preserved. 
> 
> -p::
> --preserve-merges::
> 	(deprecated) This option is similar to --recreate-merges. It has
>         no proper support for interactive mode and thus is deprecated.
>         Use '--recreate-merges' instead.

> Hi Sergey,
>
> On Fri, 9 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> [...]
>> 
>> > With this patch, the goodness of the Git garden shears comes to `git
>> > rebase -i` itself. Passing the `--recreate-merges` option will generate
>> > a todo list that can be understood readily, and where it is obvious
>> > how to reorder commits. New branches can be introduced by inserting
>> > `label` commands and calling `merge - <label> <oneline>`. And once this
>> > mode has become stable and universally accepted, we can deprecate the
>> > design mistake that was `--preserve-merges`.
>> 
>> This doesn't explain why you introduced this new --recreate-merges. Why
>> didn't you rather fix --preserve-merges to generate and use new todo
>> list format?
>
> Because that would of course break existing users of
> --preserve-merges.

> So why not --preserve-merges=v2? Because that would force me to maintain
> --preserve-merges forever. And I don't want to.
>
>> It doesn't seem likely that todo list created by one Git version is to
>> be ever used by another, right?
>
> No. But by scripts based on `git rebase -p`.
>
>> Is there some hidden reason here? Some tools outside of Git that use old
>> todo list format, maybe?
>
> Exactly.
>
> I did mention such a tool: the Git garden shears:
>
> 	https://github.com/git-for-windows/build-extra/blob/master/shears.sh
>
> Have a look at it. It will inform the discussion.

>
>> Then, if new option indeed required, please look at the resulting manual:
>> 
>> --recreate-merges::
>> 	Recreate merge commits instead of flattening the history by replaying
>> 	merges. Merge conflict resolutions or manual amendments to merge
>> 	commits are not preserved.
>> 
>> -p::
>> --preserve-merges::
>> 	Recreate merge commits instead of flattening the history by replaying
>> 	commits a merge commit introduces. Merge conflict resolutions or manual
>> 	amendments to merge commits are not preserved.
>
> As I stated in the cover letter, there are more patches lined up after
> this patch series.

>
> Also, please refrain from saying things like... "Don't you think ..."
>
> If you don't like the wording, I wold much more appreciate it if a better
> alternative was suggested.

>
>> Don't you think more explanations are needed there in the manual on
>> why do we have 2 separate options with almost the same yet subtly
>> different description? Is this subtle difference even important? How?
>> 
>> I also have trouble making sense of "Recreate merge commits instead of
>> flattening the history by replaying merges." Is it "<Recreate merge
>> commits by replaying merges> instead of <flattening the history>" or is it
>> rather "<Recreate merge commits> instead of <flattening the history by
>> replaying merges>?
>
> The documentation of the --recreate-merges option is not meant to explain
> the difference to --preserve-merges. It is meant to explain the difference
> to regular `git rebase -i`, which flattens the commit history into a
> single branch without merge commits (in fact, all merge commits are simply
> ignored).

> And I would rather not start to describe the difference between
> --recreate-merges and --preserve-merges because I want to deprecate the
> latter, and describing the difference as I get the sense is your wish
> would simply mean more work because it would have to be added and then
> removed again.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > On Fri, 9 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >> 
> >> [...]
> >> 
> >> > With this patch, the goodness of the Git garden shears comes to `git
> >> > rebase -i` itself. Passing the `--recreate-merges` option will generate
> >> > a todo list that can be understood readily, and where it is obvious
> >> > how to reorder commits. New branches can be introduced by inserting
> >> > `label` commands and calling `merge - <label> <oneline>`. And once this
> >> > mode has become stable and universally accepted, we can deprecate the
> >> > design mistake that was `--preserve-merges`.
> >> 
> >> This doesn't explain why you introduced this new --recreate-merges. Why
> >> didn't you rather fix --preserve-merges to generate and use new todo
> >> list format?
> >
> > Because that would of course break existing users of
> > --preserve-merges.
> 
> How exactly?

> > So why not --preserve-merges=v2? Because that would force me to
> > maintain --preserve-merges forever. And I don't want to.
> >
> >> It doesn't seem likely that todo list created by one Git version is
> >> to be ever used by another, right?
> >
> > No. But by scripts based on `git rebase -p`.
> >
> >> Is there some hidden reason here? Some tools outside of Git that use
> >> old todo list format, maybe?
> >
> > Exactly.
> >
> > I did mention such a tool: the Git garden shears:
> >
> > 	https://github.com/git-for-windows/build-extra/blob/master/shears.sh
> >
> > Have a look at it. It will inform the discussion.
> 
> I've searched for "-p" in the script, but didn't find positives for
> either "-p" or "--preserve-merges". How it would break if it doesn't use
> them? What am I missing?

> >> Then, if new option indeed required, please look at the resulting manual:
> >> 
> >> --recreate-merges::
> >> 	Recreate merge commits instead of flattening the history by replaying
> >> 	merges. Merge conflict resolutions or manual amendments to merge
> >> 	commits are not preserved.
> >> 
> >> -p::
> >> --preserve-merges::
> >> 	Recreate merge commits instead of flattening the history by replaying
> >> 	commits a merge commit introduces. Merge conflict resolutions or manual
> >> 	amendments to merge commits are not preserved.
> >
> > As I stated in the cover letter, there are more patches lined up after
> > this patch series.
> 
> Good, but I thought this one should better be self-consistent anyway.
> What if those that come later aren't included?

> > Have a look at https://github.com/git/git/pull/447, especially the
> > latest commit in there which is an early version of the deprecation I
> > intend to bring about.
> 
> You shouldn't want a deprecation at all should you have re-used
> --preserve-merges in the first place, and I still don't see why you
> haven't. 

> > Also, please refrain from saying things like... "Don't you think ..."
> >
> > If you don't like the wording, I wold much more appreciate it if a better
> > alternative was suggested.
> 
> Sorry, but how can I suggest one if I don't understand what you are
> doing here in the first place? That's why I ask you.

> >> Don't you think more explanations are needed there in the manual on
> >> why do we have 2 separate options with almost the same yet subtly
> >> different description? Is this subtle difference even important? How?
> >> 
> >> I also have trouble making sense of "Recreate merge commits instead of
> >> flattening the history by replaying merges." Is it "<Recreate merge
> >> commits by replaying merges> instead of <flattening the history>" or is it
> >> rather "<Recreate merge commits> instead of <flattening the history by
> >> replaying merges>?
> >
> > The documentation of the --recreate-merges option is not meant to explain
> > the difference to --preserve-merges. It is meant to explain the difference
> > to regular `git rebase -i`, which flattens the commit history into a
> > single branch without merge commits (in fact, all merge commits are simply
> > ignored).
> 
> Yeah, that's obvious, but the point is that resulting manual is ended
> up being confusing.

> > And I would rather not start to describe the difference between
> > --recreate-merges and --preserve-merges because I want to deprecate the
> > latter, and describing the difference as I get the sense is your wish
> > would simply mean more work because it would have to be added and then
> > removed again.
> 
> I suspect you actually didn't need those new option in the first place,
> and that's the core reason of these troubles.

> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>> > Have a look at https://github.com/git/git/pull/447, especially the
>> > latest commit in there which is an early version of the deprecation I
>> > intend to bring about.
>>
>> You shouldn't want a deprecation at all should you have re-used
>> --preserve-merges in the first place, and I still don't see why you
>> haven't.
>
> Keep repeating it, and it won't become truer.
>
> If you break formats, you break scripts. Git has *so* many users, there
> are very likely some who script *every* part of it.
>
> We simply cannot do that.
>
> What we can is deprecate designs which we learned on the way were not only
> incomplete from the get-go, but bad overall and hard (or impossible) to
> fix. Like --preserve-merges.
>
> Or for that matter like the design you proposed, to use --first-parent for
> --recreate-merges. Or to use --first-parent for some --recreate-merges,
> surprising users in very bad ways when it is not used (or when it is
> used). I get the impression that you still think it would be a good idea,
> even if it should be obvious that it is not.

> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Sergey,
>>
>> On Mon, 12 Feb 2018, Sergey Organov wrote:
>>> > Have a look at https://github.com/git/git/pull/447, especially the
>>> > latest commit in there which is an early version of the deprecation I
>>> > intend to bring about.
>>>
>>> You shouldn't want a deprecation at all should you have re-used
>>> --preserve-merges in the first place, and I still don't see why you
>>> haven't.
>>
>> Keep repeating it, and it won't become truer.
>>
>> If you break formats, you break scripts. Git has *so* many users, there
>> are very likely some who script *every* part of it.
>>
>> We simply cannot do that.
>>
>> What we can is deprecate designs which we learned on the way were not only
>> incomplete from the get-go, but bad overall and hard (or impossible) to
>> fix. Like --preserve-merges.
>>
>> Or for that matter like the design you proposed, to use --first-parent for
>> --recreate-merges. Or to use --first-parent for some --recreate-merges,
>> surprising users in very bad ways when it is not used (or when it is
>> used). I get the impression that you still think it would be a good idea,
>> even if it should be obvious that it is not.
>
> If we consider the addition of new todo list elements as "user
> breaking", then yes this change would be user-script breaking.

> One could argue that users should have expected new todo list elements
> to be added in the future and thus design their scripts to cope with
> such a thing. If you can convincingly argue this, then I don't
> necessarily see it as a complete user breaking change to fix
> preserve-merges in order to allow it to handle re-ordering properly..

> Hi Jake,
>
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> Hi Sergey,
>>>
>>> On Mon, 12 Feb 2018, Sergey Organov wrote:
>>>> > Have a look at https://github.com/git/git/pull/447, especially the
>>>> > latest commit in there which is an early version of the deprecation I
>>>> > intend to bring about.
>>>>
>>>> You shouldn't want a deprecation at all should you have re-used
>>>> --preserve-merges in the first place, and I still don't see why you
>>>> haven't.
>>>
>>> Keep repeating it, and it won't become truer.
>>>
>>> If you break formats, you break scripts. Git has *so* many users, there
>>> are very likely some who script *every* part of it.
>>>
>>> We simply cannot do that.
>>>
>>> What we can is deprecate designs which we learned on the way were not only
>>> incomplete from the get-go, but bad overall and hard (or impossible) to
>>> fix. Like --preserve-merges.
>>>
>>> Or for that matter like the design you proposed, to use --first-parent for
>>> --recreate-merges. Or to use --first-parent for some --recreate-merges,
>>> surprising users in very bad ways when it is not used (or when it is
>>> used). I get the impression that you still think it would be a good idea,
>>> even if it should be obvious that it is not.
>>
>> If we consider the addition of new todo list elements as "user
>> breaking", then yes this change would be user-script breaking.
>
> It _is_ user script breaking, provided such script exists. Has anybody
> actually seen one? Not that it's wrong to be extra-cautious about it,
> just curios. Note that to be actually affected, such a script must
> invoke "git rebase -p" _command_ and then tweak its todo output to
> produce outcome.
>
>> Since we did not originally spell out that todo-list items are subject
>> to enhancement by addition of operations in the future, scripts are
>> likely not designed to allow addition of new elements.
>
> Out of curiosity, are you going to spell it now, for the new todo
> format?
>
>> Thus, adding recreate-merges, and deprecating preserve-merges, seems
>> to me to be the correct action to take here.
>
> Yes, sure, provided there is actual breakage, or at least informed
> suspicion there is one.
>
>> One could argue that users should have expected new todo list elements
>> to be added in the future and thus design their scripts to cope with
>> such a thing. If you can convincingly argue this, then I don't
>> necessarily see it as a complete user breaking change to fix
>> preserve-merges in order to allow it to handle re-ordering properly..
>
> I'd not argue this way myself. If there are out-of-git-tree non-human
> users that accept and tweak todo _generated_ by current "git rebase -p"
> _command_, I also vote for a new option.
>

>> I think I lean towards agreeing with Johannes, and that adding
>> recreate-merges and removing preserve-merges is the better solution.
>
> On these grounds it is, no objections.
>
> -- Sergey

> On Mon, Feb 12, 2018 at 11:15 PM, Sergey Organov <sorganov@gmail.com> wrote:
> >
> > Jacob Keller <jacob.keller@gmail.com> writes:
> >
> >> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >>>
> >>> On Mon, 12 Feb 2018, Sergey Organov wrote:
> >>>> > Have a look at https://github.com/git/git/pull/447, especially the
> >>>> > latest commit in there which is an early version of the deprecation I
> >>>> > intend to bring about.
> >>>>
> >>>> You shouldn't want a deprecation at all should you have re-used
> >>>> --preserve-merges in the first place, and I still don't see why you
> >>>> haven't.
> >>>
> >>> Keep repeating it, and it won't become truer.
> >>>
> >>> If you break formats, you break scripts. Git has *so* many users, there
> >>> are very likely some who script *every* part of it.
> >>>
> >>> We simply cannot do that.
> >>>
> >>> What we can is deprecate designs which we learned on the way were not only
> >>> incomplete from the get-go, but bad overall and hard (or impossible) to
> >>> fix. Like --preserve-merges.
> >>>
> >>> Or for that matter like the design you proposed, to use --first-parent for
> >>> --recreate-merges. Or to use --first-parent for some --recreate-merges,
> >>> surprising users in very bad ways when it is not used (or when it is
> >>> used). I get the impression that you still think it would be a good idea,
> >>> even if it should be obvious that it is not.
> >>
> >> If we consider the addition of new todo list elements as "user
> >> breaking", then yes this change would be user-script breaking.
> >
> > It _is_ user script breaking, provided such script exists. Has anybody
> > actually seen one? Not that it's wrong to be extra-cautious about it,
> > just curios. Note that to be actually affected, such a script must
> > invoke "git rebase -p" _command_ and then tweak its todo output to
> > produce outcome.
> >
> >> Since we did not originally spell out that todo-list items are subject
> >> to enhancement by addition of operations in the future, scripts are
> >> likely not designed to allow addition of new elements.
> >
> > Out of curiosity, are you going to spell it now, for the new todo
> > format?
> >
> >> Thus, adding recreate-merges, and deprecating preserve-merges, seems
> >> to me to be the correct action to take here.
> >
> > Yes, sure, provided there is actual breakage, or at least informed
> > suspicion there is one.
> >
> >> One could argue that users should have expected new todo list elements
> >> to be added in the future and thus design their scripts to cope with
> >> such a thing. If you can convincingly argue this, then I don't
> >> necessarily see it as a complete user breaking change to fix
> >> preserve-merges in order to allow it to handle re-ordering properly..
> >
> > I'd not argue this way myself. If there are out-of-git-tree non-human
> > users that accept and tweak todo _generated_ by current "git rebase -p"
> > _command_, I also vote for a new option.
> >
> 
> To be fair, I have not seen anything that actually reads the todo list
> and tweaks it in such a manner. The closest example is the git garden
> shears script, which simply replaces the todo list.
> 
> It's certainly *possible* that such a script would exist though,

>> > I'd not argue this way myself. If there are out-of-git-tree non-human
>> > users that accept and tweak todo _generated_ by current "git rebase -p"
>> > _command_, I also vote for a new option.
>> >
>> 
>> To be fair, I have not seen anything that actually reads the todo list
>> and tweaks it in such a manner. The closest example is the git garden
>> shears script, which simply replaces the todo list.
>> 
>> It's certainly *possible* that such a script would exist though,
>
> We actually know of such scripts.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> [...]
> 
> >> > I'd not argue this way myself. If there are out-of-git-tree non-human
> >> > users that accept and tweak todo _generated_ by current "git rebase -p"
> >> > _command_, I also vote for a new option.
> >> >
> >> 
> >> To be fair, I have not seen anything that actually reads the todo list
> >> and tweaks it in such a manner. The closest example is the git garden
> >> shears script, which simply replaces the todo list.
> >> 
> >> It's certainly *possible* that such a script would exist though,
> >
> > We actually know of such scripts.
> 
> Please consider to explain this in the description of the change. I
> believe readers deserve an explanation of why you decided to invent new
> option instead of fixing the old one, even if it were only a suspicion,
> more so if it is confidence.

> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> >
>> > On Fri, 9 Feb 2018, Sergey Organov wrote:
>> >
>> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> >> 
>> >> [...]
>> >> 
>> >> > With this patch, the goodness of the Git garden shears comes to `git
>> >> > rebase -i` itself. Passing the `--recreate-merges` option will generate
>> >> > a todo list that can be understood readily, and where it is obvious
>> >> > how to reorder commits. New branches can be introduced by inserting
>> >> > `label` commands and calling `merge - <label> <oneline>`. And once this
>> >> > mode has become stable and universally accepted, we can deprecate the
>> >> > design mistake that was `--preserve-merges`.
>> >> 
>> >> This doesn't explain why you introduced this new --recreate-merges. Why
>> >> didn't you rather fix --preserve-merges to generate and use new todo
>> >> list format?
>> >
>> > Because that would of course break existing users of
>> > --preserve-merges.
>> 
>> How exactly?
>
> Power users of interactive rebase use scripting to augment Git's
> functionality. One particularly powerful trick is to override
> GIT_SEQUENCER_EDITOR with an invocation of such a script, to perform
> automated edits. Such a script breaks when we change the format of the
> content to edit. If we change the format of the todo list generated in
> --preserve-merges mode, that is exactly what happens. We break existing
> users.

> They do override the sequencer editor, and while they do not exactly
> edit the todo list (they simply through the generated one away), they
> generate a new todo list and would break if that format changes. Of
> course, the shears do not use the --preserve-merges mode,
> but from just reading about the way how the Git garden shears work, it
> is quite obvious how similar users of --preserve-merges are likely to
> exist?

>> Doesn't "--recreate-merges" produce the same result as
>> "--preserve-merges" if run non-interactively?
>
> The final result of a rebase where you do not edit the todo list? Should
> be identical, indeed.

>> > So why not --preserve-merges=v2? Because that would force me to
>> > maintain --preserve-merges forever. And I don't want to.
>> >
>> >> It doesn't seem likely that todo list created by one Git version is
>> >> to be ever used by another, right?
>> >
>> > No. But by scripts based on `git rebase -p`.
>> >
>> >> Is there some hidden reason here? Some tools outside of Git that use
>> >> old todo list format, maybe?
>> >
>> > Exactly.
>> >
>> > I did mention such a tool: the Git garden shears:
>> >
>> > 	https://github.com/git-for-windows/build-extra/blob/master/shears.sh
>> >
>> > Have a look at it. It will inform the discussion.
>> 
>> I've searched for "-p" in the script, but didn't find positives for
>> either "-p" or "--preserve-merges". How it would break if it doesn't use
>> them? What am I missing?
>
> *This* particular script does not use -p.
>
> But it is not *this* particular script that I do not want to break!

>> >> Then, if new option indeed required, please look at the resulting manual:
>> >> 
>> >> --recreate-merges::
>> >> 	Recreate merge commits instead of flattening the history by replaying
>> >> 	merges. Merge conflict resolutions or manual amendments to merge
>> >> 	commits are not preserved.
>> >> 
>> >> -p::
>> >> --preserve-merges::
>> >> 	Recreate merge commits instead of flattening the history by replaying
>> >> 	commits a merge commit introduces. Merge conflict resolutions or manual
>> >> 	amendments to merge commits are not preserved.
>> >
>> > As I stated in the cover letter, there are more patches lined up after
>> > this patch series.
>> 
>> Good, but I thought this one should better be self-consistent anyway.
>> What if those that come later aren't included?
>
> Right, let's just rip apart the partial progress because the latter
> patches might not make it in?

>
> I cannot work on that basis, and I also do not want to work on that basis.
>
> If you do not like how the documentation is worded, fine, suggest a better
> alternative.

>
>> > Have a look at https://github.com/git/git/pull/447, especially the
>> > latest commit in there which is an early version of the deprecation I
>> > intend to bring about.
>> 
>> You shouldn't want a deprecation at all should you have re-used
>> --preserve-merges in the first place, and I still don't see why you
>> haven't. 
>
> Keep repeating it, and it won't become truer.

> Or for that matter like the design you proposed, to use --first-parent for
> --recreate-merges. Or to use --first-parent for some --recreate-merges,
> surprising users in very bad ways when it is not used (or when it is
> used). I get the impression that you still think it would be a good idea,
> even if it should be obvious that it is not.

>> > Also, please refrain from saying things like... "Don't you think ..."
>> >
>> > If you don't like the wording, I wold much more appreciate it if a better
>> > alternative was suggested.
>> 
>> Sorry, but how can I suggest one if I don't understand what you are
>> doing here in the first place? That's why I ask you.
>
> There are ways to put the person you ask on trial. And there are ways to
> genuinely show interest and seek education.

> I am a really poor example how to communicate properly, of course, so
> don't try to learn from me. I am trying myself to learn better ways to
> express what I mean clearly, and to express it in a direct yet kind
> manner.
>
>> >> Don't you think more explanations are needed there in the manual on
>> >> why do we have 2 separate options with almost the same yet subtly
>> >> different description? Is this subtle difference even important? How?
>> >> 
>> >> I also have trouble making sense of "Recreate merge commits instead of
>> >> flattening the history by replaying merges." Is it "<Recreate merge
>> >> commits by replaying merges> instead of <flattening the history>" or is it
>> >> rather "<Recreate merge commits> instead of <flattening the history by
>> >> replaying merges>?
>> >
>> > The documentation of the --recreate-merges option is not meant to explain
>> > the difference to --preserve-merges. It is meant to explain the difference
>> > to regular `git rebase -i`, which flattens the commit history into a
>> > single branch without merge commits (in fact, all merge commits are simply
>> > ignored).
>> 
>> Yeah, that's obvious, but the point is that resulting manual is ended
>> up being confusing.
>
> Again, just saying something is bad, is bad. Saying something leaves room
> for improvement and then suggesting how to improve it, is good.

>> > And I would rather not start to describe the difference between
>> > --recreate-merges and --preserve-merges because I want to deprecate the
>> > latter, and describing the difference as I get the sense is your wish
>> > would simply mean more work because it would have to be added and then
>> > removed again.
>> 
>> I suspect you actually didn't need those new option in the first place,
>> and that's the core reason of these troubles.
>
> Are you suspecting that I, myself, do not use --recreate-merges?

> If so, please read the cover letter again, in particular the part where I
> describe how this entire series of patch series arose from the Git garden
> shears, which I invented myself to help with maintaining Git for Windows,
> and which I use for five years now. This should help disperse that
> suspicion rather quickly: the intent of --recreate-merges is to allow me
> to simplify the shears by quite a bit, and maybe eventually even get rid
> of the script altogether (if I ever manage to convince myself that the
> concept of a merging-rebase should be official enough to enter core Git).
>
> I am a heavy user of --recreate-merges, even if it does not really exist
> yet. I have five years of experience with it, which is the reason why I am
> so confident about its design, and why I can tell you a lot about typical
> use cases and common pitfalls, and where the original design had to be
> adjusted.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > On Mon, 12 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> >
> >> > On Fri, 9 Feb 2018, Sergey Organov wrote:
> >> >
> >> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >> >> 
> >> >> [...]
> >> >> 
> >> >> > With this patch, the goodness of the Git garden shears comes to `git
> >> >> > rebase -i` itself. Passing the `--recreate-merges` option will generate
> >> >> > a todo list that can be understood readily, and where it is obvious
> >> >> > how to reorder commits. New branches can be introduced by inserting
> >> >> > `label` commands and calling `merge - <label> <oneline>`. And once this
> >> >> > mode has become stable and universally accepted, we can deprecate the
> >> >> > design mistake that was `--preserve-merges`.
> >> >> 
> >> >> This doesn't explain why you introduced this new --recreate-merges. Why
> >> >> didn't you rather fix --preserve-merges to generate and use new todo
> >> >> list format?
> >> >
> >> > Because that would of course break existing users of
> >> > --preserve-merges.
> >> 
> >> How exactly?
> >
> > Power users of interactive rebase use scripting to augment Git's
> > functionality. One particularly powerful trick is to override
> > GIT_SEQUENCER_EDITOR with an invocation of such a script, to perform
> > automated edits. Such a script breaks when we change the format of the
> > content to edit. If we change the format of the todo list generated in
> > --preserve-merges mode, that is exactly what happens. We break existing
> > users.
> 
> I didn't say a word against "--preserve-merges mode", whatever it is,
> only about re-using "--preserve-merges" command-line option to "git
> rebase", the git user interface.

> Unless there are out-of-git scripts that do use "git rebase
> --preserve-merges" and simultaneously do rely on the todo list format
> this exact command generates, there should be no breakage of existing
> users caused by changing todo list format generated by  "git rebase
> --preserve-merges".

> > BTW it seems that you did not really read my previous reply carefully
> > because I referenced such a use case: the Git garden shears.
> 
> I thought I did. You confirm below that this script doesn't use "git
> rebase --preserve-merges" in the first place, nor will it break if "git
> rebase --preserve-merges" starts to generate new todo format, yet you
> expected I'd readily see how it's relevant? No, I'm not that clever, nor
> am I a mind-reader.

> > They do override the sequencer editor, and while they do not exactly
> > edit the todo list (they simply through the generated one away), they
> > generate a new todo list and would break if that format changes. Of
> > course, the shears do not use the --preserve-merges mode, but from
> > just reading about the way how the Git garden shears work, it is quite
> > obvious how similar users of --preserve-merges are likely to exist?
> 
> Maybe, I dunno. If even "garden shears" won't break, then what will? Do
> you know an example?

> >> Doesn't "--recreate-merges" produce the same result as
> >> "--preserve-merges" if run non-interactively?
> >
> > The final result of a rebase where you do not edit the todo list? Should
> > be identical, indeed.
> 
> That's good to hear.
> 
> > But that is the most boring, most uninteresting, and least important use
> > case.
> 
> For you. Do you suddenly stop caring about compatibility?

> > It is *all* scripts that use interactive rebase!
> 
> I'm really interested, and here I *do* ask for education. What are
> those? As I now only ask this out of curiosity, and don't argue
> --recreate-merges anymore, are you finally willing to reveal the
> information?

> > Don't you also care about not breaking existing users?
> 
> I do care. I just suspected they are very unlikely to exist, and I do
> want to be educated in this matter indeed, as they could be rather
> interesting.

> In the previous patches, we implemented the basic functionality of the
> `git rebase -i --recreate-merges` command, in particular the `merge`
> command to create merge commits in the sequencer.
>
> The interactive rebase is a lot more these days, though, than a simple
> cherry-pick in a loop. For example, it calls the post-rewrite hook (if
> any) after rebasing with a mapping of the old->new commits. And the
> interactive rebase also supports the autosquash mode, where commits
> whose oneline is of the form `fixup! <oneline>` or `squash! <oneline>`
> are rearranged to amend commits whose oneline they match.
>
> This patch implements the post-rewrite and autosquash handling for the
> `merge` command we just introduced. The other commands that were added
> recently (`label`, `reset` and `bud`) do not create new commits,
> therefore post-rewrite & autosquash do not need to handle them.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  refs.c                            |  3 ++-
>  sequencer.c                       | 10 +++++++---
>  t/t3430-rebase-recreate-merges.sh | 39 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 20ba82b4343..e8b84c189ff 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -600,7 +600,8 @@ int dwim_log(const char *str, int len, struct object_id *oid, char **log)
>  static int is_per_worktree_ref(const char *refname)
>  {
>         return !strcmp(refname, "HEAD") ||
> -               starts_with(refname, "refs/bisect/");
> +               starts_with(refname, "refs/bisect/") ||
> +               starts_with(refname, "refs/rewritten/");
>  }

>
>  static int is_pseudoref_syntax(const char *refname)
> diff --git a/sequencer.c b/sequencer.c
> index 1bef16647b4..b63bfb9a141 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2413,10 +2413,13 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>                         res = do_reset(item->arg, item->arg_len);
>                 else if (item->command == TODO_BUD)
>                         res = do_reset("onto", 4);
> -               else if (item->command == TODO_MERGE)
> +               else if (item->command == TODO_MERGE) {
>                         res = do_merge(item->commit,
>                                        item->arg, item->arg_len, opts);
> -               else if (!is_noop(item->command))
> +                       if (item->commit)
> +                               record_in_rewritten(&item->commit->object.oid,
> +                                                   peek_command(todo_list, 1));
> +               } else if (!is_noop(item->command))
>                         return error(_("unknown command %d"), item->command);
>
>                 todo_list->current++;
> @@ -3556,7 +3559,8 @@ int rearrange_squash(void)
>                 struct subject2item_entry *entry;
>
>                 next[i] = tail[i] = -1;
> -               if (item->command >= TODO_EXEC) {
> +               if (item->command >= TODO_EXEC &&
> +                   (item->command != TODO_MERGE || !item->commit)) {
>                         subjects[i] = NULL;
>                         continue;
>                 }
> diff --git a/t/t3430-rebase-recreate-merges.sh b/t/t3430-rebase-recreate-merges.sh
> index 46ae52f88b3..76e615bd7c1 100755
> --- a/t/t3430-rebase-recreate-merges.sh
> +++ b/t/t3430-rebase-recreate-merges.sh
> @@ -143,4 +143,43 @@ test_expect_success 'with a branch tip that was cherry-picked already' '
>         EOF
>  '
>
> +test_expect_success 'refs/rewritten/* is worktree-local' '
> +       git worktree add wt &&
> +       cat >wt/script-from-scratch <<-\EOF &&
> +       label xyz
> +       exec GIT_DIR=../.git git rev-parse --verify refs/rewritten/xyz >a || :
> +       exec git rev-parse --verify refs/rewritten/xyz >b
> +       EOF
> +
> +       test_config -C wt sequence.editor \""$PWD"/replace-editor.sh\" &&
> +       git -C wt rebase -i HEAD &&
> +       test_must_be_empty wt/a &&
> +       test_cmp_rev HEAD "$(cat wt/b)"
> +'
> +

> +test_expect_success 'post-rewrite hook and fixups work for merges' '
> +       git checkout -b post-rewrite &&
> +       test_commit same1 &&
> +       git reset --hard HEAD^ &&
> +       test_commit same2 &&
> +       git merge -m "to fix up" same1 &&
> +       echo same old same old >same2.t &&
> +       test_tick &&
> +       git commit --fixup HEAD same2.t &&
> +       fixup="$(git rev-parse HEAD)" &&
> +
> +       mkdir -p .git/hooks &&
> +       test_when_finished "rm .git/hooks/post-rewrite" &&
> +       echo "cat >actual" | write_script .git/hooks/post-rewrite &&
> +
> +       test_tick &&
> +       git rebase -i --autosquash --recreate-merges HEAD^^^ &&
> +       printf "%s %s\n%s %s\n%s %s\n%s %s\n" >expect $(git rev-parse \
> +               $fixup^^2 HEAD^2 \
> +               $fixup^^ HEAD^ \
> +               $fixup^ HEAD \
> +               $fixup HEAD) &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.15.1.windows.2.1430.ga56c4f9e2a9
>
>

> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > In the previous patches, we implemented the basic functionality of the
> > `git rebase -i --recreate-merges` command, in particular the `merge`
> > command to create merge commits in the sequencer.
> >
> > The interactive rebase is a lot more these days, though, than a simple
> > cherry-pick in a loop. For example, it calls the post-rewrite hook (if
> > any) after rebasing with a mapping of the old->new commits. And the
> > interactive rebase also supports the autosquash mode, where commits
> > whose oneline is of the form `fixup! <oneline>` or `squash! <oneline>`
> > are rearranged to amend commits whose oneline they match.
> >
> > This patch implements the post-rewrite and autosquash handling for the
> > `merge` command we just introduced. The other commands that were added
> > recently (`label`, `reset` and `bud`) do not create new commits,
> > therefore post-rewrite & autosquash do not need to handle them.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  refs.c                            |  3 ++-
> >  sequencer.c                       | 10 +++++++---
> >  t/t3430-rebase-recreate-merges.sh | 39 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 48 insertions(+), 4 deletions(-)
> >
> > diff --git a/refs.c b/refs.c
> > index 20ba82b4343..e8b84c189ff 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -600,7 +600,8 @@ int dwim_log(const char *str, int len, struct object_id *oid, char **log)
> >  static int is_per_worktree_ref(const char *refname)
> >  {
> >         return !strcmp(refname, "HEAD") ||
> > -               starts_with(refname, "refs/bisect/");
> > +               starts_with(refname, "refs/bisect/") ||
> > +               starts_with(refname, "refs/rewritten/");
> >  }
> 
> Would this part make more sense to move into the commit that
> introduces writing these refs, or does it only matter once you start
> this step here?
> 
> >
> >  static int is_pseudoref_syntax(const char *refname)
> > diff --git a/sequencer.c b/sequencer.c
> > index 1bef16647b4..b63bfb9a141 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2413,10 +2413,13 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
> >                         res = do_reset(item->arg, item->arg_len);
> >                 else if (item->command == TODO_BUD)
> >                         res = do_reset("onto", 4);
> > -               else if (item->command == TODO_MERGE)
> > +               else if (item->command == TODO_MERGE) {
> >                         res = do_merge(item->commit,
> >                                        item->arg, item->arg_len, opts);
> > -               else if (!is_noop(item->command))
> > +                       if (item->commit)
> > +                               record_in_rewritten(&item->commit->object.oid,
> > +                                                   peek_command(todo_list, 1));
> > +               } else if (!is_noop(item->command))
> >                         return error(_("unknown command %d"), item->command);
> >
> >                 todo_list->current++;
> > @@ -3556,7 +3559,8 @@ int rearrange_squash(void)
> >                 struct subject2item_entry *entry;
> >
> >                 next[i] = tail[i] = -1;
> > -               if (item->command >= TODO_EXEC) {
> > +               if (item->command >= TODO_EXEC &&
> > +                   (item->command != TODO_MERGE || !item->commit)) {
> >                         subjects[i] = NULL;
> >                         continue;
> >                 }
> > diff --git a/t/t3430-rebase-recreate-merges.sh b/t/t3430-rebase-recreate-merges.sh
> > index 46ae52f88b3..76e615bd7c1 100755
> > --- a/t/t3430-rebase-recreate-merges.sh
> > +++ b/t/t3430-rebase-recreate-merges.sh
> > @@ -143,4 +143,43 @@ test_expect_success 'with a branch tip that was cherry-picked already' '
> >         EOF
> >  '
> >
> > +test_expect_success 'refs/rewritten/* is worktree-local' '
> > +       git worktree add wt &&
> > +       cat >wt/script-from-scratch <<-\EOF &&
> > +       label xyz
> > +       exec GIT_DIR=../.git git rev-parse --verify refs/rewritten/xyz >a || :
> > +       exec git rev-parse --verify refs/rewritten/xyz >b
> > +       EOF
> > +
> > +       test_config -C wt sequence.editor \""$PWD"/replace-editor.sh\" &&
> > +       git -C wt rebase -i HEAD &&
> > +       test_must_be_empty wt/a &&
> > +       test_cmp_rev HEAD "$(cat wt/b)"
> > +'
> > +
> 
> Same for the test here, I can't figure out why this is necessary in
> this patch as opposed to the patch which first introduced the
> refs/rewritten/<label> refs.

>> Same for the test here, I can't figure out why this is necessary in
>> this patch as opposed to the patch which first introduced the
>> refs/rewritten/<label> refs.
>
> Woops. This was its own commit, and must have been accidentally squashed
> during one of my rebases. Will re-introduce it;

>>  static int is_per_worktree_ref(const char *refname)
>>  {
>>         return !strcmp(refname, "HEAD") ||
>> -               starts_with(refname, "refs/bisect/");
>> +               starts_with(refname, "refs/bisect/") ||
>> +               starts_with(refname, "refs/rewritten/");
>>  }
>
> Would this part make more sense to move into the commit that
> introduces writing these refs, or does it only matter once you start
> this step here?

> This one is a bit tricky to explain, so let's try with a diagram:
>
>        C
>      /   \
> A - B - E - F
>  \   /
>    D
>
> To illustrate what this new mode is all about, let's consider what
> happens upon `git rebase -i --recreate-merges B`, in particular to
> the commit `D`. In the default mode, the new branch structure is:
>
>       --- C' --
>      /         \
> A - B ------ E' - F'
>      \    /
>        D'
>
> This is not really preserving the branch topology from before! The
> reason is that the commit `D` does not have `B` as ancestor, and
> therefore it gets rebased onto `B`.
>
> However, when recreating branch structure, there are legitimate use
> cases where one might want to preserve the branch points of commits that
> do not descend from the <upstream> commit that was passed to the rebase
> command, e.g. when a branch from core Git's `next` was merged into Git
> for Windows' master we will not want to rebase those commits on top of a
> Windows-specific commit. In the example above, the desired outcome would
> look like this:
>
>       --- C' --
>      /         \
> A - B ------ E' - F'
>  \        /
>   -- D' --

>
> Let's introduce the term "cousins" for such commits ("D" in the
> example), and the "no-rebase-cousins" mode of the merge-recreating
> rebase, to help those use cases.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Documentation/git-rebase.txt      |  7 ++++++-
> builtin/rebase--helper.c          |  9 ++++++++-
> git-rebase--interactive.sh        |  1 +
> git-rebase.sh                     | 12 +++++++++++-
> sequencer.c                       |  4 ++++
> sequencer.h                       |  8 ++++++++
> t/t3430-rebase-recreate-merges.sh | 23 +++++++++++++++++++++++
> 7 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 1d061373288..ac07a5c3fc9 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -368,10 +368,15 @@ The commit list format can be changed by setting the 
> configuration option
> rebase.instructionFormat.  A customized instruction format will 
> automatically
> have the long commit hash prepended to the format.
>
> ---recreate-merges::
> +--recreate-merges[=(rebase-cousins|no-rebase-cousins)]::
>  Recreate merge commits instead of flattening the history by replaying
>  merges. Merge conflict resolutions or manual amendments to merge
>  commits are not preserved.
> ++
> +By default, or when `rebase-cousins` was specified, commits which do not 
> have
> +`<upstream>` as direct ancestor are rebased onto `<upstream>` (or 
> `<onto>`,
> +if specified). If the `rebase-cousins` mode is turned off, such commits 
> will
> +retain their original branch point.
>
> -p::
> --preserve-merges::
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index a34ab5c0655..ef08fef4d14 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -13,7 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, 
> const char *prefix)
> {
>  struct replay_opts opts = REPLAY_OPTS_INIT;
>  unsigned flags = 0, keep_empty = 0, recreate_merges = 0;
> - int abbreviate_commands = 0;
> + int abbreviate_commands = 0, no_rebase_cousins = -1;
>  enum {
>  CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>  CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> @@ -23,6 +23,8 @@ int cmd_rebase__helper(int argc, const char **argv, 
> const char *prefix)
>  OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
>  OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")),
>  OPT_BOOL(0, "recreate-merges", &recreate_merges, N_("recreate merge 
> commits")),
> + OPT_BOOL(0, "no-rebase-cousins", &no_rebase_cousins,
> + N_("keep original branch points of cousins")),
>  OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
>  CONTINUE),
>  OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
> @@ -57,8 +59,13 @@ int cmd_rebase__helper(int argc, const char **argv, 
> const char *prefix)
>  flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
>  flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
>  flags |= recreate_merges ? TODO_LIST_RECREATE_MERGES : 0;
> + flags |= no_rebase_cousins > 0 ? TODO_LIST_NO_REBASE_COUSINS : 0;
>  flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
>
> + if (no_rebase_cousins >= 0&& !recreate_merges)
> + warning(_("--[no-]rebase-cousins has no effect without "
> +   "--recreate-merges"));
> +
>  if (command == CONTINUE && argc == 1)
>  return !!sequencer_continue(&opts);
>  if (command == ABORT && argc == 1)
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 3459ec5a018..23184c77e88 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -901,6 +901,7 @@ if test t != "$preserve_merges"
> then
>  git rebase--helper --make-script ${keep_empty:+--keep-empty} \
>  ${recreate_merges:+--recreate-merges} \
> + ${no_rebase_cousins:+--no-rebase-cousins} \
>  $revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
>  die "$(gettext "Could not generate todo list")"
> else
> diff --git a/git-rebase.sh b/git-rebase.sh
> index d69bc7d0e0d..3403b1416a8 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -17,7 +17,7 @@ q,quiet!           be quiet. implies --no-stat
> autostash          automatically stash/stash pop before and after
> fork-point         use 'merge-base --fork-point' to refine upstream
> onto=!             rebase onto given branch instead of upstream
> -recreate-merges!   try to recreate merges instead of skipping them
> +recreate-merges?   try to recreate merges instead of skipping them
> p,preserve-merges! try to recreate merges instead of ignoring them
> s,strategy=!       use the given merge strategy
> no-ff!             cherry-pick all commits, even if unchanged
> @@ -88,6 +88,7 @@ state_dir=
> # One of {'', continue, skip, abort}, as parsed from command line
> action=
> recreate_merges=
> +no_rebase_cousins=
> preserve_merges=
> autosquash=
> keep_empty=
> @@ -268,6 +269,15 @@ do
>  recreate_merges=t
>  test -z "$interactive_rebase" && interactive_rebase=implied
>  ;;
> + --recreate-merges=*)
> + recreate_merges=t
> + case "${1#*=}" in
> + rebase-cousins) no_rebase_cousins=;;
> + no-rebase-cousins) no_rebase_cousins=t;;
> + *) die "Unknown mode: $1";;
> + esac
> + test -z "$interactive_rebase" && interactive_rebase=implied
> + ;;
>  --preserve-merges)
>  preserve_merges=t
>  test -z "$interactive_rebase" && interactive_rebase=implied
> diff --git a/sequencer.c b/sequencer.c
> index b63bfb9a141..2b4e6b12321 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2905,6 +2905,7 @@ static int make_script_with_merges(struct 
> pretty_print_context *pp,
>     unsigned flags)
> {
>  int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
> + int no_rebase_cousins = flags & TODO_LIST_NO_REBASE_COUSINS;
>  struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
>  struct strbuf label = STRBUF_INIT;
>  struct commit_list *commits = NULL, **tail = &commits, *iter;
> @@ -3078,6 +3079,9 @@ static int make_script_with_merges(struct 
> pretty_print_context *pp,
>     &commit->object.oid);
>  if (entry)
>  to = entry->string;
> + else if (no_rebase_cousins)
> + to = label_oid(&commit->object.oid, NULL,
> +        &state);
>
>  if (!to || !strcmp("onto", to))
>  fprintf(out, "%s\n", b);
> diff --git a/sequencer.h b/sequencer.h
> index 11d1ac925ef..9530dba3cba 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -49,6 +49,14 @@ int sequencer_remove_state(struct replay_opts *opts);
> #define TODO_LIST_SHORTEN_IDS (1U << 1)
> #define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
> #define TODO_LIST_RECREATE_MERGES (1U << 3)
> +/*
> + * When recreating merges, commits that do have the base commit as 
> ancestor
> + * ("cousins") are rebased onto the new base by default. If those commits
> + * should keep their original branch point, this flag needs to be passed.
> + *
> + * This flag only makes sense when <base> and <onto> are different.
> + */
> +#define TODO_LIST_NO_REBASE_COUSINS (1U << 4)
> int sequencer_make_script(FILE *out, int argc, const char **argv,
>    unsigned flags);
>
> diff --git a/t/t3430-rebase-recreate-merges.sh 
> b/t/t3430-rebase-recreate-merges.sh
> index 76e615bd7c1..22930e470a4 100755
> --- a/t/t3430-rebase-recreate-merges.sh
> +++ b/t/t3430-rebase-recreate-merges.sh
> @@ -143,6 +143,29 @@ test_expect_success 'with a branch tip that was 
> cherry-picked already' '
>  EOF
> '
>
> +test_expect_success 'rebase cousins unless told not to' '
> + write_script copy-editor.sh <<-\EOF &&
> + cp "$1" "$(git rev-parse --git-path ORIGINAL-TODO)"
> + EOF
> +
> + test_config sequence.editor \""$PWD"/copy-editor.sh\" &&
> + git checkout -b cousins master &&
> + before="$(git rev-parse --verify HEAD)" &&
> + test_tick &&
> + git rebase -i --recreate-merges=no-rebase-cousins HEAD^ &&
> + test_cmp_rev HEAD $before &&
> + test_tick &&
> + git rebase -i --recreate-merges HEAD^ &&
> + test_cmp_graph HEAD^.. <<-\EOF
> + *   Merge the topic branch '\''onebranch'\''
> + |\
> + | * D
> + | * G
> + |/
> + o H
> + EOF
> +'
> +
> test_expect_success 'refs/rewritten/* is worktree-local' '
>  git worktree add wt &&
>  cat >wt/script-from-scratch <<-\EOF &&
> -- 
> 2.15.1.windows.2.1430.ga56c4f9e2a9 

> From: "Johannes Schindelin" <johannes.schindelin@gmx.de>
> > This one is a bit tricky to explain, so let's try with a diagram:
> >
> >        C
> >      /   \
> > A - B - E - F
> >  \   /
> >    D
> >
> > To illustrate what this new mode is all about, let's consider what
> > happens upon `git rebase -i --recreate-merges B`, in particular to
> > the commit `D`. In the default mode, the new branch structure is:
> >
> >      --- C' --
> >      /         \
> > A - B ------ E' - F'
> >      \    /
> >        D'
> >
> > This is not really preserving the branch topology from before! The
> > reason is that the commit `D` does not have `B` as ancestor, and
> > therefore it gets rebased onto `B`.
> >
> > However, when recreating branch structure, there are legitimate use
> > cases where one might want to preserve the branch points of commits that
> > do not descend from the <upstream> commit that was passed to the rebase
> > command, e.g. when a branch from core Git's `next` was merged into Git
> > for Windows' master we will not want to rebase those commits on top of a
> > Windows-specific commit. In the example above, the desired outcome would
> > look like this:
> >
> >      --- C' --
> >      /         \
> > A - B ------ E' - F'
> >  \        /
> >   -- D' --
> 
> I'm not understanding this. I see that D properly starts from A, but
> don't see why it is now D'. Surely it's unchanged.

> Maybe it's the arc/node confusion. Maybe even spell out that the rebased
> commits from the command are B..HEAD, but that includes D, which may not
> be what folk had expected. (not even sure if the reflog comes into
> determining merge-bases here..)
> 
> I do think an exact definition is needed (e.g. via --ancestry-path or
> its equivalent?).

> This one is a bit tricky to explain, so let's try with a diagram:
> [...]
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> @@ -57,8 +59,13 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> +       if (no_rebase_cousins >= 0&& !recreate_merges)

> Once upon a time, I dreamt of an interactive rebase that would not
> flatten branch structure, but instead recreate the commit topology
> faithfully.
>
> My original attempt was --preserve-merges, but that design was so
> limited that I did not even enable it in interactive mode.
>
> Subsequently, it *was* enabled in interactive mode, with the predictable
> consequences: as the --preserve-merges design does not allow for
> specifying the parents of merge commits explicitly, all the new commits'
> parents are defined *implicitly* by the previous commit history, and
> hence it is *not possible to even reorder commits*.
>
> This design flaw cannot be fixed. Not without a complete re-design, at
> least. This patch series offers such a re-design.
>
> Think of --recreate-merges as "--preserve-merges done right". It
> introduces new verbs for the todo list, `label`, `reset` and `merge`.
> For a commit topology like this:
>
>             A - B - C
>               \   /
>                 D
>
> the generated todo list would look like this:
>
>             # branch D
>             pick 0123 A
>             label branch-point
>             pick 1234 D
>             label D
>
>             reset branch-point
>             pick 2345 B
>             merge 3456 D C
>
> There are more patches in the pipeline, based on this patch series, but
> left for later in the interest of reviewable patch series: one mini
> series to use the sequencer even for `git rebase -i --root`, and another
> one to add support for octopus merges to --recreate-merges.
>
>

> Johannes Schindelin (8):
>   sequencer: introduce new commands to reset the revision
>   sequencer: introduce the `merge` command
>   sequencer: fast-forward merge commits, if possible
>   rebase-helper --make-script: introduce a flag to recreate merges
>   rebase: introduce the --recreate-merges option
>   sequencer: handle autosquash and post-rewrite for merge commands
>   pull: accept --rebase=recreate to recreate the branch topology
>   rebase -i: introduce --recreate-merges=no-rebase-cousins
>
>  Documentation/config.txt               |   8 +
>  Documentation/git-pull.txt             |   5 +-
>  Documentation/git-rebase.txt           |  13 +-
>  builtin/pull.c                         |  14 +-
>  builtin/rebase--helper.c               |  13 +-
>  builtin/remote.c                       |   2 +
>  contrib/completion/git-completion.bash |   4 +-
>  git-rebase--interactive.sh             |   6 +
>  git-rebase.sh                          |  16 +
>  refs.c                                 |   3 +-
>  sequencer.c                            | 697 ++++++++++++++++++++++++++++++++-
>  sequencer.h                            |   9 +
>  t/t3430-rebase-recreate-merges.sh      | 208 ++++++++++
>  13 files changed, 977 insertions(+), 21 deletions(-)
>  create mode 100755 t/t3430-rebase-recreate-merges.sh
>
>
> base-commit: 2512f15446149235156528dafbe75930c712b29e
> Published-As: https://github.com/dscho/git/releases/tag/recreate-merges-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git recreate-merges-v1
> --
> 2.15.1.windows.2.1430.ga56c4f9e2a9
>

> Up to now each command took a commit as its first argument and ignored
> the rest of the line (usually the subject of the commit)
>
> Now that we have commands that take different arguments, clarify each
> command by giving the argument list.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-rebase--interactive.sh | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 23184c77e8..3cd7446d0b 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -155,17 +155,17 @@ reschedule_last_action () {
>  append_todo_help () {
>         gettext "
>  Commands:
> -p, pick = use commit
> -r, reword = use commit, but edit the commit message
> -e, edit = use commit, but stop for amending
> -s, squash = use commit, but meld into previous commit
> -f, fixup = like \"squash\", but discard this commit's log message
> -x, exec = run command (the rest of the line) using shell
> -d, drop = remove commit
> -l, label = label current HEAD with a name
> -t, reset = reset HEAD to a label
> -b, bud = reset HEAD to the revision labeled 'onto'
> -m, merge = create a merge commit using a given commit's message
> +p, pick <commit> = use commit
> +r, reword <commit> = use commit, but edit the commit message
> +e, edit <commit> = use commit, but stop for amending
> +s, squash <commit> = use commit, but meld into previous commit
> +f, fixup <commit> = like \"squash\", but discard this commit's log message
> +x, exec <commit> = run command (the rest of the line) using shell
> +d, drop <commit> = remove commit
> +l, label <label>= label current HEAD with a name
> +t, reset <label> = reset HEAD to a label
> +b, bud = reset HEAD to the revision labeled 'onto', no arguments
> +m, merge [<label-or-commit>]* = create a merge commit using a given commit's message

>
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> --
> 2.16.0.rc1.238.g530d649a79-goog
>

> Up to now each command took a commit as its first argument and ignored
> the rest of the line (usually the subject of the commit)
> 
> Now that we have commands that take different arguments, clarify each
> command by giving the argument list.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-rebase--interactive.sh | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 23184c77e8..3cd7446d0b 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -155,17 +155,17 @@ reschedule_last_action () {
>  append_todo_help () {
>  	gettext "
>  Commands:
> -p, pick = use commit
> -r, reword = use commit, but edit the commit message
> -e, edit = use commit, but stop for amending
> -s, squash = use commit, but meld into previous commit
> -f, fixup = like \"squash\", but discard this commit's log message
> -x, exec = run command (the rest of the line) using shell
> -d, drop = remove commit
> -l, label = label current HEAD with a name
> -t, reset = reset HEAD to a label
> -b, bud = reset HEAD to the revision labeled 'onto'
> -m, merge = create a merge commit using a given commit's message
> +p, pick <commit> = use commit
> +r, reword <commit> = use commit, but edit the commit message
> +e, edit <commit> = use commit, but stop for amending
> +s, squash <commit> = use commit, but meld into previous commit
> +f, fixup <commit> = like \"squash\", but discard this commit's log message
> +x, exec <commit> = run command (the rest of the line) using shell
> +d, drop <commit> = remove commit
> +l, label <label>= label current HEAD with a name
> +t, reset <label> = reset HEAD to a label
> +b, bud = reset HEAD to the revision labeled 'onto', no arguments
> +m, merge [<label-or-commit>]* = create a merge commit using a given commit's message

>
> As to `merge`: it is a bit more complicated ;-)
>
>         m, merge <original-merge-commit> ( <label> | "<label>..." ) [<oneline>]
>                 create a merge commit using the original merge commit's
>                 message (or the oneline, if "-" is given). Use a quoted
>                 list of commits to be merged for octopus merges.

> Good idea! I would rather do it as an introductory patch (that only
> converts the existing list).
>
> As to `merge`: it is a bit more complicated ;-)
>
> 	m, merge <original-merge-commit> ( <label> | "<label>..." ) [<oneline>]
> 		create a merge commit using the original merge commit's
> 		message (or the oneline, if "-" is given). Use a quoted
> 		list of commits to be merged for octopus merges.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Good idea! I would rather do it as an introductory patch (that only
>> converts the existing list).
>>
>> As to `merge`: it is a bit more complicated ;-)
>>
>>       m, merge <original-merge-commit> ( <label> | "<label>..." ) [<oneline>]
>>               create a merge commit using the original merge commit's
>>               message (or the oneline, if "-" is given). Use a quoted
>>               list of commits to be merged for octopus merges.
>
> Is it just the message that is being reused?
>
> Aren't the trees of the original commit and its parents participate
> in creating the tree of the recreated merge?  One way to preserve an
> originally evil merge is to notice how it was made by taking the
> difference between the result of mechanical merge of original merge
> parents and the original merge result, and carry it forward when
> recreating the merge across new parents.  Just being curious.
>

> On Fri, Jan 19, 2018 at 12:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> Good idea! I would rather do it as an introductory patch (that only
> >> converts the existing list).
> >>
> >> As to `merge`: it is a bit more complicated ;-)
> >>
> >>       m, merge <original-merge-commit> ( <label> | "<label>..." ) [<oneline>]
> >>               create a merge commit using the original merge commit's
> >>               message (or the oneline, if "-" is given). Use a quoted
> >>               list of commits to be merged for octopus merges.
> >
> > Is it just the message that is being reused?
> >
> > Aren't the trees of the original commit and its parents participate
> > in creating the tree of the recreated merge?  One way to preserve an
> > originally evil merge is to notice how it was made by taking the
> > difference between the result of mechanical merge of original merge
> > parents and the original merge result, and carry it forward when
> > recreating the merge across new parents.  Just being curious.
> >
> 
> It looks like currently that only the commit is kept, with no attempt
> to recreate evil merges.

> Jake suggested using "x false" instead of "edit" for some corner cases.
>
> I do prefer using "x false" for all kinds of things such as stopping
> before a commit (edit only let's you stop after a commit), and the
> knowledge that "x false" does the least amount of actions behind my back.
>
> We should have that command as well, maybe?
>

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-rebase--interactive.sh |  1 +
>  sequencer.c                | 10 ++++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 3cd7446d0b..9eac53f0c5 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -166,6 +166,7 @@ l, label <label>= label current HEAD with a name
>  t, reset <label> = reset HEAD to a label
>  b, bud = reset HEAD to the revision labeled 'onto', no arguments
>  m, merge [<label-or-commit>]* = create a merge commit using a given commit's message
> +y, stay = stop for  shortcut for
>
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 2b4e6b1232..4b3b9fe59d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -782,6 +782,7 @@ enum todo_command {
>         TODO_RESET,
>         TODO_BUD,
>         TODO_MERGE,
> +       TODO_STOP,
>         /* commands that do nothing but are counted for reporting progress */
>         TODO_NOOP,
>         TODO_DROP,
> @@ -803,6 +804,7 @@ static struct {
>         { 'l', "label" },
>         { 't', "reset" },
>         { 'b', "bud" },
> +       { 'y', "stay" },
>         { 'm', "merge" },
>         { 0,   "noop" },
>         { 'd', "drop" },
> @@ -1307,6 +1309,12 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>                 return 0;
>         }
>
> +       if (item->command == TODO_STOP) {
> +               item->commit = NULL;
> +               item->arg = "";
> +               item->arg_len = 0;
> +       }
> +
>         end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
>         item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
>         item->arg_len = (int)(eol - item->arg);
> @@ -2407,6 +2415,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>                                 /* `current` will be incremented below */
>                                 todo_list->current = -1;
>                         }
> +               } else if (item->command == TODO_STOP) {
> +                       todo_list->current = -1;
>                 } else if (item->command == TODO_LABEL)
>                         res = do_label(item->arg, item->arg_len);
>                 else if (item->command == TODO_RESET)
> --
> 2.16.0.rc1.238.g530d649a79-goog
>

> On Thu, Jan 18, 2018 at 10:36 AM, Stefan Beller <sbeller@google.com> 
> wrote:
>> Jake suggested using "x false" instead of "edit" for some corner cases.
>>
>> I do prefer using "x false" for all kinds of things such as stopping
>> before a commit (edit only let's you stop after a commit), and the
>> knowledge that "x false" does the least amount of actions behind my back.
>>
>> We should have that command as well, maybe?
>>
>
>
> I agree. I use "x false" very often, and I think stop is probably a
> better solution since it avoids spawning an extra shell that will just
> fail. Not sure if stop implies too much about "stop the whole thing"
> as opposed to "stop here and let me do something manual", but I think
> it's clear enough.
>

>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  git-rebase--interactive.sh |  1 +
>>  sequencer.c                | 10 ++++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 3cd7446d0b..9eac53f0c5 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -166,6 +166,7 @@ l, label <label>= label current HEAD with a name
>>  t, reset <label> = reset HEAD to a label
>>  b, bud = reset HEAD to the revision labeled 'onto', no arguments
>>  m, merge [<label-or-commit>]* = create a merge commit using a given 
>> commit's message
>> +y, stay = stop for  shortcut for
>>
>>  These lines can be re-ordered; they are executed from top to bottom.
>>  " | git stripspace --comment-lines >>"$todo"
>> diff --git a/sequencer.c b/sequencer.c
>> index 2b4e6b1232..4b3b9fe59d 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -782,6 +782,7 @@ enum todo_command {
>>         TODO_RESET,
>>         TODO_BUD,
>>         TODO_MERGE,
>> +       TODO_STOP,
>>         /* commands that do nothing but are counted for reporting 
>> progress */
>>         TODO_NOOP,
>>         TODO_DROP,
>> @@ -803,6 +804,7 @@ static struct {
>>         { 'l', "label" },
>>         { 't', "reset" },
>>         { 'b', "bud" },
>> +       { 'y', "stay" },
>>         { 'm', "merge" },
>>         { 0,   "noop" },
>>         { 'd', "drop" },
>> @@ -1307,6 +1309,12 @@ static int parse_insn_line(struct todo_item *item, 
>> const char *bol, char *eol)
>>                 return 0;
>>         }
>>
>> +       if (item->command == TODO_STOP) {
>> +               item->commit = NULL;
>> +               item->arg = "";
>> +               item->arg_len = 0;
>> +       }
>> +
>>         end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
>>         item->arg = end_of_object_name + strspn(end_of_object_name, " 
>> \t");
>>         item->arg_len = (int)(eol - item->arg);
>> @@ -2407,6 +2415,8 @@ static int pick_commits(struct todo_list 
>> *todo_list, struct replay_opts *opts)
>>                                 /* `current` will be incremented below */
>>                                 todo_list->current = -1;
>>                         }
>> +               } else if (item->command == TODO_STOP) {
>> +                       todo_list->current = -1;
>>                 } else if (item->command == TODO_LABEL)
>>                         res = do_label(item->arg, item->arg_len);
>>                 else if (item->command == TODO_RESET)
>> --
>> 2.16.0.rc1.238.g530d649a79-goog
>> 

> From: "Jacob Keller" <jacob.keller@gmail.com>
>>
>> On Thu, Jan 18, 2018 at 10:36 AM, Stefan Beller <sbeller@google.com>
>> wrote:
>>>
>>> Jake suggested using "x false" instead of "edit" for some corner cases.
>>>
>>> I do prefer using "x false" for all kinds of things such as stopping
>>> before a commit (edit only let's you stop after a commit), and the
>>> knowledge that "x false" does the least amount of actions behind my back.
>>>
>>> We should have that command as well, maybe?
>>>
>>
>>
>> I agree. I use "x false" very often, and I think stop is probably a
>> better solution since it avoids spawning an extra shell that will just
>> fail. Not sure if stop implies too much about "stop the whole thing"
>> as opposed to "stop here and let me do something manual", but I think
>> it's clear enough.
>>
> 'hold' or 'pause' maybe options (leads to
> http://www.thesaurus.com/browse/put+on+hold offering procastinate etc.)
> 'adjourn'.
>
>

> Jake suggested using "x false" instead of "edit" for some corner cases.
> 
> I do prefer using "x false" for all kinds of things such as stopping
> before a commit (edit only let's you stop after a commit), and the
> knowledge that "x false" does the least amount of actions behind my back.
> 
> We should have that command as well, maybe?
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-rebase--interactive.sh |  1 +
>  sequencer.c                | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 3cd7446d0b..9eac53f0c5 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -166,6 +166,7 @@ l, label <label>= label current HEAD with a name
>  t, reset <label> = reset HEAD to a label
>  b, bud = reset HEAD to the revision labeled 'onto', no arguments
>  m, merge [<label-or-commit>]* = create a merge commit using a given commit's message
> +y, stay = stop for  shortcut for
>  
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 2b4e6b1232..4b3b9fe59d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -782,6 +782,7 @@ enum todo_command {
>  	TODO_RESET,
>  	TODO_BUD,
>  	TODO_MERGE,
> +	TODO_STOP,

> @@ -2407,6 +2415,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  				/* `current` will be incremented below */
>  				todo_list->current = -1;
>  			}
> +		} else if (item->command == TODO_STOP) {
> +			todo_list->current = -1;

>> +     TODO_STOP,
>
> I see that your original idea was "stop", but then you probably realized
> that there would be no good abbreviation for that, and changed your mind.
>
> Personally, I would have called it `break`...

>
>> @@ -2407,6 +2415,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>>                               /* `current` will be incremented below */
>>                               todo_list->current = -1;
>>                       }
>> +             } else if (item->command == TODO_STOP) {
>> +                     todo_list->current = -1;
>
> That is incorrect, it will most likely write an unexpected `done` file.
>
> Did you mean `return 0` instead?

> Think of --recreate-merges as "--preserve-merges done right". It
> introduces new verbs for the todo list, `label`, `reset` and `merge`.
> For a commit topology like this:
>
>             A - B - C
>               \   /
>                 D
>
> the generated todo list would look like this:
>
>             # branch D
>             pick 0123 A
>             label branch-point
>             pick 1234 D
>             label D
>
>             reset branch-point
>             pick 2345 B
>             merge 3456 D C

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Think of --recreate-merges as "--preserve-merges done right". It
> > introduces new verbs for the todo list, `label`, `reset` and `merge`.
> > For a commit topology like this:
> >
> >             A - B - C
> >               \   /
> >                 D
> >
> > the generated todo list would look like this:
> >
> >             # branch D
> >             pick 0123 A
> >             label branch-point
> >             pick 1234 D
> >             label D
> >
> >             reset branch-point
> >             pick 2345 B
> >             merge 3456 D C
> 
> Yup.  I've seen this design talked about on list in the past, and
> I've always felt that this is "sequencer done right".
> 
> At the first glance, it may feel somewhat unsatisfying that "merge"
> has to say effects of which commits should be reflected in the
> result and which commot to take the log message from, i.e.
> (recreated)D is merged to form the resulting tree, and 3456=C is
> used for the log, to recreate C in the above example, while "pick"
> always uses the same commit for both, i.e. recreated B inherits both
> the changes and log message from the original B=2345 (or depending
> on the readers' point of view, "merge" is allowed to use two
> different commits, while "pick" is always limited to the same one).
> 
> But I think this distinction is probably fundamental and I am not
> opposed to it at all.  The result of "pick" has only one parent, and
> the parent is determined only by the previous actions and not by
> anything on the "pick" line in the todo list.  But the result of
> "merge" has to record all the other parents, and only the first
> parent is determined implicitly by the previous actions.  We need to
> tell the "merge" command about "3456=C" in order to recreate the
> effect of original merge commit (i.e. changes between B and C) as
> well as its log message, and we also need to tell it about label "D"
> that it is the "other parent" that need to be recorded.

> Obviously "merge" command syntax should allow recreating an octopus,
> so whenever I said "two" in the above, I meant "N".  The original
> merge commit is needed so that the effect to replay (roughly: a
> patch going to the original merge result from its first parent) can
> be learned from the existing history, and all the other "N-1"
> parents needs to be given (and they must have been already created
> in the todo list) so that the resulting recreated merge can be
> recorded with them as parents (in addition to the first parent that
> is implicitly given as the result of all the previous steps).

> One interesting (and probably useful) thing to notice is that if A
> were not rebased in the above sample picture, and only B were the
> one that was tweaked, then a recreated C may use the same original D
> as its side parent, and the mechanism outlined above naturally can
> support it by allowing an un-rewritten commit to be given as a side
> parent when "merge" is redoing C.

> My original attempt was --preserve-merges, but that design was so
> limited that I did not even enable it in interactive mode.
> ...
> There are more patches in the pipeline, based on this patch series, but
> left for later in the interest of reviewable patch series: one mini
> series to use the sequencer even for `git rebase -i --root`, and another
> one to add support for octopus merges to --recreate-merges.

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > My original attempt was --preserve-merges, but that design was so
> > limited that I did not even enable it in interactive mode.
> > ...
> > There are more patches in the pipeline, based on this patch series, but
> > left for later in the interest of reviewable patch series: one mini
> > series to use the sequencer even for `git rebase -i --root`, and another
> > one to add support for octopus merges to --recreate-merges.
> 
> I left comments on a handful of them, but I do not think any of them
> spotted a grave design issue to be a show stopper.  Overall, the
> series was quite a pleasant read, even with those minor nits and
> rooms for improvements.

> [...]
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
>         label <name>
>         reset <name>
> [...]
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -1253,7 +1266,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>                 if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
>                         item->command = i;
>                         break;
> -               } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
> +               } else if ((bol + 1 == eol || bol[1] == ' ') &&
> +                          *bol == todo_command_info[i].c) {

>                         bol++;
>                         item->command = i;
>                         break;
> @@ -1919,6 +1934,144 @@ static int do_exec(const char *command_line)
> +static int safe_append(const char *filename, const char *fmt, ...)
> +{
> +       va_list ap;
> +       struct lock_file lock = LOCK_INIT;
> +       int fd = hold_lock_file_for_update(&lock, filename, 0);
> +       struct strbuf buf = STRBUF_INIT;
> +
> +       if (fd < 0)
> +               return error_errno(_("could not lock '%s'"), filename);

> +
> +       if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT)
> +               return error_errno(_("could not read '%s'"), filename);
> +       strbuf_complete(&buf, '\n');
> +       va_start(ap, fmt);
> +       strbuf_vaddf(&buf, fmt, ap);
> +       va_end(ap);

> +       }
> +       if (commit_lock_file(&lock) < 0) {
> +               rollback_lock_file(&lock);
> +               return error(_("failed to finalize '%s'"), filename);
> +       }
> +
> +       return 0;
> +}
> +
> +static int do_reset(const char *name, int len)
> +{
> +       [...]
> +       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);

> On Mon, Jan 29, 2018 at 5:54 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > [...]
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> >         label <name>
> >         reset <name>
> > [...]
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -1253,7 +1266,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
> >                 if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
> >                         item->command = i;
> >                         break;
> > -               } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
> > +               } else if ((bol + 1 == eol || bol[1] == ' ') &&
> > +                          *bol == todo_command_info[i].c) {
> 
> This adds support for commands which have no arguments, however, now
> that the "bud" command has been retired, this can go away too, right?

> >                         bol++;
> >                         item->command = i;
> >                         break;
> > @@ -1919,6 +1934,144 @@ static int do_exec(const char *command_line)
> > +static int safe_append(const char *filename, const char *fmt, ...)
> > +{
> > +       va_list ap;
> > +       struct lock_file lock = LOCK_INIT;
> > +       int fd = hold_lock_file_for_update(&lock, filename, 0);
> > +       struct strbuf buf = STRBUF_INIT;
> > +
> > +       if (fd < 0)
> > +               return error_errno(_("could not lock '%s'"), filename);
> 
> Minor: unable_to_lock_message() can provide a more detailed
> explanation of the failure.

> > +
> > +       if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT)
> > +               return error_errno(_("could not read '%s'"), filename);
> > +       strbuf_complete(&buf, '\n');
> > +       va_start(ap, fmt);
> > +       strbuf_vaddf(&buf, fmt, ap);
> > +       va_end(ap);
> 
> Would it make sense to also
> 
>     strbuf_complete(&buf, '\n')
> 
> here, as well, to be a bit more robust against lazy callers?

> > +
> > +       if (write_in_full(fd, buf.buf, buf.len) < 0) {
> > +               rollback_lock_file(&lock);
> > +               return error_errno(_("could not write to '%s'"), filename);
> 
> Reading lockfile.h & tempfile.c, I see that rollback_lock_file()
> clobbers write_in_full()'s errno before error_errno() is called.

> > +       }
> > +       if (commit_lock_file(&lock) < 0) {
> > +               rollback_lock_file(&lock);
> > +               return error(_("failed to finalize '%s'"), filename);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int do_reset(const char *name, int len)
> > +{
> > +       [...]
> > +       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);
> 
> Checking my understanding: The two get_oid() calls allow the argument
> to 'reset' to be a label created with the 'label' command or any other
> way to name an object, right? If so, then I wonder if the error
> invocation should instead be:
> 
>     error(_("could not read '%.*s'"), len, name);

> In the upcoming commits, we will teach the sequencer to recreate merges.
> This will be done in a very different way from the unfortunate design of
> `git rebase --preserve-merges` (which does not allow for reordering
> commits, or changing the branch topology).
>
> The main idea is to introduce new todo list commands, to support
> labeling the current revision with a given name, resetting the current
> revision to a previous state, merging labeled revisions.
>
> This idea was developed in Git for Windows' Git garden shears (that are
> used to maintain the "thicket of branches" on top of upstream Git), and
> this patch is part of the effort to make it available to a wider
> audience, as well as to make the entire process more robust (by
> implementing it in a safe and portable language rather than a Unix shell
> script).
>
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
>         label <name>
>         reset <name>
>
> Internally, the `label <name>` command creates the ref
> `refs/rewritten/<name>`. This makes it possible to work with the labeled
> revisions interactively, or in a scripted fashion (e.g. via the todo
> list command `exec`).
>
> Later in this patch series, we will mark the `refs/rewritten/` refs as
> worktree-local, to allow for interactive rebases to be run in parallel in
> worktrees linked to the same repository.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-rebase--interactive.sh |   2 +
>  sequencer.c                | 180 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 179 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index fcedece1860..7e5281e74aa 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -162,6 +162,8 @@ s, squash <commit> = use commit, but meld into previous commit
>  f, fixup <commit> = like \"squash\", but discard this commit's log message
>  x, exec <commit> = run command (the rest of the line) using shell
>  d, drop <commit> = remove commit
> +l, label <label> = label current HEAD with a name
> +t, reset <label> = reset HEAD to a label
>
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 4d3f60594cb..92ca8d2adee 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -21,6 +21,8 @@
>  #include "log-tree.h"
>  #include "wt-status.h"
>  #include "hashmap.h"
> +#include "unpack-trees.h"
> +#include "worktree.h"
>
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
>  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 listing refs that need to be deleted after the rebase
> + * finishes. This is used by the `merge` command.
> + */

> +static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
> +
>  /*
>   * The following files are written by git-rebase just after parsing the
>   * command-line (and are only consumed, not modified, by the sequencer).
> @@ -767,6 +776,8 @@ enum todo_command {
>         TODO_SQUASH,
>         /* commands that do something else than handling a single commit */
>         TODO_EXEC,
> +       TODO_LABEL,
> +       TODO_RESET,
>         /* commands that do nothing but are counted for reporting progress */
>         TODO_NOOP,
>         TODO_DROP,
> @@ -785,6 +796,8 @@ static struct {
>         { 'f', "fixup" },
>         { 's', "squash" },
>         { 'x', "exec" },
> +       { 'l', "label" },
> +       { 't', "reset" },
>         { 0,   "noop" },
>         { 'd', "drop" },
>         { 0,   NULL }
> @@ -1253,7 +1266,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>                 if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
>                         item->command = i;
>                         break;
> -               } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
> +               } else if ((bol + 1 == eol || bol[1] == ' ') &&
> +                          *bol == todo_command_info[i].c) {
>                         bol++;
>                         item->command = i;
>                         break;
> @@ -1279,7 +1293,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>                 return error(_("missing arguments for %s"),
>                              command_to_string(item->command));
>
> -       if (item->command == TODO_EXEC) {
> +       if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
> +           item->command == TODO_RESET) {
>                 item->commit = NULL;
>                 item->arg = bol;
>                 item->arg_len = (int)(eol - bol);
> @@ -1919,6 +1934,144 @@ static int do_exec(const char *command_line)
>         return status;
>  }
>
> +static int safe_append(const char *filename, const char *fmt, ...)
> +{
> +       va_list ap;
> +       struct lock_file lock = LOCK_INIT;
> +       int fd = hold_lock_file_for_update(&lock, filename, 0);
> +       struct strbuf buf = STRBUF_INIT;
> +
> +       if (fd < 0)
> +               return error_errno(_("could not lock '%s'"), filename);
> +
> +       if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT)
> +               return error_errno(_("could not read '%s'"), filename);
> +       strbuf_complete(&buf, '\n');
> +       va_start(ap, fmt);
> +       strbuf_vaddf(&buf, fmt, ap);
> +       va_end(ap);
> +
> +       if (write_in_full(fd, buf.buf, buf.len) < 0) {
> +               rollback_lock_file(&lock);
> +               return error_errno(_("could not write to '%s'"), filename);
> +       }
> +       if (commit_lock_file(&lock) < 0) {
> +               rollback_lock_file(&lock);
> +               return error(_("failed to finalize '%s'"), filename);
> +       }
> +
> +       return 0;
> +}
> +
> +static int do_label(const char *name, int len)
> +{
> +       struct ref_store *refs = get_main_ref_store();
> +       struct ref_transaction *transaction;
> +       struct strbuf ref_name = STRBUF_INIT, err = STRBUF_INIT;
> +       struct strbuf msg = STRBUF_INIT;
> +       int ret = 0;
> +       struct object_id head_oid;
> +
> +       strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
> +       strbuf_addf(&msg, "rebase -i (label) '%.*s'", len, name);
> +
> +       transaction = ref_store_transaction_begin(refs, &err);
> +       if (!transaction) {
> +               error("%s", err.buf);
> +               ret = -1;
> +       } else if (get_oid("HEAD", &head_oid)) {
> +               error(_("could not read HEAD"));
> +               ret = -1;
> +       } else if (ref_transaction_update(transaction, ref_name.buf, &head_oid,
> +                                         NULL, 0, msg.buf, &err) < 0 ||
> +                  ref_transaction_commit(transaction, &err)) {
> +               error("%s", err.buf);
> +               ret = -1;
> +       }
> +       ref_transaction_free(transaction);
> +       strbuf_release(&err);
> +       strbuf_release(&msg);
> +
> +       if (!ret)
> +               ret = safe_append(rebase_path_refs_to_delete(),
> +                                 "%s\n", ref_name.buf);
> +       strbuf_release(&ref_name);
> +
> +       return ret;
> +}
> +
> +static int do_reset(const char *name, int len)
> +{
> +       struct strbuf ref_name = STRBUF_INIT;
> +       struct object_id oid;
> +       struct lock_file lock = LOCK_INIT;
> +       struct tree_desc desc;
> +       struct tree *tree;
> +       struct unpack_trees_options opts;
> +       int ret = 0, i;
> +
> +       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;
> +       }
> +
> +       memset(&opts, 0, sizeof(opts));
> +       opts.head_idx = 1;
> +       opts.src_index = &the_index;
> +       opts.dst_index = &the_index;
> +       opts.fn = oneway_merge;
> +       opts.merge = 1;
> +       opts.update = 1;
> +       opts.reset = 1;
> +
> +       read_cache_unmerged();

> +       if (!fill_tree_descriptor(&desc, &oid)) {
> +               error(_("failed to find tree of %s"), oid_to_hex(&oid));
> +               rollback_lock_file(&lock);
> +               free((void *)desc.buffer);
> +               strbuf_release(&ref_name);
> +               return -1;
> +       }
> +
> +       if (unpack_trees(1, &desc, &opts)) {
> +               rollback_lock_file(&lock);
> +               free((void *)desc.buffer);
> +               strbuf_release(&ref_name);
> +               return -1;
> +       }
> +
> +       tree = parse_tree_indirect(&oid);
> +       prime_cache_tree(&the_index, tree);
> +
> +       if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> +               ret = error(_("could not write index"));
> +       free((void *)desc.buffer);

> On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
> >  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 listing refs that need to be deleted after the rebase
> > + * finishes. This is used by the `merge` command.
> > + */

> which is appended in do_label, it uses refs to store the commits in
> refs/rewritten.  We do not have to worry about the contents of that file
> getting too long, or label re-use, because the directory containing all
> these helper files will be deleted upon successful rebase in
> `sequencer_remove_state()`.

> > +static int do_reset(const char *name, int len)
> > +{
> > +       struct strbuf ref_name = STRBUF_INIT;
> > +       struct object_id oid;
> > +       struct lock_file lock = LOCK_INIT;
> > +       struct tree_desc desc;
> > +       struct tree *tree;
> > +       struct unpack_trees_options opts;
> > +       int ret = 0, i;
> > +
> > +       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;
> > +       }
> > +
> > +       memset(&opts, 0, sizeof(opts));
> > +       opts.head_idx = 1;
> > +       opts.src_index = &the_index;
> > +       opts.dst_index = &the_index;
> > +       opts.fn = oneway_merge;
> > +       opts.merge = 1;
> > +       opts.update = 1;
> > +       opts.reset = 1;
> > +
> > +       read_cache_unmerged();
> 
> In read-tree.c merge.c and pull.c we guard this conditionally
> and use die_resolve_conflict to bail out. In am.c we do not.
> 
> I think we'd want to guard it here, too?

> Constructing an instruction sheet that produces a merge
> conflict just before the reset is a bit artificial, but still:
> 
>     label onto
>     pick abc
>     exec false # run "git merge out-of-rebase-merge"
>         # manually to produce a conflict

> > +       if (!fill_tree_descriptor(&desc, &oid)) {
> > +               error(_("failed to find tree of %s"), oid_to_hex(&oid));
> > +               rollback_lock_file(&lock);
> > +               free((void *)desc.buffer);
> > +               strbuf_release(&ref_name);
> > +               return -1;
> > +       }
> > +
> > +       if (unpack_trees(1, &desc, &opts)) {
> > +               rollback_lock_file(&lock);
> > +               free((void *)desc.buffer);
> > +               strbuf_release(&ref_name);
> > +               return -1;
> > +       }
> > +
> > +       tree = parse_tree_indirect(&oid);
> > +       prime_cache_tree(&the_index, tree);
> > +
> > +       if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> > +               ret = error(_("could not write index"));
> > +       free((void *)desc.buffer);
> 
> For most newer structs we have a {release, clear, free}_X,
> but for tree_desc's this seems to be the convention to avoid memleaks.

> 
> Hi Stefan,
> 
> On Tue, 30 Jan 2018, Stefan Beller wrote:
> 
>> On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>>> @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
>>>  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 listing refs that need to be deleted after the rebase
>>> + * finishes. This is used by the `merge` command.
>>> + */
> 
> Whoops. The comment "This is used by the `merge` command`" is completely
> wrong. Will fix.
> 
>> So this file contains (label -> commit),
> 
> Only `label`. No `commit`.
> 
>> which is appended in do_label, it uses refs to store the commits in
>> refs/rewritten.  We do not have to worry about the contents of that file
>> getting too long, or label re-use, because the directory containing all
>> these helper files will be deleted upon successful rebase in
>> `sequencer_remove_state()`.
> 
> Yes.
>

> On 31/01/18 13:21, Johannes Schindelin wrote:
> > 
> > On Tue, 30 Jan 2018, Stefan Beller wrote:
> > 
> >> On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
> >> <johannes.schindelin@gmx.de> wrote:
> >>> @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
> >>>  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 listing refs that need to be deleted after the rebase
> >>> + * finishes. This is used by the `merge` command.
> >>> + */
> > 
> > Whoops. The comment "This is used by the `merge` command`" is completely
> > wrong. Will fix.
> > 
> >> So this file contains (label -> commit),
> > 
> > Only `label`. No `commit`.
> > 
> >> which is appended in do_label, it uses refs to store the commits in
> >> refs/rewritten.  We do not have to worry about the contents of that file
> >> getting too long, or label re-use, because the directory containing all
> >> these helper files will be deleted upon successful rebase in
> >> `sequencer_remove_state()`.
> > 
> > Yes.
> >
> It might be a good idea to have 'git rebase --abort' delete the refs as
> well as the file though

> Once upon a time, I dreamt of an interactive rebase that would not
> flatten branch structure, but instead recreate the commit topology
> faithfully.
>
> My original attempt was --preserve-merges, but that design was so
> limited that I did not even enable it in interactive mode.
>
> Subsequently, it *was* enabled in interactive mode, with the predictable
> consequences: as the --preserve-merges design does not allow for
> specifying the parents of merge commits explicitly, all the new commits'
> parents are defined *implicitly* by the previous commit history, and
> hence it is *not possible to even reorder commits*.
>
> This design flaw cannot be fixed. Not without a complete re-design, at
> least. This patch series offers such a re-design.
>
> Think of --recreate-merges as "--preserve-merges done right". It
> introduces new verbs for the todo list, `label`, `reset` and `merge`.
> For a commit topology like this:
>
>             A - B - C
>               \   /
>                 D
>
> the generated todo list would look like this:
>
>             # branch D
>             pick 0123 A
>             label branch-point
>             pick 1234 D
>             label D
>
>             reset branch-point
>             pick 2345 B
>             merge 3456 D C
>
> There are more patches in the pipeline, based on this patch series, but
> left for later in the interest of reviewable patch series: one mini
> series to use the sequencer even for `git rebase -i --root`, and another
> one to add support for octopus merges to --recreate-merges.
>
> Changes since v1:
>
> - reintroduced "sequencer: make refs generated by the `label` command
>   worktree-local" (which was squashed into "sequencer: handle autosquash
>   and post-rewrite for merge commands" by accident)
>
> - got rid of the universally-hated `bud` command

>
> - as per Stefan's suggestion, the help blurb at the end of the todo list
>   now lists the syntax
>
> - the no-rebase-cousins mode was made the default; This not only reflects
>   the experience won from those years of using the Git garden shears, but
>   was also deemed the better default in the discussion on the PR at
>   https://github.com/git/git/pull/447
>
> - I tried to clarify the role of the `onto` label in the commit message of
>   `rebase-helper --make-script: introduce a flag to recreate merges`
>
> - fixed punctuation at the end of error(...) messages, and incorrect
>   upper-case at the start
>
> - changed the generated todo lists to separate the label and the oneline in
>   the `reset` command with a `#`, for readability
>
> - dropped redundant paragraph in the commit message that talked about
>   support for octopus merges
>
> - avoided empty error message when HEAD could not be read during do_label()
>
> - merge commits are fast-forwarded only unless --force-rebase was passed
>
> - do_merge() now errors out a lot earlier when HEAD could not be parsed
>
> - the one-letter variables to hold either abbreviated or full todo list
>   instructions in make_script_recreating_merges() were renamed to clearer
>   names
>
> - The description of rebase's --recreate-merge option has been reworded;
>   Hopefully it is a lot more clear now.
>
>
> Johannes Schindelin (9):
>   sequencer: introduce new commands to reset the revision
>   sequencer: introduce the `merge` command
>   sequencer: fast-forward merge commits, if possible
>   rebase-helper --make-script: introduce a flag to recreate merges
>   rebase: introduce the --recreate-merges option
>   sequencer: make refs generated by the `label` command worktree-local
>   sequencer: handle autosquash and post-rewrite for merge commands
>   pull: accept --rebase=recreate to recreate the branch topology
>   rebase -i: introduce --recreate-merges=[no-]rebase-cousins
>
> Stefan Beller (1):
>   git-rebase--interactive: clarify arguments

> > Stefan Beller (1):
> >   git-rebase--interactive: clarify arguments
> 
> No need to honor me with authorship, as I just wrote
> that patch in a quick hurry to express the idea.

> Changes since v1:
>
> - reintroduced "sequencer: make refs generated by the `label` command
>   worktree-local" (which was squashed into "sequencer: handle autosquash
>   and post-rewrite for merge commands" by accident)

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Changes since v1:
> >
> > - reintroduced "sequencer: make refs generated by the `label` command
> >   worktree-local" (which was squashed into "sequencer: handle autosquash
> >   and post-rewrite for merge commands" by accident)
> 
> Good.
> 
> > - got rid of the universally-hated `bud` command
> 
> Universally is a bit too strong a word, unless you want to hint that
> you are specifically ignoring my input ;-).

> I think I may want to introduce a bigger change, still. I forgot who
> exactly came up with the suggestion to use `merge -C <original-commit>
> <to-merge>` (I think it was Jake), and I reacted too forcefully in
> rejecting it.
>

> But the longer I think about this, the more I come to the conclusion that
> I was wrong, and that the -C way is the way that leaves the door open to
> the pretty elegant `-c <commit>` (imitating `git commit`'s option to
> borrow the commit message from elsewhere but still allowing to edit it).
>

> [...]
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
>         label <name>
>         reset <name>
> [...]
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -1922,6 +1951,151 @@ static int do_exec(const char *command_line)
> +static int safe_append(const char *filename, const char *fmt, ...)
> +{
> +       [...]
> +       if (write_in_full(fd, buf.buf, buf.len) < 0) {
> +               error_errno(_("could not write to '%s'"), filename);
> +               rollback_lock_file(&lock);

> +       return 0;
> +}
> +
> +static int do_reset(const char *name, int len, struct replay_opts *opts)
> +{
> +       [...]
> +       unpack_tree_opts.reset = 1;
> +
> +       if (read_cache_unmerged())

> +               return error_resolve_conflict(_(action_name(opts)));
> +
> +       if (!fill_tree_descriptor(&desc, &oid)) {
> +               error(_("failed to find tree of %s"), oid_to_hex(&oid));
> +               rollback_lock_file(&lock);
> +               free((void *)desc.buffer);
> +               strbuf_release(&ref_name);
> +               return -1;
> +       }

> On Sat, Feb 10, 2018 at 7:10 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > [...]
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> >         label <name>
> >         reset <name>
> > [...]
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -1922,6 +1951,151 @@ static int do_exec(const char *command_line)
> > +static int safe_append(const char *filename, const char *fmt, ...)
> > +{
> > +       [...]
> > +       if (write_in_full(fd, buf.buf, buf.len) < 0) {
> > +               error_errno(_("could not write to '%s'"), filename);
> > +               rollback_lock_file(&lock);
> 
> strbuf_release(&buf);
> 
> > +               return -1;
> > +       }
> > +       if (commit_lock_file(&lock) < 0) {
> > +               rollback_lock_file(&lock);
> 
> strbuf_release(&buf);
> 
> > +               return error(_("failed to finalize '%s'"), filename);
> > +       }
> > +
> 
> strbuf_release(&buf);
> 
> > +       return 0;
> > +}
> > +
> > +static int do_reset(const char *name, int len, struct replay_opts *opts)
> > +{
> > +       [...]
> > +       unpack_tree_opts.reset = 1;
> > +
> > +       if (read_cache_unmerged())
> 
> rollback_lock_file(&lock);
> strbuf_release(&ref_name);