[PATCH 0/5] Sequencer: working around historical mistakes

STALE3715d

17 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH 0/5] Sequencer: working around historical mistakes

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:23

Hi,

As described in the discussion following $gmane/179304/focus=179383,
we have decided to handle historical hacks in the sequencer itself.
This series that follows is one step in the right direction.

- Part 1/5 makes the gigantic move required to create the sequencer.
If you need an excuse to celebrate, wait till this gets merged :)
- Part 5/5 can be considered as the "ultimate objective" of the
series.  I first wrote this part, and then wrote the other parts to
make tests pass.
- Parts 3/5 and 4/5 are ugly!  Causes heartburn.

Immediate shortcomings of this iteration:
1. No tests yet.  I want to see if it's possible to make this less
ugly first.
2. This series depends on rr/revert-cherry-pick, but doesn't apply to
the current 'next'- sorry, rebasing is a massive pita due to 1/5.

Thanks for reading.

-- Ram

Ramkumar Ramachandra (5):
  sequencer: factor code out of revert builtin
  sequencer: remove CHERRY_PICK_HEAD with sequencer state
  sequencer: sequencer state is useless without todo
  sequencer: handle single commit pick separately
  sequencer: revert d3f4628e

 builtin/revert.c                |  821 +--------------------------------------
 sequencer.c                     |  832 ++++++++++++++++++++++++++++++++++++++-
 sequencer.h                     |   26 ++
 t/t3510-cherry-pick-sequence.sh |   24 --
 4 files changed, 847 insertions(+), 856 deletions(-)

-- 
1.7.6.351.gb35ac.dirty

[PATCH 2/5] sequencer: remove CHERRY_PICK_HEAD with sequencer state

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:23

Make remove_sequencer_state() remove '.git/CHERRY_PICK_HEAD' when
invoked aggressively, since we want to treat it as part of the
sequencer state now.  While at it, make some minor improvements to the
function.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 sequencer.c |   27 ++++++++++++++++-----------
 1 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 87f146b..e566043 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -25,17 +25,22 @@ static char *get_encoding(const char *message);
 
 void remove_sequencer_state(int aggressive)
 {
-	struct strbuf seq_dir = STRBUF_INIT;
-	struct strbuf seq_old_dir = STRBUF_INIT;
-
-	strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
-	strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR));
-	remove_dir_recursively(&seq_old_dir, 0);
-	rename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));
-	if (aggressive)
-		remove_dir_recursively(&seq_old_dir, 0);
-	strbuf_release(&seq_dir);
-	strbuf_release(&seq_old_dir);
+	const char *seq_dir = git_path(SEQ_DIR);
+	const char *seq_old_dir = git_path(SEQ_OLD_DIR);
+	const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");
+	struct strbuf seq_dir_buf = STRBUF_INIT;
+	struct strbuf seq_old_dir_buf = STRBUF_INIT;
+
+	strbuf_addf(&seq_dir_buf, "%s", seq_dir);
+	strbuf_addf(&seq_old_dir_buf, "%s", seq_old_dir);
+	remove_dir_recursively(&seq_old_dir_buf, 0);
+	rename(seq_dir, seq_old_dir);
+	if (aggressive) {
+		remove_dir_recursively(&seq_old_dir_buf, 0);
+		unlink(cherry_pick_head);
+	}
+	strbuf_release(&seq_dir_buf);
+	strbuf_release(&seq_old_dir_buf);
 }
 
 struct commit_message {
-- 
1.7.6.351.gb35ac.dirty

[PATCH 1/5] sequencer: factor code out of revert builtin

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:23

Start building the generalized sequencer by moving code from revert.c
into sequencer.c and sequencer.h.  Make the builtin responsible only
for command-line parsing, and expose a new sequencer_pick_revisions()
to do the actual work of sequencing commits.

This is intended to be almost a pure code movement patch with no
functional changes.  Check with:

  $ git blame -s -CCC HEAD^..HEAD -- sequencer.c | grep -C3 '^[^^]'

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 builtin/revert.c |  821 +-----------------------------------------------------
 sequencer.c      |  802 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 sequencer.h      |   26 ++
 3 files changed, 828 insertions(+), 821 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index df9459b..c272920 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1,19 +1,9 @@
 #include "cache.h"
 #include "builtin.h"
-#include "object.h"
-#include "commit.h"
-#include "tag.h"
-#include "run-command.h"
-#include "exec_cmd.h"
-#include "utf8.h"
 #include "parse-options.h"
-#include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
 #include "rerere.h"
-#include "merge-recursive.h"
-#include "refs.h"
-#include "dir.h"
 #include "sequencer.h"
 
 /*
@@ -39,40 +29,11 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
-
-struct replay_opts {
-	enum replay_action action;
-	enum replay_subcommand subcommand;
-
-	/* Boolean options */
-	int edit;
-	int record_origin;
-	int no_commit;
-	int signoff;
-	int allow_ff;
-	int allow_rerere_auto;
-
-	int mainline;
-
-	/* Merge strategy */
-	const char *strategy;
-	const char **xopts;
-	size_t xopts_nr, xopts_alloc;
-
-	/* Only used by REPLAY_NONE */
-	struct rev_info *revs;
-};
-
-#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
-
 static const char *action_name(const struct replay_opts *opts)
 {
 	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
 }
 
-static char *get_encoding(const char *message);
-
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
 	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
@@ -222,784 +183,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		usage_with_options(usage_str, options);
 }
 
-struct commit_message {
-	char *parent_label;
-	const char *label;
-	const char *subject;
-	char *reencoded_message;
-	const char *message;
-};
-
-static int get_message(struct commit *commit, struct commit_message *out)
-{
-	const char *encoding;
-	const char *abbrev, *subject;
-	int abbrev_len, subject_len;
-	char *q;
-
-	if (!commit->buffer)
-		return -1;
-	encoding = get_encoding(commit->buffer);
-	if (!encoding)
-		encoding = "UTF-8";
-	if (!git_commit_encoding)
-		git_commit_encoding = "UTF-8";
-
-	out->reencoded_message = NULL;
-	out->message = commit->buffer;
-	if (strcmp(encoding, git_commit_encoding))
-		out->reencoded_message = reencode_string(commit->buffer,
-					git_commit_encoding, encoding);
-	if (out->reencoded_message)
-		out->message = out->reencoded_message;
-
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
-
-	subject_len = find_commit_subject(out->message, &subject);
-
-	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
-			      strlen("... ") + subject_len + 1);
-	q = out->parent_label;
-	q = mempcpy(q, "parent of ", strlen("parent of "));
-	out->label = q;
-	q = mempcpy(q, abbrev, abbrev_len);
-	q = mempcpy(q, "... ", strlen("... "));
-	out->subject = q;
-	q = mempcpy(q, subject, subject_len);
-	*q = '\0';
-	return 0;
-}
-
-static void free_message(struct commit_message *msg)
-{
-	free(msg->parent_label);
-	free(msg->reencoded_message);
-}
-
-static char *get_encoding(const char *message)
-{
-	const char *p = message, *eol;
-
-	while (*p && *p != '\n') {
-		for (eol = p + 1; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
-		if (!prefixcmp(p, "encoding ")) {
-			char *result = xmalloc(eol - 8 - p);
-			strlcpy(result, p + 9, eol - 8 - p);
-			return result;
-		}
-		p = eol;
-		if (*p == '\n')
-			p++;
-	}
-	return NULL;
-}
-
-static void write_cherry_pick_head(struct commit *commit)
-{
-	int fd;
-	struct strbuf buf = STRBUF_INIT;
-
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
-
-	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"),
-			  git_path("CHERRY_PICK_HEAD"));
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
-	strbuf_release(&buf);
-}
-
-static void print_advice(int show_hint)
-{
-	char *msg = getenv("GIT_CHERRY_PICK_HELP");
-
-	if (msg) {
-		fprintf(stderr, "%s\n", msg);
-		/*
-		 * A conflict has occured but the porcelain
-		 * (typically rebase --interactive) wants to take care
-		 * of the commit itself so remove CHERRY_PICK_HEAD
-		 */
-		unlink(git_path("CHERRY_PICK_HEAD"));
-		return;
-	}
-
-	if (show_hint) {
-		advise("after resolving the conflicts, mark the corrected paths");
-		advise("with 'git add <paths>' or 'git rm <paths>'");
-		advise("and commit the result with 'git commit'");
-	}
-}
-
-static void write_message(struct strbuf *msgbuf, const char *filename)
-{
-	static struct lock_file msg_file;
-
-	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
-					       LOCK_DIE_ON_ERROR);
-	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		die_errno(_("Could not write to %s."), filename);
-	strbuf_release(msgbuf);
-	if (commit_lock_file(&msg_file) < 0)
-		die(_("Error wrapping up %s"), filename);
-}
-
-static struct tree *empty_tree(void)
-{
-	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
-}
-
-static int error_dirty_index(struct replay_opts *opts)
-{
-	if (read_cache_unmerged())
-		return error_resolve_conflict(action_name(opts));
-
-	/* Different translation strings for cherry-pick and revert */
-	if (opts->action == REPLAY_PICK)
-		error(_("Your local changes would be overwritten by cherry-pick."));
-	else
-		error(_("Your local changes would be overwritten by revert."));
-
-	if (advice_commit_before_merge)
-		advise(_("Commit your changes or stash them to proceed."));
-	return -1;
-}
-
-static int fast_forward_to(const unsigned char *to, const unsigned char *from)
-{
-	struct ref_lock *ref_lock;
-
-	read_cache();
-	if (checkout_fast_forward(from, to))
-		exit(1); /* the callee should have complained already */
-	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
-	return write_ref_sha1(ref_lock, to, "cherry-pick");
-}
-
-static int do_recursive_merge(struct commit *base, struct commit *next,
-			      const char *base_label, const char *next_label,
-			      unsigned char *head, struct strbuf *msgbuf,
-			      struct replay_opts *opts)
-{
-	struct merge_options o;
-	struct tree *result, *next_tree, *base_tree, *head_tree;
-	int clean, index_fd;
-	const char **xopt;
-	static struct lock_file index_lock;
-
-	index_fd = hold_locked_index(&index_lock, 1);
-
-	read_cache();
-
-	init_merge_options(&o);
-	o.ancestor = base ? base_label : "(empty tree)";
-	o.branch1 = "HEAD";
-	o.branch2 = next ? next_label : "(empty tree)";
-
-	head_tree = parse_tree_indirect(head);
-	next_tree = next ? next->tree : empty_tree();
-	base_tree = base ? base->tree : empty_tree();
-
-	for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
-		parse_merge_opt(&o, *xopt);
-
-	clean = merge_trees(&o,
-			    head_tree,
-			    next_tree, base_tree, &result);
-
-	if (active_cache_changed &&
-	    (write_cache(index_fd, active_cache, active_nr) ||
-	     commit_locked_index(&index_lock)))
-		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), action_name(opts));
-	rollback_lock_file(&index_lock);
-
-	if (!clean) {
-		int i;
-		strbuf_addstr(msgbuf, "\nConflicts:\n\n");
-		for (i = 0; i < active_nr;) {
-			struct cache_entry *ce = active_cache[i++];
-			if (ce_stage(ce)) {
-				strbuf_addch(msgbuf, '\t');
-				strbuf_addstr(msgbuf, ce->name);
-				strbuf_addch(msgbuf, '\n');
-				while (i < active_nr && !strcmp(ce->name,
-						active_cache[i]->name))
-					i++;
-			}
-		}
-	}
-
-	return !clean;
-}
-
-/*
- * If we are cherry-pick, and if the merge did not result in
- * hand-editing, we will hit this commit and inherit the original
- * author date and name.
- * If we are revert, or if our cherry-pick results in a hand merge,
- * we had better say that the current user is responsible for that.
- */
-static int run_git_commit(const char *defmsg, struct replay_opts *opts)
-{
-	/* 6 is max possible length of our args array including NULL */
-	const char *args[6];
-	int i = 0;
-
-	args[i++] = "commit";
-	args[i++] = "-n";
-	if (opts->signoff)
-		args[i++] = "-s";
-	if (!opts->edit) {
-		args[i++] = "-F";
-		args[i++] = defmsg;
-	}
-	args[i] = NULL;
-
-	return run_command_v_opt(args, RUN_GIT_CMD);
-}
-
-static int do_pick_commit(struct commit *commit, enum replay_action action,
-			struct replay_opts *opts)
-{
-	unsigned char head[20];
-	struct commit *base, *next, *parent;
-	const char *base_label, *next_label;
-	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
-	char *defmsg = NULL;
-	struct strbuf msgbuf = STRBUF_INIT;
-	int res;
-
-	if (opts->no_commit) {
-		/*
-		 * We do not intend to commit immediately.  We just want to
-		 * merge the differences in, so let's compute the tree
-		 * that represents the "current" state for merge-recursive
-		 * to work on.
-		 */
-		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
-	} else {
-		if (get_sha1("HEAD", head))
-			return error(_("You do not have a valid HEAD"));
-		if (index_differs_from("HEAD", 0))
-			return error_dirty_index(opts);
-	}
-	discard_cache();
-
-	if (!commit->parents) {
-		parent = NULL;
-	}
-	else if (commit->parents->next) {
-		/* Reverting or cherry-picking a merge commit */
-		int cnt;
-		struct commit_list *p;
-
-		if (!opts->mainline)
-			return error(_("Commit %s is a merge but no -m option was given."),
-				sha1_to_hex(commit->object.sha1));
-
-		for (cnt = 1, p = commit->parents;
-		     cnt != opts->mainline && p;
-		     cnt++)
-			p = p->next;
-		if (cnt != opts->mainline || !p)
-			return error(_("Commit %s does not have parent %d"),
-				sha1_to_hex(commit->object.sha1), opts->mainline);
-		parent = p->item;
-	} else if (0 < opts->mainline)
-		return error(_("Mainline was specified but commit %s is not a merge."),
-			sha1_to_hex(commit->object.sha1));
-	else
-		parent = commit->parents->item;
-
-	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
-		return fast_forward_to(commit->object.sha1, head);
-
-	if (parent && parse_commit(parent) < 0)
-		/* TRANSLATORS: The first %s will be "revert" or
-		   "cherry-pick", the second %s a SHA1 */
-		return error(_("%s: cannot parse parent commit %s"),
-			action_name(opts), sha1_to_hex(parent->object.sha1));
-
-	if (get_message(commit, &msg) != 0)
-		return error(_("Cannot get commit message for %s"),
-			sha1_to_hex(commit->object.sha1));
-
-	/*
-	 * "commit" is an existing commit.  We would want to apply
-	 * the difference it introduces since its first parent "prev"
-	 * on top of the current HEAD if we are cherry-pick.  Or the
-	 * reverse of it if we are revert.
-	 */
-
-	defmsg = git_pathdup("MERGE_MSG");
-
-	if (action == REPLAY_REVERT) {
-		base = commit;
-		base_label = msg.label;
-		next = parent;
-		next_label = msg.parent_label;
-		strbuf_addstr(&msgbuf, "Revert \"");
-		strbuf_addstr(&msgbuf, msg.subject);
-		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
-		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-
-		if (commit->parents && commit->parents->next) {
-			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
-			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
-		}
-		strbuf_addstr(&msgbuf, ".\n");
-	} else {
-		const char *p;
-
-		base = parent;
-		base_label = msg.parent_label;
-		next = commit;
-		next_label = msg.label;
-
-		/*
-		 * Append the commit log message to msgbuf; it starts
-		 * after the tree, parent, author, committer
-		 * information followed by "\n\n".
-		 */
-		p = strstr(msg.message, "\n\n");
-		if (p) {
-			p += 2;
-			strbuf_addstr(&msgbuf, p);
-		}
-
-		if (opts->record_origin) {
-			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
-			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-			strbuf_addstr(&msgbuf, ")\n");
-		}
-	}
-
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
-		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf, opts);
-		write_message(&msgbuf, defmsg);
-	} else {
-		struct commit_list *common = NULL;
-		struct commit_list *remotes = NULL;
-
-		write_message(&msgbuf, defmsg);
-
-		commit_list_insert(base, &common);
-		commit_list_insert(next, &remotes);
-		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
-					common, sha1_to_hex(head), remotes);
-		free_commit_list(common);
-		free_commit_list(remotes);
-	}
-
-	/*
-	 * If the merge was clean or if it failed due to conflict, we write
-	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
-	 * However, if the merge did not even start, then we don't want to
-	 * write it at all.
-	 */
-	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		write_cherry_pick_head(commit);
-
-	if (res) {
-		error(action == REPLAY_REVERT
-		      ? _("could not revert %s... %s")
-		      : _("could not apply %s... %s"),
-		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
-		      msg.subject);
-		print_advice(res == 1);
-		rerere(opts->allow_rerere_auto);
-	} else {
-		if (!opts->no_commit)
-			res = run_git_commit(defmsg, opts);
-	}
-
-	free_message(&msg);
-	free(defmsg);
-
-	return res;
-}
-
-static void prepare_revs(struct replay_opts *opts)
-{
-	if (opts->action != REPLAY_REVERT)
-		opts->revs->reverse ^= 1;
-
-	if (prepare_revision_walk(opts->revs))
-		die(_("revision walk setup failed"));
-
-	if (!opts->revs->commits)
-		die(_("empty commit set passed"));
-}
-
-static void read_and_refresh_cache(struct replay_opts *opts)
-{
-	static struct lock_file index_lock;
-	int index_fd = hold_locked_index(&index_lock, 0);
-	if (read_index_preload(&the_index, NULL) < 0)
-		die(_("git %s: failed to read the index"), action_name(opts));
-	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
-	if (the_index.cache_changed) {
-		if (write_index(&the_index, index_fd) ||
-		    commit_locked_index(&index_lock))
-			die(_("git %s: failed to refresh the index"), action_name(opts));
-	}
-	rollback_lock_file(&index_lock);
-}
-
-/*
- * Append a commit to the end of the commit_list.
- *
- * next starts by pointing to the variable that holds the head of an
- * empty commit_list, and is updated to point to the "next" field of
- * the last item on the list as new commits are appended.
- *
- * Usage example:
- *
- *     struct commit_list *list;
- *     struct commit_list **next = &list;
- *
- *     next = commit_list_append(c1, next);
- *     next = commit_list_append(c2, next);
- *     assert(commit_list_count(list) == 2);
- *     return list;
- */
-static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
-						struct commit *operand,
-						struct replay_insn_list **next)
-{
-	struct replay_insn_list *new = xmalloc(sizeof(*new));
-	new->action = action;
-	new->operand = operand;
-	*next = new;
-	new->next = NULL;
-	return &new->next;
-}
-
-static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
-{
-	struct replay_insn_list *cur;
-
-	for (cur = todo_list; cur; cur = cur->next) {
-		const char *sha1_abbrev, *action_str, *subject;
-		int subject_len;
-
-		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
-		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
-		subject_len = find_commit_subject(cur->operand->buffer, &subject);
-		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
-			subject_len, subject);
-	}
-	return 0;
-}
-
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
-{
-	unsigned char commit_sha1[20];
-	char *end_of_object_name;
-	int saved, status;
-
-	if (!prefixcmp(bol, "pick ")) {
-		item->action = REPLAY_PICK;
-		bol += strlen("pick ");
-	} else if (!prefixcmp(bol, "revert ")) {
-		item->action = REPLAY_REVERT;
-		bol += strlen("revert ");
-	} else {
-		size_t len = strchrnul(bol, '\n') - bol;
-		if (len > 255)
-			len = 255;
-		return error(_("Unrecognized action: %.*s"), (int)len, bol);
-	}
-
-	end_of_object_name = bol + strcspn(bol, " \n");
-	saved = *end_of_object_name;
-	*end_of_object_name = '\0';
-	status = get_sha1(bol, commit_sha1);
-	*end_of_object_name = saved;
-
-	if (status < 0)
-		return error(_("Malformed object name: %s"), bol);
-
-	item->operand = lookup_commit_reference(commit_sha1);
-	if (!item->operand)
-		return error(_("Not a valid commit: %s"), bol);
-
-	item->next = NULL;
-	return 0;
-}
-
-static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
-{
-	struct replay_insn_list **next = todo_list;
-	struct replay_insn_list item = {0, NULL, NULL};
-	char *p = buf;
-	int i;
-
-	for (i = 1; *p; i++) {
-		char *eol = strchrnul(p, '\n');
-		if (parse_insn_line(p, eol, &item) < 0)
-			return error(_("on line %d."), i);
-		next = replay_insn_list_append(item.action, item.operand, next);
-		p = *eol ? eol + 1 : eol;
-	}
-	if (!*todo_list)
-		return error(_("No commits parsed."));
-	return 0;
-}
-
-static void read_populate_todo(struct replay_insn_list **todo_list)
-{
-	const char *todo_file = git_path(SEQ_TODO_FILE);
-	struct strbuf buf = STRBUF_INIT;
-	int fd, res;
-
-	fd = open(todo_file, O_RDONLY);
-	if (fd < 0)
-		die_errno(_("Could not open %s."), todo_file);
-	if (strbuf_read(&buf, fd, 0) < 0) {
-		close(fd);
-		strbuf_release(&buf);
-		die(_("Could not read %s."), todo_file);
-	}
-	close(fd);
-
-	res = parse_insn_buffer(buf.buf, todo_list);
-	strbuf_release(&buf);
-	if (res)
-		die(_("Unusable instruction sheet: %s"), todo_file);
-}
-
-static int populate_opts_cb(const char *key, const char *value, void *data)
-{
-	struct replay_opts *opts = data;
-	int error_flag = 1;
-
-	if (!value)
-		error_flag = 0;
-	else if (!strcmp(key, "options.no-commit"))
-		opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.edit"))
-		opts->edit = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.signoff"))
-		opts->signoff = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.record-origin"))
-		opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.allow-ff"))
-		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.mainline"))
-		opts->mainline = git_config_int(key, value);
-	else if (!strcmp(key, "options.strategy"))
-		git_config_string(&opts->strategy, key, value);
-	else if (!strcmp(key, "options.strategy-option")) {
-		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-		opts->xopts[opts->xopts_nr++] = xstrdup(value);
-	} else
-		return error(_("Invalid key: %s"), key);
-
-	if (!error_flag)
-		return error(_("Invalid value for %s: %s"), key, value);
-
-	return 0;
-}
-
-static void read_populate_opts(struct replay_opts **opts_ptr)
-{
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
-
-	if (!file_exists(opts_file))
-		return;
-	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
-		die(_("Malformed options sheet: %s"), opts_file);
-}
-
-static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
-				struct replay_opts *opts)
-{
-	struct commit *commit;
-	struct replay_insn_list **next;
-
-	prepare_revs(opts);
-
-	next = todo_list;
-	while ((commit = get_revision(opts->revs)))
-		next = replay_insn_list_append(opts->action, commit, next);
-}
-
-static int create_seq_dir(void)
-{
-	const char *seq_dir = git_path(SEQ_DIR);
-
-	if (file_exists(seq_dir))
-		return error(_("%s already exists."), seq_dir);
-	else if (mkdir(seq_dir, 0777) < 0)
-		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
-	return 0;
-}
-
-static void save_head(const char *head)
-{
-	const char *head_file = git_path(SEQ_HEAD_FILE);
-	static struct lock_file head_lock;
-	struct strbuf buf = STRBUF_INIT;
-	int fd;
-
-	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
-	strbuf_addf(&buf, "%s\n", head);
-	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s."), head_file);
-	if (commit_lock_file(&head_lock) < 0)
-		die(_("Error wrapping up %s."), head_file);
-}
-
-static void save_todo(struct replay_insn_list *todo_list)
-{
-	const char *todo_file = git_path(SEQ_TODO_FILE);
-	static struct lock_file todo_lock;
-	struct strbuf buf = STRBUF_INIT;
-	int fd;
-
-	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list) < 0)
-		die(_("Could not format %s."), todo_file);
-	if (write_in_full(fd, buf.buf, buf.len) < 0) {
-		strbuf_release(&buf);
-		die_errno(_("Could not write to %s."), todo_file);
-	}
-	if (commit_lock_file(&todo_lock) < 0) {
-		strbuf_release(&buf);
-		die(_("Error wrapping up %s."), todo_file);
-	}
-	strbuf_release(&buf);
-}
-
-static void save_opts(struct replay_opts *opts)
-{
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
-
-	if (opts->no_commit)
-		git_config_set_in_file(opts_file, "options.no-commit", "true");
-	if (opts->edit)
-		git_config_set_in_file(opts_file, "options.edit", "true");
-	if (opts->signoff)
-		git_config_set_in_file(opts_file, "options.signoff", "true");
-	if (opts->record_origin)
-		git_config_set_in_file(opts_file, "options.record-origin", "true");
-	if (opts->allow_ff)
-		git_config_set_in_file(opts_file, "options.allow-ff", "true");
-	if (opts->mainline) {
-		struct strbuf buf = STRBUF_INIT;
-		strbuf_addf(&buf, "%d", opts->mainline);
-		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
-		strbuf_release(&buf);
-	}
-	if (opts->strategy)
-		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
-	if (opts->xopts) {
-		int i;
-		for (i = 0; i < opts->xopts_nr; i++)
-			git_config_set_multivar_in_file(opts_file,
-							"options.strategy-option",
-							opts->xopts[i], "^$", 0);
-	}
-}
-
-static int pick_commits(struct replay_insn_list *todo_list,
-			struct replay_opts *opts)
-{
-	struct replay_insn_list *cur;
-	int res;
-
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	if (opts->allow_ff)
-		assert(!(opts->signoff || opts->no_commit ||
-				opts->record_origin || opts->edit));
-	read_and_refresh_cache(opts);
-
-	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur);
-		res = do_pick_commit(cur->operand, cur->action, opts);
-		if (res) {
-			if (!cur->next)
-				/*
-				 * An error was encountered while
-				 * picking the last commit; the
-				 * sequencer state is useless now --
-				 * the user simply needs to resolve
-				 * the conflict and commit
-				 */
-				remove_sequencer_state(0);
-			return res;
-		}
-	}
-
-	/*
-	 * Sequence of picks finished successfully; cleanup by
-	 * removing the .git/sequencer directory
-	 */
-	remove_sequencer_state(1);
-	return 0;
-}
-
-static int pick_revisions(struct replay_opts *opts)
-{
-	struct replay_insn_list *todo_list = NULL;
-	unsigned char sha1[20];
-
-	if (opts->subcommand == REPLAY_NONE)
-		assert(opts->revs);
-
-	read_and_refresh_cache(opts);
-
-	/*
-	 * Decide what to do depending on the arguments; a fresh
-	 * cherry-pick should be handled differently from an existing
-	 * one that is being continued
-	 */
-	if (opts->subcommand == REPLAY_RESET) {
-		remove_sequencer_state(1);
-		return 0;
-	} else if (opts->subcommand == REPLAY_CONTINUE) {
-		if (!file_exists(git_path(SEQ_TODO_FILE)))
-			goto error;
-		read_populate_opts(&opts);
-		read_populate_todo(&todo_list);
-
-		/* Verify that the conflict has been resolved */
-		if (!index_differs_from("HEAD", 0))
-			todo_list = todo_list->next;
-	} else {
-		/*
-		 * Start a new cherry-pick/ revert sequence; but
-		 * first, make sure that an existing one isn't in
-		 * progress
-		 */
-
-		walk_revs_populate_todo(&todo_list, opts);
-		if (create_seq_dir() < 0) {
-			error(_("A cherry-pick or revert is in progress."));
-			advise(_("Use --continue to continue the operation"));
-			advise(_("or --reset to forget about it"));
-			return -1;
-		}
-		if (get_sha1("HEAD", sha1)) {
-			if (opts->action == REPLAY_REVERT)
-				return error(_("Can't revert as initial commit"));
-			return error(_("Can't cherry-pick into empty head"));
-		}
-		save_head(sha1_to_hex(sha1));
-		save_opts(opts);
-	}
-	return pick_commits(todo_list, opts);
-error:
-	return error(_("No %s in progress"), action_name(opts));
-}
-
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
@@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -1026,7 +209,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
diff --git a/sequencer.c b/sequencer.c
index bc2c046..87f146b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,7 +1,27 @@
 #include "cache.h"
-#include "sequencer.h"
-#include "strbuf.h"
+#include "object.h"
+#include "commit.h"
+#include "tag.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+#include "utf8.h"
+#include "cache-tree.h"
+#include "diff.h"
+#include "revision.h"
+#include "rerere.h"
+#include "merge-recursive.h"
+#include "refs.h"
 #include "dir.h"
+#include "sequencer.h"
+
+#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
+
+static const char *action_name(const struct replay_opts *opts)
+{
+	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
+}
+
+static char *get_encoding(const char *message);
 
 void remove_sequencer_state(int aggressive)
 {
@@ -17,3 +37,781 @@ void remove_sequencer_state(int aggressive)
 	strbuf_release(&seq_dir);
 	strbuf_release(&seq_old_dir);
 }
+
+struct commit_message {
+	char *parent_label;
+	const char *label;
+	const char *subject;
+	char *reencoded_message;
+	const char *message;
+};
+
+static int get_message(struct commit *commit, struct commit_message *out)
+{
+	const char *encoding;
+	const char *abbrev, *subject;
+	int abbrev_len, subject_len;
+	char *q;
+
+	if (!commit->buffer)
+		return -1;
+	encoding = get_encoding(commit->buffer);
+	if (!encoding)
+		encoding = "UTF-8";
+	if (!git_commit_encoding)
+		git_commit_encoding = "UTF-8";
+
+	out->reencoded_message = NULL;
+	out->message = commit->buffer;
+	if (strcmp(encoding, git_commit_encoding))
+		out->reencoded_message = reencode_string(commit->buffer,
+					git_commit_encoding, encoding);
+	if (out->reencoded_message)
+		out->message = out->reencoded_message;
+
+	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+	abbrev_len = strlen(abbrev);
+
+	subject_len = find_commit_subject(out->message, &subject);
+
+	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+			      strlen("... ") + subject_len + 1);
+	q = out->parent_label;
+	q = mempcpy(q, "parent of ", strlen("parent of "));
+	out->label = q;
+	q = mempcpy(q, abbrev, abbrev_len);
+	q = mempcpy(q, "... ", strlen("... "));
+	out->subject = q;
+	q = mempcpy(q, subject, subject_len);
+	*q = '\0';
+	return 0;
+}
+
+static void free_message(struct commit_message *msg)
+{
+	free(msg->parent_label);
+	free(msg->reencoded_message);
+}
+
+static char *get_encoding(const char *message)
+{
+	const char *p = message, *eol;
+
+	while (*p && *p != '\n') {
+		for (eol = p + 1; *eol && *eol != '\n'; eol++)
+			; /* do nothing */
+		if (!prefixcmp(p, "encoding ")) {
+			char *result = xmalloc(eol - 8 - p);
+			strlcpy(result, p + 9, eol - 8 - p);
+			return result;
+		}
+		p = eol;
+		if (*p == '\n')
+			p++;
+	}
+	return NULL;
+}
+
+static void write_cherry_pick_head(struct commit *commit)
+{
+	int fd;
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+
+	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
+	if (fd < 0)
+		die_errno(_("Could not open '%s' for writing"),
+			  git_path("CHERRY_PICK_HEAD"));
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
+		die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
+	strbuf_release(&buf);
+}
+
+static void print_advice(int show_hint)
+{
+	char *msg = getenv("GIT_CHERRY_PICK_HELP");
+
+	if (msg) {
+		fprintf(stderr, "%s\n", msg);
+		/*
+		 * A conflict has occured but the porcelain
+		 * (typically rebase --interactive) wants to take care
+		 * of the commit itself so remove CHERRY_PICK_HEAD
+		 */
+		unlink(git_path("CHERRY_PICK_HEAD"));
+		return;
+	}
+
+	if (show_hint) {
+		advise("after resolving the conflicts, mark the corrected paths");
+		advise("with 'git add <paths>' or 'git rm <paths>'");
+		advise("and commit the result with 'git commit'");
+	}
+}
+
+static void write_message(struct strbuf *msgbuf, const char *filename)
+{
+	static struct lock_file msg_file;
+
+	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
+					       LOCK_DIE_ON_ERROR);
+	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
+		die_errno(_("Could not write to %s."), filename);
+	strbuf_release(msgbuf);
+	if (commit_lock_file(&msg_file) < 0)
+		die(_("Error wrapping up %s"), filename);
+}
+
+static struct tree *empty_tree(void)
+{
+	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
+}
+
+static int error_dirty_index(struct replay_opts *opts)
+{
+	if (read_cache_unmerged())
+		return error_resolve_conflict(action_name(opts));
+
+	/* Different translation strings for cherry-pick and revert */
+	if (opts->action == REPLAY_PICK)
+		error(_("Your local changes would be overwritten by cherry-pick."));
+	else
+		error(_("Your local changes would be overwritten by revert."));
+
+	if (advice_commit_before_merge)
+		advise(_("Commit your changes or stash them to proceed."));
+	return -1;
+}
+
+static int fast_forward_to(const unsigned char *to, const unsigned char *from)
+{
+	struct ref_lock *ref_lock;
+
+	read_cache();
+	if (checkout_fast_forward(from, to))
+		exit(1); /* the callee should have complained already */
+	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
+	return write_ref_sha1(ref_lock, to, "cherry-pick");
+}
+
+static int do_recursive_merge(struct commit *base, struct commit *next,
+			      const char *base_label, const char *next_label,
+			      unsigned char *head, struct strbuf *msgbuf,
+			      struct replay_opts *opts)
+{
+	struct merge_options o;
+	struct tree *result, *next_tree, *base_tree, *head_tree;
+	int clean, index_fd;
+	const char **xopt;
+	static struct lock_file index_lock;
+
+	index_fd = hold_locked_index(&index_lock, 1);
+
+	read_cache();
+
+	init_merge_options(&o);
+	o.ancestor = base ? base_label : "(empty tree)";
+	o.branch1 = "HEAD";
+	o.branch2 = next ? next_label : "(empty tree)";
+
+	head_tree = parse_tree_indirect(head);
+	next_tree = next ? next->tree : empty_tree();
+	base_tree = base ? base->tree : empty_tree();
+
+	for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
+		parse_merge_opt(&o, *xopt);
+
+	clean = merge_trees(&o,
+			    head_tree,
+			    next_tree, base_tree, &result);
+
+	if (active_cache_changed &&
+	    (write_cache(index_fd, active_cache, active_nr) ||
+	     commit_locked_index(&index_lock)))
+		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
+		die(_("%s: Unable to write new index file"), action_name(opts));
+	rollback_lock_file(&index_lock);
+
+	if (!clean) {
+		int i;
+		strbuf_addstr(msgbuf, "\nConflicts:\n\n");
+		for (i = 0; i < active_nr;) {
+			struct cache_entry *ce = active_cache[i++];
+			if (ce_stage(ce)) {
+				strbuf_addch(msgbuf, '\t');
+				strbuf_addstr(msgbuf, ce->name);
+				strbuf_addch(msgbuf, '\n');
+				while (i < active_nr && !strcmp(ce->name,
+						active_cache[i]->name))
+					i++;
+			}
+		}
+	}
+
+	return !clean;
+}
+
+/*
+ * If we are cherry-pick, and if the merge did not result in
+ * hand-editing, we will hit this commit and inherit the original
+ * author date and name.
+ * If we are revert, or if our cherry-pick results in a hand merge,
+ * we had better say that the current user is responsible for that.
+ */
+static int run_git_commit(const char *defmsg, struct replay_opts *opts)
+{
+	/* 6 is max possible length of our args array including NULL */
+	const char *args[6];
+	int i = 0;
+
+	args[i++] = "commit";
+	args[i++] = "-n";
+	if (opts->signoff)
+		args[i++] = "-s";
+	if (!opts->edit) {
+		args[i++] = "-F";
+		args[i++] = defmsg;
+	}
+	args[i] = NULL;
+
+	return run_command_v_opt(args, RUN_GIT_CMD);
+}
+
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+			struct replay_opts *opts)
+{
+	unsigned char head[20];
+	struct commit *base, *next, *parent;
+	const char *base_label, *next_label;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	char *defmsg = NULL;
+	struct strbuf msgbuf = STRBUF_INIT;
+	int res;
+
+	if (opts->no_commit) {
+		/*
+		 * We do not intend to commit immediately.  We just want to
+		 * merge the differences in, so let's compute the tree
+		 * that represents the "current" state for merge-recursive
+		 * to work on.
+		 */
+		if (write_cache_as_tree(head, 0, NULL))
+			die (_("Your index file is unmerged."));
+	} else {
+		if (get_sha1("HEAD", head))
+			return error(_("You do not have a valid HEAD"));
+		if (index_differs_from("HEAD", 0))
+			return error_dirty_index(opts);
+	}
+	discard_cache();
+
+	if (!commit->parents) {
+		parent = NULL;
+	}
+	else if (commit->parents->next) {
+		/* Reverting or cherry-picking a merge commit */
+		int cnt;
+		struct commit_list *p;
+
+		if (!opts->mainline)
+			return error(_("Commit %s is a merge but no -m option was given."),
+				sha1_to_hex(commit->object.sha1));
+
+		for (cnt = 1, p = commit->parents;
+		     cnt != opts->mainline && p;
+		     cnt++)
+			p = p->next;
+		if (cnt != opts->mainline || !p)
+			return error(_("Commit %s does not have parent %d"),
+				sha1_to_hex(commit->object.sha1), opts->mainline);
+		parent = p->item;
+	} else if (0 < opts->mainline)
+		return error(_("Mainline was specified but commit %s is not a merge."),
+			sha1_to_hex(commit->object.sha1));
+	else
+		parent = commit->parents->item;
+
+	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
+		return fast_forward_to(commit->object.sha1, head);
+
+	if (parent && parse_commit(parent) < 0)
+		/* TRANSLATORS: The first %s will be "revert" or
+		   "cherry-pick", the second %s a SHA1 */
+		return error(_("%s: cannot parse parent commit %s"),
+			action_name(opts), sha1_to_hex(parent->object.sha1));
+
+	if (get_message(commit, &msg) != 0)
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
+
+	/*
+	 * "commit" is an existing commit.  We would want to apply
+	 * the difference it introduces since its first parent "prev"
+	 * on top of the current HEAD if we are cherry-pick.  Or the
+	 * reverse of it if we are revert.
+	 */
+
+	defmsg = git_pathdup("MERGE_MSG");
+
+	if (action == REPLAY_REVERT) {
+		base = commit;
+		base_label = msg.label;
+		next = parent;
+		next_label = msg.parent_label;
+		strbuf_addstr(&msgbuf, "Revert \"");
+		strbuf_addstr(&msgbuf, msg.subject);
+		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
+		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+
+		if (commit->parents && commit->parents->next) {
+			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
+		}
+		strbuf_addstr(&msgbuf, ".\n");
+	} else {
+		const char *p;
+
+		base = parent;
+		base_label = msg.parent_label;
+		next = commit;
+		next_label = msg.label;
+
+		/*
+		 * Append the commit log message to msgbuf; it starts
+		 * after the tree, parent, author, committer
+		 * information followed by "\n\n".
+		 */
+		p = strstr(msg.message, "\n\n");
+		if (p) {
+			p += 2;
+			strbuf_addstr(&msgbuf, p);
+		}
+
+		if (opts->record_origin) {
+			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+			strbuf_addstr(&msgbuf, ")\n");
+		}
+	}
+
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
+		res = do_recursive_merge(base, next, base_label, next_label,
+					 head, &msgbuf, opts);
+		write_message(&msgbuf, defmsg);
+	} else {
+		struct commit_list *common = NULL;
+		struct commit_list *remotes = NULL;
+
+		write_message(&msgbuf, defmsg);
+
+		commit_list_insert(base, &common);
+		commit_list_insert(next, &remotes);
+		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+					common, sha1_to_hex(head), remotes);
+		free_commit_list(common);
+		free_commit_list(remotes);
+	}
+
+	/*
+	 * If the merge was clean or if it failed due to conflict, we write
+	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
+	 * However, if the merge did not even start, then we don't want to
+	 * write it at all.
+	 */
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
+		write_cherry_pick_head(commit);
+
+	if (res) {
+		error(action == REPLAY_REVERT
+		      ? _("could not revert %s... %s")
+		      : _("could not apply %s... %s"),
+		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
+		      msg.subject);
+		print_advice(res == 1);
+		rerere(opts->allow_rerere_auto);
+	} else {
+		if (!opts->no_commit)
+			res = run_git_commit(defmsg, opts);
+	}
+
+	free_message(&msg);
+	free(defmsg);
+
+	return res;
+}
+
+static void prepare_revs(struct replay_opts *opts)
+{
+	if (opts->action != REPLAY_REVERT)
+		opts->revs->reverse ^= 1;
+
+	if (prepare_revision_walk(opts->revs))
+		die(_("revision walk setup failed"));
+
+	if (!opts->revs->commits)
+		die(_("empty commit set passed"));
+}
+
+static void read_and_refresh_cache(struct replay_opts *opts)
+{
+	static struct lock_file index_lock;
+	int index_fd = hold_locked_index(&index_lock, 0);
+	if (read_index_preload(&the_index, NULL) < 0)
+		die(_("git %s: failed to read the index"), action_name(opts));
+	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
+	if (the_index.cache_changed) {
+		if (write_index(&the_index, index_fd) ||
+		    commit_locked_index(&index_lock))
+			die(_("git %s: failed to refresh the index"), action_name(opts));
+	}
+	rollback_lock_file(&index_lock);
+}
+
+/*
+ * Append a commit to the end of the commit_list.
+ *
+ * next starts by pointing to the variable that holds the head of an
+ * empty commit_list, and is updated to point to the "next" field of
+ * the last item on the list as new commits are appended.
+ *
+ * Usage example:
+ *
+ *     struct commit_list *list;
+ *     struct commit_list **next = &list;
+ *
+ *     next = commit_list_append(c1, next);
+ *     next = commit_list_append(c2, next);
+ *     assert(commit_list_count(list) == 2);
+ *     return list;
+ */
+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+						struct commit *operand,
+						struct replay_insn_list **next)
+{
+	struct replay_insn_list *new = xmalloc(sizeof(*new));
+	new->action = action;
+	new->operand = operand;
+	*next = new;
+	new->next = NULL;
+	return &new->next;
+}
+
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
+{
+	struct replay_insn_list *cur;
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		const char *sha1_abbrev, *action_str, *subject;
+		int subject_len;
+
+		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		subject_len = find_commit_subject(cur->operand->buffer, &subject);
+		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+			subject_len, subject);
+	}
+	return 0;
+}
+
+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+{
+	unsigned char commit_sha1[20];
+	char *end_of_object_name;
+	int saved, status;
+
+	if (!prefixcmp(bol, "pick ")) {
+		item->action = REPLAY_PICK;
+		bol += strlen("pick ");
+	} else if (!prefixcmp(bol, "revert ")) {
+		item->action = REPLAY_REVERT;
+		bol += strlen("revert ");
+	} else {
+		size_t len = strchrnul(bol, '\n') - bol;
+		if (len > 255)
+			len = 255;
+		return error(_("Unrecognized action: %.*s"), (int)len, bol);
+	}
+
+	end_of_object_name = bol + strcspn(bol, " \n");
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	status = get_sha1(bol, commit_sha1);
+	*end_of_object_name = saved;
+
+	if (status < 0)
+		return error(_("Malformed object name: %s"), bol);
+
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return error(_("Not a valid commit: %s"), bol);
+
+	item->next = NULL;
+	return 0;
+}
+
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
+{
+	struct replay_insn_list **next = todo_list;
+	struct replay_insn_list item = {0, NULL, NULL};
+	char *p = buf;
+	int i;
+
+	for (i = 1; *p; i++) {
+		char *eol = strchrnul(p, '\n');
+		if (parse_insn_line(p, eol, &item) < 0)
+			return error(_("on line %d."), i);
+		next = replay_insn_list_append(item.action, item.operand, next);
+		p = *eol ? eol + 1 : eol;
+	}
+	if (!*todo_list)
+		return error(_("No commits parsed."));
+	return 0;
+}
+
+static void read_populate_todo(struct replay_insn_list **todo_list)
+{
+	const char *todo_file = git_path(SEQ_TODO_FILE);
+	struct strbuf buf = STRBUF_INIT;
+	int fd, res;
+
+	fd = open(todo_file, O_RDONLY);
+	if (fd < 0)
+		die_errno(_("Could not open %s."), todo_file);
+	if (strbuf_read(&buf, fd, 0) < 0) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), todo_file);
+	}
+	close(fd);
+
+	res = parse_insn_buffer(buf.buf, todo_list);
+	strbuf_release(&buf);
+	if (res)
+		die(_("Unusable instruction sheet: %s"), todo_file);
+}
+
+static int populate_opts_cb(const char *key, const char *value, void *data)
+{
+	struct replay_opts *opts = data;
+	int error_flag = 1;
+
+	if (!value)
+		error_flag = 0;
+	else if (!strcmp(key, "options.no-commit"))
+		opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.edit"))
+		opts->edit = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.signoff"))
+		opts->signoff = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.record-origin"))
+		opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.allow-ff"))
+		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.mainline"))
+		opts->mainline = git_config_int(key, value);
+	else if (!strcmp(key, "options.strategy"))
+		git_config_string(&opts->strategy, key, value);
+	else if (!strcmp(key, "options.strategy-option")) {
+		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+		opts->xopts[opts->xopts_nr++] = xstrdup(value);
+	} else
+		return error(_("Invalid key: %s"), key);
+
+	if (!error_flag)
+		return error(_("Invalid value for %s: %s"), key, value);
+
+	return 0;
+}
+
+static void read_populate_opts(struct replay_opts **opts_ptr)
+{
+	const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+	if (!file_exists(opts_file))
+		return;
+	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
+		die(_("Malformed options sheet: %s"), opts_file);
+}
+
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
+				struct replay_opts *opts)
+{
+	struct commit *commit;
+	struct replay_insn_list **next;
+
+	prepare_revs(opts);
+
+	next = todo_list;
+	while ((commit = get_revision(opts->revs)))
+		next = replay_insn_list_append(opts->action, commit, next);
+}
+
+static int create_seq_dir(void)
+{
+	const char *seq_dir = git_path(SEQ_DIR);
+
+	if (file_exists(seq_dir))
+		return error(_("%s already exists."), seq_dir);
+	else if (mkdir(seq_dir, 0777) < 0)
+		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
+	return 0;
+}
+
+static void save_head(const char *head)
+{
+	const char *head_file = git_path(SEQ_HEAD_FILE);
+	static struct lock_file head_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s\n", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), head_file);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s."), head_file);
+}
+
+static void save_todo(struct replay_insn_list *todo_list)
+{
+	const char *todo_file = git_path(SEQ_TODO_FILE);
+	static struct lock_file todo_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
+	if (format_todo(&buf, todo_list) < 0)
+		die(_("Could not format %s."), todo_file);
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not write to %s."), todo_file);
+	}
+	if (commit_lock_file(&todo_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s."), todo_file);
+	}
+	strbuf_release(&buf);
+}
+
+static void save_opts(struct replay_opts *opts)
+{
+	const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+	if (opts->no_commit)
+		git_config_set_in_file(opts_file, "options.no-commit", "true");
+	if (opts->edit)
+		git_config_set_in_file(opts_file, "options.edit", "true");
+	if (opts->signoff)
+		git_config_set_in_file(opts_file, "options.signoff", "true");
+	if (opts->record_origin)
+		git_config_set_in_file(opts_file, "options.record-origin", "true");
+	if (opts->allow_ff)
+		git_config_set_in_file(opts_file, "options.allow-ff", "true");
+	if (opts->mainline) {
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addf(&buf, "%d", opts->mainline);
+		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+		strbuf_release(&buf);
+	}
+	if (opts->strategy)
+		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+	if (opts->xopts) {
+		int i;
+		for (i = 0; i < opts->xopts_nr; i++)
+			git_config_set_multivar_in_file(opts_file,
+							"options.strategy-option",
+							opts->xopts[i], "^$", 0);
+	}
+}
+
+static int pick_commits(struct replay_insn_list *todo_list,
+			struct replay_opts *opts)
+{
+	struct replay_insn_list *cur;
+	int res;
+
+	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+	if (opts->allow_ff)
+		assert(!(opts->signoff || opts->no_commit ||
+				opts->record_origin || opts->edit));
+	read_and_refresh_cache(opts);
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
+		if (res) {
+			if (!cur->next)
+				/*
+				 * An error was encountered while
+				 * picking the last commit; the
+				 * sequencer state is useless now --
+				 * the user simply needs to resolve
+				 * the conflict and commit
+				 */
+				remove_sequencer_state(0);
+			return res;
+		}
+	}
+
+	/*
+	 * Sequence of picks finished successfully; cleanup by
+	 * removing the .git/sequencer directory
+	 */
+	remove_sequencer_state(1);
+	return 0;
+}
+
+int sequencer_pick_revisions(struct replay_opts *opts)
+{
+	struct replay_insn_list *todo_list = NULL;
+	unsigned char sha1[20];
+
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
+	read_and_refresh_cache(opts);
+
+	/*
+	 * Decide what to do depending on the arguments; a fresh
+	 * cherry-pick should be handled differently from an existing
+	 * one that is being continued
+	 */
+	if (opts->subcommand == REPLAY_RESET) {
+		remove_sequencer_state(1);
+		return 0;
+	} else if (opts->subcommand == REPLAY_CONTINUE) {
+		if (!file_exists(git_path(SEQ_TODO_FILE)))
+			goto error;
+		read_populate_opts(&opts);
+		read_populate_todo(&todo_list);
+
+		/* Verify that the conflict has been resolved */
+		if (!index_differs_from("HEAD", 0))
+			todo_list = todo_list->next;
+	} else {
+		/*
+		 * Start a new cherry-pick/ revert sequence; but
+		 * first, make sure that an existing one isn't in
+		 * progress
+		 */
+
+		walk_revs_populate_todo(&todo_list, opts);
+		if (create_seq_dir() < 0) {
+			error(_("A cherry-pick or revert is in progress."));
+			advise(_("Use --continue to continue the operation"));
+			advise(_("or --reset to forget about it"));
+			return -1;
+		}
+		if (get_sha1("HEAD", sha1)) {
+			if (opts->action == REPLAY_REVERT)
+				return error(_("Can't revert as initial commit"));
+			return error(_("Can't cherry-pick into empty head"));
+		}
+		save_head(sha1_to_hex(sha1));
+		save_opts(opts);
+	}
+	return pick_commits(todo_list, opts);
+error:
+	return error(_("No %s in progress"), action_name(opts));
+}
diff --git a/sequencer.h b/sequencer.h
index f4db257..92b2d63 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -8,6 +8,30 @@
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
 enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
+
+struct replay_opts {
+	enum replay_action action;
+	enum replay_subcommand subcommand;
+
+	/* Boolean options */
+	int edit;
+	int record_origin;
+	int no_commit;
+	int signoff;
+	int allow_ff;
+	int allow_rerere_auto;
+
+	int mainline;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+
+	/* Only used by REPLAY_NONE */
+	struct rev_info *revs;
+};
 
 struct replay_insn_list {
 	enum replay_action action;
@@ -25,4 +49,6 @@ struct replay_insn_list {
  */
 void remove_sequencer_state(int aggressive);
 
+int sequencer_pick_revisions(struct replay_opts *opts);
+
 #endif
-- 
1.7.6.351.gb35ac.dirty

[PATCH 3/5] sequencer: sequencer state is useless without todo

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:23

Later in the series, we will not write '.git/sequencer/todo' for a
single commit cherry-pick, because 'CHERRY_PICK_HEAD' already contains
this information.  So, stomp the sequencer state in create_seq_state()
unless the todo file is present.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 sequencer.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index e566043..517eb23 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 
 static int create_seq_dir(void)
 {
+	const char *todo_file = git_path(SEQ_TODO_FILE);
 	const char *seq_dir = git_path(SEQ_DIR);
 
-	if (file_exists(seq_dir))
-		return error(_("%s already exists."), seq_dir);
-	else if (mkdir(seq_dir, 0777) < 0)
+	if (file_exists(todo_file))
+		return error(_("%s already exists."), todo_file);
+
+	/* If todo_file doesn't exist, discard sequencer state */
+	remove_sequencer_state(1);
+	if (mkdir(seq_dir, 0777) < 0)
 		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
 	return 0;
 }
-- 
1.7.6.351.gb35ac.dirty

[PATCH 5/5] sequencer: revert d3f4628e

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:23

Revert d3f4628e (revert: Remove sequencer state when no commits are
pending, 2011-06-06), because this is not the right approach.  Instead
of increasing coupling between the sequencer and 'git commit', a
unified '--continue' that invokes 'git commit' on behalf of the
end-user is preferred.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 sequencer.c                     |   12 +-----------
 t/t3510-cherry-pick-sequence.sh |   24 ------------------------
 2 files changed, 1 insertions(+), 35 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 6762ceb..7caa550 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -758,18 +758,8 @@ static int pick_commits(struct replay_insn_list *todo_list,
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur);
 		res = do_pick_commit(cur->operand, cur->action, opts);
-		if (res) {
-			if (!cur->next)
-				/*
-				 * An error was encountered while
-				 * picking the last commit; the
-				 * sequencer state is useless now --
-				 * the user simply needs to resolve
-				 * the conflict and commit
-				 */
-				remove_sequencer_state(0);
+		if (res)
 			return res;
-		}
 	}
 
 	/*
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 4b12244..b30f13a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -85,30 +85,6 @@ test_expect_success '--reset cleans up sequencer state' '
 	test_path_is_missing .git/sequencer
 '
 
-test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
-	pristine_detach initial &&
-	test_must_fail git cherry-pick base..picked &&
-	test_path_is_missing .git/sequencer &&
-	echo "resolved" >foo &&
-	git add foo &&
-	git commit &&
-	{
-		git rev-list HEAD |
-		git diff-tree --root --stdin |
-		sed "s/$_x40/OBJID/g"
-	} >actual &&
-	cat >expect <<-\EOF &&
-	OBJID
-	:100644 100644 OBJID OBJID M	foo
-	OBJID
-	:100644 100644 OBJID OBJID M	unrelated
-	OBJID
-	:000000 100644 OBJID OBJID A	foo
-	:000000 100644 OBJID OBJID A	unrelated
-	EOF
-	test_cmp expect actual
-'
-
 test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..anotherpick &&
-- 
1.7.6.351.gb35ac.dirty

[PATCH 4/5] sequencer: handle single commit pick separately

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:23

Don't write a '.git/sequencer/todo', as CHERRY_PICK_HEAD already
contains this information.  However, '.git/sequencer/opts' and
'.git/sequencer/head' are required to support '--reset' and
'--continue' operations.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 sequencer.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 517eb23..6762ceb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -746,6 +746,15 @@ static int pick_commits(struct replay_insn_list *todo_list,
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(opts);
 
+	/*
+	 * Backward compatibility hack: when only a single commit is
+	 * picked, don't save_todo(), because CHERRY_PICK_HEAD will
+	 * contain this information anyway.
+	 */
+	if (opts->subcommand == REPLAY_NONE &&
+		todo_list->next == NULL && todo_list->action == REPLAY_PICK)
+		return do_pick_commit(todo_list->operand, REPLAY_PICK, opts);
+
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur);
 		res = do_pick_commit(cur->operand, cur->action, opts);
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 0/5] Sequencer: working around historical mistakes

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:23

Hey,

Ramkumar Ramachandra wrote:
As described in the discussion following $gmane/179304/focus=179383,
we have decided to handle historical hacks in the sequencer itself.
This series that follows is one step in the right direction.
I'm not sure what the above means.  But let's see what the patches
say. :)

[...]
2. This series depends on rr/revert-cherry-pick, but doesn't apply to
the current 'next'- sorry, rebasing is a massive pita due to 1/5.
Shouldn't it be based against rr/revert-cherry-pick, rather than
"next" which is more of a moving target?

Thanks,
Jonathan

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:23

Ramkumar Ramachandra wrote:
Start building the generalized sequencer by moving code from revert.c
into sequencer.c and sequencer.h.  Make the builtin responsible only
for command-line parsing, and expose a new sequencer_pick_revisions()
to do the actual work of sequencing commits.

This is intended to be almost a pure code movement patch with no
functional changes.  Check with:
Do I understand correctly that the purpose of this patch is to expose
some functions through the "sequencer.h" API, which patches later in
the series will use?  Which functions?  What is this generalized
sequencer which we are starting to build?  Why should I be happy about
(or care about, for that matter) code having moved from one source
file to another?

Rule of thumb for commit messages: after reading a commit message, I
should be able to predict what the patch will do, without reading the
patch.

I am guessing the above description started sane and then went through
a few revisions without a person reading it all the way through again.
Please consider just rewriting it.

[...]
quoted hunk
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1,19 +1,9 @@
 #include "cache.h"
 #include "builtin.h"
-#include "object.h"
-#include "commit.h"
-#include "tag.h"
-#include "run-command.h"
-#include "exec_cmd.h"
-#include "utf8.h"
 #include "parse-options.h"
-#include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
 #include "rerere.h"
-#include "merge-recursive.h"
-#include "refs.h"
-#include "dir.h"
 #include "sequencer.h"
Hoorah!

[snipping lots of deletion of code from builtin/revert.c]
quoted hunk
@@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
The new sequencer_pick_revisions is just a new name for the old
pick_revisions.  Sane, but probably worth mentioning in the log
message.

[...]
quoted hunk
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,7 +1,27 @@
 #include "cache.h"
+#include "object.h"
+#include "commit.h"
+#include "tag.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+#include "utf8.h"
+#include "cache-tree.h"
+#include "diff.h"
+#include "revision.h"
+#include "rerere.h"
+#include "merge-recursive.h"
+#include "refs.h"
-#include "sequencer.h"
-#include "strbuf.h"
 #include "dir.h"
+#include "sequencer.h"
Why did sequencer.h move to after dir.h?  Wow, we use a lot of headers
here --- I wonder if there are some pieces that could be split out
(that's not due to your patch, though).

[...]
quoted hunk
--- a/sequencer.h
+++ b/sequencer.h
@@ -8,6 +8,30 @@
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
 enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
+
+struct replay_opts {
[...]
quoted hunk
@@ -25,4 +49,6 @@ struct replay_insn_list {
  */
 void remove_sequencer_state(int aggressive);
 
+int sequencer_pick_revisions(struct replay_opts *opts);
Ah, so this moves most of the logic of "git cherry-pick" to the sequencer
but the only new API that needs to be exposed is pick_revisions().  The
calling sequence looks like this:

	memset(&opts, o, sizeof(opts));
	opts.action = REPLAY_PICK;
	opts.revs = xmalloc(sizeof(*opts.revs));

	init_revisions(opts.revs);
	add_pending_object / setup_revisions / etc

	sequencer_pick_revisions(&opts);

The small exposed interface makes this a relatively uninvasive patch,
and the immediate advantage is that we plan to reuse some of the
functionality used in pick_revisions() in other, new APIs to be used
by commands other than cherry-pick.  No functional change yet intended.

Except for the commit message, looks reasonable (though I haven't
tried the "git blame" magic to check the code movement part).  Thanks.

Re: [PATCH 2/5] sequencer: remove CHERRY_PICK_HEAD with sequencer state

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:23

Ramkumar Ramachandra wrote:
Make remove_sequencer_state() remove '.git/CHERRY_PICK_HEAD' when
invoked aggressively, since we want to treat it as part of the
sequencer state now.  While at it, make some minor improvements to the
function.
What does it mean to invoke a function aggressively?  What is the
nature of these minor improvements (are they behavior changes or just
cleanups)?  (Remember, the reader hasn't seen the patch yet.)
quoted hunk
--- a/sequencer.c
+++ b/sequencer.c
@@ -25,17 +25,22 @@ static char *get_encoding(const char *message);
 
 void remove_sequencer_state(int aggressive)
 {
+	const char *seq_dir = git_path(SEQ_DIR);
+	const char *seq_old_dir = git_path(SEQ_OLD_DIR);
+	const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");
If there were just two more like this, the behavior would change
completely.  Scary.  Are these temporary variables needed?

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:23

Ramkumar Ramachandra wrote:
Later in the series, we will not write '.git/sequencer/todo' for a
single commit cherry-pick, because 'CHERRY_PICK_HEAD' already contains
this information.  So, stomp the sequencer state in create_seq_state()
unless the todo file is present.
What problem does this solve?  How does it solve it?  What does it
mean to stomp?

The usual commit-message debugging strategy applies here: imagine you
are a BIOS clone manufacturer, and for legal reasons you are not
allowed to read this part of the git implementation embedded in the
standard BIOS.  However, you are allowed to read the commit message,
and if that message is clear enough, it will explain the purpose and
behavior of that code and you will be able to implement a compatible
implementation addressing the same problem without scratching your
head too much.
quoted hunk
--- a/sequencer.c
+++ b/sequencer.c
@@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 
 static int create_seq_dir(void)
 {
+	const char *todo_file = git_path(SEQ_TODO_FILE);
 	const char *seq_dir = git_path(SEQ_DIR);
Scary idiom.
-	if (file_exists(seq_dir))
-		return error(_("%s already exists."), seq_dir);
-	else if (mkdir(seq_dir, 0777) < 0)
+	if (file_exists(todo_file))
+		return error(_("%s already exists."), todo_file);
+
+	/* If todo_file doesn't exist, discard sequencer state */
+	remove_sequencer_state(1);
+	if (mkdir(seq_dir, 0777) < 0)
 		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
I guess this patch would make more sense after patch 4.

Re: [PATCH 4/5] sequencer: handle single commit pick separately

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:23

Ramkumar Ramachandra wrote:
Don't write a '.git/sequencer/todo', as CHERRY_PICK_HEAD already
contains this information.  However, '.git/sequencer/opts' and
'.git/sequencer/head' are required to support '--reset' and
'--continue' operations.
This is meant as a signal to later "git cherry-pick" commands that it
is okay to forget about the cherry-pick, right?  How is the reader
supposed to know that?  Say so!

By the way, it's not clear to me yet whether the resulting UI would be
more pleasant or not.  What is the expected calling sequence?  Any odd
corners of behavior changing?  What happens if I do

	git cherry-pick foo; # conflicts!
	git cherry-pick bar; # just ignore them

or

	git cherry-pick foo; # conflicts!  but resolved in index by rerere
	git checkout something-else

Is there any potential downside to the change?

[...]
quoted hunk
--- a/sequencer.c
+++ b/sequencer.c
@@ -746,6 +746,15 @@ static int pick_commits(struct replay_insn_list *todo_list,
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(opts);
 
+	/*
+	 * Backward compatibility hack: when only a single commit is
+	 * picked, don't save_todo(), because CHERRY_PICK_HEAD will
+	 * contain this information anyway.
+	 */
How does saving disk space by avoiding saving redundant information
affect backward compatibility?  I'm not sure what this comment is
trying to say.
+	if (opts->subcommand == REPLAY_NONE &&
+		todo_list->next == NULL && todo_list->action == REPLAY_PICK)
+		return do_pick_commit(todo_list->operand, REPLAY_PICK, opts);
+
 	for (cur = todo_list; cur; cur = cur->next) {

Re: [PATCH 5/5] sequencer: revert d3f4628e

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:23

Ramkumar Ramachandra wrote:
Revert d3f4628e (revert: Remove sequencer state when no commits are
pending, 2011-06-06), because this is not the right approach.  Instead
of increasing coupling between the sequencer and 'git commit', a
unified '--continue' that invokes 'git commit' on behalf of the
end-user is preferred.
Forgive me for forgetting: what is the problem that d3f4628e was going
to resolve (i.e., right approach to what)?  What is this increased
coupling, and why do we want to avoid it?  Is "to prefer" another word
for "to implement"?  Who is being united by this new --continue
switch?

Is this patch just reverting a previous patch?  If so, why doesn't the
commit message use the usual format

	Revert "<commit message>"

	This reverts commit <unabbreviated object name>.

	<explanation>

?
 sequencer.c                     |   12 +-----------
 t/t3510-cherry-pick-sequence.sh |   24 ------------------------
 2 files changed, 1 insertions(+), 35 deletions(-)
When changing behavior, it's more comforting to modify tests to describe
the new behavior than to just get rid of them. :)

To sum matters up: with a new commit message, patch 1 seems likely to
be ready.  Patches 2-5 seem to need more work --- it's not clear to me
yet what they are supposed to do.

Hope that helps,
Jonathan

Re: [PATCH 5/5] sequencer: revert d3f4628e

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:28

Hi Jonathan,

Jonathan Nieder writes:
Is this patch just reverting a previous patch?  If so, why doesn't the
commit message use the usual format

       Revert "<commit message>"

       This reverts commit <unabbreviated object name>.

       <explanation>
[...]
I'd have loved to use 'git revert d3f4628e', but that ends up creating
a lot more work: recall the big move made by 1/5?  I'm trying to
"effectively port the inverse of the changes made by d3f4628e in
revert.c to sequencer.c" -- would you still like to see a git-revert
style commit message?  Don't you think it'll be misleading?

Sorry about the shoddy commit messages though: I'm polishing the
series now that I'm convinced that it's heading in the right
direction.  Hopefully, I'll have more to show soon.

Thanks.

-- Ram

Re: [PATCH 5/5] sequencer: revert d3f4628e

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:28

Ramkumar Ramachandra wrote:
I'm trying to
"effectively port the inverse of the changes made by d3f4628e in
revert.c to sequencer.c" -- would you still like to see a git-revert
style commit message?  Don't you think it'll be misleading?
My main complaint is that the subject line (and then the body) didn't
tell me what effect the patch would have in a self-contained way.

I don't think a git-revert style commit message would be misleading.
Couldn't you avoid confusing people by providing the relevant
information directly?  "This commit was not made with 'git revert',
since there has been too much code reorganization in the meantime;
instead, I applied the inverse of the changes made by d3f4628e by
hand.  This patch also tweaks the test added in that commit instead of
removing it."
Sorry about the shoddy commit messages though: I'm polishing the
series now that I'm convinced that it's heading in the right
direction.  Hopefully, I'll have more to show soon.
Thanks.  I'll try not to be distracted and to just focus on the code
for the next round.

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:28

Hi,

Small note.

Jonathan Nieder writes:
quoted
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,7 +1,27 @@
 #include "cache.h"
+#include "object.h"
+#include "commit.h"
+#include "tag.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+#include "utf8.h"
+#include "cache-tree.h"
+#include "diff.h"
+#include "revision.h"
+#include "rerere.h"
+#include "merge-recursive.h"
+#include "refs.h"
-#include "sequencer.h"
-#include "strbuf.h"
 #include "dir.h"
+#include "sequencer.h"
Why did sequencer.h move to after dir.h?
1. I like the convention of including the "foo.h" as the last header
in "foo.c".  I suppose it has to do with the way I include standard
headers in my own code (for pet projects).
2. I didn't want to include many of these headers in sequencer.h again
-- it uses a lot of these data types.  I've noticed that ordering of
header inclusion is important in many parts of Git, so the convention
just stuck.

Thanks.

-- Ram

Re: [PATCH 0/5] Sequencer: working around historical mistakes

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:28

Hi,

Jonathan Nieder writes:
Shouldn't it be based against rr/revert-cherry-pick, rather than
"next" which is more of a moving target?
When I read this, I went: "Now why didn't I think of that?"

Thanks.

-- Ram

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:28

Hi,

Jonathan Nieder wrote:
Ramkumar Ramachandra wrote:
[...]
The usual commit-message debugging strategy applies here: imagine you
are a BIOS clone manufacturer, and for legal reasons you are not
allowed to read this part of the git implementation embedded in the
standard BIOS.  However, you are allowed to read the commit message,
and if that message is clear enough, it will explain the purpose and
behavior of that code and you will be able to implement a compatible
implementation addressing the same problem without scratching your
head too much.
Ah, it helps to think about commit messages like this.  Thanks.
quoted
--- a/sequencer.c
+++ b/sequencer.c
@@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 static int create_seq_dir(void)
 {
+     const char *todo_file = git_path(SEQ_TODO_FILE);
      const char *seq_dir = git_path(SEQ_DIR);
Scary idiom.
What's scary about it?

-- Ram
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help