Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

["Philip Oakley" <philipoakley@iee.org>]

From: "Johannes Schindelin" <johannes.schindelin@gmx.de>
> 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` and `reset` commands to label

The previous patch was [Patch 05/16] git-rebase--interactive: clarify 
arguments, so this statement doesn't appear to be true. Has a patch been 
missed or re-ordered? Or should it be simply "This patch implements" ? 
Likewise the patch subject would be updated.

> commits and to reset to labeled commits. This patch adds the `merge`

s/adds/also adds/ ?

> command, with the following syntax:
>
> merge [-C <commit>] <rev> # <oneline>
>
> The <commit> parameter in this instance is the *original* merge commit,
> whose author and message will be used for the merge commit that is about
> to be created.
>
> The <rev> parameter refers to the (possibly rewritten) revision to
> merge. Let's see an example of a todo list:
>
The example ought to also note that `label onto` is to
`# label current HEAD with a name`, seeing as this is the first occurance.
It may be obvious in retrospect, but not at first reading.

> label onto
>
> # Branch abc
> reset onto

Is this reset strictly necessary. We are already there @head.

> pick deadbeef Hello, world!
> label abc
>
> reset onto
> pick cafecafe And now for something completely different
> merge -C baaabaaa abc # Merge the branch 'abc' into master
>
> To edit the merge commit's message (a "reword" for merges, if you will),
> use `-c` (lower-case) instead of `-C`; this convention was borrowed from
> `git commit` that also supports `-c` and `-C` with similar meanings.
>
> To create *new* merges, i.e. without copying the commit message from an
> existing commit, simply omit the `-C <commit>` parameter (which will
> open an editor for the merge message):
>
> merge abc
>
> 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 a separate
> patch series, support for merges using strategies other than the
> recursive merge is left for the future.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> git-rebase--interactive.sh |   6 +
> sequencer.c                | 407 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 406 insertions(+), 7 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index e1b865f43f2..ccd5254d1c9 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -162,6 +162,12 @@ 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
> +m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
> +.       create a merge commit using the original merge commit's
> +.       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 01443e0f245..35fcacbdf0f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -23,6 +23,8 @@
> #include "hashmap.h"
> #include "notes-utils.h"
> #include "sigchain.h"
> +#include "unpack-trees.h"
> +#include "worktree.h"
>
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> @@ -120,6 +122,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 `label` command to record the need for 
> cleanup.
> + */
> +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).
> @@ -244,18 +253,34 @@ 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 (is_rebase_i(opts) &&
> +     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;
> }
> @@ -1279,6 +1304,9 @@ enum todo_command {
>  TODO_SQUASH,
>  /* commands that do something else than handling a single commit */
>  TODO_EXEC,
> + TODO_LABEL,
> + TODO_RESET,
> + TODO_MERGE,
>  /* commands that do nothing but are counted for reporting progress */
>  TODO_NOOP,
>  TODO_DROP,
> @@ -1297,6 +1325,9 @@ static struct {
>  { 'f', "fixup" },
>  { 's', "squash" },
>  { 'x', "exec" },
> + { 'l', "label" },
> + { 't', "reset" },
> + { 'm', "merge" },
>  { 0,   "noop" },
>  { 'd', "drop" },
>  { 0,   NULL }
> @@ -1724,9 +1755,14 @@ static int read_and_refresh_cache(struct 
> replay_opts *opts)
>  return 0;
> }
>
> +enum todo_item_flags {
> + TODO_EDIT_MERGE_MSG = 1
> +};
> +
> struct todo_item {
>  enum todo_command command;
>  struct commit *commit;
> + unsigned int flags;
>  const char *arg;
>  int arg_len;
>  size_t offset_in_buf;
> @@ -1761,6 +1797,8 @@ static int parse_insn_line(struct todo_item *item, 
> const char *bol, char *eol)
>  char *end_of_object_name;
>  int i, saved, status, padding;
>
> + item->flags = 0;
> +
>  /* left-trim */
>  bol += strspn(bol, " \t");
>
> @@ -1802,13 +1840,29 @@ 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);
>  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->flags |= TODO_EDIT_MERGE_MSG;
> + } else {
> + item->flags |= TODO_EDIT_MERGE_MSG;
> + 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';
> @@ -2465,6 +2519,305 @@ 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,
> +    LOCK_REPORT_ON_ERROR);
> + struct strbuf buf = STRBUF_INIT;
> +
> + if (fd < 0)
> + return -1;
> +
> + if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT) {
> + error_errno(_("could not read '%s'"), filename);
> + rollback_lock_file(&lock);
> + return -1;
> + }
> + 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) {
> + error_errno(_("could not write to '%s'"), filename);
> + strbuf_release(&buf);
> + rollback_lock_file(&lock);
> + return -1;
> + }
> + if (commit_lock_file(&lock) < 0) {
> + strbuf_release(&buf);
> + rollback_lock_file(&lock);
> + return error(_("failed to finalize '%s'"), filename);
> + }
> +
> + strbuf_release(&buf);
> + 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;
> +
> + 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);
> +
> + 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 const char *reflog_message(struct replay_opts *opts,
> + const char *sub_action, const char *fmt, ...);
> +
> +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 unpack_tree_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(&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;
> +
> + if (read_cache_unmerged()) {
> + rollback_lock_file(&lock);
> + strbuf_release(&ref_name);
> + 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;
> + }
> +
> + if (unpack_trees(1, &desc, &unpack_tree_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)
> + ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
> + len, name), "HEAD", &oid,
> + NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +
> + strbuf_release(&ref_name);
> + return ret;
> +}
> +
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> +     int flags, struct replay_opts *opts)
> +{
> + int run_commit_flags = (flags & TODO_EDIT_MERGE_MSG) ?
> + EDIT_MSG | VERIFY_MSG : 0;
> + struct strbuf ref_name = STRBUF_INIT;
> + struct commit *head_commit, *merge_commit, *i;
> + struct commit_list *bases, *j, *reversed = NULL;
> + struct merge_options o;
> + int merge_arg_len, oneline_offset, ret;
> + static struct lock_file lock;
> + const char *p;
> +
> + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> + ret = -1;
> + goto leave_merge;
> + }
> +
> + head_commit = lookup_commit_reference_by_name("HEAD");
> + if (!head_commit) {
> + ret = error(_("cannot merge without a current revision"));
> + goto leave_merge;
> + }
> +
> + oneline_offset = arg_len;
> + merge_arg_len = strcspn(arg, " \t\n");
> + p = arg + merge_arg_len;
> + p += strspn(p, " \t\n");
> + if (*p == '#' && (!p[1] || isspace(p[1]))) {
> + p += 1 + strspn(p + 1, " \t\n");
> + oneline_offset = p - arg;
> + } else if (p - arg < arg_len)
> + BUG("octopus merges are not supported yet: '%s'", p);
> +
> + 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) {
> + ret = error(_("could not resolve '%s'"), ref_name.buf);
> + goto leave_merge;
> + }
> +
> + if (commit) {
> + const char *message = get_commit_buffer(commit, NULL);
> + const char *body;
> + int len;
> +
> + if (!message) {
> + ret = error(_("could not get commit message of '%s'"),
> +     oid_to_hex(&commit->object.oid));
> + goto leave_merge;
> + }
> + write_author_script(message);
> + find_commit_subject(message, &body);
> + len = strlen(body);
> + ret = write_message(body, len, git_path_merge_msg(), 0);
> + unuse_commit_buffer(commit, message);
> + if (ret) {
> + error_errno(_("could not write '%s'"),
> +     git_path_merge_msg());
> + goto leave_merge;
> + }
> + } else {
> + struct strbuf buf = STRBUF_INIT;
> + int len;
> +
> + strbuf_addf(&buf, "author %s", git_author_info(0));
> + write_author_script(buf.buf);
> + strbuf_reset(&buf);
> +
> + if (oneline_offset < arg_len) {
> + p = arg + oneline_offset;
> + len = arg_len - oneline_offset;
> + } else {
> + strbuf_addf(&buf, "Merge branch '%.*s'",
> +     merge_arg_len, arg);
> + p = buf.buf;
> + len = buf.len;
> + }
> +
> + ret = write_message(p, len, git_path_merge_msg(), 0);
> + strbuf_release(&buf);
> + if (ret) {
> + error_errno(_("could not write '%s'"),
> +     git_path_merge_msg());
> + goto leave_merge;
> + }
> + }
> +
> + 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);
> +
> + bases = get_merge_bases(head_commit, merge_commit);
> + for (j = bases; j; j = j->next)
> + commit_list_insert(j->item, &reversed);
> + free_commit_list(bases);
> +
> + 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) {
> + error(_("could not even attempt to merge '%.*s'"),
> +       merge_arg_len, arg);
> + goto leave_merge;
> + }
> + /*
> + * The return value of merge_recursive() is 1 on clean, and 0 on
> + * unclean merge.
> + *
> + * Let's reverse that, so that do_merge() returns 0 upon success and
> + * 1 upon failed merge (keeping the return value -1 for the cases where
> + * we will want to reschedule the `merge` command).
> + */
> + ret = !ret;
> +
> + if (active_cache_changed &&
> +     write_locked_index(&the_index, &lock, COMMIT_LOCK)) {
> + ret = error(_("merge: Unable to write new index file"));
> + goto leave_merge;
> + }
> +
> + rollback_lock_file(&lock);
> + if (ret)
> + rerere(opts->allow_rerere_auto);
> + else
> + ret = run_git_commit(git_path_merge_msg(), opts,
> +      run_commit_flags);
> +
> +leave_merge:
> + strbuf_release(&ref_name);
> + rollback_lock_file(&lock);
> + return ret;
> +}
> +
> static int is_final_fixup(struct todo_list *todo_list)
> {
>  int i = todo_list->current;
> @@ -2568,7 +2921,7 @@ N_("Could not execute the todo command\n"
>
> static int pick_commits(struct todo_list *todo_list, struct replay_opts 
> *opts)
> {
> - int res = 0;
> + int res = 0, reschedule = 0;
>
>  setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>  if (opts->allow_ff)
> @@ -2639,7 +2992,7 @@ static int pick_commits(struct todo_list *todo_list, 
> struct replay_opts *opts)
>  intend_to_amend();
>  return error_failed_squash(item->commit, opts,
>  item->arg_len, item->arg);
> - } else if (res && is_rebase_i(opts))
> + } else if (res && is_rebase_i(opts) && item->commit)
>  return res | error_with_patch(item->commit,
>  item->arg, item->arg_len, opts, res,
>  item->command == TODO_REWORD);
> @@ -2665,9 +3018,41 @@ 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_LABEL) {
> + if ((res = do_label(item->arg, item->arg_len)))
> + reschedule = 1;
> + } else if (item->command == TODO_RESET) {
> + if ((res = do_reset(item->arg, item->arg_len, opts)))
> + reschedule = 1;
> + } else if (item->command == TODO_MERGE) {
> + if ((res = do_merge(item->commit,
> +     item->arg, item->arg_len,
> +     item->flags, opts)) < 0)
> + reschedule = 1;
> + else if (res > 0)
> + /* failed with merge conflicts */
> + return error_with_patch(item->commit,
> + item->arg,
> + item->arg_len, opts,
> + res, 0);
>  } else if (!is_noop(item->command))
>  return error(_("unknown command %d"), item->command);
>
> + if (reschedule) {
> + advise(_(rescheduled_advice),
> +        get_item_line_length(todo_list,
> +     todo_list->current),
> +        get_item_line(todo_list, todo_list->current));
> + todo_list->current--;
> + if (save_todo(todo_list, opts))
> + return -1;
> + if (item->commit)
> + return error_with_patch(item->commit,
> + item->arg,
> + item->arg_len, opts,
> + res, 0);
> + }
> +
>  todo_list->current++;
>  if (res)
>  return res;
> @@ -3147,8 +3532,16 @@ int transform_todos(unsigned flags)
>    short_commit_name(item->commit) :
>    oid_to_hex(&item->commit->object.oid);
>
> + if (item->command == TODO_MERGE) {
> + if (item->flags & TODO_EDIT_MERGE_MSG)
> + strbuf_addstr(&buf, " -c");
> + else
> + strbuf_addstr(&buf, " -C");
> + }
> +
>  strbuf_addf(&buf, " %s", oid);
>  }
> +
>  /* add all the rest */
>  if (!item->arg_len)
>  strbuf_addch(&buf, '\n');
> -- 
> 2.17.0.windows.1.15.gaa56ade3205
>
>
>