{"thread":{"id":"14594","subject":"[PATCH] parse-options: fix parsing of \"--foobar=\" with no value","startedAt":"2008-07-22T18:44:27Z","lastAt":"2008-07-22T20:09:11Z","messageCount":6,"participants":["Olivier Marin","Sverre Rabbelier","Pierre Habouzit","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"84346","messageId":"1216752267-12138-1-git-send-email-dkr+ml.git@free.fr","threadId":"14594","inReplyTo":null,"subject":"[PATCH] parse-options: fix parsing of \"--foobar=\" with no value","fromName":"Olivier Marin","fromEmail":"dkr+ml.git@free.fr","sentAt":"2008-07-22T18:44:27Z","receivedAt":"2008-07-22T18:44:27Z","isPatch":true,"sender":{"key":"dkr+ml.git@free.fr","avatar":null},"body":"From: Olivier Marin \n\nBefore this patch, running a git command with a \"--foobar=\" argument\nwill set the \"foobar\" option with a random value and continue.\nWe should instead, exit with an error if a value is required, or use\nthe default one if the value is optional.\n\nThis patch fix the above issue and add some test cases.\n\nSigned-off-by: Olivier Marin \n---\n parse-options.c | 8 ++++----\n t/t0040-parse-options.sh | 25 +++++++++++++++++++++++++\n test-parse-options.c | 3 +++\n 3 files changed, 32 insertions(+), 4 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 71a7acf..67be197 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -17,7 +17,7 @@ static int opterror(const struct option *opt, const char *reason, int flags)\n static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,\n \t\t int flags, const char **arg)\n {\n-\tif (p->opt) {\n+\tif (p->opt && *p->opt) {\n \t\t*arg = p->opt;\n \t\tp->opt = NULL;\n \t} else if (p->argc == 1 && (opt->flags & PARSE_OPT_LASTARG_DEFAULT)) {\n@@ -80,7 +80,7 @@ static int get_value(struct parse_opt_ctx_t *p,\n \tcase OPTION_STRING:\n \t\tif (unset)\n \t\t\t*(const char **)opt->value = NULL;\n-\t\telse if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n+\t\telse if (opt->flags & PARSE_OPT_OPTARG && (!p->opt || !*p->opt))\n \t\t\t*(const char **)opt->value = (const char *)opt->defval;\n \t\telse\n \t\t\treturn get_arg(p, opt, flags, (const char **)opt->value);\n@@ -91,7 +91,7 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn (*opt->callback)(opt, NULL, 1) ? (-1) : 0;\n \t\tif (opt->flags & PARSE_OPT_NOARG)\n \t\t\treturn (*opt->callback)(opt, NULL, 0) ? (-1) : 0;\n-\t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n+\t\tif (opt->flags & PARSE_OPT_OPTARG && (!p->opt || !*p->opt))\n \t\t\treturn (*opt->callback)(opt, NULL, 0) ? (-1) : 0;\n \t\tif (get_arg(p, opt, flags, &arg))\n \t\t\treturn -1;\n@@ -102,7 +102,7 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t\t*(int *)opt->value = 0;\n \t\t\treturn 0;\n \t\t}\n-\t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n+\t\tif (opt->flags & PARSE_OPT_OPTARG && (!p->opt || !*p->opt)) {\n \t\t\t*(int *)opt->value = opt->defval;\n \t\t\treturn 0;\n \t\t}\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 03dbe00..7ce2e86 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -26,6 +26,8 @@ String options\n --st get another string (pervert ordering)\n -o get another string\n --default-string set string to default\n+ --optional-string[=]\n+ set string to optional if none given\n \n Magic arguments\n --quux means --quux\n@@ -85,6 +87,29 @@ test_expect_success 'missing required value' '\n \ttest $? = 129\n '\n \n+test_expect_success 'missing required value with \"--foobar=\"' '\n+\ttest-parse-options --length=;\n+\ttest $? = 129 &&\n+\ttest-parse-options --len=;\n+\ttest $? = 129\n+'\n+\n+cat > expect << EOF\n+boolean: 0\n+integer: 0\n+string: optional\n+abbrev: 7\n+verbose: 0\n+quiet: no\n+dry run: no\n+EOF\n+\n+test_expect_success 'optional value with \"--foobar=\"' '\n+\ttest-parse-options --optional-string= > output 2> output.err &&\n+\ttest ! -s output.err &&\n+\ttest_cmp expect output\n+'\n+\n cat > expect << EOF\n boolean: 1\n integer: 13\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 2a79e72..76db37e 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -42,6 +42,9 @@ int main(int argc, const char **argv)\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_SET_PTR(0, \"default-string\", &string,\n \t\t\t\"set string to default\", (unsigned long)\"default\"),\n+\t\t{ OPTION_STRING, 0, \"optional-string\", &string,\n+\t\t\t\"string\", \"set string to optional if none given\",\n+\t\t\tPARSE_OPT_OPTARG, NULL, (unsigned long)\"optional\" },\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n \t\tOPT_GROUP(\"Standard options\"),\n-- \n1.6.0.rc0.16.gb169.dirty\n"},{"id":"84347","messageId":"bd6139dc0807221153n18c864b7ve83a316867e892ab@mail.gmail.com","threadId":"14594","inReplyTo":"1216752267-12138-1-git-send-email-dkr+ml.git@free.fr","subject":"Re: [PATCH] parse-options: fix parsing of \"--foobar=\" with no value","fromName":"Sverre Rabbelier","fromEmail":"alturin@gmail.com","sentAt":"2008-07-22T18:53:06Z","receivedAt":"2008-07-22T18:53:06Z","isPatch":true,"sender":{"key":"alturin@gmail.com","avatar":null},"body":"On Tue, Jul 22, 2008 at 8:44 PM, Olivier Marin wrote:\n> We should instead, exit with an error if a value is required, or use\n> the default one if the value is optional.\n\nThis makes no sense, when I run \"git foo --bar=\" I either mean \"set\nbar to empty\" or I typo-ed. Why would I specify \"--bar=\" if I want the\ndefault value?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"84348","messageId":"20080722185427.GA10453@artemis.madism.org","threadId":"14594","inReplyTo":"1216752267-12138-1-git-send-email-dkr+ml.git@free.fr","subject":"Re: [PATCH] parse-options: fix parsing of \"--foobar=\" with no value","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-22T18:54:27Z","receivedAt":"2008-07-22T18:54:27Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jul 22, 2008 at 06:44:27PM +0000, Olivier Marin wrote:\n> From: Olivier Marin \n> \n> Before this patch, running a git command with a \"--foobar=\" argument\n> will set the \"foobar\" option with a random value and continue.\n> We should instead, exit with an error if a value is required, or use\n> the default one if the value is optional.\n\n Wrong, --foobar= is the option \"foobar\" with the argument \"\" (empty\nstring). as soon as you use the --foobar=... form, that is the \"stuck\nform\" for long option, there *is* a value.\n\n IOW --foobar= is not the same as --foobar at all. If like you claim,\n--foobar= pass a \"random\" value to the option then *this* is a bug, it\nshould pass a pointer to an empty string (IOW a pointer that points to a\nNUL byte), but I see nothing in the code that would explain what you\nclaim.\n\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"84349","messageId":"48863436.50309@free.fr","threadId":"14594","inReplyTo":"20080722185427.GA10453@artemis.madism.org","subject":"Re: [PATCH] parse-options: fix parsing of \"--foobar=\" with no value","fromName":"Olivier Marin","fromEmail":"dkr+ml.git@free.fr","sentAt":"2008-07-22T19:25:42Z","receivedAt":"2008-07-22T19:25:42Z","isPatch":true,"sender":{"key":"dkr+ml.git@free.fr","avatar":null},"body":"Pierre Habouzit a écrit :\n> On Tue, Jul 22, 2008 at 06:44:27PM +0000, Olivier Marin wrote:\n> \n> Wrong, --foobar= is the option \"foobar\" with the argument \"\" (empty\n> string). as soon as you use the --foobar=... form, that is the \"stuck\n> form\" for long option, there *is* a value.\n\nAh, OK.\n\nI would have find it convenient for things like --foobar=$var where foobar\nfallback to default when $var is empty. But I don't care that much.\n\n> IOW --foobar= is not the same as --foobar at all. If like you claim,\n> --foobar= pass a \"random\" value to the option then *this* is a bug, it\n> should pass a pointer to an empty string (IOW a pointer that points to a\n> NUL byte), but I see nothing in the code that would explain what you\n> claim.\n\nI found the \"random bug\" while migrating \"git init\" to parse-options. I\nthink you can reproduce it with:\n\n$ git clone --template= \nerror: ignoring template /var/run/synaptic.socket\nfatal: cannot opendir /var/run/sudo\n\nBut now, it appears the problem is not in parse-options, sorry.\n\n-- \nOlivier.\n"},{"id":"84354","messageId":"alpine.DEB.1.00.0807222104100.8986@racer","threadId":"14594","inReplyTo":"48863436.50309@free.fr","subject":"Re: [PATCH] parse-options: fix parsing of \"--foobar=\" with no value","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-22T20:05:02Z","receivedAt":"2008-07-22T20:05:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 22 Jul 2008, Olivier Marin wrote:\n\n> I would have find it convenient for things like --foobar=$var where foobar\n> fallback to default when $var is empty.\n\n--foobar=${var:-default} will expand to --foobar=default when $var is \nempty.\n\nHth,\nDscho\n"},{"id":"84355","messageId":"20080722200911.GA3097@sigill.intra.peff.net","threadId":"14594","inReplyTo":"48863436.50309@free.fr","subject":"Re: [PATCH] parse-options: fix parsing of \"--foobar=\" with no value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-22T20:09:11Z","receivedAt":"2008-07-22T20:09:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2008 at 09:25:42PM +0200, Olivier Marin wrote:\n\n> I found the \"random bug\" while migrating \"git init\" to parse-options. I\n> think you can reproduce it with:\n> \n> $ git clone --template= \n> error: ignoring template /var/run/synaptic.socket\n> fatal: cannot opendir /var/run/sudo\n> \n> But now, it appears the problem is not in parse-options, sorry.\n\nYes, the problem is that copy_templates in builtin-init-db.c is totally\nbroken for an empty template name. It writes past the beginning of the\nstring, and then starts copying at \"/\". Oops.\n\nMaybe something like this is better? It should define --template= to\nmean \"don't copy any templates\" (and I haven't tested it at all).\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex 38b4fcb..baf0d09 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -117,6 +117,8 @@ static void copy_templates(const char *template_dir)\n \t\ttemplate_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);\n \tif (!template_dir)\n \t\ttemplate_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);\n+\tif (!template_dir[0])\n+\t\treturn;\n \tstrcpy(template_path, template_dir);\n \ttemplate_len = strlen(template_path);\n \tif (template_path[template_len-1] != '/') {\n"}]}