[RFC PATCH 0/5] Some preliminary work based on sequencer-stable

[RFC PATCH 0/5] Some preliminary work based on sequencer-stable

[Ramkumar Ramachandra]

Hi,

The series is quite unpolished at the moment, but I'm sending it to
the list because I want early feedback before I make mypoic decisions
and waste work.  Tests and rough commit messages included :)

The first patch is something I should've done earlier -- it makes
debugging much easier.  Should it go into maint? The last patch is the
most important, and everything else is just building towards it.  With
this series, we should now be able to abort at the last-but-one
commit, since 'git commit' actually cleans up the sequencer state now.

Note: All the SHA-1 references in the commit messages will change
after sequencer-stable is merged.  I hope it's clear for the moment.

Thanks for reading.

-- Ram

Ramkumar Ramachandra (5):
  revert: Don't remove the sequencer state on error
  sequencer.h: Move data structures
  revert: Allow mixed pick and revert instructions
  sequencer: Expose code that handles files in .git/sequencer
  sequencer: Remove sequencer state after final commit

 builtin/commit.c                |    5 +
 builtin/revert.c                |  417 +++------------------------------------
 sequencer.c                     |  323 ++++++++++++++++++++++++++++++
 sequencer.h                     |   53 +++++
 t/t3510-cherry-pick-sequence.sh |   62 ++++++-
 5 files changed, 471 insertions(+), 389 deletions(-)

-- 
1.7.6.351.gb35ac.dirty

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

[Ramkumar Ramachandra]

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

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

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

[PATCH 2/5] sequencer.h: Move data structures

[Ramkumar Ramachandra]

Prepare to move more stuff into generalized sequencer.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   55 ++++++++++++++---------------------------------------
 sequencer.h      |   25 ++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index a548a14..81502d4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,43 +39,18 @@ 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 {
-	enum replay_action action;
-	enum replay_subcommand subcommand;
-
-	/* Boolean options */
-	int edit;
-	int record_origin;
-	int no_commit;
-	int signoff;
-	int allow_ff;
-	int allow_rerere_auto;
-
-	int mainline;
-	int commit_argc;
-	const char **commit_argv;
-
-	/* Merge strategy */
-	const char *strategy;
-	const char **xopts;
-	size_t xopts_nr, xopts_alloc;
-};
-
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static const char *action_name(const struct replay_opts *opts)
 {
-	return opts->action == 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 +129,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 +328,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."));
@@ -532,7 +507,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->action == REVERT) {
+	if (opts->action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -575,7 +550,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") || opts->action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -594,7 +569,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	}
 
 	if (res) {
-		error(opts->action == REVERT
+		error(opts->action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -618,7 +593,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);
@@ -680,7 +655,7 @@ static int format_todo(struct strbuf *buf, struct commit_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";
+	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
 
 	for (cur = todo_list; cur; cur = cur->next) {
 		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
@@ -700,11 +675,11 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 	char *p, *q;
 
 	if (!prefixcmp(start, "pick ")) {
-		action = CHERRY_PICK;
+		action = REPLAY_PICK;
 		insn_len = strlen("pick");
 		p = start + insn_len + 1;
 	} else if (!prefixcmp(start, "revert ")) {
-		action = REVERT;
+		action = REPLAY_REVERT;
 		insn_len = strlen("revert");
 		p = start + insn_len + 1;
 	} else
@@ -723,7 +698,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 	 */
 	if (action != opts->action) {
 		const char *action_str;
-		action_str = action == REVERT ? "revert" : "cherry-pick";
+		action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
 		error(_("Cannot %s during a %s"), action_str, action_name(opts));
 		return NULL;
 	}
@@ -989,7 +964,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"));
 		}
@@ -1009,7 +984,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);
@@ -1024,7 +999,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..0b5d94e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,6 +7,31 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #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;
+	int commit_argc;
+	const char **commit_argv;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+};
+
 /*
  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset'.
-- 
1.7.6.351.gb35ac.dirty

[PATCH 3/5] revert: Allow mixed pick and revert instructions

[Ramkumar Ramachandra]

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

  pick fdc0b12 picked
  revert 965fed4 anotherpick

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

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 81502d4..d48a92e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -639,89 +639,84 @@ 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(struct replay_insn_list));
+	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 replay_insn_list *cur = NULL;
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
+	const char *action_str;
 
 	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))
+		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);
 	}
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static int parse_insn_line(char *start, enum replay_action *action,
+			struct commit **operand)
 {
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
-	enum replay_action action;
-	int insn_len = 0;
+	int keyword_len;
 	char *p, *q;
 
 	if (!prefixcmp(start, "pick ")) {
-		action = REPLAY_PICK;
-		insn_len = strlen("pick");
-		p = start + insn_len + 1;
+		*action = REPLAY_PICK;
+		keyword_len = strlen("pick");
+		p = start + keyword_len + 1;
 	} else if (!prefixcmp(start, "revert ")) {
-		action = REPLAY_REVERT;
-		insn_len = strlen("revert");
-		p = start + insn_len + 1;
+		*action = REPLAY_REVERT;
+		keyword_len = strlen("revert");
+		p = start + keyword_len + 1;
 	} else
-		return NULL;
+		return -1;
 
 	q = strchr(p, ' ');
 	if (!q)
-		return NULL;
+		return -1;
 	q++;
 
 	strlcpy(sha1_abbrev, p, q - p);
 
-	/*
-	 * 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 == REPLAY_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 -1;
+
+	*operand = lookup_commit_reference(commit_sha1);
+	if (!*operand)
+		return -1;
 
-	return lookup_commit_reference(commit_sha1);
+	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;
+	enum replay_action action;
+	struct commit *operand;
 	char *p = buf;
 	int i;
 
 	for (i = 1; *p; i++) {
-		commit = parse_insn_line(p, opts);
-		if (!commit)
+		if (parse_insn_line(p, &action, &operand) < 0)
 			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(action, operand, next);
 		p = strchrnul(p, '\n');
 		if (*p)
 			p++;
@@ -731,8 +726,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;
@@ -748,7 +742,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);
@@ -797,18 +791,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 commit *operand;
+	struct replay_insn_list **next;
 
 	prepare_revs(&revs, opts);
 
 	next = todo_list;
-	while ((commit = get_revision(&revs)))
-		next = commit_list_append(commit, next);
+	while ((operand = get_revision(&revs)))
+		next = replay_insn_list_append(opts->action, operand, next);
 }
 
 static int create_seq_dir(void)
@@ -837,7 +831,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;
@@ -845,7 +839,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);
@@ -889,9 +883,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);
@@ -901,8 +896,9 @@ 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);
+		opts->action = cur->action;
+		res = do_pick_commit(cur->operand, opts);
 		if (res) {
 			if (!cur->next && res > 0)
 				/*
@@ -927,7 +923,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);
@@ -944,7 +940,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))
diff --git a/sequencer.h b/sequencer.h
index 0b5d94e..01caa8a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -32,6 +32,12 @@ struct replay_opts {
 	size_t xopts_nr, xopts_alloc;
 };
 
+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 3bca2b3..f85372c 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -211,4 +211,62 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_must_fail git cherry-pick --continue
 '
 
+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 3/5] revert: Allow mixed pick and revert instructions

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Change the way the instruction parser works, allowing arbitrary
> (action, operand) pairs to be parsed.  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.

Nice. :)

> This patch lays the foundation for extending the parser to support
> more actions.

And why would I want to do that?  I think there's a missing "so git
rebase -i can reuse this machinery some day" at the end of the
sentence.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -639,89 +639,84 @@ 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));
> +	struct replay_insn_list *new = xmalloc(sizeof(struct replay_insn_list));

Tip: we can save the readers some reading and prepare for future
renaming of the structure (not that that's something to fear) using
the idiom

	struct replay_insn_list *new = xmalloc(sizeof(*new));

[...]
> -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 replay_insn_list *cur = NULL;
>  	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
>  	const char *sha1_abbrev = NULL;
> -	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
> +	const char *action_str;

Might be clearer with narrower scope:

	struct replay_insn_list *cur;

	for (cur = todo_list; cur; cur = cur->next) {
		struct commit_message msg = COMMIT_MESSAGE_INIT;
		const char *sha1_abbrev, *action_str;

		sha1_abbrev = ...;
		action_str = ...;
		if (get_message(cur->operand, &msg))
			return error(...);
		strbuf_addf(buf, "%s %s %s\n", ...);
	}
	return 0;

By the way, shouldn't there a free_message() call to balance out the
get_message()?

[...]
> -static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
> +static int parse_insn_line(char *start, enum replay_action *action,
> +			struct commit **operand)

Hm, why not

 static int parse_insn_line(const char *start, struct replay_insn_list *item);

>  {
>  	unsigned char commit_sha1[20];
>  	char sha1_abbrev[40];
> -	enum replay_action action;
> -	int insn_len = 0;
> +	int keyword_len;

What is this renaming about?  Maybe "action_len" would work to clarify
that this is about the length of the action string at the start, not
the entire line.

By the way, what are the semantics of that variable?  It doesn't seem
to be used, so couldn't we just eliminate it?

>  	char *p, *q;
>  
>  	if (!prefixcmp(start, "pick ")) {
> -		action = REPLAY_PICK;
> -		insn_len = strlen("pick");
> -		p = start + insn_len + 1;
> +		*action = REPLAY_PICK;
> +		keyword_len = strlen("pick");
> +		p = start + keyword_len + 1;

In such a scenario, this would say

	if (!prefixcmp(start, "pick ")) {
		item->action = REPLAY_PICK;
		p += strlen("pick ");
	}

[...]
>  	} else
> -		return NULL;
> +		return -1;

Unrelated to this patch: maybe

 	return error("unrecognized action in sequencer file: %s", start);

to be easier to debug.

>  	q = strchr(p, ' ');
>  	if (!q)
> -		return NULL;
> +		return -1;

So we reject "pick a87c8989"?  That's a shame.

>  	q++;
>  
>  	strlcpy(sha1_abbrev, p, q - p);

memcpy would be clearer.  Can't this overflow the sha1_abbrev buffer?

[...]
>  	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
> -		return NULL;
> +		return -1;

get_sha1 doesn't print a message, so maybe:

	return error("malformed object name in sequencer file: %s",
							sha1_abbrev);

> +
> +	*operand = lookup_commit_reference(commit_sha1);
> +	if (!*operand)
> +		return -1;

Perhaps something like

	return error("operand %s in sequencer file is not a commit",
							sha1_abbrev);

[...]
> @@ -797,18 +791,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 *operand;

Can avoid some churn by keeping the old name here.

> -	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);
> +	while ((operand = get_revision(&revs)))
> +		next = replay_insn_list_append(opts->action, operand, next);
[...]
> @@ -901,8 +896,9 @@ 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);
> +		opts->action = cur->action;
> +		res = do_pick_commit(cur->operand, opts);

If do_pick_commit took an "action" operand, this would be less
scary. :)

[...]
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -211,4 +211,62 @@ test_expect_success 'malformed instruction sheet 2' '
>  	test_must_fail git cherry-pick --continue
>  '
>  
> +test_expect_success 'revert --continue continues after cherry-pick' '

I haven't looked at the tests yet.  FWIW, with whatever changes above
seem suitable,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Hi,

On a note unrelated to Jonathan's review, I noticed some weird
behavior while playing with my new series: when picking a commit
that's already picked, all the code until run_git_commit runs just
fine.  Then 'git commit' fails because it's an empty commit.
Unfortunately, run_git_commit returns a positive return status from
do_pick_commit which is against the convention, leading the
cherry-picking machinery to think that a conflict was encountered.
This is highly confusing from the end-user's perspective.  The
operation has suddenly stopped without an error, and '--continue'
seems to continue the operation.  What is going on, right?

I was hesitant to send out the patch to make run_git_commit return an
error, because it breaks some tests in t3505-cherry-pick-empty.sh.
However, the problem is getting on my nerves now, and I believe that
the patch does the Right Thing (TM), even if it means that we have to
change the tests in t3505-cherry-pick-empty.sh.  Thoughts?

-- 8< --
Subject: [PATCH] revert: Classify failure to run 'git commit' as an error

Since a93d2a (revert: Propagate errors upwards from do_pick_commit,
2011-05-20), do_pick_commit differentiates between conflicts and
errors using the signed-ness of its return status.  Unfortunately, it
returns the return status from 'git commit' when it fails to run it,
breaking this convention.  Change this so that any non-zero return
status from run_git_commit is classified as an error.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 8b452e8..fcd4f3a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -603,7 +603,9 @@ static int do_pick_commit(struct commit *commit,
struct replay_opts *opts)
                rerere(opts->allow_rerere_auto);
        } else {
                if (!opts->no_commit)
-                       res = run_git_commit(defmsg, opts);
+                       if (run_git_commit(defmsg, opts))
+                               return error(_("Failed to run 'git
commit': %s"),
+                                       sha1_to_hex(commit->object.sha1));
        }

        free_message(&msg);
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Hi Jonathan,

I fixed everything; just a few comments/ doubts:

Jonathan Nieder writes:
>>       } else
>> -             return NULL;
>> +             return -1;
>
> Unrelated to this patch: maybe
>
>        return error("unrecognized action in sequencer file: %s", start);
>
> to be easier to debug.

I'd initially refrained from doing this so that errors don't become
overtly verbose, but I suppose it's alright considering the fact that
we're going to make the instruction sheet editable sometime in the
future.  I tweaked the error strings a little so that we get something
like:

  error: Unrecognized action: part
  error: Could not parse line 3.
  fatal: Unusable instruction sheet: .git/sequencer

In essence, I didn't want to be redundant and mention the sequencer in
every line.  I like the above.

>>       q = strchr(p, ' ');
>>       if (!q)
>> -             return NULL;
>> +             return -1;
>
> So we reject "pick a87c8989"?  That's a shame.

Good point.  Fixing this will have to be in another patch, where I'll
advertise the fact that I'm changing the instruction sheet format.

>>       q++;
>>
>>       strlcpy(sha1_abbrev, p, q - p);
>
> memcpy would be clearer.  Can't this overflow the sha1_abbrev buffer?

Good point.  I'm tempted to check (q - p < 40); is there a better way
to do this by not hardcoding "40" perhaps?

> Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

-- Ram

Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Good point.  I'm tempted to check (q - p < 40); is there a better way
> to do this by not hardcoding "40" perhaps?

Something like the following seems idiomatic.

	if (q - p + 1 > sizeof(buf))
		return error(...);
	memcpy(buf, p, q - p);
	buf[q - p] = '\0';

[PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer

[Ramkumar Ramachandra]

Move the saving and parsing functions for files in '.git/sequencer'
from 'builtin/revert.c' into 'sequencer.c' and expose them using a
sane API.  This is the first step towards a generalized sequencer.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  332 +----------------------------------------------------
 sequencer.c      |  309 ++++++++++++++++++++++++++++++++++++++++++++++++++
 sequencer.h      |   21 ++++
 3 files changed, 337 insertions(+), 325 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index d48a92e..491c10a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -5,7 +5,6 @@
 #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"
@@ -46,8 +45,6 @@ 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;
@@ -189,80 +186,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	opts->commit_argv = argv;
 }
 
-struct commit_message {
-	char *parent_label;
-	const char *label;
-	const char *subject;
-	char *reencoded_message;
-	const char *message;
-};
-
-static int get_message(struct commit *commit, struct commit_message *out)
-{
-	const char *encoding;
-	const char *abbrev, *subject;
-	int abbrev_len, subject_len;
-	char *q;
-
-	if (!commit->buffer)
-		return -1;
-	encoding = get_encoding(commit->buffer);
-	if (!encoding)
-		encoding = "UTF-8";
-	if (!git_commit_encoding)
-		git_commit_encoding = "UTF-8";
-
-	out->reencoded_message = NULL;
-	out->message = commit->buffer;
-	if (strcmp(encoding, git_commit_encoding))
-		out->reencoded_message = reencode_string(commit->buffer,
-					git_commit_encoding, encoding);
-	if (out->reencoded_message)
-		out->message = out->reencoded_message;
-
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
-
-	subject_len = find_commit_subject(out->message, &subject);
-
-	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
-			      strlen("... ") + subject_len + 1);
-	q = out->parent_label;
-	q = mempcpy(q, "parent of ", strlen("parent of "));
-	out->label = q;
-	q = mempcpy(q, abbrev, abbrev_len);
-	q = mempcpy(q, "... ", strlen("... "));
-	out->subject = q;
-	q = mempcpy(q, subject, subject_len);
-	*q = '\0';
-	return 0;
-}
-
 static void free_message(struct commit_message *msg)
 {
 	free(msg->parent_label);
 	free(msg->reencoded_message);
 }
 
-static char *get_encoding(const char *message)
-{
-	const char *p = message, *eol;
-
-	while (*p && *p != '\n') {
-		for (eol = p + 1; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
-		if (!prefixcmp(p, "encoding ")) {
-			char *result = xmalloc(eol - 8 - p);
-			strlcpy(result, p + 9, eol - 8 - p);
-			return result;
-		}
-		p = eol;
-		if (*p == '\n')
-			p++;
-	}
-	return NULL;
-}
-
 static void write_cherry_pick_head(struct commit *commit)
 {
 	int fd;
@@ -494,7 +423,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		return error(_("%s: cannot parse parent commit %s"),
 			action_name(opts), sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit, &msg) != 0)
+	if (get_commit_message(commit, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
 			sha1_to_hex(commit->object.sha1));
 
@@ -622,175 +551,6 @@ static void read_and_refresh_cache(struct replay_opts *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(struct replay_insn_list));
-	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 = NULL;
-	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
-	const char *sha1_abbrev = NULL;
-	const char *action_str;
-
-	for (cur = todo_list; cur; cur = cur->next) {
-		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);
-	}
-	return 0;
-}
-
-static int parse_insn_line(char *start, enum replay_action *action,
-			struct commit **operand)
-{
-	unsigned char commit_sha1[20];
-	char sha1_abbrev[40];
-	int keyword_len;
-	char *p, *q;
-
-	if (!prefixcmp(start, "pick ")) {
-		*action = REPLAY_PICK;
-		keyword_len = strlen("pick");
-		p = start + keyword_len + 1;
-	} else if (!prefixcmp(start, "revert ")) {
-		*action = REPLAY_REVERT;
-		keyword_len = strlen("revert");
-		p = start + keyword_len + 1;
-	} else
-		return -1;
-
-	q = strchr(p, ' ');
-	if (!q)
-		return -1;
-	q++;
-
-	strlcpy(sha1_abbrev, p, q - p);
-
-	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
-		return -1;
-
-	*operand = lookup_commit_reference(commit_sha1);
-	if (!*operand)
-		return -1;
-
-	return 0;
-}
-
-static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
-{
-	struct replay_insn_list **next = todo_list;
-	enum replay_action action;
-	struct commit *operand;
-	char *p = buf;
-	int i;
-
-	for (i = 1; *p; i++) {
-		if (parse_insn_line(p, &action, &operand) < 0)
-			return error(_("Could not parse line %d."), i);
-		next = replay_insn_list_append(action, 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)
 {
@@ -805,84 +565,6 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 		next = replay_insn_list_append(opts->action, operand, 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)
 {
@@ -896,7 +578,7 @@ static int pick_commits(struct replay_insn_list *todo_list,
 	read_and_refresh_cache(opts);
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur);
+		sequencer_save_todo(cur);
 		opts->action = cur->action;
 		res = do_pick_commit(cur->operand, opts);
 		if (res) {
@@ -939,8 +621,8 @@ static int pick_revisions(struct replay_opts *opts)
 	} 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);
+		sequencer_read_opts(&opts);
+		sequencer_read_todo(&todo_list);
 
 		/* Verify that the conflict has been resolved */
 		if (!index_differs_from("HEAD", 0))
@@ -953,7 +635,7 @@ static int pick_revisions(struct replay_opts *opts)
 		 */
 
 		walk_revs_populate_todo(&todo_list, opts);
-		if (create_seq_dir() < 0) {
+		if (sequencer_create_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"));
@@ -964,8 +646,8 @@ static int pick_revisions(struct replay_opts *opts)
 				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);
+		sequencer_save_head(sha1_to_hex(sha1));
+		sequencer_save_opts(opts);
 	}
 	return pick_commits(todo_list, opts);
 error:
diff --git a/sequencer.c b/sequencer.c
index bc2c046..2993846 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2,6 +2,8 @@
 #include "sequencer.h"
 #include "strbuf.h"
 #include "dir.h"
+#include "commit.h"
+#include "utf8.h"
 
 void remove_sequencer_state(int aggressive)
 {
@@ -17,3 +19,310 @@ void remove_sequencer_state(int aggressive)
 	strbuf_release(&seq_dir);
 	strbuf_release(&seq_old_dir);
 }
+
+/*
+ * 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(struct replay_insn_list));
+	new->action = action;
+	new->operand = operand;
+	*next = new;
+	new->next = NULL;
+	return &new->next;
+}
+
+static int parse_insn_line(char *start, enum replay_action *action,
+			struct commit **operand)
+{
+	unsigned char commit_sha1[20];
+	char sha1_abbrev[40];
+	int keyword_len;
+	char *p, *q;
+
+	if (!prefixcmp(start, "pick ")) {
+		*action = REPLAY_PICK;
+		keyword_len = strlen("pick");
+		p = start + keyword_len + 1;
+	} else if (!prefixcmp(start, "revert ")) {
+		*action = REPLAY_REVERT;
+		keyword_len = strlen("revert");
+		p = start + keyword_len + 1;
+	} else
+		return -1;
+
+	q = strchr(p, ' ');
+	if (!q)
+		return -1;
+	q++;
+
+	strlcpy(sha1_abbrev, p, q - p);
+
+	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+		return -1;
+
+	*operand = lookup_commit_reference(commit_sha1);
+	if (!*operand)
+		return -1;
+
+	return 0;
+}
+
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
+{
+	struct replay_insn_list **next = todo_list;
+	enum replay_action action;
+	struct commit *operand;
+	char *p = buf;
+	int i;
+
+	for (i = 1; *p; i++) {
+		if (parse_insn_line(p, &action, &operand) < 0)
+			return error(_("Could not parse line %d."), i);
+		next = replay_insn_list_append(action, operand, next);
+		p = strchrnul(p, '\n');
+		if (*p)
+			p++;
+	}
+	if (!*todo_list)
+		return error(_("No commits parsed."));
+	return 0;
+}
+
+void sequencer_read_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 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;
+}
+
+int get_commit_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;
+}
+
+void sequencer_read_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 int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
+{
+	struct replay_insn_list *cur = NULL;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	const char *sha1_abbrev = NULL;
+	const char *action_str;
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		if (get_commit_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);
+	}
+	return 0;
+}
+
+int sequencer_create_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 sequencer_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);
+}
+
+void sequencer_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);
+}
+
+void sequencer_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);
+	}
+}
diff --git a/sequencer.h b/sequencer.h
index 01caa8a..a5b951d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -38,6 +38,17 @@ struct replay_insn_list {
 	struct replay_insn_list *next;
 };
 
+/* Unrelated commit_message helper */
+struct commit_message {
+	char *parent_label;
+	const char *label;
+	const char *subject;
+	char *reencoded_message;
+	const char *message;
+};
+
+int get_commit_message(struct commit *commit, struct commit_message *out);
+
 /*
  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset'.
@@ -48,4 +59,14 @@ struct replay_insn_list {
  */
 void remove_sequencer_state(int aggressive);
 
+struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+						struct commit *operand,
+						struct replay_insn_list **next);
+void sequencer_read_todo(struct replay_insn_list **todo_list);
+void sequencer_read_opts(struct replay_opts **opts_ptr);
+int sequencer_create_dir(void);
+void sequencer_save_head(const char *head);
+void sequencer_save_todo(struct replay_insn_list *todo_list);
+void sequencer_save_opts(struct replay_opts *opts);
+
 #endif
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -38,6 +38,17 @@ struct replay_insn_list {
>  	struct replay_insn_list *next;
>  };
>  
> +/* Unrelated commit_message helper */
> +struct commit_message {
> +	char *parent_label;
> +	const char *label;
> +	const char *subject;
> +	char *reencoded_message;
> +	const char *message;
> +};
> +
> +int get_commit_message(struct commit *commit, struct commit_message *out);
> +
>  /*
>   * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
>   * any errors.  Intended to be used by 'git reset'.
> @@ -48,4 +59,14 @@ struct replay_insn_list {
>   */
>  void remove_sequencer_state(int aggressive);
>  
> +struct replay_insn_list **replay_insn_list_append(enum replay_action action,
> +						struct commit *operand,
> +						struct replay_insn_list **next);
> +void sequencer_read_todo(struct replay_insn_list **todo_list);
> +void sequencer_read_opts(struct replay_opts **opts_ptr);
> +int sequencer_create_dir(void);
> +void sequencer_save_head(const char *head);
> +void sequencer_save_todo(struct replay_insn_list *todo_list);
> +void sequencer_save_opts(struct replay_opts *opts);

This looks wrong.  What is the expected calling sequence?  Would it
be possible to expose fewer functions by moving more to sequencer.c?

Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> +struct replay_insn_list **replay_insn_list_append(enum replay_action action,
>> +                                             struct commit *operand,
>> +                                             struct replay_insn_list **next);
>> +void sequencer_read_todo(struct replay_insn_list **todo_list);
>> +void sequencer_read_opts(struct replay_opts **opts_ptr);
>> +int sequencer_create_dir(void);
>> +void sequencer_save_head(const char *head);
>> +void sequencer_save_todo(struct replay_insn_list *todo_list);
>> +void sequencer_save_opts(struct replay_opts *opts);
>
> This looks wrong.  What is the expected calling sequence?  Would it
> be possible to expose fewer functions by moving more to sequencer.c?

Thanks for the early review.  Yes, I agree with you: No caller can
make sense of this API; I want something like sequencer_start,
sequencer_handle_conflict, and sequencer_end, but I'm not sure where
to start.  I would have liked to just move these functions without
exposing them, but that would break cherry-pick/ revert.  So, we're
really faced with two choices:
1. Make this patch enormous by moving as well as refactoring
everything into a beautiful public API.  I suspect this won't be easy
to review at all.
2. Keep this patch as it is, and introduce a future patch to clean up
the API.  This is the approach I was going for.

I can't move these functions bit-by-bit either: a hypothetical
sequencer_start will require *everything*.  Could you suggest
something?

-- Ram

Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> So, we're
> really faced with two choices:
> 1. Make this patch enormous by moving as well as refactoring
> everything into a beautiful public API.  I suspect this won't be easy
> to review at all.
> 2. Keep this patch as it is, and introduce a future patch to clean up
> the API.  This is the approach I was going for.

Well, "beautiful public API" means "just what cmd_cherry_pick and
cmd_revert needs", right?  So I'd suggest:

 1. Figuring out what functions they need, and doing the minimal
    refactoring needed to make them separate functions.

 2. As patch #2, moving everything to sequencer.c and exposing those
    functions in sequencer.h.

 3. In later patches, making changes needed for what "git commit"
    needs.

Luckily step (1) is already done.  The functions are parse_args()
and pick_revisions() (though they could presumably use less generic
names).

Hmm?
Jonathan

Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Well, "beautiful public API" means "just what cmd_cherry_pick and
> cmd_revert needs", right?  So I'd suggest:

Yes, but I don't want to put stuff that's too specific to cherry-pick/
revert in the sequencer.

> [...]
> Luckily step (1) is already done.  The functions are parse_args()
> and pick_revisions() (though they could presumably use less generic
> names).

Are you certain about pick_revisions?  I've copied over the function
here for your reference.  My issue is that it's too specific
cherry-pick/ revert:
1. See what walk_revs_populate_todo does: It takes all the operands
and fills up "pick" as the operator.  Why would any other caller want
to do this?
2. You mentioned multiple entry points earlier, and that's something
I've been meaning to do: In the long run, I don't want callers to fill
up an opts structure to specify the subcommand! That'd be butt-ugly.

-- 8< --
static int pick_revisions(struct replay_opts *opts)
{
	struct replay_insn_list *todo_list = NULL;
	unsigned char sha1[20];

	read_and_refresh_cache(opts);

	/*
	 * Decide what to do depending on the arguments; a fresh
	 * cherry-pick should be handled differently from an existing
	 * one that is being continued
	 */
	if (opts->subcommand == REPLAY_RESET) {
		remove_sequencer_state(1);
		return 0;
	} else if (opts->subcommand == REPLAY_CONTINUE) {
		if (!file_exists(git_path(SEQ_TODO_FILE)))
			goto error;
		sequencer_read_opts(&opts);
		sequencer_read_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 (sequencer_create_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"));
		}
		sequencer_save_head(sha1_to_hex(sha1));
		sequencer_save_opts(opts);
	}
	return pick_commits(todo_list, opts);
error:
	return error(_("No %s in progress"), action_name(opts));
}

Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer

[Jonathan Nieder]

Hi Ram,

Ramkumar Ramachandra wrote:

> Are you certain about pick_revisions?  I've copied over the function
> here for your reference.  My issue is that it's too specific
> cherry-pick/ revert:

I'm a very pragmatic person: as long as the code and history are
readable and behave reasonably well, I'm happy.

So in this particular case, why not expose pick_revisions, with some
name like revert_or_cherry_pick?  It would be readable and behave
reasonably well. :)  A theoretical other caller could save a fork
by calling revert_or_cherry_pick instead of forking a subprocess to
do the same.

[...]
> 2. You mentioned multiple entry points earlier, and that's something
> I've been meaning to do: In the long run

Sure, and that still seems like a good idea in the long run.

But as long as we are not closing doors, ideas about the long run
should not get in the way of getting work done today.

Hoping that is clearer.
Jonathan

Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> I'm a very pragmatic person: as long as the code and history are
> readable and behave reasonably well, I'm happy.

Thanks for clarifying; I'll get to work right away.

-- Ram

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

[Ramkumar Ramachandra]

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

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

diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..7ee4269 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,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 
+	/* Remove sequencer state if we just finished the last insn */
+	if (sequencer_count_todo() == 1)
+		remove_sequencer_state(1);
+
 	if (commit_index_files())
 		die (_("Repository has been updated, but unable to write\n"
 		     "new_index file. Check that disk is not full or quota is\n"
diff --git a/builtin/revert.c b/builtin/revert.c
index 491c10a..718ba86 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -581,18 +581,8 @@ static int pick_commits(struct replay_insn_list *todo_list,
 		sequencer_save_todo(cur);
 		opts->action = cur->action;
 		res = do_pick_commit(cur->operand, opts);
-		if (res) {
-			if (!cur->next && res > 0)
-				/*
-				 * A conflict was encountered while
-				 * picking the last commit.  The
-				 * sequencer state is useless now --
-				 * the user simply needs to resolve
-				 * the conflict and commit
-				 */
-				remove_sequencer_state(0);
+		if (res)
 			return res;
-		}
 	}
 
 	/*
diff --git a/sequencer.c b/sequencer.c
index 2993846..0ad1da6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -326,3 +326,17 @@ void sequencer_save_opts(struct replay_opts *opts)
 							opts->xopts[i], "^$", 0);
 	}
 }
+
+int sequencer_count_todo(void)
+{
+	struct replay_insn_list *todo_list = NULL;
+	struct replay_insn_list *cur;
+	int insn_count = 0;
+
+	if (!file_exists(git_path(SEQ_TODO_FILE)))
+		return 0;
+	sequencer_read_todo(&todo_list);
+	for (cur = todo_list; cur; cur = cur->next)
+		insn_count += 1;
+	return insn_count;
+}
diff --git a/sequencer.h b/sequencer.h
index a5b951d..ca133e2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -68,5 +68,6 @@ int sequencer_create_dir(void);
 void sequencer_save_head(const char *head);
 void sequencer_save_todo(struct replay_insn_list *todo_list);
 void sequencer_save_opts(struct replay_opts *opts);
+int sequencer_count_todo(void);
 
 #endif
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index f85372c..832baa0 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