[RFC/PATCH 00/18] WIP implement cherry-pick/revert --continue

[RFC/PATCH 00/18] WIP implement cherry-pick/revert --continue

[Christian Couder]

This a a work in progress to show where this is going and to discuss it.

There are many things missing among other:

 * no documentation
 * missing tests, especially for "revert"
 * when cherry-pick fails the error message does not advertise --continue
 * resolving a failing cherry-pick is not handled
 * commit messages could be improved a lot
 * ...

Many patches in this series are replacing calls to "die()" by
"return error()", because the TODO and DONE files are written
only when cherry-pick fails. This is efficient but perhaps it
would be simpler and safer to write them before each cherry-pick
just in case it fails, so that the "die()" calls don't need to
be removed.

Christian Couder (17):
  advice: add error_resolve_conflict() function
  revert: change many die() calls into "return error()" calls
  usage: implement error_errno() the same way as die_errno()
  revert: don't die when write_message() fails
  commit: move reverse_commit_list() into commit.{h,c}
  revert: remove "commit" global variable
  revert: put option information in an option struct
  revert: refactor code into a new pick_commits() function
  revert: make pick_commits() return an error on --ff incompatible
    option
  revert: make read_and_refresh_cache() and prepare_revs() return
    errors
  revert: add get_todo_content() and create_todo_file()
  revert: write TODO and DONE files in case of failure
  revert: add option parsing for option --continue
  revert: move global variable "me" into "struct args_info"
  revert: add NONE action and make parse_args() manage it
  revert: add remaining instructions in todo file
  revert: implement --continue processing

Stephan Beyer (1):
  revert: implement parsing TODO and DONE files

 advice.c                            |   25 +-
 advice.h                            |    1 +
 builtin/revert.c                    |  692 ++++++++++++++++++++++++++++-------
 commit.c                            |   11 +
 commit.h                            |    2 +
 git-compat-util.h                   |    1 +
 merge-recursive.c                   |   11 -
 t/t3508-cherry-pick-many-commits.sh |  101 +++++
 usage.c                             |   28 ++-
 9 files changed, 717 insertions(+), 155 deletions(-)

-- 
1.7.3.2.504.g59d466

[RFC/PATCH 01/18] advice: add error_resolve_conflict() function

[Christian Couder]

in case we don't want to die, but still want the right
error message to be printed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 advice.c |   25 +++++++++++++++++++++----
 advice.h |    1 +
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/advice.c b/advice.c
index 0be4b5f..d4ba29f 100644
--- a/advice.c
+++ b/advice.c
@@ -34,6 +34,13 @@ int git_default_advice_config(const char *var, const char *value)
 	return 0;
 }
 
+const char unmerged_file_advice[] =
+	"'%s' is not possible because you have unmerged files.\n"
+	"Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
+	"appropriate to mark resolution and make a commit, or use 'git commit -a'.";
+const char unmerged_file_no_advice[] =
+	"'%s' is not possible because you have unmerged files.";
+
 void NORETURN die_resolve_conflict(const char *me)
 {
 	if (advice_resolve_conflict)
@@ -41,9 +48,19 @@ void NORETURN die_resolve_conflict(const char *me)
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
 		 */
-		die("'%s' is not possible because you have unmerged files.\n"
-		    "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
-		    "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
+		die(unmerged_file_advice, me);
+	else
+		die(unmerged_file_no_advice, me);
+}
+
+int error_resolve_conflict(const char *me)
+{
+	if (advice_resolve_conflict)
+		/*
+		 * Message used both when 'git commit' fails and when
+		 * other commands doing a merge do.
+		 */
+		return error(unmerged_file_advice, me);
 	else
-		die("'%s' is not possible because you have unmerged files.", me);
+		return error(unmerged_file_no_advice, me);
 }
diff --git a/advice.h b/advice.h
index 3244ebb..7b7cea5 100644
--- a/advice.h
+++ b/advice.h
@@ -13,5 +13,6 @@ extern int advice_detached_head;
 int git_default_advice_config(const char *var, const char *value);
 
 extern void NORETURN die_resolve_conflict(const char *me);
+extern int error_resolve_conflict(const char *me);
 
 #endif /* ADVICE_H */
-- 
1.7.3.2.504.g59d466

Re: [RFC/PATCH 01/18] advice: add error_resolve_conflict() function

[Jonathan Nieder]

Christian Couder wrote:

> --- a/advice.c
> +++ b/advice.c
> @@ -34,6 +34,13 @@ int git_default_advice_config(const char *var, const char *value)
>  	return 0;
>  }
>  
> +const char unmerged_file_advice[] =
> +	"'%s' is not possible because you have unmerged files.\n"
> +	"Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
> +	"appropriate to mark resolution and make a commit, or use 'git commit -a'.";
> +const char unmerged_file_no_advice[] =
> +	"'%s' is not possible because you have unmerged files.";

static?

> --- a/advice.h
> +++ b/advice.h
> @@ -13,5 +13,6 @@ extern int advice_detached_head;
>  int git_default_advice_config(const char *var, const char *value);
>  
>  extern void NORETURN die_resolve_conflict(const char *me);
> +extern int error_resolve_conflict(const char *me);

I like it.

[RFC/PATCH 02/18] revert: change many die() calls into "return error()" calls

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   43 ++++++++++++++++++++++---------------------
 1 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index bb6e9e8..9649d37 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -280,16 +280,18 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static NORETURN void die_dirty_index(const char *me)
+static int error_dirty_index(const char *me)
 {
 	if (read_cache_unmerged()) {
-		die_resolve_conflict(me);
+		return error_resolve_conflict(me);
 	} else {
 		if (advice_commit_before_merge)
-			die("Your local changes would be overwritten by %s.\n"
-			    "Please, commit your changes or stash them to proceed.", me);
+			return error("Your local changes would be overwritten "
+				     "by %s.\nPlease, commit your changes or "
+				     "stash them to proceed.", me);
 		else
-			die("Your local changes would be overwritten by %s.\n", me);
+			return error("Your local changes would be overwritten "
+				     "by %s.\n", me);
 	}
 }
 
@@ -339,7 +341,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	if (active_cache_changed &&
 	    (write_cache(index_fd, active_cache, active_nr) ||
 	     commit_locked_index(&index_lock)))
-		die("%s: Unable to write new index file", me);
+		return error("%s: Unable to write new index file", me);
 	rollback_lock_file(&index_lock);
 
 	if (!clean) {
@@ -405,18 +407,18 @@ static int do_pick_commit(void)
 		 * to work on.
 		 */
 		if (write_cache_as_tree(head, 0, NULL))
-			die ("Your index file is unmerged.");
+			return error("Your index file is unmerged.");
 	} else {
 		if (get_sha1("HEAD", head))
-			die ("You do not have a valid HEAD");
+			return error("You do not have a valid HEAD");
 		if (index_differs_from("HEAD", 0))
-			die_dirty_index(me);
+			return error_dirty_index(me);
 	}
 	discard_cache();
 
 	if (!commit->parents) {
 		if (action == REVERT)
-			die ("Cannot revert a root commit");
+			return error("Cannot revert a root commit");
 		parent = NULL;
 	}
 	else if (commit->parents->next) {
@@ -425,20 +427,19 @@ static int do_pick_commit(void)
 		struct commit_list *p;
 
 		if (!mainline)
-			die("Commit %s is a merge but no -m option was given.",
-			    sha1_to_hex(commit->object.sha1));
-
+			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 != mainline && p;
 		     cnt++)
 			p = p->next;
 		if (cnt != mainline || !p)
-			die("Commit %s does not have parent %d",
-			    sha1_to_hex(commit->object.sha1), mainline);
+			return error("Commit %s does not have parent %d",
+				     sha1_to_hex(commit->object.sha1), mainline);
 		parent = p->item;
 	} else if (0 < mainline)
-		die("Mainline was specified but commit %s is not a merge.",
-		    sha1_to_hex(commit->object.sha1));
+		return error("Mainline was specified but commit %s is not a merge.",
+			     sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
@@ -446,12 +447,12 @@ static int do_pick_commit(void)
 		return fast_forward_to(commit->object.sha1, head);
 
 	if (parent && parse_commit(parent) < 0)
-		die("%s: cannot parse parent commit %s",
-		    me, sha1_to_hex(parent->object.sha1));
+		return error("%s: cannot parse parent commit %s",
+			     me, sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit->buffer, &msg) != 0)
-		die("Cannot get commit message for %s",
-				sha1_to_hex(commit->object.sha1));
+		return error("Cannot get commit message for %s",
+			     sha1_to_hex(commit->object.sha1));
 
 	/*
 	 * "commit" is an existing commit.  We would want to apply
-- 
1.7.3.2.504.g59d466

Re: [RFC/PATCH 02/18] revert: change many die() calls into "return error()" calls

[Jonathan Nieder]

Christian Couder wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -280,16 +280,18 @@ static struct tree *empty_tree(void)
>  	return tree;
>  }
>  
> -static NORETURN void die_dirty_index(const char *me)
> +static int error_dirty_index(const char *me)

In general sounds to me like a good thing to do.  But for your use
case (writing out TODO and DONE files when cherry-pick fails),
wouldn't a set_die_routine() also work?

I am tempted to suggest a series in the following order:

 1. set die routine with the desired behavior
 2. change die() calls to return error() so the nice stack unwinding
    takes place automatically (this should help with other libification
    work, anyway)

then maybe:

 3. add an assert(0) to die routine (perhaps protected by a compile-time
    option) so missing die() calls can be noticed
 4. remove the die routine once it is clear all problematic die calls
    have been eliminated.

... but wait: would all such die calls ever be eliminated?  xmalloc,
xmkstemp, and similar functions are perhaps too convenient to avoid.
So it might be simpler to stick to (1) and treat (2) as a separate
topic.

[RFC/PATCH 03/18] usage: implement error_errno() the same way as die_errno()

[Christian Couder]

die_errno() is very useful, but sometimes we don't want to
die after printing an error message and the error message
from errno. So let's implement error_errno() that does the
same thing as die_errno() except that it calls
error_routine() instead of die_routine().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-compat-util.h |    1 +
 usage.c           |   28 ++++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 490f969..32294cc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -230,6 +230,7 @@ extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
diff --git a/usage.c b/usage.c
index ec4cf53..f3ac869 100644
--- a/usage.c
+++ b/usage.c
@@ -69,10 +69,9 @@ void die(const char *err, ...)
 	va_end(params);
 }
 
-void die_errno(const char *fmt, ...)
+static void format_errno(char *fmt_with_err, size_t fmt_with_err_size,
+			 const char *fmt)
 {
-	va_list params;
-	char fmt_with_err[1024];
 	char str_error[256], *err;
 	int i, j;
 
@@ -90,13 +89,34 @@ void die_errno(const char *fmt, ...)
 		}
 	}
 	str_error[j] = 0;
-	snprintf(fmt_with_err, sizeof(fmt_with_err), "%s: %s", fmt, str_error);
+	snprintf(fmt_with_err, fmt_with_err_size, "%s: %s", fmt, str_error);
+}
+
+void die_errno(const char *fmt, ...)
+{
+	va_list params;
+	char fmt_with_err[1024];
+
+	format_errno(fmt_with_err, sizeof(fmt_with_err), fmt);
 
 	va_start(params, fmt);
 	die_routine(fmt_with_err, params);
 	va_end(params);
 }
 
+int error_errno(const char *fmt, ...)
+{
+	va_list params;
+	char fmt_with_err[1024];
+
+	format_errno(fmt_with_err, sizeof(fmt_with_err), fmt);
+
+	va_start(params, fmt);
+	error_routine(fmt_with_err, params);
+	va_end(params);
+	return -1;
+}
+
 int error(const char *err, ...)
 {
 	va_list params;
-- 
1.7.3.2.504.g59d466

Re: [RFC/PATCH 03/18] usage: implement error_errno() the same way as die_errno()

[Jonathan Nieder]

Christian Couder wrote:

> die_errno() is very useful, but sometimes we don't want to
> die after printing an error message and the error message
> from errno.

Yes, please!  I have often wished for this helper.

Re: [RFC/PATCH 03/18] usage: implement error_errno() the same way as die_errno()

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> die_errno() is very useful, but sometimes we don't want to
> die after printing an error message and the error message
> from errno. So let's implement error_errno() that does the
> same thing as die_errno() except that it calls
> error_routine() instead of die_routine().

If this were "error_errno() is to error() as die_errno() is to die(); iow,
error_errno() does the same thing as error() but in addition shows the
errno information", it might be a good thing, but the above makes it sound
like you did something different.

[RFC/PATCH 04/18] revert: don't die when write_message() fails

[Christian Couder]

Instead we will just return an error code.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 9649d37..947e666 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -257,17 +257,19 @@ static void print_advice(void)
 		       find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
 }
 
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int 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);
+		return error_errno("Could not write to %s.", filename);
 	strbuf_release(msgbuf);
 	if (commit_lock_file(&msg_file) < 0)
-		die("Error wrapping up %s", filename);
+		return error("Error wrapping up %s", filename);
+
+	return 0;
 }
 
 static struct tree *empty_tree(void)
@@ -397,7 +399,7 @@ static int do_pick_commit(void)
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
-	int res;
+	int res, write_res;
 
 	if (no_commit) {
 		/*
@@ -495,12 +497,16 @@ static int do_pick_commit(void)
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf);
-		write_message(&msgbuf, defmsg);
+		write_res = write_message(&msgbuf, defmsg);
+		if (write_res)
+			return write_res;
 	} else {
 		struct commit_list *common = NULL;
 		struct commit_list *remotes = NULL;
 
-		write_message(&msgbuf, defmsg);
+		write_res = write_message(&msgbuf, defmsg);
+		if (write_res)
+			return write_res;
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 05/18] commit: move reverse_commit_list() into commit.{h, c}

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 commit.c          |   11 +++++++++++
 commit.h          |    2 ++
 merge-recursive.c |   11 -----------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/commit.c b/commit.c
index 0094ec1..f9d5bf9 100644
--- a/commit.c
+++ b/commit.c
@@ -351,6 +351,17 @@ unsigned commit_list_count(const struct commit_list *l)
 	return c;
 }
 
+struct commit_list *reverse_commit_list(struct commit_list *list)
+{
+	struct commit_list *next = NULL, *current, *backup;
+	for (current = list; current; current = backup) {
+		backup = current->next;
+		current->next = next;
+		next = current;
+	}
+	return next;
+}
+
 void free_commit_list(struct commit_list *list)
 {
 	while (list) {
diff --git a/commit.h b/commit.h
index 9113bbe..6a074a1 100644
--- a/commit.h
+++ b/commit.h
@@ -48,6 +48,8 @@ struct commit_list * commit_list_insert(struct commit *item, struct commit_list
 unsigned commit_list_count(const struct commit_list *l);
 struct commit_list * insert_by_date(struct commit *item, struct commit_list **list);
 
+struct commit_list *reverse_commit_list(struct commit_list *list);
+
 void free_commit_list(struct commit_list *list);
 
 void sort_by_date(struct commit_list **list);
diff --git a/merge-recursive.c b/merge-recursive.c
index 16c2dbe..d02fda6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1573,17 +1573,6 @@ int merge_trees(struct merge_options *o,
 	return clean;
 }
 
-static struct commit_list *reverse_commit_list(struct commit_list *list)
-{
-	struct commit_list *next = NULL, *current, *backup;
-	for (current = list; current; current = backup) {
-		backup = current->next;
-		current->next = next;
-		next = current;
-	}
-	return next;
-}
-
 /*
  * Merge the commits h1 and h2, return the resulting virtual
  * commit object and a flag indicating the cleanness of the merge.
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 06/18] revert: remove "commit" global variable

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   49 ++++++++++++++++++++++++-------------------------
 1 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 947e666..e3dea19 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -38,7 +38,6 @@ static const char * const cherry_pick_usage[] = {
 
 static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
-static struct commit *commit;
 static int commit_argc;
 static const char **commit_argv;
 static int allow_rerere_auto;
@@ -48,7 +47,7 @@ static const char *strategy;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
-static char *get_encoding(const char *message);
+static char *get_encoding(const char *message, const unsigned char *sha1);
 
 static const char * const *revert_or_cherry_pick_usage(void)
 {
@@ -99,7 +98,8 @@ struct commit_message {
 	const char *message;
 };
 
-static int get_message(const char *raw_message, struct commit_message *out)
+static int get_message(const char *raw_message, const unsigned char *sha1,
+		       struct commit_message *out)
 {
 	const char *encoding;
 	const char *abbrev, *subject;
@@ -108,7 +108,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
 
 	if (!raw_message)
 		return -1;
-	encoding = get_encoding(raw_message);
+	encoding = get_encoding(raw_message, sha1);
 	if (!encoding)
 		encoding = "UTF-8";
 	if (!git_commit_encoding)
@@ -122,7 +122,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
 	if (out->reencoded_message)
 		out->message = out->reencoded_message;
 
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+	abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
 	abbrev_len = strlen(abbrev);
 
 	subject_len = find_commit_subject(out->message, &subject);
@@ -146,13 +146,12 @@ static void free_message(struct commit_message *msg)
 	free(msg->reencoded_message);
 }
 
-static char *get_encoding(const char *message)
+static char *get_encoding(const char *message, const unsigned char *sha1)
 {
 	const char *p = message, *eol;
 
 	if (!p)
-		die ("Could not read commit message of %s",
-				sha1_to_hex(commit->object.sha1));
+		die ("Could not read commit message of %s", sha1_to_hex(sha1));
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
@@ -168,25 +167,25 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
+static void add_message_to_msg(struct strbuf *msgbuf, const char *message,
+			       const unsigned char *sha1)
 {
 	const char *p = message;
 	while (*p && (*p != '\n' || p[1] != '\n'))
 		p++;
 
 	if (!*p)
-		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
+		strbuf_addstr(msgbuf, sha1_to_hex(sha1));
 
 	p += 2;
 	strbuf_addstr(msgbuf, p);
 }
 
-static void set_author_ident_env(const char *message)
+static void set_author_ident_env(const char *message, const unsigned char *sha1)
 {
 	const char *p = message;
 	if (!p)
-		die ("Could not read commit message of %s",
-				sha1_to_hex(commit->object.sha1));
+		die ("Could not read commit message of %s", sha1_to_hex(sha1));
 	while (*p && *p != '\n') {
 		const char *eol;
 
@@ -200,7 +199,7 @@ static void set_author_ident_env(const char *message)
 			email = strchr(line, '<');
 			if (!email)
 				die ("Could not extract author email from %s",
-					sha1_to_hex(commit->object.sha1));
+					sha1_to_hex(sha1));
 			if (email == line)
 				pend = line;
 			else
@@ -212,7 +211,7 @@ static void set_author_ident_env(const char *message)
 			timestamp = strchr(email, '>');
 			if (!timestamp)
 				die ("Could not extract author time from %s",
-					sha1_to_hex(commit->object.sha1));
+					sha1_to_hex(sha1));
 			*timestamp = '\0';
 			for (timestamp++; *timestamp && isspace(*timestamp);
 					timestamp++)
@@ -227,8 +226,7 @@ static void set_author_ident_env(const char *message)
 		if (*p == '\n')
 			p++;
 	}
-	die ("No author information found in %s",
-			sha1_to_hex(commit->object.sha1));
+	die ("No author information found in %s", sha1_to_hex(sha1));
 }
 
 static void advise(const char *advice, ...)
@@ -240,7 +238,7 @@ static void advise(const char *advice, ...)
 	va_end(params);
 }
 
-static void print_advice(void)
+static void print_advice(const unsigned char *sha1)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
@@ -254,7 +252,7 @@ static void print_advice(void)
 
 	if (action == CHERRY_PICK)
 		advise("and commit the result with 'git commit -c %s'",
-		       find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
+		       find_unique_abbrev(sha1, DEFAULT_ABBREV));
 }
 
 static int write_message(struct strbuf *msgbuf, const char *filename)
@@ -391,7 +389,7 @@ static int run_git_commit(const char *defmsg)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(void)
+static int do_pick_commit(struct commit *commit)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -452,7 +450,7 @@ static int do_pick_commit(void)
 		return error("%s: cannot parse parent commit %s",
 			     me, sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit->buffer, &msg) != 0)
+	if (get_message(commit->buffer, commit->object.sha1, &msg) != 0)
 		return error("Cannot get commit message for %s",
 			     sha1_to_hex(commit->object.sha1));
 
@@ -485,8 +483,8 @@ static int do_pick_commit(void)
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
-		set_author_ident_env(msg.message);
-		add_message_to_msg(&msgbuf, msg.message);
+		set_author_ident_env(msg.message, commit->object.sha1);
+		add_message_to_msg(&msgbuf, msg.message, commit->object.sha1);
 		if (no_replay) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
@@ -521,7 +519,7 @@ static int do_pick_commit(void)
 		      action == REVERT ? "revert" : "apply",
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 		      msg.subject);
-		print_advice();
+		print_advice(commit->object.sha1);
 		rerere(allow_rerere_auto);
 	} else {
 		if (!no_commit)
@@ -572,6 +570,7 @@ static void read_and_refresh_cache(const char *me)
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	struct rev_info revs;
+	struct commit *commit;
 
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -594,7 +593,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	prepare_revs(&revs);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit();
+		int res = do_pick_commit(commit);
 		if (res)
 			return res;
 	}
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 07/18] revert: put option information in an option struct

[Christian Couder]

This is needed because we want to reuse the parse_args() function
so that we can parse options saved in a TODO file.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |  148 +++++++++++++++++++++++++++++-------------------------
 1 files changed, 79 insertions(+), 69 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e3dea19..443b529 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -36,58 +36,68 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
-static enum { REVERT, CHERRY_PICK } action;
-static int commit_argc;
-static const char **commit_argv;
-static int allow_rerere_auto;
-
 static const char *me;
-static const char *strategy;
+
+struct args_info {
+	enum { REVERT, CHERRY_PICK } action;
+	int edit;
+	int no_replay;
+	int no_commit;
+	int mainline;
+	int signoff;
+	int allow_ff;
+	int allow_rerere_auto;
+	const char *strategy;
+	const char **commit_argv;
+	int commit_argc;
+	const char * const * usage_str;
+};
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static char *get_encoding(const char *message, const unsigned char *sha1);
 
-static const char * const *revert_or_cherry_pick_usage(void)
+static const char * const *revert_or_cherry_pick_usage(struct args_info *info)
 {
-	return action == REVERT ? revert_usage : cherry_pick_usage;
+	return info->action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
-static void parse_args(int argc, const char **argv)
+static void parse_args(int argc, const char **argv, struct args_info *info)
 {
-	const char * const * usage_str = revert_or_cherry_pick_usage();
 	int noop;
 	struct option options[] = {
-		OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"),
-		OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"),
+		OPT_BOOLEAN('n', "no-commit", &info->no_commit,
+			    "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &info->edit, "edit the commit message"),
 		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
-		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
-		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
-		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
-		OPT_STRING(0, "strategy", &strategy, "strategy", "merge strategy"),
+		OPT_BOOLEAN('s', "signoff", &info->signoff, "add Signed-off-by:"),
+		OPT_INTEGER('m', "mainline", &info->mainline, "parent number"),
+		OPT_RERERE_AUTOUPDATE(&info->allow_rerere_auto),
+		OPT_STRING(0, "strategy", &info->strategy, "strategy",
+			   "merge strategy"),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
 	};
 
-	if (action == CHERRY_PICK) {
+	if (info->action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &no_replay, "append commit name"),
-			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+			OPT_BOOLEAN('x', NULL, &info->no_replay, "append commit name"),
+			OPT_BOOLEAN(0, "ff", &info->allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
 			die("program error");
 	}
 
-	commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-				    PARSE_OPT_KEEP_ARGV0 |
-				    PARSE_OPT_KEEP_UNKNOWN);
-	if (commit_argc < 2)
-		usage_with_options(usage_str, options);
+	info->usage_str = revert_or_cherry_pick_usage(info);
+	info->commit_argc = parse_options(argc, argv, NULL, options, info->usage_str,
+					  PARSE_OPT_KEEP_ARGV0 |
+					  PARSE_OPT_KEEP_UNKNOWN);
+	info->commit_argv = argv;
 
-	commit_argv = argv;
+	if (info->commit_argc < 2)
+		usage_with_options(info->usage_str, options);
 }
 
 struct commit_message {
@@ -238,7 +248,7 @@ static void advise(const char *advice, ...)
 	va_end(params);
 }
 
-static void print_advice(const unsigned char *sha1)
+static void print_advice(struct args_info *info, const unsigned char *sha1)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
@@ -250,7 +260,7 @@ static void print_advice(const unsigned char *sha1)
 	advise("after resolving the conflicts, mark the corrected paths");
 	advise("with 'git add <paths>' or 'git rm <paths>'");
 
-	if (action == CHERRY_PICK)
+	if (info->action == CHERRY_PICK)
 		advise("and commit the result with 'git commit -c %s'",
 		       find_unique_abbrev(sha1, DEFAULT_ABBREV));
 }
@@ -370,7 +380,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
  * 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)
+static int run_git_commit(const char *defmsg, struct args_info *info)
 {
 	/* 6 is max possible length of our args array including NULL */
 	const char *args[6];
@@ -378,9 +388,9 @@ static int run_git_commit(const char *defmsg)
 
 	args[i++] = "commit";
 	args[i++] = "-n";
-	if (signoff)
+	if (info->signoff)
 		args[i++] = "-s";
-	if (!edit) {
+	if (!info->edit) {
 		args[i++] = "-F";
 		args[i++] = defmsg;
 	}
@@ -389,7 +399,7 @@ static int run_git_commit(const char *defmsg)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit)
+static int do_pick_commit(struct args_info *info, struct commit *commit)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -399,7 +409,7 @@ static int do_pick_commit(struct commit *commit)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res, write_res;
 
-	if (no_commit) {
+	if (info->no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
 		 * merge the differences in, so let's compute the tree
@@ -417,7 +427,7 @@ static int do_pick_commit(struct commit *commit)
 	discard_cache();
 
 	if (!commit->parents) {
-		if (action == REVERT)
+		if (info->action == REVERT)
 			return error("Cannot revert a root commit");
 		parent = NULL;
 	}
@@ -426,24 +436,24 @@ static int do_pick_commit(struct commit *commit)
 		int cnt;
 		struct commit_list *p;
 
-		if (!mainline)
+		if (!info->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 != mainline && p;
+		     cnt != info->mainline && p;
 		     cnt++)
 			p = p->next;
-		if (cnt != mainline || !p)
+		if (cnt != info->mainline || !p)
 			return error("Commit %s does not have parent %d",
-				     sha1_to_hex(commit->object.sha1), mainline);
+				     sha1_to_hex(commit->object.sha1), info->mainline);
 		parent = p->item;
-	} else if (0 < mainline)
+	} else if (0 < info->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 (allow_ff && parent && !hashcmp(parent->object.sha1, head))
+	if (info->allow_ff && parent && !hashcmp(parent->object.sha1, head))
 		return fast_forward_to(commit->object.sha1, head);
 
 	if (parent && parse_commit(parent) < 0)
@@ -463,7 +473,7 @@ static int do_pick_commit(struct commit *commit)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (action == REVERT) {
+	if (info->action == REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -485,14 +495,15 @@ static int do_pick_commit(struct commit *commit)
 		next_label = msg.label;
 		set_author_ident_env(msg.message, commit->object.sha1);
 		add_message_to_msg(&msgbuf, msg.message, commit->object.sha1);
-		if (no_replay) {
+		if (info->no_replay) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
 	}
 
-	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+	if (!info->strategy || !strcmp(info->strategy, "recursive") ||
+	    info->action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf);
 		write_res = write_message(&msgbuf, defmsg);
@@ -508,7 +519,7 @@ static int do_pick_commit(struct commit *commit)
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res = try_merge_command(strategy, common,
+		res = try_merge_command(info->strategy, common,
 					sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
@@ -516,14 +527,14 @@ static int do_pick_commit(struct commit *commit)
 
 	if (res) {
 		error("could not %s %s... %s",
-		      action == REVERT ? "revert" : "apply",
+		      info->action == REVERT ? "revert" : "apply",
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 		      msg.subject);
-		print_advice(commit->object.sha1);
-		rerere(allow_rerere_auto);
+		print_advice(info, commit->object.sha1);
+		rerere(info->allow_rerere_auto);
 	} else {
-		if (!no_commit)
-			res = run_git_commit(defmsg);
+		if (!info->no_commit)
+			res = run_git_commit(defmsg, info);
 	}
 
 	free_message(&msg);
@@ -532,18 +543,18 @@ static int do_pick_commit(struct commit *commit)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs)
+static void prepare_revs(struct rev_info *revs, struct args_info *info)
 {
 	int argc;
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (action != REVERT)
+	if (info->action != REVERT)
 		revs->reverse = 1;
 
-	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
+	argc = setup_revisions(info->commit_argc, info->commit_argv, revs, NULL);
 	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage());
+		usage(*revert_or_cherry_pick_usage(info));
 
 	if (prepare_revision_walk(revs))
 		die("revision walk setup failed");
@@ -567,33 +578,36 @@ static void read_and_refresh_cache(const char *me)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv, int revert, int edit)
 {
+	struct args_info infos;
 	struct rev_info revs;
 	struct commit *commit;
 
+	memset(&infos, 0, sizeof(infos));
 	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
+	infos.action = revert ? REVERT : CHERRY_PICK;
+	me = revert ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
+	parse_args(argc, argv, &infos);
 
-	if (allow_ff) {
-		if (signoff)
+	if (infos.allow_ff) {
+		if (infos.signoff)
 			die("cherry-pick --ff cannot be used with --signoff");
-		if (no_commit)
+		if (infos.no_commit)
 			die("cherry-pick --ff cannot be used with --no-commit");
-		if (no_replay)
+		if (infos.no_replay)
 			die("cherry-pick --ff cannot be used with -x");
-		if (edit)
+		if (infos.edit)
 			die("cherry-pick --ff cannot be used with --edit");
 	}
 
 	read_and_refresh_cache(me);
 
-	prepare_revs(&revs);
+	prepare_revs(&revs, &infos);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit);
+		int res = do_pick_commit(&infos, commit);
 		if (res)
 			return res;
 	}
@@ -603,14 +617,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
-	if (isatty(0))
-		edit = 1;
-	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	return revert_or_cherry_pick(argc, argv, 1, isatty(0));
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-	action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	return revert_or_cherry_pick(argc, argv, 0, 0);
 }
-- 
1.7.3.2.504.g59d466

Re: [RFC/PATCH 07/18] revert: put option information in an option struct

[Jonathan Nieder]

Christian Couder wrote:

> This is needed because we want to reuse the parse_args() function
> so that we can parse options saved in a TODO file.

Why couldn't parse_args() write to the globals?  I would be more
convinced by an explanation like

	This helps attain greater sanity by being explicit
	about which functions depend on the parameters passed
	to cherry-pick.

Re: [RFC/PATCH 07/18] revert: put option information in an option struct

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> This is needed because we want to reuse the parse_args() function
> so that we can parse options saved in a TODO file.

This probably is not _needed_ but something you thought would be a
good thing to do while at it.

You forgot to mention something more important, though.  You added two
extra arguments to revert_or_cherry_pick, neither of which I agree with.

 * it is a regression to call the first extra argument "int revert";
   (action == REVERT) was more readable.

 * "int edit" is ill thought out; it is about giving the default of "edit"
   to revert_or_cherry_pick() depending on what action it is going to
   take.  In this particular case, the logic for the default of "edit" is
   trivial and localized (it is 0 unless we are interactive revert), so I
   would drop the argument and have default logic immediately after
   "memset(&info, 0, sizeof(info))".

   If there were many such args-info elements whose default have to be
   different depending on the action, the caller should be passing an
   instance of "struct args_info" with the default, and have the parser
   to update the default supplied.  I don't think it is warranted in this
   case.

By the way, "infos" is an eyesore at least to me.  Any data you work on is
information, naming a variable "struct args_info info", unless it
primarily works on that "args_info" and not any other kinds of info (like
"struct rev_info revs"), is like calling a variable "var", adding _no_
useful information.

[RFC/PATCH 08/18] revert: refactor code into a new pick_commits() function

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   38 ++++++++++++++++++++++----------------
 1 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 443b529..1f20251 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -578,36 +578,28 @@ static void read_and_refresh_cache(const char *me)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv, int revert, int edit)
+static int pick_commits(struct args_info *infos)
 {
-	struct args_info infos;
 	struct rev_info revs;
 	struct commit *commit;
 
-	memset(&infos, 0, sizeof(infos));
-	git_config(git_default_config, NULL);
-	infos.action = revert ? REVERT : CHERRY_PICK;
-	me = revert ? "revert" : "cherry-pick";
-	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv, &infos);
-
-	if (infos.allow_ff) {
-		if (infos.signoff)
+	if (infos->allow_ff) {
+		if (infos->signoff)
 			die("cherry-pick --ff cannot be used with --signoff");
-		if (infos.no_commit)
+		if (infos->no_commit)
 			die("cherry-pick --ff cannot be used with --no-commit");
-		if (infos.no_replay)
+		if (infos->no_replay)
 			die("cherry-pick --ff cannot be used with -x");
-		if (infos.edit)
+		if (infos->edit)
 			die("cherry-pick --ff cannot be used with --edit");
 	}
 
 	read_and_refresh_cache(me);
 
-	prepare_revs(&revs, &infos);
+	prepare_revs(&revs, infos);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(&infos, commit);
+		int res = do_pick_commit(infos, commit);
 		if (res)
 			return res;
 	}
@@ -615,6 +607,20 @@ static int revert_or_cherry_pick(int argc, const char **argv, int revert, int ed
 	return 0;
 }
 
+static int revert_or_cherry_pick(int argc, const char **argv, int revert, int edit)
+{
+	struct args_info infos;
+
+	git_config(git_default_config, NULL);
+	me = revert ? "revert" : "cherry-pick";
+	setenv(GIT_REFLOG_ACTION, me, 0);
+	memset(&infos, 0, sizeof(infos));
+	infos.action = revert ? REVERT : CHERRY_PICK;
+	parse_args(argc, argv, &infos);
+
+	return pick_commits(&infos);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	return revert_or_cherry_pick(argc, argv, 1, isatty(0));
-- 
1.7.3.2.504.g59d466

Re: [RFC/PATCH 08/18] revert: refactor code into a new pick_commits() function

[Daniel Barkalow]

On Thu, 25 Nov 2010, Christian Couder wrote:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/revert.c |   38 ++++++++++++++++++++++----------------
>  1 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 443b529..1f20251 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -578,36 +578,28 @@ static void read_and_refresh_cache(const char *me)
>  	rollback_lock_file(&index_lock);
>  }
>  
> -static int revert_or_cherry_pick(int argc, const char **argv, int revert, int edit)
> +static int pick_commits(struct args_info *infos)
>  {
> -	struct args_info infos;
>  	struct rev_info revs;
>  	struct commit *commit;
>  
> -	memset(&infos, 0, sizeof(infos));
> -	git_config(git_default_config, NULL);
> -	infos.action = revert ? REVERT : CHERRY_PICK;
> -	me = revert ? "revert" : "cherry-pick";
> -	setenv(GIT_REFLOG_ACTION, me, 0);
> -	parse_args(argc, argv, &infos);
> -
> -	if (infos.allow_ff) {
> -		if (infos.signoff)
> +	if (infos->allow_ff) {
> +		if (infos->signoff)
>  			die("cherry-pick --ff cannot be used with --signoff");
> -		if (infos.no_commit)
> +		if (infos->no_commit)
>  			die("cherry-pick --ff cannot be used with --no-commit");
> -		if (infos.no_replay)
> +		if (infos->no_replay)
>  			die("cherry-pick --ff cannot be used with -x");
> -		if (infos.edit)
> +		if (infos->edit)
>  			die("cherry-pick --ff cannot be used with --edit");
>  	}
>  
>  	read_and_refresh_cache(me);
>  
> -	prepare_revs(&revs, &infos);
> +	prepare_revs(&revs, infos);
>  
>  	while ((commit = get_revision(&revs))) {
> -		int res = do_pick_commit(&infos, commit);
> +		int res = do_pick_commit(infos, commit);
>  		if (res)
>  			return res;
>  	}
> @@ -615,6 +607,20 @@ static int revert_or_cherry_pick(int argc, const char **argv, int revert, int ed
>  	return 0;
>  }
>  
> +static int revert_or_cherry_pick(int argc, const char **argv, int revert, int edit)
> +{
> +	struct args_info infos;
> +
> +	git_config(git_default_config, NULL);
> +	me = revert ? "revert" : "cherry-pick";
> +	setenv(GIT_REFLOG_ACTION, me, 0);
> +	memset(&infos, 0, sizeof(infos));
> +	infos.action = revert ? REVERT : CHERRY_PICK;
> +	parse_args(argc, argv, &infos);
> +
> +	return pick_commits(&infos);
> +}
> +

I think it would be more obvious to put this into cmd_revert and 
cmd_cherry_pick, and have them call pick_commits directly. In fact, you 
could probably make things more clear by calling your "struct args_info" 
instead "struct pick_commits_args" (like a lot of other 
"struct {cmd}_args" we already have for similar situations).

While there's no reason to do it here, pick_commits() is a sensible 
operation that other builtins might want to call, particularly with the 
error return instead of die(), so it would be nice to name things suitably 
for that usage. That also avoids Junio's objection to the arguments to 
revert_or_cherry_pick() by not having the function with the objectionable 
arguments at all.

For that matter, you have a lot of commits in this series that put globals 
into a struct and pass the struct around and change the arguments to the 
functions that actually do things. I think it would be easier to 
understand if you squashed all of these together into a single commit, 
which does all of the necessary changes to function prototypes. And I 
think it would be similarly better to have a single commit that makes all 
of the places that call die() not do that, rather than getting some of 
them in each of several patches.

>  int cmd_revert(int argc, const char **argv, const char *prefix)
>  {
>  	return revert_or_cherry_pick(argc, argv, 1, isatty(0));
> -- 
> 1.7.3.2.504.g59d466
> 
> 
> 

[RFC/PATCH 09/18] revert: make pick_commits() return an error on --ff incompatible option

[Christian Couder]

As we want to use pick_commits() many times and write TODO and DONE
file in case of errors, we must not die in case of error inside
pick_commits() but return an error.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1f20251..57d4300 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -578,33 +578,33 @@ static void read_and_refresh_cache(const char *me)
 	rollback_lock_file(&index_lock);
 }
 
+static int ff_incompatible(int val, const char *opt)
+{
+	return val ? error("cherry-pick --ff cannot be used with %s", opt) : 0;
+}
+
 static int pick_commits(struct args_info *infos)
 {
 	struct rev_info revs;
 	struct commit *commit;
+	int res = 0;
 
-	if (infos->allow_ff) {
-		if (infos->signoff)
-			die("cherry-pick --ff cannot be used with --signoff");
-		if (infos->no_commit)
-			die("cherry-pick --ff cannot be used with --no-commit");
-		if (infos->no_replay)
-			die("cherry-pick --ff cannot be used with -x");
-		if (infos->edit)
-			die("cherry-pick --ff cannot be used with --edit");
-	}
+	if (infos->allow_ff &&
+	    ((res = ff_incompatible(infos->signoff, "--signoff")) ||
+	     (res = ff_incompatible(infos->no_commit, "--no_commit")) ||
+	     (res = ff_incompatible(infos->no_replay, "-x")) ||
+	     (res = ff_incompatible(infos->edit, "--edit"))))
+			return res;
 
 	read_and_refresh_cache(me);
 
 	prepare_revs(&revs, infos);
 
-	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(infos, commit);
-		if (res)
-			return res;
-	}
+	while ((commit = get_revision(&revs)) &&
+	       !(res = do_pick_commit(infos, commit)))
+		; /* do nothing */
 
-	return 0;
+	return res;
 }
 
 static int revert_or_cherry_pick(int argc, const char **argv, int revert, int edit)
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 10/18] revert: make read_and_refresh_cache() and prepare_revs() return errors

[Christian Couder]

They used to "die" in case of problems which is bad if we want to
take some action in case of error.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   27 ++++++++++++++++-----------
 1 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 57d4300..8b50e0c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -543,7 +543,7 @@ static int do_pick_commit(struct args_info *info, struct commit *commit)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs, struct args_info *info)
+static int prepare_revs(struct rev_info *revs, struct args_info *info)
 {
 	int argc;
 
@@ -553,29 +553,34 @@ static void prepare_revs(struct rev_info *revs, struct args_info *info)
 		revs->reverse = 1;
 
 	argc = setup_revisions(info->commit_argc, info->commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage(info));
+	if (argc > 1) {
+		fprintf(stderr, "usage: %s", *revert_or_cherry_pick_usage(info));
+		return 129;
+	}
 
 	if (prepare_revision_walk(revs))
-		die("revision walk setup failed");
+		return error("revision walk setup failed");
 
 	if (!revs->commits)
-		die("empty commit set passed");
+		return error("empty commit set passed");
+
+	return 0;
 }
 
-static void read_and_refresh_cache(const char *me)
+static int read_and_refresh_cache(const char *me)
 {
 	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", me);
+		return error("git %s: failed to read the index", me);
 	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", me);
+			return error("git %s: failed to refresh the index", me);
 	}
 	rollback_lock_file(&index_lock);
+	return 0;
 }
 
 static int ff_incompatible(int val, const char *opt)
@@ -596,9 +601,9 @@ static int pick_commits(struct args_info *infos)
 	     (res = ff_incompatible(infos->edit, "--edit"))))
 			return res;
 
-	read_and_refresh_cache(me);
-
-	prepare_revs(&revs, infos);
+	if ((res = read_and_refresh_cache(me)) ||
+	    (res = prepare_revs(&revs, infos)))
+		return res;
 
 	while ((commit = get_revision(&revs)) &&
 	       !(res = do_pick_commit(infos, commit)))
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 11/18] revert: add get_todo_content() and create_todo_file()

[Christian Couder]

These static functions will make it possible to write "todo"
and "done" files. These files will list the actions (cherry
picks or reverts) that are still to be completed and that
have already been done respectively.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8b50e0c..7429be2 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -177,6 +177,69 @@ static char *get_encoding(const char *message, const unsigned char *sha1)
 	return NULL;
 }
 
+static void get_todo_content(struct strbuf *buf, struct commit_list *list,
+			     const char *line_prefix, struct args_info *info)
+{
+	struct commit_list *cur = list;
+	struct strbuf cmd = STRBUF_INIT;
+
+	if (line_prefix)
+		strbuf_addstr(&cmd, line_prefix);
+	strbuf_addstr(&cmd, info->action == REVERT ? "revert " : "pick ");
+	if (info->no_commit)
+		strbuf_addstr(&cmd, "-n ");
+	if (info->edit)
+		strbuf_addstr(&cmd, "-e ");
+	if (info->signoff)
+		strbuf_addstr(&cmd, "-s ");
+	if (info->mainline)
+		strbuf_addf(&cmd, "-m %d ", info->mainline);
+	if (info->allow_rerere_auto)
+		strbuf_addstr(&cmd, "--rerere-autoupdate ");
+	if (info->strategy)
+		strbuf_addf(&cmd, "--strategy %s ", info->strategy);
+	if (info->no_replay)
+		strbuf_addstr(&cmd, "-x ");
+	if (info->allow_ff)
+		strbuf_addstr(&cmd, "--ff ");
+
+	for (; cur; cur = cur->next) {
+		struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+		const unsigned char *sha1 = cur->item->object.sha1;
+		if (get_message(cur->item->buffer, sha1, &msg) != 0)
+			die("Cannot get commit message for %s",
+			    sha1_to_hex(sha1));
+		strbuf_addbuf(buf, &cmd);
+		strbuf_addf(buf, " %s # %s\n",
+			    find_unique_abbrev(sha1, DEFAULT_ABBREV),
+			    msg.subject);
+		free_message(&msg);
+	}
+
+	strbuf_release(&cmd);
+}
+
+static void create_todo_file(const char *filepath, int append,
+			     struct commit_list *list, const char *line_prefix,
+			     struct args_info *info)
+{
+	int fd, flags;
+	struct strbuf buf = STRBUF_INIT;
+
+	get_todo_content(&buf, list, line_prefix, info);
+
+	flags = O_WRONLY | O_CREAT | (append ? O_APPEND : O_TRUNC);
+	fd = open(filepath, flags, 0666);
+	if (fd < 0)
+		die_errno("Could not open file '%s' for writing", filepath);
+
+	write_or_whine(fd, buf.buf, buf.len, filepath);
+
+	close(fd);
+
+	strbuf_release(&buf);
+}
+
 static void add_message_to_msg(struct strbuf *msgbuf, const char *message,
 			       const unsigned char *sha1)
 {
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 12/18] revert: write TODO and DONE files in case of failure

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c                    |   42 ++++++++++++++++++++++++++++++-----
 t/t3508-cherry-pick-many-commits.sh |   22 ++++++++++++++++++
 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7429be2..27e9d6f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -14,6 +14,7 @@
 #include "rerere.h"
 #include "merge-recursive.h"
 #include "refs.h"
+#include "dir.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -55,6 +56,11 @@ struct args_info {
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+#define SEQ_DIR		"sequencer"
+#define SEQ_PATH	git_path(SEQ_DIR)
+#define TODO_FILE	git_path(SEQ_DIR "/git-cherry-pick-todo")
+#define DONE_FILE	git_path(SEQ_DIR "/git-cherry-pick-done")
+
 static char *get_encoding(const char *message, const unsigned char *sha1);
 
 static const char * const *revert_or_cherry_pick_usage(struct args_info *info)
@@ -651,7 +657,25 @@ static int ff_incompatible(int val, const char *opt)
 	return val ? error("cherry-pick --ff cannot be used with %s", opt) : 0;
 }
 
-static int pick_commits(struct args_info *infos)
+static int save_todo_and_done(int res, struct args_info *infos,
+			      struct commit *commit,
+			      struct commit_list *todo_list,
+			      struct commit_list **done_list)
+{
+	if (res) {
+		if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777))
+			die_errno("Could not create sequencer directory '%s'",
+				  SEQ_PATH);
+		if (commit)
+			commit_list_insert(commit, &todo_list);
+		create_todo_file(TODO_FILE, 0, todo_list, "", infos);
+		*done_list = reverse_commit_list(*done_list);
+		create_todo_file(DONE_FILE, 0, *done_list, "", infos);
+	}
+	return res;
+}
+
+static int pick_commits(struct args_info *infos, struct commit_list **done_list)
 {
 	struct rev_info revs;
 	struct commit *commit;
@@ -662,22 +686,24 @@ static int pick_commits(struct args_info *infos)
 	     (res = ff_incompatible(infos->no_commit, "--no_commit")) ||
 	     (res = ff_incompatible(infos->no_replay, "-x")) ||
 	     (res = ff_incompatible(infos->edit, "--edit"))))
-			return res;
+		return save_todo_and_done(res, infos, NULL, NULL, done_list);
 
 	if ((res = read_and_refresh_cache(me)) ||
 	    (res = prepare_revs(&revs, infos)))
-		return res;
+		return save_todo_and_done(res, infos, NULL, NULL, done_list);
 
 	while ((commit = get_revision(&revs)) &&
 	       !(res = do_pick_commit(infos, commit)))
-		; /* do nothing */
+		commit_list_insert(commit, done_list);
 
-	return res;
+	return save_todo_and_done(res, infos, commit, revs.commits, done_list);
 }
 
 static int revert_or_cherry_pick(int argc, const char **argv, int revert, int edit)
 {
 	struct args_info infos;
+	struct commit_list *done_list = NULL;
+	int res;
 
 	git_config(git_default_config, NULL);
 	me = revert ? "revert" : "cherry-pick";
@@ -686,7 +712,11 @@ static int revert_or_cherry_pick(int argc, const char **argv, int revert, int ed
 	infos.action = revert ? REVERT : CHERRY_PICK;
 	parse_args(argc, argv, &infos);
 
-	return pick_commits(&infos);
+	res = pick_commits(&infos, &done_list);
+
+	free_commit_list(done_list);
+
+	return res;
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 8e09fd0..9213d59 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -156,4 +156,26 @@ test_expect_success 'cherry-pick --stdin works' '
 	check_head_differs_from fourth
 '
 
+test_expect_success 'create some files to test --continue' '
+	TODO_FILE="$(git rev-parse --git-dir)/sequencer/git-cherry-pick-todo" &&
+	DONE_FILE="$(git rev-parse --git-dir)/sequencer/git-cherry-pick-done" &&
+
+	cat <<-EOF >expected_todo &&
+	pick  $(git rev-parse --short fourth) # fourth
+	EOF
+
+	cat <<-EOF >expected_done
+	pick  $(git rev-parse --short second) # second
+	EOF
+'
+
+test_expect_success 'failed cherry-pick produces todo and done files' '
+	git checkout -f master &&
+	git reset --hard first &&
+	test_tick &&
+	test_must_fail git cherry-pick fourth~2 fourth &&
+	test_cmp expected_todo "$TODO_FILE" &&
+	test_cmp expected_done "$DONE_FILE"
+'
+
 test_done
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 13/18] revert: add option parsing for option --continue

[Christian Couder]

Right now this new option does nothing. It is just a start.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 27e9d6f..12a2409 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -29,11 +29,13 @@
 
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
+	"git revert --continue",
 	NULL
 };
 
 static const char * const cherry_pick_usage[] = {
 	"git cherry-pick [options] <commit-ish>",
+	"git cherry-pick --continue",
 	NULL
 };
 
@@ -48,6 +50,7 @@ struct args_info {
 	int signoff;
 	int allow_ff;
 	int allow_rerere_auto;
+	int continuing;
 	const char *strategy;
 	const char **commit_argv;
 	int commit_argc;
@@ -72,6 +75,8 @@ static void parse_args(int argc, const char **argv, struct args_info *info)
 {
 	int noop;
 	struct option options[] = {
+		OPT_BOOLEAN(0, "continue", &info->continuing,
+			    "continue after resolving a conflict"),
 		OPT_BOOLEAN('n', "no-commit", &info->no_commit,
 			    "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &info->edit, "edit the commit message"),
@@ -102,7 +107,18 @@ static void parse_args(int argc, const char **argv, struct args_info *info)
 					  PARSE_OPT_KEEP_UNKNOWN);
 	info->commit_argv = argv;
 
-	if (info->commit_argc < 2)
+	if (info->continuing) {
+		if (info->commit_argc != 1)
+			usage_msg_opt("No argument can be passed along "
+				      "with option --continue!",
+				      info->usage_str, options);
+		if (info->no_commit || info->edit || info->signoff ||
+		    info->mainline || info->allow_rerere_auto || info->strategy ||
+		    info->no_replay || info->allow_ff)
+			usage_msg_opt("No other option can be passed along "
+				      "with option --continue!",
+				      info->usage_str, options);
+	} else if (info->commit_argc < 2)
 		usage_with_options(info->usage_str, options);
 }
 
@@ -712,7 +728,10 @@ static int revert_or_cherry_pick(int argc, const char **argv, int revert, int ed
 	infos.action = revert ? REVERT : CHERRY_PICK;
 	parse_args(argc, argv, &infos);
 
-	res = pick_commits(&infos, &done_list);
+	if (infos.continuing)
+		res = 0;
+	else
+		res = pick_commits(&infos, &done_list);
 
 	free_commit_list(done_list);
 
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 14/18] revert: move global variable "me" into "struct args_info"

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 12a2409..7513a00 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,8 +39,6 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static const char *me;
-
 struct args_info {
 	enum { REVERT, CHERRY_PICK } action;
 	int edit;
@@ -51,6 +49,7 @@ struct args_info {
 	int allow_ff;
 	int allow_rerere_auto;
 	int continuing;
+	const char *me;
 	const char *strategy;
 	const char **commit_argv;
 	int commit_argc;
@@ -91,6 +90,9 @@ static void parse_args(int argc, const char **argv, struct args_info *info)
 		OPT_END(),
 	};
 
+	info->me = info->action == REVERT ? "revert" : "cherry-pick";
+	setenv(GIT_REFLOG_ACTION, info->me, 0);
+
 	if (info->action == CHERRY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &info->no_replay, "append commit name"),
@@ -403,7 +405,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
 
 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)
+			      unsigned char *head, const char *me,
+			      struct strbuf *msgbuf)
 {
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
@@ -507,7 +510,7 @@ static int do_pick_commit(struct args_info *info, struct commit *commit)
 		if (get_sha1("HEAD", head))
 			return error("You do not have a valid HEAD");
 		if (index_differs_from("HEAD", 0))
-			return error_dirty_index(me);
+			return error_dirty_index(info->me);
 	}
 	discard_cache();
 
@@ -543,7 +546,7 @@ static int do_pick_commit(struct args_info *info, struct commit *commit)
 
 	if (parent && parse_commit(parent) < 0)
 		return error("%s: cannot parse parent commit %s",
-			     me, sha1_to_hex(parent->object.sha1));
+			     info->me, sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit->buffer, commit->object.sha1, &msg) != 0)
 		return error("Cannot get commit message for %s",
@@ -590,7 +593,7 @@ static int do_pick_commit(struct args_info *info, struct commit *commit)
 	if (!info->strategy || !strcmp(info->strategy, "recursive") ||
 	    info->action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf);
+					 head, info->me, &msgbuf);
 		write_res = write_message(&msgbuf, defmsg);
 		if (write_res)
 			return write_res;
@@ -704,7 +707,7 @@ static int pick_commits(struct args_info *infos, struct commit_list **done_list)
 	     (res = ff_incompatible(infos->edit, "--edit"))))
 		return save_todo_and_done(res, infos, NULL, NULL, done_list);
 
-	if ((res = read_and_refresh_cache(me)) ||
+	if ((res = read_and_refresh_cache(infos->me)) ||
 	    (res = prepare_revs(&revs, infos)))
 		return save_todo_and_done(res, infos, NULL, NULL, done_list);
 
@@ -722,8 +725,6 @@ static int revert_or_cherry_pick(int argc, const char **argv, int revert, int ed
 	int res;
 
 	git_config(git_default_config, NULL);
-	me = revert ? "revert" : "cherry-pick";
-	setenv(GIT_REFLOG_ACTION, me, 0);
 	memset(&infos, 0, sizeof(infos));
 	infos.action = revert ? REVERT : CHERRY_PICK;
 	parse_args(argc, argv, &infos);
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 15/18] revert: add NONE action and make parse_args() manage it

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7513a00..fee2e38 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -40,7 +40,7 @@ static const char * const cherry_pick_usage[] = {
 };
 
 struct args_info {
-	enum { REVERT, CHERRY_PICK } action;
+	enum { NONE = 0, REVERT, CHERRY_PICK } action;
 	int edit;
 	int no_replay;
 	int no_commit;
@@ -90,6 +90,17 @@ static void parse_args(int argc, const char **argv, struct args_info *info)
 		OPT_END(),
 	};
 
+	if (info->action == NONE) {
+		if (argc < 1)
+			die("no action argument");
+		if (!strcasecmp(argv[0], "p") || !strcasecmp(argv[0], "pick"))
+			info->action = CHERRY_PICK;
+		else if (!strcasecmp(argv[0], "revert"))
+			info->action = REVERT;
+		else
+			die("unknown action argument: %s", argv[0]);
+	}
+
 	info->me = info->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, info->me, 0);
 
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 16/18] revert: implement parsing TODO and DONE files

[Christian Couder]

From: Stephan Beyer <s-beyer@gmx.net>

The code from this patch comes from the git sequencer Google
Summer of Code 2008 project available here:

http://repo.or.cz/w/git/sbeyer.git

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |  228 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 228 insertions(+), 0 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index fee2e38..ca65b92 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -70,6 +70,234 @@ static const char * const *revert_or_cherry_pick_usage(struct args_info *info)
 	return info->action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
+/*
+ * A structure for a parsed instruction line plus a next pointer
+ * to allow linked list behavior
+ */
+struct parsed_insn {
+	int argc;
+	const char **argv;
+	int line;
+	struct strbuf orig;
+	struct parsed_insn *next;
+};
+
+struct parsed_file {
+	size_t count;
+	size_t total;
+	struct parsed_insn *first;
+	struct parsed_insn *last;
+	struct parsed_insn *cur; /* a versatile helper */
+};
+
+static int parse_line(char *buf, size_t len, int lineno,
+		      struct parsed_insn **line)
+{
+	static int alloc = 0;
+	static struct strbuf arg_sb = STRBUF_INIT;
+	static enum {
+		ST_START,
+		ST_DELIMITER,
+		ST_ARGUMENT,
+		ST_ESCAPE,
+		ST_DOUBLE_QUOTES,
+		ST_DOUBLE_QUOTES_ESCAPE,
+		ST_SINGLE_QUOTES,
+	} state = ST_START;
+	/* The current rules are as follows:
+	 *  1. whitespace at the beginning is ignored
+	 *  2. insn is everything up to next whitespace or EOL
+	 *  3. now whitespace acts as delimiter for arguments,
+	 *     except if written in single or double quotes
+	 *  4. \ acts as escape inside and outside double quotes.
+	 *     Inside double quotes, this is only useful for \".
+	 *     Outside, it is useful for \', \", \\ and \ .
+	 *  5. single quotes do not have an escape character
+	 *  6. abort on "#" (comments)
+	 */
+
+	size_t i, j = 0;
+	struct parsed_insn *ret = *line;
+
+	for (i = 0; i <= len; ++i) {
+		switch (state) {
+		case ST_START:
+			switch (buf[i]) {
+			case ' ':
+			case '\t':
+				continue;
+			case 0:
+			case '#':
+				break;
+			case '\'':
+				j = i+1;
+				state = ST_SINGLE_QUOTES;
+				break;
+			case '"':
+				j = i+1;
+				state = ST_DOUBLE_QUOTES;
+				break;
+			default:
+				j = i;
+				state = ST_ARGUMENT;
+				break;
+			}
+			/* prepare everything */
+			ret = xcalloc(1, sizeof(*ret));
+			ret->line = lineno;
+			strbuf_init(&ret->orig, len+2);
+			if (!buf[i] || buf[i] == '#') /* empty/comment */
+				goto finish;
+			break;
+		case ST_DELIMITER:
+			switch (buf[i]) {
+			case ' ':
+			case '\t':
+				continue;
+			case 0:
+				break;
+			case '\'':
+				j = i+1;
+				state = ST_SINGLE_QUOTES;
+				break;
+			case '"':
+				j = i+1;
+				state = ST_DOUBLE_QUOTES;
+				break;
+			default:
+				j = i;
+				state = ST_ARGUMENT;
+				if (buf[i] == '#') /* a comment */
+					goto finish;
+				break;
+			}
+			/* prepare next argument */
+			ALLOC_GROW(ret->argv, ret->argc + 1, alloc);
+			ret->argv[ret->argc++] = strbuf_detach(&arg_sb, NULL);
+			break;
+		case ST_ARGUMENT:
+			switch (buf[i]) {
+			case ' ':
+			case '\t':
+				strbuf_add(&arg_sb, buf+j, i-j);
+				state = ST_DELIMITER;
+				break;
+			case '"':
+				strbuf_add(&arg_sb, buf+j, i-j);
+				j = i + 1;
+				state = ST_DOUBLE_QUOTES;
+				break;
+			case '\'':
+				strbuf_add(&arg_sb, buf+j, i-j);
+				j = i + 1;
+				state = ST_SINGLE_QUOTES;
+				break;
+			case '\\':
+				strbuf_add(&arg_sb, buf+j, i-j);
+				j = i + 1;
+				state = ST_ESCAPE;
+			default:
+				break;
+			}
+			break;
+		case ST_ESCAPE:
+				state = ST_ARGUMENT;
+			break;
+		case ST_DOUBLE_QUOTES:
+			switch (buf[i]) {
+			case '"':
+				strbuf_add(&arg_sb, buf+j, i-j);
+				j = i + 1;
+				state = ST_ARGUMENT;
+				break;
+			case '\\':
+				strbuf_add(&arg_sb, buf+j, i-j);
+				j = i + 1;
+				state = ST_DOUBLE_QUOTES_ESCAPE;
+				break;
+			default:
+				break;
+			}
+			break;
+		case ST_DOUBLE_QUOTES_ESCAPE:
+			state = ST_DOUBLE_QUOTES;
+			break;
+		case ST_SINGLE_QUOTES:
+			switch (buf[i]) {
+			case '\'':
+				strbuf_add(&arg_sb, buf+j, i-j);
+				j = i + 1;
+				state = ST_ARGUMENT;
+				break;
+			default:
+				break;
+			}
+			break;
+		}
+	}
+finish:
+	*line = ret;
+	switch(state) {
+	case ST_DOUBLE_QUOTES:
+	case ST_DOUBLE_QUOTES_ESCAPE:
+	case ST_SINGLE_QUOTES:
+		strbuf_add(&arg_sb, buf+j, i-j-1);
+		strbuf_add(&arg_sb, "\n", 1);
+		return 1;
+	case ST_ARGUMENT:
+		if (i-j > 1)
+			strbuf_add(&arg_sb, buf+j, i-j-1);
+		ALLOC_GROW(ret->argv, ret->argc + 1, alloc);
+		ret->argv[ret->argc++] = strbuf_detach(&arg_sb, NULL);
+	case ST_DELIMITER:
+		state = ST_START;
+		alloc = 0;
+	default:
+		strbuf_addstr(&ret->orig, buf);
+		strbuf_addch(&ret->orig, '\n');
+		return 0;
+	}
+}
+
+static void add_parsed_line_to_parsed_file(struct parsed_insn *parsed_line,
+					   struct parsed_file *contents)
+{
+	if (!contents->first) {
+		contents->first = parsed_line;
+		contents->last = parsed_line;
+	} else {
+		contents->last->next = parsed_line;
+		contents->last = parsed_line;
+	}
+	if (parsed_line->argv)
+		contents->total++;
+}
+
+/* Parse a file fp; write result into contents */
+static void parse_file(const char *filename, struct parsed_file *contents)
+{
+	struct strbuf str = STRBUF_INIT;
+	struct parsed_insn *parsed_line = NULL;
+	int r = 0;
+	int lineno = 0;
+	FILE *fp = fp = fopen(filename, "r");
+	if (!fp)
+		die_errno("Could not open file '%s'", filename);
+
+	memset(contents, 0, sizeof(*contents));
+
+	while (strbuf_getline(&str, fp, '\n') != EOF) {
+		lineno++;
+		r = parse_line(str.buf, str.len, lineno, &parsed_line);
+		if (!r)
+			add_parsed_line_to_parsed_file(parsed_line, contents);
+	}
+	strbuf_release(&str);
+	fclose(fp);
+	if (r)
+		die("Unexpected end of file.");
+}
+
 static void parse_args(int argc, const char **argv, struct args_info *info)
 {
 	int noop;
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 17/18] revert: add remaining instructions in todo file

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index ca65b92..b51f7ab 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -483,14 +483,19 @@ static void get_todo_content(struct strbuf *buf, struct commit_list *list,
 }
 
 static void create_todo_file(const char *filepath, int append,
-			     struct commit_list *list, const char *line_prefix,
-			     struct args_info *info)
+			     struct commit_list *list, struct parsed_insn *insn,
+			     const char *line_prefix, struct args_info *info)
 {
 	int fd, flags;
 	struct strbuf buf = STRBUF_INIT;
 
 	get_todo_content(&buf, list, line_prefix, info);
 
+	if (insn) {
+		while ((insn = insn->next))
+			strbuf_addbuf(&buf, &insn->orig);
+	}
+
 	flags = O_WRONLY | O_CREAT | (append ? O_APPEND : O_TRUNC);
 	fd = open(filepath, flags, 0666);
 	if (fd < 0)
@@ -926,9 +931,9 @@ static int save_todo_and_done(int res, struct args_info *infos,
 				  SEQ_PATH);
 		if (commit)
 			commit_list_insert(commit, &todo_list);
-		create_todo_file(TODO_FILE, 0, todo_list, "", infos);
+		create_todo_file(TODO_FILE, 0, todo_list, NULL, "", infos);
 		*done_list = reverse_commit_list(*done_list);
-		create_todo_file(DONE_FILE, 0, *done_list, "", infos);
+		create_todo_file(DONE_FILE, 0, *done_list, NULL, "", infos);
 	}
 	return res;
 }
-- 
1.7.3.2.504.g59d466

[RFC/PATCH 18/18] revert: implement --continue processing

[Christian Couder]

This patch adds the pick_continue() and the process_insn() functions
to process all the instructions from a todo file.
The TODO and DONE files are removed at the end if there was no
error.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c                    |   54 ++++++++++++++++++----
 t/t3508-cherry-pick-many-commits.sh |   85 +++++++++++++++++++++++++++++++++-
 2 files changed, 127 insertions(+), 12 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index b51f7ab..46445b0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -923,7 +923,8 @@ static int ff_incompatible(int val, const char *opt)
 static int save_todo_and_done(int res, struct args_info *infos,
 			      struct commit *commit,
 			      struct commit_list *todo_list,
-			      struct commit_list **done_list)
+			      struct commit_list **done_list,
+			      struct parsed_insn *insn)
 {
 	if (res) {
 		if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777))
@@ -931,14 +932,15 @@ static int save_todo_and_done(int res, struct args_info *infos,
 				  SEQ_PATH);
 		if (commit)
 			commit_list_insert(commit, &todo_list);
-		create_todo_file(TODO_FILE, 0, todo_list, NULL, "", infos);
+		create_todo_file(TODO_FILE, 0, todo_list, insn, "", infos);
 		*done_list = reverse_commit_list(*done_list);
-		create_todo_file(DONE_FILE, 0, *done_list, NULL, "", infos);
+		create_todo_file(DONE_FILE, 1, *done_list, NULL, "", infos);
 	}
 	return res;
 }
 
-static int pick_commits(struct args_info *infos, struct commit_list **done_list)
+static int pick_commits(struct args_info *infos, struct commit_list **done_list,
+			struct parsed_insn *insn)
 {
 	struct rev_info revs;
 	struct commit *commit;
@@ -949,17 +951,51 @@ static int pick_commits(struct args_info *infos, struct commit_list **done_list)
 	     (res = ff_incompatible(infos->no_commit, "--no_commit")) ||
 	     (res = ff_incompatible(infos->no_replay, "-x")) ||
 	     (res = ff_incompatible(infos->edit, "--edit"))))
-		return save_todo_and_done(res, infos, NULL, NULL, done_list);
+	  return save_todo_and_done(res, infos, NULL, NULL, done_list, insn);
 
 	if ((res = read_and_refresh_cache(infos->me)) ||
 	    (res = prepare_revs(&revs, infos)))
-		return save_todo_and_done(res, infos, NULL, NULL, done_list);
+		return save_todo_and_done(res, infos, NULL, NULL, done_list, insn);
 
 	while ((commit = get_revision(&revs)) &&
 	       !(res = do_pick_commit(infos, commit)))
 		commit_list_insert(commit, done_list);
 
-	return save_todo_and_done(res, infos, commit, revs.commits, done_list);
+	return save_todo_and_done(res, infos, commit, revs.commits, done_list, insn);
+}
+
+static int process_insn(struct parsed_insn *cur, struct commit_list **done_list)
+{
+	struct args_info infos;
+	memset(&infos, 0, sizeof(infos));
+	parse_args(cur->argc, cur->argv, &infos);
+
+	if (infos.continuing)
+		return error("option --continue is not allowed in todo file");
+
+	return pick_commits(&infos, done_list, cur);
+}
+
+static int pick_continue(struct commit_list **done_list)
+{
+	struct parsed_file content;
+	struct parsed_insn *cur;
+
+	if (!file_exists(TODO_FILE))
+		die("No %s file found, so nothing to continue", TODO_FILE);
+
+	parse_file(TODO_FILE, &content);
+
+	for (cur = content.first; cur; cur = cur->next) {
+		int res = process_insn(cur, done_list);
+		if (res)
+			return res;
+	}
+
+	unlink(TODO_FILE);
+	unlink(DONE_FILE);
+
+	return 0;
 }
 
 static int revert_or_cherry_pick(int argc, const char **argv, int revert, int edit)
@@ -974,9 +1010,9 @@ static int revert_or_cherry_pick(int argc, const char **argv, int revert, int ed
 	parse_args(argc, argv, &infos);
 
 	if (infos.continuing)
-		res = 0;
+		res = pick_continue(&done_list);
 	else
-		res = pick_commits(&infos, &done_list);
+		res = pick_commits(&infos, &done_list, NULL);
 
 	free_commit_list(done_list);
 
diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 9213d59..bbc6588 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -164,18 +164,97 @@ test_expect_success 'create some files to test --continue' '
 	pick  $(git rev-parse --short fourth) # fourth
 	EOF
 
-	cat <<-EOF >expected_done
+	cat <<-EOF >expected_done &&
 	pick  $(git rev-parse --short second) # second
 	EOF
+
+	cat <<-EOF >new_todo
+	pick  $(git rev-parse --short third) # third
+	pick  $(git rev-parse --short fourth) # fourth
+	EOF
+'
+
+test_expect_success 'cherry-pick --continue works' '
+	git checkout -f master &&
+	git reset --hard first &&
+	test_tick &&
+	test_must_fail git cherry-pick fourth~2 fourth &&
+	test_cmp expected_todo "$TODO_FILE" &&
+	test_cmp expected_done "$DONE_FILE" &&
+	git reset --merge HEAD &&
+	cp new_todo "$TODO_FILE" &&
+	git cherry-pick --continue &&
+	git diff --quiet other &&
+	git diff --quiet HEAD other &&
+	check_head_differs_from fourth &&
+	! test -e "$TODO_FILE" &&
+	! test -e "$DONE_FILE"
+'
+
+test_expect_success 'create more files to test --continue' '
+	cat <<-EOF >expected_todo_2 &&
+	pick  $(git rev-parse --short second) # second
+	EOF
+
+	cat <<-EOF >expected_done_2 &&
+	pick  $(git rev-parse --short second) # second
+	pick  $(git rev-parse --short third) # third
+	EOF
+
+	cat <<-EOF >new_todo_2
+	pick  $(git rev-parse --short third) # third
+	pick  $(git rev-parse --short second) # second
+	EOF
+'
+
+test_expect_success 'TODO and DONE files are ok when --continue fails (1)' '
+	git checkout -f master &&
+	git reset --hard first &&
+	test_tick &&
+	test_must_fail git cherry-pick fourth~2 fourth &&
+	test_cmp expected_todo "$TODO_FILE" &&
+	test_cmp expected_done "$DONE_FILE" &&
+	git reset --merge HEAD &&
+	cp new_todo_2 "$TODO_FILE" &&
+	test_must_fail git cherry-pick --continue &&
+	test_cmp expected_todo_2 "$TODO_FILE" &&
+	test_cmp expected_done_2 "$DONE_FILE" &&
+	rm "$TODO_FILE" &&
+	rm "$DONE_FILE"
+'
+
+test_expect_success 'create again more files to test --continue' '
+	cat <<-EOF >expected_todo_3 &&
+	pick  $(git rev-parse --short second) # second
+	pick  $(git rev-parse --short fourth) # fourth
+	EOF
+
+	cat <<-EOF >expected_done_3 &&
+	pick  $(git rev-parse --short second) # second
+	pick  $(git rev-parse --short third) # third
+	EOF
+
+	cat <<-EOF >new_todo_3
+	pick  $(git rev-parse --short third) # third
+	pick  $(git rev-parse --short second) # second
+	pick  $(git rev-parse --short fourth) # fourth
+	EOF
 '
 
-test_expect_success 'failed cherry-pick produces todo and done files' '
+test_expect_success 'TODO and DONE files are ok when --continue fails (2)' '
 	git checkout -f master &&
 	git reset --hard first &&
 	test_tick &&
 	test_must_fail git cherry-pick fourth~2 fourth &&
 	test_cmp expected_todo "$TODO_FILE" &&
-	test_cmp expected_done "$DONE_FILE"
+	test_cmp expected_done "$DONE_FILE" &&
+	git reset --merge HEAD &&
+	cp new_todo_3 "$TODO_FILE" &&
+	test_must_fail git cherry-pick --continue &&
+	test_cmp expected_todo_3 "$TODO_FILE" &&
+	test_cmp expected_done_3 "$DONE_FILE" &&
+	rm "$TODO_FILE" &&
+	rm "$DONE_FILE"
 '
 
 test_done
-- 
1.7.3.2.504.g59d466

Re: [RFC/PATCH 00/18] WIP implement cherry-pick/revert --continue

[Jonathan Nieder]

Hi Christian,

Christian Couder wrote:

> Many patches in this series are replacing calls to "die()" by
> "return error()", because the TODO and DONE files are written
> only when cherry-pick fails. This is efficient but perhaps it
> would be simpler and safer to write them before each cherry-pick
> just in case it fails, so that the "die()" calls don't need to
> be removed.

Another possibility would be to use set_die_routine()/atexit()/
sigchain_push_common(), but the "always write" solution does seem
simpler.

> (17):

Perhaps too many. :)

I like where this is going.  My main complaint is the commit messages;
given a clear explanation of the design it should not be too hard for
others to help write documentation, enhancements, and tests, but
without it is much harder.

Nit: the style of commit message in patch 16 is unnecessarily
demoralizing.  It basically says "track down the history in this repo
that may not exist in 2050 if you want to know what this patch is
about".  I think it would be better to say

	This code was written as part of the git sequencer
	Google Summer of Code project, 2008

and let the rest of the commit message tell the important details.
Readers can google for the detailed history.

Thanks for your work.
Jonathan