threads / patch / 29119

patch, 9 partsRe-roll rr/revert-cherry-pick v2

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

## tl;dr

45 messages between Dec 9, 2011 and Dec 15, 2011. Diffs are folded; open one to read it.

replies: 44people: 3as markdown or json

Ramkumar Ramachandra· Dec 9, 2011, 15:41 UTC · lore
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
Ramkumar Ramachandra· Dec 9, 2011, 15:41 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/revert.c +1 −0
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
Ramkumar Ramachandra· Dec 9, 2011, 15:41 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +44 −21

builtin/revert.c, t/t3510-cherry-pick-sequence.sh

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
Jonathan Nieder· Dec 9, 2011, 19:32 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
Show 5 quoted lines
>                   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.

Ramkumar Ramachandra· Dec 9, 2011, 15:42 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +16 −1

builtin/revert.c, t/t3510-cherry-pick-sequence.sh

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
Jonathan Nieder· Dec 9, 2011, 19:40 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
Show 9 quoted lines
> +++ 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?

Ramkumar Ramachandra· Dec 9, 2011, 19:58 UTC · re: Jonathan Nieder · lore

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

Hi,
Jonathan Nieder wrote:
Show 14 quoted lines
> 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:
Show changes to builtin/revert.c +9 −9
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
Junio C Hamano· Dec 9, 2011, 20:12 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 6 quoted lines
> 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()?

Ramkumar Ramachandra· Dec 9, 2011, 20:15 UTC · re: Junio C Hamano · lore

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

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
Ramkumar Ramachandra· Dec 9, 2011, 15:42 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/revert.c +5 −5
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
Jonathan Nieder· Dec 9, 2011, 19:47 UTC · re: Ramkumar Ramachandra · lore

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

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?
Ramkumar Ramachandra· Dec 9, 2011, 20:58 UTC · re: Jonathan Nieder · lore

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

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
Show 6 quoted lines
>> 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
Ramkumar Ramachandra· Dec 9, 2011, 15:42 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to t/t3510-cherry-pick-sequence.sh +31 −31
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
Jonathan Nieder· Dec 9, 2011, 20:21 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
Show 6 quoted lines
> 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
[...]
Show 7 quoted lines
> @@ -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.

[...]
Show 6 quoted lines
> @@ -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.)

[...]
Show 9 quoted lines
> @@ -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>
Ramkumar Ramachandra· Dec 9, 2011, 20:36 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
Show 16 quoted lines
> 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
Jonathan Nieder· Dec 9, 2011, 21:00 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
Show 8 quoted lines
>>> -     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.

Ramkumar Ramachandra· Dec 9, 2011, 15:42 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to t/t3510-cherry-pick-sequence.sh +2 −13
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
Jonathan Nieder· Dec 9, 2011, 20:24 UTC · re: Ramkumar Ramachandra · lore

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

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.

Ramkumar Ramachandra· Dec 9, 2011, 20:30 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
Show 9 quoted lines
> 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
Jonathan Nieder· Dec 9, 2011, 20:37 UTC · re: Ramkumar Ramachandra · lore

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

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.

Ramkumar Ramachandra· Dec 9, 2011, 15:42 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +124 −67

builtin/revert.c, t/t3510-cherry-pick-sequence.sh

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
Jonathan Nieder· Dec 9, 2011, 20:26 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
Show 9 quoted lines
> 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.
[...]
Show 8 quoted lines
> --- 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?
Ramkumar Ramachandra· Dec 9, 2011, 15:42 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/revert.c +16 −7
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
Jonathan Nieder· Dec 9, 2011, 20:47 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
Show 12 quoted lines
> --- 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?

Ramkumar Ramachandra· Dec 9, 2011, 15:42 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +40 −24

builtin/revert.c, t/t3510-cherry-pick-sequence.sh

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
Jonathan Nieder· Dec 9, 2011, 19:02 UTC · re: Ramkumar Ramachandra · lore

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

Hey,
Ramkumar Ramachandra wrote:
> From: Jonathan Nieder <jrnieder@gmail.com>
[...]
Show 10 quoted lines
> 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.

[...]
Show 16 quoted lines
> --- 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?

Ramkumar Ramachandra· Dec 9, 2011, 19:11 UTC · re: Jonathan Nieder · lore

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

Hi,
Jonathan Nieder wrote:
Show 7 quoted lines
> [...]
>        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.

Show 12 quoted lines
>> --- 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
Jonathan Nieder· Dec 9, 2011, 19:29 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
>> Ramkumar Ramachandra wrote:
Show 7 quoted lines
>>> +++ 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
>>> +'
[...]
Show 7 quoted lines
> 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.
Ramkumar Ramachandra· Dec 9, 2011, 19:49 UTC · re: Jonathan Nieder · lore

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

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
Jonathan Nieder· Dec 9, 2011, 21:09 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
Show 5 quoted lines
>                                                                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".

Jonathan Nieder· Dec 9, 2011, 20:34 UTC · re: Ramkumar Ramachandra · lore

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

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.
Ramkumar Ramachandra· Dec 9, 2011, 23:03 UTC · re: Ramkumar Ramachandra · lore

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

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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/revert.c +1 −0
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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +44 −21

builtin/revert.c, t/t3510-cherry-pick-sequence.sh

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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +23 −6

builtin/revert.c, t/t3510-cherry-pick-sequence.sh

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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/revert.c +5 −5
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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to t/t3510-cherry-pick-sequence.sh +29 −29
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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +2 −2

t/t3502-cherry-pick-merge.sh, t/t3510-cherry-pick-sequence.sh

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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to t/t3510-cherry-pick-sequence.sh +2 −13
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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +29 −25

builtin/revert.c, sequencer.h

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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 3 files +114 −57

builtin/revert.c, sequencer.h, t/t3510-cherry-pick-sequence.sh

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
Ramkumar Ramachandra· Dec 14, 2011, 16:54 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/revert.c +26 −11
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
Junio C Hamano· Dec 15, 2011, 21:23 UTC · re: Ramkumar Ramachandra · lore

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

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
Ramkumar Ramachandra· Dec 15, 2011, 23:18 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Junio C Hamano wrote:
Show 12 quoted lines
> 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

← back to recent threads