{"thread":{"id":"28092","subject":"[PATCH v2 0/7] Generalized sequencer foundations","startedAt":"2011-08-14T08:33:02Z","lastAt":"2011-08-19T09:08:30Z","messageCount":43,"participants":["Ramkumar Ramachandra","Jonathan Nieder","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"173474","messageId":"1313310789-10216-1-git-send-email-artagnon@gmail.com","threadId":"28092","inReplyTo":null,"subject":"[PATCH v2 0/7] Generalized sequencer foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T08:33:02Z","receivedAt":"2011-08-14T08:33:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThis is the second iternation prepared mainly in response to\nJonathan's reviews.  It's ready for inclusion (or very nearly so) now.\nI'll start working on more features as soon as I'm confident that this\nseries doesn't need to be rewritten.\n\nThanks.\n\n-- Ram\n\nRamkumar Ramachandra (7):\n  revert: Free memory after get_message call\n  revert: Fix buffer overflow in insn sheet parser\n  revert: Make commit descriptions in insn sheet optional\n  revert: Allow mixed pick and revert instructions\n  revert: Make the argument parser responsible for setup_revisions\n  sequencer: Expose API to cherry-picking machinery\n  sequencer: Remove sequencer state after final commit\n\n builtin/commit.c                   |    8 +\n builtin/revert.c                   |  858 +-----------------------------------\n sequencer.c                        |  806 +++++++++++++++++++++++++++++++++-\n sequencer.h                        |   36 ++\n t/t3032-merge-recursive-options.sh |    2 +\n t/t3510-cherry-pick-sequence.sh    |   87 ++++-\n 6 files changed, 956 insertions(+), 841 deletions(-)\n\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"173475","messageId":"1313310789-10216-2-git-send-email-artagnon@gmail.com","threadId":"28092","inReplyTo":"1313310789-10216-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/7] revert: Free memory after get_message call","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T08:33:03Z","receivedAt":"2011-08-14T08:33:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The format_todo function leaks memory because it forgets to call\nfree_message after get_message.  It is a potentially big leak, because\nfresh memory is allocated to store the commit message message for each\ncommit.  Fix this.\n\nReported-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.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 8b452e8..8a35bfd 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -687,6 +687,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":"173476","messageId":"1313310789-10216-3-git-send-email-artagnon@gmail.com","threadId":"28092","inReplyTo":"1313310789-10216-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/7] revert: Fix buffer overflow in insn sheet parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T08:33:04Z","receivedAt":"2011-08-14T08:33:04Z","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 a037033b\n(revert: Introduce --continue to continue the operation, 2011-07-27).\n\nReported-by: Jonathan Nieder <jrnieder@gmail.com>\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c                |    2 +-\n t/t3510-cherry-pick-sequence.sh |   11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8a35bfd..8d29003 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -712,7 +712,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..6882acb 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -211,4 +211,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 \\1\\1\\1\\1\\1\\1\\1\\1\\2/\" .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":"173477","messageId":"1313310789-10216-4-git-send-email-artagnon@gmail.com","threadId":"28092","inReplyTo":"1313310789-10216-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/7] revert: Make commit descriptions in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T08:33:05Z","receivedAt":"2011-08-14T08:33:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Change the instruction sheet format subtly so that a description of\nthe commit after the object name is optional.  As a result, an\ninstruction sheet like this is now perfectly valid:\n\n  pick 35b0426\n  pick fbd5bbcbc2e\n  pick 7362160f\n\nSuggested-by: Jonathan Nieder <jrnieder@gmail.com>\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c                |   19 ++++++++-----------\n t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++\n 2 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8d29003..89b2bba 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -697,26 +697,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n \tunsigned char commit_sha1[20];\n \tchar sha1_abbrev[40];\n \tenum replay_action action;\n-\tint insn_len = 0;\n-\tchar *p, *q;\n+\tconst char *p, *q;\n \n+\tp = start;\n \tif (!prefixcmp(start, \"pick \")) {\n \t\taction = CHERRY_PICK;\n-\t\tinsn_len = strlen(\"pick\");\n-\t\tp = start + insn_len + 1;\n+\t\tp += strlen(\"pick \");\n \t} else if (!prefixcmp(start, \"revert \")) {\n \t\taction = REVERT;\n-\t\tinsn_len = strlen(\"revert\");\n-\t\tp = start + insn_len + 1;\n+\t\tp += strlen(\"revert \");\n \t} else\n \t\treturn NULL;\n \n-\tq = strchr(p, ' ');\n-\tif (!q || q - p + 1 > sizeof(sha1_abbrev))\n+\tq = p + strcspn(p, \" \\n\");\n+\tif (q - p + 1 > sizeof(sha1_abbrev))\n \t\treturn NULL;\n-\tq++;\n-\n-\tstrlcpy(sha1_abbrev, p, q - p);\n+\tmemcpy(sha1_abbrev, p, q - p);\n+\tsha1_abbrev[q - p] = '\\0';\n \n \t/*\n \t * Verify that the action matches up with the one in\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 6882acb..734ee12 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -222,4 +222,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":"173478","messageId":"1313310789-10216-5-git-send-email-artagnon@gmail.com","threadId":"28092","inReplyTo":"1313310789-10216-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/7] revert: Allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T08:33:06Z","receivedAt":"2011-08-14T08:33:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Parse the instruction list in '.git/sequencer/todo' as a list of\n(action, operand) pairs, instead of assuming all instructions use the\nsame action.  So now, you can do:\n\n  pick fdc0b12 picked\n  revert 965fed4 anotherpick\n\nFor cherry-pick and revert, this means that a 'git cherry-pick\n--continue' can continue an ongoing revert operation and viceversa.\nThis patch lays the foundation for extending the parser to support\nmore actions so 'git rebase -i' can reuse this machinery in the\nfuture.  While at it, also improve the error messages reported by the\ninsn sheet parser.\n\nHelped-by: Jonathan Nieder <jrnider@gmail.com>\nAcked-by: Jonathan Nieder <jrnider@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c                |  137 ++++++++++++++++++++-------------------\n sequencer.h                     |    8 ++\n t/t3510-cherry-pick-sequence.sh |   58 ++++++++++++++++\n 3 files changed, 135 insertions(+), 68 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 89b2bba..8faca07 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-enum replay_action { REVERT, CHERRY_PICK };\n enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };\n \n struct replay_opts {\n@@ -68,14 +67,14 @@ struct replay_opts {\n \n static const char *action_name(const struct replay_opts *opts)\n {\n-\treturn opts->action == REVERT ? \"revert\" : \"cherry-pick\";\n+\treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n }\n \n static char *get_encoding(const char *message);\n \n static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n {\n-\treturn opts->action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n }\n \n static int option_parse_x(const struct option *opt,\n@@ -154,7 +153,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t};\n \n-\tif (opts->action == CHERRY_PICK) {\n+\tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n@@ -353,7 +352,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@@ -457,7 +456,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@@ -532,7 +532,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@@ -575,7 +575,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@@ -594,7 +594,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@@ -618,7 +618,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@@ -664,27 +664,29 @@ static void read_and_refresh_cache(struct replay_opts *opts)\n  *     assert(commit_list_count(list) == 2);\n  *     return list;\n  */\n-struct commit_list **commit_list_append(struct commit *commit,\n-\t\t\t\t\tstruct commit_list **next)\n+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-\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+\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\tif (get_message(cur->item, &msg))\n+\t\tconst char *sha1_abbrev, *action_str;\n+\t\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\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\tif (get_message(cur->operand, &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@@ -692,59 +694,58 @@ 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 int parse_insn_line(char *start, struct replay_insn_list *item)\n {\n \tunsigned char commit_sha1[20];\n \tchar sha1_abbrev[40];\n-\tenum replay_action action;\n \tconst char *p, *q;\n \n \tp = start;\n \tif (!prefixcmp(start, \"pick \")) {\n-\t\taction = CHERRY_PICK;\n+\t\titem->action = REPLAY_PICK;\n \t\tp += strlen(\"pick \");\n \t} else if (!prefixcmp(start, \"revert \")) {\n-\t\taction = REVERT;\n+\t\titem->action = REPLAY_REVERT;\n \t\tp += strlen(\"revert \");\n-\t} else\n-\t\treturn NULL;\n+\t} else {\n+\t\tint len = strchrnul(p, '\\n') - p;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"Unrecognized action: %.*s\"), len, p);\n+\t}\n \n \tq = p + strcspn(p, \" \\n\");\n-\tif (q - p + 1 > sizeof(sha1_abbrev))\n-\t\treturn NULL;\n+\tif (q - p + 1 > sizeof(sha1_abbrev)) {\n+\t\tint len = q - p;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"Object name too large: %.*s\"), len, p);\n+\t}\n \tmemcpy(sha1_abbrev, p, q - p);\n \tsha1_abbrev[q - p] = '\\0';\n \n-\t/*\n-\t * Verify that the action matches up with the one in\n-\t * opts; we don't support arbitrary instructions\n-\t */\n-\tif (action != opts->action) {\n-\t\tconst char *action_str;\n-\t\taction_str = action == REVERT ? \"revert\" : \"cherry-pick\";\n-\t\terror(_(\"Cannot %s during a %s\"), action_str, action_name(opts));\n-\t\treturn NULL;\n-\t}\n-\n \tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n-\t\treturn NULL;\n+\t\treturn error(_(\"Malformed object name: %s\"), sha1_abbrev);\n \n-\treturn lookup_commit_reference(commit_sha1);\n+\titem->operand = lookup_commit_reference(commit_sha1);\n+\tif (!item->operand)\n+\t\treturn error(_(\"Not a valid commit: %s\"), sha1_abbrev);\n+\n+\titem->next = NULL;\n+\treturn 0;\n }\n \n-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n-\t\t\tstruct replay_opts *opts)\n+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n {\n-\tstruct commit_list **next = todo_list;\n-\tstruct commit *commit;\n+\tstruct replay_insn_list **next = todo_list;\n+\tstruct replay_insn_list item = {0, NULL, NULL};\n \tchar *p = buf;\n \tint i;\n \n \tfor (i = 1; *p; i++) {\n-\t\tcommit = parse_insn_line(p, opts);\n-\t\tif (!commit)\n-\t\t\treturn error(_(\"Could not parse line %d.\"), i);\n-\t\tnext = commit_list_append(commit, next);\n+\t\tif (parse_insn_line(p, &item) < 0)\n+\t\t\treturn error(_(\"on line %d.\"), i);\n+\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n \t\tp = strchrnul(p, '\\n');\n \t\tif (*p)\n \t\t\tp++;\n@@ -754,8 +755,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@@ -771,7 +771,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@@ -820,18 +820,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@@ -860,7 +860,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@@ -868,7 +868,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@@ -912,9 +912,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@@ -924,8 +925,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@@ -950,7 +951,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@@ -967,7 +968,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@@ -987,7 +988,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@@ -1007,7 +1008,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@@ -1022,7 +1023,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 734ee12..c284c96 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -236,4 +236,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":"173479","messageId":"1313310789-10216-6-git-send-email-artagnon@gmail.com","threadId":"28092","inReplyTo":"1313310789-10216-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/7] revert: Make the argument parser responsible for setup_revisions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T08:33:07Z","receivedAt":"2011-08-14T08:33:07Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Currently, prepare_revs calls setup_revisions with the (argc, argv) in\nthe opts structure filled in by parse_args.  As a result, prepare_revs\nhas to take up the responsibility of erroring out and printing usage\ninformation when (argc, argv) is malformed.  Since parse_args is doing\nthis for other kinds of parse errors anyway, give it the task of\ncalling setup_revisions as well.\n\nBased-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.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 8faca07..e8bdeea 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -54,13 +54,14 @@ struct replay_opts {\n \tint allow_rerere_auto;\n \n \tint mainline;\n-\tint commit_argc;\n-\tconst char **commit_argv;\n \n \t/* Merge strategy */\n \tconst char *strategy;\n \tconst char **xopts;\n \tsize_t xopts_nr, xopts_alloc;\n+\n+\t/* Only used by REPLAY_NONE */\n+\tstruct rev_info *revs;\n };\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n@@ -163,9 +164,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tdie(_(\"program error\"));\n \t}\n \n-\topts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,\n-\t\t\t\t\tPARSE_OPT_KEEP_ARGV0 |\n-\t\t\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n+\targc = parse_options(argc, argv, NULL, options, usage_str,\n+\t\t\tPARSE_OPT_KEEP_ARGV0 |\n+\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n \n \t/* Check for incompatible subcommands */\n \tverify_opt_mutually_compatible(me,\n@@ -200,9 +201,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\tNULL);\n \t}\n \n-\telse if (opts->commit_argc < 2)\n-\t\tusage_with_options(usage_str, options);\n-\n \tif (opts->allow_ff)\n \t\tverify_opt_compatible(me, \"--ff\",\n \t\t\t\t\"--signoff\", opts->signoff,\n@@ -210,7 +208,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"-x\", opts->record_origin,\n \t\t\t\t\"--edit\", opts->edit,\n \t\t\t\tNULL);\n-\topts->commit_argv = argv;\n+\n+\tif (opts->subcommand == REPLAY_NONE) {\n+\t\topts->revs = xmalloc(sizeof(*opts->revs));\n+\t\tinit_revisions(opts->revs, NULL);\n+\t\topts->revs->no_walk = 1;\n+\t\tif (argc < 2)\n+\t\t\tusage_with_options(usage_str, options);\n+\t\targc = setup_revisions(argc, argv, opts->revs, NULL);\n+\t} else\n+\t\topts->revs = NULL;\n+\n+\t/* Forbid stray command-line arguments */\n+\tif (argc > 1)\n+\t\tusage_with_options(usage_str, options);\n }\n \n struct commit_message {\n@@ -612,23 +623,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@@ -823,14 +826,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@@ -954,6 +956,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":"173480","messageId":"1313310789-10216-7-git-send-email-artagnon@gmail.com","threadId":"28092","inReplyTo":"1313310789-10216-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 6/7] sequencer: Expose API to cherry-picking machinery","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T08:33:08Z","receivedAt":"2011-08-14T08:33:08Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Move code from builtin/revert.c to sequencer.c and expose a public API\nwithout making any functional changes.  Although it is useful only to\nexisting callers of cherry-pick and revert now, this patch lays the\nfoundation for future expansion.\n\nHelped-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c |  824 +-----------------------------------------------------\n sequencer.c      |  805 ++++++++++++++++++++++++++++++++++++++++++++++++++++-\n sequencer.h      |   27 ++\n 3 files changed, 832 insertions(+), 824 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e8bdeea..7ddfe84 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -1,19 +1,9 @@\n #include \"cache.h\"\n #include \"builtin.h\"\n-#include \"object.h\"\n-#include \"commit.h\"\n-#include \"tag.h\"\n-#include \"run-command.h\"\n-#include \"exec_cmd.h\"\n-#include \"utf8.h\"\n #include \"parse-options.h\"\n-#include \"cache-tree.h\"\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"rerere.h\"\n-#include \"merge-recursive.h\"\n-#include \"refs.h\"\n-#include \"dir.h\"\n #include \"sequencer.h\"\n \n /*\n@@ -39,40 +29,11 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };\n-\n-struct replay_opts {\n-\tenum replay_action action;\n-\tenum replay_subcommand subcommand;\n-\n-\t/* Boolean options */\n-\tint edit;\n-\tint record_origin;\n-\tint no_commit;\n-\tint signoff;\n-\tint allow_ff;\n-\tint allow_rerere_auto;\n-\n-\tint mainline;\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-\n static const char *action_name(const struct replay_opts *opts)\n {\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 == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n@@ -224,787 +185,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tusage_with_options(usage_str, options);\n }\n \n-struct commit_message {\n-\tchar *parent_label;\n-\tconst char *label;\n-\tconst char *subject;\n-\tchar *reencoded_message;\n-\tconst char *message;\n-};\n-\n-static int get_message(struct commit *commit, struct commit_message *out)\n-{\n-\tconst char *encoding;\n-\tconst char *abbrev, *subject;\n-\tint abbrev_len, subject_len;\n-\tchar *q;\n-\n-\tif (!commit->buffer)\n-\t\treturn -1;\n-\tencoding = get_encoding(commit->buffer);\n-\tif (!encoding)\n-\t\tencoding = \"UTF-8\";\n-\tif (!git_commit_encoding)\n-\t\tgit_commit_encoding = \"UTF-8\";\n-\n-\tout->reencoded_message = NULL;\n-\tout->message = commit->buffer;\n-\tif (strcmp(encoding, git_commit_encoding))\n-\t\tout->reencoded_message = reencode_string(commit->buffer,\n-\t\t\t\t\tgit_commit_encoding, encoding);\n-\tif (out->reencoded_message)\n-\t\tout->message = out->reencoded_message;\n-\n-\tabbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n-\tabbrev_len = strlen(abbrev);\n-\n-\tsubject_len = find_commit_subject(out->message, &subject);\n-\n-\tout->parent_label = xmalloc(strlen(\"parent of \") + abbrev_len +\n-\t\t\t      strlen(\"... \") + subject_len + 1);\n-\tq = out->parent_label;\n-\tq = mempcpy(q, \"parent of \", strlen(\"parent of \"));\n-\tout->label = q;\n-\tq = mempcpy(q, abbrev, abbrev_len);\n-\tq = mempcpy(q, \"... \", strlen(\"... \"));\n-\tout->subject = q;\n-\tq = mempcpy(q, subject, subject_len);\n-\t*q = '\\0';\n-\treturn 0;\n-}\n-\n-static void free_message(struct commit_message *msg)\n-{\n-\tfree(msg->parent_label);\n-\tfree(msg->reencoded_message);\n-}\n-\n-static char *get_encoding(const char *message)\n-{\n-\tconst char *p = message, *eol;\n-\n-\twhile (*p && *p != '\\n') {\n-\t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n-\t\t\t; /* do nothing */\n-\t\tif (!prefixcmp(p, \"encoding \")) {\n-\t\t\tchar *result = xmalloc(eol - 8 - p);\n-\t\t\tstrlcpy(result, p + 9, eol - 8 - p);\n-\t\t\treturn result;\n-\t\t}\n-\t\tp = eol;\n-\t\tif (*p == '\\n')\n-\t\t\tp++;\n-\t}\n-\treturn NULL;\n-}\n-\n-static void write_cherry_pick_head(struct commit *commit)\n-{\n-\tint fd;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\n-\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n-\n-\tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t  git_path(\"CHERRY_PICK_HEAD\"));\n-\tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n-\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"CHERRY_PICK_HEAD\"));\n-\tstrbuf_release(&buf);\n-}\n-\n-static void print_advice(void)\n-{\n-\tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n-\n-\tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n-\t\t/*\n-\t\t * A conflict has occured but the porcelain\n-\t\t * (typically rebase --interactive) wants to take care\n-\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n-\t\t */\n-\t\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-\t\treturn;\n-\t}\n-\n-\tadvise(\"after resolving the conflicts, mark the corrected paths\");\n-\tadvise(\"with 'git add <paths>' or 'git rm <paths>'\");\n-\tadvise(\"and commit the result with 'git commit'\");\n-}\n-\n-static void write_message(struct strbuf *msgbuf, const char *filename)\n-{\n-\tstatic struct lock_file msg_file;\n-\n-\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n-\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n-\tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s.\"), filename);\n-\tstrbuf_release(msgbuf);\n-\tif (commit_lock_file(&msg_file) < 0)\n-\t\tdie(_(\"Error wrapping up %s\"), filename);\n-}\n-\n-static struct tree *empty_tree(void)\n-{\n-\tstruct tree *tree = xcalloc(1, sizeof(struct tree));\n-\n-\ttree->object.parsed = 1;\n-\ttree->object.type = OBJ_TREE;\n-\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);\n-\treturn tree;\n-}\n-\n-static int error_dirty_index(struct replay_opts *opts)\n-{\n-\tif (read_cache_unmerged())\n-\t\treturn error_resolve_conflict(action_name(opts));\n-\n-\t/* Different translation strings for cherry-pick and revert */\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-\n-\tif (advice_commit_before_merge)\n-\t\tadvise(_(\"Commit your changes or stash them to proceed.\"));\n-\treturn -1;\n-}\n-\n-static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n-{\n-\tstruct ref_lock *ref_lock;\n-\n-\tread_cache();\n-\tif (checkout_fast_forward(from, to))\n-\t\texit(1); /* the callee should have complained already */\n-\tref_lock = lock_any_ref_for_update(\"HEAD\", from, 0);\n-\treturn write_ref_sha1(ref_lock, to, \"cherry-pick\");\n-}\n-\n-static int do_recursive_merge(struct commit *base, struct commit *next,\n-\t\t\t      const char *base_label, const char *next_label,\n-\t\t\t      unsigned char *head, struct strbuf *msgbuf,\n-\t\t\t      struct replay_opts *opts)\n-{\n-\tstruct merge_options o;\n-\tstruct tree *result, *next_tree, *base_tree, *head_tree;\n-\tint clean, index_fd;\n-\tconst char **xopt;\n-\tstatic struct lock_file index_lock;\n-\n-\tindex_fd = hold_locked_index(&index_lock, 1);\n-\n-\tread_cache();\n-\n-\tinit_merge_options(&o);\n-\to.ancestor = base ? base_label : \"(empty tree)\";\n-\to.branch1 = \"HEAD\";\n-\to.branch2 = next ? next_label : \"(empty tree)\";\n-\n-\thead_tree = parse_tree_indirect(head);\n-\tnext_tree = next ? next->tree : empty_tree();\n-\tbase_tree = base ? base->tree : empty_tree();\n-\n-\tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n-\t\tparse_merge_opt(&o, *xopt);\n-\n-\tclean = merge_trees(&o,\n-\t\t\t    head_tree,\n-\t\t\t    next_tree, base_tree, &result);\n-\n-\tif (active_cache_changed &&\n-\t    (write_cache(index_fd, active_cache, active_nr) ||\n-\t     commit_locked_index(&index_lock)))\n-\t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\tdie(_(\"%s: Unable to write new index file\"), action_name(opts));\n-\trollback_lock_file(&index_lock);\n-\n-\tif (!clean) {\n-\t\tint i;\n-\t\tstrbuf_addstr(msgbuf, \"\\nConflicts:\\n\\n\");\n-\t\tfor (i = 0; i < active_nr;) {\n-\t\t\tstruct cache_entry *ce = active_cache[i++];\n-\t\t\tif (ce_stage(ce)) {\n-\t\t\t\tstrbuf_addch(msgbuf, '\\t');\n-\t\t\t\tstrbuf_addstr(msgbuf, ce->name);\n-\t\t\t\tstrbuf_addch(msgbuf, '\\n');\n-\t\t\t\twhile (i < active_nr && !strcmp(ce->name,\n-\t\t\t\t\t\tactive_cache[i]->name))\n-\t\t\t\t\ti++;\n-\t\t\t}\n-\t\t}\n-\t}\n-\n-\treturn !clean;\n-}\n-\n-/*\n- * If we are cherry-pick, and if the merge did not result in\n- * hand-editing, we will hit this commit and inherit the original\n- * author date and name.\n- * If we are revert, or if our cherry-pick results in a hand merge,\n- * we had better say that the current user is responsible for that.\n- */\n-static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n-{\n-\t/* 6 is max possible length of our args array including NULL */\n-\tconst char *args[6];\n-\tint i = 0;\n-\n-\targs[i++] = \"commit\";\n-\targs[i++] = \"-n\";\n-\tif (opts->signoff)\n-\t\targs[i++] = \"-s\";\n-\tif (!opts->edit) {\n-\t\targs[i++] = \"-F\";\n-\t\targs[i++] = defmsg;\n-\t}\n-\targs[i] = NULL;\n-\n-\treturn run_command_v_opt(args, RUN_GIT_CMD);\n-}\n-\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-\tconst char *base_label, *next_label;\n-\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n-\tchar *defmsg = NULL;\n-\tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res;\n-\n-\tif (opts->no_commit) {\n-\t\t/*\n-\t\t * We do not intend to commit immediately.  We just want to\n-\t\t * merge the differences in, so let's compute the tree\n-\t\t * that represents the \"current\" state for merge-recursive\n-\t\t * to work on.\n-\t\t */\n-\t\tif (write_cache_as_tree(head, 0, NULL))\n-\t\t\tdie (_(\"Your index file is unmerged.\"));\n-\t} else {\n-\t\tif (get_sha1(\"HEAD\", head))\n-\t\t\treturn error(_(\"You do not have a valid HEAD\"));\n-\t\tif (index_differs_from(\"HEAD\", 0))\n-\t\t\treturn error_dirty_index(opts);\n-\t}\n-\tdiscard_cache();\n-\n-\tif (!commit->parents) {\n-\t\tparent = NULL;\n-\t}\n-\telse if (commit->parents->next) {\n-\t\t/* Reverting or cherry-picking a merge commit */\n-\t\tint cnt;\n-\t\tstruct commit_list *p;\n-\n-\t\tif (!opts->mainline)\n-\t\t\treturn error(_(\"Commit %s is a merge but no -m option was given.\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n-\n-\t\tfor (cnt = 1, p = commit->parents;\n-\t\t     cnt != opts->mainline && p;\n-\t\t     cnt++)\n-\t\t\tp = p->next;\n-\t\tif (cnt != opts->mainline || !p)\n-\t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1), opts->mainline);\n-\t\tparent = p->item;\n-\t} else if (0 < opts->mainline)\n-\t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n-\t\t\tsha1_to_hex(commit->object.sha1));\n-\telse\n-\t\tparent = commit->parents->item;\n-\n-\tif (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))\n-\t\treturn fast_forward_to(commit->object.sha1, head);\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-\tif (get_message(commit, &msg) != 0)\n-\t\treturn error(_(\"Cannot get commit message for %s\"),\n-\t\t\tsha1_to_hex(commit->object.sha1));\n-\n-\t/*\n-\t * \"commit\" is an existing commit.  We would want to apply\n-\t * the difference it introduces since its first parent \"prev\"\n-\t * on top of the current HEAD if we are cherry-pick.  Or the\n-\t * reverse of it if we are revert.\n-\t */\n-\n-\tdefmsg = git_pathdup(\"MERGE_MSG\");\n-\n-\tif (action == REPLAY_REVERT) {\n-\t\tbase = commit;\n-\t\tbase_label = msg.label;\n-\t\tnext = parent;\n-\t\tnext_label = msg.parent_label;\n-\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n-\t\tstrbuf_addstr(&msgbuf, msg.subject);\n-\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n-\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n-\n-\t\tif (commit->parents && commit->parents->next) {\n-\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n-\t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));\n-\t\t}\n-\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n-\t} else {\n-\t\tconst char *p;\n-\n-\t\tbase = parent;\n-\t\tbase_label = msg.parent_label;\n-\t\tnext = commit;\n-\t\tnext_label = msg.label;\n-\n-\t\t/*\n-\t\t * Append the commit log message to msgbuf; it starts\n-\t\t * after the tree, parent, author, committer\n-\t\t * information followed by \"\\n\\n\".\n-\t\t */\n-\t\tp = strstr(msg.message, \"\\n\\n\");\n-\t\tif (p) {\n-\t\t\tp += 2;\n-\t\t\tstrbuf_addstr(&msgbuf, p);\n-\t\t}\n-\n-\t\tif (opts->record_origin) {\n-\t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n-\t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n-\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n-\t\t}\n-\t\tif (!opts->no_commit)\n-\t\t\twrite_cherry_pick_head(commit);\n-\t}\n-\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-\t} else {\n-\t\tstruct commit_list *common = NULL;\n-\t\tstruct commit_list *remotes = NULL;\n-\n-\t\twrite_message(&msgbuf, defmsg);\n-\n-\t\tcommit_list_insert(base, &common);\n-\t\tcommit_list_insert(next, &remotes);\n-\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n-\t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n-\t\tfree_commit_list(common);\n-\t\tfree_commit_list(remotes);\n-\t}\n-\n-\tif (res) {\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-\t\t      msg.subject);\n-\t\tprint_advice();\n-\t\trerere(opts->allow_rerere_auto);\n-\t} else {\n-\t\tif (!opts->no_commit)\n-\t\t\tres = run_git_commit(defmsg, opts);\n-\t}\n-\n-\tfree_message(&msg);\n-\tfree(defmsg);\n-\n-\treturn res;\n-}\n-\n-static void prepare_revs(struct replay_opts *opts)\n-{\n-\tif (opts->action != REPLAY_REVERT)\n-\t\topts->revs->reverse ^= 1;\n-\n-\tif (prepare_revision_walk(opts->revs))\n-\t\tdie(_(\"revision walk setup failed\"));\n-\n-\tif (!opts->revs->commits)\n-\t\tdie(_(\"empty commit set passed\"));\n-}\n-\n-static void read_and_refresh_cache(struct replay_opts *opts)\n-{\n-\tstatic struct lock_file index_lock;\n-\tint index_fd = hold_locked_index(&index_lock, 0);\n-\tif (read_index_preload(&the_index, NULL) < 0)\n-\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n-\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n-\tif (the_index.cache_changed) {\n-\t\tif (write_index(&the_index, index_fd) ||\n-\t\t    commit_locked_index(&index_lock))\n-\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n-\t}\n-\trollback_lock_file(&index_lock);\n-}\n-\n-/*\n- * Append a commit to the end of the commit_list.\n- *\n- * next starts by pointing to the variable that holds the head of an\n- * empty commit_list, and is updated to point to the \"next\" field of\n- * the last item on the list as new commits are appended.\n- *\n- * Usage example:\n- *\n- *     struct commit_list *list;\n- *     struct commit_list **next = &list;\n- *\n- *     next = commit_list_append(c1, next);\n- *     next = commit_list_append(c2, next);\n- *     assert(commit_list_count(list) == 2);\n- *     return list;\n- */\n-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 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 replay_insn_list *todo_list)\n-{\n-\tstruct replay_insn_list *cur;\n-\n-\tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tconst char *sha1_abbrev, *action_str;\n-\t\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\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\tif (get_message(cur->operand, &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-\n-static int parse_insn_line(char *start, struct replay_insn_list *item)\n-{\n-\tunsigned char commit_sha1[20];\n-\tchar sha1_abbrev[40];\n-\tconst char *p, *q;\n-\n-\tp = start;\n-\tif (!prefixcmp(start, \"pick \")) {\n-\t\titem->action = REPLAY_PICK;\n-\t\tp += strlen(\"pick \");\n-\t} else if (!prefixcmp(start, \"revert \")) {\n-\t\titem->action = REPLAY_REVERT;\n-\t\tp += strlen(\"revert \");\n-\t} else {\n-\t\tint len = strchrnul(p, '\\n') - p;\n-\t\tif (len > 255)\n-\t\t\tlen = 255;\n-\t\treturn error(_(\"Unrecognized action: %.*s\"), len, p);\n-\t}\n-\n-\tq = p + strcspn(p, \" \\n\");\n-\tif (q - p + 1 > sizeof(sha1_abbrev)) {\n-\t\tint len = q - p;\n-\t\tif (len > 255)\n-\t\t\tlen = 255;\n-\t\treturn error(_(\"Object name too large: %.*s\"), len, p);\n-\t}\n-\tmemcpy(sha1_abbrev, p, q - p);\n-\tsha1_abbrev[q - p] = '\\0';\n-\n-\tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n-\t\treturn error(_(\"Malformed object name: %s\"), sha1_abbrev);\n-\n-\titem->operand = lookup_commit_reference(commit_sha1);\n-\tif (!item->operand)\n-\t\treturn error(_(\"Not a valid commit: %s\"), sha1_abbrev);\n-\n-\titem->next = NULL;\n-\treturn 0;\n-}\n-\n-static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n-{\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\tif (parse_insn_line(p, &item) < 0)\n-\t\t\treturn error(_(\"on line %d.\"), i);\n-\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n-\t\tp = strchrnul(p, '\\n');\n-\t\tif (*p)\n-\t\t\tp++;\n-\t}\n-\tif (!*todo_list)\n-\t\treturn error(_(\"No commits parsed.\"));\n-\treturn 0;\n-}\n-\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-\tint fd, res;\n-\n-\tfd = open(todo_file, O_RDONLY);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open %s.\"), todo_file);\n-\tif (strbuf_read(&buf, fd, 0) < 0) {\n-\t\tclose(fd);\n-\t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Could not read %s.\"), todo_file);\n-\t}\n-\tclose(fd);\n-\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-}\n-\n-static int populate_opts_cb(const char *key, const char *value, void *data)\n-{\n-\tstruct replay_opts *opts = data;\n-\tint error_flag = 1;\n-\n-\tif (!value)\n-\t\terror_flag = 0;\n-\telse if (!strcmp(key, \"options.no-commit\"))\n-\t\topts->no_commit = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.edit\"))\n-\t\topts->edit = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.signoff\"))\n-\t\topts->signoff = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.record-origin\"))\n-\t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.allow-ff\"))\n-\t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.mainline\"))\n-\t\topts->mainline = git_config_int(key, value);\n-\telse if (!strcmp(key, \"options.strategy\"))\n-\t\tgit_config_string(&opts->strategy, key, value);\n-\telse if (!strcmp(key, \"options.strategy-option\")) {\n-\t\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n-\t\topts->xopts[opts->xopts_nr++] = xstrdup(value);\n-\t} else\n-\t\treturn error(_(\"Invalid key: %s\"), key);\n-\n-\tif (!error_flag)\n-\t\treturn error(_(\"Invalid value for %s: %s\"), key, value);\n-\n-\treturn 0;\n-}\n-\n-static void read_populate_opts(struct replay_opts **opts_ptr)\n-{\n-\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n-\n-\tif (!file_exists(opts_file))\n-\t\treturn;\n-\tif (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)\n-\t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n-}\n-\n-static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n-\t\t\t\tstruct replay_opts *opts)\n-{\n-\tstruct commit *commit;\n-\tstruct replay_insn_list **next;\n-\n-\tprepare_revs(opts);\n-\n-\tnext = todo_list;\n-\twhile ((commit = get_revision(opts->revs)))\n-\t\tnext = replay_insn_list_append(opts->action, commit, next);\n-}\n-\n-static int create_seq_dir(void)\n-{\n-\tconst char *seq_dir = git_path(SEQ_DIR);\n-\n-\tif (file_exists(seq_dir))\n-\t\treturn error(_(\"%s already exists.\"), seq_dir);\n-\telse if (mkdir(seq_dir, 0777) < 0)\n-\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), seq_dir);\n-\treturn 0;\n-}\n-\n-static void save_head(const char *head)\n-{\n-\tconst char *head_file = git_path(SEQ_HEAD_FILE);\n-\tstatic struct lock_file head_lock;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tint fd;\n-\n-\tfd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);\n-\tstrbuf_addf(&buf, \"%s\\n\", head);\n-\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s.\"), head_file);\n-\tif (commit_lock_file(&head_lock) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), head_file);\n-}\n-\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-\tstruct strbuf buf = STRBUF_INIT;\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) < 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-\t\tdie_errno(_(\"Could not write to %s.\"), todo_file);\n-\t}\n-\tif (commit_lock_file(&todo_lock) < 0) {\n-\t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Error wrapping up %s.\"), todo_file);\n-\t}\n-\tstrbuf_release(&buf);\n-}\n-\n-static void save_opts(struct replay_opts *opts)\n-{\n-\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n-\n-\tif (opts->no_commit)\n-\t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n-\tif (opts->edit)\n-\t\tgit_config_set_in_file(opts_file, \"options.edit\", \"true\");\n-\tif (opts->signoff)\n-\t\tgit_config_set_in_file(opts_file, \"options.signoff\", \"true\");\n-\tif (opts->record_origin)\n-\t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n-\tif (opts->allow_ff)\n-\t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n-\tif (opts->mainline) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n-\t\tgit_config_set_in_file(opts_file, \"options.mainline\", buf.buf);\n-\t\tstrbuf_release(&buf);\n-\t}\n-\tif (opts->strategy)\n-\t\tgit_config_set_in_file(opts_file, \"options.strategy\", opts->strategy);\n-\tif (opts->xopts) {\n-\t\tint i;\n-\t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tgit_config_set_multivar_in_file(opts_file,\n-\t\t\t\t\t\t\t\"options.strategy-option\",\n-\t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n-\t}\n-}\n-\n-static int pick_commits(struct replay_insn_list *todo_list,\n-\t\t\tstruct replay_opts *opts)\n-{\n-\tstruct replay_insn_list *cur;\n-\tint res;\n-\n-\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n-\tif (opts->allow_ff)\n-\t\tassert(!(opts->signoff || opts->no_commit ||\n-\t\t\t\topts->record_origin || opts->edit));\n-\tread_and_refresh_cache(opts);\n-\n-\tfor (cur = todo_list; cur; cur = cur->next) {\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-\t\t\t\t * An error was encountered while\n-\t\t\t\t * picking the last commit; the\n-\t\t\t\t * sequencer state is useless now --\n-\t\t\t\t * the user simply needs to resolve\n-\t\t\t\t * the conflict and commit\n-\t\t\t\t */\n-\t\t\t\tremove_sequencer_state(0);\n-\t\t\treturn res;\n-\t\t}\n-\t}\n-\n-\t/*\n-\t * Sequence of picks finished successfully; cleanup by\n-\t * removing the .git/sequencer directory\n-\t */\n-\tremove_sequencer_state(1);\n-\treturn 0;\n-}\n-\n-static int pick_revisions(struct replay_opts *opts)\n-{\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-\t * Decide what to do depending on the arguments; a fresh\n-\t * cherry-pick should be handled differently from an existing\n-\t * one that is being continued\n-\t */\n-\tif (opts->subcommand == REPLAY_RESET) {\n-\t\tremove_sequencer_state(1);\n-\t\treturn 0;\n-\t} else if (opts->subcommand == REPLAY_CONTINUE) {\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);\n-\n-\t\t/* Verify that the conflict has been resolved */\n-\t\tif (!index_differs_from(\"HEAD\", 0))\n-\t\t\ttodo_list = todo_list->next;\n-\t} else {\n-\t\t/*\n-\t\t * Start a new cherry-pick/ revert sequence; but\n-\t\t * first, make sure that an existing one isn't in\n-\t\t * progress\n-\t\t */\n-\n-\t\twalk_revs_populate_todo(&todo_list, opts);\n-\t\tif (create_seq_dir() < 0) {\n-\t\t\terror(_(\"A cherry-pick or revert is in progress.\"));\n-\t\t\tadvise(_(\"Use --continue to continue the operation\"));\n-\t\t\tadvise(_(\"or --reset to forget about it\"));\n-\t\t\treturn -1;\n-\t\t}\n-\t\tif (get_sha1(\"HEAD\", sha1)) {\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-\t\tsave_head(sha1_to_hex(sha1));\n-\t\tsave_opts(opts);\n-\t}\n-\treturn pick_commits(todo_list, opts);\n-error:\n-\treturn error(_(\"No %s in progress\"), action_name(opts));\n-}\n-\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts;\n@@ -1016,7 +196,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \topts.action = REPLAY_REVERT;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n-\tres = pick_revisions(&opts);\n+\tres = sequencer_pick_revisions(&opts);\n \tif (res < 0)\n \t\tdie(_(\"revert failed\"));\n \treturn res;\n@@ -1031,7 +211,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \topts.action = REPLAY_PICK;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n-\tres = pick_revisions(&opts);\n+\tres = sequencer_pick_revisions(&opts);\n \tif (res < 0)\n \t\tdie(_(\"cherry-pick failed\"));\n \treturn res;\ndiff --git a/sequencer.c b/sequencer.c\nindex bc2c046..95bce89 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1,13 +1,656 @@\n #include \"cache.h\"\n #include \"sequencer.h\"\n-#include \"strbuf.h\"\n+#include \"object.h\"\n+#include \"commit.h\"\n+#include \"tag.h\"\n+#include \"run-command.h\"\n+#include \"exec_cmd.h\"\n+#include \"utf8.h\"\n+#include \"cache-tree.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n+#include \"rerere.h\"\n+#include \"merge-recursive.h\"\n+#include \"refs.h\"\n #include \"dir.h\"\n \n+#define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n+#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };\n+\n+struct commit_message {\n+\tchar *parent_label;\n+\tconst char *label;\n+\tconst char *subject;\n+\tchar *reencoded_message;\n+\tconst char *message;\n+};\n+\n+static const char *action_keyword(const struct replay_opts *opts)\n+{\n+\treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n+}\n+\n+static char *get_encoding(const char *message);\n+\n+static int get_message(struct commit *commit, struct commit_message *out)\n+{\n+\tconst char *encoding;\n+\tconst char *abbrev, *subject;\n+\tint abbrev_len, subject_len;\n+\tchar *q;\n+\n+\tif (!commit->buffer)\n+\t\treturn -1;\n+\tencoding = get_encoding(commit->buffer);\n+\tif (!encoding)\n+\t\tencoding = \"UTF-8\";\n+\tif (!git_commit_encoding)\n+\t\tgit_commit_encoding = \"UTF-8\";\n+\n+\tout->reencoded_message = NULL;\n+\tout->message = commit->buffer;\n+\tif (strcmp(encoding, git_commit_encoding))\n+\t\tout->reencoded_message = reencode_string(commit->buffer,\n+\t\t\t\t\tgit_commit_encoding, encoding);\n+\tif (out->reencoded_message)\n+\t\tout->message = out->reencoded_message;\n+\n+\tabbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n+\tabbrev_len = strlen(abbrev);\n+\n+\tsubject_len = find_commit_subject(out->message, &subject);\n+\n+\tout->parent_label = xmalloc(strlen(\"parent of \") + abbrev_len +\n+\t\t\t      strlen(\"... \") + subject_len + 1);\n+\tq = out->parent_label;\n+\tq = mempcpy(q, \"parent of \", strlen(\"parent of \"));\n+\tout->label = q;\n+\tq = mempcpy(q, abbrev, abbrev_len);\n+\tq = mempcpy(q, \"... \", strlen(\"... \"));\n+\tout->subject = q;\n+\tq = mempcpy(q, subject, subject_len);\n+\t*q = '\\0';\n+\treturn 0;\n+}\n+\n+static void free_message(struct commit_message *msg)\n+{\n+\tfree(msg->parent_label);\n+\tfree(msg->reencoded_message);\n+}\n+\n+static char *get_encoding(const char *message)\n+{\n+\tconst char *p = message, *eol;\n+\n+\twhile (*p && *p != '\\n') {\n+\t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n+\t\t\t; /* do nothing */\n+\t\tif (!prefixcmp(p, \"encoding \")) {\n+\t\t\tchar *result = xmalloc(eol - 8 - p);\n+\t\t\tstrlcpy(result, p + 9, eol - 8 - p);\n+\t\t\treturn result;\n+\t\t}\n+\t\tp = eol;\n+\t\tif (*p == '\\n')\n+\t\t\tp++;\n+\t}\n+\treturn NULL;\n+}\n+\n+static void write_cherry_pick_head(struct commit *commit)\n+{\n+\tint fd;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n+\n+\tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n+\t\t\t  git_path(\"CHERRY_PICK_HEAD\"));\n+\tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n+\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"CHERRY_PICK_HEAD\"));\n+\tstrbuf_release(&buf);\n+}\n+\n+static void print_advice(void)\n+{\n+\tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n+\n+\tif (msg) {\n+\t\tfprintf(stderr, \"%s\\n\", msg);\n+\t\t/*\n+\t\t * A conflict has occured but the porcelain\n+\t\t * (typically rebase --interactive) wants to take care\n+\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n+\t\t */\n+\t\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n+\t\treturn;\n+\t}\n+\n+\tadvise(\"after resolving the conflicts, mark the corrected paths\");\n+\tadvise(\"with 'git add <paths>' or 'git rm <paths>'\");\n+\tadvise(\"and commit the result with 'git commit'\");\n+}\n+\n+static void write_message(struct strbuf *msgbuf, const char *filename)\n+{\n+\tstatic struct lock_file msg_file;\n+\n+\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n+\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n+\tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), filename);\n+\tstrbuf_release(msgbuf);\n+\tif (commit_lock_file(&msg_file) < 0)\n+\t\tdie(_(\"Error wrapping up %s\"), filename);\n+}\n+\n+static struct tree *empty_tree(void)\n+{\n+\tstruct tree *tree = xcalloc(1, sizeof(struct tree));\n+\n+\ttree->object.parsed = 1;\n+\ttree->object.type = OBJ_TREE;\n+\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);\n+\treturn tree;\n+}\n+\n+static int error_dirty_index(struct replay_opts *opts)\n+{\n+\tif (read_cache_unmerged())\n+\t\treturn error_resolve_conflict(action_keyword(opts));\n+\n+\t/* Different translation strings for cherry-pick and revert */\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+\n+\tif (advice_commit_before_merge)\n+\t\tadvise(_(\"Commit your changes or stash them to proceed.\"));\n+\treturn -1;\n+}\n+\n+static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n+{\n+\tstruct ref_lock *ref_lock;\n+\n+\tread_cache();\n+\tif (checkout_fast_forward(from, to))\n+\t\texit(1); /* the callee should have complained already */\n+\tref_lock = lock_any_ref_for_update(\"HEAD\", from, 0);\n+\treturn write_ref_sha1(ref_lock, to, \"cherry-pick\");\n+}\n+\n+static int do_recursive_merge(struct commit *base, struct commit *next,\n+\t\t\t      const char *base_label, const char *next_label,\n+\t\t\t      unsigned char *head, struct strbuf *msgbuf,\n+\t\t\t      struct replay_opts *opts)\n+{\n+\tstruct merge_options o;\n+\tstruct tree *result, *next_tree, *base_tree, *head_tree;\n+\tint clean, index_fd;\n+\tconst char **xopt;\n+\tstatic struct lock_file index_lock;\n+\n+\tindex_fd = hold_locked_index(&index_lock, 1);\n+\n+\tread_cache();\n+\n+\tinit_merge_options(&o);\n+\to.ancestor = base ? base_label : \"(empty tree)\";\n+\to.branch1 = \"HEAD\";\n+\to.branch2 = next ? next_label : \"(empty tree)\";\n+\n+\thead_tree = parse_tree_indirect(head);\n+\tnext_tree = next ? next->tree : empty_tree();\n+\tbase_tree = base ? base->tree : empty_tree();\n+\n+\tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n+\t\tparse_merge_opt(&o, *xopt);\n+\n+\tclean = merge_trees(&o,\n+\t\t\t    head_tree,\n+\t\t\t    next_tree, base_tree, &result);\n+\n+\tif (active_cache_changed &&\n+\t    (write_cache(index_fd, active_cache, active_nr) ||\n+\t     commit_locked_index(&index_lock)))\n+\t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n+\t\tdie(_(\"%s: Unable to write new index file\"), action_keyword(opts));\n+\trollback_lock_file(&index_lock);\n+\n+\tif (!clean) {\n+\t\tint i;\n+\t\tstrbuf_addstr(msgbuf, \"\\nConflicts:\\n\\n\");\n+\t\tfor (i = 0; i < active_nr;) {\n+\t\t\tstruct cache_entry *ce = active_cache[i++];\n+\t\t\tif (ce_stage(ce)) {\n+\t\t\t\tstrbuf_addch(msgbuf, '\\t');\n+\t\t\t\tstrbuf_addstr(msgbuf, ce->name);\n+\t\t\t\tstrbuf_addch(msgbuf, '\\n');\n+\t\t\t\twhile (i < active_nr && !strcmp(ce->name,\n+\t\t\t\t\t\tactive_cache[i]->name))\n+\t\t\t\t\ti++;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\treturn !clean;\n+}\n+\n+/*\n+ * If we are cherry-pick, and if the merge did not result in\n+ * hand-editing, we will hit this commit and inherit the original\n+ * author date and name.\n+ * If we are revert, or if our cherry-pick results in a hand merge,\n+ * we had better say that the current user is responsible for that.\n+ */\n+static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n+{\n+\t/* 6 is max possible length of our args array including NULL */\n+\tconst char *args[6];\n+\tint i = 0;\n+\n+\targs[i++] = \"commit\";\n+\targs[i++] = \"-n\";\n+\tif (opts->signoff)\n+\t\targs[i++] = \"-s\";\n+\tif (!opts->edit) {\n+\t\targs[i++] = \"-F\";\n+\t\targs[i++] = defmsg;\n+\t}\n+\targs[i] = NULL;\n+\n+\treturn run_command_v_opt(args, RUN_GIT_CMD);\n+}\n+\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+\tconst char *base_label, *next_label;\n+\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n+\tchar *defmsg = NULL;\n+\tstruct strbuf msgbuf = STRBUF_INIT;\n+\tint res;\n+\n+\tif (opts->no_commit) {\n+\t\t/*\n+\t\t * We do not intend to commit immediately.  We just want to\n+\t\t * merge the differences in, so let's compute the tree\n+\t\t * that represents the \"current\" state for merge-recursive\n+\t\t * to work on.\n+\t\t */\n+\t\tif (write_cache_as_tree(head, 0, NULL))\n+\t\t\tdie (_(\"Your index file is unmerged.\"));\n+\t} else {\n+\t\tif (get_sha1(\"HEAD\", head))\n+\t\t\treturn error(_(\"You do not have a valid HEAD\"));\n+\t\tif (index_differs_from(\"HEAD\", 0))\n+\t\t\treturn error_dirty_index(opts);\n+\t}\n+\tdiscard_cache();\n+\n+\tif (!commit->parents) {\n+\t\tparent = NULL;\n+\t}\n+\telse if (commit->parents->next) {\n+\t\t/* Reverting or cherry-picking a merge commit */\n+\t\tint cnt;\n+\t\tstruct commit_list *p;\n+\n+\t\tif (!opts->mainline)\n+\t\t\treturn error(_(\"Commit %s is a merge but no -m option was given.\"),\n+\t\t\t\tsha1_to_hex(commit->object.sha1));\n+\n+\t\tfor (cnt = 1, p = commit->parents;\n+\t\t     cnt != opts->mainline && p;\n+\t\t     cnt++)\n+\t\t\tp = p->next;\n+\t\tif (cnt != opts->mainline || !p)\n+\t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n+\t\t\t\tsha1_to_hex(commit->object.sha1), opts->mainline);\n+\t\tparent = p->item;\n+\t} else if (0 < opts->mainline)\n+\t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n+\telse\n+\t\tparent = commit->parents->item;\n+\n+\tif (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))\n+\t\treturn fast_forward_to(commit->object.sha1, head);\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_keyword(opts), sha1_to_hex(parent->object.sha1));\n+\n+\tif (get_message(commit, &msg) != 0)\n+\t\treturn error(_(\"Cannot get commit message for %s\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n+\n+\t/*\n+\t * \"commit\" is an existing commit.  We would want to apply\n+\t * the difference it introduces since its first parent \"prev\"\n+\t * on top of the current HEAD if we are cherry-pick.  Or the\n+\t * reverse of it if we are revert.\n+\t */\n+\n+\tdefmsg = git_pathdup(\"MERGE_MSG\");\n+\n+\tif (action == REPLAY_REVERT) {\n+\t\tbase = commit;\n+\t\tbase_label = msg.label;\n+\t\tnext = parent;\n+\t\tnext_label = msg.parent_label;\n+\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n+\t\tstrbuf_addstr(&msgbuf, msg.subject);\n+\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n+\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n+\n+\t\tif (commit->parents && commit->parents->next) {\n+\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n+\t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));\n+\t\t}\n+\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n+\t} else {\n+\t\tconst char *p;\n+\n+\t\tbase = parent;\n+\t\tbase_label = msg.parent_label;\n+\t\tnext = commit;\n+\t\tnext_label = msg.label;\n+\n+\t\t/*\n+\t\t * Append the commit log message to msgbuf; it starts\n+\t\t * after the tree, parent, author, committer\n+\t\t * information followed by \"\\n\\n\".\n+\t\t */\n+\t\tp = strstr(msg.message, \"\\n\\n\");\n+\t\tif (p) {\n+\t\t\tp += 2;\n+\t\t\tstrbuf_addstr(&msgbuf, p);\n+\t\t}\n+\n+\t\tif (opts->record_origin) {\n+\t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n+\t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n+\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n+\t\t}\n+\t\tif (!opts->no_commit)\n+\t\t\twrite_cherry_pick_head(commit);\n+\t}\n+\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+\t} else {\n+\t\tstruct commit_list *common = NULL;\n+\t\tstruct commit_list *remotes = NULL;\n+\n+\t\twrite_message(&msgbuf, defmsg);\n+\n+\t\tcommit_list_insert(base, &common);\n+\t\tcommit_list_insert(next, &remotes);\n+\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n+\t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n+\t\tfree_commit_list(common);\n+\t\tfree_commit_list(remotes);\n+\t}\n+\n+\tif (res) {\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+\t\t      msg.subject);\n+\t\tprint_advice();\n+\t\trerere(opts->allow_rerere_auto);\n+\t} else {\n+\t\tif (!opts->no_commit)\n+\t\t\tres = run_git_commit(defmsg, opts);\n+\t}\n+\n+\tfree_message(&msg);\n+\tfree(defmsg);\n+\n+\treturn res;\n+}\n+\n+static void prepare_revs(struct replay_opts *opts)\n+{\n+\tif (opts->action != REPLAY_REVERT)\n+\t\topts->revs->reverse ^= 1;\n+\n+\tif (prepare_revision_walk(opts->revs))\n+\t\tdie(_(\"revision walk setup failed\"));\n+\n+\tif (!opts->revs->commits)\n+\t\tdie(_(\"empty commit set passed\"));\n+}\n+\n+static void read_and_refresh_cache(struct replay_opts *opts)\n+{\n+\tstatic struct lock_file index_lock;\n+\tint index_fd = hold_locked_index(&index_lock, 0);\n+\tif (read_index_preload(&the_index, NULL) < 0)\n+\t\tdie(_(\"git %s: failed to read the index\"), action_keyword(opts));\n+\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n+\tif (the_index.cache_changed) {\n+\t\tif (write_index(&the_index, index_fd) ||\n+\t\t    commit_locked_index(&index_lock))\n+\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_keyword(opts));\n+\t}\n+\trollback_lock_file(&index_lock);\n+}\n+\n+/*\n+ * Append a commit to the end of the commit_list.\n+ *\n+ * next starts by pointing to the variable that holds the head of an\n+ * empty commit_list, and is updated to point to the \"next\" field of\n+ * the last item on the list as new commits are appended.\n+ *\n+ * Usage example:\n+ *\n+ *     struct commit_list *list;\n+ *     struct commit_list **next = &list;\n+ *\n+ *     next = commit_list_append(c1, next);\n+ *     next = commit_list_append(c2, next);\n+ *     assert(commit_list_count(list) == 2);\n+ *     return list;\n+ */\n+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 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 replay_insn_list *todo_list)\n+{\n+\tstruct replay_insn_list *cur;\n+\n+\tfor (cur = todo_list; cur; cur = cur->next) {\n+\t\tconst char *sha1_abbrev, *action_str;\n+\t\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\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\tif (get_message(cur->operand, &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+\n+static int parse_insn_line(char *start, struct replay_insn_list *item)\n+{\n+\tunsigned char commit_sha1[20];\n+\tchar sha1_abbrev[40];\n+\tconst char *p, *q;\n+\n+\tp = start;\n+\tif (!prefixcmp(start, \"pick \")) {\n+\t\titem->action = REPLAY_PICK;\n+\t\tp += strlen(\"pick \");\n+\t} else if (!prefixcmp(start, \"revert \")) {\n+\t\titem->action = REPLAY_REVERT;\n+\t\tp += strlen(\"revert \");\n+\t} else {\n+\t\tint len = strchrnul(p, '\\n') - p;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"Unrecognized action: %.*s\"), len, p);\n+\t}\n+\n+\tq = p + strcspn(p, \" \\n\");\n+\tif (q - p + 1 > sizeof(sha1_abbrev)) {\n+\t\tint len = q - p;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"Object name too large: %.*s\"), len, p);\n+\t}\n+\tmemcpy(sha1_abbrev, p, q - p);\n+\tsha1_abbrev[q - p] = '\\0';\n+\n+\tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n+\t\treturn error(_(\"Malformed object name: %s\"), sha1_abbrev);\n+\n+\titem->operand = lookup_commit_reference(commit_sha1);\n+\tif (!item->operand)\n+\t\treturn error(_(\"Not a valid commit: %s\"), sha1_abbrev);\n+\n+\titem->next = NULL;\n+\treturn 0;\n+}\n+\n+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n+{\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\tif (parse_insn_line(p, &item) < 0)\n+\t\t\treturn error(_(\"on line %d.\"), i);\n+\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n+\t\tp = strchrnul(p, '\\n');\n+\t\tif (*p)\n+\t\t\tp++;\n+\t}\n+\tif (!*todo_list)\n+\t\treturn error(_(\"No commits parsed.\"));\n+\treturn 0;\n+}\n+\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+\tint fd, res;\n+\n+\tfd = open(todo_file, O_RDONLY);\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"Could not open %s.\"), todo_file);\n+\tif (strbuf_read(&buf, fd, 0) < 0) {\n+\t\tclose(fd);\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Could not read %s.\"), todo_file);\n+\t}\n+\tclose(fd);\n+\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+}\n+\n+static int populate_opts_cb(const char *key, const char *value, void *data)\n+{\n+\tstruct replay_opts *opts = data;\n+\tint error_flag = 1;\n+\n+\tif (!value)\n+\t\terror_flag = 0;\n+\telse if (!strcmp(key, \"options.no-commit\"))\n+\t\topts->no_commit = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.edit\"))\n+\t\topts->edit = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.signoff\"))\n+\t\topts->signoff = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.record-origin\"))\n+\t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.allow-ff\"))\n+\t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.mainline\"))\n+\t\topts->mainline = git_config_int(key, value);\n+\telse if (!strcmp(key, \"options.strategy\"))\n+\t\tgit_config_string(&opts->strategy, key, value);\n+\telse if (!strcmp(key, \"options.strategy-option\")) {\n+\t\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n+\t\topts->xopts[opts->xopts_nr++] = xstrdup(value);\n+\t} else\n+\t\treturn error(_(\"Invalid key: %s\"), key);\n+\n+\tif (!error_flag)\n+\t\treturn error(_(\"Invalid value for %s: %s\"), key, value);\n+\n+\treturn 0;\n+}\n+\n+static void read_populate_opts(struct replay_opts **opts_ptr)\n+{\n+\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n+\n+\tif (!file_exists(opts_file))\n+\t\treturn;\n+\tif (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)\n+\t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n+}\n+\n+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n+\t\t\t\tstruct replay_opts *opts)\n+{\n+\tstruct commit *commit;\n+\tstruct replay_insn_list **next;\n+\n+\tprepare_revs(opts);\n+\n+\tnext = todo_list;\n+\twhile ((commit = get_revision(opts->revs)))\n+\t\tnext = replay_insn_list_append(opts->action, commit, next);\n+}\n+\n+static int create_seq_dir(void)\n+{\n+\tconst char *seq_dir = git_path(SEQ_DIR);\n+\n+\tif (file_exists(seq_dir))\n+\t\treturn error(_(\"%s already exists.\"), seq_dir);\n+\telse if (mkdir(seq_dir, 0777) < 0)\n+\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), seq_dir);\n+\treturn 0;\n+}\n+\n void remove_sequencer_state(int aggressive)\n {\n \tstruct strbuf seq_dir = STRBUF_INIT;\n \tstruct strbuf seq_old_dir = STRBUF_INIT;\n-\n \tstrbuf_addf(&seq_dir, \"%s\", git_path(SEQ_DIR));\n \tstrbuf_addf(&seq_old_dir, \"%s\", git_path(SEQ_OLD_DIR));\n \tremove_dir_recursively(&seq_old_dir, 0);\n@@ -17,3 +660,161 @@ void remove_sequencer_state(int aggressive)\n \tstrbuf_release(&seq_dir);\n \tstrbuf_release(&seq_old_dir);\n }\n+\n+static void save_head(const char *head)\n+{\n+\tconst char *head_file = git_path(SEQ_HEAD_FILE);\n+\tstatic struct lock_file head_lock;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tfd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);\n+\tstrbuf_addf(&buf, \"%s\\n\", head);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), head_file);\n+\tif (commit_lock_file(&head_lock) < 0)\n+\t\tdie(_(\"Error wrapping up %s.\"), head_file);\n+}\n+\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+\tstruct strbuf buf = STRBUF_INIT;\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) < 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+\t\tdie_errno(_(\"Could not write to %s.\"), todo_file);\n+\t}\n+\tif (commit_lock_file(&todo_lock) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Error wrapping up %s.\"), todo_file);\n+\t}\n+\tstrbuf_release(&buf);\n+}\n+\n+static void save_opts(struct replay_opts *opts)\n+{\n+\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n+\n+\tif (opts->no_commit)\n+\t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n+\tif (opts->edit)\n+\t\tgit_config_set_in_file(opts_file, \"options.edit\", \"true\");\n+\tif (opts->signoff)\n+\t\tgit_config_set_in_file(opts_file, \"options.signoff\", \"true\");\n+\tif (opts->record_origin)\n+\t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n+\tif (opts->allow_ff)\n+\t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n+\tif (opts->mainline) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n+\t\tgit_config_set_in_file(opts_file, \"options.mainline\", buf.buf);\n+\t\tstrbuf_release(&buf);\n+\t}\n+\tif (opts->strategy)\n+\t\tgit_config_set_in_file(opts_file, \"options.strategy\", opts->strategy);\n+\tif (opts->xopts) {\n+\t\tint i;\n+\t\tfor (i = 0; i < opts->xopts_nr; i++)\n+\t\t\tgit_config_set_multivar_in_file(opts_file,\n+\t\t\t\t\t\t\t\"options.strategy-option\",\n+\t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n+\t}\n+}\n+\n+static int pick_commits(struct replay_insn_list *todo_list,\n+\t\t\tstruct replay_opts *opts)\n+{\n+\tstruct replay_insn_list *cur;\n+\tint res;\n+\n+\tsetenv(GIT_REFLOG_ACTION, action_keyword(opts), 0);\n+\tif (opts->allow_ff)\n+\t\tassert(!(opts->signoff || opts->no_commit ||\n+\t\t\t\topts->record_origin || opts->edit));\n+\tread_and_refresh_cache(opts);\n+\n+\tfor (cur = todo_list; cur; cur = cur->next) {\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+\t\t\t\t * An error was encountered while\n+\t\t\t\t * picking the last commit; the\n+\t\t\t\t * sequencer state is useless now --\n+\t\t\t\t * the user simply needs to resolve\n+\t\t\t\t * the conflict and commit\n+\t\t\t\t */\n+\t\t\t\tremove_sequencer_state(0);\n+\t\t\treturn res;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Sequence of picks finished successfully; cleanup by\n+\t * removing the .git/sequencer directory\n+\t */\n+\tremove_sequencer_state(1);\n+\treturn 0;\n+}\n+\n+int sequencer_pick_revisions(struct replay_opts *opts)\n+{\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+\t * Decide what to do depending on the arguments; a fresh\n+\t * cherry-pick should be handled differently from an existing\n+\t * one that is being continued\n+\t */\n+\tif (opts->subcommand == REPLAY_RESET) {\n+\t\tremove_sequencer_state(1);\n+\t\treturn 0;\n+\t} else if (opts->subcommand == REPLAY_CONTINUE) {\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);\n+\n+\t\t/* Verify that the conflict has been resolved */\n+\t\tif (!index_differs_from(\"HEAD\", 0))\n+\t\t\ttodo_list = todo_list->next;\n+\t} else {\n+\t\t/*\n+\t\t * Start a new cherry-pick/ revert sequence; but\n+\t\t * first, make sure that an existing one isn't in\n+\t\t * progress\n+\t\t */\n+\n+\t\twalk_revs_populate_todo(&todo_list, opts);\n+\t\tif (create_seq_dir() < 0) {\n+\t\t\terror(_(\"A cherry-pick or revert is in progress.\"));\n+\t\t\tadvise(_(\"Use --continue to continue the operation\"));\n+\t\t\tadvise(_(\"or --reset to forget about it\"));\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (get_sha1(\"HEAD\", sha1)) {\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+\t\tsave_head(sha1_to_hex(sha1));\n+\t\tsave_opts(opts);\n+\t}\n+\treturn pick_commits(todo_list, opts);\n+error:\n+\treturn error(_(\"No %s in progress\"), action_keyword(opts));\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex f4db257..5e56cd4 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -8,6 +8,30 @@\n #define SEQ_OPTS_FILE\t\"sequencer/opts\"\n \n enum replay_action { REPLAY_REVERT, REPLAY_PICK };\n+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };\n+\n+struct replay_opts {\n+\tenum replay_action action;\n+\tenum replay_subcommand subcommand;\n+\n+\t/* Boolean options */\n+\tint edit;\n+\tint record_origin;\n+\tint no_commit;\n+\tint signoff;\n+\tint allow_ff;\n+\tint allow_rerere_auto;\n+\n+\tint mainline;\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 struct replay_insn_list {\n \tenum replay_action action;\n@@ -25,4 +49,7 @@ struct replay_insn_list {\n  */\n void remove_sequencer_state(int aggressive);\n \n+void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);\n+int sequencer_pick_revisions(struct replay_opts *opts);\n+\n #endif\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"173481","messageId":"1313310789-10216-8-git-send-email-artagnon@gmail.com","threadId":"28092","inReplyTo":"1313310789-10216-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T08:33:09Z","receivedAt":"2011-08-14T08:33:09Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Since d3f4628e (revert: Remove sequencer state when no commits are\npending, 2011-07-06), the sequencer removes the sequencer state before\nthe final commit is actually completed.  This design is inherently\nflawed, as it will not allow the user to abort the sequencer operation\nat that stage.  Instead, make 'git commit' notify the sequencer after\nevery successful commit; the sequencer then removes the state if no\nmore instructions are pending.\n\nMentored-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/commit.c                   |    8 ++++++++\n sequencer.c                        |   23 ++++++++++++-----------\n sequencer.h                        |    1 +\n t/t3032-merge-recursive-options.sh |    2 ++\n t/t3510-cherry-pick-sequence.sh    |    4 ++--\n 5 files changed, 25 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e1af9b1..1699371 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -26,6 +26,7 @@\n #include \"unpack-trees.h\"\n #include \"quote.h\"\n #include \"submodule.h\"\n+#include \"sequencer.h\"\n \n static const char * const builtin_commit_usage[] = {\n \t\"git commit [options] [--] <filepattern>...\",\n@@ -1521,6 +1522,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunlink(git_path(\"MERGE_MODE\"));\n \tunlink(git_path(\"SQUASH_MSG\"));\n \n+\t/*\n+\t * Notify the sequencer that we're committing.  The routine\n+\t * removes the sequencer state if our commit just completed\n+\t * the last instruction.\n+\t */\n+\tsequencer_notify_commit();\n+\n \tif (commit_index_files())\n \t\tdie (_(\"Repository has been updated, but unable to write\\n\"\n \t\t     \"new_index file. Check that disk is not full or quota is\\n\"\ndiff --git a/sequencer.c b/sequencer.c\nindex 95bce89..caae932 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -580,6 +580,17 @@ static void read_populate_todo(struct replay_insn_list **todo_list)\n \t\tdie(_(\"Unusable instruction sheet: %s\"), todo_file);\n }\n \n+void sequencer_notify_commit(void)\n+{\n+\tstruct replay_insn_list *todo_list = NULL;\n+\n+\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n+\t\treturn;\n+\tread_populate_todo(&todo_list);\n+\tif (!todo_list->next)\n+\t\tremove_sequencer_state(1);\n+}\n+\n static int populate_opts_cb(const char *key, const char *value, void *data)\n {\n \tstruct replay_opts *opts = data;\n@@ -743,18 +754,8 @@ static int pick_commits(struct replay_insn_list *todo_list,\n \tfor (cur = todo_list; cur; cur = cur->next) {\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-\t\t\t\t * An error was encountered while\n-\t\t\t\t * picking the last commit; the\n-\t\t\t\t * sequencer state is useless now --\n-\t\t\t\t * the user simply needs to resolve\n-\t\t\t\t * the conflict and commit\n-\t\t\t\t */\n-\t\t\t\tremove_sequencer_state(0);\n+\t\tif (res)\n \t\t\treturn res;\n-\t\t}\n \t}\n \n \t/*\ndiff --git a/sequencer.h b/sequencer.h\nindex 5e56cd4..3b89d42 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -51,5 +51,6 @@ void remove_sequencer_state(int aggressive);\n \n void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);\n int sequencer_pick_revisions(struct replay_opts *opts);\n+void sequencer_notify_commit(void);\n \n #endif\ndiff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\nindex 2b17311..1da99b7 100755\n--- a/t/t3032-merge-recursive-options.sh\n+++ b/t/t3032-merge-recursive-options.sh\n@@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '\n \tgit read-tree --reset -u HEAD &&\n \ttest_must_fail git cherry-pick --no-commit remote &&\n \tgit read-tree --reset -u HEAD &&\n+\tgit cherry-pick --reset &&\n \ttest_must_fail git cherry-pick remote &&\n \ttest_must_fail git update-index --refresh &&\n+\tgit cherry-pick --reset &&\n \tgrep \"<<<<<<\" text.txt\n '\n \ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex c284c96..abd13fe 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' '\n \ttest_path_is_missing .git/sequencer\n '\n \n-test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '\n+test_expect_success 'final commit cleans up sequencer state' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick base..picked &&\n-\ttest_path_is_missing .git/sequencer &&\n \techo \"resolved\" >foo &&\n \tgit add foo &&\n \tgit commit &&\n+\ttest_path_is_missing .git/sequencer &&\n \t{\n \t\tgit rev-list HEAD |\n \t\tgit diff-tree --root --stdin |\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"173490","messageId":"20110814115821.GC18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"1313310789-10216-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/7] revert: Fix buffer overflow in insn sheet parser","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T11:58:21Z","receivedAt":"2011-08-14T11:58:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamkumar Ramachandra wrote:\n\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -211,4 +211,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 \\1\\1\\1\\1\\1\\1\\1\\1\\2/\" .git/sequencer/todo >new_sheet &&\n\nThis construct (\\+ in sed regexes) is not portable.  See the note on\ngrep in Documentation/CodingGuidelines (maybe it should be tweaked to\nsay \"grep and sed\").\n"},{"id":"173491","messageId":"20110814121244.GD18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"1313310789-10216-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/7] revert: Allow mixed pick and revert instructions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T12:12:44Z","receivedAt":"2011-08-14T12:12:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n>  \tif (!prefixcmp(start, \"pick \")) {\n> -\t\taction = CHERRY_PICK;\n> +\t\titem->action = REPLAY_PICK;\n>  \t\tp += strlen(\"pick \");\n>  \t} else if (!prefixcmp(start, \"revert \")) {\n> -\t\taction = REVERT;\n> +\t\titem->action = REPLAY_REVERT;\n>  \t\tp += strlen(\"revert \");\n> -\t} else\n> -\t\treturn NULL;\n> +\t} else {\n> +\t\tint len = strchrnul(p, '\\n') - p;\n> +\t\tif (len > 255)\n> +\t\t\tlen = 255;\n> +\t\treturn error(_(\"Unrecognized action: %.*s\"), len, p);\n\nWhat happens if the current line has more than INT_MAX characters?\n\nMaybe it would make sense to factor out a function for this\ncomputation, for brevity and so there is just one place to tweak.\n"},{"id":"173492","messageId":"20110814125223.GE18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"1313310789-10216-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 5/7] revert: Make the argument parser responsible for setup_revisions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T12:52:23Z","receivedAt":"2011-08-14T12:52:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Currently, prepare_revs calls setup_revisions with the (argc, argv) in\n> the opts structure filled in by parse_args.  As a result, prepare_revs\n> has to take up the responsibility of erroring out and printing usage\n> information when (argc, argv) is malformed.  Since parse_args is doing\n> this for other kinds of parse errors anyway, give it the task of\n> calling setup_revisions as well.\n\nI'm having trouble understanding the above.  Are you saying that we\nwant to concentrate all usage() calls in a single function for some\nreason and prepare_revs() goes against this?  Why would I (the reader)\ncare about that?\n\nPresumably a simpler explanation is that it makes my life easier in\ntwo ways:\n\n 1. If I want to understand what commandline arguments are accepted\n    by cherry-pick/revert, after this patch there is just one function\n    to look at.\n\n 2. If I want to ask the replay machinery to do something different,\n    as a caller I can set my options in a \"struct rev_info\" instead\n    of forging commandline arguments with the same effect.\n\n> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n\nSince I wrote the patch modulo the commit message and small\nimprovements, shouldn't I be blamed as the author?  (git log\n--grep=\"[Oo]riginal patch\" and git log --grep=jc: give some examples.)\nYou can have my sign-off if you'd like.\n\n[...]\n> +\tif (opts->subcommand == REPLAY_NONE) {\n> +\t\topts->revs = xmalloc(sizeof(*opts->revs));\n\nMy fault: this never gets freed.  As long as this is private to the\ncherry-pick/revert builtin, it's a one-time tiny leak cleaned up by\n_exit --- no harm done, but probably worth a comment.\n"},{"id":"173493","messageId":"20110814131303.GF18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"1313310789-10216-7-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 6/7] sequencer: Expose API to cherry-picking machinery","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T13:13:03Z","receivedAt":"2011-08-14T13:13:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Move code from builtin/revert.c to sequencer.c and expose a public API\n> without making any functional changes.  Although it is useful only to\n> existing callers of cherry-pick and revert now, this patch lays the\n> foundation for future expansion.\n\n:)  It sounds like you are running a business.\n\nI guess I would suggest clarifying that you mean exposing further\nfunctionality through this API for more callers, rather than just\ngenerally bloating git further.\n\n[...]\n> +++ b/sequencer.c\n> @@ -1,13 +1,656 @@\n[...]\n> +#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };\n\nAs much as I like this change (and I do, modulo the stray semicolon),\nit does not belong in a patch where it can be unnoticed in the code\nmovement.\n\n> +static const char *action_keyword(const struct replay_opts *opts)\n> +{\n> +\treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n> +}\n\nAnother (non-functional) change.  Probably (?) this renaming has a\ngood reason to be part of this patch, but it should definitely be\nmentioned in the commit message.\n\n> +static void write_cherry_pick_head(struct commit *commit)\n> +{\n> +\tint fd;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n> +\n> +\tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n> +\tif (fd < 0)\n> +\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n> +\t\t\t  git_path(\"CHERRY_PICK_HEAD\"));\n\ngit_path() calls vsnprintf which clobbers errno, so depending on the\nplatform this can print messages like\n\n\tfatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success\n\nThe natural fix would be to add a local for it (as a separate patch).\nSorry I missed this when the code first arrived.\n\n> +static struct tree *empty_tree(void)\n> +{\n> +\tstruct tree *tree = xcalloc(1, sizeof(struct tree));\n> +\n> +\ttree->object.parsed = 1;\n> +\ttree->object.type = OBJ_TREE;\n> +\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);\n> +\treturn tree;\n\nThis tree is leaked (for example if you cherry-pick a sequence of\nroot commits).\n\n> +static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n> +{\n> +\tstruct ref_lock *ref_lock;\n> +\n> +\tread_cache();\n> +\tif (checkout_fast_forward(from, to))\n> +\t\texit(1); /* the callee should have complained already */\n> +\tref_lock = lock_any_ref_for_update(\"HEAD\", from, 0);\n> +\treturn write_ref_sha1(ref_lock, to, \"cherry-pick\");\n> +}\n\nThe exit code here violates the usual \"exit with status 128 for\nerrors other than conflicts\" rule.  Perhaps it should be changed to\n\"return -1\" in a separate patch (to accompany the patch that returns\nerror() instead of die()-ing so often to allow callers to give\nadditional context to errors from this machinery).\n\n>  void remove_sequencer_state(int aggressive)\n>  {\n>  \tstruct strbuf seq_dir = STRBUF_INIT;\n>  \tstruct strbuf seq_old_dir = STRBUF_INIT;\n> -\n>  \tstrbuf_addf(&seq_dir, \"%s\", git_path(SEQ_DIR));\n\nUnrelated change snuck in?\n\n> --- a/sequencer.h\n> +++ b/sequencer.h\n[...]\n> @@ -25,4 +49,7 @@ struct replay_insn_list {\n>   */\n>  void remove_sequencer_state(int aggressive);\n>  \n> +void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);\n> +int sequencer_pick_revisions(struct replay_opts *opts);\n> +\n\nI thought sequencer_parse_args() wasn't being exposed.\n\nExcept where noted above, I hope this is just simple code movement,\nbut I haven't checked.  If I could be sure, it would be easier to\nreview.\n\nCiao,\nJonathan\n"},{"id":"173494","messageId":"CALkWK0m8u6sVjAQN_cHP1tjR_mkt3Nu=BKEQH+P6e43DRm7iZw@mail.gmail.com","threadId":"28092","inReplyTo":"20110814125223.GE18466@elie.gateway.2wire.net","subject":"Re: [PATCH 5/7] revert: Make the argument parser responsible for setup_revisions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T13:43:19Z","receivedAt":"2011-08-14T13:43:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>\n>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n>\n> Since I wrote the patch modulo the commit message and small\n> improvements, shouldn't I be blamed as the author?  (git log\n> --grep=\"[Oo]riginal patch\" and git log --grep=jc: give some examples.)\n> You can have my sign-off if you'd like.\n\nI'd initially blamed you for writing it, but then I was worried that I\ndidn't have your signoff -- so I quickly changed it before sending it\nout.  Will fix in the next round.  Thanks for the clarification and\nsignoff :)\n\n-- Ram\n"},{"id":"173495","messageId":"CALkWK0=zqyvL8zo9wvBGUXyf3RWSZB7dY=WaC9TN6YXnThag0Q@mail.gmail.com","threadId":"28092","inReplyTo":"20110814131303.GF18466@elie.gateway.2wire.net","subject":"Re: [PATCH 6/7] sequencer: Expose API to cherry-picking machinery","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T13:57:18Z","receivedAt":"2011-08-14T13:57:18Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> Move code from builtin/revert.c to sequencer.c and expose a public API\n>> without making any functional changes.  Although it is useful only to\n>> existing callers of cherry-pick and revert now, this patch lays the\n>> foundation for future expansion.\n>\n> :)  It sounds like you are running a business.\n\n*laughs* Now that you mention it, it certainly does :)\n\n>> +++ b/sequencer.c\n>> @@ -1,13 +1,656 @@\n>> [...]\n>> +#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };\n\nI'll get rid of this.\n\n>> +static const char *action_keyword(const struct replay_opts *opts)\n>> [...]\n>\n> Another (non-functional) change.  Probably (?) this renaming has a\n> good reason to be part of this patch, but it should definitely be\n> mentioned in the commit message.\n\nYes, I want the rename to be part of this patch (see Daniel's comment\nand my agreement).  Will clarify in the commit message.\n\n> git_path() calls vsnprintf which clobbers errno, so depending on the\n> platform this can print messages like\n>\n>        fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success\n>\n> The natural fix would be to add a local for it (as a separate patch).\n> Sorry I missed this when the code first arrived.\n\nUgh, yet another \"bugfix patch\" to queue near the beginning of the\nseries.  Thanks for catching this.\n\n>> +static struct tree *empty_tree(void)\n>> [...]\n>\n> This tree is leaked (for example if you cherry-pick a sequence of\n> root commits).\n\nThis is not something I introduced -- it can wait until later, no?\n\n>> +static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n>> [...]\n>\n> The exit code here violates the usual \"exit with status 128 for\n> errors other than conflicts\" rule.  Perhaps it should be changed to\n> \"return -1\" in a separate patch (to accompany the patch that returns\n> error() instead of die()-ing so often to allow callers to give\n> additional context to errors from this machinery).\n\nGreat catch!  Will fix.\n\n>> --- a/sequencer.h\n>> +++ b/sequencer.h\n> [...]\n>> +void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);\n>\n> I thought sequencer_parse_args() wasn't being exposed.\n\nRebase fail, sorry.  There is no sequencer_parse_args().\n\n> Except where noted above, I hope this is just simple code movement,\n> but I haven't checked.  If I could be sure, it would be easier to\n> review.\n\nIt is a simple code movement.  Is there something I can do to help?\n\n-- Ram\n"},{"id":"173496","messageId":"CALkWK0=sggexB0HDN-6LsoLz2RO-ftABTnAYp7q-jVO0oCJu2A@mail.gmail.com","threadId":"28092","inReplyTo":"20110814121244.GD18466@elie.gateway.2wire.net","subject":"Re: [PATCH 4/7] revert: Allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T14:06:03Z","receivedAt":"2011-08-14T14:06:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> +             int len = strchrnul(p, '\\n') - p;\n>> +             if (len > 255)\n>> +                     len = 255;\n>> +             return error(_(\"Unrecognized action: %.*s\"), len, p);\n>\n> What happens if the current line has more than INT_MAX characters?\n\nI don't know!  I don't think this behavior is defined by the C89\nstandard, so I'd expect every compiler to do something different.  I'd\nexpect to be able to fix it like this:\n\nint len = strchrnul(p, '\\n') - p;\nif (len > 255 || len - strchrnul(p, '\\n') + p != 0)\n    len = 255;\n\n> Maybe it would make sense to factor out a function for this\n> computation, for brevity and so there is just one place to tweak.\n\nShouldn't it be part of error() atleast in the long term?  I'll write\na quick private helper for now.\n\nThanks.\n\n-- Ram\n"},{"id":"173497","messageId":"CALkWK0k46KPCZEeOrSHS22Ay-mGt+QPqiZetrc9V1YnhecDPUQ@mail.gmail.com","threadId":"28092","inReplyTo":"20110814115821.GC18466@elie.gateway.2wire.net","subject":"Re: [PATCH 2/7] revert: Fix buffer overflow in insn sheet parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T14:07:20Z","receivedAt":"2011-08-14T14:07:20Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> --- a/t/t3510-cherry-pick-sequence.sh\n>> +++ b/t/t3510-cherry-pick-sequence.sh\n>> @@ -211,4 +211,15 @@ test_expect_success 'malformed instruction sheet 2' '\n>> [...]\n>> +     sed \"s/pick \\([0-9a-f]\\+\\)\\(.*\\)/pick \\1\\1\\1\\1\\1\\1\\1\\1\\2/\" .git/sequencer/todo >new_sheet &&\n>\n> This construct (\\+ in sed regexes) is not portable.  See the note on\n> grep in Documentation/CodingGuidelines\n\nI see.  Could you show me how to do it right?  My regex-foo is pretty\nweak, and I don't know what else to do.\n\n> (maybe it should be tweaked to\n> say \"grep and sed\").\n\nOkay.  Do you like this?\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n-- 8< --\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex fe1c1e5..0a843ea 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -69,8 +69,8 @@ For shell scripts specifically (not exhaustive):\n  - We do not write the noiseword \"function\" in front of shell\n    functions.\n\n- - As to use of grep, stick to a subset of BRE (namely, no \\{m,n\\},\n-   [::], [==], nor [..]) for portability.\n+ - As to use of grep and sed, stick to a subset of BRE (namely, no\n+   \\{m,n\\}, [::], [==], nor [..]) for portability.\n\n    - We do not use \\{m,n\\};\n\n-- Ram\n"},{"id":"173498","messageId":"20110814142846.GI18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"CALkWK0=sggexB0HDN-6LsoLz2RO-ftABTnAYp7q-jVO0oCJu2A@mail.gmail.com","subject":"Re: [PATCH 4/7] revert: Allow mixed pick and revert instructions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T14:28:46Z","receivedAt":"2011-08-14T14:28:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> int len = strchrnul(p, '\\n') - p;\n> if (len > 255 || len - strchrnul(p, '\\n') + p != 0)\n>     len = 255;\n\nYuck.  Why not just use a size_t?\n\n> Shouldn't it be part of error() atleast in the long term?\n\nThe default implementation of error() is (practically speaking) a thin\nwrapper around fprintf.  What are the new semantics you are proposing\n--- truncating %s arguments to one short line?  I don't think that\nwould be a good idea, since for example, some current callers might be\npassing \"\\n\" explicitly as a %s argument.\n"},{"id":"173500","messageId":"20110814152204.GJ18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"CALkWK0=zqyvL8zo9wvBGUXyf3RWSZB7dY=WaC9TN6YXnThag0Q@mail.gmail.com","subject":"Re: [PATCH 6/7] sequencer: Expose API to cherry-picking machinery","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T15:22:04Z","receivedAt":"2011-08-14T15:22:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Jonathan Nieder writes:\n\n>> git_path() calls vsnprintf which clobbers errno, so depending on the\n>> platform this can print messages like\n>>\n>>        fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success\n[...]\n> Ugh, yet another \"bugfix patch\" to queue near the beginning of the\n> series.  Thanks for catching this.\n\nIt's not a bug you introduced, and ideally it would be a separate\npatch on its own, against the js/cherry-pick-usability branch.\n\n>> Ramkumar Ramachandra wrote:\n\n>>> +static struct tree *empty_tree(void)\n>>> [...]\n>>\n>> This tree is leaked (for example if you cherry-pick a sequence of\n>> root commits).\n>\n> This is not something I introduced -- it can wait until later, no?\n\nYep, it's Christian's fault (for introducing multiple-cherry-pick).\n\nPatch below.  The things I do... :)\n\n[...]\n> It is a simple code movement.  Is there something I can do to help?\n\nPart of the review was about examples where it was not simple code\nmovement.  Maybe if there is another round the thing to do would be to\nsend a patch generated with -B -M to allow others to more easily check\nit.\n\nThanks.\n\n-- >8 --\nSubject: revert: plug memory leak in \"cherry-pick root commit\" codepath\n\nThe empty tree passed as common ancestor to merge_trees() when\ncherry-picking a parentless commit is allocated on the heap and never\nfreed.  Leaking such a small one-time allocation is not a very big\nproblem, but now that \"git cherry-pick\" can cherry-pick multiple\ncommits it can start to add up.\n\nAvoid the leak by storing the fake tree exactly once in the BSS\nsection (i.e., use a static).  While at it, let's add a test to make\nsure cherry-picking multiple parentless commits continues to work.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nPatch is against cc/cherry-pick-series (86c7bb47).\n\n builtin/revert.c            |   10 +++++-----\n t/t3503-cherry-pick-root.sh |   27 ++++++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 853e9e40..c1b0fb3d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -273,12 +273,12 @@ static void write_message(struct strbuf *msgbuf, const char *filename)\n \n static struct tree *empty_tree(void)\n {\n-\tstruct tree *tree = xcalloc(1, sizeof(struct tree));\n+\tstatic struct tree tree;\n \n-\ttree->object.parsed = 1;\n-\ttree->object.type = OBJ_TREE;\n-\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);\n-\treturn tree;\n+\ttree.object.parsed = 1;\n+\ttree.object.type = OBJ_TREE;\n+\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree.object.sha1);\n+\treturn &tree;\n }\n \n static NORETURN void die_dirty_index(const char *me)\ndiff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh\nindex b0faa299..472e5b80 100755\n--- a/t/t3503-cherry-pick-root.sh\n+++ b/t/t3503-cherry-pick-root.sh\n@@ -16,15 +16,40 @@ test_expect_success setup '\n \techo second > file2 &&\n \tgit add file2 &&\n \ttest_tick &&\n-\tgit commit -m \"second\"\n+\tgit commit -m \"second\" &&\n+\n+\tgit symbolic-ref HEAD refs/heads/third &&\n+\trm .git/index file2 &&\n+\techo third > file3 &&\n+\tgit add file3 &&\n+\ttest_tick &&\n+\tgit commit -m \"third\"\n \n '\n \n test_expect_success 'cherry-pick a root commit' '\n \n+\tgit checkout second^0 &&\n \tgit cherry-pick master &&\n \ttest first = $(cat file1)\n \n '\n \n+test_expect_success 'cherry-pick two root commits' '\n+\n+\techo first >expect.file1 &&\n+\techo second >expect.file2 &&\n+\techo third >expect.file3 &&\n+\n+\tgit checkout second^0 &&\n+\tgit cherry-pick master third &&\n+\n+\ttest_cmp expect.file1 file1 &&\n+\ttest_cmp expect.file2 file2 &&\n+\ttest_cmp expect.file3 file3 &&\n+\tgit rev-parse --verify HEAD^^ &&\n+\ttest_must_fail git rev-parse --verify HEAD^^^\n+\n+'\n+\n test_done\n-- \n1.7.6\n"},{"id":"173501","messageId":"20110814160440.GK18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"1313310789-10216-8-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T16:04:40Z","receivedAt":"2011-08-14T16:04:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Since d3f4628e (revert: Remove sequencer state when no commits are\n> pending, 2011-07-06), the sequencer removes the sequencer state before\n> the final commit is actually completed.  This design is inherently\n> flawed, as it will not allow the user to abort the sequencer operation\n> at that stage.  Instead, make 'git commit' notify the sequencer after\n> every successful commit; the sequencer then removes the state if no\n> more instructions are pending.\n\nSorry, I'm getting lost in all the words.  I suspect you are saying\n“d3f4628e was trying to solve such-and-such problem, but its fix was\nproblematic because it removes the data that a hypothetical \"git\ncherry-pick --abort\" command would need to work.  Back out that\nchange and adopt the following instead.”\n\nIn particular, the above does not make it clear to me:\n\n - as a user, what effect will I notice after this change?\n - what problem does it solve?\n - does it have any downsides?\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -26,6 +26,7 @@\n>  #include \"unpack-trees.h\"\n>  #include \"quote.h\"\n>  #include \"submodule.h\"\n> +#include \"sequencer.h\"\n>  \n>  static const char * const builtin_commit_usage[] = {\n>  \t\"git commit [options] [--] <filepattern>...\",\n> @@ -1521,6 +1522,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \tunlink(git_path(\"MERGE_MODE\"));\n>  \tunlink(git_path(\"SQUASH_MSG\"));\n>  \n> +\t/*\n> +\t * Notify the sequencer that we're committing.  The routine\n> +\t * removes the sequencer state if our commit just completed\n> +\t * the last instruction.\n> +\t */\n> +\tsequencer_notify_commit();\n> +\n>  \tif (commit_index_files())\n>  \t\tdie (_(\"Repository has been updated, but unable to write\\n\"\n>  \t\t     \"new_index file. Check that disk is not full or quota is\\n\"\n\nThe function name (..._notify_commit()) does not seem very intuitive.\nBased on the name, I expect it to use the sequencer to print a message\nto the user about the commit in progress.\n\nWhat happens if writing to .git/index fails?  I can think of reasons\nto remove the sequencer file before or afterward:\n\n - before, because once .git/index has been removed, the index is not\n   locked any more and further commands could take place in parallel.\n\n - afterward, because when writing the index fails, I (the user)\n   might want to react by running \"git cherry-pick --abort\".\n\nThe latter seems slightly more compelling to me --- after all, any\nfurther command wanting to touch the sequencer directory is going\nto check whether it exists --- but more importantly, the former\nreminds me that we haven't thought carefully about what concurrent\noperations using the sequencer are and aren't allowed.  Though I\ndoubt that it would come up much in practice. :)\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -580,6 +580,17 @@ static void read_populate_todo(struct replay_insn_list **todo_list)\n>  \t\tdie(_(\"Unusable instruction sheet: %s\"), todo_file);\n>  }\n>  \n> +void sequencer_notify_commit(void)\n> +{\n> +\tstruct replay_insn_list *todo_list = NULL;\n> +\n> +\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n> +\t\treturn;\n> +\tread_populate_todo(&todo_list);\n> +\tif (!todo_list->next)\n> +\t\tremove_sequencer_state(1);\n> +}\n\nNot about this patch: I keep on forgetting what the argument to\nremove_sequencer_state means.  Would it be possible to make it\na flag, which would both make the meaning more obvious and mean\nit is easy to support additional flags in the future?\n\n> --- a/t/t3032-merge-recursive-options.sh\n> +++ b/t/t3032-merge-recursive-options.sh\n> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '\n>  \tgit read-tree --reset -u HEAD &&\n>  \ttest_must_fail git cherry-pick --no-commit remote &&\n>  \tgit read-tree --reset -u HEAD &&\n> +\tgit cherry-pick --reset &&\n>  \ttest_must_fail git cherry-pick remote &&\n>  \ttest_must_fail git update-index --refresh &&\n> +\tgit cherry-pick --reset &&\n>  \tgrep \"<<<<<<\" text.txt\n>  '\n\nWhat is this about?  Maybe it would be clearer to change the \"git\nread-tree ...\" to \"git reset --hard\", so the test assertion would not\nrely on new cherry-pick features (and to mention the change in the\ncommit message!).\n\nDoesn't this point to a risk in the patch?  Do you think there might\nbe scripts out there relying on being able to use \"git read-tree\n--reset -u HEAD\" to clear away a failed cherry-pick before trying\nagain, and if so, can we do something about it?\n\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' '\n>  \ttest_path_is_missing .git/sequencer\n>  '\n>  \n> -test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '\n> +test_expect_success 'final commit cleans up sequencer state' '\n>  \tpristine_detach initial &&\n>  \ttest_must_fail git cherry-pick base..picked &&\n> -\ttest_path_is_missing .git/sequencer &&\n>  \techo \"resolved\" >foo &&\n>  \tgit add foo &&\n>  \tgit commit &&\n> +\ttest_path_is_missing .git/sequencer &&\n>  \t{\n\nIt would also be nice to check \"test_path_is_dir\" before the final\ncommit, so people working on this code in the future know both aspects\nof the patch are intentional.\n\nThanks, I'm glad to see this patch.\n"},{"id":"173502","messageId":"20110814160923.GL18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"1313310789-10216-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/7] revert: Make commit descriptions in insn sheet optional","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T16:09:23Z","receivedAt":"2011-08-14T16:09:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -697,26 +697,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n>  \tunsigned char commit_sha1[20];\n>  \tchar sha1_abbrev[40];\n[...]\n> +\tq = p + strcspn(p, \" \\n\");\n> +\tif (q - p + 1 > sizeof(sha1_abbrev))\n>  \t\treturn NULL;\n\nIf I understand correctly, the current code makes it perfectly possible\nto use commands like\n\n\tpick origin/master^^2~5\n\nwhen that is more convenient to type.  So I wonder whether 40 is actually\na good limit.  Another possibility would be to do something like\n\n\tstatic struct strbuf cmit = STRBUF_INIT;\n\n\tstrbuf_reset(&cmit);\n\t...\n\tstrbuf_add(&cmit, p, q - p);\n\nto avoid having to worry about the buffer size altogether.\n"},{"id":"173503","messageId":"20110814161552.GM18466@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"1313310789-10216-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/7] revert: Free memory after get_message call","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T16:15:52Z","receivedAt":"2011-08-14T16:15:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> The format_todo function leaks memory because it forgets to call\n> free_message after get_message.  It is a potentially big leak, because\n> fresh memory is allocated to store the commit message message for each\n> commit.  Fix this.\n\nReading this, I think, \"why does formatting an insn list require all\nthese heavy memory allocations?\".  So while the patch looks good to\nme, I start to wonder if something using find_commit_subject()\ndirectly would not be easier.\n"},{"id":"173504","messageId":"CALkWK0kdd=+w4-jiBX8hh1_-mi9US+A4nng4qeoRRNmR5yDpDw@mail.gmail.com","threadId":"28092","inReplyTo":"20110814160923.GL18466@elie.gateway.2wire.net","subject":"Re: [PATCH 3/7] revert: Make commit descriptions in insn sheet optional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T16:21:21Z","receivedAt":"2011-08-14T16:21:21Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> If I understand correctly, the current code makes it perfectly possible\n> to use commands like\n>\n>        pick origin/master^^2~5\n\nI was thinking exactly the same thing a few hours ago!  I was\nwondering what to do about such a tree'ish specification, and I came\nto the conclusion that we should discourage this atleast for normal\nusers.  Why?  Because the tree'ish has to be invariant over the entire\nsequencer operation*: something like \"pick HEAD~3\" is clearly flawed\nsince the picking operation updates HEAD.\n\n* Some of us might find some really clever uses for a tree'ish that\nrefers to different commits during the course of the sequencer\noperation, and I'm alright with that.  It might just be something\nthat's too complicated for many users to grok, and we can protect them\nby not advertising this tree'ish operand feature.\n\n> when that is more convenient to type.  So I wonder whether 40 is actually\n> a good limit.  Another possibility would be to do something like\n>\n>        static struct strbuf cmit = STRBUF_INIT;\n\nI don't like this; the sequencer will just end up drinking a lot of\nmemory for no reason if the insn sheet is horribly corrupted.  I think\n40 is a very reasonable limit, even for the most complex tree'ish I\ncan dream of.\n\nThanks.\n\n-- Ram\n"},{"id":"173505","messageId":"CALkWK0m0KyZkOqVWJET=_Xy5GxwVfpPkB0OO1b2hJ8aN_x7hGA@mail.gmail.com","threadId":"28092","inReplyTo":"20110814160440.GK18466@elie.gateway.2wire.net","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-14T16:37:31Z","receivedAt":"2011-08-14T16:37:31Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> [...]\n>\n> The function name (..._notify_commit()) does not seem very intuitive.\n> Based on the name, I expect it to use the sequencer to print a message\n> to the user about the commit in progress.\n\nFor the record, I'm not too happy with the name either.  I was hoping\nthat someone would suggest a better name during the review :P\n\n> .git/index\n>  - afterward, because when writing the index fails, I (the user)\n>   might want to react by running \"git cherry-pick --abort\".\n\nVery subtle point: will fix.  I'd originally put it along with the\nother state-removing functions.\n\n> Not about this patch: I keep on forgetting what the argument to\n> remove_sequencer_state means.  Would it be possible to make it\n> a flag, which would both make the meaning more obvious and mean\n> it is easy to support additional flags in the future?\n\nI don't want to create a struct and populate it because I highly doubt\nremove_sequencer_state will take any more arguments in the future.\nPutting an \"int aggressive = 1\" and passing the variable instead of\nthe literal is a little inelegant.  Actually making any change at this\nstage is inelegant because of the other remove_sequencer_state() calls\n:|  My call: we can fix it if and when the function needs more\narguments.\n\n>> --- a/t/t3032-merge-recursive-options.sh\n>> +++ b/t/t3032-merge-recursive-options.sh\n>> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '\n>>       git read-tree --reset -u HEAD &&\n>>       test_must_fail git cherry-pick --no-commit remote &&\n>>       git read-tree --reset -u HEAD &&\n>> +     git cherry-pick --reset &&\n>>       test_must_fail git cherry-pick remote &&\n>>       test_must_fail git update-index --refresh &&\n>> +     git cherry-pick --reset &&\n>>       grep \"<<<<<<\" text.txt\n>>  '\n>\n> What is this about?  Maybe it would be clearer to change the \"git\n> read-tree ...\" to \"git reset --hard\", so the test assertion would not\n> rely on new cherry-pick features (and to mention the change in the\n> commit message!).\n\nOkay, I can do this instead (don't check whitespace! I just typed out\nthe patch):\n\n@@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '\n      git read-tree --reset -u HEAD &&\n      test_must_fail git cherry-pick --no-commit remote &&\n-     git read-tree --reset -u HEAD &&\n+     git reset --hard &&\n      test_must_fail git cherry-pick remote &&\n      test_must_fail git update-index --refresh &&\n+     git reset --hard &&\n      grep \"<<<<<<\" text.txt\n '\n\n> Doesn't this point to a risk in the patch?  Do you think there might\n> be scripts out there relying on being able to use \"git read-tree\n> --reset -u HEAD\" to clear away a failed cherry-pick before trying\n> again, and if so, can we do something about it?\n\nI'm not sure we can do anything about it -- we should probably put\nsome kind of warning in the commit message?\n\n>> --- a/t/t3510-cherry-pick-sequence.sh\n>> +++ b/t/t3510-cherry-pick-sequence.sh\n>> @@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' '\n>>       test_path_is_missing .git/sequencer\n>>  '\n>>\n>> -test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '\n>> +test_expect_success 'final commit cleans up sequencer state' '\n>>       pristine_detach initial &&\n>>       test_must_fail git cherry-pick base..picked &&\n>> -     test_path_is_missing .git/sequencer &&\n>>       echo \"resolved\" >foo &&\n>>       git add foo &&\n>>       git commit &&\n>> +     test_path_is_missing .git/sequencer &&\n>>       {\n>\n> It would also be nice to check \"test_path_is_dir\" before the final\n> commit, so people working on this code in the future know both aspects\n> of the patch are intentional.\n\nGood point.  Fixed.\n\nThanks.\n\n-- Ram\n"},{"id":"173506","messageId":"20110814164819.GA12747@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"CALkWK0m0KyZkOqVWJET=_Xy5GxwVfpPkB0OO1b2hJ8aN_x7hGA@mail.gmail.com","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T16:48:19Z","receivedAt":"2011-08-14T16:48:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Putting an \"int aggressive = 1\" and passing the variable instead of\n> the literal is a little inelegant.\n\nWhat about a flag word, like for example read_sha1_file_extended()\ntakes?\n\n[...]\n>> Do you think there might\n>> be scripts out there relying on being able to use \"git read-tree\n>> --reset -u HEAD\" to clear away a failed cherry-pick before trying\n>> again, and if so, can we do something about it?\n>\n> I'm not sure we can do anything about it -- we should probably put\n> some kind of warning in the commit message?\n\nThat's necessary at a minimum.\n\nI actually _don't_ think there might be many scripts relying on \"git\nread-tree --reset -u HEAD\" to clear away a failed cherry-pick, simply\nbecause very few people seem to use plumbing these days :) and those\nwho do might be of a mindset to use \"git apply\" instead of\ncherry-pick.  But it would be prudent to run a code search to check\nand to think carefully about the effect on people who did use the\n\"read-tree --reset && cherry-pick again\" idiom.\n\nHope that helps.\nJonathan\n"},{"id":"173516","messageId":"7vei0nn1cn.fsf@alter.siamese.dyndns.org","threadId":"28092","inReplyTo":"20110814160440.GK18466@elie.gateway.2wire.net","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-14T21:13:12Z","receivedAt":"2011-08-14T21:13:12Z","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>> Since d3f4628e (revert: Remove sequencer state when no commits are\n>> pending, 2011-07-06), the sequencer removes the sequencer state before\n>> the final commit is actually completed.  This design is inherently\n>> flawed, as it will not allow the user to abort the sequencer operation\n>> at that stage.  Instead, make 'git commit' notify the sequencer after\n>> every successful commit; the sequencer then removes the state if no\n>> more instructions are pending.\n>\n> Sorry, I'm getting lost in all the words.  I suspect you are saying\n> “d3f4628e was trying to solve such-and-such problem, but its fix was\n> problematic because it removes the data that a hypothetical \"git\n> cherry-pick --abort\" command would need to work.  Back out that\n> change and adopt the following instead.”\n\nIt still is unclear why \"removing the sequencer state when no more insns\nare pending\" is so necessary that the codebase needs to bend backwards to\nsupport that in the first place. What problem is d3f4628e really trying to\nsolve?\n\nIf \"who is responsible for removing a stale state\" is the issue it tries\nto address, Wouldn't it be much cleaner to make \"sequencer continue\"\nnotice that state and silently succeed, saying \"I am done\"?\n"},{"id":"173517","messageId":"20110814213200.GA6555@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"7vei0nn1cn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-14T21:32:00Z","receivedAt":"2011-08-14T21:32:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Junio,\n\nJunio C Hamano wrote:\n\n> It still is unclear why \"removing the sequencer state when no more insns\n> are pending\" is so necessary that the codebase needs to bend backwards to\n> support that in the first place. What problem is d3f4628e really trying to\n> solve?\n\nI believe it is meant to support command sequences such as these:\n\n1.\n\tgit cherry-pick foo; # has conflicts\n\t... resolve conflicts and \"git add\" the resolved files ...\n\tgit commit\n\tgit cherry-pick bar\n\n2.\n\tgit cherry-pick foo bar; # has conflicts applying \"bar\"\n\t... resolve ...\n\tgit commit\n\tgit cherry-pick baz\n\nThose were intuitive things to do before the sequencer existed, and if\nI understand correctly, d3f4628e was intended to support people and\nscripts (such as the test suite) that have these commands wired into\ntheir fingers.\n"},{"id":"173521","messageId":"7vippzlj7a.fsf@alter.siamese.dyndns.org","threadId":"28092","inReplyTo":"20110814213200.GA6555@elie.gateway.2wire.net","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-14T22:30:33Z","receivedAt":"2011-08-14T22:30:33Z","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> I believe it is meant to support command sequences such as these:\n>\n> 1.\n> \tgit cherry-pick foo; # has conflicts\n> \t... resolve conflicts and \"git add\" the resolved files ...\n> \tgit commit\n> \tgit cherry-pick bar\n\nWhy does a single commit \"cherry-pick foo\" leave any sequencer state that\nmay interfere with the latter to begin with? Isn't that already a bug?\n\n> 2.\n> \tgit cherry-pick foo bar; # has conflicts applying \"bar\"\n> \t... resolve ...\n> \tgit commit\n> \tgit cherry-pick baz\n>\n> Those were intuitive things to do before the sequencer existed, and if\n> I understand correctly, d3f4628e was intended to support people and\n> scripts (such as the test suite) that have these commands wired into\n> their fingers.\n\nGiven that the latter was broken when \"foo\" stopped with conficts (it lost\n\"bar\" altogether anyway), I am not worried about it, and I do not care\nmuch about anybody who had wired such multi-pick into scripts or fingers,\neither.\n\nIOW, I do not necessarily agree with your \"those were intuitive\"\nassertion.\n"},{"id":"173550","messageId":"7v7h6eld2c.fsf@alter.siamese.dyndns.org","threadId":"28092","inReplyTo":"7vippzlj7a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-15T18:55:23Z","receivedAt":"2011-08-15T18:55:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>> I believe it is meant to support command sequences such as these:\n>>\n>> 1.\n>> \tgit cherry-pick foo; # has conflicts\n>> \t... resolve conflicts and \"git add\" the resolved files ...\n>> \tgit commit\n>> \tgit cherry-pick bar\n>\n> Why does a single commit \"cherry-pick foo\" leave any sequencer state that\n> may interfere with the latter to begin with? Isn't that already a bug?\n>\n>> 2.\n>> \tgit cherry-pick foo bar; # has conflicts applying \"bar\"\n>> \t... resolve ...\n>> \tgit commit\n>> \tgit cherry-pick baz\n>>\n>> Those were intuitive things to do before the sequencer existed, and if\n>> I understand correctly, d3f4628e was intended to support people and\n>> scripts (such as the test suite) that have these commands wired into\n>> their fingers.\n>\n> Given that the latter was broken when \"foo\" stopped with conficts (it lost\n> \"bar\" altogether anyway), I am not worried about it, and I do not care\n> much about anybody who had wired such multi-pick into scripts or fingers,\n> either.\n>\n> IOW, I do not necessarily agree with your \"those were intuitive\"\n> assertion.\n\nAfter sleeping on this, here are my random thoughts on this topic.\n\n - The sequencing flow the current \"rebase -i\" implements when resuming a\n   controlled stop (i.e. \"edit\" or \"reword\"), even as the last step of the\n   insn sheet, feels like the right thing. The actual tweaking of the\n   commit done by \"commit --amend\" is oblivious to the sequencing state,\n   and resuming is controlled by \"rebase --continue\".\n\n - The case to resume an unexpected stop (i.e. \"pick\" that causes conflict\n   or \"rebase\" without \"-i\") also feels right. The user fixes up and marks\n   things as \"done\" with \"add/rm\", and tell the stopped command that it can\n   now continue with \"rebase --continue\". The same comment applies for\n   \"am\".\n\n - Even before we started talking about the sequencer, the workflow to deal\n   with conflicted \"revert\" or \"cherry-pick\" was awkward. From the end\n   user's point of view, the operation the end user wanted to perform was\n   \"revert\" (or \"cherry-pick\"), but we tell the user to fix things up, mark\n   them as \"done\" with \"add/rm\", and commit the result themselves. We could\n   have made the UI more consistent by introducing \"revert --continue\".\n\n - In a sense, the last point got worse by a recent change to make\n   \"commit\" notice CHERRY_PICK_HEAD. This was modeled the workflow to\n   conclude a conflicted \"merge\", that noticed \"MERGE_HEAD\" etc., but from\n   the UI point of view, I think it was a mistake. Back when \"merge\" was\n   the only command that needs post-clean-up by the user, saying \"when\n   merge stops due to conflict, you conclude it with commit, but\n   everything else you conclude (or continue) with --continue\" was\n   reasonable, but now we have to say \"after conflicted merge, revert and\n   cherry-pick, you conclude it with commit; use --continue for everything\n   else\".\n\nSo I think that questions that affect us longer term are:\n\n - Does it make sense to keep this two quite different ways to resume an\n   interrupted operation? Some saying \"The operation you started was Foo\n   but you need to conclude it with 'commit'\", others saying \"When Bar\n   stops in the middle, you say 'Bar --continue'\"?\n\n - If so, which camp should the sequencer-based commands fall into?\n\n - If not, shouldn't we be unifying the user experience to one of them,\n   with backward compatibility escape hatches?\n\nI think what d3f4628e tried to do may make sense _if_ we want to make\n\"commit\" the way to conclude or continue sequencer-based commands. If we\nreally wanted to go that route, however, it would make it easier if\n\"commit\" were the _only_ way to deal with all the \"stopped because the\nuser needs to resolve conflicts or because the user told us to pause\"\nsituations. For example, when \"rebase -i\" stops (either due to an \"edit\"\nor a conflicted \"pick\"), after the user tweaks the tree and says \"git\ncommit [--amend]\" to reword, \"rebase --continue\" after that shouldn't be\nnecessary. If we are making \"commit\" aware of the sequencer status, it\nshould already know that the next thing after that invocation of \"commit\"\nis to resume the sequencing.\n\nBut I do not think that is the direction we would want to go for two\nreasons. There is a (complicated) workflow to split an commit during\n\"rebase -i\" that does _not_ want an auto-resume by a commit. Also not\nall conflicted/stopped state can be concluded with \"commit\" (think:\n\"rebase/am --skip\").\n\nI personally am leaning towards teaching \"--continue\" to \"merge\" and\n\"cherry-pick\", while keeping only existing awareness of these two commands\nin \"commit\" as historical mistakes (look for 'unlink(git_path(\"[A-Z_]*\"))'\nin builtin/commit.c). That would mean that in the long run, new users need\nto learn only one thing: when \"git Foo\" stops because it needs help from\nyou resolving conflicts, after you help it by editing the files in your\nworking tree and making that with add/rm, you say \"git Foo --continue\" to\nsignal that you are done helping it.\n\nAnd we do not have to worry about making \"commit\" only half-aware of\nsequencer states.\n"},{"id":"173624","messageId":"7v39h1i6rr.fsf@alter.siamese.dyndns.org","threadId":"28092","inReplyTo":"20110814152204.GJ18466@elie.gateway.2wire.net","subject":"Re: [PATCH 6/7] sequencer: Expose API to cherry-picking machinery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-16T17:51:52Z","receivedAt":"2011-08-16T17:51:52Z","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> Subject: revert: plug memory leak in \"cherry-pick root commit\" codepath\n>\n> The empty tree passed as common ancestor to merge_trees() when\n> cherry-picking a parentless commit is allocated on the heap and never\n> freed.\n\nThanks for noticing, but shouldn't we be just using\n\n\tlookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN)\n\nor something, instead of hand-crafting a fake tree object?\n"},{"id":"173626","messageId":"20110816181633.GB10336@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"7v39h1i6rr.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] revert: plug memory leak in \"cherry-pick root commit\" codepath","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-16T18:16:33Z","receivedAt":"2011-08-16T18:16:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Thanks for noticing, but shouldn't we be just using\n>\n> \tlookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN)\n>\n> or something, instead of hand-crafting a fake tree object?\n\nYes.  Ever since v1.5.5-rc0~180^2~1 (hard-code the empty tree object,\n2008-02-13), there is no need to call pretend_sha1_file() again to\nget a fake empty tree object, so this lookup_tree() should work\nfine.\n\n-- >8 --\nSubject: revert: plug memory leak in \"cherry-pick root commit\" codepath\n\nFor each parentless commit it is asked to reuse, \"git cherry-pick\" and\n\"git revert\" hand-craft a fake parent tree object on the heap to pass\nto merge_trees().  Leaking such a small one-time allocation would not\nbe a big deal, but now that cherry-pick/revert can take multiple\ncommit arguments, it can start to add up.\n\nThe fix is simple: don't create a new fake empty tree at all, but rely\non the built-in one that has existed since 346245a1 (hard-code the\nempty tree object, 2008-02-13).\n\nWhile at it, add a test to make sure cherry-picking multiple\nparentless commits continues to work.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nImproved-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/revert.c            |    7 +------\n t/t3503-cherry-pick-root.sh |   27 ++++++++++++++++++++++++++-\n 2 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 853e9e40..a26a7c93 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -273,12 +273,7 @@ static void write_message(struct strbuf *msgbuf, const char *filename)\n \n static struct tree *empty_tree(void)\n {\n-\tstruct tree *tree = xcalloc(1, sizeof(struct tree));\n-\n-\ttree->object.parsed = 1;\n-\ttree->object.type = OBJ_TREE;\n-\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);\n-\treturn tree;\n+\treturn lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);\n }\n \n static NORETURN void die_dirty_index(const char *me)\ndiff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh\nindex b0faa299..472e5b80 100755\n--- a/t/t3503-cherry-pick-root.sh\n+++ b/t/t3503-cherry-pick-root.sh\n@@ -16,15 +16,40 @@ test_expect_success setup '\n \techo second > file2 &&\n \tgit add file2 &&\n \ttest_tick &&\n-\tgit commit -m \"second\"\n+\tgit commit -m \"second\" &&\n+\n+\tgit symbolic-ref HEAD refs/heads/third &&\n+\trm .git/index file2 &&\n+\techo third > file3 &&\n+\tgit add file3 &&\n+\ttest_tick &&\n+\tgit commit -m \"third\"\n \n '\n \n test_expect_success 'cherry-pick a root commit' '\n \n+\tgit checkout second^0 &&\n \tgit cherry-pick master &&\n \ttest first = $(cat file1)\n \n '\n \n+test_expect_success 'cherry-pick two root commits' '\n+\n+\techo first >expect.file1 &&\n+\techo second >expect.file2 &&\n+\techo third >expect.file3 &&\n+\n+\tgit checkout second^0 &&\n+\tgit cherry-pick master third &&\n+\n+\ttest_cmp expect.file1 file1 &&\n+\ttest_cmp expect.file2 file2 &&\n+\ttest_cmp expect.file3 file3 &&\n+\tgit rev-parse --verify HEAD^^ &&\n+\ttest_must_fail git rev-parse --verify HEAD^^^\n+\n+'\n+\n test_done\n-- \n1.7.6\n"},{"id":"173627","messageId":"20110816182739.GC10336@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"20110816181633.GB10336@elie.gateway.2wire.net","subject":"Re: [PATCH v2] revert: plug memory leak in \"cherry-pick root commit\" codepath","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-16T18:27:39Z","receivedAt":"2011-08-16T18:27:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n\n>> Thanks for noticing, but shouldn't we be just using\n>>\n>> \tlookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN)\n>>\n>> or something, instead of hand-crafting a fake tree object?\n>\n> Yes.\n\nAnd here's another patch in the same spirit (no test this time,\nthough).\n\n-- >8 --\nSubject: merge-recursive: take advantage of hardcoded empty tree\n\nWhen this code was first written (v1.4.3-rc1~174^2~4, merge-recur: if\nthere is no common ancestor, fake empty one, 2006-08-09), everyone\nneeding a fake empty tree had to make her own, but ever since\nv1.5.5-rc0~180^2~1 (2008-02-13), the object lookup machinery provides\na ready-made one.  Use it.\n\nThis is just a simplification, though it also fixes a small leak\n(since the tree in the virtual common ancestor commit is never freed).\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n merge-recursive.c |    8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 206c1036..7695fe89 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1309,12 +1309,10 @@ int merge_recursive(struct merge_options *o,\n \n \tmerged_common_ancestors = pop_commit(&ca);\n \tif (merged_common_ancestors == NULL) {\n-\t\t/* if there is no common ancestor, make an empty tree */\n-\t\tstruct tree *tree = xcalloc(1, sizeof(struct tree));\n+\t\t/* if there is no common ancestor, use an empty tree */\n+\t\tstruct tree *tree;\n \n-\t\ttree->object.parsed = 1;\n-\t\ttree->object.type = OBJ_TREE;\n-\t\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);\n+\t\ttree = lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);\n \t\tmerged_common_ancestors = make_virtual_commit(tree, \"ancestor\");\n \t}\n \n-- \n1.7.6\n"},{"id":"173628","messageId":"20110816183147.GA10117@sigill.intra.peff.net","threadId":"28092","inReplyTo":"20110816181633.GB10336@elie.gateway.2wire.net","subject":"Re: [PATCH v2] revert: plug memory leak in \"cherry-pick root commit\" codepath","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-16T18:31:48Z","receivedAt":"2011-08-16T18:31:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 16, 2011 at 01:16:33PM -0500, Jonathan Nieder wrote:\n\n>  static struct tree *empty_tree(void)\n>  {\n> -\tstruct tree *tree = xcalloc(1, sizeof(struct tree));\n> -\n> -\ttree->object.parsed = 1;\n> -\ttree->object.type = OBJ_TREE;\n> -\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);\n> -\treturn tree;\n> +\treturn lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);\n>  }\n\nMuch nicer. But doesn't your dab0d41 (correct type of\nEMPTY_TREE_SHA1_BIN, 2011-02-07) make the cast unnecessary?\n\n-Peff\n"},{"id":"173630","messageId":"20110816185616.GE10336@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"20110816183147.GA10117@sigill.intra.peff.net","subject":"Re: [PATCH v2] revert: plug memory leak in \"cherry-pick root commit\" codepath","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-16T18:56:16Z","receivedAt":"2011-08-16T18:56:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Aug 16, 2011 at 01:16:33PM -0500, Jonathan Nieder wrote:\n\n>>  static struct tree *empty_tree(void)\n>>  {\n>> -\tstruct tree *tree = xcalloc(1, sizeof(struct tree));\n>> -\n>> -\ttree->object.parsed = 1;\n>> -\ttree->object.type = OBJ_TREE;\n>> -\tpretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);\n>> -\treturn tree;\n>> +\treturn lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);\n>>  }\n>\n> Much nicer. But doesn't your dab0d41 (correct type of\n> EMPTY_TREE_SHA1_BIN, 2011-02-07) make the cast unnecessary?\n\nYes, I was working against an older codebase (for no particular\nreason).\n"},{"id":"173693","messageId":"CALkWK0nKiVdeGe2Q9pHA1JO1jyXMomjMpO8f5FqmmgXOEvOwmQ@mail.gmail.com","threadId":"28092","inReplyTo":"7v7h6eld2c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-17T20:23:07Z","receivedAt":"2011-08-17T20:23:07Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano writes:\n> After sleeping on this, here are my random thoughts on this topic.\n\nThey're a bit much for me to absorb so quickly.  I'll think about this\nagain before posting a more elaborate reply.\n\nThanks for your patience.\n\n-- Ram\n"},{"id":"173769","messageId":"CALkWK0noHBnW-7zZLw=jJdDVFxXmsm2vHHYnUJc9miLLuDRnAg@mail.gmail.com","threadId":"28092","inReplyTo":"7v7h6eld2c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-18T18:51:24Z","receivedAt":"2011-08-18T18:51:24Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano writes:\n> [...]\n\nHere are some comments from my end after extensive thought.  Instead\nof responding to your comments directly, I've sprinkled bits of it\nhere and there, reordering it as I see fit.  Thanks for detailing out\nyour thoughts so well :)\n\n0. Before getting into historical mistakes and evaluating existing\nworkflows, let's first try to detail the problem the sequencer is\ntrying to solve: pick/ revert aren't the only operations we want to\nsupport.  Let's imagine some hypothetical \"foo\" operation that does\nsomething incredibly complex in the instruction sheet:\n\npick b38bcc\nfoo\npick 49ab7c\n\nNow, let's imagine that the \"foo\" command failed.  What is the\nstrategy we will employ to handle his?  I think the question boils\ndown to: should we teach '--continue' the prerequisite for executing a\n\"pick\" command (clean worktree etc), or how to finish a \"foo\" command?\n What will this answer depend upon?\n\n1. Now, let's discuss the motivation for d3f4628e.  I didn't want to\ncreate a separate workflow for pick-one-commit versus\npick-many-commits.  It would have to go something like:\n\n  parse_arguments(whatever tree'ishes specified on argc)\n  setup_revision_walker\n  n = count_revisions\n  if (n == 1)\n    do foo; // Don't create sequencer state\n  else\n    create sequencer state; do foo; cleanup sequencer state;\n\nUgly.  Wouldn't you agree?  So, what choice did I have?  I can't\nconditionally create sequencer state.  So, an idea struck me: if I\nclean it up before someone notices, I'm safe.  So, I decided to remove\nit before the last commit -- when there's only one commit, it'll be\nremoved immediately.  So far so good: no regressions introduced.\n\n2.  Okay, now let's try to touch upon the issue of marking something as \"done\":\n\n>  - The case to resume an unexpected stop (i.e. \"pick\" that causes conflict\n>   or \"rebase\" without \"-i\") also feels right. The user fixes up and marks\n>   things as \"done\" with \"add/rm\", and tell the stopped command that it can\n>   now continue with \"rebase --continue\". The same comment applies for\n>   \"am\".\n\nI could argue that \"add/rm\" need not be the only way to specify \"done\"\nin the general case.  We needn't be consistent about that- we can\nprint hints about what \"done\" would mean, and not invent a new\n\"--mark-as-done\" command.  What varies here is the definition of\n\"done\": does it mean that the user has \"done\" what the instruction\nsheet operation was supposed to do (including the committing), or has\nsimply done a part of the job and passed on the baton?\n\n3. How tight do we want the sequencer to be?  Should we allow\nexecuting other commands during the operation of the sequencer, or\nshould we simply expect the user to \"--continue\" the operation first\nand take care of everything else later?\n\n>  - The sequencing flow the current \"rebase -i\" implements when resuming a\n>   controlled stop (i.e. \"edit\" or \"reword\"), even as the last step of the\n>   insn sheet, feels like the right thing. The actual tweaking of the\n>   commit done by \"commit --amend\" is oblivious to the sequencing state,\n>   and resuming is controlled by \"rebase --continue\".\n\nI often \"git commit -m temp\" in the middle of a \"rebase -i\", and I'm\nstrongly in favor of allowing this somehow.\n\n> There is a (complicated) workflow to split an commit during\n> \"rebase -i\" that does _not_ want an auto-resume by a commit.\n\nExactly! :)\nAuto-resuming after a \"commit\" is fundamentally wrong.\n\n> Also not\n> all conflicted/stopped state can be concluded with \"commit\" (think:\n> \"rebase/am --skip\").\n\nA good point on consistency.\n\n4.  Let's correct the historical mistakes and get our act together.\nThe final pending question is: how many historical mistakes are we\nwilling to stomp now?\n\n> I personally am leaning towards teaching \"--continue\" to \"merge\" and\n> \"cherry-pick\", while keeping only existing awareness of these two commands\n> in \"commit\" as historical mistakes (look for 'unlink(git_path(\"[A-Z_]*\"))'\n> in builtin/commit.c). That would mean that in the long run, new users need\n> to learn only one thing: when \"git Foo\" stops because it needs help from\n> you resolving conflicts, after you help it by editing the files in your\n> working tree and making that with add/rm, you say \"git Foo --continue\" to\n> signal that you are done helping it.\n\nOkay, stomp nothing.  Under this constraint, the best we can do is:\n1. Introduce a 'merge --continue' to invoke 'git commit'.  MERGE_HEAD\nhelps 'git commit' finish.  Modify tests to use '--continue' instead\nof the earlier commit-to-finish workflow, and advertise this feature\neverywhere.\n2. Make 'cherry-pick --continue' invoke 'git commit' as well.\nCHERRY_PICK_HEAD helps 'git commit' finish.  If the commit finishes\nsuccessfully: (if there is one commit left, remove the sequencer\nstate; otherwise, drop the first insn on top of the list and execute\nthe next insn).  Modify tests to use '--continue' instead of the\nearlier commit-to-finish workflow, and advertise this feature\neverywhere.  Unfortunately, if the user executes 'git commit' instead\nof the newer '--continue', we're screwed: a stray sequencer state will\nbe left behind.\n\nDoes the above seem sensible, or should we do something about the\nhistorical mistakes?\n\nThanks.\n\n-- Ram\n"},{"id":"173771","messageId":"20110818191812.GG30436@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"CALkWK0noHBnW-7zZLw=jJdDVFxXmsm2vHHYnUJc9miLLuDRnAg@mail.gmail.com","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-18T19:18:12Z","receivedAt":"2011-08-18T19:18:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Ram,\n\nRamkumar Ramachandra wrote:\n\n> Here are some comments from my end after extensive thought.\n\nCould be briefer. :)\n\n[...]\n> 1. Introduce a 'merge --continue' to invoke 'git commit'.  MERGE_HEAD\n> helps 'git commit' finish.  Modify tests to use '--continue' instead\n> of the earlier commit-to-finish workflow, and advertise this feature\n> everywhere.\n\nWhy modify tests?  I think \"git merge --continue\" is a nice idea,\nand I don't see how it's inconsistent in any way with continuing to\nallow old practice.\n\n> 2. Make 'cherry-pick --continue' invoke 'git commit' as well.\n> CHERRY_PICK_HEAD helps 'git commit' finish.  If the commit finishes\n> successfully: (if there is one commit left, remove the sequencer\n> state; otherwise, drop the first insn on top of the list and execute\n> the next insn).\n\nSounds like a sensible thing to do.  I assume the \"one\" in the\nparenthesis is supposed to be \"zero\", making the \"if\" not even part of\nthe user-visible description of what it does --- it's just the\ntermination condition of a loop.\n\n\"git cherry-pick --continue\" in place of \"git commit\" does not handle\nthe following scenario.  Suppose my multiple-cherry-pick has run into\nconflicts, and while fixing them I notice something related that needs\nto be fixed.\n\n\t... resolve conflict, leaving extra change in worktree ...\n\tgit stash -k\n\t... test test test ...\n\tgit commit\n\n\tgit stash pop\n\tgit commit; # make a separate commit for extra change\n\n\t# ok, now continue.\n\tgit sequencer --continue\n\nIn other words, in this sequence of commands, \"git commit\" is used to\nsingle-step.  So if one wants to remove CHERRY_PICK_HEAD altogether, a\nnice thing to do would be to introduce a \"git sequencer --single-step\"\ncommand or something similar to handle such cases.\n\n> Modify tests to use '--continue' instead of the\n> earlier commit-to-finish workflow, and advertise this feature\n> everywhere.  Unfortunately, if the user executes 'git commit' instead\n> of the newer '--continue', we're screwed: a stray sequencer state will\n> be left behind.\n\nAs Junio hinted, it could make a lot of sense for \"git cherry-pick\n<single commit>\" to not create sequencer state in the first place.\n\"git cherry-pick --continue\" does not need it --- it is enough to\ncommit with the conflict resolved.  \"git cherry-pick --abort\" does not\nneed it, either --- it is enough to \"git reset --merge HEAD\".\n\nOne part I'm handwaving is what to do about commands like \"git\ncherry-pick foo^..foo\" which use a commit range that only happens to\ncontain one commit.  Either behavior seems fine for such commands.\n\nWhat do you think?\n"},{"id":"173773","messageId":"CALkWK0=jRAq6s1zQ5gwB4feBgC1eo=VYLWx8bsjs+exqmz0f1A@mail.gmail.com","threadId":"28092","inReplyTo":"20110818191812.GG30436@elie.gateway.2wire.net","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-18T19:50:47Z","receivedAt":"2011-08-18T19:50:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Could be briefer. :)\n\nSorry about the braindump :P\n\n>> 1. Introduce a 'merge --continue' to invoke 'git commit'.  MERGE_HEAD\n>> helps 'git commit' finish.  Modify tests to use '--continue' instead\n>> of the earlier commit-to-finish workflow, and advertise this feature\n>> everywhere.\n>\n> Why modify tests?  I think \"git merge --continue\" is a nice idea,\n> and I don't see how it's inconsistent in any way with continuing to\n> allow old practice.\n\nIn the future, we might want a 'merge' instruction in the sequencer --\nI want to make it clear that we're going for a significant UI change\nso that everyone (including tests, scripts) become comfortable with\nthe new UI.\n\n>> 2. Make 'cherry-pick --continue' invoke 'git commit' as well.\n>> CHERRY_PICK_HEAD helps 'git commit' finish.  If the commit finishes\n>> successfully: (if there is one commit left, remove the sequencer\n>> state; otherwise, drop the first insn on top of the list and execute\n>> the next insn).\n>\n> Sounds like a sensible thing to do.  I assume the \"one\" in the\n> parenthesis is supposed to be \"zero\", making the \"if\" not even part of\n> the user-visible description of what it does --- it's just the\n> termination condition of a loop.\n\nRight, sorry about the convoluted thought.\n\n> As Junio hinted, it could make a lot of sense for \"git cherry-pick\n> <single commit>\" to not create sequencer state in the first place.\n> \"git cherry-pick --continue\" does not need it --- it is enough to\n> commit with the conflict resolved.  \"git cherry-pick --abort\" does not\n> need it, either --- it is enough to \"git reset --merge HEAD\".\n\nOkay, here's my problem with the idea: it'll essentially require the\nsequencer to differentiate between one-commit operations and\nmany-commit operations.  In the case of one-commit operations, *every*\nnew command that calls into the sequencer will will need to persist\ninformation in its own way using hacks like CHERRY_PICK_HEAD and\nMERGE_HEAD.  And we have to make \"git commit\" unlink yet another file\n:)  I'm not talking about some hypothetical case: I'm already planning\nto make 'git am' call into the sequencer, so we'll need an AM_HEAD.\n\nOne final resort: Move some code back into cherry-pick, and call into\na later-function in the sequencer only if it's a many-commit\noperation.  The new commands can enjoy the comfort of calling into an\nearlier-function in the sequencer that'll do all the revision walk\nsetup and call the later-function.  I think this is reasonable.\n\n> One part I'm handwaving is what to do about commands like \"git\n> cherry-pick foo^..foo\" which use a commit range that only happens to\n> contain one commit.  Either behavior seems fine for such commands.\n\nI don't think I follow.  This will be determined as a single-commit\noperation after setting up the revisions.  I don't think it should be\ntreated as a multi-commit operation because the literal tree'ish\ncontains \"..\".\n\n-- Ram\n"},{"id":"173779","messageId":"20110818200520.GI30436@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"CALkWK0=jRAq6s1zQ5gwB4feBgC1eo=VYLWx8bsjs+exqmz0f1A@mail.gmail.com","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-18T20:05:20Z","receivedAt":"2011-08-18T20:05:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> In the future, we might want a 'merge' instruction in the sequencer --\n> I want to make it clear that we're going for a significant UI change\n> so that everyone (including tests, scripts) become comfortable with\n> the new UI.\n\nI don't follow.  I meant \"Why modify tests, rather than adding new\nones?\"  Tests document what is supposed to continue working.\n\n[...]\n> Okay, here's my problem with the idea: it'll essentially require the\n> sequencer to differentiate between one-commit operations and\n> many-commit operations.  In the case of one-commit operations, *every*\n> new command that calls into the sequencer will will need to persist\n> information in its own way using hacks like CHERRY_PICK_HEAD and\n> MERGE_HEAD.\n\nWhy wouldn't such a backward-compatibility feature apply to\ncherry-pick/revert only?\n\n> I'm not talking about some hypothetical case: I'm already planning\n> to make 'git am' call into the sequencer, so we'll need an AM_HEAD.\n\nYuck.  How does the UI distinguish between\n\n\tgit sequencer --continue; # apply the rest of the patches from mbox\n\nand\n\n\tgit sequencer --continue; # finish up \"am\" insn and continue\n\n?  Does the sequencer need an argument to indicate the level of\nnesting?  I would find it more straightforward for \"git am mbox\" to\ncreate a todo list saying something like\n\n\tapply patch 1 from mbox\n\tapply patch 2 from mbox\n\tapply patch 3 from mbox\n\tapply patch 4 from mbox\n\nso there would still be only one pending sequence of basic\ninstructions to think about.\n\n> One final resort: Move some code back into cherry-pick, and call into\n> a later-function in the sequencer only if it's a many-commit\n> operation.  The new commands can enjoy the comfort of calling into an\n> earlier-function in the sequencer that'll do all the revision walk\n> setup and call the later-function.  I think this is reasonable.\n\nI'm having trouble parsing this; sorry.  What is the effect on the\nuser-visible behavior of the program of what you propose?\n\n> Jonathan Nieder writes:\n\n>> One part I'm handwaving is what to do about commands like \"git\n>> cherry-pick foo^..foo\" which use a commit range that only happens to\n>> contain one commit.  Either behavior seems fine for such commands.\n>\n> I don't think I follow.  This will be determined as a single-commit\n> operation after setting up the revisions.  I don't think it should be\n> treated as a multi-commit operation because the literal tree'ish\n> contains \"..\".\n\nI said \"Either behavior seems fine\", didn't I?\n\nHope that clarifies a little.\nJonathan\n"},{"id":"173780","messageId":"CALkWK0nV35gNFM2NW-FrwBLnvFzmWDdARZk0Xy6LT8c2UE0fAw@mail.gmail.com","threadId":"28092","inReplyTo":"CALkWK0=jRAq6s1zQ5gwB4feBgC1eo=VYLWx8bsjs+exqmz0f1A@mail.gmail.com","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-18T20:06:05Z","receivedAt":"2011-08-18T20:06:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"HI again,\n\nRamkumar Ramachandra writes:\n> One final resort: Move some code back into cherry-pick, and call into\n> a later-function in the sequencer only if it's a many-commit\n> operation.  The new commands can enjoy the comfort of calling into an\n> earlier-function in the sequencer that'll do all the revision walk\n> setup and call the later-function.  I think this is reasonable.\n\nEr, no.  This would be amazingly ugly too: we'd need two versions of\neverything like '--continue'.  And look at this glaring inconsistency:\n\n  git cherry-pick foo..bar\n  ..conflict..\n  git sequencer --abort\n  $? = 0\n\nBut:\n\n  git cherry-pick foo\n  ..conflict..\n  git sequencer --abort\n  error: No sequencer operation in progress.\n\nIn a nutshell, the problem we're facing: We persist part of the\ncontinuation data outside the sequencer's knowledge, and part of it\ninside.  For backward compatibility, we can't move the former part\ninto the sequencer.\n\n-- Ram\n"},{"id":"173781","messageId":"20110818201220.GJ30436@elie.gateway.2wire.net","threadId":"28092","inReplyTo":"CALkWK0nV35gNFM2NW-FrwBLnvFzmWDdARZk0Xy6LT8c2UE0fAw@mail.gmail.com","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-18T20:12:20Z","receivedAt":"2011-08-18T20:12:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> But:\n>\n>   git cherry-pick foo\n>   ..conflict..\n>   git sequencer --abort\n>   error: No sequencer operation in progress.\n\nI mentioned this before: \"git sequencer --abort\" could notice that\nCHERRY_PICK_HEAD exists and .git/sequencer does not and act\naccordingly.\n\n> In a nutshell, the problem we're facing: We persist part of the\n> continuation data outside the sequencer's knowledge, and part of it\n> inside.  For backward compatibility, we can't move the former part\n> into the sequencer.\n\nWhy can't it become the sequencer's responsibility, FWIW?  That's an\nimplementation detail.\n"},{"id":"173782","messageId":"7v8vqqa2qu.fsf@alter.siamese.dyndns.org","threadId":"28092","inReplyTo":"CALkWK0=jRAq6s1zQ5gwB4feBgC1eo=VYLWx8bsjs+exqmz0f1A@mail.gmail.com","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T20:22:49Z","receivedAt":"2011-08-18T20:22:49Z","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>> Why modify tests?  I think \"git merge --continue\" is a nice idea,\n>> and I don't see how it's inconsistent in any way with continuing to\n>> allow old practice.\n\nI agree. Updating the test will hide a regression if Ram's update breaks\nthe existing workflow to conclude a conflicted merge with \"git commit\".\nIf we are to add \"git merge --continue\" sugarcoat to make it easier to\nteach new people saying that \"To any Git operation that stops and asks you\nto help, you can tell it that you are done helping by re-running the same\ncommand with --continue flag\", then _new_ tests should be added to make\nsure that \"git merge --continue\" does act just like \"git commit\" in such a\ncase.\n\n> In the future, we might want a 'merge' instruction in the sequencer --\n\nThe end-user facing frontend may be \"git rebase\" in such a case, and we\nwould be teaching the users \"When you are done helping the conflicted\n'rebase', tell it that you are done by running 'rebase --continue'\".  The\ncommand verb 'merge' on the sequencer insn sheet does not have any direct\nconnection to 'git merge' command (e.g. 'pick' does not have to be and is\nnot implemented using 'git pick' command that does not exist). So I do not\nthink we would ever say \"merge --continue\" in this context to begin with.\n\n>> As Junio hinted, it could make a lot of sense for \"git cherry-pick\n>> <single commit>\" to not create sequencer state in the first place.\n>> \"git cherry-pick --continue\" does not need it --- it is enough to\n>> commit with the conflict resolved.  \"git cherry-pick --abort\" does not\n>> need it, either --- it is enough to \"git reset --merge HEAD\".\n>\n> Okay, here's my problem with the idea: it'll essentially require the\n> sequencer to differentiate between one-commit operations and\n> many-commit operations.\n\nI think you are looking at it in a wrong way. It is just about giving\nbackward compatibility to historical hacks. cherry-pick and revert may be\nthe only ones needed, and a new command Foo that implements its workflow\nin terms of the sequencer can choose to (and should choose to unless there\nis a compelling reason not to, because of the exact reason you stated\nhere) do a single-command insn sheet with \"git Foo --continue\" to conclude\nif that one and only step needed help from the end user.\n \n\"git am\" would fit in the picture the same way. The sequencer insn sheet\nit would produce would consist of \"patch <filename>\" or something, and\nafter the last one of them fails, the user would fix things up in the\nworking tree, \"git am --continue\" will create a commit, and because there\nis no more steps in the sequence, the sequencer state will be removed.\n"},{"id":"173784","messageId":"7vvctu8naf.fsf@alter.siamese.dyndns.org","threadId":"28092","inReplyTo":"20110818191812.GG30436@elie.gateway.2wire.net","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-18T20:42:00Z","receivedAt":"2011-08-18T20:42:00Z","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> One part I'm handwaving is what to do about commands like \"git\n> cherry-pick foo^..foo\" which use a commit range that only happens to\n> contain one commit.\n\nHistorically cherry-pick took a single commit from the command line. That\ncan easily be distinguished from a \"set that ended up with one positive\ncommit after walking the history\". I think you can even check before\ntriggering the revision walking machinery in the first place.\n"},{"id":"173842","messageId":"CALkWK0mb_9DzV_Yb6ZGzFErrMLH6NAvXWE_MuEohp=oOMjaq4g@mail.gmail.com","threadId":"28092","inReplyTo":"7vvctu8naf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-19T09:08:30Z","receivedAt":"2011-08-19T09:08:30Z","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> Why can't it become the sequencer's responsibility, FWIW?  That's an\n> implementation detail.\n\nIt can be the sequencer's responsibility if we want that!  I'm just\ndabbling with the different implementation strategies and trying to\nsee the advantages/ disadvantages of each one.  The \"right\" thing to\ndo wasn't obvious to me immediately.\n\nJunio C Hamano writes:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>>> Why modify tests?  I think \"git merge --continue\" is a nice idea,\n>>> and I don't see how it's inconsistent in any way with continuing to\n>>> allow old practice.\n>\n> I agree. Updating the test will hide a regression if Ram's update breaks\n> the existing workflow to conclude a conflicted merge with \"git commit\".\n> If we are to add \"git merge --continue\" sugarcoat to make it easier to\n> teach new people saying that \"To any Git operation that stops and asks you\n> to help, you can tell it that you are done helping by re-running the same\n> command with --continue flag\", then _new_ tests should be added to make\n> sure that \"git merge --continue\" does act just like \"git commit\" in such a\n> case.\n\nRight.  We have to keep the old tests around -- my bad.\n\n>>> As Junio hinted, it could make a lot of sense for \"git cherry-pick\n>>> <single commit>\" to not create sequencer state in the first place.\n>>> \"git cherry-pick --continue\" does not need it --- it is enough to\n>>> commit with the conflict resolved.  \"git cherry-pick --abort\" does not\n>>> need it, either --- it is enough to \"git reset --merge HEAD\".\n>>\n>> Okay, here's my problem with the idea: it'll essentially require the\n>> sequencer to differentiate between one-commit operations and\n>> many-commit operations.\n>\n> I think you are looking at it in a wrong way. It is just about giving\n> backward compatibility to historical hacks. cherry-pick and revert may be\n> the only ones needed, and a new command Foo that implements its workflow\n> in terms of the sequencer can choose to (and should choose to unless there\n> is a compelling reason not to, because of the exact reason you stated\n> here) do a single-command insn sheet with \"git Foo --continue\" to conclude\n> if that one and only step needed help from the end user.\n\nNo, no.  I don't _want_ to create an AM_HEAD or FOO_HEAD for every new\ncommand that we write -- I was merely pointing out what would happen\nif the sequencer were only built to handle multi-commit-operations,\nand every new command would have to handle single-commit-operations on\ntheir own.  I was emphasizing that the sequencer will need to handle\nsingle-commit-operations as well; Jonathan has suggested that we have\nspecial hacks to handle cherry-pick and merge commands in the\nsequencer.\n\nTo conclude, let me list out the various implementation strategies\nwe've thought about, and the one we seem to have settled on:\n0. Remove the sequencer state from \"git commit\".  This is wrong, as\nJunio pointed out in the first email.\n1. Let commands handle single-commit-operations themselves, and call\ninto the sequencer only for multi-commit-operations.  If they don't\ncall into the sequencer at all, there's no state to persist or\ncleanup.  This approach is clearly wrong because each new command\nwould have to come up with AM_HEAD and FOO_HEAD to persist the\nsingle-commit-operation state.\n2. Expose two functions from the sequencer: \"earlier-function\" and\n\"later-function\".  Let cherry-pick and merge handle their own\nCHERRY_PICK_HEAD and MERGE_HEAD hacks first (by setting up the\nrevision walker, counting commits etc), and call into later-function\nin the sequencer to do less work from the sequencer's end.  All new\ncommands can directly call into earlier-function directly.  I know\nit's a little hand-wavy, but I hope it's atleast parse'able this time.\n As I pointed out in another email, this is also broken because\ncherry-pick/ merge would have to implement their own versions of every\nsubcommand like \"--continue\".\n3. Let all commands call into the sequencer for everything.  Let's\nteach the sequencer about the CHERRY_PICK_HEAD and MERGE_HEAD hacks so\nthat it can deal with them when a \"pick\", \"revert\" or \"merge\"\noperation is encountered.  We'll handle it by treating\nCHERRY_PICK_HEAD and MERGE_HEAD as part of the \"sequencer state\" (so\nthat subcommands work just fine on them).  For all future sequencer\ncommands, all data will be persisted inside '.git/sequencer'.  This\nseems most reasonable now.\n\nIn a nutshell, instead of side-stepping historical hacks, we want to\nteach the sequencer about them as specific cases to handle carefully.\nWe want to this without affecting the operation of the sequencer in\nthe general case.  Sounds great!  An uncompromising UI at the cost of\nlittle ugliness in the sequencer.\n\nThanks.\n\n-- Ram\n"}]}