threads / patch / 10133

patchPort builtin-add.c to use the new option parser.

Subject: [PATCH] Port builtin-add.c to use the new option parser.

## tl;dr

26 messages between Oct 3, 2007 and Oct 7, 2007. Diffs are folded; open one to read it.

replies: 25people: 8as markdown or json

Kristian Høgsberg· Oct 3, 2007, 21:45 UTC · lore

[PATCH] Add a simple option parser.

The option parser takes argc, argv, an array of struct option and a usage string. Each of the struct option elements in the array describes a valid option, its type and a pointer to the location where the value is written. The entry point is parse_options(), which scans through the given argv, and matches each option there against the list of valid options. During the scan, argv is rewritten to only contain the non-option command line arguments and the number of these is returned.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 Makefile        |    2 +-
 parse-options.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 parse-options.h |   33 +++++++++++++++++
 3 files changed, 140 insertions(+), 1 deletions(-)
 create mode 100644 parse-options.c
 create mode 100644 parse-options.h
Show changes to 3 files +140 −1

Makefile, parse-options.c, parse-options.h

diff --git a/Makefile b/Makefile
index 62bdac6..d90e959 100644
--- a/Makefile
+++ b/Makefile
@@ -310,7 +310,7 @@ LIB_OBJS = \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
 	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
-	transport.o bundle.o
+	transport.o bundle.o parse-options.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/parse-options.c b/parse-options.c
new file mode 100644
index 0000000..130b609
--- /dev/null
+++ b/parse-options.c
@@ -0,0 +1,106 @@
+#include "git-compat-util.h"
+#include "parse-options.h"
+
+static int parse_one(const char **argv,
+		     struct option *options, int count,
+		     const char *usage_string)
+{
+	const char *eq, *arg, *value;
+	int i, processed;
+
+	arg = argv[0];
+	value = NULL;
+
+	if (arg[0] != '-')
+		return 0;
+
+	for (i = 0; i < count; i++) {
+		if (arg[1] == '-') {
+			if (!prefixcmp(options[i].long_name, arg + 2)) {
+				if (options[i].type != OPTION_BOOLEAN) {
+					value = argv[1];
+					processed = 2;
+				} else {
+					processed = 1;
+				}
+				break;
+			}
+
+			eq = strchr(arg + 2, '=');
+			if (eq && options[i].type != OPTION_BOOLEAN &&
+			    !strncmp(arg + 2,
+				     options[i].long_name, eq - arg - 2)) {
+				value = eq + 1;
+				processed = 1;
+				break;
+			}
+		}
+
+		if (arg[1] == options[i].short_name) {
+			if (arg[2] == '\0') {
+				if (options[i].type != OPTION_BOOLEAN) {
+					value = argv[1];
+					processed = 2;
+				} else {
+					processed = 1;
+				}
+				break;
+			}
+
+			if (options[i].type != OPTION_BOOLEAN) {
+				value = arg + 2;
+				processed = 1;
+				break;
+			}
+		}
+	}
+
+	if (i == count)
+		usage(usage_string);
+	else switch (options[i].type) {
+	case OPTION_BOOLEAN:
+		(*(int *)options[i].value)++;
+		break;
+	case OPTION_STRING:
+		if (value == NULL) {
+			error("option %s requires a value.", arg);
+			usage(usage_string);
+		}
+		*(const char **)options[i].value = value;
+		break;
+	case OPTION_INTEGER:
+		if (value == NULL) {
+			error("option %s requires a value.", argv);
+			usage(usage_string);
+		}
+		*(int *)options[i].value = atoi(value);
+		break;
+	default:
+		assert(0);
+	}
+
+	return processed;
+}
+
+int parse_options(int argc, const char **argv,
+		  struct option *options, int count,
+		  const char *usage_string)
+{
+	int i, j, processed;
+
+	for (i = 1, j = 0; i < argc; ) {
+		if (!strcmp(argv[i], "--"))
+			break;
+		processed = parse_one(argv + i, options, count, usage_string);
+		if (processed == 0)
+			argv[j++] = argv[i++];
+		else
+			i += processed;
+	}
+
+	while (i < argc)
+		argv[j++] = argv[i++];
+	argv[j] = NULL;
+
+	return j;
+}
diff --git a/parse-options.h b/parse-options.h
new file mode 100644
index 0000000..5be9c20
--- /dev/null
+++ b/parse-options.h
@@ -0,0 +1,33 @@
+#ifndef PARSE_OPTIONS_H
+#define PARSE_OPTIONS_H
+
+enum option_type {
+	OPTION_BOOLEAN,
+	OPTION_STRING,
+	OPTION_INTEGER,
+	OPTION_LAST,
+};
+
+struct option {
+	enum option_type type;
+	const char *long_name;
+	char short_name;
+	void *value;
+};
+
+/* Parse the given options against the list of known options.  The
+ * order of the option structs matters, in that ambiguous
+ * abbreviations (eg, --in could be short for --include or
+ * --interactive) are matched by the first option that share the
+ * prefix.
+ *
+ * parse_options() will filter out the processed options and leave the
+ * non-option argments in argv[].  The return value is the number of
+ * arguments left in argv[].
+ */
+
+extern int parse_options(int argc, const char **argv,
+			 struct option *options, int count,
+			 const char *usage_string);
+
+#endif
-- 
1.5.2.5
Kristian Høgsberg· Oct 3, 2007, 21:45 UTC · re: Kristian Høgsberg · lore
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 builtin-add.c |   64 ++++++++++++++++++--------------------------------------
 1 files changed, 21 insertions(+), 43 deletions(-)
Show changes to builtin-add.c +21 −43
diff --git a/builtin-add.c b/builtin-add.c
index 966e145..66fd99d 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -13,6 +13,7 @@
 #include "commit.h"
 #include "revision.h"
 #include "run-command.h"
+#include "parse-options.h"
 
 static const char builtin_add_usage[] =
 "git-add [-n] [-v] [-f] [--interactive | -i] [-u] [--refresh] [--] <filepattern>...";
@@ -160,21 +161,30 @@ static struct lock_file lock_file;
 static const char ignore_error[] =
 "The following paths are ignored by one of your .gitignore files:\n";
 
+static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
+static int add_interactive = 0;
+
+static struct option builtin_add_options[] = {
+	{ OPTION_BOOLEAN, "interactive", 'i', &add_interactive },
+	{ OPTION_BOOLEAN, NULL, 'n', &show_only },
+	{ OPTION_BOOLEAN, NULL, 'f', &ignored_too },
+	{ OPTION_BOOLEAN, NULL, 'v',&verbose },
+	{ OPTION_BOOLEAN, NULL, 'u',&take_worktree_changes },
+	{ OPTION_BOOLEAN, "refresh", 0, &refresh_only }
+};
+
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int i, newfd;
-	int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
 	const char **pathspec;
 	struct dir_struct dir;
-	int add_interactive = 0;
 
-	for (i = 1; i < argc; i++) {
-		if (!strcmp("--interactive", argv[i]) ||
-		    !strcmp("-i", argv[i]))
-			add_interactive++;
-	}
+	i = parse_options(argc, argv, builtin_add_options,
+			  ARRAY_SIZE(builtin_add_options),
+			  builtin_add_usage);
+
 	if (add_interactive) {
-		if (argc != 2)
+		if (i > 0)
 			die("add --interactive does not take any parameters");
 		exit(interactive_add());
 	}
@@ -183,51 +193,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	newfd = hold_locked_index(&lock_file, 1);
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (arg[0] != '-')
-			break;
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(arg, "-n")) {
-			show_only = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-f")) {
-			ignored_too = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-v")) {
-			verbose = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u")) {
-			take_worktree_changes = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--refresh")) {
-			refresh_only = 1;
-			continue;
-		}
-		usage(builtin_add_usage);
-	}
-
 	if (take_worktree_changes) {
 		if (read_cache() < 0)
 			die("index file corrupt");
-		add_files_to_cache(verbose, prefix, argv + i);
+		add_files_to_cache(verbose, prefix, argv);
 		goto finish;
 	}
 
-	if (argc <= i) {
+	if (i == 0) {
 		fprintf(stderr, "Nothing specified, nothing added.\n");
 		fprintf(stderr, "Maybe you wanted to say 'git add .'?\n");
 		return 0;
 	}
-	pathspec = get_pathspec(prefix, argv + i);
+	pathspec = get_pathspec(prefix, argv);
 
 	if (refresh_only) {
 		refresh(verbose, pathspec);
-- 
1.5.2.5
Pierre Habouzit· Oct 3, 2007, 23:11 UTC · re: Kristian Høgsberg · lore

Re: [PATCH] Add a simple option parser.

On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:
Show 7 quoted lines
> The option parser takes argc, argv, an array of struct option
> and a usage string.  Each of the struct option elements in the array
> describes a valid option, its type and a pointer to the location where the
> value is written.  The entry point is parse_options(), which scans through
> the given argv, and matches each option there against the list of valid
> options.  During the scan, argv is rewritten to only contain the
> non-option command line arguments and the number of these is returned.
  if we are going in that direction (and I believe it's a good one), we
should be sure that the model fits with other commands as well. And as I
said on IRC, I believe the most "horrible" (as in complex) option parser
in git is the one from git-grep.
  A migration of git-grep on that API should be tried first. If this
works well enough, I believe that the rest of the git commands will be
migrated easily enough. (with maybe small addition to parse-option.[hc]
but the hardcore things should have been met with git-grep already I
think).
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Kristian Høgsberg· Oct 4, 2007, 14:57 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Add a simple option parser.

On Thu, 2007-10-04 at 01:11 +0200, Pierre Habouzit wrote:
Show 19 quoted lines
> On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:
> > The option parser takes argc, argv, an array of struct option
> > and a usage string.  Each of the struct option elements in the array
> > describes a valid option, its type and a pointer to the location where the
> > value is written.  The entry point is parse_options(), which scans through
> > the given argv, and matches each option there against the list of valid
> > options.  During the scan, argv is rewritten to only contain the
> > non-option command line arguments and the number of these is returned.
> 
>   if we are going in that direction (and I believe it's a good one), we
> should be sure that the model fits with other commands as well. And as I
> said on IRC, I believe the most "horrible" (as in complex) option parser
> in git is the one from git-grep.
> 
>   A migration of git-grep on that API should be tried first. If this
> works well enough, I believe that the rest of the git commands will be
> migrated easily enough. (with maybe small addition to parse-option.[hc]
> but the hardcore things should have been met with git-grep already I
> think).

I'm not sure - we can go with the current proposal and add new options types and probably the callback option type I suggested as we go. I don't want to block builtin-commit on figuring out what the perfect option parser should look like and what I sent out earlier work for commit. I think the way you handled the strbuf rewrites worked pretty well; extending and rewriting the API as you put it to use in more and more places. We can do the same thing with parse_options().

cheers, Kristian

Pierre Habouzit· Oct 4, 2007, 15:15 UTC · re: Kristian Høgsberg · lore

Re: [PATCH] Add a simple option parser.

On Thu, Oct 04, 2007 at 02:57:58PM +0000, Kristian Høgsberg wrote:
Show 29 quoted lines
> 
> On Thu, 2007-10-04 at 01:11 +0200, Pierre Habouzit wrote:
> > On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:
> > > The option parser takes argc, argv, an array of struct option
> > > and a usage string.  Each of the struct option elements in the array
> > > describes a valid option, its type and a pointer to the location where the
> > > value is written.  The entry point is parse_options(), which scans through
> > > the given argv, and matches each option there against the list of valid
> > > options.  During the scan, argv is rewritten to only contain the
> > > non-option command line arguments and the number of these is returned.
> > 
> >   if we are going in that direction (and I believe it's a good one), we
> > should be sure that the model fits with other commands as well. And as I
> > said on IRC, I believe the most "horrible" (as in complex) option parser
> > in git is the one from git-grep.
> > 
> >   A migration of git-grep on that API should be tried first. If this
> > works well enough, I believe that the rest of the git commands will be
> > migrated easily enough. (with maybe small addition to parse-option.[hc]
> > but the hardcore things should have been met with git-grep already I
> > think).
> 
> I'm not sure - we can go with the current proposal and add new options
> types and probably the callback option type I suggested as we go.  I
> don't want to block builtin-commit on figuring out what the perfect
> option parser should look like and what I sent out earlier work for
> commit.  I think the way you handled the strbuf rewrites worked pretty
> well; extending and rewriting the API as you put it to use in more and
> more places.  We can do the same thing with parse_options().
  Of course we can do that, or junio said that some people talked about
popt some time ago. I understand that you don't want to block the
git-commit work, but doing things right from the beginning is often a
big win on the long term.
  I don't know popt, and I don't know if it has sufficient expressivity.
For sure I don't like getopt_long APIs at all, so if popt is as
cumbersome, rolling our own based on the current parse_options you
propose is probably a good choice.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Pierre Habouzit· Oct 4, 2007, 16:31 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Add a simple option parser.

On jeu, oct 04, 2007 at 03:15:32 +0000, Pierre Habouzit wrote:
Show 18 quoted lines
> On Thu, Oct 04, 2007 at 02:57:58PM +0000, Kristian Høgsberg wrote:
> > I'm not sure - we can go with the current proposal and add new options
> > types and probably the callback option type I suggested as we go.  I
> > don't want to block builtin-commit on figuring out what the perfect
> > option parser should look like and what I sent out earlier work for
> > commit.  I think the way you handled the strbuf rewrites worked pretty
> > well; extending and rewriting the API as you put it to use in more and
> > more places.  We can do the same thing with parse_options().
> 
>   Of course we can do that, or junio said that some people talked about
> popt some time ago. I understand that you don't want to block the
> git-commit work, but doing things right from the beginning is often a
> big win on the long term.
> 
>   I don't know popt, and I don't know if it has sufficient expressivity.
> For sure I don't like getopt_long APIs at all, so if popt is as
> cumbersome, rolling our own based on the current parse_options you
> propose is probably a good choice.
  Okay, popt seems to be quite complicated, and depends upon gettext
(which we may require as per survey results, but right now it seems a
useless dependency). Don't get me wrong, I'm sure it's very powerful,
but again, I believe we can have a 200 line ad-hoc module that fits what
git really needs, the less cumbersome way.
  So well, I'd be (I'm not in position to decide anything btw ;p) in
favor of pursuing the work into git-commit like you did, and ASAP it
gets merged into next, I'm definitely willing to pursue a refactoring to
use it (now that strbufs seems to have been used where needed).
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Johannes Schindelin· Oct 4, 2007, 16:39 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Add a simple option parser.

Hi,
On Thu, 4 Oct 2007, Pierre Habouzit wrote:
>   Okay, popt seems to be quite complicated, and depends upon gettext
... which makes me vote against popt ...
> (which we may require as per survey results, but right now it seems a
> useless dependency).

Nope. git-gui got a script doing the job of msgfmt, which was the only part of that beast known as gettext anyway.

So we will not require it for git-gui.
And I do not see core git being i18n'ised.  Ever.

Ciao, Dscho

Pierre Habouzit· Oct 5, 2007, 10:08 UTC · re: Kristian Høgsberg · lore

Re: [PATCH] Add a simple option parser.

On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:
Show 6 quoted lines
> +static int parse_one(const char **argv,
> +		     struct option *options, int count,
> +		     const char *usage_string)
> +{
> +	const char *eq, *arg, *value;
> +	int i, processed;
  gcc complains processed could be returned without being initialized
first, so should be processed = 0; Even if it cannot occurs, it avoid
raising eyebrows.
> +	case OPTION_INTEGER:
> +		if (value == NULL) {
> +			error("option %s requires a value.", argv);
                                                             ^^^
                                           should probably be arg.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Pierre Habouzit· Oct 5, 2007, 14:21 UTC · re: Kristian Høgsberg · lore

Re: [PATCH] Add a simple option parser.

Show 5 quoted lines
> +/* Parse the given options against the list of known options.  The
> + * order of the option structs matters, in that ambiguous
> + * abbreviations (eg, --in could be short for --include or
> + * --interactive) are matched by the first option that share the
> + * prefix.
  Do we really want that ?
  I do believe that it's a very bad idea, as it silently breaks. Most of
the command line switches people need to use have a short form, or their
shell will complete it properly.
  A very interesting feature though, would be to finally be able to
parse aggregated switches (`git rm -rf` anyone ?).
  I also believe that it's a pity that parse_options isn't able to
generate the usage by itself. But we can add that later.
  I've though an alternate proposal, based on your work, for the first
patch.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Pierre Habouzit· Oct 5, 2007, 14:25 UTC · re: Pierre Habouzit · lore

[ALTERNATE PATCH] Add a simple option parser.

The option parser takes argc, argv, an array of struct option and a usage string. Each of the struct option elements in the array describes a valid option, its type and a pointer to the location where the value is written. The entry point is parse_options(), which scans through the given argv, and matches each option there against the list of valid options. During the scan, argv is rewritten to only contain the non-option command line arguments and the number of these is returned.

Aggregation of single switches is allowed:
  -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
Boolean switches automatically support the option with the same name,
prefixed with 'no-' to disable the switch:
  --no-color / --color only need to have an entry for "color".
Long options are supported either with '=' or without:
  --some-option=foo is the same as --some-option foo
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

I'm sorry about the "From" I don't intend to "steal" the patch in any sense, it's just an alternate proposal.

oh and I don't grok what OPTION_LAST is for, so I left it apart, but it seems unused ?

 Makefile        |    2 +-
 parse-options.c |  154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 parse-options.h |   29 ++++++++++
 3 files changed, 184 insertions(+), 1 deletions(-)
 create mode 100644 parse-options.c
 create mode 100644 parse-options.h
Show changes to 3 files +184 −1

Makefile, parse-options.c, parse-options.h

diff --git a/Makefile b/Makefile
index 62bdac6..d90e959 100644
--- a/Makefile
+++ b/Makefile
@@ -310,7 +310,7 @@ LIB_OBJS = \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
 	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
-	transport.o bundle.o
+	transport.o bundle.o parse-options.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/parse-options.c b/parse-options.c
new file mode 100644
index 0000000..eb3ff40
--- /dev/null
+++ b/parse-options.c
@@ -0,0 +1,154 @@
+#include "git-compat-util.h"
+#include "parse-options.h"
+
+struct optparse_t {
+	const char **argv;
+	int argc;
+	const char *opt;
+};
+
+static inline const char *skippfx(const char *str, const char *prefix)
+{
+	size_t len = strlen(prefix);
+	return strncmp(str, prefix, len) ? NULL : str + len;
+}
+
+static int opterror(struct option *opt, const char *reason, int shorterr)
+{
+	if (shorterr) {
+		return error("switch `%c' %s", opt->short_name, reason);
+	} else {
+		return error("option `%s' %s", opt->long_name, reason);
+	}
+}
+
+static int get_value(struct optparse_t *p, struct option *opt,
+					 int boolean, int shorterr)
+{
+	switch (opt->type) {
+		const char *s;
+		int v;
+
+	  case OPTION_BOOLEAN:
+		*(int *)opt->value = boolean;
+		return 0;
+
+	  case OPTION_STRING:
+		if (p->opt && *p->opt) {
+			*(const char **)opt->value = p->opt;
+			p->opt = NULL;
+		} else {
+			if (p->argc < 1)
+				return opterror(opt, "requires a value", shorterr);
+			*(const char **)opt->value = *++p->argv;
+			p->argc--;
+		}
+		return 0;
+
+	  case OPTION_INTEGER:
+		if (p->opt && *p->opt) {
+			v = strtol(p->opt, (char **)&s, 10);
+			p->opt = NULL;
+		} else {
+			if (p->argc < 1)
+				return opterror(opt, "requires a value", shorterr);
+			v = strtol(*++p->argv, (char **)&s, 10);
+			p->argc--;
+		}
+		if (*s)
+			return opterror(opt, "expects a numerical value", shorterr);
+		*(int *)opt->value = v;
+		return 0;
+	}
+
+	abort();
+}
+
+static int parse_short_opt(struct optparse_t *p, struct option *options, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		if (options[i].short_name == *p->opt) {
+			p->opt++;
+			return get_value(p, options + i, 1, 1);
+		}
+	}
+	return error("unknown switch `%c'", *p->opt);
+}
+
+static int parse_long_opt(struct optparse_t *p, const char *arg,
+                          struct option *options, int count)
+{
+	int boolean = 1;
+	int i;
+
+	for (i = 0; i < count; i++) {
+		const char *rest;
+		
+		if (!options[i].long_name)
+			continue;
+
+		rest = skippfx(arg, options[i].long_name);
+		if (!rest && options[i].type == OPTION_BOOLEAN) {
+			if (!rest && skippfx(arg, "no-")) {
+				rest = skippfx(arg + 3, options[i].long_name);
+				boolean = 0;
+			}
+			if (rest && *rest == '=')
+				return opterror(options + i, "takes no value", 0);
+		}
+		if (!rest || (*rest && *rest != '='))
+			continue;
+		if (*rest) {
+			p->opt = rest;
+		}
+		return get_value(p, options + i, boolean, 0);
+	}
+	return error("unknown option `%s'", arg);
+}
+
+int parse_options(int argc, const char **argv,
+		  struct option *options, int count,
+		  const char *usage_string)
+{
+	struct optparse_t optp = { argv + 1, argc - 1, NULL };
+	int j = 0;
+
+	while (optp.argc) {
+		const char *arg = optp.argv[0];
+
+		if (*arg != '-' || !arg[1]) {
+			argv[j++] = *optp.argv++;
+			optp.argc--;
+			continue;
+		}
+
+		if (arg[1] != '-') {
+			optp.opt = arg + 1;
+			while (*optp.opt) {
+				if (parse_short_opt(&optp, options, count) < 0) {
+					usage(usage_string);
+					return -1;
+				}
+			}
+			optp.argc--;
+			optp.argv++;
+			continue;
+		}
+
+		if (!arg[2]) /* "--" */
+			break;
+
+		if (parse_long_opt(&optp, arg + 2, options, count)) {
+			usage(usage_string);
+			return -1;
+		}
+		optp.argc--;
+		optp.argv++;
+	}
+
+	memmove(argv + j, optp.argv, optp.argc * sizeof(argv));
+	argv[j + optp.argc] = NULL;
+	return j + optp.argc;
+}
diff --git a/parse-options.h b/parse-options.h
new file mode 100644
index 0000000..e4749d0
--- /dev/null
+++ b/parse-options.h
@@ -0,0 +1,29 @@
+#ifndef PARSE_OPTIONS_H
+#define PARSE_OPTIONS_H
+
+enum option_type {
+	OPTION_BOOLEAN,
+	OPTION_STRING,
+	OPTION_INTEGER,
+#if 0
+	OPTION_LAST,
+#endif
+};
+
+struct option {
+	enum option_type type;
+	const char *long_name;
+	char short_name;
+	void *value;
+};
+
+/* parse_options() will filter out the processed options and leave the
+ * non-option argments in argv[].  The return value is the number of
+ * arguments left in argv[].
+ */
+
+extern int parse_options(int argc, const char **argv,
+			 struct option *options, int count,
+			 const char *usage_string);
+
+#endif
-- 
1.5.3.4.1156.ga72f9d-dirty
Mike Hommey· Oct 5, 2007, 14:30 UTC · re: Pierre Habouzit · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
Show 10 quoted lines
> The option parser takes argc, argv, an array of struct option
> and a usage string.  Each of the struct option elements in the array
> describes a valid option, its type and a pointer to the location where the
> value is written.  The entry point is parse_options(), which scans through
> the given argv, and matches each option there against the list of valid
> options.  During the scan, argv is rewritten to only contain the
> non-option command line arguments and the number of these is returned.
> 
> Aggregation of single switches is allowed:
>   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).

I like options aggregation, but I'm not sure aggregating option arguments is a good idea... I can't even think of an application that does it.

Mike
Pierre Habouzit· Oct 5, 2007, 14:45 UTC · re: Mike Hommey · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

On Fri, Oct 05, 2007 at 02:30:14PM +0000, Mike Hommey wrote:
Show 14 quoted lines
> On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
> > The option parser takes argc, argv, an array of struct option
> > and a usage string.  Each of the struct option elements in the array
> > describes a valid option, its type and a pointer to the location where the
> > value is written.  The entry point is parse_options(), which scans through
> > the given argv, and matches each option there against the list of valid
> > options.  During the scan, argv is rewritten to only contain the
> > non-option command line arguments and the number of these is returned.
> > 
> > Aggregation of single switches is allowed:
> >   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> 
> I like options aggregation, but I'm not sure aggregating option arguments
> is a good idea... I can't even think of an application that does it.
  You mean like `grep -A1` or `diff -u3` or `ls -w10` ?

getopt does that by default as well, so you may not have aware of it, but it's how things work in your system already.

  btw `ls -rw10` works, though `ls -w10r` drops the 'r' silently. FWIW I
don't, in that case, the alternate patch I propose complains about "10r"
not being a valid integer, and that's because unlike getopt, the patch
krh proposed knows what an integer is ;)
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Medve Emilian-EMMEDVE1· Oct 5, 2007, 15:45 UTC · re: Pierre Habouzit · lore

RE: [ALTERNATE PATCH] Add a simple option parser.

Hi,
You probably already considered and rejected the GNU argp parser. I used it before and I'd like to know reasons I should stay away from it.

Cheers, Emil.

Show 47 quoted lines
> -----Original Message-----
> From: git-owner@vger.kernel.org 
> [mailto:git-owner@vger.kernel.org] On Behalf Of Pierre Habouzit
> Sent: Friday, October 05, 2007 9:46 AM
> To: Mike Hommey
> Cc: Kristian Høgsberg; git@vger.kernel.org; Junio C Hamano
> Subject: Re: [ALTERNATE PATCH] Add a simple option parser.
> 
> On Fri, Oct 05, 2007 at 02:30:14PM +0000, Mike Hommey wrote:
> > On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit 
> <madcoder@debian.org> wrote:
> > > The option parser takes argc, argv, an array of struct option
> > > and a usage string.  Each of the struct option elements 
> in the array
> > > describes a valid option, its type and a pointer to the 
> location where the
> > > value is written.  The entry point is parse_options(), 
> which scans through
> > > the given argv, and matches each option there against the 
> list of valid
> > > options.  During the scan, argv is rewritten to only contain the
> > > non-option command line arguments and the number of these 
> is returned.
> > > 
> > > Aggregation of single switches is allowed:
> > >   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> > 
> > I like options aggregation, but I'm not sure aggregating 
> option arguments
> > is a good idea... I can't even think of an application that does it.
> 
>   You mean like `grep -A1` or `diff -u3` or `ls -w10` ?
> 
> getopt does that by default as well, so you may not have aware of it,
> but it's how things work in your system already.
> 
>   btw `ls -rw10` works, though `ls -w10r` drops the 'r' 
> silently. FWIW I
> don't, in that case, the alternate patch I propose complains 
> about "10r"
> not being a valid integer, and that's because unlike getopt, the patch
> krh proposed knows what an integer is ;)
> -- 
> ·O·  Pierre Habouzit
> ··O                                                madcoder@debian.org
> OOO                                                
> http://www.madism.org
Pierre Habouzit· Oct 5, 2007, 15:56 UTC · re: Medve Emilian-EMMEDVE1 · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

On ven, oct 05, 2007 at 03:45:36 +0000, Medve Emilian-EMMEDVE1 wrote:
> You probably already considered and rejected the GNU argp parser. I
> used it before and I'd like to know reasons I should stay away from
> it.
  Because it's GNU and that it's a heavy dependency to begin with.
Moreover, getopt_long doesn't deal with argument types (like integers).
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Medve Emilian-EMMEDVE1· Oct 5, 2007, 16:10 UTC · re: Pierre Habouzit · lore

RE: [ALTERNATE PATCH] Add a simple option parser.

Hi Pierre,
Show 14 quoted lines
> -----Original Message-----
> From: Pierre Habouzit [mailto:madcoder@debian.org] 
> Sent: Friday, October 05, 2007 10:57 AM
> To: Medve Emilian-EMMEDVE1
> Cc: Mike Hommey; Kristian Høgsberg; git@vger.kernel.org; 
> Junio C Hamano
> Subject: Re: [ALTERNATE PATCH] Add a simple option parser.
> 
> On ven, oct 05, 2007 at 03:45:36 +0000, Medve Emilian-EMMEDVE1 wrote:
> > You probably already considered and rejected the GNU argp parser. I
> > used it before and I'd like to know reasons I should stay away from
> > it.
> 
>   Because it's GNU and that it's a heavy dependency to begin with.
So it's more of a political decision then a technical one?
> Moreover, getopt_long doesn't deal with argument types (like 
> integers).
AFAIK, getopt_long in not argp.

Cheers, Emil.

David Kastrup· Oct 5, 2007, 16:20 UTC · re: Medve Emilian-EMMEDVE1 · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

"Medve Emilian-EMMEDVE1" <Emilian.Medve@freescale.com> writes:
Show 18 quoted lines
> Hi Pierre,
>
>> -----Original Message-----
>> From: Pierre Habouzit [mailto:madcoder@debian.org] 
>> Sent: Friday, October 05, 2007 10:57 AM
>> To: Medve Emilian-EMMEDVE1
>> Cc: Mike Hommey; Kristian Høgsberg; git@vger.kernel.org; 
>> Junio C Hamano
>> Subject: Re: [ALTERNATE PATCH] Add a simple option parser.
>> 
>> On ven, oct 05, 2007 at 03:45:36 +0000, Medve Emilian-EMMEDVE1 wrote:
>> > You probably already considered and rejected the GNU argp parser. I
>> > used it before and I'd like to know reasons I should stay away from
>> > it.
>> 
>>   Because it's GNU and that it's a heavy dependency to begin with.
>
> So it's more of a political decision then a technical one?

Well, if it is GNU then it is likely to mean GPLv3 (or GPLv3+) at some point of time, though it should certainly be possible for now to still secure a v2-licensed version (either GPL or LGPL).

GNU also means a different coding and indentation style.

Personally, I couldn't care less about both points (I prefer the GNU coding style anyway), but that's for the maintainer to decide, and one also has to take into account the effect on developer motivation.

And the typical git developer AFAICT prefers to consider themselves as unaligned with GNU and the FSF as much as possible.

-- 
David Kastrup
Pierre Habouzit· Oct 5, 2007, 16:38 UTC · re: David Kastrup · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

On Fri, Oct 05, 2007 at 04:20:33PM +0000, David Kastrup wrote:
Show 24 quoted lines
> "Medve Emilian-EMMEDVE1" <Emilian.Medve@freescale.com> writes:
> 
> > Hi Pierre,
> >
> >> -----Original Message-----
> >> From: Pierre Habouzit [mailto:madcoder@debian.org] 
> >> Sent: Friday, October 05, 2007 10:57 AM
> >> To: Medve Emilian-EMMEDVE1
> >> Cc: Mike Hommey; Kristian Høgsberg; git@vger.kernel.org; 
> >> Junio C Hamano
> >> Subject: Re: [ALTERNATE PATCH] Add a simple option parser.
> >> 
> >> On ven, oct 05, 2007 at 03:45:36 +0000, Medve Emilian-EMMEDVE1 wrote:
> >> > You probably already considered and rejected the GNU argp parser. I
> >> > used it before and I'd like to know reasons I should stay away from
> >> > it.
> >> 
> >>   Because it's GNU and that it's a heavy dependency to begin with.
> >
> > So it's more of a political decision then a technical one?
> 
> Well, if it is GNU then it is likely to mean GPLv3 (or GPLv3+) at some
> point of time, though it should certainly be possible for now to still
> secure a v2-licensed version (either GPL or LGPL).
  That is an issue indeed.
> And the typical git developer AFAICT prefers to consider themselves as
> unaligned with GNU and the FSF as much as possible.
  And is nothing near reality in my case.
  The real issue is dependency and bloat. getopt_long would need the GNU
implementation, That I believe depends upon gettext, and argp is just
bloated, and I'm not even sure it's distributed outside from the glibc
anyways.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Sven Verdoolaege· Oct 6, 2007, 08:46 UTC · re: Pierre Habouzit · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

On Fri, Oct 05, 2007 at 06:38:46PM +0200, Pierre Habouzit wrote:
>   The real issue is dependency and bloat. getopt_long would need the GNU
> implementation, That I believe depends upon gettext, and argp is just
> bloated, and I'm not even sure it's distributed outside from the glibc
> anyways.

It's part of gnulib (http://savannah.gnu.org/git/?group=gnulib). Including argp (and all its dependencies) is as easy as running some gnulib command. It does have some bloat, but for my project it was definitely more convenient to include it than to write my own parser and the bloat is still acceptable I suppose...

bash-3.00$ du lib m4 572 lib 208 m4 bash-3.00$ du -s . 20348 .

(In a source tree without the git repo.)
skimo
Linus Torvalds· Oct 5, 2007, 16:28 UTC · re: Medve Emilian-EMMEDVE1 · lore

RE: [ALTERNATE PATCH] Add a simple option parser.

On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:
> > 
> >   Because it's GNU and that it's a heavy dependency to begin with.
> 
> So it's more of a political decision then a technical one?

I'd *strongly* argue against new dependencies unless they buy us something major.

We've been good at cutting them down, including any required libraries internally. We shouldn't add new ones.

So we'd have to include GNU getopt sources with the git tree, at which point any advantage would be gone. Might as well include a private and simpler version of our own.

		Linus
Medve Emilian-EMMEDVE1· Oct 5, 2007, 16:41 UTC · re: Linus Torvalds · lore

RE: [ALTERNATE PATCH] Add a simple option parser.

Hello Linus,
Show 18 quoted lines
> On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:
> > > 
> > >   Because it's GNU and that it's a heavy dependency to begin with.
> > 
> > So it's more of a political decision then a technical one?
> 
> I'd *strongly* argue against new dependencies unless they buy us 
> something major.
> 
> We've been good at cutting them down, including any required 
> libraries 
> internally. We shouldn't add new ones.
> 
> So we'd have to include GNU getopt sources with the git tree, 
> at which 
> point any advantage would be gone. Might as well include a 
> private and 
> simpler version of our own.
>From what I understand argp is part of glibc.

Cheers, Emil.

Pierre Habouzit· Oct 5, 2007, 16:49 UTC · re: Medve Emilian-EMMEDVE1 · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

On Fri, Oct 05, 2007 at 04:41:48PM +0000, Medve Emilian-EMMEDVE1 wrote:
Show 21 quoted lines
> Hello Linus,
> 
> 
> > On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:
> > > > 
> > > >   Because it's GNU and that it's a heavy dependency to begin with.
> > > 
> > > So it's more of a political decision then a technical one?
> > 
> > I'd *strongly* argue against new dependencies unless they buy us
> > something major.
> > 
> > We've been good at cutting them down, including any required
> > libraries internally. We shouldn't add new ones.
> > 
> > So we'd have to include GNU getopt sources with the git tree, at
> > which point any advantage would be gone. Might as well include a
> > private and simpler version of our own.
> 
> 
> From what I understand argp is part of glibc.
  And of course requiring the glibc would be a big step forward for the
msys (or AIX, or HP-UX, or …) port !
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Linus Torvalds· Oct 5, 2007, 16:51 UTC · re: Medve Emilian-EMMEDVE1 · lore

RE: [ALTERNATE PATCH] Add a simple option parser.

On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:
> 
> From what I understand argp is part of glibc.

So are you arguing that we include all of glibc just to get git to be portable?

That's even worse.
		Linus
David Kastrup· Oct 5, 2007, 14:59 UTC · re: Mike Hommey · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

Mike Hommey <mh@glandium.org> writes:
Show 14 quoted lines
> On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
>> The option parser takes argc, argv, an array of struct option
>> and a usage string.  Each of the struct option elements in the array
>> describes a valid option, its type and a pointer to the location where the
>> value is written.  The entry point is parse_options(), which scans through
>> the given argv, and matches each option there against the list of valid
>> options.  During the scan, argv is rewritten to only contain the
>> non-option command line arguments and the number of these is returned.
>> 
>> Aggregation of single switches is allowed:
>>   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
>
> I like options aggregation, but I'm not sure aggregating option arguments
> is a good idea... I can't even think of an application that does it.

I think most allow this for the last option in a row. Tar is somewhat more perverse with its non-option command string:

tar xfzbv filename.tgz 40
uses filename.tgz as the option argument for "f" and 40 for "b".

Note that while tar accepts options instead of the initial command string,

tar -xfzbv filename.tgz 40
will _not_ work, while
tar -xffilename.tgz -z -b40 -v
presumably would (have no time to test this right now).
-- 
David Kastrup
Kristian Høgsberg· Oct 5, 2007, 15:33 UTC · re: Pierre Habouzit · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

On Fri, 2007-10-05 at 16:25 +0200, Pierre Habouzit wrote:
Show 17 quoted lines
> The option parser takes argc, argv, an array of struct option
> and a usage string.  Each of the struct option elements in the array
> describes a valid option, its type and a pointer to the location where the
> value is written.  The entry point is parse_options(), which scans through
> the given argv, and matches each option there against the list of valid
> options.  During the scan, argv is rewritten to only contain the
> non-option command line arguments and the number of these is returned.
> 
> Aggregation of single switches is allowed:
>   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> 
> Boolean switches automatically support the option with the same name,
> prefixed with 'no-' to disable the switch:
>   --no-color / --color only need to have an entry for "color".
> 
> Long options are supported either with '=' or without:
>   --some-option=foo is the same as --some-option foo

That looks great, works for me. One comment, though: it looks like you're not sure whether to call these things "options" or "switches". We should choose one and stick with it.

Acked-by: Kristian Høgsberg <krh@redhat.com>
Show 5 quoted lines
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> 
> I'm sorry about the "From" I don't intend to "steal" the patch in any
> sense, it's just an alternate proposal.
No worries, I'm glad to see this move forward.
> oh and I don't grok what OPTION_LAST is for, so I left it apart, but
> it seems unused ?

Oh, kill that. I used that as the option array terminator before we switched to ARRAY_SIZE().

Show 50 quoted lines
> 
>  Makefile        |    2 +-
>  parse-options.c |  154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  parse-options.h |   29 ++++++++++
>  3 files changed, 184 insertions(+), 1 deletions(-)
>  create mode 100644 parse-options.c
>  create mode 100644 parse-options.h
> 
> diff --git a/Makefile b/Makefile
> index 62bdac6..d90e959 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -310,7 +310,7 @@ LIB_OBJS = \
>  	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
>  	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
>  	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
> -	transport.o bundle.o
> +	transport.o bundle.o parse-options.o
>  
>  BUILTIN_OBJS = \
>  	builtin-add.o \
> diff --git a/parse-options.c b/parse-options.c
> new file mode 100644
> index 0000000..eb3ff40
> --- /dev/null
> +++ b/parse-options.c
> @@ -0,0 +1,154 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +
> +struct optparse_t {
> +	const char **argv;
> +	int argc;
> +	const char *opt;
> +};
> +
> +static inline const char *skippfx(const char *str, const char *prefix)
> +{
> +	size_t len = strlen(prefix);
> +	return strncmp(str, prefix, len) ? NULL : str + len;
> +}
> +
> +static int opterror(struct option *opt, const char *reason, int shorterr)
> +{
> +	if (shorterr) {
> +		return error("switch `%c' %s", opt->short_name, reason);
> +	} else {
> +		return error("option `%s' %s", opt->long_name, reason);
> +	}
> +}
option/switch?
Show 165 quoted lines
> +static int get_value(struct optparse_t *p, struct option *opt,
> +					 int boolean, int shorterr)
> +{
> +	switch (opt->type) {
> +		const char *s;
> +		int v;
> +
> +	  case OPTION_BOOLEAN:
> +		*(int *)opt->value = boolean;
> +		return 0;
> +
> +	  case OPTION_STRING:
> +		if (p->opt && *p->opt) {
> +			*(const char **)opt->value = p->opt;
> +			p->opt = NULL;
> +		} else {
> +			if (p->argc < 1)
> +				return opterror(opt, "requires a value", shorterr);
> +			*(const char **)opt->value = *++p->argv;
> +			p->argc--;
> +		}
> +		return 0;
> +
> +	  case OPTION_INTEGER:
> +		if (p->opt && *p->opt) {
> +			v = strtol(p->opt, (char **)&s, 10);
> +			p->opt = NULL;
> +		} else {
> +			if (p->argc < 1)
> +				return opterror(opt, "requires a value", shorterr);
> +			v = strtol(*++p->argv, (char **)&s, 10);
> +			p->argc--;
> +		}
> +		if (*s)
> +			return opterror(opt, "expects a numerical value", shorterr);
> +		*(int *)opt->value = v;
> +		return 0;
> +	}
> +
> +	abort();
> +}
> +
> +static int parse_short_opt(struct optparse_t *p, struct option *options, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		if (options[i].short_name == *p->opt) {
> +			p->opt++;
> +			return get_value(p, options + i, 1, 1);
> +		}
> +	}
> +	return error("unknown switch `%c'", *p->opt);
> +}
> +
> +static int parse_long_opt(struct optparse_t *p, const char *arg,
> +                          struct option *options, int count)
> +{
> +	int boolean = 1;
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		const char *rest;
> +		
> +		if (!options[i].long_name)
> +			continue;
> +
> +		rest = skippfx(arg, options[i].long_name);
> +		if (!rest && options[i].type == OPTION_BOOLEAN) {
> +			if (!rest && skippfx(arg, "no-")) {
> +				rest = skippfx(arg + 3, options[i].long_name);
> +				boolean = 0;
> +			}
> +			if (rest && *rest == '=')
> +				return opterror(options + i, "takes no value", 0);
> +		}
> +		if (!rest || (*rest && *rest != '='))
> +			continue;
> +		if (*rest) {
> +			p->opt = rest;
> +		}
> +		return get_value(p, options + i, boolean, 0);
> +	}
> +	return error("unknown option `%s'", arg);
> +}
> +
> +int parse_options(int argc, const char **argv,
> +		  struct option *options, int count,
> +		  const char *usage_string)
> +{
> +	struct optparse_t optp = { argv + 1, argc - 1, NULL };
> +	int j = 0;
> +
> +	while (optp.argc) {
> +		const char *arg = optp.argv[0];
> +
> +		if (*arg != '-' || !arg[1]) {
> +			argv[j++] = *optp.argv++;
> +			optp.argc--;
> +			continue;
> +		}
> +
> +		if (arg[1] != '-') {
> +			optp.opt = arg + 1;
> +			while (*optp.opt) {
> +				if (parse_short_opt(&optp, options, count) < 0) {
> +					usage(usage_string);
> +					return -1;
> +				}
> +			}
> +			optp.argc--;
> +			optp.argv++;
> +			continue;
> +		}
> +
> +		if (!arg[2]) /* "--" */
> +			break;
> +
> +		if (parse_long_opt(&optp, arg + 2, options, count)) {
> +			usage(usage_string);
> +			return -1;
> +		}
> +		optp.argc--;
> +		optp.argv++;
> +	}
> +
> +	memmove(argv + j, optp.argv, optp.argc * sizeof(argv));
> +	argv[j + optp.argc] = NULL;
> +	return j + optp.argc;
> +}
> diff --git a/parse-options.h b/parse-options.h
> new file mode 100644
> index 0000000..e4749d0
> --- /dev/null
> +++ b/parse-options.h
> @@ -0,0 +1,29 @@
> +#ifndef PARSE_OPTIONS_H
> +#define PARSE_OPTIONS_H
> +
> +enum option_type {
> +	OPTION_BOOLEAN,
> +	OPTION_STRING,
> +	OPTION_INTEGER,
> +#if 0
> +	OPTION_LAST,
> +#endif
> +};
> +
> +struct option {
> +	enum option_type type;
> +	const char *long_name;
> +	char short_name;
> +	void *value;
> +};
> +
> +/* parse_options() will filter out the processed options and leave the
> + * non-option argments in argv[].  The return value is the number of
> + * arguments left in argv[].
> + */
> +
> +extern int parse_options(int argc, const char **argv,
> +			 struct option *options, int count,
> +			 const char *usage_string);
> +
> +#endif
Pierre Habouzit· Oct 5, 2007, 15:54 UTC · re: Kristian Høgsberg · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

On Fri, Oct 05, 2007 at 03:33:44PM +0000, Kristian Høgsberg wrote:
Show 22 quoted lines
> On Fri, 2007-10-05 at 16:25 +0200, Pierre Habouzit wrote:
> > The option parser takes argc, argv, an array of struct option
> > and a usage string.  Each of the struct option elements in the array
> > describes a valid option, its type and a pointer to the location where the
> > value is written.  The entry point is parse_options(), which scans through
> > the given argv, and matches each option there against the list of valid
> > options.  During the scan, argv is rewritten to only contain the
> > non-option command line arguments and the number of these is returned.
> > 
> > Aggregation of single switches is allowed:
> >   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> > 
> > Boolean switches automatically support the option with the same name,
> > prefixed with 'no-' to disable the switch:
> >   --no-color / --color only need to have an entry for "color".
> > 
> > Long options are supported either with '=' or without:
> >   --some-option=foo is the same as --some-option foo
> 
> That looks great, works for me.  One comment, though: it looks like
> you're not sure whether to call these things "options" or "switches".
> We should choose one and stick with it.
  I use the word "switch" when it's a short_option, and "option" when
it's a long one. But maybe the distinction doesn't make sense, and it's
a non-native speaker glitch. I don't care that much btw.
Show 5 quoted lines
> > oh and I don't grok what OPTION_LAST is for, so I left it apart, but
> > it seems unused ?
>
> Oh, kill that.  I used that as the option array terminator before we
> switched to ARRAY_SIZE().
  Okay :)
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Pierre Habouzit· Oct 7, 2007, 17:01 UTC · re: Pierre Habouzit · lore

Re: [ALTERNATE PATCH] Add a simple option parser.

  FWIW this patch has some issues with long options parsing, I have a
fix, but am trying to migrate more builtins to this parser to see how
well it behaves.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

← back to recent threads