{"thread":{"id":"27989","subject":"[GSoC update] Iterating over a stable series","startedAt":"2011-08-02T04:54:43Z","lastAt":"2011-08-03T01:32:50Z","messageCount":6,"participants":["Ramkumar Ramachandra","Christian Couder","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"172628","messageId":"1312260884-5087-1-git-send-email-artagnon@gmail.com","threadId":"27989","inReplyTo":null,"subject":"[GSoC update] Iterating over a stable series","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-02T04:54:43Z","receivedAt":"2011-08-02T04:54:43Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThe post midterm work is suffering because I'm constantly rewriting\nthe sequencer-stable series: I often resort to throwing away big\npatches that move functions from builtin/revert.c to sequencer.c.\nHopefully, the latest iteration [1] will not require rewriting.\n\nI'd like some early feedback for one of the \"design patches\" in my new\nseries: I've chosen to use a commit + action to represent a todo_list.\nI'd initially tried a commit + opts keeping future expantion in mind\n(allowing instruction-specific command-line options), but the result\nis quite inelegant.  Although commit message/ tests are missing, I'd\nlike to describe the intent in detail:\n\nThis patch is a prerequisite for decoupling todo parsing from opts\nparsing.  I want to decouple them so that I can achieve tighter\ncoupling between \"git commit\" and the sequencer [2].  After that, I\nwant to craft a nice API and move/ expose various functions in\nbuiltin/revert.c starting with the parsing functions.\n\nThanks for reading.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/178372\n[2]: http://mid.gmane.org/CALkWK0=9kwgtZB-BA12tOQrQXS8XRbhTg6K=Ak00o2nurX16Fg@mail.gmail.com\n\nRamkumar Ramachandra (1):\n  revert: Allow arbitrary sequencer instructions\n\n builtin/revert.c |  101 +++++++++++++++++++++++++++++-------------------------\n sequencer.h      |   10 +++++\n 2 files changed, 64 insertions(+), 47 deletions(-)\n\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"172629","messageId":"1312260884-5087-2-git-send-email-artagnon@gmail.com","threadId":"27989","inReplyTo":"1312260884-5087-1-git-send-email-artagnon@gmail.com","subject":"[RFC/ PATCH] revert: Allow arbitrary sequencer instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-02T04:54:44Z","receivedAt":"2011-08-02T04:54:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Allow arbitrary sequencer instructions in the instruction sheet.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c |  101 +++++++++++++++++++++++++++++-------------------------\n sequencer.h      |   10 +++++\n 2 files changed, 64 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex bca1490..127b97e 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -646,49 +646,54 @@ static void read_and_refresh_cache(struct replay_opts *opts)\n  *     assert(commit_list_count(list) == 2);\n  *     return list;\n  */\n-struct commit_list **commit_list_append(struct commit *commit,\n-\t\t\t\t\tstruct commit_list **next)\n+struct replay_insn_list **replay_insn_list_append(struct replay_insn *insn,\n+\t\t\t\t\t\tstruct replay_insn_list **next)\n {\n-\tstruct commit_list *new = xmalloc(sizeof(struct commit_list));\n-\tnew->item = commit;\n+\tstruct replay_insn_list *new = xmalloc(sizeof(struct replay_insn_list));\n+\tnew->item = insn;\n \t*next = new;\n \tnew->next = NULL;\n \treturn &new->next;\n }\n \n-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n-\t\tstruct replay_opts *opts)\n+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n {\n-\tstruct commit_list *cur = NULL;\n+\tstruct replay_insn_list *cur = NULL;\n+\tstruct commit *commit;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tconst char *sha1_abbrev = NULL;\n-\tconst char *action_str = opts->action == REVERT ? \"revert\" : \"pick\";\n+\tconst char *action_str;\n \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\tcommit = cur->item->commit;\n+\t\taction_str = cur->item->action == REVERT ? \"revert\" : \"pick\";\n+\n+\t\tsha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n+\t\tif (get_message(commit, &msg))\n \t\t\treturn error(_(\"Cannot get commit message for %s\"), sha1_abbrev);\n \t\tstrbuf_addf(buf, \"%s %s %s\\n\", action_str, sha1_abbrev, msg.subject);\n \t}\n \treturn 0;\n }\n \n-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n+static struct replay_insn *parse_insn_line(char *start)\n {\n \tunsigned char commit_sha1[20];\n \tchar sha1_abbrev[40];\n+\tstruct commit *commit;\n+\tstruct replay_insn *insn;\n \tenum replay_action action;\n-\tint insn_len = 0;\n+\tint keyword_len;\n \tchar *p, *q;\n \n \tif (!prefixcmp(start, \"pick \")) {\n \t\taction = CHERRY_PICK;\n-\t\tinsn_len = strlen(\"pick\");\n-\t\tp = start + insn_len + 1;\n+\t\tkeyword_len = strlen(\"pick\");\n+\t\tp = start + keyword_len + 1;\n \t} else if (!prefixcmp(start, \"revert \")) {\n \t\taction = REVERT;\n-\t\tinsn_len = strlen(\"revert\");\n-\t\tp = start + insn_len + 1;\n+\t\tkeyword_len = strlen(\"revert\");\n+\t\tp = start + keyword_len + 1;\n \t} else\n \t\treturn NULL;\n \n@@ -699,30 +704,25 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)\n \n \tstrlcpy(sha1_abbrev, p, q - p);\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\tconst char *action_str;\n-\t\taction_str = action == REVERT ? \"revert\" : \"cherry-pick\";\n-\t\terror(_(\"Cannot %s during a %s\"), action_str, action_name(opts));\n-\t\treturn NULL;\n-\t}\n-\n \tif (get_sha1(sha1_abbrev, commit_sha1) < 0)\n \t\treturn NULL;\n \n-\treturn lookup_commit_reference(commit_sha1);\n+\tcommit = lookup_commit_reference(commit_sha1);\n+\tif (commit) {\n+\t\tinsn = xmalloc(sizeof(struct replay_insn));\n+\t\tinsn->commit = commit;\n+\t\tinsn->action = action;\n+\t\treturn insn;\n+\t}\n+\treturn NULL;\n }\n \n-static void read_populate_todo(struct commit_list **todo_list,\n-\t\t\tstruct replay_opts *opts)\n+static void read_populate_todo(struct replay_insn_list **todo_list)\n {\n \tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct commit_list **next;\n-\tstruct commit *commit;\n+\tstruct replay_insn *insn;\n+\tstruct replay_insn_list **next;\n \tchar *p;\n \tint fd;\n \n@@ -738,10 +738,10 @@ static void read_populate_todo(struct commit_list **todo_list,\n \n \tnext = todo_list;\n \tfor (p = buf.buf; *p; p = strchr(p, '\\n') + 1) {\n-\t\tcommit = parse_insn_line(p, opts);\n-\t\tif (!commit)\n+\t\tinsn = parse_insn_line(p);\n+\t\tif (!insn)\n \t\t\tgoto error;\n-\t\tnext = commit_list_append(commit, next);\n+\t\tnext = replay_insn_list_append(insn, next);\n \t}\n \tif (!*todo_list)\n \t\tgoto error;\n@@ -795,18 +795,23 @@ static void read_populate_opts(struct replay_opts **opts_ptr)\n \t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n }\n \n-static void walk_revs_populate_todo(struct commit_list **todo_list,\n+static void walk_revs_populate_todo(struct replay_insn_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 **next;\n+\tstruct replay_insn *insn;\n+\tstruct replay_insn_list **next;\n \n \tprepare_revs(&revs, opts);\n \n \tnext = todo_list;\n-\twhile ((commit = get_revision(&revs)))\n-\t\tnext = commit_list_append(commit, next);\n+\twhile ((commit = get_revision(&revs))) {\n+\t\tinsn = xmalloc(sizeof(struct replay_insn));\n+\t\tinsn->commit = commit;\n+\t\tinsn->action = opts->action;\n+\t\tnext = replay_insn_list_append(insn, next);\n+\t}\n }\n \n static int create_seq_dir(void)\n@@ -835,7 +840,7 @@ static void save_head(const char *head)\n \t\tdie(_(\"Error wrapping up %s.\"), head_file);\n }\n \n-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n+static void save_todo(struct replay_insn_list *todo_list)\n {\n \tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstatic struct lock_file todo_lock;\n@@ -843,7 +848,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \tint fd;\n \n \tfd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);\n-\tif (format_todo(&buf, todo_list, opts) < 0)\n+\tif (format_todo(&buf, todo_list) < 0)\n \t\tdie(_(\"Could not format %s.\"), todo_file);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_release(&buf);\n@@ -887,9 +892,10 @@ static void save_opts(struct replay_opts *opts)\n \t}\n }\n \n-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n+static int pick_commits(struct replay_insn_list *todo_list,\n+\t\t\tstruct replay_opts *opts)\n {\n-\tstruct commit_list *cur;\n+\tstruct replay_insn_list *cur;\n \tint res;\n \n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n@@ -899,8 +905,9 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \tread_and_refresh_cache(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+\t\tsave_todo(cur);\n+\t\topts->action = cur->item->action;\n+\t\tres = do_pick_commit(cur->item->commit, opts);\n \t\tif (res) {\n \t\t\tif (!cur->next)\n \t\t\t\t/*\n@@ -925,7 +932,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n \n static int pick_revisions(struct replay_opts *opts)\n {\n-\tstruct commit_list *todo_list = NULL;\n+\tstruct replay_insn_list *todo_list = NULL;\n \tunsigned char sha1[20];\n \n \tread_and_refresh_cache(opts);\n@@ -942,7 +949,7 @@ static int pick_revisions(struct replay_opts *opts)\n \t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n \t\t\tgoto error;\n \t\tread_populate_opts(&opts);\n-\t\tread_populate_todo(&todo_list, opts);\n+\t\tread_populate_todo(&todo_list);\n \n \t\t/* Verify that the conflict has been resolved */\n \t\tif (!index_differs_from(\"HEAD\", 0))\ndiff --git a/sequencer.h b/sequencer.h\nindex 931733c..eebee65 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -32,6 +32,16 @@ struct replay_opts {\n \tsize_t xopts_nr, xopts_alloc;\n };\n \n+struct replay_insn {\n+\tstruct commit *commit;\n+\tenum replay_action action;\n+};\n+\n+struct replay_insn_list {\n+\tstruct replay_insn *item;\n+\tstruct replay_insn_list *next;\n+};\n+\n /*\n  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring\n  * any errors.  Intended to be used by 'git reset'.\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"172630","messageId":"CALkWK0ky13=voStNnjyMXyDpk_wUb=r2XvdtNzcOO_of838rWQ@mail.gmail.com","threadId":"27989","inReplyTo":"1312260884-5087-1-git-send-email-artagnon@gmail.com","subject":"Re: [GSoC update] Iterating over a stable series","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-02T05:01:05Z","receivedAt":"2011-08-02T05:01:05Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nRamkumar Ramachandra writes:\n> Subject: [GSoC update] Iterating over a stable series\n\nEr, I just noticed that another GSoC update from me has a very similar\nsubject and it refers to something entirely different.  My apologies.\nA more appropriate subject: Building on rr/revert-cherry-pick-continue\n(or what I call sequencer-stable).\n\n-- Ram\n"},{"id":"172635","messageId":"CAP8UFD367s-0gSbmX7N3Ddakc1cZxWb_eU-+Uy5Vr0gbY8m8SA@mail.gmail.com","threadId":"27989","inReplyTo":"1312260884-5087-2-git-send-email-artagnon@gmail.com","subject":"Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2011-08-02T07:52:44Z","receivedAt":"2011-08-02T07:52:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Ram,\n\nOn Tue, Aug 2, 2011 at 6:54 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:\n>\n>        if (get_sha1(sha1_abbrev, commit_sha1) < 0)\n>                return NULL;\n>\n> -       return lookup_commit_reference(commit_sha1);\n> +       commit = lookup_commit_reference(commit_sha1);\n> +       if (commit) {\n> +               insn = xmalloc(sizeof(struct replay_insn));\n> +               insn->commit = commit;\n> +               insn->action = action;\n> +               return insn;\n> +       }\n> +       return NULL;\n>  }\n\nI'd suggest:\n\n       commit = lookup_commit_reference(commit_sha1);\n       if (!commit)\n               return NULL;\n\n       insn = xmalloc(sizeof(struct replay_insn));\n       insn->commit = commit;\n       insn->action = action;\n       return insn;\n\nThanks,\nChristian.\n"},{"id":"172695","messageId":"20110802205353.GF2743@elie","threadId":"27989","inReplyTo":"1312260884-5087-2-git-send-email-artagnon@gmail.com","subject":"Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-02T20:53:53Z","receivedAt":"2011-08-02T20:53:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Allow arbitrary sequencer instructions in the instruction sheet.\n\n\"So now I can ...\" wait, what does this allow me to do?  Your audience\nhasn't read the patch yet.\n\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -32,6 +32,16 @@ struct replay_opts {\n>  \tsize_t xopts_nr, xopts_alloc;\n>  };\n>  \n> +struct replay_insn {\n> +\tstruct commit *commit;\n> +\tenum replay_action action;\n> +};\n> +\n> +struct replay_insn_list {\n> +\tstruct replay_insn *item;\n> +\tstruct replay_insn_list *next;\n> +};\n\nAh, so this allows sequences like\n\n\trevert A\n\tpick B\n\tpick C\n\trevert D\n\nNit: why isn't the list-item struct something like\n\n\tstruct replay_insn item;\n\tstruct replay_insn_list *next;\n\nwhich would save a little memory management and memory access\noverhead (or even\n\n\tenum replay_action action;\n\tstruct commit *operand;\n\tstruct replay_insn_list *next;\n\nsince every \"struct replay_insn\" exists in the context of an\ninsn list afaict)?\n\nAnyway, the general idea seems good.\n"},{"id":"172723","messageId":"CALkWK0kfOjXeuswwOvgO5eotUE89rL-X4tTtf=frGR4WdR4iAA@mail.gmail.com","threadId":"27989","inReplyTo":"20110802205353.GF2743@elie","subject":"Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-08-03T01:32:50Z","receivedAt":"2011-08-03T01:32:50Z","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/sequencer.h\n>> +++ b/sequencer.h\n>> @@ -32,6 +32,16 @@ struct replay_opts {\n>>       size_t xopts_nr, xopts_alloc;\n>>  };\n>>\n>> +struct replay_insn {\n>> +     struct commit *commit;\n>> +     enum replay_action action;\n>> +};\n>> +\n>> +struct replay_insn_list {\n>> +     struct replay_insn *item;\n>> +     struct replay_insn_list *next;\n>> +};\n>\n> Ah, so this allows sequences like\n>\n>        revert A\n>        pick B\n>        pick C\n>        revert D\n\nExactly.\n\n> Nit: why isn't the list-item struct something like\n>\n>        struct replay_insn item;\n>        struct replay_insn_list *next;\n>\n> which would save a little memory management and memory access\n> overhead (or even\n>\n>        enum replay_action action;\n>        struct commit *operand;\n>        struct replay_insn_list *next;\n>\n> since every \"struct replay_insn\" exists in the context of an\n> insn list afaict)?\n\nRight.  Good suggestion.\n\n> Anyway, the general idea seems good.\n\nNice.  Thanks for the quick early feedback.  Much appreciated.\n\n-- Ram\n"}]}