[PATCH v3 00/10] Sequencer Foundations

[PATCH v3 00/10] Sequencer Foundations

[Ramkumar Ramachandra]

Hi,

Error handling is a very complex topic, and trying to build it up with
the Sequencer series had disasterous consequences, as seen in the
previous iteration of the series.  In this iteration, I've nearly
re-written everything from scratch, and tried to get the basics right,
without introducing anything new.  Time is short, and I'll have to
work very hard if I want to target "master",

I hope there are no major issues with this iteration.

Thanks for reading.

-- Ram

Ramkumar Ramachandra (10):
  advice: Introduce error_resolve_conflict
  revert: Propogate errors upwards from do_pick_commit
  revert: Eliminate global "commit" variable
  revert: Rename no_replay to record_origin
  revert: Introduce struct to keep command-line options
  revert: Separate cmdline parsing from functional code
  revert: Catch incompatible command-line options early
  revert: Introduce HEAD, TODO files to persist state, plan
  revert: Implement parsing --continue, --abort and --skip
  revert: Implement --abort processing

 advice.c         |   17 ++-
 advice.h         |    1 +
 builtin/revert.c |  484 +++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 361 insertions(+), 141 deletions(-)

-- 
1.7.5.GIT

[PATCH 01/10] advice: Introduce error_resolve_conflict

[Ramkumar Ramachandra]

Introduce error_resolve_conflict corresponding to
die_resolve_conflict, and implement the latter function in terms of
the former.

Based-on-patch-by: Christian Couder <chistian.couder@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 advice.c |   17 +++++++++++------
 advice.h |    1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/advice.c b/advice.c
index 0be4b5f..80aaa14 100644
--- a/advice.c
+++ b/advice.c
@@ -34,16 +34,21 @@ int git_default_advice_config(const char *var, const char *value)
 	return 0;
 }
 
-void NORETURN die_resolve_conflict(const char *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.
 		 */
-		die("'%s' is not possible because you have unmerged files.\n"
-		    "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
-		    "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
-	else
-		die("'%s' is not possible because you have unmerged files.", me);
+		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);
+	return error("'%s' is not possible because you have unmerged files.", me);
+}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	error_resolve_conflict(me);
+	exit(128);
 }
diff --git a/advice.h b/advice.h
index 3244ebb..f537366 100644
--- a/advice.h
+++ b/advice.h
@@ -12,6 +12,7 @@ extern int advice_detached_head;
 
 int git_default_advice_config(const char *var, const char *value);
 
+int error_resolve_conflict(const char *me);
 extern void NORETURN die_resolve_conflict(const char *me);
 
 #endif /* ADVICE_H */
-- 
1.7.5.GIT

[PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Introduce error_dirty_index, based on die_dirty_index which prints
some hints, and returns an error to be handled by its caller,
do_pick_commit.  do_pick_commit, in turn, propogates the error
upwards, and the error is finally handled by cmd_cherry_pick and
cmd_revert.  Set a simple convention for error-handling as follows:
positive return values from do_pick_commit indicate conflicts, while
negative ones indicate other errors.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..523d41a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -266,25 +266,15 @@ 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"
-					  "Please, commit your changes or stash them to proceed."));
-			else
-				die(_("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"));
-			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"));
-		}
-	}
+	if (read_cache_unmerged())
+		return error_resolve_conflict(me);
+
+	int ret = error(_("Your local changes would be overwritten by %s.\n"), me);
+	if (advice_commit_before_merge)
+		advise(_("Please, commit your changes or stash them to proceed."));
+	return ret;
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -398,18 +388,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) {
@@ -418,20 +408,20 @@ static int do_pick_commit(void)
 		struct commit_list *p;
 
 		if (!mainline)
-			die(_("Commit %s is a merge but no -m option was given."),
-			    sha1_to_hex(commit->object.sha1));
+			return error(_("Commit %s is a merge but no -m option was given."),
+				sha1_to_hex(commit->object.sha1));
 
 		for (cnt = 1, p = commit->parents;
 		     cnt != mainline && p;
 		     cnt++)
 			p = p->next;
 		if (cnt != mainline || !p)
-			die(_("Commit %s does not have parent %d"),
-			    sha1_to_hex(commit->object.sha1), mainline);
+			return error(_("Commit %s does not have parent %d"),
+				sha1_to_hex(commit->object.sha1), mainline);
 		parent = p->item;
 	} else if (0 < mainline)
-		die(_("Mainline was specified but commit %s is not a merge."),
-		    sha1_to_hex(commit->object.sha1));
+		return error(_("Mainline was specified but commit %s is not a merge."),
+			sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
@@ -441,12 +431,12 @@ 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"),
-		    me, sha1_to_hex(parent->object.sha1));
+		return error(_("%s: cannot parse parent commit %s"),
+			me, sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit->buffer, &msg) != 0)
-		die(_("Cannot get commit message for %s"),
-				sha1_to_hex(commit->object.sha1));
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
 
 	/*
 	 * "commit" is an existing commit.  We would want to apply
@@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
+	int res = 0;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res > 0)
+		/* Exit status from conflict */
+		return res;
+	if (res < 0)
+		/* Other error */
+		exit(128);
+	return 0;
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
+	int res = 0;
 	action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res > 0)
+		return res;
+	if (res < 0)
+		exit(128);
+	return 0;
 }
-- 
1.7.5.GIT

Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +static int error_dirty_index(const char *me)
>  {
> +	if (read_cache_unmerged())
> +		return error_resolve_conflict(me);
> +
> +	int ret = error(_("Your local changes would be overwritten by %s.\n"), me);
> +	if (advice_commit_before_merge)
> +		advise(_("Please, commit your changes or stash them to proceed."));
> +	return ret;
>  }

I like this rewrite whose result is short-and-sweet, but you do not even
need the "ret" variable. error() always yields -1, no?

> @@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  
>  int cmd_revert(int argc, const char **argv, const char *prefix)
>  {
> +	int res = 0;
>  	if (isatty(0))
>  		edit = 1;
>  	action = REVERT;
> -	return revert_or_cherry_pick(argc, argv);
> +	res = revert_or_cherry_pick(argc, argv);
> +	if (res > 0)
> +		/* Exit status from conflict */
> +		return res;
> +	if (res < 0)
> +		/* Other error */
> +		exit(128);
> +	return 0;
>  }
>  
>  int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>  {
> +	int res = 0;
>  	action = CHERRY_PICK;
> -	return revert_or_cherry_pick(argc, argv);
> +	res = revert_or_cherry_pick(argc, argv);
> +	if (res > 0)
> +		return res;
> +	if (res < 0)
> +		exit(128);
> +	return 0;
>  }

This hunk is dubious.

 - Why initialize res to zero if it always is assigned the return value of
   revert_or_cherry_pick() before it is used?

 - The called function seems to return errors from various places but as
   far as I see they are all return value of error(), so it would be
   equivalent to

	if (r_o_c_p(...))
		exit(128);
	return 0;

If you are going to introduce different return values from r-o-c-p() in a
later patch, these functions should be updated in that patch, I think.

Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> +static int error_dirty_index(const char *me)
>>  {
>> +     if (read_cache_unmerged())
>> +             return error_resolve_conflict(me);
>> +
>> +     int ret = error(_("Your local changes would be overwritten by %s.\n"), me);
>> +     if (advice_commit_before_merge)
>> +             advise(_("Please, commit your changes or stash them to proceed."));
>> +     return ret;
>>  }
>
> I like this rewrite whose result is short-and-sweet, but you do not even
> need the "ret" variable. error() always yields -1, no?

Okay; I didn't do this in the first place because I thought it would
be inelegant to hardcode '-1'. Fixed anyway.

>> @@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>>
>>  int cmd_revert(int argc, const char **argv, const char *prefix)
>>  {
>> +     int res = 0;
>>       if (isatty(0))
>>               edit = 1;
>>       action = REVERT;
>> -     return revert_or_cherry_pick(argc, argv);
>> +     res = revert_or_cherry_pick(argc, argv);
>> +     if (res > 0)
>> +             /* Exit status from conflict */
>> +             return res;
>> +     if (res < 0)
>> +             /* Other error */
>> +             exit(128);
>> +     return 0;
>>  }
>>
>>  int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>>  {
>> +     int res = 0;
>>       action = CHERRY_PICK;
>> -     return revert_or_cherry_pick(argc, argv);
>> +     res = revert_or_cherry_pick(argc, argv);
>> +     if (res > 0)
>> +             return res;
>> +     if (res < 0)
>> +             exit(128);
>> +     return 0;
>>  }
>
> This hunk is dubious.
>
>  - Why initialize res to zero if it always is assigned the return value of
>   revert_or_cherry_pick() before it is used?

Okay. Fixed.

>  - The called function seems to return errors from various places but as
>   far as I see they are all return value of error(), so it would be
>   equivalent to
>
>        if (r_o_c_p(...))
>                exit(128);
>        return 0;
>
> If you are going to introduce different return values from r-o-c-p() in a
> later patch, these functions should be updated in that patch, I think.

revert_or_cherry_pick *does* return different values in this patch! As
I've pointed out in the comment, positive exit status indicates a
conflict, while a negative one indicates an error. To prove to myself
that this is case, I applied this diff temporarily and ran all tests
-- and viola, t3505-cherry-pick-empty.sh broke. Is there something I'm
not understanding correctly?

Thanks for the review.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 523d41a..9c7921b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -584,28 +584,22 @@ static int revert_or_cherry_pick(int argc, const
char **argv)

 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
-	int res = 0;
+	int res;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
 	res = revert_or_cherry_pick(argc, argv);
-	if (res > 0)
-		/* Exit status from conflict */
-		return res;
-	if (res < 0)
-		/* Other error */
+	if (res)
 		exit(128);
 	return 0;
 }

 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-	int res = 0;
+	int res;
 	action = CHERRY_PICK;
 	res = revert_or_cherry_pick(argc, argv);
-	if (res > 0)
-		return res;
-	if (res < 0)
+	if (res)
 		exit(128);
 	return 0;
 }

[PATCH 03/10] revert: Eliminate global "commit" variable

[Ramkumar Ramachandra]

Since we want to develop the functionality to either pick/ revert
individual commits atomically later in the series, make "commit" a
local variable.  Modify the get_message and add_message_to_msg API to
take an additional parameter: the pre-computed SHA1 character string
to use.  When reporting a program bug in get_encoding, don't include
the commit in the message.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 523d41a..6c485f6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
 
 static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
-static struct commit *commit;
 static int commit_argc;
 static const char **commit_argv;
 static int allow_rerere_auto;
@@ -116,11 +115,12 @@ struct commit_message {
 	const char *message;
 };
 
-static int get_message(const char *raw_message, struct commit_message *out)
+static int get_message(const char *sha1_abbrev, const char *raw_message,
+		struct commit_message *out)
 {
 	const char *encoding;
-	const char *abbrev, *subject;
-	int abbrev_len, subject_len;
+	const char *subject;
+	int subject_len;
 	char *q;
 
 	if (!raw_message)
@@ -139,17 +139,14 @@ static int get_message(const char *raw_message, struct commit_message *out)
 	if (out->reencoded_message)
 		out->message = out->reencoded_message;
 
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
-
 	subject_len = find_commit_subject(out->message, &subject);
 
-	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+	out->parent_label = xmalloc(strlen("parent of ") + DEFAULT_ABBREV +
 			      strlen("... ") + subject_len + 1);
 	q = out->parent_label;
 	q = mempcpy(q, "parent of ", strlen("parent of "));
 	out->label = q;
-	q = mempcpy(q, abbrev, abbrev_len);
+	q = mempcpy(q, sha1_abbrev, DEFAULT_ABBREV);
 	q = mempcpy(q, "... ", strlen("... "));
 	out->subject = q;
 	q = mempcpy(q, subject, subject_len);
@@ -168,8 +165,7 @@ static char *get_encoding(const char *message)
 	const char *p = message, *eol;
 
 	if (!p)
-		die (_("Could not read commit message of %s"),
-				sha1_to_hex(commit->object.sha1));
+		die (_("BUG: get_encoding() called with NULL"));
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
@@ -185,25 +181,26 @@ 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(const char *fallback, struct strbuf *msgbuf,
+			const char *message)
 {
 	const char *p = message;
 	while (*p && (*p != '\n' || p[1] != '\n'))
 		p++;
 
 	if (!*p)
-		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
+		strbuf_addstr(msgbuf, fallback);
 
 	p += 2;
 	strbuf_addstr(msgbuf, p);
 }
 
-static void write_cherry_pick_head(void)
+static void write_cherry_pick_head(const char *commit_sha1_hex)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
 
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+	strbuf_addf(&buf, "%s\n", commit_sha1_hex);
 
 	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
@@ -370,7 +367,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;
@@ -378,6 +375,7 @@ static int do_pick_commit(void)
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
+	const char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
 	int res;
 
 	if (no_commit) {
@@ -434,7 +432,7 @@ static int do_pick_commit(void)
 		return error(_("%s: cannot parse parent commit %s"),
 			me, sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit->buffer, &msg) != 0)
+	if (get_message(sha1_abbrev, commit->buffer, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
 			sha1_to_hex(commit->object.sha1));
 
@@ -463,18 +461,19 @@ static int do_pick_commit(void)
 		}
 		strbuf_addstr(&msgbuf, ".\n");
 	} else {
+		const char *commit_sha1_hex = sha1_to_hex(commit->object.sha1);
 		base = parent;
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
-		add_message_to_msg(&msgbuf, msg.message);
+		add_message_to_msg(commit_sha1_hex, &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_sha1_hex);
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -552,6 +551,7 @@ static void read_and_refresh_cache(const char *me)
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	struct rev_info revs;
+	struct commit *commit;
 
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -574,7 +574,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	prepare_revs(&revs);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit();
+		int res = do_pick_commit(commit);
 		if (res)
 			return res;
 	}
-- 
1.7.5.GIT

Re: [PATCH 03/10] revert: Eliminate global "commit" variable

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 523d41a..6c485f6 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
>  
>  static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
>  static enum { REVERT, CHERRY_PICK } action;
> -static struct commit *commit;

I agree that the stated goal of this change is a worthy one.

> @@ -116,11 +115,12 @@ struct commit_message {
>  	const char *message;
>  };
>  
> -static int get_message(const char *raw_message, struct commit_message *out)
> +static int get_message(const char *sha1_abbrev, const char *raw_message,
> +		struct commit_message *out)
>  {

This somehow feels dubious and also goes against the "let's not rely on
global variable commit", no?

This function relied on the global variable and also got information
passed from the caller that this function could have derived from that
global commit object (i.e. raw_message came from commit->buffer).

A logical thing to fix that situation would be to make its signature:

    static int get_message(struct commit *commit, struct commit_message *out)

no?

Especially dubious is this part:

> @@ -139,17 +139,14 @@ static int get_message(const char *raw_message, struct commit_message *out)
>  	if (out->reencoded_message)
>  		out->message = out->reencoded_message;
>  
> -	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
> -	abbrev_len = strlen(abbrev);
> -
>  	subject_len = find_commit_subject(out->message, &subject);
>  
> -	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
> +	out->parent_label = xmalloc(strlen("parent of ") + DEFAULT_ABBREV +
>  			      strlen("... ") + subject_len + 1);

You moved the call to f-u-a() to the caller, and that is why you have to
pass its return value as an extra parameter.  If you passed commit you do
not have to.

Worse yet, the above xmalloc() and memcpy below are wrong, as you are no
longer taking the actual length of the abbreviated object name into
account, like the old code did.  f-u-a() is _about_ uniqueness, and you
can get a string longer than the minimum length you gave to it.

>  	q = out->parent_label;
>  	q = mempcpy(q, "parent of ", strlen("parent of "));
>  	out->label = q;
> -	q = mempcpy(q, abbrev, abbrev_len);
> +	q = mempcpy(q, sha1_abbrev, DEFAULT_ABBREV);
>  	q = mempcpy(q, "... ", strlen("... "));
>  	out->subject = q;
>  	q = mempcpy(q, subject, subject_len);


> @@ -168,8 +165,7 @@ static char *get_encoding(const char *message)
>  	const char *p = message, *eol;
>  
>  	if (!p)
> -		die (_("Could not read commit message of %s"),
> -				sha1_to_hex(commit->object.sha1));
> +		die (_("BUG: get_encoding() called with NULL"));

Hmm, do we really want to translate this error message?

As the function is file-scope static, I suspect that calling it with NULL
is a programmer error, not data error (i.e. it is not like we might get an
object with NULL here in some repository created by a different version or
implementation of git).  Once you got confident (and I hope you will be
before this series hits our tree) that no caller calls this function with
NULL, we might want to demote this to assert(p) or even remove that if
statement altogether.

> @@ -185,25 +181,26 @@ 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(const char *fallback, struct strbuf *msgbuf,
> +			const char *message)
>  {
>  	const char *p = message;
>  	while (*p && (*p != '\n' || p[1] != '\n'))
>  		p++;
>  
>  	if (!*p)
> -		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
> +		strbuf_addstr(msgbuf, fallback);

Can you explain what the above loop and if statements are doing for me?
It seems to be doing something a lot more than what the name of the
function suggests, which is a bad sign that the helper is not designed
properly with clear semantics in mind.  I would probably prefer to see
this function removed, and do this kind of thing in the only caller of
this confused helper function.  You could make an even smaller helper
function that implements the logic to determine if there are two
consecutive LFs in a string (seen above), but I suspect that would be just
the matter of strstr(message, "\n\n")

> -static void write_cherry_pick_head(void)
> +static void write_cherry_pick_head(const char *commit_sha1_hex)
>  {
>  	int fd;
>  	struct strbuf buf = STRBUF_INIT;
>  
> -	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
> +	strbuf_addf(&buf, "%s\n", commit_sha1_hex);
>  
>  	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
>  	if (fd < 0)
> @@ -370,7 +367,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;
> @@ -378,6 +375,7 @@ static int do_pick_commit(void)
>  	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
>  	char *defmsg = NULL;
>  	struct strbuf msgbuf = STRBUF_INIT;
> +	const char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);

Given the lifetime rule of f-u-a return value, I think it is a bad design
to call it and pass it as an extra parameter to get_message() much later.

[PATCH 04/10] revert: Rename no_replay to record_origin

[Ramkumar Ramachandra]

Give the variable "no_replay" a new name "record_origin", so that
there is no confusion when a "replay" structure is introduced later in
the series.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 6c485f6..c48a7df 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,7 +35,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
+static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
 static int commit_argc;
 static const char **commit_argv;
@@ -90,7 +90,7 @@ static void parse_args(int argc, const char **argv)
 
 	if (action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &no_replay, "append commit name"),
+			OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
@@ -467,7 +467,7 @@ static int do_pick_commit(struct commit *commit)
 		next = commit;
 		next_label = msg.label;
 		add_message_to_msg(commit_sha1_hex, &msgbuf, msg.message);
-		if (no_replay) {
+		if (record_origin) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
@@ -563,7 +563,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
 		if (no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (no_replay)
+		if (record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
 		if (edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
-- 
1.7.5.GIT

[PATCH 05/10] revert: Introduce struct to keep command-line options

[Ramkumar Ramachandra]

The current code uses a set of file-scope static variables to tell the
cherry-pick/ revert machinery how to replay the changes, and
initializes 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.  Hence, introduce a structure to group these variables, so that
the API can take them as a single replay_options parameter.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  174 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 97 insertions(+), 77 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index c48a7df..0d6a5a9 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,76 +35,88 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
-static enum { REVERT, CHERRY_PICK } action;
-static int commit_argc;
-static const char **commit_argv;
-static int allow_rerere_auto;
-
 static const char *me;
 
-/* Merge strategy. */
-static const char *strategy;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+struct replay_opts {
+	enum { REVERT, CHERRY_PICK } action;
+
+	/* Boolean options */
+	int edit;
+	int record_origin;
+	int no_commit;
+	int signoff;
+	int allow_ff;
+	int allow_rerere_auto;
+
+	int mainline;
+	int commit_argc;
+	const char **commit_argv;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+};
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static char *get_encoding(const char *message);
 
-static const char * const *revert_or_cherry_pick_usage(void)
+static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return action == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
 			  const char *arg, int unset)
 {
+	struct replay_opts **opts = opt->value;
+
 	if (unset)
 		return 0;
 
-	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-	xopts[xopts_nr++] = xstrdup(arg);
+	ALLOC_GROW((*opts)->xopts, (*opts)->xopts_nr + 1, (*opts)->xopts_alloc);
+	(*opts)->xopts[(*opts)->xopts_nr++] = xstrdup(arg);
 	return 0;
 }
 
-static void parse_args(int argc, const char **argv)
+static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
-	const char * const * usage_str = revert_or_cherry_pick_usage();
+	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	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", &opts->no_commit, "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
 		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },
-		OPT_BOOLEAN('s', "signoff", &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", &opts->signoff, "add Signed-off-by:"),
+		OPT_INTEGER('m', "mainline", &opts->mainline, "parent number"),
+		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
+		OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
+		OPT_CALLBACK('X', "strategy-option", &opts, "option",
 			"option for merge strategy", option_parse_x),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
 	};
 
-	if (action == CHERRY_PICK) {
+	if (opts->action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
-			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
+			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
 			die(_("program error"));
 	}
 
-	commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-				    PARSE_OPT_KEEP_ARGV0 |
-				    PARSE_OPT_KEEP_UNKNOWN);
-	if (commit_argc < 2)
+	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
+					PARSE_OPT_KEEP_ARGV0 |
+					PARSE_OPT_KEEP_UNKNOWN);
+	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
-	commit_argv = argv;
+	opts->commit_argv = argv;
 }
 
 struct commit_message {
@@ -287,7 +299,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
 
 static int do_recursive_merge(struct commit *base, struct commit *next,
 			      const char *base_label, const char *next_label,
-			      unsigned char *head, struct strbuf *msgbuf)
+			      unsigned char *head, struct strbuf *msgbuf,
+			      struct replay_opts *opts)
 {
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
@@ -308,7 +321,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 = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
 		parse_merge_opt(&o, *xopt);
 
 	clean = merge_trees(&o,
@@ -348,7 +361,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
  * If we are revert, or if our cherry-pick results in a hand merge,
  * we had better say that the current user is responsible for that.
  */
-static int run_git_commit(const char *defmsg)
+static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 {
 	/* 6 is max possible length of our args array including NULL */
 	const char *args[6];
@@ -356,9 +369,9 @@ static int run_git_commit(const char *defmsg)
 
 	args[i++] = "commit";
 	args[i++] = "-n";
-	if (signoff)
+	if (opts->signoff)
 		args[i++] = "-s";
-	if (!edit) {
+	if (!opts->edit) {
 		args[i++] = "-F";
 		args[i++] = defmsg;
 	}
@@ -367,7 +380,7 @@ static int run_git_commit(const char *defmsg)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit)
+static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -378,7 +391,7 @@ static int do_pick_commit(struct commit *commit)
 	const char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
 	int res;
 
-	if (no_commit) {
+	if (opts->no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
 		 * merge the differences in, so let's compute the tree
@@ -396,7 +409,7 @@ static int do_pick_commit(struct commit *commit)
 	discard_cache();
 
 	if (!commit->parents) {
-		if (action == REVERT)
+		if (opts->action == REVERT)
 			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
@@ -405,25 +418,25 @@ static int do_pick_commit(struct commit *commit)
 		int cnt;
 		struct commit_list *p;
 
-		if (!mainline)
+		if (!opts->mainline)
 			return error(_("Commit %s is a merge but no -m option was given."),
 				sha1_to_hex(commit->object.sha1));
 
 		for (cnt = 1, p = commit->parents;
-		     cnt != mainline && p;
+		     cnt != opts->mainline && p;
 		     cnt++)
 			p = p->next;
-		if (cnt != mainline || !p)
+		if (cnt != 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), opts->mainline);
 		parent = p->item;
-	} else if (0 < mainline)
+	} else if (0 < opts->mainline)
 		return error(_("Mainline was specified but commit %s is not a merge."),
 			sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
-	if (allow_ff && parent && !hashcmp(parent->object.sha1, head))
+	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
 		return fast_forward_to(commit->object.sha1, head);
 
 	if (parent && parse_commit(parent) < 0)
@@ -445,7 +458,7 @@ static int do_pick_commit(struct commit *commit)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (action == REVERT) {
+	if (opts->action == REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -467,18 +480,18 @@ static int do_pick_commit(struct commit *commit)
 		next = commit;
 		next_label = msg.label;
 		add_message_to_msg(commit_sha1_hex, &msgbuf, msg.message);
-		if (record_origin) {
+		if (opts->record_origin) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
-		if (!no_commit)
+		if (!opts->no_commit)
 			write_cherry_pick_head(commit_sha1_hex);
 	}
 
-	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf);
+					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
 	} else {
 		struct commit_list *common = NULL;
@@ -488,23 +501,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,
-					sha1_to_hex(head), remotes);
+		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+					common, sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
 	}
 
 	if (res) {
-		error(action == REVERT
+		error(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(opts->allow_rerere_auto);
 	} else {
-		if (!no_commit)
-			res = run_git_commit(defmsg);
+		if (!opts->no_commit)
+			res = run_git_commit(defmsg, opts);
 	}
 
 	free_message(&msg);
@@ -513,18 +526,18 @@ static int do_pick_commit(struct commit *commit)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs)
+static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 {
 	int argc;
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (action != REVERT)
+	if (opts->action != REVERT)
 		revs->reverse = 1;
 
-	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
+	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
 	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage());
+		usage(*revert_or_cherry_pick_usage(opts));
 
 	if (prepare_revision_walk(revs))
 		die(_("revision walk setup failed"));
@@ -533,7 +546,7 @@ static void prepare_revs(struct rev_info *revs)
 		die(_("empty commit set passed"));
 }
 
-static void read_and_refresh_cache(const char *me)
+static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 {
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
@@ -548,33 +561,34 @@ static void read_and_refresh_cache(const char *me)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv,
+				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
 
 	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
+	me = opts->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
+	parse_args(argc, argv, opts);
 
-	if (allow_ff) {
-		if (signoff)
+	if (opts->allow_ff) {
+		if (opts->signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (no_commit)
+		if (opts->no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (record_origin)
+		if (opts->record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
-		if (edit)
+		if (opts->edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	read_and_refresh_cache(me);
+	read_and_refresh_cache(me, opts);
 
-	prepare_revs(&revs);
+	prepare_revs(&revs, opts);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit);
+		int res = do_pick_commit(commit, opts);
 		if (res)
 			return res;
 	}
@@ -585,10 +599,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res = 0;
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
 	if (isatty(0))
-		edit = 1;
-	action = REVERT;
-	res = revert_or_cherry_pick(argc, argv);
+		opts.edit = 1;
+	opts.action = REVERT;
+	res = revert_or_cherry_pick(argc, argv, &opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -601,8 +618,11 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	int res = 0;
-	action = CHERRY_PICK;
-	res = revert_or_cherry_pick(argc, argv);
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
+	opts.action = CHERRY_PICK;
+	res = revert_or_cherry_pick(argc, argv, &opts);
 	if (res > 0)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT

[PATCH 06/10] revert: Separate cmdline parsing from functional code

[Ramkumar Ramachandra]

Make cmd_cherry_pick and cmd_revert call parse_args and parse all the
command-line arguments into an opts structure before sending it off to
a simplified pick_commits function.  pick_commits, in turn will set up
the revision walker and call do_pick_commit in a loop- later in the
series, it will serve as the starting point for continuation.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 0d6a5a9..9a612c6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -561,17 +561,12 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv,
-				struct replay_opts *opts)
+static int pick_commits(struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
 
-	git_config(git_default_config, NULL);
-	me = opts->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv, opts);
-
 	if (opts->allow_ff) {
 		if (opts->signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
@@ -605,7 +600,10 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	if (isatty(0))
 		opts.edit = 1;
 	opts.action = REVERT;
-	res = revert_or_cherry_pick(argc, argv, &opts);
+	git_config(git_default_config, NULL);
+	me = "revert";
+	parse_args(argc, argv, &opts);
+	res = pick_commits(&opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -622,7 +620,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 
 	memset(&opts, 0, sizeof(struct replay_opts));
 	opts.action = CHERRY_PICK;
-	res = revert_or_cherry_pick(argc, argv, &opts);
+	git_config(git_default_config, NULL);
+	me = "cherry-pick";
+	parse_args(argc, argv, &opts);
+	res = pick_commits(&opts);
 	if (res > 0)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT

[PATCH 07/10] revert: Catch incompatible command-line options early

[Ramkumar Ramachandra]

Earlier, incompatible command-line options used to be caught in
pick_commits after parse_args has parsed the options and populated the
options structure; a lot of unncessary work has already been done, and
significant amount of cleanup is required to die at this stage.
Instead, hand over this responsibility to parse_args so that the
program can die early.  Also write a die_opt_incompabile function to
handle incompatible options in a general manner; it will be used more
extensively as more command-line options are introduced later in the
series.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 9a612c6..d21af96 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -80,6 +80,22 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+static void verify_opt_compatible(const char *me, const char *base_opt, ...)
+{
+	const char *this_opt;
+	va_list ap;
+	int set;
+
+	va_start(ap, base_opt);
+	while ((this_opt = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			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, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
@@ -116,6 +132,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
+	if (opts->allow_ff)
+		verify_opt_compatible(me, "--ff",
+				"--signoff", opts->signoff,
+				"--no-commit", opts->no_commit,
+				"-x", opts->record_origin,
+				"--edit", opts->edit,
+				NULL);
 	opts->commit_argv = argv;
 }
 
@@ -567,17 +590,6 @@ static int pick_commits(struct replay_opts *opts)
 	struct commit *commit;
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	if (opts->allow_ff) {
-		if (opts->signoff)
-			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (opts->no_commit)
-			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (opts->record_origin)
-			die(_("cherry-pick --ff cannot be used with -x"));
-		if (opts->edit)
-			die(_("cherry-pick --ff cannot be used with --edit"));
-	}
-
 	read_and_refresh_cache(me, opts);
 
 	prepare_revs(&revs, opts);
-- 
1.7.5.GIT

[PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Ramkumar Ramachandra]

Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
one commit, 2010-06-02), a single invocation of "git cherry-pick" or
"git revert" can perform picks of several individual commits.  To
allow "git cherry-pick --abort" to cancel and "git cherry-pick
--continue" to resume the entire command, we will need to store some
information about the state and the plan at the beginning.  Introduce
a HEAD file to store this state, and TODO file to store the plan.

Helped-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index d21af96..4741252 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,12 @@
  * 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")
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -584,22 +591,85 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
+static void format_todo(struct strbuf *buf, struct commit_list *list,
+			struct replay_opts *opts)
+{
+	struct commit_list *cur = NULL;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	const char *sha1_abbrev = NULL;
+	const char *action;
+
+	action = (opts->action == REVERT ? "revert" : "pick");
+	for (cur = list; cur; cur = cur->next) {
+		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+		if (get_message(sha1_abbrev, cur->item->buffer, &msg))
+			die(_("Cannot get commit message for %s"), sha1_abbrev);
+		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
+	}
+}
+
+static void persist_head(unsigned char *head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file head_lock;
+	int fd;
+
+	if (file_exists(SEQ_PATH)) {
+		if (!is_directory(SEQ_PATH) && remove_path(SEQ_PATH) < 0) {
+			strbuf_release(&buf);
+			die(_("Could not remove %s"), SEQ_PATH);
+		}
+	}
+	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not create sequencer directory '%s'."), SEQ_PATH);
+	}
+
+	fd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), HEAD_FILE);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), HEAD_FILE);
+}
+
+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file todo_lock;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);
+	format_todo(&buf, todo_list, opts);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), TODO_FILE);
+	if (commit_lock_file(&todo_lock) < 0)
+		die(_("Error wrapping up %s"), TODO_FILE);
+}
+
 static int pick_commits(struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
+	unsigned char head[20];
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	read_and_refresh_cache(me, opts);
 
+	if (get_sha1("HEAD", head))
+		return error(_("You do not have a valid HEAD"));
+
+	read_and_refresh_cache(me, opts);
 	prepare_revs(&revs, opts);
+	persist_head(head);
 
 	while ((commit = get_revision(&revs))) {
 		int res = do_pick_commit(commit, opts);
-		if (res)
+		if (res) {
+			commit_list_insert(commit, &revs.commits);
+			persist_todo(revs.commits, opts);
 			return res;
+		}
 	}
-
 	return 0;
 }
 
-- 
1.7.5.GIT

[PATCH 09/10] revert: Implement parsing --continue, --abort and --skip

[Ramkumar Ramachandra]

Introduce three new command-line options: --continue, --abort, and
--skip resembling the correspoding options in "rebase -i".  For now,
just parse the options into the replay_opts structure, making sure
that two of them are not specified together.  They will actually be
implemented later in the series.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 4741252..4607676 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -47,6 +47,11 @@ static const char *me;
 struct replay_opts {
 	enum { REVERT, CHERRY_PICK } action;
 
+	/* --abort, --skip, and --continue */
+	int abort_oper;
+	int skip_oper;
+	int continue_oper;
+
 	/* Boolean options */
 	int edit;
 	int record_origin;
@@ -103,11 +108,36 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 	va_end(ap);
 }
 
+static void verify_opt_mutually_compatible(const char *me, ...)
+{
+	const char *opt1, *opt2;
+	va_list ap;
+	int set;
+
+	va_start(ap, me);
+	while ((opt1 = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			break;
+	}
+	if (!opt1) return;
+	while ((opt2 = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			die(_("%s: %s cannot be used with %s"),
+				me, opt1, opt2);
+	}
+	va_end(ap);
+}
+
 static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	int noop;
 	struct option options[] = {
+		OPT_BOOLEAN(0, "abort", &opts->abort_oper, "abort the current operation"),
+		OPT_BOOLEAN(0, "skip", &opts->skip_oper, "skip the current commit"),
+		OPT_BOOLEAN(0, "continue", &opts->continue_oper, "continue the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
@@ -136,7 +166,37 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
-	if (opts->commit_argc < 2)
+
+	/* Check for mutually incompatible command line arguments */
+	verify_opt_mutually_compatible(me,
+				"--abort", opts->abort_oper,
+				"--skip", opts->skip_oper,
+				"--continue", opts->continue_oper,
+				NULL);
+
+	/* Check for incompatible command line arguments */
+	if (opts->abort_oper || opts->skip_oper || opts->continue_oper) {
+		char *this_oper;
+		if (opts->abort_oper)
+			this_oper = "--abort";
+		else if (opts->skip_oper)
+			this_oper = "--skip";
+		else
+			this_oper = "--continue";
+
+		verify_opt_compatible(me, this_oper,
+				"--no-commit", opts->no_commit,
+				"--edit", opts->edit,
+				"--signoff", opts->signoff,
+				"--mainline", opts->mainline,
+				"--strategy", opts->strategy ? 1 : 0,
+				"--strategy-option", opts->xopts ? 1 : 0,
+				"-x", opts->record_origin,
+				"--ff", opts->allow_ff,
+				NULL);
+	}
+
+	else if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
 	if (opts->allow_ff)
@@ -146,6 +206,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
+
+	/* Remove these when the options are actually implemented */
+	if (opts->abort_oper)
+		die("--abort is not implemented yet");
+	if (opts->skip_oper)
+		die("--skip is not implemented yet");
+	if (opts->continue_oper)
+		die("--continue is not implemented yet");
+
 	opts->commit_argv = argv;
 }
 
-- 
1.7.5.GIT

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

[Ramkumar Ramachandra]

To abort, perform a "rerere clear" and "reset --hard" to the ref
specified by the HEAD file introduced earlier in the series.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 4607676..6a93175 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -208,8 +208,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 
 	/* Remove these when the options are actually implemented */
-	if (opts->abort_oper)
-		die("--abort is not implemented yet");
 	if (opts->skip_oper)
 		die("--skip is not implemented yet");
 	if (opts->continue_oper)
@@ -742,6 +740,46 @@ static int pick_commits(struct replay_opts *opts)
 	return 0;
 }
 
+static int process_continuation(struct replay_opts *opts)
+{
+	if (opts->abort_oper) {
+		char head[DEFAULT_ABBREV];
+		unsigned char sha1[20];
+		int fd;
+		rerere_clear(0);
+
+		if (!file_exists(HEAD_FILE))
+			goto error;
+		fd = open(HEAD_FILE, O_RDONLY, 0666);
+		if (fd < 0)
+			return error(_("Could not open '%s' for reading: %s"),
+				HEAD_FILE, strerror(errno));
+		if (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV) {
+			close(fd);
+			return error(_("Corrupt '%s': %s"), HEAD_FILE, strerror(errno));
+		}
+		close(fd);
+
+		if (get_sha1(head, sha1))
+			return error(_("Failed to resolve '%s' as a valid ref."), head);
+		update_ref(NULL, "HEAD", sha1, NULL, 0, MSG_ON_ERR);
+	}
+	else if (opts->skip_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+	else if (opts->continue_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+
+	return pick_commits(opts);
+error:
+	return error(_("No %s in progress"), me);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res = 0;
@@ -754,7 +792,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "revert";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+	res = process_continuation(&opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -774,7 +812,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "cherry-pick";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+	res = process_continuation(&opts);
 	if (res > 0)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT

Re: [PATCH v3 00/10] Sequencer Foundations

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I hope there are no major issues with this iteration.

I've scanned up to 07/10; except for the internal API issues in 03/10
overall you seem to be going in the right direction.

[PATCH v4 00/10] Sequencer Foundations

[Ramkumar Ramachandra]

Hi,

This is a quick re-roll after Junio's review of the previous
iteration; the previous iteration was posted less than 24 hours ago.
I'm in a hurry, because I want to make sure that this is ready (or
nearly ready) for merge before I actually implement more features.

Thanks for reading.

Ramkumar Ramachandra (10):
  advice: Introduce error_resolve_conflict
  revert: Propogate errors upwards from do_pick_commit
  revert: Eliminate global "commit" variable
  revert: Rename no_replay to record_origin
  revert: Introduce struct to keep command-line options
  revert: Separate cmdline parsing from functional code
  revert: Catch incompatible command-line options early
  revert: Introduce HEAD, TODO files to persist state, plan
  revert: Implement parsing --continue, --abort and --skip
  revert: Implement --abort processing

 advice.c         |   17 ++-
 advice.h         |    1 +
 builtin/revert.c |  492 ++++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 360 insertions(+), 150 deletions(-)

-- 
1.7.5.GIT

[PATCH 01/10] advice: Introduce error_resolve_conflict

[Ramkumar Ramachandra]

Introduce error_resolve_conflict corresponding to
die_resolve_conflict, and implement the latter function in terms of
the former.

Based-on-patch-by: Christian Couder <chistian.couder@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 advice.c |   17 +++++++++++------
 advice.h |    1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/advice.c b/advice.c
index 0be4b5f..80aaa14 100644
--- a/advice.c
+++ b/advice.c
@@ -34,16 +34,21 @@ int git_default_advice_config(const char *var, const char *value)
 	return 0;
 }
 
-void NORETURN die_resolve_conflict(const char *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.
 		 */
-		die("'%s' is not possible because you have unmerged files.\n"
-		    "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
-		    "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
-	else
-		die("'%s' is not possible because you have unmerged files.", me);
+		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);
+	return error("'%s' is not possible because you have unmerged files.", me);
+}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	error_resolve_conflict(me);
+	exit(128);
 }
diff --git a/advice.h b/advice.h
index 3244ebb..f537366 100644
--- a/advice.h
+++ b/advice.h
@@ -12,6 +12,7 @@ extern int advice_detached_head;
 
 int git_default_advice_config(const char *var, const char *value);
 
+int error_resolve_conflict(const char *me);
 extern void NORETURN die_resolve_conflict(const char *me);
 
 #endif /* ADVICE_H */
-- 
1.7.5.GIT

[PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Introduce error_dirty_index, based on die_dirty_index which prints
some hints, and returns an error to be handled by its caller,
do_pick_commit.  do_pick_commit, in turn, propogates the error
upwards, and the error is finally handled by cmd_cherry_pick and
cmd_revert.  Set a simple convention for error-handling as follows:
positive return values from do_pick_commit indicate conflicts, while
negative ones indicate other errors.

Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   72 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..ff26856 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -266,25 +266,15 @@ 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"
-					  "Please, commit your changes or stash them to proceed."));
-			else
-				die(_("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"));
-			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"));
-		}
-	}
+	if (read_cache_unmerged())
+		return error_resolve_conflict(me);
+
+	error(_("Your local changes would be overwritten by %s.\n"), me);
+	if (advice_commit_before_merge)
+		advise(_("Please, commit your changes or stash them to proceed."));
+	return -1;
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -398,18 +388,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) {
@@ -418,20 +408,20 @@ static int do_pick_commit(void)
 		struct commit_list *p;
 
 		if (!mainline)
-			die(_("Commit %s is a merge but no -m option was given."),
-			    sha1_to_hex(commit->object.sha1));
+			return error(_("Commit %s is a merge but no -m option was given."),
+				sha1_to_hex(commit->object.sha1));
 
 		for (cnt = 1, p = commit->parents;
 		     cnt != mainline && p;
 		     cnt++)
 			p = p->next;
 		if (cnt != mainline || !p)
-			die(_("Commit %s does not have parent %d"),
-			    sha1_to_hex(commit->object.sha1), mainline);
+			return error(_("Commit %s does not have parent %d"),
+				sha1_to_hex(commit->object.sha1), mainline);
 		parent = p->item;
 	} else if (0 < mainline)
-		die(_("Mainline was specified but commit %s is not a merge."),
-		    sha1_to_hex(commit->object.sha1));
+		return error(_("Mainline was specified but commit %s is not a merge."),
+			sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
@@ -441,12 +431,12 @@ 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"),
-		    me, sha1_to_hex(parent->object.sha1));
+		return error(_("%s: cannot parse parent commit %s"),
+			me, sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit->buffer, &msg) != 0)
-		die(_("Cannot get commit message for %s"),
-				sha1_to_hex(commit->object.sha1));
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
 
 	/*
 	 * "commit" is an existing commit.  We would want to apply
@@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
+	int res;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res > 0)
+		/* Exit status from conflict */
+		return res;
+	if (res < 0)
+		/* Other error */
+		exit(128);
+	return 0;
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
+	int res;
 	action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res > 0)
+		return res;
+	if (res < 0)
+		exit(128);
+	return 0;
 }
-- 
1.7.5.GIT

[PATCH 03/10] revert: Eliminate global "commit" variable

[Ramkumar Ramachandra]

Since we want to develop the functionality to either pick or revert
individual commits atomically later in the series, make "commit" a
local variable.  Modify the get_message API to take "commit" as an
argument, factor out add_message_to_msg, and don't check the sole
argument in get_encoding.

Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   47 ++++++++++++++++++-----------------------------
 1 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index ff26856..745295d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
 
 static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
-static struct commit *commit;
 static int commit_argc;
 static const char **commit_argv;
 static int allow_rerere_auto;
@@ -116,25 +115,25 @@ 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, struct commit_message *out)
 {
 	const char *encoding;
 	const char *abbrev, *subject;
 	int abbrev_len, subject_len;
 	char *q;
 
-	if (!raw_message)
+	if (!commit->buffer)
 		return -1;
-	encoding = get_encoding(raw_message);
+	encoding = get_encoding(commit->buffer);
 	if (!encoding)
 		encoding = "UTF-8";
 	if (!git_commit_encoding)
 		git_commit_encoding = "UTF-8";
 
 	out->reencoded_message = NULL;
-	out->message = raw_message;
+	out->message = commit->buffer;
 	if (strcmp(encoding, git_commit_encoding))
-		out->reencoded_message = reencode_string(raw_message,
+		out->reencoded_message = reencode_string(commit->buffer,
 					git_commit_encoding, encoding);
 	if (out->reencoded_message)
 		out->message = out->reencoded_message;
@@ -167,9 +166,6 @@ static char *get_encoding(const char *message)
 {
 	const char *p = message, *eol;
 
-	if (!p)
-		die (_("Could not read commit message of %s"),
-				sha1_to_hex(commit->object.sha1));
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
@@ -185,25 +181,12 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
-{
-	const char *p = message;
-	while (*p && (*p != '\n' || p[1] != '\n'))
-		p++;
-
-	if (!*p)
-		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
-
-	p += 2;
-	strbuf_addstr(msgbuf, p);
-}
-
-static void write_cherry_pick_head(void)
+static void write_cherry_pick_head(const char *commit_sha1_hex)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
 
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+	strbuf_addf(&buf, "%s\n", commit_sha1_hex);
 
 	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
@@ -370,7 +353,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;
@@ -434,7 +417,7 @@ static int do_pick_commit(void)
 		return error(_("%s: cannot parse parent commit %s"),
 			me, sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit->buffer, &msg) != 0)
+	if (get_message(commit, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
 			sha1_to_hex(commit->object.sha1));
 
@@ -467,14 +450,19 @@ 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 msg.message to msgbuf */
+		const char *p = strstr(msg.message, "\n\n");
+		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
+		strbuf_addstr(&msgbuf, p);
+
 		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(sha1_to_hex(commit->object.sha1));
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -552,6 +540,7 @@ static void read_and_refresh_cache(const char *me)
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	struct rev_info revs;
+	struct commit *commit;
 
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -574,7 +563,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	prepare_revs(&revs);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit();
+		int res = do_pick_commit(commit);
 		if (res)
 			return res;
 	}
-- 
1.7.5.GIT

[PATCH 04/10] revert: Rename no_replay to record_origin

[Ramkumar Ramachandra]

Give the variable "no_replay" a new name "record_origin", so that
there is no confusion when a "replay" structure is introduced later in
the series.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 745295d..e9b8533 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,7 +35,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
+static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
 static int commit_argc;
 static const char **commit_argv;
@@ -90,7 +90,7 @@ static void parse_args(int argc, const char **argv)
 
 	if (action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &no_replay, "append commit name"),
+			OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
@@ -456,7 +456,7 @@ static int do_pick_commit(struct commit *commit)
 		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
 		strbuf_addstr(&msgbuf, p);
 
-		if (no_replay) {
+		if (record_origin) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
@@ -552,7 +552,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
 		if (no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (no_replay)
+		if (record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
 		if (edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
-- 
1.7.5.GIT

[PATCH 05/10] revert: Introduce struct to keep command-line options

[Ramkumar Ramachandra]

The current code uses a set of file-scope static variables to tell the
cherry-pick/ revert machinery how to replay the changes, and
initializes 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.  Hence, introduce a structure to group these variables, so that
the API can take them as a single replay_options parameter.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  174 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 97 insertions(+), 77 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e9b8533..cc2fa73 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,76 +35,88 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
-static enum { REVERT, CHERRY_PICK } action;
-static int commit_argc;
-static const char **commit_argv;
-static int allow_rerere_auto;
-
 static const char *me;
 
-/* Merge strategy. */
-static const char *strategy;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+struct replay_opts {
+	enum { REVERT, CHERRY_PICK } action;
+
+	/* Boolean options */
+	int edit;
+	int record_origin;
+	int no_commit;
+	int signoff;
+	int allow_ff;
+	int allow_rerere_auto;
+
+	int mainline;
+	int commit_argc;
+	const char **commit_argv;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+};
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static char *get_encoding(const char *message);
 
-static const char * const *revert_or_cherry_pick_usage(void)
+static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return action == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
 			  const char *arg, int unset)
 {
+	struct replay_opts **opts = opt->value;
+
 	if (unset)
 		return 0;
 
-	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-	xopts[xopts_nr++] = xstrdup(arg);
+	ALLOC_GROW((*opts)->xopts, (*opts)->xopts_nr + 1, (*opts)->xopts_alloc);
+	(*opts)->xopts[(*opts)->xopts_nr++] = xstrdup(arg);
 	return 0;
 }
 
-static void parse_args(int argc, const char **argv)
+static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
-	const char * const * usage_str = revert_or_cherry_pick_usage();
+	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	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", &opts->no_commit, "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
 		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },
-		OPT_BOOLEAN('s', "signoff", &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", &opts->signoff, "add Signed-off-by:"),
+		OPT_INTEGER('m', "mainline", &opts->mainline, "parent number"),
+		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
+		OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
+		OPT_CALLBACK('X', "strategy-option", &opts, "option",
 			"option for merge strategy", option_parse_x),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
 	};
 
-	if (action == CHERRY_PICK) {
+	if (opts->action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
-			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
+			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
 			die(_("program error"));
 	}
 
-	commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-				    PARSE_OPT_KEEP_ARGV0 |
-				    PARSE_OPT_KEEP_UNKNOWN);
-	if (commit_argc < 2)
+	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
+					PARSE_OPT_KEEP_ARGV0 |
+					PARSE_OPT_KEEP_UNKNOWN);
+	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
-	commit_argv = argv;
+	opts->commit_argv = argv;
 }
 
 struct commit_message {
@@ -273,7 +285,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
 
 static int do_recursive_merge(struct commit *base, struct commit *next,
 			      const char *base_label, const char *next_label,
-			      unsigned char *head, struct strbuf *msgbuf)
+			      unsigned char *head, struct strbuf *msgbuf,
+			      struct replay_opts *opts)
 {
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
@@ -294,7 +307,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 = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
 		parse_merge_opt(&o, *xopt);
 
 	clean = merge_trees(&o,
@@ -334,7 +347,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
  * If we are revert, or if our cherry-pick results in a hand merge,
  * we had better say that the current user is responsible for that.
  */
-static int run_git_commit(const char *defmsg)
+static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 {
 	/* 6 is max possible length of our args array including NULL */
 	const char *args[6];
@@ -342,9 +355,9 @@ static int run_git_commit(const char *defmsg)
 
 	args[i++] = "commit";
 	args[i++] = "-n";
-	if (signoff)
+	if (opts->signoff)
 		args[i++] = "-s";
-	if (!edit) {
+	if (!opts->edit) {
 		args[i++] = "-F";
 		args[i++] = defmsg;
 	}
@@ -353,7 +366,7 @@ static int run_git_commit(const char *defmsg)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit)
+static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -363,7 +376,7 @@ static int do_pick_commit(struct commit *commit)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
 
-	if (no_commit) {
+	if (opts->no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
 		 * merge the differences in, so let's compute the tree
@@ -381,7 +394,7 @@ static int do_pick_commit(struct commit *commit)
 	discard_cache();
 
 	if (!commit->parents) {
-		if (action == REVERT)
+		if (opts->action == REVERT)
 			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
@@ -390,25 +403,25 @@ static int do_pick_commit(struct commit *commit)
 		int cnt;
 		struct commit_list *p;
 
-		if (!mainline)
+		if (!opts->mainline)
 			return error(_("Commit %s is a merge but no -m option was given."),
 				sha1_to_hex(commit->object.sha1));
 
 		for (cnt = 1, p = commit->parents;
-		     cnt != mainline && p;
+		     cnt != opts->mainline && p;
 		     cnt++)
 			p = p->next;
-		if (cnt != mainline || !p)
+		if (cnt != 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), opts->mainline);
 		parent = p->item;
-	} else if (0 < mainline)
+	} else if (0 < opts->mainline)
 		return error(_("Mainline was specified but commit %s is not a merge."),
 			sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
-	if (allow_ff && parent && !hashcmp(parent->object.sha1, head))
+	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
 		return fast_forward_to(commit->object.sha1, head);
 
 	if (parent && parse_commit(parent) < 0)
@@ -430,7 +443,7 @@ static int do_pick_commit(struct commit *commit)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (action == REVERT) {
+	if (opts->action == REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -456,18 +469,18 @@ static int do_pick_commit(struct commit *commit)
 		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
 		strbuf_addstr(&msgbuf, p);
 
-		if (record_origin) {
+		if (opts->record_origin) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
-		if (!no_commit)
+		if (!opts->no_commit)
 			write_cherry_pick_head(sha1_to_hex(commit->object.sha1));
 	}
 
-	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf);
+					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
 	} else {
 		struct commit_list *common = NULL;
@@ -477,23 +490,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,
-					sha1_to_hex(head), remotes);
+		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+					common, sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
 	}
 
 	if (res) {
-		error(action == REVERT
+		error(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(opts->allow_rerere_auto);
 	} else {
-		if (!no_commit)
-			res = run_git_commit(defmsg);
+		if (!opts->no_commit)
+			res = run_git_commit(defmsg, opts);
 	}
 
 	free_message(&msg);
@@ -502,18 +515,18 @@ static int do_pick_commit(struct commit *commit)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs)
+static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 {
 	int argc;
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (action != REVERT)
+	if (opts->action != REVERT)
 		revs->reverse = 1;
 
-	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
+	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
 	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage());
+		usage(*revert_or_cherry_pick_usage(opts));
 
 	if (prepare_revision_walk(revs))
 		die(_("revision walk setup failed"));
@@ -522,7 +535,7 @@ static void prepare_revs(struct rev_info *revs)
 		die(_("empty commit set passed"));
 }
 
-static void read_and_refresh_cache(const char *me)
+static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 {
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
@@ -537,33 +550,34 @@ static void read_and_refresh_cache(const char *me)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv,
+				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
 
 	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
+	me = opts->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
+	parse_args(argc, argv, opts);
 
-	if (allow_ff) {
-		if (signoff)
+	if (opts->allow_ff) {
+		if (opts->signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (no_commit)
+		if (opts->no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (record_origin)
+		if (opts->record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
-		if (edit)
+		if (opts->edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	read_and_refresh_cache(me);
+	read_and_refresh_cache(me, opts);
 
-	prepare_revs(&revs);
+	prepare_revs(&revs, opts);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit);
+		int res = do_pick_commit(commit, opts);
 		if (res)
 			return res;
 	}
@@ -574,10 +588,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
 	if (isatty(0))
-		edit = 1;
-	action = REVERT;
-	res = revert_or_cherry_pick(argc, argv);
+		opts.edit = 1;
+	opts.action = REVERT;
+	res = revert_or_cherry_pick(argc, argv, &opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -590,8 +607,11 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	int res;
-	action = CHERRY_PICK;
-	res = revert_or_cherry_pick(argc, argv);
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
+	opts.action = CHERRY_PICK;
+	res = revert_or_cherry_pick(argc, argv, &opts);
 	if (res > 0)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT

[PATCH 06/10] revert: Separate cmdline parsing from functional code

[Ramkumar Ramachandra]

Make cmd_cherry_pick and cmd_revert call parse_args and parse all the
command-line arguments into an opts structure before sending it off to
a simplified pick_commits function.  pick_commits, in turn will set up
the revision walker and call do_pick_commit in a loop- later in the
series, it will serve as the starting point for continuation.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index cc2fa73..2e5f260 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -550,17 +550,12 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv,
-				struct replay_opts *opts)
+static int pick_commits(struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
 
-	git_config(git_default_config, NULL);
-	me = opts->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv, opts);
-
 	if (opts->allow_ff) {
 		if (opts->signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
@@ -594,7 +589,10 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	if (isatty(0))
 		opts.edit = 1;
 	opts.action = REVERT;
-	res = revert_or_cherry_pick(argc, argv, &opts);
+	git_config(git_default_config, NULL);
+	me = "revert";
+	parse_args(argc, argv, &opts);
+	res = pick_commits(&opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -611,7 +609,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 
 	memset(&opts, 0, sizeof(struct replay_opts));
 	opts.action = CHERRY_PICK;
-	res = revert_or_cherry_pick(argc, argv, &opts);
+	git_config(git_default_config, NULL);
+	me = "cherry-pick";
+	parse_args(argc, argv, &opts);
+	res = pick_commits(&opts);
 	if (res > 0)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT

[PATCH 07/10] revert: Catch incompatible command-line options early

[Ramkumar Ramachandra]

Earlier, incompatible command-line options used to be caught in
pick_commits after parse_args has parsed the options and populated the
options structure; a lot of unncessary work has already been done, and
significant amount of cleanup is required to die at this stage.
Instead, hand over this responsibility to parse_args so that the
program can die early.  Also write a die_opt_incompabile function to
handle incompatible options in a general manner; it will be used more
extensively as more command-line options are introduced later in the
series.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 2e5f260..1c6c102 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -80,6 +80,22 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+static void verify_opt_compatible(const char *me, const char *base_opt, ...)
+{
+	const char *this_opt;
+	va_list ap;
+	int set;
+
+	va_start(ap, base_opt);
+	while ((this_opt = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			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, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
@@ -116,6 +132,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
+	if (opts->allow_ff)
+		verify_opt_compatible(me, "--ff",
+				"--signoff", opts->signoff,
+				"--no-commit", opts->no_commit,
+				"-x", opts->record_origin,
+				"--edit", opts->edit,
+				NULL);
 	opts->commit_argv = argv;
 }
 
@@ -556,17 +579,6 @@ static int pick_commits(struct replay_opts *opts)
 	struct commit *commit;
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	if (opts->allow_ff) {
-		if (opts->signoff)
-			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (opts->no_commit)
-			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (opts->record_origin)
-			die(_("cherry-pick --ff cannot be used with -x"));
-		if (opts->edit)
-			die(_("cherry-pick --ff cannot be used with --edit"));
-	}
-
 	read_and_refresh_cache(me, opts);
 
 	prepare_revs(&revs, opts);
-- 
1.7.5.GIT

[PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Ramkumar Ramachandra]

Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
one commit, 2010-06-02), a single invocation of "git cherry-pick" or
"git revert" can perform picks of several individual commits.  To
allow "git cherry-pick --abort" to cancel and "git cherry-pick
--continue" to resume the entire command, we will need to store some
information about the state and the plan at the beginning.  Introduce
a HEAD file to store this state, and TODO file to store the plan.

Helped-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1c6c102..306f5b0 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,12 @@
  * 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")
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -573,22 +580,86 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
+static void format_todo(struct strbuf *buf, struct commit_list *list,
+			struct replay_opts *opts)
+{
+	struct commit_list *cur = NULL;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	const char *sha1_abbrev = NULL;
+	const char *action;
+
+	action = (opts->action == REVERT ? "revert" : "pick");
+	for (cur = list; cur; cur = cur->next) {
+		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+		if (get_message(cur->item, &msg))
+			die(_("Cannot get commit message for %s"), sha1_abbrev);
+		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
+	}
+}
+
+static void persist_head(unsigned char *head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file head_lock;
+	int fd;
+
+	if (file_exists(SEQ_PATH)) {
+		if (!is_directory(SEQ_PATH) && remove_path(SEQ_PATH) < 0) {
+			strbuf_release(&buf);
+			die(_("Could not remove %s"), SEQ_PATH);
+		}
+	}
+	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not create sequencer directory '%s'."), SEQ_PATH);
+	}
+
+	fd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), HEAD_FILE);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), HEAD_FILE);
+}
+
+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file todo_lock;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);
+	format_todo(&buf, todo_list, opts);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), TODO_FILE);
+	if (commit_lock_file(&todo_lock) < 0)
+		die(_("Error wrapping up %s"), TODO_FILE);
+}
+
 static int pick_commits(struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
+	unsigned char head[20];
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	read_and_refresh_cache(me, opts);
 
+	if (get_sha1("HEAD", head))
+		return error(_("You do not have a valid HEAD"));
+
+	read_and_refresh_cache(me, opts);
 	prepare_revs(&revs, opts);
+	persist_head(head);
+	persist_todo(revs.commits, opts);
 
 	while ((commit = get_revision(&revs))) {
 		int res = do_pick_commit(commit, opts);
-		if (res)
+		if (res) {
+			commit_list_insert(commit, &revs.commits);
+			persist_todo(revs.commits, opts);
 			return res;
+		}
 	}
-
 	return 0;
 }
 
-- 
1.7.5.GIT

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -573,22 +580,86 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
[...]
>  	prepare_revs(&revs, opts);
> +	persist_head(head);
> +	persist_todo(revs.commits, opts);
>  
>  	while ((commit = get_revision(&revs))) {
>  		int res = do_pick_commit(commit, opts);
> -		if (res)
> +		if (res) {
> +			commit_list_insert(commit, &revs.commits);
> +			persist_todo(revs.commits, opts);
>  			return res;
> +		}
>  	}

Almost there.  To comfort overly-worried people like me that think we
have not finished converted all die() calls yet, wouldn't this need to
look like

	persist_head(head);
	while ((commit = ...)) {
		int status_or_error;

		/*
		 * Checkpoint.  If do_pick_commit exits, make sure the user
		 * can still use "git cherry-pick --continue" to recover.
		 */
		persist_todo(revs.commits, opts);

		status_or_error = do_pick_commit(...);
		if (status_or_error)
			return status_or_error;
	}

	/* Success! */
	remove_todo(opts);
	remove_head();
	return 0;

And with that, this would no longer depend on the (valuable enough on
their own terms) patches 1 and 2 so they could be treated as a
separate series, no?

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Almost there.  To comfort overly-worried people like me that think we
> have not finished converted all die() calls yet, wouldn't this need to
> look like
>
>        persist_head(head);
>        while ((commit = ...)) {
>                int status_or_error;
>
>                /*
>                 * Checkpoint.  If do_pick_commit exits, make sure the user
>                 * can still use "git cherry-pick --continue" to recover.
>                 */
>                persist_todo(revs.commits, opts);
>
>                status_or_error = do_pick_commit(...);
>                if (status_or_error)
>                        return status_or_error;
>        }
>
>        /* Success! */
>        remove_todo(opts);
>        remove_head();
>        return 0;

You told me about this on IRC last night, but I forgot to squash the
relevant patch into this series. Thanks :)

> And with that, this would no longer depend on the (valuable enough on
> their own terms) patches 1 and 2 so they could be treated as a
> separate series, no?

Yes, but I'd like to keep 1 and 2 in this series, if that's alright.
Another series will have to take care of error handling more
extensively.
With a few tests, would it be useful to get this series merged in? It
implements '--abort', which is already useful.

-- Ram

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Ramkumar Ramachandra]

Hi Junio,

Ramkumar Ramachandra writes:
> Jonathan Nieder writes:
>> And with that, this would no longer depend on the (valuable enough on
>> their own terms) patches 1 and 2 so they could be treated as a
>> separate series, no?
>
> Yes, but I'd like to keep 1 and 2 in this series, if that's alright.
> Another series will have to take care of error handling more
> extensively.
> With a few tests, would it be useful to get this series merged in? It
> implements '--abort', which is already useful.

There's a good way to create a separation between patches 1-2, and the
rest- thanks to Jonathan's suggestion on IRC.
Junio: Would you like to merge just the first two patches? I'll
continue working on the rest, and try to come up with something useful
to merge in another week or two.

-- Ram

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> There's a good way to create a separation between patches 1-2, and the
> rest- thanks to Jonathan's suggestion on IRC.
> Junio: Would you like to merge just the first two patches? I'll
> continue working on the rest, and try to come up with something useful
> to merge in another week or two.

If you can split out a smaller series that is obviously correct and
justifiable with or without the rest of the series, that is very good
in general.

I think [1/10] almost falls into that category, modulo that we used to say
it is "fatal:" when you have unmerged files and cannot cherry-pick, but
you now say "error:", which I think is an unnecessary change that you may
want to avoid if you can.

I am not yet convinced [2/10] falls into that category.

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Ramkumar Ramachandra]

Hi Jonathan and others,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> Almost there.  To comfort overly-worried people like me that think we
> have not finished converted all die() calls yet, wouldn't this need to
> look like
>
>        persist_head(head);
>        while ((commit = ...)) {
>                int status_or_error;
>
>                /*
>                 * Checkpoint.  If do_pick_commit exits, make sure the user
>                 * can still use "git cherry-pick --continue" to recover.
>                 */
>                persist_todo(revs.commits, opts);
>
>                status_or_error = do_pick_commit(...);
>                if (status_or_error)
>                        return status_or_error;
>        }
>
>        /* Success! */
>        remove_todo(opts);
>        remove_head();
>        return 0;

It's a little embarrassing, but I'm not able to figure out how to
persist the TODO early. prepare_revs() sets up the revision walker and
populates the rev_info struct: it contains a commit_list * in its
commits member, but I apparently can't access those directly -- I went
through some literature about revision walking and inspected how
get_revision must be used to iterate over all the individual commits.
Now, to persist the TODO, I need a commit_list to use: is there some
API I can use to avoid iterating over all the commits twice? Once to
populate the commit_list to persist the TODO, and the second time to
actually pick them?

Thanks.

-- Ram

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> is there some
> API I can use to avoid iterating over all the commits twice? Once to
> populate the commit_list to persist the TODO, and the second time to
> actually pick them?

Good catch.  (If I understand correctly this was a potential problem
with the previous iterations, too.)

So, before thinking about what APIs git provides, what is the desired
behavior?  It is possible to walk a revision list incrementally (like
"git log" does); should cherry-pick take advantage of that?

. If yes, the "todo list" would have to be in some form that can be
  updated incrementally (something like the original revision range
  with the commits already cherry-picked negated).  This sounds
  complicated to me.

. If no, the "todo list" needs to be fully resolved to start out, and
  then the cherry-pick loop can just walk through it.  In git API
  terms, that means first populating the commit_list (for example
  using a loop that gets revisions and inserts them at the end of a
  commit_list, or by preparing the revision walk with "limited" flag
  set), then walking through it with commit_list APIs.

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> . If no, the "todo list" needs to be fully resolved to start out, and
>  then the cherry-pick loop can just walk through it.  In git API
>  terms, that means first populating the commit_list (for example
>  using a loop that gets revisions and inserts them at the end of a
>  commit_list, or by preparing the revision walk with "limited" flag
>  set), then walking through it with commit_list APIs.

Thanks. I picked this approach :)

On a slightly unrelated note, I just found out that the persist_todo
can't be called twice due to a lockfile API limitation* -- the
atexit(3) cleanup handler is supposed to have a linked list of
lockfiles to clean up. When one lockfile is used twice, it becomes a
circularly linked list, and the entire program hangs at exit time
since traversing a circularly linked list is never-ending. Couple of
comments:
1. By reading the lockfile implementation, I understand why this
happens -- can't it be implemented differently to remove this
limitation? If not, shouldn't this limitation be documented somewhere?
2. What can I do? It'll be inelegant to use the lockfile API while
persisting the first time, but not the second time -- is there any way
out of this?

Thanks again.

* You brought this up during one of our IRC conversations, but I
didn't understand it then. I do now.

-- Ram

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Jeff King]

On Thu, Jun 02, 2011 at 06:23:03PM +0530, Ramkumar Ramachandra wrote:

> On a slightly unrelated note, I just found out that the persist_todo
> can't be called twice due to a lockfile API limitation* -- the
> atexit(3) cleanup handler is supposed to have a linked list of
> lockfiles to clean up. When one lockfile is used twice, it becomes a
> circularly linked list, and the entire program hangs at exit time
> since traversing a circularly linked list is never-ending. Couple of
> comments:
> 1. By reading the lockfile implementation, I understand why this
> happens -- can't it be implemented differently to remove this
> limitation? If not, shouldn't this limitation be documented somewhere?

Doesn't lock_file handle multiple locking already via the on_list parameter?

    $ grep -A2 on_list lockfile.c
            if (!lk->on_list) {
                    lk->next = lock_file_list;
                    lock_file_list = lk;
                    lk->on_list = 1;
            }

However, I think you have a bigger problem, which is that you are
allocating the lock_file on the stack in persist_todo and persist_head.
So by the time the atexit() handler is called, this storage has gone
away and you are just reading random data (not to mention that it also
should be zero-initialized before being passed to lock_file).

-Peff

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

[Ramkumar Ramachandra]

Hi Jeff,

Jeff King writes:
> Doesn't lock_file handle multiple locking already via the on_list parameter?
>
>    $ grep -A2 on_list lockfile.c
>            if (!lk->on_list) {
>                    lk->next = lock_file_list;
>                    lock_file_list = lk;
>                    lk->on_list = 1;
>            }
>
> However, I think you have a bigger problem, which is that you are
> allocating the lock_file on the stack in persist_todo and persist_head.
> So by the time the atexit() handler is called, this storage has gone
> away and you are just reading random data (not to mention that it also
> should be zero-initialized before being passed to lock_file).

Right. Thanks for the excellent pointer. I was struggling to see what
was wrong on GDB; little did I realize that I was viewing nonsense. I
solved the problem by making the lock_file variables static.

Thanks again, and sorry for the nonsense.

-- Ram

[PATCH 09/10] revert: Implement parsing --continue, --abort and --skip

[Ramkumar Ramachandra]

Introduce three new command-line options: --continue, --abort, and
--skip resembling the correspoding options in "rebase -i".  For now,
just parse the options into the replay_opts structure, making sure
that two of them are not specified together.  They will actually be
implemented later in the series.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 306f5b0..f33d40a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -47,6 +47,11 @@ static const char *me;
 struct replay_opts {
 	enum { REVERT, CHERRY_PICK } action;
 
+	/* --abort, --skip, and --continue */
+	int abort_oper;
+	int skip_oper;
+	int continue_oper;
+
 	/* Boolean options */
 	int edit;
 	int record_origin;
@@ -103,11 +108,36 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 	va_end(ap);
 }
 
+static void verify_opt_mutually_compatible(const char *me, ...)
+{
+	const char *opt1, *opt2;
+	va_list ap;
+	int set;
+
+	va_start(ap, me);
+	while ((opt1 = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			break;
+	}
+	if (!opt1) return;
+	while ((opt2 = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			die(_("%s: %s cannot be used with %s"),
+				me, opt1, opt2);
+	}
+	va_end(ap);
+}
+
 static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	int noop;
 	struct option options[] = {
+		OPT_BOOLEAN(0, "abort", &opts->abort_oper, "abort the current operation"),
+		OPT_BOOLEAN(0, "skip", &opts->skip_oper, "skip the current commit"),
+		OPT_BOOLEAN(0, "continue", &opts->continue_oper, "continue the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
@@ -136,7 +166,37 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
-	if (opts->commit_argc < 2)
+
+	/* Check for mutually incompatible command line arguments */
+	verify_opt_mutually_compatible(me,
+				"--abort", opts->abort_oper,
+				"--skip", opts->skip_oper,
+				"--continue", opts->continue_oper,
+				NULL);
+
+	/* Check for incompatible command line arguments */
+	if (opts->abort_oper || opts->skip_oper || opts->continue_oper) {
+		char *this_oper;
+		if (opts->abort_oper)
+			this_oper = "--abort";
+		else if (opts->skip_oper)
+			this_oper = "--skip";
+		else
+			this_oper = "--continue";
+
+		verify_opt_compatible(me, this_oper,
+				"--no-commit", opts->no_commit,
+				"--edit", opts->edit,
+				"--signoff", opts->signoff,
+				"--mainline", opts->mainline,
+				"--strategy", opts->strategy ? 1 : 0,
+				"--strategy-option", opts->xopts ? 1 : 0,
+				"-x", opts->record_origin,
+				"--ff", opts->allow_ff,
+				NULL);
+	}
+
+	else if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
 	if (opts->allow_ff)
@@ -146,6 +206,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
+
+	/* Remove these when the options are actually implemented */
+	if (opts->abort_oper)
+		die("--abort is not implemented yet");
+	if (opts->skip_oper)
+		die("--skip is not implemented yet");
+	if (opts->continue_oper)
+		die("--continue is not implemented yet");
+
 	opts->commit_argv = argv;
 }
 
-- 
1.7.5.GIT

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

[Ramkumar Ramachandra]

To abort, perform a "rerere clear" and "reset --hard" to the ref
specified by the HEAD file introduced earlier in the series.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index f33d40a..be63aee 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -208,8 +208,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 
 	/* Remove these when the options are actually implemented */
-	if (opts->abort_oper)
-		die("--abort is not implemented yet");
 	if (opts->skip_oper)
 		die("--skip is not implemented yet");
 	if (opts->continue_oper)
@@ -732,6 +730,46 @@ static int pick_commits(struct replay_opts *opts)
 	return 0;
 }
 
+static int process_continuation(struct replay_opts *opts)
+{
+	if (opts->abort_oper) {
+		char head[DEFAULT_ABBREV];
+		unsigned char sha1[20];
+		int fd;
+		rerere_clear(0);
+
+		if (!file_exists(HEAD_FILE))
+			goto error;
+		fd = open(HEAD_FILE, O_RDONLY, 0666);
+		if (fd < 0)
+			return error(_("Could not open '%s' for reading: %s"),
+				HEAD_FILE, strerror(errno));
+		if (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV) {
+			close(fd);
+			return error(_("Corrupt '%s': %s"), HEAD_FILE, strerror(errno));
+		}
+		close(fd);
+
+		if (get_sha1(head, sha1))
+			return error(_("Failed to resolve '%s' as a valid ref."), head);
+		update_ref(NULL, "HEAD", sha1, NULL, 0, MSG_ON_ERR);
+	}
+	else if (opts->skip_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+	else if (opts->continue_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+
+	return pick_commits(opts);
+error:
+	return error(_("No %s in progress"), me);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
@@ -744,7 +782,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "revert";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+	res = process_continuation(&opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -764,7 +802,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "cherry-pick";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+	res = process_continuation(&opts);
 	if (res > 0)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT

Re: [PATCH v4 00/10] Sequencer Foundations

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> This is a quick re-roll after Junio's review of the previous
> iteration; the previous iteration was posted less than 24 hours ago.
> I'm in a hurry,...

Open source is not just about pushing new code through other people's
throats, but also about the skill to make other people around you to work
with you. They may be busy spending time on helping other topics moving
forward. Or they may simply be bone-headed ;-).

If you are in a hurry, try to spend _more_ time making sure it is easier
to review for other people. What were updated, what were kept as they were
and for what reason? The purpose of adding the cover letter to the series
is to do just that.

Without spending that effort and saying "I am in a hurry" give a wrong
impression to others: "I cannot tell if this round is better cooked than
the previous round without spending nontrivial time reading on it, but
chances are it hasn't been much improved during the 24 hours.  It may be
better use of my time to skip it and work on other topics first.  After
finishing everything else I can come back---by that time Ram may have sent
out another round and reviewing this round right now may become wasted
time".

Also the series seems to be based on a rather old codebase where we didn't
allow reverting the root commit. I tried to apply a first few to
understand where these "positive" error status could be coming from, and
was stopped by patch conflicts. In [2/10] I think you are propagating
errors from run_command() coming back through try_merge_command(), and the
change is good for that particular codepath, but I haven't followed all
the other codepaths from do_cherry_pick() to convince myself that the exit
status from the merge strategy backend are _the only_ positive returns you
can possibly get. Have you?

Re: [PATCH v4 00/10] Sequencer Foundations

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Without spending that effort and saying "I am in a hurry" give a wrong
> impression to others: "I cannot tell if this round is better cooked than
> the previous round without spending nontrivial time reading on it, but
> chances are it hasn't been much improved during the 24 hours.  It may be
> better use of my time to skip it and work on other topics first.  After
> finishing everything else I can come back---by that time Ram may have sent
> out another round and reviewing this round right now may become wasted
> time".

You're right; this iteration was ill-thought-out. I'll try to put
myself in my reviewer's shoes before submitting code next time, so
that I don't waste effort re-rolling unnecessarily.

> Also the series seems to be based on a rather old codebase where we didn't
> allow reverting the root commit. I tried to apply a first few to
> understand where these "positive" error status could be coming from, and
> was stopped by patch conflicts. In [2/10] I think you are propagating
> errors from run_command() coming back through try_merge_command(), and the
> change is good for that particular codepath, but I haven't followed all
> the other codepaths from do_cherry_pick() to convince myself that the exit
> status from the merge strategy backend are _the only_ positive returns you
> can possibly get. Have you?

Oops, I'll rebase.
I'll trace all the code paths carefully and include a summary of my
learnings in the next iteration.

-- Ram