{"thread":{"id":"27453","subject":"[PATCH v3 00/10] Sequencer Foundations","startedAt":"2011-05-25T14:16:55Z","lastAt":"2011-06-08T13:14:14Z","messageCount":37,"participants":["Ramkumar Ramachandra","Junio C Hamano","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":10},"messages":[{"id":"168648","messageId":"1306333025-29893-1-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":null,"subject":"[PATCH v3 00/10] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:16:55Z","receivedAt":"2011-05-25T14:16:55Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nError handling is a very complex topic, and trying to build it up with\nthe Sequencer series had disasterous consequences, as seen in the\nprevious iteration of the series. In this iteration, I've nearly\nre-written everything from scratch, and tried to get the basics right,\nwithout introducing anything new. Time is short, and I'll have to\nwork very hard if I want to target \"master\",\n\nI hope there are no major issues with this iteration.\n\nThanks for reading.\n\n-- Ram\n\nRamkumar Ramachandra (10):\n advice: Introduce error_resolve_conflict\n revert: Propogate errors upwards from do_pick_commit\n revert: Eliminate global \"commit\" variable\n revert: Rename no_replay to record_origin\n revert: Introduce struct to keep command-line options\n revert: Separate cmdline parsing from functional code\n revert: Catch incompatible command-line options early\n revert: Introduce HEAD, TODO files to persist state, plan\n revert: Implement parsing --continue, --abort and --skip\n revert: Implement --abort processing\n\n advice.c | 17 ++-\n advice.h | 1 +\n builtin/revert.c | 484 +++++++++++++++++++++++++++++++++++++++---------------\n 3 files changed, 361 insertions(+), 141 deletions(-)\n\n-- \n1.7.5.GIT\n"},{"id":"168649","messageId":"1306333025-29893-2-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 01/10] advice: Introduce error_resolve_conflict","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:16:56Z","receivedAt":"2011-05-25T14:16:56Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce error_resolve_conflict corresponding to\ndie_resolve_conflict, and implement the latter function in terms of\nthe former.\n\nBased-on-patch-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n advice.c | 17 +++++++++++------\n advice.h | 1 +\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 0be4b5f..80aaa14 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -34,16 +34,21 @@ int git_default_advice_config(const char *var, const char *value)\n \treturn 0;\n }\n \n-void NORETURN die_resolve_conflict(const char *me)\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\tdie(\"'%s' is not possible because you have unmerged files.\\n\"\n-\t\t \"Please, fix them up in the work tree, and then use 'git add/rm ' as\\n\"\n-\t\t \"appropriate to mark resolution and make a commit, or use 'git commit -a'.\", me);\n-\telse\n-\t\tdie(\"'%s' is not possible because you have unmerged files.\", me);\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+\treturn error(\"'%s' is not possible because you have unmerged files.\", me);\n+}\n+\n+void NORETURN die_resolve_conflict(const char *me)\n+{\n+\terror_resolve_conflict(me);\n+\texit(128);\n }\ndiff --git a/advice.h b/advice.h\nindex 3244ebb..f537366 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -12,6 +12,7 @@ extern int advice_detached_head;\n \n int git_default_advice_config(const char *var, const char *value);\n \n+int error_resolve_conflict(const char *me);\n extern void NORETURN die_resolve_conflict(const char *me);\n \n #endif /* ADVICE_H */\n-- \n1.7.5.GIT\n"},{"id":"168654","messageId":"1306333025-29893-3-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 02/10] revert: Propogate errors upwards from do_pick_commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:16:57Z","receivedAt":"2011-05-25T14:16:57Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce error_dirty_index, based on die_dirty_index which prints\nsome hints, and returns an error to be handled by its caller,\ndo_pick_commit. do_pick_commit, in turn, propogates the error\nupwards, and the error is finally handled by cmd_cherry_pick and\ncmd_revert. Set a simple convention for error-handling as follows:\npositive return values from do_pick_commit indicate conflicts, while\nnegative ones indicate other errors.\n\nMentored-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 72 ++++++++++++++++++++++++++++-------------------------\n 1 files changed, 38 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex f697e66..523d41a 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -266,25 +266,15 @@ 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\t\t \"Please, commit your changes or stash them to proceed.\"));\n-\t\t\telse\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by cherry-pick.\\n\"\n-\t\t\t\t\t \"Please, commit your changes or stash them to proceed.\"));\n-\t\t} else {\n-\t\t\tif (action == REVERT)\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by revert.\\n\"));\n-\t\t\telse\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by cherry-pick.\\n\"));\n-\t\t}\n-\t}\n+\tif (read_cache_unmerged())\n+\t\treturn error_resolve_conflict(me);\n+\n+\tint ret = error(_(\"Your local changes would be overwritten by %s.\\n\"), me);\n+\tif (advice_commit_before_merge)\n+\t\tadvise(_(\"Please, commit your changes or stash them to proceed.\"));\n+\treturn ret;\n }\n \n static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n@@ -398,18 +388,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@@ -418,20 +408,20 @@ static int do_pick_commit(void)\n \t\tstruct commit_list *p;\n \n \t\tif (!mainline)\n-\t\t\tdie(_(\"Commit %s is a merge but no -m option was given.\"),\n-\t\t\t sha1_to_hex(commit->object.sha1));\n+\t\t\treturn error(_(\"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\t sha1_to_hex(commit->object.sha1), mainline);\n+\t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n+\t\t\t\tsha1_to_hex(commit->object.sha1), mainline);\n \t\tparent = p->item;\n \t} else if (0 < mainline)\n-\t\tdie(_(\"Mainline was specified but commit %s is not a merge.\"),\n-\t\t sha1_to_hex(commit->object.sha1));\n+\t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n \telse\n \t\tparent = commit->parents->item;\n \n@@ -441,12 +431,12 @@ static int do_pick_commit(void)\n \tif (parent && parse_commit(parent) < 0)\n \t\t/* TRANSLATORS: The first %s will be \"revert\" or\n \t\t \"cherry-pick\", the second %s a SHA1 */\n-\t\tdie(_(\"%s: cannot parse parent commit %s\"),\n-\t\t me, sha1_to_hex(parent->object.sha1));\n+\t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n+\t\t\tme, sha1_to_hex(parent->object.sha1));\n \n \tif (get_message(commit->buffer, &msg) != 0)\n-\t\tdie(_(\"Cannot get commit message for %s\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n+\t\treturn error(_(\"Cannot get commit message for %s\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n \n \t/*\n \t * \"commit\" is an existing commit. We would want to apply\n@@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n+\tint res = 0;\n \tif (isatty(0))\n \t\tedit = 1;\n \taction = REVERT;\n-\treturn revert_or_cherry_pick(argc, argv);\n+\tres = revert_or_cherry_pick(argc, argv);\n+\tif (res > 0)\n+\t\t/* Exit status from conflict */\n+\t\treturn res;\n+\tif (res < 0)\n+\t\t/* Other error */\n+\t\texit(128);\n+\treturn 0;\n }\n \n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n+\tint res = 0;\n \taction = CHERRY_PICK;\n-\treturn revert_or_cherry_pick(argc, argv);\n+\tres = revert_or_cherry_pick(argc, argv);\n+\tif (res > 0)\n+\t\treturn res;\n+\tif (res < 0)\n+\t\texit(128);\n+\treturn 0;\n }\n-- \n1.7.5.GIT\n"},{"id":"168650","messageId":"1306333025-29893-4-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 03/10] revert: Eliminate global \"commit\" variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:16:58Z","receivedAt":"2011-05-25T14:16:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Since we want to develop the functionality to either pick/ revert\nindividual commits atomically later in the series, make \"commit\" a\nlocal variable. Modify the get_message and add_message_to_msg API to\ntake an additional parameter: the pre-computed SHA1 character string\nto use. When reporting a program bug in get_encoding, don't include\nthe commit in the message.\n\nMentored-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 40 ++++++++++++++++++++--------------------\n 1 files changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 523d41a..6c485f6 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -37,7 +37,6 @@ 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@@ -116,11 +115,12 @@ 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(const char *sha1_abbrev, const char *raw_message,\n+\t\tstruct commit_message *out)\n {\n \tconst char *encoding;\n-\tconst char *abbrev, *subject;\n-\tint abbrev_len, subject_len;\n+\tconst char *subject;\n+\tint subject_len;\n \tchar *q;\n \n \tif (!raw_message)\n@@ -139,17 +139,14 @@ static int get_message(const char *raw_message, struct commit_message *out)\n \tif (out->reencoded_message)\n \t\tout->message = out->reencoded_message;\n \n-\tabbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n-\tabbrev_len = strlen(abbrev);\n-\n \tsubject_len = find_commit_subject(out->message, &subject);\n \n-\tout->parent_label = xmalloc(strlen(\"parent of \") + abbrev_len +\n+\tout->parent_label = xmalloc(strlen(\"parent of \") + DEFAULT_ABBREV +\n \t\t\t strlen(\"... \") + subject_len + 1);\n \tq = out->parent_label;\n \tq = mempcpy(q, \"parent of \", strlen(\"parent of \"));\n \tout->label = q;\n-\tq = mempcpy(q, abbrev, abbrev_len);\n+\tq = mempcpy(q, sha1_abbrev, DEFAULT_ABBREV);\n \tq = mempcpy(q, \"... \", strlen(\"... \"));\n \tout->subject = q;\n \tq = mempcpy(q, subject, subject_len);\n@@ -168,8 +165,7 @@ static char *get_encoding(const char *message)\n \tconst char *p = message, *eol;\n \n \tif (!p)\n-\t\tdie (_(\"Could not read commit message of %s\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n+\t\tdie (_(\"BUG: get_encoding() called with NULL\"));\n \twhile (*p && *p != '\\n') {\n \t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n \t\t\t; /* do nothing */\n@@ -185,25 +181,26 @@ 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(const char *fallback, struct strbuf *msgbuf,\n+\t\t\tconst char *message)\n {\n \tconst char *p = message;\n \twhile (*p && (*p != '\\n' || p[1] != '\\n'))\n \t\tp++;\n \n \tif (!*p)\n-\t\tstrbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));\n+\t\tstrbuf_addstr(msgbuf, fallback);\n \n \tp += 2;\n \tstrbuf_addstr(msgbuf, p);\n }\n \n-static void write_cherry_pick_head(void)\n+static void write_cherry_pick_head(const char *commit_sha1_hex)\n {\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n+\tstrbuf_addf(&buf, \"%s\\n\", commit_sha1_hex);\n \n \tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n@@ -370,7 +367,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@@ -378,6 +375,7 @@ static int do_pick_commit(void)\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n+\tconst char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n \tint res;\n \n \tif (no_commit) {\n@@ -434,7 +432,7 @@ static int do_pick_commit(void)\n \t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n \t\t\tme, sha1_to_hex(parent->object.sha1));\n \n-\tif (get_message(commit->buffer, &msg) != 0)\n+\tif (get_message(sha1_abbrev, commit->buffer, &msg) != 0)\n \t\treturn error(_(\"Cannot get commit message for %s\"),\n \t\t\tsha1_to_hex(commit->object.sha1));\n \n@@ -463,18 +461,19 @@ static int do_pick_commit(void)\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n \t} else {\n+\t\tconst char *commit_sha1_hex = sha1_to_hex(commit->object.sha1);\n \t\tbase = parent;\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_sha1_hex, &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_sha1_hex);\n \t}\n \n \tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n@@ -552,6 +551,7 @@ static void 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 \n \tgit_config(git_default_config, NULL);\n \tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n@@ -574,7 +574,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \tprepare_revs(&revs);\n \n \twhile ((commit = get_revision(&revs))) {\n-\t\tint res = do_pick_commit();\n+\t\tint res = do_pick_commit(commit);\n \t\tif (res)\n \t\t\treturn res;\n \t}\n-- \n1.7.5.GIT\n"},{"id":"168658","messageId":"1306333025-29893-5-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 04/10] revert: Rename no_replay to record_origin","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:16:59Z","receivedAt":"2011-05-25T14:16:59Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Give the variable \"no_replay\" a new name \"record_origin\", so that\nthere is no confusion when a \"replay\" structure is introduced later in\nthe series.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 6c485f6..c48a7df 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -35,7 +35,7 @@ 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 int edit, record_origin, 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@@ -90,7 +90,7 @@ static void parse_args(int argc, const char **argv)\n \n \tif (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('x', NULL, &record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &allow_ff, \"allow fast-forward\"),\n \t\t\tOPT_END(),\n \t\t};\n@@ -467,7 +467,7 @@ 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_sha1_hex, &msgbuf, msg.message);\n-\t\tif (no_replay) {\n+\t\tif (record_origin) {\n \t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n@@ -563,7 +563,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n \t\tif (no_commit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (no_replay)\n+\t\tif (record_origin)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n \t\tif (edit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n-- \n1.7.5.GIT\n"},{"id":"168656","messageId":"1306333025-29893-6-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 05/10] revert: Introduce struct to keep command-line options","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:17:00Z","receivedAt":"2011-05-25T14:17:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The current code uses a set of file-scope static variables to tell the\ncherry-pick/ revert machinery how to replay the changes, and\ninitializes them by parsing the command-line arguments. In later\nsteps in this series, we would like to introduce an API function that\ncalls into this machinery directly and have a way to tell it what to\ndo. Hence, introduce a structure to group these variables, so that\nthe API can take them as a single replay_options parameter.\n\nHelped-by: Junio C Hamano \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 174 ++++++++++++++++++++++++++++++------------------------\n 1 files changed, 97 insertions(+), 77 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex c48a7df..0d6a5a9 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -35,76 +35,88 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-static int edit, record_origin, 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 const char *me;\n \n-/* Merge strategy. */\n-static const char *strategy;\n-static const char **xopts;\n-static size_t xopts_nr, xopts_alloc;\n+struct replay_opts {\n+\tenum { REVERT, CHERRY_PICK } action;\n+\n+\t/* Boolean options */\n+\tint edit;\n+\tint record_origin;\n+\tint no_commit;\n+\tint signoff;\n+\tint allow_ff;\n+\tint allow_rerere_auto;\n+\n+\tint mainline;\n+\tint commit_argc;\n+\tconst char **commit_argv;\n+\n+\t/* Merge strategy */\n+\tconst char *strategy;\n+\tconst char **xopts;\n+\tsize_t xopts_nr, xopts_alloc;\n+};\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n static char *get_encoding(const char *message);\n \n-static const char * const *revert_or_cherry_pick_usage(void)\n+static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n {\n-\treturn action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn opts->action == REVERT ? revert_usage : cherry_pick_usage;\n }\n \n static int option_parse_x(const struct option *opt,\n \t\t\t const char *arg, int unset)\n {\n+\tstruct replay_opts **opts = opt->value;\n+\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((*opts)->xopts, (*opts)->xopts_nr + 1, (*opts)->xopts_alloc);\n+\t(*opts)->xopts[(*opts)->xopts_nr++] = xstrdup(arg);\n \treturn 0;\n }\n \n-static void parse_args(int argc, const char **argv)\n+static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n-\tconst char * const * usage_str = revert_or_cherry_pick_usage();\n+\tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tint noop;\n \tstruct option options[] = {\n-\t\tOPT_BOOLEAN('n', \"no-commit\", &no_commit, \"don't automatically commit\"),\n-\t\tOPT_BOOLEAN('e', \"edit\", &edit, \"edit the commit message\"),\n+\t\tOPT_BOOLEAN('n', \"no-commit\", &opts->no_commit, \"don't automatically commit\"),\n+\t\tOPT_BOOLEAN('e', \"edit\", &opts->edit, \"edit the commit message\"),\n \t\t{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, \"no-op (backward compatibility)\",\n \t\t PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },\n-\t\tOPT_BOOLEAN('s', \"signoff\", &signoff, \"add Signed-off-by:\"),\n-\t\tOPT_INTEGER('m', \"mainline\", &mainline, \"parent number\"),\n-\t\tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n-\t\tOPT_STRING(0, \"strategy\", &strategy, \"strategy\", \"merge strategy\"),\n-\t\tOPT_CALLBACK('X', \"strategy-option\", &xopts, \"option\",\n+\t\tOPT_BOOLEAN('s', \"signoff\", &opts->signoff, \"add Signed-off-by:\"),\n+\t\tOPT_INTEGER('m', \"mainline\", &opts->mainline, \"parent number\"),\n+\t\tOPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),\n+\t\tOPT_STRING(0, \"strategy\", &opts->strategy, \"strategy\", \"merge strategy\"),\n+\t\tOPT_CALLBACK('X', \"strategy-option\", &opts, \"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 (opts->action == CHERRY_PICK) {\n \t\tstruct option cp_extra[] = {\n-\t\t\tOPT_BOOLEAN('x', NULL, &record_origin, \"append commit name\"),\n-\t\t\tOPT_BOOLEAN(0, \"ff\", &allow_ff, \"allow fast-forward\"),\n+\t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n+\t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n \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-\t\t\t\t PARSE_OPT_KEEP_ARGV0 |\n-\t\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n-\tif (commit_argc < 2)\n+\topts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,\n+\t\t\t\t\tPARSE_OPT_KEEP_ARGV0 |\n+\t\t\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n+\tif (opts->commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n-\tcommit_argv = argv;\n+\topts->commit_argv = argv;\n }\n \n struct commit_message {\n@@ -287,7 +299,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n \n static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\t\t const char *base_label, const char *next_label,\n-\t\t\t unsigned char *head, struct strbuf *msgbuf)\n+\t\t\t unsigned char *head, struct strbuf *msgbuf,\n+\t\t\t struct replay_opts *opts)\n {\n \tstruct merge_options o;\n \tstruct tree *result, *next_tree, *base_tree, *head_tree;\n@@ -308,7 +321,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tnext_tree = next ? next->tree : empty_tree();\n \tbase_tree = base ? base->tree : empty_tree();\n \n-\tfor (xopt = xopts; xopt != xopts + xopts_nr; xopt++)\n+\tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n \t\tparse_merge_opt(&o, *xopt);\n \n \tclean = merge_trees(&o,\n@@ -348,7 +361,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n * If we are revert, or if our cherry-pick results in a hand merge,\n * we had better say that the current user is responsible for that.\n */\n-static int run_git_commit(const char *defmsg)\n+static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n {\n \t/* 6 is max possible length of our args array including NULL */\n \tconst char *args[6];\n@@ -356,9 +369,9 @@ static int run_git_commit(const char *defmsg)\n \n \targs[i++] = \"commit\";\n \targs[i++] = \"-n\";\n-\tif (signoff)\n+\tif (opts->signoff)\n \t\targs[i++] = \"-s\";\n-\tif (!edit) {\n+\tif (!opts->edit) {\n \t\targs[i++] = \"-F\";\n \t\targs[i++] = defmsg;\n \t}\n@@ -367,7 +380,7 @@ static int run_git_commit(const char *defmsg)\n \treturn run_command_v_opt(args, RUN_GIT_CMD);\n }\n \n-static int do_pick_commit(struct commit *commit)\n+static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n {\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n@@ -378,7 +391,7 @@ static int do_pick_commit(struct commit *commit)\n \tconst char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n \tint res;\n \n-\tif (no_commit) {\n+\tif (opts->no_commit) {\n \t\t/*\n \t\t * We do not intend to commit immediately. We just want to\n \t\t * merge the differences in, so let's compute the tree\n@@ -396,7 +409,7 @@ static int do_pick_commit(struct commit *commit)\n \tdiscard_cache();\n \n \tif (!commit->parents) {\n-\t\tif (action == REVERT)\n+\t\tif (opts->action == REVERT)\n \t\t\treturn error(_(\"Cannot revert a root commit\"));\n \t\tparent = NULL;\n \t}\n@@ -405,25 +418,25 @@ static int do_pick_commit(struct commit *commit)\n \t\tint cnt;\n \t\tstruct commit_list *p;\n \n-\t\tif (!mainline)\n+\t\tif (!opts->mainline)\n \t\t\treturn error(_(\"Commit %s is a merge but no -m option was given.\"),\n \t\t\t\tsha1_to_hex(commit->object.sha1));\n \n \t\tfor (cnt = 1, p = commit->parents;\n-\t\t cnt != mainline && p;\n+\t\t cnt != opts->mainline && p;\n \t\t cnt++)\n \t\t\tp = p->next;\n-\t\tif (cnt != mainline || !p)\n+\t\tif (cnt != opts->mainline || !p)\n \t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1), mainline);\n+\t\t\t\tsha1_to_hex(commit->object.sha1), opts->mainline);\n \t\tparent = p->item;\n-\t} else if (0 < mainline)\n+\t} else if (0 < opts->mainline)\n \t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n \t\t\tsha1_to_hex(commit->object.sha1));\n \telse\n \t\tparent = commit->parents->item;\n \n-\tif (allow_ff && parent && !hashcmp(parent->object.sha1, head))\n+\tif (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))\n \t\treturn fast_forward_to(commit->object.sha1, head);\n \n \tif (parent && parse_commit(parent) < 0)\n@@ -445,7 +458,7 @@ static int do_pick_commit(struct commit *commit)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (action == REVERT) {\n+\tif (opts->action == REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -467,18 +480,18 @@ 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_sha1_hex, &msgbuf, msg.message);\n-\t\tif (record_origin) {\n+\t\tif (opts->record_origin) {\n \t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n-\t\tif (!no_commit)\n+\t\tif (!opts->no_commit)\n \t\t\twrite_cherry_pick_head(commit_sha1_hex);\n \t}\n \n-\tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n+\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n-\t\t\t\t\t head, &msgbuf);\n+\t\t\t\t\t head, &msgbuf, opts);\n \t\twrite_message(&msgbuf, defmsg);\n \t} else {\n \t\tstruct commit_list *common = NULL;\n@@ -488,23 +501,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\t\t\t\tsha1_to_hex(head), remotes);\n+\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n+\t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n \t}\n \n \tif (res) {\n-\t\terror(action == REVERT\n+\t\terror(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(opts->allow_rerere_auto);\n \t} else {\n-\t\tif (!no_commit)\n-\t\t\tres = run_git_commit(defmsg);\n+\t\tif (!opts->no_commit)\n+\t\t\tres = run_git_commit(defmsg, opts);\n \t}\n \n \tfree_message(&msg);\n@@ -513,18 +526,18 @@ static int do_pick_commit(struct commit *commit)\n \treturn res;\n }\n \n-static void prepare_revs(struct rev_info *revs)\n+static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)\n {\n \tint argc;\n \n \tinit_revisions(revs, NULL);\n \trevs->no_walk = 1;\n-\tif (action != REVERT)\n+\tif (opts->action != REVERT)\n \t\trevs->reverse = 1;\n \n-\targc = setup_revisions(commit_argc, commit_argv, revs, NULL);\n+\targc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);\n \tif (argc > 1)\n-\t\tusage(*revert_or_cherry_pick_usage());\n+\t\tusage(*revert_or_cherry_pick_usage(opts));\n \n \tif (prepare_revision_walk(revs))\n \t\tdie(_(\"revision walk setup failed\"));\n@@ -533,7 +546,7 @@ static void prepare_revs(struct rev_info *revs)\n \t\tdie(_(\"empty commit set passed\"));\n }\n \n-static void read_and_refresh_cache(const char *me)\n+static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n {\n \tstatic struct lock_file index_lock;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n@@ -548,33 +561,34 @@ static void read_and_refresh_cache(const char *me)\n \trollback_lock_file(&index_lock);\n }\n \n-static int revert_or_cherry_pick(int argc, const char **argv)\n+static int revert_or_cherry_pick(int argc, const char **argv,\n+\t\t\t\tstruct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n \n \tgit_config(git_default_config, NULL);\n-\tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n+\tme = opts->action == REVERT ? \"revert\" : \"cherry-pick\";\n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tparse_args(argc, argv);\n+\tparse_args(argc, argv, opts);\n \n-\tif (allow_ff) {\n-\t\tif (signoff)\n+\tif (opts->allow_ff) {\n+\t\tif (opts->signoff)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n-\t\tif (no_commit)\n+\t\tif (opts->no_commit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (record_origin)\n+\t\tif (opts->record_origin)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n-\t\tif (edit)\n+\t\tif (opts->edit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n \t}\n \n-\tread_and_refresh_cache(me);\n+\tread_and_refresh_cache(me, opts);\n \n-\tprepare_revs(&revs);\n+\tprepare_revs(&revs, opts);\n \n \twhile ((commit = get_revision(&revs))) {\n-\t\tint res = do_pick_commit(commit);\n+\t\tint res = do_pick_commit(commit, opts);\n \t\tif (res)\n \t\t\treturn res;\n \t}\n@@ -585,10 +599,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 \tint res = 0;\n+\tstruct replay_opts opts;\n+\n+\tmemset(&opts, 0, sizeof(struct replay_opts));\n \tif (isatty(0))\n-\t\tedit = 1;\n-\taction = REVERT;\n-\tres = revert_or_cherry_pick(argc, argv);\n+\t\topts.edit = 1;\n+\topts.action = REVERT;\n+\tres = revert_or_cherry_pick(argc, argv, &opts);\n \tif (res > 0)\n \t\t/* Exit status from conflict */\n \t\treturn res;\n@@ -601,8 +618,11 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n \tint res = 0;\n-\taction = CHERRY_PICK;\n-\tres = revert_or_cherry_pick(argc, argv);\n+\tstruct replay_opts opts;\n+\n+\tmemset(&opts, 0, sizeof(struct replay_opts));\n+\topts.action = CHERRY_PICK;\n+\tres = revert_or_cherry_pick(argc, argv, &opts);\n \tif (res > 0)\n \t\treturn res;\n \tif (res < 0)\n-- \n1.7.5.GIT\n"},{"id":"168652","messageId":"1306333025-29893-7-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 06/10] revert: Separate cmdline parsing from functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:17:01Z","receivedAt":"2011-05-25T14:17:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Make cmd_cherry_pick and cmd_revert call parse_args and parse all the\ncommand-line arguments into an opts structure before sending it off to\na simplified pick_commits function. pick_commits, in turn will set up\nthe revision walker and call do_pick_commit in a loop- later in the\nseries, it will serve as the starting point for continuation.\n\nBased-on-patch-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 17 +++++++++--------\n 1 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0d6a5a9..9a612c6 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -561,17 +561,12 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n \trollback_lock_file(&index_lock);\n }\n \n-static int revert_or_cherry_pick(int argc, const char **argv,\n-\t\t\t\tstruct replay_opts *opts)\n+static int pick_commits(struct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n \n-\tgit_config(git_default_config, NULL);\n-\tme = opts->action == REVERT ? \"revert\" : \"cherry-pick\";\n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tparse_args(argc, argv, opts);\n-\n \tif (opts->allow_ff) {\n \t\tif (opts->signoff)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n@@ -605,7 +600,10 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tif (isatty(0))\n \t\topts.edit = 1;\n \topts.action = REVERT;\n-\tres = revert_or_cherry_pick(argc, argv, &opts);\n+\tgit_config(git_default_config, NULL);\n+\tme = \"revert\";\n+\tparse_args(argc, argv, &opts);\n+\tres = pick_commits(&opts);\n \tif (res > 0)\n \t\t/* Exit status from conflict */\n \t\treturn res;\n@@ -622,7 +620,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \n \tmemset(&opts, 0, sizeof(struct replay_opts));\n \topts.action = CHERRY_PICK;\n-\tres = revert_or_cherry_pick(argc, argv, &opts);\n+\tgit_config(git_default_config, NULL);\n+\tme = \"cherry-pick\";\n+\tparse_args(argc, argv, &opts);\n+\tres = pick_commits(&opts);\n \tif (res > 0)\n \t\treturn res;\n \tif (res < 0)\n-- \n1.7.5.GIT\n"},{"id":"168657","messageId":"1306333025-29893-8-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 07/10] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:17:02Z","receivedAt":"2011-05-25T14:17:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Earlier, incompatible command-line options used to be caught in\npick_commits after parse_args has parsed the options and populated the\noptions structure; a lot of unncessary work has already been done, and\nsignificant amount of cleanup is required to die at this stage.\nInstead, hand over this responsibility to parse_args so that the\nprogram can die early. Also write a die_opt_incompabile function to\nhandle incompatible options in a general manner; it will be used more\nextensively as more command-line options are introduced later in the\nseries.\n\nHelped-by: Junio C Hamano \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 34 +++++++++++++++++++++++-----------\n 1 files changed, 23 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 9a612c6..d21af96 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -80,6 +80,22 @@ static int option_parse_x(const struct option *opt,\n \treturn 0;\n }\n \n+static void verify_opt_compatible(const char *me, const char *base_opt, ...)\n+{\n+\tconst char *this_opt;\n+\tva_list ap;\n+\tint set;\n+\n+\tva_start(ap, base_opt);\n+\twhile ((this_opt = va_arg(ap, const char *))) {\n+\t\tset = va_arg(ap, int);\n+\t\tif (set)\n+\t\t\tdie(_(\"%s: %s cannot be used with %s\"),\n+\t\t\t\tme, this_opt, base_opt);\n+\t}\n+\tva_end(ap);\n+}\n+\n static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n@@ -116,6 +132,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \tif (opts->commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n+\tif (opts->allow_ff)\n+\t\tverify_opt_compatible(me, \"--ff\",\n+\t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\"--no-commit\", opts->no_commit,\n+\t\t\t\t\"-x\", opts->record_origin,\n+\t\t\t\t\"--edit\", opts->edit,\n+\t\t\t\tNULL);\n \topts->commit_argv = argv;\n }\n \n@@ -567,17 +590,6 @@ static int pick_commits(struct replay_opts *opts)\n \tstruct commit *commit;\n \n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tif (opts->allow_ff) {\n-\t\tif (opts->signoff)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n-\t\tif (opts->no_commit)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (opts->record_origin)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n-\t\tif (opts->edit)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n-\t}\n-\n \tread_and_refresh_cache(me, opts);\n \n \tprepare_revs(&revs, opts);\n-- \n1.7.5.GIT\n"},{"id":"168651","messageId":"1306333025-29893-9-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:17:03Z","receivedAt":"2011-05-25T14:17:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than\none commit, 2010-06-02), a single invocation of \"git cherry-pick\" or\n\"git revert\" can perform picks of several individual commits. To\nallow \"git cherry-pick --abort\" to cancel and \"git cherry-pick\n--continue\" to resume the entire command, we will need to store some\ninformation about the state and the plan at the beginning. Introduce\na HEAD file to store this state, and TODO file to store the plan.\n\nHelped-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 73 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex d21af96..4741252 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,12 @@\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+\n static const char * const revert_usage[] = {\n \t\"git revert [options] \",\n \tNULL\n@@ -584,22 +591,85 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n \trollback_lock_file(&index_lock);\n }\n \n+static void format_todo(struct strbuf *buf, struct commit_list *list,\n+\t\t\tstruct replay_opts *opts)\n+{\n+\tstruct commit_list *cur = NULL;\n+\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n+\tconst char *sha1_abbrev = NULL;\n+\tconst char *action;\n+\n+\taction = (opts->action == REVERT ? \"revert\" : \"pick\");\n+\tfor (cur = list; cur; cur = cur->next) {\n+\t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n+\t\tif (get_message(sha1_abbrev, cur->item->buffer, &msg))\n+\t\t\tdie(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n+\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action, sha1_abbrev, msg.subject);\n+\t}\n+}\n+\n+static void persist_head(unsigned char *head)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct lock_file head_lock;\n+\tint fd;\n+\n+\tif (file_exists(SEQ_PATH)) {\n+\t\tif (!is_directory(SEQ_PATH) && remove_path(SEQ_PATH) < 0) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\tdie(_(\"Could not remove %s\"), SEQ_PATH);\n+\t\t}\n+\t}\n+\tif (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), SEQ_PATH);\n+\t}\n+\n+\tfd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);\n+\tstrbuf_addf(&buf, \"%s\", head);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), HEAD_FILE);\n+\tif (commit_lock_file(&head_lock) < 0)\n+\t\tdie(_(\"Error wrapping up %s\"), HEAD_FILE);\n+}\n+\n+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct lock_file todo_lock;\n+\tint fd;\n+\n+\tfd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);\n+\tformat_todo(&buf, todo_list, opts);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), TODO_FILE);\n+\tif (commit_lock_file(&todo_lock) < 0)\n+\t\tdie(_(\"Error wrapping up %s\"), TODO_FILE);\n+}\n+\n static int pick_commits(struct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n+\tunsigned char head[20];\n \n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tread_and_refresh_cache(me, opts);\n \n+\tif (get_sha1(\"HEAD\", head))\n+\t\treturn error(_(\"You do not have a valid HEAD\"));\n+\n+\tread_and_refresh_cache(me, opts);\n \tprepare_revs(&revs, opts);\n+\tpersist_head(head);\n \n \twhile ((commit = get_revision(&revs))) {\n \t\tint res = do_pick_commit(commit, opts);\n-\t\tif (res)\n+\t\tif (res) {\n+\t\t\tcommit_list_insert(commit, &revs.commits);\n+\t\t\tpersist_todo(revs.commits, opts);\n \t\t\treturn res;\n+\t\t}\n \t}\n-\n \treturn 0;\n }\n \n-- \n1.7.5.GIT\n"},{"id":"168655","messageId":"1306333025-29893-10-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 09/10] revert: Implement parsing --continue, --abort and --skip","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:17:04Z","receivedAt":"2011-05-25T14:17:04Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce three new command-line options: --continue, --abort, and\n--skip resembling the correspoding options in \"rebase -i\". For now,\njust parse the options into the replay_opts structure, making sure\nthat two of them are not specified together. They will actually be\nimplemented later in the series.\n\nMentored-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 70 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 4741252..4607676 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -47,6 +47,11 @@ static const char *me;\n struct replay_opts {\n \tenum { REVERT, CHERRY_PICK } action;\n \n+\t/* --abort, --skip, and --continue */\n+\tint abort_oper;\n+\tint skip_oper;\n+\tint continue_oper;\n+\n \t/* Boolean options */\n \tint edit;\n \tint record_origin;\n@@ -103,11 +108,36 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)\n \tva_end(ap);\n }\n \n+static void verify_opt_mutually_compatible(const char *me, ...)\n+{\n+\tconst char *opt1, *opt2;\n+\tva_list ap;\n+\tint set;\n+\n+\tva_start(ap, me);\n+\twhile ((opt1 = va_arg(ap, const char *))) {\n+\t\tset = va_arg(ap, int);\n+\t\tif (set)\n+\t\t\tbreak;\n+\t}\n+\tif (!opt1) return;\n+\twhile ((opt2 = va_arg(ap, const char *))) {\n+\t\tset = va_arg(ap, int);\n+\t\tif (set)\n+\t\t\tdie(_(\"%s: %s cannot be used with %s\"),\n+\t\t\t\tme, opt1, opt2);\n+\t}\n+\tva_end(ap);\n+}\n+\n static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tint noop;\n \tstruct option options[] = {\n+\t\tOPT_BOOLEAN(0, \"abort\", &opts->abort_oper, \"abort the current operation\"),\n+\t\tOPT_BOOLEAN(0, \"skip\", &opts->skip_oper, \"skip the current commit\"),\n+\t\tOPT_BOOLEAN(0, \"continue\", &opts->continue_oper, \"continue the current operation\"),\n \t\tOPT_BOOLEAN('n', \"no-commit\", &opts->no_commit, \"don't automatically commit\"),\n \t\tOPT_BOOLEAN('e', \"edit\", &opts->edit, \"edit the commit message\"),\n \t\t{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, \"no-op (backward compatibility)\",\n@@ -136,7 +166,37 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \topts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,\n \t\t\t\t\tPARSE_OPT_KEEP_ARGV0 |\n \t\t\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n-\tif (opts->commit_argc < 2)\n+\n+\t/* Check for mutually incompatible command line arguments */\n+\tverify_opt_mutually_compatible(me,\n+\t\t\t\t\"--abort\", opts->abort_oper,\n+\t\t\t\t\"--skip\", opts->skip_oper,\n+\t\t\t\t\"--continue\", opts->continue_oper,\n+\t\t\t\tNULL);\n+\n+\t/* Check for incompatible command line arguments */\n+\tif (opts->abort_oper || opts->skip_oper || opts->continue_oper) {\n+\t\tchar *this_oper;\n+\t\tif (opts->abort_oper)\n+\t\t\tthis_oper = \"--abort\";\n+\t\telse if (opts->skip_oper)\n+\t\t\tthis_oper = \"--skip\";\n+\t\telse\n+\t\t\tthis_oper = \"--continue\";\n+\n+\t\tverify_opt_compatible(me, this_oper,\n+\t\t\t\t\"--no-commit\", opts->no_commit,\n+\t\t\t\t\"--edit\", opts->edit,\n+\t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\"--mainline\", opts->mainline,\n+\t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n+\t\t\t\t\"--strategy-option\", opts->xopts ? 1 : 0,\n+\t\t\t\t\"-x\", opts->record_origin,\n+\t\t\t\t\"--ff\", opts->allow_ff,\n+\t\t\t\tNULL);\n+\t}\n+\n+\telse if (opts->commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n \tif (opts->allow_ff)\n@@ -146,6 +206,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"-x\", opts->record_origin,\n \t\t\t\t\"--edit\", opts->edit,\n \t\t\t\tNULL);\n+\n+\t/* Remove these when the options are actually implemented */\n+\tif (opts->abort_oper)\n+\t\tdie(\"--abort is not implemented yet\");\n+\tif (opts->skip_oper)\n+\t\tdie(\"--skip is not implemented yet\");\n+\tif (opts->continue_oper)\n+\t\tdie(\"--continue is not implemented yet\");\n+\n \topts->commit_argv = argv;\n }\n \n-- \n1.7.5.GIT\n"},{"id":"168653","messageId":"1306333025-29893-11-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 10/10] revert: Implement --abort processing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-25T14:17:05Z","receivedAt":"2011-05-25T14:17:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"To abort, perform a \"rerere clear\" and \"reset --hard\" to the ref\nspecified by the HEAD file introduced earlier in the series.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 46 ++++++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 42 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 4607676..6a93175 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -208,8 +208,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\tNULL);\n \n \t/* Remove these when the options are actually implemented */\n-\tif (opts->abort_oper)\n-\t\tdie(\"--abort is not implemented yet\");\n \tif (opts->skip_oper)\n \t\tdie(\"--skip is not implemented yet\");\n \tif (opts->continue_oper)\n@@ -742,6 +740,46 @@ static int pick_commits(struct replay_opts *opts)\n \treturn 0;\n }\n \n+static int process_continuation(struct replay_opts *opts)\n+{\n+\tif (opts->abort_oper) {\n+\t\tchar head[DEFAULT_ABBREV];\n+\t\tunsigned char sha1[20];\n+\t\tint fd;\n+\t\trerere_clear(0);\n+\n+\t\tif (!file_exists(HEAD_FILE))\n+\t\t\tgoto error;\n+\t\tfd = open(HEAD_FILE, O_RDONLY, 0666);\n+\t\tif (fd < 0)\n+\t\t\treturn error(_(\"Could not open '%s' for reading: %s\"),\n+\t\t\t\tHEAD_FILE, strerror(errno));\n+\t\tif (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV) {\n+\t\t\tclose(fd);\n+\t\t\treturn error(_(\"Corrupt '%s': %s\"), HEAD_FILE, strerror(errno));\n+\t\t}\n+\t\tclose(fd);\n+\n+\t\tif (get_sha1(head, sha1))\n+\t\t\treturn error(_(\"Failed to resolve '%s' as a valid ref.\"), head);\n+\t\tupdate_ref(NULL, \"HEAD\", sha1, NULL, 0, MSG_ON_ERR);\n+\t}\n+\telse if (opts->skip_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\treturn 0;\n+\t}\n+\telse if (opts->continue_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\treturn 0;\n+\t}\n+\n+\treturn pick_commits(opts);\n+error:\n+\treturn error(_(\"No %s in progress\"), me);\n+}\n+\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tint res = 0;\n@@ -754,7 +792,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tme = \"revert\";\n \tparse_args(argc, argv, &opts);\n-\tres = pick_commits(&opts);\n+\tres = process_continuation(&opts);\n \tif (res > 0)\n \t\t/* Exit status from conflict */\n \t\treturn res;\n@@ -774,7 +812,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tme = \"cherry-pick\";\n \tparse_args(argc, argv, &opts);\n-\tres = pick_commits(&opts);\n+\tres = process_continuation(&opts);\n \tif (res > 0)\n \t\treturn res;\n \tif (res < 0)\n-- \n1.7.5.GIT\n"},{"id":"168724","messageId":"7v62oy1juh.fsf@alter.siamese.dyndns.org","threadId":"27453","inReplyTo":"1306333025-29893-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-25T22:44:22Z","receivedAt":"2011-05-25T22:44:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> +static int error_dirty_index(const char *me)\n> {\n> +\tif (read_cache_unmerged())\n> +\t\treturn error_resolve_conflict(me);\n> +\n> +\tint ret = error(_(\"Your local changes would be overwritten by %s.\\n\"), me);\n> +\tif (advice_commit_before_merge)\n> +\t\tadvise(_(\"Please, commit your changes or stash them to proceed.\"));\n> +\treturn ret;\n> }\n\nI like this rewrite whose result is short-and-sweet, but you do not even\nneed the \"ret\" variable. error() always yields -1, no?\n\n> @@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n> \n> int cmd_revert(int argc, const char **argv, const char *prefix)\n> {\n> +\tint res = 0;\n> \tif (isatty(0))\n> \t\tedit = 1;\n> \taction = REVERT;\n> -\treturn revert_or_cherry_pick(argc, argv);\n> +\tres = revert_or_cherry_pick(argc, argv);\n> +\tif (res > 0)\n> +\t\t/* Exit status from conflict */\n> +\t\treturn res;\n> +\tif (res < 0)\n> +\t\t/* Other error */\n> +\t\texit(128);\n> +\treturn 0;\n> }\n> \n> int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n> {\n> +\tint res = 0;\n> \taction = CHERRY_PICK;\n> -\treturn revert_or_cherry_pick(argc, argv);\n> +\tres = revert_or_cherry_pick(argc, argv);\n> +\tif (res > 0)\n> +\t\treturn res;\n> +\tif (res < 0)\n> +\t\texit(128);\n> +\treturn 0;\n> }\n\nThis hunk is dubious.\n\n - Why initialize res to zero if it always is assigned the return value of\n revert_or_cherry_pick() before it is used?\n\n - The called function seems to return errors from various places but as\n far as I see they are all return value of error(), so it would be\n equivalent to\n\n\tif (r_o_c_p(...))\n\t\texit(128);\n\treturn 0;\n\nIf you are going to introduce different return values from r-o-c-p() in a\nlater patch, these functions should be updated in that patch, I think.\n"},{"id":"168726","messageId":"7v1uzm1im7.fsf@alter.siamese.dyndns.org","threadId":"27453","inReplyTo":"1306333025-29893-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 03/10] revert: Eliminate global \"commit\" variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-25T23:10:56Z","receivedAt":"2011-05-25T23:10:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 523d41a..6c485f6 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -37,7 +37,6 @@ 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\nI agree that the stated goal of this change is a worthy one.\n\n> @@ -116,11 +115,12 @@ 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(const char *sha1_abbrev, const char *raw_message,\n> +\t\tstruct commit_message *out)\n> {\n\nThis somehow feels dubious and also goes against the \"let's not rely on\nglobal variable commit\", no?\n\nThis function relied on the global variable and also got information\npassed from the caller that this function could have derived from that\nglobal commit object (i.e. raw_message came from commit->buffer).\n\nA logical thing to fix that situation would be to make its signature:\n\n static int get_message(struct commit *commit, struct commit_message *out)\n\nno?\n\nEspecially dubious is this part:\n\n> @@ -139,17 +139,14 @@ static int get_message(const char *raw_message, struct commit_message *out)\n> \tif (out->reencoded_message)\n> \t\tout->message = out->reencoded_message;\n> \n> -\tabbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n> -\tabbrev_len = strlen(abbrev);\n> -\n> \tsubject_len = find_commit_subject(out->message, &subject);\n> \n> -\tout->parent_label = xmalloc(strlen(\"parent of \") + abbrev_len +\n> +\tout->parent_label = xmalloc(strlen(\"parent of \") + DEFAULT_ABBREV +\n> \t\t\t strlen(\"... \") + subject_len + 1);\n\nYou moved the call to f-u-a() to the caller, and that is why you have to\npass its return value as an extra parameter. If you passed commit you do\nnot have to.\n\nWorse yet, the above xmalloc() and memcpy below are wrong, as you are no\nlonger taking the actual length of the abbreviated object name into\naccount, like the old code did. f-u-a() is _about_ uniqueness, and you\ncan get a string longer than the minimum length you gave to it.\n\n> \tq = out->parent_label;\n> \tq = mempcpy(q, \"parent of \", strlen(\"parent of \"));\n> \tout->label = q;\n> -\tq = mempcpy(q, abbrev, abbrev_len);\n> +\tq = mempcpy(q, sha1_abbrev, DEFAULT_ABBREV);\n> \tq = mempcpy(q, \"... \", strlen(\"... \"));\n> \tout->subject = q;\n> \tq = mempcpy(q, subject, subject_len);\n\n\n> @@ -168,8 +165,7 @@ static char *get_encoding(const char *message)\n> \tconst char *p = message, *eol;\n> \n> \tif (!p)\n> -\t\tdie (_(\"Could not read commit message of %s\"),\n> -\t\t\t\tsha1_to_hex(commit->object.sha1));\n> +\t\tdie (_(\"BUG: get_encoding() called with NULL\"));\n\nHmm, do we really want to translate this error message?\n\nAs the function is file-scope static, I suspect that calling it with NULL\nis a programmer error, not data error (i.e. it is not like we might get an\nobject with NULL here in some repository created by a different version or\nimplementation of git). Once you got confident (and I hope you will be\nbefore this series hits our tree) that no caller calls this function with\nNULL, we might want to demote this to assert(p) or even remove that if\nstatement altogether.\n\n> @@ -185,25 +181,26 @@ 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(const char *fallback, struct strbuf *msgbuf,\n> +\t\t\tconst char *message)\n> {\n> \tconst char *p = message;\n> \twhile (*p && (*p != '\\n' || p[1] != '\\n'))\n> \t\tp++;\n> \n> \tif (!*p)\n> -\t\tstrbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));\n> +\t\tstrbuf_addstr(msgbuf, fallback);\n\nCan you explain what the above loop and if statements are doing for me?\nIt seems to be doing something a lot more than what the name of the\nfunction suggests, which is a bad sign that the helper is not designed\nproperly with clear semantics in mind. I would probably prefer to see\nthis function removed, and do this kind of thing in the only caller of\nthis confused helper function. You could make an even smaller helper\nfunction that implements the logic to determine if there are two\nconsecutive LFs in a string (seen above), but I suspect that would be just\nthe matter of strstr(message, \"\\n\\n\")\n\n> -static void write_cherry_pick_head(void)\n> +static void write_cherry_pick_head(const char *commit_sha1_hex)\n> {\n> \tint fd;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> -\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n> +\tstrbuf_addf(&buf, \"%s\\n\", commit_sha1_hex);\n> \n> \tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n> \tif (fd < 0)\n> @@ -370,7 +367,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> @@ -378,6 +375,7 @@ static int do_pick_commit(void)\n> \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n> \tchar *defmsg = NULL;\n> \tstruct strbuf msgbuf = STRBUF_INIT;\n> +\tconst char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n\nGiven the lifetime rule of f-u-a return value, I think it is a bad design\nto call it and pass it as an extra parameter to get_message() much later.\n"},{"id":"168727","messageId":"7vwrhez81n.fsf@alter.siamese.dyndns.org","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v3 00/10] Sequencer Foundations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-25T23:15:16Z","receivedAt":"2011-05-25T23:15:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> I hope there are no major issues with this iteration.\n\nI've scanned up to 07/10; except for the internal API issues in 03/10\noverall you seem to be going in the right direction.\n"},{"id":"168757","messageId":"BANLkTi==NG2mTARtVC4vBD8BebMpdRv6tg@mail.gmail.com","threadId":"27453","inReplyTo":"7v62oy1juh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T09:34:16Z","receivedAt":"2011-05-26T09:34:16Z","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>> +static int error_dirty_index(const char *me)\n>>  {\n>> +     if (read_cache_unmerged())\n>> +             return error_resolve_conflict(me);\n>> +\n>> +     int ret = error(_(\"Your local changes would be overwritten by %s.\\n\"), me);\n>> +     if (advice_commit_before_merge)\n>> +             advise(_(\"Please, commit your changes or stash them to proceed.\"));\n>> +     return ret;\n>>  }\n>\n> I like this rewrite whose result is short-and-sweet, but you do not even\n> need the \"ret\" variable. error() always yields -1, no?\n\nOkay; I didn't do this in the first place because I thought it would\nbe inelegant to hardcode '-1'. Fixed anyway.\n\n>> @@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n>>\n>>  int cmd_revert(int argc, const char **argv, const char *prefix)\n>>  {\n>> +     int res = 0;\n>>       if (isatty(0))\n>>               edit = 1;\n>>       action = REVERT;\n>> -     return revert_or_cherry_pick(argc, argv);\n>> +     res = revert_or_cherry_pick(argc, argv);\n>> +     if (res > 0)\n>> +             /* Exit status from conflict */\n>> +             return res;\n>> +     if (res < 0)\n>> +             /* Other error */\n>> +             exit(128);\n>> +     return 0;\n>>  }\n>>\n>>  int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n>>  {\n>> +     int res = 0;\n>>       action = CHERRY_PICK;\n>> -     return revert_or_cherry_pick(argc, argv);\n>> +     res = revert_or_cherry_pick(argc, argv);\n>> +     if (res > 0)\n>> +             return res;\n>> +     if (res < 0)\n>> +             exit(128);\n>> +     return 0;\n>>  }\n>\n> This hunk is dubious.\n>\n>  - Why initialize res to zero if it always is assigned the return value of\n>   revert_or_cherry_pick() before it is used?\n\nOkay. Fixed.\n\n>  - The called function seems to return errors from various places but as\n>   far as I see they are all return value of error(), so it would be\n>   equivalent to\n>\n>        if (r_o_c_p(...))\n>                exit(128);\n>        return 0;\n>\n> If you are going to introduce different return values from r-o-c-p() in a\n> later patch, these functions should be updated in that patch, I think.\n\nrevert_or_cherry_pick *does* return different values in this patch! As\nI've pointed out in the comment, positive exit status indicates a\nconflict, while a negative one indicates an error. To prove to myself\nthat this is case, I applied this diff temporarily and ran all tests\n-- and viola, t3505-cherry-pick-empty.sh broke. Is there something I'm\nnot understanding correctly?\n\nThanks for the review.\n\nSigned-off-by: Ramkumar Ramachandra \n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 523d41a..9c7921b 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -584,28 +584,22 @@ static int revert_or_cherry_pick(int argc, const\nchar **argv)\n\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n-\tint res = 0;\n+\tint res;\n \tif (isatty(0))\n \t\tedit = 1;\n \taction = REVERT;\n \tres = revert_or_cherry_pick(argc, argv);\n-\tif (res > 0)\n-\t\t/* Exit status from conflict */\n-\t\treturn res;\n-\tif (res < 0)\n-\t\t/* Other error */\n+\tif (res)\n \t\texit(128);\n \treturn 0;\n }\n\n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n-\tint res = 0;\n+\tint res;\n \taction = CHERRY_PICK;\n \tres = revert_or_cherry_pick(argc, argv);\n-\tif (res > 0)\n-\t\treturn res;\n-\tif (res < 0)\n+\tif (res)\n \t\texit(128);\n \treturn 0;\n }\n"},{"id":"168780","messageId":"1306425233-504-1-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306333025-29893-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 00/10] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:43Z","receivedAt":"2011-05-26T15:53:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThis is a quick re-roll after Junio's review of the previous\niteration; the previous iteration was posted less than 24 hours ago.\nI'm in a hurry, because I want to make sure that this is ready (or\nnearly ready) for merge before I actually implement more features.\n\nThanks for reading.\n\nRamkumar Ramachandra (10):\n advice: Introduce error_resolve_conflict\n revert: Propogate errors upwards from do_pick_commit\n revert: Eliminate global \"commit\" variable\n revert: Rename no_replay to record_origin\n revert: Introduce struct to keep command-line options\n revert: Separate cmdline parsing from functional code\n revert: Catch incompatible command-line options early\n revert: Introduce HEAD, TODO files to persist state, plan\n revert: Implement parsing --continue, --abort and --skip\n revert: Implement --abort processing\n\n advice.c | 17 ++-\n advice.h | 1 +\n builtin/revert.c | 492 ++++++++++++++++++++++++++++++++++++++----------------\n 3 files changed, 360 insertions(+), 150 deletions(-)\n\n-- \n1.7.5.GIT\n"},{"id":"168781","messageId":"1306425233-504-2-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 01/10] advice: Introduce error_resolve_conflict","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:44Z","receivedAt":"2011-05-26T15:53:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce error_resolve_conflict corresponding to\ndie_resolve_conflict, and implement the latter function in terms of\nthe former.\n\nBased-on-patch-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n advice.c | 17 +++++++++++------\n advice.h | 1 +\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 0be4b5f..80aaa14 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -34,16 +34,21 @@ int git_default_advice_config(const char *var, const char *value)\n \treturn 0;\n }\n \n-void NORETURN die_resolve_conflict(const char *me)\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\tdie(\"'%s' is not possible because you have unmerged files.\\n\"\n-\t\t \"Please, fix them up in the work tree, and then use 'git add/rm ' as\\n\"\n-\t\t \"appropriate to mark resolution and make a commit, or use 'git commit -a'.\", me);\n-\telse\n-\t\tdie(\"'%s' is not possible because you have unmerged files.\", me);\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+\treturn error(\"'%s' is not possible because you have unmerged files.\", me);\n+}\n+\n+void NORETURN die_resolve_conflict(const char *me)\n+{\n+\terror_resolve_conflict(me);\n+\texit(128);\n }\ndiff --git a/advice.h b/advice.h\nindex 3244ebb..f537366 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -12,6 +12,7 @@ extern int advice_detached_head;\n \n int git_default_advice_config(const char *var, const char *value);\n \n+int error_resolve_conflict(const char *me);\n extern void NORETURN die_resolve_conflict(const char *me);\n \n #endif /* ADVICE_H */\n-- \n1.7.5.GIT\n"},{"id":"168787","messageId":"1306425233-504-3-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 02/10] revert: Propogate errors upwards from do_pick_commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:45Z","receivedAt":"2011-05-26T15:53:45Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce error_dirty_index, based on die_dirty_index which prints\nsome hints, and returns an error to be handled by its caller,\ndo_pick_commit. do_pick_commit, in turn, propogates the error\nupwards, and the error is finally handled by cmd_cherry_pick and\ncmd_revert. Set a simple convention for error-handling as follows:\npositive return values from do_pick_commit indicate conflicts, while\nnegative ones indicate other errors.\n\nMentored-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 72 ++++++++++++++++++++++++++++-------------------------\n 1 files changed, 38 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex f697e66..ff26856 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -266,25 +266,15 @@ 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\t\t \"Please, commit your changes or stash them to proceed.\"));\n-\t\t\telse\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by cherry-pick.\\n\"\n-\t\t\t\t\t \"Please, commit your changes or stash them to proceed.\"));\n-\t\t} else {\n-\t\t\tif (action == REVERT)\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by revert.\\n\"));\n-\t\t\telse\n-\t\t\t\tdie(_(\"Your local changes would be overwritten by cherry-pick.\\n\"));\n-\t\t}\n-\t}\n+\tif (read_cache_unmerged())\n+\t\treturn error_resolve_conflict(me);\n+\n+\terror(_(\"Your local changes would be overwritten by %s.\\n\"), me);\n+\tif (advice_commit_before_merge)\n+\t\tadvise(_(\"Please, commit your changes or stash them to proceed.\"));\n+\treturn -1;\n }\n \n static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n@@ -398,18 +388,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@@ -418,20 +408,20 @@ static int do_pick_commit(void)\n \t\tstruct commit_list *p;\n \n \t\tif (!mainline)\n-\t\t\tdie(_(\"Commit %s is a merge but no -m option was given.\"),\n-\t\t\t sha1_to_hex(commit->object.sha1));\n+\t\t\treturn error(_(\"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\t sha1_to_hex(commit->object.sha1), mainline);\n+\t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n+\t\t\t\tsha1_to_hex(commit->object.sha1), mainline);\n \t\tparent = p->item;\n \t} else if (0 < mainline)\n-\t\tdie(_(\"Mainline was specified but commit %s is not a merge.\"),\n-\t\t sha1_to_hex(commit->object.sha1));\n+\t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n \telse\n \t\tparent = commit->parents->item;\n \n@@ -441,12 +431,12 @@ static int do_pick_commit(void)\n \tif (parent && parse_commit(parent) < 0)\n \t\t/* TRANSLATORS: The first %s will be \"revert\" or\n \t\t \"cherry-pick\", the second %s a SHA1 */\n-\t\tdie(_(\"%s: cannot parse parent commit %s\"),\n-\t\t me, sha1_to_hex(parent->object.sha1));\n+\t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n+\t\t\tme, sha1_to_hex(parent->object.sha1));\n \n \tif (get_message(commit->buffer, &msg) != 0)\n-\t\tdie(_(\"Cannot get commit message for %s\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n+\t\treturn error(_(\"Cannot get commit message for %s\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n \n \t/*\n \t * \"commit\" is an existing commit. We would want to apply\n@@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n+\tint res;\n \tif (isatty(0))\n \t\tedit = 1;\n \taction = REVERT;\n-\treturn revert_or_cherry_pick(argc, argv);\n+\tres = revert_or_cherry_pick(argc, argv);\n+\tif (res > 0)\n+\t\t/* Exit status from conflict */\n+\t\treturn res;\n+\tif (res < 0)\n+\t\t/* Other error */\n+\t\texit(128);\n+\treturn 0;\n }\n \n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n+\tint res;\n \taction = CHERRY_PICK;\n-\treturn revert_or_cherry_pick(argc, argv);\n+\tres = revert_or_cherry_pick(argc, argv);\n+\tif (res > 0)\n+\t\treturn res;\n+\tif (res < 0)\n+\t\texit(128);\n+\treturn 0;\n }\n-- \n1.7.5.GIT\n"},{"id":"168788","messageId":"1306425233-504-4-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 03/10] revert: Eliminate global \"commit\" variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:46Z","receivedAt":"2011-05-26T15:53:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Since we want to develop the functionality to either pick or revert\nindividual commits atomically later in the series, make \"commit\" a\nlocal variable. Modify the get_message API to take \"commit\" as an\nargument, factor out add_message_to_msg, and don't check the sole\nargument in get_encoding.\n\nMentored-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 47 ++++++++++++++++++-----------------------------\n 1 files changed, 18 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex ff26856..745295d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -37,7 +37,6 @@ 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@@ -116,25 +115,25 @@ 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, struct commit_message *out)\n {\n \tconst char *encoding;\n \tconst char *abbrev, *subject;\n \tint abbrev_len, subject_len;\n \tchar *q;\n \n-\tif (!raw_message)\n+\tif (!commit->buffer)\n \t\treturn -1;\n-\tencoding = get_encoding(raw_message);\n+\tencoding = get_encoding(commit->buffer);\n \tif (!encoding)\n \t\tencoding = \"UTF-8\";\n \tif (!git_commit_encoding)\n \t\tgit_commit_encoding = \"UTF-8\";\n \n \tout->reencoded_message = NULL;\n-\tout->message = raw_message;\n+\tout->message = commit->buffer;\n \tif (strcmp(encoding, git_commit_encoding))\n-\t\tout->reencoded_message = reencode_string(raw_message,\n+\t\tout->reencoded_message = reencode_string(commit->buffer,\n \t\t\t\t\tgit_commit_encoding, encoding);\n \tif (out->reencoded_message)\n \t\tout->message = out->reencoded_message;\n@@ -167,9 +166,6 @@ static char *get_encoding(const char *message)\n {\n \tconst char *p = message, *eol;\n \n-\tif (!p)\n-\t\tdie (_(\"Could not read commit message of %s\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n \twhile (*p && *p != '\\n') {\n \t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n \t\t\t; /* do nothing */\n@@ -185,25 +181,12 @@ 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-{\n-\tconst char *p = message;\n-\twhile (*p && (*p != '\\n' || p[1] != '\\n'))\n-\t\tp++;\n-\n-\tif (!*p)\n-\t\tstrbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));\n-\n-\tp += 2;\n-\tstrbuf_addstr(msgbuf, p);\n-}\n-\n-static void write_cherry_pick_head(void)\n+static void write_cherry_pick_head(const char *commit_sha1_hex)\n {\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n+\tstrbuf_addf(&buf, \"%s\\n\", commit_sha1_hex);\n \n \tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n@@ -370,7 +353,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@@ -434,7 +417,7 @@ static int do_pick_commit(void)\n \t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n \t\t\tme, sha1_to_hex(parent->object.sha1));\n \n-\tif (get_message(commit->buffer, &msg) != 0)\n+\tif (get_message(commit, &msg) != 0)\n \t\treturn error(_(\"Cannot get commit message for %s\"),\n \t\t\tsha1_to_hex(commit->object.sha1));\n \n@@ -467,14 +450,19 @@ 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+\n+\t\t/* Add msg.message to msgbuf */\n+\t\tconst char *p = strstr(msg.message, \"\\n\\n\");\n+\t\tp = p ? p + 2 : sha1_to_hex(commit->object.sha1);\n+\t\tstrbuf_addstr(&msgbuf, p);\n+\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(sha1_to_hex(commit->object.sha1));\n \t}\n \n \tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n@@ -552,6 +540,7 @@ static void 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 \n \tgit_config(git_default_config, NULL);\n \tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n@@ -574,7 +563,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \tprepare_revs(&revs);\n \n \twhile ((commit = get_revision(&revs))) {\n-\t\tint res = do_pick_commit();\n+\t\tint res = do_pick_commit(commit);\n \t\tif (res)\n \t\t\treturn res;\n \t}\n-- \n1.7.5.GIT\n"},{"id":"168783","messageId":"1306425233-504-5-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 04/10] revert: Rename no_replay to record_origin","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:47Z","receivedAt":"2011-05-26T15:53:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Give the variable \"no_replay\" a new name \"record_origin\", so that\nthere is no confusion when a \"replay\" structure is introduced later in\nthe series.\n\nMentored-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 745295d..e9b8533 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -35,7 +35,7 @@ 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 int edit, record_origin, 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@@ -90,7 +90,7 @@ static void parse_args(int argc, const char **argv)\n \n \tif (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('x', NULL, &record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &allow_ff, \"allow fast-forward\"),\n \t\t\tOPT_END(),\n \t\t};\n@@ -456,7 +456,7 @@ static int do_pick_commit(struct commit *commit)\n \t\tp = p ? p + 2 : sha1_to_hex(commit->object.sha1);\n \t\tstrbuf_addstr(&msgbuf, p);\n \n-\t\tif (no_replay) {\n+\t\tif (record_origin) {\n \t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n@@ -552,7 +552,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n \t\tif (no_commit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (no_replay)\n+\t\tif (record_origin)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n \t\tif (edit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n-- \n1.7.5.GIT\n"},{"id":"168790","messageId":"1306425233-504-6-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 05/10] revert: Introduce struct to keep command-line options","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:48Z","receivedAt":"2011-05-26T15:53:48Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The current code uses a set of file-scope static variables to tell the\ncherry-pick/ revert machinery how to replay the changes, and\ninitializes them by parsing the command-line arguments. In later\nsteps in this series, we would like to introduce an API function that\ncalls into this machinery directly and have a way to tell it what to\ndo. Hence, introduce a structure to group these variables, so that\nthe API can take them as a single replay_options parameter.\n\nHelped-by: Junio C Hamano \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 174 ++++++++++++++++++++++++++++++------------------------\n 1 files changed, 97 insertions(+), 77 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e9b8533..cc2fa73 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -35,76 +35,88 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-static int edit, record_origin, 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 const char *me;\n \n-/* Merge strategy. */\n-static const char *strategy;\n-static const char **xopts;\n-static size_t xopts_nr, xopts_alloc;\n+struct replay_opts {\n+\tenum { REVERT, CHERRY_PICK } action;\n+\n+\t/* Boolean options */\n+\tint edit;\n+\tint record_origin;\n+\tint no_commit;\n+\tint signoff;\n+\tint allow_ff;\n+\tint allow_rerere_auto;\n+\n+\tint mainline;\n+\tint commit_argc;\n+\tconst char **commit_argv;\n+\n+\t/* Merge strategy */\n+\tconst char *strategy;\n+\tconst char **xopts;\n+\tsize_t xopts_nr, xopts_alloc;\n+};\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n static char *get_encoding(const char *message);\n \n-static const char * const *revert_or_cherry_pick_usage(void)\n+static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n {\n-\treturn action == REVERT ? revert_usage : cherry_pick_usage;\n+\treturn opts->action == REVERT ? revert_usage : cherry_pick_usage;\n }\n \n static int option_parse_x(const struct option *opt,\n \t\t\t const char *arg, int unset)\n {\n+\tstruct replay_opts **opts = opt->value;\n+\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((*opts)->xopts, (*opts)->xopts_nr + 1, (*opts)->xopts_alloc);\n+\t(*opts)->xopts[(*opts)->xopts_nr++] = xstrdup(arg);\n \treturn 0;\n }\n \n-static void parse_args(int argc, const char **argv)\n+static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n-\tconst char * const * usage_str = revert_or_cherry_pick_usage();\n+\tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tint noop;\n \tstruct option options[] = {\n-\t\tOPT_BOOLEAN('n', \"no-commit\", &no_commit, \"don't automatically commit\"),\n-\t\tOPT_BOOLEAN('e', \"edit\", &edit, \"edit the commit message\"),\n+\t\tOPT_BOOLEAN('n', \"no-commit\", &opts->no_commit, \"don't automatically commit\"),\n+\t\tOPT_BOOLEAN('e', \"edit\", &opts->edit, \"edit the commit message\"),\n \t\t{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, \"no-op (backward compatibility)\",\n \t\t PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },\n-\t\tOPT_BOOLEAN('s', \"signoff\", &signoff, \"add Signed-off-by:\"),\n-\t\tOPT_INTEGER('m', \"mainline\", &mainline, \"parent number\"),\n-\t\tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n-\t\tOPT_STRING(0, \"strategy\", &strategy, \"strategy\", \"merge strategy\"),\n-\t\tOPT_CALLBACK('X', \"strategy-option\", &xopts, \"option\",\n+\t\tOPT_BOOLEAN('s', \"signoff\", &opts->signoff, \"add Signed-off-by:\"),\n+\t\tOPT_INTEGER('m', \"mainline\", &opts->mainline, \"parent number\"),\n+\t\tOPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),\n+\t\tOPT_STRING(0, \"strategy\", &opts->strategy, \"strategy\", \"merge strategy\"),\n+\t\tOPT_CALLBACK('X', \"strategy-option\", &opts, \"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 (opts->action == CHERRY_PICK) {\n \t\tstruct option cp_extra[] = {\n-\t\t\tOPT_BOOLEAN('x', NULL, &record_origin, \"append commit name\"),\n-\t\t\tOPT_BOOLEAN(0, \"ff\", &allow_ff, \"allow fast-forward\"),\n+\t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n+\t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n \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-\t\t\t\t PARSE_OPT_KEEP_ARGV0 |\n-\t\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n-\tif (commit_argc < 2)\n+\topts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,\n+\t\t\t\t\tPARSE_OPT_KEEP_ARGV0 |\n+\t\t\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n+\tif (opts->commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n-\tcommit_argv = argv;\n+\topts->commit_argv = argv;\n }\n \n struct commit_message {\n@@ -273,7 +285,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n \n static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\t\t const char *base_label, const char *next_label,\n-\t\t\t unsigned char *head, struct strbuf *msgbuf)\n+\t\t\t unsigned char *head, struct strbuf *msgbuf,\n+\t\t\t struct replay_opts *opts)\n {\n \tstruct merge_options o;\n \tstruct tree *result, *next_tree, *base_tree, *head_tree;\n@@ -294,7 +307,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tnext_tree = next ? next->tree : empty_tree();\n \tbase_tree = base ? base->tree : empty_tree();\n \n-\tfor (xopt = xopts; xopt != xopts + xopts_nr; xopt++)\n+\tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n \t\tparse_merge_opt(&o, *xopt);\n \n \tclean = merge_trees(&o,\n@@ -334,7 +347,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n * If we are revert, or if our cherry-pick results in a hand merge,\n * we had better say that the current user is responsible for that.\n */\n-static int run_git_commit(const char *defmsg)\n+static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n {\n \t/* 6 is max possible length of our args array including NULL */\n \tconst char *args[6];\n@@ -342,9 +355,9 @@ static int run_git_commit(const char *defmsg)\n \n \targs[i++] = \"commit\";\n \targs[i++] = \"-n\";\n-\tif (signoff)\n+\tif (opts->signoff)\n \t\targs[i++] = \"-s\";\n-\tif (!edit) {\n+\tif (!opts->edit) {\n \t\targs[i++] = \"-F\";\n \t\targs[i++] = defmsg;\n \t}\n@@ -353,7 +366,7 @@ static int run_git_commit(const char *defmsg)\n \treturn run_command_v_opt(args, RUN_GIT_CMD);\n }\n \n-static int do_pick_commit(struct commit *commit)\n+static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n {\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n@@ -363,7 +376,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 (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@@ -381,7 +394,7 @@ static int do_pick_commit(struct commit *commit)\n \tdiscard_cache();\n \n \tif (!commit->parents) {\n-\t\tif (action == REVERT)\n+\t\tif (opts->action == REVERT)\n \t\t\treturn error(_(\"Cannot revert a root commit\"));\n \t\tparent = NULL;\n \t}\n@@ -390,25 +403,25 @@ static int do_pick_commit(struct commit *commit)\n \t\tint cnt;\n \t\tstruct commit_list *p;\n \n-\t\tif (!mainline)\n+\t\tif (!opts->mainline)\n \t\t\treturn error(_(\"Commit %s is a merge but no -m option was given.\"),\n \t\t\t\tsha1_to_hex(commit->object.sha1));\n \n \t\tfor (cnt = 1, p = commit->parents;\n-\t\t cnt != mainline && p;\n+\t\t cnt != opts->mainline && p;\n \t\t cnt++)\n \t\t\tp = p->next;\n-\t\tif (cnt != mainline || !p)\n+\t\tif (cnt != opts->mainline || !p)\n \t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1), mainline);\n+\t\t\t\tsha1_to_hex(commit->object.sha1), opts->mainline);\n \t\tparent = p->item;\n-\t} else if (0 < mainline)\n+\t} else if (0 < opts->mainline)\n \t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n \t\t\tsha1_to_hex(commit->object.sha1));\n \telse\n \t\tparent = commit->parents->item;\n \n-\tif (allow_ff && parent && !hashcmp(parent->object.sha1, head))\n+\tif (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))\n \t\treturn fast_forward_to(commit->object.sha1, head);\n \n \tif (parent && parse_commit(parent) < 0)\n@@ -430,7 +443,7 @@ static int do_pick_commit(struct commit *commit)\n \n \tdefmsg = git_pathdup(\"MERGE_MSG\");\n \n-\tif (action == REVERT) {\n+\tif (opts->action == REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -456,18 +469,18 @@ static int do_pick_commit(struct commit *commit)\n \t\tp = p ? p + 2 : sha1_to_hex(commit->object.sha1);\n \t\tstrbuf_addstr(&msgbuf, p);\n \n-\t\tif (record_origin) {\n+\t\tif (opts->record_origin) {\n \t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n-\t\tif (!no_commit)\n+\t\tif (!opts->no_commit)\n \t\t\twrite_cherry_pick_head(sha1_to_hex(commit->object.sha1));\n \t}\n \n-\tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n+\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n-\t\t\t\t\t head, &msgbuf);\n+\t\t\t\t\t head, &msgbuf, opts);\n \t\twrite_message(&msgbuf, defmsg);\n \t} else {\n \t\tstruct commit_list *common = NULL;\n@@ -477,23 +490,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\t\t\t\tsha1_to_hex(head), remotes);\n+\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n+\t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n \t}\n \n \tif (res) {\n-\t\terror(action == REVERT\n+\t\terror(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(opts->allow_rerere_auto);\n \t} else {\n-\t\tif (!no_commit)\n-\t\t\tres = run_git_commit(defmsg);\n+\t\tif (!opts->no_commit)\n+\t\t\tres = run_git_commit(defmsg, opts);\n \t}\n \n \tfree_message(&msg);\n@@ -502,18 +515,18 @@ static int do_pick_commit(struct commit *commit)\n \treturn res;\n }\n \n-static void prepare_revs(struct rev_info *revs)\n+static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)\n {\n \tint argc;\n \n \tinit_revisions(revs, NULL);\n \trevs->no_walk = 1;\n-\tif (action != REVERT)\n+\tif (opts->action != REVERT)\n \t\trevs->reverse = 1;\n \n-\targc = setup_revisions(commit_argc, commit_argv, revs, NULL);\n+\targc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);\n \tif (argc > 1)\n-\t\tusage(*revert_or_cherry_pick_usage());\n+\t\tusage(*revert_or_cherry_pick_usage(opts));\n \n \tif (prepare_revision_walk(revs))\n \t\tdie(_(\"revision walk setup failed\"));\n@@ -522,7 +535,7 @@ static void prepare_revs(struct rev_info *revs)\n \t\tdie(_(\"empty commit set passed\"));\n }\n \n-static void read_and_refresh_cache(const char *me)\n+static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n {\n \tstatic struct lock_file index_lock;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n@@ -537,33 +550,34 @@ static void read_and_refresh_cache(const char *me)\n \trollback_lock_file(&index_lock);\n }\n \n-static int revert_or_cherry_pick(int argc, const char **argv)\n+static int revert_or_cherry_pick(int argc, const char **argv,\n+\t\t\t\tstruct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n \n \tgit_config(git_default_config, NULL);\n-\tme = action == REVERT ? \"revert\" : \"cherry-pick\";\n+\tme = opts->action == REVERT ? \"revert\" : \"cherry-pick\";\n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tparse_args(argc, argv);\n+\tparse_args(argc, argv, opts);\n \n-\tif (allow_ff) {\n-\t\tif (signoff)\n+\tif (opts->allow_ff) {\n+\t\tif (opts->signoff)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n-\t\tif (no_commit)\n+\t\tif (opts->no_commit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (record_origin)\n+\t\tif (opts->record_origin)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n-\t\tif (edit)\n+\t\tif (opts->edit)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n \t}\n \n-\tread_and_refresh_cache(me);\n+\tread_and_refresh_cache(me, opts);\n \n-\tprepare_revs(&revs);\n+\tprepare_revs(&revs, opts);\n \n \twhile ((commit = get_revision(&revs))) {\n-\t\tint res = do_pick_commit(commit);\n+\t\tint res = do_pick_commit(commit, opts);\n \t\tif (res)\n \t\t\treturn res;\n \t}\n@@ -574,10 +588,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 \tint res;\n+\tstruct replay_opts opts;\n+\n+\tmemset(&opts, 0, sizeof(struct replay_opts));\n \tif (isatty(0))\n-\t\tedit = 1;\n-\taction = REVERT;\n-\tres = revert_or_cherry_pick(argc, argv);\n+\t\topts.edit = 1;\n+\topts.action = REVERT;\n+\tres = revert_or_cherry_pick(argc, argv, &opts);\n \tif (res > 0)\n \t\t/* Exit status from conflict */\n \t\treturn res;\n@@ -590,8 +607,11 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n {\n \tint res;\n-\taction = CHERRY_PICK;\n-\tres = revert_or_cherry_pick(argc, argv);\n+\tstruct replay_opts opts;\n+\n+\tmemset(&opts, 0, sizeof(struct replay_opts));\n+\topts.action = CHERRY_PICK;\n+\tres = revert_or_cherry_pick(argc, argv, &opts);\n \tif (res > 0)\n \t\treturn res;\n \tif (res < 0)\n-- \n1.7.5.GIT\n"},{"id":"168782","messageId":"1306425233-504-7-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 06/10] revert: Separate cmdline parsing from functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:49Z","receivedAt":"2011-05-26T15:53:49Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Make cmd_cherry_pick and cmd_revert call parse_args and parse all the\ncommand-line arguments into an opts structure before sending it off to\na simplified pick_commits function. pick_commits, in turn will set up\nthe revision walker and call do_pick_commit in a loop- later in the\nseries, it will serve as the starting point for continuation.\n\nBased-on-patch-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 17 +++++++++--------\n 1 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex cc2fa73..2e5f260 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -550,17 +550,12 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n \trollback_lock_file(&index_lock);\n }\n \n-static int revert_or_cherry_pick(int argc, const char **argv,\n-\t\t\t\tstruct replay_opts *opts)\n+static int pick_commits(struct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n \n-\tgit_config(git_default_config, NULL);\n-\tme = opts->action == REVERT ? \"revert\" : \"cherry-pick\";\n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tparse_args(argc, argv, opts);\n-\n \tif (opts->allow_ff) {\n \t\tif (opts->signoff)\n \t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n@@ -594,7 +589,10 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tif (isatty(0))\n \t\topts.edit = 1;\n \topts.action = REVERT;\n-\tres = revert_or_cherry_pick(argc, argv, &opts);\n+\tgit_config(git_default_config, NULL);\n+\tme = \"revert\";\n+\tparse_args(argc, argv, &opts);\n+\tres = pick_commits(&opts);\n \tif (res > 0)\n \t\t/* Exit status from conflict */\n \t\treturn res;\n@@ -611,7 +609,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \n \tmemset(&opts, 0, sizeof(struct replay_opts));\n \topts.action = CHERRY_PICK;\n-\tres = revert_or_cherry_pick(argc, argv, &opts);\n+\tgit_config(git_default_config, NULL);\n+\tme = \"cherry-pick\";\n+\tparse_args(argc, argv, &opts);\n+\tres = pick_commits(&opts);\n \tif (res > 0)\n \t\treturn res;\n \tif (res < 0)\n-- \n1.7.5.GIT\n"},{"id":"168786","messageId":"1306425233-504-8-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 07/10] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:50Z","receivedAt":"2011-05-26T15:53:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Earlier, incompatible command-line options used to be caught in\npick_commits after parse_args has parsed the options and populated the\noptions structure; a lot of unncessary work has already been done, and\nsignificant amount of cleanup is required to die at this stage.\nInstead, hand over this responsibility to parse_args so that the\nprogram can die early. Also write a die_opt_incompabile function to\nhandle incompatible options in a general manner; it will be used more\nextensively as more command-line options are introduced later in the\nseries.\n\nHelped-by: Junio C Hamano \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 34 +++++++++++++++++++++++-----------\n 1 files changed, 23 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 2e5f260..1c6c102 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -80,6 +80,22 @@ static int option_parse_x(const struct option *opt,\n \treturn 0;\n }\n \n+static void verify_opt_compatible(const char *me, const char *base_opt, ...)\n+{\n+\tconst char *this_opt;\n+\tva_list ap;\n+\tint set;\n+\n+\tva_start(ap, base_opt);\n+\twhile ((this_opt = va_arg(ap, const char *))) {\n+\t\tset = va_arg(ap, int);\n+\t\tif (set)\n+\t\t\tdie(_(\"%s: %s cannot be used with %s\"),\n+\t\t\t\tme, this_opt, base_opt);\n+\t}\n+\tva_end(ap);\n+}\n+\n static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n@@ -116,6 +132,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \tif (opts->commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n+\tif (opts->allow_ff)\n+\t\tverify_opt_compatible(me, \"--ff\",\n+\t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\"--no-commit\", opts->no_commit,\n+\t\t\t\t\"-x\", opts->record_origin,\n+\t\t\t\t\"--edit\", opts->edit,\n+\t\t\t\tNULL);\n \topts->commit_argv = argv;\n }\n \n@@ -556,17 +579,6 @@ static int pick_commits(struct replay_opts *opts)\n \tstruct commit *commit;\n \n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tif (opts->allow_ff) {\n-\t\tif (opts->signoff)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --signoff\"));\n-\t\tif (opts->no_commit)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --no-commit\"));\n-\t\tif (opts->record_origin)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with -x\"));\n-\t\tif (opts->edit)\n-\t\t\tdie(_(\"cherry-pick --ff cannot be used with --edit\"));\n-\t}\n-\n \tread_and_refresh_cache(me, opts);\n \n \tprepare_revs(&revs, opts);\n-- \n1.7.5.GIT\n"},{"id":"168789","messageId":"1306425233-504-9-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:51Z","receivedAt":"2011-05-26T15:53:51Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than\none commit, 2010-06-02), a single invocation of \"git cherry-pick\" or\n\"git revert\" can perform picks of several individual commits. To\nallow \"git cherry-pick --abort\" to cancel and \"git cherry-pick\n--continue\" to resume the entire command, we will need to store some\ninformation about the state and the plan at the beginning. Introduce\na HEAD file to store this state, and TODO file to store the plan.\n\nHelped-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 74 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1c6c102..306f5b0 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,12 @@\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+\n static const char * const revert_usage[] = {\n \t\"git revert [options] \",\n \tNULL\n@@ -573,22 +580,86 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n \trollback_lock_file(&index_lock);\n }\n \n+static void format_todo(struct strbuf *buf, struct commit_list *list,\n+\t\t\tstruct replay_opts *opts)\n+{\n+\tstruct commit_list *cur = NULL;\n+\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n+\tconst char *sha1_abbrev = NULL;\n+\tconst char *action;\n+\n+\taction = (opts->action == REVERT ? \"revert\" : \"pick\");\n+\tfor (cur = list; cur; cur = cur->next) {\n+\t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n+\t\tif (get_message(cur->item, &msg))\n+\t\t\tdie(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n+\t\tstrbuf_addf(buf, \"%s %s %s\\n\", action, sha1_abbrev, msg.subject);\n+\t}\n+}\n+\n+static void persist_head(unsigned char *head)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct lock_file head_lock;\n+\tint fd;\n+\n+\tif (file_exists(SEQ_PATH)) {\n+\t\tif (!is_directory(SEQ_PATH) && remove_path(SEQ_PATH) < 0) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\tdie(_(\"Could not remove %s\"), SEQ_PATH);\n+\t\t}\n+\t}\n+\tif (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), SEQ_PATH);\n+\t}\n+\n+\tfd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);\n+\tstrbuf_addf(&buf, \"%s\", head);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), HEAD_FILE);\n+\tif (commit_lock_file(&head_lock) < 0)\n+\t\tdie(_(\"Error wrapping up %s\"), HEAD_FILE);\n+}\n+\n+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct lock_file todo_lock;\n+\tint fd;\n+\n+\tfd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);\n+\tformat_todo(&buf, todo_list, opts);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), TODO_FILE);\n+\tif (commit_lock_file(&todo_lock) < 0)\n+\t\tdie(_(\"Error wrapping up %s\"), TODO_FILE);\n+}\n+\n static int pick_commits(struct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n+\tunsigned char head[20];\n \n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n-\tread_and_refresh_cache(me, opts);\n \n+\tif (get_sha1(\"HEAD\", head))\n+\t\treturn error(_(\"You do not have a valid HEAD\"));\n+\n+\tread_and_refresh_cache(me, opts);\n \tprepare_revs(&revs, opts);\n+\tpersist_head(head);\n+\tpersist_todo(revs.commits, opts);\n \n \twhile ((commit = get_revision(&revs))) {\n \t\tint res = do_pick_commit(commit, opts);\n-\t\tif (res)\n+\t\tif (res) {\n+\t\t\tcommit_list_insert(commit, &revs.commits);\n+\t\t\tpersist_todo(revs.commits, opts);\n \t\t\treturn res;\n+\t\t}\n \t}\n-\n \treturn 0;\n }\n \n-- \n1.7.5.GIT\n"},{"id":"168784","messageId":"1306425233-504-10-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 09/10] revert: Implement parsing --continue, --abort and --skip","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:52Z","receivedAt":"2011-05-26T15:53:52Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce three new command-line options: --continue, --abort, and\n--skip resembling the correspoding options in \"rebase -i\". For now,\njust parse the options into the replay_opts structure, making sure\nthat two of them are not specified together. They will actually be\nimplemented later in the series.\n\nMentored-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 70 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 306f5b0..f33d40a 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -47,6 +47,11 @@ static const char *me;\n struct replay_opts {\n \tenum { REVERT, CHERRY_PICK } action;\n \n+\t/* --abort, --skip, and --continue */\n+\tint abort_oper;\n+\tint skip_oper;\n+\tint continue_oper;\n+\n \t/* Boolean options */\n \tint edit;\n \tint record_origin;\n@@ -103,11 +108,36 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)\n \tva_end(ap);\n }\n \n+static void verify_opt_mutually_compatible(const char *me, ...)\n+{\n+\tconst char *opt1, *opt2;\n+\tva_list ap;\n+\tint set;\n+\n+\tva_start(ap, me);\n+\twhile ((opt1 = va_arg(ap, const char *))) {\n+\t\tset = va_arg(ap, int);\n+\t\tif (set)\n+\t\t\tbreak;\n+\t}\n+\tif (!opt1) return;\n+\twhile ((opt2 = va_arg(ap, const char *))) {\n+\t\tset = va_arg(ap, int);\n+\t\tif (set)\n+\t\t\tdie(_(\"%s: %s cannot be used with %s\"),\n+\t\t\t\tme, opt1, opt2);\n+\t}\n+\tva_end(ap);\n+}\n+\n static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tint noop;\n \tstruct option options[] = {\n+\t\tOPT_BOOLEAN(0, \"abort\", &opts->abort_oper, \"abort the current operation\"),\n+\t\tOPT_BOOLEAN(0, \"skip\", &opts->skip_oper, \"skip the current commit\"),\n+\t\tOPT_BOOLEAN(0, \"continue\", &opts->continue_oper, \"continue the current operation\"),\n \t\tOPT_BOOLEAN('n', \"no-commit\", &opts->no_commit, \"don't automatically commit\"),\n \t\tOPT_BOOLEAN('e', \"edit\", &opts->edit, \"edit the commit message\"),\n \t\t{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, \"no-op (backward compatibility)\",\n@@ -136,7 +166,37 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \topts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,\n \t\t\t\t\tPARSE_OPT_KEEP_ARGV0 |\n \t\t\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n-\tif (opts->commit_argc < 2)\n+\n+\t/* Check for mutually incompatible command line arguments */\n+\tverify_opt_mutually_compatible(me,\n+\t\t\t\t\"--abort\", opts->abort_oper,\n+\t\t\t\t\"--skip\", opts->skip_oper,\n+\t\t\t\t\"--continue\", opts->continue_oper,\n+\t\t\t\tNULL);\n+\n+\t/* Check for incompatible command line arguments */\n+\tif (opts->abort_oper || opts->skip_oper || opts->continue_oper) {\n+\t\tchar *this_oper;\n+\t\tif (opts->abort_oper)\n+\t\t\tthis_oper = \"--abort\";\n+\t\telse if (opts->skip_oper)\n+\t\t\tthis_oper = \"--skip\";\n+\t\telse\n+\t\t\tthis_oper = \"--continue\";\n+\n+\t\tverify_opt_compatible(me, this_oper,\n+\t\t\t\t\"--no-commit\", opts->no_commit,\n+\t\t\t\t\"--edit\", opts->edit,\n+\t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\"--mainline\", opts->mainline,\n+\t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n+\t\t\t\t\"--strategy-option\", opts->xopts ? 1 : 0,\n+\t\t\t\t\"-x\", opts->record_origin,\n+\t\t\t\t\"--ff\", opts->allow_ff,\n+\t\t\t\tNULL);\n+\t}\n+\n+\telse if (opts->commit_argc < 2)\n \t\tusage_with_options(usage_str, options);\n \n \tif (opts->allow_ff)\n@@ -146,6 +206,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"-x\", opts->record_origin,\n \t\t\t\t\"--edit\", opts->edit,\n \t\t\t\tNULL);\n+\n+\t/* Remove these when the options are actually implemented */\n+\tif (opts->abort_oper)\n+\t\tdie(\"--abort is not implemented yet\");\n+\tif (opts->skip_oper)\n+\t\tdie(\"--skip is not implemented yet\");\n+\tif (opts->continue_oper)\n+\t\tdie(\"--continue is not implemented yet\");\n+\n \topts->commit_argv = argv;\n }\n \n-- \n1.7.5.GIT\n"},{"id":"168785","messageId":"1306425233-504-11-git-send-email-artagnon@gmail.com","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 10/10] revert: Implement --abort processing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T15:53:53Z","receivedAt":"2011-05-26T15:53:53Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"To abort, perform a \"rerere clear\" and \"reset --hard\" to the ref\nspecified by the HEAD file introduced earlier in the series.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 46 ++++++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 42 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex f33d40a..be63aee 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -208,8 +208,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\tNULL);\n \n \t/* Remove these when the options are actually implemented */\n-\tif (opts->abort_oper)\n-\t\tdie(\"--abort is not implemented yet\");\n \tif (opts->skip_oper)\n \t\tdie(\"--skip is not implemented yet\");\n \tif (opts->continue_oper)\n@@ -732,6 +730,46 @@ static int pick_commits(struct replay_opts *opts)\n \treturn 0;\n }\n \n+static int process_continuation(struct replay_opts *opts)\n+{\n+\tif (opts->abort_oper) {\n+\t\tchar head[DEFAULT_ABBREV];\n+\t\tunsigned char sha1[20];\n+\t\tint fd;\n+\t\trerere_clear(0);\n+\n+\t\tif (!file_exists(HEAD_FILE))\n+\t\t\tgoto error;\n+\t\tfd = open(HEAD_FILE, O_RDONLY, 0666);\n+\t\tif (fd < 0)\n+\t\t\treturn error(_(\"Could not open '%s' for reading: %s\"),\n+\t\t\t\tHEAD_FILE, strerror(errno));\n+\t\tif (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV) {\n+\t\t\tclose(fd);\n+\t\t\treturn error(_(\"Corrupt '%s': %s\"), HEAD_FILE, strerror(errno));\n+\t\t}\n+\t\tclose(fd);\n+\n+\t\tif (get_sha1(head, sha1))\n+\t\t\treturn error(_(\"Failed to resolve '%s' as a valid ref.\"), head);\n+\t\tupdate_ref(NULL, \"HEAD\", sha1, NULL, 0, MSG_ON_ERR);\n+\t}\n+\telse if (opts->skip_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\treturn 0;\n+\t}\n+\telse if (opts->continue_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\treturn 0;\n+\t}\n+\n+\treturn pick_commits(opts);\n+error:\n+\treturn error(_(\"No %s in progress\"), me);\n+}\n+\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tint res;\n@@ -744,7 +782,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tme = \"revert\";\n \tparse_args(argc, argv, &opts);\n-\tres = pick_commits(&opts);\n+\tres = process_continuation(&opts);\n \tif (res > 0)\n \t\t/* Exit status from conflict */\n \t\treturn res;\n@@ -764,7 +802,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tme = \"cherry-pick\";\n \tparse_args(argc, argv, &opts);\n-\tres = pick_commits(&opts);\n+\tres = process_continuation(&opts);\n \tif (res > 0)\n \t\treturn res;\n \tif (res < 0)\n-- \n1.7.5.GIT\n"},{"id":"168794","messageId":"20110526161102.GC24931@elie","threadId":"27453","inReplyTo":"1306425233-504-9-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-26T16:11:02Z","receivedAt":"2011-05-26T16:11:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -573,22 +580,86 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n[...]\n> \tprepare_revs(&revs, opts);\n> +\tpersist_head(head);\n> +\tpersist_todo(revs.commits, opts);\n> \n> \twhile ((commit = get_revision(&revs))) {\n> \t\tint res = do_pick_commit(commit, opts);\n> -\t\tif (res)\n> +\t\tif (res) {\n> +\t\t\tcommit_list_insert(commit, &revs.commits);\n> +\t\t\tpersist_todo(revs.commits, opts);\n> \t\t\treturn res;\n> +\t\t}\n> \t}\n\nAlmost there. To comfort overly-worried people like me that think we\nhave not finished converted all die() calls yet, wouldn't this need to\nlook like\n\n\tpersist_head(head);\n\twhile ((commit = ...)) {\n\t\tint status_or_error;\n\n\t\t/*\n\t\t * Checkpoint. If do_pick_commit exits, make sure the user\n\t\t * can still use \"git cherry-pick --continue\" to recover.\n\t\t */\n\t\tpersist_todo(revs.commits, opts);\n\n\t\tstatus_or_error = do_pick_commit(...);\n\t\tif (status_or_error)\n\t\t\treturn status_or_error;\n\t}\n\n\t/* Success! */\n\tremove_todo(opts);\n\tremove_head();\n\treturn 0;\n\nAnd with that, this would no longer depend on the (valuable enough on\ntheir own terms) patches 1 and 2 so they could be treated as a\nseparate series, no?\n"},{"id":"168795","messageId":"BANLkTi=FkK=k6OavQtqMRvPCZ7nc+iKtwQ@mail.gmail.com","threadId":"27453","inReplyTo":"20110526161102.GC24931@elie","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T16:26:19Z","receivedAt":"2011-05-26T16:26:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Almost there.  To comfort overly-worried people like me that think we\n> have not finished converted all die() calls yet, wouldn't this need to\n> look like\n>\n>        persist_head(head);\n>        while ((commit = ...)) {\n>                int status_or_error;\n>\n>                /*\n>                 * Checkpoint.  If do_pick_commit exits, make sure the user\n>                 * can still use \"git cherry-pick --continue\" to recover.\n>                 */\n>                persist_todo(revs.commits, opts);\n>\n>                status_or_error = do_pick_commit(...);\n>                if (status_or_error)\n>                        return status_or_error;\n>        }\n>\n>        /* Success! */\n>        remove_todo(opts);\n>        remove_head();\n>        return 0;\n\nYou told me about this on IRC last night, but I forgot to squash the\nrelevant patch into this series. Thanks :)\n\n> And with that, this would no longer depend on the (valuable enough on\n> their own terms) patches 1 and 2 so they could be treated as a\n> separate series, no?\n\nYes, but I'd like to keep 1 and 2 in this series, if that's alright.\nAnother series will have to take care of error handling more\nextensively.\nWith a few tests, would it be useful to get this series merged in? It\nimplements '--abort', which is already useful.\n\n-- Ram\n"},{"id":"168802","messageId":"BANLkTi=ifQYFbq-ntpVF=1+sjv-wdkEJ-w@mail.gmail.com","threadId":"27453","inReplyTo":"BANLkTi=FkK=k6OavQtqMRvPCZ7nc+iKtwQ@mail.gmail.com","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-26T17:05:25Z","receivedAt":"2011-05-26T17:05:25Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nRamkumar Ramachandra writes:\n> Jonathan Nieder writes:\n>> And with that, this would no longer depend on the (valuable enough on\n>> their own terms) patches 1 and 2 so they could be treated as a\n>> separate series, no?\n>\n> Yes, but I'd like to keep 1 and 2 in this series, if that's alright.\n> Another series will have to take care of error handling more\n> extensively.\n> With a few tests, would it be useful to get this series merged in? It\n> implements '--abort', which is already useful.\n\nThere's a good way to create a separation between patches 1-2, and the\nrest- thanks to Jonathan's suggestion on IRC.\nJunio: Would you like to merge just the first two patches? I'll\ncontinue working on the rest, and try to come up with something useful\nto merge in another week or two.\n\n-- Ram\n"},{"id":"168814","messageId":"7vpqn5w9zi.fsf@alter.siamese.dyndns.org","threadId":"27453","inReplyTo":"1306425233-504-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v4 00/10] Sequencer Foundations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T19:13:53Z","receivedAt":"2011-05-26T19:13:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> This is a quick re-roll after Junio's review of the previous\n> iteration; the previous iteration was posted less than 24 hours ago.\n> I'm in a hurry,...\n\nOpen source is not just about pushing new code through other people's\nthroats, but also about the skill to make other people around you to work\nwith you. They may be busy spending time on helping other topics moving\nforward. Or they may simply be bone-headed ;-).\n\nIf you are in a hurry, try to spend _more_ time making sure it is easier\nto review for other people. What were updated, what were kept as they were\nand for what reason? The purpose of adding the cover letter to the series\nis to do just that.\n\nWithout spending that effort and saying \"I am in a hurry\" give a wrong\nimpression to others: \"I cannot tell if this round is better cooked than\nthe previous round without spending nontrivial time reading on it, but\nchances are it hasn't been much improved during the 24 hours. It may be\nbetter use of my time to skip it and work on other topics first. After\nfinishing everything else I can come back---by that time Ram may have sent\nout another round and reviewing this round right now may become wasted\ntime\".\n\nAlso the series seems to be based on a rather old codebase where we didn't\nallow reverting the root commit. I tried to apply a first few to\nunderstand where these \"positive\" error status could be coming from, and\nwas stopped by patch conflicts. In [2/10] I think you are propagating\nerrors from run_command() coming back through try_merge_command(), and the\nchange is good for that particular codepath, but I haven't followed all\nthe other codepaths from do_cherry_pick() to convince myself that the exit\nstatus from the merge strategy backend are _the only_ positive returns you\ncan possibly get. Have you?\n"},{"id":"168823","messageId":"7vhb8hw4xn.fsf@alter.siamese.dyndns.org","threadId":"27453","inReplyTo":"BANLkTi=ifQYFbq-ntpVF=1+sjv-wdkEJ-w@mail.gmail.com","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T21:03:00Z","receivedAt":"2011-05-26T21:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> There's a good way to create a separation between patches 1-2, and the\n> rest- thanks to Jonathan's suggestion on IRC.\n> Junio: Would you like to merge just the first two patches? I'll\n> continue working on the rest, and try to come up with something useful\n> to merge in another week or two.\n\nIf you can split out a smaller series that is obviously correct and\njustifiable with or without the rest of the series, that is very good\nin general.\n\nI think [1/10] almost falls into that category, modulo that we used to say\nit is \"fatal:\" when you have unmerged files and cannot cherry-pick, but\nyou now say \"error:\", which I think is an unnecessary change that you may\nwant to avoid if you can.\n\nI am not yet convinced [2/10] falls into that category.\n"},{"id":"168855","messageId":"BANLkTik1oyCCjcAqvqZNHPecNKUJ9k0bCA@mail.gmail.com","threadId":"27453","inReplyTo":"7vpqn5w9zi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 00/10] Sequencer Foundations","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-27T07:22:07Z","receivedAt":"2011-05-27T07:22:07Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano writes:\n> Without spending that effort and saying \"I am in a hurry\" give a wrong\n> impression to others: \"I cannot tell if this round is better cooked than\n> the previous round without spending nontrivial time reading on it, but\n> chances are it hasn't been much improved during the 24 hours.  It may be\n> better use of my time to skip it and work on other topics first.  After\n> finishing everything else I can come back---by that time Ram may have sent\n> out another round and reviewing this round right now may become wasted\n> time\".\n\nYou're right; this iteration was ill-thought-out. I'll try to put\nmyself in my reviewer's shoes before submitting code next time, so\nthat I don't waste effort re-rolling unnecessarily.\n\n> Also the series seems to be based on a rather old codebase where we didn't\n> allow reverting the root commit. I tried to apply a first few to\n> understand where these \"positive\" error status could be coming from, and\n> was stopped by patch conflicts. In [2/10] I think you are propagating\n> errors from run_command() coming back through try_merge_command(), and the\n> change is good for that particular codepath, but I haven't followed all\n> the other codepaths from do_cherry_pick() to convince myself that the exit\n> status from the merge strategy backend are _the only_ positive returns you\n> can possibly get. Have you?\n\nOops, I'll rebase.\nI'll trace all the code paths carefully and include a summary of my\nlearnings in the next iteration.\n\n-- Ram\n"},{"id":"169116","messageId":"BANLkTino5gmfXjeOVih7+LHcMu1usPWAoQ@mail.gmail.com","threadId":"27453","inReplyTo":"20110526161102.GC24931@elie","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-01T15:28:08Z","receivedAt":"2011-06-01T15:28:08Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan and others,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n> Almost there.  To comfort overly-worried people like me that think we\n> have not finished converted all die() calls yet, wouldn't this need to\n> look like\n>\n>        persist_head(head);\n>        while ((commit = ...)) {\n>                int status_or_error;\n>\n>                /*\n>                 * Checkpoint.  If do_pick_commit exits, make sure the user\n>                 * can still use \"git cherry-pick --continue\" to recover.\n>                 */\n>                persist_todo(revs.commits, opts);\n>\n>                status_or_error = do_pick_commit(...);\n>                if (status_or_error)\n>                        return status_or_error;\n>        }\n>\n>        /* Success! */\n>        remove_todo(opts);\n>        remove_head();\n>        return 0;\n\nIt's a little embarrassing, but I'm not able to figure out how to\npersist the TODO early. prepare_revs() sets up the revision walker and\npopulates the rev_info struct: it contains a commit_list * in its\ncommits member, but I apparently can't access those directly -- I went\nthrough some literature about revision walking and inspected how\nget_revision must be used to iterate over all the individual commits.\nNow, to persist the TODO, I need a commit_list to use: is there some\nAPI I can use to avoid iterating over all the commits twice? Once to\npopulate the commit_list to persist the TODO, and the second time to\nactually pick them?\n\nThanks.\n\n-- Ram\n"},{"id":"169143","messageId":"20110601193126.GC9730@elie","threadId":"27453","inReplyTo":"BANLkTino5gmfXjeOVih7+LHcMu1usPWAoQ@mail.gmail.com","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-01T19:31:26Z","receivedAt":"2011-06-01T19:31:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> is there some\n> API I can use to avoid iterating over all the commits twice? Once to\n> populate the commit_list to persist the TODO, and the second time to\n> actually pick them?\n\nGood catch. (If I understand correctly this was a potential problem\nwith the previous iterations, too.)\n\nSo, before thinking about what APIs git provides, what is the desired\nbehavior? It is possible to walk a revision list incrementally (like\n\"git log\" does); should cherry-pick take advantage of that?\n\n. If yes, the \"todo list\" would have to be in some form that can be\n updated incrementally (something like the original revision range\n with the commits already cherry-picked negated). This sounds\n complicated to me.\n\n. If no, the \"todo list\" needs to be fully resolved to start out, and\n then the cherry-pick loop can just walk through it. In git API\n terms, that means first populating the commit_list (for example\n using a loop that gets revisions and inserts them at the end of a\n commit_list, or by preparing the revision walk with \"limited\" flag\n set), then walking through it with commit_list APIs.\n"},{"id":"169192","messageId":"BANLkTinc=9aW_y6s-Zb578QsMawnxFD55Q@mail.gmail.com","threadId":"27453","inReplyTo":"20110601193126.GC9730@elie","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-02T12:53:03Z","receivedAt":"2011-06-02T12:53:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> . If no, the \"todo list\" needs to be fully resolved to start out, and\n>  then the cherry-pick loop can just walk through it.  In git API\n>  terms, that means first populating the commit_list (for example\n>  using a loop that gets revisions and inserts them at the end of a\n>  commit_list, or by preparing the revision walk with \"limited\" flag\n>  set), then walking through it with commit_list APIs.\n\nThanks. I picked this approach :)\n\nOn a slightly unrelated note, I just found out that the persist_todo\ncan't be called twice due to a lockfile API limitation* -- the\natexit(3) cleanup handler is supposed to have a linked list of\nlockfiles to clean up. When one lockfile is used twice, it becomes a\ncircularly linked list, and the entire program hangs at exit time\nsince traversing a circularly linked list is never-ending. Couple of\ncomments:\n1. By reading the lockfile implementation, I understand why this\nhappens -- can't it be implemented differently to remove this\nlimitation? If not, shouldn't this limitation be documented somewhere?\n2. What can I do? It'll be inelegant to use the lockfile API while\npersisting the first time, but not the second time -- is there any way\nout of this?\n\nThanks again.\n\n* You brought this up during one of our IRC conversations, but I\ndidn't understand it then. I do now.\n\n-- Ram\n"},{"id":"169197","messageId":"20110602142933.GA32487@sigill.intra.peff.net","threadId":"27453","inReplyTo":"BANLkTinc=9aW_y6s-Zb578QsMawnxFD55Q@mail.gmail.com","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-02T14:29:34Z","receivedAt":"2011-06-02T14:29:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 02, 2011 at 06:23:03PM +0530, Ramkumar Ramachandra wrote:\n\n> On a slightly unrelated note, I just found out that the persist_todo\n> can't be called twice due to a lockfile API limitation* -- the\n> atexit(3) cleanup handler is supposed to have a linked list of\n> lockfiles to clean up. When one lockfile is used twice, it becomes a\n> circularly linked list, and the entire program hangs at exit time\n> since traversing a circularly linked list is never-ending. Couple of\n> comments:\n> 1. By reading the lockfile implementation, I understand why this\n> happens -- can't it be implemented differently to remove this\n> limitation? If not, shouldn't this limitation be documented somewhere?\n\nDoesn't lock_file handle multiple locking already via the on_list parameter?\n\n $ grep -A2 on_list lockfile.c\n if (!lk->on_list) {\n lk->next = lock_file_list;\n lock_file_list = lk;\n lk->on_list = 1;\n }\n\nHowever, I think you have a bigger problem, which is that you are\nallocating the lock_file on the stack in persist_todo and persist_head.\nSo by the time the atexit() handler is called, this storage has gone\naway and you are just reading random data (not to mention that it also\nshould be zero-initialized before being passed to lock_file).\n\n-Peff\n"},{"id":"169606","messageId":"BANLkTi=i2igTyusmVf8155VcFfVLQ==u3A@mail.gmail.com","threadId":"27453","inReplyTo":"20110602142933.GA32487@sigill.intra.peff.net","subject":"Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-08T13:14:14Z","receivedAt":"2011-06-08T13:14:14Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jeff,\n\nJeff King writes:\n> Doesn't lock_file handle multiple locking already via the on_list parameter?\n>\n>    $ grep -A2 on_list lockfile.c\n>            if (!lk->on_list) {\n>                    lk->next = lock_file_list;\n>                    lock_file_list = lk;\n>                    lk->on_list = 1;\n>            }\n>\n> However, I think you have a bigger problem, which is that you are\n> allocating the lock_file on the stack in persist_todo and persist_head.\n> So by the time the atexit() handler is called, this storage has gone\n> away and you are just reading random data (not to mention that it also\n> should be zero-initialized before being passed to lock_file).\n\nRight. Thanks for the excellent pointer. I was struggling to see what\nwas wrong on GDB; little did I realize that I was viewing nonsense. I\nsolved the problem by making the lock_file variables static.\n\nThanks again, and sorry for the nonsense.\n\n-- Ram\n"}]}