{"thread":{"id":"27321","subject":"[PATCH 6/8] revert: Introduce head, todo, done files to persist state","startedAt":"2011-05-11T08:00:14Z","lastAt":"2011-05-20T06:39:55Z","messageCount":39,"participants":["Ramkumar Ramachandra","Jonathan Nieder","Christian Couder","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"167672","messageId":"1305100822-20470-1-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":null,"subject":"[PATCH 0/8] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:14Z","receivedAt":"2011-05-11T08:00:14Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nI've not attempted to add anything new in this series -- It merely\nfixes all the mistakes in the previous iteration. I've tried to\nintegrate the improvements suggested by all the previous reviews.\n\nThe format of the instruction sheet hasn't changed yet, but will soon\nchange to the one suggested by Chistian. There are some nits I'm not\nhappy with in certain patches -- I've sprinkled those comments into\nthe individual patches. All tests pass in all patches, and I hope no\nstray lines have travelled b/w the patches during the rebase.\n\nThanks for reading.\n\nRamkumar Ramachandra (8):\n revert: Improve error handling by cascading errors upwards\n revert: Make \"commit\" and \"me\" local variables\n revert: Introduce a struct to parse command-line options into\n revert: Separate cmdline argument handling from the functional code\n revert: Catch incompatible command-line options early\n revert: Introduce head, todo, done files to persist state\n revert: Implement parsing --continue, --abort and --skip\n revert: Implement --abort processing\n\n advice.c | 14 ++\n advice.h | 1 +\n builtin/revert.c | 578 +++++++++++++++++++++++++++++++++++++++---------------\n 3 files changed, 437 insertions(+), 156 deletions(-)\n\n-- \n1.7.5.GIT\n"},{"id":"167671","messageId":"1305100822-20470-2-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/8] revert: Improve error handling by cascading errors upwards","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:15Z","receivedAt":"2011-05-11T08:00:15Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"As a prelude to libification, we would like to propogate errors\nupwards through the callchain, so as to correctly handle errors and\nclean up afterwards. Also introduce a new function\n\"error_resolve_conflict\" in advice.c in an attempt to unify the way\nconflicts are reported by the various components of Git. As a general\nguideline for libification, \"die\" should be only be called in two\ncases: by toplevel callers like command-line argument parsing\nroutines, or when an irrecoverable situation is encountered.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n Has the commit message justified this change fully?\n\n How do I trap and handle the exit status from write_message in\n do_pick_commit correctly? There's already a call to\n do_recursive_merge whose exit status is being trapped -- what happens\n when do_recursive_merge succeeds and write_message fails, or\n viceversa?\n\n Junio has suggested dropping error_errno, and simply using error and\n returning -errno by hand in one email. Considering the number of\n times I've used that tecnique, and I think we should get something\n like an error_errno atleast for the sake of terseness.\n\n advice.c | 14 +++++\n advice.h | 1 +\n builtin/revert.c | 158 ++++++++++++++++++++++++++++++-----------------------\n 3 files changed, 104 insertions(+), 69 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 0be4b5f..3c3c187 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -47,3 +47,17 @@ void NORETURN die_resolve_conflict(const char *me)\n \telse\n \t\tdie(\"'%s' is not possible because you have unmerged files.\", me);\n }\n+\n+int error_resolve_conflict(const char *me)\n+{\n+\tif (advice_resolve_conflict)\n+\t\t/*\n+\t\t * Message used both when 'git commit' fails and when\n+\t\t * other commands doing a merge do.\n+\t\t */\n+\t\treturn error(\"'%s' is not possible because you have unmerged files.\\n\"\n+\t\t\t\"Please, fix them up in the work tree, and then use 'git add/rm ' as\\n\"\n+\t\t\t\"appropriate to mark resolution and make a commit, or use 'git commit -a'.\", me);\n+\telse\n+\t\treturn error(\"'%s' is not possible because you have unmerged files.\", me);\n+}\ndiff --git a/advice.h b/advice.h\nindex 3244ebb..7b7cea5 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -13,5 +13,6 @@ extern int advice_detached_head;\n int git_default_advice_config(const char *var, const char *value);\n \n extern void NORETURN die_resolve_conflict(const char *me);\n+extern int error_resolve_conflict(const char *me);\n \n #endif /* ADVICE_H */\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex f697e66..fefb18b 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -167,9 +167,11 @@ static char *get_encoding(const char *message)\n {\n \tconst char *p = message, *eol;\n \n-\tif (!p)\n-\t\tdie (_(\"Could not read commit message of %s\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n+\tif (!p) {\n+\t\terror(_(\"Could not read commit message of %s\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n+\t\treturn NULL;\n+\t}\n \twhile (*p && *p != '\\n') {\n \t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n \t\t\t; /* do nothing */\n@@ -198,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n \tstrbuf_addstr(msgbuf, p);\n }\n \n-static void write_cherry_pick_head(void)\n+static int write_cherry_pick_head(void)\n {\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -206,12 +208,22 @@ static void write_cherry_pick_head(void)\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+\tif (fd < 0) {\n+\t\tint err = errno;\n+\t\tstrbuf_release(&buf);\n+\t\terror(_(\"Could not open '%s' for writing: %s\"),\n+\t\t\tgit_path(\"CHERRY_PICK_HEAD\"), strerror(err));\n+\t\treturn -err;\n+\t}\n+\tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd)) {\n+\t\tint err = errno;\n+\t\tstrbuf_release(&buf);\n+\t\terror(_(\"Could not write to '%s': %s\"),\n+\t\t\tgit_path(\"CHERRY_PICK_HEAD\"), strerror(err));\n+\t\treturn -err;\n+\t}\n \tstrbuf_release(&buf);\n+\treturn 0;\n }\n \n static void advise(const char *advice, ...)\n@@ -243,17 +255,22 @@ static void print_advice(void)\n \tadvise(\"and commit the result with 'git commit'\");\n }\n \n-static void write_message(struct strbuf *msgbuf, const char *filename)\n+static int 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+\tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {\n+\t\tint err = errno;\n+\t\tstrbuf_release(msgbuf);\n+\t\terror(_(\"Could not write to %s: %s\"), filename, strerror(err));\n+\t\treturn -err;\n+\t}\n \tstrbuf_release(msgbuf);\n \tif (commit_lock_file(&msg_file) < 0)\n-\t\tdie(_(\"Error wrapping up %s\"), filename);\n+\t\treturn error(_(\"Error wrapping up %s\"), filename);\n+\treturn 0;\n }\n \n static struct tree *empty_tree(void)\n@@ -266,25 +283,20 @@ static struct tree *empty_tree(void)\n \treturn tree;\n }\n \n-static NORETURN void die_dirty_index(const char *me)\n+static int verify_resolution(const char *me)\n {\n-\tif (read_cache_unmerged()) {\n-\t\tdie_resolve_conflict(me);\n-\t} else {\n-\t\tif (advice_commit_before_merge) {\n-\t\t\tif (action == REVERT)\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by revert.\\n\"\n-\t\t\t\t\t \"Please, commit your changes or stash them to proceed.\"));\n-\t\t\telse\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by cherry-pick.\\n\"\n-\t\t\t\t\t \"Please, commit your changes or stash them to proceed.\"));\n-\t\t} else {\n-\t\t\tif (action == REVERT)\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by revert.\\n\"));\n-\t\t\telse\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by cherry-pick.\\n\"));\n-\t\t}\n-\t}\n+\tif (!read_cache_unmerged())\n+\t\treturn 0;\n+\n+\treturn error_resolve_conflict(me);\n+}\n+\n+static int error_dirty_worktree(const char *me)\n+{\n+\tif (advice_commit_before_merge)\n+\t\treturn error(_(\"Your local changes would be overwritten by %s.\\n\"\n+\t\t\t\t\"Please, commit your changes or stash them to proceed.\"), me);\n+\treturn error(_(\"Your local changes would be overwritten by %s.\\n\"), me);\n }\n \n static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n@@ -329,10 +341,12 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\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(write_cache(index_fd, active_cache, active_nr) ||\n+\t\t\tcommit_locked_index(&index_lock))) {\n+\t\trollback_lock_file(&index_lock);\n \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\tdie(_(\"%s: Unable to write new index file\"), me);\n+\t\treturn error(_(\"%s: Unable to write new index file\"), me);\n+\t}\n \trollback_lock_file(&index_lock);\n \n \tif (!clean) {\n@@ -397,19 +411,21 @@ static int do_pick_commit(void)\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\tif (write_cache_as_tree(head, 0, NULL)) {\n+\t\t\tdiscard_cache();\n+\t\t\treturn error(_(\"Your index file is unmerged.\"));\n+\t\t}\n \t} else {\n \t\tif (get_sha1(\"HEAD\", head))\n-\t\t\tdie (_(\"You do not have a valid HEAD\"));\n-\t\tif (index_differs_from(\"HEAD\", 0))\n-\t\t\tdie_dirty_index(me);\n+\t\t\treturn error(_(\"You do not have a valid HEAD\"));\n+\t\tif (index_differs_from(\"HEAD\", 0) && !verify_resolution(me))\n+\t\t\treturn error_dirty_worktree(me);\n \t}\n \tdiscard_cache();\n \n \tif (!commit->parents) {\n \t\tif (action == REVERT)\n-\t\t\tdie (_(\"Cannot revert a root commit\"));\n+\t\t\treturn error(_(\"Cannot revert a root commit\"));\n \t\tparent = NULL;\n \t}\n \telse if (commit->parents->next) {\n@@ -418,19 +434,19 @@ static int do_pick_commit(void)\n \t\tstruct commit_list *p;\n \n \t\tif (!mainline)\n-\t\t\tdie(_(\"Commit %s is a merge but no -m option was given.\"),\n-\t\t\t sha1_to_hex(commit->object.sha1));\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 != mainline && p;\n \t\t cnt++)\n \t\t\tp = p->next;\n \t\tif (cnt != mainline || !p)\n-\t\t\tdie(_(\"Commit %s does not have parent %d\"),\n+\t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n \t\t\t sha1_to_hex(commit->object.sha1), mainline);\n \t\tparent = p->item;\n-\t} else if (0 < mainline)\n-\t\tdie(_(\"Mainline was specified but commit %s is not a merge.\"),\n+\t} else if (mainline > 0)\n+\t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n \t\t sha1_to_hex(commit->object.sha1));\n \telse\n \t\tparent = commit->parents->item;\n@@ -441,11 +457,11 @@ static int do_pick_commit(void)\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\tdie(_(\"%s: cannot parse parent commit %s\"),\n+\t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n \t\t me, sha1_to_hex(parent->object.sha1));\n \n \tif (get_message(commit->buffer, &msg) != 0)\n-\t\tdie(_(\"Cannot get commit message for %s\"),\n+\t\treturn error(_(\"Cannot get commit message for %s\"),\n \t\t\t\tsha1_to_hex(commit->object.sha1));\n \n \t/*\n@@ -484,7 +500,11 @@ static int do_pick_commit(void)\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n \t\tif (!no_commit)\n-\t\t\twrite_cherry_pick_head();\n+\t\t\tif ((res = write_cherry_pick_head())) {\n+\t\t\t\tfree_message(&msg);\n+\t\t\t\tfree(defmsg);\n+\t\t\t\treturn res;\n+\t\t\t}\n \t}\n \n \tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n@@ -524,44 +544,46 @@ static int do_pick_commit(void)\n \treturn res;\n }\n \n-static void prepare_revs(struct rev_info *revs)\n+static int prepare_revs(struct rev_info *revs)\n {\n-\tint argc;\n-\n \tinit_revisions(revs, NULL);\n \trevs->no_walk = 1;\n \tif (action != REVERT)\n \t\trevs->reverse = 1;\n \n-\targc = setup_revisions(commit_argc, commit_argv, revs, NULL);\n-\tif (argc > 1)\n-\t\tusage(*revert_or_cherry_pick_usage());\n+\tif (setup_revisions(commit_argc, commit_argv, revs, NULL) > 1)\n+\t\treturn error(_(\"usage: %s\"), *revert_or_cherry_pick_usage());\n \n \tif (prepare_revision_walk(revs))\n-\t\tdie(_(\"revision walk setup failed\"));\n+\t\treturn error(_(\"revision walk setup failed\"));\n \n \tif (!revs->commits)\n-\t\tdie(_(\"empty commit set passed\"));\n+\t\treturn error(_(\"empty commit set passed\"));\n+\treturn 0;\n }\n \n-static void read_and_refresh_cache(const char *me)\n+static int read_and_refresh_cache(const char *me)\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\"), me);\n+\t\treturn error(_(\"%s: failed to read the index\"), me);\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\"), me);\n+\t\t\tcommit_locked_index(&index_lock)) {\n+\t\t\trollback_lock_file(&index_lock);\n+\t\t\treturn error(_(\"%s: failed to refresh the index\"), me);\n+\t\t}\n \t}\n \trollback_lock_file(&index_lock);\n+\treturn 0;\n }\n \n static int revert_or_cherry_pick(int argc, const char **argv)\n {\n \tstruct rev_info revs;\n+\tint res;\n \n \tgit_config(git_default_config, NULL);\n \tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n@@ -579,17 +601,15 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n \t}\n \n-\tread_and_refresh_cache(me);\n+\tif ((res = read_and_refresh_cache(me)) ||\n+\t\t(res = prepare_revs(&revs)))\n+\t\treturn res;\n \n-\tprepare_revs(&revs);\n+\twhile ((commit = get_revision(&revs)) &&\n+\t\t!(res = do_pick_commit()))\n+\t\t;\n \n-\twhile ((commit = get_revision(&revs))) {\n-\t\tint res = do_pick_commit();\n-\t\tif (res)\n-\t\t\treturn res;\n-\t}\n-\n-\treturn 0;\n+\treturn res;\n }\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n-- \n1.7.5.GIT\n"},{"id":"167680","messageId":"1305100822-20470-3-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/8] revert: Make \"commit\" and \"me\" local variables","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:16Z","receivedAt":"2011-05-11T08:00:16Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Currently, \"commit\" and \"me\" are global static variables. Since we\nwant to develop the functionality to either pick/ revert individual\ncommits atomically later in the series, make them local variables.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n The variable \"me\" is nowhere as fundamental as \"commit\" -- it's\n simply a string derived from a more fundamental \"action\". Yet, the\n commit message seems to indicate that both \"me\" and \"commit\" are\n equally important -- how should it be reworded?\n\n builtin/revert.c | 42 +++++++++++++++++++++++++-----------------\n 1 files changed, 25 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex fefb18b..e5c3c6c 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -37,12 +37,10 @@ static const char * const cherry_pick_usage[] = {\n \n static int edit, no_replay, no_commit, mainline, signoff, allow_ff;\n static enum { REVERT, CHERRY_PICK } action;\n-static struct commit *commit;\n static int commit_argc;\n static const char **commit_argv;\n static int allow_rerere_auto;\n \n-static const char *me;\n \n /* Merge strategy. */\n static const char *strategy;\n@@ -51,7 +49,7 @@ static size_t xopts_nr, xopts_alloc;\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n-static char *get_encoding(const char *message);\n+static char *get_encoding(struct commit *commit, const char *message);\n \n static const char * const *revert_or_cherry_pick_usage(void)\n {\n@@ -116,7 +114,8 @@ struct commit_message {\n \tconst char *message;\n };\n \n-static int get_message(const char *raw_message, struct commit_message *out)\n+static int get_message(struct commit *commit, const char *raw_message,\n+\t\tstruct commit_message *out)\n {\n \tconst char *encoding;\n \tconst char *abbrev, *subject;\n@@ -125,7 +124,7 @@ static int get_message(const char *raw_message, struct commit_message *out)\n \n \tif (!raw_message)\n \t\treturn -1;\n-\tencoding = get_encoding(raw_message);\n+\tencoding = get_encoding(commit, raw_message);\n \tif (!encoding)\n \t\tencoding = \"UTF-8\";\n \tif (!git_commit_encoding)\n@@ -163,7 +162,7 @@ static void free_message(struct commit_message *msg)\n \tfree(msg->reencoded_message);\n }\n \n-static char *get_encoding(const char *message)\n+static char *get_encoding(struct commit *commit, const char *message)\n {\n \tconst char *p = message, *eol;\n \n@@ -187,7 +186,8 @@ static char *get_encoding(const char *message)\n \treturn NULL;\n }\n \n-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n+static void add_message_to_msg(struct commit *commit, struct strbuf *msgbuf,\n+\t\t\tconst char *message)\n {\n \tconst char *p = message;\n \twhile (*p && (*p != '\\n' || p[1] != '\\n'))\n@@ -200,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n \tstrbuf_addstr(msgbuf, p);\n }\n \n-static int write_cherry_pick_head(void)\n+static int write_cherry_pick_head(struct commit *commit)\n {\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -319,6 +319,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tint clean, index_fd;\n \tconst char **xopt;\n \tstatic struct lock_file index_lock;\n+\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n \n \tindex_fd = hold_locked_index(&index_lock, 1);\n \n@@ -394,7 +395,7 @@ static int run_git_commit(const char *defmsg)\n \treturn run_command_v_opt(args, RUN_GIT_CMD);\n }\n \n-static int do_pick_commit(void)\n+static int do_pick_commit(struct commit *commit)\n {\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n@@ -402,6 +403,7 @@ static int do_pick_commit(void)\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n+\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n \tint res;\n \n \tif (no_commit) {\n@@ -458,9 +460,10 @@ static int do_pick_commit(void)\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 me, sha1_to_hex(parent->object.sha1));\n+\t\t\taction == REVERT ? \"revert\" : \"cherry-pick\",\n+\t\t\tsha1_to_hex(parent->object.sha1));\n \n-\tif (get_message(commit->buffer, &msg) != 0)\n+\tif (get_message(commit, commit->buffer, &msg) != 0)\n \t\treturn error(_(\"Cannot get commit message for %s\"),\n \t\t\t\tsha1_to_hex(commit->object.sha1));\n \n@@ -493,14 +496,14 @@ static int do_pick_commit(void)\n \t\tbase_label = msg.parent_label;\n \t\tnext = commit;\n \t\tnext_label = msg.label;\n-\t\tadd_message_to_msg(&msgbuf, msg.message);\n+\t\tadd_message_to_msg(commit, &msgbuf, msg.message);\n \t\tif (no_replay) {\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 (!no_commit)\n-\t\t\tif ((res = write_cherry_pick_head())) {\n+\t\t\tif ((res = write_cherry_pick_head(commit))) {\n \t\t\t\tfree_message(&msg);\n \t\t\t\tfree(defmsg);\n \t\t\t\treturn res;\n@@ -562,10 +565,13 @@ static int prepare_revs(struct rev_info *revs)\n \treturn 0;\n }\n \n-static int read_and_refresh_cache(const char *me)\n+static int read_and_refresh_cache(void)\n {\n \tstatic struct lock_file index_lock;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n+\tconst char *me;\n+\n+\tme = (action == REVERT ? \"revert\" : \"cherry-pick\");\n \tif (read_index_preload(&the_index, NULL) < 0)\n \t\treturn error(_(\"%s: failed to read the index\"), me);\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n@@ -583,10 +589,12 @@ static int read_and_refresh_cache(const char *me)\n static int revert_or_cherry_pick(int argc, const char **argv)\n {\n \tstruct rev_info revs;\n+\tstruct commit *commit;\n+\tconst char *me;\n \tint res;\n \n \tgit_config(git_default_config, NULL);\n-\tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n+\tme = (action == REVERT ? \"revert\" : \"cherry-pick\");\n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n \tparse_args(argc, argv);\n \n@@ -601,12 +609,12 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n \t}\n \n-\tif ((res = read_and_refresh_cache(me)) ||\n+\tif ((res = read_and_refresh_cache()) ||\n \t\t(res = prepare_revs(&revs)))\n \t\treturn res;\n \n \twhile ((commit = get_revision(&revs)) &&\n-\t\t!(res = do_pick_commit()))\n+\t\t!(res = do_pick_commit(commit)))\n \t\t;\n \n \treturn res;\n-- \n1.7.5.GIT\n"},{"id":"167653","messageId":"1305100822-20470-4-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/8] revert: Introduce a struct to parse command-line options into","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:17Z","receivedAt":"2011-05-11T08:00:17Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The current code uses a set of file-scope static variables to tell the\ncherry-pick/ revert machinery how to replay the changes, and\ninitializes them by parsing the command-line arguments. In later\nsteps in this series, we would like to introduce an API function that\ncalls into this machinery directly and have a way to tell it what to\ndo. Hence, introduce a structure to group these variables, so that\nthe API can take them as a single \"replay_options\" parameter.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n I get the following warning from GCC: warning: useless storage class\n specifier in empty declaration (at the line where I've declared the\n replay_opts struct). What is the correct way to fix this?\n\n Also, I'm not happy with the way I'm parsing xopts-related options\n from option_parse_x into global static variables, before actually\n putting it into the structure. Is there a better way to do this?\n\n builtin/revert.c | 198 ++++++++++++++++++++++++++++++------------------------\n 1 files changed, 111 insertions(+), 87 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e5c3c6c..8550927 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -35,29 +35,42 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;\n-static enum { REVERT, CHERRY_PICK } action;\n-static int commit_argc;\n-static const char **commit_argv;\n-static int allow_rerere_auto;\n-\n-\n-/* Merge strategy. */\n-static const char *strategy;\n-static const char **xopts;\n-static size_t xopts_nr, xopts_alloc;\n+static struct replay_opts {\n+\tenum { REVERT, CHERRY_PICK } action;\n+\n+\t/* Boolean options */\n+\tint edit;\n+\tint no_replay;\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 char *get_encoding(struct commit *commit, const char *message);\n \n-static const char * const *revert_or_cherry_pick_usage(void)\n+static const char *const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n {\n-\treturn action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn opts->action == REVERT ? revert_usage : cherry_pick_usage;\n }\n \n+/* For option_parse_x */\n+static const char **xopts;\n+static size_t xopts_nr, xopts_alloc;\n+\n static int option_parse_x(const struct option *opt,\n-\t\t\t const char *arg, int unset)\n+\t\t\tconst char *arg, int unset)\n {\n \tif (unset)\n \t\treturn 0;\n@@ -67,19 +80,18 @@ static int option_parse_x(const struct option *opt,\n \treturn 0;\n }\n \n-static void parse_args(int argc, const char **argv)\n+static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n-\tconst char * const * usage_str = revert_or_cherry_pick_usage();\n+\tconst char *const *usage_str = revert_or_cherry_pick_usage(opts);\n \tint noop;\n \tstruct option options[] = {\n-\t\tOPT_BOOLEAN('n', \"no-commit\", &no_commit, \"don't automatically commit\"),\n-\t\tOPT_BOOLEAN('e', \"edit\", &edit, \"edit the commit message\"),\n-\t\t{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, \"no-op (backward compatibility)\",\n-\t\t PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },\n-\t\tOPT_BOOLEAN('s', \"signoff\", &signoff, \"add Signed-off-by:\"),\n-\t\tOPT_INTEGER('m', \"mainline\", &mainline, \"parent number\"),\n-\t\tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n-\t\tOPT_STRING(0, \"strategy\", &strategy, \"strategy\", \"merge strategy\"),\n+\t\tOPT_BOOLEAN('n', \"no-commit\", &(opts->no_commit), \"don't automatically commit\"),\n+\t\tOPT_BOOLEAN('e', \"edit\", &(opts->edit), \"edit the commit message\"),\n+\t\tOPT_BOOLEAN('r', NULL, &noop, \"no-op (backward compatibility)\"),\n+\t\tOPT_BOOLEAN('s', \"signoff\", &(opts->signoff), \"add Signed-off-by:\"),\n+\t\tOPT_INTEGER('m', \"mainline\", &(opts->mainline), \"parent number\"),\n+\t\tOPT_RERERE_AUTOUPDATE(&(opts->allow_rerere_auto)),\n+\t\tOPT_STRING(0, \"strategy\", &(opts->strategy), \"strategy\", \"merge strategy\"),\n \t\tOPT_CALLBACK('X', \"strategy-option\", &xopts, \"option\",\n \t\t\t\"option for merge strategy\", option_parse_x),\n \t\tOPT_END(),\n@@ -87,23 +99,29 @@ static void parse_args(int argc, const char **argv)\n \t\tOPT_END(),\n \t};\n \n-\tif (action == CHERRY_PICK) {\n+\tif (opts->action == CHERRY_PICK) {\n \t\tstruct option cp_extra[] = {\n-\t\t\tOPT_BOOLEAN('x', NULL, &no_replay, \"append commit name\"),\n-\t\t\tOPT_BOOLEAN(0, \"ff\", &allow_ff, \"allow fast-forward\"),\n+\t\t\tOPT_BOOLEAN('x', NULL, &(opts->no_replay), \"append commit name\"),\n+\t\t\tOPT_BOOLEAN(0, \"ff\", &(opts->allow_ff), \"allow fast-forward\"),\n \t\t\tOPT_END(),\n \t\t};\n \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\n \t\t\tdie(_(\"program error\"));\n \t}\n \n-\tcommit_argc = parse_options(argc, argv, NULL, options, usage_str,\n+\topts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,\n \t\t\t\t PARSE_OPT_KEEP_ARGV0 |\n \t\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n-\tif (commit_argc < 2)\n+\n+\t/* Fill in the opts struct from values set by option_parse_x */\n+\topts->xopts = xopts;\n+\topts->xopts_nr = xopts_nr;\n+\topts->xopts_alloc = xopts_alloc;\n+\n+\tif (opts->commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n-\tcommit_argv = argv;\n+\topts->commit_argv = argv;\n }\n \n struct commit_message {\n@@ -311,15 +329,15 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)\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\tconst char *base_label, const char *next_label,\n+\t\t\tunsigned char *head, struct strbuf *msgbuf,\n+\t\t\tstruct 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-\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n \n \tindex_fd = hold_locked_index(&index_lock, 1);\n \n@@ -334,7 +352,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tnext_tree = next ? next->tree : empty_tree();\n \tbase_tree = base ? base->tree : empty_tree();\n \n-\tfor (xopt = xopts; xopt != xopts + xopts_nr; xopt++)\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@@ -346,7 +364,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\t\tcommit_locked_index(&index_lock))) {\n \t\trollback_lock_file(&index_lock);\n \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\treturn error(_(\"%s: Unable to write new index file\"), me);\n+\t\treturn error(_(\"%s: Unable to write new index file\"),\n+\t\t\topts->action == REVERT ? \"revert\" : \"cherry-pick\");\n \t}\n \trollback_lock_file(&index_lock);\n \n@@ -376,7 +395,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\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)\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@@ -384,9 +403,9 @@ static int run_git_commit(const char *defmsg)\n \n \targs[i++] = \"commit\";\n \targs[i++] = \"-n\";\n-\tif (signoff)\n+\tif (opts->signoff)\n \t\targs[i++] = \"-s\";\n-\tif (!edit) {\n+\tif (!opts->edit) {\n \t\targs[i++] = \"-F\";\n \t\targs[i++] = defmsg;\n \t}\n@@ -395,7 +414,7 @@ static int run_git_commit(const char *defmsg)\n \treturn run_command_v_opt(args, RUN_GIT_CMD);\n }\n \n-static int do_pick_commit(struct commit *commit)\n+static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n {\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n@@ -403,10 +422,10 @@ static int do_pick_commit(struct commit *commit)\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n+\tconst char *me = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n \tint res;\n \n-\tif (no_commit) {\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@@ -426,7 +445,7 @@ static int do_pick_commit(struct commit *commit)\n \tdiscard_cache();\n \n \tif (!commit->parents) {\n-\t\tif (action == REVERT)\n+\t\tif (opts->action == REVERT)\n \t\t\treturn error(_(\"Cannot revert a root commit\"));\n \t\tparent = NULL;\n \t}\n@@ -435,32 +454,31 @@ static int do_pick_commit(struct commit *commit)\n \t\tint cnt;\n \t\tstruct commit_list *p;\n \n-\t\tif (!mainline)\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 != mainline && p;\n+\t\t cnt != opts->mainline && p;\n \t\t cnt++)\n \t\t\tp = p->next;\n-\t\tif (cnt != mainline || !p)\n+\t\tif (cnt != opts->mainline || !p)\n \t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n-\t\t\t sha1_to_hex(commit->object.sha1), mainline);\n+\t\t\t sha1_to_hex(commit->object.sha1), opts->mainline);\n \t\tparent = p->item;\n-\t} else if (mainline > 0)\n+\t} else if (opts->mainline > 0)\n \t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n \t\t sha1_to_hex(commit->object.sha1));\n \telse\n \t\tparent = commit->parents->item;\n \n-\tif (allow_ff && parent && !hashcmp(parent->object.sha1, head))\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 == REVERT ? \"revert\" : \"cherry-pick\",\n+\t\treturn error(_(\"%s: cannot parse parent commit %s\"), me,\n \t\t\tsha1_to_hex(parent->object.sha1));\n \n \tif (get_message(commit, commit->buffer, &msg) != 0)\n@@ -476,7 +494,7 @@ static int do_pick_commit(struct commit *commit)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (action == REVERT) {\n+\tif (opts->action == REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -497,12 +515,12 @@ static int do_pick_commit(struct commit *commit)\n \t\tnext = commit;\n \t\tnext_label = msg.label;\n \t\tadd_message_to_msg(commit, &msgbuf, msg.message);\n-\t\tif (no_replay) {\n+\t\tif (opts->no_replay) {\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 (!no_commit)\n+\t\tif (!opts->no_commit)\n \t\t\tif ((res = write_cherry_pick_head(commit))) {\n \t\t\t\tfree_message(&msg);\n \t\t\t\tfree(defmsg);\n@@ -510,9 +528,9 @@ static int do_pick_commit(struct commit *commit)\n \t\t\t}\n \t}\n \n-\tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n+\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n-\t\t\t\t\t head, &msgbuf);\n+\t\t\t\t\thead, &msgbuf, opts);\n \t\twrite_message(&msgbuf, defmsg);\n \t} else {\n \t\tstruct commit_list *common = NULL;\n@@ -522,23 +540,23 @@ static int do_pick_commit(struct commit *commit)\n \n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n-\t\tres = try_merge_command(strategy, xopts_nr, xopts, common,\n+\t\tres = try_merge_command(opts->strategy, opts->xopts_nr,\n+\t\t\t\t\topts->xopts, common,\n \t\t\t\t\tsha1_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 == 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\terror(_(\"could not %s %s... %s\"),\n+\t\t\topts->action == REVERT ? \"revert\" : \"apply\",\n+\t\t\tfind_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n+\t\t\tmsg.subject);\n \t\tprint_advice();\n-\t\trerere(allow_rerere_auto);\n+\t\trerere(opts->allow_rerere_auto);\n \t} else {\n-\t\tif (!no_commit)\n-\t\t\tres = run_git_commit(defmsg);\n+\t\tif (!opts->no_commit)\n+\t\t\tres = run_git_commit(defmsg, opts);\n \t}\n \n \tfree_message(&msg);\n@@ -547,15 +565,15 @@ static int do_pick_commit(struct commit *commit)\n \treturn res;\n }\n \n-static int prepare_revs(struct rev_info *revs)\n+static int prepare_revs(struct rev_info *revs, struct replay_opts *opts)\n {\n \tinit_revisions(revs, NULL);\n \trevs->no_walk = 1;\n-\tif (action != REVERT)\n+\tif (opts->action != REVERT)\n \t\trevs->reverse = 1;\n \n-\tif (setup_revisions(commit_argc, commit_argv, revs, NULL) > 1)\n-\t\treturn error(_(\"usage: %s\"), *revert_or_cherry_pick_usage());\n+\tif (setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL) > 1)\n+\t\treturn error(_(\"usage: %s\"), *revert_or_cherry_pick_usage(opts));\n \n \tif (prepare_revision_walk(revs))\n \t\treturn error(_(\"revision walk setup failed\"));\n@@ -565,13 +583,12 @@ static int prepare_revs(struct rev_info *revs)\n \treturn 0;\n }\n \n-static int read_and_refresh_cache(void)\n+static int 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-\tconst char *me;\n+\tconst char *me = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n \n-\tme = (action == REVERT ? \"revert\" : \"cherry-pick\");\n \tif (read_index_preload(&the_index, NULL) < 0)\n \t\treturn error(_(\"%s: failed to read the index\"), me);\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n@@ -586,7 +603,8 @@ static int read_and_refresh_cache(void)\n \treturn 0;\n }\n \n-static int revert_or_cherry_pick(int argc, const char **argv)\n+static int revert_or_cherry_pick(int argc, const char **argv,\n+\t\t\t\tstruct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n@@ -594,27 +612,27 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \tint res;\n \n \tgit_config(git_default_config, NULL);\n-\tme = (action == REVERT ? \"revert\" : \"cherry-pick\");\n+\tme = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tparse_args(argc, argv);\n+\tparse_args(argc, argv, opts);\n \n-\tif (allow_ff) {\n-\t\tif (signoff)\n+\tif (opts->allow_ff) {\n+\t\tif (opts->signoff)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n-\t\tif (no_commit)\n+\t\tif (opts->no_commit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (no_replay)\n+\t\tif (opts->no_replay)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n-\t\tif (edit)\n+\t\tif (opts->edit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n \t}\n \n-\tif ((res = read_and_refresh_cache()) ||\n-\t\t(res = prepare_revs(&revs)))\n+\tif ((res = read_and_refresh_cache(opts)) ||\n+\t\t(res = prepare_revs(&revs, opts)))\n \t\treturn res;\n \n \twhile ((commit = get_revision(&revs)) &&\n-\t\t!(res = do_pick_commit(commit)))\n+\t\t!(res = do_pick_commit(commit, opts)))\n \t\t;\n \n \treturn res;\n@@ -622,14 +640,20 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n+\tstruct replay_opts opts;\n+\n+\tmemset(&opts, 0, sizeof(struct replay_opts));\n \tif (isatty(0))\n-\t\tedit = 1;\n-\taction = REVERT;\n-\treturn revert_or_cherry_pick(argc, argv);\n+\t\topts.edit = 1;\n+\topts.action = REVERT;\n+\treturn revert_or_cherry_pick(argc, argv, &opts);\n }\n \n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n-\taction = CHERRY_PICK;\n-\treturn revert_or_cherry_pick(argc, argv);\n+\tstruct replay_opts opts;\n+\n+\tmemset(&opts, 0, sizeof(struct replay_opts));\n+\topts.action = CHERRY_PICK;\n+\treturn revert_or_cherry_pick(argc, argv, &opts);\n }\n-- \n1.7.5.GIT\n"},{"id":"167670","messageId":"1305100822-20470-5-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/8] revert: Separate cmdline argument handling from the functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:18Z","receivedAt":"2011-05-11T08:00:18Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Reading the Git configuration, setting environment variables, parsing\ncommand-line arguments, and populating the options structure should be\ndone in cmd_cherry_pick/ cmd_revert. The job pick_commits of\nsimplified into setting up the revision walker and calling\ndo_pick_commit in a loop- later in the series, it will handle\nfailures, and serve as the starting point for continuation.\n\nBased-on-patch-by: Christian Couder \nSigned-off-by: Ramkumar Ramachandra \n---\n This patch is fairly straightforward.\n\n builtin/revert.c | 22 ++++++++++++----------\n 1 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8550927..288c898 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n \treturn 0;\n }\n \n-static int revert_or_cherry_pick(int argc, const char **argv,\n-\t\t\t\tstruct replay_opts *opts)\n+static int pick_commits(struct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n-\tconst char *me;\n \tint res;\n \n-\tgit_config(git_default_config, NULL);\n-\tme = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n-\tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tparse_args(argc, argv, opts);\n-\n \tif (opts->allow_ff) {\n \t\tif (opts->signoff)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n@@ -642,18 +635,27 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts;\n \n+\tgit_config(git_default_config, NULL);\n \tmemset(&opts, 0, sizeof(struct replay_opts));\n \tif (isatty(0))\n \t\topts.edit = 1;\n+\n \topts.action = REVERT;\n-\treturn revert_or_cherry_pick(argc, argv, &opts);\n+\tsetenv(GIT_REFLOG_ACTION, \"revert\", 0);\n+\tparse_args(argc, argv, &opts);\n+\n+\treturn pick_commits(&opts);\n }\n \n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts;\n \n+\tgit_config(git_default_config, NULL);\n \tmemset(&opts, 0, sizeof(struct replay_opts));\n \topts.action = CHERRY_PICK;\n-\treturn revert_or_cherry_pick(argc, argv, &opts);\n+\tsetenv(GIT_REFLOG_ACTION, \"cherry-pick\", 0);\n+\tparse_args(argc, argv, &opts);\n+\n+\treturn pick_commits(&opts);\n }\n-- \n1.7.5.GIT\n"},{"id":"167668","messageId":"1305100822-20470-6-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/8] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:19Z","receivedAt":"2011-05-11T08:00:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Earlier, incompatible command-line options used to be caught in\npick_commits after parse_args has parsed the options and populated the\noptions structure; a lot of unncessary work has already been done, and\nsignificant amount of cleanup is required to die at this stage.\nInstead, hand over this responsibility to parse_args so that the\nprogram can die early. Also write a die_opt_incompabile function to\nhandle incompatible options in a general manner; it will be used more\nextensively as more command-line options are introduced later in the\nseries.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n I think we _should_ die when an error in command-line parsing occurs,\n since it should always be the toplevel caller. Thanks to Junio for\n redesigning die_opt_incompatible in a sane manner.\n\n builtin/revert.c | 37 ++++++++++++++++++++++++++-----------\n 1 files changed, 26 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 288c898..0fe87e8 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -80,10 +80,29 @@ static int option_parse_x(const struct option *opt,\n \treturn 0;\n }\n \n+static void die_opt_incompatible(const char *me, const char *base_opt, ...)\n+{\n+\tconst char *this_opt;\n+\tint this_opt_set;\n+\tva_list ap;\n+\n+\tva_start(ap, base_opt);\n+\twhile (1) {\n+\t\tif (!(this_opt = va_arg(ap, const char *)))\n+\t\t\tbreak;\n+\t\tif ((this_opt_set = va_arg(ap, int)))\n+\t\t\tdie(_(\"%s: %s cannot be used with %s\"),\n+\t\t\t\tme, this_opt, base_opt);\n+\t}\n+\tva_end(ap);\n+}\n+\n static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n \tconst char *const *usage_str = revert_or_cherry_pick_usage(opts);\n+\tconst char *me = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n \tint noop;\n+\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN('n', \"no-commit\", &(opts->no_commit), \"don't automatically commit\"),\n \t\tOPT_BOOLEAN('e', \"edit\", &(opts->edit), \"edit the commit message\"),\n@@ -121,6 +140,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \tif (opts->commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n+\tif (opts->allow_ff)\n+\t\tdie_opt_incompatible(me, \"--ff\",\n+\t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\"--no-commit\", opts->no_commit,\n+\t\t\t\t\"-x\", opts->no_replay,\n+\t\t\t\t\"--edit\", opts->edit,\n+\t\t\t\tNULL);\n \topts->commit_argv = argv;\n }\n \n@@ -609,17 +635,6 @@ static int pick_commits(struct replay_opts *opts)\n \tstruct commit *commit;\n \tint res;\n \n-\tif (opts->allow_ff) {\n-\t\tif (opts->signoff)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n-\t\tif (opts->no_commit)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (opts->no_replay)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n-\t\tif (opts->edit)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n-\t}\n-\n \tif ((res = read_and_refresh_cache(opts)) ||\n \t\t(res = prepare_revs(&revs, opts)))\n \t\treturn res;\n-- \n1.7.5.GIT\n"},{"id":"167652","messageId":"1305100822-20470-7-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 6/8] revert: Introduce head, todo, done files to persist state","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:20Z","receivedAt":"2011-05-11T08:00:20Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"A cherry-pick/ revert operation consists of several smaller steps.\nLater in the series, we would like to be able to resume a failed\noperation. As a prelude, we first need to persist the current state\nof operation. Introduce a \"head\" file to make note of the HEAD when\nthe operation stated (so that the operation can be aborted), a \"todo\"\nfile to keep the list of the steps to be performed, and a \"done\" file\nto keep a list of steps that have completed successfully. The format\nof these files is similar to the one used by the \"rebase -i\" process.\n\nHelped-by: Christian Couder \nSigned-off-by: Ramkumar Ramachandra \n---\n Note how I've used the words \"operation\" and \"step\" to differentiate\n between a picking/ reverting a single commit versus the entire\n operation in the commit message. In one discussion involving\n Jonathan and Daniel, it was discussed that multiple meta-picking/\n reverting levels is a goal that is tangential to this project. I\n only intend to support two levels: the overall \"operation\", and the\n composite \"steps\".\n\n builtin/revert.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 111 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0fe87e8..13569c2 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -13,6 +13,7 @@\n #include \"rerere.h\"\n #include \"merge-recursive.h\"\n #include \"refs.h\"\n+#include \"dir.h\"\n \n /*\n * This implements the builtins revert and cherry-pick.\n@@ -25,6 +26,13 @@\n * Copyright (c) 2005 Junio C Hamano\n */\n \n+#define SEQ_DIR \"sequencer\"\n+\n+#define SEQ_PATH\tgit_path(SEQ_DIR)\n+#define HEAD_FILE\tgit_path(SEQ_DIR \"/head\")\n+#define TODO_FILE\tgit_path(SEQ_DIR \"/todo\")\n+#define DONE_FILE\tgit_path(SEQ_DIR \"/done\")\n+\n static const char * const revert_usage[] = {\n \t\"git revert [options] \",\n \tNULL\n@@ -629,21 +637,118 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n \treturn 0;\n }\n \n+static int format_todo(struct strbuf *buf, struct commit_list *list,\n+\t\t\tstruct replay_opts *opts)\n+{\n+\tstruct commit_list *cur = NULL;\n+\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n+\tconst char *sha1 = NULL;\n+\tconst char *action;\n+\n+\taction = (opts->action == REVERT ? \"revert\" : \"pick\");\n+\tfor (cur = list; cur; cur = cur->next) {\n+\t\tsha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n+\t\tif (get_message(cur->item, cur->item->buffer, &msg))\n+\t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1);\n+\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action, sha1, msg.subject);\n+\t}\n+\treturn 0;\n+}\n+\n+static int persist_initialize(unsigned char *head)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tif (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777)) {\n+\t\tint err = errno;\n+\t\tstrbuf_release(&buf);\n+\t\terror(_(\"Could not create sequencer directory '%s': %s\"),\n+\t\t\tSEQ_PATH, strerror(err));\n+\t\treturn -err;\n+\t}\n+\n+\tif ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {\n+\t\tint err = errno;\n+\t\tstrbuf_release(&buf);\n+\t\terror(_(\"Could not open '%s' for writing: %s\"),\n+\t\t\tHEAD_FILE, strerror(err));\n+\t\treturn -err;\n+\t}\n+\n+\tstrbuf_addf(&buf, \"%s\", find_unique_abbrev(head, DEFAULT_ABBREV));\n+\twrite_or_whine(fd, buf.buf, buf.len, HEAD_FILE);\n+\tclose(fd);\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+}\n+\n+static int persist_todo_done(int res, struct commit_list *todo_list,\n+\t\t\tstruct commit_list *done_list, struct replay_opts *opts)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd, res2;\n+\n+\tif (!res)\n+\t\treturn 0;\n+\n+\t/* TODO file */\n+\tif ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {\n+\t\tint err = errno;\n+\t\tstrbuf_release(&buf);\n+\t\terror(_(\"Could not open '%s' for writing: %s\"),\n+\t\t\tTODO_FILE, strerror(err));\n+\t\treturn -err;\n+\t}\n+\n+\tif ((res2 = format_todo(&buf, todo_list, opts)))\n+\t\treturn res2;\n+\twrite_or_whine(fd, buf.buf, buf.len, TODO_FILE);\n+\tclose(fd);\n+\n+\t/* DONE file */\n+\tstrbuf_reset(&buf);\n+\tif ((fd = open(DONE_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {\n+\t\tint err = errno;\n+\t\tstrbuf_release(&buf);\n+\t\terror(_(\"Could not open '%s' for writing: %s\"),\n+\t\t\tDONE_FILE, strerror(err));\n+\t\treturn -err;\n+\t}\n+\n+\tif ((res2 = format_todo(&buf, done_list, opts)))\n+\t\treturn res2;\n+\twrite_or_whine(fd, buf.buf, buf.len, DONE_FILE);\n+\tclose(fd);\n+\tstrbuf_release(&buf);\n+\treturn res;\n+}\n+\n static int pick_commits(struct replay_opts *opts)\n {\n+\tstruct commit_list *done_list = NULL;\n \tstruct rev_info revs;\n \tstruct commit *commit;\n+\tunsigned char head[20];\n \tint res;\n \n+\tif (get_sha1(\"HEAD\", head))\n+\t\treturn error(_(\"You do not have a valid HEAD\"));\n+\n \tif ((res = read_and_refresh_cache(opts)) ||\n-\t\t(res = prepare_revs(&revs, opts)))\n+\t\t(res = prepare_revs(&revs, opts)) ||\n+\t\t(res = persist_initialize(head)))\n \t\treturn res;\n \n-\twhile ((commit = get_revision(&revs)) &&\n-\t\t!(res = do_pick_commit(commit, opts)))\n-\t\t;\n-\n-\treturn res;\n+\twhile ((commit = get_revision(&revs))) {\n+\t\tif (!(res = do_pick_commit(commit, opts)))\n+\t\t\tcommit_list_insert(commit, &done_list);\n+\t\telse {\n+\t\t\tcommit_list_insert(commit, &revs.commits);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\treturn persist_todo_done(res, revs.commits, done_list, opts);\n }\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n-- \n1.7.5.GIT\n"},{"id":"167669","messageId":"1305100822-20470-8-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 7/8] revert: Implement parsing --continue, --abort and --skip","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:21Z","receivedAt":"2011-05-11T08:00:21Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce three new command-line options: --continue, --abort, and\n--skip resembling the correspoding options in \"rebase -i\". For now,\njust parse the options into the replay_opts structure, making sure\nthat two of them are not specified together. They will actually be\nimplemented later in the series.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n This patch is fairly straightforward.\n\n builtin/revert.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 49 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 13569c2..ccfc295 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -46,6 +46,11 @@ static const char * const cherry_pick_usage[] = {\n static struct replay_opts {\n \tenum { REVERT, CHERRY_PICK } action;\n \n+\t/* --abort, --skip, and --continue */\n+\tint abort_oper;\n+\tint skip_oper;\n+\tint continue_oper;\n+\n \t/* Boolean options */\n \tint edit;\n \tint no_replay;\n@@ -112,6 +117,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \tint noop;\n \n \tstruct option options[] = {\n+\t\tOPT_BOOLEAN(0, \"abort\", &(opts->abort_oper), \"abort the current operation\"),\n+\t\tOPT_BOOLEAN(0, \"skip\", &(opts->skip_oper), \"skip the current commit\"),\n+\t\tOPT_BOOLEAN(0, \"continue\", &(opts->continue_oper), \"continue the current operation\"),\n \t\tOPT_BOOLEAN('n', \"no-commit\", &(opts->no_commit), \"don't automatically commit\"),\n \t\tOPT_BOOLEAN('e', \"edit\", &(opts->edit), \"edit the commit message\"),\n \t\tOPT_BOOLEAN('r', NULL, &noop, \"no-op (backward compatibility)\"),\n@@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \topts->xopts_nr = xopts_nr;\n \topts->xopts_alloc = xopts_alloc;\n \n-\tif (opts->commit_argc < 2)\n+\t/* Check for incompatible command line arguments */\n+\tif (opts->abort_oper || opts->skip_oper || opts->continue_oper) {\n+\t\tchar *this_oper;\n+\t\tif (opts->abort_oper) {\n+\t\t\tthis_oper = \"--abort\";\n+\t\t\tdie_opt_incompatible(me, this_oper,\n+\t\t\t\t\t\"--skip\", opts->skip_oper,\n+\t\t\t\t\tNULL);\n+\t\t\tdie_opt_incompatible(me, this_oper,\n+\t\t\t\t\t\"--continue\", opts->continue_oper,\n+\t\t\t\t\tNULL);\n+\t\t} else if (opts->skip_oper) {\n+\t\t\tthis_oper = \"--skip\";\n+\t\t\tdie_opt_incompatible(me, this_oper,\n+\t\t\t\t\t\"--abort\", opts->abort_oper,\n+\t\t\t\t\tNULL);\n+\t\t\tdie_opt_incompatible(me, this_oper,\n+\t\t\t\t\t\"--continue\", opts->continue_oper,\n+\t\t\t\t\tNULL);\n+\t\t} else {\n+\t\t\tthis_oper = \"--continue\";\n+\t\t\tdie_opt_incompatible(me, this_oper,\n+\t\t\t\t\t\"--abort\", opts->abort_oper,\n+\t\t\t\t\tNULL);\n+\t\t\tdie_opt_incompatible(me, this_oper,\n+\t\t\t\t\t\"--skip\", opts->skip_oper,\n+\t\t\t\t\tNULL);\n+\t\t}\n+\t\tdie_opt_incompatible(me, this_oper,\n+\t\t\t\t\"--no-commit\", opts->no_commit,\n+\t\t\t\t\"--edit\", opts->edit, \"-r\", noop,\n+\t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\"--mainline\", opts->mainline,\n+\t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n+\t\t\t\t\"--strategy-option\", opts->xopts ? 1 : 0,\n+\t\t\t\t\"-x\", opts->no_replay,\n+\t\t\t\t\"--ff\", opts->allow_ff,\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-- \n1.7.5.GIT\n"},{"id":"167673","messageId":"1305100822-20470-9-git-send-email-artagnon@gmail.com","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 8/8] revert: Implement --abort processing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-11T08:00:22Z","receivedAt":"2011-05-11T08:00:22Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"To abort, perform a \"rerere clear\" and \"reset --hard\" to the ref\nspecified by the \"head\" file introduced earlier in the series.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n This code is dependent on the rerere_clear public API (which I've\n posted in another thread). Have the essential parts of the \"reset\n --hard\" been copied over correctly? Should reset get a public API as\n well?\n\n builtin/revert.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 50 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex ccfc295..50c36e9 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -799,6 +799,54 @@ static int pick_commits(struct replay_opts *opts)\n \treturn persist_todo_done(res, revs.commits, done_list, opts);\n }\n \n+static int process_args(int argc, const char **argv, struct replay_opts *opts)\n+{\n+\tconst char *me;\n+\tint fd;\n+\n+\tparse_args(argc, argv, opts);\n+\tme = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n+\tif (opts->abort_oper) {\n+\t\tchar head[DEFAULT_ABBREV];\n+\t\tunsigned char sha1[20];\n+\t\trerere_clear(0);\n+\n+\t\tif (!file_exists(HEAD_FILE))\n+\t\t\tgoto error;\n+\t\tif ((fd = open(HEAD_FILE, O_RDONLY, 0666)) < 0) {\n+\t\t\tint err = errno;\n+\t\t\terror(_(\"Could not open '%s' for reading: %s\"),\n+\t\t\t\tHEAD_FILE, strerror(err));\n+\t\t\treturn -err;\n+\t\t}\n+\t\tif (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV) {\n+\t\t\tint err = errno;\n+\t\t\tclose(fd);\n+\t\t\terror(_(\"Corrupt '%s': %s\"), HEAD_FILE, strerror(err));\n+\t\t\treturn -err;\n+\t\t}\n+\t\tclose(fd);\n+\n+\t\tif (get_sha1(head, sha1))\n+\t\t\treturn error(_(\"Failed to resolve '%s' as a valid ref.\"), head);\n+\t\tupdate_ref(NULL, \"HEAD\", sha1, NULL, 0, MSG_ON_ERR);\n+\t}\n+\telse if (opts->skip_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\treturn 0;\n+\t}\n+\telse if (opts->continue_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\treturn 0;\n+\t}\n+\n+\treturn pick_commits(opts);\n+error:\n+\treturn error(_(\"No %s in progress\"), me);\n+}\n+\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts;\n@@ -810,9 +858,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \n \topts.action = REVERT;\n \tsetenv(GIT_REFLOG_ACTION, \"revert\", 0);\n-\tparse_args(argc, argv, &opts);\n-\n-\treturn pick_commits(&opts);\n+\treturn process_args(argc, argv, &opts);\n }\n \n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n@@ -823,7 +869,5 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tmemset(&opts, 0, sizeof(struct replay_opts));\n \topts.action = CHERRY_PICK;\n \tsetenv(GIT_REFLOG_ACTION, \"cherry-pick\", 0);\n-\tparse_args(argc, argv, &opts);\n-\n-\treturn pick_commits(&opts);\n+\treturn process_args(argc, argv, &opts);\n }\n-- \n1.7.5.GIT\n"},{"id":"167677","messageId":"20110511095949.GA2676@elie","threadId":"27321","inReplyTo":"1305100822-20470-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-11T09:59:49Z","receivedAt":"2011-05-11T09:59:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamkumar Ramachandra wrote:\n[reordered for convenience]\n\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -47,3 +47,17 @@ void NORETURN die_resolve_conflict(const char *me)\n> \telse\n> \t\tdie(\"'%s' is not possible because you have unmerged files.\", me);\n> }\n> +\n> +int error_resolve_conflict(const char *me)\n> +{\n> +\tif (advice_resolve_conflict)\n> +\t\t/*\n> +\t\t * Message used both when 'git commit' fails and when\n> +\t\t * other commands doing a merge do.\n> +\t\t */\n> +\t\treturn error(\"'%s' is not possible because you have unmerged files.\\n\"\n> +\t\t\t\"Please, fix them up in the work tree, and then use 'git add/rm ' as\\n\"\n> +\t\t\t\"appropriate to mark resolution and make a commit, or use 'git commit -a'.\", me);\n> +\telse\n> +\t\treturn error(\"'%s' is not possible because you have unmerged files.\", me);\n> +}\n\nWould it make sense to do\n\n void NORETURN die_resolve_conflict(const char *me)\n {\n\terror_resolve_conflict(me);\n\texit(128);\n }\n\nor is the s/fatal/error/ in output too much? I would suspect\nsaying \"error:\" instead of \"fatal:\" should be okay if it means having\nless code to deal with, but if not, maybe there is some other way to\nachieve the appropriate effect (e.g. --- please don’t use this; it’s\njust an example --- when a global in usage.c about_to_die is true,\nchanging \"error:\" to \"fatal:\" or something like that).\n\n> As a general\n> guideline for libification, \"die\" should be only be called in two\n> cases: by toplevel callers like command-line argument parsing\n> routines, or when an irrecoverable situation is encountered.\n\nThe above hints at to a downside to this principle: currently the\nmessage printed by \"die\" is clearly labelled as git’s cause of death.\nStill, I think the principle is right and that it’s worth it, and that\ngenerally speaking a library function should not have to know whether\nits errors would be an appropriate time to exit or this is a GUI that\nwill want to stay alive.\n\n> Has the commit message justified this change fully?\n\nIt didn’t mention \"GUI\", so I guess no. :)\n\nCould you give a reminder about how this fits in the sequencer series?\nIs it that you want to give advice to the user about how to recover\nwhen exiting (and if so, could an atexit handler work instead for\nthat)?\n\nI'm reviewing the rest because I like the aforementioned \"GUI\" use\ncase, but if you get tired of the exercise, please remember that it is\nnot self-evidently necessary to continue along this track for the sake\nof the sequencer alone.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -167,9 +167,11 @@ static char *get_encoding(const char *message)\n> {\n> \tconst char *p = message, *eol;\n> \n> -\tif (!p)\n> -\t\tdie (_(\"Could not read commit message of %s\"),\n> -\t\t\t\tsha1_to_hex(commit->object.sha1));\n> +\tif (!p) {\n> +\t\terror(_(\"Could not read commit message of %s\"),\n> +\t\t\tsha1_to_hex(commit->object.sha1));\n> +\t\treturn NULL;\n\nThe only caller to get_encoding is get_message, which already returns\nearly when not passed a usable message. So maybe this should say\n\n\tassert(p);\n\nor\n\n\tif (!p)\n\t\tdie(\"BUG: get_message caller forgot to give a commit message\");\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -198,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n> \tstrbuf_addstr(msgbuf, p);\n> }\n> \n> -static void write_cherry_pick_head(void)\n> +static int write_cherry_pick_head(void)\n\nwrite_cherry_pick_head gets called by do_pick_commit to help a person\npick up where \"git cherry-pick\" left off if it has to exit when\nresolving conflicts. So it’s a good example of how to avoid having\nto clean up after code when it fails. :)\n\nAnyway, the question here is, what should happen if\nwrite_cherry_pick_head fails (for example due to EPERM or ENOSPC)? If\nthis is a GUI or another program that has to stay alive, I suppose one\nwould want to report the error and back out the cherry-pick of the\ncurrent commit as far as it has proceeded (meaning freeing some\nmemory). From the signature above I would expect that it does\n\n\treturn error(...);\n\non failure (which would bring up an explanation of the failure under\nthe tab bar in my GUI).\n\n> {\n> \tint fd;\n> \tstruct strbuf buf = STRBUF_INIT;\n> @@ -206,12 +208,22 @@ static void write_cherry_pick_head(void)\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 (fd < 0) {\n> +\t\tint err = errno;\n> +\t\tstrbuf_release(&buf);\n> +\t\terror(_(\"Could not open '%s' for writing: %s\"),\n> +\t\t\tgit_path(\"CHERRY_PICK_HEAD\"), strerror(err));\n> +\t\treturn -err;\n> +\t}\n\nWhy does the caller care about errno (i.e., why \"return -errno\"\ninstead of \"return error(...)\")?\n\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> +\tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd)) {\n> +\t\tint err = errno;\n> +\t\tstrbuf_release(&buf);\n> +\t\terror(_(\"Could not write to '%s': %s\"),\n> +\t\t\tgit_path(\"CHERRY_PICK_HEAD\"), strerror(err));\n> +\t\treturn -err;\n> +\t}\n\nIn the write_in_full case, missing \"close(fd)\".\n\nIn the close(fd) case, not missing \"close(fd)\", unless we want to\ncheck for errno == EINTR and handle that. (If we do, though, it would\nprobably be in a separate patch, by introducing an xclose function\nanalagous to xwrite.) So I suppose this should look something like\n\n\tif (write_in_full(...)) {\n\t\tret = error(_(\"Could not write...\n\t\tgoto done3;\n\t}\n\tif (close(fd)) {\n\t\tret = error(_(...\n\t\tgoto done2;\n\t}\n\tgoto done;\n\n done3:\n\tclose(fd);\n done2:\n\tunlink_or_warn(git_path(\"CHERRY_PICK_HEAD\"));\n done:\n\tstrbuf_release(&buf);\n\treturn ret;\n }\n\n> @@ -243,17 +255,22 @@ static void print_advice(void)\n> \tadvise(\"and commit the result with 'git commit'\");\n> }\n> \n> -static void write_message(struct strbuf *msgbuf, const char *filename)\n> +static int write_message(struct strbuf *msgbuf, const char *filename)\n\nwrite_message writes the MERGE_MSG file. Like get_cherry_pick_head,\nits purpose is to allow recovery if the cherry-pick runs into\nconflicts, and presumably the appropriate way to recover would be to\nback out the cherry-pick (meaning to free buffers used so far and\nremove the CHERRY_PICK_HEAD file).\n\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> +\tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {\n> +\t\tint err = errno;\n> +\t\tstrbuf_release(msgbuf);\n> +\t\terror(_(\"Could not write to %s: %s\"), filename, strerror(err));\n> +\t\treturn -err;\n> +\t}\n\nSame question as before: why does the caller care about the nature of\nthe error?\n\nhold_lock_file will die on error unless we declare we want to handle\nthat ourselves through its flags word. If we are not about to exit,\nwe will also want to roll back the lockfile on errors, too.\n\n> \tstrbuf_release(msgbuf);\n> \tif (commit_lock_file(&msg_file) < 0)\n> -\t\tdie(_(\"Error wrapping up %s\"), filename);\n> +\t\treturn error(_(\"Error wrapping up %s\"), filename);\n\nIf we use the \"return -errno\" convention, that would mean \"return\n-EPERM\" on traditional Unixen.\n\n[...]\n> +static int error_dirty_worktree(const char *me)\n> +{\n> +\tif (advice_commit_before_merge)\n> +\t\treturn error(_(\"Your local changes would be overwritten by %s.\\n\"\n> +\t\t\t\t\"Please, commit your changes or stash them to proceed.\"), me);\n> +\treturn error(_(\"Your local changes would be overwritten by %s.\\n\"), me);\n> +}\n\nSide note: git still doesn’t have a good API for dealing with advice.\nI think I’d prefer\n\n\terror(_(\"Your local changes would be overwritten by %s.\\n\"), me);\n\tif (advice_commit_before_merge)\n\t\tadvise(_(\"Please, commit your...\n\nor perhaps that last part would be\n\n\tadvise(advice_commit_before_merge,\n\t _(\"Please,...\n\nwhich would write\n\n fatal: your local changes would be overwritten by the cherry-pick\n hint: please, commit your changes or stash them to proceed\n\n> -static NORETURN void die_dirty_index(const char *me)\n> -{\n> -\tif (read_cache_unmerged()) {\n> -\t\tdie_resolve_conflict(me);\n> -\t} else {\n> -\t\tif (advice_commit_before_merge) {\n> -\t\t\tif (action == REVERT)\n> -\t\t\t\tdie(_(\"Your local changes would be overwritten by revert.\\n\"\n> -\t\t\t\t\t \"Please, commit your changes or stash them to proceed.\"));\n[...]\n> +static int verify_resolution(const char *me)\n> +{\n> +\tif (!read_cache_unmerged())\n> +\t\treturn 0;\n> +\n> +\treturn error_resolve_conflict(me);\n> +}\n> +\n\nHow is this function meant to be used? die_dirty_index would\npresumably become something like\n\n\tif (verify_resolution(me))\n\t\treturn -1;\n\treturn error_dirty_worktree(me);\n\nbut that is not what your caller for this does. Will say more when I\nget to it.\n\n[...]\n> @@ -329,10 +341,12 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\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(write_cache(index_fd, active_cache, active_nr) ||\n> +\t\t\tcommit_locked_index(&index_lock))) {\n\nWhitespace changes seem to have snuck in.\n\n> +\t\trollback_lock_file(&index_lock);\n\nMakes sense.\n\n> \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n> -\t\tdie(_(\"%s: Unable to write new index file\"), me);\n> +\t\treturn error(_(\"%s: Unable to write new index file\"), me);\n> +\t}\n> \trollback_lock_file(&index_lock);\n\nmerge_recursive can die, too (e.g., \"error building trees\").\n\n[...]\n> @@ -397,19 +411,21 @@ static int do_pick_commit(void)\n\ndo_pick_commit is the main worker of \"git cherry-pick foo..bar\". When\nit returned an error, traditionally that meant it had encountered a\nconflict, and the return value would what exit status to use to\nindicate that (typically 1, one hopes, but perhaps 139 if the merge\nstrategy segfaulted, etc).\n\nTo recover, I suppose I can imagine a caller wanting to \"try harder\".\nAt any rate the CHERRY_PICK_HEAD, MERGE_HEAD, on-disk dirty index, and\nso on are good state that should be kept.\n\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\tif (write_cache_as_tree(head, 0, NULL)) {\n> +\t\t\tdiscard_cache();\n\nI suppose this is cleaning up after the read_cache call in\nwrite_cache_as_tree? But the above does not back out the index lock\nat the same time, so I don’t find it so clear. I would prefer to\nsee write_cache_as_tree taught to clean up after itself on error.\n\n[...]\n> @@ -418,19 +434,19 @@ static int do_pick_commit(void)\n> \t\tstruct commit_list *p;\n> \n> \t\tif (!mainline)\n> -\t\t\tdie(_(\"Commit %s is a merge but no -m option was given.\"),\n> -\t\t\t sha1_to_hex(commit->object.sha1));\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\nThis is not a conflict but an error in usage. Stepping back for a\nsecond, what is the best way to handle that? The aforementioned\nhypothetical caller considering \"try an alternate strategy\" would make\na wrong move, so we need to use a return value that would dissuade it.\n\nI’m not sure what the most appropriate thing to do is. Maybe\n\n\t/*\n\t * Positive numbers are exit status from conflicts; negative\n\t * numbers are other errors.\n\t */\n\tenum pick_commit_error {\n\t\tPICK_COMMIT_USAGE_ERROR = -1\n\t};\n\nbut it’s hard to think clearly about it since it seems too\nhypothetical to me. If all callers are going to exit, then\n\n\terror(...);\n\treturn 129;\n\nwill work, but in that case why not exit for them?\n\n> \n> \t\tfor (cnt = 1, p = commit->parents;\n> \t\t cnt != mainline && p;\n> \t\t cnt++)\n> \t\t\tp = p->next;\n> \t\tif (cnt != mainline || !p)\n> -\t\t\tdie(_(\"Commit %s does not have parent %d\"),\n> +\t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n> \t\t\t sha1_to_hex(commit->object.sha1), mainline);\n\nLikewise.\n\n> \t\tparent = p->item;\n> -\t} else if (0 < mainline)\n> -\t\tdie(_(\"Mainline was specified but commit %s is not a merge.\"),\n> +\t} else if (mainline > 0)\n> +\t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n> \t\t sha1_to_hex(commit->object.sha1));\n\nLikewise.\n\n> \telse\n> \t\tparent = commit->parents->item;\n> @@ -441,11 +457,11 @@ static int do_pick_commit(void)\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\tdie(_(\"%s: cannot parse parent commit %s\"),\n> +\t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n> \t\t me, sha1_to_hex(parent->object.sha1));\n> \n> \tif (get_message(commit->buffer, &msg) != 0)\n> -\t\tdie(_(\"Cannot get commit message for %s\"),\n> +\t\treturn error(_(\"Cannot get commit message for %s\"),\n> \t\t\t\tsha1_to_hex(commit->object.sha1));\n> \n> \t/*\n> @@ -484,7 +500,11 @@ static int do_pick_commit(void)\n> \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n> \t\t}\n> \t\tif (!no_commit)\n> -\t\t\twrite_cherry_pick_head();\n> +\t\t\tif ((res = write_cherry_pick_head())) {\n> +\t\t\t\tfree_message(&msg);\n> +\t\t\t\tfree(defmsg);\n> +\t\t\t\treturn res;\n> +\t\t\t}\n\n\nThese need to eventually exit with status 128.\n\n> @@ -524,44 +544,46 @@ static int do_pick_commit(void)\n> \treturn res;\n> }\n> \n> -static void prepare_revs(struct rev_info *revs)\n> +static int prepare_revs(struct rev_info *revs)\n\nprepare_revs is used by the cherry-pick porcelain after parsing\narguments to initialize the revision walker. If it fails, we can\nsave some trouble and cancel the whole operation.\n\n> {\n> -\tint argc;\n> -\n> \tinit_revisions(revs, NULL);\n> \trevs->no_walk = 1;\n> \tif (action != REVERT)\n> \t\trevs->reverse = 1;\n> \n> -\targc = setup_revisions(commit_argc, commit_argv, revs, NULL);\n> -\tif (argc > 1)\n> -\t\tusage(*revert_or_cherry_pick_usage());\n> +\tif (setup_revisions(commit_argc, commit_argv, revs, NULL) > 1)\n> +\t\treturn error(_(\"usage: %s\"), *revert_or_cherry_pick_usage());\n\nThe exit status for errors in usage ought to be 129, and the message\n\n\terror: usage: ...\n\nis not so pretty. :)\n\nI think a library version of this would want to take a struct rev_info\nthat has been prepopulated.\n\n[...]\n> -static void read_and_refresh_cache(const char *me)\n> +static int read_and_refresh_cache(const char *me)\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\"), me);\n> +\t\treturn error(_(\"%s: failed to read the index\"), me);\n\nWhat is the caller expecting to happen when this fails? Do we\nwant to roll back the lockfile and discard the index?\n\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\tcommit_locked_index(&index_lock)) {\n\nWhitespace change snuck in.\n\n> -\t\t\tdie(_(\"git %s: failed to refresh the index\"), me);\n> +\t\t\trollback_lock_file(&index_lock);\n> +\t\t\treturn error(_(\"%s: failed to refresh the index\"), me);\n\nLikewise. (Should this discard the index? If so, why? If not, why\nnot?)\n\n> +\t\t}\n> \t}\n> \trollback_lock_file(&index_lock);\n> +\treturn 0;\n> }\n> \n> static int revert_or_cherry_pick(int argc, const char **argv)\n\nThe porcelain itself, but the sequencer wants to call it.\n\n> {\n> \tstruct rev_info revs;\n> +\tint res;\n> \n> \tgit_config(git_default_config, NULL);\n> \tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n> @@ -579,17 +601,15 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n> \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n> \t}\n> \n> -\tread_and_refresh_cache(me);\n> -\tprepare_revs(&revs);\n> -\n> +\tif ((res = read_and_refresh_cache(me)) ||\n> +\t\t(res = prepare_revs(&revs)))\n> +\t\treturn res;\n\nClearer to write:\n\n\tif (read_and_refresh_cache(me) ||\n\t prepare_revs(&revs))\n\t\treturn -1;\n\nand it has the nice side-effect of making it clear to callers that\nthey don’t have to worry about return value conventions from those\nfunctions.\n\n> -\twhile ((commit = get_revision(&revs))) {\n> -\t\tint res = do_pick_commit();\n> -\t\tif (res)\n> -\t\t\treturn res;\n> -\t}\n> -\n> -\treturn 0;\n> +\twhile ((commit = get_revision(&revs)) &&\n> +\t\t!(res = do_pick_commit()))\n> +\t\t;\n> +\n> +\treturn res;\n\nI think the original is clearer here.\n\n> }\n>\n[...]\n> How do I trap and handle the exit status from write_message in\n> do_pick_commit correctly? There's already a call to\n> do_recursive_merge whose exit status is being trapped -- what happens\n> when do_recursive_merge succeeds and write_message fails, or\n> viceversa?\n\nGood question. See my confused \"enum\" ramble above.\n\n> Junio has suggested dropping error_errno, and simply using error and\n> returning -errno by hand in one email. Considering the number of\n> times I've used that tecnique, and I think we should get something\n> like an error_errno atleast for the sake of terseness.\n\nI do think an error_errno that unconditionally returns -1 (analagous\nto die_errno) would be a nice thing to have, even though it wouldn’t\nmake anything much shorter. :)\n\n*whew*\n\nThanks and hope that helps,\nJonathan\n"},{"id":"167665","messageId":"20110511103704.GB2676@elie","threadId":"27321","inReplyTo":"1305100822-20470-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/8] revert: Make \"commit\" and \"me\" local variables","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-11T10:37:04Z","receivedAt":"2011-05-11T10:37:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Currently, \"commit\" and \"me\" are global static variables. Since we\n> want to develop the functionality to either pick/ revert individual\n> commits atomically later in the series, make them local variables.\n\nI suppose the idea is that the current commit and whether we are\ncherry-picking or reverting is not global state and should be allowed\nto differ between threads, or that for easier debugging we would like\nto narrow their scope.\n\nHow does this relate to the sequencer series? Maybe the idea is that\nthey are explicit parameters in the functions that will be exposed\nrather than that they are local variables?\n\n>\n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> The variable \"me\" is nowhere as fundamental as \"commit\" -- it's\n> simply a string derived from a more fundamental \"action\".\n\nThat suggests to me that \"action\" should probably be made local at the\nsame time. On second thought, it looks like this commit is doing two\nunrelated things ---\n\n - simplifying the state that has to be kept by computing \"me\"\n from \"action\" on the fly\n\n - narrowing the scope of \"commit\" and passing it around explicitly\n\nand would be clearer as two separate commits.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n[...]\n> @@ -51,7 +49,7 @@ static size_t xopts_nr, xopts_alloc;\n> \n> #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n> \n> -static char *get_encoding(const char *message);\n> +static char *get_encoding(struct commit *commit, const char *message);\n\nIf the die is converted to an assert or die(\"BUG: ...\") without\nspecifying which commit then this first parameter is not needed.\n\n> static const char * const *revert_or_cherry_pick_usage(void)\n> {\n> @@ -116,7 +114,8 @@ struct commit_message {\n> \tconst char *message;\n> };\n> \n> -static int get_message(const char *raw_message, struct commit_message *out)\n> +static int get_message(struct commit *commit, const char *raw_message,\n> +\t\tstruct commit_message *out)\n\nLikewise.\n\n[...]\n> @@ -187,7 +186,8 @@ static char *get_encoding(const char *message)\n> \treturn NULL;\n> }\n> \n> -static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n> +static void add_message_to_msg(struct commit *commit, struct strbuf *msgbuf,\n> +\t\t\tconst char *message)\n\nPerhaps the new parameter could be \"const char *fallback\" and the\ncaller call sha1_to_hex unconditionally? (Yes, it sounds like wasted\ncomputation, but it might be worth the clarity.)\n\n> @@ -200,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n> \tstrbuf_addstr(msgbuf, p);\n> }\n> \n> -static int write_cherry_pick_head(void)\n> +static int write_cherry_pick_head(struct commit *commit)\n\nAh, it might not be wasted computation. This could take\ncommit_sha1_hex as parameter so it only needs to be computed once.\n\n> @@ -319,6 +319,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> \tint clean, index_fd;\n> \tconst char **xopt;\n> \tstatic struct lock_file index_lock;\n> +\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n\nStyle: I find this clearer without the parentheses (but feel free to\nignore).\n\n[...]\n> @@ -402,6 +403,7 @@ static int do_pick_commit(void)\n> \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n> \tchar *defmsg = NULL;\n> \tstruct strbuf msgbuf = STRBUF_INIT;\n> +\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n> \tint res;\n> \n> \tif (no_commit) {\n> @@ -458,9 +460,10 @@ static int do_pick_commit(void)\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 me, sha1_to_hex(parent->object.sha1));\n> +\t\t\taction == REVERT ? \"revert\" : \"cherry-pick\",\n> +\t\t\tsha1_to_hex(parent->object.sha1));\n\nI think one of the computations of \"me\" is left over.\n\n> @@ -562,10 +565,13 @@ static int prepare_revs(struct rev_info *revs)\n> \treturn 0;\n> }\n> \n> -static int read_and_refresh_cache(const char *me)\n> +static int read_and_refresh_cache(void)\n\nSince you seem to be moving towards having fewer statics and more\nexplicit parameters, I think this part is a step backwards. Maybe it\nshould take \"action\" as a parameter instead.\n\n> @@ -583,10 +589,12 @@ static int read_and_refresh_cache(const char *me)\n> static int revert_or_cherry_pick(int argc, const char **argv)\n> {\n> \tstruct rev_info revs;\n> +\tstruct commit *commit;\n> +\tconst char *me;\n> \tint res;\n> \n> \tgit_config(git_default_config, NULL);\n> -\tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n> +\tme = (action == REVERT ? \"revert\" : \"cherry-pick\");\n\nWhy?\n\n> \tsetenv(GIT_REFLOG_ACTION, me, 0);\n> \tparse_args(argc, argv);\n> \n\nSorry, mostly nitpicks. Still, hope that helps.\n\nRegards,\nJonathan\n"},{"id":"167675","messageId":"20110511112438.GD2676@elie","threadId":"27321","inReplyTo":"1305100822-20470-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/8] revert: Introduce a struct to parse command-line options into","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-11T11:24:38Z","receivedAt":"2011-05-11T11:24:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> I get the following warning from GCC: warning: useless storage class\n> specifier in empty declaration (at the line where I've declared the\n> replay_opts struct). What is the correct way to fix this?\n\nRemove the useless storage class specifier (\"static\"). :)\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -35,29 +35,42 @@ static const char * const cherry_pick_usage[] = {\n[...]\n> +static struct replay_opts {\n> +\tenum { REVERT, CHERRY_PICK } action;\n> +\n> +\t/* Boolean options */\n> +\tint edit;\n> +\tint no_replay;\n\nreplay but no replay?\n\nI think originally git-revert.sh had a \"replay\" variable meaning \"This\nis not a revert (which undoes a commit) but a cherry-pick (which\nre-does it).\" Later the purpose changed to \"We are not cherry-picking\nand referring to the original with cherry-pick -x but replaying a\ncommit and treating it as new\".\n\nNow with struct replay_opts you are proposing to make the term mean\n\"we are using git revert machinery, or in other words replaying the\nchange an old commit made (forwards or backwards)\", which makes sense.\nIn this case there should probably be a patch right before which\nrenames no_replay to i_really_want_to_expose_my_private_commit_object_name\n(um, I mean to record_origin or something similar).\n\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> -static const char * const *revert_or_cherry_pick_usage(void)\n> +static const char *const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n\nLine is getting long. Whitespace change snuck in?\n\nI suppose if I ran the world the argument would be of type \"enum\nreplay_action\", so it would be used as\n\n\tusage(revert_or_cherry_pick_usage(o->action));\n\n> +/* For option_parse_x */\n> +static const char **xopts;\n> +static size_t xopts_nr, xopts_alloc;\n> +\n\nHm. In C89, struct initializers are not allowed to include addresses\nthat are not known until run-time, and we used to follow that and now\nviolate it all over the place. I'm not sure if it's worth it or not.\n(I'm tempted to say, let it deteriorate further and people with the\nability to test on such platforms can fix it, but commits like\nv1.7.2-rc0~32^2~18, Rewrite dynamic structure initializations to\nruntime assignment, 2010-05-14, suggest that some people have cared in\nthe recent future.)\n\nSo.\n\nIf you want to use parse_options and support such compilers, it is\nindeed simplest to use static variables. You can give them scope\nlocal to a particular function to at least avoid namespace polution.\n\nTo avoid such static variables at the expense of support for old\ncompilers, one can pass a pointer to a struct to option_parse_x\ninstead of the dummy &xopts. Within option_parse_x, what you pass\nwill be accessible as opt->value. It's all explained in\nDocumentation/technical/api-parse-options.txt, or one can grep around\nfor OPT_CALLBACK for examples. A simple variant on this will work\nwith the old compilers, too.\n\n[...]\n> static int option_parse_x(const struct option *opt,\n> -\t\t\t const char *arg, int unset)\n> +\t\t\tconst char *arg, int unset)\n\nWhitespace change snuck in.\n\n[...]\n> @@ -67,19 +80,18 @@ static int option_parse_x(const struct option *opt,\n> \treturn 0;\n> }\n> \n> -static void parse_args(int argc, const char **argv)\n> +static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> {\n> -\tconst char * const * usage_str = revert_or_cherry_pick_usage();\n> +\tconst char *const *usage_str = revert_or_cherry_pick_usage(opts);\n> \tint noop;\n> \tstruct option options[] = {\n> -\t\tOPT_BOOLEAN('n', \"no-commit\", &no_commit, \"don't automatically commit\"),\n> +\t\tOPT_BOOLEAN('n', \"no-commit\", &(opts->no_commit), \"don't automatically commit\"),\n\nThe parentheses are not needed (and not idiomatic fwiw). The line is\ngetting long so I'd suggest splitting it, though that's more a matter\nof taste.\n\n> @@ -87,23 +99,29 @@ static void parse_args(int argc, const char **argv)\n[...]\n> -\tif (commit_argc < 2)\n> +\n> +\t/* Fill in the opts struct from values set by option_parse_x */\n> +\topts->xopts = xopts;\n> +\topts->xopts_nr = xopts_nr;\n> +\topts->xopts_alloc = xopts_alloc;\n\nYep, something like this is needed (for all the options) if we want to\nfollow C89's option-struct-initialization rules.\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\tconst char *base_label, const char *next_label,\n> +\t\t\tunsigned char *head, struct strbuf *msgbuf,\n> +\t\t\tstruct replay_opts *opts)\n\nI'm not going to point out whitespace changes that snuck in any more.\n\nI think I prefer the options struct to go in front (as in the\nmerge-recursive and diff APIs), but this is only a matter of taste.\n\n> @@ -311,15 +329,15 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n> }\n> \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> -\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n\nI think this belongs in a different patch (and likewise for its\ncounterpart below).\n\n> The current code uses a set of file-scope static variables to tell the\n> cherry-pick/ revert machinery how to replay the changes, and\n> initializes them by parsing the command-line arguments. In later\n> steps in this series, we would like to introduce an API function that\n> calls into this machinery directly and have a way to tell it what to\n> do. Hence, introduce a structure to group these variables, so that\n> the API can take them as a single \"replay_options\" parameter.\n\nStepping back, I think this is a good idea, to make the state being\npassed around a little clearer and to make it easier for callers to\nspecify what they want to happen without making up fictitious argc and\nargv. Most of what remains for this to be cooked are minor things\n(the biggest part is getting it to build with -std=c89 -pedantic if\nwanted and teaching option_parse_x to use a callback parameter).\n\nThanks.\n"},{"id":"167662","messageId":"20110511114909.GE2676@elie","threadId":"27321","inReplyTo":"1305100822-20470-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-11T11:49:09Z","receivedAt":"2011-05-11T11:49:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Reading the Git configuration, setting environment variables, parsing\n> command-line arguments, and populating the options structure should be\n> done in cmd_cherry_pick/ cmd_revert.\n\nYes, but why? :)\n\n> The job pick_commits of\n> simplified into setting up the revision walker and calling\n> do_pick_commit in a loop- later in the series, it will handle\n> failures, and serve as the starting point for continuation.\n\nENOPARSE. I assume the idea is that callers will want to decide what\nthey want pick_commits to do and specify it by filling a struct\ninstead of argc and argv. Is that it?\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n> \treturn 0;\n> }\n> \n> -static int revert_or_cherry_pick(int argc, const char **argv,\n> -\t\t\t\tstruct replay_opts *opts)\n> +static int pick_commits(struct replay_opts *opts)\n> {\n> \tstruct rev_info revs;\n> \tstruct commit *commit;\n> -\tconst char *me;\n> \tint res;\n> \n> -\tgit_config(git_default_config, NULL);\n> -\tme = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n> -\tsetenv(GIT_REFLOG_ACTION, me, 0);\n> -\tparse_args(argc, argv, opts);\n> -\n> \tif (opts->allow_ff) {\n\nI don't see why the caller sets up GIT_REFLOG_ACTION, since the caller\nis not making the commits. Is there an example where it would use\nsomething other than \"cherry-pick\" or \"revert\"?\n\nAside from clarifying that detail, this one looks good.\n"},{"id":"167660","messageId":"20110511120654.GF2676@elie","threadId":"27321","inReplyTo":"1305100822-20470-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 5/8] revert: Catch incompatible command-line options early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-11T12:06:54Z","receivedAt":"2011-05-11T12:06:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Earlier, incompatible command-line options used to be caught in\n> pick_commits after parse_args has parsed the options and populated the\n> options structure; a lot of unncessary work has already been done, and\n> significant amount of cleanup is required to die at this stage.\n> Instead, hand over this responsibility to parse_args so that the\n> program can die early.\n\nLooking at the patch, this seems like a bugfix (error messages\ncurrently say \"cherry-pick: \" when they should sometimes say\n\"revert: \") and cleanup (dealing with options incompatible with \"--ff\"\nin a loop instead of one by one) in addition to the \"check and die\nearly\" improvement you explain above.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -80,10 +80,29 @@ static int option_parse_x(const struct option *opt,\n> \treturn 0;\n> }\n> \n> +static void die_opt_incompatible(const char *me, const char *base_opt, ...)\n> +{\n> +\tconst char *this_opt;\n> +\tint this_opt_set;\n> +\tva_list ap;\n> +\n> +\tva_start(ap, base_opt);\n> +\twhile (1) {\n> +\t\tif (!(this_opt = va_arg(ap, const char *)))\n> +\t\t\tbreak;\n> +\t\tif ((this_opt_set = va_arg(ap, int)))\n> +\t\t\tdie(_(\"%s: %s cannot be used with %s\"),\n> +\t\t\t\tme, this_opt, base_opt);\n> +\t}\n> +\tva_end(ap);\n> +}\n\nWait a second --- this doesn't always die! Why is it called\ndie_opt_incompatible rather than verify_opt_compatible_or_die or\nsomething?\n\nI think I would have written the loop something like\n\n\tva_start(ap, opt1);\n\twhile ((opt2 = va_arg(ap, const char *))) {\n\t\tint set = va_arg(ap, int);\n\t\tif (set)\n\t\t\tdie(opt1 cannot be used with opt2);\n\t}\n\tva_end(ap);\n\nThanks. The refactoring into a loop is nice.\n"},{"id":"167654","messageId":"20110511124657.GG2676@elie","threadId":"27321","inReplyTo":"1305100822-20470-7-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 6/8] revert: Introduce head, todo, done files to persist state","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-11T12:47:18Z","receivedAt":"2011-05-11T12:47:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> A cherry-pick/ revert operation consists of several smaller steps.\n> Later in the series, we would like to be able to resume a failed\n> operation.\n\nWhen introducing jargon, it is hard to make the intent perfectly\nclear. I suppose what this means is:\n\n Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than\n one commit, 2010-06-02), a single invocation of \"git cherry-pick\"\n or \"git revert\" can perform picks of several individual commits. To\n allow \"git cherry-pick --abort\" to cancel and \"git cherry-pick\n --continue\" to resume the entire command, we will need to store some\n information about the state and the plan at the beginning.\n\n> Introduce a \"head\" file to make note of the HEAD when\n> the operation stated (so that the operation can be aborted), a \"todo\"\n> file to keep the list of the steps to be performed, and a \"done\" file\n> to keep a list of steps that have completed successfully. The format\n> of these files is similar to the one used by the \"rebase -i\" process.\n\ns/stated/started/ :) Makes some sense, aside from that.\n\nIt would be more conventional to use all-caps symref-like names, like\nMULTIPLE_CHERRY_PICK_ORIG_HEAD, CHERRY_PICK_TODO, and\nCHERRY_PICK_DONE, or to put these files in a subdirectory (oh, they're\nalready in a subdirectory? Why didn't you mention that? :)).\n\nBy the way, what is .git/sequencer/done used for?\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -25,6 +26,13 @@\n> * Copyright (c) 2005 Junio C Hamano\n> */\n> \n> +#define SEQ_DIR \"sequencer\"\n> +\n> +#define SEQ_PATH\tgit_path(SEQ_DIR)\n> +#define HEAD_FILE\tgit_path(SEQ_DIR \"/head\")\n> +#define TODO_FILE\tgit_path(SEQ_DIR \"/todo\")\n> +#define DONE_FILE\tgit_path(SEQ_DIR \"/done\")\n\nThese seeming constants that call a function are kind of scary.\n\n> @@ -629,21 +637,118 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n> \treturn 0;\n> }\n> \n> +static int format_todo(struct strbuf *buf, struct commit_list *list,\n> +\t\t\tstruct replay_opts *opts)\n> +{\n> +\tstruct commit_list *cur = NULL;\n> +\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n> +\tconst char *sha1 = NULL;\n> +\tconst char *action;\n> +\n> +\taction = (opts->action == REVERT ? \"revert\" : \"pick\");\n> +\tfor (cur = list; cur; cur = cur->next) {\n> +\t\tsha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n> +\t\tif (get_message(cur->item, cur->item->buffer, &msg))\n> +\t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1);\n> +\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action, sha1, msg.subject);\n\nIs this internal state or for the user? If it is internal state, I'd\nnaïvely have expected a sequence of 40-character hexadecimal lines,\nperhaps with human-readable names like \"topic~3\" for the sake of\nerror messages if git knows about them.\n\n> +static int persist_initialize(unsigned char *head)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint fd;\n> +\n> +\tif (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777)) {\n\nWhat if .git/sequencer exists and is a file? How does this interact\nwith \"[core] sharedrepository\" configuration? What happens if\n.git/sequencer contains some stale files --- if the power fails while\ngit is writing new files in .git/sequencer/, will the state be\nconfusing?\n\n> +\t\tint err = errno;\n> +\t\tstrbuf_release(&buf);\n> +\t\terror(_(\"Could not create sequencer directory '%s': %s\"),\n> +\t\t\tSEQ_PATH, strerror(err));\n> +\t\treturn -err;\n\nWhy does the caller care about which errno, and what is it going to\ndo with that information?\n\n> +\t}\n> +\n> +\tif ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {\n\nMore idiomatic in the git codebase to write:\n\n\tfd = open(...);\n\tif (fd < 0) {\n\n> +\t\tint err = errno;\n> +\t\tstrbuf_release(&buf);\n> +\t\terror(_(\"Could not open '%s' for writing: %s\"),\n> +\t\t\tHEAD_FILE, strerror(err));\n> +\t\treturn -err;\n\nAs above. Why does the caller care about errno? If backing out after\nan error, I suppose it might make sense to rmdir .git/sequencer while\nat it.\n\n> +\t}\n> +\n> +\tstrbuf_addf(&buf, \"%s\", find_unique_abbrev(head, DEFAULT_ABBREV));\n\nWhy abbreviate?\n\n> +\twrite_or_whine(fd, buf.buf, buf.len, HEAD_FILE);\n\nWhat happens and should happen on error?\n\n[...]\n> +static int persist_todo_done(int res, struct commit_list *todo_list,\n> +\t\t\tstruct commit_list *done_list, struct replay_opts *opts)\n\nThis is about recording what has been done and what remains to\nbe done? What does the res argument represent?\n\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint fd, res2;\n> +\n> +\tif (!res)\n> +\t\treturn 0;\n> +\n> +\t/* TODO file */\n> +\tif ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {\n\nWhat happens if we are interrupted in the middle of writing this?\n\n> +\t\tint err = errno;\n> +\t\tstrbuf_release(&buf);\n> +\t\terror(_(\"Could not open '%s' for writing: %s\"),\n> +\t\t\tTODO_FILE, strerror(err));\n> +\t\treturn -err;\n\nI don't think the caller should care which errno. :)\n\n[...]\n> static int pick_commits(struct replay_opts *opts)\n> {\n> +\tstruct commit_list *done_list = NULL;\n> \tstruct rev_info revs;\n> \tstruct commit *commit;\n> +\tunsigned char head[20];\n> \tint res;\n> \n> +\tif (get_sha1(\"HEAD\", head))\n> +\t\treturn error(_(\"You do not have a valid HEAD\"));\n\nWhat should happen if I try to cherry-pick onto an unborn branch? I\nhaven't checked what happens.\n\n> +\n> \tif ((res = read_and_refresh_cache(opts)) ||\n> -\t\t(res = prepare_revs(&revs, opts)))\n> +\t\t(res = prepare_revs(&revs, opts)) ||\n> +\t\t(res = persist_initialize(head)))\n> \t\treturn res;\n> \n> -\twhile ((commit = get_revision(&revs)) &&\n> -\t\t!(res = do_pick_commit(commit, opts)))\n> -\t\t;\n> -\n> -\treturn res;\n> +\twhile ((commit = get_revision(&revs))) {\n> +\t\tif (!(res = do_pick_commit(commit, opts)))\n> +\t\t\tcommit_list_insert(commit, &done_list);\n\nThis puts done_list in the reverse order that the commits were\ncherry-picked. Is that the intent?\n\n> +\t\telse {\n> +\t\t\tcommit_list_insert(commit, &revs.commits);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\treturn persist_todo_done(res, revs.commits, done_list, opts);\n\nA few potential trade-offs:\n\n - should cherry-pick record the state after every commit? This would\n be safe against stray die() calls or segfaults but requires hitting\n the filesystem which might not be wanted if doing a run of\n cherry-picks in memory (though git is far from supporting such a\n \"many cherry picks in core followed by checkout and packed\n collection of objects written to disk all at once\" optimization\n anyway).\n\n - should we use O_TRUNC or O_APPEND to modify the state in-place or\n use separate files and rename them into place? The latter is\n safer against sudden exit.\n\n - should we (perhaps optionally) fsync the state when commiting to it?\n I think no, but someone performing a rebase and running a test suite\n with the potential to crash the system between commits might appreciate\n the effort.\n"},{"id":"167661","messageId":"20110511125900.GH2676@elie","threadId":"27321","inReplyTo":"1305100822-20470-8-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 7/8] revert: Implement parsing --continue, --abort and --skip","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-11T12:59:51Z","receivedAt":"2011-05-11T12:59:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Introduce three new command-line options: --continue, --abort, and\n> --skip resembling the correspoding options in \"rebase -i\". For now,\n> just parse the options into the replay_opts structure, making sure\n> that two of them are not specified together. They will actually be\n> implemented later in the series.\n\nI'd suggest squashing this patch with the next one. If a \"git\ncherry-pick\" accepting an --abort option that does not do anything\nleaked into the wild, that would not be a good outcome.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> \topts->xopts_nr = xopts_nr;\n> \topts->xopts_alloc = xopts_alloc;\n> \n> -\tif (opts->commit_argc < 2)\n> +\t/* Check for incompatible command line arguments */\n> +\tif (opts->abort_oper || opts->skip_oper || opts->continue_oper) {\n> +\t\tchar *this_oper;\n> +\t\tif (opts->abort_oper) {\n> +\t\t\tthis_oper = \"--abort\";\n> +\t\t\tdie_opt_incompatible(me, this_oper,\n> +\t\t\t\t\t\"--skip\", opts->skip_oper,\n> +\t\t\t\t\tNULL);\n> +\t\t\tdie_opt_incompatible(me, this_oper,\n> +\t\t\t\t\t\"--continue\", opts->continue_oper,\n> +\t\t\t\t\tNULL);\n\nWhat happened to\n\n\t\t\t...(me, \"--abort\",\n\t\t\t\t\"--skip\", opts->skip,\n\t\t\t\t\"--continue\", opts->continue);\n\n? I also wonder if there should not be a function to deal with\nmutually incompatible options:\n\n\tva_start(ap, commandname);\n\twhile ((arg1 = va_arg(ap, const char *))) {\n\t\tint set = va_arg(ap, int);\n\t\tif (set)\n\t\t\tbreak;\n\t}\n\twhile ((arg2 = va_arg(ap, const char *))) {\n\t\tint set = va_arg(ap, int);\n\t\tif (set)\n\t\t\tdie(arg1 and arg2 are incompatible);\n\t}\n\tva_end(ap);\n\n> +\t\tdie_opt_incompatible(me, this_oper,\n> +\t\t\t\t\"--no-commit\", opts->no_commit,\n[...]\n\nSeems reasonable. A part of me would want to accept such options and\nonly error out if the saved state indicates that they are different\nfrom the options supplied before, so if a person has\n\n\talias applycommits = git cherry-pick --no-commit\n\nthen \"applycommits --continue\" could work without trouble, but\nthat's probably overegineering.\n"},{"id":"167664","messageId":"20110511131356.GI2676@elie","threadId":"27321","inReplyTo":"1305100822-20470-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/8] Sequencer Foundations","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-11T13:14:26Z","receivedAt":"2011-05-11T13:14:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> I've not attempted to add anything new in this series -- It merely\n> fixes all the mistakes in the previous iteration. I've tried to\n> integrate the improvements suggested by all the previous reviews.\n\nThanks! This is much more readable, probably because of the commit\nmessages. ;-)\n\n> All tests pass in all patches, and I hope no\n> stray lines have travelled b/w the patches during the rebase.\n\nSpeaking of which, some tests and documentation would be nice as icing\non the cake.\n\n> Ramkumar Ramachandra (8):\n> revert: Improve error handling by cascading errors upwards\n> revert: Make \"commit\" and \"me\" local variables\n> revert: Introduce a struct to parse command-line options into\n> revert: Separate cmdline argument handling from the functional code\n> revert: Catch incompatible command-line options early\n> revert: Introduce head, todo, done files to persist state\n> revert: Implement parsing --continue, --abort and --skip\n> revert: Implement --abort processing\n\nThe heart is patch 6/8. I have not thought about this deeply yet, but\nI wonder if it would be simpler if the behavior of \"git cherry-pick\n1..10\" looked like this:\n\n. if there is state in .git/sequencer already, error out\n. lock .git/sequencer/head with the lockfile API to prevent\n concurrent access\n. write current state, including remaining commits to cherry-pick\n. unlock .git/sequencer/head\n. cherry-pick commit #1\n. lock sequencer, check state, update state, unlock\n. cherry-pick commit #2\n ...\n\nThis way, even if cherry-picking causes git to segfault, the sequencer\nstate is in good order and we know where to pick up. More\nimportantly, massive refactoring of the merge_recursive API would not\nbe needed to keep everything in working order. An atexit and sigchain\nhandler could be added to print advice for the reader about how to\nresume, but that's just an extra hint and it's okay if it sometimes\ndoesn't happen sometimes.\n\nWhat do you think?\n\nCiao,\nJonathan\n"},{"id":"167711","messageId":"BANLkTi=zXWojMOfe9sECUu-X9euCjr4i3w@mail.gmail.com","threadId":"27321","inReplyTo":"20110511131356.GI2676@elie","subject":"Re: [PATCH 0/8] Sequencer Foundations","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2011-05-12T08:19:34Z","receivedAt":"2011-05-12T08:19:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Wed, May 11, 2011 at 3:14 PM, Jonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>\n>> Ramkumar Ramachandra (8):\n>>   revert: Improve error handling by cascading errors upwards\n>>   revert: Make \"commit\" and \"me\" local variables\n>>   revert: Introduce a struct to parse command-line options into\n>>   revert: Separate cmdline argument handling from the functional code\n>>   revert: Catch incompatible command-line options early\n>>   revert: Introduce head, todo, done files to persist state\n>>   revert: Implement parsing --continue, --abort and --skip\n>>   revert: Implement --abort processing\n\nI had no time to look at this yet but I will try do to so in the coming days.\n\n> The heart is patch 6/8.  I have not thought about this deeply yet, but\n> I wonder if it would be simpler if the behavior of \"git cherry-pick\n> 1..10\" looked like this:\n>\n> . if there is state in .git/sequencer already, error out\n> . lock .git/sequencer/head with the lockfile API to prevent\n>  concurrent access\n> . write current state, including remaining commits to cherry-pick\n> . unlock .git/sequencer/head\n> . cherry-pick commit #1\n> . lock sequencer, check state, update state, unlock\n> . cherry-pick commit #2\n>  ...\n>\n> This way, even if cherry-picking causes git to segfault, the sequencer\n> state is in good order and we know where to pick up.  More\n> importantly, massive refactoring of the merge_recursive API would not\n> be needed to keep everything in working order.  An atexit and sigchain\n> handler could be added to print advice for the reader about how to\n> resume, but that's just an extra hint and it's okay if it sometimes\n> doesn't happen sometimes.\n>\n> What do you think?\n\nI think that the risk at this point might be to overengineer things\nand to lose time, and then we will perhaps find out that we need to do\nsome refactoring of the merge_recursive API anyway.\nIf we have cherry-pick with --abort, --continue and --skip that just\nworks as well or nearly as well (because it's new) as other stuff it\nwill be already a very good thing. And with enough tests we will\nhopefully be able to build and refactor safely after that. Maybe we\nwill eventually find out that what you suggest is in fact needed even\nfor cherry-pick with --abort, --continue and --skip, but for now I\nwould prefer trying to make it work with as few changes and work as\npossible.\n\nThanks,\nChristian.\n"},{"id":"167715","messageId":"20110512084136.GD28872@elie","threadId":"27321","inReplyTo":"BANLkTi=zXWojMOfe9sECUu-X9euCjr4i3w@mail.gmail.com","subject":"Re: [PATCH 0/8] Sequencer Foundations","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-12T08:41:36Z","receivedAt":"2011-05-12T08:41:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nChristian Couder wrote:\n\n> I think that the risk at this point might be to overengineer things\n> and to lose time, and then we will perhaps find out that we need to do\n> some refactoring of the merge_recursive API anyway.\n\nI agree with the general principle... let's see if I understand the\ndetails of what you are saying.\n\n> If we have cherry-pick with --abort, --continue and --skip that just\n> works as well or nearly as well (because it's new) as other stuff it\n> will be already a very good thing.\n\nDoes \"other stuff\" mean scripts like \"git rebase\"? If I understand\ncorrectly, \"git rebase\" updates the $dotest directory before each\ncherry-pick, unlike this series which only updates $dotest after a\nfailed cherry-pick.\n\n> And with enough tests we will\n> hopefully be able to build and refactor safely after that. Maybe we\n> will eventually find out that what you suggest is in fact needed even\n> for cherry-pick with --abort, --continue and --skip, but for now I\n> would prefer trying to make it work with as few changes and work as\n> possible.\n\nI suspect that refactoring everything to use error() in place of die()\nrequires more risky changes and work than updating .git/sequencer/\nbetween cherry-picks. Of course I can easily be wrong.\n\nHoping that is clearer,\nJonathan\n"},{"id":"167725","messageId":"20110512114415.GA14724@elie","threadId":"27321","inReplyTo":"20110512084136.GD28872@elie","subject":"Re: [PATCH 0/8] Sequencer Foundations","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-12T11:44:15Z","receivedAt":"2011-05-12T11:44:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Christian Couder wrote:\n\n>> I think that the risk at this point might be to overengineer things\n>> and to lose time, and then we will perhaps find out that we need to do\n>> some refactoring of the merge_recursive API anyway.\n>\n> I agree with the general principle... let's see if I understand the\n> details of what you are saying.\n\nIt occurs to me now that you were probably talking about the\nsuggestion of using the lockfile API (i.e., the write temporary/rename\ntrick). In that case, I agree --- no need to overengineer it and\nconcurrency problems can be fixed later. Sorry for an overcomplicated\nexplanation.\n\nAnd thanks for looking out for these things.\n"},{"id":"167775","messageId":"20110513090923.GB14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110511114909.GE2676@elie","subject":"Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T09:09:26Z","receivedAt":"2011-05-13T09:09:26Z","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> \n> > Reading the Git configuration, setting environment variables, parsing\n> > command-line arguments, and populating the options structure should be\n> > done in cmd_cherry_pick/ cmd_revert.\n> \n> Yes, but why? :)\n\nHaven't I explained this sufficiently well in the next sentence?\n\n> > The job pick_commits of\n> > simplified into setting up the revision walker and calling\n> > do_pick_commit in a loop- later in the series, it will handle\n> > failures, and serve as the starting point for continuation.\n> \n> ENOPARSE. I assume the idea is that callers will want to decide what\n> they want pick_commits to do and specify it by filling a struct\n> instead of argc and argv. Is that it?\n\nSorry about the ENOPARSE. Yes, exactly.\n\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n> > \treturn 0;\n> > }\n> > \n> > -static int revert_or_cherry_pick(int argc, const char **argv,\n> > -\t\t\t\tstruct replay_opts *opts)\n> > +static int pick_commits(struct replay_opts *opts)\n> > {\n> > \tstruct rev_info revs;\n> > \tstruct commit *commit;\n> > -\tconst char *me;\n> > \tint res;\n> > \n> > -\tgit_config(git_default_config, NULL);\n> > -\tme = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n> > -\tsetenv(GIT_REFLOG_ACTION, me, 0);\n> > -\tparse_args(argc, argv, opts);\n> > -\n> > \tif (opts->allow_ff) {\n> \n> I don't see why the caller sets up GIT_REFLOG_ACTION, since the caller\n> is not making the commits. Is there an example where it would use\n> something other than \"cherry-pick\" or \"revert\"?\n\nNice catch! Yes, GIT_REFLOG_ACTION should be in pick_commits.\n\n> Aside from clarifying that detail, this one looks good.\n\nThanks for the review. This patch is probably the one with the least\nnumber of mistakes :)\n\n-- Ram\n"},{"id":"167776","messageId":"BANLkTi=8BrFXfoDwL_fXG2bXarP7d0xioA@mail.gmail.com","threadId":"27321","inReplyTo":"20110512114415.GA14724@elie","subject":"Re: [PATCH 0/8] Sequencer Foundations","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2011-05-13T09:11:34Z","receivedAt":"2011-05-13T09:11:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Thu, May 12, 2011 at 1:44 PM, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n>> Christian Couder wrote:\n>\n>>> I think that the risk at this point might be to overengineer things\n>>> and to lose time, and then we will perhaps find out that we need to do\n>>> some refactoring of the merge_recursive API anyway.\n>>\n>> I agree with the general principle... let's see if I understand the\n>> details of what you are saying.\n>\n> It occurs to me now that you were probably talking about the\n> suggestion of using the lockfile API (i.e., the write temporary/rename\n> trick).  In that case, I agree --- no need to overengineer it and\n> concurrency problems can be fixed later.  Sorry for an overcomplicated\n> explanation.\n\nYeah, it was mostly the lockfile API.\n\nAbout writing files before each cherry-pick, I am not against it, if\nit is really needed to be safe. I even suggested it in my patch series\nback in November\n(http://article.gmane.org/gmane.comp.version-control.git/162183).\nBut it will make cherry-pick less efficient, so it is a kind of\nperformance regression that we can perhaps avoid by changing some\ndie() into error().\n\nSo what i suggest and in fact started is to just try that. We may find\nthat we could indeed do it quite safely or we may find that it's too\nmuch work to be safe enough. When I tried it, it seemed to me that it\nwas not a lot of work, and not very complex, though it added many\ncommits to the patch series. But perhaps I overlooked some problems. I\nwill have another look soon.\n\nThanks,\nChristian.\n"},{"id":"167777","messageId":"20110513091619.GC14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110511125900.GH2676@elie","subject":"Re: [PATCH 7/8] revert: Implement parsing --continue, --abort and --skip","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T09:16:22Z","receivedAt":"2011-05-13T09:16:22Z","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> \n> > Introduce three new command-line options: --continue, --abort, and\n> > --skip resembling the correspoding options in \"rebase -i\". For now,\n> > just parse the options into the replay_opts structure, making sure\n> > that two of them are not specified together. They will actually be\n> > implemented later in the series.\n> \n> I'd suggest squashing this patch with the next one. If a \"git\n> cherry-pick\" accepting an --abort option that does not do anything\n> leaked into the wild, that would not be a good outcome.\n\nWhat about --continue and --skip? They're no-ops too here, and\nthere'll soon be patches adding the functionality. Do you think it's\nalright to parse and exit immediately?\n\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> > \topts->xopts_nr = xopts_nr;\n> > \topts->xopts_alloc = xopts_alloc;\n> > \n> > -\tif (opts->commit_argc < 2)\n> > +\t/* Check for incompatible command line arguments */\n> > +\tif (opts->abort_oper || opts->skip_oper || opts->continue_oper) {\n> > +\t\tchar *this_oper;\n> > +\t\tif (opts->abort_oper) {\n> > +\t\t\tthis_oper = \"--abort\";\n> > +\t\t\tdie_opt_incompatible(me, this_oper,\n> > +\t\t\t\t\t\"--skip\", opts->skip_oper,\n> > +\t\t\t\t\tNULL);\n> > +\t\t\tdie_opt_incompatible(me, this_oper,\n> > +\t\t\t\t\t\"--continue\", opts->continue_oper,\n> > +\t\t\t\t\tNULL);\n> \n> What happened to\n> \n> \t\t\t...(me, \"--abort\",\n> \t\t\t\t\"--skip\", opts->skip,\n> \t\t\t\t\"--continue\", opts->continue);\n\nHuh? Why? I've caught every possible combination of two of those\noptions -- that already covers all three.\n\n> ? I also wonder if there should not be a function to deal with\n> mutually incompatible options:\n> \n> \tva_start(ap, commandname);\n> \twhile ((arg1 = va_arg(ap, const char *))) {\n> \t\tint set = va_arg(ap, int);\n> \t\tif (set)\n> \t\t\tbreak;\n> \t}\n> \twhile ((arg2 = va_arg(ap, const char *))) {\n> \t\tint set = va_arg(ap, int);\n> \t\tif (set)\n> \t\t\tdie(arg1 and arg2 are incompatible);\n> \t}\n> \tva_end(ap);\n\nI personally think having a function is cleaner: I even like the new\nAPI suggested by Junio. We can probably even move it to a common\nplace, and have others use it as well.\n\n> > +\t\tdie_opt_incompatible(me, this_oper,\n> > +\t\t\t\t\"--no-commit\", opts->no_commit,\n> [...]\n> \n> Seems reasonable. A part of me would want to accept such options and\n> only error out if the saved state indicates that they are different\n> from the options supplied before, so if a person has\n> \n> \talias applycommits = git cherry-pick --no-commit\n> \n> then \"applycommits --continue\" could work without trouble, but\n> that's probably overegineering.\n\nOver-engineering definitely! I'm looking to get something working\nfirst; add-on functionality like this can come as later patches.\n\nAnd yes, as you pointed out in another review, the name\nverify_opt_incompatible_or_die is more appropriate.\n\nThanks for the detailed review.\n\n-- Ram\n"},{"id":"167778","messageId":"20110513093253.GD14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110511112438.GD2676@elie","subject":"Re: [PATCH 3/8] revert: Introduce a struct to parse command-line options into","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T09:32:56Z","receivedAt":"2011-05-13T09:32:56Z","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> > I get the following warning from GCC: warning: useless storage class\n> > specifier in empty declaration (at the line where I've declared the\n> > replay_opts struct). What is the correct way to fix this?\n> \n> Remove the useless storage class specifier (\"static\"). :)\n\nAh, thanks :)\n\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -35,29 +35,42 @@ static const char * const cherry_pick_usage[] = {\n> [...]\n> > +static struct replay_opts {\n> > +\tenum { REVERT, CHERRY_PICK } action;\n> > +\n> > +\t/* Boolean options */\n> > +\tint edit;\n> > +\tint no_replay;\n> \n> replay but no replay?\n> \n> I think originally git-revert.sh had a \"replay\" variable meaning \"This\n> is not a revert (which undoes a commit) but a cherry-pick (which\n> re-does it).\" Later the purpose changed to \"We are not cherry-picking\n> and referring to the original with cherry-pick -x but replaying a\n> commit and treating it as new\".\n> \n> Now with struct replay_opts you are proposing to make the term mean\n> \"we are using git revert machinery, or in other words replaying the\n> change an old commit made (forwards or backwards)\", which makes sense.\n> In this case there should probably be a patch right before which\n> renames no_replay to i_really_want_to_expose_my_private_commit_object_name\n> (um, I mean to record_origin or something similar).\n\nGreat suggestion: one more patch changing \"no_replay\" to\n\"record_origin\" it is.\n\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> > -static const char * const *revert_or_cherry_pick_usage(void)\n> > +static const char *const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n> \n> Line is getting long. Whitespace change snuck in?\n\nIn my defense, I thought whitespace (indentation, style) changes were\npermitted as long as I'm making a functional change. If this isn't\nthe case, when can I correct the style/ indentation?\n\n> I suppose if I ran the world the argument would be of type \"enum\n> replay_action\", so it would be used as\n> \n> \tusage(revert_or_cherry_pick_usage(o->action));\n> \n> > +/* For option_parse_x */\n> > +static const char **xopts;\n> > +static size_t xopts_nr, xopts_alloc;\n> > +\n> \n> Hm. In C89, struct initializers are not allowed to include addresses\n> that are not known until run-time, and we used to follow that and now\n> violate it all over the place. I'm not sure if it's worth it or not.\n> (I'm tempted to say, let it deteriorate further and people with the\n> ability to test on such platforms can fix it, but commits like\n> v1.7.2-rc0~32^2~18, Rewrite dynamic structure initializations to\n> runtime assignment, 2010-05-14, suggest that some people have cared in\n> the recent future.)\n> \n> So.\n> \n> If you want to use parse_options and support such compilers, it is\n> indeed simplest to use static variables. You can give them scope\n> local to a particular function to at least avoid namespace polution.\n> \n> To avoid such static variables at the expense of support for old\n> compilers, one can pass a pointer to a struct to option_parse_x\n> instead of the dummy &xopts. Within option_parse_x, what you pass\n> will be accessible as opt->value. It's all explained in\n> Documentation/technical/api-parse-options.txt, or one can grep around\n> for OPT_CALLBACK for examples. A simple variant on this will work\n> with the old compilers, too.\n\nGot it.\n\n> [...]\n> > static int option_parse_x(const struct option *opt,\n> > -\t\t\t const char *arg, int unset)\n> > +\t\t\tconst char *arg, int unset)\n> \n> Whitespace change snuck in.\n\nIntended. It changes indentation style to linux-tabs-only, which is\nthe style my editor currently works with.\n\n> [...]\n> > @@ -67,19 +80,18 @@ static int option_parse_x(const struct option *opt,\n> > \treturn 0;\n> > }\n> > \n> > -static void parse_args(int argc, const char **argv)\n> > +static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> > {\n> > -\tconst char * const * usage_str = revert_or_cherry_pick_usage();\n> > +\tconst char *const *usage_str = revert_or_cherry_pick_usage(opts);\n> > \tint noop;\n> > \tstruct option options[] = {\n> > -\t\tOPT_BOOLEAN('n', \"no-commit\", &no_commit, \"don't automatically commit\"),\n> > +\t\tOPT_BOOLEAN('n', \"no-commit\", &(opts->no_commit), \"don't automatically commit\"),\n> \n> The parentheses are not needed (and not idiomatic fwiw). The line is\n> getting long so I'd suggest splitting it, though that's more a matter\n> of taste.\n\nOk, I'll lose the paranthesis.\n\n> > @@ -87,23 +99,29 @@ static void parse_args(int argc, const char **argv)\n> [...]\n> > -\tif (commit_argc < 2)\n> > +\n> > +\t/* Fill in the opts struct from values set by option_parse_x */\n> > +\topts->xopts = xopts;\n> > +\topts->xopts_nr = xopts_nr;\n> > +\topts->xopts_alloc = xopts_alloc;\n> \n> Yep, something like this is needed (for all the options) if we want to\n> follow C89's option-struct-initialization rules.\n\nOuch! That's much too painful :|\nI think I'll break the rule for the moment.\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\tconst char *base_label, const char *next_label,\n> > +\t\t\tunsigned char *head, struct strbuf *msgbuf,\n> > +\t\t\tstruct replay_opts *opts)\n> \n> I'm not going to point out whitespace changes that snuck in any more.\n> \n> I think I prefer the options struct to go in front (as in the\n> merge-recursive and diff APIs), but this is only a matter of taste.\n\nIntended again, since I'm adding an argument to the list.\n\n> > @@ -311,15 +329,15 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n> > }\n> > \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> > -\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n> \n> I think this belongs in a different patch (and likewise for its\n> counterpart below).\n> \n> > The current code uses a set of file-scope static variables to tell the\n> > cherry-pick/ revert machinery how to replay the changes, and\n> > initializes them by parsing the command-line arguments. In later\n> > steps in this series, we would like to introduce an API function that\n> > calls into this machinery directly and have a way to tell it what to\n> > do. Hence, introduce a structure to group these variables, so that\n> > the API can take them as a single \"replay_options\" parameter.\n> \n> Stepping back, I think this is a good idea, to make the state being\n> passed around a little clearer and to make it easier for callers to\n> specify what they want to happen without making up fictitious argc and\n> argv. Most of what remains for this to be cooked are minor things\n> (the biggest part is getting it to build with -std=c89 -pedantic if\n> wanted and teaching option_parse_x to use a callback parameter).\n\nRight, thanks.\n\n-- Ram\n"},{"id":"167779","messageId":"20110513093501.GE14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110513090923.GB14272@ramkum.desktop.amazon.com","subject":"Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T09:35:03Z","receivedAt":"2011-05-13T09:35:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nRamkumar Ramachandra writes:\n> Jonathan Nieder writes:\n> > Ramkumar Ramachandra wrote:\n> > > --- a/builtin/revert.c\n> > > +++ b/builtin/revert.c\n> > > @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n> > > \treturn 0;\n> > > }\n> > > \n> > > -static int revert_or_cherry_pick(int argc, const char **argv,\n> > > -\t\t\t\tstruct replay_opts *opts)\n> > > +static int pick_commits(struct replay_opts *opts)\n> > > {\n> > > \tstruct rev_info revs;\n> > > \tstruct commit *commit;\n> > > -\tconst char *me;\n> > > \tint res;\n> > > \n> > > -\tgit_config(git_default_config, NULL);\n> > > -\tme = (opts->action == REVERT ? \"revert\" : \"cherry-pick\");\n> > > -\tsetenv(GIT_REFLOG_ACTION, me, 0);\n> > > -\tparse_args(argc, argv, opts);\n> > > -\n> > > \tif (opts->allow_ff) {\n> > \n> > I don't see why the caller sets up GIT_REFLOG_ACTION, since the caller\n> > is not making the commits. Is there an example where it would use\n> > something other than \"cherry-pick\" or \"revert\"?\n> \n> Nice catch! Yes, GIT_REFLOG_ACTION should be in pick_commits.\n\nEr, I mean in do_pick_commit. Right?\n\n-- Ram\n"},{"id":"167780","messageId":"20110513094038.GA30396@elie","threadId":"27321","inReplyTo":"20110513091619.GC14272@ramkum.desktop.amazon.com","subject":"Re: [PATCH 7/8] revert: Implement parsing --continue, --abort and --skip","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-13T09:40:39Z","receivedAt":"2011-05-13T09:40:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> What about --continue and --skip? They're no-ops too here, and\n> there'll soon be patches adding the functionality. Do you think it's\n> alright to parse and exit immediately?\n\nYou're right: the same considerations apply to them. If adding these\noptions before the functionality is ready makes the series easier to\nread, then I'd at least prefer to see\n\n\tif (opts->abort_oper)\n\t\tdie(\"--abort is not implemented yet\");\n\nto prevent scripts and humans from being confused. And on the other\nhand I suspect adding each option at the same time as adding the\ncorresponding functionality would be clearer anyway.\n\n> Jonathan Nieder writes:\n>> Ramkumar Ramachandra wrote:\n\n>>> --- a/builtin/revert.c\n>>> +++ b/builtin/revert.c\n>>> @@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n[...]\n>>> +\t\t\tdie_opt_incompatible(me, this_oper,\n>>> +\t\t\t\t\t\"--skip\", opts->skip_oper,\n>>> +\t\t\t\t\tNULL);\n>>> +\t\t\tdie_opt_incompatible(me, this_oper,\n>>> +\t\t\t\t\t\"--continue\", opts->continue_oper,\n>>> +\t\t\t\t\tNULL);\n>>\n>> What happened to\n>> \n>> \t\t\t...(me, \"--abort\",\n>> \t\t\t\t\"--skip\", opts->skip,\n>> \t\t\t\t\"--continue\", opts->continue);\n>\n> Huh? Why? I've caught every possible combination of two of those\n> options -- that already covers all three.\n\nSorry, that was unclear of me. What I meant to say is that one\nfunction call instead of two would suffice, like the API is\nsupposed to make possible.\n\nIn other words, nothing actually wrong here, just a possibility\nof simplification.\n\n>> ? I also wonder if there should not be a function to deal with\n>> mutually incompatible options:\n>>\n>> \tva_start(ap, commandname);\n>> \twhile ((arg1 = va_arg(ap, const char *))) {\n>> \t\tint set = va_arg(ap, int);\n>> \t\tif (set)\n>> \t\t\tbreak;\n>> \t}\n>> \twhile ((arg2 = va_arg(ap, const char *))) {\n>> \t\tint set = va_arg(ap, int);\n>> \t\tif (set)\n>> \t\t\tdie(arg1 and arg2 are incompatible);\n>> \t}\n>> \tva_end(ap);\n>\n> I personally think having a function is cleaner\n\nSorry, I was unclear again. What I meant is that there could be\ntwo functions:\n\n - one to check a single option against various options it is\n incompatible with, which you've already written\n - another to check a family of mutually incompatible options\n\nThe above was a sample implementation for the second function, but it\nhas a bug: the second \"while\" loop should have been preceded by\n\"if (!arg1) return;\".\n\n>> Seems reasonable. A part of me would want to accept such options and\n>> only error out if the saved state indicates that they are different\n[...]\n> Over-engineering definitely!\n\nYep, sorry. Was just thinking out loud.\n"},{"id":"167781","messageId":"20110513094449.GB30396@elie","threadId":"27321","inReplyTo":"20110513093501.GE14272@ramkum.desktop.amazon.com","subject":"Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-13T09:44:49Z","receivedAt":"2011-05-13T09:44:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Ramkumar Ramachandra writes:\n>>> Ramkumar Ramachandra wrote:\n\n>>>> +++ b/builtin/revert.c\n>>>> @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> -static int revert_or_cherry_pick(int argc, const char **argv,\n>>>> -\t\t\t\tstruct replay_opts *opts)\n>>>> +static int pick_commits(struct replay_opts *opts)\n>>>> {\n[...]\n>>>> -\tsetenv(GIT_REFLOG_ACTION, me, 0);\n>>>> -\tparse_args(argc, argv, opts);\n>>>> -\n>>>> \tif (opts->allow_ff) {\n[...]\n>> Nice catch! Yes, GIT_REFLOG_ACTION should be in pick_commits.\n>\n> Er, I mean in do_pick_commit. Right?\n\nIt seems somehow cleaner to set the envvar once in pick_commits,\nassuming do_pick_commit is a private function that won't be exported.\nBut either way sounds fine to me.\n"},{"id":"167783","messageId":"20110513100241.GF14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110511103704.GB2676@elie","subject":"Re: [PATCH 2/8] revert: Make \"commit\" and \"me\" local variables","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T10:02:43Z","receivedAt":"2011-05-13T10:02:43Z","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> > Currently, \"commit\" and \"me\" are global static variables. Since we\n> > want to develop the functionality to either pick/ revert individual\n> > commits atomically later in the series, make them local variables.\n> \n> I suppose the idea is that the current commit and whether we are\n> cherry-picking or reverting is not global state and should be allowed\n> to differ between threads, or that for easier debugging we would like\n> to narrow their scope.\n> \n> How does this relate to the sequencer series? Maybe the idea is that\n> they are explicit parameters in the functions that will be exposed\n> rather than that they are local variables?\n\nRight. I'll attempt to reword this in the next iteration.\n\n> >\n> > Signed-off-by: Ramkumar Ramachandra \n> > ---\n> > The variable \"me\" is nowhere as fundamental as \"commit\" -- it's\n> > simply a string derived from a more fundamental \"action\".\n> \n> That suggests to me that \"action\" should probably be made local at the\n> same time. On second thought, it looks like this commit is doing two\n> unrelated things ---\n> \n> - simplifying the state that has to be kept by computing \"me\"\n> from \"action\" on the fly\n> \n> - narrowing the scope of \"commit\" and passing it around explicitly\n> \n> and would be clearer as two separate commits.\n\nGood idea -- I'll split this up into two distinct commits in the next\niteration.\n\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> [...]\n> > @@ -51,7 +49,7 @@ static size_t xopts_nr, xopts_alloc;\n> > \n> > #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n> > \n> > -static char *get_encoding(const char *message);\n> > +static char *get_encoding(struct commit *commit, const char *message);\n> \n> If the die is converted to an assert or die(\"BUG: ...\") without\n> specifying which commit then this first parameter is not needed.\n\nAgreed. It should probably be an assertion failure, since the caller\nshould use the get_encoding calling API responsibly.\n\n> > @@ -187,7 +186,8 @@ static char *get_encoding(const char *message)\n> > \treturn NULL;\n> > }\n> > \n> > -static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n> > +static void add_message_to_msg(struct commit *commit, struct strbuf *msgbuf,\n> > +\t\t\tconst char *message)\n> \n> Perhaps the new parameter could be \"const char *fallback\" and the\n> caller call sha1_to_hex unconditionally? (Yes, it sounds like wasted\n> computation, but it might be worth the clarity.)\n\nand\n\n> > @@ -200,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n> > \tstrbuf_addstr(msgbuf, p);\n> > }\n> > \n> > -static int write_cherry_pick_head(void)\n> > +static int write_cherry_pick_head(struct commit *commit)\n> \n> Ah, it might not be wasted computation. This could take\n> commit_sha1_hex as parameter so it only needs to be computed once.\n\nOkay.\n\n> > @@ -319,6 +319,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> > \tint clean, index_fd;\n> > \tconst char **xopt;\n> > \tstatic struct lock_file index_lock;\n> > +\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n> \n> Style: I find this clearer without the parentheses (but feel free to\n> ignore).\n> \n> [...]\n> > @@ -402,6 +403,7 @@ static int do_pick_commit(void)\n> > \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n> > \tchar *defmsg = NULL;\n> > \tstruct strbuf msgbuf = STRBUF_INIT;\n> > +\tconst char *me = (action == REVERT ? \"revert\" : \"cherry-pick\");\n> > \tint res;\n> > \n> > \tif (no_commit) {\n> > @@ -458,9 +460,10 @@ static int do_pick_commit(void)\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 me, sha1_to_hex(parent->object.sha1));\n> > +\t\t\taction == REVERT ? \"revert\" : \"cherry-pick\",\n> > +\t\t\tsha1_to_hex(parent->object.sha1));\n> \n> I think one of the computations of \"me\" is left over.\n\nRight; leaked into another patch -- rebase fail :|\n\n> > @@ -562,10 +565,13 @@ static int prepare_revs(struct rev_info *revs)\n> > \treturn 0;\n> > }\n> > \n> > -static int read_and_refresh_cache(const char *me)\n> > +static int read_and_refresh_cache(void)\n> \n> Since you seem to be moving towards having fewer statics and more\n> explicit parameters, I think this part is a step backwards. Maybe it\n> should take \"action\" as a parameter instead.\n\nI'll think about this.\n\n> > @@ -583,10 +589,12 @@ static int read_and_refresh_cache(const char *me)\n> > static int revert_or_cherry_pick(int argc, const char **argv)\n> > {\n> > \tstruct rev_info revs;\n> > +\tstruct commit *commit;\n> > +\tconst char *me;\n> > \tint res;\n> > \n> > \tgit_config(git_default_config, NULL);\n> > -\tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n> > +\tme = (action == REVERT ? \"revert\" : \"cherry-pick\");\n> \n> Why?\n\nConsistency, mainly. I can't remember operator precedence, and there\nare three operators in that line. Either way, I'll lose the\nparanthesis if it's clear enough otherwise.\n\n> > \tsetenv(GIT_REFLOG_ACTION, me, 0);\n> > \tparse_args(argc, argv);\n> > \n> \n> Sorry, mostly nitpicks. Still, hope that helps.\n\nYes. Thanks.\n\n-- Ram\n"},{"id":"167784","messageId":"20110513100714.GG14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110511120654.GF2676@elie","subject":"Re: [PATCH 5/8] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T10:07:16Z","receivedAt":"2011-05-13T10:07:16Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n> \n> > Earlier, incompatible command-line options used to be caught in\n> > pick_commits after parse_args has parsed the options and populated the\n> > options structure; a lot of unncessary work has already been done, and\n> > significant amount of cleanup is required to die at this stage.\n> > Instead, hand over this responsibility to parse_args so that the\n> > program can die early.\n> \n> Looking at the patch, this seems like a bugfix (error messages\n> currently say \"cherry-pick: \" when they should sometimes say\n> \"revert: \") and cleanup (dealing with options incompatible with \"--ff\"\n> in a loop instead of one by one) in addition to the \"check and die\n> early\" improvement you explain above.\n\nOk, I'll reword.\n\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -80,10 +80,29 @@ static int option_parse_x(const struct option *opt,\n> > \treturn 0;\n> > }\n> > \n> > +static void die_opt_incompatible(const char *me, const char *base_opt, ...)\n> > +{\n> > +\tconst char *this_opt;\n> > +\tint this_opt_set;\n> > +\tva_list ap;\n> > +\n> > +\tva_start(ap, base_opt);\n> > +\twhile (1) {\n> > +\t\tif (!(this_opt = va_arg(ap, const char *)))\n> > +\t\t\tbreak;\n> > +\t\tif ((this_opt_set = va_arg(ap, int)))\n> > +\t\t\tdie(_(\"%s: %s cannot be used with %s\"),\n> > +\t\t\t\tme, this_opt, base_opt);\n> > +\t}\n> > +\tva_end(ap);\n> > +}\n> \n> Wait a second --- this doesn't always die! Why is it called\n> die_opt_incompatible rather than verify_opt_compatible_or_die or\n> something?\n> \n> I think I would have written the loop something like\n> \n> \tva_start(ap, opt1);\n> \twhile ((opt2 = va_arg(ap, const char *))) {\n> \t\tint set = va_arg(ap, int);\n> \t\tif (set)\n> \t\t\tdie(opt1 cannot be used with opt2);\n> \t}\n> \tva_end(ap);\n> \n> Thanks. The refactoring into a loop is nice.\n\nThanks. Looks like this patch is mostly right too :)\n\n-- Ram\n"},{"id":"167785","messageId":"20110513100722.GC30396@elie","threadId":"27321","inReplyTo":"20110513093253.GD14272@ramkum.desktop.amazon.com","subject":"Re: [PATCH 3/8] revert: Introduce a struct to parse command-line options into","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-13T10:07:22Z","receivedAt":"2011-05-13T10:07:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> In my defense, I thought whitespace (indentation, style) changes were\n> permitted as long as I'm making a functional change. If this isn't\n> the case, when can I correct the style/ indentation?\n\nWhat I generally try to do is to only correct style and indentation\nwhen it is making my life difficult (either in reading or writing the\ncode), through a separate commit that explains the improvement. That\nway, a person reading the diff for a functional change doesn't have to\nbe distracted by irrelevant changes.\n\nBased on the advice he gives from time to time, Junio's policy seems\nto be that trivial cosmetic cleanups should either be very compelling\nor go at the start of a series that makes other changes to the same\nsection of code.\n"},{"id":"167786","messageId":"20110513102127.GH14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110511124657.GG2676@elie","subject":"Re: [PATCH 6/8] revert: Introduce head, todo, done files to persist state","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T10:21:30Z","receivedAt":"2011-05-13T10:21:30Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n> \n> > A cherry-pick/ revert operation consists of several smaller steps.\n> > Later in the series, we would like to be able to resume a failed\n> > operation.\n> \n> When introducing jargon, it is hard to make the intent perfectly\n> clear. I suppose what this means is:\n> \n> Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than\n> one commit, 2010-06-02), a single invocation of \"git cherry-pick\"\n> or \"git revert\" can perform picks of several individual commits. To\n> allow \"git cherry-pick --abort\" to cancel and \"git cherry-pick\n> --continue\" to resume the entire command, we will need to store some\n> information about the state and the plan at the beginning.\n\nThanks for digging that up for me. I'll try to reword it in the next\niteration without introducing more jargon, if that's preferred.\n\n> > Introduce a \"head\" file to make note of the HEAD when\n> > the operation stated (so that the operation can be aborted), a \"todo\"\n> > file to keep the list of the steps to be performed, and a \"done\" file\n> > to keep a list of steps that have completed successfully. The format\n> > of these files is similar to the one used by the \"rebase -i\" process.\n> \n> s/stated/started/ :) Makes some sense, aside from that.\n> \n> It would be more conventional to use all-caps symref-like names, like\n> MULTIPLE_CHERRY_PICK_ORIG_HEAD, CHERRY_PICK_TODO, and\n> CHERRY_PICK_DONE, or to put these files in a subdirectory (oh, they're\n> already in a subdirectory? Why didn't you mention that? :)).\n\nI'll mention that in the commit message next time.\n\n> By the way, what is .git/sequencer/done used for?\n\nI don't know :p\nI just added it because \"rebase -i\" uses it too, although I can't find\na definite usecase for it.\n\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -25,6 +26,13 @@\n> > * Copyright (c) 2005 Junio C Hamano\n> > */\n> > \n> > +#define SEQ_DIR \"sequencer\"\n> > +\n> > +#define SEQ_PATH\tgit_path(SEQ_DIR)\n> > +#define HEAD_FILE\tgit_path(SEQ_DIR \"/head\")\n> > +#define TODO_FILE\tgit_path(SEQ_DIR \"/todo\")\n> > +#define DONE_FILE\tgit_path(SEQ_DIR \"/done\")\n> \n> These seeming constants that call a function are kind of scary.\n\nUh, you'd prefer seeing it literally spelt out over and over again?\n\n> > @@ -629,21 +637,118 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n> > \treturn 0;\n> > }\n> > \n> > +static int format_todo(struct strbuf *buf, struct commit_list *list,\n> > +\t\t\tstruct replay_opts *opts)\n> > +{\n> > +\tstruct commit_list *cur = NULL;\n> > +\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n> > +\tconst char *sha1 = NULL;\n> > +\tconst char *action;\n> > +\n> > +\taction = (opts->action == REVERT ? \"revert\" : \"pick\");\n> > +\tfor (cur = list; cur; cur = cur->next) {\n> > +\t\tsha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n> > +\t\tif (get_message(cur->item, cur->item->buffer, &msg))\n> > +\t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1);\n> > +\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action, sha1, msg.subject);\n> \n> Is this internal state or for the user? If it is internal state, I'd\n> naïvely have expected a sequence of 40-character hexadecimal lines,\n> perhaps with human-readable names like \"topic~3\" for the sake of\n> error messages if git knows about them.\n\nFor the user. This is the instruction sheet that the user will be\nable to edit at a later stage, when we develop something like\n\"sequencer -i\".\n\n> > +static int persist_initialize(unsigned char *head)\n> > +{\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tint fd;\n> > +\n> > +\tif (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777)) {\n> \n> What if .git/sequencer exists and is a file? How does this interact\n> with \"[core] sharedrepository\" configuration? What happens if\n> .git/sequencer contains some stale files --- if the power fails while\n> git is writing new files in .git/sequencer/, will the state be\n> confusing?\n\nAh, thanks for pointing these out -- I hadn't thought about them earlier.\n\n> > +\t\tint err = errno;\n> > +\t\tstrbuf_release(&buf);\n> > +\t\terror(_(\"Could not create sequencer directory '%s': %s\"),\n> > +\t\t\tSEQ_PATH, strerror(err));\n> > +\t\treturn -err;\n> \n> Why does the caller care about which errno, and what is it going to\n> do with that information?\n\nHm, error_errno seems that consistently returns -1 seems like a good\nidea now. I'll get it back into the series next time.\n\n> > +\t}\n> > +\n> > +\tif ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {\n> \n> More idiomatic in the git codebase to write:\n> \n> \tfd = open(...);\n> \tif (fd < 0) {\n\nOk.\n\n> > +\t\tint err = errno;\n> > +\t\tstrbuf_release(&buf);\n> > +\t\terror(_(\"Could not open '%s' for writing: %s\"),\n> > +\t\t\tHEAD_FILE, strerror(err));\n> > +\t\treturn -err;\n> \n> As above. Why does the caller care about errno? If backing out after\n> an error, I suppose it might make sense to rmdir .git/sequencer while\n> at it.\n\nOk.\n\n> > +\t}\n> > +\n> > +\tstrbuf_addf(&buf, \"%s\", find_unique_abbrev(head, DEFAULT_ABBREV));\n> \n> Why abbreviate?\n> \n> > +\twrite_or_whine(fd, buf.buf, buf.len, HEAD_FILE);\n> \n> What happens and should happen on error?\n\n\n\n> [...]\n> > +static int persist_todo_done(int res, struct commit_list *todo_list,\n> > +\t\t\tstruct commit_list *done_list, struct replay_opts *opts)\n> \n> This is about recording what has been done and what remains to\n> be done? What does the res argument represent?\n\nExit status? I have to think back to figure out why I chose to pass it\naround like this.\n\n> > +{\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tint fd, res2;\n> > +\n> > +\tif (!res)\n> > +\t\treturn 0;\n> > +\n> > +\t/* TODO file */\n> > +\tif ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {\n> \n> What happens if we are interrupted in the middle of writing this?\n\nI'll use the lockfile API if I have time before the next iteration.\n\n> > +\t\tint err = errno;\n> > +\t\tstrbuf_release(&buf);\n> > +\t\terror(_(\"Could not open '%s' for writing: %s\"),\n> > +\t\t\tTODO_FILE, strerror(err));\n> > +\t\treturn -err;\n> \n> I don't think the caller should care which errno. :)\n\nRight :)\n\n> [...]\n> > static int pick_commits(struct replay_opts *opts)\n> > {\n> > +\tstruct commit_list *done_list = NULL;\n> > \tstruct rev_info revs;\n> > \tstruct commit *commit;\n> > +\tunsigned char head[20];\n> > \tint res;\n> > \n> > +\tif (get_sha1(\"HEAD\", head))\n> > +\t\treturn error(_(\"You do not have a valid HEAD\"));\n> \n> What should happen if I try to cherry-pick onto an unborn branch? I\n> haven't checked what happens.\n\nfatal: You do not have a valid HEAD\nI just tried it :)\n\n> > +\n> > \tif ((res = read_and_refresh_cache(opts)) ||\n> > -\t\t(res = prepare_revs(&revs, opts)))\n> > +\t\t(res = prepare_revs(&revs, opts)) ||\n> > +\t\t(res = persist_initialize(head)))\n> > \t\treturn res;\n> > \n> > -\twhile ((commit = get_revision(&revs)) &&\n> > -\t\t!(res = do_pick_commit(commit, opts)))\n> > -\t\t;\n> > -\n> > -\treturn res;\n> > +\twhile ((commit = get_revision(&revs))) {\n> > +\t\tif (!(res = do_pick_commit(commit, opts)))\n> > +\t\t\tcommit_list_insert(commit, &done_list);\n> \n> This puts done_list in the reverse order that the commits were\n> cherry-picked. Is that the intent?\n\nYes, although I'm not sure what to do with the done file now (you\npointed that out earlier).\n\n> > +\t\telse {\n> > +\t\t\tcommit_list_insert(commit, &revs.commits);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\treturn persist_todo_done(res, revs.commits, done_list, opts);\n> \n> A few potential trade-offs:\n> \n> - should cherry-pick record the state after every commit? This would\n> be safe against stray die() calls or segfaults but requires hitting\n> the filesystem which might not be wanted if doing a run of\n> cherry-picks in memory (though git is far from supporting such a\n> \"many cherry picks in core followed by checkout and packed\n> collection of objects written to disk all at once\" optimization\n> anyway).\n\nAgreed, but I don't think this kind of optimization will be in the\nscope of this series. It's nice to think about though.\n\n> - should we use O_TRUNC or O_APPEND to modify the state in-place or\n> use separate files and rename them into place? The latter is\n> safer against sudden exit.\n\nThe latter, definitely. I don't want to have to deal with malformed\ninstruction sheets.\n\n> - should we (perhaps optionally) fsync the state when commiting to it?\n> I think no, but someone performing a rebase and running a test suite\n> with the potential to crash the system between commits might appreciate\n> the effort.\n\nYes, yes! I'd love this feature.\n\nThanks.\n\n-- Ram\n"},{"id":"167787","messageId":"20110513102245.GI14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110513100722.GC30396@elie","subject":"Re: [PATCH 3/8] revert: Introduce a struct to parse command-line options into","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T10:22:47Z","receivedAt":"2011-05-13T10:22:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n> \n> > In my defense, I thought whitespace (indentation, style) changes were\n> > permitted as long as I'm making a functional change. If this isn't\n> > the case, when can I correct the style/ indentation?\n> \n> What I generally try to do is to only correct style and indentation\n> when it is making my life difficult (either in reading or writing the\n> code), through a separate commit that explains the improvement. That\n> way, a person reading the diff for a functional change doesn't have to\n> be distracted by irrelevant changes.\n> \n> Based on the advice he gives from time to time, Junio's policy seems\n> to be that trivial cosmetic cleanups should either be very compelling\n> or go at the start of a series that makes other changes to the same\n> section of code.\n\nCool. Start of the series then.\n\nThanks.\n\n-- Ram\n"},{"id":"167788","messageId":"20110513103045.GJ14272@ramkum.desktop.amazon.com","threadId":"27321","inReplyTo":"20110511095949.GA2676@elie","subject":"Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-13T10:30:47Z","receivedAt":"2011-05-13T10:30:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nAs you've pointed out, this patch is a complete disaster. Error\nhandling is very non-trivial, and my earlier attempts have failed.\nI'll try to redo this, and then respond to your review.\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n> [...]\n> *whew*\n\n-- Ram\n"},{"id":"167789","messageId":"20110513103756.GC30618@elie","threadId":"27321","inReplyTo":"BANLkTi=8BrFXfoDwL_fXG2bXarP7d0xioA@mail.gmail.com","subject":"Re: [PATCH 0/8] Sequencer Foundations","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-13T10:37:56Z","receivedAt":"2011-05-13T10:37:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Christian Couder wrote:\n\n> About writing files before each cherry-pick, I am not against it, if\n> it is really needed to be safe. I even suggested it in my patch series\n> back in November\n> (http://article.gmane.org/gmane.comp.version-control.git/162183).\n> But it will make cherry-pick less efficient, so it is a kind of\n> performance regression that we can perhaps avoid by changing some\n> die() into error().\n\nYes, that's a good point. Maybe in the long term the extra safety\ncould become optional. And I am happy about the die() elimination;\nthe only part I was not as thrilled about is relying on it.\n\nSome die() calls, like the one in xmalloc, would be very difficult to\neliminate.\n"},{"id":"167830","messageId":"alpine.LNX.2.00.1105131715240.6881@iabervon.org","threadId":"27321","inReplyTo":"1305100822-20470-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/8] revert: Make \"commit\" and \"me\" local variables","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2011-05-13T21:40:49Z","receivedAt":"2011-05-13T21:40:49Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 11 May 2011, Ramkumar Ramachandra wrote:\n\n> @@ -583,10 +589,12 @@ static int read_and_refresh_cache(const char *me)\n> static int revert_or_cherry_pick(int argc, const char **argv)\n> {\n> \tstruct rev_info revs;\n> +\tstruct commit *commit;\n> +\tconst char *me;\n> \tint res;\n> \n> \tgit_config(git_default_config, NULL);\n> -\tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n> +\tme = (action == REVERT ? \"revert\" : \"cherry-pick\");\n> \tsetenv(GIT_REFLOG_ACTION, me, 0);\n> \tparse_args(argc, argv);\n\nLater in the series, you remove everything related to \"me\" that you add \nhere, having modified it once in the middle. Just go to\n\n\tsetenv(GIT_REFLOG_ACTION, action == REVERT ? \"revert\" : \"cherry-pick\");\n\nand remove \"me\" from this function in this patch. For that matter, \nsquashing together the \"opts\" patch and this one would probably make them \nmake more sense: there's not much benefit to getting rid of some globals \nwhen other globals remain that matter for the same reason. It also fixes \nthe annoyance that each place you introduce \"me\" in this patch needs to be \nchanged later in the series.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"167959","messageId":"201105160614.48215.chriscool@tuxfamily.org","threadId":"27321","inReplyTo":"20110513103756.GC30618@elie","subject":"Re: [PATCH 0/8] Sequencer Foundations","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2011-05-16T04:14:47Z","receivedAt":"2011-05-16T04:14:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Friday 13 May 2011 12:37:56 Jonathan Nieder wrote:\n> Christian Couder wrote:\n> > About writing files before each cherry-pick, I am not against it, if\n> > it is really needed to be safe. I even suggested it in my patch series\n> > back in November\n> > (http://article.gmane.org/gmane.comp.version-control.git/162183).\n> > But it will make cherry-pick less efficient, so it is a kind of\n> > performance regression that we can perhaps avoid by changing some\n> > die() into error().\n> \n> Yes, that's a good point. Maybe in the long term the extra safety\n> could become optional. And I am happy about the die() elimination;\n> the only part I was not as thrilled about is relying on it.\n> \n> Some die() calls, like the one in xmalloc, would be very difficult to\n> eliminate.\n\nYeah, but to address this problem, maybe we can use a special die routine like \nyou already suggested. I think if we use both error() and a special die \nroutine we should be pretty safe.\n\nAnyway I looked at the patch series and I found nothing that your very good \nreview had not already spotted. My only nit is that maybe as the error \nhandling patch is growing bigger, it could be splitted in 2 or 3 patchs.\n\nThanks to you and Ram,\nChristian.\n"},{"id":"168203","messageId":"20110519103938.GA2949@domU-12-31-39-06-A8-0A.compute-1.internal","threadId":"27321","inReplyTo":"20110511095949.GA2676@elie","subject":"Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-19T10:39:39Z","receivedAt":"2011-05-19T10:39:39Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nI'm preparing a large series dedicated to solving error-handling\nissues before getting to the sequencer series. I plan to post some\nquick-and-dirty diffs of various things and ask for feedback.\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n> > @@ -418,19 +434,19 @@ static int do_pick_commit(void)\n> > struct commit_list *p;\n> >\n> > if (!mainline)\n> > - die(_(\"Commit %s is a merge but no -m option was given.\"),\n> > - sha1_to_hex(commit->object.sha1));\n> > + return error(_(\"Commit %s is a merge but no -m option was given.\"),\n> > + sha1_to_hex(commit->object.sha1));\n>\n> This is not a conflict but an error in usage. Stepping back for a\n> second, what is the best way to handle that? The aforementioned\n> hypothetical caller considering \"try an alternate strategy\" would make\n> a wrong move, so we need to use a return value that would dissuade it.\n>\n> I’m not sure what the most appropriate thing to do is. Maybe\n>\n> /*\n> * Positive numbers are exit status from conflicts; negative\n> * numbers are other errors.\n> */\n> enum pick_commit_error {\n> PICK_COMMIT_USAGE_ERROR = -1\n> };\n>\n> but it’s hard to think clearly about it since it seems too\n> hypothetical to me. If all callers are going to exit, then\n>\n> error(...);\n> return 129;\n>\n> will work, but in that case why not exit for them?\n\nFor this part, I think the correct way to handle the usage error is to\nprint a message like this:\n\n usage: cherry-pick: Commit b8bf32 is a merge but no -m option was given.\n\nAnd exit with status 129. Is this acceptable?\n\nSigned-off-by: Ramkumar Ramachandra \n\n@@ -418,20 +424,20 @@ static int do_pick_commit(void)\n struct commit_list *p;\n\n if (!mainline)\n- die(_(\"Commit %s is a merge but no -m option was given.\"),\n- sha1_to_hex(commit->object.sha1));\n+ usage(_(\"%s: Commit %s is a merge but no -m option was given.\"),\n+ me, sha1_to_hex(commit->object.sha1));\n\n for (cnt = 1, p = commit->parents;\n cnt != mainline && p;\n cnt++)\n p = p->next;\n if (cnt != mainline || !p)\n- die(_(\"Commit %s does not have parent %d\"),\n- sha1_to_hex(commit->object.sha1), mainline);\n+ usage(_(\"%s: Commit %s does not have parent %d\"),\n+ me, sha1_to_hex(commit->object.sha1), mainline);\n parent = p->item;\n- } else if (0 < mainline)\n- die(_(\"Mainline was specified but commit %s is not a merge.\"),\n- sha1_to_hex(commit->object.sha1));\n+ } else if (mainline > 0)\n+ usage(_(\"%s: Mainline was specified but commit %s is not a merge.\"),\n+ me, sha1_to_hex(commit->object.sha1));\n"},{"id":"168241","messageId":"20110519180314.GA26248@elie","threadId":"27321","inReplyTo":"20110519091831.GA28723@ramkum.desktop.amazon.com","subject":"Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-19T18:03:47Z","receivedAt":"2011-05-19T18:03:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> I'm preparing a large series dedicated to solving error-handling\n> issues before getting to the sequencer series. I plan to post some\n> quick-and-dirty diffs of various things and ask for feedback.\n\nSounds good. I wonder if there's a good way to test this (maybe I'll\ntry making an XS module so Git.pm can take advantage of similar\nchanges).\n\n> Jonathan Nieder writes:\n>> Ramkumar Ramachandra wrote:\n\n>>> @@ -418,19 +434,19 @@ static int do_pick_commit(void)\n>>> \t\tstruct commit_list *p;\n>>> \n>>> \t\tif (!mainline)\n>>> -\t\t\tdie(_(\"Commit %s is a merge but no -m option was given.\"),\n>>> -\t\t\t sha1_to_hex(commit->object.sha1));\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>> This is not a conflict but an error in usage.\n[...]\n> For this part, I think the correct way to handle the usage error is to\n> print a message like this:\n>\n> usage: cherry-pick: Commit b8bf32 is a merge but no -m option was given.\n>\n> And exit with status 129. Is this acceptable?\n\nOn second thought, status 128 seems appropriate --- the caller made a\nmistake, but it's more analagous to merging with a dirty index than\n(i.e., the command was not able to be fulfilled as desired) than to\nmisspelling a command-line option (i.e., broken script). I suppose\ntreating it as an error (as in your \"return error\") would work, and\nthe caller can transform -1 to exit(128).\n\nSorry for the thinko.\n"},{"id":"168298","messageId":"BANLkTimJ-BSBiyKd1dAcUEUmfLGVjdioNQ@mail.gmail.com","threadId":"27321","inReplyTo":"20110519180314.GA26248@elie","subject":"Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-20T06:39:55Z","receivedAt":"2011-05-20T06:39:55Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nI'm a little confused, so I'm also including a commit message\njustifying the change.\nHave I understood the issue correctly? Does this diff look alright?\n\nNote: I've removed the die from get_message in another unrelated\npatch; essentially, do_pick_commit never calls die.\n\n Since do_pick_commit is only delegated the job of picking a single\n commit in an entire cherry-pick or revert operation, don't die in this\n function. Instead, return an error to be handled by the caller\n appropriately.\n\nSigned-off-by: Ramkumar Ramachandra \n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0cc3b6b..138485f 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -415,20 +415,20 @@ static int do_pick_commit(void)\n \t\tstruct commit_list *p;\n\n \t\tif (!mainline)\n-\t\t\tdie(_(\"Commit %s is a merge but no -m option was given.\"),\n-\t\t\t sha1_to_hex(commit->object.sha1));\n+\t\t\treturn error(_(\"%s: Commit %s is a merge but no -m option was given.\"),\n+\t\t\t\tme, sha1_to_hex(commit->object.sha1));\n\n \t\tfor (cnt = 1, p = commit->parents;\n \t\t cnt != mainline && p;\n \t\t cnt++)\n \t\t\tp = p->next;\n \t\tif (cnt != mainline || !p)\n-\t\t\tdie(_(\"Commit %s does not have parent %d\"),\n-\t\t\t sha1_to_hex(commit->object.sha1), mainline);\n+\t\t\treturn error(_(\"%s: Commit %s does not have parent %d\"),\n+\t\t\t\tme, sha1_to_hex(commit->object.sha1), mainline);\n \t\tparent = p->item;\n \t} else if (0 < mainline)\n-\t\tdie(_(\"Mainline was specified but commit %s is not a merge.\"),\n-\t\t sha1_to_hex(commit->object.sha1));\n+\t\treturn error(_(\"%s: Mainline was specified but commit %s is not a merge.\"),\n+\t\t\tme, sha1_to_hex(commit->object.sha1));\n \telse\n \t\tparent = commit->parents->item;\n\n@@ -438,8 +438,8 @@ static int do_pick_commit(void)\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\tdie(_(\"%s: cannot parse parent commit %s\"),\n-\t\t me, sha1_to_hex(parent->object.sha1));\n+\t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n+\t\t\tme, sha1_to_hex(parent->object.sha1));\n\n \t/*\n \t * \"commit\" is an existing commit. We would want to apply\n@@ -578,8 +578,8 @@ static int revert_or_cherry_pick(int argc, const\nchar **argv)\n\n \twhile ((commit = get_revision(&revs))) {\n \t\tint res = do_pick_commit();\n-\t\tif (res)\n-\t\t\treturn res;\n+\t\tif (res < 0)\n+\t\t\texit(128);\n \t}\n\n \treturn 0;\n"}]}