{"thread":{"id":"29119","subject":"[PATCH 0/9] Re-roll rr/revert-cherry-pick v2","startedAt":"2011-12-09T15:41:57Z","lastAt":"2011-12-15T23:18:35Z","messageCount":45,"participants":["Ramkumar Ramachandra","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"180659","messageId":"1323445326-24637-1-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":null,"subject":"[PATCH 0/9] Re-roll rr/revert-cherry-pick v2","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:41:57Z","receivedAt":"2011-12-09T15:41:57Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThe previous iteration had 5 parts ($gmane/186425), and this one has\n9. So, I have to explain where the four new patches came from:\n\n- \"revert: report fine-grained error messages from insn parser\" arises\n from Jonathan's request to split \"revert: allow mixed pick and\n revert instructions\".\n\n- \"revert: tolerate extra spaces, tabs in insn sheet\" comes from\n Junio's request to be considerate towards people with fat fingers.\n\n- I put in the other two patches on my own, because I realized that\n the tests needed some tightening, lest they hide underlying bugs\n (it's happened before).\n\nApart from that, I've just made changes in response to reviews. I'm\nnot yet sure what to do about $gmane/186433.\n\nThanks for reading.\n\n-- Ram\n\nJonathan Nieder (1):\n revert: simplify communicating command-line arguments\n\nRamkumar Ramachandra (8):\n revert: free msg in format_todo()\n revert: make commit subjects in insn sheet optional\n revert: tolerate extra spaces, tabs in insn sheet\n revert: simplify getting commit subject in format_todo()\n t3510 (cherry-pick-sequencer): use exit status\n t3510 (cherry-pick-sequencer): remove malformed sheet 2\n revert: allow mixed pick and revert instructions\n revert: report fine-grained error messages from insn parser\n\n builtin/revert.c | 237 ++++++++++++++++++++------------------\n t/t3510-cherry-pick-sequence.sh | 157 +++++++++++++++++++++-----\n 2 files changed, 252 insertions(+), 142 deletions(-)\n\n-- \n1.7.7.3\n"},{"id":"180660","messageId":"1323445326-24637-2-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/9] revert: free msg in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:41:58Z","receivedAt":"2011-12-09T15:41:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Memory allocated to the fields of msg by get_message() isn't freed.\nThis is potentially a big leak, because fresh memory is allocated to\nstore the commit message for each commit. Fix this using\nfree_message().\n\nReported-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1ea525c..0c6d3d8 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\tif (get_message(cur->item, &msg))\n \t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n \t\tstrbuf_addf(buf, \"%s %s %s\\n\", action_str, sha1_abbrev, msg.subject);\n+\t\tfree_message(&msg);\n \t}\n \treturn 0;\n }\n-- \n1.7.7.3\n"},{"id":"180661","messageId":"1323445326-24637-3-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/9] revert: make commit subjects in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:41:59Z","receivedAt":"2011-12-09T15:41:59Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Change the instruction sheet format subtly so that the subject of the\ncommit message that follows the object name is optional. As a result,\nan instruction sheet like this is now perfectly valid:\n\n pick 35b0426\n pick fbd5bbcbc2e\n pick 7362160f\n\nWhile at it, also fix a bug: currently, we use a commit-id-shaped\nbuffer to store the word after \"pick\" in '.git/sequencer/todo'. This\nis both wasteful and wrong because it places an artificial limit on\nthe line length. In addition to literal SHA-1 hexes, expressions like\nthe following are valid object names in the instruction sheet:\n\n featurebranch~4\n rr/revert-cherry-pick-continue^2~12@{12 days ago}\n\nSo, eliminate the need for the buffer to keep the object name\naltogether, and add a test demonstrating this.\n\n[jc: simplify parsing]\n\nSuggested-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 37 ++++++++++++++++---------------------\n t/t3510-cherry-pick-sequence.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0c6d3d8..70055e5 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \treturn 0;\n }\n \n-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)\n {\n \tunsigned char commit_sha1[20];\n-\tchar sha1_abbrev[40];\n \tenum replay_action action;\n-\tint insn_len = 0;\n-\tchar *p, *q;\n+\tchar *end_of_object_name;\n+\tint saved, status;\n \n-\tif (!prefixcmp(start, \"pick \")) {\n+\tif (!prefixcmp(bol, \"pick \")) {\n \t\taction = CHERRY_PICK;\n-\t\tinsn_len = strlen(\"pick\");\n-\t\tp = start + insn_len + 1;\n-\t} else if (!prefixcmp(start, \"revert \")) {\n+\t\tbol += strlen(\"pick \");\n+\t} else if (!prefixcmp(bol, \"revert \")) {\n \t\taction = REVERT;\n-\t\tinsn_len = strlen(\"revert\");\n-\t\tp = start + insn_len + 1;\n+\t\tbol += strlen(\"revert \");\n \t} else\n \t\treturn NULL;\n \n-\tq = strchr(p, ' ');\n-\tif (!q)\n-\t\treturn NULL;\n-\tq++;\n-\n-\tstrlcpy(sha1_abbrev, p, q - p);\n+\tend_of_object_name = bol + strcspn(bol, \" \\n\");\n+\tsaved = *end_of_object_name;\n+\t*end_of_object_name = '\\0';\n+\tstatus = get_sha1(bol, commit_sha1);\n+\t*end_of_object_name = saved;\n \n \t/*\n \t * Verify that the action matches up with the one in\n@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n \t\treturn NULL;\n \t}\n \n-\tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n+\tif (status < 0)\n \t\treturn NULL;\n \n \treturn lookup_commit_reference(commit_sha1);\n@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \tint i;\n \n \tfor (i = 1; *p; i++) {\n-\t\tcommit = parse_insn_line(p, opts);\n+\t\tchar *eol = strchrnul(p, '\\n');\n+\t\tcommit = parse_insn_line(p, eol, opts);\n \t\tif (!commit)\n \t\t\treturn error(_(\"Could not parse line %d.\"), i);\n \t\tnext = commit_list_append(commit, next);\n-\t\tp = strchrnul(p, '\\n');\n-\t\tif (*p)\n-\t\t\tp++;\n+\t\tp = *eol ? eol + 1 : eol;\n \t}\n \tif (!*todo_list)\n \t\treturn error(_(\"No commits parsed.\"));\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 2c4c1c8..6390f2a 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -13,6 +13,9 @@ test_description='Test cherry-pick continuation features\n \n . ./test-lib.sh\n \n+# Repeat first match 10 times\n+_r10='\\1\\1\\1\\1\\1\\1\\1\\1\\1\\1'\n+\n pristine_detach () {\n \tgit cherry-pick --quit &&\n \tgit checkout -f \"$1^0\" &&\n@@ -328,4 +331,29 @@ test_expect_success 'malformed instruction sheet 2' '\n \ttest_must_fail git cherry-pick --continue\n '\n \n+test_expect_success 'malformed instruction sheet 3' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick base..anotherpick &&\n+\techo \"resolved\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tsed \"s/pick \\([0-9a-f]*\\)/pick $_r10/\" .git/sequencer/todo >new_sheet &&\n+\tcp new_sheet .git/sequencer/todo &&\n+\ttest_must_fail git cherry-pick --continue\n+'\n+\n+test_expect_success 'commit descriptions in insn sheet are optional' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tcut -d\" \" -f1,2 .git/sequencer/todo >new_sheet &&\n+\tcp new_sheet .git/sequencer/todo &&\n+\tgit cherry-pick --continue &&\n+\ttest_path_is_missing .git/sequencer &&\n+\tgit rev-list HEAD >commits &&\n+\ttest_line_count = 4 commits\n+'\n+\n test_done\n-- \n1.7.7.3\n"},{"id":"180662","messageId":"1323445326-24637-4-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:42:00Z","receivedAt":"2011-12-09T15:42:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Tolerate extra spaces and tabs as part of the the field separator in\n'.git/sequencer/todo', for people with fat fingers.\n\nRequested-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 6 +++++-\n t/t3510-cherry-pick-sequence.sh | 11 +++++++++++\n 2 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 70055e5..b976562 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -727,7 +727,11 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \t} else\n \t\treturn NULL;\n \n-\tend_of_object_name = bol + strcspn(bol, \" \\n\");\n+\t/* Eat up extra spaces/ tabs before object name */\n+\twhile (*bol == ' ' || *bol == '\\t')\n+\t\tbol += 1;\n+\n+\tend_of_object_name = bol + strcspn(bol, \" \\t\\n\");\n \tsaved = *end_of_object_name;\n \t*end_of_object_name = '\\0';\n \tstatus = get_sha1(bol, commit_sha1);\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 6390f2a..781c5ac 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -342,6 +342,17 @@ test_expect_success 'malformed instruction sheet 3' '\n \ttest_must_fail git cherry-pick --continue\n '\n \n+test_expect_success 'instruction sheet, fat-fingers version' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tsed \"s/pick \\([0-9a-f]*\\)/pick \t \\1 \t/\" .git/sequencer/todo >new_sheet &&\n+\tcp new_sheet .git/sequencer/todo &&\n+\tgit cherry-pick --continue\n+'\n+\n test_expect_success 'commit descriptions in insn sheet are optional' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick base..anotherpick &&\n-- \n1.7.7.3\n"},{"id":"180663","messageId":"1323445326-24637-5-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/9] revert: simplify getting commit subject in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:42:01Z","receivedAt":"2011-12-09T15:42:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"format_todo() calls get_message(), but uses only the subject line of\nthe commit message. As a minor optimization, save work and\nunnecessary memory allocations by using find_commit_subject() instead.\nAlso, remove the unnecessary check on cur->item: the previous patch\nmakes sure that instruction sheets with missing commit subjects are\nparsable.\n\nSuggested-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex b976562..86af516 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -697,16 +697,16 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\tstruct replay_opts *opts)\n {\n \tstruct commit_list *cur = NULL;\n-\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tconst char *sha1_abbrev = NULL;\n \tconst char *action_str = opts->action == REVERT ? \"revert\" : \"pick\";\n+\tconst char *subject;\n+\tint subject_len;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n-\t\tif (get_message(cur->item, &msg))\n-\t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n-\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action_str, sha1_abbrev, msg.subject);\n-\t\tfree_message(&msg);\n+\t\tsubject_len = find_commit_subject(cur->item->buffer, &subject);\n+\t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n+\t\t\tsubject_len, subject);\n \t}\n \treturn 0;\n }\n-- \n1.7.7.3\n"},{"id":"180664","messageId":"1323445326-24637-6-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:42:02Z","receivedAt":"2011-12-09T15:42:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Since cf3e2486 (revert: Propagate errors upwards from do_pick_commit,\n2011-08-04), 'git cherry-pick' has three different ways of failing:\n\n1. die() with the exit status 128.\n3. error() out with exit status -1.\n2. exit with positive exit status to indicate a conflict.\n\nHowever, all the tests asserting its failure use 'test_must_fail',\nwhich simply checks for a non-zero exit status, potentially hiding\nunderlying bugs. So, replace all instances of 'test_must_fail' with\n'test_expect_code' to check the exit status explicitly.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n t/t3510-cherry-pick-sequence.sh | 62 +++++++++++++++++++-------------------\n 1 files changed, 31 insertions(+), 31 deletions(-)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 781c5ac..9ffc085 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -44,7 +44,7 @@ test_expect_success setup '\n \n test_expect_success 'cherry-pick persists data on failure' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick -s base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick -s base..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '\n \n test_expect_success 'cherry-pick persists opts correctly' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n+\ttest_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -88,12 +88,12 @@ test_expect_success '--quit does not complain when no cherry-pick is in progress\n \n test_expect_success '--abort requires cherry-pick in progress' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick --abort\n+\ttest_expect_code 128 git cherry-pick --abort\n '\n \n test_expect_success '--quit cleans up sequencer state' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_expect_code 1 git cherry-pick base..picked &&\n \tgit cherry-pick --quit &&\n \ttest_path_is_missing .git/sequencer\n '\n@@ -107,7 +107,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '\n \t:000000 100644 OBJID OBJID A\tfoo\n \t:000000 100644 OBJID OBJID A\tunrelated\n \tEOF\n-\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_expect_code 1 git cherry-pick base..picked &&\n \tgit cherry-pick --quit &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_must_fail git update-index --refresh &&\n@@ -121,7 +121,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '\n \n test_expect_success '--abort to cancel multiple cherry-pick' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \tgit cherry-pick --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev initial HEAD &&\n@@ -131,7 +131,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' '\n \n test_expect_success '--abort to cancel single cherry-pick' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick picked &&\n+\ttest_expect_code 1 git cherry-pick picked &&\n \tgit cherry-pick --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev initial HEAD &&\n@@ -141,7 +141,7 @@ test_expect_success '--abort to cancel single cherry-pick' '\n \n test_expect_success 'cherry-pick --abort to cancel multiple revert' '\n \tpristine_detach anotherpick &&\n-\ttest_must_fail git revert base..picked &&\n+\ttest_expect_code 1 git revert base..picked &&\n \tgit cherry-pick --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev anotherpick HEAD &&\n@@ -151,7 +151,7 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' '\n \n test_expect_success 'revert --abort works, too' '\n \tpristine_detach anotherpick &&\n-\ttest_must_fail git revert base..picked &&\n+\ttest_expect_code 1 git revert base..picked &&\n \tgit revert --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev anotherpick HEAD\n@@ -159,7 +159,7 @@ test_expect_success 'revert --abort works, too' '\n \n test_expect_success '--abort to cancel single revert' '\n \tpristine_detach anotherpick &&\n-\ttest_must_fail git revert picked &&\n+\ttest_expect_code 1 git revert picked &&\n \tgit revert --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev anotherpick HEAD &&\n@@ -170,7 +170,7 @@ test_expect_success '--abort to cancel single revert' '\n test_expect_success '--abort keeps unrelated change, easy case' '\n \tpristine_detach unrelatedpick &&\n \techo changed >expect &&\n-\ttest_must_fail git cherry-pick picked..yetanotherpick &&\n+\ttest_expect_code 1 git cherry-pick picked..yetanotherpick &&\n \techo changed >unrelated &&\n \tgit cherry-pick --abort &&\n \ttest_cmp expect unrelated\n@@ -179,9 +179,9 @@ test_expect_success '--abort keeps unrelated change, easy case' '\n test_expect_success '--abort refuses to clobber unrelated change, harder case' '\n \tpristine_detach initial &&\n \techo changed >expect &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo changed >unrelated &&\n-\ttest_must_fail git cherry-pick --abort &&\n+\ttest_expect_code 128 git cherry-pick --abort &&\n \ttest_cmp expect unrelated &&\n \tgit rev-list HEAD >log &&\n \ttest_line_count = 2 log &&\n@@ -194,7 +194,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '\n \n test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_expect_code 1 git cherry-pick base..picked &&\n \ttest_path_is_missing .git/sequencer &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n@@ -218,7 +218,7 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le\n \n test_expect_failure '--abort after last commit in sequence' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_expect_code 1 git cherry-pick base..picked &&\n \tgit cherry-pick --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev initial HEAD &&\n@@ -228,27 +228,27 @@ test_expect_failure '--abort after last commit in sequence' '\n \n test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \ttest-chmtime -v +0 .git/sequencer >expect &&\n-\ttest_must_fail git cherry-pick unrelatedpick &&\n+\ttest_expect_code 128 git cherry-pick unrelatedpick &&\n \ttest-chmtime -v +0 .git/sequencer >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '--continue complains when no cherry-pick is in progress' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success '--continue complains when there are unresolved conflicts' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success '--continue continues after conflicts are resolved' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n@@ -275,7 +275,7 @@ test_expect_success '--continue continues after conflicts are resolved' '\n \n test_expect_success '--continue respects opts' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick -x base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick -x base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n@@ -293,7 +293,7 @@ test_expect_success '--continue respects opts' '\n \n test_expect_success '--signoff is not automatically propagated to resolved conflict' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick --signoff base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick --signoff base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n@@ -311,40 +311,40 @@ test_expect_success '--signoff is not automatically propagated to resolved confl\n \n test_expect_success 'malformed instruction sheet 1' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n \tsed \"s/pick /pick/\" .git/sequencer/todo >new_sheet &&\n \tcp new_sheet .git/sequencer/todo &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success 'malformed instruction sheet 2' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n \tsed \"s/pick/revert/\" .git/sequencer/todo >new_sheet &&\n \tcp new_sheet .git/sequencer/todo &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success 'malformed instruction sheet 3' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n \tsed \"s/pick \\([0-9a-f]*\\)/pick $_r10/\" .git/sequencer/todo >new_sheet &&\n \tcp new_sheet .git/sequencer/todo &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success 'instruction sheet, fat-fingers version' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n@@ -355,7 +355,7 @@ test_expect_success 'instruction sheet, fat-fingers version' '\n \n test_expect_success 'commit descriptions in insn sheet are optional' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n-- \n1.7.7.3\n"},{"id":"180666","messageId":"1323445326-24637-7-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:42:03Z","receivedAt":"2011-12-09T15:42:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The next patch allows mixing \"pick\" and \"revert\" instruction in the\nsame instruction sheet. By removing the \"malformed instruction sheet\n2\" test in advance, it'll be easier to see the changes made by the\nnext patch.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n t/t3510-cherry-pick-sequence.sh | 15 ++-------------\n 1 files changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 9ffc085..70fd54b 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -309,7 +309,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl\n \tgrep \"Signed-off-by:\" anotherpick_msg\n '\n \n-test_expect_success 'malformed instruction sheet 1' '\n+test_expect_success 'malformed instruction sheet, action' '\n \tpristine_detach initial &&\n \ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n@@ -320,18 +320,7 @@ test_expect_success 'malformed instruction sheet 1' '\n \ttest_expect_code 128 git cherry-pick --continue\n '\n \n-test_expect_success 'malformed instruction sheet 2' '\n-\tpristine_detach initial &&\n-\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n-\techo \"resolved\" >foo &&\n-\tgit add foo &&\n-\tgit commit &&\n-\tsed \"s/pick/revert/\" .git/sequencer/todo >new_sheet &&\n-\tcp new_sheet .git/sequencer/todo &&\n-\ttest_expect_code 128 git cherry-pick --continue\n-'\n-\n-test_expect_success 'malformed instruction sheet 3' '\n+test_expect_success 'malformed instruction sheet, object name' '\n \tpristine_detach initial &&\n \ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n-- \n1.7.7.3\n"},{"id":"180665","messageId":"1323445326-24637-8-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 7/9] revert: allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:42:04Z","receivedAt":"2011-12-09T15:42:04Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Parse the instruction sheet in '.git/sequencer/todo' as a list of\n(action, operand) pairs, instead of assuming that all instructions use\nthe same action. Now you can do:\n\n pick fdc0b12 picked\n revert 965fed4 anotherpick\n\nFor cherry-pick and revert, this means that a 'git cherry-pick\n--continue' can continue an ongoing revert operation and viceversa.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 133 +++++++++++++++++++--------------------\n t/t3510-cherry-pick-sequence.sh | 58 +++++++++++++++++\n 2 files changed, 124 insertions(+), 67 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 86af516..cc55823 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -39,7 +39,7 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-enum replay_action { REVERT, CHERRY_PICK };\n+enum replay_action { REPLAY_REVERT, REPLAY_PICK };\n enum replay_subcommand {\n \tREPLAY_NONE,\n \tREPLAY_REMOVE_STATE,\n@@ -47,6 +47,12 @@ enum replay_subcommand {\n \tREPLAY_ROLLBACK\n };\n \n+struct replay_insn_list {\n+\tenum replay_action action;\n+\tstruct commit *operand;\n+\tstruct replay_insn_list *next;\n+};\n+\n struct replay_opts {\n \tenum replay_action action;\n \tenum replay_subcommand subcommand;\n@@ -73,14 +79,14 @@ struct replay_opts {\n \n static const char *action_name(const struct replay_opts *opts)\n {\n-\treturn opts->action == REVERT ? \"revert\" : \"cherry-pick\";\n+\treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n }\n \n static char *get_encoding(const char *message);\n \n static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n {\n-\treturn opts->action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n }\n \n static int option_parse_x(const struct option *opt,\n@@ -159,7 +165,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t};\n \n-\tif (opts->action == CHERRY_PICK) {\n+\tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n@@ -363,7 +369,7 @@ static int error_dirty_index(struct replay_opts *opts)\n \t\treturn error_resolve_conflict(action_name(opts));\n \n \t/* Different translation strings for cherry-pick and revert */\n-\tif (opts->action == CHERRY_PICK)\n+\tif (opts->action == REPLAY_PICK)\n \t\terror(_(\"Your local changes would be overwritten by cherry-pick.\"));\n \telse\n \t\terror(_(\"Your local changes would be overwritten by revert.\"));\n@@ -467,7 +473,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n \treturn run_command_v_opt(args, RUN_GIT_CMD);\n }\n \n-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n+static int do_pick_commit(struct commit *commit, enum replay_action action,\n+\t\t\tstruct replay_opts *opts)\n {\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n@@ -542,7 +549,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (opts->action == REVERT) {\n+\tif (action == REPLAY_REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -583,7 +590,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \t}\n \n-\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REVERT) {\n+\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || action == REPLAY_REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n \t\t\t\t\t head, &msgbuf, opts);\n \t\twrite_message(&msgbuf, defmsg);\n@@ -607,13 +614,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t * However, if the merge did not even start, then we don't want to\n \t * write it at all.\n \t */\n-\tif (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))\n+\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))\n \t\twrite_cherry_pick_head(commit, \"CHERRY_PICK_HEAD\");\n-\tif (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))\n+\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))\n \t\twrite_cherry_pick_head(commit, \"REVERT_HEAD\");\n \n \tif (res) {\n-\t\terror(opts->action == REVERT\n+\t\terror(action == REPLAY_REVERT\n \t\t ? _(\"could not revert %s... %s\")\n \t\t : _(\"could not apply %s... %s\"),\n \t\t find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n@@ -637,7 +644,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)\n \n \tinit_revisions(revs, NULL);\n \trevs->no_walk = 1;\n-\tif (opts->action != REVERT)\n+\tif (opts->action != REPLAY_REVERT)\n \t\trevs->reverse = 1;\n \n \targc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);\n@@ -683,49 +690,49 @@ static void read_and_refresh_cache(struct replay_opts *opts)\n * assert(commit_list_count(list) == 2);\n * return list;\n */\n-static struct commit_list **commit_list_append(struct commit *commit,\n-\t\t\t\t\t struct commit_list **next)\n+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,\n+\t\t\t\t\t\tstruct commit *operand,\n+\t\t\t\t\t\tstruct replay_insn_list **next)\n {\n-\tstruct commit_list *new = xmalloc(sizeof(struct commit_list));\n-\tnew->item = commit;\n+\tstruct replay_insn_list *new = xmalloc(sizeof(*new));\n+\tnew->action = action;\n+\tnew->operand = operand;\n \t*next = new;\n \tnew->next = NULL;\n \treturn &new->next;\n }\n \n-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n-\t\tstruct replay_opts *opts)\n+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n {\n-\tstruct commit_list *cur = NULL;\n-\tconst char *sha1_abbrev = NULL;\n-\tconst char *action_str = opts->action == REVERT ? \"revert\" : \"pick\";\n-\tconst char *subject;\n-\tint subject_len;\n+\tstruct replay_insn_list *cur;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n-\t\tsubject_len = find_commit_subject(cur->item->buffer, &subject);\n+\t\tconst char *sha1_abbrev, *action_str, *subject;\n+\t\tint subject_len;\n+\n+\t\taction_str = cur->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n+\t\tsha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);\n+\t\tsubject_len = find_commit_subject(cur->operand->buffer, &subject);\n \t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n \t\t\tsubject_len, subject);\n \t}\n \treturn 0;\n }\n \n-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)\n+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n {\n \tunsigned char commit_sha1[20];\n-\tenum replay_action action;\n \tchar *end_of_object_name;\n \tint saved, status;\n \n \tif (!prefixcmp(bol, \"pick \")) {\n-\t\taction = CHERRY_PICK;\n+\t\titem->action = REPLAY_PICK;\n \t\tbol += strlen(\"pick \");\n \t} else if (!prefixcmp(bol, \"revert \")) {\n-\t\taction = REVERT;\n+\t\titem->action = REPLAY_REVERT;\n \t\tbol += strlen(\"revert \");\n \t} else\n-\t\treturn NULL;\n+\t\treturn -1;\n \n \t/* Eat up extra spaces/ tabs before object name */\n \twhile (*bol == ' ' || *bol == '\\t')\n@@ -737,37 +744,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \tstatus = get_sha1(bol, commit_sha1);\n \t*end_of_object_name = saved;\n \n-\t/*\n-\t * Verify that the action matches up with the one in\n-\t * opts; we don't support arbitrary instructions\n-\t */\n-\tif (action != opts->action) {\n-\t\tconst char *action_str;\n-\t\taction_str = action == REVERT ? \"revert\" : \"cherry-pick\";\n-\t\terror(_(\"Cannot %s during a %s\"), action_str, action_name(opts));\n-\t\treturn NULL;\n-\t}\n-\n \tif (status < 0)\n-\t\treturn NULL;\n+\t\treturn -1;\n+\n+\titem->operand = lookup_commit_reference(commit_sha1);\n+\tif (!item->operand)\n+\t\treturn -1;\n \n-\treturn lookup_commit_reference(commit_sha1);\n+\titem->next = NULL;\n+\treturn 0;\n }\n \n-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n-\t\t\tstruct replay_opts *opts)\n+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n {\n-\tstruct commit_list **next = todo_list;\n-\tstruct commit *commit;\n+\tstruct replay_insn_list **next = todo_list;\n+\tstruct replay_insn_list item = { 0, NULL, NULL };\n \tchar *p = buf;\n \tint i;\n \n \tfor (i = 1; *p; i++) {\n \t\tchar *eol = strchrnul(p, '\\n');\n-\t\tcommit = parse_insn_line(p, eol, opts);\n-\t\tif (!commit)\n+\t\tif (parse_insn_line(p, eol, &item) < 0)\n \t\t\treturn error(_(\"Could not parse line %d.\"), i);\n-\t\tnext = commit_list_append(commit, next);\n+\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n \t\tp = *eol ? eol + 1 : eol;\n \t}\n \tif (!*todo_list)\n@@ -775,8 +774,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \treturn 0;\n }\n \n-static void read_populate_todo(struct commit_list **todo_list,\n-\t\t\tstruct replay_opts *opts)\n+static void read_populate_todo(struct replay_insn_list **todo_list)\n {\n \tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -792,7 +790,7 @@ static void read_populate_todo(struct commit_list **todo_list,\n \t}\n \tclose(fd);\n \n-\tres = parse_insn_buffer(buf.buf, todo_list, opts);\n+\tres = parse_insn_buffer(buf.buf, todo_list);\n \tstrbuf_release(&buf);\n \tif (res)\n \t\tdie(_(\"Unusable instruction sheet: %s\"), todo_file);\n@@ -841,18 +839,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)\n \t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n }\n \n-static void walk_revs_populate_todo(struct commit_list **todo_list,\n+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n-\tstruct commit_list **next;\n+\tstruct replay_insn_list **next;\n \n \tprepare_revs(&revs, opts);\n \n \tnext = todo_list;\n \twhile ((commit = get_revision(&revs)))\n-\t\tnext = commit_list_append(commit, next);\n+\t\tnext = replay_insn_list_append(opts->action, commit, next);\n }\n \n static int create_seq_dir(void)\n@@ -950,7 +948,7 @@ fail:\n \treturn -1;\n }\n \n-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+static void save_todo(struct replay_insn_list *todo_list)\n {\n \tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstatic struct lock_file todo_lock;\n@@ -958,7 +956,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \tint fd;\n \n \tfd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);\n-\tif (format_todo(&buf, todo_list, opts) < 0)\n+\tif (format_todo(&buf, todo_list) < 0)\n \t\tdie(_(\"Could not format %s.\"), todo_file);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_release(&buf);\n@@ -1002,9 +1000,10 @@ static void save_opts(struct replay_opts *opts)\n \t}\n }\n \n-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n+static int pick_commits(struct replay_insn_list *todo_list,\n+\t\t\tstruct replay_opts *opts)\n {\n-\tstruct commit_list *cur;\n+\tstruct replay_insn_list *cur;\n \tint res;\n \n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n@@ -1014,8 +1013,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tread_and_refresh_cache(opts);\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsave_todo(cur, opts);\n-\t\tres = do_pick_commit(cur->item, opts);\n+\t\tsave_todo(cur);\n+\t\tres = do_pick_commit(cur->operand, cur->action, opts);\n \t\tif (res) {\n \t\t\tif (!cur->next)\n \t\t\t\t/*\n@@ -1040,7 +1039,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \n static int pick_revisions(struct replay_opts *opts)\n {\n-\tstruct commit_list *todo_list = NULL;\n+\tstruct replay_insn_list *todo_list = NULL;\n \tunsigned char sha1[20];\n \n \tread_and_refresh_cache(opts);\n@@ -1060,7 +1059,7 @@ static int pick_revisions(struct replay_opts *opts)\n \t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n \t\t\treturn error(_(\"No %s in progress\"), action_name(opts));\n \t\tread_populate_opts(&opts);\n-\t\tread_populate_todo(&todo_list, opts);\n+\t\tread_populate_todo(&todo_list);\n \n \t\t/* Verify that the conflict has been resolved */\n \t\tif (!index_differs_from(\"HEAD\", 0))\n@@ -1078,7 +1077,7 @@ static int pick_revisions(struct replay_opts *opts)\n \tif (create_seq_dir() < 0)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1)) {\n-\t\tif (opts->action == REVERT)\n+\t\tif (opts->action == REPLAY_REVERT)\n \t\t\treturn error(_(\"Can't revert as initial commit\"));\n \t\treturn error(_(\"Can't cherry-pick into empty head\"));\n \t}\n@@ -1095,7 +1094,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tmemset(&opts, 0, sizeof(opts));\n \tif (isatty(0))\n \t\topts.edit = 1;\n-\topts.action = REVERT;\n+\topts.action = REPLAY_REVERT;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = pick_revisions(&opts);\n@@ -1110,7 +1109,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tint res;\n \n \tmemset(&opts, 0, sizeof(opts));\n-\topts.action = CHERRY_PICK;\n+\topts.action = REPLAY_PICK;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = pick_revisions(&opts);\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 70fd54b..aee869d 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '\n \ttest_line_count = 4 commits\n '\n \n+test_expect_success 'revert --continue continues after cherry-pick' '\n+\tpristine_detach initial &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tgit revert --continue &&\n+\ttest_path_is_missing .git/sequencer &&\n+\t{\n+\t\tgit rev-list HEAD |\n+\t\tgit diff-tree --root --stdin |\n+\t\tsed \"s/$_x40/OBJID/g\"\n+\t} >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tunrelated\n+\tOBJID\n+\t:000000 100644 OBJID OBJID A\tfoo\n+\t:000000 100644 OBJID OBJID A\tunrelated\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'mixed pick and revert instructions' '\n+\tpristine_detach initial &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\toldsha=`git rev-parse --short HEAD~1` &&\n+\techo \"revert $oldsha unrelatedpick\" >>.git/sequencer/todo &&\n+\tgit cherry-pick --continue &&\n+\ttest_path_is_missing .git/sequencer &&\n+\t{\n+\t\tgit rev-list HEAD |\n+\t\tgit diff-tree --root --stdin |\n+\t\tsed \"s/$_x40/OBJID/g\"\n+\t} >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tunrelated\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tunrelated\n+\tOBJID\n+\t:000000 100644 OBJID OBJID A\tfoo\n+\t:000000 100644 OBJID OBJID A\tunrelated\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.7.3\n"},{"id":"180667","messageId":"1323445326-24637-9-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 8/9] revert: report fine-grained error messages from insn parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:42:05Z","receivedAt":"2011-12-09T15:42:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Three kinds of errors can arise from parsing '.git/sequencer/todo':\n1. Unrecognized action\n2. Malformed object name\n3. Object name does not refer to a valid commit\n\nSince we would like to make the instruction sheet user-editable in the\nfuture (much like the 'rebase -i' sheet), report more fine-grained\nparse errors prefixed with the filename and line number.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 23 ++++++++++++++++-------\n 1 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex cc55823..e2355d8 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -719,8 +719,10 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n \treturn 0;\n }\n \n-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n+static int parse_insn_line(char *bol, char *eol,\n+\t\t\tstruct replay_insn_list *item, int lineno)\n {\n+\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tunsigned char commit_sha1[20];\n \tchar *end_of_object_name;\n \tint saved, status;\n@@ -731,8 +733,13 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n \t} else if (!prefixcmp(bol, \"revert \")) {\n \t\titem->action = REPLAY_REVERT;\n \t\tbol += strlen(\"revert \");\n-\t} else\n-\t\treturn -1;\n+\t} else {\n+\t\tsize_t len = eol - bol;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"%s:%d: Unrecognized action: %.*s\"),\n+\t\t\ttodo_file, lineno, (int)len, bol);\n+\t}\n \n \t/* Eat up extra spaces/ tabs before object name */\n \twhile (*bol == ' ' || *bol == '\\t')\n@@ -745,11 +752,13 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n \t*end_of_object_name = saved;\n \n \tif (status < 0)\n-\t\treturn -1;\n+\t\treturn error(_(\"%s:%d: Malformed object name: %s\"),\n+\t\t\ttodo_file, lineno, bol);\n \n \titem->operand = lookup_commit_reference(commit_sha1);\n \tif (!item->operand)\n-\t\treturn -1;\n+\t\treturn error(_(\"%s:%d: Not a valid commit: %s\"),\n+\t\t\ttodo_file, lineno, bol);\n \n \titem->next = NULL;\n \treturn 0;\n@@ -764,8 +773,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n \n \tfor (i = 1; *p; i++) {\n \t\tchar *eol = strchrnul(p, '\\n');\n-\t\tif (parse_insn_line(p, eol, &item) < 0)\n-\t\t\treturn error(_(\"Could not parse line %d.\"), i);\n+\t\tif (parse_insn_line(p, eol, &item, i) < 0)\n+\t\t\treturn -1;\n \t\tnext = replay_insn_list_append(item.action, item.operand, next);\n \t\tp = *eol ? eol + 1 : eol;\n \t}\n-- \n1.7.7.3\n"},{"id":"180668","messageId":"1323445326-24637-10-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 9/9] revert: simplify communicating command-line arguments","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T15:42:06Z","receivedAt":"2011-12-09T15:42:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"From: Jonathan Nieder \n\nSince 7e2bfd3f (revert: allow cherry-picking more than one commit,\n2010-07-02), the pick/revert machinery has kept track of the set of\ncommits to be cherry-picked or reverted using commit_argc and\ncommit_argv variables, storing the corresponding command-line\nparameters.\n\nFuture callers as other commands are built in (am, rebase, sequencer)\nmay find it easier to pass rev-list options to this machinery in\nalready-parsed form. So, teach cmd_cherry_pick and cmd_revert to\nparse the rev-list arguments in advance and pass the commit set to\npick_revisions() as a \"struct rev_info\".\n\nSigned-off-by: Ramkumar Ramachandra \nAcked-by: Jonathan Nieder \nSigned-off-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 53 +++++++++++++++++++++-----------------\n t/t3510-cherry-pick-sequence.sh | 11 ++++++++\n 2 files changed, 40 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e2355d8..8d86bfd 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -66,13 +66,14 @@ struct replay_opts {\n \tint allow_rerere_auto;\n \n \tint mainline;\n-\tint commit_argc;\n-\tconst char **commit_argv;\n \n \t/* Merge strategy */\n \tconst char *strategy;\n \tconst char **xopts;\n \tsize_t xopts_nr, xopts_alloc;\n+\n+\t/* Only used by REPLAY_NONE */\n+\tstruct rev_info *revs;\n };\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n@@ -175,9 +176,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tdie(_(\"program error\"));\n \t}\n \n-\topts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,\n-\t\t\t\t\tPARSE_OPT_KEEP_ARGV0 |\n-\t\t\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n+\targc = parse_options(argc, argv, NULL, options, usage_str,\n+\t\t\tPARSE_OPT_KEEP_ARGV0 |\n+\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n \n \t/* Check for incompatible subcommands */\n \tverify_opt_mutually_compatible(me,\n@@ -219,9 +220,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\tNULL);\n \t}\n \n-\telse if (opts->commit_argc < 2)\n-\t\tusage_with_options(usage_str, options);\n-\n \tif (opts->allow_ff)\n \t\tverify_opt_compatible(me, \"--ff\",\n \t\t\t\t\"--signoff\", opts->signoff,\n@@ -229,7 +227,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"-x\", opts->record_origin,\n \t\t\t\t\"--edit\", opts->edit,\n \t\t\t\tNULL);\n-\topts->commit_argv = argv;\n+\n+\tif (opts->subcommand == REPLAY_NONE) {\n+\t\topts->revs = xmalloc(sizeof(*opts->revs));\n+\t\tinit_revisions(opts->revs, NULL);\n+\t\topts->revs->no_walk = 1;\n+\t\tif (argc < 2)\n+\t\t\tusage_with_options(usage_str, options);\n+\t\targc = setup_revisions(argc, argv, opts->revs, NULL);\n+\t} else\n+\t\topts->revs = NULL;\n+\n+\t/* Forbid stray command-line arguments */\n+\tif (argc > 1)\n+\t\tusage_with_options(usage_str, options);\n }\n \n struct commit_message {\n@@ -638,23 +649,15 @@ static int do_pick_commit(struct commit *commit, enum replay_action action,\n \treturn res;\n }\n \n-static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)\n+static void prepare_revs(struct replay_opts *opts)\n {\n-\tint argc;\n-\n-\tinit_revisions(revs, NULL);\n-\trevs->no_walk = 1;\n \tif (opts->action != REPLAY_REVERT)\n-\t\trevs->reverse = 1;\n+\t\topts->revs->reverse ^= 1;\n \n-\targc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);\n-\tif (argc > 1)\n-\t\tusage(*revert_or_cherry_pick_usage(opts));\n-\n-\tif (prepare_revision_walk(revs))\n+\tif (prepare_revision_walk(opts->revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \n-\tif (!revs->commits)\n+\tif (!opts->revs->commits)\n \t\tdie(_(\"empty commit set passed\"));\n }\n \n@@ -851,14 +854,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)\n static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n-\tstruct rev_info revs;\n \tstruct commit *commit;\n \tstruct replay_insn_list **next;\n \n-\tprepare_revs(&revs, opts);\n+\tprepare_revs(opts);\n \n \tnext = todo_list;\n-\twhile ((commit = get_revision(&revs)))\n+\twhile ((commit = get_revision(opts->revs)))\n \t\tnext = replay_insn_list_append(opts->action, commit, next);\n }\n \n@@ -1051,6 +1053,9 @@ static int pick_revisions(struct replay_opts *opts)\n \tstruct replay_insn_list *todo_list = NULL;\n \tunsigned char sha1[20];\n \n+\tif (opts->subcommand == REPLAY_NONE)\n+\t\tassert(opts->revs);\n+\n \tread_and_refresh_cache(opts);\n \n \t/*\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex aee869d..1f4685a 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'empty commit set' '\n+\tpristine_detach initial &&\n+\ttest_expect_code 128 git cherry-pick base..base\n+'\n+\n+test_expect_success 'commit set passed through --all' '\n+\tpristine_detach initial &&\n+\ttest_expect_code 1 git cherry-pick --all &&\n+\tgit cherry-pick --continue\n+'\n+\n test_done\n-- \n1.7.7.3\n"},{"id":"180686","messageId":"20111209190236.GA20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-10-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 9/9] revert: simplify communicating command-line arguments","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T19:02:36Z","receivedAt":"2011-12-09T19:02:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hey,\n\nRamkumar Ramachandra wrote:\n\n> From: Jonathan Nieder \n[...]\n> Future callers as other commands are built in (am, rebase, sequencer)\n> may find it easier to pass rev-list options to this machinery in\n> already-parsed form. So, teach cmd_cherry_pick and cmd_revert to\n> parse the rev-list arguments in advance and pass the commit set to\n> pick_revisions() as a \"struct rev_info\".\n>\n> Signed-off-by: Ramkumar Ramachandra \n> Acked-by: Jonathan Nieder \n> Signed-off-by: Jonathan Nieder \n> Signed-off-by: Junio C Hamano \n\nThis sign-off chain suggests that the life of the patch happened in\nthree stages:\n\n - first, you wrote the patch or some component of it and passed it to\n me.\n - then, I accepted it, tweaked it so much that I felt the need to claim\n authorship and save you from blame (hence the \"From:\" field), and\n passed it on to Junio.\n - finally, Junio applied it to some tree.\n\nand that you are sending out a copy of the patch Junio applied for\nadditional comments.\n\nBut in fact, this started with a patch from me (I don't rememember\nwhether it was signed off, but that doesn't matter --- I happily\nretroactively sign off on it) and now you are sending it to the list\nand Junio for comments. So I would think it should simply say\n\n\tSigned-off-by: Jonathan Nieder \n\tSigned-off-by: Ramkumar Ramachandra \n\nand Junio can add his sign-off below that when applying some version\nto his tree. Bonus points if you mention what tweaks you made when\nyou are not just passing it on.\n\n[...]\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'empty commit set' '\n> +\tpristine_detach initial &&\n> +\ttest_expect_code 128 git cherry-pick base..base\n> +'\n> +\n> +test_expect_success 'commit set passed through --all' '\n> +\tpristine_detach initial &&\n> +\ttest_expect_code 1 git cherry-pick --all &&\n> +\tgit cherry-pick --continue\n> +'\n\nWhat does this test mean? \"git cherry-pick --all\" should report a\nspurious conflict, and then --continue will clean it up automagically\nand all will be well?\n"},{"id":"180687","messageId":"CALkWK0m_6yGuyLt-VqvRZkAiDoSxES8KeNzeXaejYRCpW=HAOg@mail.gmail.com","threadId":"29119","inReplyTo":"20111209190236.GA20913@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 9/9] revert: simplify communicating command-line arguments","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T19:11:47Z","receivedAt":"2011-12-09T19:11:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder wrote:\n> [...]\n>        Signed-off-by: Jonathan Nieder \n>        Signed-off-by: Ramkumar Ramachandra \n>\n> and Junio can add his sign-off below that when applying some version\n> to his tree.  Bonus points if you mention what tweaks you made when\n> you are not just passing it on.\n\nTrue, the signoff-chain does look like quite a mess now :P\n[rr: minor improvements, tests] should be fine.\n\n>> --- a/t/t3510-cherry-pick-sequence.sh\n>> +++ b/t/t3510-cherry-pick-sequence.sh\n>> @@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '\n>> +test_expect_success 'commit set passed through --all' '\n>> +     pristine_detach initial &&\n>> +     test_expect_code 1 git cherry-pick --all &&\n>> +     git cherry-pick --continue\n>> +'\n>\n> What does this test mean?  \"git cherry-pick --all\" should report a\n> spurious conflict, and then --continue will clean it up automagically\n> and all will be well?\n\nThis one's actually quite interesting. \"git cherry-pick --all\" first\ntries to apply everything from \"intial\" to \"yetanotherpatch\" (both\ninclusive) -- its first \"git commit\" invocation returns 1, refusing to\ncreate an empty commit. Then when we say \"--continue\", it notices\nthat the worktree and index are clean, removes \"initial\" from the\ninstruction sheet and executes everything else as usual. This is\nsomething we should attempt to fix in the future: I think the Right\nWay starts with creating an API for \"git commit\".\n\nThanks.\n\n-- Ram\n"},{"id":"180689","messageId":"20111209192919.GB20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"CALkWK0m_6yGuyLt-VqvRZkAiDoSxES8KeNzeXaejYRCpW=HAOg@mail.gmail.com","subject":"Re: [PATCH 9/9] revert: simplify communicating command-line arguments","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T19:29:19Z","receivedAt":"2011-12-09T19:29:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n>> Ramkumar Ramachandra wrote:\n\n>>> +++ b/t/t3510-cherry-pick-sequence.sh\n>>> @@ -414,4 +414,15 @@ test_expect_success 'mixed pick and revert instructions' '\n>>> +test_expect_success 'commit set passed through --all' '\n>>> +     pristine_detach initial &&\n>>> +     test_expect_code 1 git cherry-pick --all &&\n>>> +     git cherry-pick --continue\n>>> +'\n[...]\n> This one's actually quite interesting. \"git cherry-pick --all\" first\n> tries to apply everything from \"intial\" to \"yetanotherpatch\" (both\n> inclusive) -- its first \"git commit\" invocation returns 1, refusing to\n> create an empty commit. Then when we say \"--continue\", it notices\n> that the worktree and index are clean, removes \"initial\" from the\n> instruction sheet and executes everything else as usual. This is\n> something we should attempt to fix\n\nSo, is it a bad test? Was the initial command crazy and ill-defined\nenough that no one would actually do that? Is the response to the\ncommand incorrect, meaning that the new test should be instead\nchecking for some different result with test_expect_failure?\n\nI only mentioned --all in the first place because it is a \"revision\npseudo-option\" (i.e., option starting with \"--\" whose position on the\nrev-list command line matters) and gets handled by a slightly\ndifferent revision parsing code path than foo..bar. There are other\nrevision pseudo-options that are easier to control and might make for\na better test if it's wanted, like --remotes=foo.\n\n> Way starts with creating an API for \"git commit\".\n\nNot sure what this means.\n"},{"id":"180690","messageId":"20111209193216.GC20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/9] revert: make commit subjects in insn sheet optional","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T19:32:16Z","receivedAt":"2011-12-09T19:32:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> In addition to literal SHA-1 hexes, expressions like\n> the following are valid object names in the instruction sheet:\n>\n> featurebranch~4\n> rr/revert-cherry-pick-continue^2~12@{12 days ago}\n\nGlad to see this new mention. My comment from reviewing the last\niteration doesn't seem to have been addressed, though. Will follow\nup there.\n"},{"id":"180692","messageId":"20111209194003.GE20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T19:40:03Z","receivedAt":"2011-12-09T19:40:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> +++ b/builtin/revert.c\n> @@ -727,7 +727,11 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n> \t} else\n> \t\treturn NULL;\n> \n> +\t/* Eat up extra spaces/ tabs before object name */\n> +\twhile (*bol == ' ' || *bol == '\\t')\n> +\t\tbol += 1;\n> -\tend_of_object_name = bol + strcspn(bol, \" \\n\");\n\nWhy not use strspn? What happens if I use a tab immediately\nafter the pick/revert keyword?\n"},{"id":"180694","messageId":"20111209194727.GF20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/9] revert: simplify getting commit subject in format_todo()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T19:47:27Z","receivedAt":"2011-12-09T19:47:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> format_todo() calls get_message(), but uses only the subject line of\n> the commit message. As a minor optimization, save work and\n> unnecessary memory allocations by using find_commit_subject() instead.\n\nNice. Thanks for explaining.\n\n> Also, remove the unnecessary check on cur->item: the previous patch\n> makes sure that instruction sheets with missing commit subjects are\n> parsable.\n\nI guess you mean the check that cur->item->buffer is non-NULL. But\nnow I am confused --- what would that have to do with instruction\nsheets with missing commit subjects? If cur->item->buffer is NULL,\nisn't find_commit_subject going to segfault regardless?\n\nCan cur->item->buffer be NULL?\n"},{"id":"180695","messageId":"CALkWK0m2veE8FmFVTPEqNAmbtvm1sWVHtFt0QOWU=huQFafeBw@mail.gmail.com","threadId":"29119","inReplyTo":"20111209192919.GB20913@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 9/9] revert: simplify communicating command-line arguments","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T19:49:41Z","receivedAt":"2011-12-09T19:49:41Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n> [...]\n>\n> So, is it a bad test?\n\nI'll just drop this test.\n\n>> Way starts with creating an API for \"git commit\".\n>\n> Not sure what this means.\n\nIt certainly doesn't make any sense when you quote it like that. What\nI meant is that: by convention, 'git cherry-pick' is supposed to exit\nwith positive status on conflict. Conversely, this means that a\npositive exit status can be interpreted as a conflict, but this is\nclearly not the case here. How do we fix this problem? By creating\nan API for \"git commit\", not by shelling out like this and letting it\ntake over the exit status.\n\n-- Ram\n"},{"id":"180698","messageId":"CALkWK0kCZ7r2-_jF2st3h8MUF_kCgaLvrNZc0uxBxdpiK-7bLg@mail.gmail.com","threadId":"29119","inReplyTo":"20111209194003.GE20913@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T19:58:21Z","receivedAt":"2011-12-09T19:58:21Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>\n>> +++ b/builtin/revert.c\n>> @@ -727,7 +727,11 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n>>       } else\n>>               return NULL;\n>>\n>> +     /* Eat up extra spaces/ tabs before object name */\n>> +     while (*bol == ' ' || *bol == '\\t')\n>> +             bol += 1;\n>> -     end_of_object_name = bol + strcspn(bol, \" \\n\");\n>\n> Why not use strspn?  What happens if I use a tab immediately\n> after the pick/revert keyword?\n\n:facepalm: Fixed. Inter-diff:\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex b976562..be0686d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -716,20 +716,22 @@ static struct commit *parse_insn_line(\n \tunsigned char commit_sha1[20];\n \tenum replay_action action;\n \tchar *end_of_object_name;\n-\tint saved, status;\n+\tint saved, status, padding;\n\n-\tif (!prefixcmp(bol, \"pick \")) {\n+\tif (!prefixcmp(bol, \"pick\")) {\n \t\taction = CHERRY_PICK;\n-\t\tbol += strlen(\"pick \");\n-\t} else if (!prefixcmp(bol, \"revert \")) {\n+\t\tbol += strlen(\"pick\");\n+\t} else if (!prefixcmp(bol, \"revert\")) {\n \t\taction = REVERT;\n-\t\tbol += strlen(\"revert \");\n+\t\tbol += strlen(\"revert\");\n \t} else\n \t\treturn NULL;\n\n \t/* Eat up extra spaces/ tabs before object name */\n-\twhile (*bol == ' ' || *bol == '\\t')\n-\t\tbol += 1;\n+\tpadding = strspn(bol, \" \\t\");\n+\tif (!padding)\n+\t\treturn NULL;\n+\tbol += padding;\n\n \tend_of_object_name = bol + strcspn(bol, \" \\t\\n\");\n \tsaved = *end_of_object_name;\n--\n\nThanks.\n\n-- Ram\n"},{"id":"180700","messageId":"7vty59ec6w.fsf@alter.siamese.dyndns.org","threadId":"29119","inReplyTo":"1323445326-24637-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-09T20:12:55Z","receivedAt":"2011-12-09T20:12:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Tolerate extra spaces and tabs as part of the the field separator in\n> '.git/sequencer/todo', for people with fat fingers.\n>\n> Requested-by: Junio C Hamano \n> Signed-off-by: Ramkumar Ramachandra \n> ---\n\nI did not request it, by the way. I actually was hinting to start tight\nand later loosen the parsing.\n\nAlso if you are using strcspn() why use a hand-rolled loop instead of\nstrspn()?\n"},{"id":"180701","messageId":"CALkWK0n3-X3EXptFqSB_L3eAsu-5LXsM3jJETPf3pN5C3ghSSg@mail.gmail.com","threadId":"29119","inReplyTo":"7vty59ec6w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/9] revert: tolerate extra spaces, tabs in insn sheet","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T20:15:39Z","receivedAt":"2011-12-09T20:15:39Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano wrote:\n> Also if you are using strcspn() why use a hand-rolled loop instead of\n> strspn()?\n\nHonestly, it didn't occur to me: this is the first time I'm using\neither strcspn() or stcspn().\n\nThanks.\n\n-- Ram\n"},{"id":"180702","messageId":"20111209202149.GH20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T20:21:49Z","receivedAt":"2011-12-09T20:21:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Since cf3e2486 (revert: Propagate errors upwards from do_pick_commit,\n> 2011-08-04), 'git cherry-pick' has three different ways of failing:\n>\n> 1. die() with the exit status 128.\n> 3. error() out with exit status -1.\n> 2. exit with positive exit status to indicate a conflict.\n\nI think your list-item counter is showing a little jitter. :)\n\nerror() does not produce exit status -1, and any situation other than\npropagating exit status from a user-defined script in which git exits\nwith status 255 is a bug (yes, I know there are a couple, though none\nI know of in cherry-pick code paths).\n\nHasn't cherry-pick had two ways to exit with failing status like \"git\nmerge\" does (conflicts versus error that didn't even allow us to\nstart) since the very beginning?\n\n[...]\n> So, replace all instances of 'test_must_fail' with\n> 'test_expect_code' to check the exit status explicitly.\n\nSounds like a sensible idea. Probably this one sentence, plus a quick\nnote on the user-visible exit status convention, would suffice for\nexplaining it.\n\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n[...]\n> @@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '\n> \n> test_expect_success 'cherry-pick persists opts correctly' '\n> \tpristine_detach initial &&\n> -\ttest_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n> +\ttest_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n> \ttest_path_is_dir .git/sequencer &&\n\nEncountered conflicts, preserving options, but the exit is with status\n128? Smells like a bug.\n\n[...]\n> @@ -88,12 +88,12 @@ test_expect_success '--quit does not complain when no cherry-pick is in progress\n> \n> test_expect_success '--abort requires cherry-pick in progress' '\n> \tpristine_detach initial &&\n> -\ttest_must_fail git cherry-pick --abort\n> +\ttest_expect_code 128 git cherry-pick --abort\n\nI don't think the exit status is important for this one. (I.e., I can\nimagine some future version of cherry-pick using different small\npositive integers to refer to different reasons for --abort to fail,\nand I don't think that would be a problem or break anything.)\n\n[...]\n> @@ -179,9 +179,9 @@ test_expect_success '--abort keeps unrelated change, easy case' '\n> test_expect_success '--abort refuses to clobber unrelated change, harder case' '\n> \tpristine_detach initial &&\n> \techo changed >expect &&\n> -\ttest_must_fail git cherry-pick base..anotherpick &&\n> +\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n> \techo changed >unrelated &&\n> -\ttest_must_fail git cherry-pick --abort &&\n> +\ttest_expect_code 128 git cherry-pick --abort &&\n\nLikewise here.\n\nExcept as noted above,\nReviewed-by: Jonathan Nieder \n"},{"id":"180708","messageId":"20111209202449.GI20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-7-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T20:24:49Z","receivedAt":"2011-12-09T20:24:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> By removing the \"malformed instruction sheet\n> 2\" test in advance, it'll be easier to see the changes made by the\n> next patch.\n\nSo, this is a regression in test coverage without a redeeming upside\nother than allowing the next patch to be prettier. Naturally, I don't\nlike it.\n"},{"id":"180709","messageId":"20111209202645.GJ20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-8-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 7/9] revert: allow mixed pick and revert instructions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T20:26:45Z","receivedAt":"2011-12-09T20:26:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Parse the instruction sheet in '.git/sequencer/todo' as a list of\n> (action, operand) pairs, instead of assuming that all instructions use\n> the same action. Now you can do:\n>\n> pick fdc0b12 picked\n> revert 965fed4 anotherpick\n>\n> For cherry-pick and revert, this means that a 'git cherry-pick\n> --continue' can continue an ongoing revert operation and viceversa.\n\nSounds like a good thing.\n\n[...]\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -39,7 +39,7 @@ static const char * const cherry_pick_usage[] = {\n> \tNULL\n> };\n> \n> -enum replay_action { REVERT, CHERRY_PICK };\n> +enum replay_action { REPLAY_REVERT, REPLAY_PICK };\n\nWhat does this have to do with it?\n"},{"id":"180710","messageId":"CALkWK0mEP5nDgdosOiquQ_FWbNRZesi38NeCD_yGPvJ8JQxkGg@mail.gmail.com","threadId":"29119","inReplyTo":"20111209202449.GI20913@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T20:30:46Z","receivedAt":"2011-12-09T20:30:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>\n>>                          By removing the \"malformed instruction sheet\n>> 2\" test in advance, it'll be easier to see the changes made by the\n>> next patch.\n>\n> So, this is a regression in test coverage without a redeeming upside\n> other than allowing the next patch to be prettier.  Naturally, I don't\n> like it.\n\nWithout this patch, the diffs of _all_ the future commits in this\nseries touching this file are totally unreadable. I've noticed that\nthe diffing algorithm performs especially badly for t/*.sh -- rebasing\ntests is generally a huge pain.\n\n-- Ram\n"},{"id":"180711","messageId":"20111209203442.GK20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T20:34:42Z","receivedAt":"2011-12-09T20:34:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> - \"revert: report fine-grained error messages from insn parser\" arises\n> from Jonathan's request to split \"revert: allow mixed pick and\n> revert instructions\".\n\nJust to be clear: I wasn't directly requesting that you do anything.\nIf I were, then you could carefully read my requirements, fulfill\nthem, and you would be done.\n\nInstead, I was reviewing the patch and giving my reaction. After\nreceiving that information, one has plenty of choices:\n\n - add documentation to avoid the confusion the reaction was based on\n - rework to fix the underlying problem that caused the reaction\n - think carefully about it, conclude that the reviewer is crazy, and\n ignore it\n - drop the patch\n - send out a call for help to get others to help work on the\n underlying problem\n - ask for clarification\n ...\n\n>From my point of view as a reviewer, I am happiest when someone\nfigures out how I missed the point and comes up with some fix that\naddresses the underlying problem instead (and, incidentally, gets rid\nof the symptom that provoked my reaction on the way).\n\nWell, you get the idea.\n"},{"id":"180712","messageId":"CALkWK0=a=-4N4aZHuu=5zkNtwmfLsog5WkBVbuJAbYpvaLUsAg@mail.gmail.com","threadId":"29119","inReplyTo":"20111209202149.GH20913@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T20:36:47Z","receivedAt":"2011-12-09T20:36:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>\n> [...]\n>> --- a/t/t3510-cherry-pick-sequence.sh\n>> +++ b/t/t3510-cherry-pick-sequence.sh\n> [...]\n>> @@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '\n>>\n>>  test_expect_success 'cherry-pick persists opts correctly' '\n>>       pristine_detach initial &&\n>> -     test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n>> +     test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n>>       test_path_is_dir .git/sequencer &&\n>\n> Encountered conflicts, preserving options, but the exit is with status\n> 128?  Smells like a bug.\n\nNo bug. Notice that \"-m 1\" is used when \"initial\" isn't a merge\ncommit. But yeah, I should probably clarify this by changing the\nrevision range to \"initial..anotherpick\" so as not to distract the\nuser.\n\n-- Ram\n"},{"id":"180713","messageId":"20111209203729.GL20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"CALkWK0mEP5nDgdosOiquQ_FWbNRZesi38NeCD_yGPvJ8JQxkGg@mail.gmail.com","subject":"Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T20:37:30Z","receivedAt":"2011-12-09T20:37:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> I've noticed that\n> the diffing algorithm performs especially badly for t/*.sh -- rebasing\n> tests is generally a huge pain.\n\nNo clue about this particular situation, but I suspect the general\ncause for such rebasing trouble is adding tests at the end of the file\n(or some other contended place). Better to figure out a logical place\nfor each test and put it there from the start.\n"},{"id":"180714","messageId":"20111209204715.GM20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"1323445326-24637-9-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 8/9] revert: report fine-grained error messages from insn parser","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T20:47:15Z","receivedAt":"2011-12-09T20:47:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -719,8 +719,10 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n> \treturn 0;\n> }\n> \n> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n> +static int parse_insn_line(char *bol, char *eol,\n> +\t\t\tstruct replay_insn_list *item, int lineno)\n> {\n> +\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n> \tunsigned char commit_sha1[20];\n\nI know that this function does not call git_path() again before the\nvalue is used, so this is safe today, but I do not trust people in the\nfuture to preserve that property (for example, maybe someone will want\nto call get_sha1() earlier). Why not wait to call git_path() when it\nis time to use the value it returns?\n"},{"id":"180715","messageId":"CALkWK0nCqWj0Hx0EGoONMAs-85wvwjMnEAaBW06AMT6Oz373vA@mail.gmail.com","threadId":"29119","inReplyTo":"20111209194727.GF20913@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 4/9] revert: simplify getting commit subject in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T20:58:19Z","receivedAt":"2011-12-09T20:58:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n> [...]\n> Can cur->item->buffer be NULL?\n\nAs Junio correctly points out in $gmane/186365, no. To quote him:\n\n parse_insn_line() uses lookup_commit_reference() which\n calls parse_object() on the object name (and if it is a tag, deref_tag()\n will parse the tagged object until we see something that is not a tag), so\n we know cur->item is parsed before we see it\n\n>> Also, remove the unnecessary check on cur->item: the previous patch\n>> makes sure that instruction sheets with missing commit subjects are\n>> parsable.\n>\n> But now I am confused\n> [...]\n\nThis part of the commit message is simply awful. Replaced with:\n\n Also remove the unnecessary check on cur->item->buffer:\n the lookup_commit_reference() call in parse_insn_line() has already\n made sure of this.\n\n-- Ram\n"},{"id":"180716","messageId":"20111209210058.GN20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"CALkWK0=a=-4N4aZHuu=5zkNtwmfLsog5WkBVbuJAbYpvaLUsAg@mail.gmail.com","subject":"Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T21:00:58Z","receivedAt":"2011-12-09T21:00:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Jonathan Nieder wrote:\n>> Ramkumar Ramachandra wrote:\n\n>>> -     test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n>>> +     test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n>>>       test_path_is_dir .git/sequencer &&\n>>\n>> Encountered conflicts, preserving options, but the exit is with status\n>> 128?  Smells like a bug.\n>\n> No bug.\n\nOk. I'm fuzzy on the details, but is it possible to make this change\nin such a way as to make that obvious? For example, perhaps this\nshould be split into several tests: one to check that such mistaken\nuse of \"-m 1\" with non-merge commits correctly interrupts the\ncherry-pick and pleads to the user for advice (should it?), another to\ncheck that doing so produces an exit status of 128 (if it should), and\nanother to make sure that doing so, fixing things up somehow, and\nresuming the sequence allows the effect of \"-m 1\" to carry over to\nlater commits.\n"},{"id":"180717","messageId":"20111209210928.GO20913@elie.hsd1.il.comcast.net","threadId":"29119","inReplyTo":"CALkWK0m2veE8FmFVTPEqNAmbtvm1sWVHtFt0QOWU=huQFafeBw@mail.gmail.com","subject":"Re: [PATCH 9/9] revert: simplify communicating command-line arguments","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T21:09:28Z","receivedAt":"2011-12-09T21:09:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> a\n> positive exit status can be interpreted as a conflict, but this is\n> clearly not the case here. How do we fix this problem? By creating\n> an API for \"git commit\", not by shelling out like this and letting it\n> take over the exit status.\n\nThat might be a nice thing to do anyway, but I don't see how it would\nsolve anything. The new \"git commit\" API would presumably return an\ninteger or enum value to indicate the result of trying to commit.\nTests in the testsuite for the \"git commit\" API would use the \"git\ncommit\" command, which would expose the newly fine-grained values\nsomehow. And other people scripting but wanting the porcelain to take\ncare of basic UI would benefit, too. Right?\n\nActually, I think cherry-pick returning a positive exit status for\n\"nothing left to commit after resolving conflicts\" would be sensible.\nIt is \"I did what you asked but need your help to determine the final\ncontent of the commit or decide to skip it\", rather than \"you asked\nfor something unsensible and I am bailing out\".\n"},{"id":"180721","messageId":"CALkWK0kbV2WFfGVrA9m_Uwd4J8+U9Yde9Wxb-OZE9Y8K+Ta_4A@mail.gmail.com","threadId":"29119","inReplyTo":"1323445326-24637-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T23:03:59Z","receivedAt":"2011-12-09T23:03:59Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nI'm off on a short vacation next week. I've already fixed up the\nseries in response to most (if not all) reviews. If anyone's\ninterested in putting in some finishing touches, it's available here:\n\n https://github.com/artagnon/git.git rr/revert-cherry-pick ;# 195e68e\n\nThanks.\n\n-- Ram\n"},{"id":"181162","messageId":"1323881677-11117-1-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"CALkWK0kbV2WFfGVrA9m_Uwd4J8+U9Yde9Wxb-OZE9Y8K+Ta_4A@mail.gmail.com","subject":"[PATCH 00/10] Re-roll rr/revert-cherry-pick v3","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:27Z","receivedAt":"2011-12-14T16:54:27Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThere are 10 parts in this iteration. The main changes are:\n\n- \"t3502, t3510: clarify cherry-pick -m failure\" and \"revert: move\nreplay_action, replay_subcommand to header\" are new. They've been\nmade separate parts for clarity.\n\n- Since jn/maint-sequencer-fixes contains \"revert: pass around\nrev-list args in already-parsed form\", I've dropped it from this\nseries. Except for the tests, I've checked that it doesn't conflict\nwith the topic.\n\n- Rebased on new `master`.\n\n- Other changes are fairly minor, and mainly arise as responses to\n Jonathan's reviews.\n\nThanks.\n\n-- Ram\n\nRamkumar Ramachandra (10):\n revert: free msg in format_todo()\n revert: make commit subjects in insn sheet optional\n revert: tolerate extra spaces, tabs in insn sheet\n revert: simplify getting commit subject in format_todo()\n t3510 (cherry-pick-sequencer): use exit status\n t3502, t3510: clarify cherry-pick -m failure\n t3510 (cherry-pick-sequencer): remove malformed sheet 2\n revert: move replay_action, replay_subcommand to header\n revert: allow mixed pick and revert instructions\n revert: report fine-grained error messages from insn parser\n\n builtin/revert.c | 194 ++++++++++++++++++++-------------------\n sequencer.h | 18 ++++\n t/t3502-cherry-pick-merge.sh | 2 +-\n t/t3510-cherry-pick-sequence.sh | 142 +++++++++++++++++++++++------\n 4 files changed, 231 insertions(+), 125 deletions(-)\n\n-- \n1.7.4.1\n"},{"id":"181163","messageId":"1323881677-11117-2-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 01/10] revert: free msg in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:28Z","receivedAt":"2011-12-14T16:54:28Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Memory allocated to the fields of msg by get_message() isn't freed.\nThis is potentially a big leak, because fresh memory is allocated to\nstore the commit message for each commit. Fix this using\nfree_message().\n\nReported-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1ea525c..0c6d3d8 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\tif (get_message(cur->item, &msg))\n \t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n \t\tstrbuf_addf(buf, \"%s %s %s\\n\", action_str, sha1_abbrev, msg.subject);\n+\t\tfree_message(&msg);\n \t}\n \treturn 0;\n }\n-- \n1.7.4.1\n"},{"id":"181164","messageId":"1323881677-11117-3-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 02/10] revert: make commit subjects in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:29Z","receivedAt":"2011-12-14T16:54:29Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Change the instruction sheet format subtly so that the subject of the\ncommit message that follows the object name is optional. As a result,\nan instruction sheet like this is now perfectly valid:\n\n pick 35b0426\n pick fbd5bbcbc2e\n pick 7362160f\n\nWhile at it, also fix a bug introduced by 5a5d80f4 (revert: Introduce\n--continue to continue the operation, 2011-08-04) that failed to read\nlines that are too long to fit on the commit-id-shaped buffer we\ncurrently use; eliminate the need for the buffer altogether. In\naddition to literal SHA-1 hexes, you can now safely use expressions\nlike the following in the instruction sheet:\n\n featurebranch~4\n rr/revert-cherry-pick-continue^2~12@{12 days ago}\n\n[jc: simplify parsing]\n\nSuggested-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 37 ++++++++++++++++---------------------\n t/t3510-cherry-pick-sequence.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0c6d3d8..70055e5 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \treturn 0;\n }\n \n-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)\n {\n \tunsigned char commit_sha1[20];\n-\tchar sha1_abbrev[40];\n \tenum replay_action action;\n-\tint insn_len = 0;\n-\tchar *p, *q;\n+\tchar *end_of_object_name;\n+\tint saved, status;\n \n-\tif (!prefixcmp(start, \"pick \")) {\n+\tif (!prefixcmp(bol, \"pick \")) {\n \t\taction = CHERRY_PICK;\n-\t\tinsn_len = strlen(\"pick\");\n-\t\tp = start + insn_len + 1;\n-\t} else if (!prefixcmp(start, \"revert \")) {\n+\t\tbol += strlen(\"pick \");\n+\t} else if (!prefixcmp(bol, \"revert \")) {\n \t\taction = REVERT;\n-\t\tinsn_len = strlen(\"revert\");\n-\t\tp = start + insn_len + 1;\n+\t\tbol += strlen(\"revert \");\n \t} else\n \t\treturn NULL;\n \n-\tq = strchr(p, ' ');\n-\tif (!q)\n-\t\treturn NULL;\n-\tq++;\n-\n-\tstrlcpy(sha1_abbrev, p, q - p);\n+\tend_of_object_name = bol + strcspn(bol, \" \\n\");\n+\tsaved = *end_of_object_name;\n+\t*end_of_object_name = '\\0';\n+\tstatus = get_sha1(bol, commit_sha1);\n+\t*end_of_object_name = saved;\n \n \t/*\n \t * Verify that the action matches up with the one in\n@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n \t\treturn NULL;\n \t}\n \n-\tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n+\tif (status < 0)\n \t\treturn NULL;\n \n \treturn lookup_commit_reference(commit_sha1);\n@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \tint i;\n \n \tfor (i = 1; *p; i++) {\n-\t\tcommit = parse_insn_line(p, opts);\n+\t\tchar *eol = strchrnul(p, '\\n');\n+\t\tcommit = parse_insn_line(p, eol, opts);\n \t\tif (!commit)\n \t\t\treturn error(_(\"Could not parse line %d.\"), i);\n \t\tnext = commit_list_append(commit, next);\n-\t\tp = strchrnul(p, '\\n');\n-\t\tif (*p)\n-\t\t\tp++;\n+\t\tp = *eol ? eol + 1 : eol;\n \t}\n \tif (!*todo_list)\n \t\treturn error(_(\"No commits parsed.\"));\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 2c4c1c8..6390f2a 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -13,6 +13,9 @@ test_description='Test cherry-pick continuation features\n \n . ./test-lib.sh\n \n+# Repeat first match 10 times\n+_r10='\\1\\1\\1\\1\\1\\1\\1\\1\\1\\1'\n+\n pristine_detach () {\n \tgit cherry-pick --quit &&\n \tgit checkout -f \"$1^0\" &&\n@@ -328,4 +331,29 @@ test_expect_success 'malformed instruction sheet 2' '\n \ttest_must_fail git cherry-pick --continue\n '\n \n+test_expect_success 'malformed instruction sheet 3' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick base..anotherpick &&\n+\techo \"resolved\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tsed \"s/pick \\([0-9a-f]*\\)/pick $_r10/\" .git/sequencer/todo >new_sheet &&\n+\tcp new_sheet .git/sequencer/todo &&\n+\ttest_must_fail git cherry-pick --continue\n+'\n+\n+test_expect_success 'commit descriptions in insn sheet are optional' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tcut -d\" \" -f1,2 .git/sequencer/todo >new_sheet &&\n+\tcp new_sheet .git/sequencer/todo &&\n+\tgit cherry-pick --continue &&\n+\ttest_path_is_missing .git/sequencer &&\n+\tgit rev-list HEAD >commits &&\n+\ttest_line_count = 4 commits\n+'\n+\n test_done\n-- \n1.7.4.1\n"},{"id":"181165","messageId":"1323881677-11117-4-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 03/10] revert: tolerate extra spaces, tabs in insn sheet","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:30Z","receivedAt":"2011-12-14T16:54:30Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Tolerate extra spaces and tabs as part of the the field separator in\n'.git/sequencer/todo', for people with fat fingers.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 18 ++++++++++++------\n t/t3510-cherry-pick-sequence.sh | 11 +++++++++++\n 2 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 70055e5..be0686d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -716,18 +716,24 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \tunsigned char commit_sha1[20];\n \tenum replay_action action;\n \tchar *end_of_object_name;\n-\tint saved, status;\n+\tint saved, status, padding;\n \n-\tif (!prefixcmp(bol, \"pick \")) {\n+\tif (!prefixcmp(bol, \"pick\")) {\n \t\taction = CHERRY_PICK;\n-\t\tbol += strlen(\"pick \");\n-\t} else if (!prefixcmp(bol, \"revert \")) {\n+\t\tbol += strlen(\"pick\");\n+\t} else if (!prefixcmp(bol, \"revert\")) {\n \t\taction = REVERT;\n-\t\tbol += strlen(\"revert \");\n+\t\tbol += strlen(\"revert\");\n \t} else\n \t\treturn NULL;\n \n-\tend_of_object_name = bol + strcspn(bol, \" \\n\");\n+\t/* Eat up extra spaces/ tabs before object name */\n+\tpadding = strspn(bol, \" \\t\");\n+\tif (!padding)\n+\t\treturn NULL;\n+\tbol += padding;\n+\n+\tend_of_object_name = bol + strcspn(bol, \" \\t\\n\");\n \tsaved = *end_of_object_name;\n \t*end_of_object_name = '\\0';\n \tstatus = get_sha1(bol, commit_sha1);\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 6390f2a..781c5ac 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -342,6 +342,17 @@ test_expect_success 'malformed instruction sheet 3' '\n \ttest_must_fail git cherry-pick --continue\n '\n \n+test_expect_success 'instruction sheet, fat-fingers version' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tsed \"s/pick \\([0-9a-f]*\\)/pick \t \\1 \t/\" .git/sequencer/todo >new_sheet &&\n+\tcp new_sheet .git/sequencer/todo &&\n+\tgit cherry-pick --continue\n+'\n+\n test_expect_success 'commit descriptions in insn sheet are optional' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick base..anotherpick &&\n-- \n1.7.4.1\n"},{"id":"181166","messageId":"1323881677-11117-5-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 04/10] revert: simplify getting commit subject in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:31Z","receivedAt":"2011-12-14T16:54:31Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"format_todo() calls get_message(), but uses only the subject line of\nthe commit message. As a minor optimization, save work and\nunnecessary memory allocations by using find_commit_subject() instead.\nAlso, remove the unnecessary check on cur->item->buffer: the\nlookup_commit_reference() call in parse_insn_line() has already made\nsure of this.\n\nSuggested-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex be0686d..0a4b215 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -697,16 +697,16 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\tstruct replay_opts *opts)\n {\n \tstruct commit_list *cur = NULL;\n-\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tconst char *sha1_abbrev = NULL;\n \tconst char *action_str = opts->action == REVERT ? \"revert\" : \"pick\";\n+\tconst char *subject;\n+\tint subject_len;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n-\t\tif (get_message(cur->item, &msg))\n-\t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n-\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action_str, sha1_abbrev, msg.subject);\n-\t\tfree_message(&msg);\n+\t\tsubject_len = find_commit_subject(cur->item->buffer, &subject);\n+\t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n+\t\t\tsubject_len, subject);\n \t}\n \treturn 0;\n }\n-- \n1.7.4.1\n"},{"id":"181167","messageId":"1323881677-11117-6-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 05/10] t3510 (cherry-pick-sequencer): use exit status","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:32Z","receivedAt":"2011-12-14T16:54:32Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"All the tests asserting failure use 'test_must_fail', which simply\nchecks for a non-zero exit status, potentially hiding underlying bugs.\nSo, replace instances of 'test_must_fail' with 'test_expect_code' to\ncheck the exit status explicitly, where appropriate.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n t/t3510-cherry-pick-sequence.sh | 58 +++++++++++++++++++-------------------\n 1 files changed, 29 insertions(+), 29 deletions(-)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 781c5ac..b6e822e 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -44,7 +44,7 @@ test_expect_success setup '\n \n test_expect_success 'cherry-pick persists data on failure' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick -s base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick -s base..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '\n \n test_expect_success 'cherry-pick persists opts correctly' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n+\ttest_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -93,7 +93,7 @@ test_expect_success '--abort requires cherry-pick in progress' '\n \n test_expect_success '--quit cleans up sequencer state' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_expect_code 1 git cherry-pick base..picked &&\n \tgit cherry-pick --quit &&\n \ttest_path_is_missing .git/sequencer\n '\n@@ -107,7 +107,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '\n \t:000000 100644 OBJID OBJID A\tfoo\n \t:000000 100644 OBJID OBJID A\tunrelated\n \tEOF\n-\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_expect_code 1 git cherry-pick base..picked &&\n \tgit cherry-pick --quit &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_must_fail git update-index --refresh &&\n@@ -121,7 +121,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '\n \n test_expect_success '--abort to cancel multiple cherry-pick' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \tgit cherry-pick --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev initial HEAD &&\n@@ -131,7 +131,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' '\n \n test_expect_success '--abort to cancel single cherry-pick' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick picked &&\n+\ttest_expect_code 1 git cherry-pick picked &&\n \tgit cherry-pick --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev initial HEAD &&\n@@ -141,7 +141,7 @@ test_expect_success '--abort to cancel single cherry-pick' '\n \n test_expect_success 'cherry-pick --abort to cancel multiple revert' '\n \tpristine_detach anotherpick &&\n-\ttest_must_fail git revert base..picked &&\n+\ttest_expect_code 1 git revert base..picked &&\n \tgit cherry-pick --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev anotherpick HEAD &&\n@@ -151,7 +151,7 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' '\n \n test_expect_success 'revert --abort works, too' '\n \tpristine_detach anotherpick &&\n-\ttest_must_fail git revert base..picked &&\n+\ttest_expect_code 1 git revert base..picked &&\n \tgit revert --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev anotherpick HEAD\n@@ -159,7 +159,7 @@ test_expect_success 'revert --abort works, too' '\n \n test_expect_success '--abort to cancel single revert' '\n \tpristine_detach anotherpick &&\n-\ttest_must_fail git revert picked &&\n+\ttest_expect_code 1 git revert picked &&\n \tgit revert --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev anotherpick HEAD &&\n@@ -170,7 +170,7 @@ test_expect_success '--abort to cancel single revert' '\n test_expect_success '--abort keeps unrelated change, easy case' '\n \tpristine_detach unrelatedpick &&\n \techo changed >expect &&\n-\ttest_must_fail git cherry-pick picked..yetanotherpick &&\n+\ttest_expect_code 1 git cherry-pick picked..yetanotherpick &&\n \techo changed >unrelated &&\n \tgit cherry-pick --abort &&\n \ttest_cmp expect unrelated\n@@ -179,7 +179,7 @@ test_expect_success '--abort keeps unrelated change, easy case' '\n test_expect_success '--abort refuses to clobber unrelated change, harder case' '\n \tpristine_detach initial &&\n \techo changed >expect &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo changed >unrelated &&\n \ttest_must_fail git cherry-pick --abort &&\n \ttest_cmp expect unrelated &&\n@@ -194,7 +194,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '\n \n test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_expect_code 1 git cherry-pick base..picked &&\n \ttest_path_is_missing .git/sequencer &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n@@ -218,7 +218,7 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le\n \n test_expect_failure '--abort after last commit in sequence' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_expect_code 1 git cherry-pick base..picked &&\n \tgit cherry-pick --abort &&\n \ttest_path_is_missing .git/sequencer &&\n \ttest_cmp_rev initial HEAD &&\n@@ -228,27 +228,27 @@ test_expect_failure '--abort after last commit in sequence' '\n \n test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \ttest-chmtime -v +0 .git/sequencer >expect &&\n-\ttest_must_fail git cherry-pick unrelatedpick &&\n+\ttest_expect_code 128 git cherry-pick unrelatedpick &&\n \ttest-chmtime -v +0 .git/sequencer >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '--continue complains when no cherry-pick is in progress' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success '--continue complains when there are unresolved conflicts' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success '--continue continues after conflicts are resolved' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n@@ -275,7 +275,7 @@ test_expect_success '--continue continues after conflicts are resolved' '\n \n test_expect_success '--continue respects opts' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick -x base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick -x base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n@@ -293,7 +293,7 @@ test_expect_success '--continue respects opts' '\n \n test_expect_success '--signoff is not automatically propagated to resolved conflict' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick --signoff base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick --signoff base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n@@ -311,40 +311,40 @@ test_expect_success '--signoff is not automatically propagated to resolved confl\n \n test_expect_success 'malformed instruction sheet 1' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n \tsed \"s/pick /pick/\" .git/sequencer/todo >new_sheet &&\n \tcp new_sheet .git/sequencer/todo &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success 'malformed instruction sheet 2' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n \tsed \"s/pick/revert/\" .git/sequencer/todo >new_sheet &&\n \tcp new_sheet .git/sequencer/todo &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success 'malformed instruction sheet 3' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n \tsed \"s/pick \\([0-9a-f]*\\)/pick $_r10/\" .git/sequencer/todo >new_sheet &&\n \tcp new_sheet .git/sequencer/todo &&\n-\ttest_must_fail git cherry-pick --continue\n+\ttest_expect_code 128 git cherry-pick --continue\n '\n \n test_expect_success 'instruction sheet, fat-fingers version' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n@@ -355,7 +355,7 @@ test_expect_success 'instruction sheet, fat-fingers version' '\n \n test_expect_success 'commit descriptions in insn sheet are optional' '\n \tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..anotherpick &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"c\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n-- \n1.7.4.1\n"},{"id":"181168","messageId":"1323881677-11117-7-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 06/10] t3502, t3510: clarify cherry-pick -m failure","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:33Z","receivedAt":"2011-12-14T16:54:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The \"cherry-pick persists opts correctly\" test in t3510\n(cherry-pick-sequence) can cause some confusion, because the command\nactually has two points of failure:\n\n1. \"-m 1\" is specified on the command-line despite the base commit\n \"initial\" not being a merge-commit.\n2. The revision range indicates that there will be a conflict that\n needs to be resolved.\n\nAlthough the former error is trapped, and cherry-pick die()s with the\nexit status 128, the reader may be distracted by the latter. Fix this\nby changing the revision range to something that wouldn't cause a\nconflict. Additionally, explicitly check the exit code in\n\"cherry-pick a non-merge with -m should fail\" in t3502\n(cherry-pick-merge) to reassure the reader that this failure has\nnothing to do with the sequencer itself.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n t/t3502-cherry-pick-merge.sh | 2 +-\n t/t3510-cherry-pick-sequence.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh\nindex 0ab52da..e37547f 100755\n--- a/t/t3502-cherry-pick-merge.sh\n+++ b/t/t3502-cherry-pick-merge.sh\n@@ -35,7 +35,7 @@ test_expect_success 'cherry-pick a non-merge with -m should fail' '\n \n \tgit reset --hard &&\n \tgit checkout a^0 &&\n-\ttest_must_fail git cherry-pick -m 1 b &&\n+\ttest_expect_code 128 git cherry-pick -m 1 b &&\n \tgit diff --exit-code a --\n \n '\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex b6e822e..163ce7d 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '\n \n test_expect_success 'cherry-pick persists opts correctly' '\n \tpristine_detach initial &&\n-\ttest_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&\n+\ttest_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n-- \n1.7.4.1\n"},{"id":"181170","messageId":"1323881677-11117-8-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 07/10] t3510 (cherry-pick-sequencer): remove malformed sheet 2","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:34Z","receivedAt":"2011-12-14T16:54:34Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The next patch allows mixing \"pick\" and \"revert\" instruction in the\nsame instruction sheet. By removing the \"malformed instruction sheet\n2\" test in advance, it'll be easier to see the changes made by the\nfuture patches that touch this file.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n t/t3510-cherry-pick-sequence.sh | 15 ++-------------\n 1 files changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 163ce7d..1bfbe41 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -309,7 +309,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl\n \tgrep \"Signed-off-by:\" anotherpick_msg\n '\n \n-test_expect_success 'malformed instruction sheet 1' '\n+test_expect_success 'malformed instruction sheet, action' '\n \tpristine_detach initial &&\n \ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n@@ -320,18 +320,7 @@ test_expect_success 'malformed instruction sheet 1' '\n \ttest_expect_code 128 git cherry-pick --continue\n '\n \n-test_expect_success 'malformed instruction sheet 2' '\n-\tpristine_detach initial &&\n-\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n-\techo \"resolved\" >foo &&\n-\tgit add foo &&\n-\tgit commit &&\n-\tsed \"s/pick/revert/\" .git/sequencer/todo >new_sheet &&\n-\tcp new_sheet .git/sequencer/todo &&\n-\ttest_expect_code 128 git cherry-pick --continue\n-'\n-\n-test_expect_success 'malformed instruction sheet 3' '\n+test_expect_success 'malformed instruction sheet, object name' '\n \tpristine_detach initial &&\n \ttest_expect_code 1 git cherry-pick base..anotherpick &&\n \techo \"resolved\" >foo &&\n-- \n1.7.4.1\n"},{"id":"181171","messageId":"1323881677-11117-9-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 08/10] revert: move replay_action, replay_subcommand to header","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:35Z","receivedAt":"2011-12-14T16:54:35Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"In our vision to build a generalized sequencer, the revert builtin\nwill be left only with argument parsing work before calling into the\nsequencer. Since the enums replay_action and replay_subcommand have\nnothing to do with this argument parsing, move them to sequencer.h.\n\nHowever, REVERT and CHERRY_PICK are unsuitable variable names for\npublicly exposing, as their purpose is unclear in the global context:\nrename them to REPLAY_REVERT and REPLAY_PICK respectively.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 42 +++++++++++++++++-------------------------\n sequencer.h | 12 ++++++++++++\n 2 files changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0a4b215..ed062ea 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -39,14 +39,6 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-enum replay_action { REVERT, CHERRY_PICK };\n-enum replay_subcommand {\n-\tREPLAY_NONE,\n-\tREPLAY_REMOVE_STATE,\n-\tREPLAY_CONTINUE,\n-\tREPLAY_ROLLBACK\n-};\n-\n struct replay_opts {\n \tenum replay_action action;\n \tenum replay_subcommand subcommand;\n@@ -73,14 +65,14 @@ struct replay_opts {\n \n static const char *action_name(const struct replay_opts *opts)\n {\n-\treturn opts->action == REVERT ? \"revert\" : \"cherry-pick\";\n+\treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n }\n \n static char *get_encoding(const char *message);\n \n static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n {\n-\treturn opts->action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n }\n \n static int option_parse_x(const struct option *opt,\n@@ -159,7 +151,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t};\n \n-\tif (opts->action == CHERRY_PICK) {\n+\tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n@@ -363,7 +355,7 @@ static int error_dirty_index(struct replay_opts *opts)\n \t\treturn error_resolve_conflict(action_name(opts));\n \n \t/* Different translation strings for cherry-pick and revert */\n-\tif (opts->action == CHERRY_PICK)\n+\tif (opts->action == REPLAY_PICK)\n \t\terror(_(\"Your local changes would be overwritten by cherry-pick.\"));\n \telse\n \t\terror(_(\"Your local changes would be overwritten by revert.\"));\n@@ -542,7 +534,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (opts->action == REVERT) {\n+\tif (opts->action == REPLAY_REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -583,7 +575,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \t}\n \n-\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REVERT) {\n+\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REPLAY_REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n \t\t\t\t\t head, &msgbuf, opts);\n \t\twrite_message(&msgbuf, defmsg);\n@@ -607,13 +599,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t * However, if the merge did not even start, then we don't want to\n \t * write it at all.\n \t */\n-\tif (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))\n+\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))\n \t\twrite_cherry_pick_head(commit, \"CHERRY_PICK_HEAD\");\n-\tif (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))\n+\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))\n \t\twrite_cherry_pick_head(commit, \"REVERT_HEAD\");\n \n \tif (res) {\n-\t\terror(opts->action == REVERT\n+\t\terror(opts->action == REPLAY_REVERT\n \t\t ? _(\"could not revert %s... %s\")\n \t\t : _(\"could not apply %s... %s\"),\n \t\t find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n@@ -637,7 +629,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)\n \n \tinit_revisions(revs, NULL);\n \trevs->no_walk = 1;\n-\tif (opts->action != REVERT)\n+\tif (opts->action != REPLAY_REVERT)\n \t\trevs->reverse = 1;\n \n \targc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);\n@@ -698,7 +690,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n {\n \tstruct commit_list *cur = NULL;\n \tconst char *sha1_abbrev = NULL;\n-\tconst char *action_str = opts->action == REVERT ? \"revert\" : \"pick\";\n+\tconst char *action_str = opts->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n \tconst char *subject;\n \tint subject_len;\n \n@@ -719,10 +711,10 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \tint saved, status, padding;\n \n \tif (!prefixcmp(bol, \"pick\")) {\n-\t\taction = CHERRY_PICK;\n+\t\taction = REPLAY_PICK;\n \t\tbol += strlen(\"pick\");\n \t} else if (!prefixcmp(bol, \"revert\")) {\n-\t\taction = REVERT;\n+\t\taction = REPLAY_REVERT;\n \t\tbol += strlen(\"revert\");\n \t} else\n \t\treturn NULL;\n@@ -745,7 +737,7 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \t */\n \tif (action != opts->action) {\n \t\tconst char *action_str;\n-\t\taction_str = action == REVERT ? \"revert\" : \"cherry-pick\";\n+\t\taction_str = action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n \t\terror(_(\"Cannot %s during a %s\"), action_str, action_name(opts));\n \t\treturn NULL;\n \t}\n@@ -1080,7 +1072,7 @@ static int pick_revisions(struct replay_opts *opts)\n \tif (create_seq_dir() < 0)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1)) {\n-\t\tif (opts->action == REVERT)\n+\t\tif (opts->action == REPLAY_REVERT)\n \t\t\treturn error(_(\"Can't revert as initial commit\"));\n \t\treturn error(_(\"Can't cherry-pick into empty head\"));\n \t}\n@@ -1097,7 +1089,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tmemset(&opts, 0, sizeof(opts));\n \tif (isatty(0))\n \t\topts.edit = 1;\n-\topts.action = REVERT;\n+\topts.action = REPLAY_REVERT;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = pick_revisions(&opts);\n@@ -1112,7 +1104,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tint res;\n \n \tmemset(&opts, 0, sizeof(opts));\n-\topts.action = CHERRY_PICK;\n+\topts.action = REPLAY_PICK;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = pick_revisions(&opts);\ndiff --git a/sequencer.h b/sequencer.h\nindex f435fdb..9b1b94d 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -7,6 +7,18 @@\n #define SEQ_TODO_FILE\t\"sequencer/todo\"\n #define SEQ_OPTS_FILE\t\"sequencer/opts\"\n \n+enum replay_action {\n+\tREPLAY_REVERT,\n+\tREPLAY_PICK\n+};\n+\n+enum replay_subcommand {\n+\tREPLAY_NONE,\n+\tREPLAY_REMOVE_STATE,\n+\tREPLAY_CONTINUE,\n+\tREPLAY_ROLLBACK\n+};\n+\n /*\n * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring\n * any errors. Intended to be used by 'git reset'.\n-- \n1.7.4.1\n"},{"id":"181172","messageId":"1323881677-11117-10-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 09/10] revert: allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:36Z","receivedAt":"2011-12-14T16:54:36Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Parse the instruction sheet in '.git/sequencer/todo' as a list of\n(action, operand) pairs, instead of assuming that all instructions use\nthe same action. Now you can do:\n\n pick fdc0b12 picked\n revert 965fed4 anotherpick\n\nFor cherry-pick and revert, this means that a 'git cherry-pick\n--continue' can continue an ongoing revert operation and viceversa.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 107 ++++++++++++++++++---------------------\n sequencer.h | 6 ++\n t/t3510-cherry-pick-sequence.sh | 58 +++++++++++++++++++++\n 3 files changed, 114 insertions(+), 57 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex ed062ea..50af439 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -459,7 +459,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n \treturn run_command_v_opt(args, RUN_GIT_CMD);\n }\n \n-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n+static int do_pick_commit(struct commit *commit, enum replay_action action,\n+\t\t\tstruct replay_opts *opts)\n {\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n@@ -534,7 +535,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (opts->action == REPLAY_REVERT) {\n+\tif (action == REPLAY_REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -575,7 +576,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \t}\n \n-\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REPLAY_REVERT) {\n+\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || action == REPLAY_REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n \t\t\t\t\t head, &msgbuf, opts);\n \t\twrite_message(&msgbuf, defmsg);\n@@ -605,7 +606,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\twrite_cherry_pick_head(commit, \"REVERT_HEAD\");\n \n \tif (res) {\n-\t\terror(opts->action == REPLAY_REVERT\n+\t\terror(action == REPLAY_REVERT\n \t\t ? _(\"could not revert %s... %s\")\n \t\t : _(\"could not apply %s... %s\"),\n \t\t find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n@@ -675,54 +676,54 @@ static void read_and_refresh_cache(struct replay_opts *opts)\n * assert(commit_list_count(list) == 2);\n * return list;\n */\n-static struct commit_list **commit_list_append(struct commit *commit,\n-\t\t\t\t\t struct commit_list **next)\n+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,\n+\t\t\t\t\t\tstruct commit *operand,\n+\t\t\t\t\t\tstruct replay_insn_list **next)\n {\n-\tstruct commit_list *new = xmalloc(sizeof(struct commit_list));\n-\tnew->item = commit;\n+\tstruct replay_insn_list *new = xmalloc(sizeof(*new));\n+\tnew->action = action;\n+\tnew->operand = operand;\n \t*next = new;\n \tnew->next = NULL;\n \treturn &new->next;\n }\n \n-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n-\t\tstruct replay_opts *opts)\n+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n {\n-\tstruct commit_list *cur = NULL;\n-\tconst char *sha1_abbrev = NULL;\n-\tconst char *action_str = opts->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n-\tconst char *subject;\n-\tint subject_len;\n+\tstruct replay_insn_list *cur;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n-\t\tsubject_len = find_commit_subject(cur->item->buffer, &subject);\n+\t\tconst char *sha1_abbrev, *action_str, *subject;\n+\t\tint subject_len;\n+\n+\t\taction_str = cur->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n+\t\tsha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);\n+\t\tsubject_len = find_commit_subject(cur->operand->buffer, &subject);\n \t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n \t\t\tsubject_len, subject);\n \t}\n \treturn 0;\n }\n \n-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)\n+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n {\n \tunsigned char commit_sha1[20];\n-\tenum replay_action action;\n \tchar *end_of_object_name;\n \tint saved, status, padding;\n \n \tif (!prefixcmp(bol, \"pick\")) {\n-\t\taction = REPLAY_PICK;\n+\t\titem->action = REPLAY_PICK;\n \t\tbol += strlen(\"pick\");\n \t} else if (!prefixcmp(bol, \"revert\")) {\n-\t\taction = REPLAY_REVERT;\n+\t\titem->action = REPLAY_REVERT;\n \t\tbol += strlen(\"revert\");\n \t} else\n-\t\treturn NULL;\n+\t\treturn -1;\n \n \t/* Eat up extra spaces/ tabs before object name */\n \tpadding = strspn(bol, \" \\t\");\n \tif (!padding)\n-\t\treturn NULL;\n+\t\treturn -1;\n \tbol += padding;\n \n \tend_of_object_name = bol + strcspn(bol, \" \\t\\n\");\n@@ -731,37 +732,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \tstatus = get_sha1(bol, commit_sha1);\n \t*end_of_object_name = saved;\n \n-\t/*\n-\t * Verify that the action matches up with the one in\n-\t * opts; we don't support arbitrary instructions\n-\t */\n-\tif (action != opts->action) {\n-\t\tconst char *action_str;\n-\t\taction_str = action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n-\t\terror(_(\"Cannot %s during a %s\"), action_str, action_name(opts));\n-\t\treturn NULL;\n-\t}\n-\n \tif (status < 0)\n-\t\treturn NULL;\n+\t\treturn -1;\n \n-\treturn lookup_commit_reference(commit_sha1);\n+\titem->operand = lookup_commit_reference(commit_sha1);\n+\tif (!item->operand)\n+\t\treturn -1;\n+\n+\titem->next = NULL;\n+\treturn 0;\n }\n \n-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n-\t\t\tstruct replay_opts *opts)\n+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n {\n-\tstruct commit_list **next = todo_list;\n-\tstruct commit *commit;\n+\tstruct replay_insn_list **next = todo_list;\n+\tstruct replay_insn_list item = { 0, NULL, NULL };\n \tchar *p = buf;\n \tint i;\n \n \tfor (i = 1; *p; i++) {\n \t\tchar *eol = strchrnul(p, '\\n');\n-\t\tcommit = parse_insn_line(p, eol, opts);\n-\t\tif (!commit)\n+\t\tif (parse_insn_line(p, eol, &item) < 0)\n \t\t\treturn error(_(\"Could not parse line %d.\"), i);\n-\t\tnext = commit_list_append(commit, next);\n+\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n \t\tp = *eol ? eol + 1 : eol;\n \t}\n \tif (!*todo_list)\n@@ -769,8 +762,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \treturn 0;\n }\n \n-static void read_populate_todo(struct commit_list **todo_list,\n-\t\t\tstruct replay_opts *opts)\n+static void read_populate_todo(struct replay_insn_list **todo_list)\n {\n \tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -786,7 +778,7 @@ static void read_populate_todo(struct commit_list **todo_list,\n \t}\n \tclose(fd);\n \n-\tres = parse_insn_buffer(buf.buf, todo_list, opts);\n+\tres = parse_insn_buffer(buf.buf, todo_list);\n \tstrbuf_release(&buf);\n \tif (res)\n \t\tdie(_(\"Unusable instruction sheet: %s\"), todo_file);\n@@ -835,18 +827,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)\n \t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n }\n \n-static void walk_revs_populate_todo(struct commit_list **todo_list,\n+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n-\tstruct commit_list **next;\n+\tstruct replay_insn_list **next;\n \n \tprepare_revs(&revs, opts);\n \n \tnext = todo_list;\n \twhile ((commit = get_revision(&revs)))\n-\t\tnext = commit_list_append(commit, next);\n+\t\tnext = replay_insn_list_append(opts->action, commit, next);\n }\n \n static int create_seq_dir(void)\n@@ -944,7 +936,7 @@ fail:\n \treturn -1;\n }\n \n-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+static void save_todo(struct replay_insn_list *todo_list)\n {\n \tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstatic struct lock_file todo_lock;\n@@ -952,7 +944,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \tint fd;\n \n \tfd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);\n-\tif (format_todo(&buf, todo_list, opts) < 0)\n+\tif (format_todo(&buf, todo_list) < 0)\n \t\tdie(_(\"Could not format %s.\"), todo_file);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_release(&buf);\n@@ -996,9 +988,10 @@ static void save_opts(struct replay_opts *opts)\n \t}\n }\n \n-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n+static int pick_commits(struct replay_insn_list *todo_list,\n+\t\t\tstruct replay_opts *opts)\n {\n-\tstruct commit_list *cur;\n+\tstruct replay_insn_list *cur;\n \tint res;\n \n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n@@ -1008,8 +1001,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tread_and_refresh_cache(opts);\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsave_todo(cur, opts);\n-\t\tres = do_pick_commit(cur->item, opts);\n+\t\tsave_todo(cur);\n+\t\tres = do_pick_commit(cur->operand, cur->action, opts);\n \t\tif (res) {\n \t\t\tif (!cur->next)\n \t\t\t\t/*\n@@ -1034,7 +1027,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \n static int pick_revisions(struct replay_opts *opts)\n {\n-\tstruct commit_list *todo_list = NULL;\n+\tstruct replay_insn_list *todo_list = NULL;\n \tunsigned char sha1[20];\n \n \tread_and_refresh_cache(opts);\n@@ -1054,7 +1047,7 @@ static int pick_revisions(struct replay_opts *opts)\n \t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n \t\t\treturn error(_(\"No %s in progress\"), action_name(opts));\n \t\tread_populate_opts(&opts);\n-\t\tread_populate_todo(&todo_list, opts);\n+\t\tread_populate_todo(&todo_list);\n \n \t\t/* Verify that the conflict has been resolved */\n \t\tif (!index_differs_from(\"HEAD\", 0))\ndiff --git a/sequencer.h b/sequencer.h\nindex 9b1b94d..648f7bf 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -19,6 +19,12 @@ enum replay_subcommand {\n \tREPLAY_ROLLBACK\n };\n \n+struct replay_insn_list {\n+\tenum replay_action action;\n+\tstruct commit *operand;\n+\tstruct replay_insn_list *next;\n+};\n+\n /*\n * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring\n * any errors. Intended to be used by 'git reset'.\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 1bfbe41..9b19917 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '\n \ttest_line_count = 4 commits\n '\n \n+test_expect_success 'revert --continue continues after cherry-pick' '\n+\tpristine_detach initial &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tgit revert --continue &&\n+\ttest_path_is_missing .git/sequencer &&\n+\t{\n+\t\tgit rev-list HEAD |\n+\t\tgit diff-tree --root --stdin |\n+\t\tsed \"s/$_x40/OBJID/g\"\n+\t} >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tunrelated\n+\tOBJID\n+\t:000000 100644 OBJID OBJID A\tfoo\n+\t:000000 100644 OBJID OBJID A\tunrelated\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'mixed pick and revert instructions' '\n+\tpristine_detach initial &&\n+\ttest_expect_code 1 git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\toldsha=`git rev-parse --short HEAD~1` &&\n+\techo \"revert $oldsha unrelatedpick\" >>.git/sequencer/todo &&\n+\tgit cherry-pick --continue &&\n+\ttest_path_is_missing .git/sequencer &&\n+\t{\n+\t\tgit rev-list HEAD |\n+\t\tgit diff-tree --root --stdin |\n+\t\tsed \"s/$_x40/OBJID/g\"\n+\t} >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tunrelated\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tunrelated\n+\tOBJID\n+\t:000000 100644 OBJID OBJID A\tfoo\n+\t:000000 100644 OBJID OBJID A\tunrelated\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.4.1\n"},{"id":"181169","messageId":"1323881677-11117-11-git-send-email-artagnon@gmail.com","threadId":"29119","inReplyTo":"1323881677-11117-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 10/10] revert: report fine-grained error messages from insn parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-14T16:54:37Z","receivedAt":"2011-12-14T16:54:37Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Three kinds of errors can arise from parsing '.git/sequencer/todo':\n1. Unrecognized action\n2. Missing space after valid action prefix\n3. Malformed object name\n4. Object name does not refer to a valid commit\n\nSince we would like to make the instruction sheet user-editable in the\nfuture (much like the 'rebase -i' sheet), report more fine-grained\nparse errors prefixed with the filename and line number.\n\nHelped-by: Junio C Hamano \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 37 ++++++++++++++++++++++++++-----------\n 1 files changed, 26 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 50af439..d106c3c 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -705,11 +705,14 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n \treturn 0;\n }\n \n-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n+static int parse_insn_line(char *bol, char *eol,\n+\t\t\tstruct replay_insn_list *item, int lineno)\n {\n+\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tunsigned char commit_sha1[20];\n \tchar *end_of_object_name;\n \tint saved, status, padding;\n+\tsize_t error_len;\n \n \tif (!prefixcmp(bol, \"pick\")) {\n \t\titem->action = REPLAY_PICK;\n@@ -717,13 +720,19 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n \t} else if (!prefixcmp(bol, \"revert\")) {\n \t\titem->action = REPLAY_REVERT;\n \t\tbol += strlen(\"revert\");\n-\t} else\n-\t\treturn -1;\n+\t} else {\n+\t\terror_len = eol - bol > 255 ? 255 : eol - bol;\n+\t\treturn error(_(\"%s:%d: Unrecognized action: %.*s\"),\n+\t\t\ttodo_file, lineno, (int)error_len, bol);\n+\t}\n \n \t/* Eat up extra spaces/ tabs before object name */\n \tpadding = strspn(bol, \" \\t\");\n-\tif (!padding)\n-\t\treturn -1;\n+\tif (!padding) {\n+\t\terror_len = eol - bol > 255 ? 255 : eol - bol;\n+\t\treturn error(_(\"%s:%d: Missing space after action: %.*s\"),\n+\t\t\ttodo_file, lineno, (int)error_len, bol);\n+\t}\n \tbol += padding;\n \n \tend_of_object_name = bol + strcspn(bol, \" \\t\\n\");\n@@ -732,12 +741,18 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n \tstatus = get_sha1(bol, commit_sha1);\n \t*end_of_object_name = saved;\n \n-\tif (status < 0)\n-\t\treturn -1;\n+\tif (status < 0) {\n+\t\terror_len = eol - bol > 255 ? 255 : eol - bol;\n+\t\treturn error(_(\"%s:%d: Malformed object name: %.*s\"),\n+\t\t\ttodo_file, lineno, (int)error_len, bol);\n+\t}\n \n \titem->operand = lookup_commit_reference(commit_sha1);\n-\tif (!item->operand)\n-\t\treturn -1;\n+\tif (!item->operand) {\n+\t\terror_len = eol - bol > 255 ? 255 : eol - bol;\n+\t\treturn error(_(\"%s:%d: Not a valid commit: %.*s\"),\n+\t\t\ttodo_file, lineno, (int)error_len, bol);\n+\t}\n \n \titem->next = NULL;\n \treturn 0;\n@@ -752,8 +767,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n \n \tfor (i = 1; *p; i++) {\n \t\tchar *eol = strchrnul(p, '\\n');\n-\t\tif (parse_insn_line(p, eol, &item) < 0)\n-\t\t\treturn error(_(\"Could not parse line %d.\"), i);\n+\t\tif (parse_insn_line(p, eol, &item, i) < 0)\n+\t\t\treturn -1;\n \t\tnext = replay_insn_list_append(item.action, item.operand, next);\n \t\tp = *eol ? eol + 1 : eol;\n \t}\n-- \n1.7.4.1\n"},{"id":"181254","messageId":"7vsjklmsvt.fsf@alter.siamese.dyndns.org","threadId":"29119","inReplyTo":"1323881677-11117-11-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 10/10] revert: report fine-grained error messages from insn parser","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T21:23:18Z","receivedAt":"2011-12-15T21:23:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Three kinds of errors can arise from parsing '.git/sequencer/todo':\n> 1. Unrecognized action\n> 2. Missing space after valid action prefix\n\nI think these two are the same and shouldn't result in different error\nmessages, i.e. the first one in this sequence is still an \"Unrecognized\naction\" error and does not have anything to do with \"pick\" action.\n\n\tpickle rr/cherry-pick~4\n pick rr/cherry-pick~3\n"},{"id":"181274","messageId":"CALkWK0k6Vf51nk3jLs3z1LvzRnCb46obVDZh8h5byzTzCBctJw@mail.gmail.com","threadId":"29119","inReplyTo":"7vsjklmsvt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 10/10] revert: report fine-grained error messages from insn parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-15T23:18:35Z","receivedAt":"2011-12-15T23:18:35Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano wrote:\n> Ramkumar Ramachandra writes:\n>\n>> Three kinds of errors can arise from parsing '.git/sequencer/todo':\n>> 1. Unrecognized action\n>> 2. Missing space after valid action prefix\n>\n> I think these two are the same and shouldn't result in different error\n> messages, i.e. the first one in this sequence is still an \"Unrecognized\n> action\" error and does not have anything to do with \"pick\" action.\n>\n>        pickle rr/cherry-pick~4\n>        pick rr/cherry-pick~3\n\nGood point. I'll change the parser to check for \"pick \" || \"pick\\t\"\nand \"revert \" || \"revert\\t\" explicitly in the re-roll.\n\nThanks.\n\n-- Ram\n"}]}