{"thread":{"id":"28633","subject":"[PATCH 0/6] Sequencer fixups mini-series","startedAt":"2011-10-08T17:36:41Z","lastAt":"2011-10-17T10:10:34Z","messageCount":16,"participants":["Ramkumar Ramachandra","Tay Ray Chuan","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"177232","messageId":"1318095407-26429-1-git-send-email-artagnon@gmail.com","threadId":"28633","inReplyTo":null,"subject":"[PATCH 0/6] Sequencer fixups mini-series","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-08T17:36:41Z","receivedAt":"2011-10-08T17:36:41Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nNow that the original sequencer series has hit 'master' (cd4093b6), we\ncan now build on it! Unfortunately, as outlined in $gmane/179613,\nthere are several UI design difficulties that we need to surmount. As\na prelude, I've decided to prepare this mini-series for fixing up a\nfew minor issues before attacking the problem; please see\n$gmane/179304 for relevant discussions.\n\nThe differences are:\n1. I've dropped the last two parts in the previous iteration.\n2. Part 2 is new. Thanks to Jonathan for the suggestion.\n3. Minor fixups and commit message improvements in response to\nreviews.\n\np.s- I'm travelling this week, and won't be able to respond to\nreviews until the 16th.\n\nThanks.\n\n-- Ram\n\nJonathan Nieder (1):\n revert: Simplify passing command-line arguments around\n\nRamkumar Ramachandra (5):\n revert: Free memory after get_message call\n revert: Simplify getting commit subject\n revert: Fix buffer overflow in insn sheet parser\n revert: Make commit descriptions in insn sheet optional\n revert: Allow mixed pick and revert instructions\n\n builtin/revert.c | 209 ++++++++++++++++++++-------------------\n sequencer.h | 8 ++\n t/t3510-cherry-pick-sequence.sh | 86 ++++++++++++++++\n 3 files changed, 200 insertions(+), 103 deletions(-)\n\n-- \n1.7.4.1\n"},{"id":"177233","messageId":"1318095407-26429-2-git-send-email-artagnon@gmail.com","threadId":"28633","inReplyTo":"1318095407-26429-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/6] revert: Free memory after get_message call","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-08T17:36:42Z","receivedAt":"2011-10-08T17:36:42Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The format_todo function leaks memory because it forgets to call\nfree_message after get_message. It is a potentially big leak, because\nfresh memory is allocated to store the commit message message for each\ncommit. Fix this.\n\nReported-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \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 ba27cf1..a2c304d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -682,6 +682,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":"177235","messageId":"1318095407-26429-3-git-send-email-artagnon@gmail.com","threadId":"28633","inReplyTo":"1318095407-26429-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/6] revert: Simplify getting commit subject","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-08T17:36:43Z","receivedAt":"2011-10-08T17:36:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The heavy parsing and memory allocations performed by get_message is\nunnecessary when only the commit subject is desired. Use\nfind_commit_subject instead.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \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 a2c304d..b3c5e0e 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -673,16 +673,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":"177236","messageId":"1318095407-26429-4-git-send-email-artagnon@gmail.com","threadId":"28633","inReplyTo":"1318095407-26429-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/6] revert: Fix buffer overflow in insn sheet parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-08T17:36:44Z","receivedAt":"2011-10-08T17:36:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Check that the commit name argument to a \"pick\" or \"revert\" action in\n'.git/sequencer/todo' is not too long, to avoid overflowing an\non-stack buffer. This fixes a regression introduced by 5a5d80f4\n(revert: Introduce --continue to continue the operation, 2011-08-04).\n\nReported-by: Jonathan Nieder \nAcked-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 2 +-\n t/t3510-cherry-pick-sequence.sh | 14 ++++++++++++++\n 2 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex b3c5e0e..6451089 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -707,7 +707,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n \t\treturn NULL;\n \n \tq = strchr(p, ' ');\n-\tif (!q)\n+\tif (!q || q - p + 1 > sizeof(sha1_abbrev))\n \t\treturn NULL;\n \tq++;\n \ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 3bca2b3..2113308 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -12,6 +12,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 --reset &&\n \tgit checkout -f \"$1^0\" &&\n@@ -211,4 +214,15 @@ 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_done\n-- \n1.7.4.1\n"},{"id":"177234","messageId":"1318095407-26429-5-git-send-email-artagnon@gmail.com","threadId":"28633","inReplyTo":"1318095407-26429-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/6] revert: Make commit descriptions in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-08T17:36:45Z","receivedAt":"2011-10-08T17:36:45Z","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 a description of\nthe commit after the object name is optional. As a result, an\ninstruction sheet like this is now perfectly valid:\n\n pick 35b0426\n pick fbd5bbcbc2e\n pick 7362160f\n\nSuggested-by: Jonathan Nieder \nAcked-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 19 ++++++++-----------\n t/t3510-cherry-pick-sequence.sh | 14 ++++++++++++++\n 2 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 6451089..aa6c34e 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -692,26 +692,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\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+\tconst char *p, *q;\n \n+\tp = start;\n \tif (!prefixcmp(start, \"pick \")) {\n \t\taction = CHERRY_PICK;\n-\t\tinsn_len = strlen(\"pick\");\n-\t\tp = start + insn_len + 1;\n+\t\tp += strlen(\"pick \");\n \t} else if (!prefixcmp(start, \"revert \")) {\n \t\taction = REVERT;\n-\t\tinsn_len = strlen(\"revert\");\n-\t\tp = start + insn_len + 1;\n+\t\tp += strlen(\"revert \");\n \t} else\n \t\treturn NULL;\n \n-\tq = strchr(p, ' ');\n-\tif (!q || q - p + 1 > sizeof(sha1_abbrev))\n+\tq = p + strcspn(p, \" \\n\");\n+\tif (q - p + 1 > sizeof(sha1_abbrev))\n \t\treturn NULL;\n-\tq++;\n-\n-\tstrlcpy(sha1_abbrev, p, q - p);\n+\tmemcpy(sha1_abbrev, p, q - p);\n+\tsha1_abbrev[q - p] = '\\0';\n \n \t/*\n \t * Verify that the action matches up with the one in\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 2113308..39b55c1 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -225,4 +225,18 @@ test_expect_success 'malformed instruction sheet 3' '\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":"177237","messageId":"1318095407-26429-6-git-send-email-artagnon@gmail.com","threadId":"28633","inReplyTo":"1318095407-26429-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/6] revert: Allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-08T17:36:46Z","receivedAt":"2011-10-08T17:36:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Parse the instruction list in '.git/sequencer/todo' as a list of\n(action, operand) pairs, instead of assuming all instructions use the\nsame action. So 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.\nThis patch lays the foundation for extending the parser to support\nmore actions so 'git rebase -i' can reuse this machinery in the\nfuture. While at it, also improve the error messages reported by the\ninsn sheet parser.\n\nHelped-by: Jonathan Nieder \nAcked-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 138 +++++++++++++++++++-------------------\n sequencer.h | 8 ++\n t/t3510-cherry-pick-sequence.sh | 58 ++++++++++++++++\n 3 files changed, 135 insertions(+), 69 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex aa6c34e..a9dd210 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-enum replay_action { REVERT, CHERRY_PICK };\n enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };\n \n struct replay_opts {\n@@ -68,14 +67,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@@ -154,7 +153,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@@ -348,7 +347,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@@ -452,7 +451,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@@ -527,7 +527,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@@ -570,7 +570,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t\twrite_cherry_pick_head(commit);\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@@ -589,7 +589,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t}\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@@ -613,7 +613,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@@ -659,87 +659,87 @@ 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 *start, struct replay_opts *opts)\n+static int parse_insn_line(char *start, struct replay_insn_list *item)\n {\n \tunsigned char commit_sha1[20];\n \tchar sha1_abbrev[40];\n-\tenum replay_action action;\n \tconst char *p, *q;\n \n \tp = start;\n \tif (!prefixcmp(start, \"pick \")) {\n-\t\taction = CHERRY_PICK;\n+\t\titem->action = REPLAY_PICK;\n \t\tp += strlen(\"pick \");\n \t} else if (!prefixcmp(start, \"revert \")) {\n-\t\taction = REVERT;\n+\t\titem->action = REPLAY_REVERT;\n \t\tp += strlen(\"revert \");\n-\t} else\n-\t\treturn NULL;\n+\t} else {\n+\t\tsize_t len = strchrnul(p, '\\n') - p;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"Unrecognized action: %.*s\"), len, p);\n+\t}\n \n \tq = p + strcspn(p, \" \\n\");\n-\tif (q - p + 1 > sizeof(sha1_abbrev))\n-\t\treturn NULL;\n+\tif (q - p + 1 > sizeof(sha1_abbrev)) {\n+\t\tsize_t len = q - p;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"Object name too large: %.*s\"), len, p);\n+\t}\n \tmemcpy(sha1_abbrev, p, q - p);\n \tsha1_abbrev[q - p] = '\\0';\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 (get_sha1(sha1_abbrev, commit_sha1) < 0)\n-\t\treturn NULL;\n+\t\treturn error(_(\"Malformed object name: %s\"), sha1_abbrev);\n \n-\treturn lookup_commit_reference(commit_sha1);\n+\titem->operand = lookup_commit_reference(commit_sha1);\n+\tif (!item->operand)\n+\t\treturn error(_(\"Not a valid commit: %s\"), sha1_abbrev);\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\tcommit = parse_insn_line(p, 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\tif (parse_insn_line(p, &item) < 0)\n+\t\t\treturn error(_(\"on line %d.\"), i);\n+\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n \t\tp = strchrnul(p, '\\n');\n \t\tif (*p)\n \t\t\tp++;\n@@ -749,8 +749,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@@ -766,7 +765,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@@ -815,18 +814,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@@ -855,7 +854,7 @@ static void save_head(const char *head)\n \t\tdie(_(\"Error wrapping up %s.\"), head_file);\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@@ -863,7 +862,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@@ -907,9 +906,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@@ -919,8 +919,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@@ -945,7 +945,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@@ -962,7 +962,7 @@ static int pick_revisions(struct replay_opts *opts)\n \t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n \t\t\tgoto error;\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@@ -982,7 +982,7 @@ static int pick_revisions(struct replay_opts *opts)\n \t\t\treturn -1;\n \t\t}\n \t\tif (get_sha1(\"HEAD\", sha1)) {\n-\t\t\tif (opts->action == REVERT)\n+\t\t\tif (opts->action == REPLAY_REVERT)\n \t\t\t\treturn error(_(\"Can't revert as initial commit\"));\n \t\t\treturn error(_(\"Can't cherry-pick into empty head\"));\n \t\t}\n@@ -1002,7 +1002,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@@ -1017,7 +1017,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 905d295..f4db257 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -7,6 +7,14 @@\n #define SEQ_TODO_FILE\t\"sequencer/todo\"\n #define SEQ_OPTS_FILE\t\"sequencer/opts\"\n \n+enum replay_action { REPLAY_REVERT, REPLAY_PICK };\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 39b55c1..4b12244 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -239,4 +239,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_must_fail 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_must_fail 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":"177238","messageId":"1318095407-26429-7-git-send-email-artagnon@gmail.com","threadId":"28633","inReplyTo":"1318095407-26429-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 6/6] revert: Simplify passing command-line arguments around","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-08T17:36:47Z","receivedAt":"2011-10-08T17:36:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"From: Jonathan Nieder \n\nThe chief command-line argument parser, parse_args, currently parses\narguments into an (argc, argv) and lets prepare_revs, a later\nfunction, turn it into a structure. Change this so that callers of\nthe cherry-pick machinery will have to fill in an options structure\ninstead of crafting command-line arguments.\n\n[rr: minor improvements, commit message]\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Jonathan Nieder \n---\n builtin/revert.c | 53 +++++++++++++++++++++++++++++------------------------\n 1 files changed, 29 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex a9dd210..a72b20d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -54,13 +54,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@@ -163,9 +164,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@@ -200,9 +201,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@@ -210,7 +208,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@@ -607,23 +618,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@@ -817,14 +820,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@@ -948,6 +950,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/*\n-- \n1.7.4.1\n"},{"id":"177256","messageId":"CALUzUxo=xN735+=Yz9eS_VSW3fpiTeng9s-66qM0Jno40-DPXQ@mail.gmail.com","threadId":"28633","inReplyTo":"1318095407-26429-7-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 6/6] revert: Simplify passing command-line arguments around","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-10-09T02:14:12Z","receivedAt":"2011-10-09T02:14:12Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sun, Oct 9, 2011 at 1:36 AM, Ramkumar Ramachandra wrote:\n> [snip]\n> [rr: minor improvements, commit message]\n\nThis \"[]\" could go below, under the 3-dash (but before the stat):\n\n> [snip]\n> ---\n>  builtin/revert.c |   53 +++++++++++++++++++++++++++++------------------------\n>  1 files changed, 29 insertions(+), 24 deletions(-)\n\n-- \nCheers,\nRay Chuan\n"},{"id":"177264","messageId":"CALkWK0kA=zhpsmYhjMwv11xyHNhA0Ps=BjUDao0+HFLMKnADUg@mail.gmail.com","threadId":"28633","inReplyTo":"CALUzUxo=xN735+=Yz9eS_VSW3fpiTeng9s-66qM0Jno40-DPXQ@mail.gmail.com","subject":"Re: [PATCH 6/6] revert: Simplify passing command-line arguments around","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-09T08:28:37Z","receivedAt":"2011-10-09T08:28:37Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Tay,\n\nTay Ray Chuan writes:\n> On Sun, Oct 9, 2011 at 1:36 AM, Ramkumar Ramachandra wrote:\n>> [rr: minor improvements, commit message]\n>\n> This \"[]\" could go below, under the 3-dash (but before the stat):\n\nActually, I intended to put it in the commit message. Seems to be\nidiomatic: grep the log for \"\\[jc:\" etc.\n\nThanks.\n\n-- Ram\n"},{"id":"177265","messageId":"20111009085306.GA9209@elie.hsd1.il.comcast.net","threadId":"28633","inReplyTo":"CALkWK0kA=zhpsmYhjMwv11xyHNhA0Ps=BjUDao0+HFLMKnADUg@mail.gmail.com","subject":"Re: [PATCH 6/6] revert: Simplify passing command-line arguments around","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-09T08:53:07Z","receivedAt":"2011-10-09T08:53:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Actually, I intended to put it in the commit message. Seems to be\n> idiomatic: grep the log for \"\\[jc:\" etc.\n\nMore important than the idiom is the intent. Presumably that intent\nwas something like \"I wrote the commit message, so if it makes you\nscratch your head, blame me, not Jonathan; and I made some other\n(minor) improvements, so consider blaming me even if it's the\nfunctional part that makes you scratch your head.\"\n\nSorry I haven't had a chance to look over the patch yet. Is it\nsupposed to introduce a behavior change, does it prepare for some\nfuture change, or is it just a cleanup?\n"},{"id":"177267","messageId":"CALkWK0niwg1Ogs+xBr8NHEce-MUUzPc_Upn0ZKRi3iCZ6fA4BQ@mail.gmail.com","threadId":"28633","inReplyTo":"20111009085306.GA9209@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 6/6] revert: Simplify passing command-line arguments around","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-09T09:04:40Z","receivedAt":"2011-10-09T09:04:40Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> More important than the idiom is the intent.  Presumably that intent\n> was something like \"I wrote the commit message, so if it makes you\n> scratch your head, blame me, not Jonathan; and I made some other\n> (minor) improvements, so consider blaming me even if it's the\n> functional part that makes you scratch your head.\"\n\nExactly.\n\n> Sorry I haven't had a chance to look over the patch yet.  Is it\n> supposed to introduce a behavior change, does it prepare for some\n> future change, or is it just a cleanup?\n\nPrepare for some future change. See $gmane/179282 for original discussion.\n\n-- Ram\n"},{"id":"177268","messageId":"20111009092432.GB29150@elie.hsd1.il.comcast.net","threadId":"28633","inReplyTo":"CALkWK0niwg1Ogs+xBr8NHEce-MUUzPc_Upn0ZKRi3iCZ6fA4BQ@mail.gmail.com","subject":"Re: [PATCH 6/6] revert: Simplify passing command-line arguments around","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-09T09:24:32Z","receivedAt":"2011-10-09T09:24:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Jonathan Nieder writes:\n\n>> Sorry I haven't had a chance to look over the patch yet.  Is it\n>> supposed to introduce a behavior change, does it prepare for some\n>> future change, or is it just a cleanup?\n>\n> Prepare for some future change. See $gmane/179282 for original discussion.\n\nThanks, but I shouldn't have had to ask. Care to fix the commit\nmessage? :)\n"},{"id":"177321","messageId":"7vwrccn34l.fsf@alter.siamese.dyndns.org","threadId":"28633","inReplyTo":"1318095407-26429-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-10T17:54:50Z","receivedAt":"2011-10-10T17:54:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Change the instruction sheet format subtly so that a description of\n> the commit after the object name is optional. As a result, an\n> instruction sheet like this is now perfectly valid:\n>\n> pick 35b0426\n> pick fbd5bbcbc2e\n> pick 7362160f\n>\n> Suggested-by: Jonathan Nieder \n> Acked-by: Jonathan Nieder \n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> builtin/revert.c | 19 ++++++++-----------\n> t/t3510-cherry-pick-sequence.sh | 14 ++++++++++++++\n> 2 files changed, 22 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 6451089..aa6c34e 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -692,26 +692,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\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> +\tconst char *p, *q;\n> \n> +\tp = start;\n> \tif (!prefixcmp(start, \"pick \")) {\n> \t\taction = CHERRY_PICK;\n> -\t\tinsn_len = strlen(\"pick\");\n> -\t\tp = start + insn_len + 1;\n> +\t\tp += strlen(\"pick \");\n> \t} else if (!prefixcmp(start, \"revert \")) {\n> \t\taction = REVERT;\n> -\t\tinsn_len = strlen(\"revert\");\n> -\t\tp = start + insn_len + 1;\n> +\t\tp += strlen(\"revert \");\n> \t} else\n> \t\treturn NULL;\n> \n> -\tq = strchr(p, ' ');\n> -\tif (!q || q - p + 1 > sizeof(sha1_abbrev))\n> +\tq = p + strcspn(p, \" \\n\");\n> +\tif (q - p + 1 > sizeof(sha1_abbrev))\n> \t\treturn NULL;\n> -\tq++;\n> -\n> -\tstrlcpy(sha1_abbrev, p, q - p);\n> +\tmemcpy(sha1_abbrev, p, q - p);\n> +\tsha1_abbrev[q - p] = '\\0';\n\nSince this is a part of clean-up series...\n\nDo you even need to have a sha1_abbrev[] local array that is limited to 40\nbytes here? The incoming _line_ is not \"const char *start\", so you should\nat least be able to temporarily terminate the commit object name with NUL\n(while remembering what byte there was before), give it to get_sha1(), and\nthen restore the byte at the end before returning from this function.\n\nA bonus point would be to introduce a variant of get_sha1() that can take\n(a pointer + len) not (a pointer to NUL terminated string). While I think\nthat would be a right thing to do in the longer term, it is outside of the\nscope of this series.\n"},{"id":"177486","messageId":"7vwrc97wfe.fsf@alter.siamese.dyndns.org","threadId":"28633","inReplyTo":"7vwrccn34l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T21:05:25Z","receivedAt":"2011-10-12T21:05:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Since this is a part of clean-up series...\n>\n> Do you even need to have a sha1_abbrev[] local array that is limited to 40\n> bytes here? The incoming _line_ is not \"const char *start\", so you should\n> at least be able to temporarily terminate the commit object name with NUL\n> (while remembering what byte there was before), give it to get_sha1(), and\n> then restore the byte at the end before returning from this function.\n\nLike this, perhaps. I did this on top of the whole series only as a\ndemonstration but the change should be squashed into this step when the\nseries is rerolled.\n\n builtin/revert.c | 47 +++++++++++++++++++----------------------------\n 1 files changed, 19 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex b28c3ca..170a6c1 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -691,42 +691,34 @@ 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 *start, struct replay_insn_list *item)\n+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n {\n \tunsigned char commit_sha1[20];\n-\tchar sha1_abbrev[40];\n-\tconst char *p, *q;\n+\tchar *end_of_object_name;\n+\tint saved, status;\n \n-\tp = start;\n-\tif (!prefixcmp(start, \"pick \")) {\n+\tif (!prefixcmp(bol, \"pick \")) {\n \t\titem->action = REPLAY_PICK;\n-\t\tp += strlen(\"pick \");\n-\t} else if (!prefixcmp(start, \"revert \")) {\n+\t\tbol += strlen(\"pick \");\n+\t} else if (!prefixcmp(bol, \"revert \")) {\n \t\titem->action = REPLAY_REVERT;\n-\t\tp += strlen(\"revert \");\n+\t\tbol += strlen(\"revert \");\n \t} else {\n-\t\tsize_t len = strchrnul(p, '\\n') - p;\n-\t\tif (len > 255)\n-\t\t\tlen = 255;\n-\t\treturn error(_(\"Unrecognized action: %.*s\"), (int)len, p);\n+\t\treturn error(_(\"Unrecognized action: %s\"), bol);\n \t}\n \n-\tq = p + strcspn(p, \" \\n\");\n-\tif (q - p + 1 > sizeof(sha1_abbrev)) {\n-\t\tsize_t len = q - p;\n-\t\tif (len > 255)\n-\t\t\tlen = 255;\n-\t\treturn error(_(\"Object name too large: %.*s\"), (int)len, p);\n-\t}\n-\tmemcpy(sha1_abbrev, p, q - p);\n-\tsha1_abbrev[q - p] = '\\0';\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-\tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n-\t\treturn error(_(\"Malformed object name: %s\"), sha1_abbrev);\n+\tif (status < 0)\n+\t\treturn error(_(\"Malformed object name: %s\"), bol);\n \n \titem->operand = lookup_commit_reference(commit_sha1);\n \tif (!item->operand)\n-\t\treturn error(_(\"Not a valid commit: %s\"), sha1_abbrev);\n+\t\treturn error(_(\"Not a valid commit: %s\"), bol);\n \n \titem->next = NULL;\n \treturn 0;\n@@ -740,12 +732,11 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n \tint i;\n \n \tfor (i = 1; *p; i++) {\n-\t\tif (parse_insn_line(p, &item) < 0)\n+\t\tchar *eol = strchrnul(p, '\\n');\n+\t\tif (parse_insn_line(p, eol, &item) < 0)\n \t\t\treturn error(_(\"on line %d.\"), i);\n \t\tnext = replay_insn_list_append(item.action, item.operand, 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.\"));\n"},{"id":"177855","messageId":"CALkWK0=+-4ZTg1hp6-e6k7SyA7jG39-RM31tqcq2K4u2Cf5pcg@mail.gmail.com","threadId":"28633","inReplyTo":"7vwrc97wfe.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-17T10:06:24Z","receivedAt":"2011-10-17T10:06:24Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano writes:\n> Like this, perhaps.  I did this on top of the whole series only as a\n> demonstration but the change should be squashed into this step when the\n> series is rerolled.\n>\n>  builtin/revert.c |   47 +++++++++++++++++++----------------------------\n> [...]\n\nThanks! This is fantastic. I'll re-roll soon with better commit messages :)\n\n-- Ram\n"},{"id":"177857","messageId":"CALkWK0msrEYPzpR1rgx0BzHpGiBiP9+doDKV+2N5mEruzhPqUg@mail.gmail.com","threadId":"28633","inReplyTo":"20111009092432.GB29150@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 6/6] revert: Simplify passing command-line arguments around","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-17T10:10:34Z","receivedAt":"2011-10-17T10:10:34Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Thanks, but I shouldn't have had to ask.  Care to fix the commit\n> message? :)\n\nAh, yes- my persistent lack of clarity in commit messages :|\nHopefully, the next iteration will have better commit messages.\n\n-- Ram\n"}]}