[PATCH v2 0/7] Generalized sequencer foundations

[PATCH v2 0/7] Generalized sequencer foundations

[Ramkumar Ramachandra]

Hi,

This is the second iternation prepared mainly in response to
Jonathan's reviews.  It's ready for inclusion (or very nearly so) now.
I'll start working on more features as soon as I'm confident that this
series doesn't need to be rewritten.

Thanks.

-- Ram

Ramkumar Ramachandra (7):
  revert: Free memory after get_message call
  revert: Fix buffer overflow in insn sheet parser
  revert: Make commit descriptions in insn sheet optional
  revert: Allow mixed pick and revert instructions
  revert: Make the argument parser responsible for setup_revisions
  sequencer: Expose API to cherry-picking machinery
  sequencer: Remove sequencer state after final commit

 builtin/commit.c                   |    8 +
 builtin/revert.c                   |  858 +-----------------------------------
 sequencer.c                        |  806 +++++++++++++++++++++++++++++++++-
 sequencer.h                        |   36 ++
 t/t3032-merge-recursive-options.sh |    2 +
 t/t3510-cherry-pick-sequence.sh    |   87 ++++-
 6 files changed, 956 insertions(+), 841 deletions(-)

-- 
1.7.6.351.gb35ac.dirty

[PATCH 1/7] 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.  It is a potentially big leak, because
fresh memory is allocated to store the commit message message for each
commit.  Fix this.

Reported-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 8b452e8..8a35bfd 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -687,6 +687,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 		if (get_message(cur->item, &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;
 }
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 1/7] 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.  It is a potentially big leak, because
> fresh memory is allocated to store the commit message message for each
> commit.  Fix this.

Reading this, I think, "why does formatting an insn list require all
these heavy memory allocations?".  So while the patch looks good to
me, I start to wonder if something using find_commit_subject()
directly would not be easier.

[PATCH 2/7] revert: Fix buffer overflow in insn sheet parser

[Ramkumar Ramachandra]

Check that the commit name argument to a "pick" or "revert" action in
'.git/sequencer/todo' is not too long, to avoid overflowing an
on-stack buffer.  This fixes a regression introduced by a037033b
(revert: Introduce --continue to continue the operation, 2011-07-27).

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 8a35bfd..8d29003 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -712,7 +712,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 		return NULL;
 
 	q = strchr(p, ' ');
-	if (!q)
+	if (!q || q - p + 1 > sizeof(sha1_abbrev))
 		return NULL;
 	q++;
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3bca2b3..6882acb 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -211,4 +211,15 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'malformed instruction sheet 3' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick \([0-9a-f]\+\)\(.*\)/pick \1\1\1\1\1\1\1\1\2/" .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	test_must_fail git cherry-pick --continue
+'
+
 test_done
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 2/7] revert: Fix buffer overflow in insn sheet parser

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -211,4 +211,15 @@ test_expect_success 'malformed instruction sheet 2' '
>  	test_must_fail git cherry-pick --continue
>  '
>  
> +test_expect_success 'malformed instruction sheet 3' '
> +	pristine_detach initial &&
> +	test_must_fail git cherry-pick base..anotherpick &&
> +	echo "resolved" >foo &&
> +	git add foo &&
> +	git commit &&
> +	sed "s/pick \([0-9a-f]\+\)\(.*\)/pick \1\1\1\1\1\1\1\1\2/" .git/sequencer/todo >new_sheet &&

This construct (\+ in sed regexes) is not portable.  See the note on
grep in Documentation/CodingGuidelines (maybe it should be tweaked to
say "grep and sed").

Re: [PATCH 2/7] revert: Fix buffer overflow in insn sheet parser

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
>> @@ -211,4 +211,15 @@ test_expect_success 'malformed instruction sheet 2' '
>> [...]
>> +     sed "s/pick \([0-9a-f]\+\)\(.*\)/pick \1\1\1\1\1\1\1\1\2/" .git/sequencer/todo >new_sheet &&
>
> This construct (\+ in sed regexes) is not portable.  See the note on
> grep in Documentation/CodingGuidelines

I see.  Could you show me how to do it right?  My regex-foo is pretty
weak, and I don't know what else to do.

> (maybe it should be tweaked to
> say "grep and sed").

Okay.  Do you like this?

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
-- 8< --
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index fe1c1e5..0a843ea 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -69,8 +69,8 @@ For shell scripts specifically (not exhaustive):
  - We do not write the noiseword "function" in front of shell
    functions.

- - As to use of grep, stick to a subset of BRE (namely, no \{m,n\},
-   [::], [==], nor [..]) for portability.
+ - As to use of grep and sed, stick to a subset of BRE (namely, no
+   \{m,n\}, [::], [==], nor [..]) for portability.

    - We do not use \{m,n\};

-- Ram

[PATCH 3/7] revert: Make commit descriptions in insn sheet optional

[Ramkumar Ramachandra]

Change the instruction sheet format subtly so that a description of
the commit after the object name is optional.  As a result, an
instruction sheet like this is now perfectly valid:

  pick 35b0426
  pick fbd5bbcbc2e
  pick 7362160f

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 8d29003..89b2bba 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -697,26 +697,23 @@ 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;
+	const char *p, *q;
 
+	p = start;
 	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 - p + 1 > sizeof(sha1_abbrev))
+	q = p + strcspn(p, " \n");
+	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 6882acb..734ee12 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -222,4 +222,18 @@ test_expect_success 'malformed instruction sheet 3' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'commit descriptions in insn sheet are optional' '
+	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 >commits
+	test_line_count = 4 commits
+'
+
 test_done
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 3/7] revert: Make commit descriptions in insn sheet optional

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -697,26 +697,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
>  	unsigned char commit_sha1[20];
>  	char sha1_abbrev[40];
[...]
> +	q = p + strcspn(p, " \n");
> +	if (q - p + 1 > sizeof(sha1_abbrev))
>  		return NULL;

If I understand correctly, the current code makes it perfectly possible
to use commands like

	pick origin/master^^2~5

when that is more convenient to type.  So I wonder whether 40 is actually
a good limit.  Another possibility would be to do something like

	static struct strbuf cmit = STRBUF_INIT;

	strbuf_reset(&cmit);
	...
	strbuf_add(&cmit, p, q - p);

to avoid having to worry about the buffer size altogether.

Re: [PATCH 3/7] revert: Make commit descriptions in insn sheet optional

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> If I understand correctly, the current code makes it perfectly possible
> to use commands like
>
>        pick origin/master^^2~5

I was thinking exactly the same thing a few hours ago!  I was
wondering what to do about such a tree'ish specification, and I came
to the conclusion that we should discourage this atleast for normal
users.  Why?  Because the tree'ish has to be invariant over the entire
sequencer operation*: something like "pick HEAD~3" is clearly flawed
since the picking operation updates HEAD.

* Some of us might find some really clever uses for a tree'ish that
refers to different commits during the course of the sequencer
operation, and I'm alright with that.  It might just be something
that's too complicated for many users to grok, and we can protect them
by not advertising this tree'ish operand feature.

> when that is more convenient to type.  So I wonder whether 40 is actually
> a good limit.  Another possibility would be to do something like
>
>        static struct strbuf cmit = STRBUF_INIT;

I don't like this; the sequencer will just end up drinking a lot of
memory for no reason if the insn sheet is horribly corrupted.  I think
40 is a very reasonable limit, even for the most complex tree'ish I
can dream of.

Thanks.

-- Ram

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

[Ramkumar Ramachandra]

Parse the instruction list in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming all instructions use the
same action.  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.  While at it, also improve the error messages reported by the
insn sheet parser.

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                |  137 ++++++++++++++++++++-------------------
 sequencer.h                     |    8 ++
 t/t3510-cherry-pick-sequence.sh |   58 ++++++++++++++++
 3 files changed, 135 insertions(+), 68 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 89b2bba..8faca07 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;
@@ -532,7 +532,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 +575,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 +594,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 +618,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,27 +664,29 @@ 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);
@@ -692,59 +694,58 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 	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;
 	const char *p, *q;
 
 	p = start;
 	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;
+	} else {
+		int len = strchrnul(p, '\n') - p;
+		if (len > 255)
+			len = 255;
+		return error(_("Unrecognized action: %.*s"), len, p);
+	}
 
 	q = p + strcspn(p, " \n");
-	if (q - p + 1 > sizeof(sha1_abbrev))
-		return NULL;
+	if (q - p + 1 > sizeof(sha1_abbrev)) {
+		int len = q - p;
+		if (len > 255)
+			len = 255;
+		return error(_("Object name too large: %.*s"), len, 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);
 
-	return lookup_commit_reference(commit_sha1);
+	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 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)
-			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		if (parse_insn_line(p, &item) < 0)
+			return error(_("on line %d."), i);
+		next = replay_insn_list_append(item.action, item.operand, next);
 		p = strchrnul(p, '\n');
 		if (*p)
 			p++;
@@ -754,8 +755,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;
@@ -771,7 +771,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);
@@ -820,18 +820,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)
@@ -860,7 +860,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;
@@ -868,7 +868,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);
@@ -912,9 +912,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);
@@ -924,8 +925,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)
 				/*
@@ -950,7 +951,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);
@@ -967,7 +968,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))
@@ -987,7 +988,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"));
 		}
@@ -1007,7 +1008,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);
@@ -1022,7 +1023,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 734ee12..c284c96 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -236,4 +236,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
 	test_line_count = 4 commits
 '
 
+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

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>  	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;
> +	} else {
> +		int len = strchrnul(p, '\n') - p;
> +		if (len > 255)
> +			len = 255;
> +		return error(_("Unrecognized action: %.*s"), len, p);

What happens if the current line has more than INT_MAX characters?

Maybe it would make sense to factor out a function for this
computation, for brevity and so there is just one place to tweak.

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

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> +             int len = strchrnul(p, '\n') - p;
>> +             if (len > 255)
>> +                     len = 255;
>> +             return error(_("Unrecognized action: %.*s"), len, p);
>
> What happens if the current line has more than INT_MAX characters?

I don't know!  I don't think this behavior is defined by the C89
standard, so I'd expect every compiler to do something different.  I'd
expect to be able to fix it like this:

int len = strchrnul(p, '\n') - p;
if (len > 255 || len - strchrnul(p, '\n') + p != 0)
    len = 255;

> Maybe it would make sense to factor out a function for this
> computation, for brevity and so there is just one place to tweak.

Shouldn't it be part of error() atleast in the long term?  I'll write
a quick private helper for now.

Thanks.

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> int len = strchrnul(p, '\n') - p;
> if (len > 255 || len - strchrnul(p, '\n') + p != 0)
>     len = 255;

Yuck.  Why not just use a size_t?

> Shouldn't it be part of error() atleast in the long term?

The default implementation of error() is (practically speaking) a thin
wrapper around fprintf.  What are the new semantics you are proposing
--- truncating %s arguments to one short line?  I don't think that
would be a good idea, since for example, some current callers might be
passing "\n" explicitly as a %s argument.

[PATCH 5/7] revert: Make the argument parser responsible for setup_revisions

[Ramkumar Ramachandra]

Currently, prepare_revs calls setup_revisions with the (argc, argv) in
the opts structure filled in by parse_args.  As a result, prepare_revs
has to take up the responsibility of erroring out and printing usage
information when (argc, argv) is malformed.  Since parse_args is doing
this for other kinds of parse errors anyway, give it the task of
calling setup_revisions as well.

Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   53 +++++++++++++++++++++++++++++------------------------
 1 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8faca07..e8bdeea 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -54,13 +54,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 REPLAY_NONE */
+	struct rev_info *revs;
 };
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -163,9 +164,9 @@ 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,
-					PARSE_OPT_KEEP_ARGV0 |
-					PARSE_OPT_KEEP_UNKNOWN);
+	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,
@@ -200,9 +201,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,
@@ -210,7 +208,20 @@ 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 = 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);
+	} else
+		opts->revs = NULL;
+
+	/* Forbid stray command-line arguments */
+	if (argc > 1)
+		usage_with_options(usage_str, options);
 }
 
 struct commit_message {
@@ -612,23 +623,15 @@ static int do_pick_commit(struct commit *commit, enum replay_action action,
 	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 != REPLAY_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"));
 }
 
@@ -823,14 +826,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 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);
+	prepare_revs(opts);
 
 	next = todo_list;
-	while ((commit = get_revision(&revs)))
+	while ((commit = get_revision(opts->revs)))
 		next = replay_insn_list_append(opts->action, commit, next);
 }
 
@@ -954,6 +956,9 @@ static int pick_revisions(struct replay_opts *opts)
 	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
 	read_and_refresh_cache(opts);
 
 	/*
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 5/7] revert: Make the argument parser responsible for setup_revisions

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Currently, prepare_revs calls setup_revisions with the (argc, argv) in
> the opts structure filled in by parse_args.  As a result, prepare_revs
> has to take up the responsibility of erroring out and printing usage
> information when (argc, argv) is malformed.  Since parse_args is doing
> this for other kinds of parse errors anyway, give it the task of
> calling setup_revisions as well.

I'm having trouble understanding the above.  Are you saying that we
want to concentrate all usage() calls in a single function for some
reason and prepare_revs() goes against this?  Why would I (the reader)
care about that?

Presumably a simpler explanation is that it makes my life easier in
two ways:

 1. If I want to understand what commandline arguments are accepted
    by cherry-pick/revert, after this patch there is just one function
    to look at.

 2. If I want to ask the replay machinery to do something different,
    as a caller I can set my options in a "struct rev_info" instead
    of forging commandline arguments with the same effect.

> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Since I wrote the patch modulo the commit message and small
improvements, shouldn't I be blamed as the author?  (git log
--grep="[Oo]riginal patch" and git log --grep=jc: give some examples.)
You can have my sign-off if you'd like.

[...]
> +	if (opts->subcommand == REPLAY_NONE) {
> +		opts->revs = xmalloc(sizeof(*opts->revs));

My fault: this never gets freed.  As long as this is private to the
cherry-pick/revert builtin, it's a one-time tiny leak cleaned up by
_exit --- no harm done, but probably worth a comment.

Re: [PATCH 5/7] revert: Make the argument parser responsible for setup_revisions

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>
> Since I wrote the patch modulo the commit message and small
> improvements, shouldn't I be blamed as the author?  (git log
> --grep="[Oo]riginal patch" and git log --grep=jc: give some examples.)
> You can have my sign-off if you'd like.

I'd initially blamed you for writing it, but then I was worried that I
didn't have your signoff -- so I quickly changed it before sending it
out.  Will fix in the next round.  Thanks for the clarification and
signoff :)

-- Ram

[PATCH 6/7] 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 |  824 +-----------------------------------------------------
 sequencer.c      |  805 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 sequencer.h      |   27 ++
 3 files changed, 832 insertions(+), 824 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e8bdeea..7ddfe84 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1,19 +1,9 @@
 #include "cache.h"
 #include "builtin.h"
-#include "object.h"
-#include "commit.h"
-#include "tag.h"
-#include "run-command.h"
-#include "exec_cmd.h"
-#include "utf8.h"
 #include "parse-options.h"
-#include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
 #include "rerere.h"
-#include "merge-recursive.h"
-#include "refs.h"
-#include "dir.h"
 #include "sequencer.h"
 
 /*
@@ -39,40 +29,11 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
-
-struct replay_opts {
-	enum replay_action action;
-	enum replay_subcommand subcommand;
-
-	/* Boolean options */
-	int edit;
-	int record_origin;
-	int no_commit;
-	int signoff;
-	int allow_ff;
-	int allow_rerere_auto;
-
-	int mainline;
-
-	/* Merge strategy */
-	const char *strategy;
-	const char **xopts;
-	size_t xopts_nr, xopts_alloc;
-
-	/* Only used by REPLAY_NONE */
-	struct rev_info *revs;
-};
-
-#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
-
 static const char *action_name(const struct replay_opts *opts)
 {
 	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
 }
 
-static char *get_encoding(const char *message);
-
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
 	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
@@ -224,787 +185,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		usage_with_options(usage_str, options);
 }
 
-struct commit_message {
-	char *parent_label;
-	const char *label;
-	const char *subject;
-	char *reencoded_message;
-	const char *message;
-};
-
-static int get_message(struct commit *commit, struct commit_message *out)
-{
-	const char *encoding;
-	const char *abbrev, *subject;
-	int abbrev_len, subject_len;
-	char *q;
-
-	if (!commit->buffer)
-		return -1;
-	encoding = get_encoding(commit->buffer);
-	if (!encoding)
-		encoding = "UTF-8";
-	if (!git_commit_encoding)
-		git_commit_encoding = "UTF-8";
-
-	out->reencoded_message = NULL;
-	out->message = commit->buffer;
-	if (strcmp(encoding, git_commit_encoding))
-		out->reencoded_message = reencode_string(commit->buffer,
-					git_commit_encoding, encoding);
-	if (out->reencoded_message)
-		out->message = out->reencoded_message;
-
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
-
-	subject_len = find_commit_subject(out->message, &subject);
-
-	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
-			      strlen("... ") + subject_len + 1);
-	q = out->parent_label;
-	q = mempcpy(q, "parent of ", strlen("parent of "));
-	out->label = q;
-	q = mempcpy(q, abbrev, abbrev_len);
-	q = mempcpy(q, "... ", strlen("... "));
-	out->subject = q;
-	q = mempcpy(q, subject, subject_len);
-	*q = '\0';
-	return 0;
-}
-
-static void free_message(struct commit_message *msg)
-{
-	free(msg->parent_label);
-	free(msg->reencoded_message);
-}
-
-static char *get_encoding(const char *message)
-{
-	const char *p = message, *eol;
-
-	while (*p && *p != '\n') {
-		for (eol = p + 1; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
-		if (!prefixcmp(p, "encoding ")) {
-			char *result = xmalloc(eol - 8 - p);
-			strlcpy(result, p + 9, eol - 8 - p);
-			return result;
-		}
-		p = eol;
-		if (*p == '\n')
-			p++;
-	}
-	return NULL;
-}
-
-static void write_cherry_pick_head(struct commit *commit)
-{
-	int fd;
-	struct strbuf buf = STRBUF_INIT;
-
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
-
-	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"),
-			  git_path("CHERRY_PICK_HEAD"));
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
-	strbuf_release(&buf);
-}
-
-static void print_advice(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_name(opts), sha1_to_hex(parent->object.sha1));
-
-	if (get_message(commit, &msg) != 0)
-		return error(_("Cannot get commit message for %s"),
-			sha1_to_hex(commit->object.sha1));
-
-	/*
-	 * "commit" is an existing commit.  We would want to apply
-	 * the difference it introduces since its first parent "prev"
-	 * on top of the current HEAD if we are cherry-pick.  Or the
-	 * reverse of it if we are revert.
-	 */
-
-	defmsg = git_pathdup("MERGE_MSG");
-
-	if (action == REPLAY_REVERT) {
-		base = commit;
-		base_label = msg.label;
-		next = parent;
-		next_label = msg.parent_label;
-		strbuf_addstr(&msgbuf, "Revert \"");
-		strbuf_addstr(&msgbuf, msg.subject);
-		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
-		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-
-		if (commit->parents && commit->parents->next) {
-			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
-			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
-		}
-		strbuf_addstr(&msgbuf, ".\n");
-	} else {
-		const char *p;
-
-		base = parent;
-		base_label = msg.parent_label;
-		next = commit;
-		next_label = msg.label;
-
-		/*
-		 * Append the commit log message to msgbuf; it starts
-		 * after the tree, parent, author, committer
-		 * information followed by "\n\n".
-		 */
-		p = strstr(msg.message, "\n\n");
-		if (p) {
-			p += 2;
-			strbuf_addstr(&msgbuf, p);
-		}
-
-		if (opts->record_origin) {
-			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
-			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-			strbuf_addstr(&msgbuf, ")\n");
-		}
-		if (!opts->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 replay_opts *opts)
-{
-	if (opts->action != REPLAY_REVERT)
-		opts->revs->reverse ^= 1;
-
-	if (prepare_revision_walk(opts->revs))
-		die(_("revision walk setup failed"));
-
-	if (!opts->revs->commits)
-		die(_("empty commit set passed"));
-}
-
-static void read_and_refresh_cache(struct replay_opts *opts)
-{
-	static struct lock_file index_lock;
-	int index_fd = hold_locked_index(&index_lock, 0);
-	if (read_index_preload(&the_index, NULL) < 0)
-		die(_("git %s: failed to read the index"), action_name(opts));
-	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
-	if (the_index.cache_changed) {
-		if (write_index(&the_index, index_fd) ||
-		    commit_locked_index(&index_lock))
-			die(_("git %s: failed to refresh the index"), action_name(opts));
-	}
-	rollback_lock_file(&index_lock);
-}
-
-/*
- * Append a commit to the end of the commit_list.
- *
- * next starts by pointing to the variable that holds the head of an
- * empty commit_list, and is updated to point to the "next" field of
- * the last item on the list as new commits are appended.
- *
- * Usage example:
- *
- *     struct commit_list *list;
- *     struct commit_list **next = &list;
- *
- *     next = commit_list_append(c1, next);
- *     next = commit_list_append(c2, next);
- *     assert(commit_list_count(list) == 2);
- *     return list;
- */
-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];
-	const char *p, *q;
-
-	p = start;
-	if (!prefixcmp(start, "pick ")) {
-		item->action = REPLAY_PICK;
-		p += strlen("pick ");
-	} else if (!prefixcmp(start, "revert ")) {
-		item->action = REPLAY_REVERT;
-		p += strlen("revert ");
-	} else {
-		int len = strchrnul(p, '\n') - p;
-		if (len > 255)
-			len = 255;
-		return error(_("Unrecognized action: %.*s"), len, p);
-	}
-
-	q = p + strcspn(p, " \n");
-	if (q - p + 1 > sizeof(sha1_abbrev)) {
-		int len = q - p;
-		if (len > 255)
-			len = 255;
-		return error(_("Object name too large: %.*s"), len, 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(_("on 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 commit *commit;
-	struct replay_insn_list **next;
-
-	prepare_revs(opts);
-
-	next = todo_list;
-	while ((commit = get_revision(opts->revs)))
-		next = replay_insn_list_append(opts->action, commit, next);
-}
-
-static int create_seq_dir(void)
-{
-	const char *seq_dir = git_path(SEQ_DIR);
-
-	if (file_exists(seq_dir))
-		return error(_("%s already exists."), seq_dir);
-	else if (mkdir(seq_dir, 0777) < 0)
-		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
-	return 0;
-}
-
-static void save_head(const char *head)
-{
-	const char *head_file = git_path(SEQ_HEAD_FILE);
-	static struct lock_file head_lock;
-	struct strbuf buf = STRBUF_INIT;
-	int fd;
-
-	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
-	strbuf_addf(&buf, "%s\n", head);
-	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s."), head_file);
-	if (commit_lock_file(&head_lock) < 0)
-		die(_("Error wrapping up %s."), head_file);
-}
-
-static void save_todo(struct replay_insn_list *todo_list)
-{
-	const char *todo_file = git_path(SEQ_TODO_FILE);
-	static struct lock_file todo_lock;
-	struct strbuf buf = STRBUF_INIT;
-	int fd;
-
-	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list) < 0)
-		die(_("Could not format %s."), todo_file);
-	if (write_in_full(fd, buf.buf, buf.len) < 0) {
-		strbuf_release(&buf);
-		die_errno(_("Could not write to %s."), todo_file);
-	}
-	if (commit_lock_file(&todo_lock) < 0) {
-		strbuf_release(&buf);
-		die(_("Error wrapping up %s."), todo_file);
-	}
-	strbuf_release(&buf);
-}
-
-static void save_opts(struct replay_opts *opts)
-{
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
-
-	if (opts->no_commit)
-		git_config_set_in_file(opts_file, "options.no-commit", "true");
-	if (opts->edit)
-		git_config_set_in_file(opts_file, "options.edit", "true");
-	if (opts->signoff)
-		git_config_set_in_file(opts_file, "options.signoff", "true");
-	if (opts->record_origin)
-		git_config_set_in_file(opts_file, "options.record-origin", "true");
-	if (opts->allow_ff)
-		git_config_set_in_file(opts_file, "options.allow-ff", "true");
-	if (opts->mainline) {
-		struct strbuf buf = STRBUF_INIT;
-		strbuf_addf(&buf, "%d", opts->mainline);
-		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
-		strbuf_release(&buf);
-	}
-	if (opts->strategy)
-		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
-	if (opts->xopts) {
-		int i;
-		for (i = 0; i < opts->xopts_nr; i++)
-			git_config_set_multivar_in_file(opts_file,
-							"options.strategy-option",
-							opts->xopts[i], "^$", 0);
-	}
-}
-
-static int pick_commits(struct replay_insn_list *todo_list,
-			struct replay_opts *opts)
-{
-	struct replay_insn_list *cur;
-	int res;
-
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	if (opts->allow_ff)
-		assert(!(opts->signoff || opts->no_commit ||
-				opts->record_origin || opts->edit));
-	read_and_refresh_cache(opts);
-
-	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur);
-		res = do_pick_commit(cur->operand, cur->action, opts);
-		if (res) {
-			if (!cur->next)
-				/*
-				 * An error was encountered while
-				 * picking the last commit; the
-				 * sequencer state is useless now --
-				 * the user simply needs to resolve
-				 * the conflict and commit
-				 */
-				remove_sequencer_state(0);
-			return res;
-		}
-	}
-
-	/*
-	 * Sequence of picks finished successfully; cleanup by
-	 * removing the .git/sequencer directory
-	 */
-	remove_sequencer_state(1);
-	return 0;
-}
-
-static int pick_revisions(struct replay_opts *opts)
-{
-	struct replay_insn_list *todo_list = NULL;
-	unsigned char sha1[20];
-
-	if (opts->subcommand == REPLAY_NONE)
-		assert(opts->revs);
-
-	read_and_refresh_cache(opts);
-
-	/*
-	 * Decide what to do depending on the arguments; a fresh
-	 * cherry-pick should be handled differently from an existing
-	 * one that is being continued
-	 */
-	if (opts->subcommand == REPLAY_RESET) {
-		remove_sequencer_state(1);
-		return 0;
-	} else if (opts->subcommand == REPLAY_CONTINUE) {
-		if (!file_exists(git_path(SEQ_TODO_FILE)))
-			goto error;
-		read_populate_opts(&opts);
-		read_populate_todo(&todo_list);
-
-		/* Verify that the conflict has been resolved */
-		if (!index_differs_from("HEAD", 0))
-			todo_list = todo_list->next;
-	} else {
-		/*
-		 * Start a new cherry-pick/ revert sequence; but
-		 * first, make sure that an existing one isn't in
-		 * progress
-		 */
-
-		walk_revs_populate_todo(&todo_list, opts);
-		if (create_seq_dir() < 0) {
-			error(_("A cherry-pick or revert is in progress."));
-			advise(_("Use --continue to continue the operation"));
-			advise(_("or --reset to forget about it"));
-			return -1;
-		}
-		if (get_sha1("HEAD", sha1)) {
-			if (opts->action == REPLAY_REVERT)
-				return error(_("Can't revert as initial commit"));
-			return error(_("Can't cherry-pick into empty head"));
-		}
-		save_head(sha1_to_hex(sha1));
-		save_opts(opts);
-	}
-	return pick_commits(todo_list, opts);
-error:
-	return error(_("No %s in progress"), action_name(opts));
-}
-
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
@@ -1016,7 +196,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -1031,7 +211,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
diff --git a/sequencer.c b/sequencer.c
index bc2c046..95bce89 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,13 +1,656 @@
 #include "cache.h"
 #include "sequencer.h"
-#include "strbuf.h"
+#include "object.h"
+#include "commit.h"
+#include "tag.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+#include "utf8.h"
+#include "cache-tree.h"
+#include "diff.h"
+#include "revision.h"
+#include "rerere.h"
+#include "merge-recursive.h"
+#include "refs.h"
 #include "dir.h"
 
+#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
+#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };
+
+struct commit_message {
+	char *parent_label;
+	const char *label;
+	const char *subject;
+	char *reencoded_message;
+	const char *message;
+};
+
+static const char *action_keyword(const struct replay_opts *opts)
+{
+	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
+}
+
+static char *get_encoding(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_keyword(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_keyword(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_keyword(opts), sha1_to_hex(parent->object.sha1));
+
+	if (get_message(commit, &msg) != 0)
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
+
+	/*
+	 * "commit" is an existing commit.  We would want to apply
+	 * the difference it introduces since its first parent "prev"
+	 * on top of the current HEAD if we are cherry-pick.  Or the
+	 * reverse of it if we are revert.
+	 */
+
+	defmsg = git_pathdup("MERGE_MSG");
+
+	if (action == REPLAY_REVERT) {
+		base = commit;
+		base_label = msg.label;
+		next = parent;
+		next_label = msg.parent_label;
+		strbuf_addstr(&msgbuf, "Revert \"");
+		strbuf_addstr(&msgbuf, msg.subject);
+		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
+		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+
+		if (commit->parents && commit->parents->next) {
+			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
+		}
+		strbuf_addstr(&msgbuf, ".\n");
+	} else {
+		const char *p;
+
+		base = parent;
+		base_label = msg.parent_label;
+		next = commit;
+		next_label = msg.label;
+
+		/*
+		 * Append the commit log message to msgbuf; it starts
+		 * after the tree, parent, author, committer
+		 * information followed by "\n\n".
+		 */
+		p = strstr(msg.message, "\n\n");
+		if (p) {
+			p += 2;
+			strbuf_addstr(&msgbuf, p);
+		}
+
+		if (opts->record_origin) {
+			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+			strbuf_addstr(&msgbuf, ")\n");
+		}
+		if (!opts->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 replay_opts *opts)
+{
+	if (opts->action != REPLAY_REVERT)
+		opts->revs->reverse ^= 1;
+
+	if (prepare_revision_walk(opts->revs))
+		die(_("revision walk setup failed"));
+
+	if (!opts->revs->commits)
+		die(_("empty commit set passed"));
+}
+
+static void read_and_refresh_cache(struct replay_opts *opts)
+{
+	static struct lock_file index_lock;
+	int index_fd = hold_locked_index(&index_lock, 0);
+	if (read_index_preload(&the_index, NULL) < 0)
+		die(_("git %s: failed to read the index"), action_keyword(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_keyword(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];
+	const char *p, *q;
+
+	p = start;
+	if (!prefixcmp(start, "pick ")) {
+		item->action = REPLAY_PICK;
+		p += strlen("pick ");
+	} else if (!prefixcmp(start, "revert ")) {
+		item->action = REPLAY_REVERT;
+		p += strlen("revert ");
+	} else {
+		int len = strchrnul(p, '\n') - p;
+		if (len > 255)
+			len = 255;
+		return error(_("Unrecognized action: %.*s"), len, p);
+	}
+
+	q = p + strcspn(p, " \n");
+	if (q - p + 1 > sizeof(sha1_abbrev)) {
+		int len = q - p;
+		if (len > 255)
+			len = 255;
+		return error(_("Object name too large: %.*s"), len, 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(_("on 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 commit *commit;
+	struct replay_insn_list **next;
+
+	prepare_revs(opts);
+
+	next = todo_list;
+	while ((commit = get_revision(opts->revs)))
+		next = replay_insn_list_append(opts->action, commit, next);
+}
+
+static int create_seq_dir(void)
+{
+	const char *seq_dir = git_path(SEQ_DIR);
+
+	if (file_exists(seq_dir))
+		return error(_("%s already exists."), seq_dir);
+	else if (mkdir(seq_dir, 0777) < 0)
+		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
+	return 0;
+}
+
 void remove_sequencer_state(int aggressive)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
 	struct strbuf seq_old_dir = STRBUF_INIT;
-
 	strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
 	strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR));
 	remove_dir_recursively(&seq_old_dir, 0);
@@ -17,3 +660,161 @@ 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_keyword(opts), 0);
+	if (opts->allow_ff)
+		assert(!(opts->signoff || opts->no_commit ||
+				opts->record_origin || opts->edit));
+	read_and_refresh_cache(opts);
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
+		if (res) {
+			if (!cur->next)
+				/*
+				 * An error was encountered while
+				 * picking the last commit; the
+				 * sequencer state is useless now --
+				 * the user simply needs to resolve
+				 * the conflict and commit
+				 */
+				remove_sequencer_state(0);
+			return res;
+		}
+	}
+
+	/*
+	 * Sequence of picks finished successfully; cleanup by
+	 * removing the .git/sequencer directory
+	 */
+	remove_sequencer_state(1);
+	return 0;
+}
+
+int sequencer_pick_revisions(struct replay_opts *opts)
+{
+	struct replay_insn_list *todo_list = NULL;
+	unsigned char sha1[20];
+
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
+	read_and_refresh_cache(opts);
+
+	/*
+	 * Decide what to do depending on the arguments; a fresh
+	 * cherry-pick should be handled differently from an existing
+	 * one that is being continued
+	 */
+	if (opts->subcommand == REPLAY_RESET) {
+		remove_sequencer_state(1);
+		return 0;
+	} else if (opts->subcommand == REPLAY_CONTINUE) {
+		if (!file_exists(git_path(SEQ_TODO_FILE)))
+			goto error;
+		read_populate_opts(&opts);
+		read_populate_todo(&todo_list);
+
+		/* Verify that the conflict has been resolved */
+		if (!index_differs_from("HEAD", 0))
+			todo_list = todo_list->next;
+	} else {
+		/*
+		 * Start a new cherry-pick/ revert sequence; but
+		 * first, make sure that an existing one isn't in
+		 * progress
+		 */
+
+		walk_revs_populate_todo(&todo_list, opts);
+		if (create_seq_dir() < 0) {
+			error(_("A cherry-pick or revert is in progress."));
+			advise(_("Use --continue to continue the operation"));
+			advise(_("or --reset to forget about it"));
+			return -1;
+		}
+		if (get_sha1("HEAD", sha1)) {
+			if (opts->action == REPLAY_REVERT)
+				return error(_("Can't revert as initial commit"));
+			return error(_("Can't cherry-pick into empty head"));
+		}
+		save_head(sha1_to_hex(sha1));
+		save_opts(opts);
+	}
+	return pick_commits(todo_list, opts);
+error:
+	return error(_("No %s in progress"), action_keyword(opts));
+}
diff --git a/sequencer.h b/sequencer.h
index f4db257..5e56cd4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -8,6 +8,30 @@
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
 enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
+
+struct replay_opts {
+	enum replay_action action;
+	enum replay_subcommand subcommand;
+
+	/* Boolean options */
+	int edit;
+	int record_origin;
+	int no_commit;
+	int signoff;
+	int allow_ff;
+	int allow_rerere_auto;
+
+	int mainline;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+
+	/* Only used by REPLAY_NONE */
+	struct rev_info *revs;
+};
 
 struct replay_insn_list {
 	enum replay_action action;
@@ -25,4 +49,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

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> 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.

:)  It sounds like you are running a business.

I guess I would suggest clarifying that you mean exposing further
functionality through this API for more callers, rather than just
generally bloating git further.

[...]
> +++ b/sequencer.c
> @@ -1,13 +1,656 @@
[...]
> +#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };

As much as I like this change (and I do, modulo the stray semicolon),
it does not belong in a patch where it can be unnoticed in the code
movement.

> +static const char *action_keyword(const struct replay_opts *opts)
> +{
> +	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
> +}

Another (non-functional) change.  Probably (?) this renaming has a
good reason to be part of this patch, but it should definitely be
mentioned in the commit message.

> +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"));

git_path() calls vsnprintf which clobbers errno, so depending on the
platform this can print messages like

	fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success

The natural fix would be to add a local for it (as a separate patch).
Sorry I missed this when the code first arrived.

> +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;

This tree is leaked (for example if you cherry-pick a sequence of
root commits).

> +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");
> +}

The exit code here violates the usual "exit with status 128 for
errors other than conflicts" rule.  Perhaps it should be changed to
"return -1" in a separate patch (to accompany the patch that returns
error() instead of die()-ing so often to allow callers to give
additional context to errors from this machinery).

>  void remove_sequencer_state(int aggressive)
>  {
>  	struct strbuf seq_dir = STRBUF_INIT;
>  	struct strbuf seq_old_dir = STRBUF_INIT;
> -
>  	strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));

Unrelated change snuck in?

> --- a/sequencer.h
> +++ b/sequencer.h
[...]
> @@ -25,4 +49,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);
> +

I thought sequencer_parse_args() wasn't being exposed.

Except where noted above, I hope this is just simple code movement,
but I haven't checked.  If I could be sure, it would be easier to
review.

Ciao,
Jonathan

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

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> 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.
>
> :)  It sounds like you are running a business.

*laughs* Now that you mention it, it certainly does :)

>> +++ b/sequencer.c
>> @@ -1,13 +1,656 @@
>> [...]
>> +#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };

I'll get rid of this.

>> +static const char *action_keyword(const struct replay_opts *opts)
>> [...]
>
> Another (non-functional) change.  Probably (?) this renaming has a
> good reason to be part of this patch, but it should definitely be
> mentioned in the commit message.

Yes, I want the rename to be part of this patch (see Daniel's comment
and my agreement).  Will clarify in the commit message.

> git_path() calls vsnprintf which clobbers errno, so depending on the
> platform this can print messages like
>
>        fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success
>
> The natural fix would be to add a local for it (as a separate patch).
> Sorry I missed this when the code first arrived.

Ugh, yet another "bugfix patch" to queue near the beginning of the
series.  Thanks for catching this.

>> +static struct tree *empty_tree(void)
>> [...]
>
> This tree is leaked (for example if you cherry-pick a sequence of
> root commits).

This is not something I introduced -- it can wait until later, no?

>> +static int fast_forward_to(const unsigned char *to, const unsigned char *from)
>> [...]
>
> The exit code here violates the usual "exit with status 128 for
> errors other than conflicts" rule.  Perhaps it should be changed to
> "return -1" in a separate patch (to accompany the patch that returns
> error() instead of die()-ing so often to allow callers to give
> additional context to errors from this machinery).

Great catch!  Will fix.

>> --- a/sequencer.h
>> +++ b/sequencer.h
> [...]
>> +void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);
>
> I thought sequencer_parse_args() wasn't being exposed.

Rebase fail, sorry.  There is no sequencer_parse_args().

> Except where noted above, I hope this is just simple code movement,
> but I haven't checked.  If I could be sure, it would be easier to
> review.

It is a simple code movement.  Is there something I can do to help?

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> git_path() calls vsnprintf which clobbers errno, so depending on the
>> platform this can print messages like
>>
>>        fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success
[...]
> Ugh, yet another "bugfix patch" to queue near the beginning of the
> series.  Thanks for catching this.

It's not a bug you introduced, and ideally it would be a separate
patch on its own, against the js/cherry-pick-usability branch.

>> Ramkumar Ramachandra wrote:

>>> +static struct tree *empty_tree(void)
>>> [...]
>>
>> This tree is leaked (for example if you cherry-pick a sequence of
>> root commits).
>
> This is not something I introduced -- it can wait until later, no?

Yep, it's Christian's fault (for introducing multiple-cherry-pick).

Patch below.  The things I do... :)

[...]
> It is a simple code movement.  Is there something I can do to help?

Part of the review was about examples where it was not simple code
movement.  Maybe if there is another round the thing to do would be to
send a patch generated with -B -M to allow others to more easily check
it.

Thanks.

-- >8 --
Subject: revert: plug memory leak in "cherry-pick root commit" codepath

The empty tree passed as common ancestor to merge_trees() when
cherry-picking a parentless commit is allocated on the heap and never
freed.  Leaking such a small one-time allocation is not a very big
problem, but now that "git cherry-pick" can cherry-pick multiple
commits it can start to add up.

Avoid the leak by storing the fake tree exactly once in the BSS
section (i.e., use a static).  While at it, let's add a test to make
sure cherry-picking multiple parentless commits continues to work.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Patch is against cc/cherry-pick-series (86c7bb47).

 builtin/revert.c            |   10 +++++-----
 t/t3503-cherry-pick-root.sh |   27 ++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 853e9e40..c1b0fb3d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -273,12 +273,12 @@ static void write_message(struct strbuf *msgbuf, const char *filename)
 
 static struct tree *empty_tree(void)
 {
-	struct tree *tree = xcalloc(1, sizeof(struct tree));
+	static struct tree tree;
 
-	tree->object.parsed = 1;
-	tree->object.type = OBJ_TREE;
-	pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
-	return tree;
+	tree.object.parsed = 1;
+	tree.object.type = OBJ_TREE;
+	pretend_sha1_file(NULL, 0, OBJ_TREE, tree.object.sha1);
+	return &tree;
 }
 
 static NORETURN void die_dirty_index(const char *me)
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index b0faa299..472e5b80 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -16,15 +16,40 @@ test_expect_success setup '
 	echo second > file2 &&
 	git add file2 &&
 	test_tick &&
-	git commit -m "second"
+	git commit -m "second" &&
+
+	git symbolic-ref HEAD refs/heads/third &&
+	rm .git/index file2 &&
+	echo third > file3 &&
+	git add file3 &&
+	test_tick &&
+	git commit -m "third"
 
 '
 
 test_expect_success 'cherry-pick a root commit' '
 
+	git checkout second^0 &&
 	git cherry-pick master &&
 	test first = $(cat file1)
 
 '
 
+test_expect_success 'cherry-pick two root commits' '
+
+	echo first >expect.file1 &&
+	echo second >expect.file2 &&
+	echo third >expect.file3 &&
+
+	git checkout second^0 &&
+	git cherry-pick master third &&
+
+	test_cmp expect.file1 file1 &&
+	test_cmp expect.file2 file2 &&
+	test_cmp expect.file3 file3 &&
+	git rev-parse --verify HEAD^^ &&
+	test_must_fail git rev-parse --verify HEAD^^^
+
+'
+
 test_done
-- 
1.7.6

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Subject: revert: plug memory leak in "cherry-pick root commit" codepath
>
> The empty tree passed as common ancestor to merge_trees() when
> cherry-picking a parentless commit is allocated on the heap and never
> freed.

Thanks for noticing, but shouldn't we be just using

	lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN)

or something, instead of hand-crafting a fake tree object?

[PATCH v2] revert: plug memory leak in "cherry-pick root commit" codepath

[Jonathan Nieder]

Junio C Hamano wrote:

> Thanks for noticing, but shouldn't we be just using
>
> 	lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN)
>
> or something, instead of hand-crafting a fake tree object?

Yes.  Ever since v1.5.5-rc0~180^2~1 (hard-code the empty tree object,
2008-02-13), there is no need to call pretend_sha1_file() again to
get a fake empty tree object, so this lookup_tree() should work
fine.

-- >8 --
Subject: revert: plug memory leak in "cherry-pick root commit" codepath

For each parentless commit it is asked to reuse, "git cherry-pick" and
"git revert" hand-craft a fake parent tree object on the heap to pass
to merge_trees().  Leaking such a small one-time allocation would not
be a big deal, but now that cherry-pick/revert can take multiple
commit arguments, it can start to add up.

The fix is simple: don't create a new fake empty tree at all, but rely
on the built-in one that has existed since 346245a1 (hard-code the
empty tree object, 2008-02-13).

While at it, add a test to make sure cherry-picking multiple
parentless commits continues to work.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c            |    7 +------
 t/t3503-cherry-pick-root.sh |   27 ++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 853e9e40..a26a7c93 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -273,12 +273,7 @@ static void write_message(struct strbuf *msgbuf, const char *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;
+	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
 }
 
 static NORETURN void die_dirty_index(const char *me)
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index b0faa299..472e5b80 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -16,15 +16,40 @@ test_expect_success setup '
 	echo second > file2 &&
 	git add file2 &&
 	test_tick &&
-	git commit -m "second"
+	git commit -m "second" &&
+
+	git symbolic-ref HEAD refs/heads/third &&
+	rm .git/index file2 &&
+	echo third > file3 &&
+	git add file3 &&
+	test_tick &&
+	git commit -m "third"
 
 '
 
 test_expect_success 'cherry-pick a root commit' '
 
+	git checkout second^0 &&
 	git cherry-pick master &&
 	test first = $(cat file1)
 
 '
 
+test_expect_success 'cherry-pick two root commits' '
+
+	echo first >expect.file1 &&
+	echo second >expect.file2 &&
+	echo third >expect.file3 &&
+
+	git checkout second^0 &&
+	git cherry-pick master third &&
+
+	test_cmp expect.file1 file1 &&
+	test_cmp expect.file2 file2 &&
+	test_cmp expect.file3 file3 &&
+	git rev-parse --verify HEAD^^ &&
+	test_must_fail git rev-parse --verify HEAD^^^
+
+'
+
 test_done
-- 
1.7.6

Re: [PATCH v2] revert: plug memory leak in "cherry-pick root commit" codepath

[Jonathan Nieder]

Jonathan Nieder wrote:
> Junio C Hamano wrote:

>> Thanks for noticing, but shouldn't we be just using
>>
>> 	lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN)
>>
>> or something, instead of hand-crafting a fake tree object?
>
> Yes.

And here's another patch in the same spirit (no test this time,
though).

-- >8 --
Subject: merge-recursive: take advantage of hardcoded empty tree

When this code was first written (v1.4.3-rc1~174^2~4, merge-recur: if
there is no common ancestor, fake empty one, 2006-08-09), everyone
needing a fake empty tree had to make her own, but ever since
v1.5.5-rc0~180^2~1 (2008-02-13), the object lookup machinery provides
a ready-made one.  Use it.

This is just a simplification, though it also fixes a small leak
(since the tree in the virtual common ancestor commit is never freed).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 merge-recursive.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 206c1036..7695fe89 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1309,12 +1309,10 @@ int merge_recursive(struct merge_options *o,
 
 	merged_common_ancestors = pop_commit(&ca);
 	if (merged_common_ancestors == NULL) {
-		/* if there is no common ancestor, make an empty tree */
-		struct tree *tree = xcalloc(1, sizeof(struct tree));
+		/* if there is no common ancestor, use an empty tree */
+		struct tree *tree;
 
-		tree->object.parsed = 1;
-		tree->object.type = OBJ_TREE;
-		pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
+		tree = lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
 		merged_common_ancestors = make_virtual_commit(tree, "ancestor");
 	}
 
-- 
1.7.6

Re: [PATCH v2] revert: plug memory leak in "cherry-pick root commit" codepath

[Jeff King]

On Tue, Aug 16, 2011 at 01:16:33PM -0500, Jonathan Nieder wrote:

>  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;
> +	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
>  }

Much nicer. But doesn't your dab0d41 (correct type of
EMPTY_TREE_SHA1_BIN, 2011-02-07) make the cast unnecessary?

-Peff

Re: [PATCH v2] revert: plug memory leak in "cherry-pick root commit" codepath

[Jonathan Nieder]

Jeff King wrote:
> On Tue, Aug 16, 2011 at 01:16:33PM -0500, Jonathan Nieder wrote:

>>  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;
>> +	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
>>  }
>
> Much nicer. But doesn't your dab0d41 (correct type of
> EMPTY_TREE_SHA1_BIN, 2011-02-07) make the cast unnecessary?

Yes, I was working against an older codebase (for no particular
reason).

[PATCH 7/7] 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, make 'git commit' notify the sequencer after
every successful commit; the sequencer then removes the state if no
more instructions are pending.

Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/commit.c                   |    8 ++++++++
 sequencer.c                        |   23 ++++++++++++-----------
 sequencer.h                        |    1 +
 t/t3032-merge-recursive-options.sh |    2 ++
 t/t3510-cherry-pick-sequence.sh    |    4 ++--
 5 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..1699371 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,6 +1522,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 
+	/*
+	 * Notify the sequencer that we're committing.  The routine
+	 * removes the sequencer state if our commit just completed
+	 * the last instruction.
+	 */
+	sequencer_notify_commit();
+
 	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"
diff --git a/sequencer.c b/sequencer.c
index 95bce89..caae932 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -580,6 +580,17 @@ static void read_populate_todo(struct replay_insn_list **todo_list)
 		die(_("Unusable instruction sheet: %s"), todo_file);
 }
 
+void sequencer_notify_commit(void)
+{
+	struct replay_insn_list *todo_list = NULL;
+
+	if (!file_exists(git_path(SEQ_TODO_FILE)))
+		return;
+	read_populate_todo(&todo_list);
+	if (!todo_list->next)
+		remove_sequencer_state(1);
+}
+
 static int populate_opts_cb(const char *key, const char *value, void *data)
 {
 	struct replay_opts *opts = data;
@@ -743,18 +754,8 @@ static int pick_commits(struct replay_insn_list *todo_list,
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur);
 		res = do_pick_commit(cur->operand, cur->action, opts);
-		if (res) {
-			if (!cur->next)
-				/*
-				 * An error was encountered while
-				 * picking the last commit; the
-				 * sequencer state is useless now --
-				 * the user simply needs to resolve
-				 * the conflict and commit
-				 */
-				remove_sequencer_state(0);
+		if (res)
 			return res;
-		}
 	}
 
 	/*
diff --git a/sequencer.h b/sequencer.h
index 5e56cd4..3b89d42 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -51,5 +51,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);
+void sequencer_notify_commit(void);
 
 #endif
diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
index 2b17311..1da99b7 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-options.sh
@@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
 	git read-tree --reset -u HEAD &&
 	test_must_fail git cherry-pick --no-commit remote &&
 	git read-tree --reset -u HEAD &&
+	git cherry-pick --reset &&
 	test_must_fail git cherry-pick remote &&
 	test_must_fail git update-index --refresh &&
+	git cherry-pick --reset &&
 	grep "<<<<<<" text.txt
 '
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c284c96..abd13fe 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 7/7] sequencer: Remove sequencer state after final commit

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> 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, make 'git commit' notify the sequencer after
> every successful commit; the sequencer then removes the state if no
> more instructions are pending.

Sorry, I'm getting lost in all the words.  I suspect you are saying
“d3f4628e was trying to solve such-and-such problem, but its fix was
problematic because it removes the data that a hypothetical "git
cherry-pick --abort" command would need to work.  Back out that
change and adopt the following instead.”

In particular, the above does not make it clear to me:

 - as a user, what effect will I notice after this change?
 - what problem does it solve?
 - does it have any downsides?

> --- 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,6 +1522,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	unlink(git_path("MERGE_MODE"));
>  	unlink(git_path("SQUASH_MSG"));
>  
> +	/*
> +	 * Notify the sequencer that we're committing.  The routine
> +	 * removes the sequencer state if our commit just completed
> +	 * the last instruction.
> +	 */
> +	sequencer_notify_commit();
> +
>  	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"

The function name (..._notify_commit()) does not seem very intuitive.
Based on the name, I expect it to use the sequencer to print a message
to the user about the commit in progress.

What happens if writing to .git/index fails?  I can think of reasons
to remove the sequencer file before or afterward:

 - before, because once .git/index has been removed, the index is not
   locked any more and further commands could take place in parallel.

 - afterward, because when writing the index fails, I (the user)
   might want to react by running "git cherry-pick --abort".

The latter seems slightly more compelling to me --- after all, any
further command wanting to touch the sequencer directory is going
to check whether it exists --- but more importantly, the former
reminds me that we haven't thought carefully about what concurrent
operations using the sequencer are and aren't allowed.  Though I
doubt that it would come up much in practice. :)

> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -580,6 +580,17 @@ static void read_populate_todo(struct replay_insn_list **todo_list)
>  		die(_("Unusable instruction sheet: %s"), todo_file);
>  }
>  
> +void sequencer_notify_commit(void)
> +{
> +	struct replay_insn_list *todo_list = NULL;
> +
> +	if (!file_exists(git_path(SEQ_TODO_FILE)))
> +		return;
> +	read_populate_todo(&todo_list);
> +	if (!todo_list->next)
> +		remove_sequencer_state(1);
> +}

Not about this patch: I keep on forgetting what the argument to
remove_sequencer_state means.  Would it be possible to make it
a flag, which would both make the meaning more obvious and mean
it is easy to support additional flags in the future?

> --- a/t/t3032-merge-recursive-options.sh
> +++ b/t/t3032-merge-recursive-options.sh
> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
>  	git read-tree --reset -u HEAD &&
>  	test_must_fail git cherry-pick --no-commit remote &&
>  	git read-tree --reset -u HEAD &&
> +	git cherry-pick --reset &&
>  	test_must_fail git cherry-pick remote &&
>  	test_must_fail git update-index --refresh &&
> +	git cherry-pick --reset &&
>  	grep "<<<<<<" text.txt
>  '

What is this about?  Maybe it would be clearer to change the "git
read-tree ..." to "git reset --hard", so the test assertion would not
rely on new cherry-pick features (and to mention the change in the
commit message!).

Doesn't this point to a risk in the patch?  Do you think there might
be scripts out there relying on being able to use "git read-tree
--reset -u HEAD" to clear away a failed cherry-pick before trying
again, and if so, can we do something about it?

> --- 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 &&
>  	{

It would also be nice to check "test_path_is_dir" before the final
commit, so people working on this code in the future know both aspects
of the patch are intentional.

Thanks, I'm glad to see this patch.

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

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> [...]
>
> The function name (..._notify_commit()) does not seem very intuitive.
> Based on the name, I expect it to use the sequencer to print a message
> to the user about the commit in progress.

For the record, I'm not too happy with the name either.  I was hoping
that someone would suggest a better name during the review :P

> .git/index
>  - afterward, because when writing the index fails, I (the user)
>   might want to react by running "git cherry-pick --abort".

Very subtle point: will fix.  I'd originally put it along with the
other state-removing functions.

> Not about this patch: I keep on forgetting what the argument to
> remove_sequencer_state means.  Would it be possible to make it
> a flag, which would both make the meaning more obvious and mean
> it is easy to support additional flags in the future?

I don't want to create a struct and populate it because I highly doubt
remove_sequencer_state will take any more arguments in the future.
Putting an "int aggressive = 1" and passing the variable instead of
the literal is a little inelegant.  Actually making any change at this
stage is inelegant because of the other remove_sequencer_state() calls
:|  My call: we can fix it if and when the function needs more
arguments.

>> --- a/t/t3032-merge-recursive-options.sh
>> +++ b/t/t3032-merge-recursive-options.sh
>> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
>>       git read-tree --reset -u HEAD &&
>>       test_must_fail git cherry-pick --no-commit remote &&
>>       git read-tree --reset -u HEAD &&
>> +     git cherry-pick --reset &&
>>       test_must_fail git cherry-pick remote &&
>>       test_must_fail git update-index --refresh &&
>> +     git cherry-pick --reset &&
>>       grep "<<<<<<" text.txt
>>  '
>
> What is this about?  Maybe it would be clearer to change the "git
> read-tree ..." to "git reset --hard", so the test assertion would not
> rely on new cherry-pick features (and to mention the change in the
> commit message!).

Okay, I can do this instead (don't check whitespace! I just typed out
the patch):

@@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
      git read-tree --reset -u HEAD &&
      test_must_fail git cherry-pick --no-commit remote &&
-     git read-tree --reset -u HEAD &&
+     git reset --hard &&
      test_must_fail git cherry-pick remote &&
      test_must_fail git update-index --refresh &&
+     git reset --hard &&
      grep "<<<<<<" text.txt
 '

> Doesn't this point to a risk in the patch?  Do you think there might
> be scripts out there relying on being able to use "git read-tree
> --reset -u HEAD" to clear away a failed cherry-pick before trying
> again, and if so, can we do something about it?

I'm not sure we can do anything about it -- we should probably put
some kind of warning in the commit message?

>> --- 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 &&
>>       {
>
> It would also be nice to check "test_path_is_dir" before the final
> commit, so people working on this code in the future know both aspects
> of the patch are intentional.

Good point.  Fixed.

Thanks.

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Putting an "int aggressive = 1" and passing the variable instead of
> the literal is a little inelegant.

What about a flag word, like for example read_sha1_file_extended()
takes?

[...]
>> Do you think there might
>> be scripts out there relying on being able to use "git read-tree
>> --reset -u HEAD" to clear away a failed cherry-pick before trying
>> again, and if so, can we do something about it?
>
> I'm not sure we can do anything about it -- we should probably put
> some kind of warning in the commit message?

That's necessary at a minimum.

I actually _don't_ think there might be many scripts relying on "git
read-tree --reset -u HEAD" to clear away a failed cherry-pick, simply
because very few people seem to use plumbing these days :) and those
who do might be of a mindset to use "git apply" instead of
cherry-pick.  But it would be prudent to run a code search to check
and to think carefully about the effect on people who did use the
"read-tree --reset && cherry-pick again" idiom.

Hope that helps.
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ramkumar Ramachandra wrote:
>
>> 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, make 'git commit' notify the sequencer after
>> every successful commit; the sequencer then removes the state if no
>> more instructions are pending.
>
> Sorry, I'm getting lost in all the words.  I suspect you are saying
> “d3f4628e was trying to solve such-and-such problem, but its fix was
> problematic because it removes the data that a hypothetical "git
> cherry-pick --abort" command would need to work.  Back out that
> change and adopt the following instead.”

It still is unclear why "removing the sequencer state when no more insns
are pending" is so necessary that the codebase needs to bend backwards to
support that in the first place. What problem is d3f4628e really trying to
solve?

If "who is responsible for removing a stale state" is the issue it tries
to address, Wouldn't it be much cleaner to make "sequencer continue"
notice that state and silently succeed, saying "I am done"?

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

[Jonathan Nieder]

Hi Junio,

Junio C Hamano wrote:

> It still is unclear why "removing the sequencer state when no more insns
> are pending" is so necessary that the codebase needs to bend backwards to
> support that in the first place. What problem is d3f4628e really trying to
> solve?

I believe it is meant to support command sequences such as these:

1.
	git cherry-pick foo; # has conflicts
	... resolve conflicts and "git add" the resolved files ...
	git commit
	git cherry-pick bar

2.
	git cherry-pick foo bar; # has conflicts applying "bar"
	... resolve ...
	git commit
	git cherry-pick baz

Those were intuitive things to do before the sequencer existed, and if
I understand correctly, d3f4628e was intended to support people and
scripts (such as the test suite) that have these commands wired into
their fingers.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I believe it is meant to support command sequences such as these:
>
> 1.
> 	git cherry-pick foo; # has conflicts
> 	... resolve conflicts and "git add" the resolved files ...
> 	git commit
> 	git cherry-pick bar

Why does a single commit "cherry-pick foo" leave any sequencer state that
may interfere with the latter to begin with? Isn't that already a bug?

> 2.
> 	git cherry-pick foo bar; # has conflicts applying "bar"
> 	... resolve ...
> 	git commit
> 	git cherry-pick baz
>
> Those were intuitive things to do before the sequencer existed, and if
> I understand correctly, d3f4628e was intended to support people and
> scripts (such as the test suite) that have these commands wired into
> their fingers.

Given that the latter was broken when "foo" stopped with conficts (it lost
"bar" altogether anyway), I am not worried about it, and I do not care
much about anybody who had wired such multi-pick into scripts or fingers,
either.

IOW, I do not necessarily agree with your "those were intuitive"
assertion.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> I believe it is meant to support command sequences such as these:
>>
>> 1.
>> 	git cherry-pick foo; # has conflicts
>> 	... resolve conflicts and "git add" the resolved files ...
>> 	git commit
>> 	git cherry-pick bar
>
> Why does a single commit "cherry-pick foo" leave any sequencer state that
> may interfere with the latter to begin with? Isn't that already a bug?
>
>> 2.
>> 	git cherry-pick foo bar; # has conflicts applying "bar"
>> 	... resolve ...
>> 	git commit
>> 	git cherry-pick baz
>>
>> Those were intuitive things to do before the sequencer existed, and if
>> I understand correctly, d3f4628e was intended to support people and
>> scripts (such as the test suite) that have these commands wired into
>> their fingers.
>
> Given that the latter was broken when "foo" stopped with conficts (it lost
> "bar" altogether anyway), I am not worried about it, and I do not care
> much about anybody who had wired such multi-pick into scripts or fingers,
> either.
>
> IOW, I do not necessarily agree with your "those were intuitive"
> assertion.

After sleeping on this, here are my random thoughts on this topic.

 - The sequencing flow the current "rebase -i" implements when resuming a
   controlled stop (i.e. "edit" or "reword"), even as the last step of the
   insn sheet, feels like the right thing. The actual tweaking of the
   commit done by "commit --amend" is oblivious to the sequencing state,
   and resuming is controlled by "rebase --continue".

 - The case to resume an unexpected stop (i.e. "pick" that causes conflict
   or "rebase" without "-i") also feels right. The user fixes up and marks
   things as "done" with "add/rm", and tell the stopped command that it can
   now continue with "rebase --continue". The same comment applies for
   "am".

 - Even before we started talking about the sequencer, the workflow to deal
   with conflicted "revert" or "cherry-pick" was awkward. From the end
   user's point of view, the operation the end user wanted to perform was
   "revert" (or "cherry-pick"), but we tell the user to fix things up, mark
   them as "done" with "add/rm", and commit the result themselves. We could
   have made the UI more consistent by introducing "revert --continue".

 - In a sense, the last point got worse by a recent change to make
   "commit" notice CHERRY_PICK_HEAD. This was modeled the workflow to
   conclude a conflicted "merge", that noticed "MERGE_HEAD" etc., but from
   the UI point of view, I think it was a mistake. Back when "merge" was
   the only command that needs post-clean-up by the user, saying "when
   merge stops due to conflict, you conclude it with commit, but
   everything else you conclude (or continue) with --continue" was
   reasonable, but now we have to say "after conflicted merge, revert and
   cherry-pick, you conclude it with commit; use --continue for everything
   else".

So I think that questions that affect us longer term are:

 - Does it make sense to keep this two quite different ways to resume an
   interrupted operation? Some saying "The operation you started was Foo
   but you need to conclude it with 'commit'", others saying "When Bar
   stops in the middle, you say 'Bar --continue'"?

 - If so, which camp should the sequencer-based commands fall into?

 - If not, shouldn't we be unifying the user experience to one of them,
   with backward compatibility escape hatches?

I think what d3f4628e tried to do may make sense _if_ we want to make
"commit" the way to conclude or continue sequencer-based commands. If we
really wanted to go that route, however, it would make it easier if
"commit" were the _only_ way to deal with all the "stopped because the
user needs to resolve conflicts or because the user told us to pause"
situations. For example, when "rebase -i" stops (either due to an "edit"
or a conflicted "pick"), after the user tweaks the tree and says "git
commit [--amend]" to reword, "rebase --continue" after that shouldn't be
necessary. If we are making "commit" aware of the sequencer status, it
should already know that the next thing after that invocation of "commit"
is to resume the sequencing.

But I do not think that is the direction we would want to go for two
reasons. There is a (complicated) workflow to split an commit during
"rebase -i" that does _not_ want an auto-resume by a commit. Also not
all conflicted/stopped state can be concluded with "commit" (think:
"rebase/am --skip").

I personally am leaning towards teaching "--continue" to "merge" and
"cherry-pick", while keeping only existing awareness of these two commands
in "commit" as historical mistakes (look for 'unlink(git_path("[A-Z_]*"))'
in builtin/commit.c). That would mean that in the long run, new users need
to learn only one thing: when "git Foo" stops because it needs help from
you resolving conflicts, after you help it by editing the files in your
working tree and making that with add/rm, you say "git Foo --continue" to
signal that you are done helping it.

And we do not have to worry about making "commit" only half-aware of
sequencer states.

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

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> After sleeping on this, here are my random thoughts on this topic.

They're a bit much for me to absorb so quickly.  I'll think about this
again before posting a more elaborate reply.

Thanks for your patience.

-- Ram

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

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> [...]

Here are some comments from my end after extensive thought.  Instead
of responding to your comments directly, I've sprinkled bits of it
here and there, reordering it as I see fit.  Thanks for detailing out
your thoughts so well :)

0. Before getting into historical mistakes and evaluating existing
workflows, let's first try to detail the problem the sequencer is
trying to solve: pick/ revert aren't the only operations we want to
support.  Let's imagine some hypothetical "foo" operation that does
something incredibly complex in the instruction sheet:

pick b38bcc
foo
pick 49ab7c

Now, let's imagine that the "foo" command failed.  What is the
strategy we will employ to handle his?  I think the question boils
down to: should we teach '--continue' the prerequisite for executing a
"pick" command (clean worktree etc), or how to finish a "foo" command?
 What will this answer depend upon?

1. Now, let's discuss the motivation for d3f4628e.  I didn't want to
create a separate workflow for pick-one-commit versus
pick-many-commits.  It would have to go something like:

  parse_arguments(whatever tree'ishes specified on argc)
  setup_revision_walker
  n = count_revisions
  if (n == 1)
    do foo; // Don't create sequencer state
  else
    create sequencer state; do foo; cleanup sequencer state;

Ugly.  Wouldn't you agree?  So, what choice did I have?  I can't
conditionally create sequencer state.  So, an idea struck me: if I
clean it up before someone notices, I'm safe.  So, I decided to remove
it before the last commit -- when there's only one commit, it'll be
removed immediately.  So far so good: no regressions introduced.

2.  Okay, now let's try to touch upon the issue of marking something as "done":

>  - The case to resume an unexpected stop (i.e. "pick" that causes conflict
>   or "rebase" without "-i") also feels right. The user fixes up and marks
>   things as "done" with "add/rm", and tell the stopped command that it can
>   now continue with "rebase --continue". The same comment applies for
>   "am".

I could argue that "add/rm" need not be the only way to specify "done"
in the general case.  We needn't be consistent about that- we can
print hints about what "done" would mean, and not invent a new
"--mark-as-done" command.  What varies here is the definition of
"done": does it mean that the user has "done" what the instruction
sheet operation was supposed to do (including the committing), or has
simply done a part of the job and passed on the baton?

3. How tight do we want the sequencer to be?  Should we allow
executing other commands during the operation of the sequencer, or
should we simply expect the user to "--continue" the operation first
and take care of everything else later?

>  - The sequencing flow the current "rebase -i" implements when resuming a
>   controlled stop (i.e. "edit" or "reword"), even as the last step of the
>   insn sheet, feels like the right thing. The actual tweaking of the
>   commit done by "commit --amend" is oblivious to the sequencing state,
>   and resuming is controlled by "rebase --continue".

I often "git commit -m temp" in the middle of a "rebase -i", and I'm
strongly in favor of allowing this somehow.

> There is a (complicated) workflow to split an commit during
> "rebase -i" that does _not_ want an auto-resume by a commit.

Exactly! :)
Auto-resuming after a "commit" is fundamentally wrong.

> Also not
> all conflicted/stopped state can be concluded with "commit" (think:
> "rebase/am --skip").

A good point on consistency.

4.  Let's correct the historical mistakes and get our act together.
The final pending question is: how many historical mistakes are we
willing to stomp now?

> I personally am leaning towards teaching "--continue" to "merge" and
> "cherry-pick", while keeping only existing awareness of these two commands
> in "commit" as historical mistakes (look for 'unlink(git_path("[A-Z_]*"))'
> in builtin/commit.c). That would mean that in the long run, new users need
> to learn only one thing: when "git Foo" stops because it needs help from
> you resolving conflicts, after you help it by editing the files in your
> working tree and making that with add/rm, you say "git Foo --continue" to
> signal that you are done helping it.

Okay, stomp nothing.  Under this constraint, the best we can do is:
1. Introduce a 'merge --continue' to invoke 'git commit'.  MERGE_HEAD
helps 'git commit' finish.  Modify tests to use '--continue' instead
of the earlier commit-to-finish workflow, and advertise this feature
everywhere.
2. Make 'cherry-pick --continue' invoke 'git commit' as well.
CHERRY_PICK_HEAD helps 'git commit' finish.  If the commit finishes
successfully: (if there is one commit left, remove the sequencer
state; otherwise, drop the first insn on top of the list and execute
the next insn).  Modify tests to use '--continue' instead of the
earlier commit-to-finish workflow, and advertise this feature
everywhere.  Unfortunately, if the user executes 'git commit' instead
of the newer '--continue', we're screwed: a stray sequencer state will
be left behind.

Does the above seem sensible, or should we do something about the
historical mistakes?

Thanks.

-- Ram

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

[Jonathan Nieder]

Hi Ram,

Ramkumar Ramachandra wrote:

> Here are some comments from my end after extensive thought.

Could be briefer. :)

[...]
> 1. Introduce a 'merge --continue' to invoke 'git commit'.  MERGE_HEAD
> helps 'git commit' finish.  Modify tests to use '--continue' instead
> of the earlier commit-to-finish workflow, and advertise this feature
> everywhere.

Why modify tests?  I think "git merge --continue" is a nice idea,
and I don't see how it's inconsistent in any way with continuing to
allow old practice.

> 2. Make 'cherry-pick --continue' invoke 'git commit' as well.
> CHERRY_PICK_HEAD helps 'git commit' finish.  If the commit finishes
> successfully: (if there is one commit left, remove the sequencer
> state; otherwise, drop the first insn on top of the list and execute
> the next insn).

Sounds like a sensible thing to do.  I assume the "one" in the
parenthesis is supposed to be "zero", making the "if" not even part of
the user-visible description of what it does --- it's just the
termination condition of a loop.

"git cherry-pick --continue" in place of "git commit" does not handle
the following scenario.  Suppose my multiple-cherry-pick has run into
conflicts, and while fixing them I notice something related that needs
to be fixed.

	... resolve conflict, leaving extra change in worktree ...
	git stash -k
	... test test test ...
	git commit

	git stash pop
	git commit; # make a separate commit for extra change

	# ok, now continue.
	git sequencer --continue

In other words, in this sequence of commands, "git commit" is used to
single-step.  So if one wants to remove CHERRY_PICK_HEAD altogether, a
nice thing to do would be to introduce a "git sequencer --single-step"
command or something similar to handle such cases.

> Modify tests to use '--continue' instead of the
> earlier commit-to-finish workflow, and advertise this feature
> everywhere.  Unfortunately, if the user executes 'git commit' instead
> of the newer '--continue', we're screwed: a stray sequencer state will
> be left behind.

As Junio hinted, it could make a lot of sense for "git cherry-pick
<single commit>" to not create sequencer state in the first place.
"git cherry-pick --continue" does not need it --- it is enough to
commit with the conflict resolved.  "git cherry-pick --abort" does not
need it, either --- it is enough to "git reset --merge HEAD".

One part I'm handwaving is what to do about commands like "git
cherry-pick foo^..foo" which use a commit range that only happens to
contain one commit.  Either behavior seems fine for such commands.

What do you think?

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

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Could be briefer. :)

Sorry about the braindump :P

>> 1. Introduce a 'merge --continue' to invoke 'git commit'.  MERGE_HEAD
>> helps 'git commit' finish.  Modify tests to use '--continue' instead
>> of the earlier commit-to-finish workflow, and advertise this feature
>> everywhere.
>
> Why modify tests?  I think "git merge --continue" is a nice idea,
> and I don't see how it's inconsistent in any way with continuing to
> allow old practice.

In the future, we might want a 'merge' instruction in the sequencer --
I want to make it clear that we're going for a significant UI change
so that everyone (including tests, scripts) become comfortable with
the new UI.

>> 2. Make 'cherry-pick --continue' invoke 'git commit' as well.
>> CHERRY_PICK_HEAD helps 'git commit' finish.  If the commit finishes
>> successfully: (if there is one commit left, remove the sequencer
>> state; otherwise, drop the first insn on top of the list and execute
>> the next insn).
>
> Sounds like a sensible thing to do.  I assume the "one" in the
> parenthesis is supposed to be "zero", making the "if" not even part of
> the user-visible description of what it does --- it's just the
> termination condition of a loop.

Right, sorry about the convoluted thought.

> As Junio hinted, it could make a lot of sense for "git cherry-pick
> <single commit>" to not create sequencer state in the first place.
> "git cherry-pick --continue" does not need it --- it is enough to
> commit with the conflict resolved.  "git cherry-pick --abort" does not
> need it, either --- it is enough to "git reset --merge HEAD".

Okay, here's my problem with the idea: it'll essentially require the
sequencer to differentiate between one-commit operations and
many-commit operations.  In the case of one-commit operations, *every*
new command that calls into the sequencer will will need to persist
information in its own way using hacks like CHERRY_PICK_HEAD and
MERGE_HEAD.  And we have to make "git commit" unlink yet another file
:)  I'm not talking about some hypothetical case: I'm already planning
to make 'git am' call into the sequencer, so we'll need an AM_HEAD.

One final resort: Move some code back into cherry-pick, and call into
a later-function in the sequencer only if it's a many-commit
operation.  The new commands can enjoy the comfort of calling into an
earlier-function in the sequencer that'll do all the revision walk
setup and call the later-function.  I think this is reasonable.

> One part I'm handwaving is what to do about commands like "git
> cherry-pick foo^..foo" which use a commit range that only happens to
> contain one commit.  Either behavior seems fine for such commands.

I don't think I follow.  This will be determined as a single-commit
operation after setting up the revisions.  I don't think it should be
treated as a multi-commit operation because the literal tree'ish
contains "..".

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> In the future, we might want a 'merge' instruction in the sequencer --
> I want to make it clear that we're going for a significant UI change
> so that everyone (including tests, scripts) become comfortable with
> the new UI.

I don't follow.  I meant "Why modify tests, rather than adding new
ones?"  Tests document what is supposed to continue working.

[...]
> Okay, here's my problem with the idea: it'll essentially require the
> sequencer to differentiate between one-commit operations and
> many-commit operations.  In the case of one-commit operations, *every*
> new command that calls into the sequencer will will need to persist
> information in its own way using hacks like CHERRY_PICK_HEAD and
> MERGE_HEAD.

Why wouldn't such a backward-compatibility feature apply to
cherry-pick/revert only?

> I'm not talking about some hypothetical case: I'm already planning
> to make 'git am' call into the sequencer, so we'll need an AM_HEAD.

Yuck.  How does the UI distinguish between

	git sequencer --continue; # apply the rest of the patches from mbox

and

	git sequencer --continue; # finish up "am" insn and continue

?  Does the sequencer need an argument to indicate the level of
nesting?  I would find it more straightforward for "git am mbox" to
create a todo list saying something like

	apply patch 1 from mbox
	apply patch 2 from mbox
	apply patch 3 from mbox
	apply patch 4 from mbox

so there would still be only one pending sequence of basic
instructions to think about.

> One final resort: Move some code back into cherry-pick, and call into
> a later-function in the sequencer only if it's a many-commit
> operation.  The new commands can enjoy the comfort of calling into an
> earlier-function in the sequencer that'll do all the revision walk
> setup and call the later-function.  I think this is reasonable.

I'm having trouble parsing this; sorry.  What is the effect on the
user-visible behavior of the program of what you propose?

> Jonathan Nieder writes:

>> One part I'm handwaving is what to do about commands like "git
>> cherry-pick foo^..foo" which use a commit range that only happens to
>> contain one commit.  Either behavior seems fine for such commands.
>
> I don't think I follow.  This will be determined as a single-commit
> operation after setting up the revisions.  I don't think it should be
> treated as a multi-commit operation because the literal tree'ish
> contains "..".

I said "Either behavior seems fine", didn't I?

Hope that clarifies a little.
Jonathan

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

[Ramkumar Ramachandra]

HI again,

Ramkumar Ramachandra writes:
> One final resort: Move some code back into cherry-pick, and call into
> a later-function in the sequencer only if it's a many-commit
> operation.  The new commands can enjoy the comfort of calling into an
> earlier-function in the sequencer that'll do all the revision walk
> setup and call the later-function.  I think this is reasonable.

Er, no.  This would be amazingly ugly too: we'd need two versions of
everything like '--continue'.  And look at this glaring inconsistency:

  git cherry-pick foo..bar
  ..conflict..
  git sequencer --abort
  $? = 0

But:

  git cherry-pick foo
  ..conflict..
  git sequencer --abort
  error: No sequencer operation in progress.

In a nutshell, the problem we're facing: We persist part of the
continuation data outside the sequencer's knowledge, and part of it
inside.  For backward compatibility, we can't move the former part
into the sequencer.

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> But:
>
>   git cherry-pick foo
>   ..conflict..
>   git sequencer --abort
>   error: No sequencer operation in progress.

I mentioned this before: "git sequencer --abort" could notice that
CHERRY_PICK_HEAD exists and .git/sequencer does not and act
accordingly.

> In a nutshell, the problem we're facing: We persist part of the
> continuation data outside the sequencer's knowledge, and part of it
> inside.  For backward compatibility, we can't move the former part
> into the sequencer.

Why can't it become the sequencer's responsibility, FWIW?  That's an
implementation detail.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>> Why modify tests?  I think "git merge --continue" is a nice idea,
>> and I don't see how it's inconsistent in any way with continuing to
>> allow old practice.

I agree. Updating the test will hide a regression if Ram's update breaks
the existing workflow to conclude a conflicted merge with "git commit".
If we are to add "git merge --continue" sugarcoat to make it easier to
teach new people saying that "To any Git operation that stops and asks you
to help, you can tell it that you are done helping by re-running the same
command with --continue flag", then _new_ tests should be added to make
sure that "git merge --continue" does act just like "git commit" in such a
case.

> In the future, we might want a 'merge' instruction in the sequencer --

The end-user facing frontend may be "git rebase" in such a case, and we
would be teaching the users "When you are done helping the conflicted
'rebase', tell it that you are done by running 'rebase --continue'".  The
command verb 'merge' on the sequencer insn sheet does not have any direct
connection to 'git merge' command (e.g. 'pick' does not have to be and is
not implemented using 'git pick' command that does not exist). So I do not
think we would ever say "merge --continue" in this context to begin with.

>> As Junio hinted, it could make a lot of sense for "git cherry-pick
>> <single commit>" to not create sequencer state in the first place.
>> "git cherry-pick --continue" does not need it --- it is enough to
>> commit with the conflict resolved.  "git cherry-pick --abort" does not
>> need it, either --- it is enough to "git reset --merge HEAD".
>
> Okay, here's my problem with the idea: it'll essentially require the
> sequencer to differentiate between one-commit operations and
> many-commit operations.

I think you are looking at it in a wrong way. It is just about giving
backward compatibility to historical hacks. cherry-pick and revert may be
the only ones needed, and a new command Foo that implements its workflow
in terms of the sequencer can choose to (and should choose to unless there
is a compelling reason not to, because of the exact reason you stated
here) do a single-command insn sheet with "git Foo --continue" to conclude
if that one and only step needed help from the end user.
 
"git am" would fit in the picture the same way. The sequencer insn sheet
it would produce would consist of "patch <filename>" or something, and
after the last one of them fails, the user would fix things up in the
working tree, "git am --continue" will create a commit, and because there
is no more steps in the sequence, the sequencer state will be removed.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> One part I'm handwaving is what to do about commands like "git
> cherry-pick foo^..foo" which use a commit range that only happens to
> contain one commit.

Historically cherry-pick took a single commit from the command line. That
can easily be distinguished from a "set that ended up with one positive
commit after walking the history". I think you can even check before
triggering the revision walking machinery in the first place.

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

[Ramkumar Ramachandra]

Hi Jonathan and Junio,

Jonathan Nieder writes:
> Why can't it become the sequencer's responsibility, FWIW?  That's an
> implementation detail.

It can be the sequencer's responsibility if we want that!  I'm just
dabbling with the different implementation strategies and trying to
see the advantages/ disadvantages of each one.  The "right" thing to
do wasn't obvious to me immediately.

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>> Why modify tests?  I think "git merge --continue" is a nice idea,
>>> and I don't see how it's inconsistent in any way with continuing to
>>> allow old practice.
>
> I agree. Updating the test will hide a regression if Ram's update breaks
> the existing workflow to conclude a conflicted merge with "git commit".
> If we are to add "git merge --continue" sugarcoat to make it easier to
> teach new people saying that "To any Git operation that stops and asks you
> to help, you can tell it that you are done helping by re-running the same
> command with --continue flag", then _new_ tests should be added to make
> sure that "git merge --continue" does act just like "git commit" in such a
> case.

Right.  We have to keep the old tests around -- my bad.

>>> As Junio hinted, it could make a lot of sense for "git cherry-pick
>>> <single commit>" to not create sequencer state in the first place.
>>> "git cherry-pick --continue" does not need it --- it is enough to
>>> commit with the conflict resolved.  "git cherry-pick --abort" does not
>>> need it, either --- it is enough to "git reset --merge HEAD".
>>
>> Okay, here's my problem with the idea: it'll essentially require the
>> sequencer to differentiate between one-commit operations and
>> many-commit operations.
>
> I think you are looking at it in a wrong way. It is just about giving
> backward compatibility to historical hacks. cherry-pick and revert may be
> the only ones needed, and a new command Foo that implements its workflow
> in terms of the sequencer can choose to (and should choose to unless there
> is a compelling reason not to, because of the exact reason you stated
> here) do a single-command insn sheet with "git Foo --continue" to conclude
> if that one and only step needed help from the end user.

No, no.  I don't _want_ to create an AM_HEAD or FOO_HEAD for every new
command that we write -- I was merely pointing out what would happen
if the sequencer were only built to handle multi-commit-operations,
and every new command would have to handle single-commit-operations on
their own.  I was emphasizing that the sequencer will need to handle
single-commit-operations as well; Jonathan has suggested that we have
special hacks to handle cherry-pick and merge commands in the
sequencer.

To conclude, let me list out the various implementation strategies
we've thought about, and the one we seem to have settled on:
0. Remove the sequencer state from "git commit".  This is wrong, as
Junio pointed out in the first email.
1. Let commands handle single-commit-operations themselves, and call
into the sequencer only for multi-commit-operations.  If they don't
call into the sequencer at all, there's no state to persist or
cleanup.  This approach is clearly wrong because each new command
would have to come up with AM_HEAD and FOO_HEAD to persist the
single-commit-operation state.
2. Expose two functions from the sequencer: "earlier-function" and
"later-function".  Let cherry-pick and merge handle their own
CHERRY_PICK_HEAD and MERGE_HEAD hacks first (by setting up the
revision walker, counting commits etc), and call into later-function
in the sequencer to do less work from the sequencer's end.  All new
commands can directly call into earlier-function directly.  I know
it's a little hand-wavy, but I hope it's atleast parse'able this time.
 As I pointed out in another email, this is also broken because
cherry-pick/ merge would have to implement their own versions of every
subcommand like "--continue".
3. Let all commands call into the sequencer for everything.  Let's
teach the sequencer about the CHERRY_PICK_HEAD and MERGE_HEAD hacks so
that it can deal with them when a "pick", "revert" or "merge"
operation is encountered.  We'll handle it by treating
CHERRY_PICK_HEAD and MERGE_HEAD as part of the "sequencer state" (so
that subcommands work just fine on them).  For all future sequencer
commands, all data will be persisted inside '.git/sequencer'.  This
seems most reasonable now.

In a nutshell, instead of side-stepping historical hacks, we want to
teach the sequencer about them as specific cases to handle carefully.
We want to this without affecting the operation of the sequencer in
the general case.  Sounds great!  An uncompromising UI at the cost of
little ugliness in the sequencer.

Thanks.

-- Ram