{"thread":{"id":"27050","subject":"[RFC PATCH 00/11] Sequencer Foundations","startedAt":"2011-04-10T15:11:46Z","lastAt":"2011-05-08T12:18:40Z","messageCount":36,"participants":["Ramkumar Ramachandra","Jonathan Nieder","Daniel Barkalow","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"165548","messageId":"1302448317-32387-1-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":null,"subject":"[RFC PATCH 00/11] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:46Z","receivedAt":"2011-04-10T15:11:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nI've started working on building a sequencer for Git. While the\noutline is described in [1], I'd like some additional clarifications.\nA big thanks to Christian's series [2] for the valuable roadmap.\n\nPlease note that 10/11 is not related to this series, but seems to be\na minor nit that's required to make all existing tests pass.\n\n0. Is the general flow alright?\n\n1. Is it okay to use this kind of adaptive error handling (calling\n'die' in some places and returning error in other places), or should\nit be more uniform?\n\n2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly\ninelegant, mainly because of the command-line argument parsing\noverhead. Re-implementing it using more low-level functions doesn't\nseem to be the way to go either: for example, 'reset --hard' has some\nadditional logic of writing HEAD and ORIG_HEAD, which I don't want to\nduplicate. Should I work on reworking parts of 'rerere.c' and\n'revert.c', or is there some other way?\n\n3. From the format of the TODO and DONE files, one more thing should\nbe clear- I'm trying to stick to a slight variation of the 'rebase -i'\nformat. This part will go into the sequencer. Then I'll use a\ncherry-pick specific file to keep the command-line options. Yes, I'm\ntrying to work on Daniel's idea [3] from the very start. Is this a\ngood idea?\n\n4. I have a feeling that I've broken translation strings. Is there a\nREADME, plus a bunch of tests I can run to make sure that I've not\nbroken anything?\n\nThanks for reading.\n\n-- Ram\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/162183\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834\n\nRamkumar Ramachandra (11):\n revert: Avoid calling die; return error instead\n revert: Lose global variables \"commit\" and \"me\"\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: Implement parsing --continue, --abort and --skip\n revert: Handle conflict resolutions more elegantly\n usage: Introduce error_errno correspoding to die_errno\n revert: Write head, todo, done files\n revert: Give noop a default value while argument parsing\n revert: Implement --abort processing\n\n advice.c | 14 ++\n advice.h | 1 +\n builtin/revert.c | 454 ++++++++++++++++++++++++++++++++++++++---------------\n git-compat-util.h | 2 +\n usage.c | 34 ++++\n 5 files changed, 381 insertions(+), 124 deletions(-)\n\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165549","messageId":"1302448317-32387-2-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 01/11] revert: Avoid calling die; return error instead","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:47Z","receivedAt":"2011-04-10T15:11:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 71 ++++++++++++++++++++++++++++-------------------------\n 1 files changed, 37 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 2bb13eb..017b1af 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -265,23 +265,23 @@ static struct tree *empty_tree(void)\n \treturn tree;\n }\n \n-static NORETURN void die_dirty_index(const char *me)\n+static int error_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\treturn error(_(\"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\treturn error(_(\"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\t\treturn error(_(\"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\t\treturn error(_(\"Your local changes would be overwritten by cherry-pick.\\n\"));\n \t\t}\n \t}\n }\n@@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \t (write_cache(index_fd, active_cache, active_nr) ||\n \t commit_locked_index(&index_lock)))\n \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\tdie(_(\"%s: Unable to write new index file\"), me);\n+\t\treturn error(_(\"%s: Unable to write new index file\"), me);\n \trollback_lock_file(&index_lock);\n \n \tif (!clean) {\n@@ -397,18 +397,18 @@ static int do_pick_commit(void)\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\t\treturn error(_(\"Your index file is unmerged.\"));\n \t} else {\n \t\tif (get_sha1(\"HEAD\", head))\n-\t\t\tdie (_(\"You do not have a valid HEAD\"));\n+\t\t\treturn error(_(\"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_dirty_index(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@@ -417,19 +417,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@@ -440,11 +440,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@@ -523,7 +523,7 @@ 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@@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)\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 (argc > 1) {\n+\t\tfprintf(stderr, \"usage: %s\", _(*revert_or_cherry_pick_usage()));\n+\t\treturn 129;\n+\t}\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\treturn error(_(\"%s: failed to refresh the index\"), me);\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@@ -578,17 +583,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(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.4.rc1.7.g2cf08.dirty\n"},{"id":"165559","messageId":"1302448317-32387-3-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 02/11] revert: Lose global variables \"commit\" and \"me\"","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:48Z","receivedAt":"2011-04-10T15:11:48Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 44 +++++++++++++++++++++++++-------------------\n 1 files changed, 25 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 017b1af..fdbacad 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@@ -115,7 +113,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@@ -124,7 +123,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@@ -162,7 +161,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@@ -184,7 +183,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@@ -197,7 +197,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 void write_cherry_pick_head(struct commit *commit)\n {\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -265,10 +265,10 @@ static struct tree *empty_tree(void)\n \treturn tree;\n }\n \n-static int error_dirty_index(const char *me)\n+static int error_dirty_index()\n {\n \tif (read_cache_unmerged()) {\n-\t\tdie_resolve_conflict(me);\n+\t\tdie_resolve_conflict(action == REVERT ? \"revert\" : \"cherry-pick\");\n \t} else {\n \t\tif (advice_commit_before_merge) {\n \t\t\tif (action == REVERT)\n@@ -379,7 +379,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,7 +402,7 @@ static int do_pick_commit(void)\n \t\tif (get_sha1(\"HEAD\", head))\n \t\t\treturn error(_(\"You do not have a valid HEAD\"));\n \t\tif (index_differs_from(\"HEAD\", 0))\n-\t\t\treturn error_dirty_index(me);\n+\t\t\treturn error_dirty_index();\n \t}\n \tdiscard_cache();\n \n@@ -441,9 +441,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@@ -476,14 +477,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\twrite_cherry_pick_head();\n+\t\t\twrite_cherry_pick_head(commit);\n \t}\n \n \tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n@@ -546,10 +547,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 = (cmd_opts.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@@ -565,10 +569,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@@ -583,7 +589,7 @@ 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-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165550","messageId":"1302448317-32387-4-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 03/11] revert: Introduce a struct to parse command-line options into","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:49Z","receivedAt":"2011-04-10T15:11:49Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 132 +++++++++++++++++++++++++++++------------------------\n 1 files changed, 72 insertions(+), 60 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex fdbacad..2b33220 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -35,17 +35,27 @@ 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+static struct {\n+\tenum { REVERT, CHERRY_PICK } action;\n \n-/* Merge strategy. */\n-static const char *strategy;\n-static const char **xopts;\n-static size_t xopts_nr, xopts_alloc;\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+} cmd_opts;\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -53,7 +63,7 @@ static char *get_encoding(struct commit *commit, const char *message);\n \n static const char * const *revert_or_cherry_pick_usage(void)\n {\n-\treturn action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn cmd_opts.action == REVERT ? revert_usage : cherry_pick_usage;\n }\n \n static int option_parse_x(const struct option *opt,\n@@ -62,8 +72,8 @@ static int option_parse_x(const struct option *opt,\n \tif (unset)\n \t\treturn 0;\n \n-\tALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);\n-\txopts[xopts_nr++] = xstrdup(arg);\n+\tALLOC_GROW(cmd_opts.xopts, cmd_opts.xopts_nr + 1, cmd_opts.xopts_alloc);\n+\tcmd_opts.xopts[cmd_opts.xopts_nr++] = xstrdup(arg);\n \treturn 0;\n }\n \n@@ -72,37 +82,37 @@ static void parse_args(int argc, const char **argv)\n \tconst char * const * usage_str = revert_or_cherry_pick_usage();\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\tOPT_BOOLEAN('n', \"no-commit\", &cmd_opts.no_commit, \"don't automatically commit\"),\n+\t\tOPT_BOOLEAN('e', \"edit\", &cmd_opts.edit, \"edit the commit message\"),\n \t\tOPT_BOOLEAN('r', NULL, &noop, \"no-op (backward compatibility)\"),\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_CALLBACK('X', \"strategy-option\", &xopts, \"option\",\n+\t\tOPT_BOOLEAN('s', \"signoff\", &cmd_opts.signoff, \"add Signed-off-by:\"),\n+\t\tOPT_INTEGER('m', \"mainline\", &cmd_opts.mainline, \"parent number\"),\n+\t\tOPT_RERERE_AUTOUPDATE(&cmd_opts.allow_rerere_auto),\n+\t\tOPT_STRING(0, \"strategy\", &cmd_opts.strategy, \"strategy\", \"merge strategy\"),\n+\t\tOPT_CALLBACK('X', \"strategy-option\", &cmd_opts.xopts, \"option\",\n \t\t\t\"option for merge strategy\", option_parse_x),\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t};\n \n-\tif (action == CHERRY_PICK) {\n+\tif (cmd_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, &cmd_opts.no_replay, \"append commit name\"),\n+\t\t\tOPT_BOOLEAN(0, \"ff\", &cmd_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+\tcmd_opts.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+\tif (cmd_opts.commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n-\tcommit_argv = argv;\n+\tcmd_opts.commit_argv = argv;\n }\n \n struct commit_message {\n@@ -268,17 +278,17 @@ static struct tree *empty_tree(void)\n static int error_dirty_index()\n {\n \tif (read_cache_unmerged()) {\n-\t\tdie_resolve_conflict(action == REVERT ? \"revert\" : \"cherry-pick\");\n+\t\tdie_resolve_conflict(cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n \t} else {\n \t\tif (advice_commit_before_merge) {\n-\t\t\tif (action == REVERT)\n+\t\t\tif (cmd_opts.action == REVERT)\n \t\t\t\treturn error(_(\"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\treturn error(_(\"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\tif (cmd_opts.action == REVERT)\n \t\t\t\treturn error(_(\"Your local changes would be overwritten by revert.\\n\"));\n \t\t\telse\n \t\t\t\treturn error(_(\"Your local changes would be overwritten by cherry-pick.\\n\"));\n@@ -320,7 +330,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 = cmd_opts.xopts; xopt != cmd_opts.xopts + cmd_opts.xopts_nr; xopt++)\n \t\tparse_merge_opt(&o, *xopt);\n \n \tclean = merge_trees(&o,\n@@ -331,7 +341,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \t (write_cache(index_fd, active_cache, active_nr) ||\n \t commit_locked_index(&index_lock)))\n \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\treturn error(_(\"%s: Unable to write new index file\"), me);\n+\t\treturn error(_(\"%s: Unable to write new index file\"),\n+\t\t\tcmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n \trollback_lock_file(&index_lock);\n \n \tif (!clean) {\n@@ -368,9 +379,9 @@ static int run_git_commit(const char *defmsg)\n \n \targs[i++] = \"commit\";\n \targs[i++] = \"-n\";\n-\tif (signoff)\n+\tif (cmd_opts.signoff)\n \t\targs[i++] = \"-s\";\n-\tif (!edit) {\n+\tif (!cmd_opts.edit) {\n \t\targs[i++] = \"-F\";\n \t\targs[i++] = defmsg;\n \t}\n@@ -389,7 +400,7 @@ static int do_pick_commit(struct commit *commit)\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res;\n \n-\tif (no_commit) {\n+\tif (cmd_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@@ -407,7 +418,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 (cmd_opts.action == REVERT)\n \t\t\treturn error(_(\"Cannot revert a root commit\"));\n \t\tparent = NULL;\n \t}\n@@ -416,32 +427,32 @@ 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 (!cmd_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 != cmd_opts.mainline && p;\n \t\t cnt++)\n \t\t\tp = p->next;\n-\t\tif (cnt != mainline || !p)\n+\t\tif (cnt != cmd_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), cmd_opts.mainline);\n \t\tparent = p->item;\n-\t} else if (mainline > 0)\n+\t} else if (cmd_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 (cmd_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\t\tcmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\",\n \t\t\tsha1_to_hex(parent->object.sha1));\n \n \tif (get_message(commit, commit->buffer, &msg) != 0)\n@@ -457,7 +468,7 @@ static int do_pick_commit(struct commit *commit)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (action == REVERT) {\n+\tif (cmd_opts.action == REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -478,16 +489,16 @@ 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 (cmd_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 (!cmd_opts.no_commit)\n \t\t\twrite_cherry_pick_head(commit);\n \t}\n \n-\tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n+\tif (!cmd_opts.strategy || !strcmp(cmd_opts.strategy, \"recursive\") || cmd_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\twrite_message(&msgbuf, defmsg);\n@@ -499,22 +510,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(cmd_opts.strategy, cmd_opts.xopts_nr,\n+\t\t\t\t\tcmd_opts.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\terror(cmd_opts.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\tprint_advice();\n-\t\trerere(allow_rerere_auto);\n+\t\trerere(cmd_opts.allow_rerere_auto);\n \t} else {\n-\t\tif (!no_commit)\n+\t\tif (!cmd_opts.no_commit)\n \t\t\tres = run_git_commit(defmsg);\n \t}\n \n@@ -530,10 +542,10 @@ static int prepare_revs(struct rev_info *revs)\n \n \tinit_revisions(revs, NULL);\n \trevs->no_walk = 1;\n-\tif (action != REVERT)\n+\tif (cmd_opts.action != REVERT)\n \t\trevs->reverse = 1;\n \n-\targc = setup_revisions(commit_argc, commit_argv, revs, NULL);\n+\targc = setup_revisions(cmd_opts.commit_argc, cmd_opts.commit_argv, revs, NULL);\n \tif (argc > 1) {\n \t\tfprintf(stderr, \"usage: %s\", _(*revert_or_cherry_pick_usage()));\n \t\treturn 129;\n@@ -574,18 +586,18 @@ 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 = (cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n \tparse_args(argc, argv);\n \n-\tif (allow_ff) {\n-\t\tif (signoff)\n+\tif (cmd_opts.allow_ff) {\n+\t\tif (cmd_opts.signoff)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n-\t\tif (no_commit)\n+\t\tif (cmd_opts.no_commit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (no_replay)\n+\t\tif (cmd_opts.no_replay)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n-\t\tif (edit)\n+\t\tif (cmd_opts.edit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n \t}\n \n@@ -603,13 +615,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tif (isatty(0))\n-\t\tedit = 1;\n-\taction = REVERT;\n+\t\tcmd_opts.edit = 1;\n+\tcmd_opts.action = REVERT;\n \treturn revert_or_cherry_pick(argc, argv);\n }\n \n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n-\taction = CHERRY_PICK;\n+\tcmd_opts.action = CHERRY_PICK;\n \treturn revert_or_cherry_pick(argc, argv);\n }\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165551","messageId":"1302448317-32387-5-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 04/11] revert: Separate cmdline argument handling from the functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:50Z","receivedAt":"2011-04-10T15:11:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Based-on-patch-by: Christian Couder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 23 ++++++++++++++---------\n 1 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 2b33220..9381541 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -578,18 +578,12 @@ 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 pick_commits(void)\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 = (cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n-\tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tparse_args(argc, argv);\n-\n \tif (cmd_opts.allow_ff) {\n \t\tif (cmd_opts.signoff)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n@@ -616,12 +610,23 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tif (isatty(0))\n \t\tcmd_opts.edit = 1;\n+\n+\tgit_config(git_default_config, NULL);\n+\tmemset(&cmd_opts, 0, sizeof(cmd_opts));\n \tcmd_opts.action = REVERT;\n-\treturn revert_or_cherry_pick(argc, argv);\n+\tsetenv(GIT_REFLOG_ACTION, \"revert\", 0);\n+\tparse_args(argc, argv);\n+\n+\treturn pick_commits();\n }\n \n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n+\tgit_config(git_default_config, NULL);\n+\tmemset(&cmd_opts, 0, sizeof(cmd_opts));\n \tcmd_opts.action = CHERRY_PICK;\n-\treturn revert_or_cherry_pick(argc, argv);\n+\tsetenv(GIT_REFLOG_ACTION, \"cherry-pick\", 0);\n+\tparse_args(argc, argv);\n+\n+\treturn pick_commits();\n }\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165552","messageId":"1302448317-32387-6-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 05/11] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:51Z","receivedAt":"2011-04-10T15:11:51Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 36 +++++++++++++++++++++++++-----------\n 1 files changed, 25 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 9381541..facae24 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -77,6 +77,24 @@ 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+\t\t\t\tint nargs, int opt_bitarray[], ...)\n+{\n+\tconst char *this_opt = NULL;\n+\tva_list ap;\n+\tint i;\n+\n+\tva_start(ap, opt_bitarray);\n+\tfor (i = 0; i < nargs; i ++) {\n+\t\tthis_opt = va_arg(ap, const char *);\n+\t\tif (opt_bitarray[i]) {\n+\t\t\tdie(_(\"%s: %s cannot be used with %s\"),\n+\t\t\t\tme, this_opt, base_opt);\n+\t\t}\n+\t}\n+\tva_end(ap);\n+}\n+\n static void parse_args(int argc, const char **argv)\n {\n \tconst char * const * usage_str = revert_or_cherry_pick_usage();\n@@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)\n \tif (cmd_opts.commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n+\tif (cmd_opts.allow_ff) {\n+\t\tint opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,\n+\t\t\t\t cmd_opts.no_replay, cmd_opts.edit};\n+\t\tdie_opt_incompatible(me, \"--ff\", 4, opt_bitarray, \"--signoff\",\n+\t\t\t\t\"--no-commit\", \"-x\", \"--edit\");\n+\t}\n+\n \tcmd_opts.commit_argv = argv;\n }\n \n@@ -584,17 +609,6 @@ static int pick_commits(void)\n \tstruct commit *commit;\n \tint res;\n \n-\tif (cmd_opts.allow_ff) {\n-\t\tif (cmd_opts.signoff)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n-\t\tif (cmd_opts.no_commit)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (cmd_opts.no_replay)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n-\t\tif (cmd_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 \t\treturn res;\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165553","messageId":"1302448317-32387-7-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 06/11] revert: Implement parsing --continue, --abort and --skip","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:52Z","receivedAt":"2011-04-10T15:11:52Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 45 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex facae24..b90f3d0 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -39,6 +39,11 @@ static const char * const cherry_pick_usage[] = {\n static struct {\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@@ -98,8 +103,12 @@ static void die_opt_incompatible(const char *me, const char *base_opt,\n static void parse_args(int argc, const char **argv)\n {\n \tconst char * const * usage_str = revert_or_cherry_pick_usage();\n+\tconst char *me;\n \tint noop;\n \tstruct option options[] = {\n+\t\tOPT_BOOLEAN(0, \"abort\", &cmd_opts.abort_oper, \"abort the current operation\"),\n+\t\tOPT_BOOLEAN(0, \"skip\", &cmd_opts.skip_oper, \"skip the current commit\"),\n+\t\tOPT_BOOLEAN(0, \"continue\", &cmd_opts.continue_oper, \"continue the current operation\"),\n \t\tOPT_BOOLEAN('n', \"no-commit\", &cmd_opts.no_commit, \"don't automatically commit\"),\n \t\tOPT_BOOLEAN('e', \"edit\", &cmd_opts.edit, \"edit the commit message\"),\n \t\tOPT_BOOLEAN('r', NULL, &noop, \"no-op (backward compatibility)\"),\n@@ -127,7 +136,42 @@ static void parse_args(int argc, const char **argv)\n \tcmd_opts.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 (cmd_opts.commit_argc < 2)\n+\n+\tme = (cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n+\n+\t/* Check for incompatible command line arguments */\n+\tif (cmd_opts.abort_oper || cmd_opts.skip_oper || cmd_opts.continue_oper) {\n+\t\tchar *this_oper;\n+\t\tint opt_bitarray[] = {cmd_opts.no_commit, cmd_opts.edit, noop,\n+\t\t\t\t cmd_opts.signoff, cmd_opts.mainline,\n+\t\t\t\t (cmd_opts.strategy ? 1 : 0),\n+\t\t\t\t (cmd_opts.xopts ? 1 : 0),\n+\t\t\t\t cmd_opts.no_replay, cmd_opts.allow_ff};\n+\t\tif (cmd_opts.abort_oper) {\n+\t\t\tthis_oper = \"--abort\";\n+\t\t\tdie_opt_incompatible(me, this_oper, 1,\n+\t\t\t\t\t&cmd_opts.skip_oper, \"--skip\");\n+\t\t\tdie_opt_incompatible(me, this_oper, 1,\n+\t\t\t\t\t&cmd_opts.continue_oper, \"--continue\");\n+\t\t} else if (cmd_opts.skip_oper) {\n+\t\t\tthis_oper = \"--skip\";\n+\t\t\tdie_opt_incompatible(me, this_oper, 1,\n+\t\t\t\t\t&cmd_opts.abort_oper, \"--abort\");\n+\t\t\tdie_opt_incompatible(me, this_oper, 1,\n+\t\t\t\t\t&cmd_opts.continue_oper, \"--continue\");\n+\t\t} else {\n+\t\t\tthis_oper = \"--continue\";\n+\t\t\tdie_opt_incompatible(me, this_oper, 1,\n+\t\t\t\t\t&cmd_opts.abort_oper, \"--abort\");\n+\t\t\tdie_opt_incompatible(me, this_oper, 1,\n+\t\t\t\t\t&cmd_opts.skip_oper, \"--skip\");\n+\t\t}\n+\t\tdie_opt_incompatible(me, this_oper, 9, opt_bitarray, \"--no-commit\",\n+\t\t\t\t\"--edit\", \"-r\",\t\"--signoff\", \"--mainline\",\n+\t\t\t\t\"--strategy\", \"--strategy-option\", \"-x\", \"--ff\");\n+\t}\n+\n+\telse if (cmd_opts.commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n \tif (cmd_opts.allow_ff) {\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165554","messageId":"1302448317-32387-8-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 07/11] revert: Handle conflict resolutions more elegantly","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:53Z","receivedAt":"2011-04-10T15:11:53Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Avoid calling die; return error instead.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n advice.c | 14 ++++++++++++++\n advice.h | 1 +\n builtin/revert.c | 40 ++++++++++++++++++----------------------\n 3 files changed, 33 insertions(+), 22 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 b90f3d0..e0352d4 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -344,25 +344,20 @@ static struct tree *empty_tree(void)\n \treturn tree;\n }\n \n-static int error_dirty_index()\n+static int verify_resolution(const char *me)\n {\n-\tif (read_cache_unmerged()) {\n-\t\tdie_resolve_conflict(cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n-\t} else {\n-\t\tif (advice_commit_before_merge) {\n-\t\t\tif (cmd_opts.action == REVERT)\n-\t\t\t\treturn error(_(\"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\treturn error(_(\"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 (cmd_opts.action == REVERT)\n-\t\t\t\treturn error(_(\"Your local changes would be overwritten by revert.\\n\"));\n-\t\t\telse\n-\t\t\t\treturn error(_(\"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@@ -467,8 +462,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;\n \tint res;\n \n+\tme = (cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n \tif (cmd_opts.no_commit) {\n \t\t/*\n \t\t * We do not intend to commit immediately. We just want to\n@@ -481,8 +478,8 @@ static int do_pick_commit(struct commit *commit)\n \t} else {\n \t\tif (get_sha1(\"HEAD\", head))\n \t\t\treturn error(_(\"You do not have a valid HEAD\"));\n-\t\tif (index_differs_from(\"HEAD\", 0))\n-\t\t\treturn error_dirty_index();\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@@ -521,8 +518,7 @@ static int do_pick_commit(struct commit *commit)\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\tcmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\",\n-\t\t\tsha1_to_hex(parent->object.sha1));\n+\t\t\tme, sha1_to_hex(parent->object.sha1));\n \n \tif (get_message(commit, commit->buffer, &msg) != 0)\n \t\treturn error(_(\"Cannot get commit message for %s\"),\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165555","messageId":"1302448317-32387-9-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 08/11] usage: Introduce error_errno correspoding to die_errno","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:54Z","receivedAt":"2011-04-10T15:11:54Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n git-compat-util.h | 2 ++\n usage.c | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 0 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 40498b3..00d2815 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -240,10 +240,12 @@ extern NORETURN void usage(const char *err);\n extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n+extern void set_error_routine(void (*routine)(const char *err, va_list params));\n \n extern int prefixcmp(const char *str, const char *prefix);\n extern int suffixcmp(const char *str, const char *suffix);\ndiff --git a/usage.c b/usage.c\nindex b5e67e3..8724b41 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -46,6 +46,11 @@ void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list param\n \tdie_routine = routine;\n }\n \n+void set_error_routine(void (*routine)(const char *err, va_list params))\n+{\n+\terror_routine = routine;\n+}\n+\n void NORETURN usagef(const char *err, ...)\n {\n \tva_list params;\n@@ -97,6 +102,35 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n+int error_errno(const char *fmt, ...)\n+{\n+\tva_list params;\n+\tchar fmt_with_err[1024];\n+\tchar str_error[256], *err;\n+\tint i, j;\n+\n+\terr = strerror(errno);\n+\tfor (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {\n+\t\tif ((str_error[j++] = err[i++]) != '%')\n+\t\t\tcontinue;\n+\t\tif (j < sizeof(str_error) - 1) {\n+\t\t\tstr_error[j++] = '%';\n+\t\t} else {\n+\t\t\t/* No room to double the '%', so we overwrite it with\n+\t\t\t * '\\0' below */\n+\t\t\tj--;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tstr_error[j] = 0;\n+\tsnprintf(fmt_with_err, sizeof(fmt_with_err), \"%s: %s\", fmt, str_error);\n+\n+\tva_start(params, fmt);\n+\terror_routine(fmt_with_err, params);\n+\tva_end(params);\n+\treturn -1;\n+}\n+\n int error(const char *err, ...)\n {\n \tva_list params;\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165556","messageId":"1302448317-32387-10-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 09/11] revert: Write head, todo, done files","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:55Z","receivedAt":"2011-04-10T15:11:55Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 88 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e0352d4..25969a5 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@@ -643,21 +651,95 @@ static int read_and_refresh_cache(void)\n \treturn 0;\n }\n \n+static void format_todo(struct strbuf *buf, struct commit_list *list)\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 = (cmd_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\tdie(\"Cannot get commit message for %s\", sha1);\n+\n+\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action, sha1, msg.subject);\n+\t}\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\treturn error_errno(_(\"Could not create sequencer directory '%s'\"), SEQ_PATH);\n+\n+\tif ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0)\n+\t\treturn error_errno(_(\"Could not open '%s' for writing\"), HEAD_FILE);\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)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\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\treturn error_errno(_(\"Could not open '%s' for writing\"), TODO_FILE);\n+\n+\tformat_todo(&buf, todo_list);\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\treturn error_errno(_(\"Could not open '%s' for writing\"), DONE_FILE);\n+\n+\tformat_todo(&buf, done_list);\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(void)\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()) ||\n-\t\t(res = prepare_revs(&revs)))\n+\t\t(res = prepare_revs(&revs)) ||\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)))\n-\t\t;\n-\n-\treturn res;\n+\twhile ((commit = get_revision(&revs))) {\n+\t\tif (!(res = do_pick_commit(commit)))\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);\n }\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165557","messageId":"1302448317-32387-11-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 10/11] revert: Give noop a default value while argument parsing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:56Z","receivedAt":"2011-04-10T15:11:56Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 25969a5..0720721 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -112,7 +112,8 @@ static void parse_args(int argc, const char **argv)\n {\n \tconst char * const * usage_str = revert_or_cherry_pick_usage();\n \tconst char *me;\n-\tint noop;\n+\tint noop = 0;\n+\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"abort\", &cmd_opts.abort_oper, \"abort the current operation\"),\n \t\tOPT_BOOLEAN(0, \"skip\", &cmd_opts.skip_oper, \"skip the current commit\"),\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165558","messageId":"1302448317-32387-12-git-send-email-artagnon@gmail.com","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 11/11] revert: Implement --abort processing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-10T15:11:57Z","receivedAt":"2011-04-10T15:11:57Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 47 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0720721..c050d4f 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -743,6 +743,51 @@ static int pick_commits(void)\n \treturn persist_todo_done(res, revs.commits, done_list);\n }\n \n+static int process_args(int argc, const char **argv)\n+{\n+\tconst char *me;\n+\tint fd;\n+\n+\tparse_args(argc, argv);\n+\tme = (cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n+\tif (cmd_opts.abort_oper) {\n+\t\tconst char *args[3] = {NULL, NULL, NULL};\n+\t\tchar head[DEFAULT_ABBREV];\n+\n+\t\targs[0] = \"rerere\";\n+\t\targs[1] = \"clear\";\n+\t\tcmd_rerere(2, args, NULL);\n+\n+\t\tif (!file_exists(HEAD_FILE))\n+\t\t\tgoto error;\n+\n+\t\tif ((fd = open(HEAD_FILE, O_RDONLY, 0666)) < 0)\n+\t\t\treturn error_errno(_(\"Could not open '%s' for reading\"), HEAD_FILE);\n+\t\tif (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV)\n+\t\t\treturn error_errno(_(\"Corrupt '%s'\"), HEAD_FILE);\n+\t\tclose(fd);\n+\n+\t\targs[0] = \"reset\";\n+\t\targs[1] = \"--hard\";\n+\t\targs[2] = head;\n+\t\treturn cmd_reset(3, args, NULL);\n+\t}\n+\telse if (cmd_opts.skip_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\treturn 0;\n+\t}\n+\telse if (cmd_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();\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 \tif (isatty(0))\n@@ -752,9 +797,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tmemset(&cmd_opts, 0, sizeof(cmd_opts));\n \tcmd_opts.action = REVERT;\n \tsetenv(GIT_REFLOG_ACTION, \"revert\", 0);\n-\tparse_args(argc, argv);\n \n-\treturn pick_commits();\n+\treturn process_args(argc, argv);\n }\n \n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n@@ -763,7 +807,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tmemset(&cmd_opts, 0, sizeof(cmd_opts));\n \tcmd_opts.action = CHERRY_PICK;\n \tsetenv(GIT_REFLOG_ACTION, \"cherry-pick\", 0);\n-\tparse_args(argc, argv);\n \n-\treturn pick_commits();\n+\treturn process_args(argc, argv);\n }\n-- \n1.7.4.rc1.7.g2cf08.dirty\n"},{"id":"165564","messageId":"20110410191458.GA28163@elie","threadId":"27050","inReplyTo":"1302448317-32387-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 01/11] revert: Avoid calling die; return error instead","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-10T19:14:58Z","receivedAt":"2011-04-10T19:14:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> [Subject: revert: Avoid calling die; return error instead]\n>\n> Signed-off-by: Ramkumar Ramachandra \n\nPresumably this is because the sequencer is going to pick up after the\nerror and clean up a little (why doesn't the change description say\nso?). Will it be resuming after that or just performing a little\ncleanup before the exit?\n\nMight make sense, but:\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -265,23 +265,23 @@ static struct tree *empty_tree(void)\n> \treturn tree;\n> }\n> \n> -static NORETURN void die_dirty_index(const char *me)\n> +static int error_dirty_index(const char *me)\n> {\n> \tif (read_cache_unmerged()) {\n> \t\tdie_resolve_conflict(me);\n\nWon't that exit? \n\n> \t} else {\n\nThis \"else\" could be removed (decreasing the indent of the rest by\none tab stop) since the \"if\" case has already returned or exited.\nNot the subject of this patch, just an idea for earlier or later. ;-)\n\n[...]\n> @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> \t (write_cache(index_fd, active_cache, active_nr) ||\n> \t commit_locked_index(&index_lock)))\n> \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n> -\t\tdie(_(\"%s: Unable to write new index file\"), me);\n> +\t\treturn error(_(\"%s: Unable to write new index file\"), me);\n> \trollback_lock_file(&index_lock);\n\nWhat happens to index_lock in the error case?\n\n[...]\n> @@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)\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 (argc > 1) {\n> +\t\tfprintf(stderr, \"usage: %s\", _(*revert_or_cherry_pick_usage()));\n> +\t\treturn 129;\n> +\t}\n\nYuck. Maybe the error can be returned to the caller somehow, but\nthat seems somehow ambitious given that setup_revisions has all sorts\nof ways to die anyway.\n\nSo you are bending the assumptions of many existing git functions (in\na good way).\n\nI can think of at least three ways to go:\n\n 1) Come up with a convention to give more information about the nature\n of returned errors in the functions you are touching. For\n example, make sure errno is valid after the relevant functions, or\n use multiple negative values to express the nature of the error.\n\n So a caller could do:\n\n\tif (prepare_revs(...)) {\n\t\tif (errno == EINVAL)\n\t\t\tusage(*revert_or_cherry_pick_usage());\n\t\tdie(\"BUG: unexpected error from prepare_revs\");\n\t}\n\n Or:\n\n 2) Use set_die_routine or sigchain_push + atexit to declare what cleanup\n has to happen before exiting. Keep using die().\n\n 3) Provide a new facility to register cleanup handlers that will free\n resources and otherwise return to a consistent state before\n unwinding the stack. This way, you'd still have to audit die()\n calls to look for missing cleanup handlers, but they could stay as\n die() rather than changing to \"return error\" and the worried\n caller could use\n\n\tset_die_routine(longjmp_to_here);\n\n to keep git alive. I don't suggest doing this. It is a pain to\n get right and not obviously cleaner than \"return error\", and some\n errors really are unrecoverable (rather than just being a symptom\n of programmers to lazy to write error recovery code :)).\n\nOkay, I'm stopping here. Hope that helps.\n\nThanks,\nJonathan\n"},{"id":"165565","messageId":"20110410192154.GB28163@elie","threadId":"27050","inReplyTo":"1302448317-32387-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-10T19:21:54Z","receivedAt":"2011-04-10T19:21:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> [Subject: revert: Introduce a struct to parse command-line options into]\n>\n> Signed-off-by: Ramkumar Ramachandra \n\nThis gives a hint of another way to avoid the usage() trouble\nmentioned in patch 1: it might be possible to let the caller take care\nof parsing arguments.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -35,17 +35,27 @@ 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> +static struct {\n> +\tenum { REVERT, CHERRY_PICK } action;\n\nThat would require giving this struct a name, so it can be passed\naround. Not a bad idea anyway imho since then a person reading\ntop-to-bottom is not left in suspense:\n\n struct cherry_pick_opts {\n\tenum { REVERT, CHERRY_PICK } action;\n\n\tunsigned edit:1;\n\tunsigned no_replay:1;\n\t...\n };\n"},{"id":"165566","messageId":"alpine.LNX.2.00.1104101445460.14365@iabervon.org","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2011-04-10T19:33:17Z","receivedAt":"2011-04-10T19:33:17Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 10 Apr 2011, Ramkumar Ramachandra wrote:\n\n> Hi,\n> \n> I've started working on building a sequencer for Git. While the\n> outline is described in [1], I'd like some additional clarifications.\n> A big thanks to Christian's series [2] for the valuable roadmap.\n> \n> Please note that 10/11 is not related to this series, but seems to be\n> a minor nit that's required to make all existing tests pass.\n\nThat looks like an actual bug that only doesn't matter currently because \nthe function is never called with enough junk on the stack.\n\n> 0. Is the general flow alright?\n\nI suspect it would be easier to review some of this with certain things \nsquashed together; one patch that changes all of the variable references \nto what you want them to be is easier to understand than one that moves \nstatics to function arguments, one that moves statics to struct fields, \netc. Likewise, when you're converting some of the die() calls to error(), \nit's easier to review the patch if all of the die() calls that aren't \nchanged in that patch don't get changed later in the series.\n\n> 1. Is it okay to use this kind of adaptive error handling (calling\n> 'die' in some places and returning error in other places), or should\n> it be more uniform?\n\nI think it should be systematic but not necessarily uniform. You should be \nable to give a guideline as to how to decide which to use (and you should \nprobably actually give the guideline, so future developers make consistent \nchoices). I think of \"die\" as being ideally for situations where the \nprogram can't understand what has happened well enough to know what to do \nabout it.\n\n> 2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly\n> inelegant, mainly because of the command-line argument parsing\n> overhead. Re-implementing it using more low-level functions doesn't\n> seem to be the way to go either: for example, 'reset --hard' has some\n> additional logic of writing HEAD and ORIG_HEAD, which I don't want to\n> duplicate. Should I work on reworking parts of 'rerere.c' and\n> 'revert.c', or is there some other way?\n\n(ITYM cmd_reset here)\n\nI think rerere.c should get a rerere_clear(). I think it would make sense \nto implement the reset locally; the abort ought to be undoing exactly \nthose things that you did, and I'm not actually sure the ORIG_HEAD is \nentirely appropriate. You ought to be able to use cleanup functions that \ncorrespond to the functions you used to make the mess in the first place.\n\n> 3. From the format of the TODO and DONE files, one more thing should\n> be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> format. This part will go into the sequencer. Then I'll use a\n> cherry-pick specific file to keep the command-line options. Yes, I'm\n> trying to work on Daniel's idea [3] from the very start. Is this a\n> good idea?\n\nI certainly like it. :)\n\n> Thanks for reading.\n\nYou're welcome. :)\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"165576","messageId":"20110410194739.GC28163@elie","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-10T19:47:39Z","receivedAt":"2011-04-10T19:47:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nRamkumar Ramachandra wrote:\n\n> I've started working on building a sequencer for Git.\n\nHappy to see.\n\n> 0. Is the general flow alright?\n\nNot sure --- I don't have the big picture. Could you give a quick\nsummary of the flow in the cover letter (\"patch 1 does such-and-such,\nso patch 2 can do such-and-such, leading to...\") to the next revision,\nand quick explanations of the purpose (i.e., why we should want each\nchange) in the individual change descriptions?\n\n> 1. Is it okay to use this kind of adaptive error handling (calling\n> 'die' in some places and returning error in other places), or should\n> it be more uniform?\n\n'die' gets used in two ways (well, one way really):\n\n - to say \"there is no sane way to recover from this failure\". For\n example, xmalloc dies if even after trying to free up memory,\n malloc still could not satisfy the request.\n\n - to say \"so far we've been too lazy to implement recovery from\n this failure\". Or \"while we *could* recover from this failure, no\n one has needed it, so let's not --- that code would just bitrot.\"\n\nTherefore a mixture of 'die' and 'return error' seems inevitable. The\ndangerous mixtures to avoid are those likely to trip up callers (e.g.,\nif all code paths 'return error' except one edge case).\n\n> 2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly\n> inelegant, mainly because of the command-line argument parsing\n> overhead. Re-implementing it using more low-level functions doesn't\n> seem to be the way to go either: for example, 'reset --hard' has some\n> additional logic of writing HEAD and ORIG_HEAD, which I don't want to\n> duplicate. Should I work on reworking parts of 'rerere.c' and\n> 'revert.c', or is there some other way?\n\nSee \"git log --grep=libify\" for examples. Isn't rerere already\nlibified? Ah, you need \"rerere clear\" --- I think a rerere_clear\nfunction alongside rerere_forget et al would make sense.\n\nMore generally, it should be feasible to expose a nice, simple API for\nany functionality you need (with params struct if necessary, etc).\nThat's how many of the current APIs (revision walking, diffcore, ...)\ncame about.\n\n> 3. From the format of the TODO and DONE files, one more thing should\n> be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> format. This part will go into the sequencer. Then I'll use a\n> cherry-pick specific file to keep the command-line options. Yes, I'm\n> trying to work on Daniel's idea [3] from the very start. Is this a\n> good idea?\n\nThis is still bouncing in my head. I think I like it --- is the idea\nthat some day you could put commands like\n\n\tam topic.mbox\n\nin your insn sheet, or do nested rebases with a --force-nested option?\nThat does sound useful. How would one request \"skip to the next\noperation in the outer rebase\" on the command line? This is starting\nto feel like a debugger.\n\n> 4. I have a feeling that I've broken translation strings. Is there a\n> README, plus a bunch of tests I can run to make sure that I've not\n> broken anything?\n\nIf you put \"GETTEXT_POISON = YesPlease\" in your config.mak, the\ntranslations will be translated to gibberish when the GIT_GETTEXT_POISON\nenvvar is set, so you can use the test suite as a sanity check.\n\"make pot\" can be used to get the translation template that\ntranslators will see.\n\nAs for a readme explaining how to use _, N_, and Q_, yes, I think that\nwould be useful. I think Ævar's series has something like that (but\ntargetted at translators) later on; it might make sense to prod him or\nme for a simpler explanation can be merged immediately. Until then,\nthere is \"git log gettext.h\".\n\nRegards,\nJonathan\n"},{"id":"165593","messageId":"alpine.LNX.2.00.1104102028240.14365@iabervon.org","threadId":"27050","inReplyTo":"20110410194739.GC28163@elie","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2011-04-11T01:16:44Z","receivedAt":"2011-04-11T01:16:44Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 10 Apr 2011, Jonathan Nieder wrote:\n\n> > 3. From the format of the TODO and DONE files, one more thing should\n> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> > format. This part will go into the sequencer. Then I'll use a\n> > cherry-pick specific file to keep the command-line options. Yes, I'm\n> > trying to work on Daniel's idea [3] from the very start. Is this a\n> > good idea?\n> \n> This is still bouncing in my head. I think I like it --- is the idea\n> that some day you could put commands like\n> \n> \tam topic.mbox\n> \n> in your insn sheet, or do nested rebases with a --force-nested option?\n> That does sound useful. How would one request \"skip to the next\n> operation in the outer rebase\" on the command line? This is starting\n> to feel like a debugger.\n\nThe most basic idea is that, in the case of a rebase where a patch fails \nto apply, there are actually two levels of operation that have to be \ndeferred until after the human intervention: making the commit where the \npick failed, and applying further patches. If there are going to be \nmultiple higher-level operations (e.g., rebase and rebase -i), they should \nbe able to share the completion of the pick operation, and a basic \"git \ncherry-pick \" ought to be able to share it, too, so you can finish \nthat up without having to remember what the command is. People should be \nable to implement custom higher-level operations (e.g., \"cherry-pick all \nthe commits attached to bugs in bugzilla that block the release tracker\" \nor something), they should be able to rely on library code that knows how \nto finish the conflicted cherry-pick. (Another interesting example might \nbe \"reconstruct the linux-next tree\", which is a series of merges, and \nwhich should have --continue share the post-intervention code from the \nsingle merge process before going on to do the custom higher-level thing.)\n\nMy feeling is that \"--skip\" is actually \"abort the pick, but continue the \nrebase\". I suppose there could be more than two levels, and people could \nwant to skip a higher-level chunk, but that's something to get to when \nsomeone actually wants it; we've obviously already got the two-level \nsituation now, so we can implement that.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"165594","messageId":"201104110518.04413.chriscool@tuxfamily.org","threadId":"27050","inReplyTo":"1302448317-32387-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2011-04-11T03:18:04Z","receivedAt":"2011-04-11T03:18:04Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:\n> Hi,\n> \n> I've started working on building a sequencer for Git. \n\nSo you are starting the GSoC early! Great!\nWhen (or before) it really starts, just make sure you put your work on a \npublic Git repository and you send status updates regularly (weekly if \npossible).\n\n> 3. From the format of the TODO and DONE files, one more thing should\n> be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> format. This part will go into the sequencer. Then I'll use a\n> cherry-pick specific file to keep the command-line options. Yes, I'm\n> trying to work on Daniel's idea [3] from the very start. Is this a\n> good idea?\n\nI think that the TODO and DONE file format will need at one point to include \noptions and it is simpler if this change is done early. Using a cherry-pick \nspecific file to keep the options is not very generic for a sequencer that could \nbe used for many things.\n\nFor example, as we have rebase --interactive, we will probably want to have \ncherry-pick --interactive, and when editing the TODO file we might want to use \ndifferent cherry-pick options when picking different commits.\n\nThis would also make the different cherry-pick options available when using \nrebase --interactive once it uses the sequencer.\n\n> [1]:\n> http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908\n> [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183 [3]:\n> http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834\n\n[3] is missing here.\n\nThanks,\nChristian.\n"},{"id":"165595","messageId":"201104110524.21104.chriscool@tuxfamily.org","threadId":"27050","inReplyTo":"1302448317-32387-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 02/11] revert: Lose global variables \"commit\" and \"me\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2011-04-11T03:24:20Z","receivedAt":"2011-04-11T03:24:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sunday 10 April 2011 17:11:48 Ramkumar Ramachandra wrote:\n> @@ -546,10 +547,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 = (cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n\nIt looks like this patch will not compile as cmd_opts is introduced in patch \n03/11.\n\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,\n\nBest regards,\nChristian.\n"},{"id":"165596","messageId":"20110411044900.GA20939@kytes","threadId":"27050","inReplyTo":"201104110518.04413.chriscool@tuxfamily.org","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-11T04:49:05Z","receivedAt":"2011-04-11T04:49:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Christian,\n\nChristian Couder writes:\n> On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:\n> > Hi,\n> > \n> > I've started working on building a sequencer for Git. \n> \n> So you are starting the GSoC early! Great!\n> When (or before) it really starts, just make sure you put your work on a \n> public Git repository and you send status updates regularly (weekly if \n> possible).\n\nOfcourse. I've already discussed many of these issues last year [1].\nThe work corresponding to this particular series can be found in the\n'sequencer' branch on my GitHub fork [2]. Since the results haven't\nbeen announced, and the coding period hasn't begun, this work should\nbe treated like \"normal work\" -- I just wrote it this weekend.\n\n> > 3. From the format of the TODO and DONE files, one more thing should\n> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> > format. This part will go into the sequencer. Then I'll use a\n> > cherry-pick specific file to keep the command-line options. Yes, I'm\n> > trying to work on Daniel's idea [3] from the very start. Is this a\n> > good idea?\n> \n> I think that the TODO and DONE file format will need at one point to include \n> options and it is simpler if this change is done early. Using a cherry-pick \n> specific file to keep the options is not very generic for a sequencer that could \n> be used for many things.\n> \n> For example, as we have rebase --interactive, we will probably want to have \n> cherry-pick --interactive, and when editing the TODO file we might want to use \n> different cherry-pick options when picking different commits.\n\nPoint noted -- I shouldn't narrow down the various things I can do\nwith a single commit early on and lock us into a more restrictive\ndesign. However, I'm not in favor of making it too generic; I\ncertainly wouldn't like to edit an instruction sheet that looks like\nthis:\n\ncherry-pick -m 1 -s -r 83a4fe9\nrevert -n 3a6fe42\ncherry-pick -x --ff dacfe41\ncherry-pick -s recursive -Xpatience b31d4e2\n\nIt'll become impossible to tell which options are disallowed over what\nelse, and it'll become a nightmare to debug when something goes wrong.\nMy idea is that we add commit-specific options in an optional\nbackward-compatible manner later:\n\npick 83a4fe9\nrevert 3a6fe42 # -n\npick dacfe41 # -s\npick b31d4e2\n\nThat way, there'll be two sets of options:\n\n1. One \"global\" set of command-line switches that applies\nto all the commits, which will be written to a command-specific\nlocation. The sequencer itself knows nothing about this.\n\n2. Optional commit-specific stuff that's passed in the\nform of a (modified) commit_list to the sequencer API to write to the\ntodo/ done files.\n\nDo you like this idea?\n\n> This would also make the different cherry-pick options available when using \n> rebase --interactive once it uses the sequencer.\n> \n> > [1]:\n> > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908\n> > [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183 [3]:\n> > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834\n> \n> [3] is missing here.\n\nYour email client is perhaps wrapping too aggressively? It's fine in\nmy original email [3].\n\n-- Ram\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/142623/focus=142821\n[2]: https://github.com/artagnon/git\n[2]: http://article.gmane.org/gmane.comp.version-control.git/171255\n"},{"id":"165597","messageId":"alpine.LNX.2.00.1104110031530.14365@iabervon.org","threadId":"27050","inReplyTo":"201104110518.04413.chriscool@tuxfamily.org","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2011-04-11T05:30:20Z","receivedAt":"2011-04-11T05:30:20Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 11 Apr 2011, Christian Couder wrote:\n\n> > 3. From the format of the TODO and DONE files, one more thing should\n> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> > format. This part will go into the sequencer. Then I'll use a\n> > cherry-pick specific file to keep the command-line options. Yes, I'm\n> > trying to work on Daniel's idea [3] from the very start. Is this a\n> > good idea?\n> \n> I think that the TODO and DONE file format will need at one point to include \n> options and it is simpler if this change is done early. Using a cherry-pick \n> specific file to keep the options is not very generic for a sequencer that could \n> be used for many things.\n\nMy idea is that cherry-pick (or merge, or whatever) would have its own \nstate to store what it was trying to do when it got a conflict. This is to \nenable the cherry-pick (etc) to complete without needing to know anything \nabout what called it. The sequencer's information would keep track of \nwhat was left to do, what was already complete, and how to abort the whole \nsequence.\n\nIt would be nice to get the following commands to work:\n\n$ git cherry-pick \nAutomatic cherry-pick failed. After resolving the conflicts,\nmark the corrected paths with 'git add ' or 'git rm ' and \nrun 'git continue'\n$ (resolve conflicts)\n$ git continue\nFinished one cherry-pick.\n[master sha1b] Subject\n 2 files changes, 2 insertions(+)\n\nAnd, of course, if this happened as a step in a rebase, the rebase would \ncontinue after the cherry-pick finished. But it's annoying that, in order \nto finish a conflicted \"git cherry-pick \", you currently need to \ngo back and find the instruction that says to commit it yourself, with the \noption \"-c \" to retain authorship and message. And if you want to \nabort it, you need to remember \"git reset --hard HEAD\" (and maybe you also \nwant \"git rerere clear\"). That's what cherry-pick should be keeping its \nown state for. This is also parallel to how \"merge\" works, with its \ninformation kept in private state files, including some things like the \nlist of files that had conflicts which isn't trivial to reproduce after \nthe user has resolved them.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"165598","messageId":"20110411053819.GA6502@elie","threadId":"27050","inReplyTo":"alpine.LNX.2.00.1104110031530.14365@iabervon.org","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-11T05:38:19Z","receivedAt":"2011-04-11T05:38:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDaniel Barkalow wrote:\n\n> But it's annoying that, in order \n> to finish a conflicted \"git cherry-pick \", you currently need to \n> go back and find the instruction that says to commit it yourself, with the \n> option \"-c \" to retain authorship and message.\n\nYou might like v1.7.5-rc0~88^2 (Teach commit about CHERRY_PICK_HEAD,\n2011-02-19).\n\n> And if you want to \n> abort it, you need to remember \"git reset --hard HEAD\" (and maybe you also \n> want \"git rerere clear\").\n\nHm, I had assumed reset --hard (or \"git reset --merge HEAD\") would\ntake care of the \"rerere clear\" but it seems that indeed it doesn't.\n"},{"id":"165600","messageId":"201104110820.20342.chriscool@tuxfamily.org","threadId":"27050","inReplyTo":"20110411044900.GA20939@kytes","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2011-04-11T06:20:19Z","receivedAt":"2011-04-11T06:20:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Ram,\n\nOn Monday 11 April 2011 06:49:05 Ramkumar Ramachandra wrote:\n> Hi Christian,\n> \n> Christian Couder writes:\n> > On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:\n> > > Hi,\n> > > \n> > > I've started working on building a sequencer for Git.\n> > \n> > So you are starting the GSoC early! Great!\n> > When (or before) it really starts, just make sure you put your work on a\n> > public Git repository and you send status updates regularly (weekly if\n> > possible).\n> \n> Ofcourse. I've already discussed many of these issues last year [1].\n> The work corresponding to this particular series can be found in the\n> 'sequencer' branch on my GitHub fork [2]. \n\nGreat! Thanks!\n\n> Since the results haven't\n> been announced, and the coding period hasn't begun, this work should\n> be treated like \"normal work\" -- I just wrote it this weekend.\n\nOk.\n\n> > > 3. From the format of the TODO and DONE files, one more thing should\n> > > be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> > > format. This part will go into the sequencer. Then I'll use a\n> > > cherry-pick specific file to keep the command-line options. Yes, I'm\n> > > trying to work on Daniel's idea [3] from the very start. Is this a\n> > > good idea?\n> > \n> > I think that the TODO and DONE file format will need at one point to\n> > include options and it is simpler if this change is done early. Using a\n> > cherry-pick specific file to keep the options is not very generic for a\n> > sequencer that could be used for many things.\n> > \n> > For example, as we have rebase --interactive, we will probably want to\n> > have cherry-pick --interactive, and when editing the TODO file we might\n> > want to use different cherry-pick options when picking different\n> > commits.\n> \n> Point noted -- I shouldn't narrow down the various things I can do\n> with a single commit early on and lock us into a more restrictive\n> design. However, I'm not in favor of making it too generic; I\n> certainly wouldn't like to edit an instruction sheet that looks like\n> this:\n> \n> cherry-pick -m 1 -s -r 83a4fe9\n> revert -n 3a6fe42\n> cherry-pick -x --ff dacfe41\n> cherry-pick -s recursive -Xpatience b31d4e2\n\nI wouldn't like either, but I would like it even less if it was like this:\n\npick 83a4fe9 # -m 1 -s -r \nrevert 3a6fe42 # -n \npick dacfe41 # -x --ff \npick b31d4e2 # -s recursive -Xpatience \n\nI mean that of course people should not use too many options for no good \nreason, but if they do need to use some options, it's better if they are shown \nlike in a shell, as they will be more familiar with them this way.\n\nThere is no point of making options look different to prevent people from \nabusing them.\n\n> It'll become impossible to tell which options are disallowed over what\n> else, and it'll become a nightmare to debug when something goes wrong.\n> My idea is that we add commit-specific options in an optional\n> backward-compatible manner later:\n> \n> pick 83a4fe9\n> revert 3a6fe42 # -n\n> pick dacfe41 # -s\n> pick b31d4e2\n> \n> That way, there'll be two sets of options:\n> \n> 1. One \"global\" set of command-line switches that applies\n> to all the commits, which will be written to a command-specific\n> location. The sequencer itself knows nothing about this.\n\nI don't see the point of this global set. And if the sequencer knows nothing \nabout it, the user may not know about it too and so may not understand how \nthings work.\n\n> 2. Optional commit-specific stuff that's passed in the\n> form of a (modified) commit_list to the sequencer API to write to the\n> todo/ done files.\n> \n> Do you like this idea?\n> \n> > This would also make the different cherry-pick options available when\n> > using rebase --interactive once it uses the sequencer.\n> > \n> > > [1]:\n> > > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170\n> > > 908 [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183\n> > > [3]:\n> > > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=17\n> > > 0834\n> > \n> > [3] is missing here.\n> \n> Your email client is perhaps wrapping too aggressively? It's fine in\n> my original email [3].\n\nYeah, sorry about that!\nChristian.\n"},{"id":"165601","messageId":"alpine.LNX.2.00.1104110146550.14365@iabervon.org","threadId":"27050","inReplyTo":"20110411053819.GA6502@elie","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2011-04-11T06:34:26Z","receivedAt":"2011-04-11T06:34:26Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 11 Apr 2011, Jonathan Nieder wrote:\n\n> Hi,\n> \n> Daniel Barkalow wrote:\n> \n> > But it's annoying that, in order \n> > to finish a conflicted \"git cherry-pick \", you currently need to \n> > go back and find the instruction that says to commit it yourself, with the \n> > option \"-c \" to retain authorship and message.\n> \n> You might like v1.7.5-rc0~88^2 (Teach commit about CHERRY_PICK_HEAD,\n> 2011-02-19).\n\nAh, so I'm actually proposing that we not revert that series, as far as \nhaving cherry-pick-specific state is concerned. :) On the other hand, I'd \nlike to have the logic for using that state be in the cherry-pick \nimplementation, rather than having commit.c need to understand merge and \ncherry-pick (and revert, and applying a patch from email, and...). Which \nis to say, there should be a core state file that ends up with \n\"cherry-pick\" in it, and revert.c has registered that it handles that, and \ncommit.c should call the registered function to use the saved state. \n\nAlso, git-rebase should be able to take advantage of the fact that other \ncode knows how to start a cherry-pick, find that it has a conflict, tell \nthe user, and use saved information to finish it.\n\n> > And if you want to \n> > abort it, you need to remember \"git reset --hard HEAD\" (and maybe you also \n> > want \"git rerere clear\").\n> \n> Hm, I had assumed reset --hard (or \"git reset --merge HEAD\") would\n> take care of the \"rerere clear\" but it seems that indeed it doesn't.\n\nIn fact, Ram's series makes sure to call both, which is how I knew.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"165603","messageId":"20110411064247.GB6608@elie","threadId":"27050","inReplyTo":"alpine.LNX.2.00.1104102028240.14365@iabervon.org","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-11T06:42:47Z","receivedAt":"2011-04-11T06:42:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Daniel Barkalow wrote:\n\n> My feeling is that \"--skip\" is actually \"abort the pick, but continue the \n> rebase\". I suppose there could be more than two levels, and people could \n> want to skip a higher-level chunk, but that's something to get to when \n> someone actually wants it\n\nAh, I think I see better now. What was confusing was\n\n\tgit cherry-pick foo..bar\n\nfor which --skip would mean \"abort the pick, but continue the pick\".\nIt fits very well into your story. There are two levels: \"atomic\"\n(for lack of a better word) operations, like a single cherry-pick, and\nsequences of operations.\n\n(As for the more-than-two-levels story, I want it, but I agree with\nyou that that is tangential to this project. I find myself attempting\nto go back and rearrange earlier commits during an interactive rebase\nvery often.)\n"},{"id":"165608","messageId":"20110411085521.GB28959@kytes","threadId":"27050","inReplyTo":"alpine.LNX.2.00.1104101445460.14365@iabervon.org","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-11T08:55:23Z","receivedAt":"2011-04-11T08:55:23Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Daniel,\n\nDaniel Barkalow writes:\n> > Please note that 10/11 is not related to this series, but seems to be\n> > a minor nit that's required to make all existing tests pass.\n> \n> That looks like an actual bug that only doesn't matter currently because \n> the function is never called with enough junk on the stack.\n\nFixing this bug was one is one of the hidden motives of the patch I\nsend out just afterward [1].\n\n> > 0. Is the general flow alright?\n> \n> I suspect it would be easier to review some of this with certain things \n> squashed together; one patch that changes all of the variable references \n> to what you want them to be is easier to understand than one that moves \n> statics to function arguments, one that moves statics to struct fields, \n> etc. Likewise, when you're converting some of the die() calls to error(), \n> it's easier to review the patch if all of the die() calls that aren't \n> changed in that patch don't get changed later in the series.\n\nAgreed -- I'll post a reworked series shortly. Jonathan was finding\nit hard to follow as well.\n\n> > 1. Is it okay to use this kind of adaptive error handling (calling\n> > 'die' in some places and returning error in other places), or should\n> > it be more uniform?\n> \n> I think it should be systematic but not necessarily uniform. You should be \n> able to give a guideline as to how to decide which to use (and you should \n> probably actually give the guideline, so future developers make consistent \n> choices). I think of \"die\" as being ideally for situations where the \n> program can't understand what has happened well enough to know what to do \n> about it.\n\nJonathan concurs. Where should I document this?\n\n> > 2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly\n> > inelegant, mainly because of the command-line argument parsing\n> > overhead. Re-implementing it using more low-level functions doesn't\n> > seem to be the way to go either: for example, 'reset --hard' has some\n> > additional logic of writing HEAD and ORIG_HEAD, which I don't want to\n> > duplicate. Should I work on reworking parts of 'rerere.c' and\n> > 'revert.c', or is there some other way?\n> \n> (ITYM cmd_reset here)\n\nYeah, sorry.\n\n> I think rerere.c should get a rerere_clear(). I think it would make sense \n> to implement the reset locally; the abort ought to be undoing exactly \n> those things that you did, and I'm not actually sure the ORIG_HEAD is \n> entirely appropriate. You ought to be able to use cleanup functions that \n> correspond to the functions you used to make the mess in the first place.\n\nGood suggestion -- rerere_clear is done [2]; now I have to figure out\nif it makes sense to write a library for reset.\n\n-- Ram\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/171267\n[2]: http://article.gmane.org/gmane.comp.version-control.git/171314\n"},{"id":"165609","messageId":"20110411085717.GC28959@kytes","threadId":"27050","inReplyTo":"201104110524.21104.chriscool@tuxfamily.org","subject":"Re: [PATCH 02/11] revert: Lose global variables \"commit\" and \"me\"","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-11T08:57:19Z","receivedAt":"2011-04-11T08:57:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Christian,\n\nChristian Couder writes:\n> On Sunday 10 April 2011 17:11:48 Ramkumar Ramachandra wrote:\n> > @@ -546,10 +547,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 = (cmd_opts.action == REVERT ? \"revert\" : \"cherry-pick\");\n> \n> It looks like this patch will not compile as cmd_opts is introduced in patch \n> 03/11.\n\nRebase fail. Thanks for pointing it out. I'll add some features and\nrework the series shortly.\n\n-- Ram\n"},{"id":"165611","messageId":"20110411090747.GD28959@kytes","threadId":"27050","inReplyTo":"20110410194739.GC28163@elie","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-11T09:07:49Z","receivedAt":"2011-04-11T09:07:49Z","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> > 0. Is the general flow alright?\n> \n> Not sure --- I don't have the big picture. Could you give a quick\n> summary of the flow in the cover letter (\"patch 1 does such-and-such,\n> so patch 2 can do such-and-such, leading to...\") to the next revision,\n> and quick explanations of the purpose (i.e., why we should want each\n> change) in the individual change descriptions?\n\nAgreed. I'll ask again after the next re-roll.\n\n> > 1. Is it okay to use this kind of adaptive error handling (calling\n> > 'die' in some places and returning error in other places), or should\n> > it be more uniform?\n> \n> 'die' gets used in two ways (well, one way really):\n> \n> - to say \"there is no sane way to recover from this failure\". For\n> example, xmalloc dies if even after trying to free up memory,\n> malloc still could not satisfy the request.\n> \n> - to say \"so far we've been too lazy to implement recovery from\n> this failure\". Or \"while we *could* recover from this failure, no\n> one has needed it, so let's not --- that code would just bitrot.\"\n> \n> Therefore a mixture of 'die' and 'return error' seems inevitable. The\n> dangerous mixtures to avoid are those likely to trip up callers (e.g.,\n> if all code paths 'return error' except one edge case).\n\nMy thoughts precisely. I was worried about what would happen in\nfuture when Git is completely lib'ified, but I suppose it makes little\nsense to think about that now.\n\n> > 2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly\n> > inelegant, mainly because of the command-line argument parsing\n> > overhead. Re-implementing it using more low-level functions doesn't\n> > seem to be the way to go either: for example, 'reset --hard' has some\n> > additional logic of writing HEAD and ORIG_HEAD, which I don't want to\n> > duplicate. Should I work on reworking parts of 'rerere.c' and\n> > 'revert.c', or is there some other way?\n> \n> See \"git log --grep=libify\" for examples. Isn't rerere already\n> libified? Ah, you need \"rerere clear\" --- I think a rerere_clear\n> function alongside rerere_forget et al would make sense.\n\nDone in [1].\n\n> More generally, it should be feasible to expose a nice, simple API for\n> any functionality you need (with params struct if necessary, etc).\n> That's how many of the current APIs (revision walking, diffcore, ...)\n> came about.\n\nI see. Okay, I'll see if it makes sense to craft an API for rebase.\n\n> > 3. From the format of the TODO and DONE files, one more thing should\n> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> > format. This part will go into the sequencer. Then I'll use a\n> > cherry-pick specific file to keep the command-line options. Yes, I'm\n> > trying to work on Daniel's idea [3] from the very start. Is this a\n> > good idea?\n> \n> This is still bouncing in my head. I think I like it --- is the idea\n> that some day you could put commands like\n> \n> \tam topic.mbox\n> \n> in your insn sheet, or do nested rebases with a --force-nested option?\n> That does sound useful. How would one request \"skip to the next\n> operation in the outer rebase\" on the command line? This is starting\n> to feel like a debugger.\n\nI'll respond to this later in the thread, since Daniel has already\nsaid something. I just need help with crafting a nice instruction\nsheet now -- please join the discussion at [2].\n\n> > 4. I have a feeling that I've broken translation strings. Is there a\n> > README, plus a bunch of tests I can run to make sure that I've not\n> > broken anything?\n> \n> If you put \"GETTEXT_POISON = YesPlease\" in your config.mak, the\n> translations will be translated to gibberish when the GIT_GETTEXT_POISON\n> envvar is set, so you can use the test suite as a sanity check.\n> \"make pot\" can be used to get the translation template that\n> translators will see.\n> \n> As for a readme explaining how to use _, N_, and Q_, yes, I think that\n> would be useful. I think Ævar's series has something like that (but\n> targetted at translators) later on; it might make sense to prod him or\n> me for a simpler explanation can be merged immediately. Until then,\n> there is \"git log gettext.h\".\n\nThanks! Someone should really document this whole translations thing.\n\n-- Ram\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/171314\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/171255/focus=171307\n"},{"id":"165620","messageId":"20110411104801.GA18448@kytes","threadId":"27050","inReplyTo":"201104110820.20342.chriscool@tuxfamily.org","subject":"Re: [RFC PATCH 00/11] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-04-11T10:48:02Z","receivedAt":"2011-04-11T10:48:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Christian,\n\nChristian Couder writes:\n> On Monday 11 April 2011 06:49:05 Ramkumar Ramachandra wrote:\n> > > > 3. From the format of the TODO and DONE files, one more thing should\n> > > > be clear- I'm trying to stick to a slight variation of the 'rebase -i'\n> > > > format. This part will go into the sequencer. Then I'll use a\n> > > > cherry-pick specific file to keep the command-line options. Yes, I'm\n> > > > trying to work on Daniel's idea [3] from the very start. Is this a\n> > > > good idea?\n> > > \n> > > I think that the TODO and DONE file format will need at one point to\n> > > include options and it is simpler if this change is done early. Using a\n> > > cherry-pick specific file to keep the options is not very generic for a\n> > > sequencer that could be used for many things.\n> > > \n> > > For example, as we have rebase --interactive, we will probably want to\n> > > have cherry-pick --interactive, and when editing the TODO file we might\n> > > want to use different cherry-pick options when picking different\n> > > commits.\n> > \n> > Point noted -- I shouldn't narrow down the various things I can do\n> > with a single commit early on and lock us into a more restrictive\n> > design. However, I'm not in favor of making it too generic; I\n> > certainly wouldn't like to edit an instruction sheet that looks like\n> > this:\n> > \n> > cherry-pick -m 1 -s -r 83a4fe9\n> > revert -n 3a6fe42\n> > cherry-pick -x --ff dacfe41\n> > cherry-pick -s recursive -Xpatience b31d4e2\n> \n> I wouldn't like either, but I would like it even less if it was like this:\n> \n> pick 83a4fe9 # -m 1 -s -r \n> revert 3a6fe42 # -n \n> pick dacfe41 # -x --ff \n> pick b31d4e2 # -s recursive -Xpatience \n> \n> I mean that of course people should not use too many options for no good \n> reason, but if they do need to use some options, it's better if they are shown \n> like in a shell, as they will be more familiar with them this way.\n> \n> There is no point of making options look different to prevent people from \n> abusing them.\n\nYou're right -- no point obscuring things. I can't help thinking that\nthere must be a more elegant representation. I'll think about it for\na while, and try to come up with something.\n\n> > It'll become impossible to tell which options are disallowed over what\n> > else, and it'll become a nightmare to debug when something goes wrong.\n> > My idea is that we add commit-specific options in an optional\n> > backward-compatible manner later:\n> > \n> > pick 83a4fe9\n> > revert 3a6fe42 # -n\n> > pick dacfe41 # -s\n> > pick b31d4e2\n> > \n> > That way, there'll be two sets of options:\n> > \n> > 1. One \"global\" set of command-line switches that applies\n> > to all the commits, which will be written to a command-specific\n> > location. The sequencer itself knows nothing about this.\n> \n> I don't see the point of this global set. And if the sequencer knows nothing \n> about it, the user may not know about it too and so may not understand how \n> things work.\n\nHm. I originally wanted this so that each commit in the instruction\nsheet isn't polluted with the same command-line options, but this\ndoesn't seem to be a good solution.\n\n-- Ram\n"},{"id":"165647","messageId":"7vvcykv8j7.fsf@alter.siamese.dyndns.org","threadId":"27050","inReplyTo":"1302448317-32387-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 01/11] revert: Avoid calling die; return error instead","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-11T20:26:52Z","receivedAt":"2011-04-11T20:26:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Signed-off-by: Ramkumar Ramachandra \n\nYou would need to write a lot more than that to justify why this is a good\nchange and does not regress the existing codepaths. The above Subject:\nimplies as if all you did was to replace \"die()\" with \"return error()\",\nbut I am sure that you would also have audited all the existing callers of\nthe affected codepaths and either they already handled an error return\ncorrectly by dying or exiting with non-zero status, or you adjusted them\nto expect an error return and exit with 129 in this patch.\n\nAlso we know from the context of this post that you are planning to add\nnew callsites to some of the functions that are converted to give an error\nreturn with this patch, but it is nevertheless a good idea to briefly\nmention that (just \"the codepath to implement new nitfol feature will be\nmaking calls to xyzzy and frotz and it does not want these to die; rather\nit wants to handle error cases itself\" would do).\n\n> @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> \t (write_cache(index_fd, active_cache, active_nr) ||\n> \t commit_locked_index(&index_lock)))\n> \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n> -\t\tdie(_(\"%s: Unable to write new index file\"), me);\n> +\t\treturn error(_(\"%s: Unable to write new index file\"), me);\n> \trollback_lock_file(&index_lock);\n\nDo the callers rollback the lockfile in their error return codepaths now?\nShould they? If not why not?\n\nOne acceptable answer is \"the only thing the current callers do in their\nerror codepaths is to exit(129), and that will roll it back for us\", but\nthen that might mean this patch made the API more error prone to use when\nthe next callsite you add wants to do more than just exitting.\n\n> @@ -397,18 +397,18 @@ static int do_pick_commit(void)\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\t\treturn error(_(\"Your index file is unmerged.\"));\n> \t} else {\n> \t\tif (get_sha1(\"HEAD\", head))\n> -\t\t\tdie (_(\"You do not have a valid HEAD\"));\n> +\t\t\treturn error(_(\"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_dirty_index(me);\n> \t}\n> \tdiscard_cache();\n\nLikewise for this \"discard-cache\". Should it be the responsibility to the\ncaller to discard the in-core cache when they handle an error return and\npossibly take an alternative action, or should this function be the one to\ndo so for them?\n"},{"id":"165654","messageId":"7vmxjwtqhz.fsf@alter.siamese.dyndns.org","threadId":"27050","inReplyTo":"1302448317-32387-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-11T21:41:44Z","receivedAt":"2011-04-11T21:41:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Signed-off-by: Ramkumar Ramachandra \n\nAgain, \"In later steps, a new API that takes a single commit and replays\nit in forward (cherry-pick) or backward (revert) direction will be\nintroduced, and will take this structure as a parameter to tell it what to\ndo\" is missing from the above description.\n\nMore importantly, the primary purpose of these variables is _not_ \"to\nparse command line options into\". It is to actively affect what happens\nin the code, and \"parse command line\" is merely a way to assign the\ninitial values to them. So I'd rather see this patch described perhaps\nlike this:\n\n cherry-pick/revert: introduce \"struct replay_options\"\n\n The current code uses a set of file-scope static variables to instruct\n the cherry-pick/revert machinery how to replay the changes, and\n initialises them by parsing the command line arguments. In later steps\n in this series, we would like to introduce an API function that calls\n into this machinery directly and have a way to tell it what to do.\n\n Introduce a structure to group these variables, so that the API can\n take them as a single \"replay_options\" parameter.\n\nI strongly prefer to see this patch also update the callchain to pass a\npointer to the options struct as parameter. I can guess without reading\nthe rest the series that at some later step you would do that, but I think\nit makes more sense to do the conversion at this step, as you will be\ntouching lines that use the global variables in this patch anyway, like\nthis:\n\n-\treturn action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn cmd_opts.action == REVERT ? revert_usage : cherry_pick_usage;\n\nwhich should eventually become something like:\n\n\treturn opts->usage_message;\n\nat the very last step when this codepath is _fully_ libified (I don't know\nif the final result of your series still is a \"choose only from these two\ncanned messages\" API, or gives the ability to the caller to specify the\nusage message---I am just showing how it should look like at the end of a\nfull libification). So it would make sense to see:\n\n\treturn opts->action == REVERT ? revert_usage : cherry_pick_usage;\n\nin this patch.\n\n> @@ -268,17 +278,17 @@ static struct tree *empty_tree(void)\n> static int error_dirty_index()\n\nIt is probably a remnant of the earlier patches in this series, but this\nshould start with:\n\n\tstatic int error_dirty_index(void)\n\nOf course, you will actually be passing the options structure, so it would\nbecome:\n\n\tstatic int error_dirty_index(struct replay_options *opts)\n {\n \t...\n if (opts->action == REVERT)\n \t...\n\t}\n"},{"id":"165655","messageId":"7vfwpotqdx.fsf@alter.siamese.dyndns.org","threadId":"27050","inReplyTo":"1302448317-32387-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 05/11] revert: Catch incompatible command-line options early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-11T21:44:10Z","receivedAt":"2011-04-11T21:44:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> +static void die_opt_incompatible(const char *me, const char *base_opt,\n> +\t\t\t\tint nargs, int opt_bitarray[], ...)\n> +{\n\nThis feels way overkill to use vararg. This may be _one_ way to do what\nyou want to do, and while I don't think it is the best way, it may be Ok.\n\n\"opt_bitarray[]\" is a horrible name, though. It does not convey what\nthe variable is about (its \"purpose\"). It doesn't even correctly describe\nthe way that unspecified purpose happens to be implemented (claims to be\nbitarray but it is just an array of ints and what matters is if any bit is\nset in the array).\n\n> static void parse_args(int argc, const char **argv)\n> {\n> \tconst char * const * usage_str = revert_or_cherry_pick_usage();\n> @@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)\n> \tif (cmd_opts.commit_argc < 2)\n> \t\tusage_with_options(usage_str, options);\n> \n> +\tif (cmd_opts.allow_ff) {\n> +\t\tint opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,\n> +\t\t\t\t cmd_opts.no_replay, cmd_opts.edit};\n> +\t\tdie_opt_incompatible(me, \"--ff\", 4, opt_bitarray, \"--signoff\",\n> +\t\t\t\t\"--no-commit\", \"-x\", \"--edit\");\n> +\t}\n\nWhy not do it like this instead?\n\n\tstruct incompatible {\n \tunsigned option_bit;\n const char *option_name;\n\t} incompatible[] = {\n\t\t{ opts->signoff, \"--signoff\" },\n { opts->no_commit, \"--no-commit\" },\n ...\n\t};\n\tverify_compatible(\"me\", \"--ff\", incompatible, ARRAY_SIZE(incompatible));\n\nOr if you are shooting for ease-of-use, it might make sense to do it like\nthis:\n\n\tverify_compatible(\"me\", \"--ff\",\n \t\t\"--signoff\", opts->signoff,\n \"--no-commit\", opts->no_commit,\n ...\n NULL);\n\nand make verify_compatible() a varargs function that takes two optional\narguments at a time, i.e. const char *, followed by an int. Then there is\nno need for extra \"int opt_bitarray[]\" or \"struct incompatible\".\n\nThat would justify use of varargs, I think.\n"},{"id":"167364","messageId":"20110508114749.GA3114@ramkum.desktop.amazon.com","threadId":"27050","inReplyTo":"7vfwpotqdx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 05/11] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-08T11:47:51Z","receivedAt":"2011-05-08T11:47:51Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano writes:\n> Ramkumar Ramachandra writes:\n> > @@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)\n> > \tif (cmd_opts.commit_argc < 2)\n> > \t\tusage_with_options(usage_str, options);\n> > \n> > +\tif (cmd_opts.allow_ff) {\n> > +\t\tint opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,\n> > +\t\t\t\t cmd_opts.no_replay, cmd_opts.edit};\n> > +\t\tdie_opt_incompatible(me, \"--ff\", 4, opt_bitarray, \"--signoff\",\n> > +\t\t\t\t\"--no-commit\", \"-x\", \"--edit\");\n> > +\t}\n> \n> Why not do it like this instead?\n> \n> \tstruct incompatible {\n> \tunsigned option_bit;\n> const char *option_name;\n> \t} incompatible[] = {\n> \t\t{ opts->signoff, \"--signoff\" },\n> { opts->no_commit, \"--no-commit\" },\n> ...\n> \t};\n> \tverify_compatible(\"me\", \"--ff\", incompatible, ARRAY_SIZE(incompatible));\n> \n> Or if you are shooting for ease-of-use, it might make sense to do it like\n> this:\n> \n> \tverify_compatible(\"me\", \"--ff\",\n> \t\t\"--signoff\", opts->signoff,\n> \"--no-commit\", opts->no_commit,\n> ...\n> NULL);\n> \n> and make verify_compatible() a varargs function that takes two optional\n> arguments at a time, i.e. const char *, followed by an int. Then there is\n> no need for extra \"int opt_bitarray[]\" or \"struct incompatible\".\n> \n> That would justify use of varargs, I think.\n\nNow that you point it out, my original approach was unnecessarily\ncryptic and convoluted. I've followed this approach in my new series,\nand kept varargs -- the code looks much prettier now. Thanks :)\n\nI can't justify changing the name from \"die_opt_incompatible\" to\n\"verify_compatible\" though; the name I've chosen seems to be more\nappropriate/ descriptive. Further, I think command-line parsing\nshould always be the toplevel caller, and \"die\" is therefore more\nappropriate than \"return error\" here.\n\n-- Ram\n"},{"id":"167366","messageId":"20110508120358.GB3114@ramkum.desktop.amazon.com","threadId":"27050","inReplyTo":"20110410191458.GA28163@elie","subject":"Re: [PATCH 01/11] revert: Avoid calling die; return error instead","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-08T12:04:00Z","receivedAt":"2011-05-08T12:04:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n> > [Subject: revert: Avoid calling die; return error instead]\n> >\n> > Signed-off-by: Ramkumar Ramachandra \n> \n> Presumably this is because the sequencer is going to pick up after the\n> error and clean up a little (why doesn't the change description say\n> so?). Will it be resuming after that or just performing a little\n> cleanup before the exit?\n\nI didn't write commit messages for any of the patches in the previous\nround -- I just wanted to show the idea quickly. Anyway, it's fixed\nin the next round (coming soon).\n\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -265,23 +265,23 @@ static struct tree *empty_tree(void)\n> > \treturn tree;\n> > }\n> > \n> > -static NORETURN void die_dirty_index(const char *me)\n> > +static int error_dirty_index(const char *me)\n> > {\n> > \tif (read_cache_unmerged()) {\n> > \t\tdie_resolve_conflict(me);\n> \n> Won't that exit? \n\nFixed.\n\n> > \t} else {\n> \n> This \"else\" could be removed (decreasing the indent of the rest by\n> one tab stop) since the \"if\" case has already returned or exited.\n> Not the subject of this patch, just an idea for earlier or later. ;-)\n\nFixed.\n\n> [...]\n> > @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> > \t (write_cache(index_fd, active_cache, active_nr) ||\n> > \t commit_locked_index(&index_lock)))\n> > \t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n> > -\t\tdie(_(\"%s: Unable to write new index file\"), me);\n> > +\t\treturn error(_(\"%s: Unable to write new index file\"), me);\n> > \trollback_lock_file(&index_lock);\n> \n> What happens to index_lock in the error case?\n\nFixed.\n\n> [...]\n> > @@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)\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 (argc > 1) {\n> > +\t\tfprintf(stderr, \"usage: %s\", _(*revert_or_cherry_pick_usage()));\n> > +\t\treturn 129;\n> > +\t}\n> \n> Yuck. Maybe the error can be returned to the caller somehow, but\n> that seems somehow ambitious given that setup_revisions has all sorts\n> of ways to die anyway.\n> \n> So you are bending the assumptions of many existing git functions (in\n> a good way).\n> \n> I can think of at least three ways to go:\n> \n> 1) Come up with a convention to give more information about the nature\n> of returned errors in the functions you are touching. For\n> example, make sure errno is valid after the relevant functions, or\n> use multiple negative values to express the nature of the error.\n> \n> So a caller could do:\n> \n> \tif (prepare_revs(...)) {\n> \t\tif (errno == EINVAL)\n> \t\t\tusage(*revert_or_cherry_pick_usage());\n> \t\tdie(\"BUG: unexpected error from prepare_revs\");\n> \t}\n> \n> Or:\n> \n> 2) Use set_die_routine or sigchain_push + atexit to declare what cleanup\n> has to happen before exiting. Keep using die().\n> \n> 3) Provide a new facility to register cleanup handlers that will free\n> resources and otherwise return to a consistent state before\n> unwinding the stack. This way, you'd still have to audit die()\n> calls to look for missing cleanup handlers, but they could stay as\n> die() rather than changing to \"return error\" and the worried\n> caller could use\n> \n> \tset_die_routine(longjmp_to_here);\n> \n> to keep git alive. I don't suggest doing this. It is a pain to\n> get right and not obviously cleaner than \"return error\", and some\n> errors really are unrecoverable (rather than just being a symptom\n> of programmers to lazy to write error recovery code :)).\n\nHm, I'm a little confused about error handling now. I'll defer this\npart until my patches naturally establish some convention for error\nhandling -- designing one in advance isn't as easy as I thought.\n\nFinally, thanks for the review! I'll look forward to more reviews as\nI post more iterations of the series.\n\n-- Ram\n"},{"id":"167367","messageId":"20110508120905.GC3114@ramkum.desktop.amazon.com","threadId":"27050","inReplyTo":"7vmxjwtqhz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-08T12:09:08Z","receivedAt":"2011-05-08T12:09:08Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano writes:\n> Ramkumar Ramachandra writes:\n> \n> > Signed-off-by: Ramkumar Ramachandra \n> \n> Again, \"In later steps, a new API that takes a single commit and replays\n> it in forward (cherry-pick) or backward (revert) direction will be\n> introduced, and will take this structure as a parameter to tell it what to\n> do\" is missing from the above description.\n> \n> More importantly, the primary purpose of these variables is _not_ \"to\n> parse command line options into\". It is to actively affect what happens\n> in the code, and \"parse command line\" is merely a way to assign the\n> initial values to them. So I'd rather see this patch described perhaps\n> like this:\n> \n> cherry-pick/revert: introduce \"struct replay_options\"\n> \n> The current code uses a set of file-scope static variables to instruct\n> the cherry-pick/revert machinery how to replay the changes, and\n> initialises them by parsing the command line arguments. In later steps\n> in this series, we would like to introduce an API function that calls\n> into this machinery directly and have a way to tell it what to do.\n> \n> Introduce a structure to group these variables, so that the API can\n> take them as a single \"replay_options\" parameter.\n\nRight. Thanks for the nice commit message :)\n\n> I strongly prefer to see this patch also update the callchain to pass a\n> pointer to the options struct as parameter. I can guess without reading\n> the rest the series that at some later step you would do that, but I think\n> it makes more sense to do the conversion at this step, as you will be\n> touching lines that use the global variables in this patch anyway, like\n> this:\n\nGood idea. I've passed the opts pointer around in the new series.\n\n> > @@ -268,17 +278,17 @@ static struct tree *empty_tree(void)\n> > static int error_dirty_index()\n> \n> It is probably a remnant of the earlier patches in this series, but this\n> should start with:\n> \n> \tstatic int error_dirty_index(void)\n> \n> Of course, you will actually be passing the options structure, so it would\n> become:\n> \n> \tstatic int error_dirty_index(struct replay_options *opts)\n> {\n> \t...\n> if (opts->action == REVERT)\n> \t...\n> \t}\n\nThe very first patch removes this function (and puts the functionality\nelsewhere) in the new series, so this comment doesn't apply.\n\nThanks for the review!\n\n-- Ram\n"},{"id":"167373","messageId":"20110508121839.GD3114@ramkum.desktop.amazon.com","threadId":"27050","inReplyTo":"20110410192154.GB28163@elie","subject":"Re: [PATCH 03/11] revert: Introduce a struct to parse command-line options into","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-08T12:18:40Z","receivedAt":"2011-05-08T12:18:40Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -35,17 +35,27 @@ 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> > +static struct {\n> > +\tenum { REVERT, CHERRY_PICK } action;\n> \n> That would require giving this struct a name, so it can be passed\n> around. Not a bad idea anyway imho since then a person reading\n> top-to-bottom is not left in suspense:\n> \n> struct cherry_pick_opts {\n> \tenum { REVERT, CHERRY_PICK } action;\n> \n> \tunsigned edit:1;\n> \tunsigned no_replay:1;\n> \t...\n> };\n\nThanks. A couple of nits:\n1. You can't take the address of a bitfield, so 'edit' and 'replay'\n can't be passed to the command line argument parsing framework\n directly.\n2. GCC throws a \"warning: useless storage class specifier in empty\n declaration\" for this kind of declaration.\n\n-- Ram\n"}]}