{"thread":{"id":"27674","subject":"[PATCH 00/13] Sequencer with continuation features","startedAt":"2011-06-21T13:04:36Z","lastAt":"2011-07-05T10:47:06Z","messageCount":47,"participants":["Ramkumar Ramachandra","Jonathan Nieder","Junio C Hamano","Miles Bader"],"isPatch":true,"patchVersion":1,"patchTotal":13},"messages":[{"id":"170367","messageId":"1308661489-20080-1-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":null,"subject":"[PATCH 00/13] Sequencer with continuation features","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:36Z","receivedAt":"2011-06-21T13:04:36Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThe much awaited cherry-pick --continue and --skip-all are here. Much\nthanks to Jonathan for all his inputs. I think I can safely say that\nI'm quite happy with the general state of this series now. Apart from\nthe introduction of the the new features, there have been a few minor\nimprovements in the rest of the series since the last iteration.\nUnfortunately, it's still not ready for inclusion for the following\nreasons:\n1. --skip-all isn't a great name; maybe --quit as Christian suggested?\n2. I don't know what to do when a reset is invoked. If we do nothing,\nwe'd break users' habits. Jonathan suggested that we move\n.git/sequencer to .git/sequencer-cancelled on a hard reset, and this\nseems like a plausible solution. More inputs on this?\n3. --continue is obviously broken, because of lack of command-line\noption support. I think I should evolve the parser little by little-\nfirst, I can persist the global opts structure, and then start\nsupporting per-command options. That way, it'll be easy to get the\nparser reviewed.\n4. Tests are too basic. I need to write more elaborate tests.\n\nThanks for reading.\n\nNote: The series is also available on the 'sequencer-continue' branch of my\nGitHub fork: http://github.com/artagnon/git\n\n-- Ram\n\nRamkumar Ramachandra (13):\n advice: Introduce error_resolve_conflict\n revert: Factor out add_message_to_msg function\n revert: Don't check lone argument in get_encoding\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: Persist data for continuation\n revert: Introduce a layer of indirection over pick_commits\n revert: Introduce --skip-all to cleanup sequencer data\n revert: Introduce --continue to continue the operation\n\n advice.c | 19 +-\n advice.h | 1 +\n builtin/revert.c | 585 +++++++++++++++++++++++++++---------\n git-rebase--interactive.sh | 25 ++-\n t/t3032-merge-recursive-options.sh | 2 +\n t/t3501-revert-cherry-pick.sh | 1 +\n t/t3502-cherry-pick-merge.sh | 9 +-\n t/t3504-cherry-pick-rerere.sh | 2 +\n t/t3505-cherry-pick-empty.sh | 14 +-\n t/t3506-cherry-pick-ff.sh | 3 +\n t/t3507-cherry-pick-conflict.sh | 24 ++-\n t/t3510-cherry-pick-sequence.sh | 79 +++++\n t/t7502-commit.sh | 1 +\n 13 files changed, 596 insertions(+), 169 deletions(-)\n create mode 100644 t/t3510-cherry-pick-sequence.sh\n\n-- \n1.7.5.GIT\n"},{"id":"170368","messageId":"1308661489-20080-2-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 01/13] advice: Introduce error_resolve_conflict","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:37Z","receivedAt":"2011-06-21T13:04:37Z","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. The only trade-off is that die_resolve_conflict is a\nlittle noisier now.\n\nInspired-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n advice.c | 19 +++++++++++++------\n advice.h | 1 +\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 0be4b5f..652680a 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -34,16 +34,23 @@ 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+\terror(\"'%s' is not possible because you have unmerged files.\", me);\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(\"Please, fix them up in the work tree, \"\n+\t\t\t\"and then use 'git add/rm ' as\\n\"\n+\t\t\t\"appropriate to mark resolution and make a commit, \"\n+\t\t\t\"or use 'git commit -a'.\");\n+\treturn -1;\n+}\n+\n+void NORETURN die_resolve_conflict(const char *me)\n+{\n+\terror_resolve_conflict(me);\n+\tdie(\"Exiting because of an unresolved conflict.\");\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":"170369","messageId":"1308661489-20080-3-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 02/13] revert: Factor out add_message_to_msg function","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:38Z","receivedAt":"2011-06-21T13:04:38Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The add_message_to_msg function is poorly implemented, has an unclear\nAPI, and only one callsite. Replace the callsite with a cleaner\nimplementation.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 21 +++++++--------------\n 1 files changed, 7 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1f27c63..d6d2356 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -185,19 +185,6 @@ 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 {\n \tint fd;\n@@ -471,11 +458,17 @@ static int do_pick_commit(void)\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n \t} else {\n+\t\tconst char *p = strstr(msg.message, \"\\n\\n\");\n+\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+\n+\t\t/* Add msg.message to msgbuf */\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-- \n1.7.5.GIT\n"},{"id":"170370","messageId":"1308661489-20080-4-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 03/13] revert: Don't check lone argument in get_encoding","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:39Z","receivedAt":"2011-06-21T13:04:39Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The get_encoding function has only one callsite, and its caller makes\nsure that a NULL argument isn't passed. Don't unnecessarily double\ncheck the same argument in get_encoding.\n\nSuggested-by: Jonathan Nieder \nSuggested-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 3 ---\n 1 files changed, 0 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex d6d2356..2de2e75 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -167,9 +167,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-- \n1.7.5.GIT\n"},{"id":"170380","messageId":"1308661489-20080-5-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 04/13] revert: Propogate errors upwards from do_pick_commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:40Z","receivedAt":"2011-06-21T13:04:40Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Earlier, revert_or_cherry_pick only used to return a non-negative\nnumber or die. Change this so that it can return negative values too;\npostive return values indicate conflicts, while negative ones indicate\nother errors, and this return status is propogated updwards from\ndo_pick_commit, to be finally handled in cmd_cherry_pick and\ncmd_revert. While revert_or_cherry_pick can still die due to several\nother reasons, this patches attempts to factor out some of the die\ncalls. In the same spirit, also introduce a new function\nerror_dirty_index, based on die_dirty_index which prints some hints\nand returns an error to its caller do_pick_commit.\n\nMentored-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 70 ++++++++++++++++++++++++++++-------------------------\n 1 files changed, 37 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 2de2e75..26f39d1 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -250,25 +250,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@@ -382,12 +372,12 @@ 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@@ -400,20 +390,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@@ -423,12 +413,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@@ -582,14 +572,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\tdie(_(\"%s failed\"), me);\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\tdie(_(\"%s failed\"), me);\n+\treturn 0;\n }\n-- \n1.7.5.GIT\n"},{"id":"170371","messageId":"1308661489-20080-6-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 05/13] revert: Eliminate global \"commit\" variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:41Z","receivedAt":"2011-06-21T13:04:41Z","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. Doing this involves changing several functions to\ntake an additional argument, but no other functional changes.\n\nHelped-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 24 ++++++++++++------------\n 1 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 26f39d1..1b04b3c 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@@ -182,12 +181,12 @@ static char *get_encoding(const char *message)\n \treturn NULL;\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@@ -354,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@@ -416,7 +415,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@@ -462,7 +461,7 @@ static int do_pick_commit(void)\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n \t\tif (!no_commit)\n-\t\t\twrite_cherry_pick_head();\n+\t\t\twrite_cherry_pick_head(sha1_to_hex(commit->object.sha1));\n \t}\n \n \tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n@@ -540,6 +539,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@@ -562,7 +562,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":"170373","messageId":"1308661489-20080-7-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 06/13] revert: Rename no_replay to record_origin","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:42Z","receivedAt":"2011-06-21T13:04:42Z","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 1b04b3c..cf62f93 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@@ -455,7 +455,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@@ -551,7 +551,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":"170372","messageId":"1308661489-20080-8-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 07/13] revert: Introduce struct to keep command-line options","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:43Z","receivedAt":"2011-06-21T13:04:43Z","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\nUnfortunately, parsing strategy-option violates a C89 rule:\nInitializers cannot refer to variables whose address is not known at\ncompile time.\n\nHelped-by: Junio C Hamano \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 176 ++++++++++++++++++++++++++++++------------------------\n 1 files changed, 99 insertions(+), 77 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex cf62f93..e2e290c 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -35,76 +35,90 @@ 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+enum replay_action { REVERT, CHERRY_PICK };\n+\n+struct replay_opts {\n+\tenum replay_action 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_ptr = opt->value;\n+\tstruct replay_opts *opts = *opts_ptr;\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+\topts->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 +287,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 +309,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 +349,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 +357,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 +368,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 +378,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@@ -388,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@@ -428,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@@ -455,18 +470,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@@ -476,23 +491,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@@ -501,18 +516,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@@ -521,7 +536,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@@ -536,33 +551,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@@ -573,10 +589,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@@ -589,8 +608,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":"170375","messageId":"1308661489-20080-9-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 08/13] revert: Separate cmdline parsing from functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:44Z","receivedAt":"2011-06-21T13:04:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Currently, revert_or_cherry_pick does too many things including\nargument parsing and setting up to pick the commits; this doesn't make\na good API. Simplify and rename the function to pick_commits, so that\nit just has the responsibility of setting up the revision walker and\ncalling do_pick_commit in a loop. Transfer the remaining work to its\ncallers cmd_cherry_pick and cmd_revert. Later in the series,\npick_commits will serve as the starting point for continuation.\n\nInspired-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 e2e290c..93e0531 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -551,17 +551,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@@ -595,7 +590,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@@ -612,7 +610,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":"170379","messageId":"1308661489-20080-10-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 09/13] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:45Z","receivedAt":"2011-06-21T13:04:45Z","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. Instead, hand over the responsibility of catching\nincompatible command-line options to parse_args so that the program\ncan die early. Also write a verify_opt_compatible function to handle\nincompatible options in a general manner.\n\nInspired-by: Christian Couder \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 93e0531..cfa898f 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -82,6 +82,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@@ -118,6 +134,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@@ -557,17 +580,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":"170378","messageId":"1308661489-20080-11-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 10/13] revert: Persist data for continuation","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:46Z","receivedAt":"2011-06-21T13:04:46Z","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\nimplement features like \"--continue\" to continue the whole operation,\nwe will need to store some information about the state and the plan at\nthe beginning. Introduce a \".git/sequencer/head\" file to store this\nstate, and \".git/sequencer/todo\" file to store the plan. Don't touch\nCHERRY_PICK_HEAD -- it will still be useful when a conflict is\nencountered.\n\nInspired-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 123 ++++++++++++++++++++++++++++++++++++--\n t/t3510-cherry-pick-sequence.sh | 37 ++++++++++++\n 2 files changed, 153 insertions(+), 7 deletions(-)\n create mode 100644 t/t3510-cherry-pick-sequence.sh\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex cfa898f..ca5756b 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,10 @@\n * Copyright (c) 2005 Junio C Hamano\n */\n \n+#define SEQ_DIR\t\tgit_path(\"sequencer\")\n+#define HEAD_FILE\tgit_path(\"sequencer/head\")\n+#define TODO_FILE\tgit_path(\"sequencer/todo\")\n+\n static const char * const revert_usage[] = {\n \t\"git revert [options] \",\n \tNULL\n@@ -412,7 +417,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t\treturn error(_(\"Your index file is unmerged.\"));\n \t} else {\n \t\tif (get_sha1(\"HEAD\", head))\n-\t\t\treturn error(_(\"You do not have a valid HEAD\"));\n+\t\t\treturn error(_(\"Can't %s on an unborn branch\"), me);\n \t\tif (index_differs_from(\"HEAD\", 0))\n \t\t\treturn error_dirty_index(me);\n \t}\n@@ -574,23 +579,127 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n \trollback_lock_file(&index_lock);\n }\n \n-static int pick_commits(struct replay_opts *opts)\n+static void format_todo(struct strbuf *buf, struct commit_list *todo_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 = todo_list; cur; cur = cur->next) {\n+\t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n+\t\tif (get_message(cur->item, &msg))\n+\t\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 walk_revs_populate_todo(struct commit_list **todo_list,\n+\t\t\t\tstruct replay_opts *opts)\n {\n \tstruct rev_info revs;\n \tstruct commit *commit;\n+\tstruct commit_list *new_item;\n+\tstruct commit_list *cur = NULL;\n+\n+\t/* Insert into todo_list in the same order */\n+\tprepare_revs(&revs, opts);\n+\twhile ((commit = get_revision(&revs))) {\n+\t\tnew_item = xmalloc(sizeof(struct commit_list));\n+\t\tnew_item->item = commit;\n+\t\tif (cur)\n+\t\t\tcur->next = new_item;\n+\t\telse\n+\t\t\t*todo_list = new_item;\n+\t\tcur = new_item;\n+\t}\n+\tcur->next = NULL;\n+}\n+\n+static void persist_head(const char *head)\n+{\n+\tstatic struct lock_file head_lock;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tif (file_exists(SEQ_DIR)) {\n+\t\tif (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\tdie(_(\"Could not remove %s\"), SEQ_DIR);\n+\t\t}\n+\t} else {\n+\t\tif (mkdir(SEQ_DIR, 0777) < 0) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), SEQ_DIR);\n+\t\t}\n+\t}\n+\tfd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);\n+\tstrbuf_addf(&buf, \"%s\\n\", head);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), HEAD_FILE);\n+\tif (commit_lock_file(&head_lock) < 0)\n+\t\tdie(_(\"Error wrapping up %s\"), HEAD_FILE);\n+}\n+\n+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+{\n+\tstatic struct lock_file todo_lock;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tfd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);\n+\tformat_todo(&buf, todo_list, opts);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie_errno(_(\"Could not write to %s.\"), TODO_FILE);\n+\t}\n+\tif (commit_lock_file(&todo_lock) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Error wrapping up %s\"), TODO_FILE);\n+\t}\n+\tstrbuf_release(&buf);\n+}\n+\n+static int cleanup_sequencer_data(void)\n+{\n+\tstatic struct strbuf seq_dir = STRBUF_INIT;\n+\n+\tstrbuf_addf(&seq_dir, \"%s\", SEQ_DIR);\n+\tif (remove_dir_recursively(&seq_dir, 0) < 0) {\n+\t\tstrbuf_release(&seq_dir);\n+\t\treturn error(_(\"Unable to clean up after successful %s\"), me);\n+\t}\n+\tstrbuf_release(&seq_dir);\n+\treturn 0;\n+}\n+\n+static int pick_commits(struct replay_opts *opts)\n+{\n+\tstruct commit_list *todo_list = NULL;\n+\tunsigned char sha1[20];\n+\tstruct commit_list *cur;\n+\tint res;\n \n-\tsetenv(GIT_REFLOG_ACTION, me, 0);\n \tread_and_refresh_cache(me, opts);\n+\tsetenv(GIT_REFLOG_ACTION, me, 0);\n \n-\tprepare_revs(&revs, opts);\n+\twalk_revs_populate_todo(&todo_list, opts);\n+\tif (!get_sha1(\"HEAD\", sha1))\n+\t\tpersist_head(sha1_to_hex(sha1));\n+\tpersist_todo(todo_list, opts);\n \n-\twhile ((commit = get_revision(&revs))) {\n-\t\tint res = do_pick_commit(commit, opts);\n+\tfor (cur = todo_list; cur; cur = cur->next) {\n+\t\tpersist_todo(cur, opts);\n+\t\tres = do_pick_commit(cur->item, opts);\n \t\tif (res)\n \t\t\treturn res;\n \t}\n \n-\treturn 0;\n+\t/* Sequence of picks finished successfully; cleanup by\n+\t removing the .git/sequencer directory */\n+\treturn cleanup_sequencer_data();\n }\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nnew file mode 100644\nindex 0000000..83e2722\n--- /dev/null\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='Test cherry-pick continuation features\n+\n+ + picked: rewrites foo to c\n+ + unrelatedpick: rewrites unrelated to reallyunrelated\n+ + base: rewrites foo to b\n+ + initial: writes foo as a, unrelated as unrelated\n+\n+'\n+\n+. ./test-lib.sh\n+\n+pristine_detach () {\n+\tgit checkout -f \"$1^0\" &&\n+\tgit read-tree -u --reset HEAD &&\n+\tgit clean -d -f -f -q -x\n+}\n+\n+test_expect_success setup '\n+\techo unrelated >unrelated &&\n+\tgit add unrelated &&\n+\ttest_commit initial foo a &&\n+\ttest_commit base foo b &&\n+\ttest_commit unrelatedpick unrelated reallyunrelated &&\n+\ttest_commit picked foo c &&\n+\tgit config advice.detachedhead false\n+\n+'\n+\n+test_expect_success 'cherry-pick cleans up sequencer directory upon success' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick initial..picked &&\n+\ttest_path_is_missing .git/sequencer\n+'\n+\n+test_done\n-- \n1.7.5.GIT\n"},{"id":"170374","messageId":"1308661489-20080-12-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 11/13] revert: Introduce a layer of indirection over pick_commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:47Z","receivedAt":"2011-06-21T13:04:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Write a new function called process_continuation to prepare a\ntodo_list to call pick_commits with; the job of pick_commits is\nsimplified into performing the tasks listed in todo_list. This will\nbe useful when continuation functionality like \"--continue\" is\nintroduced later in the series.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 33 ++++++++++++++++++++++-----------\n 1 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex ca5756b..5c0b97e 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -675,21 +675,13 @@ static int cleanup_sequencer_data(void)\n \treturn 0;\n }\n \n-static int pick_commits(struct replay_opts *opts)\n+static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n {\n-\tstruct commit_list *todo_list = NULL;\n-\tunsigned char sha1[20];\n \tstruct commit_list *cur;\n \tint res;\n \n-\tread_and_refresh_cache(me, opts);\n \tsetenv(GIT_REFLOG_ACTION, me, 0);\n \n-\twalk_revs_populate_todo(&todo_list, opts);\n-\tif (!get_sha1(\"HEAD\", sha1))\n-\t\tpersist_head(sha1_to_hex(sha1));\n-\tpersist_todo(todo_list, opts);\n-\n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tpersist_todo(cur, opts);\n \t\tres = do_pick_commit(cur->item, opts);\n@@ -702,6 +694,21 @@ static int pick_commits(struct replay_opts *opts)\n \treturn cleanup_sequencer_data();\n }\n \n+static int process_continuation(struct replay_opts *opts)\n+{\n+\tstruct commit_list *todo_list = NULL;\n+\tunsigned char sha1[20];\n+\n+\tread_and_refresh_cache(me, opts);\n+\n+\twalk_revs_populate_todo(&todo_list, opts);\n+\tif (!get_sha1(\"HEAD\", sha1))\n+\t\tpersist_head(sha1_to_hex(sha1));\n+\tpersist_todo(todo_list, opts);\n+\n+\treturn pick_commits(todo_list, opts);\n+}\n+\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tint res;\n@@ -714,7 +721,11 @@ 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+\n+\t/* Decide what to do depending on the arguments; a fresh\n+\t cherry-pick should be handled differently from an existing\n+\t one that is being continued */\n+\tres = process_continuation(&opts);\n \tif (res > 0)\n \t\t/* Exit status from conflict */\n \t\treturn res;\n@@ -734,7 +745,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":"170377","messageId":"1308661489-20080-13-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:48Z","receivedAt":"2011-06-21T13:04:48Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"When the sequencer data is persisted after a failed cherry-pick, don't\nallow subsequent calls to cherry-pick to clobber this state: instead,\nerror out with the complaint that an existing cherry-pick is in\nprogress. To fix existing tests and the \"rebase -i\" script, introduce\na new \"--skip-all\" command-line option to call after every failed\ncherry-pick; it essentially clears out the sequencer data, thereby\nallowing subsequent calls.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 53 ++++++++++++++++++++++++++++-------\n git-rebase--interactive.sh | 25 +++++++++++++---\n t/t3032-merge-recursive-options.sh | 2 +\n t/t3501-revert-cherry-pick.sh | 1 +\n t/t3502-cherry-pick-merge.sh | 9 +++++-\n t/t3504-cherry-pick-rerere.sh | 2 +\n t/t3505-cherry-pick-empty.sh | 14 ++++-----\n t/t3506-cherry-pick-ff.sh | 3 ++\n t/t3507-cherry-pick-conflict.sh | 24 +++++++++++++---\n t/t3510-cherry-pick-sequence.sh | 14 +++++++++\n t/t7502-commit.sh | 1 +\n 11 files changed, 117 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 5c0b97e..eb68068 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -46,6 +46,9 @@ enum replay_action { REVERT, CHERRY_PICK };\n struct replay_opts {\n \tenum replay_action action;\n \n+\t/* --skip-all */\n+\tint skipall_oper;\n+\n \t/* Boolean options */\n \tint edit;\n \tint record_origin;\n@@ -108,6 +111,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tint noop;\n \tstruct option options[] = {\n+\t\tOPT_BOOLEAN(0, \"skip-all\", &opts->skipall_oper, \"skip remaining instructions\"),\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 +140,21 @@ 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 incompatible command line arguments */\n+\tif (opts->skipall_oper) {\n+\t\tverify_opt_compatible(me, \"--skip-all\",\n+\t\t\t\t\"--no-commit\", opts->no_commit,\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@@ -624,12 +642,9 @@ static void persist_head(const char *head)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tif (file_exists(SEQ_DIR)) {\n-\t\tif (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {\n-\t\t\tstrbuf_release(&buf);\n-\t\t\tdie(_(\"Could not remove %s\"), SEQ_DIR);\n-\t\t}\n-\t} else {\n+\tif (file_exists(SEQ_DIR))\n+\t\tdie(_(\"%s already exists. Please examine and remove before continuing.\"), SEQ_DIR);\n+\telse {\n \t\tif (mkdir(SEQ_DIR, 0777) < 0) {\n \t\t\tstrbuf_release(&buf);\n \t\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), SEQ_DIR);\n@@ -701,12 +716,28 @@ static int process_continuation(struct replay_opts *opts)\n \n \tread_and_refresh_cache(me, opts);\n \n-\twalk_revs_populate_todo(&todo_list, opts);\n-\tif (!get_sha1(\"HEAD\", sha1))\n-\t\tpersist_head(sha1_to_hex(sha1));\n-\tpersist_todo(todo_list, opts);\n+\tif (opts->skipall_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\treturn cleanup_sequencer_data();\n+\t} else {\n+\t\t/* Start a new cherry-pick/ revert sequence; but\n+\t\t first, make sure that an existing one isn't in\n+\t\t progress */\n+\t\tif (file_exists(TODO_FILE)) {\n+\t\t\terror(_(\"A %s is already in progress\"), me);\n+\t\t\tadvise(_(\"Use %s --skip-all to forget about it\"), me);\n+\t\t\treturn -1;\n+\t\t}\n \n+\t\twalk_revs_populate_todo(&todo_list, opts);\n+\t\tif (!get_sha1(\"HEAD\", sha1))\n+\t\t\tpersist_head(sha1_to_hex(sha1));\n+\t\tpersist_todo(todo_list, opts);\n+\t}\n \treturn pick_commits(todo_list, 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)\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 65690af..e0c9bd8 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -136,6 +136,10 @@ make_patch () {\n \t\tget_author_ident_from_commit \"$1\" > \"$author_script\"\n }\n \n+clear_cherry_pick_state () {\n+\tgit cherry-pick --skip-all\n+}\n+\n die_with_patch () {\n \techo \"$1\" > \"$state_dir\"/stopped-sha\n \tmake_patch \"$1\"\n@@ -279,8 +283,10 @@ pick_one_preserving_merges () {\n \t\t\techo \"$sha1 $(git rev-parse HEAD^0)\" >> \"$rewritten_list\"\n \t\t\t;;\n \t\t*)\n-\t\t\toutput git cherry-pick \"$@\" ||\n+\t\t\toutput git cherry-pick \"$@\" || {\n+\t\t\t\tclear_cherry_pick_state\n \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n+\t\t\t}\n \t\t\t;;\n \t\tesac\n \t\t;;\n@@ -385,16 +391,20 @@ do_next () {\n \t\tcomment_for_reflog pick\n \n \t\tmark_action_done\n-\t\tpick_one $sha1 ||\n+\t\tpick_one $sha1 || {\n+\t\t\tclear_cherry_pick_state\n \t\t\tdie_with_patch $sha1 \"Could not apply $sha1... $rest\"\n+\t\t}\n \t\trecord_in_rewritten $sha1\n \t\t;;\n \treword|r)\n \t\tcomment_for_reflog reword\n \n \t\tmark_action_done\n-\t\tpick_one $sha1 ||\n+\t\tpick_one $sha1 || {\n+\t\t\tclear_cherry_pick_state\n \t\t\tdie_with_patch $sha1 \"Could not apply $sha1... $rest\"\n+\t\t}\n \t\tgit commit --amend --no-post-rewrite\n \t\trecord_in_rewritten $sha1\n \t\t;;\n@@ -402,8 +412,10 @@ do_next () {\n \t\tcomment_for_reflog edit\n \n \t\tmark_action_done\n-\t\tpick_one $sha1 ||\n+\t\tpick_one $sha1 || {\n+\t\t\tclear_cherry_pick_state\n \t\t\tdie_with_patch $sha1 \"Could not apply $sha1... $rest\"\n+\t\t}\n \t\techo \"$sha1\" > \"$state_dir\"/stopped-sha\n \t\tmake_patch $sha1\n \t\tgit rev-parse --verify HEAD > \"$amend\"\n@@ -438,7 +450,10 @@ do_next () {\n \t\techo \"$author_script_content\" > \"$author_script\"\n \t\teval \"$author_script_content\"\n \t\toutput git reset --soft HEAD^\n-\t\tpick_one -n $sha1 || die_failed_squash $sha1 \"$rest\"\n+\t\tpick_one -n $sha1 || {\n+\t\t\tclear_cherry_pick_state\n+\t\t\tdie_failed_squash $sha1 \"$rest\"\n+\t\t}\n \t\tcase \"$(peek_next_command)\" in\n \t\tsquash|s|fixup|f)\n \t\t\t# This is an intermediate commit; its message will only be\ndiff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\nindex 2b17311..50e8ca9 100755\n--- a/t/t3032-merge-recursive-options.sh\n+++ b/t/t3032-merge-recursive-options.sh\n@@ -113,8 +113,10 @@ test_expect_success '--ignore-space-change makes merge succeed' '\n test_expect_success 'naive cherry-pick fails' '\n \tgit read-tree --reset -u HEAD &&\n \ttest_must_fail git cherry-pick --no-commit remote &&\n+\tgit cherry-pick --skip-all &&\n \tgit read-tree --reset -u HEAD &&\n \ttest_must_fail git cherry-pick remote &&\n+\tgit cherry-pick --skip-all &&\n \ttest_must_fail git update-index --refresh &&\n \tgrep \"<<<<<<\" text.txt\n '\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 595d2ff..8970af4 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -96,6 +96,7 @@ test_expect_success 'revert forbidden on dirty working tree' '\n \techo content >extra_file &&\n \tgit add extra_file &&\n \ttest_must_fail git revert HEAD 2>errors &&\n+\tgit revert --skip-all &&\n \ttest_i18ngrep \"Your local changes would be overwritten by \" errors\n \n '\ndiff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh\nindex 0ab52da..cc132f8 100755\n--- a/t/t3502-cherry-pick-merge.sh\n+++ b/t/t3502-cherry-pick-merge.sh\n@@ -36,6 +36,7 @@ test_expect_success 'cherry-pick a non-merge with -m should fail' '\n \tgit reset --hard &&\n \tgit checkout a^0 &&\n \ttest_must_fail git cherry-pick -m 1 b &&\n+\tgit cherry-pick --skip-all &&\n \tgit diff --exit-code a --\n \n '\n@@ -45,6 +46,7 @@ test_expect_success 'cherry pick a merge without -m should fail' '\n \tgit reset --hard &&\n \tgit checkout a^0 &&\n \ttest_must_fail git cherry-pick c &&\n+\tgit cherry-pick --skip-all &&\n \tgit diff --exit-code a --\n \n '\n@@ -71,8 +73,8 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f\n \n \tgit reset --hard &&\n \tgit checkout b^0 &&\n-\ttest_must_fail git cherry-pick -m 3 c\n-\n+\ttest_must_fail git cherry-pick -m 3 c &&\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'revert a non-merge with -m should fail' '\n@@ -80,6 +82,7 @@ test_expect_success 'revert a non-merge with -m should fail' '\n \tgit reset --hard &&\n \tgit checkout c^0 &&\n \ttest_must_fail git revert -m 1 b &&\n+\tgit cherry-pick --skip-all &&\n \tgit diff --exit-code c\n \n '\n@@ -89,6 +92,7 @@ test_expect_success 'revert a merge without -m should fail' '\n \tgit reset --hard &&\n \tgit checkout c^0 &&\n \ttest_must_fail git revert c &&\n+\tgit cherry-pick --skip-all &&\n \tgit diff --exit-code c\n \n '\n@@ -116,6 +120,7 @@ test_expect_success 'revert a merge relative to nonexistent parent should fail'\n \tgit reset --hard &&\n \tgit checkout c^0 &&\n \ttest_must_fail git revert -m 3 c &&\n+\tgit cherry-pick --skip-all &&\n \tgit diff --exit-code c\n \n '\ndiff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh\nindex e6a6481..e7be835 100755\n--- a/t/t3504-cherry-pick-rerere.sh\n+++ b/t/t3504-cherry-pick-rerere.sh\n@@ -29,6 +29,7 @@ test_expect_success 'fixup' '\n \n test_expect_success 'cherry-pick conflict' '\n \ttest_must_fail git cherry-pick master &&\n+\tgit cherry-pick --skip-all &&\n \ttest_cmp expect foo\n '\n \n@@ -39,6 +40,7 @@ test_expect_success 'reconfigure' '\n \n test_expect_success 'cherry-pick conflict without rerere' '\n \ttest_must_fail git cherry-pick master &&\n+\tgit cherry-pick --skip-all &&\n \ttest_must_fail test_cmp expect foo\n '\n \ndiff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\nindex c10b28c..c8d828f 100755\n--- a/t/t3505-cherry-pick-empty.sh\n+++ b/t/t3505-cherry-pick-empty.sh\n@@ -23,10 +23,9 @@ test_expect_success setup '\n '\n \n test_expect_success 'cherry-pick an empty commit' '\n-\tgit checkout master && {\n-\t\tgit cherry-pick empty-branch^\n-\t\ttest \"$?\" = 1\n-\t}\n+\tgit checkout master &&\n+\ttest_expect_code 1 git cherry-pick empty-branch^\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'index lockfile was removed' '\n@@ -36,10 +35,9 @@ test_expect_success 'index lockfile was removed' '\n '\n \n test_expect_success 'cherry-pick a commit with an empty message' '\n-\tgit checkout master && {\n-\t\tgit cherry-pick empty-branch\n-\t\ttest \"$?\" = 1\n-\t}\n+\tgit checkout master &&\n+\ttest_expect_code 1 git cherry-pick empty-branch &&\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'index lockfile was removed' '\ndiff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh\nindex 51ca391..de7368e 100755\n--- a/t/t3506-cherry-pick-ff.sh\n+++ b/t/t3506-cherry-pick-ff.sh\n@@ -67,12 +67,14 @@ test_expect_success 'merge setup' '\n test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '\n \tgit reset --hard A -- &&\n \ttest_must_fail git cherry-pick --ff -m 1 B &&\n+\tgit cherry-pick --skip-all &&\n \tgit diff --exit-code A --\n '\n \n test_expect_success 'cherry pick a merge with --ff but without -m should fail' '\n \tgit reset --hard A -- &&\n \ttest_must_fail git cherry-pick --ff C &&\n+\tgit cherry-pick --skip-all &&\n \tgit diff --exit-code A --\n '\n \n@@ -93,6 +95,7 @@ test_expect_success 'cherry pick with --ff a merge (2)' '\n test_expect_success 'cherry pick a merge relative to nonexistent parent with --ff should fail' '\n \tgit reset --hard B -- &&\n \ttest_must_fail git cherry-pick --ff -m 3 C\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'cherry pick a root commit with --ff' '\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 212ec54..07cd805 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -39,6 +39,7 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '\n \n \thead=$(git rev-parse HEAD) &&\n \ttest_must_fail git cherry-pick picked &&\n+\tgit cherry-pick --skip-all &&\n \tnewhead=$(git rev-parse HEAD) &&\n \n \ttest \"$head\" = \"$newhead\"\n@@ -55,6 +56,7 @@ test_expect_success 'advice from failed cherry-pick' \"\n \thint: and commit the result with 'git commit'\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n+\tgit cherry-pick --skip-all &&\n \n \ttest_i18ncmp expected actual\n \"\n@@ -62,7 +64,8 @@ test_expect_success 'advice from failed cherry-pick' \"\n test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked &&\n-\ttest_cmp_rev picked CHERRY_PICK_HEAD\n+\ttest_cmp_rev picked CHERRY_PICK_HEAD &&\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '\n@@ -84,13 +87,15 @@ test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n \t\texport GIT_CHERRY_PICK_HELP &&\n \t\ttest_must_fail git cherry-pick picked\n \t) &&\n-\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n+\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \n \ttest_must_fail git cherry-pick picked &&\n+\tgit cherry-pick --skip-all &&\n \tgit reset &&\n \n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n@@ -102,7 +107,8 @@ test_expect_success 'failed commit does not clear CHERRY_PICK_HEAD' '\n \ttest_must_fail git cherry-pick picked &&\n \ttest_must_fail git commit &&\n \n-\ttest_cmp_rev picked CHERRY_PICK_HEAD\n+\ttest_cmp_rev picked CHERRY_PICK_HEAD &&\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' '\n@@ -119,7 +125,8 @@ test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' '\n \t\ttest_must_fail git commit\n \t) &&\n \n-\ttest_cmp_rev picked CHERRY_PICK_HEAD\n+\ttest_cmp_rev picked CHERRY_PICK_HEAD &&\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '\n@@ -130,13 +137,15 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '\n \tgit add foo &&\n \tgit commit &&\n \n-\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n+\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&\n+\tgit cherry-pick --skip-all\n '\n \n test_expect_success 'failed cherry-pick produces dirty index' '\n \tpristine_detach initial &&\n \n \ttest_must_fail git cherry-pick picked &&\n+\tgit cherry-pick --skip-all &&\n \n \ttest_must_fail git update-index --refresh -q &&\n \ttest_must_fail git diff-index --exit-code HEAD\n@@ -160,6 +169,7 @@ test_expect_success 'failed cherry-pick registers participants in index' '\n \tgit read-tree -u --reset HEAD &&\n \n \ttest_must_fail git cherry-pick picked &&\n+\tgit cherry-pick --skip-all &&\n \tgit ls-files --stage --unmerged > actual &&\n \n \ttest_cmp expected actual\n@@ -176,6 +186,7 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' '\n \tEOF\n \n \ttest_must_fail git cherry-pick picked &&\n+\tgit cherry-pick --skip-all &&\n \n \tsed \"s/[a-f0-9]*\\.\\.\\./objid/\" foo > actual &&\n \ttest_cmp expected actual\n@@ -195,6 +206,7 @@ test_expect_success 'diff3 -m style' '\n \tEOF\n \n \ttest_must_fail git cherry-pick picked &&\n+\tgit cherry-pick --skip-all &&\n \n \tsed \"s/[a-f0-9]*\\.\\.\\./objid/\" foo > actual &&\n \ttest_cmp expected actual\n@@ -227,6 +239,7 @@ test_expect_success 'revert also handles conflicts sanely' '\n \n \thead=$(git rev-parse HEAD) &&\n \ttest_must_fail git revert picked &&\n+\tgit revert --skip-all &&\n \tnewhead=$(git rev-parse HEAD) &&\n \tgit ls-files --stage --unmerged > actual-stages &&\n \n@@ -252,6 +265,7 @@ test_expect_success 'revert conflict, diff3 -m style' '\n \tEOF\n \n \ttest_must_fail git revert picked &&\n+\tgit revert --skip-all &&\n \n \tsed \"s/[a-f0-9]*\\.\\.\\./objid/\" foo > actual &&\n \ttest_cmp expected actual\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 83e2722..a797ae3 100644\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -34,4 +34,18 @@ test_expect_success 'cherry-pick cleans up sequencer directory upon success' '\n \ttest_path_is_missing .git/sequencer\n '\n \n+test_expect_success '--skip-all complains when no cherry-pick is in progress' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --skip-all >actual 2>&1 &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success '--skip-all cleans up sequencer directory' '\n+\tpristine_detach initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\ttest_must_fail git cherry-pick base..picked &&\n+\tgit cherry-pick --skip-all &&\n+\ttest_must_fail test -d .git/sequencer\n+'\n+\n test_done\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 3f3adc3..50d474d 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -291,6 +291,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' '\n \tgit commit -m second &&\n \t# Must fail due to conflict\n \ttest_must_fail git cherry-pick -n master &&\n+\tgit cherry-pick --skip-all &&\n \techo \"editor not started\" >.git/result &&\n \t(\n \t\tGIT_EDITOR=\"$(pwd)/.git/FAKE_EDITOR\" &&\n-- \n1.7.5.GIT\n"},{"id":"170376","messageId":"1308661489-20080-14-git-send-email-artagnon@gmail.com","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"[RFC PATCH 13/13] revert: Introduce --continue to continue the operation","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-21T13:04:49Z","receivedAt":"2011-06-21T13:04:49Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"After resolving a conflict, the user has the option to continue the\noperation. Mixing operations (cherry-pick and revert in the same\ntodo_list) and command-line options are unsupported at this stage.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 127 +++++++++++++++++++++++++++++++++++++-\n t/t3510-cherry-pick-sequence.sh | 28 +++++++++\n 2 files changed, 151 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex eb68068..7c28268 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -46,8 +46,9 @@ enum replay_action { REVERT, CHERRY_PICK };\n struct replay_opts {\n \tenum replay_action action;\n \n-\t/* --skip-all */\n+\t/* --skip-all and --continue */\n \tint skipall_oper;\n+\tint continue_oper;\n \n \t/* Boolean options */\n \tint edit;\n@@ -106,12 +107,37 @@ 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)\n+\t\tgoto ok;\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+ok:\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, \"skip-all\", &opts->skipall_oper, \"skip remaining instructions\"),\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@@ -141,9 +167,21 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\tPARSE_OPT_KEEP_ARGV0 |\n \t\t\t\t\tPARSE_OPT_KEEP_UNKNOWN);\n \n+\t/* Check for mutually incompatible command line arguments */\n+\tverify_opt_mutually_compatible(me,\n+\t\t\t\t\"--skip-all\", opts->skipall_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->skipall_oper) {\n-\t\tverify_opt_compatible(me, \"--skip-all\",\n+\tif (opts->skipall_oper || opts->continue_oper) {\n+\t\tchar *this_oper;\n+\t\tif (opts->skipall_oper)\n+\t\t\tthis_oper = \"--skip-all\";\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\"--signoff\", opts->signoff,\n \t\t\t\t\"--mainline\", opts->mainline,\n@@ -614,6 +652,78 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t}\n }\n \n+static void read_populate_todo(struct commit_list **todo_list,\n+\t\t\tstruct replay_opts *opts)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tenum replay_action action;\n+\tstruct commit *commit;\n+\tstruct commit_list *new_item;\n+\tstruct commit_list *cur = NULL;\n+\tunsigned char commit_sha1[20];\n+\tchar sha1_abbrev[40];\n+\tchar *p;\n+\tint insn_len = 0;\n+\tchar *insn;\n+\tint fd;\n+\n+\tfd = open(TODO_FILE, O_RDONLY);\n+\tif (fd < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie_errno(_(\"Could not open %s.\"), TODO_FILE);\n+\t}\n+\tif (strbuf_read(&buf, fd, 0) < buf.len) {\n+\t\tclose(fd);\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Could not read %s.\"), TODO_FILE);\n+\t}\n+\tclose(fd);\n+\n+\tfor (p = buf.buf; *p; p = strchr(p, '\\n') + 1) {\n+\t\tinsn = p;\n+\t\tif (!(p = strchr(p, ' ')))\n+\t\t\tgoto error;\n+\t\tinsn_len = p - insn;\n+\t\tif (!(p = strchr(p + 1, ' ')))\n+\t\t\tgoto error;\n+\t\tp += 1;\n+\t\tstrlcpy(sha1_abbrev, insn + insn_len + 1,\n+\t\t\tp - (insn + insn_len + 1));\n+\n+\t\tif (!strncmp(insn, \"pick\", insn_len))\n+\t\t\taction = CHERRY_PICK;\n+\t\telse if (!strncmp(insn, \"revert\", insn_len))\n+\t\t\taction = REVERT;\n+\t\telse\n+\t\t\tgoto error;\n+\n+\t\t/* Verify that the action matches up with the one in\n+\t\t opts; we don't support arbitrary instructions */\n+\t\tif (action != opts->action)\n+\t\t\tgoto error;\n+\n+\t\t/* Now create a commit corresponding to sha1_abbrev\n+\t\t and put it into the todo_list */\n+\t\tif ((get_sha1(sha1_abbrev, commit_sha1) < 0)\n+\t\t\t|| !(commit = lookup_commit_reference(commit_sha1)))\n+\t\t\tgoto error;\n+\t\tnew_item = xmalloc(sizeof(struct commit_list));\n+\t\tnew_item->item = commit;\n+\t\tif (cur)\n+\t\t\tcur->next = new_item;\n+\t\telse\n+\t\t\t*todo_list = new_item;\n+\t\tcur = new_item;\n+\t}\n+\tcur->next = NULL;\n+\tstrbuf_release(&buf);\n+\n+\treturn;\n+error:\n+\tstrbuf_release(&buf);\n+\tdie(_(\"Malformed instruction sheet: %s\"), TODO_FILE);\n+}\n+\n static void walk_revs_populate_todo(struct commit_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n@@ -720,13 +830,22 @@ static int process_continuation(struct replay_opts *opts)\n \t\tif (!file_exists(TODO_FILE))\n \t\t\tgoto error;\n \t\treturn cleanup_sequencer_data();\n+\t} else if (opts->continue_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t\tread_populate_todo(&todo_list, opts);\n+\n+\t\t/* Verify that the conflict has been resolved */\n+\t\tif (!index_differs_from(\"HEAD\", 0))\n+\t\t\ttodo_list = todo_list->next;\n \t} else {\n \t\t/* Start a new cherry-pick/ revert sequence; but\n \t\t first, make sure that an existing one isn't in\n \t\t progress */\n \t\tif (file_exists(TODO_FILE)) {\n \t\t\terror(_(\"A %s is already in progress\"), me);\n-\t\t\tadvise(_(\"Use %s --skip-all to forget about it\"), me);\n+\t\t\tadvise(_(\"Use %s --continue to continue the operation\"), me);\n+\t\t\tadvise(_(\"or --skip-all to skip all the remaining instructions\"), me);\n \t\t\treturn -1;\n \t\t}\n \ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex a797ae3..159be55 100644\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -48,4 +48,32 @@ test_expect_success '--skip-all cleans up sequencer directory' '\n \ttest_must_fail test -d .git/sequencer\n '\n \n+test_expect_success '--continue complains when no cherry-pick is in progress' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --continue >actual 2>&1 &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success '--continue complains when there are unresolved conflicts' '\n+\tpristine_detach initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\ttest_must_fail git cherry-pick base..picked &&\n+\ttest_must_fail git cherry-pick --continue &&\n+\tgit cherry-pick --skip-all\n+'\n+\n+test_expect_success '--continue continues after conflicts are resolved' '\n+\tpristine_detach initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\ttest_must_fail git cherry-pick base..picked &&\n+\techo \"resolved\" >foo &&\n+\tgit add foo &&\n+\tgit commit &&\n+\tgit cherry-pick --continue &&\n+\ttest_must_fail test -d .git/sequencer &&\n+\tgit rev-list --count HEAD >actual &&\n+\techo 3 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.5.GIT\n"},{"id":"170390","messageId":"20110621154808.GE15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 00/13] Sequencer with continuation features","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T15:48:09Z","receivedAt":"2011-06-21T15:48:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamkumar Ramachandra wrote:\n\n> I think I can safely say that\n> I'm quite happy with the general state of this series now.\n\nIt's certainly starting to shape up. I'll quickly review the style\nand commit messages now, since the part I was lobbying for most has\nbeen dealt with (hoorah!) and the rest of the substantive part seems\nto have some potential changes queued up if I understand correctly.\nIn particular, I have high hopes for changes rippling through once\ntests get added to make the detailed behavior more clear. :)\n\nGeneral rule of thumb about style, especially commit messages: if\nsomething looks wrong, it is. Which is to say, the goal is to make\nthe code easy to read and hack on, and part of that means avoiding\nthat moment of surprise that might interfere with someone getting on\nwith the task at hand. If I seem to be not making sense, please as\nusual do not assume I am right, but think carefully about it and\n(ideally) gently correct me if I'm wrong.\n\nOkay, on to the patches. The general shape of the series is a little\nodd --- some die() elimination, removal of globals in preparation for\nmaking this a reusable API, and then the title feature that does not\nrely on any of the patches before it. Still, it makes a kind of sense\nfrom the point of view of development:\n\n 1. First, getting to know the API and using that process to come\n up with obvious improvements.\n\n 2. Next, implementing the new feature.\n\nIf I had been writing it, I would have rebased the last 4 patches\nagainst master as a new series (so they could be integrated more\nquickly), but I don't mind reading it this way.\n\nRegards,\nJonathan\n"},{"id":"170391","messageId":"20110621155555.GF15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 01/13] advice: Introduce error_resolve_conflict","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T15:55:55Z","receivedAt":"2011-06-21T15:55:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Introduce error_resolve_conflict corresponding to\n> die_resolve_conflict, and implement the latter function in terms of\n> the former. The only trade-off is that die_resolve_conflict is a\n> little noisier now.\n\nThe above doesn't tell me what I would want to know, namely:\n\n 1. The impact of this patch is to change an error message.\n\n 2. The change is from\n\n\tfatal: 'commit' is not possible because you have unmerged files.\n\tPlease, fix them up in the work tree ...\n\t... etc, etc ...\n\n to\n\n\terror: 'commit' is not ...\n\terror: Please, fix them up...\n\t... etc, etc ...\n\tfatal: Exiting because of an unresolved conflict.\n\n 3. The intended benefit is that new, future callers may want the \"error\"\n without exiting.\n\nNotice that after writing the above, a little detail jumps out:\nnamely, the second \"error:\" line is giving advice, so it might make\nsense to make it say \"hint:\" instead.\n"},{"id":"170392","messageId":"20110621155836.GG15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 02/13] revert: Factor out add_message_to_msg function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T15:58:36Z","receivedAt":"2011-06-21T15:58:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> [Subject: [PATCH 02/13] revert: Factor out add_message_to_msg function]\n\n\"Factor out\" means to introduce a new function. I think you mean\n\"inline\". :)\n\n> The add_message_to_msg function is poorly implemented, has an unclear\n> API, and only one callsite. Replace the callsite with a cleaner\n> implementation.\n\nThe above does not answer the question I would have, namely what\nexactly is wrong with add_message_to_msg. Is it too slow? Not\nrobust? Is the generated assembly too long? Is it hard for a reader\nto figure out the intent? Does it blend in poorly with its\nsurroundings?\n"},{"id":"170394","messageId":"20110621160306.GH15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 03/13] revert: Don't check lone argument in get_encoding","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T16:03:06Z","receivedAt":"2011-06-21T16:03:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> The get_encoding function has only one callsite, and its caller makes\n> sure that a NULL argument isn't passed. Don't unnecessarily double\n> check the same argument in get_encoding.\n\nYes, this one's pretty good, though I suspect the actual reason for\nthe patch is not just dead code elimination.\n\nThe patch itself gives reason to be comfortable: the \"while\" loop\nright after the removed \"if\" checks the value of *p immediately, so if\ncallers start passing NULL, it will be pretty obvious.\n\nI suspect the actual motivation is to avoid access to the \"commit\"\nglobal, in preparation for making it local. A nice side-benefit is\nremoving a translated message.\n"},{"id":"170397","messageId":"20110621162232.GI15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T16:22:32Z","receivedAt":"2011-06-21T16:22:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Earlier, revert_or_cherry_pick only used to return a non-negative\n> number or die. Change this so that it can return negative values too;\n> postive return values indicate conflicts, while negative ones indicate\n> other errors, and this return status is propogated updwards from\n> do_pick_commit, to be finally handled in cmd_cherry_pick and\n> cmd_revert. While revert_or_cherry_pick can still die due to several\n> other reasons, this patches attempts to factor out some of the die\n> calls. In the same spirit, also introduce a new function\n> error_dirty_index, based on die_dirty_index which prints some hints\n> and returns an error to its caller do_pick_commit.\n\nThe \"Earlier\" combined with imperfect tense somehow oddly trips me up\n(somehow it does not register that you mean \"Before this patch\"). I\nusually find using the present or the simple past for the status quo\nis clearer (\"Currently the return value from revert_or_cherry_pick is\na non-negative integer representing the intended exit status from git\nrevert or git cherry-pick. Change that by ...\").\n\nCarrying forward from that confusion, it's hard to concentrate hard\nenough to parse and figure out the rest. That shouldn't be necessary:\nby setting the scene with an explanation of the problem being solved\nit should be possible to make this much simpler.\n\nSuppose, forgetting the above, that I asked you to explain what this\npatch is about. You'd say it's about teaching \"revert_or_cherry_pick\"\nto return error() instead of die()-ing a little more often, right?\nThen if I asked you why, you might say one of many things --- for\nexample, that in the long term the hope is to allow speeding up\nmultiple-cherry-pick by not writing CHERRY_PICK_HEAD et al until a\nsingle-commit pick fails, and that die()-ing interferes with that by\nnot moving the thread of control to the caller that wants to write\nfiles like CHERRY_PICK_HEAD on error. After that, I might ask what\nthis patch does to address that, and you might tell me that in\naddition to converting various die() calls to \"return error()\", it\nupdates the callers to cope with the negative return codes that could\nresult from that. Eventually the negative value gets returned from\nrevert_or_cherry_pick; currently the convention there is to return a\nnonnegative integer (representing an appropriate \"normal\" exit status\nfor the command) but in the new regime it can also be -1, meaning the\ncommand failed and should die(). Finally if I asked you what the\nimpact of applying this patch is, you might tell me that the benefits\nwill require converting more die() calls so the only immediate impact\nshould be to change various messages from \"fatal:\" to \"error:\" and add\na new \n\n\tfatal: cherry-pick failed\n\nat the end.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -250,25 +250,15 @@ static struct tree *empty_tree(void)\n[...]\n> @@ -582,14 +572,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\tdie(_(\"%s failed\"), me);\n> +\treturn 0;\n> }\n\nWith the above explanation in mind, it seems simpler to say\n\n\tif (res < 0)\n\t\tdie(...);\n\treturn res;\n\nthe idea being that there are two cases --- the new \"die for failure\"\ncase and the old \"pass on the exit status requested by\nrevert_or_cherry_pick\" case.\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\tdie(_(\"%s failed\"), me);\n> +\treturn 0;\n\nShould the \"revert\" or \"cherry-pick\" here be part of the message\nmarked for translation? A translator might want to paraphrase to\n\n\tfatal: failed to cherry-pick\n\nif that is more natural in the language at hand.\n"},{"id":"170398","messageId":"20110621165233.GK15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 05/13] revert: Eliminate global \"commit\" variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T16:52:34Z","receivedAt":"2011-06-21T16:52:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamkumar Ramachandra wrote:\n\n> Since we want to develop the functionality to either pick or revert\n> individual commits atomically later in the series, make \"commit\" a\n> local variable. Doing this involves changing several functions to\n> take an additional argument, but no other functional changes.\n\nOkay, one more message review. Suppose I asked you what this patch\ndoes. You'd say it makes \"commit\" no longer a static variable,\npassing it around instead. Next I ask you why. The answer would\npresumably be something like \"because ... static variables are bad\",\nor \"because passing it around explicitly makes it easier for a person\nreading the code to see how the state evolves\", or \"because some day\nwe will want this to be thread-safe, meaning there could be more than\none commit being cherry-picked at the same time\". Right?\n"},{"id":"170399","messageId":"20110621165854.GL15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-8-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 07/13] revert: Introduce struct to keep command-line options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T16:58:54Z","receivedAt":"2011-06-21T16:58:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> The current code uses a set of file-scope static variables to tell the\n> cherry-pick/ revert machinery how to replay the changes, and\n> initializes them by parsing the command-line arguments. In later\n> steps in this series, we would like to introduce an API function that\n> calls into this machinery directly and have a way to tell it what to\n> do. Hence, introduce a structure to group these variables, so that\n> the API can take them as a single replay_options parameter.\n\nThe struct leaves out the variable \"me\". A person might wonder why\nthe above rationale applies to the other variables but not that one. :)\n\n> Unfortunately, parsing strategy-option violates a C89 rule:\n> Initializers cannot refer to variables whose address is not known at\n> compile time.\n\nReading this, one is led to wonder:\n\n - is this a regression?\n - is it a necessary consequence of the positive change?\n - will it matter --- i.e., does the rest of git follow that rule to\n be able to build on old-fashioned compilers, and is there any\n intention to?\n - how can I get my compiler to tell me about it?\n - is it fundamental or not? i.e., is this a \"we introduced this\n minor regression and expect it will be fixed later\" or \"this points\n to a flaw in the design and we hope someone comes up with a better\n design that doesn't have that problem\"?\n\nBy the way, as a side note, here are a couple of patches about the\nsame subject that I use to test with \"-std=c89 -pedantic\" privately.\n\n http://repo.or.cz/w/git/jrn.git/commit/faa4b89731e5411fbdf6812f11748a20e664361d\n http://repo.or.cz/w/git/jrn.git/commit/92a0c179cb7690bdf6f8fbbfa2253b883d02bd17\n"},{"id":"170400","messageId":"20110621170030.GM15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-9-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 08/13] revert: Separate cmdline parsing from functional code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T17:00:30Z","receivedAt":"2011-06-21T17:00:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Currently, revert_or_cherry_pick does too many things including\n> argument parsing and setting up to pick the commits; this doesn't make\n> a good API. Simplify and rename the function to pick_commits, so that\n> it just has the responsibility of setting up the revision walker and\n> calling do_pick_commit in a loop. Transfer the remaining work to its\n> callers cmd_cherry_pick and cmd_revert.\n\nMakes sense to me. :)\n\n> Later in the series,\n> pick_commits will serve as the starting point for continuation.\n\nThis sentence is a little cryptic. The naive reader wonders\n\"continuation of what?\"\n"},{"id":"170401","messageId":"20110621170416.GN15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-10-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 09/13] revert: Catch incompatible command-line options early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T17:04:17Z","receivedAt":"2011-06-21T17:04:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Earlier, incompatible command-line options used to be caught in\n> pick_commits after parse_args has parsed the options and populated the\n> options structure. Instead, hand over the responsibility of catching\n> incompatible command-line options to parse_args so that the program\n> can die early.\n\nThe \"Earlier ... used to\" phrasing is tripping me up again.\n\nThe naive reader (i.e., me) wonders: how long does this option parsing\nand populating the options structure that we want to delay until after\nverify_opt_compatible take? Does that delay matter or is there some\nother reason for this change?\n\n> Also write a verify_opt_compatible function to handle\n> incompatible options in a general manner.\n\nNitpick: the commit message generally commands the program or the\ncodebase to behave in a certain way, but in this example it is\ncommanding the author. I'd say \"add\" or \"provide\".\n"},{"id":"170402","messageId":"20110621171139.GO15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-11-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 10/13] revert: Persist data for continuation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T17:11:39Z","receivedAt":"2011-06-21T17:11:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than\n> one commit, 2010-06-02), a single invocation of \"git cherry-pick\" or\n> \"git revert\" can perform picks of several individual commits. To\n> implement features like \"--continue\" to continue the whole operation,\n> we will need to store some information about the state and the plan at\n> the beginning. Introduce a \".git/sequencer/head\" file to store this\n> state, and \".git/sequencer/todo\" file to store the plan.\n\nMakes a lot of sense.\n\n> Don't touch\n> CHERRY_PICK_HEAD -- it will still be useful when a conflict is\n> encountered.\n\nI think there's some logical connector or something missing. Why would\nintroducing a .git/sequencer dir involve touching CHERRY_PICK_HEAD?\n\nMaybe the idea is to say: \"The purpose of these new files is orthogonal\nto the existing CHERRY_PICK_HEAD\" with some explanation of that.\n\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,10 @@\n> * Copyright (c) 2005 Junio C Hamano\n> */\n> \n> +#define SEQ_DIR\t\tgit_path(\"sequencer\")\n> +#define HEAD_FILE\tgit_path(\"sequencer/head\")\n> +#define TODO_FILE\tgit_path(\"sequencer/todo\")\n\nI've failed to convince you in the past that these fake constants are\nscary, but believe me, they really are. Consider the following code\n--- what would you expect it to print? What does it actually print?\n(Hint: there's not one right answer.)\n\n\tprintf(\"%s %s %s %s %s\\n\", SEQ_DIR, HEAD_FILE, TODO_FILE,\n\t\t\t\t\tSEQ_DIR, HEAD_FILE);\n\n> +static void walk_revs_populate_todo(struct commit_list **todo_list,\n> +\t\t\t\tstruct replay_opts *opts)\n> {\n> \tstruct rev_info revs;\n> \tstruct commit *commit;\n> +\tstruct commit_list *new_item;\n> +\tstruct commit_list *cur = NULL;\n> +\n> +\t/* Insert into todo_list in the same order */\n> +\tprepare_revs(&revs, opts);\n> +\twhile ((commit = get_revision(&revs))) {\n> +\t\tnew_item = xmalloc(sizeof(struct commit_list));\n> +\t\tnew_item->item = commit;\n> +\t\tif (cur)\n> +\t\t\tcur->next = new_item;\n> +\t\telse\n> +\t\t\t*todo_list = new_item;\n> +\t\tcur = new_item;\n> +\t}\n> +\tcur->next = NULL;\n\nThe naive reader, perhaps stupidly, wonders: \"why not use\ncommit_list_insert\"? A comment or something to explain \"NEEDSWORK:\nexpose this as commit_list_append\" could help him.\n\n[...]\n> -\treturn 0;\n> +\t/* Sequence of picks finished successfully; cleanup by\n> +\t removing the .git/sequencer directory */\n> +\treturn cleanup_sequencer_data();\n\nGNU-style comment seems to have snuck in.\n\nThanks; this one was pretty pleasant.\n"},{"id":"170404","messageId":"20110621171933.GP15461@elie","threadId":"27674","inReplyTo":"1308661489-20080-14-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH 13/13] revert: Introduce --continue to continue the operation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T17:19:34Z","receivedAt":"2011-06-21T17:19:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> After resolving a conflict, the user has the option to continue the\n> operation.\n\nWhen reading this, I wonder: \"does the user have that option or will\nshe gain that option after this patch? And if the latter, what does\nthe UI look like? And what is the purpose --- if I just cherry-picked\na commit and ran into a conflict, won't 'git commit' be good enough?\"\n\n> Mixing operations (cherry-pick and revert in the same\n> todo_list) and command-line options are unsupported at this stage.\n\nIt's not obvious what this means.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n[...]\n> @@ -614,6 +652,78 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,\n> \t}\n> }\n> \n> +static void read_populate_todo(struct commit_list **todo_list,\n> +\t\t\tstruct replay_opts *opts)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tenum replay_action action;\n> +\tstruct commit *commit;\n> +\tstruct commit_list *new_item;\n> +\tstruct commit_list *cur = NULL;\n> +\tunsigned char commit_sha1[20];\n> +\tchar sha1_abbrev[40];\n> +\tchar *p;\n> +\tint insn_len = 0;\n> +\tchar *insn;\n> +\tint fd;\n\nThe naive reader is not sure what this function is about and is\nintimidated by its size. (I know you have plans regarding that\nalready; just mentioning it so it doesn't get forgotten.)\n\n> +\t\t/* Verify that the action matches up with the one in\n> +\t\t opts; we don't support arbitrary instructions */\n\nI haven't reviewed the function in full, but this GNU-style comment\njumped out on the way down while scrolling. I believe you already\nknow what comments should look like to be consistent with the rest\nof the git codebase.\n\nOk, I've reached the end. Probably some of these comments apply to\ncode that might be changed anyway, but still, hope that helps a\nlittle. And thanks for working on this.\n"},{"id":"170409","messageId":"7vtybj81ox.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"20110621155555.GF15461@elie","subject":"Re: [PATCH 01/13] advice: Introduce error_resolve_conflict","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T18:43:58Z","receivedAt":"2011-06-21T18:43:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Ramkumar Ramachandra wrote:\n>\n>> Introduce error_resolve_conflict corresponding to\n>> die_resolve_conflict, and implement the latter function in terms of\n>> the former. The only trade-off is that die_resolve_conflict is a\n>> little noisier now.\n>\n> The above doesn't tell me what I would want to know, namely:\n>\n> 1. The impact of this patch is to change an error message.\n>\n> 2. The change is from\n>\n> \tfatal: 'commit' is not possible because you have unmerged files.\n> \tPlease, fix them up in the work tree ...\n> \t... etc, etc ...\n>\n> to\n>\n> \terror: 'commit' is not ...\n> \terror: Please, fix them up...\n> \t... etc, etc ...\n> \tfatal: Exiting because of an unresolved conflict.\n>\n> 3. The intended benefit is that new, future callers may want the \"error\"\n> without exiting.\n\nYour good suggestions for inexperienced people are always appreciated.\n\n> Notice that after writing the above, a little detail jumps out:\n> namely, the second \"error:\" line is giving advice, so it might make\n> sense to make it say \"hint:\" instead.\n\nYes, and also I suspect that it shouldn't be hard to do this refactoring\nwithout changing the output.\n"},{"id":"170411","messageId":"7vmxhb80vp.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"20110621155836.GG15461@elie","subject":"Re: [PATCH 02/13] revert: Factor out add_message_to_msg function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T19:01:30Z","receivedAt":"2011-06-21T19:01:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> The add_message_to_msg function is poorly implemented, has an unclear\n>> API, and only one callsite. Replace the callsite with a cleaner\n>> implementation.\n>\n> The above does not answer the question I would have, namely what\n> exactly is wrong with add_message_to_msg. Is it too slow? Not\n> robust? Is the generated assembly too long? Is it hard for a reader\n> to figure out the intent? Does it blend in poorly with its\n> surroundings?\n\nI do not know if I _suggested_ this, but ...\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 1f27c63..d6d2356 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -185,19 +185,6 @@ 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\nThis is a loop that manually implements strstr(\"\\n\\n\"); rewriting of which\nis a Good Thing.\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\nI am not sure if this corresponds to what the new code does. What happens\nif *p was NUL (i.e. we did not find any \"\\n\\n\" in the \"message\" at all)?\nWe go two bytes beyond that NUL and run addstr from that unknown space?\nThe new code does not seem to do that ;-)\n\nI suspect that this was a bug from the original C-rewrite of git-revert\nthat was done by 9509af6 (Make git-revert & git-cherry-pick a builtin,\n2007-03-01), and the new code fixes the bug, but then it would be nice to\nnote that in the commit log message.\n\n> @@ -471,11 +458,17 @@ static int do_pick_commit(void)\n> \t\t}\n> \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n> \t} else {\n> +\t\tconst char *p = strstr(msg.message, \"\\n\\n\");\n> +\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> +\n> +\t\t/* Add msg.message to msgbuf */\n\nThis is an incorrect comment that confuses the reader. Think:\n\n - What did you skip by looking for the first \"\\n\\n\"?\n - What do you instead when no such \"\\n\\n\" is found?\n\nPerhaps what it is doing is: \"Add commit log message, if exists, or the\ncommit object name if it doesn't\"\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"},{"id":"170412","messageId":"7viprz8018.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"20110621162232.GI15461@elie","subject":"Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T19:19:47Z","receivedAt":"2011-06-21T19:19:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> +\tif (res < 0)\n>> +\t\tdie(_(\"%s failed\"), me);\n>> +\treturn 0;\n>\n> Should the \"revert\" or \"cherry-pick\" here be part of the message\n> marked for translation? A translator might want to paraphrase to\n>\n> \tfatal: failed to cherry-pick\n>\n> if that is more natural in the language at hand.\n\nWouldn't such a message file simply say\n\n\tmsgid \"%s failed\"\n msgstr \"failed to %s\"\n\nIOW, I am not sure what problem you are seeing.\n"},{"id":"170413","messageId":"7vei2n7zuj.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"1308661489-20080-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 05/13] revert: Eliminate global \"commit\" variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T19:23:48Z","receivedAt":"2011-06-21T19:23:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Since we want to develop the functionality to either pick or revert\n> individual commits atomically later in the series, make \"commit\" a\n> local variable. Doing this involves changing several functions to\n> take an additional argument, but no other functional changes.\n\nActually you didn't make it a local variable. You made it a parameter to\nbe passed around, which is even better ;-).\n\n> -static void write_cherry_pick_head(void)\n> +static void write_cherry_pick_head(const char *commit_sha1_hex)\n\nWouldn't we prefer to see this parameter also to be a commit objet, not a\npreformatted hex string? The only exception would be if you are going to\nintroduce a future callsite that does not have a commit object but may\nknow the commit object name, but if you are not going to do so, then...\n"},{"id":"170414","messageId":"7vaadb7zn4.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"20110621165854.GL15461@elie","subject":"Re: [PATCH 07/13] revert: Introduce struct to keep command-line options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T19:28:15Z","receivedAt":"2011-06-21T19:28:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Ramkumar Ramachandra wrote:\n> ...\n> The struct leaves out the variable \"me\". A person might wonder why\n> the above rationale applies to the other variables but not that one. :)\n\nGood point.\n\n>> Unfortunately, parsing strategy-option violates a C89 rule:\n>> Initializers cannot refer to variables whose address is not known at\n>> compile time.\n>\n> Reading this, one is led to wonder:\n> ...\n> - is this a regression?\n\nLet me add one.\n\n - how does this patch work around that issue?\n"},{"id":"170415","messageId":"20110621193217.GA17390@elie","threadId":"27674","inReplyTo":"7viprz8018.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T19:32:17Z","receivedAt":"2011-06-21T19:32:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jun 21, 2011 at 12:19:47PM -0700, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>>> +\tif (res < 0)\n>>> +\t\tdie(_(\"%s failed\"), me);\n>>> +\treturn 0;\n>>\n>> Should the \"revert\" or \"cherry-pick\" here be part of the message\n>> marked for translation? A translator might want to paraphrase to\n>>\n>> \tfatal: failed to cherry-pick\n>>\n>> if that is more natural in the language at hand.\n>\n> Wouldn't such a message file simply say\n>\n> \tmsgid \"%s failed\"\n> msgstr \"failed to %s\"\n>\n> IOW, I am not sure what problem you are seeing.\n\nAh, sorry for the lack of clarity. What I meant is that the noun\nand verb will be different words in many languages. There can also be\nproblems of subject/verb agreement. Also \"me\" is used elsewhere to\nhold the command name as typed on the command line even when LANG\npoints to a language other than English if I remember correctly.\n\nIf the message were in revert_or_cherry_pick instead of having two\nidentical copies in cmd_revert and cmd_cherry_pick, it would have been\nless striking (but still a potential problem).\n"},{"id":"170418","messageId":"7v39j37ynu.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"1308661489-20080-11-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 10/13] revert: Persist data for continuation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T19:49:25Z","receivedAt":"2011-06-21T19:49:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> ... Don't touch\n> CHERRY_PICK_HEAD -- it will still be useful when a conflict is\n> encountered.\n\nWhat does \"Don't touch\" here mean? Keep doing the same thing as before so\nthat people can rely on it? Or stop touching it? I presume the former, but\nthe description is suboptimal.\n\n> @@ -25,6 +26,10 @@\n> * Copyright (c) 2005 Junio C Hamano\n> */\n> \n> +#define SEQ_DIR\t\tgit_path(\"sequencer\")\n> +#define HEAD_FILE\tgit_path(\"sequencer/head\")\n> +#define TODO_FILE\tgit_path(\"sequencer/todo\")\n\nHEAD is to keep track of the original, or does it get updated during a\nsequencer run?\n\n> +static void walk_revs_populate_todo(struct commit_list **todo_list,\n> +\t\t\t\tstruct replay_opts *opts)\n> {\n> \tstruct rev_info revs;\n> \tstruct commit *commit;\n> +\tstruct commit_list *new_item;\n> +\tstruct commit_list *cur = NULL;\n> +\n> +\t/* Insert into todo_list in the same order */\n> +\tprepare_revs(&revs, opts);\n> +\twhile ((commit = get_revision(&revs))) {\n> +\t\tnew_item = xmalloc(sizeof(struct commit_list));\n> +\t\tnew_item->item = commit;\n> +\t\tif (cur)\n> +\t\t\tcur->next = new_item;\n> +\t\telse\n> +\t\t\t*todo_list = new_item;\n> +\t\tcur = new_item;\n> +\t}\n> +\tcur->next = NULL;\n> +}\n\nYuck; you do not want if/else to do this (I am _not_ saying \"you can do\nthat but here is a better way\"). Have a variable that points at a pointer\nof type \"struct commit_list *\", initialize it to where todo_list points\nat, and then update to point at the next field of the new object.\n\nSomething like this:\n\n\tnext = todo_list;\n\twhile ((commit = ...) != NULL) {\n\t\tnew = xcalloc(1, sizeof(struct commit_list));\n new->item = commit;\n *next = new;\n next = &new->next;\n\t}\n\t*next = NULL;\n\nDoing it this way would avoid a segfault when get_revision() returned\nnothing as an added bonus ;-).\n\n> +static void persist_head(const char *head)\n\ns/persist/save/ perhaps?\n\n> +{\n> +\tstatic struct lock_file head_lock;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint fd;\n> +\n\n> +\tif (file_exists(SEQ_DIR)) {\n> +\t\tif (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {\n> +\t\t\tstrbuf_release(&buf);\n> +\t\t\tdie(_(\"Could not remove %s\"), SEQ_DIR);\n> +\t\t}\n> +\t} else {\n> +\t\tif (mkdir(SEQ_DIR, 0777) < 0) {\n> +\t\t\tstrbuf_release(&buf);\n> +\t\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), SEQ_DIR);\n> +\t\t}\n> +\t}\n\nSplit this part into a separate helper.\n\n> +\tfd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);\n> +\tstrbuf_addf(&buf, \"%s\\n\", head);\n> +\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n> +\t\tdie_errno(_(\"Could not write to %s.\"), HEAD_FILE);\n> +\tif (commit_lock_file(&head_lock) < 0)\n> +\t\tdie(_(\"Error wrapping up %s\"), HEAD_FILE);\n> +}\n\nIs it sufficient to write the object name of the commit HEAD points at?\nWould it be useful to also record the branch if the HEAD points at one?\n\n> +static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)\n\ns/persist/save/ perhaps?\n\n> +{\n> +\tstatic struct lock_file todo_lock;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint fd;\n> +\tfd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);\n\nThe current set of callers might always save head and then todo, but it\nprobably is a good idea to lift that restriction (which is why I suggested\nto separate the initialization of seq-dir into a separate helper).\n\n> +static int pick_commits(struct replay_opts *opts)\n> +{\n> +\tstruct commit_list *todo_list = NULL;\n> +\tunsigned char sha1[20];\n> +\tstruct commit_list *cur;\n> +\tint res;\n> \n> -\tsetenv(GIT_REFLOG_ACTION, me, 0);\n> \tread_and_refresh_cache(me, opts);\n> +\tsetenv(GIT_REFLOG_ACTION, me, 0);\n\nWhy?\n\n> -\tprepare_revs(&revs, opts);\n> +\twalk_revs_populate_todo(&todo_list, opts);\n> +\tif (!get_sha1(\"HEAD\", sha1))\n> +\t\tpersist_head(sha1_to_hex(sha1));\n> +\tpersist_todo(todo_list, opts);\n> \n> -\twhile ((commit = get_revision(&revs))) {\n> -\t\tint res = do_pick_commit(commit, opts);\n> +\tfor (cur = todo_list; cur; cur = cur->next) {\n> +\t\tpersist_todo(cur, opts);\n> +\t\tres = do_pick_commit(cur->item, opts);\n> \t\tif (res)\n> \t\t\treturn res;\n> \t}\n> \n> -\treturn 0;\n> +\t/* Sequence of picks finished successfully; cleanup by\n> +\t removing the .git/sequencer directory */\n\nJust a style nit.\n\n /*\n * We write our multi-line comments\n * this way.\n */\n\n> +\treturn cleanup_sequencer_data();\n> }\n"},{"id":"170419","messageId":"7vy60v6jn4.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"1308661489-20080-12-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 11/13] revert: Introduce a layer of indirection over pick_commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T19:59:11Z","receivedAt":"2011-06-21T19:59:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> -static int pick_commits(struct replay_opts *opts)\n> +static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n> {\n\nHmm, I was hoping that we would have something that can express a DAG, but\nthis interface seems to forever wed us with a linear history. Just making\na mental note, and not rejecting---we have to start from linear before\ngoing fancier.\n"},{"id":"170420","messageId":"7vtybj6ji2.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"1308661489-20080-13-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T20:02:13Z","receivedAt":"2011-06-21T20:02:13Z","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 5c0b97e..eb68068 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -46,6 +46,9 @@ enum replay_action { REVERT, CHERRY_PICK };\n> struct replay_opts {\n> \tenum replay_action action;\n> \n> +\t/* --skip-all */\n> +\tint skipall_oper;\n\nYikes what is that \"oper\" doing there? Don't truncate a word in the\nmiddle only to shorten names and make it unclear what you want to say. Is\nthat operand? operation? In this case, I think \"int skip_all\" is\nsufficient, and you can lose the comment that adds no extra information.\n"},{"id":"170424","messageId":"7vei2n6irh.fsf@alter.siamese.dyndns.org","threadId":"27674","inReplyTo":"20110621193217.GA17390@elie","subject":"Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T20:18:10Z","receivedAt":"2011-06-21T20:18:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jun 21, 2011 at 12:19:47PM -0700, Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>\n>>>> +\tif (res < 0)\n>>>> +\t\tdie(_(\"%s failed\"), me);\n>>>> +\treturn 0;\n>>>\n>>> Should the \"revert\" or \"cherry-pick\" here be part of the message\n>>> marked for translation? A translator might want to paraphrase to\n>>>\n>>> \tfatal: failed to cherry-pick\n>>>\n>>> if that is more natural in the language at hand.\n>>\n>> Wouldn't such a message file simply say\n>>\n>> \tmsgid \"%s failed\"\n>> msgstr \"failed to %s\"\n>>\n>> IOW, I am not sure what problem you are seeing.\n>\n> Ah, sorry for the lack of clarity. What I meant is that the noun\n> and verb will be different words in many languages. There can also be\n> problems of subject/verb agreement. Also \"me\" is used elsewhere to\n> hold the command name as typed on the command line even when LANG\n> points to a language other than English if I remember correctly.\n\nYes, \"me\" is the name of the command as the user types, so to whatever\nlanguage you are translating, it can only be usable as the name of the\ncommand in the message. Even if your language has a single verb to\nexpress the act of \"running the cherry-pick command\", say, \"distim\", you\ncannot translate the message to \"failed to distim\". Your message has to\nsay something like \"Sorry, I got a failure while running the 'cherry-pick'\ncommand\", and in English (which is msgid is in) \"%s failed\" would be good\nenough to convey that meaning.\n\nBUT.\n\n> If the message were in revert_or_cherry_pick instead of having two\n> identical copies in cmd_revert and cmd_cherry_pick, it would have been\n> less striking (but still a potential problem).\n\nThe effort by Ævar to switch among totally different message strings\ndepending on \"action\" was going in the direction to actually allow you to\ntranslate one of these messages to \"failed to distim\", and it was lost\nearlier in this patch (error-dirty-index). We may want to fix that in\naddition to this one.\n"},{"id":"170764","messageId":"BANLkTimy17b-F7eg+R9ReRUrx0rhA1cumg@mail.gmail.com","threadId":"27674","inReplyTo":"7vtybj6ji2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-02T06:24:27Z","receivedAt":"2011-07-02T06:24:27Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano writes:\n> Ramkumar Ramachandra writes:\n>\n>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index 5c0b97e..eb68068 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -46,6 +46,9 @@ enum replay_action { REVERT, CHERRY_PICK };\n>>  struct replay_opts {\n>>       enum replay_action action;\n>>\n>> +     /* --skip-all */\n>> +     int skipall_oper;\n>\n> Yikes what is that \"oper\" doing there?  Don't truncate a word in the\n> middle only to shorten names and make it unclear what you want to say. Is\n> that operand? operation? In this case, I think \"int skip_all\" is\n> sufficient, and you can lose the comment that adds no extra information.\n\nInteresting side note: I'd initially wanted to use \"skip_all\" and\n\"continue\", but \"continue\" is a C keyword. That's why I'd reluctantly\nsuffixed \"_oper\" to both for consistency.\n\n-- Ram\n"},{"id":"170765","messageId":"BANLkTik2A7K84dbeL5XkaGv8nbske=EmCA@mail.gmail.com","threadId":"27674","inReplyTo":"20110621155555.GF15461@elie","subject":"Re: [PATCH 01/13] advice: Introduce error_resolve_conflict","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-02T09:44:24Z","receivedAt":"2011-07-02T09:44:24Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n>  2. The change is from\n>\n>        fatal: 'commit' is not possible because you have unmerged files.\n>        Please, fix them up in the work tree ...\n>        ... etc, etc ...\n>\n>    to\n>\n>        error: 'commit' is not ...\n>        error: Please, fix them up...\n>        ... etc, etc ...\n>        fatal: Exiting because of an unresolved conflict.\n\n> Notice that after writing the above, a little detail jumps out:\n> namely, the second \"error:\" line is giving advice, so it might make\n> sense to make it say \"hint:\" instead.\n\nHow do I do this correctly? Changing the \"return error\" statement to\n\"fprintf(stderr, \"hint: Please fix them up ...\")\" doesn't feel right.\n\n--- Ram\n"},{"id":"170766","messageId":"BANLkTinN6KdPnuMWbrdE4jf4ijKDC=35Aw@mail.gmail.com","threadId":"27674","inReplyTo":"20110621170416.GN15461@elie","subject":"Re: [PATCH 09/13] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-02T09:47:15Z","receivedAt":"2011-07-02T09:47:15Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> Earlier, incompatible command-line options used to be caught in\n>> pick_commits after parse_args has parsed the options and populated the\n>> options structure.  Instead, hand over the responsibility of catching\n>> incompatible command-line options to parse_args so that the program\n>> can die early.\n>\n> The \"Earlier ... used to\" phrasing is tripping me up again.\n>\n> The naive reader (i.e., me) wonders: how long does this option parsing\n> and populating the options structure that we want to delay until after\n> verify_opt_compatible take?  Does that delay matter or is there some\n> other reason for this change?\n\n\nI've tried to spell out the motivation for this change more clearly in\nmy latest iteration. Does this look alright?\n\n revert: Catch incompatible command-line options in parse_args\n\n Some incompatible command-line options are caught in pick_commits\n after parse_args has parsed the options and populated the options\n structure. Change this so that parse_args only parses valid\n command-line options instead of returning an unusable options\n structure. However, this does not mean that we will never need to\n check the options structure; when a sequencer API is written future,\n callers will call in with a pre-populated options structure whose\n validity we will still need to check. Also, provide a\n verify_opt_compatible function to handle incompatible options in a\n general manner.\n\n\n-- Ram\n"},{"id":"170768","messageId":"20110702095334.GA19090@elie","threadId":"27674","inReplyTo":"BANLkTinN6KdPnuMWbrdE4jf4ijKDC=35Aw@mail.gmail.com","subject":"Re: [PATCH 09/13] revert: Catch incompatible command-line options early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-02T09:53:34Z","receivedAt":"2011-07-02T09:53:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Ram,\n\nRamkumar Ramachandra wrote:\n> Jonathan Nieder writes:\n\n>> The naive reader (i.e., me) wonders: how long does this option parsing\n>> and populating the options structure that we want to delay until after\n>> verify_opt_compatible take?  Does that delay matter or is there some\n>> other reason for this change?\n>\n> I've tried to spell out the motivation for this change more clearly in\n> my latest iteration. Does this look alright?\n>\n> revert: Catch incompatible command-line options in parse_args\n>\n> Some incompatible command-line options are caught in pick_commits\n> after parse_args has parsed the options and populated the options\n> structure. Change this so that parse_args only parses valid\n> command-line options instead of returning an unusable options\n> structure. However, this does not mean that we will never need to\n> check the options structure; when a sequencer API is written future,\n> callers will call in with a pre-populated options structure whose\n> validity we will still need to check. Also, provide a\n> verify_opt_compatible function to handle incompatible options in a\n> general manner.\n\nAlas, I still don't get it. How can I (the fearful reader) demonstrate\nthe problem that this fixes? Is it a big problem? How does the patch\nfix it? Does the patch have downsides, or is it mostly risk-free?\n"},{"id":"170769","messageId":"20110702100436.GB19090@elie","threadId":"27674","inReplyTo":"20110702095334.GA19090@elie","subject":"Re: [PATCH 09/13] revert: Catch incompatible command-line options early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-02T10:04:36Z","receivedAt":"2011-07-02T10:04:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n\n>> Some incompatible command-line options are caught in pick_commits\n>> after parse_args has parsed the options and populated the options\n>> structure. Change this so that parse_args only parses valid\n>> command-line options\n[...]\n> Alas, I still don't get it. How can I (the fearful reader) demonstrate\n> the problem that this fixes? Is it a big problem? How does the patch\n> fix it? Does the patch have downsides, or is it mostly risk-free?\n\nTo be clearer, here's a quick example in some hypothetical alternate\nworld:\n\n\tcherry-pick/revert: do not create a nonsense \"struct opts\" when given nonsense argv\n\n\tThe --foo and --bar options make no sense together, but if I\n\trun \"git cherry-pick --foo --bar\" then parse_cherry_pick_option\n\twill return a nonsense struct with both \"fooing\" and \"barring\"\n\tset to true, which makes no sense. Currently it's not a\n\tproblem since the code that cares is protected by a check:\n\n\t\tif (opts.fooing && opts.barring)\n\t\t\tdie(\"cannot foo and bar at the same time\");\n\n\tBut that is very fragile --- the frob() code-path relies on\n\topts.fooing and opts.barring not both being true without such\n\ta check and it's only a coincidence that futz() is called\n\tfirst and makes that check, protecting it.\n\n\tWe can make sure such a nonsensical options struct is never\n\tcreated by checking right away that --foo and --bar are not\n\tpassed together at option-parsing time. Meanwhile, make sure\n\tregressions in maintaining this invariant are caught in the\n\tfuture by adding an assertion \"assert(!opts->fooing ||\n\t!opts->barring)\" to both frob() and futz().\n\n\tThe discussion above also applies to --bar and --qux. Fix\n\tthat, too.\n"},{"id":"170770","messageId":"20110702100951.GC19090@elie","threadId":"27674","inReplyTo":"BANLkTik2A7K84dbeL5XkaGv8nbske=EmCA@mail.gmail.com","subject":"Re: [PATCH 01/13] advice: Introduce error_resolve_conflict","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-02T10:09:51Z","receivedAt":"2011-07-02T10:09:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Jonathan Nieder writes:\n\n>> Notice that after writing the above, a little detail jumps out:\n>> namely, the second \"error:\" line is giving advice, so it might make\n>> sense to make it say \"hint:\" instead.\n>\n> How do I do this correctly?\n\nIn an ideal world, it would involve two patches.\n\n 1. first, restructure the code without changing output\n 2. then, change output\n\nOr:\n\n 1. first, change output without restructuring the code\n 2. then, restructure the code\n\nThat way, readers could evaluate the merits of steps (1) and (2)\nseparately.\n\nOh, you mean how to write a \"hint:\" line analagous to \"error:\" lines?\nThere's no standard API for that yet but there's an example in\nbuiltin/revert.c (see v1.7.3-rc0~26^2~3, Introduce advise() to print\nhints, 2010-08-11).\n"},{"id":"170771","messageId":"BANLkTin6dMPkhr4W+Z1VNJ51M4hNPLsLnQ@mail.gmail.com","threadId":"27674","inReplyTo":"7vei2n6irh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-02T10:31:27Z","receivedAt":"2011-07-02T10:31:27Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio and Jonathan,\n\nJunio C Hamano writes:\n> Jonathan Nieder writes:\n>> Jun 21, 2011 at 12:19:47PM -0700, Junio C Hamano wrote:\n>>> Jonathan Nieder writes:\n>>\n>>>>> +  if (res < 0)\n>>>>> +          die(_(\"%s failed\"), me);\n>>>>> +  return 0;\n>>>>\n>>>> Should the \"revert\" or \"cherry-pick\" here be part of the message\n>>>> marked for translation?  A translator might want to paraphrase to\n>>>>\n>>>>     fatal: failed to cherry-pick\n>>>>\n>>>> if that is more natural in the language at hand.\n>>>\n>>> Wouldn't such a message file simply say\n>>>\n>>>      msgid \"%s failed\"\n>>>         msgstr \"failed to %s\"\n>>>\n>>> IOW, I am not sure what problem you are seeing.\n>>\n>> Ah, sorry for the lack of clarity.  What I meant is that the noun\n>> and verb will be different words in many languages.  There can also be\n>> problems of subject/verb agreement.  Also \"me\" is used elsewhere to\n>> hold the command name as typed on the command line even when LANG\n>> points to a language other than English if I remember correctly.\n>\n> Yes, \"me\" is the name of the command as the user types, so to whatever\n> language you are translating, it can only be usable as the name of the\n> command in the message.  Even if your language has a single verb to\n> express the act of \"running the cherry-pick command\", say, \"distim\", you\n> cannot translate the message to \"failed to distim\".  Your message has to\n> say something like \"Sorry, I got a failure while running the 'cherry-pick'\n> command\", and in English (which is msgid is in) \"%s failed\" would be good\n> enough to convey that meaning.\n\nRight.\n\n> BUT.\n>\n>> If the message were in revert_or_cherry_pick instead of having two\n>> identical copies in cmd_revert and cmd_cherry_pick, it would have been\n>> less striking (but still a potential problem).\n>\n> The effort by Ævar to switch among totally different message strings\n> depending on \"action\" was going in the direction to actually allow you to\n> translate one of these messages to \"failed to distim\", and it was lost\n> earlier in this patch (error-dirty-index). We may want to fix that in\n> addition to this one.\n\nBut why is it required here? I already know what \"me\" is depending on\nwhether I'm in cmd_cherry_pick or cmd_revert and there are two\ndifferent translation strings anyway. I fixed the problem I\nintroduced in error-dirty-index earlier in this patch.\n\n-- Ram\n"},{"id":"170775","messageId":"BANLkTik2t=qPfJe7JUsoeka79ZYYRmU_MA@mail.gmail.com","threadId":"27674","inReplyTo":"20110702100436.GB19090@elie","subject":"Re: [PATCH 09/13] revert: Catch incompatible command-line options early","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-02T11:19:20Z","receivedAt":"2011-07-02T11:19:20Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nOn Sat, Jul 2, 2011 at 3:34 PM, Jonathan Nieder wrote:\n>        cherry-pick/revert: do not create a nonsense \"struct opts\" when given nonsense argv\n>\n>        The --foo and --bar options make no sense together, but if I\n>        run \"git cherry-pick --foo --bar\" then parse_cherry_pick_option\n>        will return a nonsense struct with both \"fooing\" and \"barring\"\n>        set to true, which makes no sense.  Currently it's not a\n>        problem since the code that cares is protected by a check:\n>\n>                if (opts.fooing && opts.barring)\n>                        die(\"cannot foo and bar at the same time\");\n>\n>        But that is very fragile --- the frob() code-path relies on\n>        opts.fooing and opts.barring not both being true without such\n>        a check and it's only a coincidence that futz() is called\n>        first and makes that check, protecting it.\n>\n>        We can make sure such a nonsensical options struct is never\n>        created by checking right away that --foo and --bar are not\n>        passed together at option-parsing time.  Meanwhile, make sure\n>        regressions in maintaining this invariant are caught in the\n>        future by adding an assertion \"assert(!opts->fooing ||\n>        !opts->barring)\" to both frob() and futz().\n>\n>        The discussion above also applies to --bar and --qux.  Fix\n>        that, too.\n\nThanks! I wish I could write commit messages like this. I hope I've\ngot it right this time:\n\n revert: Don't create invalid replay_opts in parse_args\n\n The \"--ff\" command-line option cannot be used with four other\n command-line options. However, when these options are specified with\n \"--ff\" on the command-line, parse_args will still parse these\n incompatible options into a replay_opts structure for use by the rest\n of the program. Although pick_commits checks the validity of the\n replay_opts strucutre before before starting its operation, this is\n fragile design because the validity of the replay_opts structure\n depends on pick_commits being called before anything else. Change\n this so that an invalid replay_opts structure is not created by\n parse_args in the first place, and make sure regressions in\n maintaining this invariant are caught in the future by adding an\n assertion in pick_commits.\n\nAnd yes, adding an \"assert\" statement in pick_commits does seem like a\ngreat reminder for the future :)\n\n-- Ram\n"},{"id":"170776","messageId":"20110702113008.GA21526@elie","threadId":"27674","inReplyTo":"BANLkTik2t=qPfJe7JUsoeka79ZYYRmU_MA@mail.gmail.com","subject":"Re: [PATCH 09/13] revert: Catch incompatible command-line options early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-02T11:30:08Z","receivedAt":"2011-07-02T11:30:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Although pick_commits checks the validity of the\n> replay_opts strucutre before before starting its operation, this is\n> fragile design because the validity of the replay_opts structure\n> depends on pick_commits being called before anything else.\n\nThanks, that's a bit more concrete. But it's not fragile, is it?\npick_commits() is the entry point to the cherry-pick machinery ---\nanything that uses a struct replay_opts is called indirectly by\npick_commits(), no?\n\nSo (genuinely, even when I'm not pretending to be naive) I still don't\nsee the point. I'm not even convinced it's a bad idea, since it's\nstill not clear to me what the idea is.\n"},{"id":"170785","messageId":"20110702200209.GA16818@elie","threadId":"27674","inReplyTo":"20110702113008.GA21526@elie","subject":"Re: [PATCH 09/13] revert: Catch incompatible command-line options early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-02T20:02:09Z","receivedAt":"2011-07-02T20:02:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n\n>> Although pick_commits checks the validity of the\n>> replay_opts strucutre before before starting its operation, this is\n>> fragile design because the validity of the replay_opts structure\n>> depends on pick_commits being called before anything else.\n>\n> Thanks, that's a bit more concrete. But it's not fragile, is it?\n> pick_commits() is the entry point to the cherry-pick machinery ---\n> anything that uses a struct replay_opts is called indirectly by\n> pick_commits(), no?\n\nPerhaps the idea is that in the future, there will be multiple entry\npoints?\n\nAgain, the questions the reader will have are:\n\n - what is wrong with the code? (i.e., assume I'm a total stick in\n the mud and convince me there needs to be *some* change)\n - why does the current code work at all?\n - what is the proposed fix?\n - how will it work better?\n - what are the downsides, and how does the patch mitigate them?\n\nExact wording does not matter, as long as those questions are\nanswered.\n"},{"id":"170808","messageId":"buowrfy8wtt.fsf@dhlpc061.dev.necel.com","threadId":"27674","inReplyTo":"BANLkTimy17b-F7eg+R9ReRUrx0rhA1cumg@mail.gmail.com","subject":"Re: [PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2011-07-04T04:59:58Z","receivedAt":"2011-07-04T04:59:58Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"Ramkumar Ramachandra writes:\n> Interesting side note: I'd initially wanted to use \"skip_all\" and\n> \"continue\", but \"continue\" is a C keyword. That's why I'd reluctantly\n> suffixed \"_oper\" to both for consistency.\n\nIt seems a good idea to restrict such uglification to only those cases\nwhere it's necessary, not make _everything_ ugly just for the sake of\nconsistency.....\n\n[The traditional thing to do with C-keyword conflicts, when there's no\nobvious and natural alternative, seems to just be intentional mispelling\n-- \"continu\", \"kontinue\", \"cont\", \"_continue\", whatever. Yes, they're\nugly, but people will know why you did it, and they'll forgive you.]\n\n-Miles\n\n-- \nHelpmate, n. A wife, or bitter half.\n"},{"id":"170826","messageId":"CALkWK0mfoPOc5bgWBHwCR3+kZRyJWcQVnHgA12COoQimsrh3Vg@mail.gmail.com","threadId":"27674","inReplyTo":"buowrfy8wtt.fsf@dhlpc061.dev.necel.com","subject":"Re: [PATCH 12/13] revert: Introduce skip-all to cleanup sequencer data","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-05T10:47:06Z","receivedAt":"2011-07-05T10:47:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Miles,\n\nMiles Bader writes:\n> Ramkumar Ramachandra writes:\n>> Interesting side note: I'd initially wanted to use \"skip_all\" and\n>> \"continue\", but \"continue\" is a C keyword.  That's why I'd reluctantly\n>> suffixed \"_oper\" to both for consistency.\n>\n> It seems a good idea to restrict such uglification to only those cases\n> where it's necessary, not make _everything_ ugly just for the sake of\n> consistency.....\n>\n> [The traditional thing to do with C-keyword conflicts, when there's no\n> obvious and natural alternative, seems to just be intentional mispelling\n> -- \"continu\", \"kontinue\", \"cont\", \"_continue\", whatever.  Yes, they're\n> ugly, but people will know why you did it, and they'll forgive you.]\n\nInteresting. Are there such examples in the Git codebase as well?\n\n-- Ram\n"}]}