{"thread":{"id":"29103","subject":"[PATCH 0/5] Re-roll rr/revert-cherry-pick","startedAt":"2011-12-07T06:37:52Z","lastAt":"2011-12-09T22:30:33Z","messageCount":23,"participants":["Ramkumar Ramachandra","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"180449","messageId":"1323239877-24101-1-git-send-email-artagnon@gmail.com","threadId":"29103","inReplyTo":null,"subject":"[PATCH 0/5] Re-roll rr/revert-cherry-pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-07T06:37:52Z","receivedAt":"2011-12-07T06:37:52Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThis is a re-roll of rr/revert-cherry-pick, with Junio's suggestions\n($gname/186365) implemented.  My new branch rr/sequencer will be based\non this branch.\n\nThanks.\n\nJonathan Nieder (1):\n  revert: simplify communicating command-line arguments\n\nRamkumar Ramachandra (4):\n  revert: free msg in format_todo()\n  revert: make commit subjects in insn sheet optional\n  revert: simplify getting commit subject in format_todo()\n  revert: allow mixed pick and revert instructions\n\n builtin/revert.c                |  225 +++++++++++++++++++--------------------\n sequencer.h                     |    8 ++\n t/t3510-cherry-pick-sequence.sh |   86 +++++++++++++++\n 3 files changed, 206 insertions(+), 113 deletions(-)\n\n-- \n1.7.7.3\n"},{"id":"180450","messageId":"1323239877-24101-2-git-send-email-artagnon@gmail.com","threadId":"29103","inReplyTo":"1323239877-24101-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/5] revert: free msg in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-07T06:37:53Z","receivedAt":"2011-12-07T06:37:53Z","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 1ea525c..0c6d3d8 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\tif (get_message(cur->item, &msg))\n \t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n \t\tstrbuf_addf(buf, \"%s %s %s\\n\", action_str, sha1_abbrev, msg.subject);\n+\t\tfree_message(&msg);\n \t}\n \treturn 0;\n }\n-- \n1.7.7.3\n"},{"id":"180451","messageId":"1323239877-24101-3-git-send-email-artagnon@gmail.com","threadId":"29103","inReplyTo":"1323239877-24101-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/5] revert: make commit subjects in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-07T06:37:54Z","receivedAt":"2011-12-07T06:37:54Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Change the instruction sheet format subtly so that the subject of the\ncommit message that follows the object name is optional.  As a result,\nan instruction sheet like this is now perfectly valid:\n\n  pick 35b0426\n  pick fbd5bbcbc2e\n  pick 7362160f\n\nWhile at it, also fix a bug: currently, we use a commit-id-shaped\nbuffer to store the word after \"pick\" in '.git/sequencer/todo'.  This\nis both wasteful and wrong because it places an artificial limit on\nthe line length.  Eliminate the need for the buffer altogether, and\nadd a test demonstrating this.\n\n[jc: simplify parsing]\n\nSuggested-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 |   28 ++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0c6d3d8..70055e5 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \treturn 0;\n }\n \n-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)\n {\n \tunsigned char commit_sha1[20];\n-\tchar sha1_abbrev[40];\n \tenum replay_action action;\n-\tint insn_len = 0;\n-\tchar *p, *q;\n+\tchar *end_of_object_name;\n+\tint saved, status;\n \n-\tif (!prefixcmp(start, \"pick \")) {\n+\tif (!prefixcmp(bol, \"pick \")) {\n \t\taction = CHERRY_PICK;\n-\t\tinsn_len = strlen(\"pick\");\n-\t\tp = start + insn_len + 1;\n-\t} else if (!prefixcmp(start, \"revert \")) {\n+\t\tbol += strlen(\"pick \");\n+\t} else if (!prefixcmp(bol, \"revert \")) {\n \t\taction = REVERT;\n-\t\tinsn_len = strlen(\"revert\");\n-\t\tp = start + insn_len + 1;\n+\t\tbol += strlen(\"revert \");\n \t} else\n \t\treturn NULL;\n \n-\tq = strchr(p, ' ');\n-\tif (!q)\n-\t\treturn NULL;\n-\tq++;\n-\n-\tstrlcpy(sha1_abbrev, p, q - p);\n+\tend_of_object_name = bol + strcspn(bol, \" \\n\");\n+\tsaved = *end_of_object_name;\n+\t*end_of_object_name = '\\0';\n+\tstatus = get_sha1(bol, commit_sha1);\n+\t*end_of_object_name = saved;\n \n \t/*\n \t * Verify that the action matches up with the one in\n@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n \t\treturn NULL;\n \t}\n \n-\tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n+\tif (status < 0)\n \t\treturn NULL;\n \n \treturn lookup_commit_reference(commit_sha1);\n@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \tint i;\n \n \tfor (i = 1; *p; i++) {\n-\t\tcommit = parse_insn_line(p, opts);\n+\t\tchar *eol = strchrnul(p, '\\n');\n+\t\tcommit = parse_insn_line(p, eol, opts);\n \t\tif (!commit)\n \t\t\treturn error(_(\"Could not parse line %d.\"), i);\n \t\tnext = commit_list_append(commit, next);\n-\t\tp = strchrnul(p, '\\n');\n-\t\tif (*p)\n-\t\t\tp++;\n+\t\tp = *eol ? eol + 1 : eol;\n \t}\n \tif (!*todo_list)\n \t\treturn error(_(\"No commits parsed.\"));\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 2c4c1c8..6390f2a 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -13,6 +13,9 @@ test_description='Test cherry-pick continuation features\n \n . ./test-lib.sh\n \n+# Repeat first match 10 times\n+_r10='\\1\\1\\1\\1\\1\\1\\1\\1\\1\\1'\n+\n pristine_detach () {\n \tgit cherry-pick --quit &&\n \tgit checkout -f \"$1^0\" &&\n@@ -328,4 +331,29 @@ test_expect_success 'malformed instruction sheet 2' '\n \ttest_must_fail git cherry-pick --continue\n '\n \n+test_expect_success 'malformed instruction sheet 3' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick base..anotherpick &&\n+\techo \"resolved\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tsed \"s/pick \\([0-9a-f]*\\)/pick $_r10/\" .git/sequencer/todo >new_sheet &&\n+\tcp new_sheet .git/sequencer/todo &&\n+\ttest_must_fail git cherry-pick --continue\n+'\n+\n+test_expect_success 'commit descriptions in insn sheet are optional' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick base..anotherpick &&\n+\techo \"c\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tcut -d\" \" -f1,2 .git/sequencer/todo >new_sheet &&\n+\tcp new_sheet .git/sequencer/todo &&\n+\tgit cherry-pick --continue &&\n+\ttest_path_is_missing .git/sequencer &&\n+\tgit rev-list HEAD >commits &&\n+\ttest_line_count = 4 commits\n+'\n+\n test_done\n-- \n1.7.7.3\n"},{"id":"180452","messageId":"1323239877-24101-4-git-send-email-artagnon@gmail.com","threadId":"29103","inReplyTo":"1323239877-24101-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/5] revert: simplify getting commit subject in format_todo()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-07T06:37:55Z","receivedAt":"2011-12-07T06:37:55Z","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.  Also, remove the spurious check\non cur->item: the previous patch makes sure that instruction sheets\nwith missing commit subjects are parsable.\n\nSuggested-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 |   10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 70055e5..706b8d4 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -697,16 +697,16 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\tstruct replay_opts *opts)\n {\n \tstruct commit_list *cur = NULL;\n-\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tconst char *sha1_abbrev = NULL;\n \tconst char *action_str = opts->action == REVERT ? \"revert\" : \"pick\";\n+\tconst char *subject;\n+\tint subject_len;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n-\t\tif (get_message(cur->item, &msg))\n-\t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n-\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action_str, sha1_abbrev, msg.subject);\n-\t\tfree_message(&msg);\n+\t\tsubject_len = find_commit_subject(cur->item->buffer, &subject);\n+\t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n+\t\t\tsubject_len, subject);\n \t}\n \treturn 0;\n }\n-- \n1.7.7.3\n"},{"id":"180453","messageId":"1323239877-24101-5-git-send-email-artagnon@gmail.com","threadId":"29103","inReplyTo":"1323239877-24101-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/5] revert: allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-07T06:37:56Z","receivedAt":"2011-12-07T06:37:56Z","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                |  136 +++++++++++++++++++--------------------\n sequencer.h                     |    8 ++\n t/t3510-cherry-pick-sequence.sh |   58 +++++++++++++++++\n 3 files changed, 133 insertions(+), 69 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 706b8d4..f2bf198 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 {\n \tREPLAY_NONE,\n \tREPLAY_REMOVE_STATE,\n@@ -73,14 +72,14 @@ struct replay_opts {\n \n static const char *action_name(const struct replay_opts *opts)\n {\n-\treturn opts->action == REVERT ? \"revert\" : \"cherry-pick\";\n+\treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n }\n \n static char *get_encoding(const char *message);\n \n static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n {\n-\treturn opts->action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n }\n \n static int option_parse_x(const struct option *opt,\n@@ -159,7 +158,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t};\n \n-\tif (opts->action == CHERRY_PICK) {\n+\tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n@@ -363,7 +362,7 @@ static int error_dirty_index(struct replay_opts *opts)\n \t\treturn error_resolve_conflict(action_name(opts));\n \n \t/* Different translation strings for cherry-pick and revert */\n-\tif (opts->action == CHERRY_PICK)\n+\tif (opts->action == REPLAY_PICK)\n \t\terror(_(\"Your local changes would be overwritten by cherry-pick.\"));\n \telse\n \t\terror(_(\"Your local changes would be overwritten by revert.\"));\n@@ -467,7 +466,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n \treturn run_command_v_opt(args, RUN_GIT_CMD);\n }\n \n-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n+static int do_pick_commit(struct commit *commit, enum replay_action action,\n+\t\t\tstruct replay_opts *opts)\n {\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n@@ -542,7 +542,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (opts->action == REVERT) {\n+\tif (action == REPLAY_REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -583,7 +583,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \t}\n \n-\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REVERT) {\n+\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || action == REPLAY_REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n \t\t\t\t\t head, &msgbuf, opts);\n \t\twrite_message(&msgbuf, defmsg);\n@@ -607,13 +607,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t * However, if the merge did not even start, then we don't want to\n \t * write it at all.\n \t */\n-\tif (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))\n+\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))\n \t\twrite_cherry_pick_head(commit, \"CHERRY_PICK_HEAD\");\n-\tif (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))\n+\tif (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))\n \t\twrite_cherry_pick_head(commit, \"REVERT_HEAD\");\n \n \tif (res) {\n-\t\terror(opts->action == REVERT\n+\t\terror(action == REPLAY_REVERT\n \t\t      ? _(\"could not revert %s... %s\")\n \t\t      : _(\"could not apply %s... %s\"),\n \t\t      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n@@ -637,7 +637,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)\n \n \tinit_revisions(revs, NULL);\n \trevs->no_walk = 1;\n-\tif (opts->action != REVERT)\n+\tif (opts->action != REPLAY_REVERT)\n \t\trevs->reverse = 1;\n \n \targc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);\n@@ -683,49 +683,55 @@ 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,\n+\t\t\tstruct replay_insn_list *item, int lineno)\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(_(\"Line %d: Unrecognized action: %.*s\"),\n+\t\t\tlineno, (int)len, bol);\n+\t}\n \n \tend_of_object_name = bol + strcspn(bol, \" \\n\");\n \tsaved = *end_of_object_name;\n@@ -733,37 +739,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(_(\"Line %d: Malformed object name: %s\"), lineno, 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(_(\"Line %d: Not a valid commit: %s\"), lineno, 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, i) < 0)\n+\t\t\treturn -1;\n+\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n \t\tp = *eol ? eol + 1 : eol;\n \t}\n \tif (!*todo_list)\n@@ -771,8 +769,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@@ -788,7 +785,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@@ -837,18 +834,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@@ -946,7 +943,7 @@ fail:\n \treturn -1;\n }\n \n-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+static void save_todo(struct replay_insn_list *todo_list)\n {\n \tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstatic struct lock_file todo_lock;\n@@ -954,7 +951,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@@ -998,9 +995,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@@ -1010,8 +1008,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@@ -1036,7 +1034,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@@ -1056,7 +1054,7 @@ static int pick_revisions(struct replay_opts *opts)\n \t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n \t\t\treturn error(_(\"No %s in progress\"), action_name(opts));\n \t\tread_populate_opts(&opts);\n-\t\tread_populate_todo(&todo_list, opts);\n+\t\tread_populate_todo(&todo_list);\n \n \t\t/* Verify that the conflict has been resolved */\n \t\tif (!index_differs_from(\"HEAD\", 0))\n@@ -1074,7 +1072,7 @@ static int pick_revisions(struct replay_opts *opts)\n \tif (create_seq_dir() < 0)\n \t\treturn -1;\n \tif (get_sha1(\"HEAD\", sha1)) {\n-\t\tif (opts->action == REVERT)\n+\t\tif (opts->action == REPLAY_REVERT)\n \t\t\treturn error(_(\"Can't revert as initial commit\"));\n \t\treturn error(_(\"Can't cherry-pick into empty head\"));\n \t}\n@@ -1091,7 +1089,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tmemset(&opts, 0, sizeof(opts));\n \tif (isatty(0))\n \t\topts.edit = 1;\n-\topts.action = REVERT;\n+\topts.action = REPLAY_REVERT;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = pick_revisions(&opts);\n@@ -1106,7 +1104,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tint res;\n \n \tmemset(&opts, 0, sizeof(opts));\n-\topts.action = CHERRY_PICK;\n+\topts.action = REPLAY_PICK;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = pick_revisions(&opts);\ndiff --git a/sequencer.h b/sequencer.h\nindex f435fdb..71cfcae 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 6390f2a..d52ad59 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '\n \ttest_line_count = 4 commits\n '\n \n+test_expect_success 'revert --continue continues after cherry-pick' '\n+\tpristine_detach initial &&\n+\ttest_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.7.3\n"},{"id":"180454","messageId":"1323239877-24101-6-git-send-email-artagnon@gmail.com","threadId":"29103","inReplyTo":"1323239877-24101-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/5] revert: simplify communicating command-line arguments","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-07T06:37:57Z","receivedAt":"2011-12-07T06:37:57Z","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 f2bf198..3b25a0c 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -59,13 +59,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@@ -168,9 +169,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@@ -212,9 +213,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@@ -222,7 +220,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@@ -631,23 +642,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@@ -837,14 +840,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@@ -1037,6 +1039,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.7.3\n"},{"id":"180456","messageId":"20111207064358.GB11737@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"1323239877-24101-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/5] revert: free msg in format_todo()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-07T06:43:58Z","receivedAt":"2011-12-07T06:43:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Memory allocated to the fields of msg by get_message() isn't freed.\n> This is potentially a big leak,\n[because it is in a loop over commits being picked]\n\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\n\n>  builtin/revert.c |    1 +\n>  1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 1ea525c..0c6d3d8 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n>  \t\tif (get_message(cur->item, &msg))\n>  \t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n>  \t\tstrbuf_addf(buf, \"%s %s %s\\n\", action_str, sha1_abbrev, msg.subject);\n> +\t\tfree_message(&msg);\n>  \t}\n>  \treturn 0;\n>  }\n"},{"id":"180457","messageId":"20111207070223.GC11737@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"1323239877-24101-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-07T07:02:24Z","receivedAt":"2011-12-07T07:02:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Change the instruction sheet format subtly so that the subject of the\n> commit message that follows the object name is optional.  As a result,\n> an instruction sheet like this is now perfectly valid:\n>\n>   pick 35b0426\n>   pick fbd5bbcbc2e\n>   pick 7362160f\n>\n> While at it, also fix a bug: currently, we use a commit-id-shaped\n> buffer to store the word after \"pick\" in '.git/sequencer/todo'.  This\n> is both wasteful and wrong because it places an artificial limit on\n> the line length.  Eliminate the need for the buffer altogether, and\n> add a test demonstrating this.\n>\n> [jc: simplify parsing]\n\nReading the above does not make it at all obvious that I should want\nto apply this patch because otherwise my prankster friend can cause my\ncopy of git to crash or run arbitrary code by putting a long commit\nname in .git/sequencer/todo in our NFS-mounted shared checkout.\n\n(Yes, I know there are other problems with such a setup, especially if\n.git/hooks or .git/config is writable by untrusted people.  So it is\nnot actually a security bug, but a robustness one.)\n"},{"id":"180458","messageId":"20111207070627.GD11737@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"1323239877-24101-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/5] revert: simplify getting commit subject in format_todo()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-07T07:06:27Z","receivedAt":"2011-12-07T07:06:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> format_todo() calls get_message(), but uses only the subject line of\n> the commit message.  Save work and unnecessary memory allocations by\n> using find_commit_subject() instead.  Also, remove the spurious check\n> on cur->item: the previous patch makes sure that instruction sheets\n> with missing commit subjects are parsable.\n\nSo, what effect will this patch actually have?  Is it an optimization\nwith no other observable effect?\n"},{"id":"180461","messageId":"20111207074501.GE11737@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"1323239877-24101-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/5] revert: allow mixed pick and revert instructions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-07T07:45:01Z","receivedAt":"2011-12-07T07:45:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Parse the instruction sheet in '.git/sequencer/todo' as a list of\n> (action, operand) pairs, instead of assuming that all instructions use\n> the same action.  Now you can do:\n>\n>   pick fdc0b12 picked\n>   revert 965fed4 anotherpick\n>\n> For cherry-pick and revert, this means that a 'git cherry-pick\n> --continue' can continue an ongoing revert operation and viceversa.\n\nGood idea.\n\n> This patch lays the foundation for extending the parser to support\n> more actions so 'git rebase -i' can reuse this machinery in the\n> future.  While at it, also improve the error messages reported by the\n> insn sheet parser.\n\nI don't know what this part is about...\n\n[...]\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\nRemoving the enum, to expose it in sequencer.h.  What does this have\nto do with the stated goal of the patch?\n\n>  enum replay_subcommand {\n>  \tREPLAY_NONE,\n>  \tREPLAY_REMOVE_STATE,\n> @@ -73,14 +72,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[... lots of similar changes snipped ...]\n> @@ -467,7 +466,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\nOk, now we're at the title feature.\n\nPassing \"action\" as a separate parameter in addition to \"opts\" makes\nsense since it can vary from item to item on the todo list.  It\nunfortunately makes reviewers' lives hard since it is easy to miss\nadditional unconverted uses of opts->action in the function and\ncompilers will not warn about that.\n\n[...]\n>\t\tif (index_differs_from(\"HEAD\", 0))\n>\t\t\treturn error_dirty_index(opts);\n[...]\n>\tif (parent && parse_commit(parent) < 0)\n>\t\t/* TRANSLATORS: The first %s will be \"revert\" or\n>\t\t   \"cherry-pick\", the second %s a SHA1 */\n>\t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n>\t\t\taction_name(opts), sha1_to_hex(parent->object.sha1));\n[...]\n>\t\tres = do_recursive_merge(base, next, base_label, next_label,\n>\t\t\t\t\t head, &msgbuf, opts);\n\nMakes sense.  This is the command name passed on the command line, not\nthe action name from the todo list, so it is left alone.\n\n[...]\n> @@ -607,13 +607,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n[...]\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\n... and that's the last use of \"action\" in this function on the\ncurrent master branch.  Good.\n\n[... more distracting s/REVERT/REPLAY_REVERT/ stuff snipped...]\n> @@ -683,49 +683,55 @@ static void read_and_refresh_cache(struct replay_opts *opts)\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\nLooks good.\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,\n> +\t\t\tstruct replay_insn_list *item, int lineno)\n[...]\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(_(\"Line %d: Unrecognized action: %.*s\"),\n> +\t\t\tlineno, (int)len, bol);\n> +\t}\n\nA new error message when an insn line does not start with \"pick\" or\n\"revert\".  Looks like a good change, but what does it have to do with\nthe subject of the patch?\n\n[...]\n> @@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n[...]\n>  \tstatus = get_sha1(bol, commit_sha1);\n>  \t*end_of_object_name = saved;\n[...]\n>  \tif (status < 0)\n> -\t\treturn NULL;\n> +\t\treturn error(_(\"Line %d: Malformed object name: %s\"), lineno, bol);\n\n(Not about this patch, but an earlier one)  What is this idiom about?\nWhy not\n\n\tif (get_sha1(bol, commit_sha1))\n\t\treturn error(_(...));\n\n?\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\nNice to see.\n\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\nStyle: elsewhere in this file, initializers look like \"{ 0, NULL, NULL }\"\n(with space between the braces and values).\n\n[...]\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '\n\nThanks for writing these.\n\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\nStops in \"picked\", asking user to resolve conflicts.\n\n> +\techo \"c\" >foo &&\n> +\tgit add foo &&\n> +\tgit commit &&\n\nUser resolves conflicts, as in the previous test.\n\n> +\tgit revert --continue &&\n\nAnd asks to continue.\n\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\nI wonder if there is some simpler way to check the result of resuming\na cherry-pick sequence, though that doesn't have much to do with the\npatch.  I guess I'd find\n\n\techo d >expect &&\n\t... &&\n\t# index is clean\n\tgit update-index --refresh &&\n\tgit rev-list HEAD >commits &&\n\ttest_line_count = 4 commits &&\n\ttest_cmp expect foo &&\n\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n\na more convincing demonstration that the cherry-pick finished without\nincident.\n\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\nSame setup.\n\n> +\toldsha=`git rev-parse --short HEAD~1` &&\n> +\techo \"revert $oldsha unrelatedpick\" >>.git/sequencer/todo &&\n\nAdding to the insn sheet.  Makes sense.\n\nSumming up: I quite like the idea of this patch, but I would be a lot\nhappier if changes unrelated to its goal were split off to be\nconsidered separately.\n\n> Acked-by: Jonathan Nieder <jrnider@gmail.com>\n\nNo, not acked.\n\nThanks,\nJonathan\n"},{"id":"180464","messageId":"20111207080944.GF11737@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"1323239877-24101-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 5/5] revert: simplify communicating command-line arguments","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-07T08:09:44Z","receivedAt":"2011-12-07T08:09:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> From: Jonathan Nieder <jrnieder@gmail.com>\n>\n> Currently, command-line arguments are communicated using (argc, argv)\n> until a prepare_revs() turns it into a terse structure.  However,\n> since we plan to expose the cherry-picking machinery through a public\n> API in the future, we want callers to be able to call in with a\n> filled-in structure.  For the revert builtin, this means that the\n> chief argument parser, parse_args(), should parse into such a\n> structure.  Make this change.\n\nFine.  I guess it can be said more clearly while emphasizing public\ninterfaces over implementation details:\n\n\tSince the \"multiple cherry-pick\" feature was introduced (commit\n\t7e2bfd3f, 2010-07-02), the pick/revert machinery has kept track\n\tof the set of commits to be cherry-picked or reverted using\n\tcommit_argc and commit_argv variables storing the relevant\n\tcommand-line parameters.\n\n\tFuture callers as other commands are built in (am, rebase,\n\tsequencer) may find it easier to pass rev-list options to this\n\tmachinery in already-parsed form, though.  So teach\n\tcmd_cherry_pick and cmd_revert to parse the rev-list arguments\n\tin advance and pass the commit set to pick_revisions() as a\n\tvalue of type \"struct rev_info\".\n\n[...]\n> @@ -222,7 +220,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\nLooks good.\n\nTests?  What happens if I try \"git cherry-pick --all\"?  How about \"git\ncherry-pick HEAD..HEAD\"?\n\nFWIW, with the commit message clarified and with or without new tests,\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"180467","messageId":"20111207081734.GG11737@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"1323239877-24101-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-07T08:17:34Z","receivedAt":"2011-12-07T08:17:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions\n> ($gname/186365) implemented.\n\nThanks for this.  I am worried that some of my comments in previous\nreviews (especially about change descriptions) were not addressed,\nwhich could mean one of a few things:\n\n - you didn't notice them\n - they were wrong\n - you noticed them and they were describing real problems, but it\n   wasn't worth the time to fix them\n\nFine.  But I would like to know which case they fell into, so I can\nlearn in order to do a better job reviewing the future and know my\ntime is well spent.\n\nThe series makes various changes, all essentially good, which leaves\nme all the more motivated to figure out how to get this sort of thing\nin smoothly without making life difficult for people in the future\ntracking down regressions.\n\nCiao,\nJonathan\n"},{"id":"180469","messageId":"CALkWK0k+GF9qjwYd_TzA3o8FqAH6G+cxREZ5r9x8E2G5k-mmmg@mail.gmail.com","threadId":"29103","inReplyTo":"20111207081734.GG11737@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-07T08:26:43Z","receivedAt":"2011-12-07T08:26:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>\n>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions\n>> ($gname/186365) implemented.\n>\n> Thanks for this.  I am worried that some of my comments in previous\n> reviews (especially about change descriptions) were not addressed,\n> which could mean one of a few things:\n>\n>  - you didn't notice them\n>  - they were wrong\n>  - you noticed them and they were describing real problems, but it\n>   wasn't worth the time to fix them\n>\n> Fine.  But I would like to know which case they fell into, so I can\n> learn in order to do a better job reviewing the future and know my\n> time is well spent.\n\nI suppose they were a combination of all three.  I'm sorry- I'll try\nto be more careful and communicative in the future.\n\nThanks for this round of reviews!\n\n-- Ram\n"},{"id":"180470","messageId":"20111207082822.GH11737@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"20111207081734.GG11737@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-07T08:28:22Z","receivedAt":"2011-12-07T08:28:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Fine.  But I would like to know which case they fell into, so I can\n> learn in order to do a better job reviewing the future and know my\n                                             ^in\n> time is well spent.\n\nSorry for the nonsense. :)  And now that I look back over previous\nrevisions of the patches, I see that I _hadn't_ clearly complained\nabout the same aspects of these particular commit messages before.\nSo what am I talking about here?\n\nI guess it is just a pattern: commit messages I have looked at in the\nsequencer series lately seem to focus too much on implementation\ndetails and not enough on the \"big picture\" of what the user or\ninternal API user will experience.  One symptom is that I get lost in\nreading the commit message without looking at the patch.  Another\nsymptom is that the commit messages tend to mention the particular\n(private) functions that were changed, instead of the more prominent\n(often public) callers that the reader might have cause to call.\n\nHoping that clarifies a little,\nJonathan\n"},{"id":"180476","messageId":"20111207095636.GA13374@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"CALkWK0k+GF9qjwYd_TzA3o8FqAH6G+cxREZ5r9x8E2G5k-mmmg@mail.gmail.com","subject":"Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-07T09:56:36Z","receivedAt":"2011-12-07T09:56:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> I suppose they were a combination of all three.  I'm sorry- I'll try\n> to be more careful and communicative in the future.\n\nHm, that is not exactly what I was going for, either.  First, I didn't\nmean to harp on anything in the past --- it was a request regarding\nthis current review.  And on the other hand, I am not sure it is a\nmatter of you being uncareful or uncommunicative, exactly.  It's more\nthat _I_ am doing a poor job of clearly conveying what Junio described\nas easier to learn on one's own at [1].\n\nTo put it more concretely:\n\nI am going to be stubborn.  I will not ack a patch unless I can\nimagine myself, years from now, after running into a bug in a related\narea, not being confused by it.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/186402/focus=186415\n"},{"id":"180607","messageId":"7vpqfyj47m.fsf@alter.siamese.dyndns.org","threadId":"29103","inReplyTo":"20111207081734.GG11737@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-08T18:41:49Z","receivedAt":"2011-12-08T18:41:49Z","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> Ramkumar Ramachandra wrote:\n>\n>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions\n>> ($gname/186365) implemented.\n>\n> Thanks for this.  I am worried that some of my comments in previous\n> reviews (especially about change descriptions) were not addressed,\n> which could mean one of a few things:\n>\n>  - you didn't notice them\n>  - they were wrong\n>  - you noticed them and they were describing real problems, but it\n>    wasn't worth the time to fix them\n>\n> Fine.  But I would like to know which case they fell into, so I can\n> learn in order to do a better job reviewing the future and know my\n> time is well spent.\n\nAnother thing I often notice after raising an issue during a review (this\nis not limited to Ram) is that I do so in the form of a question \"why is\nthis function done this way?\" expecting either \"ah, doing it that way\ninstead is simpler and easier to read\" or \"because of such and such, which\nyou may have missed. after all this is a tricky codepath with this and\nthat constraints\" as a response. The former should and often does result\nin an update of the code in the re-rolled series, but the latter often\nresults in a frustrating nothing in a re-roll, when the fact that a\nreviewer needed to ask the question was a sure sign that the code needs\nexplanation either in the log message or in-code comment. \n\n> The series makes various changes, all essentially good, which leaves\n> me all the more motivated to figure out how to get this sort of thing\n> in smoothly without making life difficult for people in the future\n> tracking down regressions.\n\nI looked the series over with your comments and tend to agree with many of\nthem. Perhaps I should wait for another round before picking it up again\n(I've already dropped the old series I kept before 1.7.8).\n"},{"id":"180630","messageId":"CALkWK0=4Mzu57t3_CCTpbWrTFfHu3aF=5aNv-NqcmQ6BMG2tSg@mail.gmail.com","threadId":"29103","inReplyTo":"20111207074501.GE11737@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 4/5] revert: allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T05:34:15Z","receivedAt":"2011-12-09T05:34:15Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>\n>> This patch lays the foundation for extending the parser to support\n>> more actions so 'git rebase -i' can reuse this machinery in the\n>> future.  While at it, also improve the error messages reported by the\n>> insn sheet parser.\n>\n> I don't know what this part is about...\n\nI'll separate out these changes in the re-roll.  Thanks.\n\n>> @@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n> [...]\n>>       status = get_sha1(bol, commit_sha1);\n>>       *end_of_object_name = saved;\n> [...]\n>>       if (status < 0)\n>> -             return NULL;\n>> +             return error(_(\"Line %d: Malformed object name: %s\"), lineno, bol);\n>\n> (Not about this patch, but an earlier one)  What is this idiom about?\n> Why not\n>\n>        if (get_sha1(bol, commit_sha1))\n>                return error(_(...));\n>\n> ?\n\nI'm first detecting the end of the object name and terminating it with\na '\\0', before using using get_sha1() to read out the object name.\nThen, I restore the instruction sheet to the original state by\nrestoring the character from 'saved'.  If I use the idiom you\nsuggested, I'll leave midway after editing the instruction sheet, and\nthis is undesirable.\n\n-- Ram\n"},{"id":"180631","messageId":"CALkWK0nkPB5WptJ9nSkSOif5_0R_f39Dg_HR3Rmg02hG_4Q1Tg@mail.gmail.com","threadId":"29103","inReplyTo":"20111207070223.GC11737@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T05:57:18Z","receivedAt":"2011-12-09T05:57:18Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>\n>> [...]\n>> While at it, also fix a bug: currently, we use a commit-id-shaped\n>> buffer to store the word after \"pick\" in '.git/sequencer/todo'.  This\n>> is both wasteful and wrong because it places an artificial limit on\n>> the line length.  Eliminate the need for the buffer altogether, and\n>> add a test demonstrating this.\n>\n> Reading the above does not make it at all obvious that I should want\n> to apply this patch because otherwise my prankster friend can cause my\n> copy of git to crash or run arbitrary code by putting a long commit\n\nWorking backwards:\nget_sha1() is what will finally misbehave: how?  It uses strlen() and\nlet's assume that the number returned by it is too big to fit in a\nsize_t.  Surely, this means that we should only use get_sha1() on\nsomething whose length is bounded.  So, do we ever try to get to the\nend of the line?  Yes!  Let's assume that the problem starts when\nend_of_object_name calls strcspn which returns something too big to\nfit in a size_t.  From the manpage it has no standard way of reporting\nfailure.  I'm not sure what to do; I think I have two choices:\n1. Implement the strscpn() using two strchrnul() calls.\n2. Drop this patch and use strbuf to replace the fixed-size buffer.\n\nI think I'll go with the second option.  What do you think?\n\n-- Ram\n"},{"id":"180632","messageId":"CALkWK0mEE_v_Frm-N0ErsUY4vJ8rX1Fi1Wx_70a72j9EY+mmCA@mail.gmail.com","threadId":"29103","inReplyTo":"CALkWK0nkPB5WptJ9nSkSOif5_0R_f39Dg_HR3Rmg02hG_4Q1Tg@mail.gmail.com","subject":"Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T06:12:35Z","receivedAt":"2011-12-09T06:12:35Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nRamkumar Ramachandra wrote:\n> 1. Implement the strscpn() using two strchrnul() calls.\n> 2. Drop this patch and use strbuf to replace the fixed-size buffer.\n>\n> I think I'll go with the second option.  What do you think?\n> [...]\n\n\nEr, I have an 'eol' which I'm not using.  Making sure that 'eol' is\nvalid is good enough to avoid unexpected failures: if strchrnul() is\nable to find a '\\n', strcspn() should have no trouble finding either a\n' ' or '\\n' (whichever comes first).\n\nSorry for the nonsense.\n\n-- Ram\n"},{"id":"180691","messageId":"20111209193527.GD20913@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"CALkWK0nkPB5WptJ9nSkSOif5_0R_f39Dg_HR3Rmg02hG_4Q1Tg@mail.gmail.com","subject":"Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T19:35:28Z","receivedAt":"2011-12-09T19:35:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Jonathan Nieder wrote:\n>> Ramkumar Ramachandra wrote:\n\n>>> [...]\n>>> While at it, also fix a bug: currently, we use a commit-id-shaped\n>>> buffer to store the word after \"pick\" in '.git/sequencer/todo'.  This\n>>> is both wasteful and wrong because it places an artificial limit on\n>>> the line length.  Eliminate the need for the buffer altogether, and\n>>> add a test demonstrating this.\n>>\n>> Reading the above does not make it at all obvious that I should want\n>> to apply this patch because otherwise my prankster friend can cause my\n>> copy of git to crash or run arbitrary code by putting a long commit\n>\n> Working backwards:\n> get_sha1() is what will finally misbehave: how?\n\nNot sure what you're talking about.  I was saying that any commit that\ngoes from \"git can segfault in such-and-such circumstance\" to \"git no\nlonger segfaults in that circumstance\" should loudly proclaim so!\nOtherwise, when my script is causing git to segfault, I will not know\nwhich commits to point my risk-averse sysadmin to when advocating\nupgrading our copy of git.\n"},{"id":"180693","messageId":"CALkWK0ki1r5AqYb8qyGHUNupTAhCa2TKwVgkrCpLr+zo_pCy9g@mail.gmail.com","threadId":"29103","inReplyTo":"20111209193527.GD20913@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T19:44:50Z","receivedAt":"2011-12-09T19:44:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>> Jonathan Nieder wrote:\n>>> Ramkumar Ramachandra wrote:\n>\n>>>> [...]\n>>>> While at it, also fix a bug: currently, we use a commit-id-shaped\n>>>> buffer to store the word after \"pick\" in '.git/sequencer/todo'.  This\n>>>> is both wasteful and wrong because it places an artificial limit on\n>>>> the line length.  Eliminate the need for the buffer altogether, and\n>>>> add a test demonstrating this.\n>>>\n>>> Reading the above does not make it at all obvious that I should want\n>>> to apply this patch because otherwise my prankster friend can cause my\n>>> copy of git to crash or run arbitrary code by putting a long commit\n>>\n>> Working backwards:\n>> get_sha1() is what will finally misbehave: how?\n>\n> Not sure what you're talking about.  I was saying that any commit that\n> goes from \"git can segfault in such-and-such circumstance\" to \"git no\n> longer segfaults in that circumstance\" should loudly proclaim so!\n\nThat's what I'm confused about: how can I make git misbehave (let\nalone segfault) after applying this commit?  In the previous message,\nI was trying to work backwards to figure that out -- I clearly didn't\nget anywhere.  In other words: what happens if an overly long (a\nlength that doesn't fit into a size_t) line is present?  How does\nstrchrnul() behave?  How do we work around this without imposing some\nkind of artificial hard limit?\n\nThanks.\n\n-- Ram\n"},{"id":"180696","messageId":"20111209195042.GG20913@elie.hsd1.il.comcast.net","threadId":"29103","inReplyTo":"CALkWK0ki1r5AqYb8qyGHUNupTAhCa2TKwVgkrCpLr+zo_pCy9g@mail.gmail.com","subject":"Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-09T19:50:42Z","receivedAt":"2011-12-09T19:50:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Jonathan Nieder wrote:\n\n>> Not sure what you're talking about.  I was saying that any commit that\n>> goes from \"git can segfault in such-and-such circumstance\" to \"git no\n>> longer segfaults in that circumstance\" should loudly proclaim so!\n>\n> That's what I'm confused about: how can I make git misbehave (let\n> alone segfault) after applying this commit?\n\nYou can make git misbehave before applying the commit.  Applying the\npatch fixes it.  Unfortunately the patch description does not actually\nsay so, and at least my small mind is not able to read between the\nlines in a hurry to deduce it.\n\nSorry for the lack of clarity,\nJonathan\n"},{"id":"180720","messageId":"CALkWK0=vL8MakgaKNkDWLGHp1d_48GP5KKBGSSHkYG3ygB_rQg@mail.gmail.com","threadId":"29103","inReplyTo":"20111209195042.GG20913@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-09T22:30:33Z","receivedAt":"2011-12-09T22:30:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hey,\n\nJonathan Nieder wrote:\n> You can make git misbehave before applying the commit.\n\nRight, it's all in the discussion surrounding $gmane/184031.  I\ncouldn't recall because it was so long ago :\\\n\nThanks.\n\n-- Ram\n"}]}