[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 <redacted>
---
 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

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

Signed-off-by: Kristian Høgsberg <redacted>
---
 builtin-add.c |   64 ++++++++++++++++++--------------------------------------
 1 files changed, 21 insertions(+), 43 deletions(-)

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

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).

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

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:

quoted

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

On Thu, Oct 04, 2007 at 02:57:58PM +0000, Kristian Høgsberg wrote:

On Thu, 2007-10-04 at 01:11 +0200, Pierre Habouzit wrote:

quoted

On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:

quoted

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

On jeu, oct 04, 2007 at 03:15:32 +0000, Pierre Habouzit wrote:

On Thu, Oct 04, 2007 at 02:57:58PM +0000, Kristian Høgsberg wrote:

quoted

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

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

On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:

+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

+/* 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

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 <redacted>
Signed-off-by: Pierre Habouzit <redacted>
---

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

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

On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit [off-list ref] 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.

Mike

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 [off-list ref] wrote:

quoted

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

Mike Hommey [off-list ref] writes:

On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit [off-list ref] wrote:

quoted

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

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.

-----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:

quoted

On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit 

[off-list ref] wrote:

quoted

quoted

The option parser takes argc, argv, an array of struct option
and a usage string.  Each of the struct option elements 

in the array

quoted

quoted

describes a valid option, its type and a pointer to the 

location where the

quoted

quoted

value is written.  The entry point is parse_options(), 

which scans through

quoted

quoted

the given argv, and matches each option there against the 

list of valid

quoted

quoted

options.  During the scan, argv is rewritten to only contain the
non-option command line arguments and the number of these 

is returned.

quoted

quoted

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

quoted

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

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.

Acked-by: Kristian Høgsberg <redacted>

Signed-off-by: Pierre Habouzit <redacted>
---

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().

option/switch?

On Fri, Oct 05, 2007 at 03:33:44PM +0000, Kristian Høgsberg wrote:

On Fri, 2007-10-05 at 16:25 +0200, Pierre Habouzit wrote:

quoted

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.

quoted

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

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

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:

quoted

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.

"Medve Emilian-EMMEDVE1" [off-list ref] writes:

Hi Pierre,

quoted

-----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:

quoted

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

On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:

quoted

  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

On Fri, Oct 05, 2007 at 04:20:33PM +0000, David Kastrup wrote:

"Medve Emilian-EMMEDVE1" [off-list ref] writes:

quoted

Hi Pierre,

quoted

-----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:

quoted

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

Hello Linus,

On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:

quoted

quoted

  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.

On Fri, Oct 05, 2007 at 04:41:48PM +0000, Medve Emilian-EMMEDVE1 wrote:

Hello Linus,

quoted

On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:

quoted

quoted

  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

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

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

  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