[RFC PATCH 00/11] Sequencer Foundations

[RFC PATCH 00/11] Sequencer Foundations

[Ramkumar Ramachandra]

Hi,

I've started working on building a sequencer for Git.  While the
outline is described in [1], I'd like some additional clarifications.
A big thanks to Christian's series [2] for the valuable roadmap.

Please note that 10/11 is not related to this series, but seems to be
a minor nit that's required to make all existing tests pass.

0. Is the general flow alright?

1. Is it okay to use this kind of adaptive error handling (calling
'die' in some places and returning error in other places), or should
it be more uniform?

2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
inelegant, mainly because of the command-line argument parsing
overhead.  Re-implementing it using more low-level functions doesn't
seem to be the way to go either: for example, 'reset --hard' has some
additional logic of writing HEAD and ORIG_HEAD, which I don't want to
duplicate.  Should I work on reworking parts of 'rerere.c' and
'revert.c', or is there some other way?

3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format.  This part will go into the sequencer.  Then I'll use a
cherry-pick specific file to keep the command-line options.  Yes, I'm
trying to work on Daniel's idea [3] from the very start.  Is this a
good idea?

4. I have a feeling that I've broken translation strings.  Is there a
README, plus a bunch of tests I can run to make sure that I've not
broken anything?

Thanks for reading.

-- Ram

[1]: http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908
[2]: http://thread.gmane.org/gmane.comp.version-control.git/162183
[3]: http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834

Ramkumar Ramachandra (11):
  revert: Avoid calling die; return error instead
  revert: Lose global variables "commit" and "me"
  revert: Introduce a struct to parse command-line options into
  revert: Separate cmdline argument handling from the functional code
  revert: Catch incompatible command-line options early
  revert: Implement parsing --continue, --abort and --skip
  revert: Handle conflict resolutions more elegantly
  usage: Introduce error_errno correspoding to die_errno
  revert: Write head, todo, done files
  revert: Give noop a default value while argument parsing
  revert: Implement --abort processing

 advice.c          |   14 ++
 advice.h          |    1 +
 builtin/revert.c  |  454 ++++++++++++++++++++++++++++++++++++++---------------
 git-compat-util.h |    2 +
 usage.c           |   34 ++++
 5 files changed, 381 insertions(+), 124 deletions(-)

-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 01/11] revert: Avoid calling die; return error instead

[Ramkumar Ramachandra]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 2bb13eb..017b1af 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -265,23 +265,23 @@ 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);
 	} else {
 		if (advice_commit_before_merge) {
 			if (action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"
+				return error(_("Your local changes would be overwritten by revert.\n"
 					  "Please, commit your changes or stash them to proceed."));
 			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"
+				return error(_("Your local changes would be overwritten by cherry-pick.\n"
 					  "Please, commit your changes or stash them to proceed."));
 		} else {
 			if (action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"));
+				return error(_("Your local changes would be overwritten by revert.\n"));
 			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"));
+				return error(_("Your local changes would be overwritten by cherry-pick.\n"));
 		}
 	}
 }
@@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	    (write_cache(index_fd, active_cache, active_nr) ||
 	     commit_locked_index(&index_lock)))
 		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), me);
+		return error(_("%s: Unable to write new index file"), me);
 	rollback_lock_file(&index_lock);
 
 	if (!clean) {
@@ -397,18 +397,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) {
@@ -417,19 +417,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"),
+			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."),
+	} else if (mainline > 0)
+		return error(_("Mainline was specified but commit %s is not a merge."),
 		    sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
@@ -440,11 +440,11 @@ static int do_pick_commit(void)
 	if (parent && parse_commit(parent) < 0)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
-		die(_("%s: cannot parse parent commit %s"),
+		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"),
+		return error(_("Cannot get commit message for %s"),
 				sha1_to_hex(commit->object.sha1));
 
 	/*
@@ -523,7 +523,7 @@ static int do_pick_commit(void)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs)
+static int prepare_revs(struct rev_info *revs)
 {
 	int argc;
 
@@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)
 		revs->reverse = 1;
 
 	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage());
+	if (argc > 1) {
+		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
+		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(_("%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(_("%s: failed to refresh the index"), me);
 	}
 	rollback_lock_file(&index_lock);
+	return 0;
 }
 
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	struct rev_info revs;
+	int res;
 
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -578,17 +583,15 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	read_and_refresh_cache(me);
+	if ((res = read_and_refresh_cache(me)) ||
+		(res = prepare_revs(&revs)))
+		return res;
 
-	prepare_revs(&revs);
+	while ((commit = get_revision(&revs)) &&
+		!(res = do_pick_commit(commit)))
+		;
 
-	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit();
-		if (res)
-			return res;
-	}
-
-	return 0;
+	return res;
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 02/11] revert: Lose global variables "commit" and "me"

[Ramkumar Ramachandra]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 017b1af..fdbacad 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,12 +37,10 @@ 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;
 
-static const char *me;
 
 /* Merge strategy. */
 static const char *strategy;
@@ -51,7 +49,7 @@ static size_t xopts_nr, xopts_alloc;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
-static char *get_encoding(const char *message);
+static char *get_encoding(struct commit *commit, const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(void)
 {
@@ -115,7 +113,8 @@ struct commit_message {
 	const char *message;
 };
 
-static int get_message(const char *raw_message, struct commit_message *out)
+static int get_message(struct commit *commit, const char *raw_message,
+		struct commit_message *out)
 {
 	const char *encoding;
 	const char *abbrev, *subject;
@@ -124,7 +123,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(commit, raw_message);
 	if (!encoding)
 		encoding = "UTF-8";
 	if (!git_commit_encoding)
@@ -162,7 +161,7 @@ static void free_message(struct commit_message *msg)
 	free(msg->reencoded_message);
 }
 
-static char *get_encoding(const char *message)
+static char *get_encoding(struct commit *commit, const char *message)
 {
 	const char *p = message, *eol;
 
@@ -184,7 +183,8 @@ 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 commit *commit, struct strbuf *msgbuf,
+			const char *message)
 {
 	const char *p = message;
 	while (*p && (*p != '\n' || p[1] != '\n'))
@@ -197,7 +197,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
 	strbuf_addstr(msgbuf, p);
 }
 
-static void write_cherry_pick_head(void)
+static void write_cherry_pick_head(struct commit *commit)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
@@ -265,10 +265,10 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static int error_dirty_index(const char *me)
+static int error_dirty_index()
 {
 	if (read_cache_unmerged()) {
-		die_resolve_conflict(me);
+		die_resolve_conflict(action == REVERT ? "revert" : "cherry-pick");
 	} else {
 		if (advice_commit_before_merge) {
 			if (action == REVERT)
@@ -379,7 +379,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;
@@ -402,7 +402,7 @@ static int do_pick_commit(void)
 		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();
 	}
 	discard_cache();
 
@@ -441,9 +441,10 @@ static int do_pick_commit(void)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
 		return error(_("%s: cannot parse parent commit %s"),
-		    me, sha1_to_hex(parent->object.sha1));
+			action == REVERT ? "revert" : "cherry-pick",
+			sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit->buffer, &msg) != 0)
+	if (get_message(commit, commit->buffer, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
 				sha1_to_hex(commit->object.sha1));
 
@@ -476,14 +477,14 @@ static int do_pick_commit(void)
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
-		add_message_to_msg(&msgbuf, msg.message);
+		add_message_to_msg(commit, &msgbuf, msg.message);
 		if (no_replay) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
 		if (!no_commit)
-			write_cherry_pick_head();
+			write_cherry_pick_head(commit);
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -546,10 +547,13 @@ static int prepare_revs(struct rev_info *revs)
 	return 0;
 }
 
-static int read_and_refresh_cache(const char *me)
+static int read_and_refresh_cache(void)
 {
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
+	const char *me;
+
+	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	if (read_index_preload(&the_index, NULL) < 0)
 		return error(_("%s: failed to read the index"), me);
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
@@ -565,10 +569,12 @@ static int 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;
+	const char *me;
 	int res;
 
 	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
+	me = (action == REVERT ? "revert" : "cherry-pick");
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	parse_args(argc, argv);
 
@@ -583,7 +589,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	if ((res = read_and_refresh_cache(me)) ||
+	if ((res = read_and_refresh_cache()) ||
 		(res = prepare_revs(&revs)))
 		return res;
 
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 03/11] revert: Introduce a struct to parse command-line options into

[Ramkumar Ramachandra]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index fdbacad..2b33220 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,17 +35,27 @@ 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 struct {
+	enum { REVERT, CHERRY_PICK } action;
 
-/* Merge strategy. */
-static const char *strategy;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+	/* Boolean options */
+	int edit;
+	int no_replay;
+	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;
+} cmd_opts;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -53,7 +63,7 @@ static char *get_encoding(struct commit *commit, const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(void)
 {
-	return action == REVERT ? revert_usage : cherry_pick_usage;
+	return cmd_opts.action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
@@ -62,8 +72,8 @@ static int option_parse_x(const struct option *opt,
 	if (unset)
 		return 0;
 
-	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-	xopts[xopts_nr++] = xstrdup(arg);
+	ALLOC_GROW(cmd_opts.xopts, cmd_opts.xopts_nr + 1, cmd_opts.xopts_alloc);
+	cmd_opts.xopts[cmd_opts.xopts_nr++] = xstrdup(arg);
 	return 0;
 }
 
@@ -72,37 +82,37 @@ static void parse_args(int argc, const char **argv)
 	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", &cmd_opts.no_commit, "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &cmd_opts.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_CALLBACK('X', "strategy-option", &xopts, "option",
+		OPT_BOOLEAN('s', "signoff", &cmd_opts.signoff, "add Signed-off-by:"),
+		OPT_INTEGER('m', "mainline", &cmd_opts.mainline, "parent number"),
+		OPT_RERERE_AUTOUPDATE(&cmd_opts.allow_rerere_auto),
+		OPT_STRING(0, "strategy", &cmd_opts.strategy, "strategy", "merge strategy"),
+		OPT_CALLBACK('X', "strategy-option", &cmd_opts.xopts, "option",
 			"option for merge strategy", option_parse_x),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
 	};
 
-	if (action == CHERRY_PICK) {
+	if (cmd_opts.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, &cmd_opts.no_replay, "append commit name"),
+			OPT_BOOLEAN(0, "ff", &cmd_opts.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,
+	cmd_opts.commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 				    PARSE_OPT_KEEP_ARGV0 |
 				    PARSE_OPT_KEEP_UNKNOWN);
-	if (commit_argc < 2)
+	if (cmd_opts.commit_argc < 2)
 		usage_with_options(usage_str, options);
 
-	commit_argv = argv;
+	cmd_opts.commit_argv = argv;
 }
 
 struct commit_message {
@@ -268,17 +278,17 @@ static struct tree *empty_tree(void)
 static int error_dirty_index()
 {
 	if (read_cache_unmerged()) {
-		die_resolve_conflict(action == REVERT ? "revert" : "cherry-pick");
+		die_resolve_conflict(cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	} else {
 		if (advice_commit_before_merge) {
-			if (action == REVERT)
+			if (cmd_opts.action == REVERT)
 				return error(_("Your local changes would be overwritten by revert.\n"
 					  "Please, commit your changes or stash them to proceed."));
 			else
 				return error(_("Your local changes would be overwritten by cherry-pick.\n"
 					  "Please, commit your changes or stash them to proceed."));
 		} else {
-			if (action == REVERT)
+			if (cmd_opts.action == REVERT)
 				return error(_("Your local changes would be overwritten by revert.\n"));
 			else
 				return error(_("Your local changes would be overwritten by cherry-pick.\n"));
@@ -320,7 +330,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	next_tree = next ? next->tree : empty_tree();
 	base_tree = base ? base->tree : empty_tree();
 
-	for (xopt = xopts; xopt != xopts + xopts_nr; xopt++)
+	for (xopt = cmd_opts.xopts; xopt != cmd_opts.xopts + cmd_opts.xopts_nr; xopt++)
 		parse_merge_opt(&o, *xopt);
 
 	clean = merge_trees(&o,
@@ -331,7 +341,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	    (write_cache(index_fd, active_cache, active_nr) ||
 	     commit_locked_index(&index_lock)))
 		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		return error(_("%s: Unable to write new index file"), me);
+		return error(_("%s: Unable to write new index file"),
+			cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	rollback_lock_file(&index_lock);
 
 	if (!clean) {
@@ -368,9 +379,9 @@ static int run_git_commit(const char *defmsg)
 
 	args[i++] = "commit";
 	args[i++] = "-n";
-	if (signoff)
+	if (cmd_opts.signoff)
 		args[i++] = "-s";
-	if (!edit) {
+	if (!cmd_opts.edit) {
 		args[i++] = "-F";
 		args[i++] = defmsg;
 	}
@@ -389,7 +400,7 @@ static int do_pick_commit(struct commit *commit)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
 
-	if (no_commit) {
+	if (cmd_opts.no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
 		 * merge the differences in, so let's compute the tree
@@ -407,7 +418,7 @@ static int do_pick_commit(struct commit *commit)
 	discard_cache();
 
 	if (!commit->parents) {
-		if (action == REVERT)
+		if (cmd_opts.action == REVERT)
 			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
@@ -416,32 +427,32 @@ static int do_pick_commit(struct commit *commit)
 		int cnt;
 		struct commit_list *p;
 
-		if (!mainline)
+		if (!cmd_opts.mainline)
 			return error(_("Commit %s is a merge but no -m option was given."),
 				sha1_to_hex(commit->object.sha1));
 
 		for (cnt = 1, p = commit->parents;
-		     cnt != mainline && p;
+		     cnt != cmd_opts.mainline && p;
 		     cnt++)
 			p = p->next;
-		if (cnt != mainline || !p)
+		if (cnt != cmd_opts.mainline || !p)
 			return error(_("Commit %s does not have parent %d"),
-			    sha1_to_hex(commit->object.sha1), mainline);
+			    sha1_to_hex(commit->object.sha1), cmd_opts.mainline);
 		parent = p->item;
-	} else if (mainline > 0)
+	} else if (cmd_opts.mainline > 0)
 		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 (cmd_opts.allow_ff && parent && !hashcmp(parent->object.sha1, head))
 		return fast_forward_to(commit->object.sha1, head);
 
 	if (parent && parse_commit(parent) < 0)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
 		return error(_("%s: cannot parse parent commit %s"),
-			action == REVERT ? "revert" : "cherry-pick",
+			cmd_opts.action == REVERT ? "revert" : "cherry-pick",
 			sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit, commit->buffer, &msg) != 0)
@@ -457,7 +468,7 @@ static int do_pick_commit(struct commit *commit)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (action == REVERT) {
+	if (cmd_opts.action == REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -478,16 +489,16 @@ static int do_pick_commit(struct commit *commit)
 		next = commit;
 		next_label = msg.label;
 		add_message_to_msg(commit, &msgbuf, msg.message);
-		if (no_replay) {
+		if (cmd_opts.no_replay) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
-		if (!no_commit)
+		if (!cmd_opts.no_commit)
 			write_cherry_pick_head(commit);
 	}
 
-	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+	if (!cmd_opts.strategy || !strcmp(cmd_opts.strategy, "recursive") || cmd_opts.action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf);
 		write_message(&msgbuf, defmsg);
@@ -499,22 +510,23 @@ static int do_pick_commit(struct commit *commit)
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res = try_merge_command(strategy, xopts_nr, xopts, common,
+		res = try_merge_command(cmd_opts.strategy, cmd_opts.xopts_nr,
+					cmd_opts.xopts, common,
 					sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
 	}
 
 	if (res) {
-		error(action == REVERT
+		error(cmd_opts.action == REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 		      msg.subject);
 		print_advice();
-		rerere(allow_rerere_auto);
+		rerere(cmd_opts.allow_rerere_auto);
 	} else {
-		if (!no_commit)
+		if (!cmd_opts.no_commit)
 			res = run_git_commit(defmsg);
 	}
 
@@ -530,10 +542,10 @@ static int prepare_revs(struct rev_info *revs)
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (action != REVERT)
+	if (cmd_opts.action != REVERT)
 		revs->reverse = 1;
 
-	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
+	argc = setup_revisions(cmd_opts.commit_argc, cmd_opts.commit_argv, revs, NULL);
 	if (argc > 1) {
 		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
 		return 129;
@@ -574,18 +586,18 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	int res;
 
 	git_config(git_default_config, NULL);
-	me = (action == REVERT ? "revert" : "cherry-pick");
+	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	parse_args(argc, argv);
 
-	if (allow_ff) {
-		if (signoff)
+	if (cmd_opts.allow_ff) {
+		if (cmd_opts.signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (no_commit)
+		if (cmd_opts.no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (no_replay)
+		if (cmd_opts.no_replay)
 			die(_("cherry-pick --ff cannot be used with -x"));
-		if (edit)
+		if (cmd_opts.edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
@@ -603,13 +615,13 @@ 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;
+		cmd_opts.edit = 1;
+	cmd_opts.action = REVERT;
 	return revert_or_cherry_pick(argc, argv);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-	action = CHERRY_PICK;
+	cmd_opts.action = CHERRY_PICK;
 	return revert_or_cherry_pick(argc, argv);
 }
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 04/11] revert: Separate cmdline argument handling from the functional code

[Ramkumar Ramachandra]

Based-on-patch-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 2b33220..9381541 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -578,18 +578,12 @@ static int read_and_refresh_cache(void)
 	return 0;
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int pick_commits(void)
 {
 	struct rev_info revs;
 	struct commit *commit;
-	const char *me;
 	int res;
 
-	git_config(git_default_config, NULL);
-	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
-	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
-
 	if (cmd_opts.allow_ff) {
 		if (cmd_opts.signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
@@ -616,12 +610,23 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	if (isatty(0))
 		cmd_opts.edit = 1;
+
+	git_config(git_default_config, NULL);
+	memset(&cmd_opts, 0, sizeof(cmd_opts));
 	cmd_opts.action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	setenv(GIT_REFLOG_ACTION, "revert", 0);
+	parse_args(argc, argv);
+
+	return pick_commits();
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
+	git_config(git_default_config, NULL);
+	memset(&cmd_opts, 0, sizeof(cmd_opts));
 	cmd_opts.action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	setenv(GIT_REFLOG_ACTION, "cherry-pick", 0);
+	parse_args(argc, argv);
+
+	return pick_commits();
 }
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 05/11] revert: Catch incompatible command-line options early

[Ramkumar Ramachandra]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 9381541..facae24 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -77,6 +77,24 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+static void die_opt_incompatible(const char *me, const char *base_opt,
+				int nargs, int opt_bitarray[], ...)
+{
+	const char *this_opt = NULL;
+	va_list ap;
+	int i;
+
+	va_start(ap, opt_bitarray);
+	for (i = 0; i < nargs; i ++) {
+		this_opt = va_arg(ap, const char *);
+		if (opt_bitarray[i]) {
+			die(_("%s: %s cannot be used with %s"),
+				me, this_opt, base_opt);
+		}
+	}
+	va_end(ap);
+}
+
 static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage();
@@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)
 	if (cmd_opts.commit_argc < 2)
 		usage_with_options(usage_str, options);
 
+	if (cmd_opts.allow_ff) {
+		int opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,
+				      cmd_opts.no_replay, cmd_opts.edit};
+		die_opt_incompatible(me, "--ff", 4, opt_bitarray, "--signoff",
+				"--no-commit", "-x", "--edit");
+	}
+
 	cmd_opts.commit_argv = argv;
 }
 
@@ -584,17 +609,6 @@ static int pick_commits(void)
 	struct commit *commit;
 	int res;
 
-	if (cmd_opts.allow_ff) {
-		if (cmd_opts.signoff)
-			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (cmd_opts.no_commit)
-			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (cmd_opts.no_replay)
-			die(_("cherry-pick --ff cannot be used with -x"));
-		if (cmd_opts.edit)
-			die(_("cherry-pick --ff cannot be used with --edit"));
-	}
-
 	if ((res = read_and_refresh_cache()) ||
 		(res = prepare_revs(&revs)))
 		return res;
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 06/11] revert: Implement parsing --continue, --abort and --skip

[Ramkumar Ramachandra]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index facae24..b90f3d0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,6 +39,11 @@ static const char * const cherry_pick_usage[] = {
 static struct {
 	enum { REVERT, CHERRY_PICK } action;
 
+	/* --abort, --skip, and --continue */
+	int abort_oper;
+	int skip_oper;
+	int continue_oper;
+
 	/* Boolean options */
 	int edit;
 	int no_replay;
@@ -98,8 +103,12 @@ static void die_opt_incompatible(const char *me, const char *base_opt,
 static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage();
+	const char *me;
 	int noop;
 	struct option options[] = {
+		OPT_BOOLEAN(0, "abort", &cmd_opts.abort_oper, "abort the current operation"),
+		OPT_BOOLEAN(0, "skip", &cmd_opts.skip_oper, "skip the current commit"),
+		OPT_BOOLEAN(0, "continue", &cmd_opts.continue_oper, "continue the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &cmd_opts.no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &cmd_opts.edit, "edit the commit message"),
 		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
@@ -127,7 +136,42 @@ static void parse_args(int argc, const char **argv)
 	cmd_opts.commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 				    PARSE_OPT_KEEP_ARGV0 |
 				    PARSE_OPT_KEEP_UNKNOWN);
-	if (cmd_opts.commit_argc < 2)
+
+	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
+
+	/* Check for incompatible command line arguments */
+	if (cmd_opts.abort_oper || cmd_opts.skip_oper || cmd_opts.continue_oper) {
+		char *this_oper;
+		int opt_bitarray[] = {cmd_opts.no_commit, cmd_opts.edit, noop,
+				      cmd_opts.signoff, cmd_opts.mainline,
+				      (cmd_opts.strategy ? 1 : 0),
+				      (cmd_opts.xopts ? 1 : 0),
+				      cmd_opts.no_replay, cmd_opts.allow_ff};
+		if (cmd_opts.abort_oper) {
+			this_oper = "--abort";
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.skip_oper, "--skip");
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.continue_oper, "--continue");
+		} else if (cmd_opts.skip_oper) {
+			this_oper = "--skip";
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.abort_oper, "--abort");
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.continue_oper, "--continue");
+		} else {
+			this_oper = "--continue";
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.abort_oper, "--abort");
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.skip_oper, "--skip");
+		}
+		die_opt_incompatible(me, this_oper, 9, opt_bitarray, "--no-commit",
+				"--edit", "-r",	"--signoff", "--mainline",
+				"--strategy", "--strategy-option", "-x", "--ff");
+	}
+
+	else if (cmd_opts.commit_argc < 2)
 		usage_with_options(usage_str, options);
 
 	if (cmd_opts.allow_ff) {
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 07/11] revert: Handle conflict resolutions more elegantly

[Ramkumar Ramachandra]

Avoid calling die; return error instead.

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

diff --git a/advice.c b/advice.c
index 0be4b5f..3c3c187 100644
--- a/advice.c
+++ b/advice.c
@@ -47,3 +47,17 @@ void NORETURN die_resolve_conflict(const char *me)
 	else
 		die("'%s' is not possible because you have unmerged files.", 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("'%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);
+	else
+		return error("'%s' is not possible because you have unmerged files.", 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 */
diff --git a/builtin/revert.c b/builtin/revert.c
index b90f3d0..e0352d4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -344,25 +344,20 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static int error_dirty_index()
+static int verify_resolution(const char *me)
 {
-	if (read_cache_unmerged()) {
-		die_resolve_conflict(cmd_opts.action == REVERT ? "revert" : "cherry-pick");
-	} else {
-		if (advice_commit_before_merge) {
-			if (cmd_opts.action == REVERT)
-				return error(_("Your local changes would be overwritten by revert.\n"
-					  "Please, commit your changes or stash them to proceed."));
-			else
-				return error(_("Your local changes would be overwritten by cherry-pick.\n"
-					  "Please, commit your changes or stash them to proceed."));
-		} else {
-			if (cmd_opts.action == REVERT)
-				return error(_("Your local changes would be overwritten by revert.\n"));
-			else
-				return error(_("Your local changes would be overwritten by cherry-pick.\n"));
-		}
-	}
+	if (!read_cache_unmerged())
+		return 0;
+
+	return error_resolve_conflict(me);
+}
+
+static int error_dirty_worktree(const char *me)
+{
+	if (advice_commit_before_merge)
+		return error(_("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.\n"), me);
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -467,8 +462,10 @@ static int do_pick_commit(struct commit *commit)
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
+	const char *me;
 	int res;
 
+	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	if (cmd_opts.no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
@@ -481,8 +478,8 @@ static int do_pick_commit(struct commit *commit)
 	} else {
 		if (get_sha1("HEAD", head))
 			return error(_("You do not have a valid HEAD"));
-		if (index_differs_from("HEAD", 0))
-			return error_dirty_index();
+		if (index_differs_from("HEAD", 0) && !verify_resolution(me))
+			return error_dirty_worktree(me);
 	}
 	discard_cache();
 
@@ -521,8 +518,7 @@ static int do_pick_commit(struct commit *commit)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
 		return error(_("%s: cannot parse parent commit %s"),
-			cmd_opts.action == REVERT ? "revert" : "cherry-pick",
-			sha1_to_hex(parent->object.sha1));
+			me, sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit, commit->buffer, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 08/11] usage: Introduce error_errno correspoding to die_errno

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-compat-util.h |    2 ++
 usage.c           |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 40498b3..00d2815 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -240,10 +240,12 @@ 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)));
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
+extern void set_error_routine(void (*routine)(const char *err, va_list params));
 
 extern int prefixcmp(const char *str, const char *prefix);
 extern int suffixcmp(const char *str, const char *suffix);
diff --git a/usage.c b/usage.c
index b5e67e3..8724b41 100644
--- a/usage.c
+++ b/usage.c
@@ -46,6 +46,11 @@ void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list param
 	die_routine = routine;
 }
 
+void set_error_routine(void (*routine)(const char *err, va_list params))
+{
+	error_routine = routine;
+}
+
 void NORETURN usagef(const char *err, ...)
 {
 	va_list params;
@@ -97,6 +102,35 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
+int error_errno(const char *fmt, ...)
+{
+	va_list params;
+	char fmt_with_err[1024];
+	char str_error[256], *err;
+	int i, j;
+
+	err = strerror(errno);
+	for (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {
+		if ((str_error[j++] = err[i++]) != '%')
+			continue;
+		if (j < sizeof(str_error) - 1) {
+			str_error[j++] = '%';
+		} else {
+			/* No room to double the '%', so we overwrite it with
+			 * '\0' below */
+			j--;
+			break;
+		}
+	}
+	str_error[j] = 0;
+	snprintf(fmt_with_err, sizeof(fmt_with_err), "%s: %s", fmt, str_error);
+
+	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.4.rc1.7.g2cf08.dirty

[PATCH 09/11] revert: Write head, todo, done files

[Ramkumar Ramachandra]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index e0352d4..25969a5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -13,6 +13,7 @@
 #include "rerere.h"
 #include "merge-recursive.h"
 #include "refs.h"
+#include "dir.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -25,6 +26,13 @@
  * Copyright (c) 2005 Junio C Hamano
  */
 
+#define SEQ_DIR "sequencer"
+
+#define SEQ_PATH	git_path(SEQ_DIR)
+#define HEAD_FILE	git_path(SEQ_DIR "/head")
+#define TODO_FILE	git_path(SEQ_DIR "/todo")
+#define DONE_FILE	git_path(SEQ_DIR "/done")
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -643,21 +651,95 @@ static int read_and_refresh_cache(void)
 	return 0;
 }
 
+static void format_todo(struct strbuf *buf, struct commit_list *list)
+{
+	struct commit_list *cur = NULL;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	const char *sha1 = NULL;
+	const char *action;
+
+	action = (cmd_opts.action == REVERT ? "revert" : "pick");
+	for (cur = list; cur; cur = cur->next) {
+		sha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+		if (get_message(cur->item, cur->item->buffer, &msg))
+			die("Cannot get commit message for %s", sha1);
+
+		strbuf_addf(buf, "%s %s %s\n", action, sha1, msg.subject);
+	}
+}
+
+static int persist_initialize(unsigned char *head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777))
+		return error_errno(_("Could not create sequencer directory '%s'"), SEQ_PATH);
+
+	if ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0)
+		return error_errno(_("Could not open '%s' for writing"), HEAD_FILE);
+
+	strbuf_addf(&buf, "%s", find_unique_abbrev(head, DEFAULT_ABBREV));
+	write_or_whine(fd, buf.buf, buf.len, HEAD_FILE);
+	close(fd);
+	strbuf_release(&buf);
+	return 0;
+}
+
+static int persist_todo_done(int res, struct commit_list *todo_list,
+			struct commit_list *done_list)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	if (!res)
+		return 0;
+
+	/* TODO file */
+	if ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0)
+		return error_errno(_("Could not open '%s' for writing"), TODO_FILE);
+
+	format_todo(&buf, todo_list);
+	write_or_whine(fd, buf.buf, buf.len, TODO_FILE);
+	close(fd);
+
+	/* DONE file */
+	strbuf_reset(&buf);
+	if ((fd = open(DONE_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0)
+		return error_errno(_("Could not open '%s' for writing"), DONE_FILE);
+
+	format_todo(&buf, done_list);
+	write_or_whine(fd, buf.buf, buf.len, DONE_FILE);
+	close(fd);
+	strbuf_release(&buf);
+	return res;
+}
+
 static int pick_commits(void)
 {
+	struct commit_list *done_list = NULL;
 	struct rev_info revs;
 	struct commit *commit;
+	unsigned char head[20];
 	int res;
 
+	if (get_sha1("HEAD", head))
+		return error(_("You do not have a valid HEAD"));
+
 	if ((res = read_and_refresh_cache()) ||
-		(res = prepare_revs(&revs)))
+		(res = prepare_revs(&revs)) ||
+		(res = persist_initialize(head)))
 		return res;
 
-	while ((commit = get_revision(&revs)) &&
-		!(res = do_pick_commit(commit)))
-		;
-
-	return res;
+	while ((commit = get_revision(&revs))) {
+		if (!(res = do_pick_commit(commit)))
+			commit_list_insert(commit, &done_list);
+		else {
+			commit_list_insert(commit, &revs.commits);
+			break;
+		}
+	}
+	return persist_todo_done(res, revs.commits, done_list);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 10/11] revert: Give noop a default value while argument parsing

[Ramkumar Ramachandra]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 25969a5..0720721 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -112,7 +112,8 @@ static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage();
 	const char *me;
-	int noop;
+	int noop = 0;
+
 	struct option options[] = {
 		OPT_BOOLEAN(0, "abort", &cmd_opts.abort_oper, "abort the current operation"),
 		OPT_BOOLEAN(0, "skip", &cmd_opts.skip_oper, "skip the current commit"),
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 11/11] revert: Implement --abort processing

[Ramkumar Ramachandra]

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 0720721..c050d4f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -743,6 +743,51 @@ static int pick_commits(void)
 	return persist_todo_done(res, revs.commits, done_list);
 }
 
+static int process_args(int argc, const char **argv)
+{
+	const char *me;
+	int fd;
+
+	parse_args(argc, argv);
+	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
+	if (cmd_opts.abort_oper) {
+		const char *args[3] = {NULL, NULL, NULL};
+		char head[DEFAULT_ABBREV];
+
+		args[0] = "rerere";
+		args[1] = "clear";
+		cmd_rerere(2, args, NULL);
+
+		if (!file_exists(HEAD_FILE))
+			goto error;
+
+		if ((fd = open(HEAD_FILE, O_RDONLY, 0666)) < 0)
+			return error_errno(_("Could not open '%s' for reading"), HEAD_FILE);
+		if (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV)
+			return error_errno(_("Corrupt '%s'"), HEAD_FILE);
+		close(fd);
+
+		args[0] = "reset";
+		args[1] = "--hard";
+		args[2] = head;
+		return cmd_reset(3, args, NULL);
+	}
+	else if (cmd_opts.skip_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+	else if (cmd_opts.continue_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+
+	return pick_commits();
+error:
+	return error(_("No %s in progress"), me);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	if (isatty(0))
@@ -752,9 +797,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	memset(&cmd_opts, 0, sizeof(cmd_opts));
 	cmd_opts.action = REVERT;
 	setenv(GIT_REFLOG_ACTION, "revert", 0);
-	parse_args(argc, argv);
 
-	return pick_commits();
+	return process_args(argc, argv);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
@@ -763,7 +807,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	memset(&cmd_opts, 0, sizeof(cmd_opts));
 	cmd_opts.action = CHERRY_PICK;
 	setenv(GIT_REFLOG_ACTION, "cherry-pick", 0);
-	parse_args(argc, argv);
 
-	return pick_commits();
+	return process_args(argc, argv);
 }
-- 
1.7.4.rc1.7.g2cf08.dirty

Re: [PATCH 01/11] revert: Avoid calling die; return error instead

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> [Subject: revert: Avoid calling die; return error instead]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Presumably this is because the sequencer is going to pick up after the
error and clean up a little (why doesn't the change description say
so?).  Will it be resuming after that or just performing a little
cleanup before the exit?

Might make sense, but:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -265,23 +265,23 @@ 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);

Won't that exit?  

>  	} else {

This "else" could be removed (decreasing the indent of the rest by
one tab stop) since the "if" case has already returned or exited.
Not the subject of this patch, just an idea for earlier or later. ;-)

[...]
> @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	    (write_cache(index_fd, active_cache, active_nr) ||
>  	     commit_locked_index(&index_lock)))
>  		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> -		die(_("%s: Unable to write new index file"), me);
> +		return error(_("%s: Unable to write new index file"), me);
>  	rollback_lock_file(&index_lock);

What happens to index_lock in the error case?

[...]
> @@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)
>  		revs->reverse = 1;
>  
>  	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
> -	if (argc > 1)
> -		usage(*revert_or_cherry_pick_usage());
> +	if (argc > 1) {
> +		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
> +		return 129;
> +	}

Yuck.  Maybe the error can be returned to the caller somehow, but
that seems somehow ambitious given that setup_revisions has all sorts
of ways to die anyway.

So you are bending the assumptions of many existing git functions (in
a good way).

I can think of at least three ways to go:

 1) Come up with a convention to give more information about the nature
    of returned errors in the functions you are touching.  For
    example, make sure errno is valid after the relevant functions, or
    use multiple negative values to express the nature of the error.

    So a caller could do:

	if (prepare_revs(...)) {
		if (errno == EINVAL)
			usage(*revert_or_cherry_pick_usage());
		die("BUG: unexpected error from prepare_revs");
	}

    Or:

 2) Use set_die_routine or sigchain_push + atexit to declare what cleanup
    has to happen before exiting.  Keep using die().

 3) Provide a new facility to register cleanup handlers that will free
    resources and otherwise return to a consistent state before
    unwinding the stack.  This way, you'd still have to audit die()
    calls to look for missing cleanup handlers, but they could stay as
    die() rather than changing to "return error" and the worried
    caller could use

	set_die_routine(longjmp_to_here);

    to keep git alive.  I don't suggest doing this.  It is a pain to
    get right and not obviously cleaner than "return error", and some
    errors really are unrecoverable (rather than just being a symptom
    of programmers to lazy to write error recovery code :)).

Okay, I'm stopping here.  Hope that helps.

Thanks,
Jonathan

Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> [Subject: revert: Introduce a struct to parse command-line options into]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

This gives a hint of another way to avoid the usage() trouble
mentioned in patch 1: it might be possible to let the caller take care
of parsing arguments.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -35,17 +35,27 @@ 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 struct {
> +	enum { REVERT, CHERRY_PICK } action;

That would require giving this struct a name, so it can be passed
around.  Not a bad idea anyway imho since then a person reading
top-to-bottom is not left in suspense:

 struct cherry_pick_opts {
	enum { REVERT, CHERRY_PICK } action;

	unsigned edit:1;
	unsigned no_replay:1;
	...
 };

Re: [RFC PATCH 00/11] Sequencer Foundations

[Daniel Barkalow]

On Sun, 10 Apr 2011, Ramkumar Ramachandra wrote:

> Hi,
> 
> I've started working on building a sequencer for Git.  While the
> outline is described in [1], I'd like some additional clarifications.
> A big thanks to Christian's series [2] for the valuable roadmap.
> 
> Please note that 10/11 is not related to this series, but seems to be
> a minor nit that's required to make all existing tests pass.

That looks like an actual bug that only doesn't matter currently because 
the function is never called with enough junk on the stack.

> 0. Is the general flow alright?

I suspect it would be easier to review some of this with certain things 
squashed together; one patch that changes all of the variable references 
to what you want them to be is easier to understand than one that moves 
statics to function arguments, one that moves statics to struct fields, 
etc. Likewise, when you're converting some of the die() calls to error(), 
it's easier to review the patch if all of the die() calls that aren't 
changed in that patch don't get changed later in the series.

> 1. Is it okay to use this kind of adaptive error handling (calling
> 'die' in some places and returning error in other places), or should
> it be more uniform?

I think it should be systematic but not necessarily uniform. You should be 
able to give a guideline as to how to decide which to use (and you should 
probably actually give the guideline, so future developers make consistent 
choices). I think of "die" as being ideally for situations where the 
program can't understand what has happened well enough to know what to do 
about it.

> 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> inelegant, mainly because of the command-line argument parsing
> overhead.  Re-implementing it using more low-level functions doesn't
> seem to be the way to go either: for example, 'reset --hard' has some
> additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> duplicate.  Should I work on reworking parts of 'rerere.c' and
> 'revert.c', or is there some other way?

(ITYM cmd_reset here)

I think rerere.c should get a rerere_clear(). I think it would make sense 
to implement the reset locally; the abort ought to be undoing exactly 
those things that you did, and I'm not actually sure the ORIG_HEAD is 
entirely appropriate. You ought to be able to use cleanup functions that 
correspond to the functions you used to make the mess in the first place.

> 3. From the format of the TODO and DONE files, one more thing should
> be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> format.  This part will go into the sequencer.  Then I'll use a
> cherry-pick specific file to keep the command-line options.  Yes, I'm
> trying to work on Daniel's idea [3] from the very start.  Is this a
> good idea?

I certainly like it. :)

> Thanks for reading.

You're welcome. :)

	-Daniel
*This .sig left intentionally blank*

Re: [RFC PATCH 00/11] Sequencer Foundations

[Jonathan Nieder]

Hi again,

Ramkumar Ramachandra wrote:

> I've started working on building a sequencer for Git.

Happy to see.

> 0. Is the general flow alright?

Not sure --- I don't have the big picture.  Could you give a quick
summary of the flow in the cover letter ("patch 1 does such-and-such,
so patch 2 can do such-and-such, leading to...") to the next revision,
and quick explanations of the purpose (i.e., why we should want each
change) in the individual change descriptions?

> 1. Is it okay to use this kind of adaptive error handling (calling
> 'die' in some places and returning error in other places), or should
> it be more uniform?

'die' gets used in two ways (well, one way really):

 - to say "there is no sane way to recover from this failure".  For
   example, xmalloc dies if even after trying to free up memory,
   malloc still could not satisfy the request.

 - to say "so far we've been too lazy to implement recovery from
   this failure".  Or "while we *could* recover from this failure, no
   one has needed it, so let's not --- that code would just bitrot."

Therefore a mixture of 'die' and 'return error' seems inevitable.  The
dangerous mixtures to avoid are those likely to trip up callers (e.g.,
if all code paths 'return error' except one edge case).

> 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> inelegant, mainly because of the command-line argument parsing
> overhead.  Re-implementing it using more low-level functions doesn't
> seem to be the way to go either: for example, 'reset --hard' has some
> additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> duplicate.  Should I work on reworking parts of 'rerere.c' and
> 'revert.c', or is there some other way?

See "git log --grep=libify" for examples.  Isn't rerere already
libified?  Ah, you need "rerere clear" --- I think a rerere_clear
function alongside rerere_forget et al would make sense.

More generally, it should be feasible to expose a nice, simple API for
any functionality you need (with params struct if necessary, etc).
That's how many of the current APIs (revision walking, diffcore, ...)
came about.

> 3. From the format of the TODO and DONE files, one more thing should
> be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> format.  This part will go into the sequencer.  Then I'll use a
> cherry-pick specific file to keep the command-line options.  Yes, I'm
> trying to work on Daniel's idea [3] from the very start.  Is this a
> good idea?

This is still bouncing in my head.  I think I like it --- is the idea
that some day you could put commands like

	am topic.mbox

in your insn sheet, or do nested rebases with a --force-nested option?
That does sound useful.  How would one request "skip to the next
operation in the outer rebase" on the command line?  This is starting
to feel like a debugger.

> 4. I have a feeling that I've broken translation strings.  Is there a
> README, plus a bunch of tests I can run to make sure that I've not
> broken anything?

If you put "GETTEXT_POISON = YesPlease" in your config.mak, the
translations will be translated to gibberish when the GIT_GETTEXT_POISON
envvar is set, so you can use the test suite as a sanity check.
"make pot" can be used to get the translation template that
translators will see.

As for a readme explaining how to use _, N_, and Q_, yes, I think that
would be useful.  I think Ævar's series has something like that (but
targetted at translators) later on; it might make sense to prod him or
me for a simpler explanation can be merged immediately.  Until then,
there is "git log gettext.h".

Regards,
Jonathan

Re: [RFC PATCH 00/11] Sequencer Foundations

[Daniel Barkalow]

On Sun, 10 Apr 2011, Jonathan Nieder wrote:

> > 3. From the format of the TODO and DONE files, one more thing should
> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > format.  This part will go into the sequencer.  Then I'll use a
> > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > trying to work on Daniel's idea [3] from the very start.  Is this a
> > good idea?
> 
> This is still bouncing in my head.  I think I like it --- is the idea
> that some day you could put commands like
> 
> 	am topic.mbox
> 
> in your insn sheet, or do nested rebases with a --force-nested option?
> That does sound useful.  How would one request "skip to the next
> operation in the outer rebase" on the command line?  This is starting
> to feel like a debugger.

The most basic idea is that, in the case of a rebase where a patch fails 
to apply, there are actually two levels of operation that have to be 
deferred until after the human intervention: making the commit where the 
pick failed, and applying further patches. If there are going to be 
multiple higher-level operations (e.g., rebase and rebase -i), they should 
be able to share the completion of the pick operation, and a basic "git 
cherry-pick <sha1>" ought to be able to share it, too, so you can finish 
that up without having to remember what the command is. People should be 
able to implement custom higher-level operations (e.g., "cherry-pick all 
the commits attached to bugs in bugzilla that block the release tracker" 
or something), they should be able to rely on library code that knows how 
to finish the conflicted cherry-pick. (Another interesting example might 
be "reconstruct the linux-next tree", which is a series of merges, and 
which should have --continue share the post-intervention code from the 
single merge process before going on to do the custom higher-level thing.)

My feeling is that "--skip" is actually "abort the pick, but continue the 
rebase". I suppose there could be more than two levels, and people could 
want to skip a higher-level chunk, but that's something to get to when 
someone actually wants it; we've obviously already got the two-level 
situation now, so we can implement that.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC PATCH 00/11] Sequencer Foundations

[Christian Couder]

On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
> Hi,
> 
> I've started working on building a sequencer for Git.  

So you are starting the GSoC early! Great!
When (or before) it really starts, just make sure you put your work on a 
public Git repository and you send status updates regularly (weekly if 
possible).

> 3. From the format of the TODO and DONE files, one more thing should
> be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> format.  This part will go into the sequencer.  Then I'll use a
> cherry-pick specific file to keep the command-line options.  Yes, I'm
> trying to work on Daniel's idea [3] from the very start.  Is this a
> good idea?

I think that the TODO and DONE file format will need at one point to include 
options and it is simpler if this change is done early. Using a cherry-pick 
specific file to keep the options is not very generic for a sequencer that could 
be used for many things.

For example, as we have rebase --interactive, we will probably want to have 
cherry-pick --interactive, and when editing the TODO file we might want to use 
different cherry-pick options when picking different commits.

This would also make the different cherry-pick options available when using 
rebase --interactive once it uses the sequencer.

> [1]:
> http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183 [3]:
> http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834

[3] is missing here.

Thanks,
Christian.

Re: [PATCH 02/11] revert: Lose global variables "commit" and "me"

[Christian Couder]

On Sunday 10 April 2011 17:11:48 Ramkumar Ramachandra wrote:
> @@ -546,10 +547,13 @@ static int prepare_revs(struct rev_info *revs)
>  	return 0;
>  }
> 
> -static int read_and_refresh_cache(const char *me)
> +static int read_and_refresh_cache(void)
>  {
>  	static struct lock_file index_lock;
>  	int index_fd = hold_locked_index(&index_lock, 0);
> +	const char *me;
> +
> +	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");

It looks like this patch will not compile as cmd_opts is introduced in patch 
03/11.

>  	if (read_index_preload(&the_index, NULL) < 0)
>  		return error(_("%s: failed to read the index"), me);
>  	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL,

Best regards,
Christian.

Re: [RFC PATCH 00/11] Sequencer Foundations

[Ramkumar Ramachandra]

Hi Christian,

Christian Couder writes:
> On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
> > Hi,
> > 
> > I've started working on building a sequencer for Git.  
> 
> So you are starting the GSoC early! Great!
> When (or before) it really starts, just make sure you put your work on a 
> public Git repository and you send status updates regularly (weekly if 
> possible).

Ofcourse.  I've already discussed many of these issues last year [1].
The work corresponding to this particular series can be found in the
'sequencer' branch on my GitHub fork [2].  Since the results haven't
been announced, and the coding period hasn't begun, this work should
be treated like "normal work" -- I just wrote it this weekend.

> > 3. From the format of the TODO and DONE files, one more thing should
> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > format.  This part will go into the sequencer.  Then I'll use a
> > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > trying to work on Daniel's idea [3] from the very start.  Is this a
> > good idea?
> 
> I think that the TODO and DONE file format will need at one point to include 
> options and it is simpler if this change is done early. Using a cherry-pick 
> specific file to keep the options is not very generic for a sequencer that could 
> be used for many things.
> 
> For example, as we have rebase --interactive, we will probably want to have 
> cherry-pick --interactive, and when editing the TODO file we might want to use 
> different cherry-pick options when picking different commits.

Point noted -- I shouldn't narrow down the various things I can do
with a single commit early on and lock us into a more restrictive
design.  However, I'm not in favor of making it too generic; I
certainly wouldn't like to edit an instruction sheet that looks like
this:

cherry-pick -m 1 -s -r 83a4fe9
revert -n 3a6fe42
cherry-pick -x --ff dacfe41
cherry-pick -s recursive -Xpatience b31d4e2

It'll become impossible to tell which options are disallowed over what
else, and it'll become a nightmare to debug when something goes wrong.
My idea is that we add commit-specific options in an optional
backward-compatible manner later:

pick 83a4fe9
revert 3a6fe42 # -n
pick dacfe41 # -s
pick b31d4e2

That way, there'll be two sets of options:

1. One "global" set of command-line switches that applies
to all the commits, which will be written to a command-specific
location.  The sequencer itself knows nothing about this.

2. Optional commit-specific stuff that's passed in the
form of a (modified) commit_list to the sequencer API to write to the
todo/ done files.

Do you like this idea?

> This would also make the different cherry-pick options available when using 
> rebase --interactive once it uses the sequencer.
> 
> > [1]:
> > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908
> > [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183 [3]:
> > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834
> 
> [3] is missing here.

Your email client is perhaps wrapping too aggressively? It's fine in
my original email [3].

-- Ram

[1]: http://thread.gmane.org/gmane.comp.version-control.git/142623/focus=142821
[2]: https://github.com/artagnon/git
[2]: http://article.gmane.org/gmane.comp.version-control.git/171255

Re: [RFC PATCH 00/11] Sequencer Foundations

[Daniel Barkalow]

On Mon, 11 Apr 2011, Christian Couder wrote:

> > 3. From the format of the TODO and DONE files, one more thing should
> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > format.  This part will go into the sequencer.  Then I'll use a
> > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > trying to work on Daniel's idea [3] from the very start.  Is this a
> > good idea?
> 
> I think that the TODO and DONE file format will need at one point to include 
> options and it is simpler if this change is done early. Using a cherry-pick 
> specific file to keep the options is not very generic for a sequencer that could 
> be used for many things.

My idea is that cherry-pick (or merge, or whatever) would have its own 
state to store what it was trying to do when it got a conflict. This is to 
enable the cherry-pick (etc) to complete without needing to know anything 
about what called it. The sequencer's information would keep track of 
what was left to do, what was already complete, and how to abort the whole 
sequence.

It would be nice to get the following commands to work:

$ git cherry-pick <sha1>
Automatic cherry-pick failed.  After resolving the conflicts,
mark the corrected paths with 'git add <paths>' or 'git rm <paths>' and 
run 'git continue'
$ (resolve conflicts)
$ git continue
Finished one cherry-pick.
[master sha1b] Subject
 2 files changes, 2 insertions(+)

And, of course, if this happened as a step in a rebase, the rebase would 
continue after the cherry-pick finished. But it's annoying that, in order 
to finish a conflicted "git cherry-pick <branch>", you currently need to 
go back and find the instruction that says to commit it yourself, with the 
option "-c <sha1>" to retain authorship and message. And if you want to 
abort it, you need to remember "git reset --hard HEAD" (and maybe you also 
want "git rerere clear"). That's what cherry-pick should be keeping its 
own state for. This is also parallel to how "merge" works, with its 
information kept in private state files, including some things like the 
list of files that had conflicts which isn't trivial to reproduce after 
the user has resolved them.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC PATCH 00/11] Sequencer Foundations

[Jonathan Nieder]

Hi,

Daniel Barkalow wrote:

> But it's annoying that, in order 
> to finish a conflicted "git cherry-pick <branch>", you currently need to 
> go back and find the instruction that says to commit it yourself, with the 
> option "-c <sha1>" to retain authorship and message.

You might like v1.7.5-rc0~88^2 (Teach commit about CHERRY_PICK_HEAD,
2011-02-19).

> And if you want to 
> abort it, you need to remember "git reset --hard HEAD" (and maybe you also 
> want "git rerere clear").

Hm, I had assumed reset --hard (or "git reset --merge HEAD") would
take care of the "rerere clear" but it seems that indeed it doesn't.

Re: [RFC PATCH 00/11] Sequencer Foundations

[Christian Couder]

Hi Ram,

On Monday 11 April 2011 06:49:05 Ramkumar Ramachandra wrote:
> Hi Christian,
> 
> Christian Couder writes:
> > On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
> > > Hi,
> > > 
> > > I've started working on building a sequencer for Git.
> > 
> > So you are starting the GSoC early! Great!
> > When (or before) it really starts, just make sure you put your work on a
> > public Git repository and you send status updates regularly (weekly if
> > possible).
> 
> Ofcourse.  I've already discussed many of these issues last year [1].
> The work corresponding to this particular series can be found in the
> 'sequencer' branch on my GitHub fork [2].  

Great! Thanks!

> Since the results haven't
> been announced, and the coding period hasn't begun, this work should
> be treated like "normal work" -- I just wrote it this weekend.

Ok.

> > > 3. From the format of the TODO and DONE files, one more thing should
> > > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > > format.  This part will go into the sequencer.  Then I'll use a
> > > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > > trying to work on Daniel's idea [3] from the very start.  Is this a
> > > good idea?
> > 
> > I think that the TODO and DONE file format will need at one point to
> > include options and it is simpler if this change is done early. Using a
> > cherry-pick specific file to keep the options is not very generic for a
> > sequencer that could be used for many things.
> > 
> > For example, as we have rebase --interactive, we will probably want to
> > have cherry-pick --interactive, and when editing the TODO file we might
> > want to use different cherry-pick options when picking different
> > commits.
> 
> Point noted -- I shouldn't narrow down the various things I can do
> with a single commit early on and lock us into a more restrictive
> design.  However, I'm not in favor of making it too generic; I
> certainly wouldn't like to edit an instruction sheet that looks like
> this:
> 
> cherry-pick -m 1 -s -r 83a4fe9
> revert -n 3a6fe42
> cherry-pick -x --ff dacfe41
> cherry-pick -s recursive -Xpatience b31d4e2

I wouldn't like either, but I would like it even less if it was like this:

pick 83a4fe9 # -m 1 -s -r 
revert 3a6fe42 # -n 
pick dacfe41 # -x --ff 
pick b31d4e2 # -s recursive -Xpatience 

I mean that of course people should not use too many options for no good 
reason, but if they do need to use some options, it's better if they are shown 
like in a shell, as they will be more familiar with them this way.

There is no point of making options look different to prevent people from 
abusing them.

> It'll become impossible to tell which options are disallowed over what
> else, and it'll become a nightmare to debug when something goes wrong.
> My idea is that we add commit-specific options in an optional
> backward-compatible manner later:
> 
> pick 83a4fe9
> revert 3a6fe42 # -n
> pick dacfe41 # -s
> pick b31d4e2
> 
> That way, there'll be two sets of options:
> 
> 1. One "global" set of command-line switches that applies
> to all the commits, which will be written to a command-specific
> location.  The sequencer itself knows nothing about this.

I don't see the point of this global set. And if the sequencer knows nothing 
about it, the user may not know about it too and so may not understand how 
things work.

> 2. Optional commit-specific stuff that's passed in the
> form of a (modified) commit_list to the sequencer API to write to the
> todo/ done files.
> 
> Do you like this idea?
> 
> > This would also make the different cherry-pick options available when
> > using rebase --interactive once it uses the sequencer.
> > 
> > > [1]:
> > > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170
> > > 908 [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183
> > > [3]:
> > > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=17
> > > 0834
> > 
> > [3] is missing here.
> 
> Your email client is perhaps wrapping too aggressively? It's fine in
> my original email [3].

Yeah, sorry about that!
Christian.

Re: [RFC PATCH 00/11] Sequencer Foundations

[Daniel Barkalow]

On Mon, 11 Apr 2011, Jonathan Nieder wrote:

> Hi,
> 
> Daniel Barkalow wrote:
> 
> > But it's annoying that, in order 
> > to finish a conflicted "git cherry-pick <branch>", you currently need to 
> > go back and find the instruction that says to commit it yourself, with the 
> > option "-c <sha1>" to retain authorship and message.
> 
> You might like v1.7.5-rc0~88^2 (Teach commit about CHERRY_PICK_HEAD,
> 2011-02-19).

Ah, so I'm actually proposing that we not revert that series, as far as 
having cherry-pick-specific state is concerned. :) On the other hand, I'd 
like to have the logic for using that state be in the cherry-pick 
implementation, rather than having commit.c need to understand merge and 
cherry-pick (and revert, and applying a patch from email, and...). Which 
is to say, there should be a core state file that ends up with 
"cherry-pick" in it, and revert.c has registered that it handles that, and 
commit.c should call the registered function to use the saved state. 

Also, git-rebase should be able to take advantage of the fact that other 
code knows how to start a cherry-pick, find that it has a conflict, tell 
the user, and use saved information to finish it.

> > And if you want to 
> > abort it, you need to remember "git reset --hard HEAD" (and maybe you also 
> > want "git rerere clear").
> 
> Hm, I had assumed reset --hard (or "git reset --merge HEAD") would
> take care of the "rerere clear" but it seems that indeed it doesn't.

In fact, Ram's series makes sure to call both, which is how I knew.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC PATCH 00/11] Sequencer Foundations

[Jonathan Nieder]

Daniel Barkalow wrote:

> My feeling is that "--skip" is actually "abort the pick, but continue the 
> rebase". I suppose there could be more than two levels, and people could 
> want to skip a higher-level chunk, but that's something to get to when 
> someone actually wants it

Ah, I think I see better now.  What was confusing was

	git cherry-pick foo..bar

for which --skip would mean "abort the pick, but continue the pick".
It fits very well into your story.  There are two levels: "atomic"
(for lack of a better word) operations, like a single cherry-pick, and
sequences of operations.

(As for the more-than-two-levels story, I want it, but I agree with
you that that is tangential to this project.  I find myself attempting
to go back and rearrange earlier commits during an interactive rebase
very often.)

Re: [RFC PATCH 00/11] Sequencer Foundations

[Ramkumar Ramachandra]

Hi Daniel,

Daniel Barkalow writes:
> > Please note that 10/11 is not related to this series, but seems to be
> > a minor nit that's required to make all existing tests pass.
> 
> That looks like an actual bug that only doesn't matter currently because 
> the function is never called with enough junk on the stack.

Fixing this bug was one is one of the hidden motives of the patch I
send out just afterward [1].

> > 0. Is the general flow alright?
> 
> I suspect it would be easier to review some of this with certain things 
> squashed together; one patch that changes all of the variable references 
> to what you want them to be is easier to understand than one that moves 
> statics to function arguments, one that moves statics to struct fields, 
> etc. Likewise, when you're converting some of the die() calls to error(), 
> it's easier to review the patch if all of the die() calls that aren't 
> changed in that patch don't get changed later in the series.

Agreed -- I'll post a reworked series shortly.  Jonathan was finding
it hard to follow as well.

> > 1. Is it okay to use this kind of adaptive error handling (calling
> > 'die' in some places and returning error in other places), or should
> > it be more uniform?
> 
> I think it should be systematic but not necessarily uniform. You should be 
> able to give a guideline as to how to decide which to use (and you should 
> probably actually give the guideline, so future developers make consistent 
> choices). I think of "die" as being ideally for situations where the 
> program can't understand what has happened well enough to know what to do 
> about it.

Jonathan concurs.  Where should I document this?

> > 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> > inelegant, mainly because of the command-line argument parsing
> > overhead.  Re-implementing it using more low-level functions doesn't
> > seem to be the way to go either: for example, 'reset --hard' has some
> > additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> > duplicate.  Should I work on reworking parts of 'rerere.c' and
> > 'revert.c', or is there some other way?
> 
> (ITYM cmd_reset here)

Yeah, sorry.

> I think rerere.c should get a rerere_clear(). I think it would make sense 
> to implement the reset locally; the abort ought to be undoing exactly 
> those things that you did, and I'm not actually sure the ORIG_HEAD is 
> entirely appropriate. You ought to be able to use cleanup functions that 
> correspond to the functions you used to make the mess in the first place.

Good suggestion -- rerere_clear is done [2]; now I have to figure out
if it makes sense to write a library for reset.

-- Ram

[1]: http://article.gmane.org/gmane.comp.version-control.git/171267
[2]: http://article.gmane.org/gmane.comp.version-control.git/171314

Re: [PATCH 02/11] revert: Lose global variables "commit" and "me"

[Ramkumar Ramachandra]

Hi Christian,

Christian Couder writes:
> On Sunday 10 April 2011 17:11:48 Ramkumar Ramachandra wrote:
> > @@ -546,10 +547,13 @@ static int prepare_revs(struct rev_info *revs)
> >  	return 0;
> >  }
> > 
> > -static int read_and_refresh_cache(const char *me)
> > +static int read_and_refresh_cache(void)
> >  {
> >  	static struct lock_file index_lock;
> >  	int index_fd = hold_locked_index(&index_lock, 0);
> > +	const char *me;
> > +
> > +	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
> 
> It looks like this patch will not compile as cmd_opts is introduced in patch 
> 03/11.

Rebase fail.  Thanks for pointing it out.  I'll add some features and
rework the series shortly.

-- Ram

Re: [RFC PATCH 00/11] Sequencer Foundations

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> > 0. Is the general flow alright?
> 
> Not sure --- I don't have the big picture.  Could you give a quick
> summary of the flow in the cover letter ("patch 1 does such-and-such,
> so patch 2 can do such-and-such, leading to...") to the next revision,
> and quick explanations of the purpose (i.e., why we should want each
> change) in the individual change descriptions?

Agreed.  I'll ask again after the next re-roll.

> > 1. Is it okay to use this kind of adaptive error handling (calling
> > 'die' in some places and returning error in other places), or should
> > it be more uniform?
> 
> 'die' gets used in two ways (well, one way really):
> 
>  - to say "there is no sane way to recover from this failure".  For
>    example, xmalloc dies if even after trying to free up memory,
>    malloc still could not satisfy the request.
> 
>  - to say "so far we've been too lazy to implement recovery from
>    this failure".  Or "while we *could* recover from this failure, no
>    one has needed it, so let's not --- that code would just bitrot."
> 
> Therefore a mixture of 'die' and 'return error' seems inevitable.  The
> dangerous mixtures to avoid are those likely to trip up callers (e.g.,
> if all code paths 'return error' except one edge case).

My thoughts precisely.  I was worried about what would happen in
future when Git is completely lib'ified, but I suppose it makes little
sense to think about that now.

> > 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> > inelegant, mainly because of the command-line argument parsing
> > overhead.  Re-implementing it using more low-level functions doesn't
> > seem to be the way to go either: for example, 'reset --hard' has some
> > additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> > duplicate.  Should I work on reworking parts of 'rerere.c' and
> > 'revert.c', or is there some other way?
> 
> See "git log --grep=libify" for examples.  Isn't rerere already
> libified?  Ah, you need "rerere clear" --- I think a rerere_clear
> function alongside rerere_forget et al would make sense.

Done in [1].

> More generally, it should be feasible to expose a nice, simple API for
> any functionality you need (with params struct if necessary, etc).
> That's how many of the current APIs (revision walking, diffcore, ...)
> came about.

I see.  Okay, I'll see if it makes sense to craft an API for rebase.

> > 3. From the format of the TODO and DONE files, one more thing should
> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > format.  This part will go into the sequencer.  Then I'll use a
> > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > trying to work on Daniel's idea [3] from the very start.  Is this a
> > good idea?
> 
> This is still bouncing in my head.  I think I like it --- is the idea
> that some day you could put commands like
> 
> 	am topic.mbox
> 
> in your insn sheet, or do nested rebases with a --force-nested option?
> That does sound useful.  How would one request "skip to the next
> operation in the outer rebase" on the command line?  This is starting
> to feel like a debugger.

I'll respond to this later in the thread, since Daniel has already
said something.  I just need help with crafting a nice instruction
sheet now -- please join the discussion at [2].

> > 4. I have a feeling that I've broken translation strings.  Is there a
> > README, plus a bunch of tests I can run to make sure that I've not
> > broken anything?
> 
> If you put "GETTEXT_POISON = YesPlease" in your config.mak, the
> translations will be translated to gibberish when the GIT_GETTEXT_POISON
> envvar is set, so you can use the test suite as a sanity check.
> "make pot" can be used to get the translation template that
> translators will see.
> 
> As for a readme explaining how to use _, N_, and Q_, yes, I think that
> would be useful.  I think Ævar's series has something like that (but
> targetted at translators) later on; it might make sense to prod him or
> me for a simpler explanation can be merged immediately.  Until then,
> there is "git log gettext.h".

Thanks!  Someone should really document this whole translations thing.

-- Ram

[1]: http://article.gmane.org/gmane.comp.version-control.git/171314
[2]: http://thread.gmane.org/gmane.comp.version-control.git/171255/focus=171307

Re: [RFC PATCH 00/11] Sequencer Foundations

[Ramkumar Ramachandra]

Hi Christian,

Christian Couder writes:
> On Monday 11 April 2011 06:49:05 Ramkumar Ramachandra wrote:
> > > > 3. From the format of the TODO and DONE files, one more thing should
> > > > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > > > format.  This part will go into the sequencer.  Then I'll use a
> > > > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > > > trying to work on Daniel's idea [3] from the very start.  Is this a
> > > > good idea?
> > > 
> > > I think that the TODO and DONE file format will need at one point to
> > > include options and it is simpler if this change is done early. Using a
> > > cherry-pick specific file to keep the options is not very generic for a
> > > sequencer that could be used for many things.
> > > 
> > > For example, as we have rebase --interactive, we will probably want to
> > > have cherry-pick --interactive, and when editing the TODO file we might
> > > want to use different cherry-pick options when picking different
> > > commits.
> > 
> > Point noted -- I shouldn't narrow down the various things I can do
> > with a single commit early on and lock us into a more restrictive
> > design.  However, I'm not in favor of making it too generic; I
> > certainly wouldn't like to edit an instruction sheet that looks like
> > this:
> > 
> > cherry-pick -m 1 -s -r 83a4fe9
> > revert -n 3a6fe42
> > cherry-pick -x --ff dacfe41
> > cherry-pick -s recursive -Xpatience b31d4e2
> 
> I wouldn't like either, but I would like it even less if it was like this:
> 
> pick 83a4fe9 # -m 1 -s -r 
> revert 3a6fe42 # -n 
> pick dacfe41 # -x --ff 
> pick b31d4e2 # -s recursive -Xpatience 
> 
> I mean that of course people should not use too many options for no good 
> reason, but if they do need to use some options, it's better if they are shown 
> like in a shell, as they will be more familiar with them this way.
> 
> There is no point of making options look different to prevent people from 
> abusing them.

You're right -- no point obscuring things.  I can't help thinking that
there must be a more elegant representation.  I'll think about it for
a while, and try to come up with something.

> > It'll become impossible to tell which options are disallowed over what
> > else, and it'll become a nightmare to debug when something goes wrong.
> > My idea is that we add commit-specific options in an optional
> > backward-compatible manner later:
> > 
> > pick 83a4fe9
> > revert 3a6fe42 # -n
> > pick dacfe41 # -s
> > pick b31d4e2
> > 
> > That way, there'll be two sets of options:
> > 
> > 1. One "global" set of command-line switches that applies
> > to all the commits, which will be written to a command-specific
> > location.  The sequencer itself knows nothing about this.
> 
> I don't see the point of this global set. And if the sequencer knows nothing 
> about it, the user may not know about it too and so may not understand how 
> things work.

Hm.  I originally wanted this so that each commit in the instruction
sheet isn't polluted with the same command-line options, but this
doesn't seem to be a good solution.

-- Ram

Re: [PATCH 01/11] revert: Avoid calling die; return error instead

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

You would need to write a lot more than that to justify why this is a good
change and does not regress the existing codepaths.  The above Subject:
implies as if all you did was to replace "die()" with "return error()",
but I am sure that you would also have audited all the existing callers of
the affected codepaths and either they already handled an error return
correctly by dying or exiting with non-zero status, or you adjusted them
to expect an error return and exit with 129 in this patch.

Also we know from the context of this post that you are planning to add
new callsites to some of the functions that are converted to give an error
return with this patch, but it is nevertheless a good idea to briefly
mention that (just "the codepath to implement new nitfol feature will be
making calls to xyzzy and frotz and it does not want these to die; rather
it wants to handle error cases itself" would do).

> @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	    (write_cache(index_fd, active_cache, active_nr) ||
>  	     commit_locked_index(&index_lock)))
>  		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> -		die(_("%s: Unable to write new index file"), me);
> +		return error(_("%s: Unable to write new index file"), me);
>  	rollback_lock_file(&index_lock);

Do the callers rollback the lockfile in their error return codepaths now?
Should they?  If not why not?

One acceptable answer is "the only thing the current callers do in their
error codepaths is to exit(129), and that will roll it back for us", but
then that might mean this patch made the API more error prone to use when
the next callsite you add wants to do more than just exitting.

> @@ -397,18 +397,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();

Likewise for this "discard-cache".  Should it be the responsibility to the
caller to discard the in-core cache when they handle an error return and
possibly take an alternative action, or should this function be the one to
do so for them?

Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Again, "In later steps, a new API that takes a single commit and replays
it in forward (cherry-pick) or backward (revert) direction will be
introduced, and will take this structure as a parameter to tell it what to
do" is missing from the above description.

More importantly, the primary purpose of these variables is _not_ "to
parse command line options into".  It is to actively affect what happens
in the code, and "parse command line" is merely a way to assign the
initial values to them.  So I'd rather see this patch described perhaps
like this:

    cherry-pick/revert: introduce "struct replay_options"

    The current code uses a set of file-scope static variables to instruct
    the cherry-pick/revert machinery how to replay the changes, and
    initialises them by parsing the command line arguments. In later steps
    in this series, we would like to introduce an API function that calls
    into this machinery directly and have a way to tell it what to do.

    Introduce a structure to group these variables, so that the API can
    take them as a single "replay_options" parameter.

I strongly prefer to see this patch also update the callchain to pass a
pointer to the options struct as parameter.  I can guess without reading
the rest the series that at some later step you would do that, but I think
it makes more sense to do the conversion at this step, as you will be
touching lines that use the global variables in this patch anyway, like
this:

-	return action == REVERT ? revert_usage : cherry_pick_usage;
+	return cmd_opts.action == REVERT ? revert_usage : cherry_pick_usage;

which should eventually become something like:

	return opts->usage_message;

at the very last step when this codepath is _fully_ libified (I don't know
if the final result of your series still is a "choose only from these two
canned messages" API, or gives the ability to the caller to specify the
usage message---I am just showing how it should look like at the end of a
full libification).  So it would make sense to see:

	return opts->action == REVERT ? revert_usage : cherry_pick_usage;

in this patch.

> @@ -268,17 +278,17 @@ static struct tree *empty_tree(void)
>  static int error_dirty_index()

It is probably a remnant of the earlier patches in this series, but this
should start with:

	static int error_dirty_index(void)

Of course, you will actually be passing the options structure, so it would
become:

	static int error_dirty_index(struct replay_options *opts)
        {
        	...
                if (opts->action == REVERT)
                	...
	}

Re: [PATCH 05/11] revert: Catch incompatible command-line options early

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +static void die_opt_incompatible(const char *me, const char *base_opt,
> +				int nargs, int opt_bitarray[], ...)
> +{

This feels way overkill to use vararg.  This may be _one_ way to do what
you want to do, and while I don't think it is the best way, it may be Ok.

"opt_bitarray[]" is a horrible name, though.  It does not convey what
the variable is about (its "purpose").  It doesn't even correctly describe
the way that unspecified purpose happens to be implemented (claims to be
bitarray but it is just an array of ints and what matters is if any bit is
set in the array).

>  static void parse_args(int argc, const char **argv)
>  {
>  	const char * const * usage_str = revert_or_cherry_pick_usage();
> @@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)
>  	if (cmd_opts.commit_argc < 2)
>  		usage_with_options(usage_str, options);
>  
> +	if (cmd_opts.allow_ff) {
> +		int opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,
> +				      cmd_opts.no_replay, cmd_opts.edit};
> +		die_opt_incompatible(me, "--ff", 4, opt_bitarray, "--signoff",
> +				"--no-commit", "-x", "--edit");
> +	}

Why not do it like this instead?

	struct incompatible {
        	unsigned option_bit;
                const char *option_name;
	} incompatible[] = {
		{ opts->signoff, "--signoff" },
                { opts->no_commit, "--no-commit" },
                ...
	};
	verify_compatible("me", "--ff", incompatible, ARRAY_SIZE(incompatible));

Or if you are shooting for ease-of-use, it might make sense to do it like
this:

	verify_compatible("me", "--ff",
        		"--signoff", opts->signoff,
                        "--no-commit", opts->no_commit,
                        ...
                        NULL);

and make verify_compatible() a varargs function that takes two optional
arguments at a time, i.e. const char *, followed by an int.  Then there is
no need for extra "int opt_bitarray[]" or "struct incompatible".

That would justify use of varargs, I think.

Re: [PATCH 05/11] revert: Catch incompatible command-line options early

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> > @@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)
> >  	if (cmd_opts.commit_argc < 2)
> >  		usage_with_options(usage_str, options);
> >  
> > +	if (cmd_opts.allow_ff) {
> > +		int opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,
> > +				      cmd_opts.no_replay, cmd_opts.edit};
> > +		die_opt_incompatible(me, "--ff", 4, opt_bitarray, "--signoff",
> > +				"--no-commit", "-x", "--edit");
> > +	}
> 
> Why not do it like this instead?
> 
> 	struct incompatible {
>         	unsigned option_bit;
>                 const char *option_name;
> 	} incompatible[] = {
> 		{ opts->signoff, "--signoff" },
>                 { opts->no_commit, "--no-commit" },
>                 ...
> 	};
> 	verify_compatible("me", "--ff", incompatible, ARRAY_SIZE(incompatible));
> 
> Or if you are shooting for ease-of-use, it might make sense to do it like
> this:
> 
> 	verify_compatible("me", "--ff",
>         		"--signoff", opts->signoff,
>                         "--no-commit", opts->no_commit,
>                         ...
>                         NULL);
> 
> and make verify_compatible() a varargs function that takes two optional
> arguments at a time, i.e. const char *, followed by an int.  Then there is
> no need for extra "int opt_bitarray[]" or "struct incompatible".
> 
> That would justify use of varargs, I think.

Now that you point it out, my original approach was unnecessarily
cryptic and convoluted.  I've followed this approach in my new series,
and kept varargs -- the code looks much prettier now.  Thanks :)

I can't justify changing the name from "die_opt_incompatible" to
"verify_compatible" though; the name I've chosen seems to be more
appropriate/ descriptive.  Further, I think command-line parsing
should always be the toplevel caller, and "die" is therefore more
appropriate than "return error" here.

-- Ram

Re: [PATCH 01/11] revert: Avoid calling die; return error instead

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> > [Subject: revert: Avoid calling die; return error instead]
> >
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> 
> Presumably this is because the sequencer is going to pick up after the
> error and clean up a little (why doesn't the change description say
> so?).  Will it be resuming after that or just performing a little
> cleanup before the exit?

I didn't write commit messages for any of the patches in the previous
round -- I just wanted to show the idea quickly.  Anyway, it's fixed
in the next round (coming soon).

> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -265,23 +265,23 @@ 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);
> 
> Won't that exit?  

Fixed.

> >  	} else {
> 
> This "else" could be removed (decreasing the indent of the rest by
> one tab stop) since the "if" case has already returned or exited.
> Not the subject of this patch, just an idea for earlier or later. ;-)

Fixed.

> [...]
> > @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> >  	    (write_cache(index_fd, active_cache, active_nr) ||
> >  	     commit_locked_index(&index_lock)))
> >  		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> > -		die(_("%s: Unable to write new index file"), me);
> > +		return error(_("%s: Unable to write new index file"), me);
> >  	rollback_lock_file(&index_lock);
> 
> What happens to index_lock in the error case?

Fixed.

> [...]
> > @@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)
> >  		revs->reverse = 1;
> >  
> >  	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
> > -	if (argc > 1)
> > -		usage(*revert_or_cherry_pick_usage());
> > +	if (argc > 1) {
> > +		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
> > +		return 129;
> > +	}
> 
> Yuck.  Maybe the error can be returned to the caller somehow, but
> that seems somehow ambitious given that setup_revisions has all sorts
> of ways to die anyway.
> 
> So you are bending the assumptions of many existing git functions (in
> a good way).
> 
> I can think of at least three ways to go:
> 
>  1) Come up with a convention to give more information about the nature
>     of returned errors in the functions you are touching.  For
>     example, make sure errno is valid after the relevant functions, or
>     use multiple negative values to express the nature of the error.
> 
>     So a caller could do:
> 
> 	if (prepare_revs(...)) {
> 		if (errno == EINVAL)
> 			usage(*revert_or_cherry_pick_usage());
> 		die("BUG: unexpected error from prepare_revs");
> 	}
> 
>     Or:
> 
>  2) Use set_die_routine or sigchain_push + atexit to declare what cleanup
>     has to happen before exiting.  Keep using die().
> 
>  3) Provide a new facility to register cleanup handlers that will free
>     resources and otherwise return to a consistent state before
>     unwinding the stack.  This way, you'd still have to audit die()
>     calls to look for missing cleanup handlers, but they could stay as
>     die() rather than changing to "return error" and the worried
>     caller could use
> 
> 	set_die_routine(longjmp_to_here);
> 
>     to keep git alive.  I don't suggest doing this.  It is a pain to
>     get right and not obviously cleaner than "return error", and some
>     errors really are unrecoverable (rather than just being a symptom
>     of programmers to lazy to write error recovery code :)).

Hm, I'm a little confused about error handling now.  I'll defer this
part until my patches naturally establish some convention for error
handling -- designing one in advance isn't as easy as I thought.

Finally, thanks for the review!  I'll look forward to more reviews as
I post more iterations of the series.

-- Ram

Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> 
> Again, "In later steps, a new API that takes a single commit and replays
> it in forward (cherry-pick) or backward (revert) direction will be
> introduced, and will take this structure as a parameter to tell it what to
> do" is missing from the above description.
> 
> More importantly, the primary purpose of these variables is _not_ "to
> parse command line options into".  It is to actively affect what happens
> in the code, and "parse command line" is merely a way to assign the
> initial values to them.  So I'd rather see this patch described perhaps
> like this:
> 
>     cherry-pick/revert: introduce "struct replay_options"
> 
>     The current code uses a set of file-scope static variables to instruct
>     the cherry-pick/revert machinery how to replay the changes, and
>     initialises them by parsing the command line arguments. In later steps
>     in this series, we would like to introduce an API function that calls
>     into this machinery directly and have a way to tell it what to do.
> 
>     Introduce a structure to group these variables, so that the API can
>     take them as a single "replay_options" parameter.

Right.  Thanks for the nice commit message :)

> I strongly prefer to see this patch also update the callchain to pass a
> pointer to the options struct as parameter.  I can guess without reading
> the rest the series that at some later step you would do that, but I think
> it makes more sense to do the conversion at this step, as you will be
> touching lines that use the global variables in this patch anyway, like
> this:

Good idea.  I've passed the opts pointer around in the new series.

> > @@ -268,17 +278,17 @@ static struct tree *empty_tree(void)
> >  static int error_dirty_index()
> 
> It is probably a remnant of the earlier patches in this series, but this
> should start with:
> 
> 	static int error_dirty_index(void)
> 
> Of course, you will actually be passing the options structure, so it would
> become:
> 
> 	static int error_dirty_index(struct replay_options *opts)
>         {
>         	...
>                 if (opts->action == REVERT)
>                 	...
> 	}

The very first patch removes this function (and puts the functionality
elsewhere) in the new series, so this comment doesn't apply.

Thanks for the review!

-- Ram

Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -35,17 +35,27 @@ 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 struct {
> > +	enum { REVERT, CHERRY_PICK } action;
> 
> That would require giving this struct a name, so it can be passed
> around.  Not a bad idea anyway imho since then a person reading
> top-to-bottom is not left in suspense:
> 
>  struct cherry_pick_opts {
> 	enum { REVERT, CHERRY_PICK } action;
> 
> 	unsigned edit:1;
> 	unsigned no_replay:1;
> 	...
>  };

Thanks.  A couple of nits:
1. You can't take the address of a bitfield, so 'edit' and 'replay'
   can't be passed to the command line argument parsing framework
   directly.
2. GCC throws a "warning: useless storage class specifier in empty
   declaration" for this kind of declaration.

-- Ram