{"thread":{"id":"47369","subject":"[PATCH 1/3] git-compat-util: introduce skip_to_opt_val()","startedAt":"2017-12-03T17:04:36Z","lastAt":"2017-12-07T06:26:43Z","messageCount":11,"participants":["Christian Couder","Junio C Hamano","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"333999","messageId":"20171203170415.15939-1-chriscool@tuxfamily.org","threadId":"47369","inReplyTo":null,"subject":"[PATCH 1/3] git-compat-util: introduce skip_to_opt_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-03T17:04:13Z","receivedAt":"2017-12-03T17:04:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nWe often accept both a \"--key\" option and a \"--key=\" 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_opt_val(arg, \"--key\", &arg)) {\n\t/* do something with arg */\n}\n\nNote that, when using skip_to_opt_val(), it is not possible any more\nto do something different when the first argument is exactly \"--key\"\nthan when it is exactly \"--key=\", but in most cases we already don't\nmake any difference, which is a good thing.\n\nNote that \"opt\" in the function name actually means \"optional\" as\nthe function can be used to parse any \"key=value\" string where \"key\"\nis also considered as valid, not just command line options.\n\nSigned-off-by: Christian Couder \n---\n git-compat-util.h | 35 +++++++++++++++++++++++++++++++++++\n 1 file changed, 35 insertions(+)\n\nAnother possibility would be to add a \"const char *default\"\nargument to the function, and to do: \n\n\tif (!*p) {\n\t\t*val = default;\n\t\treturn 1;\n\t}\n\nThis could make the function more useful in some cases.\n\nI also wonder if the function is too big to be inlined, and\nin that case, in which file it should be added. \n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cedad4d581..7ee040388f 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -534,6 +534,41 @@ static inline int ends_with(const char *str, const char *suffix)\n \treturn strip_suffix(str, suffix, &len);\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 empty string 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=\" 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+static inline int skip_to_opt_val(const char *str, const char *prefix,\n+\t\t\t\t const char **val)\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 = \"\";\n+\t\treturn 1;\n+\t}\n+\n+\tif (*p == '=') {\n+\t\t*val = p + 1;\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n #define SWAP(a, b) do {\t\t\t\t\t\t\\\n \tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n \tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n-- \n2.15.1.271.g1a4e40aa5d.dirty\n\n"},{"id":"334000","messageId":"20171203170415.15939-2-chriscool@tuxfamily.org","threadId":"47369","inReplyTo":"20171203170415.15939-1-chriscool@tuxfamily.org","subject":"[PATCH 2/3] index-pack: use skip_to_opt_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-03T17:04:14Z","receivedAt":"2017-12-03T17:04:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nLet's simplify index-pack option parsing using skip_to_opt_val().\n\nSigned-off-by: Christian Couder \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..5cf252c885 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_opt_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_opt_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.271.g1a4e40aa5d.dirty\n\n"},{"id":"334001","messageId":"20171203170415.15939-3-chriscool@tuxfamily.org","threadId":"47369","inReplyTo":"20171203170415.15939-1-chriscool@tuxfamily.org","subject":"[PATCH 3/3] diff: use skip_to_opt_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-03T17:04:15Z","receivedAt":"2017-12-03T17:04:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nLet's simplify diff option parsing using skip_to_opt_val().\n\nSigned-off-by: Christian Couder \n---\n diff.c | 22 ++++++++--------------\n 1 file changed, 8 insertions(+), 14 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2ebe2227b4..067b498187 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4508,17 +4508,11 @@ 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) || skip_to_opt_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_opt_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 +4534,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_opt_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_opt_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 +4548,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_opt_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-- \n2.15.1.271.g1a4e40aa5d.dirty\n\n"},{"id":"334002","messageId":"xmqqk1y3ira9.fsf@gitster.mtv.corp.google.com","threadId":"47369","inReplyTo":"20171203170415.15939-1-chriscool@tuxfamily.org","subject":"Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-03T18:45:50Z","receivedAt":"2017-12-03T18:45:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> From: Christian Couder \n>\n> We often accept both a \"--key\" option and a \"--key=\" 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_opt_val(arg, \"--key\", &arg)) {\n> \t/* do something with arg */\n> }\n\nSounds sensible; especially if there are many existing code that can\nbe shortened by using this helper, that is.\n\n> Note that, when using skip_to_opt_val(), it is not possible any more\n> to do something different when the first argument is exactly \"--key\"\n> than when it is exactly \"--key=\", but in most cases we already don't\n> make any difference, which is a good thing.\n\nNote that \"it is not possible\" is misleading. skip_to_opt_val()\ncould be written to allow the caller to tell the difference if you\nchose to, but *you* made it impossible by assigning \"\" to arg upon\nseeing \"--key\". You could assign NULL to arg in that case, and\n\"--key\" and \"--key=\" can be differenciated by checking arg; the\nformer will give you NULL and the latter \"\".\n\nNot that I am convinced that it is a bad idea to deliberately lose\ninformation like this implementation does. At least not yet.\n\nIf there will be no conceivable caller that wants to differenticate\nbetween the two, the implementation that \"loses information\" can\nclaim to be easier to use, as the callers do not have to be forced\nto write something like:\n\n\tif (skip_to_optional_val(arg, \"--key\", &arg)\n\t\tdo_something(arg ? arg : \"\");\n\nto treat them the same.\n\nHaving said all that, I would expect skip_to_optional_val() (as a\nname of the helper I suspect skip_to_optional_arg() is more\nappropriate, though) to store NULL in the arg thing if there is no\noptional argument given. Also, the caller does not have to even do\nthe 'arg ? arg : \"\"' dance if it is so common and natural for the\napplication to treat \"--key\" and \"--key=\" equivalently, as it is\nexpected that the actual code to work on the arg, i.e. do_something()\nin the above example, _should_ be prepared to take NULL and \"\" and\ntreat them the same way (that is the definition of \"it is so common\nand natural for the application to treat them the same).\n\nSo, I think you identified interesting pattern that can be cleaned\nup by introducing a helper, but I am not sure if I agree with the\nexact design of the helper.\n\n> Note that \"opt\" in the function name actually means \"optional\" as\n> the function can be used to parse any \"key=value\" string where \"key\"\n> is also considered as valid, not just command line options.\n\nYup. This paragraph is a good thing to have in the proposed log\nmessage, to explain the reason why you force callers of this helper\nto spell out the leading dashes like \"--key\" (not \"key\"). I think\nthat it is a sane design of the input side of this function---it\ndoes not limit it to an overly narrow case of command line option\nprocessing. For the same reason, I think it is saner design of the\noutput side to allow callers to tell between \"key=\" and \"key\".\n\nWhile staring at this helper and writing \"does not limit to command\nline option processing\", it occurs to me that a helper that allows\nyou to look for an optional ':' (instead of '=') may also be useful,\nso the final version might become a pair of functions, perhaps like\nso:\n\n int skip_to_optional_delim(const char *s,\n\t\t\t const char *prefix, char delim,\n\t\t\t const char **rest)\n {\n\tconst char *p;\n\n\tif (!skip_prefix(str, prefix, &p))\n\t\treturn 0;\n\tif (!*p)\n\t\t*rest = NULL;\n\telse if (*p != delim)\n\t\treturn 0;\n\telse\n\t\t*rest = p + 1;\n\treturn 1;\n }\n\n int skip_to_optional_arg(const char *s,\n\t\t\t const char *prefix,\n\t\t\t const char **arg)\n {\n\treturn skip_to_optional_delim(s, prefix, '=', arg);\n }\n\nAs the first one would not (by definition) have any callers\ninitially after your series, it can be static to a file that\nimplements both of them and it is OK to expose only the latter to\nthe public. \n\nI do think it is a premature optimization to inline this function,\nwhose initial callers will be (from the look of the remainder of\nyour patches) command line parsing that sits farthest from any\nperformance critical code.\n\n>\n> Signed-off-by: Christian Couder \n> ---\n> git-compat-util.h | 35 +++++++++++++++++++++++++++++++++++\n> 1 file changed, 35 insertions(+)\n>\n> Another possibility would be to add a \"const char *default\"\n> argument to the function, and to do: \n>\n> \tif (!*p) {\n> \t\t*val = default;\n> \t\treturn 1;\n> \t}\n>\n> This could make the function more useful in some cases.\n>\n> I also wonder if the function is too big to be inlined, and\n> in that case, in which file it should be added. \n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index cedad4d581..7ee040388f 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -534,6 +534,41 @@ static inline int ends_with(const char *str, const char *suffix)\n> \treturn strip_suffix(str, suffix, &len);\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 empty string 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=\" 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> +static inline int skip_to_opt_val(const char *str, const char *prefix,\n> +\t\t\t\t const char **val)\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 = \"\";\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tif (*p == '=') {\n> +\t\t*val = p + 1;\n> +\t\treturn 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> #define SWAP(a, b) do {\t\t\t\t\t\t\\\n> \tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n> \tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n"},{"id":"334004","messageId":"CAP8UFD2OSsqzhyAL-QG1TOowB-xgbf=kC9wHre+FLc+0J1Xy+Q@mail.gmail.com","threadId":"47369","inReplyTo":"xmqqk1y3ira9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-03T20:34:29Z","receivedAt":"2017-12-03T20:34:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Dec 3, 2017 at 7:45 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> From: Christian Couder \n>>\n>> We often accept both a \"--key\" option and a \"--key=\" option.\n>>\n>> These options currently are parsed using something like:\n>>\n>> if (!strcmp(arg, \"--key\")) {\n>> /* do something */\n>> } else if (skip_prefix(arg, \"--key=\", &arg)) {\n>> /* do something with arg */\n>> }\n>>\n>> which is a bit cumbersome compared to just:\n>>\n>> if (skip_to_opt_val(arg, \"--key\", &arg)) {\n>> /* do something with arg */\n>> }\n>\n> Sounds sensible; especially if there are many existing code that can\n> be shortened by using this helper, that is.\n>\n>> Note that, when using skip_to_opt_val(), it is not possible any more\n>> to do something different when the first argument is exactly \"--key\"\n>> than when it is exactly \"--key=\", but in most cases we already don't\n>> make any difference, which is a good thing.\n>\n> Note that \"it is not possible\" is misleading. skip_to_opt_val()\n> could be written to allow the caller to tell the difference if you\n> chose to, but *you* made it impossible by assigning \"\" to arg upon\n> seeing \"--key\". You could assign NULL to arg in that case, and\n> \"--key\" and \"--key=\" can be differenciated by checking arg; the\n> former will give you NULL and the latter \"\".\n\nYeah, what I meant was \"the design of the function in this patch makes\nit impossible...\"\nThat's why I wrote after the diffstat:\n\n\"\"\"Another possibility would be to add a \"const char *default\"\nargument to the function, and to do:\n\n if (!*p) {\n *val = default;\n return 1;\n }\n\nThis could make the function more useful in some cases.\"\"\"\n\nBut yeah I could have added the above to the commit message and it\nhopefully would have made it clear what I meant.\n\nAnyway there is a design choice to be made. Adding a \"const char\n*default\" argument makes the function more generic, but this might not\nbe very useful as there are few cases that might benefit. And if we\nwant to make the command line interface consistent and perhaps avoid\nuser errors, we might want to have a rule that says that \"--key\" and\n\"--key=\" should always give the same result. In this case we may also\nwant to make it easy to implement options that follow the rule.\n\nSo my preference is to not add the \"const char *default\" argument to\nthe function. But it is not a strong preference.\n\n> Not that I am convinced that it is a bad idea to deliberately lose\n> information like this implementation does. At least not yet.\n>\n> If there will be no conceivable caller that wants to differenticate\n> between the two, the implementation that \"loses information\" can\n> claim to be easier to use, as the callers do not have to be forced\n> to write something like:\n>\n> if (skip_to_optional_val(arg, \"--key\", &arg)\n> do_something(arg ? arg : \"\");\n>\n> to treat them the same.\n>\n> Having said all that, I would expect skip_to_optional_val() (as a\n> name of the helper I suspect skip_to_optional_arg() is more\n> appropriate, though)\n\nOk I will rename it skip_to_optional_arg().\n\n> to store NULL in the arg thing if there is no\n> optional argument given. Also, the caller does not have to even do\n> the 'arg ? arg : \"\"' dance if it is so common and natural for the\n> application to treat \"--key\" and \"--key=\" equivalently, as it is\n> expected that the actual code to work on the arg, i.e. do_something()\n> in the above example, _should_ be prepared to take NULL and \"\" and\n> treat them the same way (that is the definition of \"it is so common\n> and natural for the application to treat them the same).\n\nI'd rather add the \"const char *default\" argument to the function\nrather than have the function just set \"arg\" to NULL. I think setting\n\"arg\" to NULL increases the risk of crashes and makes it too easy to\nmake \"--key\" and \"--key=\" behave differently which I think is not\nconsistent and not intuitive.\n\n> So, I think you identified interesting pattern that can be cleaned\n> up by introducing a helper, but I am not sure if I agree with the\n> exact design of the helper.\n\nOk, maybe the above explains the design a bit better.\n\n>> Note that \"opt\" in the function name actually means \"optional\" as\n>> the function can be used to parse any \"key=value\" string where \"key\"\n>> is also considered as valid, not just command line options.\n>\n> Yup. This paragraph is a good thing to have in the proposed log\n> message, to explain the reason why you force callers of this helper\n> to spell out the leading dashes like \"--key\" (not \"key\"). I think\n> that it is a sane design of the input side of this function---it\n> does not limit it to an overly narrow case of command line option\n> processing. For the same reason, I think it is saner design of the\n> output side to allow callers to tell between \"key=\" and \"key\".\n>\n> While staring at this helper and writing \"does not limit to command\n> line option processing\", it occurs to me that a helper that allows\n> you to look for an optional ':' (instead of '=') may also be useful,\n> so the final version might become a pair of functions, perhaps like\n> so:\n>\n> int skip_to_optional_delim(const char *s,\n> const char *prefix, char delim,\n> const char **rest)\n> {\n> const char *p;\n>\n> if (!skip_prefix(str, prefix, &p))\n> return 0;\n> if (!*p)\n> *rest = NULL;\n> else if (*p != delim)\n> return 0;\n> else\n> *rest = p + 1;\n> return 1;\n> }\n>\n> int skip_to_optional_arg(const char *s,\n> const char *prefix,\n> const char **arg)\n> {\n> return skip_to_optional_delim(s, prefix, '=', arg);\n> }\n>\n> As the first one would not (by definition) have any callers\n> initially after your series, it can be static to a file that\n> implements both of them and it is OK to expose only the latter to\n> the public.\n\nYeah, I thought about the above, but I am not very much interested in\nimplementing it now. I wonder if the callers will always want\nskip_to_optional_delim() to take only one delim character or if they\nwill want more than one delim character.\n\n> I do think it is a premature optimization to inline this function,\n> whose initial callers will be (from the look of the remainder of\n> your patches) command line parsing that sits farthest from any\n> performance critical code.\n\nOk, I will not inline it.\n\nThanks!\n"},{"id":"334024","messageId":"xmqqfu8rig10.fsf@gitster.mtv.corp.google.com","threadId":"47369","inReplyTo":"CAP8UFD2OSsqzhyAL-QG1TOowB-xgbf=kC9wHre+FLc+0J1Xy+Q@mail.gmail.com","subject":"Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-03T22:48:59Z","receivedAt":"2017-12-03T22:49:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Anyway there is a design choice to be made. Adding a \"const char\n> *default\" argument makes the function more generic,...\n\nI didn't suggest anything of that sort, and I do not understand why\nyou are repeatedly talking about \"default\" that you considered and\nrejected, as if it were an alternative viable option. I agree that\n\"default\" is not yet a good idea and it is a solution to a problem\nthat is not yet shown to exist. \n\nOn the other hand, just assigning NULL to *arg when you did not see\na delimiting '=', on the other hand, is an alternative option that\nis viable.\n\n> .... I think setting\n> \"arg\" to NULL increases the risk of crashes and makes it too easy to\n> make \"--key\" and \"--key=\" behave differently which I think is not\n> consistent and not intuitive.\n\nSo now this is very specific to the need of command line argument\nparsing and is not a generic thing? You cannot have your cake and\neat it too, though.\n\n"},{"id":"334033","messageId":"CAP8UFD0UH62fyLLAx8QP37L22if9zGjX=7f_c23asOFiKEEB7Q@mail.gmail.com","threadId":"47369","inReplyTo":"xmqqfu8rig10.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-04T07:59:12Z","receivedAt":"2017-12-04T07:59:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Dec 3, 2017 at 11:48 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> Anyway there is a design choice to be made. Adding a \"const char\n>> *default\" argument makes the function more generic,...\n>\n> I didn't suggest anything of that sort, and I do not understand why\n> you are repeatedly talking about \"default\" that you considered and\n> rejected, as if it were an alternative viable option. I agree that\n> \"default\" is not yet a good idea and it is a solution to a problem\n> that is not yet shown to exist.\n>\n> On the other hand, just assigning NULL to *arg when you did not see\n> a delimiting '=', on the other hand, is an alternative option that\n> is viable.\n\nWhat I am saying is that I'd rather have a lot of code like:\n\n if (skip_to_optional_val(arg, \"--key\", &arg, \"\") /* the last\nargument is the default value */\n do_something(arg);\n\nthan a lot of code like this:\n\n if (skip_to_optional_val(arg, \"--key\", &arg) /* no default can\nbe passed, NULL is the default */\n do_something(arg ? arg : \"\");\n\nbecause in the former case the `arg ? arg : \"\"` pattern is captured\ninside the function, so the code is simpler.\n\nIn the few cases where do_something() accepts NULL and does something\ndifferent with it, the former can be changed to:\n\n if (skip_to_optional_val(arg, \"--key\", &arg, NULL) /* the last\nargument is the default value */\n do_something(arg);\n\nSo yeah I rejected it, but my preference is not strong and I never\nsaid or thought that it was not viable. I just think that there are\nfew cases that might benefit. So the benefits are not big and it might\nbe better for consistency and simplicity of the UI to nudge people to\nmake \"--key\" and \"--key=\" behave the same. That's why having \"\" as the\ndefault and no default argument is a little better in my opinion.\n\n>> .... I think setting\n>> \"arg\" to NULL increases the risk of crashes and makes it too easy to\n>> make \"--key\" and \"--key=\" behave differently which I think is not\n>> consistent and not intuitive.\n>\n> So now this is very specific to the need of command line argument\n> parsing and is not a generic thing? You cannot have your cake and\n> eat it too, though.\n\nI think that even when we are not parsing options, it is probably good\nin general for UI consistency and simplicity that \"key\" and \"key=\" are\ninterpreted in the same way.\n"},{"id":"334044","messageId":"xmqqpo7uobuc.fsf@gitster.mtv.corp.google.com","threadId":"47369","inReplyTo":"CAP8UFD0UH62fyLLAx8QP37L22if9zGjX=7f_c23asOFiKEEB7Q@mail.gmail.com","subject":"Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-04T13:35:07Z","receivedAt":"2017-12-04T13:35:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> In the few cases where do_something() accepts NULL and does something\n> different with it, the former can be changed to:\n>\n> if (skip_to_optional_val(arg, \"--key\", &arg, NULL) /* the last\n> argument is the default value */\n> do_something(arg);\n\nThat's the thing. If do_something() that takes \"\" and NULL and\nbehaves differently is rare, that indicates that the existing code\nmay not be committed to treat \"--key\" and \"--key=''\" the same in the\nfirst place, and I am not 100% convinced that I want to see us\ncommitted to force that design throughout the system by introducing\na helper that hardcodes the equivalence and encourages to use it.\n\nImagine a command that takes \"--do-something\" option and does that\n\"something\" unconditionally. We may later extend it to take an\noptional argument, i.e. \"--do-something=c1,c2,...\", to tell the\ncommand to do that \"something\" under some but not all conditions.\nThe values c1,c2 would tell that we want that something done only\nunder either conditions c1 or c2 holds true.\n\nIt would be natural to expect that \"--do-something=\" to do that\n\"something\" under no condition (i.e. as if no such option was\ngiven); that would help scripts that accumulate the set of\nconditions in a variable and say \"--do-something=$when\", by making\nit a no-op when the variable $when turns out to be an empty string.\n\"--do-something\" without \"=\" would not want to mean the same thing.\n\nThe above observation makes me suspect that it depends on the \"key\"\nwhat \"--key=$value\" we want to be equivalent to \"--key\". In the\n\"--do-something\" case, we do not want to pretend as if we got an\nempty string; instead we'd pretend as if we got \"always\" or\nsomething like that.\n\nAnd your \"default\" would work well for this \"default is tied to what\nthe key is\" paradigm, i.e.\n\n\tskip_to_optional_arg(arg, \"--do-something\", &arg, \"always\")\n\nwould make us treat \"--do-something\" and \"--do-something=always\" the\nsame way.\n\nIf it turns out that the default arg almost always is an empty\nstring, I do not mind \n\n\t#define skip_to_opt_arg(s,k,v) skip_to_optional_arg(s,k,v,\"\")\n\nof course.\n"},{"id":"334292","messageId":"CA+P7+xqD80v=CsbT003b7czro4CZ77CJMppAhbQOPo1ssqTsKA@mail.gmail.com","threadId":"47369","inReplyTo":"20171203170415.15939-3-chriscool@tuxfamily.org","subject":"Re: [PATCH 3/3] diff: use skip_to_opt_val()","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-12-07T00:16:43Z","receivedAt":"2017-12-07T00:17:10Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Dec 3, 2017 at 9:04 AM, Christian Couder\n wrote:\n> From: Christian Couder \n>\n> Let's simplify diff option parsing using skip_to_opt_val().\n>\n> Signed-off-by: Christian Couder \n> ---\n> diff.c | 22 ++++++++--------------\n> 1 file changed, 8 insertions(+), 14 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 2ebe2227b4..067b498187 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4508,17 +4508,11 @@ int diff_opt_parse(struct diff_options *options,\n> options->output_format |= DIFF_FORMAT_NUMSTAT;\n> else if (!strcmp(arg, \"--shortstat\"))\n> options->output_format |= DIFF_FORMAT_SHORTSTAT;\n> - else if (!strcmp(arg, \"-X\") || !strcmp(arg, \"--dirstat\"))\n> - return parse_dirstat_opt(options, \"\");\n> - else if (skip_prefix(arg, \"-X\", &arg))\n> - return parse_dirstat_opt(options, arg);\n> - else if (skip_prefix(arg, \"--dirstat=\", &arg))\n> + else if (skip_prefix(arg, \"-X\", &arg) || skip_to_opt_val(arg, \"--dirstat\", &arg))\n> return parse_dirstat_opt(options, arg);\n> else if (!strcmp(arg, \"--cumulative\"))\n> return parse_dirstat_opt(options, \"cumulative\");\n> - else if (!strcmp(arg, \"--dirstat-by-file\"))\n> - return parse_dirstat_opt(options, \"files\");\n> - else if (skip_prefix(arg, \"--dirstat-by-file=\", &arg)) {\n> + else if (skip_to_opt_val(arg, \"--dirstat-by-file\", &arg)) {\n> parse_dirstat_opt(options, \"files\");\n> return parse_dirstat_opt(options, arg);\n> }\n> @@ -4540,13 +4534,13 @@ int diff_opt_parse(struct diff_options *options,\n> return stat_opt(options, av);\n>\n> /* renames options */\n> - else if (starts_with(arg, \"-B\") || starts_with(arg, \"--break-rewrites=\") ||\n> - !strcmp(arg, \"--break-rewrites\")) {\n> + else if (starts_with(arg, \"-B\") ||\n> + skip_to_opt_val(arg, \"--break-rewrites\", &optarg)) {\n> if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n> return error(\"invalid argument to -B: %s\", arg+2);\n> }\n> - else if (starts_with(arg, \"-M\") || starts_with(arg, \"--find-renames=\") ||\n> - !strcmp(arg, \"--find-renames\")) {\n> + else if (starts_with(arg, \"-M\") ||\n> + skip_to_opt_val(arg, \"--find-renames\", &optarg)) {\n> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n> return error(\"invalid argument to -M: %s\", arg+2);\n> options->detect_rename = DIFF_DETECT_RENAME;\n> @@ -4554,8 +4548,8 @@ int diff_opt_parse(struct diff_options *options,\n> else if (!strcmp(arg, \"-D\") || !strcmp(arg, \"--irreversible-delete\")) {\n> options->irreversible_delete = 1;\n> }\n> - else if (starts_with(arg, \"-C\") || starts_with(arg, \"--find-copies=\") ||\n> - !strcmp(arg, \"--find-copies\")) {\n> + else if (starts_with(arg, \"-C\") ||\n> + skip_to_opt_val(arg, \"--find-copies\", &optarg)) {\n> if (options->detect_rename == DIFF_DETECT_COPY)\n> options->flags.find_copies_harder = 1;\n> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n> --\n> 2.15.1.271.g1a4e40aa5d.dirty\n>\n\nThis causes a regression in the --relative option which prevents it\nfrom working properly.\n\nIf I have a repository with a modified file in a subdirectory, such as:\n\na/file\n\nthen git diff-index --relative --name-only HEAD from within \"a\" will\nreturn \"a/file\" instead of \"file\"\n\nThis breaks git completion, (among other things).\n\nThanks,\nJake\n"},{"id":"334293","messageId":"CA+P7+xpeBsH4QGmv-m2i4RQ7DdZ9pMnpHmFi_=akDkg0CTj+pg@mail.gmail.com","threadId":"47369","inReplyTo":"CA+P7+xqD80v=CsbT003b7czro4CZ77CJMppAhbQOPo1ssqTsKA@mail.gmail.com","subject":"Re: [PATCH 3/3] diff: use skip_to_opt_val()","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-12-07T00:18:20Z","receivedAt":"2017-12-07T00:18:46Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Dec 6, 2017 at 4:16 PM, Jacob Keller wrote:\n> On Sun, Dec 3, 2017 at 9:04 AM, Christian Couder\n> wrote:\n>> From: Christian Couder \n>>\n>> Let's simplify diff option parsing using skip_to_opt_val().\n>>\n>> Signed-off-by: Christian Couder \n>> ---\n>> diff.c | 22 ++++++++--------------\n>> 1 file changed, 8 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index 2ebe2227b4..067b498187 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -4508,17 +4508,11 @@ int diff_opt_parse(struct diff_options *options,\n>> options->output_format |= DIFF_FORMAT_NUMSTAT;\n>> else if (!strcmp(arg, \"--shortstat\"))\n>> options->output_format |= DIFF_FORMAT_SHORTSTAT;\n>> - else if (!strcmp(arg, \"-X\") || !strcmp(arg, \"--dirstat\"))\n>> - return parse_dirstat_opt(options, \"\");\n>> - else if (skip_prefix(arg, \"-X\", &arg))\n>> - return parse_dirstat_opt(options, arg);\n>> - else if (skip_prefix(arg, \"--dirstat=\", &arg))\n>> + else if (skip_prefix(arg, \"-X\", &arg) || skip_to_opt_val(arg, \"--dirstat\", &arg))\n>> return parse_dirstat_opt(options, arg);\n>> else if (!strcmp(arg, \"--cumulative\"))\n>> return parse_dirstat_opt(options, \"cumulative\");\n>> - else if (!strcmp(arg, \"--dirstat-by-file\"))\n>> - return parse_dirstat_opt(options, \"files\");\n>> - else if (skip_prefix(arg, \"--dirstat-by-file=\", &arg)) {\n>> + else if (skip_to_opt_val(arg, \"--dirstat-by-file\", &arg)) {\n>> parse_dirstat_opt(options, \"files\");\n>> return parse_dirstat_opt(options, arg);\n>> }\n>> @@ -4540,13 +4534,13 @@ int diff_opt_parse(struct diff_options *options,\n>> return stat_opt(options, av);\n>>\n>> /* renames options */\n>> - else if (starts_with(arg, \"-B\") || starts_with(arg, \"--break-rewrites=\") ||\n>> - !strcmp(arg, \"--break-rewrites\")) {\n>> + else if (starts_with(arg, \"-B\") ||\n>> + skip_to_opt_val(arg, \"--break-rewrites\", &optarg)) {\n>> if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n>> return error(\"invalid argument to -B: %s\", arg+2);\n>> }\n>> - else if (starts_with(arg, \"-M\") || starts_with(arg, \"--find-renames=\") ||\n>> - !strcmp(arg, \"--find-renames\")) {\n>> + else if (starts_with(arg, \"-M\") ||\n>> + skip_to_opt_val(arg, \"--find-renames\", &optarg)) {\n>> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n>> return error(\"invalid argument to -M: %s\", arg+2);\n>> options->detect_rename = DIFF_DETECT_RENAME;\n>> @@ -4554,8 +4548,8 @@ int diff_opt_parse(struct diff_options *options,\n>> else if (!strcmp(arg, \"-D\") || !strcmp(arg, \"--irreversible-delete\")) {\n>> options->irreversible_delete = 1;\n>> }\n>> - else if (starts_with(arg, \"-C\") || starts_with(arg, \"--find-copies=\") ||\n>> - !strcmp(arg, \"--find-copies\")) {\n>> + else if (starts_with(arg, \"-C\") ||\n>> + skip_to_opt_val(arg, \"--find-copies\", &optarg)) {\n>> if (options->detect_rename == DIFF_DETECT_COPY)\n>> options->flags.find_copies_harder = 1;\n>> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n>> --\n>> 2.15.1.271.g1a4e40aa5d.dirty\n>>\n>\n> This causes a regression in the --relative option which prevents it\n> from working properly.\n>\n> If I have a repository with a modified file in a subdirectory, such as:\n>\n> a/file\n>\n> then git diff-index --relative --name-only HEAD from within \"a\" will\n> return \"a/file\" instead of \"file\"\n>\n> This breaks git completion, (among other things).\n>\n> Thanks,\n> Jake\n\nI believe this occurs because skip_to_optional_val overwrites the arg\neven if it's not there, and the --relative argument expects prefix to\nremain unchanged if the optional value is not provided.\n\nThanks,\nJake\n"},{"id":"334305","messageId":"CAP8UFD3H3PGqFyQoNwwi9ZA2_WM7Q_3pikbmEOnLLHfD4k4ahA@mail.gmail.com","threadId":"47369","inReplyTo":"CA+P7+xpeBsH4QGmv-m2i4RQ7DdZ9pMnpHmFi_=akDkg0CTj+pg@mail.gmail.com","subject":"Re: [PATCH 3/3] diff: use skip_to_opt_val()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-07T06:26:37Z","receivedAt":"2017-12-07T06:26:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Dec 7, 2017 at 1:18 AM, Jacob Keller wrote:\n> On Wed, Dec 6, 2017 at 4:16 PM, Jacob Keller wrote:\n>>\n>> This causes a regression in the --relative option which prevents it\n>> from working properly.\n>>\n>> If I have a repository with a modified file in a subdirectory, such as:\n>>\n>> a/file\n>>\n>> then git diff-index --relative --name-only HEAD from within \"a\" will\n>> return \"a/file\" instead of \"file\"\n>>\n>> This breaks git completion, (among other things).\n\nOk, thanks for the report. I will fix that.\n\n> I believe this occurs because skip_to_optional_val overwrites the arg\n> even if it's not there, and the --relative argument expects prefix to\n> remain unchanged if the optional value is not provided.\n\nYeah, that makes sense.\n"}]}