{"thread":{"id":"28060","subject":"[RFC PATCH 0/5] Some preliminary work based on sequencer-stable","startedAt":"2011-08-10T09:55:46Z","lastAt":"2011-08-11T10:08:46Z","messageCount":16,"participants":["Ramkumar Ramachandra","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"173231","messageId":"1312970151-18906-1-git-send-email-artagnon@gmail.com","threadId":"28060","inReplyTo":null,"subject":"[RFC PATCH 0/5] Some preliminary work based on sequencer-stable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-10T09:55:46Z","receivedAt":"2011-08-10T09:55:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThe series is quite unpolished at the moment, but I'm sending it to\nthe list because I want early feedback before I make mypoic decisions\nand waste work.  Tests and rough commit messages included :)\n\nThe first patch is something I should've done earlier -- it makes\ndebugging much easier.  Should it go into maint? The last patch is the\nmost important, and everything else is just building towards it.  With\nthis series, we should now be able to abort at the last-but-one\ncommit, since 'git commit' actually cleans up the sequencer state now.\n\nNote: All the SHA-1 references in the commit messages will change\nafter sequencer-stable is merged.  I hope it's clear for the moment.\n\nThanks for reading.\n\n-- Ram\n\nRamkumar Ramachandra (5):\n  revert: Don't remove the sequencer state on error\n  sequencer.h: Move data structures\n  revert: Allow mixed pick and revert instructions\n  sequencer: Expose code that handles files in .git/sequencer\n  sequencer: Remove sequencer state after final commit\n\n builtin/commit.c                |    5 +\n builtin/revert.c                |  417 +++------------------------------------\n sequencer.c                     |  323 ++++++++++++++++++++++++++++++\n sequencer.h                     |   53 +++++\n t/t3510-cherry-pick-sequence.sh |   62 ++++++-\n 5 files changed, 471 insertions(+), 389 deletions(-)\n\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"173232","messageId":"1312970151-18906-2-git-send-email-artagnon@gmail.com","threadId":"28060","inReplyTo":"1312970151-18906-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/5] revert: Don't remove the sequencer state on error","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-10T09:55:47Z","receivedAt":"2011-08-10T09:55:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The cherry-pick/ revert machinery now removes the sequencer state when\ndo_pick_commit returns a non-zero, and when only one instruction is\nleft in the todo_list.  Since do_pick_commit has a way to distinguish\nerrors from conflicts using the signed-ness of the return value,\nutilize this to ensure that the sequencer state is only removed when\nthere's a conflict and there is only one instruction left in the\ntodo_list.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8b452e8..a548a14 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -929,10 +929,10 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \t\tsave_todo(cur, opts);\n \t\tres = do_pick_commit(cur->item, opts);\n \t\tif (res) {\n-\t\t\tif (!cur->next)\n+\t\t\tif (!cur->next && res > 0)\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 * A conflict 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-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"173233","messageId":"1312970151-18906-3-git-send-email-artagnon@gmail.com","threadId":"28060","inReplyTo":"1312970151-18906-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/5] sequencer.h: Move data structures","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-10T09:55:48Z","receivedAt":"2011-08-10T09:55:48Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Prepare to move more stuff into generalized sequencer.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c |   55 ++++++++++++++---------------------------------------\n sequencer.h      |   25 ++++++++++++++++++++++++\n 2 files changed, 40 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex a548a14..81502d4 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -39,43 +39,18 @@ 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-\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-\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-\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 == 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 +129,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 +328,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@@ -532,7 +507,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (opts->action == REVERT) {\n+\tif (opts->action == REPLAY_REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -575,7 +550,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\") || opts->action == REPLAY_REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n \t\t\t\t\t head, &msgbuf, opts);\n \t\twrite_message(&msgbuf, defmsg);\n@@ -594,7 +569,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(opts->action == REPLAY_REVERT\n \t\t      ? _(\"could not revert %s... %s\")\n \t\t      : _(\"could not apply %s... %s\"),\n \t\t      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n@@ -618,7 +593,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@@ -680,7 +655,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \tstruct commit_list *cur = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tconst char *sha1_abbrev = NULL;\n-\tconst char *action_str = opts->action == REVERT ? \"revert\" : \"pick\";\n+\tconst char *action_str = opts->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n@@ -700,11 +675,11 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n \tchar *p, *q;\n \n \tif (!prefixcmp(start, \"pick \")) {\n-\t\taction = CHERRY_PICK;\n+\t\taction = REPLAY_PICK;\n \t\tinsn_len = strlen(\"pick\");\n \t\tp = start + insn_len + 1;\n \t} else if (!prefixcmp(start, \"revert \")) {\n-\t\taction = REVERT;\n+\t\taction = REPLAY_REVERT;\n \t\tinsn_len = strlen(\"revert\");\n \t\tp = start + insn_len + 1;\n \t} else\n@@ -723,7 +698,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n \t */\n \tif (action != opts->action) {\n \t\tconst char *action_str;\n-\t\taction_str = action == REVERT ? \"revert\" : \"cherry-pick\";\n+\t\taction_str = action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n \t\terror(_(\"Cannot %s during a %s\"), action_str, action_name(opts));\n \t\treturn NULL;\n \t}\n@@ -989,7 +964,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@@ -1009,7 +984,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@@ -1024,7 +999,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..0b5d94e 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -7,6 +7,31 @@\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+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+\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+\n /*\n  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring\n  * any errors.  Intended to be used by 'git reset'.\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"173234","messageId":"1312970151-18906-4-git-send-email-artagnon@gmail.com","threadId":"28060","inReplyTo":"1312970151-18906-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/5] revert: Allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-10T09:55:49Z","receivedAt":"2011-08-10T09:55:49Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Change the way the instruction parser works, allowing arbitrary\n(action, operand) pairs to be parsed.  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.\n\nHelped-by: Jonathan Nieder <jrnider@gmail.com>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c                |  110 +++++++++++++++++++--------------------\n sequencer.h                     |    6 ++\n t/t3510-cherry-pick-sequence.sh |   58 ++++++++++++++++++++\n 3 files changed, 117 insertions(+), 57 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 81502d4..d48a92e 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -639,89 +639,84 @@ 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(struct replay_insn_list));\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 replay_insn_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 == REPLAY_REVERT ? \"revert\" : \"pick\";\n+\tconst char *action_str;\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\taction_str = cur->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n+\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}\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, enum replay_action *action,\n+\t\t\tstruct commit **operand)\n {\n \tunsigned char commit_sha1[20];\n \tchar sha1_abbrev[40];\n-\tenum replay_action action;\n-\tint insn_len = 0;\n+\tint keyword_len;\n \tchar *p, *q;\n \n \tif (!prefixcmp(start, \"pick \")) {\n-\t\taction = REPLAY_PICK;\n-\t\tinsn_len = strlen(\"pick\");\n-\t\tp = start + insn_len + 1;\n+\t\t*action = REPLAY_PICK;\n+\t\tkeyword_len = strlen(\"pick\");\n+\t\tp = start + keyword_len + 1;\n \t} else if (!prefixcmp(start, \"revert \")) {\n-\t\taction = REPLAY_REVERT;\n-\t\tinsn_len = strlen(\"revert\");\n-\t\tp = start + insn_len + 1;\n+\t\t*action = REPLAY_REVERT;\n+\t\tkeyword_len = strlen(\"revert\");\n+\t\tp = start + keyword_len + 1;\n \t} else\n-\t\treturn NULL;\n+\t\treturn -1;\n \n \tq = strchr(p, ' ');\n \tif (!q)\n-\t\treturn NULL;\n+\t\treturn -1;\n \tq++;\n \n \tstrlcpy(sha1_abbrev, p, q - p);\n \n-\t/*\n-\t * Verify that the action matches up with the one in\n-\t * opts; we don't support arbitrary instructions\n-\t */\n-\tif (action != opts->action) {\n-\t\tconst char *action_str;\n-\t\taction_str = action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n-\t\terror(_(\"Cannot %s during a %s\"), action_str, action_name(opts));\n-\t\treturn NULL;\n-\t}\n-\n \tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n-\t\treturn NULL;\n+\t\treturn -1;\n+\n+\t*operand = lookup_commit_reference(commit_sha1);\n+\tif (!*operand)\n+\t\treturn -1;\n \n-\treturn lookup_commit_reference(commit_sha1);\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+\tenum replay_action action;\n+\tstruct commit *operand;\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\tif (parse_insn_line(p, &action, &operand) < 0)\n \t\t\treturn error(_(\"Could not parse line %d.\"), i);\n-\t\tnext = commit_list_append(commit, next);\n+\t\tnext = replay_insn_list_append(action, operand, next);\n \t\tp = strchrnul(p, '\\n');\n \t\tif (*p)\n \t\t\tp++;\n@@ -731,8 +726,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@@ -748,7 +742,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@@ -797,18 +791,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 commit *operand;\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+\twhile ((operand = get_revision(&revs)))\n+\t\tnext = replay_insn_list_append(opts->action, operand, next);\n }\n \n static int create_seq_dir(void)\n@@ -837,7 +831,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@@ -845,7 +839,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@@ -889,9 +883,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@@ -901,8 +896,9 @@ 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\topts->action = cur->action;\n+\t\tres = do_pick_commit(cur->operand, opts);\n \t\tif (res) {\n \t\t\tif (!cur->next && res > 0)\n \t\t\t\t/*\n@@ -927,7 +923,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@@ -944,7 +940,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))\ndiff --git a/sequencer.h b/sequencer.h\nindex 0b5d94e..01caa8a 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -32,6 +32,12 @@ struct replay_opts {\n \tsize_t xopts_nr, xopts_alloc;\n };\n \n+struct replay_insn_list {\n+\tenum replay_action action;\n+\tstruct commit *operand;\n+\tstruct replay_insn_list *next;\n+};\n+\n /*\n  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring\n  * any errors.  Intended to be used by 'git reset'.\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 3bca2b3..f85372c 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -211,4 +211,62 @@ test_expect_success 'malformed instruction sheet 2' '\n \ttest_must_fail git cherry-pick --continue\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":"173236","messageId":"1312970151-18906-5-git-send-email-artagnon@gmail.com","threadId":"28060","inReplyTo":"1312970151-18906-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-10T09:55:50Z","receivedAt":"2011-08-10T09:55:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Move the saving and parsing functions for files in '.git/sequencer'\nfrom 'builtin/revert.c' into 'sequencer.c' and expose them using a\nsane API.  This is the first step towards a generalized sequencer.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c |  332 +----------------------------------------------------\n sequencer.c      |  309 ++++++++++++++++++++++++++++++++++++++++++++++++++\n sequencer.h      |   21 ++++\n 3 files changed, 337 insertions(+), 325 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex d48a92e..491c10a 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -5,7 +5,6 @@\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@@ -46,8 +45,6 @@ static const char *action_name(const struct replay_opts *opts)\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@@ -189,80 +186,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \topts->commit_argv = argv;\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@@ -494,7 +423,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\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+\tif (get_commit_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@@ -622,175 +551,6 @@ static void read_and_refresh_cache(struct replay_opts *opts)\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(struct replay_insn_list));\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 = NULL;\n-\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n-\tconst char *sha1_abbrev = NULL;\n-\tconst char *action_str;\n-\n-\tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\taction_str = cur->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n-\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}\n-\treturn 0;\n-}\n-\n-static int parse_insn_line(char *start, enum replay_action *action,\n-\t\t\tstruct commit **operand)\n-{\n-\tunsigned char commit_sha1[20];\n-\tchar sha1_abbrev[40];\n-\tint keyword_len;\n-\tchar *p, *q;\n-\n-\tif (!prefixcmp(start, \"pick \")) {\n-\t\t*action = REPLAY_PICK;\n-\t\tkeyword_len = strlen(\"pick\");\n-\t\tp = start + keyword_len + 1;\n-\t} else if (!prefixcmp(start, \"revert \")) {\n-\t\t*action = REPLAY_REVERT;\n-\t\tkeyword_len = strlen(\"revert\");\n-\t\tp = start + keyword_len + 1;\n-\t} else\n-\t\treturn -1;\n-\n-\tq = strchr(p, ' ');\n-\tif (!q)\n-\t\treturn -1;\n-\tq++;\n-\n-\tstrlcpy(sha1_abbrev, p, q - p);\n-\n-\tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n-\t\treturn -1;\n-\n-\t*operand = lookup_commit_reference(commit_sha1);\n-\tif (!*operand)\n-\t\treturn -1;\n-\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-\tenum replay_action action;\n-\tstruct commit *operand;\n-\tchar *p = buf;\n-\tint i;\n-\n-\tfor (i = 1; *p; i++) {\n-\t\tif (parse_insn_line(p, &action, &operand) < 0)\n-\t\t\treturn error(_(\"Could not parse line %d.\"), i);\n-\t\tnext = replay_insn_list_append(action, 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@@ -805,84 +565,6 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n \t\tnext = replay_insn_list_append(opts->action, operand, 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@@ -896,7 +578,7 @@ static int pick_commits(struct replay_insn_list *todo_list,\n \tread_and_refresh_cache(opts);\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsave_todo(cur);\n+\t\tsequencer_save_todo(cur);\n \t\topts->action = cur->action;\n \t\tres = do_pick_commit(cur->operand, opts);\n \t\tif (res) {\n@@ -939,8 +621,8 @@ static int pick_revisions(struct replay_opts *opts)\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+\t\tsequencer_read_opts(&opts);\n+\t\tsequencer_read_todo(&todo_list);\n \n \t\t/* Verify that the conflict has been resolved */\n \t\tif (!index_differs_from(\"HEAD\", 0))\n@@ -953,7 +635,7 @@ static int pick_revisions(struct replay_opts *opts)\n \t\t */\n \n \t\twalk_revs_populate_todo(&todo_list, opts);\n-\t\tif (create_seq_dir() < 0) {\n+\t\tif (sequencer_create_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@@ -964,8 +646,8 @@ static int pick_revisions(struct replay_opts *opts)\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\tsequencer_save_head(sha1_to_hex(sha1));\n+\t\tsequencer_save_opts(opts);\n \t}\n \treturn pick_commits(todo_list, opts);\n error:\ndiff --git a/sequencer.c b/sequencer.c\nindex bc2c046..2993846 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2,6 +2,8 @@\n #include \"sequencer.h\"\n #include \"strbuf.h\"\n #include \"dir.h\"\n+#include \"commit.h\"\n+#include \"utf8.h\"\n \n void remove_sequencer_state(int aggressive)\n {\n@@ -17,3 +19,310 @@ void remove_sequencer_state(int aggressive)\n \tstrbuf_release(&seq_dir);\n \tstrbuf_release(&seq_old_dir);\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(struct replay_insn_list));\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 parse_insn_line(char *start, enum replay_action *action,\n+\t\t\tstruct commit **operand)\n+{\n+\tunsigned char commit_sha1[20];\n+\tchar sha1_abbrev[40];\n+\tint keyword_len;\n+\tchar *p, *q;\n+\n+\tif (!prefixcmp(start, \"pick \")) {\n+\t\t*action = REPLAY_PICK;\n+\t\tkeyword_len = strlen(\"pick\");\n+\t\tp = start + keyword_len + 1;\n+\t} else if (!prefixcmp(start, \"revert \")) {\n+\t\t*action = REPLAY_REVERT;\n+\t\tkeyword_len = strlen(\"revert\");\n+\t\tp = start + keyword_len + 1;\n+\t} else\n+\t\treturn -1;\n+\n+\tq = strchr(p, ' ');\n+\tif (!q)\n+\t\treturn -1;\n+\tq++;\n+\n+\tstrlcpy(sha1_abbrev, p, q - p);\n+\n+\tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n+\t\treturn -1;\n+\n+\t*operand = lookup_commit_reference(commit_sha1);\n+\tif (!*operand)\n+\t\treturn -1;\n+\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+\tenum replay_action action;\n+\tstruct commit *operand;\n+\tchar *p = buf;\n+\tint i;\n+\n+\tfor (i = 1; *p; i++) {\n+\t\tif (parse_insn_line(p, &action, &operand) < 0)\n+\t\t\treturn error(_(\"Could not parse line %d.\"), i);\n+\t\tnext = replay_insn_list_append(action, 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+void sequencer_read_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 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+int get_commit_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+void sequencer_read_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 int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n+{\n+\tstruct replay_insn_list *cur = NULL;\n+\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n+\tconst char *sha1_abbrev = NULL;\n+\tconst char *action_str;\n+\n+\tfor (cur = todo_list; cur; cur = cur->next) {\n+\t\taction_str = cur->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n+\n+\t\tsha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);\n+\t\tif (get_commit_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}\n+\treturn 0;\n+}\n+\n+int sequencer_create_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 sequencer_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+void sequencer_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+void sequencer_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+}\ndiff --git a/sequencer.h b/sequencer.h\nindex 01caa8a..a5b951d 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -38,6 +38,17 @@ struct replay_insn_list {\n \tstruct replay_insn_list *next;\n };\n \n+/* Unrelated commit_message helper */\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+int get_commit_message(struct commit *commit, struct commit_message *out);\n+\n /*\n  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring\n  * any errors.  Intended to be used by 'git reset'.\n@@ -48,4 +59,14 @@ struct replay_insn_list {\n  */\n void remove_sequencer_state(int aggressive);\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+void sequencer_read_todo(struct replay_insn_list **todo_list);\n+void sequencer_read_opts(struct replay_opts **opts_ptr);\n+int sequencer_create_dir(void);\n+void sequencer_save_head(const char *head);\n+void sequencer_save_todo(struct replay_insn_list *todo_list);\n+void sequencer_save_opts(struct replay_opts *opts);\n+\n #endif\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"173235","messageId":"1312970151-18906-6-git-send-email-artagnon@gmail.com","threadId":"28060","inReplyTo":"1312970151-18906-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/5] sequencer: Remove sequencer state after final commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-10T09:55:51Z","receivedAt":"2011-08-10T09:55:51Z","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, write and expose a new function to count the\nnumber of commits left in the instruction sheet; use this in\nbuiltin/commit.c to remove the sequencer state when a commit has\nsuccessfully completed and there is only one instruction left in the\nsheet.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/commit.c                |    5 +++++\n builtin/revert.c                |   12 +-----------\n sequencer.c                     |   14 ++++++++++++++\n sequencer.h                     |    1 +\n t/t3510-cherry-pick-sequence.sh |    4 ++--\n 5 files changed, 23 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e1af9b1..7ee4269 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,10 @@ 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/* Remove sequencer state if we just finished the last insn */\n+\tif (sequencer_count_todo() == 1)\n+\t\tremove_sequencer_state(1);\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/builtin/revert.c b/builtin/revert.c\nindex 491c10a..718ba86 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -581,18 +581,8 @@ static int pick_commits(struct replay_insn_list *todo_list,\n \t\tsequencer_save_todo(cur);\n \t\topts->action = cur->action;\n \t\tres = do_pick_commit(cur->operand, opts);\n-\t\tif (res) {\n-\t\t\tif (!cur->next && res > 0)\n-\t\t\t\t/*\n-\t\t\t\t * A conflict 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.c b/sequencer.c\nindex 2993846..0ad1da6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -326,3 +326,17 @@ void sequencer_save_opts(struct replay_opts *opts)\n \t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n \t}\n }\n+\n+int sequencer_count_todo(void)\n+{\n+\tstruct replay_insn_list *todo_list = NULL;\n+\tstruct replay_insn_list *cur;\n+\tint insn_count = 0;\n+\n+\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n+\t\treturn 0;\n+\tsequencer_read_todo(&todo_list);\n+\tfor (cur = todo_list; cur; cur = cur->next)\n+\t\tinsn_count += 1;\n+\treturn insn_count;\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex a5b951d..ca133e2 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -68,5 +68,6 @@ int sequencer_create_dir(void);\n void sequencer_save_head(const char *head);\n void sequencer_save_todo(struct replay_insn_list *todo_list);\n void sequencer_save_opts(struct replay_opts *opts);\n+int sequencer_count_todo(void);\n \n #endif\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex f85372c..832baa0 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":"173241","messageId":"20110810151527.GC31315@elie.gateway.2wire.net","threadId":"28060","inReplyTo":"1312970151-18906-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-10T15:15:27Z","receivedAt":"2011-08-10T15:15:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Change the way the instruction parser works, allowing arbitrary\n> (action, operand) pairs to be parsed.  So now, you can do:\n>\n>   pick fdc0b12 picked\n>   revert 965fed4 anotherpick\n>\n> For cherry-pick and revert, this means that a 'git cherry-pick\n> --continue' can continue an ongoing revert operation and viceversa.\n\nNice. :)\n\n> This patch lays the foundation for extending the parser to support\n> more actions.\n\nAnd why would I want to do that?  I think there's a missing \"so git\nrebase -i can reuse this machinery some day\" at the end of the\nsentence.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -639,89 +639,84 @@ 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> +\tstruct replay_insn_list *new = xmalloc(sizeof(struct replay_insn_list));\n\nTip: we can save the readers some reading and prepare for future\nrenaming of the structure (not that that's something to fear) using\nthe idiom\n\n\tstruct replay_insn_list *new = xmalloc(sizeof(*new));\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 replay_insn_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 == REPLAY_REVERT ? \"revert\" : \"pick\";\n> +\tconst char *action_str;\n\nMight be clearer with narrower scope:\n\n\tstruct replay_insn_list *cur;\n\n\tfor (cur = todo_list; cur; cur = cur->next) {\n\t\tstruct commit_message msg = COMMIT_MESSAGE_INIT;\n\t\tconst char *sha1_abbrev, *action_str;\n\n\t\tsha1_abbrev = ...;\n\t\taction_str = ...;\n\t\tif (get_message(cur->operand, &msg))\n\t\t\treturn error(...);\n\t\tstrbuf_addf(buf, \"%s %s %s\\n\", ...);\n\t}\n\treturn 0;\n\nBy the way, shouldn't there a free_message() call to balance out the\nget_message()?\n\n[...]\n> -static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n> +static int parse_insn_line(char *start, enum replay_action *action,\n> +\t\t\tstruct commit **operand)\n\nHm, why not\n\n static int parse_insn_line(const char *start, struct replay_insn_list *item);\n\n>  {\n>  \tunsigned char commit_sha1[20];\n>  \tchar sha1_abbrev[40];\n> -\tenum replay_action action;\n> -\tint insn_len = 0;\n> +\tint keyword_len;\n\nWhat is this renaming about?  Maybe \"action_len\" would work to clarify\nthat this is about the length of the action string at the start, not\nthe entire line.\n\nBy the way, what are the semantics of that variable?  It doesn't seem\nto be used, so couldn't we just eliminate it?\n\n>  \tchar *p, *q;\n>  \n>  \tif (!prefixcmp(start, \"pick \")) {\n> -\t\taction = REPLAY_PICK;\n> -\t\tinsn_len = strlen(\"pick\");\n> -\t\tp = start + insn_len + 1;\n> +\t\t*action = REPLAY_PICK;\n> +\t\tkeyword_len = strlen(\"pick\");\n> +\t\tp = start + keyword_len + 1;\n\nIn such a scenario, this would say\n\n\tif (!prefixcmp(start, \"pick \")) {\n\t\titem->action = REPLAY_PICK;\n\t\tp += strlen(\"pick \");\n\t}\n\n[...]\n>  \t} else\n> -\t\treturn NULL;\n> +\t\treturn -1;\n\nUnrelated to this patch: maybe\n\n \treturn error(\"unrecognized action in sequencer file: %s\", start);\n\nto be easier to debug.\n\n>  \tq = strchr(p, ' ');\n>  \tif (!q)\n> -\t\treturn NULL;\n> +\t\treturn -1;\n\nSo we reject \"pick a87c8989\"?  That's a shame.\n\n>  \tq++;\n>  \n>  \tstrlcpy(sha1_abbrev, p, q - p);\n\nmemcpy would be clearer.  Can't this overflow the sha1_abbrev buffer?\n\n[...]\n>  \tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n> -\t\treturn NULL;\n> +\t\treturn -1;\n\nget_sha1 doesn't print a message, so maybe:\n\n\treturn error(\"malformed object name in sequencer file: %s\",\n\t\t\t\t\t\t\tsha1_abbrev);\n\n> +\n> +\t*operand = lookup_commit_reference(commit_sha1);\n> +\tif (!*operand)\n> +\t\treturn -1;\n\nPerhaps something like\n\n\treturn error(\"operand %s in sequencer file is not a commit\",\n\t\t\t\t\t\t\tsha1_abbrev);\n\n[...]\n> @@ -797,18 +791,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 *operand;\n\nCan avoid some churn by keeping the old name here.\n\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> +\twhile ((operand = get_revision(&revs)))\n> +\t\tnext = replay_insn_list_append(opts->action, operand, next);\n[...]\n> @@ -901,8 +896,9 @@ 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\topts->action = cur->action;\n> +\t\tres = do_pick_commit(cur->operand, opts);\n\nIf do_pick_commit took an \"action\" operand, this would be less\nscary. :)\n\n[...]\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -211,4 +211,62 @@ test_expect_success 'malformed instruction sheet 2' '\n>  \ttest_must_fail git cherry-pick --continue\n>  '\n>  \n> +test_expect_success 'revert --continue continues after cherry-pick' '\n\nI haven't looked at the tests yet.  FWIW, with whatever changes above\nseem suitable,\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"173242","messageId":"20110810152126.GE31315@elie.gateway.2wire.net","threadId":"28060","inReplyTo":"1312970151-18906-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-10T15:21:26Z","receivedAt":"2011-08-10T15:21:26Z","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/sequencer.h\n> +++ b/sequencer.h\n> @@ -38,6 +38,17 @@ struct replay_insn_list {\n>  \tstruct replay_insn_list *next;\n>  };\n>  \n> +/* Unrelated commit_message helper */\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> +int get_commit_message(struct commit *commit, struct commit_message *out);\n> +\n>  /*\n>   * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring\n>   * any errors.  Intended to be used by 'git reset'.\n> @@ -48,4 +59,14 @@ struct replay_insn_list {\n>   */\n>  void remove_sequencer_state(int aggressive);\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> +void sequencer_read_todo(struct replay_insn_list **todo_list);\n> +void sequencer_read_opts(struct replay_opts **opts_ptr);\n> +int sequencer_create_dir(void);\n> +void sequencer_save_head(const char *head);\n> +void sequencer_save_todo(struct replay_insn_list *todo_list);\n> +void sequencer_save_opts(struct replay_opts *opts);\n\nThis looks wrong.  What is the expected calling sequence?  Would it\nbe possible to expose fewer functions by moving more to sequencer.c?\n"},{"id":"173243","messageId":"CALkWK0nJUTtNgCHF6CE2-w+3ZwdBrscBRw0e0L8wX86Za0G=DA@mail.gmail.com","threadId":"28060","inReplyTo":"20110810152126.GE31315@elie.gateway.2wire.net","subject":"Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-10T15:34:02Z","receivedAt":"2011-08-10T15:34:02Z","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>> +struct replay_insn_list **replay_insn_list_append(enum replay_action action,\n>> +                                             struct commit *operand,\n>> +                                             struct replay_insn_list **next);\n>> +void sequencer_read_todo(struct replay_insn_list **todo_list);\n>> +void sequencer_read_opts(struct replay_opts **opts_ptr);\n>> +int sequencer_create_dir(void);\n>> +void sequencer_save_head(const char *head);\n>> +void sequencer_save_todo(struct replay_insn_list *todo_list);\n>> +void sequencer_save_opts(struct replay_opts *opts);\n>\n> This looks wrong.  What is the expected calling sequence?  Would it\n> be possible to expose fewer functions by moving more to sequencer.c?\n\nThanks for the early review.  Yes, I agree with you: No caller can\nmake sense of this API; I want something like sequencer_start,\nsequencer_handle_conflict, and sequencer_end, but I'm not sure where\nto start.  I would have liked to just move these functions without\nexposing them, but that would break cherry-pick/ revert.  So, we're\nreally faced with two choices:\n1. Make this patch enormous by moving as well as refactoring\neverything into a beautiful public API.  I suspect this won't be easy\nto review at all.\n2. Keep this patch as it is, and introduce a future patch to clean up\nthe API.  This is the approach I was going for.\n\nI can't move these functions bit-by-bit either: a hypothetical\nsequencer_start will require *everything*.  Could you suggest\nsomething?\n\n-- Ram\n"},{"id":"173244","messageId":"20110810155332.GB4076@elie.gateway.2wire.net","threadId":"28060","inReplyTo":"CALkWK0nJUTtNgCHF6CE2-w+3ZwdBrscBRw0e0L8wX86Za0G=DA@mail.gmail.com","subject":"Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-10T15:53:32Z","receivedAt":"2011-08-10T15:53:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> So, we're\n> really faced with two choices:\n> 1. Make this patch enormous by moving as well as refactoring\n> everything into a beautiful public API.  I suspect this won't be easy\n> to review at all.\n> 2. Keep this patch as it is, and introduce a future patch to clean up\n> the API.  This is the approach I was going for.\n\nWell, \"beautiful public API\" means \"just what cmd_cherry_pick and\ncmd_revert needs\", right?  So I'd suggest:\n\n 1. Figuring out what functions they need, and doing the minimal\n    refactoring needed to make them separate functions.\n\n 2. As patch #2, moving everything to sequencer.c and exposing those\n    functions in sequencer.h.\n\n 3. In later patches, making changes needed for what \"git commit\"\n    needs.\n\nLuckily step (1) is already done.  The functions are parse_args()\nand pick_revisions() (though they could presumably use less generic\nnames).\n\nHmm?\nJonathan\n"},{"id":"173255","messageId":"CALkWK0kxtyPABBUOrXtKDxPCBKt3CynoP4Fm8f_+C1ymkLTo-w@mail.gmail.com","threadId":"28060","inReplyTo":"20110810155332.GB4076@elie.gateway.2wire.net","subject":"Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-11T06:16:58Z","receivedAt":"2011-08-11T06:16:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Well, \"beautiful public API\" means \"just what cmd_cherry_pick and\n> cmd_revert needs\", right?  So I'd suggest:\n\nYes, but I don't want to put stuff that's too specific to cherry-pick/\nrevert in the sequencer.\n\n> [...]\n> Luckily step (1) is already done.  The functions are parse_args()\n> and pick_revisions() (though they could presumably use less generic\n> names).\n\nAre you certain about pick_revisions?  I've copied over the function\nhere for your reference.  My issue is that it's too specific\ncherry-pick/ revert:\n1. See what walk_revs_populate_todo does: It takes all the operands\nand fills up \"pick\" as the operator.  Why would any other caller want\nto do this?\n2. You mentioned multiple entry points earlier, and that's something\nI've been meaning to do: In the long run, I don't want callers to fill\nup an opts structure to specify the subcommand! That'd be butt-ugly.\n\n-- 8< --\nstatic int pick_revisions(struct replay_opts *opts)\n{\n\tstruct replay_insn_list *todo_list = NULL;\n\tunsigned char sha1[20];\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\tsequencer_read_opts(&opts);\n\t\tsequencer_read_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 (sequencer_create_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\tsequencer_save_head(sha1_to_hex(sha1));\n\t\tsequencer_save_opts(opts);\n\t}\n\treturn pick_commits(todo_list, opts);\nerror:\n\treturn error(_(\"No %s in progress\"), action_name(opts));\n}\n"},{"id":"173256","messageId":"20110811062236.GA27394@elie.gateway.2wire.net","threadId":"28060","inReplyTo":"CALkWK0kxtyPABBUOrXtKDxPCBKt3CynoP4Fm8f_+C1ymkLTo-w@mail.gmail.com","subject":"Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-11T06:22:36Z","receivedAt":"2011-08-11T06:22:36Z","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> Are you certain about pick_revisions?  I've copied over the function\n> here for your reference.  My issue is that it's too specific\n> cherry-pick/ revert:\n\nI'm a very pragmatic person: as long as the code and history are\nreadable and behave reasonably well, I'm happy.\n\nSo in this particular case, why not expose pick_revisions, with some\nname like revert_or_cherry_pick?  It would be readable and behave\nreasonably well. :)  A theoretical other caller could save a fork\nby calling revert_or_cherry_pick instead of forking a subprocess to\ndo the same.\n\n[...]\n> 2. You mentioned multiple entry points earlier, and that's something\n> I've been meaning to do: In the long run\n\nSure, and that still seems like a good idea in the long run.\n\nBut as long as we are not closing doors, ideas about the long run\nshould not get in the way of getting work done today.\n\nHoping that is clearer.\nJonathan\n"},{"id":"173257","messageId":"CALkWK0=X-cwH+v4sbikhcJ3LO6U7e3heWVPUhu0AtE-S9qxU_g@mail.gmail.com","threadId":"28060","inReplyTo":"20110811062236.GA27394@elie.gateway.2wire.net","subject":"Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-11T06:27:14Z","receivedAt":"2011-08-11T06:27:14Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> I'm a very pragmatic person: as long as the code and history are\n> readable and behave reasonably well, I'm happy.\n\nThanks for clarifying; I'll get to work right away.\n\n-- Ram\n"},{"id":"173258","messageId":"CALkWK0kxi+-UEG9g_=BC8vK9DGA85+OKJSrXWH=3XsxpLg-E8g@mail.gmail.com","threadId":"28060","inReplyTo":"20110810151527.GC31315@elie.gateway.2wire.net","subject":"Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-11T06:52:10Z","receivedAt":"2011-08-11T06:52:10Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nOn a note unrelated to Jonathan's review, I noticed some weird\nbehavior while playing with my new series: when picking a commit\nthat's already picked, all the code until run_git_commit runs just\nfine.  Then 'git commit' fails because it's an empty commit.\nUnfortunately, run_git_commit returns a positive return status from\ndo_pick_commit which is against the convention, leading the\ncherry-picking machinery to think that a conflict was encountered.\nThis is highly confusing from the end-user's perspective.  The\noperation has suddenly stopped without an error, and '--continue'\nseems to continue the operation.  What is going on, right?\n\nI was hesitant to send out the patch to make run_git_commit return an\nerror, because it breaks some tests in t3505-cherry-pick-empty.sh.\nHowever, the problem is getting on my nerves now, and I believe that\nthe patch does the Right Thing (TM), even if it means that we have to\nchange the tests in t3505-cherry-pick-empty.sh.  Thoughts?\n\n-- 8< --\nSubject: [PATCH] revert: Classify failure to run 'git commit' as an error\n\nSince a93d2a (revert: Propagate errors upwards from do_pick_commit,\n2011-05-20), do_pick_commit differentiates between conflicts and\nerrors using the signed-ness of its return status.  Unfortunately, it\nreturns the return status from 'git commit' when it fails to run it,\nbreaking this convention.  Change this so that any non-zero return\nstatus from run_git_commit is classified as an error.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8b452e8..fcd4f3a 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -603,7 +603,9 @@ static int do_pick_commit(struct commit *commit,\nstruct replay_opts *opts)\n                rerere(opts->allow_rerere_auto);\n        } else {\n                if (!opts->no_commit)\n-                       res = run_git_commit(defmsg, opts);\n+                       if (run_git_commit(defmsg, opts))\n+                               return error(_(\"Failed to run 'git\ncommit': %s\"),\n+                                       sha1_to_hex(commit->object.sha1));\n        }\n\n        free_message(&msg);\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"173272","messageId":"CALkWK0mMVgnngU5JB4O+7crCjGCdRnVrbm4jjsgkFFUMeBS9_A@mail.gmail.com","threadId":"28060","inReplyTo":"20110810151527.GC31315@elie.gateway.2wire.net","subject":"Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-11T09:50:57Z","receivedAt":"2011-08-11T09:50:57Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nI fixed everything; just a few comments/ doubts:\n\nJonathan Nieder writes:\n>>       } else\n>> -             return NULL;\n>> +             return -1;\n>\n> Unrelated to this patch: maybe\n>\n>        return error(\"unrecognized action in sequencer file: %s\", start);\n>\n> to be easier to debug.\n\nI'd initially refrained from doing this so that errors don't become\novertly verbose, but I suppose it's alright considering the fact that\nwe're going to make the instruction sheet editable sometime in the\nfuture.  I tweaked the error strings a little so that we get something\nlike:\n\n  error: Unrecognized action: part\n  error: Could not parse line 3.\n  fatal: Unusable instruction sheet: .git/sequencer\n\nIn essence, I didn't want to be redundant and mention the sequencer in\nevery line.  I like the above.\n\n>>       q = strchr(p, ' ');\n>>       if (!q)\n>> -             return NULL;\n>> +             return -1;\n>\n> So we reject \"pick a87c8989\"?  That's a shame.\n\nGood point.  Fixing this will have to be in another patch, where I'll\nadvertise the fact that I'm changing the instruction sheet format.\n\n>>       q++;\n>>\n>>       strlcpy(sha1_abbrev, p, q - p);\n>\n> memcpy would be clearer.  Can't this overflow the sha1_abbrev buffer?\n\nGood point.  I'm tempted to check (q - p < 40); is there a better way\nto do this by not hardcoding \"40\" perhaps?\n\n> Acked-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\n-- Ram\n"},{"id":"173273","messageId":"20110811100846.GA5012@elie.gateway.2wire.net","threadId":"28060","inReplyTo":"CALkWK0mMVgnngU5JB4O+7crCjGCdRnVrbm4jjsgkFFUMeBS9_A@mail.gmail.com","subject":"Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-11T10:08:46Z","receivedAt":"2011-08-11T10:08:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Good point.  I'm tempted to check (q - p < 40); is there a better way\n> to do this by not hardcoding \"40\" perhaps?\n\nSomething like the following seems idiomatic.\n\n\tif (q - p + 1 > sizeof(buf))\n\t\treturn error(...);\n\tmemcpy(buf, p, q - p);\n\tbuf[q - p] = '\\0';\n"}]}