[PATCH 00/13] Sequencer with continuation features

[PATCH 00/13] Sequencer with continuation features

[Ramkumar Ramachandra]

Hi,

The much awaited cherry-pick --continue and --skip-all are here.  Much
thanks to Jonathan for all his inputs.  I think I can safely say that
I'm quite happy with the general state of this series now.  Apart from
the introduction of the the new features, there have been a few minor
improvements in the rest of the series since the last iteration.
Unfortunately, it's still not ready for inclusion for the following
reasons:
1. --skip-all isn't a great name; maybe --quit as Christian suggested?
2. I don't know what to do when a reset is invoked.  If we do nothing,
we'd break users' habits.  Jonathan suggested that we move
.git/sequencer to .git/sequencer-cancelled on a hard reset, and this
seems like a plausible solution.  More inputs on this?
3. --continue is obviously broken, because of lack of command-line
option support.  I think I should evolve the parser little by little-
first, I can persist the global opts structure, and then start
supporting per-command options.  That way, it'll be easy to get the
parser reviewed.
4. Tests are too basic.  I need to write more elaborate tests.

Thanks for reading.

Note: The series is also available on the 'sequencer-continue' branch of my
GitHub fork: http://github.com/artagnon/git

-- Ram

Ramkumar Ramachandra (13):
  advice: Introduce error_resolve_conflict
  revert: Factor out add_message_to_msg function
  revert: Don't check lone argument in get_encoding
  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: Persist data for continuation
  revert: Introduce a layer of indirection over pick_commits
  revert: Introduce --skip-all to cleanup sequencer data
  revert: Introduce --continue to continue the operation

 advice.c                           |   19 +-
 advice.h                           |    1 +
 builtin/revert.c                   |  585 +++++++++++++++++++++++++++---------
 git-rebase--interactive.sh         |   25 ++-
 t/t3032-merge-recursive-options.sh |    2 +
 t/t3501-revert-cherry-pick.sh      |    1 +
 t/t3502-cherry-pick-merge.sh       |    9 +-
 t/t3504-cherry-pick-rerere.sh      |    2 +
 t/t3505-cherry-pick-empty.sh       |   14 +-
 t/t3506-cherry-pick-ff.sh          |    3 +
 t/t3507-cherry-pick-conflict.sh    |   24 ++-
 t/t3510-cherry-pick-sequence.sh    |   79 +++++
 t/t7502-commit.sh                  |    1 +
 13 files changed, 596 insertions(+), 169 deletions(-)
 create mode 100644 t/t3510-cherry-pick-sequence.sh

-- 
1.7.5.GIT

[PATCH 01/13] 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.  The only trade-off is that die_resolve_conflict is a
little noisier now.

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

diff --git a/advice.c b/advice.c
index 0be4b5f..652680a 100644
--- a/advice.c
+++ b/advice.c
@@ -34,16 +34,23 @@ 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)
 {
+	error("'%s' is not possible because you have unmerged files.", 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("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'.");
+	return -1;
+}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	error_resolve_conflict(me);
+	die("Exiting because of an unresolved conflict.");
 }
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

Re: [PATCH 01/13] advice: Introduce error_resolve_conflict

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Introduce error_resolve_conflict corresponding to
> die_resolve_conflict, and implement the latter function in terms of
> the former.  The only trade-off is that die_resolve_conflict is a
> little noisier now.

The above doesn't tell me what I would want to know, namely:

 1. The impact of this patch is to change an error message.

 2. The change is from

	fatal: 'commit' is not possible because you have unmerged files.
	Please, fix them up in the work tree ...
	... etc, etc ...

    to

	error: 'commit' is not ...
	error: Please, fix them up...
	... etc, etc ...
	fatal: Exiting because of an unresolved conflict.

 3. The intended benefit is that new, future callers may want the "error"
    without exiting.

Notice that after writing the above, a little detail jumps out:
namely, the second "error:" line is giving advice, so it might make
sense to make it say "hint:" instead.

Re: [PATCH 01/13] advice: Introduce error_resolve_conflict

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ramkumar Ramachandra wrote:
>
>> Introduce error_resolve_conflict corresponding to
>> die_resolve_conflict, and implement the latter function in terms of
>> the former.  The only trade-off is that die_resolve_conflict is a
>> little noisier now.
>
> The above doesn't tell me what I would want to know, namely:
>
>  1. The impact of this patch is to change an error message.
>
>  2. The change is from
>
> 	fatal: 'commit' is not possible because you have unmerged files.
> 	Please, fix them up in the work tree ...
> 	... etc, etc ...
>
>     to
>
> 	error: 'commit' is not ...
> 	error: Please, fix them up...
> 	... etc, etc ...
> 	fatal: Exiting because of an unresolved conflict.
>
>  3. The intended benefit is that new, future callers may want the "error"
>     without exiting.

Your good suggestions for inexperienced people are always appreciated.

> Notice that after writing the above, a little detail jumps out:
> namely, the second "error:" line is giving advice, so it might make
> sense to make it say "hint:" instead.

Yes, and also I suspect that it shouldn't be hard to do this refactoring
without changing the output.

Re: [PATCH 01/13] advice: Introduce error_resolve_conflict

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
>  2. The change is from
>
>        fatal: 'commit' is not possible because you have unmerged files.
>        Please, fix them up in the work tree ...
>        ... etc, etc ...
>
>    to
>
>        error: 'commit' is not ...
>        error: Please, fix them up...
>        ... etc, etc ...
>        fatal: Exiting because of an unresolved conflict.

> Notice that after writing the above, a little detail jumps out:
> namely, the second "error:" line is giving advice, so it might make
> sense to make it say "hint:" instead.

How do I do this correctly?  Changing the "return error" statement to
"fprintf(stderr, "hint: Please fix them up ...")" doesn't feel right.

--- Ram

Re: [PATCH 01/13] advice: Introduce error_resolve_conflict

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> Notice that after writing the above, a little detail jumps out:
>> namely, the second "error:" line is giving advice, so it might make
>> sense to make it say "hint:" instead.
>
> How do I do this correctly?

In an ideal world, it would involve two patches.

 1. first, restructure the code without changing output
 2. then, change output

Or:

 1. first, change output without restructuring the code
 2. then, restructure the code

That way, readers could evaluate the merits of steps (1) and (2)
separately.

Oh, you mean how to write a "hint:" line analagous to "error:" lines?
There's no standard API for that yet but there's an example in
builtin/revert.c (see v1.7.3-rc0~26^2~3, Introduce advise() to print
hints, 2010-08-11).

[PATCH 02/13] revert: Factor out add_message_to_msg function

[Ramkumar Ramachandra]

The add_message_to_msg function is poorly implemented, has an unclear
API, and only one callsite.  Replace the callsite with a cleaner
implementation.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1f27c63..d6d2356 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -185,19 +185,6 @@ 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)
 {
 	int fd;
@@ -471,11 +458,17 @@ static int do_pick_commit(void)
 		}
 		strbuf_addstr(&msgbuf, ".\n");
 	} else {
+		const char *p = strstr(msg.message, "\n\n");
+
 		base = parent;
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
-		add_message_to_msg(&msgbuf, msg.message);
+
+		/* Add msg.message to msgbuf */
+		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));
-- 
1.7.5.GIT

Re: [PATCH 02/13] revert: Factor out add_message_to_msg function

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> [Subject: [PATCH 02/13] revert: Factor out add_message_to_msg function]

"Factor out" means to introduce a new function.  I think you mean
"inline". :)

> The add_message_to_msg function is poorly implemented, has an unclear
> API, and only one callsite.  Replace the callsite with a cleaner
> implementation.

The above does not answer the question I would have, namely what
exactly is wrong with add_message_to_msg.  Is it too slow?  Not
robust?  Is the generated assembly too long?  Is it hard for a reader
to figure out the intent?  Does it blend in poorly with its
surroundings?

Re: [PATCH 02/13] revert: Factor out add_message_to_msg function

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> The add_message_to_msg function is poorly implemented, has an unclear
>> API, and only one callsite.  Replace the callsite with a cleaner
>> implementation.
>
> The above does not answer the question I would have, namely what
> exactly is wrong with add_message_to_msg.  Is it too slow?  Not
> robust?  Is the generated assembly too long?  Is it hard for a reader
> to figure out the intent?  Does it blend in poorly with its
> surroundings?

I do not know if I _suggested_ this, but ...

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1f27c63..d6d2356 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -185,19 +185,6 @@ 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++;

This is a loop that manually implements strstr("\n\n"); rewriting of which
is a Good Thing.

> -	if (!*p)
> -		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
> -
> -	p += 2;
> -	strbuf_addstr(msgbuf, p);

I am not sure if this corresponds to what the new code does. What happens
if *p was NUL (i.e. we did not find any "\n\n" in the "message" at all)?
We go two bytes beyond that NUL and run addstr from that unknown space?
The new code does not seem to do that ;-)

I suspect that this was a bug from the original C-rewrite of git-revert
that was done by 9509af6 (Make git-revert & git-cherry-pick a builtin,
2007-03-01), and the new code fixes the bug, but then it would be nice to
note that in the commit log message.

> @@ -471,11 +458,17 @@ static int do_pick_commit(void)
>  		}
>  		strbuf_addstr(&msgbuf, ".\n");
>  	} else {
> +		const char *p = strstr(msg.message, "\n\n");
> +
>  		base = parent;
>  		base_label = msg.parent_label;
>  		next = commit;
>  		next_label = msg.label;
> -		add_message_to_msg(&msgbuf, msg.message);
> +
> +		/* Add msg.message to msgbuf */

This is an incorrect comment that confuses the reader. Think:

 - What did you skip by looking for the first "\n\n"?
 - What do you instead when no such "\n\n" is found?

Perhaps what it is doing is: "Add commit log message, if exists, or the
commit object name if it doesn't"

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

[PATCH 03/13] revert: Don't check lone argument in get_encoding

[Ramkumar Ramachandra]

The get_encoding function has only one callsite, and its caller makes
sure that a NULL argument isn't passed.  Don't unnecessarily double
check the same argument in get_encoding.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index d6d2356..2de2e75 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -167,9 +167,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 */
-- 
1.7.5.GIT

Re: [PATCH 03/13] revert: Don't check lone argument in get_encoding

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> The get_encoding function has only one callsite, and its caller makes
> sure that a NULL argument isn't passed.  Don't unnecessarily double
> check the same argument in get_encoding.

Yes, this one's pretty good, though I suspect the actual reason for
the patch is not just dead code elimination.

The patch itself gives reason to be comfortable: the "while" loop
right after the removed "if" checks the value of *p immediately, so if
callers start passing NULL, it will be pretty obvious.

I suspect the actual motivation is to avoid access to the "commit"
global, in preparation for making it local.  A nice side-benefit is
removing a translated message.

[PATCH 04/13] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Earlier, revert_or_cherry_pick only used to return a non-negative
number or die.  Change this so that it can return negative values too;
postive return values indicate conflicts, while negative ones indicate
other errors, and this return status is propogated updwards from
do_pick_commit, to be finally handled in cmd_cherry_pick and
cmd_revert.  While revert_or_cherry_pick can still die due to several
other reasons, this patches attempts to factor out some of the die
calls.  In the same spirit, also introduce a new function
error_dirty_index, based on die_dirty_index which prints some hints
and returns an error to its caller do_pick_commit.

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 |   70 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 2de2e75..26f39d1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -250,25 +250,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)
@@ -382,12 +372,12 @@ 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();
 
@@ -400,20 +390,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;
 
@@ -423,12 +413,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
@@ -582,14 +572,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 */
+		die(_("%s failed"), me);
+	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)
+		die(_("%s failed"), me);
+	return 0;
 }
-- 
1.7.5.GIT

Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Earlier, revert_or_cherry_pick only used to return a non-negative
> number or die.  Change this so that it can return negative values too;
> postive return values indicate conflicts, while negative ones indicate
> other errors, and this return status is propogated updwards from
> do_pick_commit, to be finally handled in cmd_cherry_pick and
> cmd_revert.  While revert_or_cherry_pick can still die due to several
> other reasons, this patches attempts to factor out some of the die
> calls.  In the same spirit, also introduce a new function
> error_dirty_index, based on die_dirty_index which prints some hints
> and returns an error to its caller do_pick_commit.

The "Earlier" combined with imperfect tense somehow oddly trips me up
(somehow it does not register that you mean "Before this patch").  I
usually find using the present or the simple past for the status quo
is clearer ("Currently the return value from revert_or_cherry_pick is
a non-negative integer representing the intended exit status from git
revert or git cherry-pick.  Change that by ...").

Carrying forward from that confusion, it's hard to concentrate hard
enough to parse and figure out the rest.  That shouldn't be necessary:
by setting the scene with an explanation of the problem being solved
it should be possible to make this much simpler.

Suppose, forgetting the above, that I asked you to explain what this
patch is about.  You'd say it's about teaching "revert_or_cherry_pick"
to return error() instead of die()-ing a little more often, right?
Then if I asked you why, you might say one of many things --- for
example, that in the long term the hope is to allow speeding up
multiple-cherry-pick by not writing CHERRY_PICK_HEAD et al until a
single-commit pick fails, and that die()-ing interferes with that by
not moving the thread of control to the caller that wants to write
files like CHERRY_PICK_HEAD on error.  After that, I might ask what
this patch does to address that, and you might tell me that in
addition to converting various die() calls to "return error()", it
updates the callers to cope with the negative return codes that could
result from that.  Eventually the negative value gets returned from
revert_or_cherry_pick; currently the convention there is to return a
nonnegative integer (representing an appropriate "normal" exit status
for the command) but in the new regime it can also be -1, meaning the
command failed and should die().  Finally if I asked you what the
impact of applying this patch is, you might tell me that the benefits
will require converting more die() calls so the only immediate impact
should be to change various messages from "fatal:" to "error:" and add
a new 

	fatal: cherry-pick failed

at the end.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -250,25 +250,15 @@ static struct tree *empty_tree(void)
[...]
> @@ -582,14 +572,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 */
> +		die(_("%s failed"), me);
> +	return 0;
>  }

With the above explanation in mind, it seems simpler to say

	if (res < 0)
		die(...);
	return res;

the idea being that there are two cases --- the new "die for failure"
case and the old "pass on the exit status requested by
revert_or_cherry_pick" case.

>  
>  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)
> +		die(_("%s failed"), me);
> +	return 0;

Should the "revert" or "cherry-pick" here be part of the message
marked for translation?  A translator might want to paraphrase to

	fatal: failed to cherry-pick

if that is more natural in the language at hand.

Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> +	if (res < 0)
>> +		die(_("%s failed"), me);
>> +	return 0;
>
> Should the "revert" or "cherry-pick" here be part of the message
> marked for translation?  A translator might want to paraphrase to
>
> 	fatal: failed to cherry-pick
>
> if that is more natural in the language at hand.

Wouldn't such a message file simply say

	msgid "%s failed"
        msgstr "failed to %s"

IOW, I am not sure what problem you are seeing.

Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit

[Jonathan Nieder]

Jun 21, 2011 at 12:19:47PM -0700, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>>> +	if (res < 0)
>>> +		die(_("%s failed"), me);
>>> +	return 0;
>>
>> Should the "revert" or "cherry-pick" here be part of the message
>> marked for translation?  A translator might want to paraphrase to
>>
>> 	fatal: failed to cherry-pick
>>
>> if that is more natural in the language at hand.
>
> Wouldn't such a message file simply say
>
> 	msgid "%s failed"
>         msgstr "failed to %s"
>
> IOW, I am not sure what problem you are seeing.

Ah, sorry for the lack of clarity.  What I meant is that the noun
and verb will be different words in many languages.  There can also be
problems of subject/verb agreement.  Also "me" is used elsewhere to
hold the command name as typed on the command line even when LANG
points to a language other than English if I remember correctly.

If the message were in revert_or_cherry_pick instead of having two
identical copies in cmd_revert and cmd_cherry_pick, it would have been
less striking (but still a potential problem).

Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jun 21, 2011 at 12:19:47PM -0700, Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>>> +	if (res < 0)
>>>> +		die(_("%s failed"), me);
>>>> +	return 0;
>>>
>>> Should the "revert" or "cherry-pick" here be part of the message
>>> marked for translation?  A translator might want to paraphrase to
>>>
>>> 	fatal: failed to cherry-pick
>>>
>>> if that is more natural in the language at hand.
>>
>> Wouldn't such a message file simply say
>>
>> 	msgid "%s failed"
>>         msgstr "failed to %s"
>>
>> IOW, I am not sure what problem you are seeing.
>
> Ah, sorry for the lack of clarity.  What I meant is that the noun
> and verb will be different words in many languages.  There can also be
> problems of subject/verb agreement.  Also "me" is used elsewhere to
> hold the command name as typed on the command line even when LANG
> points to a language other than English if I remember correctly.

Yes, "me" is the name of the command as the user types, so to whatever
language you are translating, it can only be usable as the name of the
command in the message.  Even if your language has a single verb to
express the act of "running the cherry-pick command", say, "distim", you
cannot translate the message to "failed to distim".  Your message has to
say something like "Sorry, I got a failure while running the 'cherry-pick'
command", and in English (which is msgid is in) "%s failed" would be good
enough to convey that meaning.

BUT.

> If the message were in revert_or_cherry_pick instead of having two
> identical copies in cmd_revert and cmd_cherry_pick, it would have been
> less striking (but still a potential problem).

The effort by Ævar to switch among totally different message strings
depending on "action" was going in the direction to actually allow you to
translate one of these messages to "failed to distim", and it was lost
earlier in this patch (error-dirty-index). We may want to fix that in
addition to this one.

Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Hi Junio and Jonathan,

Junio C Hamano writes:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Jun 21, 2011 at 12:19:47PM -0700, Junio C Hamano wrote:
>>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>>>> +  if (res < 0)
>>>>> +          die(_("%s failed"), me);
>>>>> +  return 0;
>>>>
>>>> Should the "revert" or "cherry-pick" here be part of the message
>>>> marked for translation?  A translator might want to paraphrase to
>>>>
>>>>     fatal: failed to cherry-pick
>>>>
>>>> if that is more natural in the language at hand.
>>>
>>> Wouldn't such a message file simply say
>>>
>>>      msgid "%s failed"
>>>         msgstr "failed to %s"
>>>
>>> IOW, I am not sure what problem you are seeing.
>>
>> Ah, sorry for the lack of clarity.  What I meant is that the noun
>> and verb will be different words in many languages.  There can also be
>> problems of subject/verb agreement.  Also "me" is used elsewhere to
>> hold the command name as typed on the command line even when LANG
>> points to a language other than English if I remember correctly.
>
> Yes, "me" is the name of the command as the user types, so to whatever
> language you are translating, it can only be usable as the name of the
> command in the message.  Even if your language has a single verb to
> express the act of "running the cherry-pick command", say, "distim", you
> cannot translate the message to "failed to distim".  Your message has to
> say something like "Sorry, I got a failure while running the 'cherry-pick'
> command", and in English (which is msgid is in) "%s failed" would be good
> enough to convey that meaning.

Right.

> BUT.
>
>> If the message were in revert_or_cherry_pick instead of having two
>> identical copies in cmd_revert and cmd_cherry_pick, it would have been
>> less striking (but still a potential problem).
>
> The effort by Ævar to switch among totally different message strings
> depending on "action" was going in the direction to actually allow you to
> translate one of these messages to "failed to distim", and it was lost
> earlier in this patch (error-dirty-index). We may want to fix that in
> addition to this one.

But why is it required here?  I already know what "me" is depending on
whether I'm in cmd_cherry_pick or cmd_revert and there are two
different translation strings anyway.  I fixed the problem I
introduced in error-dirty-index earlier in this patch.

-- Ram

[PATCH 05/13] 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.  Doing this involves changing several functions to
take an additional argument, but no other functional changes.

Helped-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 |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 26f39d1..1b04b3c 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;
@@ -182,12 +181,12 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-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)
@@ -354,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;
@@ -416,7 +415,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));
 
@@ -462,7 +461,7 @@ static int do_pick_commit(void)
 			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) {
@@ -540,6 +539,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";
@@ -562,7 +562,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 05/13] revert: Eliminate global "commit" variable

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> Since we want to develop the functionality to either pick or revert
> individual commits atomically later in the series, make "commit" a
> local variable.  Doing this involves changing several functions to
> take an additional argument, but no other functional changes.

Okay, one more message review.  Suppose I asked you what this patch
does.  You'd say it makes "commit" no longer a static variable,
passing it around instead.  Next I ask you why.  The answer would
presumably be something like "because ... static variables are bad",
or "because passing it around explicitly makes it easier for a person
reading the code to see how the state evolves", or "because some day
we will want this to be thread-safe, meaning there could be more than
one commit being cherry-picked at the same time".  Right?

Re: [PATCH 05/13] revert: Eliminate global "commit" variable

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Since we want to develop the functionality to either pick or revert
> individual commits atomically later in the series, make "commit" a
> local variable.  Doing this involves changing several functions to
> take an additional argument, but no other functional changes.

Actually you didn't make it a local variable. You made it a parameter to
be passed around, which is even better ;-).

> -static void write_cherry_pick_head(void)
> +static void write_cherry_pick_head(const char *commit_sha1_hex)

Wouldn't we prefer to see this parameter also to be a commit objet, not a
preformatted hex string?  The only exception would be if you are going to
introduce a future callsite that does not have a commit object but may
know the commit object name, but if you are not going to do so, then...

[PATCH 06/13] 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 1b04b3c..cf62f93 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(),
 		};
@@ -455,7 +455,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");
@@ -551,7 +551,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 07/13] 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.

Unfortunately, parsing strategy-option violates a C89 rule:
Initializers cannot refer to variables whose address is not known at
compile time.

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 |  176 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 99 insertions(+), 77 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index cf62f93..e2e290c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,76 +35,90 @@ 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;
+enum replay_action { REVERT, CHERRY_PICK };
+
+struct replay_opts {
+	enum replay_action 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_ptr = opt->value;
+	struct replay_opts *opts = *opts_ptr;
+
 	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 +287,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 +309,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 +349,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 +357,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 +368,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 +378,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
@@ -388,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)
@@ -428,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;
@@ -455,18 +470,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;
@@ -476,23 +491,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);
@@ -501,18 +516,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"));
@@ -521,7 +536,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);
@@ -536,33 +551,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;
 	}
@@ -573,10 +589,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;
@@ -589,8 +608,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

Re: [PATCH 07/13] revert: Introduce struct to keep command-line options

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

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

The struct leaves out the variable "me".  A person might wonder why
the above rationale applies to the other variables but not that one. :)

> Unfortunately, parsing strategy-option violates a C89 rule:
> Initializers cannot refer to variables whose address is not known at
> compile time.

Reading this, one is led to wonder:

 - is this a regression?
 - is it a necessary consequence of the positive change?
 - will it matter --- i.e., does the rest of git follow that rule to
   be able to build on old-fashioned compilers, and is there any
   intention to?
 - how can I get my compiler to tell me about it?
 - is it fundamental or not?  i.e., is this a "we introduced this
   minor regression and expect it will be fixed later" or "this points
   to a flaw in the design and we hope someone comes up with a better
   design that doesn't have that problem"?

By the way, as a side note, here are a couple of patches about the
same subject that I use to test with "-std=c89 -pedantic" privately.

 http://repo.or.cz/w/git/jrn.git/commit/faa4b89731e5411fbdf6812f11748a20e664361d
 http://repo.or.cz/w/git/jrn.git/commit/92a0c179cb7690bdf6f8fbbfa2253b883d02bd17

Re: [PATCH 07/13] revert: Introduce struct to keep command-line options

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ramkumar Ramachandra wrote:
> ...
> The struct leaves out the variable "me".  A person might wonder why
> the above rationale applies to the other variables but not that one. :)

Good point.

>> Unfortunately, parsing strategy-option violates a C89 rule:
>> Initializers cannot refer to variables whose address is not known at
>> compile time.
>
> Reading this, one is led to wonder:
> ...
>  - is this a regression?

Let me add one.

 - how does this patch work around that issue?

[PATCH 08/13] revert: Separate cmdline parsing from functional code

[Ramkumar Ramachandra]

Currently, revert_or_cherry_pick does too many things including
argument parsing and setting up to pick the commits; this doesn't make
a good API.  Simplify and rename the function to pick_commits, so that
it just has the responsibility of setting up the revision walker and
calling do_pick_commit in a loop.  Transfer the remaining work to its
callers cmd_cherry_pick and cmd_revert.  Later in the series,
pick_commits will serve as the starting point for continuation.

Inspired-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 e2e290c..93e0531 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -551,17 +551,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"));
@@ -595,7 +590,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;
@@ -612,7 +610,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

Re: [PATCH 08/13] revert: Separate cmdline parsing from functional code

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Currently, revert_or_cherry_pick does too many things including
> argument parsing and setting up to pick the commits; this doesn't make
> a good API.  Simplify and rename the function to pick_commits, so that
> it just has the responsibility of setting up the revision walker and
> calling do_pick_commit in a loop.  Transfer the remaining work to its
> callers cmd_cherry_pick and cmd_revert.

Makes sense to me. :)

> Later in the series,
> pick_commits will serve as the starting point for continuation.

This sentence is a little cryptic.  The naive reader wonders
"continuation of what?"

[PATCH 09/13] 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.  Instead, hand over the responsibility of catching
incompatible command-line options to parse_args so that the program
can die early.  Also write a verify_opt_compatible function to handle
incompatible options in a general manner.

Inspired-by: Christian Couder <chiscool@tuxfamily.org>
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 93e0531..cfa898f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -82,6 +82,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);
@@ -118,6 +134,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;
 }
 
@@ -557,17 +580,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

Re: [PATCH 09/13] revert: Catch incompatible command-line options early

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Earlier, incompatible command-line options used to be caught in
> pick_commits after parse_args has parsed the options and populated the
> options structure.  Instead, hand over the responsibility of catching
> incompatible command-line options to parse_args so that the program
> can die early.

The "Earlier ... used to" phrasing is tripping me up again.

The naive reader (i.e., me) wonders: how long does this option parsing
and populating the options structure that we want to delay until after
verify_opt_compatible take?  Does that delay matter or is there some
other reason for this change?

> Also write a verify_opt_compatible function to handle
> incompatible options in a general manner.

Nitpick: the commit message generally commands the program or the
codebase to behave in a certain way, but in this example it is
commanding the author.  I'd say "add" or "provide".

Re: [PATCH 09/13] revert: Catch incompatible command-line options early

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Earlier, incompatible command-line options used to be caught in
>> pick_commits after parse_args has parsed the options and populated the
>> options structure.  Instead, hand over the responsibility of catching
>> incompatible command-line options to parse_args so that the program
>> can die early.
>
> The "Earlier ... used to" phrasing is tripping me up again.
>
> The naive reader (i.e., me) wonders: how long does this option parsing
> and populating the options structure that we want to delay until after
> verify_opt_compatible take?  Does that delay matter or is there some
> other reason for this change?


I've tried to spell out the motivation for this change more clearly in
my latest iteration.  Does this look alright?

    revert: Catch incompatible command-line options in parse_args

    Some incompatible command-line options are caught in pick_commits
    after parse_args has parsed the options and populated the options
    structure.  Change this so that parse_args only parses valid
    command-line options instead of returning an unusable options
    structure.  However, this does not mean that we will never need to
    check the options structure; when a sequencer API is written future,
    callers will call in with a pre-populated options structure whose
    validity we will still need to check.  Also, provide a
    verify_opt_compatible function to handle incompatible options in a
    general manner.


-- Ram

Re: [PATCH 09/13] revert: Catch incompatible command-line options early

[Jonathan Nieder]

Hi Ram,

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> The naive reader (i.e., me) wonders: how long does this option parsing
>> and populating the options structure that we want to delay until after
>> verify_opt_compatible take?  Does that delay matter or is there some
>> other reason for this change?
>
> I've tried to spell out the motivation for this change more clearly in
> my latest iteration.  Does this look alright?
>
>     revert: Catch incompatible command-line options in parse_args
>
>     Some incompatible command-line options are caught in pick_commits
>     after parse_args has parsed the options and populated the options
>     structure.  Change this so that parse_args only parses valid
>     command-line options instead of returning an unusable options
>     structure.  However, this does not mean that we will never need to
>     check the options structure; when a sequencer API is written future,
>     callers will call in with a pre-populated options structure whose
>     validity we will still need to check.  Also, provide a
>     verify_opt_compatible function to handle incompatible options in a
>     general manner.

Alas, I still don't get it.  How can I (the fearful reader) demonstrate
the problem that this fixes?  Is it a big problem?  How does the patch
fix it?  Does the patch have downsides, or is it mostly risk-free?

Re: [PATCH 09/13] revert: Catch incompatible command-line options early

[Jonathan Nieder]

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:

>>     Some incompatible command-line options are caught in pick_commits
>>     after parse_args has parsed the options and populated the options
>>     structure.  Change this so that parse_args only parses valid
>>     command-line options
[...]
> Alas, I still don't get it.  How can I (the fearful reader) demonstrate
> the problem that this fixes?  Is it a big problem?  How does the patch
> fix it?  Does the patch have downsides, or is it mostly risk-free?

To be clearer, here's a quick example in some hypothetical alternate
world:

	cherry-pick/revert: do not create a nonsense "struct opts" when given nonsense argv

	The --foo and --bar options make no sense together, but if I
	run "git cherry-pick --foo --bar" then parse_cherry_pick_option
	will return a nonsense struct with both "fooing" and "barring"
	set to true, which makes no sense.  Currently it's not a
	problem since the code that cares is protected by a check:

		if (opts.fooing && opts.barring)
			die("cannot foo and bar at the same time");

	But that is very fragile --- the frob() code-path relies on
	opts.fooing and opts.barring not both being true without such
	a check and it's only a coincidence that futz() is called
	first and makes that check, protecting it.

	We can make sure such a nonsensical options struct is never
	created by checking right away that --foo and --bar are not
	passed together at option-parsing time.  Meanwhile, make sure
	regressions in maintaining this invariant are caught in the
	future by adding an assertion "assert(!opts->fooing ||
	!opts->barring)" to both frob() and futz().

	The discussion above also applies to --bar and --qux.  Fix
	that, too.

Re: [PATCH 09/13] revert: Catch incompatible command-line options early

[Ramkumar Ramachandra]

Hi again,

On Sat, Jul 2, 2011 at 3:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>        cherry-pick/revert: do not create a nonsense "struct opts" when given nonsense argv
>
>        The --foo and --bar options make no sense together, but if I
>        run "git cherry-pick --foo --bar" then parse_cherry_pick_option
>        will return a nonsense struct with both "fooing" and "barring"
>        set to true, which makes no sense.  Currently it's not a
>        problem since the code that cares is protected by a check:
>
>                if (opts.fooing && opts.barring)
>                        die("cannot foo and bar at the same time");
>
>        But that is very fragile --- the frob() code-path relies on
>        opts.fooing and opts.barring not both being true without such
>        a check and it's only a coincidence that futz() is called
>        first and makes that check, protecting it.
>
>        We can make sure such a nonsensical options struct is never
>        created by checking right away that --foo and --bar are not
>        passed together at option-parsing time.  Meanwhile, make sure
>        regressions in maintaining this invariant are caught in the
>        future by adding an assertion "assert(!opts->fooing ||
>        !opts->barring)" to both frob() and futz().
>
>        The discussion above also applies to --bar and --qux.  Fix
>        that, too.

Thanks! I wish I could write commit messages like this.  I hope I've
got it right this time:

    revert: Don't create invalid replay_opts in parse_args

    The "--ff" command-line option cannot be used with four other
    command-line options.  However, when these options are specified with
    "--ff" on the command-line, parse_args will still parse these
    incompatible options into a replay_opts structure for use by the rest
    of the program.  Although pick_commits checks the validity of the
    replay_opts strucutre before before starting its operation, this is
    fragile design because the validity of the replay_opts structure
    depends on pick_commits being called before anything else.  Change
    this so that an invalid replay_opts structure is not created by
    parse_args in the first place, and make sure regressions in
    maintaining this invariant are caught in the future by adding an
    assertion in pick_commits.

And yes, adding an "assert" statement in pick_commits does seem like a
great reminder for the future :)

-- Ram

Re: [PATCH 09/13] revert: Catch incompatible command-line options early

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Although pick_commits checks the validity of the
>     replay_opts strucutre before before starting its operation, this is
>     fragile design because the validity of the replay_opts structure
>     depends on pick_commits being called before anything else.

Thanks, that's a bit more concrete.  But it's not fragile, is it?
pick_commits() is the entry point to the cherry-pick machinery ---
anything that uses a struct replay_opts is called indirectly by
pick_commits(), no?

So (genuinely, even when I'm not pretending to be naive) I still don't
see the point.  I'm not even convinced it's a bad idea, since it's
still not clear to me what the idea is.

Re: [PATCH 09/13] revert: Catch incompatible command-line options early

[Jonathan Nieder]

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:

>> Although pick_commits checks the validity of the
>>     replay_opts strucutre before before starting its operation, this is
>>     fragile design because the validity of the replay_opts structure
>>     depends on pick_commits being called before anything else.
>
> Thanks, that's a bit more concrete.  But it's not fragile, is it?
> pick_commits() is the entry point to the cherry-pick machinery ---
> anything that uses a struct replay_opts is called indirectly by
> pick_commits(), no?

Perhaps the idea is that in the future, there will be multiple entry
points?

Again, the questions the reader will have are:

 - what is wrong with the code? (i.e., assume I'm a total stick in
   the mud and convince me there needs to be *some* change)
 - why does the current code work at all?
 - what is the proposed fix?
 - how will it work better?
 - what are the downsides, and how does the patch mitigate them?

Exact wording does not matter, as long as those questions are
answered.

[PATCH 10/13] revert: Persist data for continuation

[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
implement features like "--continue" to continue the whole operation,
we will need to store some information about the state and the plan at
the beginning.  Introduce a ".git/sequencer/head" file to store this
state, and ".git/sequencer/todo" file to store the plan.  Don't touch
CHERRY_PICK_HEAD -- it will still be useful when a conflict is
encountered.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |  123 ++++++++++++++++++++++++++++++++++++--
 t/t3510-cherry-pick-sequence.sh |   37 ++++++++++++
 2 files changed, 153 insertions(+), 7 deletions(-)
 create mode 100644 t/t3510-cherry-pick-sequence.sh

diff --git a/builtin/revert.c b/builtin/revert.c
index cfa898f..ca5756b 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,10 @@
  * Copyright (c) 2005 Junio C Hamano
  */
 
+#define SEQ_DIR		git_path("sequencer")
+#define HEAD_FILE	git_path("sequencer/head")
+#define TODO_FILE	git_path("sequencer/todo")
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -412,7 +417,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 			return error(_("Your index file is unmerged."));
 	} else {
 		if (get_sha1("HEAD", head))
-			return error(_("You do not have a valid HEAD"));
+			return error(_("Can't %s on an unborn branch"), me);
 		if (index_differs_from("HEAD", 0))
 			return error_dirty_index(me);
 	}
@@ -574,23 +579,127 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
-static int pick_commits(struct replay_opts *opts)
+static void format_todo(struct strbuf *buf, struct commit_list *todo_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 = todo_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 walk_revs_populate_todo(struct commit_list **todo_list,
+				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
+	struct commit_list *new_item;
+	struct commit_list *cur = NULL;
+
+	/* Insert into todo_list in the same order */
+	prepare_revs(&revs, opts);
+	while ((commit = get_revision(&revs))) {
+		new_item = xmalloc(sizeof(struct commit_list));
+		new_item->item = commit;
+		if (cur)
+			cur->next = new_item;
+		else
+			*todo_list = new_item;
+		cur = new_item;
+	}
+	cur->next = NULL;
+}
+
+static void persist_head(const char *head)
+{
+	static struct lock_file head_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	if (file_exists(SEQ_DIR)) {
+		if (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {
+			strbuf_release(&buf);
+			die(_("Could not remove %s"), SEQ_DIR);
+		}
+	} else {
+		if (mkdir(SEQ_DIR, 0777) < 0) {
+			strbuf_release(&buf);
+			die_errno(_("Could not create sequencer directory '%s'."), SEQ_DIR);
+		}
+	}
+	fd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s\n", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), HEAD_FILE);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), HEAD_FILE);
+}
+
+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)
+{
+	static struct lock_file todo_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);
+	format_todo(&buf, todo_list, opts);
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not write to %s."), TODO_FILE);
+	}
+	if (commit_lock_file(&todo_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s"), TODO_FILE);
+	}
+	strbuf_release(&buf);
+}
+
+static int cleanup_sequencer_data(void)
+{
+	static struct strbuf seq_dir = STRBUF_INIT;
+
+	strbuf_addf(&seq_dir, "%s", SEQ_DIR);
+	if (remove_dir_recursively(&seq_dir, 0) < 0) {
+		strbuf_release(&seq_dir);
+		return error(_("Unable to clean up after successful %s"), me);
+	}
+	strbuf_release(&seq_dir);
+	return 0;
+}
+
+static int pick_commits(struct replay_opts *opts)
+{
+	struct commit_list *todo_list = NULL;
+	unsigned char sha1[20];
+	struct commit_list *cur;
+	int res;
 
-	setenv(GIT_REFLOG_ACTION, me, 0);
 	read_and_refresh_cache(me, opts);
+	setenv(GIT_REFLOG_ACTION, me, 0);
 
-	prepare_revs(&revs, opts);
+	walk_revs_populate_todo(&todo_list, opts);
+	if (!get_sha1("HEAD", sha1))
+		persist_head(sha1_to_hex(sha1));
+	persist_todo(todo_list, opts);
 
-	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit, opts);
+	for (cur = todo_list; cur; cur = cur->next) {
+		persist_todo(cur, opts);
+		res = do_pick_commit(cur->item, opts);
 		if (res)
 			return res;
 	}
 
-	return 0;
+	/* Sequence of picks finished successfully; cleanup by
+	   removing the .git/sequencer directory */
+	return cleanup_sequencer_data();
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
new file mode 100644
index 0000000..83e2722
--- /dev/null
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='Test cherry-pick continuation features
+
+  + picked: rewrites foo to c
+  + unrelatedpick: rewrites unrelated to reallyunrelated
+  + base: rewrites foo to b
+  + initial: writes foo as a, unrelated as unrelated
+
+'
+
+. ./test-lib.sh
+
+pristine_detach () {
+	git checkout -f "$1^0" &&
+	git read-tree -u --reset HEAD &&
+	git clean -d -f -f -q -x
+}
+
+test_expect_success setup '
+	echo unrelated >unrelated &&
+	git add unrelated &&
+	test_commit initial foo a &&
+	test_commit base foo b &&
+	test_commit unrelatedpick unrelated reallyunrelated &&
+	test_commit picked foo c &&
+	git config advice.detachedhead false
+
+'
+
+test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
+	pristine_detach initial &&
+	git cherry-pick initial..picked &&
+	test_path_is_missing .git/sequencer
+'
+
+test_done
-- 
1.7.5.GIT

Re: [PATCH 10/13] revert: Persist data for continuation

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> 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
> implement features like "--continue" to continue the whole operation,
> we will need to store some information about the state and the plan at
> the beginning.  Introduce a ".git/sequencer/head" file to store this
> state, and ".git/sequencer/todo" file to store the plan.

Makes a lot of sense.

> Don't touch
> CHERRY_PICK_HEAD -- it will still be useful when a conflict is
> encountered.

I think there's some logical connector or something missing.  Why would
introducing a .git/sequencer dir involve touching CHERRY_PICK_HEAD?

Maybe the idea is to say: "The purpose of these new files is orthogonal
to the existing CHERRY_PICK_HEAD" with some explanation of that.

> --- 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,10 @@
>   * Copyright (c) 2005 Junio C Hamano
>   */
>  
> +#define SEQ_DIR		git_path("sequencer")
> +#define HEAD_FILE	git_path("sequencer/head")
> +#define TODO_FILE	git_path("sequencer/todo")

I've failed to convince you in the past that these fake constants are
scary, but believe me, they really are.  Consider the following code
--- what would you expect it to print?  What does it actually print?
(Hint: there's not one right answer.)

	printf("%s %s %s %s %s\n", SEQ_DIR, HEAD_FILE, TODO_FILE,
					SEQ_DIR, HEAD_FILE);

> +static void walk_revs_populate_todo(struct commit_list **todo_list,
> +				struct replay_opts *opts)
>  {
>  	struct rev_info revs;
>  	struct commit *commit;
> +	struct commit_list *new_item;
> +	struct commit_list *cur = NULL;
> +
> +	/* Insert into todo_list in the same order */
> +	prepare_revs(&revs, opts);
> +	while ((commit = get_revision(&revs))) {
> +		new_item = xmalloc(sizeof(struct commit_list));
> +		new_item->item = commit;
> +		if (cur)
> +			cur->next = new_item;
> +		else
> +			*todo_list = new_item;
> +		cur = new_item;
> +	}
> +	cur->next = NULL;

The naive reader, perhaps stupidly, wonders: "why not use
commit_list_insert"?  A comment or something to explain "NEEDSWORK:
expose this as commit_list_append" could help him.

[...]
> -	return 0;
> +	/* Sequence of picks finished successfully; cleanup by
> +	   removing the .git/sequencer directory */
> +	return cleanup_sequencer_data();

GNU-style comment seems to have snuck in.

Thanks; this one was pretty pleasant.

Re: [PATCH 10/13] revert: Persist data for continuation

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> ...  Don't touch
> CHERRY_PICK_HEAD -- it will still be useful when a conflict is
> encountered.

What does "Don't touch" here mean? Keep doing the same thing as before so
that people can rely on it? Or stop touching it? I presume the former, but
the description is suboptimal.

> @@ -25,6 +26,10 @@
>   * Copyright (c) 2005 Junio C Hamano
>   */
>  
> +#define SEQ_DIR		git_path("sequencer")
> +#define HEAD_FILE	git_path("sequencer/head")
> +#define TODO_FILE	git_path("sequencer/todo")

HEAD is to keep track of the original, or does it get updated during a
sequencer run?

> +static void walk_revs_populate_todo(struct commit_list **todo_list,
> +				struct replay_opts *opts)
>  {
>  	struct rev_info revs;
>  	struct commit *commit;
> +	struct commit_list *new_item;
> +	struct commit_list *cur = NULL;
> +
> +	/* Insert into todo_list in the same order */
> +	prepare_revs(&revs, opts);
> +	while ((commit = get_revision(&revs))) {
> +		new_item = xmalloc(sizeof(struct commit_list));
> +		new_item->item = commit;
> +		if (cur)
> +			cur->next = new_item;
> +		else
> +			*todo_list = new_item;
> +		cur = new_item;
> +	}
> +	cur->next = NULL;
> +}

Yuck; you do not want if/else to do this (I am _not_ saying "you can do
that but here is a better way"). Have a variable that points at a pointer
of type "struct commit_list *", initialize it to where todo_list points
at, and then update to point at the next field of the new object.

Something like this:

	next = todo_list;
	while ((commit = ...) != NULL) {
		new = xcalloc(1, sizeof(struct commit_list));
                new->item = commit;
                *next = new;
                next = &new->next;
	}
	*next = NULL;

Doing it this way would avoid a segfault when get_revision() returned
nothing as an added bonus ;-).

> +static void persist_head(const char *head)

s/persist/save/ perhaps?

> +{
> +	static struct lock_file head_lock;
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd;
> +

> +	if (file_exists(SEQ_DIR)) {
> +		if (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {
> +			strbuf_release(&buf);
> +			die(_("Could not remove %s"), SEQ_DIR);
> +		}
> +	} else {
> +		if (mkdir(SEQ_DIR, 0777) < 0) {
> +			strbuf_release(&buf);
> +			die_errno(_("Could not create sequencer directory '%s'."), SEQ_DIR);
> +		}
> +	}

Split this part into a separate helper.

> +	fd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);
> +	strbuf_addf(&buf, "%s\n", head);
> +	if (write_in_full(fd, buf.buf, buf.len) < 0)
> +		die_errno(_("Could not write to %s."), HEAD_FILE);
> +	if (commit_lock_file(&head_lock) < 0)
> +		die(_("Error wrapping up %s"), HEAD_FILE);
> +}

Is it sufficient to write the object name of the commit HEAD points at?
Would it be useful to also record the branch if the HEAD points at one?

> +static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)

s/persist/save/ perhaps?

> +{
> +	static struct lock_file todo_lock;
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd;
> +	fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);

The current set of callers might always save head and then todo, but it
probably is a good idea to lift that restriction (which is why I suggested
to separate the initialization of seq-dir into a separate helper).

> +static int pick_commits(struct replay_opts *opts)
> +{
> +	struct commit_list *todo_list = NULL;
> +	unsigned char sha1[20];
> +	struct commit_list *cur;
> +	int res;
>  
> -	setenv(GIT_REFLOG_ACTION, me, 0);
>  	read_and_refresh_cache(me, opts);
> +	setenv(GIT_REFLOG_ACTION, me, 0);

Why?

> -	prepare_revs(&revs, opts);
> +	walk_revs_populate_todo(&todo_list, opts);
> +	if (!get_sha1("HEAD", sha1))
> +		persist_head(sha1_to_hex(sha1));
> +	persist_todo(todo_list, opts);
>  
> -	while ((commit = get_revision(&revs))) {
> -		int res = do_pick_commit(commit, opts);
> +	for (cur = todo_list; cur; cur = cur->next) {
> +		persist_todo(cur, opts);
> +		res = do_pick_commit(cur->item, opts);
>  		if (res)
>  			return res;
>  	}
>  
> -	return 0;
> +	/* Sequence of picks finished successfully; cleanup by
> +	   removing the .git/sequencer directory */

Just a style nit.

        /*
         * We write our multi-line comments
         * this way.
         */

> +	return cleanup_sequencer_data();
>  }

[PATCH 11/13] revert: Introduce a layer of indirection over pick_commits

[Ramkumar Ramachandra]

Write a new function called process_continuation to prepare a
todo_list to call pick_commits with; the job of pick_commits is
simplified into performing the tasks listed in todo_list.  This will
be useful when continuation functionality like "--continue" is
introduced later in the series.

Helped-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   33 ++++++++++++++++++++++-----------
 1 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index ca5756b..5c0b97e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -675,21 +675,13 @@ static int cleanup_sequencer_data(void)
 	return 0;
 }
 
-static int pick_commits(struct replay_opts *opts)
+static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
-	unsigned char sha1[20];
 	struct commit_list *cur;
 	int res;
 
-	read_and_refresh_cache(me, opts);
 	setenv(GIT_REFLOG_ACTION, me, 0);
 
-	walk_revs_populate_todo(&todo_list, opts);
-	if (!get_sha1("HEAD", sha1))
-		persist_head(sha1_to_hex(sha1));
-	persist_todo(todo_list, opts);
-
 	for (cur = todo_list; cur; cur = cur->next) {
 		persist_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
@@ -702,6 +694,21 @@ static int pick_commits(struct replay_opts *opts)
 	return cleanup_sequencer_data();
 }
 
+static int process_continuation(struct replay_opts *opts)
+{
+	struct commit_list *todo_list = NULL;
+	unsigned char sha1[20];
+
+	read_and_refresh_cache(me, opts);
+
+	walk_revs_populate_todo(&todo_list, opts);
+	if (!get_sha1("HEAD", sha1))
+		persist_head(sha1_to_hex(sha1));
+	persist_todo(todo_list, opts);
+
+	return pick_commits(todo_list, opts);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
@@ -714,7 +721,11 @@ 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);
+
+	/* Decide what to do depending on the arguments; a fresh
+	   cherry-pick should be handled differently from an existing
+	   one that is being continued */
+	res = process_continuation(&opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -734,7 +745,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 11/13] revert: Introduce a layer of indirection over pick_commits

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> -static int pick_commits(struct replay_opts *opts)
> +static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>  {

Hmm, I was hoping that we would have something that can express a DAG, but
this interface seems to forever wed us with a linear history. Just making
a mental note, and not rejecting---we have to start from linear before
going fancier.

[PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data

[Ramkumar Ramachandra]

When the sequencer data is persisted after a failed cherry-pick, don't
allow subsequent calls to cherry-pick to clobber this state: instead,
error out with the complaint that an existing cherry-pick is in
progress.  To fix existing tests and the "rebase -i" script, introduce
a new "--skip-all" command-line option to call after every failed
cherry-pick; it essentially clears out the sequencer data, thereby
allowing subsequent calls.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                   |   53 ++++++++++++++++++++++++++++-------
 git-rebase--interactive.sh         |   25 +++++++++++++---
 t/t3032-merge-recursive-options.sh |    2 +
 t/t3501-revert-cherry-pick.sh      |    1 +
 t/t3502-cherry-pick-merge.sh       |    9 +++++-
 t/t3504-cherry-pick-rerere.sh      |    2 +
 t/t3505-cherry-pick-empty.sh       |   14 ++++-----
 t/t3506-cherry-pick-ff.sh          |    3 ++
 t/t3507-cherry-pick-conflict.sh    |   24 +++++++++++++---
 t/t3510-cherry-pick-sequence.sh    |   14 +++++++++
 t/t7502-commit.sh                  |    1 +
 11 files changed, 117 insertions(+), 31 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 5c0b97e..eb68068 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -46,6 +46,9 @@ enum replay_action { REVERT, CHERRY_PICK };
 struct replay_opts {
 	enum replay_action action;
 
+	/* --skip-all */
+	int skipall_oper;
+
 	/* Boolean options */
 	int edit;
 	int record_origin;
@@ -108,6 +111,7 @@ 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, "skip-all", &opts->skipall_oper, "skip remaining instructions"),
 		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 +140,21 @@ 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 incompatible command line arguments */
+	if (opts->skipall_oper) {
+		verify_opt_compatible(me, "--skip-all",
+				"--no-commit", opts->no_commit,
+				"--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)
@@ -624,12 +642,9 @@ static void persist_head(const char *head)
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	if (file_exists(SEQ_DIR)) {
-		if (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {
-			strbuf_release(&buf);
-			die(_("Could not remove %s"), SEQ_DIR);
-		}
-	} else {
+	if (file_exists(SEQ_DIR))
+		die(_("%s already exists.  Please examine and remove before continuing."), SEQ_DIR);
+	else {
 		if (mkdir(SEQ_DIR, 0777) < 0) {
 			strbuf_release(&buf);
 			die_errno(_("Could not create sequencer directory '%s'."), SEQ_DIR);
@@ -701,12 +716,28 @@ static int process_continuation(struct replay_opts *opts)
 
 	read_and_refresh_cache(me, opts);
 
-	walk_revs_populate_todo(&todo_list, opts);
-	if (!get_sha1("HEAD", sha1))
-		persist_head(sha1_to_hex(sha1));
-	persist_todo(todo_list, opts);
+	if (opts->skipall_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return cleanup_sequencer_data();
+	} else {
+		/* Start a new cherry-pick/ revert sequence; but
+		   first, make sure that an existing one isn't in
+		   progress */
+		if (file_exists(TODO_FILE)) {
+			error(_("A %s is already in progress"), me);
+			advise(_("Use %s --skip-all to forget about it"), me);
+			return -1;
+		}
 
+		walk_revs_populate_todo(&todo_list, opts);
+		if (!get_sha1("HEAD", sha1))
+			persist_head(sha1_to_hex(sha1));
+		persist_todo(todo_list, opts);
+	}
 	return pick_commits(todo_list, opts);
+error:
+	return error(_("No %s in progress"), me);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 65690af..e0c9bd8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -136,6 +136,10 @@ make_patch () {
 		get_author_ident_from_commit "$1" > "$author_script"
 }
 
+clear_cherry_pick_state () {
+	git cherry-pick --skip-all
+}
+
 die_with_patch () {
 	echo "$1" > "$state_dir"/stopped-sha
 	make_patch "$1"
@@ -279,8 +283,10 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output git cherry-pick "$@" ||
+			output git cherry-pick "$@" || {
+				clear_cherry_pick_state
 				die_with_patch $sha1 "Could not pick $sha1"
+			}
 			;;
 		esac
 		;;
@@ -385,16 +391,20 @@ do_next () {
 		comment_for_reflog pick
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		record_in_rewritten $sha1
 		;;
 	reword|r)
 		comment_for_reflog reword
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		git commit --amend --no-post-rewrite
 		record_in_rewritten $sha1
 		;;
@@ -402,8 +412,10 @@ do_next () {
 		comment_for_reflog edit
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		echo "$sha1" > "$state_dir"/stopped-sha
 		make_patch $sha1
 		git rev-parse --verify HEAD > "$amend"
@@ -438,7 +450,10 @@ do_next () {
 		echo "$author_script_content" > "$author_script"
 		eval "$author_script_content"
 		output git reset --soft HEAD^
-		pick_one -n $sha1 || die_failed_squash $sha1 "$rest"
+		pick_one -n $sha1 || {
+			clear_cherry_pick_state
+			die_failed_squash $sha1 "$rest"
+		}
 		case "$(peek_next_command)" in
 		squash|s|fixup|f)
 			# This is an intermediate commit; its message will only be
diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
index 2b17311..50e8ca9 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-options.sh
@@ -113,8 +113,10 @@ test_expect_success '--ignore-space-change makes merge succeed' '
 test_expect_success 'naive cherry-pick fails' '
 	git read-tree --reset -u HEAD &&
 	test_must_fail git cherry-pick --no-commit remote &&
+	git cherry-pick --skip-all &&
 	git read-tree --reset -u HEAD &&
 	test_must_fail git cherry-pick remote &&
+	git cherry-pick --skip-all &&
 	test_must_fail git update-index --refresh &&
 	grep "<<<<<<" text.txt
 '
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 595d2ff..8970af4 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -96,6 +96,7 @@ test_expect_success 'revert forbidden on dirty working tree' '
 	echo content >extra_file &&
 	git add extra_file &&
 	test_must_fail git revert HEAD 2>errors &&
+	git revert --skip-all &&
 	test_i18ngrep "Your local changes would be overwritten by " errors
 
 '
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 0ab52da..cc132f8 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -36,6 +36,7 @@ test_expect_success 'cherry-pick a non-merge with -m should fail' '
 	git reset --hard &&
 	git checkout a^0 &&
 	test_must_fail git cherry-pick -m 1 b &&
+	git cherry-pick --skip-all &&
 	git diff --exit-code a --
 
 '
@@ -45,6 +46,7 @@ test_expect_success 'cherry pick a merge without -m should fail' '
 	git reset --hard &&
 	git checkout a^0 &&
 	test_must_fail git cherry-pick c &&
+	git cherry-pick --skip-all &&
 	git diff --exit-code a --
 
 '
@@ -71,8 +73,8 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
 
 	git reset --hard &&
 	git checkout b^0 &&
-	test_must_fail git cherry-pick -m 3 c
-
+	test_must_fail git cherry-pick -m 3 c &&
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'revert a non-merge with -m should fail' '
@@ -80,6 +82,7 @@ test_expect_success 'revert a non-merge with -m should fail' '
 	git reset --hard &&
 	git checkout c^0 &&
 	test_must_fail git revert -m 1 b &&
+	git cherry-pick --skip-all &&
 	git diff --exit-code c
 
 '
@@ -89,6 +92,7 @@ test_expect_success 'revert a merge without -m should fail' '
 	git reset --hard &&
 	git checkout c^0 &&
 	test_must_fail git revert c &&
+	git cherry-pick --skip-all &&
 	git diff --exit-code c
 
 '
@@ -116,6 +120,7 @@ test_expect_success 'revert a merge relative to nonexistent parent should fail'
 	git reset --hard &&
 	git checkout c^0 &&
 	test_must_fail git revert -m 3 c &&
+	git cherry-pick --skip-all &&
 	git diff --exit-code c
 
 '
diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index e6a6481..e7be835 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -29,6 +29,7 @@ test_expect_success 'fixup' '
 
 test_expect_success 'cherry-pick conflict' '
 	test_must_fail git cherry-pick master &&
+	git cherry-pick --skip-all &&
 	test_cmp expect foo
 '
 
@@ -39,6 +40,7 @@ test_expect_success 'reconfigure' '
 
 test_expect_success 'cherry-pick conflict without rerere' '
 	test_must_fail git cherry-pick master &&
+	git cherry-pick --skip-all &&
 	test_must_fail test_cmp expect foo
 '
 
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index c10b28c..c8d828f 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -23,10 +23,9 @@ test_expect_success setup '
 '
 
 test_expect_success 'cherry-pick an empty commit' '
-	git checkout master && {
-		git cherry-pick empty-branch^
-		test "$?" = 1
-	}
+	git checkout master &&
+	test_expect_code 1 git cherry-pick empty-branch^
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'index lockfile was removed' '
@@ -36,10 +35,9 @@ test_expect_success 'index lockfile was removed' '
 '
 
 test_expect_success 'cherry-pick a commit with an empty message' '
-	git checkout master && {
-		git cherry-pick empty-branch
-		test "$?" = 1
-	}
+	git checkout master &&
+	test_expect_code 1 git cherry-pick empty-branch &&
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'index lockfile was removed' '
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index 51ca391..de7368e 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -67,12 +67,14 @@ test_expect_success 'merge setup' '
 test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
 	git reset --hard A -- &&
 	test_must_fail git cherry-pick --ff -m 1 B &&
+	git cherry-pick --skip-all &&
 	git diff --exit-code A --
 '
 
 test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
 	git reset --hard A -- &&
 	test_must_fail git cherry-pick --ff C &&
+	git cherry-pick --skip-all &&
 	git diff --exit-code A --
 '
 
@@ -93,6 +95,7 @@ test_expect_success 'cherry pick with --ff a merge (2)' '
 test_expect_success 'cherry pick a merge relative to nonexistent parent with --ff should fail' '
 	git reset --hard B -- &&
 	test_must_fail git cherry-pick --ff -m 3 C
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'cherry pick a root commit with --ff' '
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 212ec54..07cd805 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -39,6 +39,7 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '
 
 	head=$(git rev-parse HEAD) &&
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --skip-all &&
 	newhead=$(git rev-parse HEAD) &&
 
 	test "$head" = "$newhead"
@@ -55,6 +56,7 @@ test_expect_success 'advice from failed cherry-pick' "
 	hint: and commit the result with 'git commit'
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
+	git cherry-pick --skip-all &&
 
 	test_i18ncmp expected actual
 "
@@ -62,7 +64,8 @@ test_expect_success 'advice from failed cherry-pick' "
 test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick picked &&
-	test_cmp_rev picked CHERRY_PICK_HEAD
+	test_cmp_rev picked CHERRY_PICK_HEAD &&
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '
@@ -84,13 +87,15 @@ test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '
 		export GIT_CHERRY_PICK_HELP &&
 		test_must_fail git cherry-pick picked
 	) &&
-	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'git reset clears CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --skip-all &&
 	git reset &&
 
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
@@ -102,7 +107,8 @@ test_expect_success 'failed commit does not clear CHERRY_PICK_HEAD' '
 	test_must_fail git cherry-pick picked &&
 	test_must_fail git commit &&
 
-	test_cmp_rev picked CHERRY_PICK_HEAD
+	test_cmp_rev picked CHERRY_PICK_HEAD &&
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' '
@@ -119,7 +125,8 @@ test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' '
 		test_must_fail git commit
 	) &&
 
-	test_cmp_rev picked CHERRY_PICK_HEAD
+	test_cmp_rev picked CHERRY_PICK_HEAD &&
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '
@@ -130,13 +137,15 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '
 	git add foo &&
 	git commit &&
 
-	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	git cherry-pick --skip-all
 '
 
 test_expect_success 'failed cherry-pick produces dirty index' '
 	pristine_detach initial &&
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --skip-all &&
 
 	test_must_fail git update-index --refresh -q &&
 	test_must_fail git diff-index --exit-code HEAD
@@ -160,6 +169,7 @@ test_expect_success 'failed cherry-pick registers participants in index' '
 	git read-tree -u --reset HEAD &&
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --skip-all &&
 	git ls-files --stage --unmerged > actual &&
 
 	test_cmp expected actual
@@ -176,6 +186,7 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' '
 	EOF
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --skip-all &&
 
 	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
 	test_cmp expected actual
@@ -195,6 +206,7 @@ test_expect_success 'diff3 -m style' '
 	EOF
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --skip-all &&
 
 	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
 	test_cmp expected actual
@@ -227,6 +239,7 @@ test_expect_success 'revert also handles conflicts sanely' '
 
 	head=$(git rev-parse HEAD) &&
 	test_must_fail git revert picked &&
+	git revert --skip-all &&
 	newhead=$(git rev-parse HEAD) &&
 	git ls-files --stage --unmerged > actual-stages &&
 
@@ -252,6 +265,7 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	EOF
 
 	test_must_fail git revert picked &&
+	git revert --skip-all &&
 
 	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
 	test_cmp expected actual
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 83e2722..a797ae3 100644
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -34,4 +34,18 @@ test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
 	test_path_is_missing .git/sequencer
 '
 
+test_expect_success '--skip-all complains when no cherry-pick is in progress' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --skip-all >actual 2>&1 &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success '--skip-all cleans up sequencer directory' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	git cherry-pick --skip-all &&
+	test_must_fail test -d .git/sequencer
+'
+
 test_done
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 3f3adc3..50d474d 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -291,6 +291,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' '
 	git commit -m second &&
 	# Must fail due to conflict
 	test_must_fail git cherry-pick -n master &&
+	git cherry-pick --skip-all &&
 	echo "editor not started" >.git/result &&
 	(
 		GIT_EDITOR="$(pwd)/.git/FAKE_EDITOR" &&
-- 
1.7.5.GIT

Re: [PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 5c0b97e..eb68068 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -46,6 +46,9 @@ enum replay_action { REVERT, CHERRY_PICK };
>  struct replay_opts {
>  	enum replay_action action;
>  
> +	/* --skip-all */
> +	int skipall_oper;

Yikes what is that "oper" doing there?  Don't truncate a word in the
middle only to shorten names and make it unclear what you want to say. Is
that operand? operation? In this case, I think "int skip_all" is
sufficient, and you can lose the comment that adds no extra information.

Re: [PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index 5c0b97e..eb68068 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -46,6 +46,9 @@ enum replay_action { REVERT, CHERRY_PICK };
>>  struct replay_opts {
>>       enum replay_action action;
>>
>> +     /* --skip-all */
>> +     int skipall_oper;
>
> Yikes what is that "oper" doing there?  Don't truncate a word in the
> middle only to shorten names and make it unclear what you want to say. Is
> that operand? operation? In this case, I think "int skip_all" is
> sufficient, and you can lose the comment that adds no extra information.

Interesting side note: I'd initially wanted to use "skip_all" and
"continue", but "continue" is a C keyword.  That's why I'd reluctantly
suffixed "_oper" to both for consistency.

-- Ram

Re: [PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data

[Miles Bader]

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Interesting side note: I'd initially wanted to use "skip_all" and
> "continue", but "continue" is a C keyword.  That's why I'd reluctantly
> suffixed "_oper" to both for consistency.

It seems a good idea to restrict such uglification to only those cases
where it's necessary, not make _everything_ ugly just for the sake of
consistency.....

[The traditional thing to do with C-keyword conflicts, when there's no
obvious and natural alternative, seems to just be intentional mispelling
-- "continu", "kontinue", "cont", "_continue", whatever.  Yes, they're
ugly, but people will know why you did it, and they'll forgive you.]

-Miles

-- 
Helpmate, n. A wife, or bitter half.

Re: [PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data

[Ramkumar Ramachandra]

Hi Miles,

Miles Bader writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> Interesting side note: I'd initially wanted to use "skip_all" and
>> "continue", but "continue" is a C keyword.  That's why I'd reluctantly
>> suffixed "_oper" to both for consistency.
>
> It seems a good idea to restrict such uglification to only those cases
> where it's necessary, not make _everything_ ugly just for the sake of
> consistency.....
>
> [The traditional thing to do with C-keyword conflicts, when there's no
> obvious and natural alternative, seems to just be intentional mispelling
> -- "continu", "kontinue", "cont", "_continue", whatever.  Yes, they're
> ugly, but people will know why you did it, and they'll forgive you.]

Interesting.  Are there such examples in the Git codebase as well?

-- Ram

[RFC PATCH 13/13] revert: Introduce --continue to continue the operation

[Ramkumar Ramachandra]

After resolving a conflict, the user has the option to continue the
operation.  Mixing operations (cherry-pick and revert in the same
todo_list) and command-line options are unsupported at this stage.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |  127 +++++++++++++++++++++++++++++++++++++-
 t/t3510-cherry-pick-sequence.sh |   28 +++++++++
 2 files changed, 151 insertions(+), 4 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index eb68068..7c28268 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -46,8 +46,9 @@ enum replay_action { REVERT, CHERRY_PICK };
 struct replay_opts {
 	enum replay_action action;
 
-	/* --skip-all */
+	/* --skip-all and --continue */
 	int skipall_oper;
+	int continue_oper;
 
 	/* Boolean options */
 	int edit;
@@ -106,12 +107,37 @@ 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)
+		goto ok;
+	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);
+	}
+ok:
+	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, "skip-all", &opts->skipall_oper, "skip remaining instructions"),
+		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)",
@@ -141,9 +167,21 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
 
+	/* Check for mutually incompatible command line arguments */
+	verify_opt_mutually_compatible(me,
+				"--skip-all", opts->skipall_oper,
+				"--continue", opts->continue_oper,
+				NULL);
+
 	/* Check for incompatible command line arguments */
-	if (opts->skipall_oper) {
-		verify_opt_compatible(me, "--skip-all",
+	if (opts->skipall_oper || opts->continue_oper) {
+		char *this_oper;
+		if (opts->skipall_oper)
+			this_oper = "--skip-all";
+		else
+			this_oper = "--continue";
+
+		verify_opt_compatible(me, this_oper,
 				"--no-commit", opts->no_commit,
 				"--signoff", opts->signoff,
 				"--mainline", opts->mainline,
@@ -614,6 +652,78 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
 	}
 }
 
+static void read_populate_todo(struct commit_list **todo_list,
+			struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	enum replay_action action;
+	struct commit *commit;
+	struct commit_list *new_item;
+	struct commit_list *cur = NULL;
+	unsigned char commit_sha1[20];
+	char sha1_abbrev[40];
+	char *p;
+	int insn_len = 0;
+	char *insn;
+	int fd;
+
+	fd = open(TODO_FILE, O_RDONLY);
+	if (fd < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not open %s."), TODO_FILE);
+	}
+	if (strbuf_read(&buf, fd, 0) < buf.len) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), TODO_FILE);
+	}
+	close(fd);
+
+	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
+		insn = p;
+		if (!(p = strchr(p, ' ')))
+			goto error;
+		insn_len = p - insn;
+		if (!(p = strchr(p + 1, ' ')))
+			goto error;
+		p += 1;
+		strlcpy(sha1_abbrev, insn + insn_len + 1,
+			p - (insn + insn_len + 1));
+
+		if (!strncmp(insn, "pick", insn_len))
+			action = CHERRY_PICK;
+		else if (!strncmp(insn, "revert", insn_len))
+			action = REVERT;
+		else
+			goto error;
+
+		/* Verify that the action matches up with the one in
+		   opts; we don't support arbitrary instructions */
+		if (action != opts->action)
+			goto error;
+
+		/* Now create a commit corresponding to sha1_abbrev
+		   and put it into the todo_list */
+		if ((get_sha1(sha1_abbrev, commit_sha1) < 0)
+			|| !(commit = lookup_commit_reference(commit_sha1)))
+			goto error;
+		new_item = xmalloc(sizeof(struct commit_list));
+		new_item->item = commit;
+		if (cur)
+			cur->next = new_item;
+		else
+			*todo_list = new_item;
+		cur = new_item;
+	}
+	cur->next = NULL;
+	strbuf_release(&buf);
+
+	return;
+error:
+	strbuf_release(&buf);
+	die(_("Malformed instruction sheet: %s"), TODO_FILE);
+}
+
 static void walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
@@ -720,13 +830,22 @@ static int process_continuation(struct replay_opts *opts)
 		if (!file_exists(TODO_FILE))
 			goto error;
 		return cleanup_sequencer_data();
+	} else if (opts->continue_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		read_populate_todo(&todo_list, opts);
+
+		/* Verify that the conflict has been resolved */
+		if (!index_differs_from("HEAD", 0))
+			todo_list = todo_list->next;
 	} else {
 		/* Start a new cherry-pick/ revert sequence; but
 		   first, make sure that an existing one isn't in
 		   progress */
 		if (file_exists(TODO_FILE)) {
 			error(_("A %s is already in progress"), me);
-			advise(_("Use %s --skip-all to forget about it"), me);
+			advise(_("Use %s --continue to continue the operation"), me);
+			advise(_("or --skip-all to skip all the remaining instructions"), me);
 			return -1;
 		}
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index a797ae3..159be55 100644
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -48,4 +48,32 @@ test_expect_success '--skip-all cleans up sequencer directory' '
 	test_must_fail test -d .git/sequencer
 '
 
+test_expect_success '--continue complains when no cherry-pick is in progress' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success '--continue complains when there are unresolved conflicts' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	test_must_fail git cherry-pick --continue &&
+	git cherry-pick --skip-all
+'
+
+test_expect_success '--continue continues after conflicts are resolved' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	git cherry-pick --continue &&
+	test_must_fail test -d .git/sequencer &&
+	git rev-list --count HEAD >actual &&
+	echo 3 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.5.GIT

Re: [RFC PATCH 13/13] revert: Introduce --continue to continue the operation

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> After resolving a conflict, the user has the option to continue the
> operation.

When reading this, I wonder: "does the user have that option or will
she gain that option after this patch?  And if the latter, what does
the UI look like?  And what is the purpose --- if I just cherry-picked
a commit and ran into a conflict, won't 'git commit' be good enough?"

> Mixing operations (cherry-pick and revert in the same
> todo_list) and command-line options are unsupported at this stage.

It's not obvious what this means.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -614,6 +652,78 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
>  	}
>  }
>  
> +static void read_populate_todo(struct commit_list **todo_list,
> +			struct replay_opts *opts)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	enum replay_action action;
> +	struct commit *commit;
> +	struct commit_list *new_item;
> +	struct commit_list *cur = NULL;
> +	unsigned char commit_sha1[20];
> +	char sha1_abbrev[40];
> +	char *p;
> +	int insn_len = 0;
> +	char *insn;
> +	int fd;

The naive reader is not sure what this function is about and is
intimidated by its size.  (I know you have plans regarding that
already; just mentioning it so it doesn't get forgotten.)

> +		/* Verify that the action matches up with the one in
> +		   opts; we don't support arbitrary instructions */

I haven't reviewed the function in full, but this GNU-style comment
jumped out on the way down while scrolling.  I believe you already
know what comments should look like to be consistent with the rest
of the git codebase.

Ok, I've reached the end.  Probably some of these comments apply to
code that might be changed anyway, but still, hope that helps a
little.  And thanks for working on this.

Re: [PATCH 00/13] Sequencer with continuation features

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> I think I can safely say that
> I'm quite happy with the general state of this series now.

It's certainly starting to shape up.  I'll quickly review the style
and commit messages now, since the part I was lobbying for most has
been dealt with (hoorah!) and the rest of the substantive part seems
to have some potential changes queued up if I understand correctly.
In particular, I have high hopes for changes rippling through once
tests get added to make the detailed behavior more clear. :)

General rule of thumb about style, especially commit messages: if
something looks wrong, it is.  Which is to say, the goal is to make
the code easy to read and hack on, and part of that means avoiding
that moment of surprise that might interfere with someone getting on
with the task at hand.  If I seem to be not making sense, please as
usual do not assume I am right, but think carefully about it and
(ideally) gently correct me if I'm wrong.

Okay, on to the patches.  The general shape of the series is a little
odd --- some die() elimination, removal of globals in preparation for
making this a reusable API, and then the title feature that does not
rely on any of the patches before it.  Still, it makes a kind of sense
from the point of view of development:

 1. First, getting to know the API and using that process to come
    up with obvious improvements.

 2. Next, implementing the new feature.

If I had been writing it, I would have rebased the last 4 patches
against master as a new series (so they could be integrated more
quickly), but I don't mind reading it this way.

Regards,
Jonathan