[PATCH v3 00/12] rebase -i: offer to recreate merge commits

[Johannes Schindelin <johannes.schindelin@gmx.de>]

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 -C 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 v2:

- fixed the incorrect comment for rebase_path_refs_to_delete.

- we now error out properly if read_cache_unmerged() fails.

- if there are unresolved merge conflicts, the `reset` command now errors out
  (even if the current design should not allow for such a scenario to occur).

- a diff hunk that was necessary to support `bud` was dropped from 2/10.

- changed all `rollback_lock_file(); return error_errno(...);` patterns to
  first show the errors (i.e. using the correct errno). This added 1/11.

- The temporary refs are now also cleaned up upon `git rebase --abort`.

- Reworked the entire patch series to support

	merge -C <commit> <tip> # <oneline>

  instead of the previous `merge <commit> <tip> <oneline>`.

- Dropped the octopus part of the description of the `merge` command in
  the usage at the bottom of the todo list, as it is subject to change.

- The autosquash handling was not elegant, and cuddled into the same
  commit as the post-rewrite changes. Now, the autosquash handling is a
  lot more elegant, and a separate introductory patch (as it arguably
  improves the current code on its own).


Johannes Schindelin (11):
  sequencer: avoid using errno clobbered by rollback_lock_file()
  sequencer: make rearrange_squash() a bit more obvious
  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 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

 Documentation/config.txt               |   8 +
 Documentation/git-pull.txt             |   5 +-
 Documentation/git-rebase.txt           |  14 +-
 builtin/pull.c                         |  14 +-
 builtin/rebase--helper.c               |  13 +-
 builtin/remote.c                       |   2 +
 contrib/completion/git-completion.bash |   4 +-
 git-rebase--interactive.sh             |  22 +-
 git-rebase.sh                          |  16 +
 refs.c                                 |   3 +-
 sequencer.c                            | 736 ++++++++++++++++++++++++++++++++-
 sequencer.h                            |   7 +
 t/t3430-rebase-recreate-merges.sh      | 208 ++++++++++
 13 files changed, 1021 insertions(+), 31 deletions(-)
 create mode 100755 t/t3430-rebase-recreate-merges.sh


base-commit: 5be1f00a9a701532232f57958efab4be8c959a29
Published-As: https://github.com/dscho/git/releases/tag/recreate-merges-v3
Fetch-It-Via: git fetch https://github.com/dscho/git recreate-merges-v3

Interdiff vs v2:
 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
 index 5e21e4cf269..e199fe1cca5 100644
 --- a/git-rebase--interactive.sh
 +++ b/git-rebase--interactive.sh
 @@ -164,10 +164,10 @@ 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
 -m, merge <original-merge-commit> ( <label> | \"<label>...\" ) [<oneline>]
 +m, merge [-C <commit> | -c <commit>] <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.
 +.       message (or the oneline, if no original merge commit was
 +.       specified). Use -c <commit> to reword the commit 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 cd2f2ae5d53..c877432d7b4 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -123,7 +123,7 @@ static GIT_PATH_FUNC(rebase_path_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.
 + * finishes. This is used by the `label` command to record the need for cleanup.
   */
  static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
  
 @@ -206,18 +206,33 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
  
  int sequencer_remove_state(struct replay_opts *opts)
  {
 -	struct strbuf dir = STRBUF_INIT;
 +	struct strbuf buf = STRBUF_INIT;
  	int i;
  
 +	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;
 +		}
 +	}
 +
  	free(opts->gpg_sign);
  	free(opts->strategy);
  	for (i = 0; i < opts->xopts_nr; i++)
  		free(opts->xopts[i]);
  	free(opts->xopts);
  
 -	strbuf_addstr(&dir, get_dir(opts));
 -	remove_dir_recursively(&dir, 0);
 -	strbuf_release(&dir);
 +	strbuf_reset(&buf);
 +	strbuf_addstr(&buf, get_dir(opts));
 +	remove_dir_recursively(&buf, 0);
 +	strbuf_release(&buf);
  
  	return 0;
  }
 @@ -307,12 +322,14 @@ static int write_message(const void *buf, size_t len, const char *filename,
  	if (msg_fd < 0)
  		return error_errno(_("could not lock '%s'"), filename);
  	if (write_in_full(msg_fd, buf, len) < 0) {
 +		error_errno(_("could not write to '%s'"), filename);
  		rollback_lock_file(&msg_file);
 -		return error_errno(_("could not write to '%s'"), filename);
 +		return -1;
  	}
  	if (append_eol && write(msg_fd, "\n", 1) < 0) {
 +		error_errno(_("could not write eol to '%s'"), filename);
  		rollback_lock_file(&msg_file);
 -		return error_errno(_("could not write eol to '%s'"), filename);
 +		return -1;
  	}
  	if (commit_lock_file(&msg_file) < 0) {
  		rollback_lock_file(&msg_file);
 @@ -781,6 +798,7 @@ enum todo_command {
  	TODO_LABEL,
  	TODO_RESET,
  	TODO_MERGE,
 +	TODO_MERGE_AND_EDIT,
  	/* commands that do nothing but are counted for reporting progress */
  	TODO_NOOP,
  	TODO_DROP,
 @@ -802,6 +820,7 @@ static struct {
  	{ 'l', "label" },
  	{ 't', "reset" },
  	{ 'm', "merge" },
 +	{ 0, "merge" }, /* MERGE_AND_EDIT */
  	{ 0,   "noop" },
  	{ 'd', "drop" },
  	{ 0,   NULL }
 @@ -1270,8 +1289,7 @@ 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 == eol || bol[1] == ' ') &&
 -			   *bol == todo_command_info[i].c) {
 +		} else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
  			bol++;
  			item->command = i;
  			break;
 @@ -1305,21 +1323,30 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
  		return 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);
 -
 -	if (item->command == TODO_MERGE && *bol == '-' &&
 -	    bol + 1 == end_of_object_name) {
 -		item->commit = NULL;
 -		return 0;
 +	if (item->command == TODO_MERGE) {
 +		if (skip_prefix(bol, "-C", &bol))
 +			bol += strspn(bol, " \t");
 +		else if (skip_prefix(bol, "-c", &bol)) {
 +			bol += strspn(bol, " \t");
 +			item->command = TODO_MERGE_AND_EDIT;
 +		} else {
 +			item->command = TODO_MERGE_AND_EDIT;
 +			item->commit = NULL;
 +			item->arg = bol;
 +			item->arg_len = (int)(eol - bol);
 +			return 0;
 +		}
  	}
  
 +	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
  	saved = *end_of_object_name;
  	*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;
  
 @@ -1609,16 +1636,17 @@ static int save_head(const char *head)
  
  	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
  	if (fd < 0) {
 +		error_errno(_("could not lock HEAD"));
  		rollback_lock_file(&head_lock);
 -		return error_errno(_("could not lock HEAD"));
 +		return -1;
  	}
  	strbuf_addf(&buf, "%s\n", head);
  	written = write_in_full(fd, buf.buf, buf.len);
  	strbuf_release(&buf);
  	if (written < 0) {
 +		error_errno(_("could not write to '%s'"), git_path_head_file());
  		rollback_lock_file(&head_lock);
 -		return error_errno(_("could not write to '%s'"),
 -				   git_path_head_file());
 +		return -1;
  	}
  	if (commit_lock_file(&head_lock) < 0) {
  		rollback_lock_file(&head_lock);
 @@ -1948,11 +1976,12 @@ 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);
 +	int fd = hold_lock_file_for_update(&lock, filename,
 +					   LOCK_REPORT_ON_ERROR);
  	struct strbuf buf = STRBUF_INIT;
  
  	if (fd < 0)
 -		return error_errno(_("could not lock '%s'"), filename);
 +		return -1;
  
  	if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT)
  		return error_errno(_("could not read '%s'"), filename);
 @@ -1962,8 +1991,9 @@ static int safe_append(const char *filename, const char *fmt, ...)
  	va_end(ap);
  
  	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 +		error_errno(_("could not write to '%s'"), filename);
  		rollback_lock_file(&lock);
 -		return error_errno(_("could not write to '%s'"), filename);
 +		return -1;
  	}
  	if (commit_lock_file(&lock) < 0) {
  		rollback_lock_file(&lock);
 @@ -1982,6 +2012,9 @@ static int do_label(const char *name, int len)
  	int ret = 0;
  	struct object_id head_oid;
  
 +	if (len == 1 && *name == '#')
 +		return error("Illegal label name: '%.*s'", len, name);
 +
  	strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
  	strbuf_addf(&msg, "rebase -i (label) '%.*s'", len, name);
  
 @@ -2010,14 +2043,14 @@ static int do_label(const char *name, int len)
  	return ret;
  }
  
 -static int do_reset(const char *name, int len)
 +static int do_reset(const char *name, int len, struct replay_opts *opts)
  {
  	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;
 +	struct unpack_trees_options unpack_tree_opts;
  	int ret = 0, i;
  
  	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
 @@ -2037,16 +2070,18 @@ static int do_reset(const char *name, int len)
  		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;
 +	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
 +	unpack_tree_opts.head_idx = 1;
 +	unpack_tree_opts.src_index = &the_index;
 +	unpack_tree_opts.dst_index = &the_index;
 +	unpack_tree_opts.fn = oneway_merge;
 +	unpack_tree_opts.merge = 1;
 +	unpack_tree_opts.update = 1;
 +	unpack_tree_opts.reset = 1;
 +
 +	if (read_cache_unmerged())
 +		return error_resolve_conflict(_(action_name(opts)));
  
 -	read_cache_unmerged();
  	if (!fill_tree_descriptor(&desc, &oid)) {
  		error(_("failed to find tree of %s"), oid_to_hex(&oid));
  		rollback_lock_file(&lock);
 @@ -2055,7 +2090,7 @@ static int do_reset(const char *name, int len)
  		return -1;
  	}
  
 -	if (unpack_trees(1, &desc, &opts)) {
 +	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
  		rollback_lock_file(&lock);
  		free((void *)desc.buffer);
  		strbuf_release(&ref_name);
 @@ -2083,7 +2118,7 @@ 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)
 +		    int run_commit_flags, struct replay_opts *opts)
  {
  	int merge_arg_len;
  	struct strbuf ref_name = STRBUF_INIT;
 @@ -2137,6 +2172,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
  		strbuf_reset(&buf);
  
  		p += strspn(p, " \t");
 +		if (*p == '#' && isspace(p[1]))
 +			p += 1 + strspn(p + 1, " \t");
  		if (*p)
  			len = strlen(p);
  		else {
 @@ -2221,7 +2258,7 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
  	}
  	rollback_lock_file(&lock);
  
 -	ret = run_git_commit(git_path_merge_msg(), opts, 0);
 +	ret = run_git_commit(git_path_merge_msg(), opts, run_commit_flags);
  	strbuf_release(&ref_name);
  
  	return ret;
 @@ -2413,10 +2450,12 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
  		} 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_MERGE) {
 -			res = do_merge(item->commit,
 -				       item->arg, item->arg_len, opts);
 +			res = do_reset(item->arg, item->arg_len, opts);
 +		else if (item->command == TODO_MERGE ||
 +			 item->command == TODO_MERGE_AND_EDIT) {
 +			res = do_merge(item->commit, item->arg, item->arg_len,
 +				       item->command == TODO_MERGE_AND_EDIT ?
 +				       EDIT_MSG | VERIFY_MSG : 0, opts);
  			if (item->commit)
  				record_in_rewritten(&item->commit->object.oid,
  						    peek_command(todo_list, 1));
 @@ -2525,23 +2564,6 @@ 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);
  
 @@ -2867,11 +2889,14 @@ static const char *label_oid(struct object_id *oid, const char *label,
  		}
  	} else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
  		    !get_oid_hex(label, &dummy)) ||
 +		   (len == 1 && *label == '#') ||
  		   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.
 +		 * OID, or the label is a '#' (which we use as a separator
 +		 * between merge heads and oneline), we append a dash and a
 +		 * number to make it unique.
  		 */
  		struct strbuf *buf = &state->buf;
  
 @@ -2997,7 +3022,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
  				*(char *)p1 = '-';
  
  		strbuf_reset(&buf);
 -		strbuf_addf(&buf, "%s %s",
 +		strbuf_addf(&buf, "%s -C %s",
  			    cmd_merge, oid_to_hex(&commit->object.oid));
  
  		/* label the tip of merged branch */
 @@ -3012,7 +3037,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
  
  			strbuf_addstr(&buf, label_oid(oid, label.buf, &state));
  		}
 -		strbuf_addf(&buf, " %s", oneline.buf);
 +		strbuf_addf(&buf, " # %s", oneline.buf);
  
  		FLEX_ALLOC_STR(entry, string, buf.buf);
  		oidcpy(&entry->entry.oid, &commit->object.oid);
 @@ -3262,9 +3287,14 @@ int transform_todos(unsigned flags)
  					  short_commit_name(item->commit) :
  					  oid_to_hex(&item->commit->object.oid);
  
 +			if (item->command == TODO_MERGE)
 +				strbuf_addstr(&buf, " -C");
 +			else if (item->command == TODO_MERGE_AND_EDIT)
 +				strbuf_addstr(&buf, " -c");
 +
  			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');
 @@ -3567,8 +3597,7 @@ int rearrange_squash(void)
  		struct subject2item_entry *entry;
  
  		next[i] = tail[i] = -1;
 -		if (item->command >= TODO_EXEC &&
 -		    (item->command != TODO_MERGE || !item->commit)) {
 +		if (!item->commit || item->command == TODO_DROP) {
  			subjects[i] = NULL;
  			continue;
  		}
 diff --git a/t/t3430-rebase-recreate-merges.sh b/t/t3430-rebase-recreate-merges.sh
 index ab51b584ff9..9a59f12b670 100755
 --- a/t/t3430-rebase-recreate-merges.sh
 +++ b/t/t3430-rebase-recreate-merges.sh
 @@ -59,8 +59,8 @@ pick B
  label second
  
  reset onto
 -merge H second
 -merge - onebranch Merge the topic branch 'onebranch'
 +merge -C H second
 +merge onebranch # Merge the topic branch 'onebranch'
  EOF
  
  test_cmp_graph () {
 @@ -106,8 +106,8 @@ test_expect_success 'generate correct todo list' '
  
  	reset branch-point # C
  	pick 12bd07b D
 -	merge 2051b56 E E
 -	merge 233d48a H H
 +	merge -C 2051b56 E # E
 +	merge -C 233d48a H # H
  
  	EOF
  
-- 
2.16.1.windows.1