[PATCH 0/6] Towards a generalized sequencer

[PATCH 0/6] Towards a generalized sequencer

[Ramkumar Ramachandra]

Hi,

I've prepared a nicer series after Jonathan's feedback on the previous
one.  No new ideas; just the same ideas implemented in a more sane
way.  The first three patches fix some minor annoyances and don't
dontribute much to the series in general.  The fourth patch implements
a very important idea: the ability to parse a general (action,
operand) pair in the instruction sheet.  The fifth patch may come as a
real shocker; I was shocked myself, but I'm now convinced that this is
the right way forward.  Since it's only code movement, it should be
very easy to review.  The final patch solves a long-standing problem
by introducing tighter coupling between 'git commit' and the
sequencer.

Thanks for lending a ear.  Enjoy reading the rest!

Note: I didn't know what to do with the license header in the fifth
patch.  I just assumed that it was some historical cruft and removed
it.

Ramkumar Ramachandra (6):
  revert: Don't remove the sequencer state on error
  revert: Free memory after get_message call
  revert: Parse instruction sheet more cautiously
  revert: Allow mixed pick and revert instructions
  sequencer: Expose API to cherry-picking machinery
  sequencer: Remove sequencer state after final commit

 builtin/commit.c                |    7 +-
 builtin/revert.c                | 1012 +--------------------------------------
 sequencer.c                     |  962 +++++++++++++++++++++++++++++++++++++-
 sequencer.h                     |   37 ++
 t/t3510-cherry-pick-sequence.sh |   91 ++++-
 5 files changed, 1099 insertions(+), 1010 deletions(-)

-- 
1.7.6.351.gb35ac.dirty

[PATCH 1/6] revert: Don't remove the sequencer state on error

[Ramkumar Ramachandra]

The cherry-pick/ revert machinery now removes the sequencer state when
do_pick_commit returns a non-zero, and when only one instruction is
left in the todo_list.  Since do_pick_commit has a way to distinguish
errors from conflicts using the signed-ness of the return value,
utilize this to ensure that the sequencer state is only removed when
there's a conflict and there is only one instruction left in the
todo_list.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8b452e8..a548a14 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -929,10 +929,10 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 		save_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
 		if (res) {
-			if (!cur->next)
+			if (!cur->next && res > 0)
 				/*
-				 * An error was encountered while
-				 * picking the last commit; the
+				 * A conflict was encountered while
+				 * picking the last commit.  The
 				 * sequencer state is useless now --
 				 * the user simply needs to resolve
 				 * the conflict and commit
-- 
1.7.6.351.gb35ac.dirty

[PATCH 2/6] revert: Free memory after get_message call

[Ramkumar Ramachandra]

The format_todo function leaks memory because it forgets to call
free_message after get_message.  Fix this.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index a548a14..1a4187a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -688,6 +688,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 			return error(_("Cannot get commit message for %s"), sha1_abbrev);
 		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
 	}
+	free_message(&msg);
 	return 0;
 }
 
-- 
1.7.6.351.gb35ac.dirty

[PATCH 3/6] revert: Parse instruction sheet more cautiously

[Ramkumar Ramachandra]

Fix a buffer overflow bug by checking that parsed SHA-1 hex will fit
in the buffer we've created for it.  Also change the instruction sheet
format subtly so that a description of the commit after the object
name is optional.  So now, an instruction sheet like this is perfectly
valid:

  pick 35b0426
  pick fbd5bbcbc2e
  pick 7362160f

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |   20 +++++++++-----------
 t/t3510-cherry-pick-sequence.sh |   29 +++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1a4187a..f44f749 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -697,26 +697,24 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
 	enum replay_action action;
-	int insn_len = 0;
-	char *p, *q;
+	char *p = start, *q, *end = strchrnul(start, '\n');
 
 	if (!prefixcmp(start, "pick ")) {
 		action = CHERRY_PICK;
-		insn_len = strlen("pick");
-		p = start + insn_len + 1;
+		p += strlen("pick ");
 	} else if (!prefixcmp(start, "revert ")) {
 		action = REVERT;
-		insn_len = strlen("revert");
-		p = start + insn_len + 1;
+		p += strlen("revert ");
 	} else
 		return NULL;
 
-	q = strchr(p, ' ');
-	if (!q)
+	q = strchrnul(p, ' ');
+	if (q > end)
+		q = end;
+	if (q - p + 1 > sizeof(sha1_abbrev))
 		return NULL;
-	q++;
-
-	strlcpy(sha1_abbrev, p, q - p);
+	memcpy(sha1_abbrev, p, q - p);
+	sha1_abbrev[q - p] = '\0';
 
 	/*
 	 * Verify that the action matches up with the one in
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3bca2b3..bc5f0b8 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -211,4 +211,33 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'missing commit descriptions in instruction sheet' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		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	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_done
-- 
1.7.6.351.gb35ac.dirty

[PATCH 4/6] revert: Allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Change the way the instruction parser works, allowing arbitrary
(action, operand) pairs to be parsed.  So now, you can do:

  pick fdc0b12 picked
  revert 965fed4 anotherpick

For cherry-pick and revert, this means that a 'git cherry-pick
--continue' can continue an ongoing revert operation and viceversa.
This patch lays the foundation for extending the parser to support
more actions so 'git rebase -i' can reuse this machinery in the
future.

Helped-by: Jonathan Nieder <jrnider@gmail.com>
Acked-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |  128 ++++++++++++++++++--------------------
 sequencer.h                     |    8 +++
 t/t3510-cherry-pick-sequence.sh |   58 ++++++++++++++++++
 3 files changed, 127 insertions(+), 67 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index f44f749..483c957 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-enum replay_action { REVERT, CHERRY_PICK };
 enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
 
 struct replay_opts {
@@ -68,14 +67,14 @@ struct replay_opts {
 
 static const char *action_name(const struct replay_opts *opts)
 {
-	return opts->action == REVERT ? "revert" : "cherry-pick";
+	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 == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
@@ -154,7 +153,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 	};
 
-	if (opts->action == CHERRY_PICK) {
+	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -353,7 +352,7 @@ static int error_dirty_index(struct replay_opts *opts)
 		return error_resolve_conflict(action_name(opts));
 
 	/* Different translation strings for cherry-pick and revert */
-	if (opts->action == CHERRY_PICK)
+	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."));
@@ -457,7 +456,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+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;
@@ -517,7 +517,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		/* 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));
+			action == REPLAY_REVERT ? "revert" : "cherry-pick",
+			sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
@@ -532,7 +533,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->action == REVERT) {
+	if (action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -575,7 +576,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 			write_cherry_pick_head(commit);
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+	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);
@@ -594,7 +595,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	}
 
 	if (res) {
-		error(opts->action == REVERT
+		error(action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -618,7 +619,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (opts->action != REVERT)
+	if (opts->action != REPLAY_REVERT)
 		revs->reverse = 1;
 
 	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
@@ -664,88 +665,81 @@ static void read_and_refresh_cache(struct replay_opts *opts)
  *     assert(commit_list_count(list) == 2);
  *     return list;
  */
-struct commit_list **commit_list_append(struct commit *commit,
-					struct commit_list **next)
+struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+						struct commit *operand,
+						struct replay_insn_list **next)
 {
-	struct commit_list *new = xmalloc(sizeof(struct commit_list));
-	new->item = commit;
+	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 commit_list *todo_list,
-		struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 {
-	struct commit_list *cur = NULL;
-	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
-	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REVERT ? "revert" : "pick";
+	struct replay_insn_list *cur;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
-		if (get_message(cur->item, &msg))
+		const char *sha1_abbrev, *action_str;
+		struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+
+		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		if (get_message(cur->operand, &msg))
 			return error(_("Cannot get commit message for %s"), sha1_abbrev);
 		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+		free_message(&msg);
 	}
-	free_message(&msg);
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static int parse_insn_line(char *start, struct replay_insn_list *item)
 {
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
-	enum replay_action action;
 	char *p = start, *q, *end = strchrnul(start, '\n');
 
 	if (!prefixcmp(start, "pick ")) {
-		action = CHERRY_PICK;
+		item->action = REPLAY_PICK;
 		p += strlen("pick ");
 	} else if (!prefixcmp(start, "revert ")) {
-		action = REVERT;
+		item->action = REPLAY_REVERT;
 		p += strlen("revert ");
 	} else
-		return NULL;
+		return error(_("Unrecognized action: %s"), start);
 
 	q = strchrnul(p, ' ');
 	if (q > end)
 		q = end;
 	if (q - p + 1 > sizeof(sha1_abbrev))
-		return NULL;
+		return error(_("Object name too large: %s"), p);
 	memcpy(sha1_abbrev, p, q - p);
 	sha1_abbrev[q - p] = '\0';
 
-	/*
-	 * Verify that the action matches up with the one in
-	 * opts; we don't support arbitrary instructions
-	 */
-	if (action != opts->action) {
-		const char *action_str;
-		action_str = action == REVERT ? "revert" : "cherry-pick";
-		error(_("Cannot %s during a %s"), action_str, action_name(opts));
-		return NULL;
-	}
-
 	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
-		return NULL;
+		return error(_("Malformed object name: %s"), sha1_abbrev);
+
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return error(_("Not a valid commit: %s"), sha1_abbrev);
 
-	return lookup_commit_reference(commit_sha1);
+	item->next = NULL;
+	return 0;
 }
 
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
-			struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 {
-	struct commit_list **next = todo_list;
-	struct commit *commit;
+	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++) {
-		commit = parse_insn_line(p, opts);
-		if (!commit)
+		if (parse_insn_line(p, &item) < 0)
 			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(item.action, item.operand, next);
 		p = strchrnul(p, '\n');
 		if (*p)
 			p++;
@@ -755,8 +749,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
-			struct replay_opts *opts)
+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;
@@ -772,7 +765,7 @@ static void read_populate_todo(struct commit_list **todo_list,
 	}
 	close(fd);
 
-	res = parse_insn_buffer(buf.buf, todo_list, opts);
+	res = parse_insn_buffer(buf.buf, todo_list);
 	strbuf_release(&buf);
 	if (res)
 		die(_("Unusable instruction sheet: %s"), todo_file);
@@ -821,18 +814,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 		die(_("Malformed options sheet: %s"), opts_file);
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
-	struct commit_list **next;
+	struct replay_insn_list **next;
 
 	prepare_revs(&revs, opts);
 
 	next = todo_list;
 	while ((commit = get_revision(&revs)))
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(opts->action, commit, next);
 }
 
 static int create_seq_dir(void)
@@ -861,7 +854,7 @@ static void save_head(const char *head)
 		die(_("Error wrapping up %s."), head_file);
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+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;
@@ -869,7 +862,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	int fd;
 
 	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list, opts) < 0)
+	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);
@@ -913,9 +906,10 @@ static void save_opts(struct replay_opts *opts)
 	}
 }
 
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list,
+			struct replay_opts *opts)
 {
-	struct commit_list *cur;
+	struct replay_insn_list *cur;
 	int res;
 
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -925,8 +919,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	read_and_refresh_cache(opts);
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
-		res = do_pick_commit(cur->item, opts);
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
 		if (res) {
 			if (!cur->next && res > 0)
 				/*
@@ -951,7 +945,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 
 static int pick_revisions(struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
+	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
 	read_and_refresh_cache(opts);
@@ -968,7 +962,7 @@ static int pick_revisions(struct replay_opts *opts)
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			goto error;
 		read_populate_opts(&opts);
-		read_populate_todo(&todo_list, opts);
+		read_populate_todo(&todo_list);
 
 		/* Verify that the conflict has been resolved */
 		if (!index_differs_from("HEAD", 0))
@@ -988,7 +982,7 @@ static int pick_revisions(struct replay_opts *opts)
 			return -1;
 		}
 		if (get_sha1("HEAD", sha1)) {
-			if (opts->action == REVERT)
+			if (opts->action == REPLAY_REVERT)
 				return error(_("Can't revert as initial commit"));
 			return error(_("Can't cherry-pick into empty head"));
 		}
@@ -1008,7 +1002,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	if (isatty(0))
 		opts.edit = 1;
-	opts.action = REVERT;
+	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
@@ -1023,7 +1017,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	int res;
 
 	memset(&opts, 0, sizeof(opts));
-	opts.action = CHERRY_PICK;
+	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
diff --git a/sequencer.h b/sequencer.h
index 905d295..f4db257 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,6 +7,14 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
+enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+
+struct replay_insn_list {
+	enum replay_action action;
+	struct commit *operand;
+	struct replay_insn_list *next;
+};
+
 /*
  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset'.
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index bc5f0b8..bc7fb13 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -240,4 +240,62 @@ test_expect_success 'missing commit descriptions in instruction sheet' '
 	test_cmp expect actual
 '
 
+test_expect_success 'revert --continue continues after cherry-pick' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	git revert --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		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	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 'mixed pick and revert instructions' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	oldsha=`git rev-parse --short HEAD~1` &&
+	echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	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_done
-- 
1.7.6.351.gb35ac.dirty

[PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Ramkumar Ramachandra]

Move code from builtin/revert.c to sequencer.c and expose a public API
without making any functional changes.  Although it is useful only to
existing callers of cherry-pick and revert now, this patch lays the
foundation for future expansion.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c | 1001 +-----------------------------------------------------
 sequencer.c      |  958 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 sequencer.h      |   28 ++
 3 files changed, 989 insertions(+), 998 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 483c957..fc818fd 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1,999 +1,6 @@
 #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"
 
-/*
- * This implements the builtins revert and cherry-pick.
- *
- * Copyright (c) 2007 Johannes E. Schindelin
- *
- * Based on git-revert.sh, which is
- *
- * Copyright (c) 2005 Linus Torvalds
- * Copyright (c) 2005 Junio C Hamano
- */
-
-static const char * const revert_usage[] = {
-	"git revert [options] <commit-ish>",
-	"git revert <subcommand>",
-	NULL
-};
-
-static const char * const cherry_pick_usage[] = {
-	"git cherry-pick [options] <commit-ish>",
-	"git cherry-pick <subcommand>",
-	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;
-	int commit_argc;
-	const char **commit_argv;
-
-	/* Merge strategy */
-	const char *strategy;
-	const char **xopts;
-	size_t xopts_nr, xopts_alloc;
-};
-
-#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;
-}
-
-static int option_parse_x(const struct option *opt,
-			  const char *arg, int unset)
-{
-	struct replay_opts **opts_ptr = opt->value;
-	struct replay_opts *opts = *opts_ptr;
-
-	if (unset)
-		return 0;
-
-	ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-	opts->xopts[opts->xopts_nr++] = xstrdup(arg);
-	return 0;
-}
-
-static void verify_opt_compatible(const char *me, const char *base_opt, ...)
-{
-	const char *this_opt;
-	va_list ap;
-
-	va_start(ap, base_opt);
-	while ((this_opt = va_arg(ap, const char *))) {
-		if (va_arg(ap, int))
-			break;
-	}
-	va_end(ap);
-
-	if (this_opt)
-		die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
-}
-
-static void verify_opt_mutually_compatible(const char *me, ...)
-{
-	const char *opt1, *opt2;
-	va_list ap;
-
-	va_start(ap, me);
-	while ((opt1 = va_arg(ap, const char *))) {
-		if (va_arg(ap, int))
-			break;
-	}
-	if (opt1) {
-		while ((opt2 = va_arg(ap, const char *))) {
-			if (va_arg(ap, int))
-				break;
-		}
-	}
-
-	if (opt1 && opt2)
-		die(_("%s: %s cannot be used with %s"),	me, opt1, opt2);
-}
-
-static void parse_args(int argc, const char **argv, struct replay_opts *opts)
-{
-	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
-	const char *me = action_name(opts);
-	int noop;
-	int reset = 0;
-	int contin = 0;
-	struct option options[] = {
-		OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"),
-		OPT_BOOLEAN(0, "continue", &contin, "continue the current operation"),
-		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
-		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
-		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
-		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },
-		OPT_BOOLEAN('s', "signoff", &opts->signoff, "add Signed-off-by:"),
-		OPT_INTEGER('m', "mainline", &opts->mainline, "parent number"),
-		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
-		OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
-		OPT_CALLBACK('X', "strategy-option", &opts, "option",
-			"option for merge strategy", option_parse_x),
-		OPT_END(),
-		OPT_END(),
-		OPT_END(),
-	};
-
-	if (opts->action == REPLAY_PICK) {
-		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
-			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
-			OPT_END(),
-		};
-		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
-			die(_("program error"));
-	}
-
-	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-					PARSE_OPT_KEEP_ARGV0 |
-					PARSE_OPT_KEEP_UNKNOWN);
-
-	/* Check for incompatible subcommands */
-	verify_opt_mutually_compatible(me,
-				"--reset", reset,
-				"--continue", contin,
-				NULL);
-
-	/* Set the subcommand */
-	if (reset)
-		opts->subcommand = REPLAY_RESET;
-	else if (contin)
-		opts->subcommand = REPLAY_CONTINUE;
-	else
-		opts->subcommand = REPLAY_NONE;
-
-	/* Check for incompatible command line arguments */
-	if (opts->subcommand != REPLAY_NONE) {
-		char *this_operation;
-		if (opts->subcommand == REPLAY_RESET)
-			this_operation = "--reset";
-		else
-			this_operation = "--continue";
-
-		verify_opt_compatible(me, this_operation,
-				"--no-commit", opts->no_commit,
-				"--signoff", opts->signoff,
-				"--mainline", opts->mainline,
-				"--strategy", opts->strategy ? 1 : 0,
-				"--strategy-option", opts->xopts ? 1 : 0,
-				"-x", opts->record_origin,
-				"--ff", opts->allow_ff,
-				NULL);
-	}
-
-	else if (opts->commit_argc < 2)
-		usage_with_options(usage_str, options);
-
-	if (opts->allow_ff)
-		verify_opt_compatible(me, "--ff",
-				"--signoff", opts->signoff,
-				"--no-commit", opts->no_commit,
-				"-x", opts->record_origin,
-				"--edit", opts->edit,
-				NULL);
-	opts->commit_argv = argv;
-}
-
-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(void)
-{
-	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;
-	}
-
-	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)
-{
-	struct tree *tree = xcalloc(1, sizeof(struct tree));
-
-	tree->object.parsed = 1;
-	tree->object.type = OBJ_TREE;
-	pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
-	return tree;
-}
-
-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 == REPLAY_REVERT ? "revert" : "cherry-pick",
-			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->no_commit)
-			write_cherry_pick_head(commit);
-	}
-
-	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 (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();
-		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 rev_info *revs, struct replay_opts *opts)
-{
-	int argc;
-
-	init_revisions(revs, NULL);
-	revs->no_walk = 1;
-	if (opts->action != REPLAY_REVERT)
-		revs->reverse = 1;
-
-	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage(opts));
-
-	if (prepare_revision_walk(revs))
-		die(_("revision walk setup failed"));
-
-	if (!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;
- */
-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;
-		struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
-
-		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
-		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
-		if (get_message(cur->operand, &msg))
-			return error(_("Cannot get commit message for %s"), sha1_abbrev);
-		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
-		free_message(&msg);
-	}
-	return 0;
-}
-
-static int parse_insn_line(char *start, struct replay_insn_list *item)
-{
-	unsigned char commit_sha1[20];
-	char sha1_abbrev[40];
-	char *p = start, *q, *end = strchrnul(start, '\n');
-
-	if (!prefixcmp(start, "pick ")) {
-		item->action = REPLAY_PICK;
-		p += strlen("pick ");
-	} else if (!prefixcmp(start, "revert ")) {
-		item->action = REPLAY_REVERT;
-		p += strlen("revert ");
-	} else
-		return error(_("Unrecognized action: %s"), start);
-
-	q = strchrnul(p, ' ');
-	if (q > end)
-		q = end;
-	if (q - p + 1 > sizeof(sha1_abbrev))
-		return error(_("Object name too large: %s"), p);
-	memcpy(sha1_abbrev, p, q - p);
-	sha1_abbrev[q - p] = '\0';
-
-	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
-		return error(_("Malformed object name: %s"), sha1_abbrev);
-
-	item->operand = lookup_commit_reference(commit_sha1);
-	if (!item->operand)
-		return error(_("Not a valid commit: %s"), sha1_abbrev);
-
-	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++) {
-		if (parse_insn_line(p, &item) < 0)
-			return error(_("Could not parse line %d."), i);
-		next = replay_insn_list_append(item.action, item.operand, next);
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
-	}
-	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 rev_info revs;
-	struct commit *commit;
-	struct replay_insn_list **next;
-
-	prepare_revs(&revs, opts);
-
-	next = todo_list;
-	while ((commit = get_revision(&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 && res > 0)
-				/*
-				 * A conflict 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];
-
-	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;
@@ -1004,8 +11,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 		opts.edit = 1;
 	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
-	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	sequencer_parse_args(argc, argv, &opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -1019,8 +26,8 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
-	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	sequencer_parse_args(argc, argv, &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..e72618c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,8 +1,809 @@
 #include "cache.h"
 #include "sequencer.h"
-#include "strbuf.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"
 
+static const char * const revert_usage[] = {
+	"git revert [options] <commit-ish>",
+	"git revert <subcommand>",
+	NULL
+};
+
+static const char * const cherry_pick_usage[] = {
+	"git cherry-pick [options] <commit-ish>",
+	"git cherry-pick <subcommand>",
+	NULL
+};
+
+#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;
+}
+
+static int option_parse_x(const struct option *opt,
+			  const char *arg, int unset)
+{
+	struct replay_opts **opts_ptr = opt->value;
+	struct replay_opts *opts = *opts_ptr;
+
+	if (unset)
+		return 0;
+
+	ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+	opts->xopts[opts->xopts_nr++] = xstrdup(arg);
+	return 0;
+}
+
+static void verify_opt_compatible(const char *me, const char *base_opt, ...)
+{
+	const char *this_opt;
+	va_list ap;
+
+	va_start(ap, base_opt);
+	while ((this_opt = va_arg(ap, const char *))) {
+		if (va_arg(ap, int))
+			break;
+	}
+	va_end(ap);
+
+	if (this_opt)
+		die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
+}
+
+static void verify_opt_mutually_compatible(const char *me, ...)
+{
+	const char *opt1, *opt2;
+	va_list ap;
+
+	va_start(ap, me);
+	while ((opt1 = va_arg(ap, const char *))) {
+		if (va_arg(ap, int))
+			break;
+	}
+	if (opt1) {
+		while ((opt2 = va_arg(ap, const char *))) {
+			if (va_arg(ap, int))
+				break;
+		}
+	}
+
+	if (opt1 && opt2)
+		die(_("%s: %s cannot be used with %s"),	me, opt1, opt2);
+}
+
+void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts)
+{
+	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
+	const char *me = action_name(opts);
+	int noop;
+	int reset = 0;
+	int contin = 0;
+	struct option options[] = {
+		OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"),
+		OPT_BOOLEAN(0, "continue", &contin, "continue the current operation"),
+		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
+		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
+		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },
+		OPT_BOOLEAN('s', "signoff", &opts->signoff, "add Signed-off-by:"),
+		OPT_INTEGER('m', "mainline", &opts->mainline, "parent number"),
+		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
+		OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
+		OPT_CALLBACK('X', "strategy-option", &opts, "option",
+			"option for merge strategy", option_parse_x),
+		OPT_END(),
+		OPT_END(),
+		OPT_END(),
+	};
+
+	if (opts->action == REPLAY_PICK) {
+		struct option cp_extra[] = {
+			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
+			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
+			OPT_END(),
+		};
+		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
+			die(_("program error"));
+	}
+
+	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
+					PARSE_OPT_KEEP_ARGV0 |
+					PARSE_OPT_KEEP_UNKNOWN);
+
+	/* Check for incompatible subcommands */
+	verify_opt_mutually_compatible(me,
+				"--reset", reset,
+				"--continue", contin,
+				NULL);
+
+	/* Set the subcommand */
+	if (reset)
+		opts->subcommand = REPLAY_RESET;
+	else if (contin)
+		opts->subcommand = REPLAY_CONTINUE;
+	else
+		opts->subcommand = REPLAY_NONE;
+
+	/* Check for incompatible command line arguments */
+	if (opts->subcommand != REPLAY_NONE) {
+		char *this_operation;
+		if (opts->subcommand == REPLAY_RESET)
+			this_operation = "--reset";
+		else
+			this_operation = "--continue";
+
+		verify_opt_compatible(me, this_operation,
+				"--no-commit", opts->no_commit,
+				"--signoff", opts->signoff,
+				"--mainline", opts->mainline,
+				"--strategy", opts->strategy ? 1 : 0,
+				"--strategy-option", opts->xopts ? 1 : 0,
+				"-x", opts->record_origin,
+				"--ff", opts->allow_ff,
+				NULL);
+	}
+
+	else if (opts->commit_argc < 2)
+		usage_with_options(usage_str, options);
+
+	if (opts->allow_ff)
+		verify_opt_compatible(me, "--ff",
+				"--signoff", opts->signoff,
+				"--no-commit", opts->no_commit,
+				"-x", opts->record_origin,
+				"--edit", opts->edit,
+				NULL);
+	opts->commit_argv = argv;
+}
+
+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(void)
+{
+	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;
+	}
+
+	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)
+{
+	struct tree *tree = xcalloc(1, sizeof(struct tree));
+
+	tree->object.parsed = 1;
+	tree->object.type = OBJ_TREE;
+	pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
+	return tree;
+}
+
+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 = COMMIT_MESSAGE_INIT;
+	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 == REPLAY_REVERT ? "revert" : "cherry-pick",
+			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->no_commit)
+			write_cherry_pick_head(commit);
+	}
+
+	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 (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();
+		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 rev_info *revs, struct replay_opts *opts)
+{
+	int argc;
+
+	init_revisions(revs, NULL);
+	revs->no_walk = 1;
+	if (opts->action != REPLAY_REVERT)
+		revs->reverse = 1;
+
+	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
+	if (argc > 1)
+		usage(*revert_or_cherry_pick_usage(opts));
+
+	if (prepare_revision_walk(revs))
+		die(_("revision walk setup failed"));
+
+	if (!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;
+ */
+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;
+		struct commit_message msg = COMMIT_MESSAGE_INIT;
+
+		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		if (get_message(cur->operand, &msg))
+			return error(_("Cannot get commit message for %s"), sha1_abbrev);
+		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+		free_message(&msg);
+	}
+	return 0;
+}
+
+static int parse_insn_line(char *start, struct replay_insn_list *item)
+{
+	unsigned char commit_sha1[20];
+	char sha1_abbrev[40];
+	char *p = start, *q, *end = strchrnul(start, '\n');
+
+	if (!prefixcmp(start, "pick ")) {
+		item->action = REPLAY_PICK;
+		p += strlen("pick ");
+	} else if (!prefixcmp(start, "revert ")) {
+		item->action = REPLAY_REVERT;
+		p += strlen("revert ");
+	} else
+		return error(_("Unrecognized action: %s"), start);
+
+	q = strchrnul(p, ' ');
+	if (q > end)
+		q = end;
+	if (q - p + 1 > sizeof(sha1_abbrev))
+		return error(_("Object name too large: %s"), p);
+	memcpy(sha1_abbrev, p, q - p);
+	sha1_abbrev[q - p] = '\0';
+
+	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+		return error(_("Malformed object name: %s"), sha1_abbrev);
+
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return error(_("Not a valid commit: %s"), sha1_abbrev);
+
+	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++) {
+		if (parse_insn_line(p, &item) < 0)
+			return error(_("Could not parse line %d."), i);
+		next = replay_insn_list_append(item.action, item.operand, next);
+		p = strchrnul(p, '\n');
+		if (*p)
+			p++;
+	}
+	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 rev_info revs;
+	struct commit *commit;
+	struct replay_insn_list **next;
+
+	prepare_revs(&revs, opts);
+
+	next = todo_list;
+	while ((commit = get_revision(&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;
+}
+
 void remove_sequencer_state(int aggressive)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -17,3 +818,158 @@ void remove_sequencer_state(int aggressive)
 	strbuf_release(&seq_dir);
 	strbuf_release(&seq_old_dir);
 }
+
+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 && res > 0)
+				/*
+				 * A conflict 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];
+
+	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..ebf20cb 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,7 +7,32 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
+#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };
+
 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;
+	int commit_argc;
+	const char **commit_argv;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+};
 
 struct replay_insn_list {
 	enum replay_action action;
@@ -25,4 +50,7 @@ struct replay_insn_list {
  */
 void remove_sequencer_state(int aggressive);
 
+void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);
+int sequencer_pick_revisions(struct replay_opts *opts);
+
 #endif
-- 
1.7.6.351.gb35ac.dirty

[PATCH 6/6] sequencer: Remove sequencer state after final commit

[Ramkumar Ramachandra]

Since d3f4628e (revert: Remove sequencer state when no commits are
pending, 2011-07-06), the sequencer removes the sequencer state before
the final commit is actually completed.  This design is inherently
flawed, as it will not allow the user to abort the sequencer operation
at that stage.  Instead, write and expose a new function to count the
number of commits left in the instruction sheet; use this in
builtin/commit.c to remove the sequencer state when a commit has
successfully completed and there is only one instruction left in the
sheet.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/commit.c                |    7 ++++++-
 sequencer.c                     |   26 +++++++++++++++-----------
 sequencer.h                     |    1 +
 t/t3510-cherry-pick-sequence.sh |    4 ++--
 4 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..4a5af9a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -26,6 +26,7 @@
 #include "unpack-trees.h"
 #include "quote.h"
 #include "submodule.h"
+#include "sequencer.h"
 
 static const char * const builtin_commit_usage[] = {
 	"git commit [options] [--] <filepattern>...",
@@ -1521,7 +1522,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 
-	if (commit_index_files())
+	/* Remove sequencer state if we just finished the last insn */
+	if (sequencer_count_todo() == 1)
+		remove_sequencer_state(1);
+
+       if (commit_index_files())
 		die (_("Repository has been updated, but unable to write\n"
 		     "new_index file. Check that disk is not full or quota is\n"
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
diff --git a/sequencer.c b/sequencer.c
index e72618c..783b4a9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -736,6 +736,20 @@ static void read_populate_todo(struct replay_insn_list **todo_list)
 		die(_("Unusable instruction sheet: %s"), todo_file);
 }
 
+int sequencer_count_todo(void)
+{
+	struct replay_insn_list *todo_list = NULL;
+	struct replay_insn_list *cur;
+	int insn_count = 0;
+
+	if (!file_exists(git_path(SEQ_TODO_FILE)))
+		return 0;
+	read_populate_todo(&todo_list);
+	for (cur = todo_list; cur; cur = cur->next)
+		insn_count += 1;
+	return insn_count;
+}
+
 static int populate_opts_cb(const char *key, const char *value, void *data)
 {
 	struct replay_opts *opts = data;
@@ -901,18 +915,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 && res > 0)
-				/*
-				 * A conflict 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/sequencer.h b/sequencer.h
index ebf20cb..c64ba91 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -52,5 +52,6 @@ void remove_sequencer_state(int aggressive);
 
 void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);
 int sequencer_pick_revisions(struct replay_opts *opts);
+int sequencer_count_todo(void);
 
 #endif
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index bc7fb13..57e9e7c 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -82,13 +82,13 @@ 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' '
+test_expect_success 'final commit cleans up sequencer state' '
 	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 &&
+	test_path_is_missing .git/sequencer &&
 	{
 		git rev-list HEAD |
 		git diff-tree --root --stdin |
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 0/6] Towards a generalized sequencer

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> No new ideas; just the same ideas implemented in a more sane
> way.

Thanks!  Will try it out.

> Note: I didn't know what to do with the license header in the fifth
> patch.  I just assumed that it was some historical cruft and removed
> it.

Please don't.  Technically it's allowed by the license if I understand
correctly (since the copyright notices are not accompanied by a
disclaimer of warranty) but it's almost always the wrong thing to do
to remove a copyright notice without the author's permission.

Re: [PATCH 1/6] revert: Don't remove the sequencer state on error

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> The cherry-pick/ revert machinery now removes the sequencer state when
> do_pick_commit returns a non-zero, and when only one instruction is
> left in the todo_list.  Since do_pick_commit has a way to distinguish
> errors from conflicts using the signed-ness of the return value,
> utilize this to ensure that the sequencer state is only removed when
> there's a conflict and there is only one instruction left in the
> todo_list.

I'm having trouble parsing this.  Is the idea of this one to mitigate
some of the problems with the "remove sequencer state when a conflict
is encountered in the last commit" hack, by suppressing such behavior
when there is an internal error rather than a conflict?  Why bother,
when the behavior is suppressed altogether later in the series?

Re: [PATCH 2/6] revert: Free memory after get_message call

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> The format_todo function leaks memory because it forgets to call
> free_message after get_message.  Fix this.
>
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

That's "Reported-by", I think. :)

Is this a big leak or a small one?  Is it one-time or in a loop?

> ---
>  builtin/revert.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index a548a14..1a4187a 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -688,6 +688,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
>  			return error(_("Cannot get commit message for %s"), sha1_abbrev);
>  		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
>  	}
> +	free_message(&msg);
>  	return 0;
>  }

I don't see how this could work.  Since there an xmalloc() in each
loop iteration, I would have expected the free() to be in the loop
body, too.

Re: [PATCH 3/6] revert: Parse instruction sheet more cautiously

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Fix a buffer overflow bug by checking that parsed SHA-1 hex will fit
> in the buffer we've created for it.

Nit: it seems best to either describe the behavior or the code change.
So, for example:

	Do not overflow a buffer when the second word in a "pick"
	or "revert" line in .git/sequencer/todo is very long.

Or:

	Check that the commit name argument to a "pick" or "revert"
	action is not too long, to avoid overflowing an on-stack
	buffer.

It would also be comforting to readers to mention when the bug was
introduced, so they don't start worrying about protecting their
installations against privilege escalation:

	This fixes a regression introduced by <...>, which has not
	escaped into the wild yet, luckily.

Could we have a test for this?

> Also change the instruction sheet format

How is this "Also" part related to the earlier bit?  Does one make the
other easier, or is it more of a "While we're changing this code" kind
of thing?

> subtly so that a description of the commit after the object
> name is optional.  So now, an instruction sheet like this is perfectly
> valid:
>
>   pick 35b0426
>   pick fbd5bbcbc2e
>   pick 7362160f

Sounds convenient. :)  Thanks.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -697,26 +697,24 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
>  	unsigned char commit_sha1[20];
>  	char sha1_abbrev[40];
>  	enum replay_action action;
> -	int insn_len = 0;
> -	char *p, *q;
> +	char *p = start, *q, *end = strchrnul(start, '\n');

By the way, why are these non-const?  (Not about this patch.)

I don't know why, but I would be more comfortable reading something
like this:

	const char *p, *q;

	p = start;
	if (!prefixcmp(p, "pick ")) {
		action = CHERRY_PICK;
		p += strlen("pick ");
	} else if (...) {
		...
	} ...

	q = p + strcspn(p, " \n");
	if (q - p + 1 > sizeof(...))
		...
	...

Maybe because I can imagine how the pointers move, always forward and
never past the end of the line.

> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -211,4 +211,33 @@ test_expect_success 'malformed instruction sheet 2' '
>  	test_must_fail git cherry-pick --continue
>  '
>  
> +test_expect_success 'missing commit descriptions in instruction sheet' '

What assertion does this test check?  "commit descriptions in insn
sheet are optional"?

> +	pristine_detach initial &&
> +	test_must_fail git cherry-pick base..anotherpick &&

A failed cherry-pick.

> +	echo "c" >foo &&
> +	git add foo &&
> +	git commit &&

Resolving the conflict.

> +	cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
> +	cp new_sheet .git/sequencer/todo &&

Mucking about with the insn sheet.

> +	git cherry-pick --continue &&

Continuing.

> +	test_path_is_missing .git/sequencer &&
> +	{
> +		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	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

Checking that the cherry-pick succeeded and the resulting list of
commits.  How is this expected to potentially fail?  Maybe something
like

	git rev-list HEAD >commits &&
	test_line_count = 4 commits

or

	git diff --exit-code <something>

would make what this is intended to check clearer.  As hinted above,
some blank lines or comments might make the earlier part easier to
read.

Thanks, this patch does two good things.  For what it's worth, with
the changes hinted at above and a split into two patches if that seems
sensible,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 4/6] revert: Allow mixed pick and revert instructions

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Change the way the instruction parser works, allowing arbitrary
> (action, operand) pairs to be parsed.

The first part of this sentence is not very satisfying.  Maybe it
means something like

	Parse the instruction list in .git/sequencer/todo as a list
	of (action, operand) pairs, instead of assuming all instructions
	use the same action.

[...]
> This patch lays the foundation for extending the parser to support
> more actions so 'git rebase -i' can reuse this machinery in the
> future.

Exciting stuff. :)

[...]
> +++ b/builtin/revert.c
[...]
> @@ -457,7 +456,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
>  	return run_command_v_opt(args, RUN_GIT_CMD);
>  }
>  
> -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> +static int do_pick_commit(struct commit *commit, enum replay_action action,
> +			struct replay_opts *opts)
[...]
> @@ -517,7 +517,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  		/* 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));
> +			action == REPLAY_REVERT ? "revert" : "cherry-pick",
> +			sha1_to_hex(parent->object.sha1));

My first thought was "why stop using the helper function action_name"?
But now I see that it previously came from "opts" (i.e., the command
line) and now comes from the todo file.

The command name there was never really important except when
cherry-pick or revert is being called by a script, and the message
indicates which command was having trouble parsing the commit.  If I
am using "git cherry-pick --continue" to continue after a failed
revert, I suspect action_name(opts) ["cherry-pick: "] would actually be
more sensible than the command name corresponding to the particular
pick/revert line.

[...]
> -		return NULL;
> +		return error(_("Unrecognized action: %s"), start);

Probably should be mentioned in the commit message.  Doesn't this
print the problematic line and all lines after it?

Maybe something like

	len = strchrnul(p, '\n') - p;
	if (len > 255)
		len = 255;
	return error(_("Unrecognized action: %.*s"), (int) len, p);

would do.

[...]
> -static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
> -			struct replay_opts *opts)
> +static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>  {
> -	struct commit_list **next = todo_list;
> -	struct commit *commit;
> +	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++) {
> -		commit = parse_insn_line(p, opts);
> -		if (!commit)
> +		if (parse_insn_line(p, &item) < 0)
>  			return error(_("Could not parse line %d."), i);

Could we can make this error message more clearly suggest that it's
giving context to the error above it?  For example, something vaguely
like

	error: unrecognized action: reset c78a78c9 Going back
	error: on line 7
	fatal: unusable instruction sheet ".git/sequencer/todo"
	hint: to continue after fixing it, use "git cherry-pick --continue"
	hint: or to bail out, use "git cherry-pick --abort"

Does a "cherry-pick --continue" in this scenario skip the first commit
in the todo list?  Should it?

> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -240,4 +240,62 @@ test_expect_success 'missing commit descriptions in instruction sheet' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'revert --continue continues after cherry-pick' '

Haven't read the tests yet.  The general idea of this patch still
seems very sane.

Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -7,7 +7,32 @@
>  #define SEQ_TODO_FILE	"sequencer/todo"
>  #define SEQ_OPTS_FILE	"sequencer/opts"
>  
> +#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };

I don't think this should be exposed.  The rest seems pretty sane,
though I haven't read the patch carefully.

Re: [PATCH 6/6] sequencer: Remove sequencer state after final commit

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -26,6 +26,7 @@
>  #include "unpack-trees.h"
>  #include "quote.h"
>  #include "submodule.h"
> +#include "sequencer.h"
>  
>  static const char * const builtin_commit_usage[] = {
>  	"git commit [options] [--] <filepattern>...",
> @@ -1521,7 +1522,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	unlink(git_path("MERGE_MODE"));
>  	unlink(git_path("SQUASH_MSG"));
>  
> -	if (commit_index_files())
> +	/* Remove sequencer state if we just finished the last insn */
> +	if (sequencer_count_todo() == 1)
> +		remove_sequencer_state(1);
> +
> +       if (commit_index_files())
>  		die (_("Repository has been updated, but unable to write\n"

Whitespace damage?

I think even this is too much knowledge of sequencer details on "git
commit"'s part.  It should be enough to say

	Dear sequencer, I'm commiting!

and let the sequencer take care of the rest.

Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> +++ b/sequencer.c
> @@ -1,8 +1,809 @@
[...]
> +static const char * const revert_usage[] = {
> +	"git revert [options] <commit-ish>",
> +	"git revert <subcommand>",
> +	NULL
> +};
[...]
> +++ b/sequencer.h
[...]
> @@ -25,4 +50,7 @@ struct replay_insn_list {
>   */
>  void remove_sequencer_state(int aggressive);
>
> +void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);

Another thought.  I wonder if it's possible to leave
sequencer_parse_args() private to builtin/revert.c, making the split
a little more logical:

 - the builtin takes responsibility for its commandline interface
 - the library takes over once the builtin has figured out what the
   user wanted.

(If another command wants to reuse some subset of cherry-pick/revert's
commandline options, an appropriate function can always be exposed
later.)

Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Another thought.  I wonder if it's possible to leave
> sequencer_parse_args() private to builtin/revert.c, making the split
> a little more logical:
>
>  - the builtin takes responsibility for its commandline interface
>  - the library takes over once the builtin has figured out what the
>    user wanted.

That's a very natural separation of tasks.

Re: [PATCH 2/6] revert: Free memory after get_message call

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> I don't see how this could work.  Since there an xmalloc() in each
> loop iteration, I would have expected the free() to be in the loop
> body, too.

Oops, fixed.  Thanks.

-- Ram

Re: [PATCH 0/6] Towards a generalized sequencer

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Note: I didn't know what to do with the license header in the fifth
>> patch.  I just assumed that it was some historical cruft and removed
>> it.
>
> Please don't.  Technically it's allowed by the license if I understand
> correctly (since the copyright notices are not accompanied by a
> disclaimer of warranty) but it's almost always the wrong thing to do
> to remove a copyright notice without the author's permission.

Um, ok.  I still don't know what to do:
1. Should I leave it in builtin/revert.c?  There are only a few tiny
functions left there.
2. Should I move it to sequencer.c and and modify it accordingly?
It'll read Copyright (C) 2011 <Me>, based on builtin/revert.c which is
Copyright Dscho, which in turn is based on git-revert.sh Copyright
Linus and Junio?  Isn't this information redundant? Can't `log` and
`blame -M -C` do a much better job?

Thanks.

-- Ram

Re: [PATCH 6/6] sequencer: Remove sequencer state after final commit

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
>        Dear sequencer, I'm commiting!

Interesting.  I was worried that sequencer_count_todo() might be a bit
of an overkill.  I can't find any other usecase for counting the
number of commits in the instruction sheet right now, and it's
unlikely that I'll find any callers for it in the future either.  I
suppose we can save time by just counting until we figure out that
more than one commit exists -- there's no need to find the exact
number.

Thanks.

-- Ram

Re: [PATCH 0/6] Towards a generalized sequencer

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> 2. Should I move it to sequencer.c and and modify it accordingly?
> It'll read Copyright (C) 2011 <Me>, based on builtin/revert.c which is
> Copyright Dscho, which in turn is based on git-revert.sh
[...]

Isn't it still the same built-in cherry-pick code, originally by Dscho
and improved over time by others?  The filename is irrelevant.

If you want to add your and Christian's names to reflect
multiple-cherry-pick and the restructuring, I guess that would be ok.
Better yet, if you want to remove Dscho's name to encourage people to
look at the commit log and get a richer story, just ask Dscho.

Asking would (1) show that you value his contribution and respect his
wishes and (2) allow him to say, "no, I really prefer to keep my name
there for such-and-such copyright-related reason".  Of course these
are only my own thoughts.  Maybe I'm too picky.

Re: [PATCH 0/6] Towards a generalized sequencer

[Ramkumar Ramachandra]

Hi,

[+CC: Johannes Schindelin]

Jonathan Nieder writes:
> Isn't it still the same built-in cherry-pick code, originally by Dscho
> and improved over time by others?  The filename is irrelevant.

True, but we can't have "This implements the builtins revert and
cherry-pick." in sequencer.c, no?

> If you want to add your and Christian's names to reflect
> multiple-cherry-pick and the restructuring, I guess that would be ok.
> Better yet, if you want to remove Dscho's name to encourage people to
> look at the commit log and get a richer story, just ask Dscho.

I thought about this a bit.  I'm completely against the former option
-- I merely meant it as a "I'm confused; help!" rather than a
suggestion; adding contributors' names to the list is a bad idea.  I
looked at some copyright notices in other files and re-evaluated: it's
a cute historical note to have. Unless Johannes wants to remove it
now, I'd like to put the following in sequencer.c:

/*
 * Used to implement the builtins revert and cherry-pick.
 *
 * Copyright (c) 2007 Johannes E. Schindelin
 *
 * Based on git-revert.sh, which is
 *
 * Copyright (c) 2005 Linus Torvalds
 * Copyright (c) 2005 Junio C Hamano
 */

Sorry about the confusion. Feel free to suggest a better "title line".

Thanks.

-- Ram

Re: [PATCH 1/6] revert: Don't remove the sequencer state on error

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Why bother,
> when the behavior is suppressed altogether later in the series?

Actually, this patch helped me while debugging:  I'd originally
intended for it to go into 'maint'.  Things have changed though- the
'sequencer-stable' is close to hitting 'next', and this series seems
to be close behind*; I'll drop this patch altogether during the
re-roll.

Thanks for making me explain this :)

-- Ram

Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -7,7 +7,32 @@
>>  #define SEQ_TODO_FILE        "sequencer/todo"
>>  #define SEQ_OPTS_FILE        "sequencer/opts"
>>
>> +#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };
>
> I don't think this should be exposed.  The rest seems pretty sane,
> though I haven't read the patch carefully.

Gah, my stupidity.  What sense does it make to expose
COMMIT_MESSAGE_INIT when 'struct commit_message' itself isn't?  Moved
to sequencer.c now, thanks.

> Another thought.  I wonder if it's possible to leave
> sequencer_parse_args() private to builtin/revert.c, making the split
> a little more logical:

Yes, I'd like this too.  However, there are two new issues:
revert_or_cherry_pick_usage and action_name.  The former has two
callsites: one in prepare_revs (in sequencer.c) and another in
parse_args (in builtin/revert.c).  Unfortunately, action_name is even
more complicated to get rid of: the information from it is used all
over the place.  Attempting to attack the problems one by one:
1. Make prepare_revs and walk_revs_populate_todo return errors to be
handled by callers.  This is a fairly small patch that can come before
the big "code moving patch".
2. Duplicate action_name in both files.  I don't think it's too
serious, and we can fix this later.

It has been enormously annoying to work with this "code moving patch":
everytime I make some changes to the earlier patches, I have to
recreate this one by hand; rebase offers no help whatsoever.  After
throwing away code based on this patch several times, I learnt my
lesson and restricted my series to avoid building on this patch.  I
consider this a very serious glitch* and I'm interested in fixing it.
Thoughts?

Thanks.

* We don't track renames, and I fully subscribe to that design.
However, that doesn't prevent us from building small helpers.

-- Ram

Re: [PATCH 4/6] revert: Allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Change the way the instruction parser works, allowing arbitrary
>> (action, operand) pairs to be parsed.
>
>        Parse the instruction list in .git/sequencer/todo as a list
>        of (action, operand) pairs, instead of assuming all instructions
>        use the same action.

Fixed.  Thanks :)

>> @@ -517,7 +517,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>>               /* 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));
>> +                     action == REPLAY_REVERT ? "revert" : "cherry-pick",
>> +                     sha1_to_hex(parent->object.sha1));
>
> My first thought was "why stop using the helper function action_name"?
> But now I see that it previously came from "opts" (i.e., the command
> line) and now comes from the todo file.
>
> The command name there was never really important except when
> cherry-pick or revert is being called by a script, and the message
> indicates which command was having trouble parsing the commit.  If I
> am using "git cherry-pick --continue" to continue after a failed
> revert, I suspect action_name(opts) ["cherry-pick: "] would actually be
> more sensible than the command name corresponding to the particular
> pick/revert line.

You're right.  Removed this hunk.

> Maybe something like
>
>        len = strchrnul(p, '\n') - p;
>        if (len > 255)
>                len = 255;
>        return error(_("Unrecognized action: %.*s"), (int) len, p);
>
> would do.

Excellent idea!  I fixed the buffer overflow message to do this too.
By the way, shouldn't error() do this?

> Could we can make this error message more clearly suggest that it's
> giving context to the error above it?  For example, something vaguely
> like
>
>        error: unrecognized action: reset c78a78c9 Going back
>        error: on line 7

Good suggestion.  Fixed.

> Does a "cherry-pick --continue" in this scenario skip the first commit
> in the todo list?  Should it?

It shouldn't and it doesn't.  See how read_populate_opts and
read_populate_todo are called before the segment that drops the first
commit.

-- Ram

Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3676 bytes --]

On Sat, 13 Aug 2011, Ramkumar Ramachandra wrote:

> Hi again,
> 
> Jonathan Nieder writes:
> > Another thought.  I wonder if it's possible to leave
> > sequencer_parse_args() private to builtin/revert.c, making the split
> > a little more logical:
> 
> Yes, I'd like this too.  However, there are two new issues:
> revert_or_cherry_pick_usage and action_name.  The former has two
> callsites: one in prepare_revs (in sequencer.c) and another in
> parse_args (in builtin/revert.c).  Unfortunately, action_name is even
> more complicated to get rid of: the information from it is used all
> over the place.

One thing to consider is that sequencer.c will be used by all sorts of 
different builtins, which are each implementing instructions given 
differently by the user. Just because a message makes sense as output from 
revert.c doesn't mean it will make sense from (for example) bisect*.

> Attempting to attack the problems one by one:
> 1. Make prepare_revs and walk_revs_populate_todo return errors to be
> handled by callers.  This is a fairly small patch that can come before
> the big "code moving patch".

This makes sense. If you type "git log --stat=foo", you don't get a 
diff usage message, even though it's an error parsing options that were 
originally part of diff.

> 2. Duplicate action_name in both files.  I don't think it's too
> serious, and we can fix this later.

This is actually probably even a good idea, because the two functions 
might actually want to give different results. Maybe revert.c will end up 
doing different sequencer operations depending on whether the commit is a 
merge, but if revert.c has to give an error, it would call it the same 
thing either way, because the difference doesn't matter at the level of 
detail the revert.c works at; on the other hand, sequencer.c would want to 
distinguish the cases so that it is explaining exactly what it's trying to 
do in this step because it matters to how the issue would be resolved.

Of course, at the point where you move the code, you only have one piece 
of code that you're starting from, so they'll be the same. But you might 
want to name them differently.

> It has been enormously annoying to work with this "code moving patch":
> everytime I make some changes to the earlier patches, I have to
> recreate this one by hand; rebase offers no help whatsoever.  After
> throwing away code based on this patch several times, I learnt my
> lesson and restricted my series to avoid building on this patch.  I
> consider this a very serious glitch and I'm interested in fixing it.
> Thoughts?

It's a hard problem, although likely worthwhile to solve. But only when 
you're not doing anything else, because it's complicated and will take you 
far afield. Essentially, what you need to do is implement a diff algorithm 
that can detect reorganization (or copying) of sections; this isn't 
something you can represent in unified diff output, but that's okay 
because you're not going to output it. You merge two of these results and 
apply the result to the base, which gives you a file (potentially with 
conflicts, which is another interesting issue because you have to 
represent and explain them somehow).

*: bisect could be using sequencer in order to handle the situation where 
the user has said "commit A is good, commit B is bad, commit C breaks my 
system in a way that's unrelated"; the system should then be able to check 
out a maybe-bad commit and revert C from it, but it would be doing this in 
response to an instruction from the user: "give me something to test 
next", and would have to present errors differently.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> Another thought.  I wonder if it's possible to leave
>> sequencer_parse_args() private to builtin/revert.c, making the split
>> a little more logical:
>
> Yes, I'd like this too.  However, there are two new issues:
> revert_or_cherry_pick_usage and action_name.  The former has two
> callsites: one in prepare_revs (in sequencer.c) and another in
> parse_args (in builtin/revert.c).

So it sounds like the answer is "no, it's not possible without
further changes".  Alas. :)  Thanks for checking.

(Q. Wait, what further changes?

 A. The above suggests that the setup_revisions call should also be
    the responsibility of the builtin, and that it could communicate
    revs and other rev-list options using a

	struct rev_info *revs;

    instead of

	int commit_argc;
	const char **commit_argv;

    Like this, maybe:

 builtin/revert.c |   52 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 30 insertions(+), 22 deletions(-)

diff --git i/builtin/revert.c w/builtin/revert.c
index 8b452e81..f602ece0 100644
--- i/builtin/revert.c
+++ w/builtin/revert.c
@@ -55,13 +55,14 @@ struct replay_opts {
 	int allow_rerere_auto;
 
 	int mainline;
-	int commit_argc;
-	const char **commit_argv;
 
 	/* Merge strategy */
 	const char *strategy;
 	const char **xopts;
 	size_t xopts_nr, xopts_alloc;
+
+	/* Only used by the default subcommand ("git revert <revs>") */
+	struct rev_info *revs;
 };
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -164,7 +165,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			die(_("program error"));
 	}
 
-	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
+	argc = parse_options(argc, argv, NULL, options, usage_str,
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
 
@@ -201,9 +202,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 	}
 
-	else if (opts->commit_argc < 2)
-		usage_with_options(usage_str, options);
-
 	if (opts->allow_ff)
 		verify_opt_compatible(me, "--ff",
 				"--signoff", opts->signoff,
@@ -211,7 +209,23 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
-	opts->commit_argv = argv;
+
+	if (opts->subcommand != REPLAY_NONE) {
+		opts->revs = NULL;
+		if (argc > 1)
+			usage_with_options(usage_str, options);
+	} else {
+		opts->revs = xmalloc(sizeof(*opts->revs));
+		init_revisions(opts->revs, NULL);
+		opts->revs->no_walk = 1;
+
+		if (argc < 2)
+			usage_with_options(usage_str, options);
+
+		argc = setup_revisions(argc, argv, opts->revs, NULL);
+		if (argc > 1)
+			usage_with_options(usage_str, options);
+	}
 }
 
 struct commit_message {
@@ -612,23 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
+static void prepare_revs(struct replay_opts *opts)
 {
-	int argc;
-
-	init_revisions(revs, NULL);
-	revs->no_walk = 1;
 	if (opts->action != REVERT)
-		revs->reverse = 1;
+		opts->revs->reverse ^= 1;
 
-	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage(opts));
-
-	if (prepare_revision_walk(revs))
+	if (prepare_revision_walk(opts->revs))
 		die(_("revision walk setup failed"));
 
-	if (!revs->commits)
+	if (!opts->revs->commits)
 		die(_("empty commit set passed"));
 }
 
@@ -825,14 +831,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 static void walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
-	struct rev_info revs;
 	struct commit *commit;
 	struct commit_list **next;
 
-	prepare_revs(&revs, opts);
+	prepare_revs(opts);
 
 	next = todo_list;
-	while ((commit = get_revision(&revs)))
+	while ((commit = get_revision(opts->revs)))
 		next = commit_list_append(commit, next);
 }
 
@@ -955,6 +960,9 @@ static int pick_revisions(struct replay_opts *opts)
 	struct commit_list *todo_list = NULL;
 	unsigned char sha1[20];
 
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
 	read_and_refresh_cache(opts);
 
 	/*
-- 
)

Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Ramkumar Ramachandra]

Hi Daniel,

Daniel Barkalow writes:
>> 1. Make prepare_revs and walk_revs_populate_todo return errors to be
>> handled by callers.  This is a fairly small patch that can come before
>> the big "code moving patch".
>
> This makes sense. If you type "git log --stat=foo", you don't get a
> diff usage message, even though it's an error parsing options that were
> originally part of diff.

Yes :) I really liked the "revert: Propagate errors upwards from
do_pick_commit" patch.  I hope we improve the error handling in other
parts of Git soon.

>> 2. Duplicate action_name in both files.  I don't think it's too
>> serious, and we can fix this later.
>
> This is actually probably even a good idea, because the two functions
> might actually want to give different results. Maybe revert.c will end up
> doing different sequencer operations depending on whether the commit is a
> merge, but if revert.c has to give an error, it would call it the same
> thing either way, because the difference doesn't matter at the level of
> detail the revert.c works at; on the other hand, sequencer.c would want to
> distinguish the cases so that it is explaining exactly what it's trying to
> do in this step because it matters to how the issue would be resolved.
>
> Of course, at the point where you move the code, you only have one piece
> of code that you're starting from, so they'll be the same. But you might
> want to name them differently.

Excellent suggestion- I want to create a differentiation between the
sequencer "pick" action and the builtin "cherry-pick" operation.  I
mentioned earlier that I wanted the sequencer to be more than a fast
git-shell.  It'll be interesting to see the kinds of composite
"actions" we invent later; especially ones that only make sense in the
context of sequencing commits.

>> It has been enormously annoying to work with this "code moving patch":
>> everytime I make some changes to the earlier patches, I have to
>> recreate this one by hand; rebase offers no help whatsoever.  After
>> throwing away code based on this patch several times, I learnt my
>> lesson and restricted my series to avoid building on this patch.  I
>> consider this a very serious glitch and I'm interested in fixing it.
>> Thoughts?
>
> It's a hard problem, although likely worthwhile to solve. But only when
> you're not doing anything else, because it's complicated and will take you
> far afield. Essentially, what you need to do is implement a diff algorithm
> that can detect reorganization (or copying) of sections; this isn't
> something you can represent in unified diff output, but that's okay
> because you're not going to output it. You merge two of these results and
> apply the result to the base, which gives you a file (potentially with
> conflicts, which is another interesting issue because you have to
> represent and explain them somehow).

Hm, I was actually thinking of a much less ambitious helper that would
kick in when the heuristic correlation between two files is above a
certain threshold.  Your idea is better!  It would be totally awesome
if we could modify the diffing algorithm to work between files,
although I can't even imagine where to start.  Oh, and I think we have
to go far beyond the traditional in-file conflict markers to resolve
conflicts.  Pretty insane challenge :D  I'm definitely not experienced
enough to take this on now, although I hope to be good enough someday.

> *: bisect could be using sequencer in order to handle the situation where
> the user has said "commit A is good, commit B is bad, commit C breaks my
> system in a way that's unrelated"; the system should then be able to check
> out a maybe-bad commit and revert C from it, but it would be doing this in
> response to an instruction from the user: "give me something to test
> next", and would have to present errors differently.

Thanks for the interesting sidenote.

-- Ram

Reusing changes after renaming a file (Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery)

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Hm, I was actually thinking of a much less ambitious helper that would
> kick in when the heuristic correlation between two files is above a
> certain threshold.

Doesn't that already work?  For example:

	git cherry-pick -Xrename-threshold=50 foo

Re: Reusing changes after renaming a file (Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery)

[Ramkumar Ramachandra]

Hi Jonathan,

Disclaimer: I'm sorry if I sound incredibly stupid.  I know nothing
about how the diffing algorithm works.

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Hm, I was actually thinking of a much less ambitious helper that would
>> kick in when the heuristic correlation between two files is above a
>> certain threshold.
>
> Doesn't that already work?  For example:
>
>        git cherry-pick -Xrename-threshold=50 foo

Okay, I'm still trying to figure out what this does.  It's supposed to
"influence the rename detection" according to the 1.7.4 release notes.
 I tried:

$ git cherry-pick -Xrename-threshold=90 sequencer~1
[test 7400609] sequencer: Expose API to cherry-picking machinery
 3 files changed, 256 insertions(+), 1247 deletions(-)
 rewrite builtin/revert.c (81%)
 rename builtin/revert.c => sequencer.c (81%)

Is it supposed to influence the diffstat?  I don't see any conflict
markers in sequencer.c.  I can see lots of unintelligent (and useless)
conflict markers in builtin/revert.c.  What am I supposed to be
looking at?

-- Ram

Re: Reusing changes after renaming a file (Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery)

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> $ git cherry-pick -Xrename-threshold=90 sequencer~1
> [test 7400609] sequencer: Expose API to cherry-picking machinery
>  3 files changed, 256 insertions(+), 1247 deletions(-)
>  rewrite builtin/revert.c (81%)
>  rename builtin/revert.c => sequencer.c (81%)
>
> Is it supposed to influence the diffstat?  I don't see any conflict
> markers in sequencer.c.  I can see lots of unintelligent (and useless)
> conflict markers in builtin/revert.c.

Perhaps "git merge-recursive" and its callers don't use the equivalent
of -B, so it doesn't look like a rename to them.  Cases involving
plain renames work fine for me.

Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
>  builtin/revert.c |   52 ++++++++++++++++++++++++++++++----------------------
>  1 files changed, 30 insertions(+), 22 deletions(-)

Nice! That fixes everything :)
I'll send out a fresh iteration in the morning.

Thanks.

-- Ram