{"thread":{"id":"27780","subject":"[RFC PATCH] An option parser for the sequencer","startedAt":"2011-07-09T15:41:57Z","lastAt":"2011-07-11T06:12:44Z","messageCount":6,"participants":["Ramkumar Ramachandra","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"171029","messageId":"1310226118-10201-1-git-send-email-artagnon@gmail.com","threadId":"27780","inReplyTo":null,"subject":"[RFC PATCH] An option parser for the sequencer","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-09T15:41:57Z","receivedAt":"2011-07-09T15:41:57Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nI've decided not to support arbitrary command-line options in the\ninstruction sheet.  A typical instruction sheet will looks like this\n(inspired heavily by the rebase -i instruction sheet format):\n\npick 3b36854 t: add tests for cloning remotes with detached HEAD\npick 61adfd3 consider only branches in guess_remote_head\npick 8537f0e submodule add: test failure when url is not configured in superproject\npick 4d68932 submodule add: allow relative repository path even when no url is set\npick f22a17e submodule add: clean up duplicated code\npick 59a5775 make copy_ref globally available\npick c1921c1 clone: always fetch remote HEAD\n\nFor persisting one set of options for every \"git cherry-pick\"/ \"git\nrevert\" invocation, I've decided to use a simple \"key = value\" format\nand put it in .git/sequencer/opts (to sit beside .git/sequencer/head\nand .git/sequencer/todo).  For strategy-option, I thought it would be\ncute to separate the various options using ' | '.  So, it'll look\nsomething like this in the end:\n\nsignoff = true\nmainline = 1\nstrategy-option = recursive | ours\n\nThe implementation is a little rough around the edges, but I'm pretty\nhappy with the overall design: it looks like a scaled-down version of\nparse-options.  Quite a lot of context is missing (where did\nsequencer.h come from?!), but I hope it's clear enough to convey the\nidea.\n\nThanks for reading.\n\nRamkumar Ramachandra (1):\n  revert: Persist per-session opts\n\n builtin/revert.c |  143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n sequencer.h      |    8 +++\n 2 files changed, 151 insertions(+), 0 deletions(-)\n\n-- \n1.7.5.GIT\n"},{"id":"171030","messageId":"1310226118-10201-2-git-send-email-artagnon@gmail.com","threadId":"27780","inReplyTo":"1310226118-10201-1-git-send-email-artagnon@gmail.com","subject":"[RFC PATCH] revert: Persist per-session opts","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-09T15:41:58Z","receivedAt":"2011-07-09T15:41:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Save the replay_opts struct in .git/sequencer/opts using a simple \"key\n= value\" format.  Parse it and populate the options structure before\nreplaying.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/revert.c |  143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n sequencer.h      |    8 +++\n 2 files changed, 151 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1feecd5..0693fd8 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -635,6 +635,32 @@ 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_opts(struct strbuf *buf, struct replay_opts *opts)\n+{\n+\tint i;\n+\n+\tif (opts->no_commit)\n+\t\tstrbuf_addstr(buf, \"no-commit = true\\n\");\n+\tif (opts->edit)\n+\t\tstrbuf_addstr(buf, \"edit = true\\n\");\n+\tif (opts->signoff)\n+\t\tstrbuf_addstr(buf, \"signoff = true\\n\");\n+\tif (opts->record_origin)\n+\t\tstrbuf_addstr(buf, \"record-origin = true\\n\");\n+\tif (opts->allow_ff)\n+\t\tstrbuf_addstr(buf, \"ff = true\\n\");\n+\tif (opts->mainline)\n+\t\tstrbuf_addf(buf, \"mainline = %d\\n\", opts->mainline);\n+\tif (opts->strategy)\n+\t\tstrbuf_addf(buf, \"strategy = %s\\n\", opts->strategy);\n+\tif (opts->xopts) {\n+\t\tstrbuf_addf(buf, \"strategy-option = \");\n+\t\tfor (i = 0; i < opts->xopts_nr - 1; i ++)\n+\t\t\tstrbuf_addf(buf, \"%s | \", opts->xopts[i]);\n+\t\tstrbuf_addf(buf, \"%s\\n\", opts->xopts[i]);\n+\t}\n+}\n+\n static void format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n@@ -733,6 +759,102 @@ error:\n \tdie(_(\"Malformed instruction sheet: %s\"), git_path(SEQ_TODO_FILE));\n }\n \n+static char *parse_opt_value(char *p, void *key, enum seq_opt_type type,\n+\t\t\tparse_opt_cb *cb_function) {\n+\tstruct option opt;\n+\tchar *val, *cur, *end;\n+\n+\tif (!(val = strchr(p, '=')))\n+\t\tgoto error;\n+\tif (!*(val + 1))\n+\t\tgoto error;\n+\tif (!(end = strchr(p, '\\n')))\n+\t\tgoto error;\n+\tval += 2;\n+\t*end = '\\0'; /* Remove trailing '\\n' */\n+\n+\tswitch (type) {\n+\tcase SEQ_OPTION_BOOLEAN:\n+\t\tif (!strncmp(val, \"true\", strlen(\"true\")))\n+\t\t\t*(int *)key = 1;\n+\t\telse if (!strncmp(val, \"false\", strlen(\"false\")))\n+\t\t\t*(int *)key = 0;\n+\t\telse\n+\t\t\tgoto error;\n+\t\tbreak;\n+\tcase SEQ_OPTION_INTEGER:\n+\t\t*(int *)key = strtol(val, NULL, 10);\n+\t\tbreak;\n+\tcase SEQ_OPTION_STRING:\n+\t\t*(char **)key = xstrdup(val);\n+\t\tbreak;\n+\tcase SEQ_OPTION_CALLBACK:\n+\t\topt.value = (struct replay_opts **)key;\n+\t\twhile (val) {\n+\t\t\tif ((cur = strchr(val, '|'))) {\n+\t\t\t\t*(cur - 1) = '\\0';\n+\t\t\t\t(*cb_function)(&opt, val, 0);\n+\t\t\t\tval = cur + 2;\n+\t\t\t} else {\n+\t\t\t\t(*cb_function)(&opt, val, 0);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(_(\"program error\"));\n+\t}\n+\treturn end + 1;\n+error:\n+\tdie(_(\"Malformed options sheet: %s\"), git_path(SEQ_OPTS_FILE));\n+}\n+\n+static void read_populate_opts(struct replay_opts **opts_ptr)\n+{\n+\tstruct replay_opts *opts = *opts_ptr;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tchar *p;\n+\tint fd;\n+\n+\tfd = open(git_path(SEQ_OPTS_FILE), O_RDONLY);\n+\tif (fd < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie_errno(_(\"Could not open %s.\"), git_path(SEQ_OPTS_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_OPTS_FILE));\n+\t}\n+\tclose(fd);\n+\n+\tfor (p = buf.buf; *p;) {\n+\t\tif (!strncmp(p, \"no-commit \", strlen(\"no-commit \")))\n+\t\t\tp = parse_opt_value(p, &opts->no_commit, SEQ_OPTION_BOOLEAN, NULL);\n+\t\telse if (!strncmp(p, \"edit \", strlen(\"edit \")))\n+\t\t\tp = parse_opt_value(p, &opts->edit, SEQ_OPTION_BOOLEAN, NULL);\n+\t\telse if (!strncmp(p, \"signoff \", strlen(\"signoff \")))\n+\t\t\tp = parse_opt_value(p, &opts->signoff, SEQ_OPTION_BOOLEAN, NULL);\n+\t\telse if (!strncmp(p, \"mainline \", strlen(\"mainline \")))\n+\t\t\tp = parse_opt_value(p, &opts->mainline, SEQ_OPTION_INTEGER, NULL);\n+\t\telse if (!strncmp(p, \"record-origin \", strlen(\"record-origin \")))\n+\t\t\tp = parse_opt_value(p, &opts->record_origin, SEQ_OPTION_BOOLEAN, NULL);\n+\t\telse if (!strncmp(p, \"ff \", strlen(\"ff \")))\n+\t\t\tp = parse_opt_value(p, &opts->allow_ff, SEQ_OPTION_BOOLEAN, NULL);\n+\t\telse if (!strncmp(p, \"strategy \", strlen(\"strategy \")))\n+\t\t\tp = parse_opt_value(p, &opts->strategy, SEQ_OPTION_STRING, NULL);\n+\t\telse if (!strncmp(p, \"strategy-option \", strlen(\"strategy-option \")))\n+\t\t\tp = parse_opt_value(p, &opts, SEQ_OPTION_CALLBACK, option_parse_x);\n+\t\telse\n+\t\t\tgoto error;\n+\t}\n+\tstrbuf_release(&buf);\n+\treturn;\n+error:\n+\tstrbuf_release(&buf);\n+\tdie(_(\"Malformed options sheet: %s\"), git_path(SEQ_OPTS_FILE));\n+}\n+\n static void walk_revs_populate_todo(struct commit_list **todo_list,\n \t\t\t\tstruct replay_opts *opts)\n {\n@@ -800,6 +922,25 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \tstrbuf_release(&buf);\n }\n \n+static void save_opts(struct replay_opts *opts)\n+{\n+\tstatic struct lock_file opts_lock;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tfd = hold_lock_file_for_update(&opts_lock, git_path(SEQ_OPTS_FILE), LOCK_DIE_ON_ERROR);\n+\tformat_opts(&buf, 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_OPTS_FILE));\n+\t}\n+\tif (commit_lock_file(&opts_lock) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Error wrapping up %s\"), git_path(SEQ_OPTS_FILE));\n+\t}\n+\tstrbuf_release(&buf);\n+}\n+\n static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)\n {\n \tstruct commit_list *cur;\n@@ -849,6 +990,7 @@ static int process_continuation(struct replay_opts *opts)\n \t} else if (opts->subcommand == REPLAY_CONTINUE) {\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 \n \t\t/* Verify that the conflict has been resolved */\n@@ -871,6 +1013,7 @@ static int process_continuation(struct replay_opts *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_opts(opts);\n \t\tsave_todo(todo_list, opts);\n \t}\n \treturn pick_commits(todo_list, opts);\ndiff --git a/sequencer.h b/sequencer.h\nindex d6fe6e0..e7bef5d 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -5,6 +5,14 @@\n #define SEQ_OLD_DIR\t\"sequencer-old\"\n #define SEQ_HEAD_FILE\t\"sequencer/head\"\n #define SEQ_TODO_FILE\t\"sequencer/todo\"\n+#define SEQ_OPTS_FILE\t\"sequencer/opts\"\n+\n+enum seq_opt_type {\n+\tSEQ_OPTION_BOOLEAN,\n+\tSEQ_OPTION_INTEGER,\n+\tSEQ_OPTION_STRING,\n+\tSEQ_OPTION_CALLBACK,\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 --hard'.\n-- \n1.7.5.GIT\n"},{"id":"171034","messageId":"201107101002.06385.chriscool@tuxfamily.org","threadId":"27780","inReplyTo":"1310226118-10201-2-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] revert: Persist per-session opts","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2011-07-10T08:02:06Z","receivedAt":"2011-07-10T08:02:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Ram,\n\nOn Saturday 09 July 2011 17:41:58 Ramkumar Ramachandra wrote:\n> Save the replay_opts struct in .git/sequencer/opts using a simple \"key\n> = value\" format.  Parse it and populate the options structure before\n> replaying.\n\n[...]\n\n>  static void format_todo(struct strbuf *buf, struct commit_list *todo_list,\n>  \t\t\tstruct replay_opts *opts)\n>  {\n> @@ -733,6 +759,102 @@ error:\n>  \tdie(_(\"Malformed instruction sheet: %s\"), git_path(SEQ_TODO_FILE));\n>  }\n> \n> +static char *parse_opt_value(char *p, void *key, enum seq_opt_type type,\n> +\t\t\tparse_opt_cb *cb_function) {\n> +\tstruct option opt;\n> +\tchar *val, *cur, *end;\n> +\n> +\tif (!(val = strchr(p, '=')))\n> +\t\tgoto error;\n> +\tif (!*(val + 1))\n> +\t\tgoto error;\n> +\tif (!(end = strchr(p, '\\n')))\n> +\t\tgoto error;\n> +\tval += 2;\n\nIt looks like you rely on all lines ending with \\n and having a space after \nthe '='. It may be a little bit too fragile.\n\n> +\t*end = '\\0'; /* Remove trailing '\\n' */\n> +\n> +\tswitch (type) {\n> +\tcase SEQ_OPTION_BOOLEAN:\n> +\t\tif (!strncmp(val, \"true\", strlen(\"true\")))\n> +\t\t\t*(int *)key = 1;\n> +\t\telse if (!strncmp(val, \"false\", strlen(\"false\")))\n> +\t\t\t*(int *)key = 0;\n> +\t\telse\n> +\t\t\tgoto error;\n> +\t\tbreak;\n> +\tcase SEQ_OPTION_INTEGER:\n> +\t\t*(int *)key = strtol(val, NULL, 10);\n> +\t\tbreak;\n> +\tcase SEQ_OPTION_STRING:\n> +\t\t*(char **)key = xstrdup(val);\n> +\t\tbreak;\n> +\tcase SEQ_OPTION_CALLBACK:\n> +\t\topt.value = (struct replay_opts **)key;\n> +\t\twhile (val) {\n> +\t\t\tif ((cur = strchr(val, '|'))) {\n> +\t\t\t\t*(cur - 1) = '\\0';\n> +\t\t\t\t(*cb_function)(&opt, val, 0);\n> +\t\t\t\tval = cur + 2;\n> +\t\t\t} else {\n> +\t\t\t\t(*cb_function)(&opt, val, 0);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tdie(_(\"program error\"));\n> +\t}\n> +\treturn end + 1;\n> +error:\n> +\tdie(_(\"Malformed options sheet: %s\"), git_path(SEQ_OPTS_FILE));\n> +}\n\nOtherwise it looks good to me.\n\nThanks,\nChristian.\n"},{"id":"171035","messageId":"201107101015.29762.chriscool@tuxfamily.org","threadId":"27780","inReplyTo":"1310226118-10201-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] An option parser for the sequencer","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2011-07-10T08:15:29Z","receivedAt":"2011-07-10T08:15:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Ram,\n\nOn Saturday 09 July 2011 17:41:57 Ramkumar Ramachandra wrote:\n> Hi,\n> \n> I've decided not to support arbitrary command-line options in the\n> instruction sheet.\n\nIt may be a good decision, but could you explain why? You could say for \nexample that the series would be already an improvement over what we have if \nwe don't parse arbitrary command line options.\n\n> A typical instruction sheet will looks like this\n> (inspired heavily by the rebase -i instruction sheet format):\n\n> pick 3b36854 t: add tests for cloning remotes with detached HEAD\n> pick 61adfd3 consider only branches in guess_remote_head\n> pick 8537f0e submodule add: test failure when url is not configured in\n> superproject pick 4d68932 submodule add: allow relative repository path\n> even when no url is set pick f22a17e submodule add: clean up duplicated\n> code\n> pick 59a5775 make copy_ref globally available\n> pick c1921c1 clone: always fetch remote HEAD\n\nWould it be easy to change the format to support arbitrary command line \noptions if we want to do it afterwards, especially after the end of the GSoC?\n\n> For persisting one set of options for every \"git cherry-pick\"/ \"git\n> revert\" invocation, I've decided to use a simple \"key = value\" format\n> and put it in .git/sequencer/opts (to sit beside .git/sequencer/head\n> and .git/sequencer/todo).  For strategy-option, I thought it would be\n> cute to separate the various options using ' | '.  So, it'll look\n> something like this in the end:\n> \n> signoff = true\n> mainline = 1\n> strategy-option = recursive | ours\n\nIs it the same format as the .git/config file format? Would it be possible to \nreuse some config parsing/writing code?\n\n> The implementation is a little rough around the edges, but I'm pretty\n> happy with the overall design: it looks like a scaled-down version of\n> parse-options.  Quite a lot of context is missing (where did\n> sequencer.h come from?!), but I hope it's clear enough to convey the\n> idea.\n> \n> Thanks for reading.\n> \n> Ramkumar Ramachandra (1):\n>   revert: Persist per-session opts\n> \n>  builtin/revert.c |  143\n> ++++++++++++++++++++++++++++++++++++++++++++++++++++++ sequencer.h      | \n>   8 +++\n>  2 files changed, 151 insertions(+), 0 deletions(-)\n\nThe subject should have been \"[RFC PATCH 0/1] ...\" instead of \"[RFC PATCH] \n...\" because this is just a patch series header.\n\nThanks,\nChristian.\n"},{"id":"171043","messageId":"CALkWK0nqiCQ5VMh7RXOJ7Z8tR6x-OJ4wWvNgUKcFuyBGY9Pj4w@mail.gmail.com","threadId":"27780","inReplyTo":"201107101015.29762.chriscool@tuxfamily.org","subject":"Re: [RFC PATCH] An option parser for the sequencer","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-11T06:11:50Z","receivedAt":"2011-07-11T06:11:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nChristian Couder writes:\n> On Saturday 09 July 2011 17:41:57 Ramkumar Ramachandra wrote:\n>> I've decided not to support arbitrary command-line options in the\n>> instruction sheet.\n>\n> It may be a good decision, but could you explain why? You could say for\n> example that the series would be already an improvement over what we have if\n> we don't parse arbitrary command line options.\n\nHere's an elaborate justification:\n1. Parsing arbitrary command-line options means that we'd have to\nsupport two versions of every command: the short version and the long\nversion.  As a result, two configuration files may be functionally\nidentical, but not really identical because one uses short versions\nand the other uses long versions.   This is ugly/ inelegant.\n2. The format can be quite loose: for example \"--ff -s\" is equivalent\nto \"---ff   '-s'\".  We'll either have to go through the trouble to\nsanitize everything at the time of persistence, or go through that\ntrouble while reading.  We can't entirely avoid this ugliness.\n3. Where do we stop? Is something like \"for i in \"moo foo\"; do $i;\ndone;\" allowed?  How do we draw clear boundaries?\n4. Most importantly, we're NOT building a fast git shell.  There's no\npoint implementing a reduced shell parser inside Git -- there are many\nexisting shells we can leverage, if that's what we want.\n\nBesides, I can't think of a good enough usecase for per-instruction\ncommand-line options right now.  How often would someone want to\ncherry-pick 10 commits, but signoff on 3 of them somewhere in the\nmiddle?  Not to say that we should not support per-instruction\ncommand-line options: just that we can wait until someone figures out\na great usecase + a good implementation.  It's not critical.\n\nLet's go back to the problem at hand -- what are we trying to do?  The\nproblem is to get cherry-pick/ revert to support '--continue', and\n'--reset' as soon as possible and get it merged.  So, all we need is\none set of options to persist along with the head and todo.  A simple\n\"key = value\" format is both simple and elegant; so we can go ahead\nwith it, and see how things play out later.  Since none of this is\nexposed to the end-user, I don't see us locking ourselves into\nshort-sighted design at any rate.\n\n>> A typical instruction sheet will looks like this\n>> (inspired heavily by the rebase -i instruction sheet format):\n>\n>> pick 3b36854 t: add tests for cloning remotes with detached HEAD\n>> pick 61adfd3 consider only branches in guess_remote_head\n>> pick 8537f0e submodule add: test failure when url is not configured in\n>> superproject pick 4d68932 submodule add: allow relative repository path\n>> even when no url is set pick f22a17e submodule add: clean up duplicated\n>> code\n>> pick 59a5775 make copy_ref globally available\n>> pick c1921c1 clone: always fetch remote HEAD\n>\n> Would it be easy to change the format to support arbitrary command line\n> options if we want to do it afterwards, especially after the end of the GSoC?\n\nWhy not?  We aren't explicitly showing this to the end-user yet, so I\nsee no problem with changing it.  We should worry about that only when\nwe develop a feature like \"cherry-pick -i\".\n\n>> For persisting one set of options for every \"git cherry-pick\"/ \"git\n>> revert\" invocation, I've decided to use a simple \"key = value\" format\n>> and put it in .git/sequencer/opts (to sit beside .git/sequencer/head\n>> and .git/sequencer/todo).  For strategy-option, I thought it would be\n>> cute to separate the various options using ' | '.  So, it'll look\n>> something like this in the end:\n>>\n>> signoff = true\n>> mainline = 1\n>> strategy-option = recursive | ours\n>\n> Is it the same format as the .git/config file format? Would it be possible to\n> reuse some config parsing/writing code?\n\nYes; I'm currently working on it.  I just wanted to show a simple PoC,\nbut I will try to avoid introducing new parsers and the bugs that come\nalong with it.\n\nThanks.\n\n-- Ram\n"},{"id":"171044","messageId":"CALkWK0ndSWkPudM_sJnyUyh93t9JLk-t-YFQb7=WO2H_svnGdQ@mail.gmail.com","threadId":"27780","inReplyTo":"201107101002.06385.chriscool@tuxfamily.org","subject":"Re: [RFC PATCH] revert: Persist per-session opts","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-07-11T06:12:44Z","receivedAt":"2011-07-11T06:12:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Christian,\n\nChristian Couder writes:\n> On Saturday 09 July 2011 17:41:58 Ramkumar Ramachandra wrote:\n>> Save the replay_opts struct in .git/sequencer/opts using a simple \"key\n>> = value\" format.  Parse it and populate the options structure before\n>> replaying.\n>\n> [...]\n>\n>>  static void format_todo(struct strbuf *buf, struct commit_list *todo_list,\n>>                       struct replay_opts *opts)\n>>  {\n>> @@ -733,6 +759,102 @@ error:\n>>       die(_(\"Malformed instruction sheet: %s\"), git_path(SEQ_TODO_FILE));\n>>  }\n>>\n>> +static char *parse_opt_value(char *p, void *key, enum seq_opt_type type,\n>> +                     parse_opt_cb *cb_function) {\n>> +     struct option opt;\n>> +     char *val, *cur, *end;\n>> +\n>> +     if (!(val = strchr(p, '=')))\n>> +             goto error;\n>> +     if (!*(val + 1))\n>> +             goto error;\n>> +     if (!(end = strchr(p, '\\n')))\n>> +             goto error;\n>> +     val += 2;\n>\n> It looks like you rely on all lines ending with \\n and having a space after\n> the '='. It may be a little bit too fragile.\n\nRight.  Thanks for the review -- will try to reuse bits of the\ngit-config parser.\n\n-- Ram\n"}]}