{"thread":{"id":"28723","subject":"[PATCH v2 0/6] Sequencer fixups mini-series","startedAt":"2011-10-19T21:03:22Z","lastAt":"2011-10-22T18:57:05Z","messageCount":16,"participants":["Ramkumar Ramachandra","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"178027","messageId":"1319058208-17923-1-git-send-email-artagnon@gmail.com","threadId":"28723","inReplyTo":null,"subject":"[PATCH v2 0/6] Sequencer fixups mini-series","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-19T21:03:22Z","receivedAt":"2011-10-19T21:03:22Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThis is the second iteration of series I first posted in\n$gmane/183157.  The improvements can be summarized as follows:\n1. Junio wrote some extra code to squash into the fourth part.  It\ntook me some time to resolve conflicts correctly while rebasing.\n2. Jonathan and Tay (off-list) have helped improve all the commit\nmessages.\n\nThanks.\n\nJonathan Nieder (1):\n  revert: simplify communicating command-line arguments\n\nRamkumar Ramachandra (5):\n  revert: free msg in format_todo()\n  revert: simplify getting commit subject in format_todo()\n  revert: fix buffer overflow in insn sheet parser\n  revert: make commit subjects in insn sheet optional\n  revert: allow mixed pick and revert instructions\n\n builtin/revert.c                |  219 +++++++++++++++++++--------------------\n sequencer.h                     |    8 ++\n t/t3510-cherry-pick-sequence.sh |   86 +++++++++++++++\n 3 files changed, 202 insertions(+), 111 deletions(-)\n\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"178028","messageId":"1319058208-17923-2-git-send-email-artagnon@gmail.com","threadId":"28723","inReplyTo":"1319058208-17923-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/6] revert: free msg in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-19T21:03:23Z","receivedAt":"2011-10-19T21:03:23Z","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 <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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 010508d..94b7c50 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -680,6 +680,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.6.351.gb35ac.dirty\n"},{"id":"178029","messageId":"1319058208-17923-3-git-send-email-artagnon@gmail.com","threadId":"28723","inReplyTo":"1319058208-17923-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/6] revert: simplify getting commit subject in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-19T21:03:24Z","receivedAt":"2011-10-19T21:03:24Z","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.  Save work and unnecessary memory allocations by\nusing find_commit_subject() instead.\n\nSuggested-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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 94b7c50..acb357d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -671,16 +671,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.6.351.gb35ac.dirty\n"},{"id":"178031","messageId":"1319058208-17923-4-git-send-email-artagnon@gmail.com","threadId":"28723","inReplyTo":"1319058208-17923-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-19T21:03:25Z","receivedAt":"2011-10-19T21:03:25Z","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 <jrnieder@gmail.com>\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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 acb357d..474dda1 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -705,7 +705,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.6.351.gb35ac.dirty\n"},{"id":"178030","messageId":"1319058208-17923-5-git-send-email-artagnon@gmail.com","threadId":"28723","inReplyTo":"1319058208-17923-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/6] revert: make commit subjects in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-19T21:03:26Z","receivedAt":"2011-10-19T21:03:26Z","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\nSuggested-by: Jonathan Nieder <jrnieder@gmail.com>\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/revert.c                |   37 ++++++++++++++++---------------------\n t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++\n 2 files changed, 30 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 474dda1..8e34dc0 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -685,31 +685,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 || q - p + 1 > sizeof(sha1_abbrev))\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@@ -722,7 +718,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@@ -737,13 +733,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 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.6.351.gb35ac.dirty\n"},{"id":"178032","messageId":"1319058208-17923-6-git-send-email-artagnon@gmail.com","threadId":"28723","inReplyTo":"1319058208-17923-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-19T21:03:27Z","receivedAt":"2011-10-19T21:03:27Z","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.\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 <jrnider@gmail.com>\nAcked-by: Jonathan Nieder <jrnider@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/revert.c                |  130 +++++++++++++++++++--------------------\n sequencer.h                     |    8 +++\n t/t3510-cherry-pick-sequence.sh |   58 +++++++++++++++++\n 3 files changed, 129 insertions(+), 67 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8e34dc0..0201e98 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@@ -152,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@@ -346,7 +345,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@@ -450,7 +449,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@@ -525,7 +525,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@@ -568,7 +568,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@@ -587,7 +587,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@@ -611,7 +611,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@@ -657,49 +657,53 @@ 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} else {\n+\t\tsize_t len = strchrnul(bol, '\\n') - bol;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"Unrecognized action: %.*s\"), (int)len, bol);\n+\t}\n \n \tend_of_object_name = bol + strcspn(bol, \" \\n\");\n \tsaved = *end_of_object_name;\n@@ -707,37 +711,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 error(_(\"Malformed object name: %s\"), bol);\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\"), bol);\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\t\treturn error(_(\"Could not parse line %d.\"), i);\n-\t\tnext = commit_list_append(commit, next);\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 = *eol ? eol + 1 : eol;\n \t}\n \tif (!*todo_list)\n@@ -745,8 +741,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@@ -762,7 +757,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@@ -811,18 +806,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@@ -851,7 +846,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@@ -859,7 +854,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@@ -903,9 +898,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@@ -915,8 +911,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@@ -941,7 +937,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@@ -958,7 +954,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@@ -978,7 +974,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@@ -998,7 +994,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@@ -1013,7 +1009,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.6.351.gb35ac.dirty\n"},{"id":"178033","messageId":"1319058208-17923-7-git-send-email-artagnon@gmail.com","threadId":"28723","inReplyTo":"1319058208-17923-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 6/6] revert: simplify communicating command-line arguments","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-19T21:03:28Z","receivedAt":"2011-10-19T21:03:28Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"From: Jonathan Nieder <jrnieder@gmail.com>\n\nCurrently, command-line arguments are communicated using (argc, argv)\nuntil a prepare_revs() turns it into a terse structure.  However,\nsince we plan to expose the cherry-picking machinery through a public\nAPI in the future, we want callers to be able to call in with a\nfilled-in structure.  For the revert builtin, this means that the\nchief argument parser, parse_args(), should parse into such a\nstructure.  Make this change.\n\n[rr: minor improvements, commit message]\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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 0201e98..576b9e7 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@@ -161,9 +162,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@@ -198,9 +199,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@@ -208,7 +206,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@@ -605,23 +616,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@@ -809,14 +812,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@@ -940,6 +942,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.6.351.gb35ac.dirty\n"},{"id":"178062","messageId":"7v8vogbgai.fsf@alter.siamese.dyndns.org","threadId":"28723","inReplyTo":"1319058208-17923-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-20T01:30:45Z","receivedAt":"2011-10-20T01:30:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> 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\n> on-stack buffer.  This fixes a regression introduced by 5a5d80f4\n> (revert: Introduce --continue to continue the operation, 2011-08-04).\n\nGiven that this function is going to be fixed properly so that it does not\neven need to use the \"on-stack buffer\", is this really necessary?\n"},{"id":"178079","messageId":"20111020080328.GA12337@elie.hsd1.il.comcast.net","threadId":"28723","inReplyTo":"7v8vogbgai.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-20T08:03:28Z","receivedAt":"2011-10-20T08:03:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n>> Check that the commit name argument to a \"pick\" or \"revert\" action in\n>> '.git/sequencer/todo' is not too long\n[...]\n> Given that this function is going to be fixed properly so that it does not\n> even need to use the \"on-stack buffer\", is this really necessary?\n\nRight, I don't think it is.  Keeping a testcase sounds worthwhile,\nthough.\n"},{"id":"178080","messageId":"4E9FE061.3080601@gmail.com","threadId":"28723","inReplyTo":"20111020080328.GA12337@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-20T08:48:33Z","receivedAt":"2011-10-20T08:48:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan and Junio,\n\nJonathan Nieder writes:\n> Junio C Hamano wrote:\n>> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n>>> Check that the commit name argument to a \"pick\" or \"revert\" action in\n>>> '.git/sequencer/todo' is not too long\n> [...]\n>> Given that this function is going to be fixed properly so that it does not\n>> even need to use the \"on-stack buffer\", is this really necessary?\n>\n> Right, I don't think it is.  Keeping a testcase sounds worthwhile,\n> though.\n\nOkay.  How about putting this after 5/6?\n\n-- 8< --\nSubject: [PATCH] t3510: guard against buffer overflows in parser\n\nTo guard against a buffer overflow in the parser, verify that\ninstruction sheets with overly long object names are parsed.\n\nSuggested-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++\n 1 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 0e29e03..39b55c1 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        git cherry-pick --reset &&\n        git checkout -f \"$1^0\" &&\n@@ -211,6 +214,17 @@ test_expect_success 'malformed instruction sheet 2' '\n        test_must_fail git cherry-pick --continue\n '\n \n+test_expect_success 'malformed instruction sheet 3' '\n+       pristine_detach initial &&\n+       test_must_fail git cherry-pick base..anotherpick &&\n+       echo \"resolved\" >foo &&\n+       git add foo &&\n+       git commit &&\n+       sed \"s/pick \\([0-9a-f]*\\)/pick $_r10/\" .git/sequencer/todo >new_sheet &&\n+       cp new_sheet .git/sequencer/todo &&\n+       test_must_fail git cherry-pick --continue\n+'\n+\n test_expect_success 'commit descriptions in insn sheet are optional' '\n        pristine_detach initial &&\n        test_must_fail git cherry-pick base..anotherpick &&\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"178081","messageId":"20111020090912.GA21471@elie.hsd1.il.comcast.net","threadId":"28723","inReplyTo":"4E9FE061.3080601@gmail.com","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-20T09:09:12Z","receivedAt":"2011-10-20T09:09:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Okay.  How about putting this after 5/6?\n>\n> -- 8< --\n> Subject: [PATCH] t3510: guard against buffer overflows in parser\n> \n> To guard against a buffer overflow in the parser, verify that\n> instruction sheets with overly long object names are parsed.\n\nLooks good, except I would explain it differently, to avoid referring\nto hypothetical implementation details (\"What buffer overflow?\"):\n\n\ttest: git cherry-pick --continue should cope with long object names\n\n\tA naive implementation that uses a commit-id-shaped buffer\n\tto store the word after \"pick\" in .git/sequencer/todo lines\n\twould crash often.  Our implementation is not so naive, but\n\tadd a test anyway to futureproof it.\n\nOr:\n\n\ttest: make sure the \"cherry-pick --continue\" buffer overflow doesn't come back\n\n\tBefore commit ..., \"git cherry-pick --continue\" would overflow\n\tunder ... circumstance.  Add a test to make sure it doesn't\n\thappen again.\n\nThough the implementation is actually better than that --- it can even\ncope with a valid object name (e.g., a long name of a branch, or\nsomething like \"HEAD^{/refs.c: ensure struct whose member}\") that is\nthat long, without truncating it.  So if you have time for it, I think\nit would be worth a test where the \"git cherry-pick --continue\"\nsucceeds, too.\n\nThanks,\nJonathan\n"},{"id":"178089","messageId":"CALkWK0nQD7sw506D7shE8TYe5xGLNJkzS5_6L-oOR7gT54PbAA@mail.gmail.com","threadId":"28723","inReplyTo":"20111020090912.GA21471@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-20T15:36:00Z","receivedAt":"2011-10-20T15:36:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> [...]\n> Looks good, except I would explain it differently, to avoid referring\n> to hypothetical implementation details (\"What buffer overflow?\"):\n>\n>        test: git cherry-pick --continue should cope with long object names\n>\n>        A naive implementation that uses a commit-id-shaped buffer\n>        to store the word after \"pick\" in .git/sequencer/todo lines\n>        would crash often.  Our implementation is not so naive, but\n>        add a test anyway to futureproof it.\n> [...]\n\nI picked this one.\n\n> Though the implementation is actually better than that --- it can even\n> cope with a valid object name (e.g., a long name of a branch, or\n> something like \"HEAD^{/refs.c: ensure struct whose member}\") that is\n> that long, without truncating it.  So if you have time for it, I think\n> it would be worth a test where the \"git cherry-pick --continue\"\n> succeeds, too.\n\nGood idea.  Will re-roll shortly.\n\nThanks.\n\n-- Ram\n"},{"id":"178094","messageId":"7vmxcva8k1.fsf@alter.siamese.dyndns.org","threadId":"28723","inReplyTo":"20111020090912.GA21471@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-20T17:15:26Z","receivedAt":"2011-10-20T17:15:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Looks good, except I would explain it differently, to avoid referring\n> to hypothetical implementation details (\"What buffer overflow?\"):\n>\n> \ttest: git cherry-pick --continue should cope with long object names\n>\n> \tA naive implementation that uses a commit-id-shaped buffer\n> \tto store the word after \"pick\" in .git/sequencer/todo lines\n> \twould crash often.  Our implementation is not so naive, but\n> \tadd a test anyway to futureproof it.\n>\n> Or:\n>\n> \ttest: make sure the \"cherry-pick --continue\" buffer overflow doesn't come back\n>\n> \tBefore commit ..., \"git cherry-pick --continue\" would overflow\n> \tunder ... circumstance.  Add a test to make sure it doesn't\n> \thappen again.\n\nI doubt you would need any of that.\n\nYou can just explain the commit that stops copying the lines into a\nprivate, fixed buffer a bit better (e.g. \"such copying is not just\nwasteful but is wrong by unnecessary placing an artificial limit on the\nline length\"), and say \"Incidentally, this fixes a bug in the earlier\nround of this series that failed to read lines that are too long to fit on\nthe buffer, demonstrated by the test added by this patch\", or something.\n\nThen the additional test can become part of the patch that corrects the\nparsing logic, no?\n"},{"id":"178095","messageId":"20111020180533.GA5563@elie.hsd1.il.comcast.net","threadId":"28723","inReplyTo":"7vmxcva8k1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-20T18:05:33Z","receivedAt":"2011-10-20T18:05:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n[...]\n> Then the additional test can become part of the patch that corrects the\n> parsing logic, no?\n\nYes, that works, too.  All I was trying to say was that the\ndescription in the patch I quoted didn't make sense to me, since it\nincluded a mention of a buffer overflow without giving any explanation\nof what it was talking about.  I don't actually care in this case\nwhether it is fixed by mentioning which patch this is testing the fix\nfrom or by squashing the two patches (though the latter certainly\nseems reasonable).\n\nIncidentally, Ram might wonder why I fuss so much about commit\nmessages.  It's actually very simple --- I think of them as part of\nthe code.  Suppose someone discovers a regression was introduced by\nsuch-and-such part of the patch 1.7.7 -> 1.7.8, but at first glance it\nis not clear whether that code change was supposed to have any effect\non the behavior of the code at all.  Such a person is likely to make\nmistakes in fixing it, right?  So after getting the right behavior,\npatch authors spend a few extra minutes to make sure the code is\nintuitive to humans, too, and this includes making sure the rationale\ndescription is clear.\n\nJust like the code for the computer, this is very much something that\nisn't always going to be right the first time and sometimes takes some\ndebugging.  So, sorry for the fuss, but I hope it helps.\n\nJonathan\n"},{"id":"178098","messageId":"7vaa8va3xm.fsf@alter.siamese.dyndns.org","threadId":"28723","inReplyTo":"20111020180533.GA5563@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-20T18:55:17Z","receivedAt":"2011-10-20T18:55:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Incidentally, Ram might wonder why I fuss so much about commit\n> messages.  It's actually very simple --- I think of them as part of\n> the code.\n\nAnd another reason is because I do fuss about them too ;-)\n\nIt is easy to tell a good patch from a bad one by just reading the message\nwithout actually reading the patch text itself.\n\nWhen the log message justifies the cause and the approach in the right\nway, the actual patch becomes self evident. Also I often find myself\ncoming up with a _better_ solution than the patch I originally prepared\nwhile writing the commit log message to explain it, and redoing the patch\ntext to match the description.\n"},{"id":"178170","messageId":"CALkWK0n3srWcsDxaWvgd7s7jHVuSKv+aLJ1n0inae2hMA_C3kQ@mail.gmail.com","threadId":"28723","inReplyTo":"7vaa8va3xm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-10-22T18:57:05Z","receivedAt":"2011-10-22T18:57:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio and Jonathan,\n\nThanks for the good suggestions.  Will post the next iteration in a\nfew minutes (some tests running now).\n\nJunio C Hamano writes:\n> [...]\n> When the log message justifies the cause and the approach in the right\n> way, the actual patch becomes self evident. Also I often find myself\n> coming up with a _better_ solution than the patch I originally prepared\n> while writing the commit log message to explain it, and redoing the patch\n> text to match the description.\n\nWow.  It looks like I have a long way to go :/\nMaybe I should practice writing more Haskell.\n\n-- Ram\n"}]}