[PATCH 0/9] Re-roll rr/revert-cherry-pick v2

[PATCH 0/9] Re-roll rr/revert-cherry-pick v2

[Ramkumar Ramachandra]

Hi,

The previous iteration had 5 parts ($gmane/186425), and this one has
9.  So, I have to explain where the four new patches came from:

- "revert: report fine-grained error messages from insn parser" arises
  from Jonathan's request to split "revert: allow mixed pick and
  revert instructions".

- "revert: tolerate extra spaces, tabs in insn sheet" comes from
  Junio's request to be considerate towards people with fat fingers.

- I put in the other two patches on my own, because I realized that
  the tests needed some tightening, lest they hide underlying bugs
  (it's happened before).

Apart from that, I've just made changes in response to reviews.  I'm
not yet sure what to do about $gmane/186433.

Thanks for reading.

-- Ram

Jonathan Nieder (1):
  revert: simplify communicating command-line arguments

Ramkumar Ramachandra (8):
  revert: free msg in format_todo()
  revert: make commit subjects in insn sheet optional
  revert: tolerate extra spaces, tabs in insn sheet
  revert: simplify getting commit subject in format_todo()
  t3510 (cherry-pick-sequencer): use exit status
  t3510 (cherry-pick-sequencer): remove malformed sheet 2
  revert: allow mixed pick and revert instructions
  revert: report fine-grained error messages from insn parser

 builtin/revert.c                |  237 ++++++++++++++++++++------------------
 t/t3510-cherry-pick-sequence.sh |  157 +++++++++++++++++++++-----
 2 files changed, 252 insertions(+), 142 deletions(-)

-- 
1.7.7.3

[PATCH 1/9] revert: free msg in format_todo()

[Ramkumar Ramachandra]

Memory allocated to the fields of msg by get_message() isn't freed.
This is potentially a big leak, because fresh memory is allocated to
store the commit message for each commit.  Fix this using
free_message().

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..0c6d3d8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 		if (get_message(cur->item, &msg))
 			return error(_("Cannot get commit message for %s"), sha1_abbrev);
 		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+		free_message(&msg);
 	}
 	return 0;
 }
-- 
1.7.7.3

[PATCH 2/9] revert: make commit subjects in insn sheet optional

[Ramkumar Ramachandra]

Change the instruction sheet format subtly so that the subject of the
commit message that follows the object name is optional.  As a result,
an instruction sheet like this is now perfectly valid:

  pick 35b0426
  pick fbd5bbcbc2e
  pick 7362160f

While at it, also fix a bug: currently, we use a commit-id-shaped
buffer to store the word after "pick" in '.git/sequencer/todo'.  This
is both wasteful and wrong because it places an artificial limit on
the line length.  In addition to literal SHA-1 hexes, expressions like
the following are valid object names in the instruction sheet:

  featurebranch~4
  rr/revert-cherry-pick-continue^2~12@{12 days ago}

So, eliminate the need for the buffer to keep the object name
altogether, and add a test demonstrating this.

[jc: simplify parsing]

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c                |   37 ++++++++++++++++---------------------
 t/t3510-cherry-pick-sequence.sh |   28 ++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0c6d3d8..70055e5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
 {
 	unsigned char commit_sha1[20];
-	char sha1_abbrev[40];
 	enum replay_action action;
-	int insn_len = 0;
-	char *p, *q;
+	char *end_of_object_name;
+	int saved, status;
 
-	if (!prefixcmp(start, "pick ")) {
+	if (!prefixcmp(bol, "pick ")) {
 		action = CHERRY_PICK;
-		insn_len = strlen("pick");
-		p = start + insn_len + 1;
-	} else if (!prefixcmp(start, "revert ")) {
+		bol += strlen("pick ");
+	} else if (!prefixcmp(bol, "revert ")) {
 		action = REVERT;
-		insn_len = strlen("revert");
-		p = start + insn_len + 1;
+		bol += strlen("revert ");
 	} else
 		return NULL;
 
-	q = strchr(p, ' ');
-	if (!q)
-		return NULL;
-	q++;
-
-	strlcpy(sha1_abbrev, p, q - p);
+	end_of_object_name = bol + strcspn(bol, " \n");
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	status = get_sha1(bol, commit_sha1);
+	*end_of_object_name = saved;
 
 	/*
 	 * Verify that the action matches up with the one in
@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 		return NULL;
 	}
 
-	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+	if (status < 0)
 		return NULL;
 
 	return lookup_commit_reference(commit_sha1);
@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	int i;
 
 	for (i = 1; *p; i++) {
-		commit = parse_insn_line(p, opts);
+		char *eol = strchrnul(p, '\n');
+		commit = parse_insn_line(p, eol, opts);
 		if (!commit)
 			return error(_("Could not parse line %d."), i);
 		next = commit_list_append(commit, next);
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
+		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)
 		return error(_("No commits parsed."));
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2c4c1c8..6390f2a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -13,6 +13,9 @@ test_description='Test cherry-pick continuation features
 
 . ./test-lib.sh
 
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
 pristine_detach () {
 	git cherry-pick --quit &&
 	git checkout -f "$1^0" &&
@@ -328,4 +331,29 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'malformed instruction sheet 3' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	test_must_fail git cherry-pick --continue
+'
+
+test_expect_success 'commit descriptions in insn sheet are optional' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	git rev-list HEAD >commits &&
+	test_line_count = 4 commits
+'
+
 test_done
-- 
1.7.7.3

Re: [PATCH 2/9] revert: make commit subjects in insn sheet optional

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>                   In addition to literal SHA-1 hexes, expressions like
> the following are valid object names in the instruction sheet:
>
>   featurebranch~4
>   rr/revert-cherry-pick-continue^2~12@{12 days ago}

Glad to see this new mention.  My comment from reviewing the last
iteration doesn't seem to have been addressed, though.  Will follow
up there.

[PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet

[Ramkumar Ramachandra]

Tolerate extra spaces and tabs as part of the the field separator in
'.git/sequencer/todo', for people with fat fingers.

Requested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |    6 +++++-
 t/t3510-cherry-pick-sequence.sh |   11 +++++++++++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 70055e5..b976562 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -727,7 +727,11 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	} else
 		return NULL;
 
-	end_of_object_name = bol + strcspn(bol, " \n");
+	/* Eat up extra spaces/ tabs before object name */
+	while (*bol == ' ' || *bol == '\t')
+		bol += 1;
+
+	end_of_object_name = bol + strcspn(bol, " \t\n");
 	saved = *end_of_object_name;
 	*end_of_object_name = '\0';
 	status = get_sha1(bol, commit_sha1);
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 6390f2a..781c5ac 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -342,6 +342,17 @@ test_expect_success 'malformed instruction sheet 3' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'instruction sheet, fat-fingers version' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick \([0-9a-f]*\)/pick 	 \1 	/" .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	git cherry-pick --continue
+'
+
 test_expect_success 'commit descriptions in insn sheet are optional' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..anotherpick &&
-- 
1.7.7.3

Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> +++ b/builtin/revert.c
> @@ -727,7 +727,11 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
>  	} else
>  		return NULL;
>  
> +	/* Eat up extra spaces/ tabs before object name */
> +	while (*bol == ' ' || *bol == '\t')
> +		bol += 1;
> -	end_of_object_name = bol + strcspn(bol, " \n");

Why not use strspn?  What happens if I use a tab immediately
after the pick/revert keyword?

Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> +++ b/builtin/revert.c
>> @@ -727,7 +727,11 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
>>       } else
>>               return NULL;
>>
>> +     /* Eat up extra spaces/ tabs before object name */
>> +     while (*bol == ' ' || *bol == '\t')
>> +             bol += 1;
>> -     end_of_object_name = bol + strcspn(bol, " \n");
>
> Why not use strspn?  What happens if I use a tab immediately
> after the pick/revert keyword?

:facepalm: Fixed.  Inter-diff:

diff --git a/builtin/revert.c b/builtin/revert.c
index b976562..be0686d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -716,20 +716,22 @@ static struct commit *parse_insn_line(
 	unsigned char commit_sha1[20];
 	enum replay_action action;
 	char *end_of_object_name;
-	int saved, status;
+	int saved, status, padding;

-	if (!prefixcmp(bol, "pick ")) {
+	if (!prefixcmp(bol, "pick")) {
 		action = CHERRY_PICK;
-		bol += strlen("pick ");
-	} else if (!prefixcmp(bol, "revert ")) {
+		bol += strlen("pick");
+	} else if (!prefixcmp(bol, "revert")) {
 		action = REVERT;
-		bol += strlen("revert ");
+		bol += strlen("revert");
 	} else
 		return NULL;

 	/* Eat up extra spaces/ tabs before object name */
-	while (*bol == ' ' || *bol == '\t')
-		bol += 1;
+	padding = strspn(bol, " \t");
+	if (!padding)
+		return NULL;
+	bol += padding;

 	end_of_object_name = bol + strcspn(bol, " \t\n");
 	saved = *end_of_object_name;
--

Thanks.

-- Ram

Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Tolerate extra spaces and tabs as part of the the field separator in
> '.git/sequencer/todo', for people with fat fingers.
>
> Requested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---

I did not request it, by the way. I actually was hinting to start tight
and later loosen the parsing.

Also if you are using strcspn() why use a hand-rolled loop instead of
strspn()?

Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> Also if you are using strcspn() why use a hand-rolled loop instead of
> strspn()?

Honestly, it didn't occur to me: this is the first time I'm using
either strcspn() or stcspn().

Thanks.

-- Ram

[PATCH 4/9] revert: simplify getting commit subject in format_todo()

[Ramkumar Ramachandra]

format_todo() calls get_message(), but uses only the subject line of
the commit message.  As a minor optimization, save work and
unnecessary memory allocations by using find_commit_subject() instead.
Also, remove the unnecessary check on cur->item: the previous patch
makes sure that instruction sheets with missing commit subjects are
parsable.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index b976562..86af516 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -697,16 +697,16 @@ static int 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_str = opts->action == REVERT ? "revert" : "pick";
+	const char *subject;
+	int subject_len;
 
 	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))
-			return error(_("Cannot get commit message for %s"), sha1_abbrev);
-		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
-		free_message(&msg);
+		subject_len = find_commit_subject(cur->item->buffer, &subject);
+		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+			subject_len, subject);
 	}
 	return 0;
 }
-- 
1.7.7.3

Re: [PATCH 4/9] revert: simplify getting commit subject in format_todo()

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> format_todo() calls get_message(), but uses only the subject line of
> the commit message.  As a minor optimization, save work and
> unnecessary memory allocations by using find_commit_subject() instead.

Nice.  Thanks for explaining.

> Also, remove the unnecessary check on cur->item: the previous patch
> makes sure that instruction sheets with missing commit subjects are
> parsable.

I guess you mean the check that cur->item->buffer is non-NULL.  But
now I am confused --- what would that have to do with instruction
sheets with missing commit subjects?  If cur->item->buffer is NULL,
isn't find_commit_subject going to segfault regardless?

Can cur->item->buffer be NULL?

Re: [PATCH 4/9] revert: simplify getting commit subject in format_todo()

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
> Can cur->item->buffer be NULL?

As Junio correctly points out in $gmane/186365, no.  To quote him:

   parse_insn_line() uses lookup_commit_reference() which
   calls parse_object() on the object name (and if it is a tag, deref_tag()
   will parse the tagged object until we see something that is not a tag), so
   we know cur->item is parsed before we see it

>> Also, remove the unnecessary check on cur->item: the previous patch
>> makes sure that instruction sheets with missing commit subjects are
>> parsable.
>
> But now I am confused
> [...]

This part of the commit message is simply awful.  Replaced with:

   Also remove the unnecessary check on cur->item->buffer:
   the lookup_commit_reference() call in parse_insn_line() has already
   made sure of this.

-- Ram

[PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status

[Ramkumar Ramachandra]

Since cf3e2486 (revert: Propagate errors upwards from do_pick_commit,
2011-08-04), 'git cherry-pick' has three different ways of failing:

1. die() with the exit status 128.
3. error() out with exit status -1.
2. exit with positive exit status to indicate a conflict.

However, all the tests asserting its failure use 'test_must_fail',
which simply checks for a non-zero exit status, potentially hiding
underlying bugs.  So, replace all instances of 'test_must_fail' with
'test_expect_code' to check the exit status explicitly.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3510-cherry-pick-sequence.sh |   62 +++++++++++++++++++-------------------
 1 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 781c5ac..9ffc085 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -44,7 +44,7 @@ test_expect_success setup '
 
 test_expect_success 'cherry-pick persists data on failure' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick -s base..anotherpick &&
+	test_expect_code 1 git cherry-pick -s base..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '
 
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
+	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -88,12 +88,12 @@ test_expect_success '--quit does not complain when no cherry-pick is in progress
 
 test_expect_success '--abort requires cherry-pick in progress' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick --abort
+	test_expect_code 128 git cherry-pick --abort
 '
 
 test_expect_success '--quit cleans up sequencer state' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..picked &&
+	test_expect_code 1 git cherry-pick base..picked &&
 	git cherry-pick --quit &&
 	test_path_is_missing .git/sequencer
 '
@@ -107,7 +107,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
 	:000000 100644 OBJID OBJID A	foo
 	:000000 100644 OBJID OBJID A	unrelated
 	EOF
-	test_must_fail git cherry-pick base..picked &&
+	test_expect_code 1 git cherry-pick base..picked &&
 	git cherry-pick --quit &&
 	test_path_is_missing .git/sequencer &&
 	test_must_fail git update-index --refresh &&
@@ -121,7 +121,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
 
 test_expect_success '--abort to cancel multiple cherry-pick' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev initial HEAD &&
@@ -131,7 +131,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' '
 
 test_expect_success '--abort to cancel single cherry-pick' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick picked &&
+	test_expect_code 1 git cherry-pick picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev initial HEAD &&
@@ -141,7 +141,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
 
 test_expect_success 'cherry-pick --abort to cancel multiple revert' '
 	pristine_detach anotherpick &&
-	test_must_fail git revert base..picked &&
+	test_expect_code 1 git revert base..picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev anotherpick HEAD &&
@@ -151,7 +151,7 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' '
 
 test_expect_success 'revert --abort works, too' '
 	pristine_detach anotherpick &&
-	test_must_fail git revert base..picked &&
+	test_expect_code 1 git revert base..picked &&
 	git revert --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev anotherpick HEAD
@@ -159,7 +159,7 @@ test_expect_success 'revert --abort works, too' '
 
 test_expect_success '--abort to cancel single revert' '
 	pristine_detach anotherpick &&
-	test_must_fail git revert picked &&
+	test_expect_code 1 git revert picked &&
 	git revert --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev anotherpick HEAD &&
@@ -170,7 +170,7 @@ test_expect_success '--abort to cancel single revert' '
 test_expect_success '--abort keeps unrelated change, easy case' '
 	pristine_detach unrelatedpick &&
 	echo changed >expect &&
-	test_must_fail git cherry-pick picked..yetanotherpick &&
+	test_expect_code 1 git cherry-pick picked..yetanotherpick &&
 	echo changed >unrelated &&
 	git cherry-pick --abort &&
 	test_cmp expect unrelated
@@ -179,9 +179,9 @@ test_expect_success '--abort keeps unrelated change, easy case' '
 test_expect_success '--abort refuses to clobber unrelated change, harder case' '
 	pristine_detach initial &&
 	echo changed >expect &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo changed >unrelated &&
-	test_must_fail git cherry-pick --abort &&
+	test_expect_code 128 git cherry-pick --abort &&
 	test_cmp expect unrelated &&
 	git rev-list HEAD >log &&
 	test_line_count = 2 log &&
@@ -194,7 +194,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '
 
 test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..picked &&
+	test_expect_code 1 git cherry-pick base..picked &&
 	test_path_is_missing .git/sequencer &&
 	echo "resolved" >foo &&
 	git add foo &&
@@ -218,7 +218,7 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le
 
 test_expect_failure '--abort after last commit in sequence' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..picked &&
+	test_expect_code 1 git cherry-pick base..picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev initial HEAD &&
@@ -228,27 +228,27 @@ test_expect_failure '--abort after last commit in sequence' '
 
 test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	test-chmtime -v +0 .git/sequencer >expect &&
-	test_must_fail git cherry-pick unrelatedpick &&
+	test_expect_code 128 git cherry-pick unrelatedpick &&
 	test-chmtime -v +0 .git/sequencer >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success '--continue complains when no cherry-pick is in progress' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick --continue
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success '--continue complains when there are unresolved conflicts' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
-	test_must_fail git cherry-pick --continue
+	test_expect_code 1 git cherry-pick base..anotherpick &&
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success '--continue continues after conflicts are resolved' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
@@ -275,7 +275,7 @@ test_expect_success '--continue continues after conflicts are resolved' '
 
 test_expect_success '--continue respects opts' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick -x base..anotherpick &&
+	test_expect_code 1 git cherry-pick -x base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
@@ -293,7 +293,7 @@ test_expect_success '--continue respects opts' '
 
 test_expect_success '--signoff is not automatically propagated to resolved conflict' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick --signoff base..anotherpick &&
+	test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
@@ -311,40 +311,40 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 
 test_expect_success 'malformed instruction sheet 1' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	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
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success 'malformed instruction sheet 2' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	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
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success 'malformed instruction sheet 3' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "resolved" >foo &&
 	git add foo &&
 	git commit &&
 	sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
 	cp new_sheet .git/sequencer/todo &&
-	test_must_fail git cherry-pick --continue
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success 'instruction sheet, fat-fingers version' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
@@ -355,7 +355,7 @@ test_expect_success 'instruction sheet, fat-fingers version' '
 
 test_expect_success 'commit descriptions in insn sheet are optional' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
-- 
1.7.7.3

Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Since cf3e2486 (revert: Propagate errors upwards from do_pick_commit,
> 2011-08-04), 'git cherry-pick' has three different ways of failing:
>
> 1. die() with the exit status 128.
> 3. error() out with exit status -1.
> 2. exit with positive exit status to indicate a conflict.

I think your list-item counter is showing a little jitter. :)

error() does not produce exit status -1, and any situation other than
propagating exit status from a user-defined script in which git exits
with status 255 is a bug (yes, I know there are a couple, though none
I know of in cherry-pick code paths).

Hasn't cherry-pick had two ways to exit with failing status like "git
merge" does (conflicts versus error that didn't even allow us to
start) since the very beginning?

[...]
>                   So, replace all instances of 'test_must_fail' with
> 'test_expect_code' to check the exit status explicitly.

Sounds like a sensible idea.  Probably this one sentence, plus a quick
note on the user-visible exit status convention, would suffice for
explaining it.

> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
[...]
> @@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '
>  
>  test_expect_success 'cherry-pick persists opts correctly' '
>  	pristine_detach initial &&
> -	test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
> +	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>  	test_path_is_dir .git/sequencer &&

Encountered conflicts, preserving options, but the exit is with status
128?  Smells like a bug.

[...]
> @@ -88,12 +88,12 @@ test_expect_success '--quit does not complain when no cherry-pick is in progress
>  
>  test_expect_success '--abort requires cherry-pick in progress' '
>  	pristine_detach initial &&
> -	test_must_fail git cherry-pick --abort
> +	test_expect_code 128 git cherry-pick --abort

I don't think the exit status is important for this one.  (I.e., I can
imagine some future version of cherry-pick using different small
positive integers to refer to different reasons for --abort to fail,
and I don't think that would be a problem or break anything.)

[...]
> @@ -179,9 +179,9 @@ test_expect_success '--abort keeps unrelated change, easy case' '
>  test_expect_success '--abort refuses to clobber unrelated change, harder case' '
>  	pristine_detach initial &&
>  	echo changed >expect &&
> -	test_must_fail git cherry-pick base..anotherpick &&
> +	test_expect_code 1 git cherry-pick base..anotherpick &&
>  	echo changed >unrelated &&
> -	test_must_fail git cherry-pick --abort &&
> +	test_expect_code 128 git cherry-pick --abort &&

Likewise here.

Except as noted above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
> [...]
>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
> [...]
>> @@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '
>>
>>  test_expect_success 'cherry-pick persists opts correctly' '
>>       pristine_detach initial &&
>> -     test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>> +     test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>>       test_path_is_dir .git/sequencer &&
>
> Encountered conflicts, preserving options, but the exit is with status
> 128?  Smells like a bug.

No bug.  Notice that "-m 1" is used when "initial" isn't a merge
commit.  But yeah, I should probably clarify this by changing the
revision range to "initial..anotherpick" so as not to distract the
user.

-- Ram

Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:

>>> -     test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>>> +     test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>>>       test_path_is_dir .git/sequencer &&
>>
>> Encountered conflicts, preserving options, but the exit is with status
>> 128?  Smells like a bug.
>
> No bug.

Ok.  I'm fuzzy on the details, but is it possible to make this change
in such a way as to make that obvious?  For example, perhaps this
should be split into several tests: one to check that such mistaken
use of "-m 1" with non-merge commits correctly interrupts the
cherry-pick and pleads to the user for advice (should it?), another to
check that doing so produces an exit status of 128 (if it should), and
another to make sure that doing so, fixing things up somehow, and
resuming the sequence allows the effect of "-m 1" to carry over to
later commits.

[PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2

[Ramkumar Ramachandra]

The next patch allows mixing "pick" and "revert" instruction in the
same instruction sheet.  By removing the "malformed instruction sheet
2" test in advance, it'll be easier to see the changes made by the
next patch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3510-cherry-pick-sequence.sh |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 9ffc085..70fd54b 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -309,7 +309,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 	grep "Signed-off-by:" anotherpick_msg
 '
 
-test_expect_success 'malformed instruction sheet 1' '
+test_expect_success 'malformed instruction sheet, action' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "resolved" >foo &&
@@ -320,18 +320,7 @@ test_expect_success 'malformed instruction sheet 1' '
 	test_expect_code 128 git cherry-pick --continue
 '
 
-test_expect_success 'malformed instruction sheet 2' '
-	pristine_detach initial &&
-	test_expect_code 1 git cherry-pick base..anotherpick &&
-	echo "resolved" >foo &&
-	git add foo &&
-	git commit &&
-	sed "s/pick/revert/" .git/sequencer/todo >new_sheet &&
-	cp new_sheet .git/sequencer/todo &&
-	test_expect_code 128 git cherry-pick --continue
-'
-
-test_expect_success 'malformed instruction sheet 3' '
+test_expect_success 'malformed instruction sheet, object name' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "resolved" >foo &&
-- 
1.7.7.3

Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>                          By removing the "malformed instruction sheet
> 2" test in advance, it'll be easier to see the changes made by the
> next patch.

So, this is a regression in test coverage without a redeeming upside
other than allowing the next patch to be prettier.  Naturally, I don't
like it.

Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>>                          By removing the "malformed instruction sheet
>> 2" test in advance, it'll be easier to see the changes made by the
>> next patch.
>
> So, this is a regression in test coverage without a redeeming upside
> other than allowing the next patch to be prettier.  Naturally, I don't
> like it.

Without this patch, the diffs of _all_ the future commits in this
series touching this file are totally unreadable.  I've noticed that
the diffing algorithm performs especially badly for t/*.sh -- rebasing
tests is generally a huge pain.

-- Ram

Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>                                                    I've noticed that
> the diffing algorithm performs especially badly for t/*.sh -- rebasing
> tests is generally a huge pain.

No clue about this particular situation, but I suspect the general
cause for such rebasing trouble is adding tests at the end of the file
(or some other contended place).  Better to figure out a logical place
for each test and put it there from the start.

[PATCH 7/9] revert: allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Parse the instruction sheet in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming that all instructions use
the same action.  Now you can do:

  pick fdc0b12 picked
  revert 965fed4 anotherpick

For cherry-pick and revert, this means that a 'git cherry-pick
--continue' can continue an ongoing revert operation and viceversa.

Helped-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c                |  133 +++++++++++++++++++--------------------
 t/t3510-cherry-pick-sequence.sh |   58 +++++++++++++++++
 2 files changed, 124 insertions(+), 67 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 86af516..cc55823 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,7 +39,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-enum replay_action { REVERT, CHERRY_PICK };
+enum replay_action { REPLAY_REVERT, REPLAY_PICK };
 enum replay_subcommand {
 	REPLAY_NONE,
 	REPLAY_REMOVE_STATE,
@@ -47,6 +47,12 @@ enum replay_subcommand {
 	REPLAY_ROLLBACK
 };
 
+struct replay_insn_list {
+	enum replay_action action;
+	struct commit *operand;
+	struct replay_insn_list *next;
+};
+
 struct replay_opts {
 	enum replay_action action;
 	enum replay_subcommand subcommand;
@@ -73,14 +79,14 @@ struct replay_opts {
 
 static const char *action_name(const struct replay_opts *opts)
 {
-	return opts->action == REVERT ? "revert" : "cherry-pick";
+	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
 }
 
 static char *get_encoding(const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
@@ -159,7 +165,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 	};
 
-	if (opts->action == CHERRY_PICK) {
+	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -363,7 +369,7 @@ static int error_dirty_index(struct replay_opts *opts)
 		return error_resolve_conflict(action_name(opts));
 
 	/* Different translation strings for cherry-pick and revert */
-	if (opts->action == CHERRY_PICK)
+	if (opts->action == REPLAY_PICK)
 		error(_("Your local changes would be overwritten by cherry-pick."));
 	else
 		error(_("Your local changes would be overwritten by revert."));
@@ -467,7 +473,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+			struct replay_opts *opts)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -542,7 +549,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->action == REVERT) {
+	if (action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -583,7 +590,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -607,13 +614,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
 		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
-	if (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))
+	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
 		write_cherry_pick_head(commit, "REVERT_HEAD");
 
 	if (res) {
-		error(opts->action == REVERT
+		error(action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -637,7 +644,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (opts->action != REVERT)
+	if (opts->action != REPLAY_REVERT)
 		revs->reverse = 1;
 
 	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
@@ -683,49 +690,49 @@ static void read_and_refresh_cache(struct replay_opts *opts)
  *     assert(commit_list_count(list) == 2);
  *     return list;
  */
-static struct commit_list **commit_list_append(struct commit *commit,
-					       struct commit_list **next)
+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+						struct commit *operand,
+						struct replay_insn_list **next)
 {
-	struct commit_list *new = xmalloc(sizeof(struct commit_list));
-	new->item = commit;
+	struct replay_insn_list *new = xmalloc(sizeof(*new));
+	new->action = action;
+	new->operand = operand;
 	*next = new;
 	new->next = NULL;
 	return &new->next;
 }
 
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
-		struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 {
-	struct commit_list *cur = NULL;
-	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REVERT ? "revert" : "pick";
-	const char *subject;
-	int subject_len;
+	struct replay_insn_list *cur;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
-		subject_len = find_commit_subject(cur->item->buffer, &subject);
+		const char *sha1_abbrev, *action_str, *subject;
+		int subject_len;
+
+		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		subject_len = find_commit_subject(cur->operand->buffer, &subject);
 		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
 			subject_len, subject);
 	}
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 {
 	unsigned char commit_sha1[20];
-	enum replay_action action;
 	char *end_of_object_name;
 	int saved, status;
 
 	if (!prefixcmp(bol, "pick ")) {
-		action = CHERRY_PICK;
+		item->action = REPLAY_PICK;
 		bol += strlen("pick ");
 	} else if (!prefixcmp(bol, "revert ")) {
-		action = REVERT;
+		item->action = REPLAY_REVERT;
 		bol += strlen("revert ");
 	} else
-		return NULL;
+		return -1;
 
 	/* Eat up extra spaces/ tabs before object name */
 	while (*bol == ' ' || *bol == '\t')
@@ -737,37 +744,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	status = get_sha1(bol, commit_sha1);
 	*end_of_object_name = saved;
 
-	/*
-	 * Verify that the action matches up with the one in
-	 * opts; we don't support arbitrary instructions
-	 */
-	if (action != opts->action) {
-		const char *action_str;
-		action_str = action == REVERT ? "revert" : "cherry-pick";
-		error(_("Cannot %s during a %s"), action_str, action_name(opts));
-		return NULL;
-	}
-
 	if (status < 0)
-		return NULL;
+		return -1;
+
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return -1;
 
-	return lookup_commit_reference(commit_sha1);
+	item->next = NULL;
+	return 0;
 }
 
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
-			struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 {
-	struct commit_list **next = todo_list;
-	struct commit *commit;
+	struct replay_insn_list **next = todo_list;
+	struct replay_insn_list item = { 0, NULL, NULL };
 	char *p = buf;
 	int i;
 
 	for (i = 1; *p; i++) {
 		char *eol = strchrnul(p, '\n');
-		commit = parse_insn_line(p, eol, opts);
-		if (!commit)
+		if (parse_insn_line(p, eol, &item) < 0)
 			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(item.action, item.operand, next);
 		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)
@@ -775,8 +774,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
-			struct replay_opts *opts)
+static void read_populate_todo(struct replay_insn_list **todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	struct strbuf buf = STRBUF_INIT;
@@ -792,7 +790,7 @@ static void read_populate_todo(struct commit_list **todo_list,
 	}
 	close(fd);
 
-	res = parse_insn_buffer(buf.buf, todo_list, opts);
+	res = parse_insn_buffer(buf.buf, todo_list);
 	strbuf_release(&buf);
 	if (res)
 		die(_("Unusable instruction sheet: %s"), todo_file);
@@ -841,18 +839,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 		die(_("Malformed options sheet: %s"), opts_file);
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
-	struct commit_list **next;
+	struct replay_insn_list **next;
 
 	prepare_revs(&revs, opts);
 
 	next = todo_list;
 	while ((commit = get_revision(&revs)))
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(opts->action, commit, next);
 }
 
 static int create_seq_dir(void)
@@ -950,7 +948,7 @@ fail:
 	return -1;
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
@@ -958,7 +956,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	int fd;
 
 	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list, opts) < 0)
+	if (format_todo(&buf, todo_list) < 0)
 		die(_("Could not format %s."), todo_file);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
@@ -1002,9 +1000,10 @@ static void save_opts(struct replay_opts *opts)
 	}
 }
 
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list,
+			struct replay_opts *opts)
 {
-	struct commit_list *cur;
+	struct replay_insn_list *cur;
 	int res;
 
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -1014,8 +1013,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	read_and_refresh_cache(opts);
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
-		res = do_pick_commit(cur->item, opts);
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
 		if (res) {
 			if (!cur->next)
 				/*
@@ -1040,7 +1039,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 
 static int pick_revisions(struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
+	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
 	read_and_refresh_cache(opts);
@@ -1060,7 +1059,7 @@ static int pick_revisions(struct replay_opts *opts)
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			return error(_("No %s in progress"), action_name(opts));
 		read_populate_opts(&opts);
-		read_populate_todo(&todo_list, opts);
+		read_populate_todo(&todo_list);
 
 		/* Verify that the conflict has been resolved */
 		if (!index_differs_from("HEAD", 0))
@@ -1078,7 +1077,7 @@ static int pick_revisions(struct replay_opts *opts)
 	if (create_seq_dir() < 0)
 		return -1;
 	if (get_sha1("HEAD", sha1)) {
-		if (opts->action == REVERT)
+		if (opts->action == REPLAY_REVERT)
 			return error(_("Can't revert as initial commit"));
 		return error(_("Can't cherry-pick into empty head"));
 	}
@@ -1095,7 +1094,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	if (isatty(0))
 		opts.edit = 1;
-	opts.action = REVERT;
+	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
@@ -1110,7 +1109,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	int res;
 
 	memset(&opts, 0, sizeof(opts));
-	opts.action = CHERRY_PICK;
+	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 70fd54b..aee869d 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
 	test_line_count = 4 commits
 '
 
+test_expect_success 'revert --continue continues after cherry-pick' '
+	pristine_detach initial &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	git revert --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	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 'mixed pick and revert instructions' '
+	pristine_detach initial &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	oldsha=`git rev-parse --short HEAD~1` &&
+	echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	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_done
-- 
1.7.7.3

Re: [PATCH 7/9] revert: allow mixed pick and revert instructions

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Parse the instruction sheet in '.git/sequencer/todo' as a list of
> (action, operand) pairs, instead of assuming that all instructions use
> the same action.  Now you can do:
>
>   pick fdc0b12 picked
>   revert 965fed4 anotherpick
>
> For cherry-pick and revert, this means that a 'git cherry-pick
> --continue' can continue an ongoing revert operation and viceversa.

Sounds like a good thing.

[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -39,7 +39,7 @@ static const char * const cherry_pick_usage[] = {
>  	NULL
>  };
>  
> -enum replay_action { REVERT, CHERRY_PICK };
> +enum replay_action { REPLAY_REVERT, REPLAY_PICK };

What does this have to do with it?

[PATCH 8/9] revert: report fine-grained error messages from insn parser

[Ramkumar Ramachandra]

Three kinds of errors can arise from parsing '.git/sequencer/todo':
1. Unrecognized action
2. Malformed object name
3. Object name does not refer to a valid commit

Since we would like to make the instruction sheet user-editable in the
future (much like the 'rebase -i' sheet), report more fine-grained
parse errors prefixed with the filename and line number.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index cc55823..e2355d8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -719,8 +719,10 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 	return 0;
 }
 
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+			struct replay_insn_list *item, int lineno)
 {
+	const char *todo_file = git_path(SEQ_TODO_FILE);
 	unsigned char commit_sha1[20];
 	char *end_of_object_name;
 	int saved, status;
@@ -731,8 +733,13 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 	} else if (!prefixcmp(bol, "revert ")) {
 		item->action = REPLAY_REVERT;
 		bol += strlen("revert ");
-	} else
-		return -1;
+	} else {
+		size_t len = eol - bol;
+		if (len > 255)
+			len = 255;
+		return error(_("%s:%d: Unrecognized action: %.*s"),
+			todo_file, lineno, (int)len, bol);
+	}
 
 	/* Eat up extra spaces/ tabs before object name */
 	while (*bol == ' ' || *bol == '\t')
@@ -745,11 +752,13 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 	*end_of_object_name = saved;
 
 	if (status < 0)
-		return -1;
+		return error(_("%s:%d: Malformed object name: %s"),
+			todo_file, lineno, bol);
 
 	item->operand = lookup_commit_reference(commit_sha1);
 	if (!item->operand)
-		return -1;
+		return error(_("%s:%d: Not a valid commit: %s"),
+			todo_file, lineno, bol);
 
 	item->next = NULL;
 	return 0;
@@ -764,8 +773,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 
 	for (i = 1; *p; i++) {
 		char *eol = strchrnul(p, '\n');
-		if (parse_insn_line(p, eol, &item) < 0)
-			return error(_("Could not parse line %d."), i);
+		if (parse_insn_line(p, eol, &item, i) < 0)
+			return -1;
 		next = replay_insn_list_append(item.action, item.operand, next);
 		p = *eol ? eol + 1 : eol;
 	}
-- 
1.7.7.3

Re: [PATCH 8/9] revert: report fine-grained error messages from insn parser

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -719,8 +719,10 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
>  	return 0;
>  }
>  
> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> +static int parse_insn_line(char *bol, char *eol,
> +			struct replay_insn_list *item, int lineno)
>  {
> +	const char *todo_file = git_path(SEQ_TODO_FILE);
>  	unsigned char commit_sha1[20];

I know that this function does not call git_path() again before the
value is used, so this is safe today, but I do not trust people in the
future to preserve that property (for example, maybe someone will want
to call get_sha1() earlier).  Why not wait to call git_path() when it
is time to use the value it returns?

[PATCH 9/9] revert: simplify communicating command-line arguments

[Ramkumar Ramachandra]

From: Jonathan Nieder <jrnieder@gmail.com>

Since 7e2bfd3f (revert: allow cherry-picking more than one commit,
2010-07-02), the pick/revert machinery has kept track of the set of
commits to be cherry-picked or reverted using commit_argc and
commit_argv variables, storing the corresponding command-line
parameters.

Future callers as other commands are built in (am, rebase, sequencer)
may find it easier to pass rev-list options to this machinery in
already-parsed form.  So, teach cmd_cherry_pick and cmd_revert to
parse the rev-list arguments in advance and pass the commit set to
pick_revisions() as a "struct rev_info".

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c                |   53 +++++++++++++++++++++-----------------
 t/t3510-cherry-pick-sequence.sh |   11 ++++++++
 2 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e2355d8..8d86bfd 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -66,13 +66,14 @@ struct replay_opts {
 	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;
+
+	/* Only used by REPLAY_NONE */
+	struct rev_info *revs;
 };
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -175,9 +176,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			die(_("program error"));
 	}
 
-	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-					PARSE_OPT_KEEP_ARGV0 |
-					PARSE_OPT_KEEP_UNKNOWN);
+	argc = parse_options(argc, argv, NULL, options, usage_str,
+			PARSE_OPT_KEEP_ARGV0 |
+			PARSE_OPT_KEEP_UNKNOWN);
 
 	/* Check for incompatible subcommands */
 	verify_opt_mutually_compatible(me,
@@ -219,9 +220,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 	}
 
-	else if (opts->commit_argc < 2)
-		usage_with_options(usage_str, options);
-
 	if (opts->allow_ff)
 		verify_opt_compatible(me, "--ff",
 				"--signoff", opts->signoff,
@@ -229,7 +227,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
-	opts->commit_argv = argv;
+
+	if (opts->subcommand == REPLAY_NONE) {
+		opts->revs = xmalloc(sizeof(*opts->revs));
+		init_revisions(opts->revs, NULL);
+		opts->revs->no_walk = 1;
+		if (argc < 2)
+			usage_with_options(usage_str, options);
+		argc = setup_revisions(argc, argv, opts->revs, NULL);
+	} else
+		opts->revs = NULL;
+
+	/* Forbid stray command-line arguments */
+	if (argc > 1)
+		usage_with_options(usage_str, options);
 }
 
 struct commit_message {
@@ -638,23 +649,15 @@ static int do_pick_commit(struct commit *commit, enum replay_action action,
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
+static void prepare_revs(struct replay_opts *opts)
 {
-	int argc;
-
-	init_revisions(revs, NULL);
-	revs->no_walk = 1;
 	if (opts->action != REPLAY_REVERT)
-		revs->reverse = 1;
+		opts->revs->reverse ^= 1;
 
-	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage(opts));
-
-	if (prepare_revision_walk(revs))
+	if (prepare_revision_walk(opts->revs))
 		die(_("revision walk setup failed"));
 
-	if (!revs->commits)
+	if (!opts->revs->commits)
 		die(_("empty commit set passed"));
 }
 
@@ -851,14 +854,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 				struct replay_opts *opts)
 {
-	struct rev_info revs;
 	struct commit *commit;
 	struct replay_insn_list **next;
 
-	prepare_revs(&revs, opts);
+	prepare_revs(opts);
 
 	next = todo_list;
-	while ((commit = get_revision(&revs)))
+	while ((commit = get_revision(opts->revs)))
 		next = replay_insn_list_append(opts->action, commit, next);
 }
 
@@ -1051,6 +1053,9 @@ static int pick_revisions(struct replay_opts *opts)
 	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
 	read_and_refresh_cache(opts);
 
 	/*
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index aee869d..1f4685a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '
 	test_cmp expect actual
 '
 
+test_expect_success 'empty commit set' '
+	pristine_detach initial &&
+	test_expect_code 128 git cherry-pick base..base
+'
+
+test_expect_success 'commit set passed through --all' '
+	pristine_detach initial &&
+	test_expect_code 1 git cherry-pick --all &&
+	git cherry-pick --continue
+'
+
 test_done
-- 
1.7.7.3

Re: [PATCH 9/9] revert: simplify communicating command-line arguments

[Jonathan Nieder]

Hey,

Ramkumar Ramachandra wrote:

> From: Jonathan Nieder <jrnieder@gmail.com>
[...]
> Future callers as other commands are built in (am, rebase, sequencer)
> may find it easier to pass rev-list options to this machinery in
> already-parsed form.  So, teach cmd_cherry_pick and cmd_revert to
> parse the rev-list arguments in advance and pass the commit set to
> pick_revisions() as a "struct rev_info".
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

This sign-off chain suggests that the life of the patch happened in
three stages:

 - first, you wrote the patch or some component of it and passed it to
   me.
 - then, I accepted it, tweaked it so much that I felt the need to claim
   authorship and save you from blame (hence the "From:" field), and
   passed it on to Junio.
 - finally, Junio applied it to some tree.

and that you are sending out a copy of the patch Junio applied for
additional comments.

But in fact, this started with a patch from me (I don't rememember
whether it was signed off, but that doesn't matter --- I happily
retroactively sign off on it) and now you are sending it to the list
and Junio for comments.  So I would think it should simply say

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

and Junio can add his sign-off below that when applying some version
to his tree.  Bonus points if you mention what tweaks you made when
you are not just passing it on.

[...]
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'empty commit set' '
> +	pristine_detach initial &&
> +	test_expect_code 128 git cherry-pick base..base
> +'
> +
> +test_expect_success 'commit set passed through --all' '
> +	pristine_detach initial &&
> +	test_expect_code 1 git cherry-pick --all &&
> +	git cherry-pick --continue
> +'

What does this test mean?  "git cherry-pick --all" should report a
spurious conflict, and then --continue will clean it up automagically
and all will be well?

Re: [PATCH 9/9] revert: simplify communicating command-line arguments

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> [...]
>        Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>        Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>
> and Junio can add his sign-off below that when applying some version
> to his tree.  Bonus points if you mention what tweaks you made when
> you are not just passing it on.

True, the signoff-chain does look like quite a mess now :P
[rr: minor improvements, tests] should be fine.

>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
>> @@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '
>> +test_expect_success 'commit set passed through --all' '
>> +     pristine_detach initial &&
>> +     test_expect_code 1 git cherry-pick --all &&
>> +     git cherry-pick --continue
>> +'
>
> What does this test mean?  "git cherry-pick --all" should report a
> spurious conflict, and then --continue will clean it up automagically
> and all will be well?

This one's actually quite interesting.  "git cherry-pick --all" first
tries to apply everything from "intial" to "yetanotherpatch" (both
inclusive) -- its first "git commit" invocation returns 1, refusing to
create an empty commit.  Then when we say "--continue", it notices
that the worktree and index are clean, removes "initial" from the
instruction sheet and executes everything else as usual.  This is
something we should attempt to fix in the future: I think the Right
Way starts with creating an API for "git commit".

Thanks.

-- Ram

Re: [PATCH 9/9] revert: simplify communicating command-line arguments

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
>> Ramkumar Ramachandra wrote:

>>> +++ b/t/t3510-cherry-pick-sequence.sh
>>> @@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '
>>> +test_expect_success 'commit set passed through --all' '
>>> +     pristine_detach initial &&
>>> +     test_expect_code 1 git cherry-pick --all &&
>>> +     git cherry-pick --continue
>>> +'
[...]
> This one's actually quite interesting.  "git cherry-pick --all" first
> tries to apply everything from "intial" to "yetanotherpatch" (both
> inclusive) -- its first "git commit" invocation returns 1, refusing to
> create an empty commit.  Then when we say "--continue", it notices
> that the worktree and index are clean, removes "initial" from the
> instruction sheet and executes everything else as usual.  This is
> something we should attempt to fix

So, is it a bad test?  Was the initial command crazy and ill-defined
enough that no one would actually do that?  Is the response to the
command incorrect, meaning that the new test should be instead
checking for some different result with test_expect_failure?

I only mentioned --all in the first place because it is a "revision
pseudo-option" (i.e., option starting with "--" whose position on the
rev-list command line matters) and gets handled by a slightly
different revision parsing code path than foo..bar.  There are other
revision pseudo-options that are easier to control and might make for
a better test if it's wanted, like --remotes=foo.

> Way starts with creating an API for "git commit".

Not sure what this means.

Re: [PATCH 9/9] revert: simplify communicating command-line arguments

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
>
> So, is it a bad test?

I'll just drop this test.

>> Way starts with creating an API for "git commit".
>
> Not sure what this means.

It certainly doesn't make any sense when you quote it like that.  What
I meant is that: by convention, 'git cherry-pick' is supposed to exit
with positive status on conflict.  Conversely, this means that a
positive exit status can be interpreted as a conflict, but this is
clearly not the case here.  How do we fix this problem?  By creating
an API for "git commit", not by shelling out like this and letting it
take over the exit status.

-- Ram

Re: [PATCH 9/9] revert: simplify communicating command-line arguments

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>                                                                a
> positive exit status can be interpreted as a conflict, but this is
> clearly not the case here.  How do we fix this problem?  By creating
> an API for "git commit", not by shelling out like this and letting it
> take over the exit status.

That might be a nice thing to do anyway, but I don't see how it would
solve anything.  The new "git commit" API would presumably return an
integer or enum value to indicate the result of trying to commit.
Tests in the testsuite for the "git commit" API would use the "git
commit" command, which would expose the newly fine-grained values
somehow.  And other people scripting but wanting the porcelain to take
care of basic UI would benefit, too.  Right?

Actually, I think cherry-pick returning a positive exit status for
"nothing left to commit after resolving conflicts" would be sensible.
It is "I did what you asked but need your help to determine the final
content of the commit or decide to skip it", rather than "you asked
for something unsensible and I am bailing out".

Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> - "revert: report fine-grained error messages from insn parser" arises
>   from Jonathan's request to split "revert: allow mixed pick and
>   revert instructions".

Just to be clear: I wasn't directly requesting that you do anything.
If I were, then you could carefully read my requirements, fulfill
them, and you would be done.

Instead, I was reviewing the patch and giving my reaction.  After
receiving that information, one has plenty of choices:

 - add documentation to avoid the confusion the reaction was based on
 - rework to fix the underlying problem that caused the reaction
 - think carefully about it, conclude that the reviewer is crazy, and
   ignore it
 - drop the patch
 - send out a call for help to get others to help work on the
   underlying problem
 - ask for clarification
 ...

>From my point of view as a reviewer, I am happiest when someone
figures out how I missed the point and comes up with some fix that
addresses the underlying problem instead (and, incidentally, gets rid
of the symptom that provoked my reaction on the way).

Well, you get the idea.

Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2

[Ramkumar Ramachandra]

Hi,

I'm off on a short vacation next week.  I've already fixed up the
series in response to most (if not all) reviews.  If anyone's
interested in putting in some finishing touches, it's available here:

  https://github.com/artagnon/git.git rr/revert-cherry-pick ;# 195e68e

Thanks.

-- Ram

[PATCH 00/10] Re-roll rr/revert-cherry-pick v3

[Ramkumar Ramachandra]

Hi,

There are 10 parts in this iteration.  The main changes are:

- "t3502, t3510: clarify cherry-pick -m failure" and "revert: move
replay_action, replay_subcommand to header" are new.  They've been
made separate parts for clarity.

- Since jn/maint-sequencer-fixes contains "revert: pass around
rev-list args in already-parsed form", I've dropped it from this
series.  Except for the tests, I've checked that it doesn't conflict
with the topic.

- Rebased on new `master`.

- Other changes are fairly minor, and mainly arise as responses to
  Jonathan's reviews.

Thanks.

-- Ram

Ramkumar Ramachandra (10):
  revert: free msg in format_todo()
  revert: make commit subjects in insn sheet optional
  revert: tolerate extra spaces, tabs in insn sheet
  revert: simplify getting commit subject in format_todo()
  t3510 (cherry-pick-sequencer): use exit status
  t3502, t3510: clarify cherry-pick -m failure
  t3510 (cherry-pick-sequencer): remove malformed sheet 2
  revert: move replay_action, replay_subcommand to header
  revert: allow mixed pick and revert instructions
  revert: report fine-grained error messages from insn parser

 builtin/revert.c                |  194 ++++++++++++++++++++-------------------
 sequencer.h                     |   18 ++++
 t/t3502-cherry-pick-merge.sh    |    2 +-
 t/t3510-cherry-pick-sequence.sh |  142 +++++++++++++++++++++++------
 4 files changed, 231 insertions(+), 125 deletions(-)

-- 
1.7.4.1

[PATCH 01/10] revert: free msg in format_todo()

[Ramkumar Ramachandra]

Memory allocated to the fields of msg by get_message() isn't freed.
This is potentially a big leak, because fresh memory is allocated to
store the commit message for each commit.  Fix this using
free_message().

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..0c6d3d8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 		if (get_message(cur->item, &msg))
 			return error(_("Cannot get commit message for %s"), sha1_abbrev);
 		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+		free_message(&msg);
 	}
 	return 0;
 }
-- 
1.7.4.1

[PATCH 02/10] revert: make commit subjects in insn sheet optional

[Ramkumar Ramachandra]

Change the instruction sheet format subtly so that the subject of the
commit message that follows the object name is optional.  As a result,
an instruction sheet like this is now perfectly valid:

  pick 35b0426
  pick fbd5bbcbc2e
  pick 7362160f

While at it, also fix a bug introduced by 5a5d80f4 (revert: Introduce
--continue to continue the operation, 2011-08-04) that failed to read
lines that are too long to fit on the commit-id-shaped buffer we
currently use; eliminate the need for the buffer altogether.  In
addition to literal SHA-1 hexes, you can now safely use expressions
like the following in the instruction sheet:

  featurebranch~4
  rr/revert-cherry-pick-continue^2~12@{12 days ago}

[jc: simplify parsing]

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c                |   37 ++++++++++++++++---------------------
 t/t3510-cherry-pick-sequence.sh |   28 ++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0c6d3d8..70055e5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
 {
 	unsigned char commit_sha1[20];
-	char sha1_abbrev[40];
 	enum replay_action action;
-	int insn_len = 0;
-	char *p, *q;
+	char *end_of_object_name;
+	int saved, status;
 
-	if (!prefixcmp(start, "pick ")) {
+	if (!prefixcmp(bol, "pick ")) {
 		action = CHERRY_PICK;
-		insn_len = strlen("pick");
-		p = start + insn_len + 1;
-	} else if (!prefixcmp(start, "revert ")) {
+		bol += strlen("pick ");
+	} else if (!prefixcmp(bol, "revert ")) {
 		action = REVERT;
-		insn_len = strlen("revert");
-		p = start + insn_len + 1;
+		bol += strlen("revert ");
 	} else
 		return NULL;
 
-	q = strchr(p, ' ');
-	if (!q)
-		return NULL;
-	q++;
-
-	strlcpy(sha1_abbrev, p, q - p);
+	end_of_object_name = bol + strcspn(bol, " \n");
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	status = get_sha1(bol, commit_sha1);
+	*end_of_object_name = saved;
 
 	/*
 	 * Verify that the action matches up with the one in
@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 		return NULL;
 	}
 
-	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+	if (status < 0)
 		return NULL;
 
 	return lookup_commit_reference(commit_sha1);
@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	int i;
 
 	for (i = 1; *p; i++) {
-		commit = parse_insn_line(p, opts);
+		char *eol = strchrnul(p, '\n');
+		commit = parse_insn_line(p, eol, opts);
 		if (!commit)
 			return error(_("Could not parse line %d."), i);
 		next = commit_list_append(commit, next);
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
+		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)
 		return error(_("No commits parsed."));
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2c4c1c8..6390f2a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -13,6 +13,9 @@ test_description='Test cherry-pick continuation features
 
 . ./test-lib.sh
 
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
 pristine_detach () {
 	git cherry-pick --quit &&
 	git checkout -f "$1^0" &&
@@ -328,4 +331,29 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'malformed instruction sheet 3' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	test_must_fail git cherry-pick --continue
+'
+
+test_expect_success 'commit descriptions in insn sheet are optional' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	git rev-list HEAD >commits &&
+	test_line_count = 4 commits
+'
+
 test_done
-- 
1.7.4.1

[PATCH 03/10] revert: tolerate extra spaces, tabs in insn sheet

[Ramkumar Ramachandra]

Tolerate extra spaces and tabs as part of the the field separator in
'.git/sequencer/todo', for people with fat fingers.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |   18 ++++++++++++------
 t/t3510-cherry-pick-sequence.sh |   11 +++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 70055e5..be0686d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -716,18 +716,24 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	unsigned char commit_sha1[20];
 	enum replay_action action;
 	char *end_of_object_name;
-	int saved, status;
+	int saved, status, padding;
 
-	if (!prefixcmp(bol, "pick ")) {
+	if (!prefixcmp(bol, "pick")) {
 		action = CHERRY_PICK;
-		bol += strlen("pick ");
-	} else if (!prefixcmp(bol, "revert ")) {
+		bol += strlen("pick");
+	} else if (!prefixcmp(bol, "revert")) {
 		action = REVERT;
-		bol += strlen("revert ");
+		bol += strlen("revert");
 	} else
 		return NULL;
 
-	end_of_object_name = bol + strcspn(bol, " \n");
+	/* Eat up extra spaces/ tabs before object name */
+	padding = strspn(bol, " \t");
+	if (!padding)
+		return NULL;
+	bol += padding;
+
+	end_of_object_name = bol + strcspn(bol, " \t\n");
 	saved = *end_of_object_name;
 	*end_of_object_name = '\0';
 	status = get_sha1(bol, commit_sha1);
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 6390f2a..781c5ac 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -342,6 +342,17 @@ test_expect_success 'malformed instruction sheet 3' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'instruction sheet, fat-fingers version' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick \([0-9a-f]*\)/pick 	 \1 	/" .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&
+	git cherry-pick --continue
+'
+
 test_expect_success 'commit descriptions in insn sheet are optional' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..anotherpick &&
-- 
1.7.4.1

[PATCH 04/10] revert: simplify getting commit subject in format_todo()

[Ramkumar Ramachandra]

format_todo() calls get_message(), but uses only the subject line of
the commit message.  As a minor optimization, save work and
unnecessary memory allocations by using find_commit_subject() instead.
Also, remove the unnecessary check on cur->item->buffer: the
lookup_commit_reference() call in parse_insn_line() has already made
sure of this.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index be0686d..0a4b215 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -697,16 +697,16 @@ static int 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_str = opts->action == REVERT ? "revert" : "pick";
+	const char *subject;
+	int subject_len;
 
 	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))
-			return error(_("Cannot get commit message for %s"), sha1_abbrev);
-		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
-		free_message(&msg);
+		subject_len = find_commit_subject(cur->item->buffer, &subject);
+		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+			subject_len, subject);
 	}
 	return 0;
 }
-- 
1.7.4.1

[PATCH 05/10] t3510 (cherry-pick-sequencer): use exit status

[Ramkumar Ramachandra]

All the tests asserting failure use 'test_must_fail', which simply
checks for a non-zero exit status, potentially hiding underlying bugs.
So, replace instances of 'test_must_fail' with 'test_expect_code' to
check the exit status explicitly, where appropriate.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3510-cherry-pick-sequence.sh |   58 +++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 781c5ac..b6e822e 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -44,7 +44,7 @@ test_expect_success setup '
 
 test_expect_success 'cherry-pick persists data on failure' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick -s base..anotherpick &&
+	test_expect_code 1 git cherry-pick -s base..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '
 
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
+	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -93,7 +93,7 @@ test_expect_success '--abort requires cherry-pick in progress' '
 
 test_expect_success '--quit cleans up sequencer state' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..picked &&
+	test_expect_code 1 git cherry-pick base..picked &&
 	git cherry-pick --quit &&
 	test_path_is_missing .git/sequencer
 '
@@ -107,7 +107,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
 	:000000 100644 OBJID OBJID A	foo
 	:000000 100644 OBJID OBJID A	unrelated
 	EOF
-	test_must_fail git cherry-pick base..picked &&
+	test_expect_code 1 git cherry-pick base..picked &&
 	git cherry-pick --quit &&
 	test_path_is_missing .git/sequencer &&
 	test_must_fail git update-index --refresh &&
@@ -121,7 +121,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
 
 test_expect_success '--abort to cancel multiple cherry-pick' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev initial HEAD &&
@@ -131,7 +131,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' '
 
 test_expect_success '--abort to cancel single cherry-pick' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick picked &&
+	test_expect_code 1 git cherry-pick picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev initial HEAD &&
@@ -141,7 +141,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
 
 test_expect_success 'cherry-pick --abort to cancel multiple revert' '
 	pristine_detach anotherpick &&
-	test_must_fail git revert base..picked &&
+	test_expect_code 1 git revert base..picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev anotherpick HEAD &&
@@ -151,7 +151,7 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' '
 
 test_expect_success 'revert --abort works, too' '
 	pristine_detach anotherpick &&
-	test_must_fail git revert base..picked &&
+	test_expect_code 1 git revert base..picked &&
 	git revert --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev anotherpick HEAD
@@ -159,7 +159,7 @@ test_expect_success 'revert --abort works, too' '
 
 test_expect_success '--abort to cancel single revert' '
 	pristine_detach anotherpick &&
-	test_must_fail git revert picked &&
+	test_expect_code 1 git revert picked &&
 	git revert --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev anotherpick HEAD &&
@@ -170,7 +170,7 @@ test_expect_success '--abort to cancel single revert' '
 test_expect_success '--abort keeps unrelated change, easy case' '
 	pristine_detach unrelatedpick &&
 	echo changed >expect &&
-	test_must_fail git cherry-pick picked..yetanotherpick &&
+	test_expect_code 1 git cherry-pick picked..yetanotherpick &&
 	echo changed >unrelated &&
 	git cherry-pick --abort &&
 	test_cmp expect unrelated
@@ -179,7 +179,7 @@ test_expect_success '--abort keeps unrelated change, easy case' '
 test_expect_success '--abort refuses to clobber unrelated change, harder case' '
 	pristine_detach initial &&
 	echo changed >expect &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo changed >unrelated &&
 	test_must_fail git cherry-pick --abort &&
 	test_cmp expect unrelated &&
@@ -194,7 +194,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '
 
 test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..picked &&
+	test_expect_code 1 git cherry-pick base..picked &&
 	test_path_is_missing .git/sequencer &&
 	echo "resolved" >foo &&
 	git add foo &&
@@ -218,7 +218,7 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le
 
 test_expect_failure '--abort after last commit in sequence' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..picked &&
+	test_expect_code 1 git cherry-pick base..picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
 	test_cmp_rev initial HEAD &&
@@ -228,27 +228,27 @@ test_expect_failure '--abort after last commit in sequence' '
 
 test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	test-chmtime -v +0 .git/sequencer >expect &&
-	test_must_fail git cherry-pick unrelatedpick &&
+	test_expect_code 128 git cherry-pick unrelatedpick &&
 	test-chmtime -v +0 .git/sequencer >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success '--continue complains when no cherry-pick is in progress' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick --continue
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success '--continue complains when there are unresolved conflicts' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
-	test_must_fail git cherry-pick --continue
+	test_expect_code 1 git cherry-pick base..anotherpick &&
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success '--continue continues after conflicts are resolved' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
@@ -275,7 +275,7 @@ test_expect_success '--continue continues after conflicts are resolved' '
 
 test_expect_success '--continue respects opts' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick -x base..anotherpick &&
+	test_expect_code 1 git cherry-pick -x base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
@@ -293,7 +293,7 @@ test_expect_success '--continue respects opts' '
 
 test_expect_success '--signoff is not automatically propagated to resolved conflict' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick --signoff base..anotherpick &&
+	test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
@@ -311,40 +311,40 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 
 test_expect_success 'malformed instruction sheet 1' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	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
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success 'malformed instruction sheet 2' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	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
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success 'malformed instruction sheet 3' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "resolved" >foo &&
 	git add foo &&
 	git commit &&
 	sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
 	cp new_sheet .git/sequencer/todo &&
-	test_must_fail git cherry-pick --continue
+	test_expect_code 128 git cherry-pick --continue
 '
 
 test_expect_success 'instruction sheet, fat-fingers version' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
@@ -355,7 +355,7 @@ test_expect_success 'instruction sheet, fat-fingers version' '
 
 test_expect_success 'commit descriptions in insn sheet are optional' '
 	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "c" >foo &&
 	git add foo &&
 	git commit &&
-- 
1.7.4.1

[PATCH 06/10] t3502, t3510: clarify cherry-pick -m failure

[Ramkumar Ramachandra]

The "cherry-pick persists opts correctly" test in t3510
(cherry-pick-sequence) can cause some confusion, because the command
actually has two points of failure:

1. "-m 1" is specified on the command-line despite the base commit
   "initial" not being a merge-commit.
2. The revision range indicates that there will be a conflict that
   needs to be resolved.

Although the former error is trapped, and cherry-pick die()s with the
exit status 128, the reader may be distracted by the latter.  Fix this
by changing the revision range to something that wouldn't cause a
conflict.  Additionally, explicitly check the exit code in
"cherry-pick a non-merge with -m should fail" in t3502
(cherry-pick-merge) to reassure the reader that this failure has
nothing to do with the sequencer itself.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3502-cherry-pick-merge.sh    |    2 +-
 t/t3510-cherry-pick-sequence.sh |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 0ab52da..e37547f 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -35,7 +35,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 &&
+	test_expect_code 128 git cherry-pick -m 1 b &&
 	git diff --exit-code a --
 
 '
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index b6e822e..163ce7d 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '
 
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
-	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
+	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
-- 
1.7.4.1

[PATCH 07/10] t3510 (cherry-pick-sequencer): remove malformed sheet 2

[Ramkumar Ramachandra]

The next patch allows mixing "pick" and "revert" instruction in the
same instruction sheet.  By removing the "malformed instruction sheet
2" test in advance, it'll be easier to see the changes made by the
future patches that touch this file.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3510-cherry-pick-sequence.sh |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 163ce7d..1bfbe41 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -309,7 +309,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 	grep "Signed-off-by:" anotherpick_msg
 '
 
-test_expect_success 'malformed instruction sheet 1' '
+test_expect_success 'malformed instruction sheet, action' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "resolved" >foo &&
@@ -320,18 +320,7 @@ test_expect_success 'malformed instruction sheet 1' '
 	test_expect_code 128 git cherry-pick --continue
 '
 
-test_expect_success 'malformed instruction sheet 2' '
-	pristine_detach initial &&
-	test_expect_code 1 git cherry-pick base..anotherpick &&
-	echo "resolved" >foo &&
-	git add foo &&
-	git commit &&
-	sed "s/pick/revert/" .git/sequencer/todo >new_sheet &&
-	cp new_sheet .git/sequencer/todo &&
-	test_expect_code 128 git cherry-pick --continue
-'
-
-test_expect_success 'malformed instruction sheet 3' '
+test_expect_success 'malformed instruction sheet, object name' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..anotherpick &&
 	echo "resolved" >foo &&
-- 
1.7.4.1

[PATCH 08/10] revert: move replay_action, replay_subcommand to header

[Ramkumar Ramachandra]

In our vision to build a generalized sequencer, the revert builtin
will be left only with argument parsing work before calling into the
sequencer.  Since the enums replay_action and replay_subcommand have
nothing to do with this argument parsing, move them to sequencer.h.

However, REVERT and CHERRY_PICK are unsuitable variable names for
publicly exposing, as their purpose is unclear in the global context:
rename them to REPLAY_REVERT and REPLAY_PICK respectively.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   42 +++++++++++++++++-------------------------
 sequencer.h      |   12 ++++++++++++
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0a4b215..ed062ea 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,14 +39,6 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-enum replay_action { REVERT, CHERRY_PICK };
-enum replay_subcommand {
-	REPLAY_NONE,
-	REPLAY_REMOVE_STATE,
-	REPLAY_CONTINUE,
-	REPLAY_ROLLBACK
-};
-
 struct replay_opts {
 	enum replay_action action;
 	enum replay_subcommand subcommand;
@@ -73,14 +65,14 @@ struct replay_opts {
 
 static const char *action_name(const struct replay_opts *opts)
 {
-	return opts->action == REVERT ? "revert" : "cherry-pick";
+	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
 }
 
 static char *get_encoding(const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
@@ -159,7 +151,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 	};
 
-	if (opts->action == CHERRY_PICK) {
+	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -363,7 +355,7 @@ static int error_dirty_index(struct replay_opts *opts)
 		return error_resolve_conflict(action_name(opts));
 
 	/* Different translation strings for cherry-pick and revert */
-	if (opts->action == CHERRY_PICK)
+	if (opts->action == REPLAY_PICK)
 		error(_("Your local changes would be overwritten by cherry-pick."));
 	else
 		error(_("Your local changes would be overwritten by revert."));
@@ -542,7 +534,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->action == REVERT) {
+	if (opts->action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -583,7 +575,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -607,13 +599,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
 		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
-	if (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))
+	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
 		write_cherry_pick_head(commit, "REVERT_HEAD");
 
 	if (res) {
-		error(opts->action == REVERT
+		error(opts->action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -637,7 +629,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (opts->action != REVERT)
+	if (opts->action != REPLAY_REVERT)
 		revs->reverse = 1;
 
 	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
@@ -698,7 +690,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 {
 	struct commit_list *cur = NULL;
 	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REVERT ? "revert" : "pick";
+	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
 	const char *subject;
 	int subject_len;
 
@@ -719,10 +711,10 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	int saved, status, padding;
 
 	if (!prefixcmp(bol, "pick")) {
-		action = CHERRY_PICK;
+		action = REPLAY_PICK;
 		bol += strlen("pick");
 	} else if (!prefixcmp(bol, "revert")) {
-		action = REVERT;
+		action = REPLAY_REVERT;
 		bol += strlen("revert");
 	} else
 		return NULL;
@@ -745,7 +737,7 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	 */
 	if (action != opts->action) {
 		const char *action_str;
-		action_str = action == REVERT ? "revert" : "cherry-pick";
+		action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
 		error(_("Cannot %s during a %s"), action_str, action_name(opts));
 		return NULL;
 	}
@@ -1080,7 +1072,7 @@ static int pick_revisions(struct replay_opts *opts)
 	if (create_seq_dir() < 0)
 		return -1;
 	if (get_sha1("HEAD", sha1)) {
-		if (opts->action == REVERT)
+		if (opts->action == REPLAY_REVERT)
 			return error(_("Can't revert as initial commit"));
 		return error(_("Can't cherry-pick into empty head"));
 	}
@@ -1097,7 +1089,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	if (isatty(0))
 		opts.edit = 1;
-	opts.action = REVERT;
+	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
@@ -1112,7 +1104,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	int res;
 
 	memset(&opts, 0, sizeof(opts));
-	opts.action = CHERRY_PICK;
+	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = pick_revisions(&opts);
diff --git a/sequencer.h b/sequencer.h
index f435fdb..9b1b94d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,6 +7,18 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
+enum replay_action {
+	REPLAY_REVERT,
+	REPLAY_PICK
+};
+
+enum replay_subcommand {
+	REPLAY_NONE,
+	REPLAY_REMOVE_STATE,
+	REPLAY_CONTINUE,
+	REPLAY_ROLLBACK
+};
+
 /*
  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset'.
-- 
1.7.4.1

[PATCH 09/10] revert: allow mixed pick and revert instructions

[Ramkumar Ramachandra]

Parse the instruction sheet in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming that all instructions use
the same action.  Now you can do:

  pick fdc0b12 picked
  revert 965fed4 anotherpick

For cherry-pick and revert, this means that a 'git cherry-pick
--continue' can continue an ongoing revert operation and viceversa.

Helped-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c                |  107 ++++++++++++++++++---------------------
 sequencer.h                     |    6 ++
 t/t3510-cherry-pick-sequence.sh |   58 +++++++++++++++++++++
 3 files changed, 114 insertions(+), 57 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index ed062ea..50af439 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -459,7 +459,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+			struct replay_opts *opts)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -534,7 +535,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (opts->action == REPLAY_REVERT) {
+	if (action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -575,7 +576,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 	}
 
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
@@ -605,7 +606,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		write_cherry_pick_head(commit, "REVERT_HEAD");
 
 	if (res) {
-		error(opts->action == REPLAY_REVERT
+		error(action == REPLAY_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -675,54 +676,54 @@ static void read_and_refresh_cache(struct replay_opts *opts)
  *     assert(commit_list_count(list) == 2);
  *     return list;
  */
-static struct commit_list **commit_list_append(struct commit *commit,
-					       struct commit_list **next)
+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+						struct commit *operand,
+						struct replay_insn_list **next)
 {
-	struct commit_list *new = xmalloc(sizeof(struct commit_list));
-	new->item = commit;
+	struct replay_insn_list *new = xmalloc(sizeof(*new));
+	new->action = action;
+	new->operand = operand;
 	*next = new;
 	new->next = NULL;
 	return &new->next;
 }
 
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
-		struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 {
-	struct commit_list *cur = NULL;
-	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
-	const char *subject;
-	int subject_len;
+	struct replay_insn_list *cur;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
-		subject_len = find_commit_subject(cur->item->buffer, &subject);
+		const char *sha1_abbrev, *action_str, *subject;
+		int subject_len;
+
+		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		subject_len = find_commit_subject(cur->operand->buffer, &subject);
 		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
 			subject_len, subject);
 	}
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 {
 	unsigned char commit_sha1[20];
-	enum replay_action action;
 	char *end_of_object_name;
 	int saved, status, padding;
 
 	if (!prefixcmp(bol, "pick")) {
-		action = REPLAY_PICK;
+		item->action = REPLAY_PICK;
 		bol += strlen("pick");
 	} else if (!prefixcmp(bol, "revert")) {
-		action = REPLAY_REVERT;
+		item->action = REPLAY_REVERT;
 		bol += strlen("revert");
 	} else
-		return NULL;
+		return -1;
 
 	/* Eat up extra spaces/ tabs before object name */
 	padding = strspn(bol, " \t");
 	if (!padding)
-		return NULL;
+		return -1;
 	bol += padding;
 
 	end_of_object_name = bol + strcspn(bol, " \t\n");
@@ -731,37 +732,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	status = get_sha1(bol, commit_sha1);
 	*end_of_object_name = saved;
 
-	/*
-	 * Verify that the action matches up with the one in
-	 * opts; we don't support arbitrary instructions
-	 */
-	if (action != opts->action) {
-		const char *action_str;
-		action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
-		error(_("Cannot %s during a %s"), action_str, action_name(opts));
-		return NULL;
-	}
-
 	if (status < 0)
-		return NULL;
+		return -1;
 
-	return lookup_commit_reference(commit_sha1);
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return -1;
+
+	item->next = NULL;
+	return 0;
 }
 
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
-			struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 {
-	struct commit_list **next = todo_list;
-	struct commit *commit;
+	struct replay_insn_list **next = todo_list;
+	struct replay_insn_list item = { 0, NULL, NULL };
 	char *p = buf;
 	int i;
 
 	for (i = 1; *p; i++) {
 		char *eol = strchrnul(p, '\n');
-		commit = parse_insn_line(p, eol, opts);
-		if (!commit)
+		if (parse_insn_line(p, eol, &item) < 0)
 			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(item.action, item.operand, next);
 		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)
@@ -769,8 +762,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
-			struct replay_opts *opts)
+static void read_populate_todo(struct replay_insn_list **todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	struct strbuf buf = STRBUF_INIT;
@@ -786,7 +778,7 @@ static void read_populate_todo(struct commit_list **todo_list,
 	}
 	close(fd);
 
-	res = parse_insn_buffer(buf.buf, todo_list, opts);
+	res = parse_insn_buffer(buf.buf, todo_list);
 	strbuf_release(&buf);
 	if (res)
 		die(_("Unusable instruction sheet: %s"), todo_file);
@@ -835,18 +827,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 		die(_("Malformed options sheet: %s"), opts_file);
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
-	struct commit_list **next;
+	struct replay_insn_list **next;
 
 	prepare_revs(&revs, opts);
 
 	next = todo_list;
 	while ((commit = get_revision(&revs)))
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(opts->action, commit, next);
 }
 
 static int create_seq_dir(void)
@@ -944,7 +936,7 @@ fail:
 	return -1;
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
@@ -952,7 +944,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	int fd;
 
 	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list, opts) < 0)
+	if (format_todo(&buf, todo_list) < 0)
 		die(_("Could not format %s."), todo_file);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
@@ -996,9 +988,10 @@ static void save_opts(struct replay_opts *opts)
 	}
 }
 
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list,
+			struct replay_opts *opts)
 {
-	struct commit_list *cur;
+	struct replay_insn_list *cur;
 	int res;
 
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -1008,8 +1001,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	read_and_refresh_cache(opts);
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
-		res = do_pick_commit(cur->item, opts);
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
 		if (res) {
 			if (!cur->next)
 				/*
@@ -1034,7 +1027,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 
 static int pick_revisions(struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
+	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
 	read_and_refresh_cache(opts);
@@ -1054,7 +1047,7 @@ static int pick_revisions(struct replay_opts *opts)
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			return error(_("No %s in progress"), action_name(opts));
 		read_populate_opts(&opts);
-		read_populate_todo(&todo_list, opts);
+		read_populate_todo(&todo_list);
 
 		/* Verify that the conflict has been resolved */
 		if (!index_differs_from("HEAD", 0))
diff --git a/sequencer.h b/sequencer.h
index 9b1b94d..648f7bf 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -19,6 +19,12 @@ enum replay_subcommand {
 	REPLAY_ROLLBACK
 };
 
+struct replay_insn_list {
+	enum replay_action action;
+	struct commit *operand;
+	struct replay_insn_list *next;
+};
+
 /*
  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset'.
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 1bfbe41..9b19917 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
 	test_line_count = 4 commits
 '
 
+test_expect_success 'revert --continue continues after cherry-pick' '
+	pristine_detach initial &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	git revert --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	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 'mixed pick and revert instructions' '
+	pristine_detach initial &&
+	test_expect_code 1 git cherry-pick base..anotherpick &&
+	echo "c" >foo &&
+	git add foo &&
+	git commit &&
+	oldsha=`git rev-parse --short HEAD~1` &&
+	echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	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_done
-- 
1.7.4.1

[PATCH 10/10] revert: report fine-grained error messages from insn parser

[Ramkumar Ramachandra]

Three kinds of errors can arise from parsing '.git/sequencer/todo':
1. Unrecognized action
2. Missing space after valid action prefix
3. Malformed object name
4. Object name does not refer to a valid commit

Since we would like to make the instruction sheet user-editable in the
future (much like the 'rebase -i' sheet), report more fine-grained
parse errors prefixed with the filename and line number.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 50af439..d106c3c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -705,11 +705,14 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 	return 0;
 }
 
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+			struct replay_insn_list *item, int lineno)
 {
+	const char *todo_file = git_path(SEQ_TODO_FILE);
 	unsigned char commit_sha1[20];
 	char *end_of_object_name;
 	int saved, status, padding;
+	size_t error_len;
 
 	if (!prefixcmp(bol, "pick")) {
 		item->action = REPLAY_PICK;
@@ -717,13 +720,19 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 	} else if (!prefixcmp(bol, "revert")) {
 		item->action = REPLAY_REVERT;
 		bol += strlen("revert");
-	} else
-		return -1;
+	} else {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Unrecognized action: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
 
 	/* Eat up extra spaces/ tabs before object name */
 	padding = strspn(bol, " \t");
-	if (!padding)
-		return -1;
+	if (!padding) {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Missing space after action: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
 	bol += padding;
 
 	end_of_object_name = bol + strcspn(bol, " \t\n");
@@ -732,12 +741,18 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 	status = get_sha1(bol, commit_sha1);
 	*end_of_object_name = saved;
 
-	if (status < 0)
-		return -1;
+	if (status < 0) {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Malformed object name: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
 
 	item->operand = lookup_commit_reference(commit_sha1);
-	if (!item->operand)
-		return -1;
+	if (!item->operand) {
+		error_len = eol - bol > 255 ? 255 : eol - bol;
+		return error(_("%s:%d: Not a valid commit: %.*s"),
+			todo_file, lineno, (int)error_len, bol);
+	}
 
 	item->next = NULL;
 	return 0;
@@ -752,8 +767,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
 
 	for (i = 1; *p; i++) {
 		char *eol = strchrnul(p, '\n');
-		if (parse_insn_line(p, eol, &item) < 0)
-			return error(_("Could not parse line %d."), i);
+		if (parse_insn_line(p, eol, &item, i) < 0)
+			return -1;
 		next = replay_insn_list_append(item.action, item.operand, next);
 		p = *eol ? eol + 1 : eol;
 	}
-- 
1.7.4.1

Re: [PATCH 10/10] revert: report fine-grained error messages from insn parser

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Three kinds of errors can arise from parsing '.git/sequencer/todo':
> 1. Unrecognized action
> 2. Missing space after valid action prefix

I think these two are the same and shouldn't result in different error
messages, i.e. the first one in this sequence is still an "Unrecognized
action" error and does not have anything to do with "pick" action.

	pickle rr/cherry-pick~4
        pick rr/cherry-pick~3

Re: [PATCH 10/10] revert: report fine-grained error messages from insn parser

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Three kinds of errors can arise from parsing '.git/sequencer/todo':
>> 1. Unrecognized action
>> 2. Missing space after valid action prefix
>
> I think these two are the same and shouldn't result in different error
> messages, i.e. the first one in this sequence is still an "Unrecognized
> action" error and does not have anything to do with "pick" action.
>
>        pickle rr/cherry-pick~4
>        pick rr/cherry-pick~3

Good point.  I'll change the parser to check for "pick " || "pick\t"
and "revert " || "revert\t" explicitly in the re-roll.

Thanks.

-- Ram