{"thread":{"id":"50281","subject":"[PATCH 0/3] nd/diff-parseopt fixups","startedAt":"2019-01-22T00:26:42Z","lastAt":"2019-01-22T23:18:23Z","messageCount":10,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"367273","messageId":"20190122002635.9411-1-pclouds@gmail.com","threadId":"50281","inReplyTo":null,"subject":"[PATCH 0/3] nd/diff-parseopt fixups","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T00:26:32Z","receivedAt":"2019-01-22T00:26:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Let me see if I could just fix up this series on top like this.\nFailing this, I'll break it down and resend in small chunks.\n\nThis redoes a85e5a0108 (parse-options: allow keep-unknown +\nstop-at-non-opt combination - 2019-01-17), deletes dead functions and\nrewrites b9b760ed1c (diff.c: convert -B|--break-rewrites - 2019-01-17) a\nbit.\n\nNguyễn Thái Ngọc Duy (3):\n Revert \"parse-options: allow keep-unknown + stop-at-non-opt\n combination\"\n diff.c: delete unused functions\n diff.c: simplify diff_opt_break_rewrites()\n\n diff.c | 74 ++-----------------------------------------------\n parse-options.c | 4 +++\n 2 files changed, 6 insertions(+), 72 deletions(-)\n\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367274","messageId":"20190122002635.9411-2-pclouds@gmail.com","threadId":"50281","inReplyTo":"20190122002635.9411-1-pclouds@gmail.com","subject":"[PATCH 1/3] Revert \"parse-options: allow keep-unknown + stop-at-non-opt combination\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T00:26:33Z","receivedAt":"2019-01-22T00:26:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This reverts commit a85e5a01080e35da0f94bf98f5f334c64e214ce3, which is a\nrevert of 0d260f9a09 (parseopt: prevent KEEP_UNKNOWN and\nSTOP_AT_NON_OPTION from being used together - 2009-03-09).\n\nAs described in 0d260f9a09, this option combination is tricky to get\nright and should be prevented unless you know what you're doing. In the\none-shot mode, the diff and revision parsers are very well aware of\nremaining arguments being potentially unknown options or revisions and\nshould handle the combination safely.\n\nReinstate the check added by 0d260f9a09. Modify it a bit to let one-shot\nmode through.\n\nNoticed-by: Stefan Beller \n---\n parse-options.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a0ff8ea225..cec74522e5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -466,6 +466,10 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->prefix = prefix;\n \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n+\tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n+\t (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&\n+\t !(flags & PARSE_OPT_ONE_SHOT))\n+\t\tBUG(\"STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together\");\n \tif ((flags & PARSE_OPT_ONE_SHOT) &&\n \t (flags & PARSE_OPT_KEEP_ARGV0))\n \t\tBUG(\"Can't keep argv0 if you don't have it\");\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367275","messageId":"20190122002635.9411-4-pclouds@gmail.com","threadId":"50281","inReplyTo":"20190122002635.9411-1-pclouds@gmail.com","subject":"[PATCH 3/3] diff.c: simplify diff_opt_break_rewrites()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T00:26:35Z","receivedAt":"2019-01-22T00:26:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Helped-by: Johannes Schindelin \n---\n diff.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex da5ba835ce..2351571251 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4814,20 +4814,15 @@ static int diff_opt_break_rewrites(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n \tint *break_opt = opt->value;\n-\tint opt1, opt2;\n+\tint opt1, opt2 = 0;\n \n \tBUG_ON_OPT_NEG(unset);\n \tif (!arg)\n \t\targ = \"\";\n \topt1 = parse_rename_score(&arg);\n-\tswitch (*arg) {\n-\tcase '\\0':\n-\t\topt2 = 0;\n-\t\tbreak;\n-\tcase '/':\n+\tif (*arg == '/') {\n \t\targ++;\n \t\topt2 = parse_rename_score(&arg);\n-\t\tbreak;\n \t}\n \tif (*arg != 0)\n \t\treturn error(_(\"%s expects / form\"), opt->long_name);\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367276","messageId":"20190122002635.9411-3-pclouds@gmail.com","threadId":"50281","inReplyTo":"20190122002635.9411-1-pclouds@gmail.com","subject":"[PATCH 2/3] diff.c: delete unused functions","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T00:26:34Z","receivedAt":"2019-01-22T00:26:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"After the parseopt conversion, a few functions are no longer\nneeded. Delete them.\n\nNoticed-by: SZEDER Gábor \nNoticed-by: Johannes Schindelin \n---\n diff.c | 65 ----------------------------------------------------------\n 1 file changed, 65 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 747e691190..da5ba835ce 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -104,11 +104,6 @@ static const char *color_diff_slots[] = {\n \t[DIFF_FILE_NEW_BOLD]\t = \"newBold\",\n };\n \n-static NORETURN void die_want_option(const char *option_name)\n-{\n-\tdie(_(\"option '%s' requires a value\"), option_name);\n-}\n-\n define_list_config_array_extra(color_diff_slots, {\"plain\"});\n \n static int parse_diff_color_slot(const char *var)\n@@ -4578,66 +4573,6 @@ void diff_setup_done(struct diff_options *options)\n \tFREE_AND_NULL(options->parseopts);\n }\n \n-static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\n-{\n-\tchar c, *eq;\n-\tint len;\n-\n-\tif (*arg != '-')\n-\t\treturn 0;\n-\tc = *++arg;\n-\tif (!c)\n-\t\treturn 0;\n-\tif (c == arg_short) {\n-\t\tc = *++arg;\n-\t\tif (!c)\n-\t\t\treturn 1;\n-\t\tif (val && isdigit(c)) {\n-\t\t\tchar *end;\n-\t\t\tint n = strtoul(arg, &end, 10);\n-\t\t\tif (*end)\n-\t\t\t\treturn 0;\n-\t\t\t*val = n;\n-\t\t\treturn 1;\n-\t\t}\n-\t\treturn 0;\n-\t}\n-\tif (c != '-')\n-\t\treturn 0;\n-\targ++;\n-\teq = strchrnul(arg, '=');\n-\tlen = eq - arg;\n-\tif (!len || strncmp(arg, arg_long, len))\n-\t\treturn 0;\n-\tif (*eq) {\n-\t\tint n;\n-\t\tchar *end;\n-\t\tif (!isdigit(*++eq))\n-\t\t\treturn 0;\n-\t\tn = strtoul(eq, &end, 10);\n-\t\tif (*end)\n-\t\t\treturn 0;\n-\t\t*val = n;\n-\t}\n-\treturn 1;\n-}\n-\n-static inline int short_opt(char opt, const char **argv,\n-\t\t\t const char **optarg)\n-{\n-\tconst char *arg = argv[0];\n-\tif (arg[0] != '-' || arg[1] != opt)\n-\t\treturn 0;\n-\tif (arg[2] != '\\0') {\n-\t\t*optarg = arg + 2;\n-\t\treturn 1;\n-\t}\n-\tif (!argv[1])\n-\t\tdie(\"Option '%c' requires a value\", opt);\n-\t*optarg = argv[1];\n-\treturn 2;\n-}\n-\n int parse_long_opt(const char *opt, const char **argv,\n \t\t const char **optarg)\n {\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367277","messageId":"20190122003020.9672-1-pclouds@gmail.com","threadId":"50281","inReplyTo":"20190122002635.9411-1-pclouds@gmail.com","subject":"[PATCH v2 0/3] nd/diff-parseopt fixups","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T00:30:17Z","receivedAt":"2019-01-22T00:30:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Resend with s-o-b. Oops.\n\nNguyễn Thái Ngọc Duy (3):\n Revert \"parse-options: allow keep-unknown + stop-at-non-opt\n combination\"\n diff.c: delete unused functions\n diff.c: simplify diff_opt_break_rewrites()\n\n diff.c | 74 ++-----------------------------------------------\n parse-options.c | 4 +++\n 2 files changed, 6 insertions(+), 72 deletions(-)\n\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367278","messageId":"20190122003020.9672-2-pclouds@gmail.com","threadId":"50281","inReplyTo":"20190122003020.9672-1-pclouds@gmail.com","subject":"[PATCH v2 1/3] Revert \"parse-options: allow keep-unknown + stop-at-non-opt combination\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T00:30:18Z","receivedAt":"2019-01-22T00:30:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This reverts commit a85e5a01080e35da0f94bf98f5f334c64e214ce3, which is a\nrevert of 0d260f9a09 (parseopt: prevent KEEP_UNKNOWN and\nSTOP_AT_NON_OPTION from being used together - 2009-03-09).\n\nAs described in 0d260f9a09, this option combination is tricky to get\nright and should be prevented unless you know what you're doing. In the\none-shot mode, the diff and revision parsers are very well aware of\nremaining arguments being potentially unknown options or revisions and\nshould handle the combination safely.\n\nReinstate the check added by 0d260f9a09. Modify it a bit to let one-shot\nmode through.\n\nNoticed-by: Stefan Beller \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n parse-options.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a0ff8ea225..cec74522e5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -466,6 +466,10 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->prefix = prefix;\n \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n+\tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n+\t (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&\n+\t !(flags & PARSE_OPT_ONE_SHOT))\n+\t\tBUG(\"STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together\");\n \tif ((flags & PARSE_OPT_ONE_SHOT) &&\n \t (flags & PARSE_OPT_KEEP_ARGV0))\n \t\tBUG(\"Can't keep argv0 if you don't have it\");\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367279","messageId":"20190122003020.9672-3-pclouds@gmail.com","threadId":"50281","inReplyTo":"20190122003020.9672-1-pclouds@gmail.com","subject":"[PATCH v2 2/3] diff.c: delete unused functions","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T00:30:19Z","receivedAt":"2019-01-22T00:30:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"After the parseopt conversion, a few functions are no longer\nneeded. Delete them.\n\nNoticed-by: SZEDER Gábor \nNoticed-by: Johannes Schindelin \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n diff.c | 65 ----------------------------------------------------------\n 1 file changed, 65 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 747e691190..da5ba835ce 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -104,11 +104,6 @@ static const char *color_diff_slots[] = {\n \t[DIFF_FILE_NEW_BOLD]\t = \"newBold\",\n };\n \n-static NORETURN void die_want_option(const char *option_name)\n-{\n-\tdie(_(\"option '%s' requires a value\"), option_name);\n-}\n-\n define_list_config_array_extra(color_diff_slots, {\"plain\"});\n \n static int parse_diff_color_slot(const char *var)\n@@ -4578,66 +4573,6 @@ void diff_setup_done(struct diff_options *options)\n \tFREE_AND_NULL(options->parseopts);\n }\n \n-static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\n-{\n-\tchar c, *eq;\n-\tint len;\n-\n-\tif (*arg != '-')\n-\t\treturn 0;\n-\tc = *++arg;\n-\tif (!c)\n-\t\treturn 0;\n-\tif (c == arg_short) {\n-\t\tc = *++arg;\n-\t\tif (!c)\n-\t\t\treturn 1;\n-\t\tif (val && isdigit(c)) {\n-\t\t\tchar *end;\n-\t\t\tint n = strtoul(arg, &end, 10);\n-\t\t\tif (*end)\n-\t\t\t\treturn 0;\n-\t\t\t*val = n;\n-\t\t\treturn 1;\n-\t\t}\n-\t\treturn 0;\n-\t}\n-\tif (c != '-')\n-\t\treturn 0;\n-\targ++;\n-\teq = strchrnul(arg, '=');\n-\tlen = eq - arg;\n-\tif (!len || strncmp(arg, arg_long, len))\n-\t\treturn 0;\n-\tif (*eq) {\n-\t\tint n;\n-\t\tchar *end;\n-\t\tif (!isdigit(*++eq))\n-\t\t\treturn 0;\n-\t\tn = strtoul(eq, &end, 10);\n-\t\tif (*end)\n-\t\t\treturn 0;\n-\t\t*val = n;\n-\t}\n-\treturn 1;\n-}\n-\n-static inline int short_opt(char opt, const char **argv,\n-\t\t\t const char **optarg)\n-{\n-\tconst char *arg = argv[0];\n-\tif (arg[0] != '-' || arg[1] != opt)\n-\t\treturn 0;\n-\tif (arg[2] != '\\0') {\n-\t\t*optarg = arg + 2;\n-\t\treturn 1;\n-\t}\n-\tif (!argv[1])\n-\t\tdie(\"Option '%c' requires a value\", opt);\n-\t*optarg = argv[1];\n-\treturn 2;\n-}\n-\n int parse_long_opt(const char *opt, const char **argv,\n \t\t const char **optarg)\n {\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367280","messageId":"20190122003020.9672-4-pclouds@gmail.com","threadId":"50281","inReplyTo":"20190122003020.9672-1-pclouds@gmail.com","subject":"[PATCH v2 3/3] diff.c: simplify diff_opt_break_rewrites()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T00:30:20Z","receivedAt":"2019-01-22T00:30:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Helped-by: Johannes Schindelin \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n diff.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex da5ba835ce..2351571251 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4814,20 +4814,15 @@ static int diff_opt_break_rewrites(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n \tint *break_opt = opt->value;\n-\tint opt1, opt2;\n+\tint opt1, opt2 = 0;\n \n \tBUG_ON_OPT_NEG(unset);\n \tif (!arg)\n \t\targ = \"\";\n \topt1 = parse_rename_score(&arg);\n-\tswitch (*arg) {\n-\tcase '\\0':\n-\t\topt2 = 0;\n-\t\tbreak;\n-\tcase '/':\n+\tif (*arg == '/') {\n \t\targ++;\n \t\topt2 = parse_rename_score(&arg);\n-\t\tbreak;\n \t}\n \tif (*arg != 0)\n \t\treturn error(_(\"%s expects / form\"), opt->long_name);\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367357","messageId":"xmqqtvi0e7sv.fsf@gitster-ct.c.googlers.com","threadId":"50281","inReplyTo":"20190122002635.9411-4-pclouds@gmail.com","subject":"Re: [PATCH 3/3] diff.c: simplify diff_opt_break_rewrites()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T20:50:08Z","receivedAt":"2019-01-22T20:50:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> -\tint opt1, opt2;\n> +\tint opt1, opt2 = 0;\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \tif (!arg)\n> \t\targ = \"\";\n> \topt1 = parse_rename_score(&arg);\n> -\tswitch (*arg) {\n> -\tcase '\\0':\n> -\t\topt2 = 0;\n> -\t\tbreak;\n> -\tcase '/':\n> +\tif (*arg == '/') {\n> \t\targ++;\n> \t\topt2 = parse_rename_score(&arg);\n> -\t\tbreak;\n> \t}\n> \tif (*arg != 0)\n> \t\treturn error(_(\"%s expects / form\"), opt->long_name);\n\nGood. I was about to complain on the lack of \"default\" that catches\nthe error at the end of \"\" in the switch(), but because this\nfollow-up validation is trying to catch \"\" form (i.e. single\nscore without slash) whose \"\" is malformed, and \"/\" form\nwhose \"\" is malformed, which is kind of clever, but it is not\nvery easy to understand what is going on, it makes sense to get rid\nof the switch() and make it if() statement.\n\nIt would make it even easier to follow if you did\n\n\tif (*arg == '/') {\n\t\topt2 = ...;\n\t\targ++;\n\t} else {\n\t\topt2 = 0;\n\t}\n\tif (*arg)\n\t\treturn error(...);\n\nIt makes it clear that opt2==0 means form and not / form,\nby having an explicit assignment while we parse what *arg points at,\nwithout the initialization to 0 in the variable definition.\n\nBut this should be squashed in the original patch. Having an \"oops,\nit was horribly unreadble---how about doing something like this?\"\nincremental is good during a discussion, and having a \"what we have\nseen is basically good and proven solid, but here is a small\nimprovement\" incremental is good for code that is stable enough to\nbuild on (read: at least in 'next'), but it is not particularly good\nfor a topic not yet in 'next' yet.\n\n\n"},{"id":"367389","messageId":"xmqqpnsocmdh.fsf@gitster-ct.c.googlers.com","threadId":"50281","inReplyTo":"xmqqtvi0e7sv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] diff.c: simplify diff_opt_break_rewrites()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T23:18:18Z","receivedAt":"2019-01-22T23:18:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It would make it even easier to follow if you did\n>\n> \tif (*arg == '/') {\n> \t\topt2 = ...;\n> \t\targ++;\n> \t} else {\n\nOops, this should read \"else if (!*arg) {\", of course, to match the\noriginal. Sorry for the noise.\n\n> \t\topt2 = 0;\n> \t}\n\nAnd then we'd want a blank line here to make it clear that the\nprevious if/else cascade has finished, and the error checking we see\nnext is not part of it.\n\n> \tif (*arg)\n> \t\treturn error(...);\n>\n> It makes it clear that opt2==0 means form and not / form,\n> by having an explicit assignment while we parse what *arg points at,\n> without the initialization to 0 in the variable definition.\n\n"}]}