{"thread":{"id":"27761","subject":"[GSoC update] Sequencer: The insn sheet format","startedAt":"2011-07-06T07:54:14Z","lastAt":"2011-07-12T06:33:35Z","messageCount":55,"participants":["Ramkumar Ramachandra","Jonathan Nieder","Junio C Hamano","Drew Northup"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"170863","messageId":"1309938868-2028-1-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":null,"subject":"[GSoC update] Sequencer: The insn sheet format","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:14Z","receivedAt":"2011-07-06T07:54:14Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nSorry about the delay -- I was travelling again. I've chistled\neverything until the 12th patch to near-perfection. One of the major\nuser-visible changes is renaming '--skip-all' to '--reset'. Initially\nsuggested by Christian, I really like this name; I think there will be\nenough justification for it, especially after we get 'git reset\n--hard' to clear away the sequencer state. Now, we're back to the\npoint where we need to finalize the instruction sheet format.\n\nThanks for reading.\n\n-- Ram\n\nRamkumar Ramachandra (14):\n advice: Introduce error_resolve_conflict\n revert: Inline add_message_to_msg function\n revert: Don't check lone argument in get_encoding\n revert: Rename no_replay to record_origin\n revert: Propogate errors upwards from do_pick_commit\n revert: Eliminate global \"commit\" variable\n revert: Introduce struct to keep command-line options\n revert: Separate cmdline parsing from functional code\n revert: Don't create invalid replay_opts in parse_args\n revert: Persist data for continuation\n revert: Introduce a layer of indirection over pick_commits\n revert: Introduce --reset to cleanup sequencer data\n revert: Introduce --continue to continue the operation\n revert: Change insn sheet format\n\n advice.c | 31 ++-\n advice.h | 1 +\n builtin/revert.c | 657 ++++++++++++++++++++++++++++--------\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 | 119 +++++++\n t/t7502-commit.sh | 1 +\n 13 files changed, 720 insertions(+), 169 deletions(-)\n create mode 100644 t/t3510-cherry-pick-sequence.sh\n\n-- \n1.7.5.GIT\n"},{"id":"170864","messageId":"1309938868-2028-2-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 01/14] advice: Introduce error_resolve_conflict","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:15Z","receivedAt":"2011-07-06T07:54:15Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Enable future callers to report a conflict and not die immediately by\nintroducing a new function called error_resolve_conflict.\nRe-implement die_resolve_conflict as a call to error_resolve_conflict\nfollowed by a call to die. Consequently, the message printed by\ndie_resolve_conflict changes from\n\nfatal: 'commit' is not possible because you have unmerged files.\n Please, fix them up in the work tree ...\n ...\n\nto\n\nerror: 'commit' is not possible because you have unmerged files.\nhint: Please, fix them up in the work tree ...\nhint: ...\nfatal: Exiting because of an unresolved conflict.\n\nHints are printed using the same advise function introduced in\nv1.7.3-rc0~26^2~3 (Introduce advise() to print hints, 2010-08-11).\n\nInspired-by: Christian Couder \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n advice.c | 31 ++++++++++++++++++++++++-------\n advice.h | 1 +\n 2 files changed, 25 insertions(+), 7 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 0be4b5f..a031732 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -19,6 +19,15 @@ static struct {\n \t{ \"detachedhead\", &advice_detached_head },\n };\n \n+static void advise(const char *advice, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, advice);\n+\tvreportf(\"hint: \", advice, params);\n+\tva_end(params);\n+}\n+\n int git_default_advice_config(const char *var, const char *value)\n {\n \tconst char *k = skip_prefix(var, \"advice.\");\n@@ -34,16 +43,24 @@ int git_default_advice_config(const char *var, const char *value)\n \treturn 0;\n }\n \n-void NORETURN die_resolve_conflict(const char *me)\n+int error_resolve_conflict(const char *me)\n {\n-\tif (advice_resolve_conflict)\n+\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\tadvise(\"Please, fix them up in the work tree,\");\n+\t\tadvise(\"and then use 'git add/rm ' as\");\n+\t\tadvise(\"appropriate to mark resolution and make a commit,\");\n+\t\tadvise(\"or use 'git commit -a'.\");\n+\t}\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":"170865","messageId":"1309938868-2028-3-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 02/14] revert: Inline add_message_to_msg function","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:16Z","receivedAt":"2011-07-06T07:54:16Z","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. Additionally, fix a bug introduced in 9509af6 (Make\ngit-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer\nwas being incremented when \"\\n\\n\" was not found in \"message\".\n\nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 20 ++++++--------------\n 1 files changed, 6 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1f27c63..19d604c 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,16 @@ 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\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":"170877","messageId":"1309938868-2028-4-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 03/14] revert: Don't check lone argument in get_encoding","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:17Z","receivedAt":"2011-07-06T07:54:17Z","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 19d604c..a6b51b1 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":"170866","messageId":"1309938868-2028-5-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 04/14] revert: Rename no_replay to record_origin","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:18Z","receivedAt":"2011-07-06T07:54:18Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Rename the variable corresponding to the \"-x\" command-line option from\n\"no_replay\" to a more apt \"record_origin\".\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex a6b51b1..1612911 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 struct commit *commit;\n static int commit_argc;\n@@ -91,7 +91,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@@ -465,7 +465,7 @@ static int do_pick_commit(void)\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@@ -560,7 +560,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":"170867","messageId":"1309938868-2028-6-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 05/14] revert: Propogate errors upwards from do_pick_commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:19Z","receivedAt":"2011-07-06T07:54:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Currently, the return value from revert_or_cherry_pick is a\nnon-negative number representing the intended exit status from `git\nrevert` or `git cherry-pick`. Change this by replacing some of the\ncalls to \"die\" with calls to \"error\", so that it can return negative\nvalues too. Postive return values indicate conflicts, while negative\nones indicate other errors. This return status is propogated updwards\nfrom do_pick_commit, to be finally handled in cmd_cherry_pick and\ncmd_revert.\n\nIn the same spirit, also introduce a new function error_dirty_index,\nbased on die_dirty_index, which prints some hints and returns an error\nto its caller do_pick_commit.\n\nWhile the full benefits of this patch will only be seen once all the\n\"die\" calls are replaced with calls to \"error\", its immediate impact\nis to change some of the \"die:\" messages to \"error:\" messages and\nprint a new \"fatal: cherry-pick failed\" message when the operation\nfails.\n\nInspired-by: Christian Couder \nMentored-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 69 ++++++++++++++++++++++++++++-------------------------\n 1 files changed, 36 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1612911..3de7ec4 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -250,25 +250,20 @@ static struct tree *empty_tree(void)\n \treturn tree;\n }\n \n-static NORETURN void die_dirty_index(const char *me)\n+static int 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+\t/* Different translation strings for cherry-pick and revert */\n+\tif (action == CHERRY_PICK)\n+\t\terror(_(\"Your local changes would be overwritten by %s.\"), me);\n+\telse\n+\t\terror(_(\"Your local changes would be overwritten by %s.\"), me);\n+\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 +377,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 +395,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 +418,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@@ -581,14 +576,22 @@ 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\tdie(_(\"%s failed\"), me);\n+\treturn res;\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\tdie(_(\"%s failed\"), me);\n+\treturn res;\n }\n-- \n1.7.5.GIT\n"},{"id":"170873","messageId":"1309938868-2028-7-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 06/14] revert: Eliminate global \"commit\" variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:20Z","receivedAt":"2011-07-06T07:54:20Z","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\nvariable to be passed around explicitly as an argument for clarity.\nThis involves changing several functions to take an additional\nargument, but no functional changes. Additionaly, this will permit\nmore than one commit to be cherry-picked at once, should we choose to\ndevelop this functionality in future.\n\nInspired-by: Christian Couder \nHelped-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 22 +++++++++++-----------\n 1 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 3de7ec4..fcc2047 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, record_origin, 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,7 +181,7 @@ 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(struct commit *commit)\n {\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -359,7 +358,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@@ -421,7 +420,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@@ -466,7 +465,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));\n \t}\n \n \tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n@@ -544,6 +543,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@@ -566,7 +566,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":"170868","messageId":"1309938868-2028-8-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 07/14] revert: Introduce struct to keep command-line options","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:21Z","receivedAt":"2011-07-06T07:54:21Z","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\nThe variable \"me\" is left as a file-scope static variable because it\nis not an independent option. \"me\" is simply a string that needs to\nbe inferred from the \"action\" option, and is kept global to save each\nfunction the trouble of determining it independently.\n\nUnfortunately, this patch introduces a minor regression. Parsing\nstrategy-option violates a C89 rule: Initializers cannot refer to\nvariables whose address is not known at compile time. Currently, this\nrule is violated by some other parts of Git as well, and it is\npossible to get GCC to report these instances using the \"-std=c89\n-pedantic\" option.\n\nInspired-by: Christian Couder \nMentored-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 182 ++++++++++++++++++++++++++++++------------------------\n 1 files changed, 102 insertions(+), 80 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex fcc2047..f34e51f 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@@ -249,7 +263,7 @@ static struct tree *empty_tree(void)\n \treturn tree;\n }\n \n-static int error_dirty_index(const char *me)\n+static int error_dirty_index(const char *me, enum replay_action action)\n {\n \tif (read_cache_unmerged())\n \t\treturn error_resolve_conflict(me);\n@@ -278,7 +292,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@@ -299,7 +314,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@@ -339,7 +354,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@@ -347,9 +362,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@@ -358,7 +373,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@@ -368,7 +383,7 @@ static int do_pick_commit(struct commit *commit)\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res;\n \n-\tif (no_commit) {\n+\tif (opts->no_commit) {\n \t\t/*\n \t\t * We do not intend to commit immediately. We just want to\n \t\t * merge the differences in, so let's compute the tree\n@@ -381,7 +396,7 @@ static int do_pick_commit(struct commit *commit)\n \t\tif (get_sha1(\"HEAD\", head))\n \t\t\treturn error(_(\"You do not have a valid HEAD\"));\n \t\tif (index_differs_from(\"HEAD\", 0))\n-\t\t\treturn error_dirty_index(me);\n+\t\t\treturn error_dirty_index(me, opts->action);\n \t}\n \tdiscard_cache();\n \n@@ -393,25 +408,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@@ -433,7 +448,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@@ -459,18 +474,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\t\twrite_cherry_pick_head(sha1_to_hex(commit));\n+\t\tif (!opts->no_commit)\n+\t\t\twrite_cherry_pick_head(commit);\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@@ -480,23 +495,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@@ -505,18 +520,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@@ -525,7 +540,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@@ -540,33 +555,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@@ -577,10 +593,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\tdie(_(\"%s failed\"), me);\n \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\tdie(_(\"%s failed\"), me);\n \treturn res;\n-- \n1.7.5.GIT\n"},{"id":"170869","messageId":"1309938868-2028-9-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 08/14] revert: Separate cmdline parsing from functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:22Z","receivedAt":"2011-07-06T07:54:22Z","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 continuing the\ncherry-pick or revert.\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 f34e51f..e3a7c9e 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -555,17 +555,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@@ -599,7 +594,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\tdie(_(\"%s failed\"), me);\n \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\tdie(_(\"%s failed\"), me);\n \treturn res;\n-- \n1.7.5.GIT\n"},{"id":"170872","messageId":"1309938868-2028-10-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 09/14] revert: Don't create invalid replay_opts in parse_args","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:23Z","receivedAt":"2011-07-06T07:54:23Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The \"--ff\" command-line option cannot be used with four other\ncommand-line options. However, when these options are specified with\n\"--ff\" on the command-line, parse_args will still parse these\nincompatible options into a replay_opts structure for use by the rest\nof the program. Although pick_commits checks the validity of the\nreplay_opts strucutre before before starting its operation, this is\ninelegant design; pick_commits is currently the gatekeeper to the\ncherry-pick machinery, but this will change in future. To futureproof\nthe code and catch these errors in one place, make sure that an\ninvalid replay_opts structure is not created by parse_args in the\nfirst place. Also ensure that regressions in maintaining this\ninvariant are caught in the future by adding an assertion in\npick_commits.\n\nInspired-by: Christian Couder \nMentored-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 37 ++++++++++++++++++++++++++-----------\n 1 files changed, 26 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e3a7c9e..27dc538 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@@ -561,17 +584,9 @@ 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+\tif (opts->allow_ff)\n+\t\tassert(!(opts->signoff || opts->no_commit ||\n+\t\t\t\topts->record_origin || opts->edit));\n \tread_and_refresh_cache(me, opts);\n \n \tprepare_revs(&revs, opts);\n-- \n1.7.5.GIT\n"},{"id":"170870","messageId":"1309938868-2028-11-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 10/14] revert: Persist data for continuation","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:24Z","receivedAt":"2011-07-06T07:54:24Z","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. These new\nfiles are unrelated to the existing CHERRY_PICK_HEAD, which will still\nbe useful when a conflict is encountered.\n\nInspired-by: Christian Couder \nHelped-by: Jonathan Nieder \nHelped-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 122 +++++++++++++++++++++++++++++++++++++--\n t/t3510-cherry-pick-sequence.sh | 37 ++++++++++++\n 2 files changed, 153 insertions(+), 6 deletions(-)\n create mode 100644 t/t3510-cherry-pick-sequence.sh\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 27dc538..7d76f92 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\t\"sequencer\"\n+#define SEQ_HEAD_FILE\t\"sequencer/head\"\n+#define SEQ_TODO_FILE\t\"sequencer/todo\"\n+\n static const char * const revert_usage[] = {\n \t\"git revert [options] \",\n \tNULL\n@@ -417,7 +422,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, opts->action);\n \t}\n@@ -578,10 +583,106 @@ 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;\n+\tstruct commit_list **next;\n+\n+\tprepare_revs(&revs, opts);\n+\n+\t/* Insert into todo_list in the same order */\n+\t/* NEEDSWORK: Expose this as commit_list_append */\n+\tnext = todo_list;\n+\twhile ((commit = get_revision(&revs))) {\n+\t\tnew = xmalloc(sizeof(struct commit_list));\n+\t\tnew->item = commit;\n+\t\t*next = new;\n+\t\tnext = &new->next;\n+\t}\n+\t*next = NULL;\n+}\n+\n+static void create_seq_dir(void)\n+{\n+\tif (file_exists(git_path(SEQ_DIR))) {\n+\t\tif (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)\n+\t\t\tdie(_(\"Could not remove %s\"), git_path(SEQ_DIR));\n+\t} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)\n+\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), git_path(SEQ_DIR));\n+}\n+\n+static void save_head(const char *head)\n+{\n+\tstatic struct lock_file head_lock;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tfd = hold_lock_file_for_update(&head_lock, git_path(SEQ_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.\"), git_path(SEQ_HEAD_FILE));\n+\tif (commit_lock_file(&head_lock) < 0)\n+\t\tdie(_(\"Error wrapping up %s\"), git_path(SEQ_HEAD_FILE));\n+}\n+\n+static void save_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, git_path(SEQ_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.\"), git_path(SEQ_TODO_FILE));\n+\t}\n+\tif (commit_lock_file(&todo_lock) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Error wrapping up %s\"), git_path(SEQ_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\", git_path(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 \tif (opts->allow_ff)\n@@ -589,15 +690,24 @@ static int pick_commits(struct replay_opts *opts)\n \t\t\t\topts->record_origin || opts->edit));\n \tread_and_refresh_cache(me, opts);\n \n-\tprepare_revs(&revs, opts);\n+\twalk_revs_populate_todo(&todo_list, opts);\n+\tcreate_seq_dir();\n+\tif (!get_sha1(\"HEAD\", sha1))\n+\t\tsave_head(sha1_to_hex(sha1));\n+\tsave_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\tsave_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/*\n+\t * Sequence of picks finished successfully; cleanup by\n+\t * removing the .git/sequencer directory\n+\t */\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":"170871","messageId":"1309938868-2028-12-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 11/14] revert: Introduce a layer of indirection over pick_commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:25Z","receivedAt":"2011-07-06T07:54:25Z","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 | 36 +++++++++++++++++++++++++-----------\n 1 files changed, 25 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 7d76f92..8cdcdb6 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -677,10 +677,8 @@ 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@@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)\n \t\t\t\topts->record_origin || opts->edit));\n \tread_and_refresh_cache(me, opts);\n \n-\twalk_revs_populate_todo(&todo_list, opts);\n-\tcreate_seq_dir();\n-\tif (!get_sha1(\"HEAD\", sha1))\n-\t\tsave_head(sha1_to_hex(sha1));\n-\tsave_todo(todo_list, opts);\n-\n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsave_todo(cur, opts);\n \t\tres = do_pick_commit(cur->item, opts);\n@@ -710,6 +702,22 @@ 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+\tcreate_seq_dir();\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@@ -722,7 +730,13 @@ 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/*\n+\t * Decide what to do depending on the arguments; a fresh\n+\t * cherry-pick should be handled differently from an existing\n+\t * one that is being continued\n+\t */\n+\tres = process_continuation(&opts);\n \tif (res < 0)\n \t\tdie(_(\"%s failed\"), me);\n \treturn res;\n@@ -738,7 +752,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\tdie(_(\"%s failed\"), me);\n \treturn res;\n-- \n1.7.5.GIT\n"},{"id":"170876","messageId":"1309938868-2028-13-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 12/14] revert: Introduce --reset to cleanup sequencer data","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:26Z","receivedAt":"2011-07-06T07:54:26Z","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 \"--reset\" 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 This is perfect but for the fact that 'git reset --hard' doesn't blow\n away the sequencer state. Why I haven't implemented that yet: should\n ONLY a hard reset blow away the state?\n\n builtin/revert.c | 54 ++++++++++++++++++++++++++++++-----\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, 121 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8cdcdb6..9e18d64 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -46,6 +46,8 @@ enum replay_action { REVERT, CHERRY_PICK };\n struct replay_opts {\n \tenum replay_action action;\n \n+\tint reset;\n+\n \t/* Boolean options */\n \tint edit;\n \tint record_origin;\n@@ -108,6 +110,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, \"reset\", &opts->reset, \"forget the current operation\"),\n \t\tOPT_BOOLEAN('n', \"no-commit\", &opts->no_commit, \"don't automatically commit\"),\n \t\tOPT_BOOLEAN('e', \"edit\", &opts->edit, \"edit the commit message\"),\n \t\t{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, \"no-op (backward compatibility)\",\n@@ -136,7 +139,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->reset) {\n+\t\tverify_opt_compatible(me, \"--reset\",\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@@ -625,8 +642,11 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,\n static void create_seq_dir(void)\n {\n \tif (file_exists(git_path(SEQ_DIR))) {\n-\t\tif (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)\n-\t\t\tdie(_(\"Could not remove %s\"), git_path(SEQ_DIR));\n+\t\terror(_(\"%s already exists.\"), git_path(SEQ_DIR));\n+\t\tadvise(_(\"This usually means that a %s operation is in progress.\"), me);\n+\t\tadvise(_(\"Use %s --continue to continue the operation\"), me);\n+\t\tadvise(_(\"or use %s --reset to forget about it\"), me);\n+\t\tdie(_(\"%s failed\"), me);\n \t} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)\n \t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), git_path(SEQ_DIR));\n }\n@@ -709,13 +729,31 @@ 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-\tcreate_seq_dir();\n-\tif (!get_sha1(\"HEAD\", sha1))\n-\t\tpersist_head(sha1_to_hex(sha1));\n-\tpersist_todo(todo_list, opts);\n+\tif (opts->reset) {\n+\t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n+\t\t\tgoto error;\n+\t\treturn cleanup_sequencer_data();\n+\t} else {\n+\t\t/*\n+\t\t * Start a new cherry-pick/ revert sequence; but\n+\t\t * first, make sure that an existing one isn't in\n+\t\t * progress\n+\t\t */\n+\t\tif (file_exists(git_path(SEQ_TODO_FILE))) {\n+\t\t\terror(_(\"A %s is already in progress\"), me);\n+\t\t\tadvise(_(\"Use %s --reset 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\tcreate_seq_dir();\n+\t\tif (!get_sha1(\"HEAD\", sha1))\n+\t\t\tsave_head(sha1_to_hex(sha1));\n+\t\tsave_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..14c57dd 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 --reset\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..81191ae 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 --reset &&\n \tgit read-tree --reset -u HEAD &&\n \ttest_must_fail git cherry-pick remote &&\n+\tgit cherry-pick --reset &&\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..e0c805d 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 --reset &&\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..db21e47 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 --reset &&\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 --reset &&\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 --reset\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 --reset &&\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 --reset &&\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 --reset &&\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..85d5fc1 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 --reset &&\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 --reset &&\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..dc02227 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 --reset\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 --reset\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..abd63a5 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 --reset &&\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 --reset &&\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 --reset\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..86f8626 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 --reset &&\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 --reset &&\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 --reset\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 --reset\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 --reset &&\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 --reset\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 --reset\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 --reset\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 --reset &&\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 --reset &&\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 --reset &&\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 --reset &&\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 --reset &&\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 --reset &&\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..9e97a7b 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 '--reset complains when no cherry-pick is in progress' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --reset >actual 2>&1 &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success '--reset 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 --reset &&\n+\ttest_path_is_missing .git/sequencer\n+'\n+\n test_done\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 3f3adc3..0d54027 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 --reset &&\n \techo \"editor not started\" >.git/result &&\n \t(\n \t\tGIT_EDITOR=\"$(pwd)/.git/FAKE_EDITOR\" &&\n-- \n1.7.5.GIT\n"},{"id":"170875","messageId":"1309938868-2028-14-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 13/14] revert: Introduce --continue to continue the operation","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:27Z","receivedAt":"2011-07-06T07:54:27Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"After resolving a conflict, the user simply has to `git commit`\n(CHERRY_PICK_HEAD will help), and then `git cherry-pick --continue` to\ncontinue the operation. Command-line options are currently\nunsupported. Note that a cherry-pick operation cannot be resumed with\na `git revert --continue` and vice-versa.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n As suggested by Miles and Junio, I've obfuscated the variable name to\n \"contin\". Would it make sense to persist the entire replay_opts\n structure (so that it applies to all the commits, and no\n commit-specific command-line options are supported) somewhere? Where\n and how? My idea is that patch can be considered for inclusion then.\n Patch 14 onwards can then start adding features like \"commit-specific\n command-line options\", \"mixed commands\" etc.\n\n builtin/revert.c | 130 ++++++++++++++++++++++++++++++++++++++-\n t/t3510-cherry-pick-sequence.sh | 68 ++++++++++++++++++++\n 2 files changed, 195 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 9e18d64..6ef56ee 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -47,6 +47,7 @@ struct replay_opts {\n \tenum replay_action action;\n \n \tint reset;\n+\tint contin;\n \n \t/* Boolean options */\n \tint edit;\n@@ -105,12 +106,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, \"reset\", &opts->reset, \"forget the current operation\"),\n+\t\tOPT_BOOLEAN(0, \"continue\", &opts->contin, \"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@@ -140,9 +166,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\"--reset\", opts->reset,\n+\t\t\t\t\"--continue\", opts->contin,\n+\t\t\t\tNULL);\n+\n \t/* Check for incompatible command line arguments */\n-\tif (opts->reset) {\n-\t\tverify_opt_compatible(me, \"--reset\",\n+\tif (opts->reset || opts->contin) {\n+\t\tchar *this_operation;\n+\t\tif (opts->reset)\n+\t\t\tthis_operation = \"--reset\";\n+\t\telse\n+\t\t\tthis_operation = \"--continue\";\n+\n+\t\tverify_opt_compatible(me, this_operation,\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@@ -617,6 +655,83 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t}\n }\n \n+static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n+{\n+\tunsigned char commit_sha1[20];\n+\tchar sha1_abbrev[40];\n+\tstruct commit *commit;\n+\tenum replay_action action;\n+\tint insn_len = 0;\n+\tchar *p;\n+\n+\tp = start;\n+\tif (!(p = strchr(p, ' ')))\n+\t\treturn NULL;\n+\tinsn_len = p - start;\n+\tif (!(p = strchr(p + 1, ' ')))\n+\t\treturn NULL;\n+\tp += 1;\n+\tstrlcpy(sha1_abbrev, start + insn_len + 1,\n+\t\tp - (start + insn_len + 1));\n+\n+\tif (!strncmp(start, \"pick\", insn_len))\n+\t\taction = CHERRY_PICK;\n+\telse if (!strncmp(start, \"revert\", insn_len))\n+\t\taction = REVERT;\n+\telse\n+\t\treturn NULL;\n+\n+\t/*\n+\t * Verify that the action matches up with the one in\n+\t * opts; we don't support arbitrary instructions\n+\t */\n+\tif (action != opts->action)\n+\t\treturn NULL;\n+\n+\tif ((get_sha1(sha1_abbrev, commit_sha1) < 0)\n+\t\t|| !(commit = lookup_commit_reference(commit_sha1)))\n+\t\treturn NULL;\n+\n+\treturn commit;\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+\tstruct commit_list *new;\n+\tstruct commit_list **next;\n+\tstruct commit *commit;\n+\tchar *p;\n+\tint fd;\n+\n+\tfd = open(git_path(SEQ_TODO_FILE), O_RDONLY);\n+\tif (fd < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie_errno(_(\"Could not open %s.\"), git_path(SEQ_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.\"), git_path(SEQ_TODO_FILE));\n+\t}\n+\tclose(fd);\n+\n+\tnext = todo_list;\n+\tfor (p = buf.buf; *p; p = strchr(p, '\\n') + 1) {\n+\t\tif (!(commit = parse_insn_line(p, opts))) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\tdie(_(\"Malformed instruction sheet: %s\"), git_path(SEQ_TODO_FILE));\n+\t\t}\n+\t\tnew = xmalloc(sizeof(struct commit_list));\n+\t\tnew->item = commit;\n+\t\t*next = new;\n+\t\tnext = &new->next;\n+\t}\n+\t*next = NULL;\n+\tstrbuf_release(&buf);\n+}\n+\n static void walk_revs_populate_todo(struct commit_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n@@ -733,6 +848,14 @@ static int process_continuation(struct replay_opts *opts)\n \t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n \t\t\tgoto error;\n \t\treturn cleanup_sequencer_data();\n+\t} else if (opts->contin) {\n+\t\tif (!file_exists(git_path(SEQ_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/*\n \t\t * Start a new cherry-pick/ revert sequence; but\n@@ -741,7 +864,8 @@ static int process_continuation(struct replay_opts *opts)\n \t\t */\n \t\tif (file_exists(git_path(SEQ_TODO_FILE))) {\n \t\t\terror(_(\"A %s is already in progress\"), me);\n-\t\t\tadvise(_(\"Use %s --reset to forget about it\"), me);\n+\t\t\tadvise(_(\"Use %s --continue to continue the operation\"), me);\n+\t\t\tadvise(_(\"or use %s --reset to forget about it\"), 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 9e97a7b..7a57216 100644\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -48,4 +48,72 @@ test_expect_success '--reset cleans up sequencer directory' '\n \ttest_path_is_missing .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 --reset\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_path_is_missing .git/sequencer &&\n+\t{\n+\t\tgit rev-list HEAD |\n+\t\tgit diff-tree --root --stdin |\n+\t\tsed \"s/[0-9a-f]\\{40\\}/OBJID/g\"\n+\t} >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tfoo\n+\tOBJID\n+\t:100644 100644 OBJID OBJID M\tunrelated\n+\tOBJID\n+\t:000000 100644 OBJID OBJID A\tfoo\n+\t:000000 100644 OBJID OBJID A\tunrelated\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'malformed instruction sheet 1' '\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+\tsed \"s/pick /pick/\" .git/sequencer/todo >new_sheet\n+\tcp new_sheet .git/sequencer/todo\n+\ttest_must_fail git cherry-pick --continue >actual 2>&1 &&\n+\tgit cherry-pick --reset &&\n+\ttest_i18ngrep \"fatal\" actual\n+'\n+\n+test_expect_success 'malformed instruction sheet 2' '\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+\tsed \"s/pick/revert/\" .git/sequencer/todo >new_sheet\n+\tcp new_sheet .git/sequencer/todo\n+\ttest_must_fail git cherry-pick --continue >actual 2>&1 &&\n+\tgit cherry-pick --reset &&\n+\ttest_i18ngrep \"fatal\" actual\n+'\n+\n test_done\n-- \n1.7.5.GIT\n"},{"id":"170874","messageId":"1309938868-2028-15-git-send-email-artagnon@gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"[RFC PATCH 14/14] revert: Change insn sheet format","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T07:54:28Z","receivedAt":"2011-07-06T07:54:28Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Add command-line options to the instruction sheet, but don't let the\nexisting parser break. Parsing out the command-line options back into\na replay_opts struct is unimplemented.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n I've intentionally left parse_cmdline_args unimplemented -- Although\n Stephen (and Christian subsequently) have written a parser [1], I'm\n not happy with it. The point of this patch is to illustrate the\n problem with this instruction sheet format, and provoke some\n discussion about this. I don't want to parse command-line arguments\n by hand handling all sorts of corner cases in quoting etc -- is there\n any other way? Existing implementations in libraries like Glib are\n much too heavyweight.\n\n Note that I've used the '#' character as a separator to simplify\n parsing. It's not the most elegant solution, but I think it should\n work.\n \n [1]: http://article.gmane.org/gmane.comp.version-control.git/162198\n\n builtin/revert.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++------\n 1 files changed, 66 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 6ef56ee..a0a4377 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -638,6 +638,34 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n \trollback_lock_file(&index_lock);\n }\n \n+static void format_args(struct strbuf *buf, struct replay_opts *opts)\n+{\n+\tint i;\n+\n+\tif (opts->no_commit)\n+\t\tstrbuf_addstr(buf, \"-n \");\n+\tif (opts->edit)\n+\t\tstrbuf_addstr(buf, \"-e \");\n+\tif (opts->signoff)\n+\t\tstrbuf_addstr(buf, \"-s \");\n+\tif (opts->mainline)\n+\t\tstrbuf_addf(buf, \"-m %d \", opts->mainline);\n+\tif (opts->strategy)\n+\t\tstrbuf_addf(buf, \"--strategy %s \", opts->strategy);\n+\tif (opts->xopts)\n+\t\tfor (i = 0; i < opts->xopts_nr; i ++)\n+\t\t\tstrbuf_addf(buf, \"-X %s \", opts->xopts[i]);\n+\tif (opts->record_origin)\n+\t\tstrbuf_addstr(buf, \"-x \");\n+\tif (opts->allow_ff)\n+\t\tstrbuf_addstr(buf, \"--ff \");\n+}\n+\n+/*\n+ * Instruction sheet format ::\n+ * pick -s 537d2e # revert: Introduce --continue to continue the operation\n+ * pick 4a15c1 # revert: Introduce --reset to cleanup sequencer data\n+ */\n static void format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n@@ -651,29 +679,53 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,\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\tstrbuf_addf(buf, \"%s \", action);\n+\t\tformat_args(buf, opts);\n+\t\tstrbuf_addf(buf, \"%s # %s\\n\", sha1_abbrev, msg.subject);\n \t}\n }\n \n-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n+static void parse_cmdline_args(struct strbuf *args_to_parse,\n+\t\t\tint *argc, const char ***argv)\n+{\n+\treturn;\n+}\n+\n+static struct commit *parse_insn_line(const char *start, struct replay_opts *opts)\n {\n \tunsigned char commit_sha1[20];\n \tchar sha1_abbrev[40];\n \tstruct commit *commit;\n \tenum replay_action action;\n \tint insn_len = 0;\n-\tchar *p;\n+\tchar *p, *end;\n+\n+\tint argc = 0;\n+\tconst char **argv = NULL;\n+\tstruct strbuf args_to_parse = STRBUF_INIT;\n \n-\tp = start;\n+\tp = (char *) start;\n \tif (!(p = strchr(p, ' ')))\n \t\treturn NULL;\n \tinsn_len = p - start;\n-\tif (!(p = strchr(p + 1, ' ')))\n+\tif (!(end = strchr(p + 1, '#')))\n \t\treturn NULL;\n-\tp += 1;\n-\tstrlcpy(sha1_abbrev, start + insn_len + 1,\n-\t\tp - (start + insn_len + 1));\n+\t*(end - 1) = '\\0';\n+\tstrbuf_addstr(&args_to_parse, p + 1);\n+\n+\t/* Parse argc, argv out of args_to_parse */\n+\t/* TODO: Implement parse_cmdline_args! */\n+\tparse_cmdline_args(&args_to_parse, &argc, &argv);\n+\tstrbuf_release(&args_to_parse);\n+\tif (argc)\n+\t\tparse_args(argc, argv, opts);\n+\n+\t/* Copy out the sha1_abbrev explicitly */\n+\tif (!(p = strrchr(p, ' ')))\n+\t\treturn NULL;\n+\tstrcpy(sha1_abbrev, p + 1);\n \n+\t/* Determine the action */\n \tif (!strncmp(start, \"pick\", insn_len))\n \t\taction = CHERRY_PICK;\n \telse if (!strncmp(start, \"revert\", insn_len))\n@@ -718,7 +770,7 @@ static void read_populate_todo(struct commit_list **todo_list,\n \tclose(fd);\n \n \tnext = todo_list;\n-\tfor (p = buf.buf; *p; p = strchr(p, '\\n') + 1) {\n+\tfor (p = buf.buf; *p;) {\n \t\tif (!(commit = parse_insn_line(p, opts))) {\n \t\t\tstrbuf_release(&buf);\n \t\t\tdie(_(\"Malformed instruction sheet: %s\"), git_path(SEQ_TODO_FILE));\n@@ -727,6 +779,11 @@ static void read_populate_todo(struct commit_list **todo_list,\n \t\tnew->item = commit;\n \t\t*next = new;\n \t\tnext = &new->next;\n+\n+\t\tif ((p = strchr(p, '\\n')))\n+\t\t\tp += 1;\n+\t\telse\n+\t\t\tbreak;\n \t}\n \t*next = NULL;\n \tstrbuf_release(&buf);\n-- \n1.7.5.GIT\n"},{"id":"170879","messageId":"20110706083512.GA15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 01/14] advice: Introduce error_resolve_conflict","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T08:35:12Z","receivedAt":"2011-07-06T08:35:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Enable future callers to report a conflict and not die immediately by\n> introducing a new function called error_resolve_conflict.\n> Re-implement die_resolve_conflict as a call to error_resolve_conflict\n> followed by a call to die. Consequently, the message printed by\n> die_resolve_conflict changes from\n>\n> fatal: 'commit' is not possible because you have unmerged files.\n> Please, fix them up in the work tree ...\n> ...\n>\n> to\n>\n> error: 'commit' is not possible because you have unmerged files.\n> hint: Please, fix them up in the work tree ...\n> hint: ...\n> fatal: Exiting because of an unresolved conflict.\n\nThanks! Personally, I like it (since the tags on the left make it a\nlittle clearer to the reader what is happening).\n\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -19,6 +19,15 @@ static struct {\n> \t{ \"detachedhead\", &advice_detached_head },\n> };\n> \n> +static void advise(const char *advice, ...)\n> +{\n> +\tva_list params;\n> +\n> +\tva_start(params, advice);\n> +\tvreportf(\"hint: \", advice, params);\n> +\tva_end(params);\n> +}\n\nRather than copy+pasting this code verbatim, wouldn't it make sense to\nmove it and expose it through advice.h so the old call site can use\nthe same code?\n\nFor what it's worth, with that change,\nReviewed-by: Jonathan Nieder \n"},{"id":"170880","messageId":"20110706085007.GB15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 05/14] revert: Propogate errors upwards from do_pick_commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T08:50:08Z","receivedAt":"2011-07-06T08:50:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamkumar Ramachandra wrote:\n\n> Currently, the return value from revert_or_cherry_pick is a\n> non-negative number representing the intended exit status from `git\n> revert` or `git cherry-pick`. Change this by replacing some of the\n> calls to \"die\" with calls to \"error\", so that it can return negative\n> values too. Postive return values indicate conflicts, while negative\n\nThe above seems to be suggesting that the current return value is a\n_problem_, and that this change _fixes_ it.\n\nBut I had thought that the bulk of this patch's changes (die-to-error\nconversions) were not meant as a means to that end but an end in\nthemselves. Wouldn't a clearer problem statement be \"Currently,\nrevert_or_cherry_pick can fail in two ways. If it encounters\nconflicts, it returns a positive number indicating the intended exit\nstatus for the git wrapper to pass on; for all other errors, it\ndie()s. Some callers may not like the latter behavior because of\n\"?\n\nOnly after the reader understands that, she will be ready to\nappreciate the value of the proposed alternate return value\nconvention. Similar comments apply to the commit messages of the few\npatches before --- they are not terribly confusing, but they could\nstill easily be improved by mentioning what problem the patches are\nsupposed to solve.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -250,25 +250,20 @@ static struct tree *empty_tree(void)\n[...]\n> +\tif (action == CHERRY_PICK)\n> +\t\terror(_(\"Your local changes would be overwritten by %s.\"), me);\n> +\telse\n> +\t\terror(_(\"Your local changes would be overwritten by %s.\"), me);\n\ngettext creates one msgid for these two strings, so translators have\nno choice but to give them the same translation. Is that the intent?\n\n[...]\n> +\tif (res < 0)\n> +\t\tdie(_(\"%s failed\"), me);\n> +\treturn res;\n> }\n\nLikewise.\n\nI haven't looked carefully again at the unwinding-after-error, but\naside from that and except as noted above this looks good.\n"},{"id":"170881","messageId":"20110706085519.GC15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-7-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 06/14] revert: Eliminate global \"commit\" variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T08:55:19Z","receivedAt":"2011-07-06T08:55:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar 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> variable to be passed around explicitly as an argument for clarity.\n> This involves changing several functions to take an additional\n> argument, but no functional changes. Additionaly, this will permit\n> more than one commit to be cherry-picked at once, should we choose to\n> develop this functionality in future.\n\nI don't understand the last sentence above --- doesn't \"git cherry-pick\nA B\" work already?\n\nThe patch looks good, except for:\n\n[...]\n> -static void write_cherry_pick_head(void)\n> +static void write_cherry_pick_head(struct commit *commit)\n[...]\n> -\t\t\twrite_cherry_pick_head();\n> +\t\t\twrite_cherry_pick_head(sha1_to_hex(commit));\n\nI don't see how this would even compile.\n"},{"id":"170883","messageId":"20110706090915.GD15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-8-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 07/14] revert: Introduce struct to keep command-line options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T09:09:15Z","receivedAt":"2011-07-06T09:09:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> In later\n> steps in this series, we would like to introduce an API function that\n> calls into this machinery directly and have a way to tell it what to\n> do. Hence, introduce a structure to group these variables, so that\n> the API can take them as a single replay_options parameter.\n>\n> The variable \"me\" is left as a file-scope static variable because it\n> is not an independent option. \"me\" is simply a string that needs to\n> be inferred from the \"action\" option, and is kept global to save each\n> function the trouble of determining it independently.\n\nHm, would it make sense for there to be a \"private\" section at the\nend of the replay_opts struct for variables like this?\n\n> Unfortunately, this patch introduces a minor regression. Parsing\n> strategy-option violates a C89 rule: Initializers cannot refer to\n> variables whose address is not known at compile time. Currently, this\n> rule is violated by some other parts of Git as well, and it is\n> possible to get GCC to report these instances using the \"-std=c89\n> -pedantic\" option.\n\nI would be interested in fixing that (as a patch on top, maybe).\nWhat do you suggest:\n\n A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args\n manipulate a static \"struct replay_opts\" while pick_commits et al\n pass around a pointer to it\n\n B. Make parse_args work like this:\n\n\tcopy from argument to private static struct replay_opts\n\tcall parse_options()\n\tcopy private static struct replay_opts to argument\n\n C. Use new option types:\n\n\tOPT_BOOL_MEMBER('n', \"no-commit\",\n\t\toffsetof(struct replay_opts, no_commit),\n\t\t\"don't automatically commit\"),\n\n and teach parse_options to take an additional parameter like it\n takes \"prefix\" now, to be used as a base address for options that\n write to an offset instead of a pointer\n\nI'm leaning towards A but not sure if that would be wasted work in\nlight of your plans for these APIs in the long run (i.e., is\nparse_args() going to be exposed and want to act on a caller-supplied\nstruct)?\n"},{"id":"170884","messageId":"20110706091334.GE15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-9-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 08/14] revert: Separate cmdline parsing from functional code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T09:13:34Z","receivedAt":"2011-07-06T09:13:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n[...]\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\tdie(_(\"%s failed\"), me);\n> \treturn res;\n\nI'd put the \"me =\" line right after \"opts.action =\" if doing it this\nway. This means callers to pick_commits() are responsible for setting\nthe \"me\" variable and in particular it will not make sense to export\nthat function to callers outside of this file any more, right?\n"},{"id":"170885","messageId":"20110706092009.GF15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-10-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 09/14] revert: Don't create invalid replay_opts in parse_args","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T09:20:09Z","receivedAt":"2011-07-06T09:20:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\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> inelegant design; pick_commits is currently the gatekeeper to the\n> cherry-pick machinery, but this will change in future. To futureproof\n> the code and catch these errors in one place, make sure that an\n> invalid replay_opts structure is not created by parse_args in the\n> first place. Also ensure that regressions in maintaining this\n> invariant are caught in the future by adding an assertion in\n> pick_commits.\n\nAgh! The above seems totally convoluted, and worse, I can see some of\nmy own words in there so I feel I am to blame. Could you please\nexplain, simply, as though I am just an ordinary person, what the idea\nof this patch is? I've heard you talk before. You are quite capable\nof explaining things clearly.\n\nThe patch itself looks good.\n"},{"id":"170886","messageId":"CALkWK0k57L5vc7oVQKw=G9FXtYkmnp8QaWrW_p1CvupyNGofZg@mail.gmail.com","threadId":"27761","inReplyTo":"20110706083512.GA15682@elie","subject":"Re: [PATCH 01/14] advice: Introduce error_resolve_conflict","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T09:28:42Z","receivedAt":"2011-07-06T09:28:42Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> --- a/advice.c\n>> +++ b/advice.c\n>> @@ -19,6 +19,15 @@ static struct {\n>>       { \"detachedhead\", &advice_detached_head },\n>>  };\n>>\n>> +static void advise(const char *advice, ...)\n>> +{\n>> +     va_list params;\n>> +\n>> +     va_start(params, advice);\n>> +     vreportf(\"hint: \", advice, params);\n>> +     va_end(params);\n>> +}\n>\n> Rather than copy+pasting this code verbatim, wouldn't it make sense to\n> move it and expose it through advice.h so the old call site can use\n> the same code?\n\nYes, but I was worried that I shouldn't expose it because your commit\nmessage (2a41df) says:\n\n It is local to revert.c for now because I am not sure this is\n the right API (we may want to take an array of advice lines or a\n boolean argument for easy suppression of unwanted advice).\n\nSo, is it still alright to expose it in advice.h?\n\n> For what it's worth, with that change,\n> Reviewed-by: Jonathan Nieder \n\nThanks.\n\n-- Ram\n"},{"id":"170887","messageId":"CALkWK0kC36d72kxmWz1L=1tdjGeq9BVc2_UUE5SvvKXnefEG4g@mail.gmail.com","threadId":"27761","inReplyTo":"20110706085007.GB15682@elie","subject":"Re: [PATCH 05/14] revert: Propogate errors upwards from do_pick_commit","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T09:30:46Z","receivedAt":"2011-07-06T09:30:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nJonathan Nieder writes:\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -250,25 +250,20 @@ static struct tree *empty_tree(void)\n> [...]\n>> +     if (action == CHERRY_PICK)\n>> +             error(_(\"Your local changes would be overwritten by %s.\"), me);\n>> +     else\n>> +             error(_(\"Your local changes would be overwritten by %s.\"), me);\n>\n> gettext creates one msgid for these two strings, so translators have\n> no choice but to give them the same translation.  Is that the intent?\n>\n> [...]\n>> +     if (res < 0)\n>> +             die(_(\"%s failed\"), me);\n>> +     return res;\n>>  }\n>\n> Likewise.\n\nOops, I didn't realize that! Will fix.\n\n-- Ram\n"},{"id":"170888","messageId":"CALkWK0=eMfMYvysKpixUWt_nCmK3x8EUgKxnstJrAReN8Xi=FA@mail.gmail.com","threadId":"27761","inReplyTo":"20110706091334.GE15682@elie","subject":"Re: [PATCH 08/14] revert: Separate cmdline parsing from functional code","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T09:34:03Z","receivedAt":"2011-07-06T09:34:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n> [...]\n>> @@ -612,7 +610,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n>>\n>>       memset(&opts, 0, sizeof(struct replay_opts));\n>>       opts.action = CHERRY_PICK;\n>> -     res = revert_or_cherry_pick(argc, argv, &opts);\n>> +     git_config(git_default_config, NULL);\n>> +     me = \"cherry-pick\";\n>> +     parse_args(argc, argv, &opts);\n>> +     res = pick_commits(&opts);\n>>       if (res < 0)\n>>               die(_(\"%s failed\"), me);\n>>       return res;\n>\n> I'd put the \"me =\" line right after \"opts.action =\" if doing it this\n> way.  This means callers to pick_commits() are responsible for setting\n> the \"me\" variable and in particular it will not make sense to export\n> that function to callers outside of this file any more, right?\n\nExactly. Good point.\n\n-- Ram\n"},{"id":"170889","messageId":"CALkWK0=qrtKhvg_Ob_ipeG66R=A=2AaxFSFjYrtFqBFxa=UekQ@mail.gmail.com","threadId":"27761","inReplyTo":"20110706085519.GC15682@elie","subject":"Re: [PATCH 06/14] revert: Eliminate global \"commit\" variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T09:37:32Z","receivedAt":"2011-07-06T09:37:32Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> Since we want to develop the functionality to either pick or revert\n>> individual commits atomically later in the series, make \"commit\" a\n>> variable to be passed around explicitly as an argument for clarity.\n>> This involves changing several functions to take an additional\n>> argument, but no functional changes.  Additionaly, this will permit\n>> more than one commit to be cherry-picked at once, should we choose to\n>> develop this functionality in future.\n>\n> I don't understand the last sentence above --- doesn't \"git cherry-pick\n> A B\" work already?\n\nUgh. Removed the line.\n\n> The patch looks good, except for:\n>\n> [...]\n>> -static void write_cherry_pick_head(void)\n>> +static void write_cherry_pick_head(struct commit *commit)\n> [...]\n>> -                     write_cherry_pick_head();\n>> +                     write_cherry_pick_head(sha1_to_hex(commit));\n>\n> I don't see how this would even compile.\n\nRebase fail. Thanks.\n\n-- Ram\n"},{"id":"170891","messageId":"20110706100119.GG15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-11-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 10/14] revert: Persist data for continuation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T10:01:19Z","receivedAt":"2011-07-06T10:01:19Z","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\nI think I remember Junio being curious about which commit is stored in\n\"head\"; this might be a good place to put a reminder so future readers\ndon't have to be confused.\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n[...]\n> @@ -25,6 +26,10 @@\n> * Copyright (c) 2005 Junio C Hamano\n> */\n> \n> +#define SEQ_DIR\t\t\"sequencer\"\n> +#define SEQ_HEAD_FILE\t\"sequencer/head\"\n> +#define SEQ_TODO_FILE\t\"sequencer/todo\"\n\nYay. :)\n\n> @@ -417,7 +422,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\nRemember that \"me\" is an untranslated command name, and see also\nhttp://thread.gmane.org/gmane.comp.version-control.git/153026\n\nPerhaps it would make sense to do something like\n\n\tif (get_sha1(\"HEAD\", head)) {\n\t\tif (opts->action == REVERT)\n\t\t\treturn error(_(\"can't revert as initial commit\"));\n\t\treturn error(_(\"cherry-pick into empty head not supported yet\"));\n\t}\n\nIn a way they feel like different operations, anyway. On the other\nhand, there's no reason I can think of not to allow reverting a patch\nthat only removes files as the initial commit other than not having\nimplemented it.\n\n> @@ -578,10 +583,106 @@ 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\nMaybe some word like \"command\", \"insn\", or \"keyword\" would be more\nsuggestive than \"action\". It also might be worth mentioning somewhere\n(in the commit message?) that this format is inspired by\nrebase--interactive's insn sheet.\n\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;\n> +\tstruct commit_list **next;\n> +\n> +\tprepare_revs(&revs, opts);\n> +\n> +\t/* Insert into todo_list in the same order */\n> +\t/* NEEDSWORK: Expose this as commit_list_append */\n> +\tnext = todo_list;\n> +\twhile ((commit = get_revision(&revs))) {\n> +\t\tnew = xmalloc(sizeof(struct commit_list));\n> +\t\tnew->item = commit;\n> +\t\t*next = new;\n> +\t\tnext = &new->next;\n> +\t}\n> +\t*next = NULL;\n\nThe operation that could be exposed does not include get_revision,\ndoes it?\n\n /*\n * Example:\n *\n *\tstruct commit_list *list;\n *\tstruct commit_list **next = &list;\n *\n *\tnext = commit_list_append(c1, next);\n *\tnext = commit_list_append(c2, next);\n *\t*next = NULL;\n *\tassert(commit_list_count(list) == 2);\n *\treturn list;\n *\n * Don't forget to NULL-terminate!\n */\n struct commit_list **commit_list_append(struct commit *commit,\n\t\t\t\t\tstruct commit_list **next)\n {\n\tstruct commit_list *new = xmalloc(sizeof(*new_list));\n\tnew->item = commit;\n\t*next = new;\n\treturn &new->next;\n }\n\n\n> +static void create_seq_dir(void)\n> +{\n> +\tif (file_exists(git_path(SEQ_DIR))) {\n> +\t\tif (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)\n> +\t\t\tdie(_(\"Could not remove %s\"), git_path(SEQ_DIR));\n> +\t} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)\n> +\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), git_path(SEQ_DIR));\n> +}\n\nA local variable to cache the git_path result would make this much\neasier to read.\n\n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> new file mode 100644\n\nShould \"chmod +x\" so the test can be run directly (I forget to do that\nall the time).\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\nThanks for thinking about these things. Maybe another test\ndemonstrating that the .git/sequencer directory is left behind on\nfailure would help put this in context.\n"},{"id":"170892","messageId":"20110706100331.GH15682@elie","threadId":"27761","inReplyTo":"CALkWK0k57L5vc7oVQKw=G9FXtYkmnp8QaWrW_p1CvupyNGofZg@mail.gmail.com","subject":"Re: [PATCH 01/14] advice: Introduce error_resolve_conflict","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T10:03:31Z","receivedAt":"2011-07-06T10:03:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Yes, but I was worried that I shouldn't expose it because your commit\n> message (2a41df) says:\n>\n> It is local to revert.c for now because I am not sure this is\n> the right API (we may want to take an array of advice lines or a\n> boolean argument for easy suppression of unwanted advice).\n>\n> So, is it still alright to expose it in advice.h?\n\nWell, presumably this second caller is evidence that it is the right\nAPI, no? :)\n\nOf course the API can still easily be changed later.\n"},{"id":"170895","messageId":"20110706101423.GI15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-13-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T10:14:23Z","receivedAt":"2011-07-06T10:14:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> This is perfect but for the fact that 'git reset --hard' doesn't blow\n> away the sequencer state. Why I haven't implemented that yet: should\n> ONLY a hard reset blow away the state?\n\nI don't know. What do you think?\n\nThe constraints I see are:\n\n 1. outside scripts that use \"git cherry-pick\" should continue to work\n\n 2. as a small indication that that's vaguely possible, unrelated parts\n of the test suite should not need to be patched\n\n 3. when a person uses commands like \"git reset --hard\" without\n _intending_ to blow away the sequencer state, it should be possible\n to get the sequencer state back.\n\nFor dealing with \"git rebase --interactive\" and similar porcelain-ish\nscripts, the CHERRY_PICK_HEAD code-path has its own trick of falling\nback to traditional behavior when the GIT_CHERRY_PICK_HELP environment\nvariable is set (see v1.5.4-rc0~106^2~1, revert/cherry-pick: Allow\noverriding the help text by the calling Porcelain, 2007-11-28).\n"},{"id":"170897","messageId":"20110706102509.GJ15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-14-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 13/14] revert: Introduce --continue to continue the operation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T10:25:09Z","receivedAt":"2011-07-06T10:25:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -47,6 +47,7 @@ struct replay_opts {\n> \tenum replay_action action;\n>\n> \tint reset;\n> +\tint contin;\n\nMaybe:\n\n\tint just_remove_state;\n\tint resume;\n\tint abort;\n\nOr:\n\n\tenum replay_subcommand {\n\t\tREPLAY_RESET,\n\t\tREPLAY_CONTINUE,\n\t\tREPLAY_ABORT\n\t};\n\n\tenum replay_subcommand subcommand;\n\nOr perhaps this does not need to be part of the replay_opts struct but\ncan be communicated by which API function gets called (e.g., via\nparse_args returning an \"enum replay_subcommand\"). I dunno.\n"},{"id":"170898","messageId":"20110706103321.GK15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-15-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH 14/14] revert: Change insn sheet format","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T10:33:21Z","receivedAt":"2011-07-06T10:33:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> I've intentionally left parse_cmdline_args unimplemented\n[...]\n> Existing implementations in libraries like Glib are\n> much too heavyweight.\n\nWait, how did glib enter the picture? :) The implementation of\nshell-style quoting in [1] is not very complicated; perhaps it could\ncomplement git's existing parsers for shell-style single-quoted\nexpressions and C-style double-quoted expressions in quote.c.\n\nOf course, a more basic question is whether we want to allow passing\narbitrary command-line arguments through the insn sheet at all (a\npart of me wishes \"no\", at least at first).\n\nCould you give an example to illustrate what this functionality would\nbe used for? I can understand wanting to pass \"-s\" and \"-X\" flags to\na merge insn and \"-X\" to pick, but that's as far as my imagination\ngoes.\n\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/162198\n"},{"id":"170899","messageId":"20110706103734.GL15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-12-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T10:37:34Z","receivedAt":"2011-07-06T10:37:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Write a new function called process_continuation to prepare a\n> todo_list to call pick_commits with; the job of pick_commits is\n> simplified into performing the tasks listed in todo_list.\n\nWhy is it called process_continuation? What is its responsibility?\nWhen would I call it?\n\n> +\t/*\n> +\t * Decide what to do depending on the arguments; a fresh\n> +\t * cherry-pick should be handled differently from an existing\n> +\t * one that is being continued\n> +\t */\n> +\tres = process_continuation(&opts);\n\nIs this the new sole entry point to the cherry-pick/revert machinery?\nIn that case, I'd be mildly tempted to call it something crazy like\nstart_or_continue_replay(), and even more tempted to split it into\nseparate entry points for new_replay(), continue_replay(),\nabort_replay(), and remove_replay_state() (but please don't trust me\nabout the names; this is just to get the idea across).\n"},{"id":"170900","messageId":"20110706104151.GM15682@elie","threadId":"27761","inReplyTo":"1309938868-2028-1-git-send-email-artagnon@gmail.com","subject":"Re: [GSoC update] Sequencer: The insn sheet format","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T10:41:51Z","receivedAt":"2011-07-06T10:41:51Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Thanks for reading.\n\nThanks for the many improvements!\n\nI think the hardest piece in the series so far is what to do about\nthe\n\n\tgit cherry-pick something-i-shouldnt-cherry-pick\n\tgit reset --hard; # or git reset --merge HEAD\n\tgit cherry-pick what-i-should-have-cherry-picked-instead\n\nhabit. Aside from that, it would be nice to have more tests and some\nsimple documentation, and most other things I found were nitpicks.\n"},{"id":"170901","messageId":"CALkWK0nThSNg5Os4b1=S=9h38jDB7X77wM8j=1Jr4FvpRbJtZw@mail.gmail.com","threadId":"27761","inReplyTo":"20110706103321.GK15682@elie","subject":"Re: [RFC PATCH 14/14] revert: Change insn sheet format","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T10:49:14Z","receivedAt":"2011-07-06T10:49:14Z","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>>  I've intentionally left parse_cmdline_args unimplemented\n> [...]\n>>                  Existing implementations in libraries like Glib are\n>>  much too heavyweight.\n>\n> Wait, how did glib enter the picture? :)\n\nCompletely unrelated -- I just incidentally saw \"g_shell_parse_argv\"\nin glib which does what we want, but it's an overkill.\n\n> The implementation of\n> shell-style quoting in [1] is not very complicated; perhaps it could\n> complement git's existing parsers for shell-style single-quoted\n> expressions and C-style double-quoted expressions in quote.c.\n\nOkay.\n\n> Of course, a more basic question is whether we want to allow passing\n> arbitrary command-line arguments through the insn sheet at all (a\n> part of me wishes \"no\", at least at first).\n\nI have an overwhelming desire to say \"no\", but I can't think of an alternative.\n\n> Could you give an example to illustrate what this functionality would\n> be used for?  I can understand wanting to pass \"-s\" and \"-X\" flags to\n> a merge insn and \"-X\" to pick, but that's as far as my imagination\n> goes.\n\nI wasn't imagining anything else. That's just it -- I've just been\nbreaking my head trying to figure out how to do it :|\n\n-- Ram\n"},{"id":"170902","messageId":"CALkWK0mLaCy3LePYYEpby-sd9Z2a=DWq4uWvFiadin7sa_h2+w@mail.gmail.com","threadId":"27761","inReplyTo":"20110706101423.GI15682@elie","subject":"Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T10:55:02Z","receivedAt":"2011-07-06T10:55:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>>  This is perfect but for the fact that 'git reset --hard' doesn't blow\n>>  away the sequencer state.  Why I haven't implemented that yet: should\n>>  ONLY a hard reset blow away the state?\n>\n> I don't know.  What do you think?\n\nI personally don't think any other kind of reset would warrant blowing\naway the sequencer state.\n\n> The constraints I see are:\n>\n>  1. outside scripts that use \"git cherry-pick\" should continue to work\n>\n>  2. as a small indication that that's vaguely possible, unrelated parts\n>    of the test suite should not need to be patched\n\nI don't see how this is possible. I'm tempted to say that we should\ncall this new cherry-picking mechanism with the sequencing\nfunctionality something else like \"git sequencer\"; then it's possible\nto avoid breaking existing scripts.\n\n>  3. when a person uses commands like \"git reset --hard\" without\n>   _intending_ to blow away the sequencer state, it should be possible\n>   to get the sequencer state back.\n\nI like your suggestion here -- simply move .git/sequencer to\n.git/sequencer-old or similar :)\n\n> For dealing with \"git rebase --interactive\" and similar porcelain-ish\n> scripts, the CHERRY_PICK_HEAD code-path has its own trick of falling\n> back to traditional behavior when the GIT_CHERRY_PICK_HELP environment\n> variable is set (see v1.5.4-rc0~106^2~1, revert/cherry-pick: Allow\n> overriding the help text by the calling Porcelain, 2007-11-28).\n\nAh, I see.\n\n-- Ram\n"},{"id":"170905","messageId":"CALkWK0=gm-y3CB83TsSiWWF4qyS5hOJDhOYCz4HTWyT7iHP6MA@mail.gmail.com","threadId":"27761","inReplyTo":"20110706090915.GD15682@elie","subject":"Re: [PATCH 07/14] revert: Introduce struct to keep command-line options","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T11:20:14Z","receivedAt":"2011-07-06T11:20:14Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> In later\n>> steps in this series, we would like to introduce an API function that\n>> calls into this machinery directly and have a way to tell it what to\n>> do.  Hence, introduce a structure to group these variables, so that\n>> the API can take them as a single replay_options parameter.\n>>\n>> The variable \"me\" is left as a file-scope static variable because it\n>> is not an independent option.  \"me\" is simply a string that needs to\n>> be inferred from the \"action\" option, and is kept global to save each\n>> function the trouble of determining it independently.\n>\n> Hm, would it make sense for there to be a \"private\" section at the\n> end of the replay_opts struct for variables like this?\n\nNo. My justification: in later steps, we'd want to be able to mix\n\"pick\" and \"revert\" instructions in the same instruction sheet. This\nwill essentially require the parser to return a commit + a replay_opts\nstruct (which will contain the action information). There's little\npoint in storing 100 \"revert\" strings for the 100 commits we want to\npick when that can easily be inferred from the action.\n\n>> Unfortunately, this patch introduces a minor regression.  Parsing\n>> strategy-option violates a C89 rule: Initializers cannot refer to\n>> variables whose address is not known at compile time.  Currently, this\n>> rule is violated by some other parts of Git as well, and it is\n>> possible to get GCC to report these instances using the \"-std=c89\n>> -pedantic\" option.\n>\n> I would be interested in fixing that (as a patch on top, maybe).\n> What do you suggest:\n>\n>  A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args\n>    manipulate a static \"struct replay_opts\" while pick_commits et al\n>    pass around a pointer to it\n>\n>  B. Make parse_args work like this:\n>\n>        copy from argument to private static struct replay_opts\n>        call parse_options()\n>        copy private static struct replay_opts to argument\n\nThis is something you suggested earlier, but I find it extremely inelegant.\n\n>  C. Use new option types:\n>\n>        OPT_BOOL_MEMBER('n', \"no-commit\",\n>                offsetof(struct replay_opts, no_commit),\n>                \"don't automatically commit\"),\n>\n>    and teach parse_options to take an additional parameter like it\n>    takes \"prefix\" now, to be used as a base address for options that\n>    write to an offset instead of a pointer\n>\n> I'm leaning towards A but not sure if that would be wasted work in\n> light of your plans for these APIs in the long run (i.e., is\n> parse_args() going to be exposed and want to act on a caller-supplied\n> struct)?\n\nYes, I'm definitely considering exposing parse_args in the future,\nespecially since I want to support command-line options in my\ninstruction sheet. Implementing (C) correctly will probably have\nseveral long-term benefits as well -- what do you feel about it?\n\n-- Ram\n"},{"id":"170906","messageId":"CALkWK0=0vBUG_UOLWt+SFMctfW1__OOG-=BAY4WwMGM5OfDj4A@mail.gmail.com","threadId":"27761","inReplyTo":"20110706103734.GL15682@elie","subject":"Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T11:24:12Z","receivedAt":"2011-07-06T11:24:12Z","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>> Write a new function called process_continuation to prepare a\n>> todo_list to call pick_commits with; the job of pick_commits is\n>> simplified into performing the tasks listed in todo_list.\n>\n> Why is it called process_continuation?  What is its responsibility?\n> When would I call it?\n\nI wanted a general name for the features I'm writing (--reset,\n--continue): I want to call these \"continuation features\". I\npersonally like the term, because it reminds me of the \"call/cc\" in\nScheme.\n\n>> +     /*\n>> +      * Decide what to do depending on the arguments; a fresh\n>> +      * cherry-pick should be handled differently from an existing\n>> +      * one that is being continued\n>> +      */\n>> +     res = process_continuation(&opts);\n>\n> Is this the new sole entry point to the cherry-pick/revert machinery?\n\nYes.\n\n> In that case, I'd be mildly tempted to call it something crazy like\n> start_or_continue_replay(), and even more tempted to split it into\n> separate entry points for new_replay(), continue_replay(),\n> abort_replay(), and remove_replay_state() (but please don't trust me\n> about the names; this is just to get the idea across).\n\nWhy? Is introducing new terminology so bad? Should I explain what I\nmean by \"continuation\" in the commit message/ a comment?\n\n-- Ram\n"},{"id":"170909","messageId":"20110706113928.GO15682@elie","threadId":"27761","inReplyTo":"CALkWK0=0vBUG_UOLWt+SFMctfW1__OOG-=BAY4WwMGM5OfDj4A@mail.gmail.com","subject":"Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T11:39:28Z","receivedAt":"2011-07-06T11:39:28Z","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>> In that case, I'd be mildly tempted to call it something crazy like\n>> start_or_continue_replay()\n[...]\n> Why? Is introducing new terminology so bad? Should I explain what I\n> mean by \"continuation\" in the commit message/ a comment?\n\nIf \"process_continuation\" means \"parse .git/sequencer state, which we\nare pretending is a serialized continuation object, and either (a)\ncall it, (b) throw it away, or (c) modify it and then call it\", then\nyes, how do you expect anyone to know what you are talking about?\n\nLess importantly, starting a cherry-pick (which is what pick_commits()\nalready does) doesn't seem to fit in that picture.\n\nA simpler jargon-filled description of this model is checkpoint/\nrestart. But it is an incomplete analogy and still not a great name.\nWith a goal of making future writers' lives happier and more\nproductive in mind, I do not think it is often worth confusing them by\nchoosing a clever presentation of ideas instead of a clear one.\n"},{"id":"170911","messageId":"CALkWK0mXQszjM57E==YCisUng8buyMmP6EGkcrncJOkNJhK35Q@mail.gmail.com","threadId":"27761","inReplyTo":"20110706113928.GO15682@elie","subject":"Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T11:44:59Z","receivedAt":"2011-07-06T11:44:59Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> Jonathan Nieder writes:\n>>> In that case, I'd be mildly tempted to call it something crazy like\n>>> start_or_continue_replay()\n> [...]\n>> Why? Is introducing new terminology so bad?  Should I explain what I\n>> mean by \"continuation\" in the commit message/ a comment?\n>\n> If \"process_continuation\" means \"parse .git/sequencer state, which we\n> are pretending is a serialized continuation object, and either (a)\n> call it, (b) throw it away, or (c) modify it and then call it\", then\n> yes, how do you expect anyone to know what you are talking about?\n>\n> Less importantly, starting a cherry-pick (which is what pick_commits()\n> already does) doesn't seem to fit in that picture.\n>\n> A simpler jargon-filled description of this model is checkpoint/\n> restart.  But it is an incomplete analogy and still not a great name.\n> With a goal of making future writers' lives happier and more\n> productive in mind, I do not think it is often worth confusing them by\n> choosing a clever presentation of ideas instead of a clear one.\n\nThanks for the elaborate explanation; I can see what's wrong with it\nnow. However, I \"start_or_continue_or_stop_or_[insert more options\nhere]_replay\" isn't a good name. I want something future-proof,\nbecause I intend to extend this with more nifty helpers like \"skip\none\". Your earlier \"pick_revisions\" suggestion doesn't sound like a\nbad alternative now -- let me know if you have any other suggestions.\n\nThanks.\n\n-- Ram\n"},{"id":"170913","messageId":"20110706115323.GP15682@elie","threadId":"27761","inReplyTo":"CALkWK0mXQszjM57E==YCisUng8buyMmP6EGkcrncJOkNJhK35Q@mail.gmail.com","subject":"Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T11:53:23Z","receivedAt":"2011-07-06T11:53:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Thanks for the elaborate explanation; I can see what's wrong with it\n> now. However, I \"start_or_continue_or_stop_or_[insert more options\n> here]_replay\" isn't a good name. I want something future-proof,\n> because I intend to extend this with more nifty helpers like \"skip\n> one\". Your earlier \"pick_revisions\" suggestion doesn't sound like a\n> bad alternative now -- let me know if you have any other suggestions.\n\nSounds like sensible reasoning. You are free to choose a name; I just\nwanted to make sure the effect of that name is clear.\n\nIf I were doing it, I would let the API include multiple entry points,\none for each operation (\"start\", \"abort\", \"skip one\", etc).\n\nSide note: I believe a previous patch had a justification of allowing\nfor multiple entry points, which I had thought was preparation for\nthis. It is possible that that patch some other intended positive\neffects, too, though.\n"},{"id":"170914","messageId":"CALkWK0kztvKjfEdx-FTAvU7rNBO2oke8b-Wp3u=yxE9K0WvOCA@mail.gmail.com","threadId":"27761","inReplyTo":"20110706100119.GG15682@elie","subject":"Re: [PATCH 10/14] revert: Persist data for continuation","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T11:55:33Z","receivedAt":"2011-07-06T11:55:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>> 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>\n> I think I remember Junio being curious about which commit is stored in\n> \"head\"; this might be a good place to put a reminder so future readers\n> don't have to be confused.\n\nOops, I totally forgot -- sorry Junio.\nHe suggested that we store the corresponding ref also somewhere. Have\nto think about this some more before the next iteration.\n\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n> [...]\n>> @@ -25,6 +26,10 @@\n>>   * Copyright (c) 2005 Junio C Hamano\n>>   */\n>>\n>> +#define SEQ_DIR              \"sequencer\"\n>> +#define SEQ_HEAD_FILE        \"sequencer/head\"\n>> +#define SEQ_TODO_FILE        \"sequencer/todo\"\n>\n> Yay. :)\n\nSorry it took me so long to understand this. Your elaborate\nexplanation last time drove the point home.\n\n>> @@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n>>                       return error(_(\"Your index file is unmerged.\"));\n>>       } else {\n>>               if (get_sha1(\"HEAD\", head))\n>> -                     return error(_(\"You do not have a valid HEAD\"));\n>> +                     return error(_(\"Can't %s on an unborn branch\"), me);\n>\n> Remember that \"me\" is an untranslated command name, and see also\n> http://thread.gmane.org/gmane.comp.version-control.git/153026\n>\n> Perhaps it would make sense to do something like\n>\n>        if (get_sha1(\"HEAD\", head)) {\n>                if (opts->action == REVERT)\n>                        return error(_(\"can't revert as initial commit\"));\n>                return error(_(\"cherry-pick into empty head not supported yet\"));\n>        }\n>\n> In a way they feel like different operations, anyway.  On the other\n> hand, there's no reason I can think of not to allow reverting a patch\n> that only removes files as the initial commit other than not having\n> implemented it.\n\nOkay. That would be unrelated to this patch though -- I'll make it a\nseparate patch and move it to the beginning of the series.\n\n>> @@ -578,10 +583,106 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n>>       rollback_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>> +                     struct replay_opts *opts)\n>> +{\n>> +     struct commit_list *cur = NULL;\n>> +     struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n>> +     const char *sha1_abbrev = NULL;\n>> +     const char *action;\n>> +\n>> +     action = (opts->action == REVERT ? \"revert\" : \"pick\");\n>> +     for (cur = todo_list; cur; cur = cur->next) {\n>> +             sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n>> +             if (get_message(cur->item, &msg))\n>> +                     die(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n>> +             strbuf_addf(buf, \"%s %s %s\\n\", action, sha1_abbrev, msg.subject);\n>\n> Maybe some word like \"command\", \"insn\", or \"keyword\" would be more\n> suggestive than \"action\".  It also might be worth mentioning somewhere\n> (in the commit message?) that this format is inspired by\n> rebase--interactive's insn sheet.\n\nOkay, will do.\n\n> The operation that could be exposed does not include get_revision,\n> does it?\n>\n>  /*\n>  * Example:\n>  *\n>  *     struct commit_list *list;\n>  *     struct commit_list **next = &list;\n>  *\n>  *     next = commit_list_append(c1, next);\n>  *     next = commit_list_append(c2, next);\n>  *     *next = NULL;\n>  *     assert(commit_list_count(list) == 2);\n>  *     return list;\n>  *\n>  * Don't forget to NULL-terminate!\n>  */\n>  struct commit_list **commit_list_append(struct commit *commit,\n>                                        struct commit_list **next)\n>  {\n>        struct commit_list *new = xmalloc(sizeof(*new_list));\n>        new->item = commit;\n>        *next = new;\n>        return &new->next;\n>  }\n\nI would have done this, but I was worried about what the NULL\ntermination would mean API-wise. In retrospect, a lot of APIs\ndescribed in Documentation/technical are pretty non-trivial, and it's\nnot obvious how to use it without the documentation. Would it be okay\nto expose this in commit.c and write some documentation? I already\nhave two callers.\n\n>> +static void create_seq_dir(void)\n>> +{\n>> +     if (file_exists(git_path(SEQ_DIR))) {\n>> +             if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)\n>> +                     die(_(\"Could not remove %s\"), git_path(SEQ_DIR));\n>> +     } else if (mkdir(git_path(SEQ_DIR), 0777) < 0)\n>> +             die_errno(_(\"Could not create sequencer directory '%s'.\"), git_path(SEQ_DIR));\n>> +}\n>\n> A local variable to cache the git_path result would make this much\n> easier to read.\n\nOkay.\n\n>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n>> new file mode 100644\n>\n> Should \"chmod +x\" so the test can be run directly (I forget to do that\n> all the time).\n\nGood catch.\n\n>> +test_expect_success 'cherry-pick cleans up sequencer directory upon success' '\n>> +     pristine_detach initial &&\n>> +     git cherry-pick initial..picked &&\n>> +     test_path_is_missing .git/sequencer\n>> +'\n>\n> Thanks for thinking about these things.  Maybe another test\n> demonstrating that the .git/sequencer directory is left behind on\n> failure would help put this in context.\n\nGood suggestion.\n\n-- Ram\n"},{"id":"170915","messageId":"20110706120616.GQ15682@elie","threadId":"27761","inReplyTo":"CALkWK0=gm-y3CB83TsSiWWF4qyS5hOJDhOYCz4HTWyT7iHP6MA@mail.gmail.com","subject":"Re: [PATCH 07/14] revert: Introduce struct to keep command-line options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T12:06:16Z","receivedAt":"2011-07-06T12:06:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> My justification: in later steps, we'd want to be able to mix\n> \"pick\" and \"revert\" instructions in the same instruction sheet. This\n> will essentially require the parser to return a commit + a replay_opts\n> struct (which will contain the action information).\n\nSide note: is it intended to support insns like\n\n pick A..B\n\n?\n\nAnyway, the above explanation about the intended use for \"struct\nreplay_opts\" (it needs to be small, I guess?) would be a good thing\nto add to the commit message, too. Basically, whatever information a\nperson needs in order to understand the design is a useful thing to\nadd.\n\n[...]\n> Yes, I'm definitely considering exposing parse_args in the future,\n> especially since I want to support command-line options in my\n> instruction sheet.\n\nHm, I am not sure what to think about this direction (is the git\nsequencer actually just a fast git shell? in that case, why is pick\nspelled \"pick\" instead of \"cherry-pick\"?). Maybe it's a good thing.\n\nThanks for some useful clarifications.\n"},{"id":"170922","messageId":"CALkWK0nhH=_3+Gj=O4Ov2zOGBoVquzD--+RW_QYYnb8YMAHniA@mail.gmail.com","threadId":"27761","inReplyTo":"1309938868-2028-13-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-06T14:32:29Z","receivedAt":"2011-07-06T14:32:29Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nRamkumar Ramachandra writes:\n>  builtin/revert.c                   |   54 ++++++++++++++++++++++++++++++-----\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, 121 insertions(+), 28 deletions(-)\n\nBy making `reset --hard` blow away the sequencer state, and trying\nhard not to modify existing scripts, the diffstat still contains these\nfiles:\nbuiltin/revert.c\ngit-rebase--interactive.sh\nt/t3032-merge-recursive-options.sh\nr/t3505-cherry-pick-empty.sh\nr/t3507-cherry-pick-conflict.sh\nr/t3501-cherry-pick-sequence.sh\n\n> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\n> index 2b17311..81191ae 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>        git read-tree --reset -u HEAD &&\n>        test_must_fail git cherry-pick --no-commit remote &&\n> +       git cherry-pick --reset &&\n>        git read-tree --reset -u HEAD &&\n>        test_must_fail git cherry-pick remote &&\n> +       git cherry-pick --reset &&\n>        test_must_fail git update-index --refresh &&\n>        grep \"<<<<<<\" text.txt\n>  '\n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index 595d2ff..e0c805d 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>        echo content >extra_file &&\n>        git add extra_file &&\n>        test_must_fail git revert HEAD 2>errors &&\n> +       git revert --reset &&\n>        test_i18ngrep \"Your local changes would be overwritten by \" errors\n>\n>  '\n\n> diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\n> index c10b28c..dc02227 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> -       git checkout master && {\n> -               git cherry-pick empty-branch^\n> -               test \"$?\" = 1\n> -       }\n> +       git checkout master &&\n> +       test_expect_code 1 git cherry-pick empty-branch^\n> +       git cherry-pick --reset\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> -       git checkout master && {\n> -               git cherry-pick empty-branch\n> -               test \"$?\" = 1\n> -       }\n> +       git checkout master &&\n> +       test_expect_code 1 git cherry-pick empty-branch &&\n> +       git cherry-pick --reset\n>  '\n>\n>  test_expect_success 'index lockfile was removed' '\n\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 212ec54..86f8626 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>        head=$(git rev-parse HEAD) &&\n>        test_must_fail git cherry-pick picked &&\n> +       git cherry-pick --reset &&\n>        newhead=$(git rev-parse HEAD) &&\n>\n>        test \"$head\" = \"$newhead\"\n> @@ -55,6 +56,7 @@ test_expect_success 'advice from failed cherry-pick' \"\n>        hint: and commit the result with 'git commit'\n>        EOF\n>        test_must_fail git cherry-pick picked 2>actual &&\n> +       git cherry-pick --reset &&\n>\n>        test_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>        pristine_detach initial &&\n>        test_must_fail git cherry-pick picked &&\n> -       test_cmp_rev picked CHERRY_PICK_HEAD\n> +       test_cmp_rev picked CHERRY_PICK_HEAD &&\n> +       git cherry-pick --reset\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>                export GIT_CHERRY_PICK_HELP &&\n>                test_must_fail git cherry-pick picked\n>        ) &&\n> -       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&\n> +       git cherry-pick --reset\n>  '\n>\n>  test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n>        pristine_detach initial &&\n>\n>        test_must_fail git cherry-pick picked &&\n> +       git cherry-pick --reset &&\n>        git reset &&\n>\n>        test_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>        test_must_fail git cherry-pick picked &&\n>        test_must_fail git commit &&\n>\n> -       test_cmp_rev picked CHERRY_PICK_HEAD\n> +       test_cmp_rev picked CHERRY_PICK_HEAD &&\n> +       git cherry-pick --reset\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>                test_must_fail git commit\n>        ) &&\n>\n> -       test_cmp_rev picked CHERRY_PICK_HEAD\n> +       test_cmp_rev picked CHERRY_PICK_HEAD &&\n> +       git cherry-pick --reset\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>        git add foo &&\n>        git commit &&\n>\n> -       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&\n> +       git cherry-pick --reset\n>  '\n>\n>  test_expect_success 'failed cherry-pick produces dirty index' '\n>        pristine_detach initial &&\n>\n>        test_must_fail git cherry-pick picked &&\n> +       git cherry-pick --reset &&\n>\n>        test_must_fail git update-index --refresh -q &&\n>        test_must_fail git diff-index --exit-code HEAD\n> @@ -160,6 +169,7 @@ test_expect_success 'failed cherry-pick registers participants in index' '\n>        git read-tree -u --reset HEAD &&\n>\n>        test_must_fail git cherry-pick picked &&\n> +       git cherry-pick --reset &&\n>        git ls-files --stage --unmerged > actual &&\n>\n>        test_cmp expected actual\n> @@ -176,6 +186,7 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' '\n>        EOF\n>\n>        test_must_fail git cherry-pick picked &&\n> +       git cherry-pick --reset &&\n>\n>        sed \"s/[a-f0-9]*\\.\\.\\./objid/\" foo > actual &&\n>        test_cmp expected actual\n> @@ -195,6 +206,7 @@ test_expect_success 'diff3 -m style' '\n>        EOF\n>\n>        test_must_fail git cherry-pick picked &&\n> +       git cherry-pick --reset &&\n>\n>        sed \"s/[a-f0-9]*\\.\\.\\./objid/\" foo > actual &&\n>        test_cmp expected actual\n> @@ -227,6 +239,7 @@ test_expect_success 'revert also handles conflicts sanely' '\n>\n>        head=$(git rev-parse HEAD) &&\n>        test_must_fail git revert picked &&\n> +       git revert --reset &&\n>        newhead=$(git rev-parse HEAD) &&\n>        git ls-files --stage --unmerged > actual-stages &&\n>\n> @@ -252,6 +265,7 @@ test_expect_success 'revert conflict, diff3 -m style' '\n>        EOF\n>\n>        test_must_fail git revert picked &&\n> +       git revert --reset &&\n>\n>        sed \"s/[a-f0-9]*\\.\\.\\./objid/\" foo > actual &&\n>        test_cmp expected actual\n\nAs you can see, there is no \"reset --hard\" in these, and I don't see\nwhat other command I can piggy-bank on to blow away the sequencer\nstate. There is however, one other thing I can do: if there is\nnothing left to cherry-pick after a successful conflict resolution +\ngit commit, I can modify commit.c to blow away the sequencer state\nafter checking appropriately. This will also have a nice end-user\nexperience side-effect:\n$ git cherry-pick moo\nfatal: Conflict in foo!\n$ echo \"Resolved\" > foo\n$ git add moo\n$ git commit\n$ git cherry-pick --continue # This no-op will be unnecessary\n\nThen again, teaching commit about the sequencer is inelegant, and it's\npossible to achieve this effect in another way: when a conflict is\nencountered in the sequencer && length(todo_file) == 1, throw away the\nsequencer state. When I say \"throw away\", I really mean \"move\n.git/sequencer to .git/sequencer-old\". Does this seem reasonable?\n\n-- Ram\n"},{"id":"170941","messageId":"20110706192141.GA23516@elie","threadId":"27761","inReplyTo":"CALkWK0nhH=_3+Gj=O4Ov2zOGBoVquzD--+RW_QYYnb8YMAHniA@mail.gmail.com","subject":"Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T19:21:41Z","receivedAt":"2011-07-06T19:21:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n>> --- a/t/t3507-cherry-pick-conflict.sh\n>> +++ b/t/t3507-cherry-pick-conflict.sh\n[...]\n> As you can see, there is no \"reset --hard\" in these\n\nIn this one, there's \"git checkout -f && git read-tree -u --reset HEAD\n&& git clean -fdx\", which is almost the same thing.\n\n>> --- a/t/t3505-cherry-pick-empty.sh\n>> +++ b/t/t3505-cherry-pick-empty.sh\n\nThis one is not a typical script, I think --- if you knew the\ncherry-pick was going to be empty, why did you try it in the first\nplace? I think it would make sense to make it \"git reset --hard\" at\nthe beginning of each test as a separate, preparatory patch with\nexplanation.\n\n[...]\n> There is however, one other thing I can do: if there is\n> nothing left to cherry-pick after a successful conflict resolution +\n> git commit, I can modify commit.c to blow away the sequencer state\n> after checking appropriately. This will also have a nice end-user\n> experience side-effect:\n> $ git cherry-pick moo\n> fatal: Conflict in foo!\n> $ echo \"Resolved\" > foo\n> $ git add moo\n> $ git commit\n> $ git cherry-pick --continue # This no-op will be unnecessary\n\nThough it's not obvious to me how this would affect the scripts above,\nit sounds like a nice enhancement to me independently, fwiw.\n\n> Then again, teaching commit about the sequencer is inelegant,\n\nIt's possible to add some hook-like thing to do this, or to structure\nthe code as if a hook was used.\n\n> and it's\n> possible to achieve this effect in another way: when a conflict is\n> encountered in the sequencer && length(todo_file) == 1, throw away the\n> sequencer state.\n\nYep, that seems like basically the same effect. Are there downsides?\n(Maybe years from now when a \"git cherry-pick --rewind\" is introduced\nwe would regret this? But that can be figured out years from now.)\n"},{"id":"170944","messageId":"20110706195610.GA23984@elie","threadId":"27761","inReplyTo":"20110706192141.GA23516@elie","subject":"Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-06T19:56:10Z","receivedAt":"2011-07-06T19:56:10Z","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>> and it's\n>> possible to achieve this effect in another way: when a conflict is\n>> encountered in the sequencer && length(todo_file) == 1, throw away the\n>> sequencer state.\n>\n> Yep, that seems like basically the same effect. Are there downsides?\n> (Maybe years from now when a \"git cherry-pick --rewind\" is introduced\n> we would regret this? But that can be figured out years from now.)\n\nDoh, I'm not thinking straight. Would this break \"git cherry-pick\n--abort\", or is there some hack layered on top to avoid that? Making\n\"git commit\" remove the .git/sequencer for the last commit of the\nsequence seems a little saner.\n"},{"id":"170945","messageId":"7vd3hnktuf.fsf@alter.siamese.dyndns.org","threadId":"27761","inReplyTo":"1309938868-2028-12-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-06T21:00:24Z","receivedAt":"2011-07-06T21:00:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Write a new function called process_continuation to prepare a\n> todo_list to call pick_commits with; the job of pick_commits is\n> simplified into performing the tasks listed in todo_list. This will\n> be useful when continuation functionality like \"--continue\" is\n> introduced later in the series.\n>\n> Helped-by: Jonathan Nieder \n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> builtin/revert.c | 36 +++++++++++++++++++++++++-----------\n> 1 files changed, 25 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 7d76f92..8cdcdb6 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -677,10 +677,8 @@ 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> @@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)\n> \t\t\t\topts->record_origin || opts->edit));\n> \tread_and_refresh_cache(me, opts);\n> \n> -\twalk_revs_populate_todo(&todo_list, opts);\n> -\tcreate_seq_dir();\n> -\tif (!get_sha1(\"HEAD\", sha1))\n> -\t\tsave_head(sha1_to_hex(sha1));\n> -\tsave_todo(todo_list, opts);\n> -\n> \tfor (cur = todo_list; cur; cur = cur->next) {\n> \t\tsave_todo(cur, opts);\n> \t\tres = do_pick_commit(cur->item, opts);\n> @@ -710,6 +702,22 @@ 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> +\tcreate_seq_dir();\n> +\tif (!get_sha1(\"HEAD\", sha1))\n> +\t\tpersist_head(sha1_to_hex(sha1));\n> +\tpersist_todo(todo_list, opts);\n\nDon't these two need forward declarations before their use?\n"},{"id":"170946","messageId":"7vzkkrjec2.fsf@alter.siamese.dyndns.org","threadId":"27761","inReplyTo":"1309938868-2028-8-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 07/14] revert: Introduce struct to keep command-line options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-06T21:20:45Z","receivedAt":"2011-07-06T21:20:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> The variable \"me\" is left as a file-scope static variable because it\n> is not an independent option. \"me\" is simply a string that needs to\n> be inferred from the \"action\" option, and is kept global to save each\n> function the trouble of determining it independently.\n\nWould it make more sense to remove the variable, pass \"action\" around\nwhere only \"me\" is passed around right now, and introduce a function\n\"static const char *action_name()\" to help places that wants textual\ncommand name for display purposes?\n"},{"id":"170947","messageId":"7vsjqjjebq.fsf@alter.siamese.dyndns.org","threadId":"27761","inReplyTo":"20110706100119.GG15682@elie","subject":"Re: [PATCH 10/14] revert: Persist data for continuation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-06T21:20:57Z","receivedAt":"2011-07-06T21:20:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> /*\n> * Example:\n> *\n> *\tstruct commit_list *list;\n> *\tstruct commit_list **next = &list;\n> *\n> *\tnext = commit_list_append(c1, next);\n> *\tnext = commit_list_append(c2, next);\n> *\t*next = NULL;\n> *\tassert(commit_list_count(list) == 2);\n> *\treturn list;\n> *\n> * Don't forget to NULL-terminate!\n> */\n\nI wonder if the optimization to allow omitting unnecessary NULL\ntermination inside a tight loop by the caller is really worth the\npotential trouble, though.\n"},{"id":"170948","messageId":"7vliwbjebk.fsf@alter.siamese.dyndns.org","threadId":"27761","inReplyTo":"1309938868-2028-14-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 13/14] revert: Introduce --continue to continue the operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-06T21:21:03Z","receivedAt":"2011-07-06T21:21:03Z","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 9e18d64..6ef56ee 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -47,6 +47,7 @@ struct replay_opts {\n> \tenum replay_action action;\n> \n> \tint reset;\n> +\tint contin;\n\nAs Jonathan mentiond, these three look like all action's to me.\n\n> +static void verify_opt_mutually_compatible(const char *me, ...)\n> +{\n\nIsn't \"being compatible\" by definition \"mutual\"? \n\nI.e. verify-option-compatibility perhaps?\n"},{"id":"170949","messageId":"1309989158.13231.7.camel@drew-northup.unet.maine.edu","threadId":"27761","inReplyTo":"7vliwbjebk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 13/14] revert: Introduce --continue to continue the operation","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-07-06T21:52:38Z","receivedAt":"2011-07-06T21:52:38Z","isPatch":true,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Wed, 2011-07-06 at 14:21 -0700, Junio C Hamano wrote:\n> > +static void verify_opt_mutually_compatible(const char *me, ...)\n> > +{\n> \n> Isn't \"being compatible\" by definition \"mutual\"? \n\nAs a strict matter of language, no. And example we see far too often\n@work is that MS Office 2010 is compatible with MS Office 2007, but the\nsame cannot be said the other way around. (Now if I can convince them\nthat not everyone in the world has MS Office...)\n> \n> I.e. verify-option-compatibility perhaps?\n\n+1 sensible, if that's the intention.\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"170955","messageId":"CALkWK0nfakukih6m=r86RA8xvNt2BxJaUUNw9XRJ4Z6n3e=sJg@mail.gmail.com","threadId":"27761","inReplyTo":"20110706192141.GA23516@elie","subject":"Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-07T03:03:54Z","receivedAt":"2011-07-07T03:03:54Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nJonathan Nieder writes:\n> Ramkumar Ramachandra wrote:\n>\n>>> --- a/t/t3507-cherry-pick-conflict.sh\n>>> +++ b/t/t3507-cherry-pick-conflict.sh\n> [...]\n>> As you can see, there is no \"reset --hard\" in these\n>\n> In this one, there's \"git checkout -f && git read-tree -u --reset HEAD\n> && git clean -fdx\", which is almost the same thing.\n\nOfcourse, but it's a combination of three commands -- are you\nsuggesting that I replace this sequence of commands with \"git reset\n--hard\"?.\n\n>>> --- a/t/t3505-cherry-pick-empty.sh\n>>> +++ b/t/t3505-cherry-pick-empty.sh\n>\n> This one is not a typical script, I think --- if you knew the\n> cherry-pick was going to be empty, why did you try it in the first\n> place?  I think it would make sense to make it \"git reset --hard\" at\n> the beginning of each test as a separate, preparatory patch with\n> explanation.\n>\n> [...]\n>> There is however, one other thing I can do: if there is\n>> nothing left to cherry-pick after a successful conflict resolution +\n>> git commit, I can modify commit.c to blow away the sequencer state\n>> after checking appropriately.  This will also have a nice end-user\n>> experience side-effect:\n>> $ git cherry-pick moo\n>> fatal: Conflict in foo!\n>> $ echo \"Resolved\" > foo\n>> $ git add moo\n>> $ git commit\n>> $ git cherry-pick --continue # This no-op will be unnecessary\n>\n> Though it's not obvious to me how this would affect the scripts above,\n> it sounds like a nice enhancement to me independently, fwiw.\n\nOh, it fixes everything :)\nJust see my GitHub fork.\n\n>> Then again, teaching commit about the sequencer is inelegant,\n>\n> It's possible to add some hook-like thing to do this, or to structure\n> the code as if a hook was used.\n>\n>> and it's\n>> possible to achieve this effect in another way: when a conflict is\n>> encountered in the sequencer && length(todo_file) == 1, throw away the\n>> sequencer state.\n>\n> Yep, that seems like basically the same effect.  Are there downsides?\n> (Maybe years from now when a \"git cherry-pick --rewind\" is introduced\n> we would regret this?  But that can be figured out years from now.)\n\n> Doh, I'm not thinking straight.  Would this break \"git cherry-pick\n> --abort\", or is there some hack layered on top to avoid that?  Making\n> \"git commit\" remove the .git/sequencer for the last commit of the\n> sequence seems a little saner.\n\nWe could always inject a hack to avoid this. I'm not yet convinced\nthat we should teach commit about the sequencer, especially since the\npatch to do it from the sequencer end is so simple. Please have a\nlook at the patch [1], and let me know what you think. I could submit\nit as an RFC patch to the list for convenience, but I'm afraid it'll\nbe missing context.\n\n[1]: https://github.com/artagnon/git/commit/0653bcccfa8d69687ed939f07f5b32dd14d302d3\n\n-- Ram\n"},{"id":"170966","messageId":"CALkWK0mQS_8sNKP2T4QOCM_ahKTYvpRMnJz4Ltzq=zNsQTxkKQ@mail.gmail.com","threadId":"27761","inReplyTo":"7vd3hnktuf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-07T06:31:27Z","receivedAt":"2011-07-07T06:31: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>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index 7d76f92..8cdcdb6 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -677,10 +677,8 @@ static int cleanup_sequencer_data(void)\n>>       return 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>> -     struct commit_list *todo_list = NULL;\n>> -     unsigned char sha1[20];\n>>       struct commit_list *cur;\n>>       int res;\n>>\n>> @@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)\n>>                               opts->record_origin || opts->edit));\n>>       read_and_refresh_cache(me, opts);\n>>\n>> -     walk_revs_populate_todo(&todo_list, opts);\n>> -     create_seq_dir();\n>> -     if (!get_sha1(\"HEAD\", sha1))\n>> -             save_head(sha1_to_hex(sha1));\n>> -     save_todo(todo_list, opts);\n>> -\n>>       for (cur = todo_list; cur; cur = cur->next) {\n>>               save_todo(cur, opts);\n>>               res = do_pick_commit(cur->item, opts);\n>> @@ -710,6 +702,22 @@ static int pick_commits(struct replay_opts *opts)\n>>       return cleanup_sequencer_data();\n>>  }\n>>\n>> +static int process_continuation(struct replay_opts *opts)\n>> +{\n>> +     struct commit_list *todo_list = NULL;\n>> +     unsigned char sha1[20];\n>> +\n>> +     read_and_refresh_cache(me, opts);\n>> +\n>> +     walk_revs_populate_todo(&todo_list, opts);\n>> +     create_seq_dir();\n>> +     if (!get_sha1(\"HEAD\", sha1))\n>> +             persist_head(sha1_to_hex(sha1));\n>> +     persist_todo(todo_list, opts);\n>\n> Don't these two need forward declarations before their use?\n\nGood catch. I'm surprised the compiler didn't catch these.\n\n-- Ram\n"},{"id":"170967","messageId":"CALkWK0kSqOkUBZhK2ZxrGpJCMmgzB2mPeLoyZUGLDnHzdDvJsA@mail.gmail.com","threadId":"27761","inReplyTo":"7vliwbjebk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 13/14] revert: Introduce --continue to continue the operation","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-07T06:35:31Z","receivedAt":"2011-07-07T06:35:31Z","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>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index 9e18d64..6ef56ee 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -47,6 +47,7 @@ struct replay_opts {\n>>       enum replay_action action;\n>>\n>>       int reset;\n>> +     int contin;\n>\n> As Jonathan mentiond, these three look like all action's to me.\n\nTrue. I'll try to figure something out before the next iteration.\n\n>> +static void verify_opt_mutually_compatible(const char *me, ...)\n>> +{\n>\n> Isn't \"being compatible\" by definition \"mutual\"?\n>\n> I.e. verify-option-compatibility perhaps?\n\nThere's already a verify_opt_compatible which checks that the first\noption supplied is compatible with all the other options. This one\nchecks that all the the options all the options are compatible with\neach other. What names do you suggest?\n\n-- Ram\n"},{"id":"171134","messageId":"CALkWK0mnhsCZoz0PLBAEL_Z9MOSfGDYTWj6ToNqyutxvfkfNCA@mail.gmail.com","threadId":"27761","inReplyTo":"20110706120616.GQ15682@elie","subject":"Re: [PATCH 07/14] revert: Introduce struct to keep command-line options","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-12T06:14:53Z","receivedAt":"2011-07-12T06:14:53Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nSorry about the delayed replay -- I intended to reply to this earlier;\nI'm not sure why I didn't.\n\nJonathan Nieder writes:\n>> My justification: in later steps, we'd want to be able to mix\n>> \"pick\" and \"revert\" instructions in the same instruction sheet.  This\n>> will essentially require the parser to return a commit + a replay_opts\n>> struct (which will contain the action information).\n>\n> Side note: is it intended to support insns like\n>\n>  pick A..B\n>\n> ?\n\nI wouldn't point to that instruction specifically, but yes- I plan to\nsupport complex instructions in future.\n\n> Anyway, the above explanation about the intended use for \"struct\n> replay_opts\" (it needs to be small, I guess?) would be a good thing\n> to add to the commit message, too.  Basically, whatever information a\n> person needs in order to understand the design is a useful thing to\n> add.\n\nI'm not yet sure about this. On a related note, I'd like to ask: will\nwe encounter two commands like \"am\" and \"revert\" which have the same\ncommand-line option name for two different functionality? If not, I'd\nprobably like to stick to the per-session opts for as long as possible\n(ie. until someone finds a good usecase + implementation for\nper-action command-line options).\n\n>> Yes, I'm definitely considering exposing parse_args in the future,\n>> especially since I want to support command-line options in my\n>> instruction sheet.\n>\n> Hm, I am not sure what to think about this direction (is the git\n> sequencer actually just a fast git shell?  in that case, why is pick\n> spelled \"pick\" instead of \"cherry-pick\"?).  Maybe it's a good thing.\n\nThis is a very important question. Yes, it's intentionally named\n\"pick\" and not \"cherry-pick\" because I don't want the Sequencer to\nmerely be a fast git shell. That's part of the reason I don't want\narbitrary command-line options on the insn sheet. I want the\npossibility of accommodating complex instructions in the insn sheet\n(single instructions that might even involve talking to more than one\ngit command). For the same reason, I also felt that \"action\" is the\nmost appropriate name for the first word in each line in the insn\nsheet. What I'd call an \"instruction\" is an action + the relevant\noption(s) picked up from the opts sheet.\n\n-- Ram\n"},{"id":"171135","messageId":"CALkWK0mxXwvurJzDOYOdXS33cb563WnKmG1_cue6F4pgqOU_WQ@mail.gmail.com","threadId":"27761","inReplyTo":"7vzkkrjec2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 07/14] revert: Introduce struct to keep command-line options","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-12T06:21:56Z","receivedAt":"2011-07-12T06:21:56Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nAgain- intended to reply to this earlier; sorry.\n\nJunio C Hamano writes:\n> Ramkumar Ramachandra writes:\n>> The variable \"me\" is left as a file-scope static variable because it\n>> is not an independent option.  \"me\" is simply a string that needs to\n>> be inferred from the \"action\" option, and is kept global to save each\n>> function the trouble of determining it independently.\n>\n> Would it make more sense to remove the variable, pass \"action\" around\n> where only \"me\" is passed around right now, and introduce a function\n> \"static const char *action_name()\" to help places that wants textual\n> command name for display purposes?\n\nOkay, let me put it like this: \"me\" exists because cherry-pick and\nrevert functionalities are mixed in the same file; builtin/revert.c.\nIn future, the sequencer in general will support many more actions --\nand we will definitely require an \"opts->action to instruction sheet\nkeyword\" translation, and that'll probably be some sort of struct.\nSince I'm not sure the function you propose will make it to\nsequencer.c, I don't want to introduce it now. Let's wait and see how\nit shapes up.\n\nThanks.\n\n-- Ram\n"},{"id":"171139","messageId":"20110712063335.GA10742@elie","threadId":"27761","inReplyTo":"CALkWK0mxXwvurJzDOYOdXS33cb563WnKmG1_cue6F4pgqOU_WQ@mail.gmail.com","subject":"Re: [PATCH 07/14] revert: Introduce struct to keep command-line options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-07-12T06:33:35Z","receivedAt":"2011-07-12T06:33:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Since I'm not sure the function you propose will make it to\n> sequencer.c, I don't want to introduce it now.\n\nWouldn't it be easy to remove such a function later?\n\nPractically speaking, it is not obvious to me that making any of these\nvariables non-static is needed for \"cherry-pick --continue\" to work,\nbut given that most of the state is being made non-static anyway,\nreaders will be likely to wonder why \"me\" is left behind. So the\nobvious choices would be to\n\n a. make \"me\" a member of the replay_opts struct; or\n b. compute \"me\" in each function that needs it by calling a helper\n function; or\n c. add some explanation to the commit message to clarify the status\n of \"me\" as static-but-won't-be-in-the-long-term and a reason for\n that\n\nbased on the needs of the current code. (b) sounds simplest to me,\nthough I haven't tried it.\n"}]}