[PATCH] parse-options: fix parsing of "--foobar=" with no value

From: Olivier Marin <redacted>

Before this patch, running a git command with a "--foobar=" argument
will set the "foobar" option with a random value and continue.
We should instead, exit with an error if a value is required, or use
the default one if the value is optional.

This patch fix the above issue and add some test cases.

Signed-off-by: Olivier Marin <redacted>
---
 parse-options.c          |    8 ++++----
 t/t0040-parse-options.sh |   25 +++++++++++++++++++++++++
 test-parse-options.c     |    3 +++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 71a7acf..67be197 100644
--- a/parse-options.c
+++ b/parse-options.c

@@ -17,7 +17,7 @@ static int opterror(const struct option *opt, const char *reason, int flags)
 static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
 		   int flags, const char **arg)
 {
-	if (p->opt) {
+	if (p->opt && *p->opt) {
 		*arg = p->opt;
 		p->opt = NULL;
 	} else if (p->argc == 1 && (opt->flags & PARSE_OPT_LASTARG_DEFAULT)) {

@@ -80,7 +80,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 	case OPTION_STRING:
 		if (unset)
 			*(const char **)opt->value = NULL;
-		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
+		else if (opt->flags & PARSE_OPT_OPTARG && (!p->opt || !*p->opt))
 			*(const char **)opt->value = (const char *)opt->defval;
 		else
 			return get_arg(p, opt, flags, (const char **)opt->value);

@@ -91,7 +91,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 			return (*opt->callback)(opt, NULL, 1) ? (-1) : 0;
 		if (opt->flags & PARSE_OPT_NOARG)
 			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
-		if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
+		if (opt->flags & PARSE_OPT_OPTARG && (!p->opt || !*p->opt))
 			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
 		if (get_arg(p, opt, flags, &arg))
 			return -1;

@@ -102,7 +102,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 			*(int *)opt->value = 0;
 			return 0;
 		}
-		if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
+		if (opt->flags & PARSE_OPT_OPTARG && (!p->opt || !*p->opt)) {
 			*(int *)opt->value = opt->defval;
 			return 0;
 		}

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 03dbe00..7ce2e86 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh

@@ -26,6 +26,8 @@ String options
     --st <st>             get another string (pervert ordering)
     -o <str>              get another string
     --default-string      set string to default
+    --optional-string[=<string>]
+                          set string to optional if none given
 
 Magic arguments
     --quux                means --quux

@@ -85,6 +87,29 @@ test_expect_success 'missing required value' '
 	test $? = 129
 '
 
+test_expect_success 'missing required value with "--foobar="' '
+	test-parse-options --length=;
+	test $? = 129 &&
+	test-parse-options --len=;
+	test $? = 129
+'
+
+cat > expect << EOF
+boolean: 0
+integer: 0
+string: optional
+abbrev: 7
+verbose: 0
+quiet: no
+dry run: no
+EOF
+
+test_expect_success 'optional value with "--foobar="' '
+	test-parse-options --optional-string= > output 2> output.err &&
+	test ! -s output.err &&
+	test_cmp expect output
+'
+
 cat > expect << EOF
 boolean: 1
 integer: 13

diff --git a/test-parse-options.c b/test-parse-options.c
index 2a79e72..76db37e 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c

@@ -42,6 +42,9 @@ int main(int argc, const char **argv)
 		OPT_STRING('o', NULL, &string, "str", "get another string"),
 		OPT_SET_PTR(0, "default-string", &string,
 			"set string to default", (unsigned long)"default"),
+		{ OPTION_STRING, 0, "optional-string", &string,
+			"string", "set string to optional if none given",
+			PARSE_OPT_OPTARG, NULL, (unsigned long)"optional" },
 		OPT_GROUP("Magic arguments"),
 		OPT_ARGUMENT("quux", "means --quux"),
 		OPT_GROUP("Standard options"),

-- 
1.6.0.rc0.16.gb169.dirty

On Tue, Jul 22, 2008 at 8:44 PM, Olivier Marin [off-list ref] wrote:

We should instead, exit with an error if a value is required, or use
the default one if the value is optional.

This makes no sense, when I run "git foo --bar=" I either mean "set
bar to empty" or I typo-ed. Why would I specify "--bar=" if I want the
default value?

-- 
Cheers,

Sverre Rabbelier

On Tue, Jul 22, 2008 at 06:44:27PM +0000, Olivier Marin wrote:

From: Olivier Marin <redacted>

Before this patch, running a git command with a "--foobar=" argument
will set the "foobar" option with a random value and continue.
We should instead, exit with an error if a value is required, or use
the default one if the value is optional.

  Wrong, --foobar= is the option "foobar" with the argument "" (empty
string). as soon as you use the --foobar=... form, that is the "stuck
form" for long option, there *is* a value.

  IOW --foobar= is not the same as --foobar at all. If like you claim,
--foobar= pass a "random" value to the option then *this* is a bug, it
should pass a pointer to an empty string (IOW a pointer that points to a
NUL byte), but I see nothing in the code that would explain what you
claim.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Pierre Habouzit a écrit :

On Tue, Jul 22, 2008 at 06:44:27PM +0000, Olivier Marin wrote:

  Wrong, --foobar= is the option "foobar" with the argument "" (empty
string). as soon as you use the --foobar=... form, that is the "stuck
form" for long option, there *is* a value.

Ah, OK.

I would have find it convenient for things like --foobar=$var where foobar
fallback to default when $var is empty. But I don't care that much.

  IOW --foobar= is not the same as --foobar at all. If like you claim,
--foobar= pass a "random" value to the option then *this* is a bug, it
should pass a pointer to an empty string (IOW a pointer that points to a
NUL byte), but I see nothing in the code that would explain what you
claim.

I found the "random bug" while migrating "git init" to parse-options. I
think you can reproduce it with:

$ git clone --template= <repo>
error: ignoring template /var/run/synaptic.socket
fatal: cannot opendir /var/run/sudo

But now, it appears the problem is not in parse-options, sorry.

-- 
Olivier.

Hi,

On Tue, 22 Jul 2008, Olivier Marin wrote:

I would have find it convenient for things like --foobar=$var where foobar
fallback to default when $var is empty.

--foobar=${var:-default} will expand to --foobar=default when $var is 
empty.

Hth,
Dscho

On Tue, Jul 22, 2008 at 09:25:42PM +0200, Olivier Marin wrote:

I found the "random bug" while migrating "git init" to parse-options. I
think you can reproduce it with:

$ git clone --template= <repo>
error: ignoring template /var/run/synaptic.socket
fatal: cannot opendir /var/run/sudo

But now, it appears the problem is not in parse-options, sorry.

Yes, the problem is that copy_templates in builtin-init-db.c is totally
broken for an empty template name. It writes past the beginning of the
string, and then starts copying at "/". Oops.

Maybe something like this is better? It should define --template= to
mean "don't copy any templates" (and I haven't tested it at all).

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 38b4fcb..baf0d09 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c

@@ -117,6 +117,8 @@ static void copy_templates(const char *template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
 		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
+	if (!template_dir[0])
+		return;
 	strcpy(template_path, template_dir);
 	template_len = strlen(template_path);
 	if (template_path[template_len-1] != '/') {