[GSoC update] Sequencer: The insn sheet format

[GSoC update] Sequencer: The insn sheet format

[Ramkumar Ramachandra]

Hi,

Sorry about the delay -- I was travelling again.  I've chistled
everything until the 12th patch to near-perfection.  One of the major
user-visible changes is renaming '--skip-all' to '--reset'.  Initially
suggested by Christian, I really like this name; I think there will be
enough justification for it, especially after we get 'git reset
--hard' to clear away the sequencer state.  Now, we're back to the
point where we need to finalize the instruction sheet format.

Thanks for reading.

-- Ram

Ramkumar Ramachandra (14):
  advice: Introduce error_resolve_conflict
  revert: Inline add_message_to_msg function
  revert: Don't check lone argument in get_encoding
  revert: Rename no_replay to record_origin
  revert: Propogate errors upwards from do_pick_commit
  revert: Eliminate global "commit" variable
  revert: Introduce struct to keep command-line options
  revert: Separate cmdline parsing from functional code
  revert: Don't create invalid replay_opts in parse_args
  revert: Persist data for continuation
  revert: Introduce a layer of indirection over pick_commits
  revert: Introduce --reset to cleanup sequencer data
  revert: Introduce --continue to continue the operation
  revert: Change insn sheet format

 advice.c                           |   31 ++-
 advice.h                           |    1 +
 builtin/revert.c                   |  657 ++++++++++++++++++++++++++++--------
 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    |  119 +++++++
 t/t7502-commit.sh                  |    1 +
 13 files changed, 720 insertions(+), 169 deletions(-)
 create mode 100644 t/t3510-cherry-pick-sequence.sh

-- 
1.7.5.GIT

[PATCH 01/14] advice: Introduce error_resolve_conflict

[Ramkumar Ramachandra]

Enable future callers to report a conflict and not die immediately by
introducing a new function called error_resolve_conflict.
Re-implement die_resolve_conflict as a call to error_resolve_conflict
followed by a call to die.  Consequently, the message printed by
die_resolve_conflict changes from

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

to

error: 'commit' is not possible because you have unmerged files.
hint: Please, fix them up in the work tree ...
hint: ...
fatal: Exiting because of an unresolved conflict.

Hints are printed using the same advise function introduced in
v1.7.3-rc0~26^2~3 (Introduce advise() to print hints, 2010-08-11).

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 |   31 ++++++++++++++++++++++++-------
 advice.h |    1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/advice.c b/advice.c
index 0be4b5f..a031732 100644
--- a/advice.c
+++ b/advice.c
@@ -19,6 +19,15 @@ static struct {
 	{ "detachedhead", &advice_detached_head },
 };
 
+static void advise(const char *advice, ...)
+{
+	va_list params;
+
+	va_start(params, advice);
+	vreportf("hint: ", advice, params);
+	va_end(params);
+}
+
 int git_default_advice_config(const char *var, const char *value)
 {
 	const char *k = skip_prefix(var, "advice.");
@@ -34,16 +43,24 @@ int git_default_advice_config(const char *var, const char *value)
 	return 0;
 }
 
-void NORETURN die_resolve_conflict(const char *me)
+int error_resolve_conflict(const char *me)
 {
-	if (advice_resolve_conflict)
+	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);
+		advise("Please, fix them up in the work tree,");
+		advise("and then use 'git add/rm <file>' as");
+		advise("appropriate to mark resolution and make a commit,");
+		advise("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/14] advice: Introduce error_resolve_conflict

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Enable future callers to report a conflict and not die immediately by
> introducing a new function called error_resolve_conflict.
> Re-implement die_resolve_conflict as a call to error_resolve_conflict
> followed by a call to die.  Consequently, the message printed by
> die_resolve_conflict changes from
>
> fatal: 'commit' is not possible because you have unmerged files.
>        Please, fix them up in the work tree ...
>        ...
>
> to
>
> error: 'commit' is not possible because you have unmerged files.
> hint: Please, fix them up in the work tree ...
> hint: ...
> fatal: Exiting because of an unresolved conflict.

Thanks!  Personally, I like it (since the tags on the left make it a
little clearer to the reader what is happening).

> --- a/advice.c
> +++ b/advice.c
> @@ -19,6 +19,15 @@ static struct {
>  	{ "detachedhead", &advice_detached_head },
>  };
> 
> +static void advise(const char *advice, ...)
> +{
> +	va_list params;
> +
> +	va_start(params, advice);
> +	vreportf("hint: ", advice, params);
> +	va_end(params);
> +}

Rather than copy+pasting this code verbatim, wouldn't it make sense to
move it and expose it through advice.h so the old call site can use
the same code?

For what it's worth, with that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> --- a/advice.c
>> +++ b/advice.c
>> @@ -19,6 +19,15 @@ static struct {
>>       { "detachedhead", &advice_detached_head },
>>  };
>>
>> +static void advise(const char *advice, ...)
>> +{
>> +     va_list params;
>> +
>> +     va_start(params, advice);
>> +     vreportf("hint: ", advice, params);
>> +     va_end(params);
>> +}
>
> Rather than copy+pasting this code verbatim, wouldn't it make sense to
> move it and expose it through advice.h so the old call site can use
> the same code?

Yes, but I was worried that I shouldn't expose it because your commit
message (2a41df) says:

    It is local to revert.c for now because I am not sure this is
    the right API (we may want to take an array of advice lines or a
    boolean argument for easy suppression of unwanted advice).

So, is it still alright to expose it in advice.h?

> For what it's worth, with that change,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Yes, but I was worried that I shouldn't expose it because your commit
> message (2a41df) says:
>
>     It is local to revert.c for now because I am not sure this is
>     the right API (we may want to take an array of advice lines or a
>     boolean argument for easy suppression of unwanted advice).
>
> So, is it still alright to expose it in advice.h?

Well, presumably this second caller is evidence that it is the right
API, no? :)

Of course the API can still easily be changed later.

[PATCH 02/14] revert: Inline 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.  Additionally, fix a bug introduced in 9509af6 (Make
git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
was being incremented when "\n\n" was not found in "message".

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 1f27c63..19d604c 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,16 @@ 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);
+
+		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

[PATCH 03/14] 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 19d604c..a6b51b1 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

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

[Ramkumar Ramachandra]

Rename the variable corresponding to the "-x" command-line option from
"no_replay" to a more apt "record_origin".

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

diff --git a/builtin/revert.c b/builtin/revert.c
index a6b51b1..1612911 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 struct commit *commit;
 static int commit_argc;
@@ -91,7 +91,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(),
 		};
@@ -465,7 +465,7 @@ static int do_pick_commit(void)
 		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");
@@ -560,7 +560,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
 		if (no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (no_replay)
+		if (record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
 		if (edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
-- 
1.7.5.GIT

[PATCH 05/14] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Currently, the return value from revert_or_cherry_pick is a
non-negative number representing the intended exit status from `git
revert` or `git cherry-pick`.  Change this by replacing some of the
calls to "die" with calls to "error", so that it can return negative
values too.  Postive return values indicate conflicts, while negative
ones indicate other errors.  This return status is propogated updwards
from do_pick_commit, to be finally handled in cmd_cherry_pick and
cmd_revert.

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.

While the full benefits of this patch will only be seen once all the
"die" calls are replaced with calls to "error", its immediate impact
is to change some of the "die:" messages to "error:" messages and
print a new "fatal: cherry-pick failed" message when the operation
fails.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
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 |   69 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1612911..3de7ec4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -250,25 +250,20 @@ 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);
+
+	/* Different translation strings for cherry-pick and revert */
+	if (action == CHERRY_PICK)
+		error(_("Your local changes would be overwritten by %s."), me);
+	else
+		error(_("Your local changes would be overwritten by %s."), 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 +377,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 +395,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 +418,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
@@ -581,14 +576,22 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
+	int res;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res < 0)
+		die(_("%s failed"), me);
+	return res;
 }
 
 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)
+		die(_("%s failed"), me);
+	return res;
 }
-- 
1.7.5.GIT

Re: [PATCH 05/14] revert: Propogate errors upwards from do_pick_commit

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> Currently, the return value from revert_or_cherry_pick is a
> non-negative number representing the intended exit status from `git
> revert` or `git cherry-pick`.  Change this by replacing some of the
> calls to "die" with calls to "error", so that it can return negative
> values too.  Postive return values indicate conflicts, while negative

The above seems to be suggesting that the current return value is a
_problem_, and that this change _fixes_ it.

But I had thought that the bulk of this patch's changes (die-to-error
conversions) were not meant as a means to that end but an end in
themselves.  Wouldn't a clearer problem statement be "Currently,
revert_or_cherry_pick can fail in two ways.  If it encounters
conflicts, it returns a positive number indicating the intended exit
status for the git wrapper to pass on; for all other errors, it
die()s.  Some callers may not like the latter behavior because of
<reasons here>"?

Only after the reader understands that, she will be ready to
appreciate the value of the proposed alternate return value
convention.  Similar comments apply to the commit messages of the few
patches before --- they are not terribly confusing, but they could
still easily be improved by mentioning what problem the patches are
supposed to solve.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -250,25 +250,20 @@ static struct tree *empty_tree(void)
[...]
> +	if (action == CHERRY_PICK)
> +		error(_("Your local changes would be overwritten by %s."), me);
> +	else
> +		error(_("Your local changes would be overwritten by %s."), me);

gettext creates one msgid for these two strings, so translators have
no choice but to give them the same translation.  Is that the intent?

[...]
> +	if (res < 0)
> +		die(_("%s failed"), me);
> +	return res;
>  }

Likewise.

I haven't looked carefully again at the unwinding-after-error, but
aside from that and except as noted above this looks good.

Re: [PATCH 05/14] revert: Propogate errors upwards from do_pick_commit

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -250,25 +250,20 @@ static struct tree *empty_tree(void)
> [...]
>> +     if (action == CHERRY_PICK)
>> +             error(_("Your local changes would be overwritten by %s."), me);
>> +     else
>> +             error(_("Your local changes would be overwritten by %s."), me);
>
> gettext creates one msgid for these two strings, so translators have
> no choice but to give them the same translation.  Is that the intent?
>
> [...]
>> +     if (res < 0)
>> +             die(_("%s failed"), me);
>> +     return res;
>>  }
>
> Likewise.

Oops, I didn't realize that! Will fix.

-- Ram

[PATCH 06/14] 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
variable to be passed around explicitly as an argument for clarity.
This involves changing several functions to take an additional
argument, but no functional changes.  Additionaly, this will permit
more than one commit to be cherry-picked at once, should we choose to
develop this functionality in future.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
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 |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 3de7ec4..fcc2047 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
 
 static int edit, record_origin, 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,7 +181,7 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void write_cherry_pick_head(void)
+static void write_cherry_pick_head(struct commit *commit)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
@@ -359,7 +358,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;
@@ -421,7 +420,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));
 
@@ -466,7 +465,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));
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -544,6 +543,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";
@@ -566,7 +566,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 06/14] revert: Eliminate global "commit" variable

[Jonathan Nieder]

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
> variable to be passed around explicitly as an argument for clarity.
> This involves changing several functions to take an additional
> argument, but no functional changes.  Additionaly, this will permit
> more than one commit to be cherry-picked at once, should we choose to
> develop this functionality in future.

I don't understand the last sentence above --- doesn't "git cherry-pick
A B" work already?

The patch looks good, except for:

[...]
> -static void write_cherry_pick_head(void)
> +static void write_cherry_pick_head(struct commit *commit)
[...]
> -			write_cherry_pick_head();
> +			write_cherry_pick_head(sha1_to_hex(commit));

I don't see how this would even compile.

Re: [PATCH 06/14] revert: Eliminate global "commit" variable

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> 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
>> variable to be passed around explicitly as an argument for clarity.
>> This involves changing several functions to take an additional
>> argument, but no functional changes.  Additionaly, this will permit
>> more than one commit to be cherry-picked at once, should we choose to
>> develop this functionality in future.
>
> I don't understand the last sentence above --- doesn't "git cherry-pick
> A B" work already?

Ugh.  Removed the line.

> The patch looks good, except for:
>
> [...]
>> -static void write_cherry_pick_head(void)
>> +static void write_cherry_pick_head(struct commit *commit)
> [...]
>> -                     write_cherry_pick_head();
>> +                     write_cherry_pick_head(sha1_to_hex(commit));
>
> I don't see how this would even compile.

Rebase fail.  Thanks.

-- Ram

[PATCH 07/14] 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.

The variable "me" is left as a file-scope static variable because it
is not an independent option.  "me" is simply a string that needs to
be inferred from the "action" option, and is kept global to save each
function the trouble of determining it independently.

Unfortunately, this patch introduces a minor regression.  Parsing
strategy-option violates a C89 rule: Initializers cannot refer to
variables whose address is not known at compile time.  Currently, this
rule is violated by some other parts of Git as well, and it is
possible to get GCC to report these instances using the "-std=c89
-pedantic" option.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
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 |  182 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 102 insertions(+), 80 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index fcc2047..f34e51f 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 {
@@ -249,7 +263,7 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static int error_dirty_index(const char *me)
+static int error_dirty_index(const char *me, enum replay_action action)
 {
 	if (read_cache_unmerged())
 		return error_resolve_conflict(me);
@@ -278,7 +292,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;
@@ -299,7 +314,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,
@@ -339,7 +354,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];
@@ -347,9 +362,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;
 	}
@@ -358,7 +373,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;
@@ -368,7 +383,7 @@ static int do_pick_commit(struct commit *commit)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
 
-	if (no_commit) {
+	if (opts->no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
 		 * merge the differences in, so let's compute the tree
@@ -381,7 +396,7 @@ static int do_pick_commit(struct commit *commit)
 		if (get_sha1("HEAD", head))
 			return error(_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			return error_dirty_index(me);
+			return error_dirty_index(me, opts->action);
 	}
 	discard_cache();
 
@@ -393,25 +408,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)
@@ -433,7 +448,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;
@@ -459,18 +474,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)
-			write_cherry_pick_head(sha1_to_hex(commit));
+		if (!opts->no_commit)
+			write_cherry_pick_head(commit);
 	}
 
-	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;
@@ -480,23 +495,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);
@@ -505,18 +520,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"));
@@ -525,7 +540,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);
@@ -540,33 +555,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;
 	}
@@ -577,10 +593,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)
 		die(_("%s failed"), me);
 	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)
 		die(_("%s failed"), me);
 	return res;
-- 
1.7.5.GIT

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> 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 variable "me" is left as a file-scope static variable because it
> is not an independent option.  "me" is simply a string that needs to
> be inferred from the "action" option, and is kept global to save each
> function the trouble of determining it independently.

Hm, would it make sense for there to be a "private" section at the
end of the replay_opts struct for variables like this?

> Unfortunately, this patch introduces a minor regression.  Parsing
> strategy-option violates a C89 rule: Initializers cannot refer to
> variables whose address is not known at compile time.  Currently, this
> rule is violated by some other parts of Git as well, and it is
> possible to get GCC to report these instances using the "-std=c89
> -pedantic" option.

I would be interested in fixing that (as a patch on top, maybe).
What do you suggest:

 A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args
    manipulate a static "struct replay_opts" while pick_commits et al
    pass around a pointer to it

 B. Make parse_args work like this:

	copy from argument to private static struct replay_opts
	call parse_options()
	copy private static struct replay_opts to argument

 C. Use new option types:

	OPT_BOOL_MEMBER('n', "no-commit",
		offsetof(struct replay_opts, no_commit),
		"don't automatically commit"),

    and teach parse_options to take an additional parameter like it
    takes "prefix" now, to be used as a base address for options that
    write to an offset instead of a pointer

I'm leaning towards A but not sure if that would be wasted work in
light of your plans for these APIs in the long run (i.e., is
parse_args() going to be exposed and want to act on a caller-supplied
struct)?

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

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> 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 variable "me" is left as a file-scope static variable because it
>> is not an independent option.  "me" is simply a string that needs to
>> be inferred from the "action" option, and is kept global to save each
>> function the trouble of determining it independently.
>
> Hm, would it make sense for there to be a "private" section at the
> end of the replay_opts struct for variables like this?

No.  My justification: in later steps, we'd want to be able to mix
"pick" and "revert" instructions in the same instruction sheet.  This
will essentially require the parser to return a commit + a replay_opts
struct (which will contain the action information).  There's little
point in storing 100 "revert" strings for the 100 commits we want to
pick when that can easily be inferred from the action.

>> Unfortunately, this patch introduces a minor regression.  Parsing
>> strategy-option violates a C89 rule: Initializers cannot refer to
>> variables whose address is not known at compile time.  Currently, this
>> rule is violated by some other parts of Git as well, and it is
>> possible to get GCC to report these instances using the "-std=c89
>> -pedantic" option.
>
> I would be interested in fixing that (as a patch on top, maybe).
> What do you suggest:
>
>  A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args
>    manipulate a static "struct replay_opts" while pick_commits et al
>    pass around a pointer to it
>
>  B. Make parse_args work like this:
>
>        copy from argument to private static struct replay_opts
>        call parse_options()
>        copy private static struct replay_opts to argument

This is something you suggested earlier, but I find it extremely inelegant.

>  C. Use new option types:
>
>        OPT_BOOL_MEMBER('n', "no-commit",
>                offsetof(struct replay_opts, no_commit),
>                "don't automatically commit"),
>
>    and teach parse_options to take an additional parameter like it
>    takes "prefix" now, to be used as a base address for options that
>    write to an offset instead of a pointer
>
> I'm leaning towards A but not sure if that would be wasted work in
> light of your plans for these APIs in the long run (i.e., is
> parse_args() going to be exposed and want to act on a caller-supplied
> struct)?

Yes, I'm definitely considering exposing parse_args in the future,
especially since I want to support command-line options in my
instruction sheet.  Implementing (C) correctly will probably have
several long-term benefits as well -- what do you feel about it?

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> My justification: in later steps, we'd want to be able to mix
> "pick" and "revert" instructions in the same instruction sheet.  This
> will essentially require the parser to return a commit + a replay_opts
> struct (which will contain the action information).

Side note: is it intended to support insns like

 pick A..B

?

Anyway, the above explanation about the intended use for "struct
replay_opts" (it needs to be small, I guess?) would be a good thing
to add to the commit message, too.  Basically, whatever information a
person needs in order to understand the design is a useful thing to
add.

[...]
> Yes, I'm definitely considering exposing parse_args in the future,
> especially since I want to support command-line options in my
> instruction sheet.

Hm, I am not sure what to think about this direction (is the git
sequencer actually just a fast git shell?  in that case, why is pick
spelled "pick" instead of "cherry-pick"?).  Maybe it's a good thing.

Thanks for some useful clarifications.

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

[Ramkumar Ramachandra]

Hi Jonathan,

Sorry about the delayed replay -- I intended to reply to this earlier;
I'm not sure why I didn't.

Jonathan Nieder writes:
>> My justification: in later steps, we'd want to be able to mix
>> "pick" and "revert" instructions in the same instruction sheet.  This
>> will essentially require the parser to return a commit + a replay_opts
>> struct (which will contain the action information).
>
> Side note: is it intended to support insns like
>
>  pick A..B
>
> ?

I wouldn't point to that instruction specifically, but yes- I plan to
support complex instructions in future.

> Anyway, the above explanation about the intended use for "struct
> replay_opts" (it needs to be small, I guess?) would be a good thing
> to add to the commit message, too.  Basically, whatever information a
> person needs in order to understand the design is a useful thing to
> add.

I'm not yet sure about this.  On a related note, I'd like to ask: will
we encounter two commands like "am" and "revert" which have the same
command-line option name for two different functionality?  If not, I'd
probably like to stick to the per-session opts for as long as possible
(ie. until someone finds a good usecase + implementation for
per-action command-line options).

>> Yes, I'm definitely considering exposing parse_args in the future,
>> especially since I want to support command-line options in my
>> instruction sheet.
>
> Hm, I am not sure what to think about this direction (is the git
> sequencer actually just a fast git shell?  in that case, why is pick
> spelled "pick" instead of "cherry-pick"?).  Maybe it's a good thing.

This is a very important question.  Yes, it's intentionally named
"pick" and not "cherry-pick" because I don't want the Sequencer to
merely be a fast git shell.  That's part of the reason I don't want
arbitrary command-line options on the insn sheet.  I want the
possibility of accommodating complex instructions in the insn sheet
(single instructions that might even involve talking to more than one
git command).  For the same reason, I also felt that "action" is the
most appropriate name for the first word in each line in the insn
sheet.  What I'd call an "instruction" is an action + the relevant
option(s) picked up from the opts sheet.

-- Ram

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The variable "me" is left as a file-scope static variable because it
> is not an independent option.  "me" is simply a string that needs to
> be inferred from the "action" option, and is kept global to save each
> function the trouble of determining it independently.

Would it make more sense to remove the variable, pass "action" around
where only "me" is passed around right now, and introduce a function
"static const char *action_name()" to help places that wants textual
command name for display purposes?

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

[Ramkumar Ramachandra]

Hi Junio,

Again- intended to reply to this earlier; sorry.

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> The variable "me" is left as a file-scope static variable because it
>> is not an independent option.  "me" is simply a string that needs to
>> be inferred from the "action" option, and is kept global to save each
>> function the trouble of determining it independently.
>
> Would it make more sense to remove the variable, pass "action" around
> where only "me" is passed around right now, and introduce a function
> "static const char *action_name()" to help places that wants textual
> command name for display purposes?

Okay, let me put it like this: "me" exists because cherry-pick and
revert functionalities are mixed in the same file; builtin/revert.c.
In future, the sequencer in general will support many more actions --
and we will definitely require an "opts->action to instruction sheet
keyword" translation, and that'll probably be some sort of struct.
Since I'm not sure the function you propose will make it to
sequencer.c, I don't want to introduce it now.  Let's wait and see how
it shapes up.

Thanks.

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Since I'm not sure the function you propose will make it to
> sequencer.c, I don't want to introduce it now.

Wouldn't it be easy to remove such a function later?

Practically speaking, it is not obvious to me that making any of these
variables non-static is needed for "cherry-pick --continue" to work,
but given that most of the state is being made non-static anyway,
readers will be likely to wonder why "me" is left behind.  So the
obvious choices would be to

 a. make "me" a member of the replay_opts struct; or
 b. compute "me" in each function that needs it by calling a helper
    function; or
 c. add some explanation to the commit message to clarify the status
    of "me" as static-but-won't-be-in-the-long-term and a reason for
    that

based on the needs of the current code.  (b) sounds simplest to me,
though I haven't tried it.

[PATCH 08/14] 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 continuing the
cherry-pick or revert.

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 f34e51f..e3a7c9e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -555,17 +555,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"));
@@ -599,7 +594,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)
 		die(_("%s failed"), me);
 	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)
 		die(_("%s failed"), me);
 	return res;
-- 
1.7.5.GIT

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -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)
>  		die(_("%s failed"), me);
>  	return res;

I'd put the "me =" line right after "opts.action =" if doing it this
way.  This means callers to pick_commits() are responsible for setting
the "me" variable and in particular it will not make sense to export
that function to callers outside of this file any more, right?

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

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -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)
>>               die(_("%s failed"), me);
>>       return res;
>
> I'd put the "me =" line right after "opts.action =" if doing it this
> way.  This means callers to pick_commits() are responsible for setting
> the "me" variable and in particular it will not make sense to export
> that function to callers outside of this file any more, right?

Exactly.  Good point.

-- Ram

[PATCH 09/14] revert: Don't create invalid replay_opts in parse_args

[Ramkumar Ramachandra]

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
inelegant design; pick_commits is currently the gatekeeper to the
cherry-pick machinery, but this will change in future.  To futureproof
the code and catch these errors in one place, make sure that an
invalid replay_opts structure is not created by parse_args in the
first place.  Also ensure that regressions in maintaining this
invariant are caught in the future by adding an assertion in
pick_commits.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
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 |   37 ++++++++++++++++++++++++++-----------
 1 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e3a7c9e..27dc538 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;
 }
 
@@ -561,17 +584,9 @@ 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"));
-	}
-
+	if (opts->allow_ff)
+		assert(!(opts->signoff || opts->no_commit ||
+				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
 	prepare_revs(&revs, opts);
-- 
1.7.5.GIT

Re: [PATCH 09/14] revert: Don't create invalid replay_opts in parse_args

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> 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
> inelegant design; pick_commits is currently the gatekeeper to the
> cherry-pick machinery, but this will change in future.  To futureproof
> the code and catch these errors in one place, make sure that an
> invalid replay_opts structure is not created by parse_args in the
> first place.  Also ensure that regressions in maintaining this
> invariant are caught in the future by adding an assertion in
> pick_commits.

Agh!  The above seems totally convoluted, and worse, I can see some of
my own words in there so I feel I am to blame.  Could you please
explain, simply, as though I am just an ordinary person, what the idea
of this patch is?  I've heard you talk before.  You are quite capable
of explaining things clearly.

The patch itself looks good.

[PATCH 10/14] 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.  These new
files are unrelated to the existing CHERRY_PICK_HEAD, which will still
be useful when a conflict is encountered.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
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                |  122 +++++++++++++++++++++++++++++++++++++--
 t/t3510-cherry-pick-sequence.sh |   37 ++++++++++++
 2 files changed, 153 insertions(+), 6 deletions(-)
 create mode 100644 t/t3510-cherry-pick-sequence.sh

diff --git a/builtin/revert.c b/builtin/revert.c
index 27dc538..7d76f92 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		"sequencer"
+#define SEQ_HEAD_FILE	"sequencer/head"
+#define SEQ_TODO_FILE	"sequencer/todo"
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -417,7 +422,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, opts->action);
 	}
@@ -578,10 +583,106 @@ 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;
+	struct commit_list **next;
+
+	prepare_revs(&revs, opts);
+
+	/* Insert into todo_list in the same order */
+	/* NEEDSWORK: Expose this as commit_list_append */
+	next = todo_list;
+	while ((commit = get_revision(&revs))) {
+		new = xmalloc(sizeof(struct commit_list));
+		new->item = commit;
+		*next = new;
+		next = &new->next;
+	}
+	*next = NULL;
+}
+
+static void create_seq_dir(void)
+{
+	if (file_exists(git_path(SEQ_DIR))) {
+		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
+			die(_("Could not remove %s"), git_path(SEQ_DIR));
+	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
+		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
+}
+
+static void save_head(const char *head)
+{
+	static struct lock_file head_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&head_lock, git_path(SEQ_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."), git_path(SEQ_HEAD_FILE));
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), git_path(SEQ_HEAD_FILE));
+}
+
+static void save_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, git_path(SEQ_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."), git_path(SEQ_TODO_FILE));
+	}
+	if (commit_lock_file(&todo_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s"), git_path(SEQ_TODO_FILE));
+	}
+	strbuf_release(&buf);
+}
+
+static int cleanup_sequencer_data(void)
+{
+	static struct strbuf seq_dir = STRBUF_INIT;
+
+	strbuf_addf(&seq_dir, "%s", git_path(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);
 	if (opts->allow_ff)
@@ -589,15 +690,24 @@ static int pick_commits(struct replay_opts *opts)
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
-	prepare_revs(&revs, opts);
+	walk_revs_populate_todo(&todo_list, opts);
+	create_seq_dir();
+	if (!get_sha1("HEAD", sha1))
+		save_head(sha1_to_hex(sha1));
+	save_todo(todo_list, opts);
 
-	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit, opts);
+	for (cur = todo_list; cur; cur = cur->next) {
+		save_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/14] 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.

I think I remember Junio being curious about which commit is stored in
"head"; this might be a good place to put a reminder so future readers
don't have to be confused.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -25,6 +26,10 @@
>   * Copyright (c) 2005 Junio C Hamano
>   */
>  
> +#define SEQ_DIR		"sequencer"
> +#define SEQ_HEAD_FILE	"sequencer/head"
> +#define SEQ_TODO_FILE	"sequencer/todo"

Yay. :)

> @@ -417,7 +422,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);

Remember that "me" is an untranslated command name, and see also
http://thread.gmane.org/gmane.comp.version-control.git/153026

Perhaps it would make sense to do something like

	if (get_sha1("HEAD", head)) {
		if (opts->action == REVERT)
			return error(_("can't revert as initial commit"));
		return error(_("cherry-pick into empty head not supported yet"));
	}

In a way they feel like different operations, anyway.  On the other
hand, there's no reason I can think of not to allow reverting a patch
that only removes files as the initial commit other than not having
implemented it.

> @@ -578,10 +583,106 @@ 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);

Maybe some word like "command", "insn", or "keyword" would be more
suggestive than "action".  It also might be worth mentioning somewhere
(in the commit message?) that this format is inspired by
rebase--interactive's insn sheet.

> +	}
> +}
> +
> +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;
> +	struct commit_list **next;
> +
> +	prepare_revs(&revs, opts);
> +
> +	/* Insert into todo_list in the same order */
> +	/* NEEDSWORK: Expose this as commit_list_append */
> +	next = todo_list;
> +	while ((commit = get_revision(&revs))) {
> +		new = xmalloc(sizeof(struct commit_list));
> +		new->item = commit;
> +		*next = new;
> +		next = &new->next;
> +	}
> +	*next = NULL;

The operation that could be exposed does not include get_revision,
does it?

 /*
  * Example:
  *
  *	struct commit_list *list;
  *	struct commit_list **next = &list;
  *
  *	next = commit_list_append(c1, next);
  *	next = commit_list_append(c2, next);
  *	*next = NULL;
  *	assert(commit_list_count(list) == 2);
  *	return list;
  *
  * Don't forget to NULL-terminate!
  */
 struct commit_list **commit_list_append(struct commit *commit,
					struct commit_list **next)
 {
	struct commit_list *new = xmalloc(sizeof(*new_list));
	new->item = commit;
	*next = new;
	return &new->next;
 }


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

A local variable to cache the git_path result would make this much
easier to read.

> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> new file mode 100644

Should "chmod +x" so the test can be run directly (I forget to do that
all the time).

[...]
> +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
> +'

Thanks for thinking about these things.  Maybe another test
demonstrating that the .git/sequencer directory is left behind on
failure would help put this in context.

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

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> 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.
>
> I think I remember Junio being curious about which commit is stored in
> "head"; this might be a good place to put a reminder so future readers
> don't have to be confused.

Oops, I totally forgot -- sorry Junio.
He suggested that we store the corresponding ref also somewhere.  Have
to think about this some more before the next iteration.

>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -25,6 +26,10 @@
>>   * Copyright (c) 2005 Junio C Hamano
>>   */
>>
>> +#define SEQ_DIR              "sequencer"
>> +#define SEQ_HEAD_FILE        "sequencer/head"
>> +#define SEQ_TODO_FILE        "sequencer/todo"
>
> Yay. :)

Sorry it took me so long to understand this.  Your elaborate
explanation last time drove the point home.

>> @@ -417,7 +422,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);
>
> Remember that "me" is an untranslated command name, and see also
> http://thread.gmane.org/gmane.comp.version-control.git/153026
>
> Perhaps it would make sense to do something like
>
>        if (get_sha1("HEAD", head)) {
>                if (opts->action == REVERT)
>                        return error(_("can't revert as initial commit"));
>                return error(_("cherry-pick into empty head not supported yet"));
>        }
>
> In a way they feel like different operations, anyway.  On the other
> hand, there's no reason I can think of not to allow reverting a patch
> that only removes files as the initial commit other than not having
> implemented it.

Okay.  That would be unrelated to this patch though -- I'll make it a
separate patch and move it to the beginning of the series.

>> @@ -578,10 +583,106 @@ 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);
>
> Maybe some word like "command", "insn", or "keyword" would be more
> suggestive than "action".  It also might be worth mentioning somewhere
> (in the commit message?) that this format is inspired by
> rebase--interactive's insn sheet.

Okay, will do.

> The operation that could be exposed does not include get_revision,
> does it?
>
>  /*
>  * Example:
>  *
>  *     struct commit_list *list;
>  *     struct commit_list **next = &list;
>  *
>  *     next = commit_list_append(c1, next);
>  *     next = commit_list_append(c2, next);
>  *     *next = NULL;
>  *     assert(commit_list_count(list) == 2);
>  *     return list;
>  *
>  * Don't forget to NULL-terminate!
>  */
>  struct commit_list **commit_list_append(struct commit *commit,
>                                        struct commit_list **next)
>  {
>        struct commit_list *new = xmalloc(sizeof(*new_list));
>        new->item = commit;
>        *next = new;
>        return &new->next;
>  }

I would have done this, but I was worried about what the NULL
termination would mean API-wise.  In retrospect, a lot of APIs
described in Documentation/technical are pretty non-trivial, and it's
not obvious how to use it without the documentation.  Would it be okay
to expose this in commit.c and write some documentation?  I already
have two callers.

>> +static void create_seq_dir(void)
>> +{
>> +     if (file_exists(git_path(SEQ_DIR))) {
>> +             if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
>> +                     die(_("Could not remove %s"), git_path(SEQ_DIR));
>> +     } else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
>> +             die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
>> +}
>
> A local variable to cache the git_path result would make this much
> easier to read.

Okay.

>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
>> new file mode 100644
>
> Should "chmod +x" so the test can be run directly (I forget to do that
> all the time).

Good catch.

>> +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
>> +'
>
> Thanks for thinking about these things.  Maybe another test
> demonstrating that the .git/sequencer directory is left behind on
> failure would help put this in context.

Good suggestion.

-- Ram

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>  /*
>   * Example:
>   *
>   *	struct commit_list *list;
>   *	struct commit_list **next = &list;
>   *
>   *	next = commit_list_append(c1, next);
>   *	next = commit_list_append(c2, next);
>   *	*next = NULL;
>   *	assert(commit_list_count(list) == 2);
>   *	return list;
>   *
>   * Don't forget to NULL-terminate!
>   */

I wonder if the optimization to allow omitting unnecessary NULL
termination inside a tight loop by the caller is really worth the
potential trouble, though.

[PATCH 11/14] 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 |   36 +++++++++++++++++++++++++-----------
 1 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7d76f92..8cdcdb6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -677,10 +677,8 @@ 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;
 
@@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
-	walk_revs_populate_todo(&todo_list, opts);
-	create_seq_dir();
-	if (!get_sha1("HEAD", sha1))
-		save_head(sha1_to_hex(sha1));
-	save_todo(todo_list, opts);
-
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
@@ -710,6 +702,22 @@ 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);
+	create_seq_dir();
+	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;
@@ -722,7 +730,13 @@ 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)
 		die(_("%s failed"), me);
 	return res;
@@ -738,7 +752,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)
 		die(_("%s failed"), me);
 	return res;
-- 
1.7.5.GIT

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

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

Why is it called process_continuation?  What is its responsibility?
When would I call it?

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

Is this the new sole entry point to the cherry-pick/revert machinery?
In that case, I'd be mildly tempted to call it something crazy like
start_or_continue_replay(), and even more tempted to split it into
separate entry points for new_replay(), continue_replay(),
abort_replay(), and remove_replay_state() (but please don't trust me
about the names; this is just to get the idea across).

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

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> 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.
>
> Why is it called process_continuation?  What is its responsibility?
> When would I call it?

I wanted a general name for the features I'm writing (--reset,
--continue): I want to call these "continuation features".  I
personally like the term, because it reminds me of the "call/cc" in
Scheme.

>> +     /*
>> +      * 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);
>
> Is this the new sole entry point to the cherry-pick/revert machinery?

Yes.

> In that case, I'd be mildly tempted to call it something crazy like
> start_or_continue_replay(), and even more tempted to split it into
> separate entry points for new_replay(), continue_replay(),
> abort_replay(), and remove_replay_state() (but please don't trust me
> about the names; this is just to get the idea across).

Why? Is introducing new terminology so bad?  Should I explain what I
mean by "continuation" in the commit message/ a comment?

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> In that case, I'd be mildly tempted to call it something crazy like
>> start_or_continue_replay()
[...]
> Why? Is introducing new terminology so bad?  Should I explain what I
> mean by "continuation" in the commit message/ a comment?

If "process_continuation" means "parse .git/sequencer state, which we
are pretending is a serialized continuation object, and either (a)
call it, (b) throw it away, or (c) modify it and then call it", then
yes, how do you expect anyone to know what you are talking about?

Less importantly, starting a cherry-pick (which is what pick_commits()
already does) doesn't seem to fit in that picture.

A simpler jargon-filled description of this model is checkpoint/
restart.  But it is an incomplete analogy and still not a great name.
With a goal of making future writers' lives happier and more
productive in mind, I do not think it is often worth confusing them by
choosing a clever presentation of ideas instead of a clear one.

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

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder writes:
>>> In that case, I'd be mildly tempted to call it something crazy like
>>> start_or_continue_replay()
> [...]
>> Why? Is introducing new terminology so bad?  Should I explain what I
>> mean by "continuation" in the commit message/ a comment?
>
> If "process_continuation" means "parse .git/sequencer state, which we
> are pretending is a serialized continuation object, and either (a)
> call it, (b) throw it away, or (c) modify it and then call it", then
> yes, how do you expect anyone to know what you are talking about?
>
> Less importantly, starting a cherry-pick (which is what pick_commits()
> already does) doesn't seem to fit in that picture.
>
> A simpler jargon-filled description of this model is checkpoint/
> restart.  But it is an incomplete analogy and still not a great name.
> With a goal of making future writers' lives happier and more
> productive in mind, I do not think it is often worth confusing them by
> choosing a clever presentation of ideas instead of a clear one.

Thanks for the elaborate explanation; I can see what's wrong with it
now.  However, I "start_or_continue_or_stop_or_[insert more options
here]_replay" isn't a good name.  I want something future-proof,
because I intend to extend this with more nifty helpers like "skip
one".  Your earlier "pick_revisions" suggestion doesn't sound like a
bad alternative now -- let me know if you have any other suggestions.

Thanks.

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Thanks for the elaborate explanation; I can see what's wrong with it
> now.  However, I "start_or_continue_or_stop_or_[insert more options
> here]_replay" isn't a good name.  I want something future-proof,
> because I intend to extend this with more nifty helpers like "skip
> one".  Your earlier "pick_revisions" suggestion doesn't sound like a
> bad alternative now -- let me know if you have any other suggestions.

Sounds like sensible reasoning.  You are free to choose a name; I just
wanted to make sure the effect of that name is clear.

If I were doing it, I would let the API include multiple entry points,
one for each operation ("start", "abort", "skip one", etc).

Side note: I believe a previous patch had a justification of allowing
for multiple entry points, which I had thought was preparation for
this.  It is possible that that patch some other intended positive
effects, too, though.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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 |   36 +++++++++++++++++++++++++-----------
>  1 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 7d76f92..8cdcdb6 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -677,10 +677,8 @@ 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;
>  
> @@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)
>  				opts->record_origin || opts->edit));
>  	read_and_refresh_cache(me, opts);
>  
> -	walk_revs_populate_todo(&todo_list, opts);
> -	create_seq_dir();
> -	if (!get_sha1("HEAD", sha1))
> -		save_head(sha1_to_hex(sha1));
> -	save_todo(todo_list, opts);
> -
>  	for (cur = todo_list; cur; cur = cur->next) {
>  		save_todo(cur, opts);
>  		res = do_pick_commit(cur->item, opts);
> @@ -710,6 +702,22 @@ 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);
> +	create_seq_dir();
> +	if (!get_sha1("HEAD", sha1))
> +		persist_head(sha1_to_hex(sha1));
> +	persist_todo(todo_list, opts);

Don't these two need forward declarations before their use?

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

[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 7d76f92..8cdcdb6 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -677,10 +677,8 @@ 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;
>>
>> @@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)
>>                               opts->record_origin || opts->edit));
>>       read_and_refresh_cache(me, opts);
>>
>> -     walk_revs_populate_todo(&todo_list, opts);
>> -     create_seq_dir();
>> -     if (!get_sha1("HEAD", sha1))
>> -             save_head(sha1_to_hex(sha1));
>> -     save_todo(todo_list, opts);
>> -
>>       for (cur = todo_list; cur; cur = cur->next) {
>>               save_todo(cur, opts);
>>               res = do_pick_commit(cur->item, opts);
>> @@ -710,6 +702,22 @@ 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);
>> +     create_seq_dir();
>> +     if (!get_sha1("HEAD", sha1))
>> +             persist_head(sha1_to_hex(sha1));
>> +     persist_todo(todo_list, opts);
>
> Don't these two need forward declarations before their use?

Good catch.  I'm surprised the compiler didn't catch these.

-- Ram

[PATCH 12/14] revert: Introduce --reset 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 "--reset" 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>
---
 This is perfect but for the fact that 'git reset --hard' doesn't blow
 away the sequencer state.  Why I haven't implemented that yet: should
 ONLY a hard reset blow away the state?

 builtin/revert.c                   |   54 ++++++++++++++++++++++++++++++-----
 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, 121 insertions(+), 28 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8cdcdb6..9e18d64 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -46,6 +46,8 @@ enum replay_action { REVERT, CHERRY_PICK };
 struct replay_opts {
 	enum replay_action action;
 
+	int reset;
+
 	/* Boolean options */
 	int edit;
 	int record_origin;
@@ -108,6 +110,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, "reset", &opts->reset, "forget the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
@@ -136,7 +139,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->reset) {
+		verify_opt_compatible(me, "--reset",
+				"--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)
@@ -625,8 +642,11 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,
 static void create_seq_dir(void)
 {
 	if (file_exists(git_path(SEQ_DIR))) {
-		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
-			die(_("Could not remove %s"), git_path(SEQ_DIR));
+		error(_("%s already exists."), git_path(SEQ_DIR));
+		advise(_("This usually means that a %s operation is in progress."), me);
+		advise(_("Use %s --continue to continue the operation"), me);
+		advise(_("or use %s --reset to forget about it"), me);
+		die(_("%s failed"), me);
 	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
 		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
 }
@@ -709,13 +729,31 @@ static int process_continuation(struct replay_opts *opts)
 
 	read_and_refresh_cache(me, opts);
 
-	walk_revs_populate_todo(&todo_list, opts);
-	create_seq_dir();
-	if (!get_sha1("HEAD", sha1))
-		persist_head(sha1_to_hex(sha1));
-	persist_todo(todo_list, opts);
+	if (opts->reset) {
+		if (!file_exists(git_path(SEQ_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(git_path(SEQ_TODO_FILE))) {
+			error(_("A %s is already in progress"), me);
+			advise(_("Use %s --reset to forget about it"), me);
+			return -1;
+		}
 
+		walk_revs_populate_todo(&todo_list, opts);
+		create_seq_dir();
+		if (!get_sha1("HEAD", sha1))
+			save_head(sha1_to_hex(sha1));
+		save_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..14c57dd 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 --reset
+}
+
 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..81191ae 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 --reset &&
 	git read-tree --reset -u HEAD &&
 	test_must_fail git cherry-pick remote &&
+	git cherry-pick --reset &&
 	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..e0c805d 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 --reset &&
 	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..db21e47 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 --reset &&
 	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 --reset &&
 	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 --reset
 '
 
 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 --reset &&
 	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 --reset &&
 	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 --reset &&
 	git diff --exit-code c
 
 '
diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index e6a6481..85d5fc1 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 --reset &&
 	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 --reset &&
 	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..dc02227 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 --reset
 '
 
 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 --reset
 '
 
 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..abd63a5 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 --reset &&
 	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 --reset &&
 	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 --reset
 '
 
 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..86f8626 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 --reset &&
 	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 --reset &&
 
 	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 --reset
 '
 
 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 --reset
 '
 
 test_expect_success 'git reset clears CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --reset &&
 	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 --reset
 '
 
 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 --reset
 '
 
 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 --reset
 '
 
 test_expect_success 'failed cherry-pick produces dirty index' '
 	pristine_detach initial &&
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --reset &&
 
 	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 --reset &&
 	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 --reset &&
 
 	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 --reset &&
 
 	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 --reset &&
 	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 --reset &&
 
 	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..9e97a7b 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 '--reset complains when no cherry-pick is in progress' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --reset >actual 2>&1 &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success '--reset cleans up sequencer directory' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	git cherry-pick --reset &&
+	test_path_is_missing .git/sequencer
+'
+
 test_done
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 3f3adc3..0d54027 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 --reset &&
 	echo "editor not started" >.git/result &&
 	(
 		GIT_EDITOR="$(pwd)/.git/FAKE_EDITOR" &&
-- 
1.7.5.GIT

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>  This is perfect but for the fact that 'git reset --hard' doesn't blow
>  away the sequencer state.  Why I haven't implemented that yet: should
>  ONLY a hard reset blow away the state?

I don't know.  What do you think?

The constraints I see are:

 1. outside scripts that use "git cherry-pick" should continue to work

 2. as a small indication that that's vaguely possible, unrelated parts
    of the test suite should not need to be patched

 3. when a person uses commands like "git reset --hard" without
   _intending_ to blow away the sequencer state, it should be possible
   to get the sequencer state back.

For dealing with "git rebase --interactive" and similar porcelain-ish
scripts, the CHERRY_PICK_HEAD code-path has its own trick of falling
back to traditional behavior when the GIT_CHERRY_PICK_HELP environment
variable is set (see v1.5.4-rc0~106^2~1, revert/cherry-pick: Allow
overriding the help text by the calling Porcelain, 2007-11-28).

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>>  This is perfect but for the fact that 'git reset --hard' doesn't blow
>>  away the sequencer state.  Why I haven't implemented that yet: should
>>  ONLY a hard reset blow away the state?
>
> I don't know.  What do you think?

I personally don't think any other kind of reset would warrant blowing
away the sequencer state.

> The constraints I see are:
>
>  1. outside scripts that use "git cherry-pick" should continue to work
>
>  2. as a small indication that that's vaguely possible, unrelated parts
>    of the test suite should not need to be patched

I don't see how this is possible.  I'm tempted to say that we should
call this new cherry-picking mechanism with the sequencing
functionality something else like "git sequencer"; then it's possible
to avoid breaking existing scripts.

>  3. when a person uses commands like "git reset --hard" without
>   _intending_ to blow away the sequencer state, it should be possible
>   to get the sequencer state back.

I like your suggestion here -- simply move .git/sequencer to
.git/sequencer-old or similar :)

> For dealing with "git rebase --interactive" and similar porcelain-ish
> scripts, the CHERRY_PICK_HEAD code-path has its own trick of falling
> back to traditional behavior when the GIT_CHERRY_PICK_HELP environment
> variable is set (see v1.5.4-rc0~106^2~1, revert/cherry-pick: Allow
> overriding the help text by the calling Porcelain, 2007-11-28).

Ah, I see.

-- Ram

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

[Ramkumar Ramachandra]

Hi,

Ramkumar Ramachandra writes:
>  builtin/revert.c                   |   54 ++++++++++++++++++++++++++++++-----
>  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, 121 insertions(+), 28 deletions(-)

By making `reset --hard` blow away the sequencer state, and trying
hard not to modify existing scripts, the diffstat still contains these
files:
builtin/revert.c
git-rebase--interactive.sh
t/t3032-merge-recursive-options.sh
r/t3505-cherry-pick-empty.sh
r/t3507-cherry-pick-conflict.sh
r/t3501-cherry-pick-sequence.sh

> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
> index 2b17311..81191ae 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 --reset &&
>        git read-tree --reset -u HEAD &&
>        test_must_fail git cherry-pick remote &&
> +       git cherry-pick --reset &&
>        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..e0c805d 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 --reset &&
>        test_i18ngrep "Your local changes would be overwritten by " errors
>
>  '

> diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
> index c10b28c..dc02227 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 --reset
>  '
>
>  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 --reset
>  '
>
>  test_expect_success 'index lockfile was removed' '

> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 212ec54..86f8626 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 --reset &&
>        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 --reset &&
>
>        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 --reset
>  '
>
>  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 --reset
>  '
>
>  test_expect_success 'git reset clears CHERRY_PICK_HEAD' '
>        pristine_detach initial &&
>
>        test_must_fail git cherry-pick picked &&
> +       git cherry-pick --reset &&
>        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 --reset
>  '
>
>  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 --reset
>  '
>
>  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 --reset
>  '
>
>  test_expect_success 'failed cherry-pick produces dirty index' '
>        pristine_detach initial &&
>
>        test_must_fail git cherry-pick picked &&
> +       git cherry-pick --reset &&
>
>        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 --reset &&
>        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 --reset &&
>
>        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 --reset &&
>
>        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 --reset &&
>        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 --reset &&
>
>        sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
>        test_cmp expected actual

As you can see, there is no "reset --hard" in these, and I don't see
what other command I can piggy-bank on to blow away the sequencer
state.  There is however, one other thing I can do: if there is
nothing left to cherry-pick after a successful conflict resolution +
git commit, I can modify commit.c to blow away the sequencer state
after checking appropriately.  This will also have a nice end-user
experience side-effect:
$ git cherry-pick moo
fatal: Conflict in foo!
$ echo "Resolved" > foo
$ git add moo
$ git commit
$ git cherry-pick --continue # This no-op will be unnecessary

Then again, teaching commit about the sequencer is inelegant, and it's
possible to achieve this effect in another way: when a conflict is
encountered in the sequencer && length(todo_file) == 1, throw away the
sequencer state.  When I say "throw away", I really mean "move
.git/sequencer to .git/sequencer-old".  Does this seem reasonable?

-- Ram

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>> --- a/t/t3507-cherry-pick-conflict.sh
>> +++ b/t/t3507-cherry-pick-conflict.sh
[...]
> As you can see, there is no "reset --hard" in these

In this one, there's "git checkout -f && git read-tree -u --reset HEAD
&& git clean -fdx", which is almost the same thing.

>> --- a/t/t3505-cherry-pick-empty.sh
>> +++ b/t/t3505-cherry-pick-empty.sh

This one is not a typical script, I think --- if you knew the
cherry-pick was going to be empty, why did you try it in the first
place?  I think it would make sense to make it "git reset --hard" at
the beginning of each test as a separate, preparatory patch with
explanation.

[...]
> There is however, one other thing I can do: if there is
> nothing left to cherry-pick after a successful conflict resolution +
> git commit, I can modify commit.c to blow away the sequencer state
> after checking appropriately.  This will also have a nice end-user
> experience side-effect:
> $ git cherry-pick moo
> fatal: Conflict in foo!
> $ echo "Resolved" > foo
> $ git add moo
> $ git commit
> $ git cherry-pick --continue # This no-op will be unnecessary

Though it's not obvious to me how this would affect the scripts above,
it sounds like a nice enhancement to me independently, fwiw.

> Then again, teaching commit about the sequencer is inelegant,

It's possible to add some hook-like thing to do this, or to structure
the code as if a hook was used.

> and it's
> possible to achieve this effect in another way: when a conflict is
> encountered in the sequencer && length(todo_file) == 1, throw away the
> sequencer state.

Yep, that seems like basically the same effect.  Are there downsides?
(Maybe years from now when a "git cherry-pick --rewind" is introduced
we would regret this?  But that can be figured out years from now.)

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

[Jonathan Nieder]

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:

>> and it's
>> possible to achieve this effect in another way: when a conflict is
>> encountered in the sequencer && length(todo_file) == 1, throw away the
>> sequencer state.
>
> Yep, that seems like basically the same effect.  Are there downsides?
> (Maybe years from now when a "git cherry-pick --rewind" is introduced
> we would regret this?  But that can be figured out years from now.)

Doh, I'm not thinking straight.  Would this break "git cherry-pick
--abort", or is there some hack layered on top to avoid that?  Making
"git commit" remove the .git/sequencer for the last commit of the
sequence seems a little saner.

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>
>>> --- a/t/t3507-cherry-pick-conflict.sh
>>> +++ b/t/t3507-cherry-pick-conflict.sh
> [...]
>> As you can see, there is no "reset --hard" in these
>
> In this one, there's "git checkout -f && git read-tree -u --reset HEAD
> && git clean -fdx", which is almost the same thing.

Ofcourse, but it's a combination of three commands -- are you
suggesting that I replace this sequence of commands with "git reset
--hard"?.

>>> --- a/t/t3505-cherry-pick-empty.sh
>>> +++ b/t/t3505-cherry-pick-empty.sh
>
> This one is not a typical script, I think --- if you knew the
> cherry-pick was going to be empty, why did you try it in the first
> place?  I think it would make sense to make it "git reset --hard" at
> the beginning of each test as a separate, preparatory patch with
> explanation.
>
> [...]
>> There is however, one other thing I can do: if there is
>> nothing left to cherry-pick after a successful conflict resolution +
>> git commit, I can modify commit.c to blow away the sequencer state
>> after checking appropriately.  This will also have a nice end-user
>> experience side-effect:
>> $ git cherry-pick moo
>> fatal: Conflict in foo!
>> $ echo "Resolved" > foo
>> $ git add moo
>> $ git commit
>> $ git cherry-pick --continue # This no-op will be unnecessary
>
> Though it's not obvious to me how this would affect the scripts above,
> it sounds like a nice enhancement to me independently, fwiw.

Oh, it fixes everything :)
Just see my GitHub fork.

>> Then again, teaching commit about the sequencer is inelegant,
>
> It's possible to add some hook-like thing to do this, or to structure
> the code as if a hook was used.
>
>> and it's
>> possible to achieve this effect in another way: when a conflict is
>> encountered in the sequencer && length(todo_file) == 1, throw away the
>> sequencer state.
>
> Yep, that seems like basically the same effect.  Are there downsides?
> (Maybe years from now when a "git cherry-pick --rewind" is introduced
> we would regret this?  But that can be figured out years from now.)

> Doh, I'm not thinking straight.  Would this break "git cherry-pick
> --abort", or is there some hack layered on top to avoid that?  Making
> "git commit" remove the .git/sequencer for the last commit of the
> sequence seems a little saner.

We could always inject a hack to avoid this.  I'm not yet convinced
that we should teach commit about the sequencer, especially since the
patch to do it from the sequencer end is so simple.  Please have a
look at the patch [1], and let me know what you think.  I could submit
it as an RFC patch to the list for convenience, but I'm afraid it'll
be missing context.

[1]: https://github.com/artagnon/git/commit/0653bcccfa8d69687ed939f07f5b32dd14d302d3

-- Ram

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

[Ramkumar Ramachandra]

After resolving a conflict, the user simply has to `git commit`
(CHERRY_PICK_HEAD will help), and then `git cherry-pick --continue` to
continue the operation.  Command-line options are currently
unsupported.  Note that a cherry-pick operation cannot be resumed with
a `git revert --continue` and vice-versa.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 As suggested by Miles and Junio, I've obfuscated the variable name to
 "contin".  Would it make sense to persist the entire replay_opts
 structure (so that it applies to all the commits, and no
 commit-specific command-line options are supported) somewhere?  Where
 and how?  My idea is that patch can be considered for inclusion then.
 Patch 14 onwards can then start adding features like "commit-specific
 command-line options", "mixed commands" etc.

 builtin/revert.c                |  130 ++++++++++++++++++++++++++++++++++++++-
 t/t3510-cherry-pick-sequence.sh |   68 ++++++++++++++++++++
 2 files changed, 195 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 9e18d64..6ef56ee 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -47,6 +47,7 @@ struct replay_opts {
 	enum replay_action action;
 
 	int reset;
+	int contin;
 
 	/* Boolean options */
 	int edit;
@@ -105,12 +106,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, "reset", &opts->reset, "forget the current operation"),
+		OPT_BOOLEAN(0, "continue", &opts->contin, "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)",
@@ -140,9 +166,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,
+				"--reset", opts->reset,
+				"--continue", opts->contin,
+				NULL);
+
 	/* Check for incompatible command line arguments */
-	if (opts->reset) {
-		verify_opt_compatible(me, "--reset",
+	if (opts->reset || opts->contin) {
+		char *this_operation;
+		if (opts->reset)
+			this_operation = "--reset";
+		else
+			this_operation = "--continue";
+
+		verify_opt_compatible(me, this_operation,
 				"--no-commit", opts->no_commit,
 				"--signoff", opts->signoff,
 				"--mainline", opts->mainline,
@@ -617,6 +655,83 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
 	}
 }
 
+static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+{
+	unsigned char commit_sha1[20];
+	char sha1_abbrev[40];
+	struct commit *commit;
+	enum replay_action action;
+	int insn_len = 0;
+	char *p;
+
+	p = start;
+	if (!(p = strchr(p, ' ')))
+		return NULL;
+	insn_len = p - start;
+	if (!(p = strchr(p + 1, ' ')))
+		return NULL;
+	p += 1;
+	strlcpy(sha1_abbrev, start + insn_len + 1,
+		p - (start + insn_len + 1));
+
+	if (!strncmp(start, "pick", insn_len))
+		action = CHERRY_PICK;
+	else if (!strncmp(start, "revert", insn_len))
+		action = REVERT;
+	else
+		return NULL;
+
+	/*
+	 * Verify that the action matches up with the one in
+	 * opts; we don't support arbitrary instructions
+	 */
+	if (action != opts->action)
+		return NULL;
+
+	if ((get_sha1(sha1_abbrev, commit_sha1) < 0)
+		|| !(commit = lookup_commit_reference(commit_sha1)))
+		return NULL;
+
+	return commit;
+}
+
+static void read_populate_todo(struct commit_list **todo_list,
+			struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct commit_list *new;
+	struct commit_list **next;
+	struct commit *commit;
+	char *p;
+	int fd;
+
+	fd = open(git_path(SEQ_TODO_FILE), O_RDONLY);
+	if (fd < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not open %s."), git_path(SEQ_TODO_FILE));
+	}
+	if (strbuf_read(&buf, fd, 0) < buf.len) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), git_path(SEQ_TODO_FILE));
+	}
+	close(fd);
+
+	next = todo_list;
+	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
+		if (!(commit = parse_insn_line(p, opts))) {
+			strbuf_release(&buf);
+			die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
+		}
+		new = xmalloc(sizeof(struct commit_list));
+		new->item = commit;
+		*next = new;
+		next = &new->next;
+	}
+	*next = NULL;
+	strbuf_release(&buf);
+}
+
 static void walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
@@ -733,6 +848,14 @@ static int process_continuation(struct replay_opts *opts)
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			goto error;
 		return cleanup_sequencer_data();
+	} else if (opts->contin) {
+		if (!file_exists(git_path(SEQ_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
@@ -741,7 +864,8 @@ static int process_continuation(struct replay_opts *opts)
 		 */
 		if (file_exists(git_path(SEQ_TODO_FILE))) {
 			error(_("A %s is already in progress"), me);
-			advise(_("Use %s --reset to forget about it"), me);
+			advise(_("Use %s --continue to continue the operation"), me);
+			advise(_("or use %s --reset to forget about it"), me);
 			return -1;
 		}
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 9e97a7b..7a57216 100644
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -48,4 +48,72 @@ test_expect_success '--reset cleans up sequencer directory' '
 	test_path_is_missing .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 --reset
+'
+
+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_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/[0-9a-f]\{40\}/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:000000 100644 OBJID OBJID A	foo
+	:000000 100644 OBJID OBJID A	unrelated
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'malformed instruction sheet 1' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick /pick/" .git/sequencer/todo >new_sheet
+	cp new_sheet .git/sequencer/todo
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	git cherry-pick --reset &&
+	test_i18ngrep "fatal" actual
+'
+
+test_expect_success 'malformed instruction sheet 2' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick/revert/" .git/sequencer/todo >new_sheet
+	cp new_sheet .git/sequencer/todo
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	git cherry-pick --reset &&
+	test_i18ngrep "fatal" actual
+'
+
 test_done
-- 
1.7.5.GIT

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -47,6 +47,7 @@ struct replay_opts {
>  	enum replay_action action;
>
>  	int reset;
> +	int contin;

Maybe:

	int just_remove_state;
	int resume;
	int abort;

Or:

	enum replay_subcommand {
		REPLAY_RESET,
		REPLAY_CONTINUE,
		REPLAY_ABORT
	};

	enum replay_subcommand subcommand;

Or perhaps this does not need to be part of the replay_opts struct but
can be communicated by which API function gets called (e.g., via
parse_args returning an "enum replay_subcommand").  I dunno.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 9e18d64..6ef56ee 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -47,6 +47,7 @@ struct replay_opts {
>  	enum replay_action action;
>  
>  	int reset;
> +	int contin;

As Jonathan mentiond, these three look like all action's to me.

> +static void verify_opt_mutually_compatible(const char *me, ...)
> +{

Isn't "being compatible" by definition "mutual"?  

I.e. verify-option-compatibility perhaps?

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

[Drew Northup]

On Wed, 2011-07-06 at 14:21 -0700, Junio C Hamano wrote:
> > +static void verify_opt_mutually_compatible(const char *me, ...)
> > +{
> 
> Isn't "being compatible" by definition "mutual"?  

As a strict matter of language, no. And example we see far too often
@work is that MS Office 2010 is compatible with MS Office 2007, but the
same cannot be said the other way around. (Now if I can convince them
that not everyone in the world has MS Office...)
> 
> I.e. verify-option-compatibility perhaps?

+1 sensible, if that's the intention.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

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

[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 9e18d64..6ef56ee 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -47,6 +47,7 @@ struct replay_opts {
>>       enum replay_action action;
>>
>>       int reset;
>> +     int contin;
>
> As Jonathan mentiond, these three look like all action's to me.

True.  I'll try to figure something out before the next iteration.

>> +static void verify_opt_mutually_compatible(const char *me, ...)
>> +{
>
> Isn't "being compatible" by definition "mutual"?
>
> I.e. verify-option-compatibility perhaps?

There's already a verify_opt_compatible which checks that the first
option supplied is compatible with all the other options.  This one
checks that all the the options all the options are compatible with
each other.  What names do you suggest?

-- Ram

[RFC PATCH 14/14] revert: Change insn sheet format

[Ramkumar Ramachandra]

Add command-line options to the instruction sheet, but don't let the
existing parser break.  Parsing out the command-line options back into
a replay_opts struct is unimplemented.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I've intentionally left parse_cmdline_args unimplemented -- Although
 Stephen (and Christian subsequently) have written a parser [1], I'm
 not happy with it.  The point of this patch is to illustrate the
 problem with this instruction sheet format, and provoke some
 discussion about this.  I don't want to parse command-line arguments
 by hand handling all sorts of corner cases in quoting etc -- is there
 any other way?  Existing implementations in libraries like Glib are
 much too heavyweight.

 Note that I've used the '#' character as a separator to simplify
 parsing.  It's not the most elegant solution, but I think it should
 work.
 
 [1]: http://article.gmane.org/gmane.comp.version-control.git/162198

 builtin/revert.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 6ef56ee..a0a4377 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -638,6 +638,34 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
+static void format_args(struct strbuf *buf, struct replay_opts *opts)
+{
+	int i;
+
+	if (opts->no_commit)
+		strbuf_addstr(buf, "-n ");
+	if (opts->edit)
+		strbuf_addstr(buf, "-e ");
+	if (opts->signoff)
+		strbuf_addstr(buf, "-s ");
+	if (opts->mainline)
+		strbuf_addf(buf, "-m %d ", opts->mainline);
+	if (opts->strategy)
+		strbuf_addf(buf, "--strategy %s ", opts->strategy);
+	if (opts->xopts)
+		for (i = 0; i < opts->xopts_nr; i ++)
+			strbuf_addf(buf, "-X %s ", opts->xopts[i]);
+	if (opts->record_origin)
+		strbuf_addstr(buf, "-x ");
+	if (opts->allow_ff)
+		strbuf_addstr(buf, "--ff ");
+}
+
+/*
+ * Instruction sheet format ::
+ * pick -s 537d2e # revert: Introduce --continue to continue the operation
+ * pick 4a15c1 # revert: Introduce --reset to cleanup sequencer data
+ */
 static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
 			struct replay_opts *opts)
 {
@@ -651,29 +679,53 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
 		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);
+		strbuf_addf(buf, "%s ", action);
+		format_args(buf, opts);
+		strbuf_addf(buf, "%s # %s\n", sha1_abbrev, msg.subject);
 	}
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static void parse_cmdline_args(struct strbuf *args_to_parse,
+			int *argc, const char ***argv)
+{
+	return;
+}
+
+static struct commit *parse_insn_line(const char *start, struct replay_opts *opts)
 {
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
 	struct commit *commit;
 	enum replay_action action;
 	int insn_len = 0;
-	char *p;
+	char *p, *end;
+
+	int argc = 0;
+	const char **argv = NULL;
+	struct strbuf args_to_parse = STRBUF_INIT;
 
-	p = start;
+	p = (char *) start;
 	if (!(p = strchr(p, ' ')))
 		return NULL;
 	insn_len = p - start;
-	if (!(p = strchr(p + 1, ' ')))
+	if (!(end = strchr(p + 1, '#')))
 		return NULL;
-	p += 1;
-	strlcpy(sha1_abbrev, start + insn_len + 1,
-		p - (start + insn_len + 1));
+	*(end - 1) = '\0';
+	strbuf_addstr(&args_to_parse, p + 1);
+
+	/* Parse argc, argv out of args_to_parse */
+	/* TODO: Implement parse_cmdline_args! */
+	parse_cmdline_args(&args_to_parse, &argc, &argv);
+	strbuf_release(&args_to_parse);
+	if (argc)
+		parse_args(argc, argv, opts);
+
+	/* Copy out the sha1_abbrev explicitly */
+	if (!(p = strrchr(p, ' ')))
+		return NULL;
+	strcpy(sha1_abbrev, p + 1);
 
+	/* Determine the action */
 	if (!strncmp(start, "pick", insn_len))
 		action = CHERRY_PICK;
 	else if (!strncmp(start, "revert", insn_len))
@@ -718,7 +770,7 @@ static void read_populate_todo(struct commit_list **todo_list,
 	close(fd);
 
 	next = todo_list;
-	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
+	for (p = buf.buf; *p;) {
 		if (!(commit = parse_insn_line(p, opts))) {
 			strbuf_release(&buf);
 			die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
@@ -727,6 +779,11 @@ static void read_populate_todo(struct commit_list **todo_list,
 		new->item = commit;
 		*next = new;
 		next = &new->next;
+
+		if ((p = strchr(p, '\n')))
+			p += 1;
+		else
+			break;
 	}
 	*next = NULL;
 	strbuf_release(&buf);
-- 
1.7.5.GIT

Re: [RFC PATCH 14/14] revert: Change insn sheet format

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>  I've intentionally left parse_cmdline_args unimplemented
[...]
>                  Existing implementations in libraries like Glib are
>  much too heavyweight.

Wait, how did glib enter the picture? :)  The implementation of
shell-style quoting in [1] is not very complicated; perhaps it could
complement git's existing parsers for shell-style single-quoted
expressions and C-style double-quoted expressions in quote.c.

Of course, a more basic question is whether we want to allow passing
arbitrary command-line arguments through the insn sheet at all (a
part of me wishes "no", at least at first).

Could you give an example to illustrate what this functionality would
be used for?  I can understand wanting to pass "-s" and "-X" flags to
a merge insn and "-X" to pick, but that's as far as my imagination
goes.

> [1]: http://article.gmane.org/gmane.comp.version-control.git/162198

Re: [RFC PATCH 14/14] revert: Change insn sheet format

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>>  I've intentionally left parse_cmdline_args unimplemented
> [...]
>>                  Existing implementations in libraries like Glib are
>>  much too heavyweight.
>
> Wait, how did glib enter the picture? :)

Completely unrelated -- I just incidentally saw "g_shell_parse_argv"
in glib which does what we want, but it's an overkill.

> The implementation of
> shell-style quoting in [1] is not very complicated; perhaps it could
> complement git's existing parsers for shell-style single-quoted
> expressions and C-style double-quoted expressions in quote.c.

Okay.

> Of course, a more basic question is whether we want to allow passing
> arbitrary command-line arguments through the insn sheet at all (a
> part of me wishes "no", at least at first).

I have an overwhelming desire to say "no", but I can't think of an alternative.

> Could you give an example to illustrate what this functionality would
> be used for?  I can understand wanting to pass "-s" and "-X" flags to
> a merge insn and "-X" to pick, but that's as far as my imagination
> goes.

I wasn't imagining anything else.  That's just it -- I've just been
breaking my head trying to figure out how to do it :|

-- Ram

Re: [GSoC update] Sequencer: The insn sheet format

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Thanks for reading.

Thanks for the many improvements!

I think the hardest piece in the series so far is what to do about
the

	git cherry-pick something-i-shouldnt-cherry-pick
	git reset --hard; # or git reset --merge HEAD
	git cherry-pick what-i-should-have-cherry-picked-instead

habit.  Aside from that, it would be nice to have more tests and some
simple documentation, and most other things I found were nitpicks.