{"thread":{"id":"47372","subject":"[PATCH v2 1/4] git-compat-util: introduce skip_to_optional_val()","startedAt":"2017-12-04T12:57:22Z","lastAt":"2017-12-04T14:31:14Z","messageCount":7,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"334040","messageId":"20171204125617.4931-1-chriscool@tuxfamily.org","threadId":"47372","inReplyTo":null,"subject":"[PATCH v2 1/4] git-compat-util: introduce skip_to_optional_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-04T12:56:14Z","receivedAt":"2017-12-04T12:57:22Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nWe often accept both a \"--key\" option and a \"--key=<val>\" option.\n\nThese options currently are parsed using something like:\n\nif (!strcmp(arg, \"--key\")) {\n\t/* do something */\n} else if (skip_prefix(arg, \"--key=\", &arg)) {\n\t/* do something with arg */\n}\n\nwhich is a bit cumbersome compared to just:\n\nif (skip_to_optional_val(arg, \"--key\", &arg)) {\n\t/* do something with arg */\n}\n\nThis also introduces skip_to_optional_val_default() for the few\ncases where something different should be done when the first\nargument is exactly \"--key\" than when it is exactly \"--key=\".\n\nIn general it is better for UI consistency and simplicity if\n\"--key\" and \"--key=\" do the same thing though, so that using\nskip_to_optional_val() should be encouraged compared to\nskip_to_optional_val_default().\n\nNote that these functions can be used to parse any \"key=value\"\nstring where \"key\" is also considered as valid, not just\ncommand line options.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n git-compat-util.h | 23 +++++++++++++++++++++++\n strbuf.c          | 20 ++++++++++++++++++++\n 2 files changed, 43 insertions(+)\n\nAfter thinking about it a bit more and taking a look at the\ncurrent code, I thought that it was probably best to have\nboth skip_to_optional_val() and skip_to_optional_val_default().\n\nThe changes compared to previous version are:\n\n  - 2 new functions instead of 1\n  - \"optional\" instead of \"opt\" in the function names\n  - the big function is not inlined\n  - more code in diff.c is simplified using the functions \n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cedad4d581..2858d66f85 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -484,6 +484,29 @@ static inline int skip_prefix(const char *str, const char *prefix,\n \treturn 0;\n }\n \n+/*\n+ * If the string \"str\" is the same as the string in \"prefix\", then the \"val\"\n+ * parameter is set to the \"def\" parameter and 1 is returned.\n+ * If the string \"str\" begins with the string found in \"prefix\" and then a\n+ * \"=\" sign, then the \"val\" parameter is set to \"str + strlen(prefix) + 1\"\n+ * (i.e., to the point in the string right after the prefix and the \"=\" sign),\n+ * and 1 is returned.\n+ *\n+ * Otherwise, return 0 and leave \"val\" untouched.\n+ *\n+ * When we accept both a \"--key\" and a \"--key=<val>\" option, this function\n+ * can be used instead of !strcmp(arg, \"--key\") and then\n+ * skip_prefix(arg, \"--key=\", &arg) to parse such an option.\n+ */\n+int skip_to_optional_val_default(const char *str, const char *prefix,\n+\t\t\t\t const char **val, const char *def);\n+\n+static inline int skip_to_optional_val(const char *str, const char *prefix,\n+\t\t\t\t       const char **val)\n+{\n+\treturn skip_to_optional_val_default(str, prefix, val, \"\");\n+}\n+\n /*\n  * Like skip_prefix, but promises never to read past \"len\" bytes of the input\n  * buffer, and returns the remaining number of bytes in \"out\" via \"outlen\".\ndiff --git a/strbuf.c b/strbuf.c\nindex 323c49ceb3..3430499f9e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -11,6 +11,26 @@ int starts_with(const char *str, const char *prefix)\n \t\t\treturn 0;\n }\n \n+int skip_to_optional_val_default(const char *str, const char *prefix,\n+\t\t\t\t const char **val, const char *def)\n+{\n+\tconst char *p;\n+\n+\tif (!skip_prefix(str, prefix, &p))\n+\t\treturn 0;\n+\n+\tif (!*p) {\n+\t\t*val = def;\n+\t\treturn 1;\n+\t}\n+\n+\tif (*p != '=')\n+\t\treturn 0;\n+\n+\t*val = p + 1;\n+\treturn 1;\n+}\n+\n /*\n  * Used as the default ->buf value, so that people can always assume\n  * buf is non NULL and ->buf is NUL terminated even for a freshly\n-- \n2.15.1.274.g3f22e311ce.dirty\n\n"},{"id":"334041","messageId":"20171204125617.4931-2-chriscool@tuxfamily.org","threadId":"47372","inReplyTo":"20171204125617.4931-1-chriscool@tuxfamily.org","subject":"[PATCH v2 2/4] index-pack: use skip_to_optional_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-04T12:56:15Z","receivedAt":"2017-12-04T12:57:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nLet's simplify index-pack option parsing using\nskip_to_optional_val().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/index-pack.c | 11 +++--------\n 1 file changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8ec459f522..20b4b85a6a 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1660,10 +1660,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tfrom_stdin = 1;\n \t\t\t} else if (!strcmp(arg, \"--fix-thin\")) {\n \t\t\t\tfix_thin_pack = 1;\n-\t\t\t} else if (!strcmp(arg, \"--strict\")) {\n-\t\t\t\tstrict = 1;\n-\t\t\t\tdo_fsck_object = 1;\n-\t\t\t} else if (skip_prefix(arg, \"--strict=\", &arg)) {\n+\t\t\t} else if (skip_to_optional_val(arg, \"--strict\", &arg)) {\n \t\t\t\tstrict = 1;\n \t\t\t\tdo_fsck_object = 1;\n \t\t\t\tfsck_set_msg_types(&fsck_options, arg);\n@@ -1679,10 +1676,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tverify = 1;\n \t\t\t\tshow_stat = 1;\n \t\t\t\tstat_only = 1;\n-\t\t\t} else if (!strcmp(arg, \"--keep\")) {\n-\t\t\t\tkeep_msg = \"\";\n-\t\t\t} else if (starts_with(arg, \"--keep=\")) {\n-\t\t\t\tkeep_msg = arg + 7;\n+\t\t\t} else if (skip_to_optional_val(arg, \"--keep\", &keep_msg)) {\n+\t\t\t\t; /* nothing to do */\n \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n \t\t\t\tchar *end;\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n-- \n2.15.1.274.g3f22e311ce.dirty\n\n"},{"id":"334042","messageId":"20171204125617.4931-3-chriscool@tuxfamily.org","threadId":"47372","inReplyTo":"20171204125617.4931-1-chriscool@tuxfamily.org","subject":"[PATCH v2 3/4] diff: use skip_to_optional_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-04T12:56:16Z","receivedAt":"2017-12-04T12:57:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nLet's simplify diff option parsing using\nskip_to_optional_val().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n diff.c | 29 ++++++++++-------------------\n 1 file changed, 10 insertions(+), 19 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2ebe2227b4..83509f0658 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4508,17 +4508,12 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->output_format |= DIFF_FORMAT_NUMSTAT;\n \telse if (!strcmp(arg, \"--shortstat\"))\n \t\toptions->output_format |= DIFF_FORMAT_SHORTSTAT;\n-\telse if (!strcmp(arg, \"-X\") || !strcmp(arg, \"--dirstat\"))\n-\t\treturn parse_dirstat_opt(options, \"\");\n-\telse if (skip_prefix(arg, \"-X\", &arg))\n-\t\treturn parse_dirstat_opt(options, arg);\n-\telse if (skip_prefix(arg, \"--dirstat=\", &arg))\n+\telse if (skip_prefix(arg, \"-X\", &arg) ||\n+\t\t skip_to_optional_val(arg, \"--dirstat\", &arg))\n \t\treturn parse_dirstat_opt(options, arg);\n \telse if (!strcmp(arg, \"--cumulative\"))\n \t\treturn parse_dirstat_opt(options, \"cumulative\");\n-\telse if (!strcmp(arg, \"--dirstat-by-file\"))\n-\t\treturn parse_dirstat_opt(options, \"files\");\n-\telse if (skip_prefix(arg, \"--dirstat-by-file=\", &arg)) {\n+\telse if (skip_to_optional_val(arg, \"--dirstat-by-file\", &arg)) {\n \t\tparse_dirstat_opt(options, \"files\");\n \t\treturn parse_dirstat_opt(options, arg);\n \t}\n@@ -4540,13 +4535,13 @@ int diff_opt_parse(struct diff_options *options,\n \t\treturn stat_opt(options, av);\n \n \t/* renames options */\n-\telse if (starts_with(arg, \"-B\") || starts_with(arg, \"--break-rewrites=\") ||\n-\t\t !strcmp(arg, \"--break-rewrites\")) {\n+\telse if (starts_with(arg, \"-B\") ||\n+\t\t skip_to_optional_val(arg, \"--break-rewrites\", &optarg)) {\n \t\tif ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n \t\t\treturn error(\"invalid argument to -B: %s\", arg+2);\n \t}\n-\telse if (starts_with(arg, \"-M\") || starts_with(arg, \"--find-renames=\") ||\n-\t\t !strcmp(arg, \"--find-renames\")) {\n+\telse if (starts_with(arg, \"-M\") ||\n+\t\t skip_to_optional_val(arg, \"--find-renames\", &optarg)) {\n \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n \t\t\treturn error(\"invalid argument to -M: %s\", arg+2);\n \t\toptions->detect_rename = DIFF_DETECT_RENAME;\n@@ -4554,8 +4549,8 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"-D\") || !strcmp(arg, \"--irreversible-delete\")) {\n \t\toptions->irreversible_delete = 1;\n \t}\n-\telse if (starts_with(arg, \"-C\") || starts_with(arg, \"--find-copies=\") ||\n-\t\t !strcmp(arg, \"--find-copies\")) {\n+\telse if (starts_with(arg, \"-C\") ||\n+\t\t skip_to_optional_val(arg, \"--find-copies\", &optarg)) {\n \t\tif (options->detect_rename == DIFF_DETECT_COPY)\n \t\t\toptions->flags.find_copies_harder = 1;\n \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n@@ -4568,12 +4563,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->flags.rename_empty = 1;\n \telse if (!strcmp(arg, \"--no-rename-empty\"))\n \t\toptions->flags.rename_empty = 0;\n-\telse if (!strcmp(arg, \"--relative\"))\n-\t\toptions->flags.relative_name = 1;\n-\telse if (skip_prefix(arg, \"--relative=\", &arg)) {\n+\telse if (skip_to_optional_val(arg, \"--relative\", &options->prefix))\n \t\toptions->flags.relative_name = 1;\n-\t\toptions->prefix = arg;\n-\t}\n \n \t/* xdiff options */\n \telse if (!strcmp(arg, \"--minimal\"))\n-- \n2.15.1.274.g3f22e311ce.dirty\n\n"},{"id":"334043","messageId":"20171204125617.4931-4-chriscool@tuxfamily.org","threadId":"47372","inReplyTo":"20171204125617.4931-1-chriscool@tuxfamily.org","subject":"[PATCH v2 4/4] diff: use skip_to_optional_val_default()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-04T12:56:17Z","receivedAt":"2017-12-04T12:57:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Let's simplify diff option parsing using\nskip_to_optional_val_default().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n diff.c | 20 ++++----------------\n 1 file changed, 4 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 83509f0658..d234b4f0cb 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4619,9 +4619,7 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--no-follow\")) {\n \t\toptions->flags.follow_renames = 0;\n \t\toptions->flags.default_follow_renames = 0;\n-\t} else if (!strcmp(arg, \"--color\"))\n-\t\toptions->use_color = 1;\n-\telse if (skip_prefix(arg, \"--color=\", &arg)) {\n+\t} else if (skip_to_optional_val_default(arg, \"--color\", &arg, \"always\")) {\n \t\tint value = git_config_colorbool(NULL, arg);\n \t\tif (value < 0)\n \t\t\treturn error(\"option `color' expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\");\n@@ -4641,14 +4639,9 @@ int diff_opt_parse(struct diff_options *options,\n \t\tif (cm < 0)\n \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n \t\toptions->color_moved = cm;\n-\t} else if (!strcmp(arg, \"--color-words\")) {\n-\t\toptions->use_color = 1;\n-\t\toptions->word_diff = DIFF_WORDS_COLOR;\n-\t}\n-\telse if (skip_prefix(arg, \"--color-words=\", &arg)) {\n+\t} else if (skip_to_optional_val_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n-\t\toptions->word_regex = arg;\n \t}\n \telse if (!strcmp(arg, \"--word-diff\")) {\n \t\tif (options->word_diff == DIFF_WORDS_NONE)\n@@ -4687,15 +4680,10 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->flags.textconv_set_via_cmdline = 1;\n \t} else if (!strcmp(arg, \"--no-textconv\"))\n \t\toptions->flags.allow_textconv = 0;\n-\telse if (!strcmp(arg, \"--ignore-submodules\")) {\n-\t\toptions->flags.override_submodule_config = 1;\n-\t\thandle_ignore_submodules_arg(options, \"all\");\n-\t} else if (skip_prefix(arg, \"--ignore-submodules=\", &arg)) {\n+\telse if (skip_to_optional_val_default(arg, \"--ignore-submodules\", &arg, \"all\")) {\n \t\toptions->flags.override_submodule_config = 1;\n \t\thandle_ignore_submodules_arg(options, arg);\n-\t} else if (!strcmp(arg, \"--submodule\"))\n-\t\toptions->submodule_format = DIFF_SUBMODULE_LOG;\n-\telse if (skip_prefix(arg, \"--submodule=\", &arg))\n+\t} else if (skip_to_optional_val_default(arg, \"--submodule\", &arg, \"log\"))\n \t\treturn parse_submodule_opt(options, arg);\n \telse if (skip_prefix(arg, \"--ws-error-highlight=\", &arg))\n \t\treturn parse_ws_error_highlight_opt(options, arg);\n-- \n2.15.1.274.g3f22e311ce.dirty\n\n"},{"id":"334049","messageId":"xmqqh8t6o9me.fsf@gitster.mtv.corp.google.com","threadId":"47372","inReplyTo":"20171204125617.4931-3-chriscool@tuxfamily.org","subject":"Re: [PATCH v2 3/4] diff: use skip_to_optional_val()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-04T14:23:05Z","receivedAt":"2017-12-04T14:23:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> @@ -4540,13 +4535,13 @@ int diff_opt_parse(struct diff_options *options,\n>  \t\treturn stat_opt(options, av);\n>  \n>  \t/* renames options */\n> -\telse if (starts_with(arg, \"-B\") || starts_with(arg, \"--break-rewrites=\") ||\n> -\t\t !strcmp(arg, \"--break-rewrites\")) {\n> +\telse if (starts_with(arg, \"-B\") ||\n> +\t\t skip_to_optional_val(arg, \"--break-rewrites\", &optarg)) {\n>  \t\tif ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n>  \t\t\treturn error(\"invalid argument to -B: %s\", arg+2);\n>  \t}\n\nThis is curious; \"optarg\" gets something, but it is not used (what\nis passed to scoreopt_parse() is still \"arg\".\n\nIt merely is curious and not wrong; the actual parsing of the whole\nthing is done in scoreopt_parse() and skip_to_optional_val() is used\nmerely as a substitute for \"the thing must either be --break-rewrites\nor must begin with --break-rewrites=\" check that is done with\nstarts_with() and !strcmp().\n\nIt probably makes sense to allow skip_to_optional() to take a NULL\nfor the third parameter to clarify a callsite like this.  Otherwise\nthe readers will wonder who consumes optarg, and why it is OK for it\nto be sometimes set and sometimes left undefined.\n\n> -\telse if (starts_with(arg, \"-M\") || starts_with(arg, \"--find-renames=\") ||\n> -\t\t !strcmp(arg, \"--find-renames\")) {\n> +\telse if (starts_with(arg, \"-M\") ||\n> +\t\t skip_to_optional_val(arg, \"--find-renames\", &optarg)) {\n\nLikewise.\n\n> @@ -4554,8 +4549,8 @@ int diff_opt_parse(struct diff_options *options,\n>  \telse if (!strcmp(arg, \"-D\") || !strcmp(arg, \"--irreversible-delete\")) {\n>  \t\toptions->irreversible_delete = 1;\n>  \t}\n> -\telse if (starts_with(arg, \"-C\") || starts_with(arg, \"--find-copies=\") ||\n> -\t\t !strcmp(arg, \"--find-copies\")) {\n> +\telse if (starts_with(arg, \"-C\") ||\n> +\t\t skip_to_optional_val(arg, \"--find-copies\", &optarg)) {\n>  \t\tif (options->detect_rename == DIFF_DETECT_COPY)\n>  \t\t\toptions->flags.find_copies_harder = 1;\n>  \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n\nLikewise.\n"},{"id":"334050","messageId":"xmqqd13uo9d1.fsf@gitster.mtv.corp.google.com","threadId":"47372","inReplyTo":"20171204125617.4931-4-chriscool@tuxfamily.org","subject":"Re: [PATCH v2 4/4] diff: use skip_to_optional_val_default()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-04T14:28:42Z","receivedAt":"2017-12-04T14:28:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> -\t} else if (!strcmp(arg, \"--submodule\"))\n> -\t\toptions->submodule_format = DIFF_SUBMODULE_LOG;\n> -\telse if (skip_prefix(arg, \"--submodule=\", &arg))\n> +\t} else if (skip_to_optional_val_default(arg, \"--submodule\", &arg, \"log\"))\n>  \t\treturn parse_submodule_opt(options, arg);\n\nIt may be annoying if we later diagnose that DIFF_SUBMODULE_LOG may\nnot be used in the context (e.g. together with some other optoins\nthat are set in \"options\" variable) and have to throw an error\nmessage at the user.  parse_submodule_opt() would not know if that\nstring \"log\" came from the user or substituted by the helper, and in\nthe latter case, \"don't use --submodule=log here\" is a message that\nis not quite appropriate.\n\nThis may be minor enough not matter in practice, but I dunno.  I\ndidn't look at other hunks in this step very carefully but I would\nexpect that whenever you use \"default\" to substitute, you'll have\nthe same information lossage.\n\n"},{"id":"334051","messageId":"xmqq8teio991.fsf@gitster.mtv.corp.google.com","threadId":"47372","inReplyTo":"20171204125617.4931-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2 1/4] git-compat-util: introduce skip_to_optional_val()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-04T14:31:06Z","receivedAt":"2017-12-04T14:31:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> From: Christian Couder <christian.couder@gmail.com>\n>\n> We often accept both a \"--key\" option and a \"--key=<val>\" option.\n>\n> These options currently are parsed using something like:\n>\n> if (!strcmp(arg, \"--key\")) {\n> \t/* do something */\n> } else if (skip_prefix(arg, \"--key=\", &arg)) {\n> \t/* do something with arg */\n> }\n>\n> which is a bit cumbersome compared to just:\n>\n> if (skip_to_optional_val(arg, \"--key\", &arg)) {\n> \t/* do something with arg */\n> }\n>\n> This also introduces skip_to_optional_val_default() for the few\n> cases where something different should be done when the first\n> argument is exactly \"--key\" than when it is exactly \"--key=\".\n>\n> In general it is better for UI consistency and simplicity if\n> \"--key\" and \"--key=\" do the same thing though, so that using\n> skip_to_optional_val() should be encouraged compared to\n> skip_to_optional_val_default().\n>\n> Note that these functions can be used to parse any \"key=value\"\n> string where \"key\" is also considered as valid, not just\n> command line options.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  git-compat-util.h | 23 +++++++++++++++++++++++\n>  strbuf.c          | 20 ++++++++++++++++++++\n>  2 files changed, 43 insertions(+)\n>\n> After thinking about it a bit more and taking a look at the\n> current code, I thought that it was probably best to have\n> both skip_to_optional_val() and skip_to_optional_val_default().\n\nI guess we came to the same conclusion independently while our mails\ncrossed ;-)\n\n>   - 2 new functions instead of 1\n>   - \"optional\" instead of \"opt\" in the function names\n\nI thought that the more important part you agreed was s/val/arg/,\nthough.\n\nI had a few small comments on later steps, but I think these are\nmoving in the right direction.\n\nThanks.\n"}]}