Merge branch 'rr/revert-cherry-pick'

* rr/revert-cherry-pick:
  t3502, t3510: clarify cherry-pick -m failure
  t3510 (cherry-pick-sequencer): use exit status
  revert: simplify getting commit subject in format_todo()
  revert: tolerate extra spaces, tabs in insn sheet
  revert: make commit subjects in insn sheet optional
  revert: free msg in format_todo()

3 files changed, 90 insertions, 49 deletions

diff --git a/builtin/revert.c b/builtin/revert.c
index fce3f92981..0d8020cf64 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -700,44 +700,47 @@ 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);
+		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;
 }
 
-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, padding;
 
-	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)
+	/* Eat up extra spaces/ tabs before object name */
+	padding = strspn(bol, " \t");
+	if (!padding)
 		return NULL;
-	q++;
+	bol += padding;
 
-	strlcpy(sha1_abbrev, p, q - p);
+	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);
+	*end_of_object_name = saved;
 
 	/*
 	 * Verify that the action matches up with the one in
@@ -750,7 +753,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);
@@ -765,13 +768,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/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 0ab52da902..e37547f41a 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 e80050e1fe..97f3710700 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -14,6 +14,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" &&
@@ -43,7 +46,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 &&
@@ -64,7 +67,7 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
 
 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 initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -104,7 +107,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
 '
@@ -118,7 +121,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 &&
@@ -132,7 +135,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 &&
@@ -142,7 +145,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 &&
@@ -152,7 +155,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 &&
@@ -162,7 +165,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
@@ -170,7 +173,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 &&
@@ -181,7 +184,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
@@ -190,7 +193,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 &&
@@ -205,7 +208,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '
 
 test_expect_success 'cherry-pick still writes 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_dir .git/sequencer &&
 	echo "resolved" >foo &&
 	git add foo &&
@@ -229,7 +232,7 @@ test_expect_success 'cherry-pick still writes sequencer state when one commit is
 
 test_expect_success '--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 &&
@@ -239,22 +242,22 @@ test_expect_success '--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 of single cherry-pick' '
@@ -318,7 +321,7 @@ test_expect_success '--continue after resolving conflicts' '
 
 test_expect_success '--continue after resolving conflicts and committing' '
 	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 &&
@@ -368,7 +371,7 @@ test_expect_success 'follow advice and skip nil patch' '
 
 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 &&
@@ -409,7 +412,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
 
 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 &&
@@ -453,24 +456,24 @@ test_expect_success 'sign-off needs to be reaffirmed after conflict resolution,
 
 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 'empty commit set' '
@@ -478,4 +481,40 @@ test_expect_success 'empty commit set' '
 	test_expect_code 128 git cherry-pick base..base
 '
 
+test_expect_success 'malformed instruction sheet 3' '
+	pristine_detach initial &&
+	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_expect_code 128 git cherry-pick --continue
+'
+
+test_expect_success 'instruction sheet, fat-fingers version' '
+	pristine_detach initial &&
+	test_expect_code 1 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_expect_code 1 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