[PATCH] Add a simple option parser.

[PATCH] Add a simple option parser.

[Kristian Høgsberg]

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

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

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

[Kristian Høgsberg]

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 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

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a simple option parser.

[Kristian Høgsberg]

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

cheers,
Kristian

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2510 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a simple option parser.

[Johannes Schindelin]

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

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[ALTERNATE PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 6456 bytes --]

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

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


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Mike Hommey]

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.

Mike

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Medve Emilian-EMMEDVE1]

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

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Medve Emilian-EMMEDVE1]

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?

> Moreover, getopt_long doesn't deal with argument types (like 
> integers).

AFAIK, getopt_long in not argp.


Cheers,
Emil.

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

[David Kastrup]

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

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

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

On Fri, Oct 05, 2007 at 04:20:33PM +0000, David Kastrup wrote:
> "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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Sven Verdoolaege]

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

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

[Linus Torvalds]

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

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

[Medve Emilian-EMMEDVE1]

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.


Cheers,
Emil.

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

On Fri, Oct 05, 2007 at 04:41:48PM +0000, Medve Emilian-EMMEDVE1 wrote:
> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Linus Torvalds]

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

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

[David Kastrup]

Mike Hommey <mh@glandium.org> writes:

> 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

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

[Kristian Høgsberg]

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

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

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

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

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

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

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

  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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[RFC] CLI option parsing and usage generation for porcelains

[Pierre Habouzit]

  Following Kristian momentum, I've reworked his parse_option module
quite a lot, and now have some quite interesting features. The series is
available from git://git.madism.org/git.git (branch ph/strbuf).

  The following series is open for comments, it's not 100% ready for
inclusion IMHO, as some details may need to be sorted out first, and
that I've not re-read the patches thoroughly yet. Though I uses the tip
of that branch as my everyday git for 2 weeks or so without any
noticeable issues.

  And as examples are always easier to grok:

$ git fetch -h
usage: git-fetch [options] [<repository> <refspec>...]

  -q, --quiet           be quiet
  -v, --verbose         be verbose

  -a, --append          append in .git/FETCH_HEAD
  -f, --force           force non fast-forwards updates
  --no-tags             don't follow tags at all
  -t, --tags            fetch all tags
  --depth <depth>       deepen history of a shallow clone

Advanced Options
  -k, --keep            keep downloaded pack
  -u, --update-head-ok  allow to update the head in the current branch
  --upload-pack <path>  path to git-upload-pack on the remote

$ git rm -rf xdiff # yeah -rf now works !
rm 'xdiff/xdiff.h'
rm 'xdiff/xdiffi.c'
rm 'xdiff/xdiffi.h'
rm 'xdiff/xemit.c'
rm 'xdiff/xemit.h'
rm 'xdiff/xinclude.h'
rm 'xdiff/xmacros.h'
rm 'xdiff/xmerge.c'
rm 'xdiff/xprepare.c'
rm 'xdiff/xprepare.h'
rm 'xdiff/xtypes.h'
rm 'xdiff/xutils.c'
rm 'xdiff/xutils.h'

Re: [PATCH] Add a simple option parser.

[Johannes Schindelin]

Hi,

On Sat, 13 Oct 2007, Pierre Habouzit wrote:

> Aggregation of single switches is allowed:
>   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).

I'd be more interested in "-rC 0" working...  Is that supported, too?

> diff --git a/parse-options.c b/parse-options.c
> new file mode 100644
> index 0000000..07abb50
> --- /dev/null
> +++ b/parse-options.c
> @@ -0,0 +1,227 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +#include "strbuf.h"
> +
> +#define OPT_SHORT 1
> +#define OPT_UNSET 2
> +
> +struct optparse_t {
> +	const char **argv;
> +	int argc;
> +	const char *opt;
> +};
> +
> +static inline const char *get_arg(struct optparse_t *p)
> +{
> +	if (p->opt) {
> +		const char *res = p->opt;
> +		p->opt = NULL;
> +		return res;
> +	}
> +	p->argc--;
> +	return *++p->argv;
> +}

This is only used once; I wonder if it is really that more readable having 
this as a function in its own right.

> +static inline const char *skippfx(const char *str, const char *prefix)

Personally, I do not like abbreviations like that.  They do not save that 
much screen estate (skip_prefix is only 4 characters longer, but much more 
readable).  Same goes for "cnt" later.

> +static int get_value(struct optparse_t *p, struct option *opt, int flags)
> +{
> +	if (p->opt && (flags & OPT_UNSET))
> +		return opterror(opt, "takes no value", flags);
> +
> +	switch (opt->type) {
> +	case OPTION_BOOLEAN:
> +		if (!(flags & OPT_SHORT) && p->opt)
> +			return opterror(opt, "takes no value", flags);
> +		if (flags & OPT_UNSET) {
> +			*(int *)opt->value = 0;
> +		} else {
> +			(*(int *)opt->value)++;
> +		}
> +		return 0;
> +
> +	case OPTION_STRING:
> +		if (flags & OPT_UNSET) {
> +			*(const char **)opt->value = (const char *)NULL;
> +		} else {
> +			if (!p->opt && p->argc < 1)
> +				return opterror(opt, "requires a value", flags);
> +			*(const char **)opt->value = get_arg(p);
> +		}
> +		return 0;
> +
> +	case OPTION_INTEGER:
> +		if (flags & OPT_UNSET) {
> +			*(int *)opt->value = 0;
> +		} else {
> +			const char *s;
> +			if (!p->opt && p->argc < 1)
> +				return opterror(opt, "requires a value", flags);
> +			*(int *)opt->value = strtol(*p->argv, (char **)&s, 10);
> +			if (*s)
> +				return opterror(opt, "expects a numerical value", flags);
> +		}
> +		return 0;
> +
> +	default:
> +		die("should not happen, someone must be hit on the forehead");

:-P

> +static int parse_long_opt(struct optparse_t *p, const char *arg,
> +                          struct option *options, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		const char *rest;
> +		int flags = 0;
> +		
> +		if (!options[i].long_name)
> +			continue;
> +
> +		rest = skippfx(arg, options[i].long_name);
> +		if (!rest && !strncmp(arg, "no-", 3)) {
> +			rest = skippfx(arg + 3, options[i].long_name);
> +			flags |= OPT_SHORT;
> +		}

Would this not be more intuitive as

		if (!prefixcmp(arg, "no-")) {
			arg += 3;
			flags |= OPT_UNSET;
		}
		rest = skip_prefix(arg, options[i].long_name);

Hm?  (Note that I say UNSET, not SHORT... ;-)

> +		if (!rest)
> +			continue;
> +		if (*rest) {
> +			if (*rest != '=')
> +				continue;

Is this really no error?  For example, "git log 
--decorate-walls-and-roofs" would not fail...

> +int parse_options(int argc, const char **argv,
> +                  struct option *options, int count,
> +				  const char * const usagestr[], int flags)

Please indent by the same amount.

> +		if (arg[1] != '-') {
> +			optp.opt = arg + 1;
> +			do {
> +				if (*optp.opt == 'h')
> +					make_usage(usagestr, options, count);

How about calling this "usage_with_options()"?  With that name I expected 
make_usage() to return a strbuf.

> +		if (!arg[2]) { /* "--" */
> +			if (!(flags & OPT_COPY_DASHDASH))
> +				optp.argc--, optp.argv++;

I would prefer this as 

			if (!(flags & OPT_COPY_DASHDASH)) {
				optp.argc--;
				optp.argv++;
			}

While I'm at it: could you use "args" instead of "optp"?  It is misleading 
both in that it not only contains options (but other arguments, too) as in 
that it is not a pointer (the trailing "p" is used as an indicator of that 
very often, including git's source code).

In the same vein, OPT_COPY_DASHDASH should be named 
PARSE_OPT_KEEP_DASHDASH.

> +		if (opts->short_name) {
> +			strbuf_addf(&sb, "-%c", opts->short_name);
> +		}
> +		if (opts->long_name) {
> +			strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
> +						opts->long_name);
> +		}

Please lose the curly brackets.

> +		if (sb.len - pos <= USAGE_OPTS_WIDTH) {
> +			int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
> +			strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
> +		} else {
> +			strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> +						opts->help);
> +		}

Same here.  (And I'd also make sure that the lines are not that long.)

> diff --git a/parse-options.h b/parse-options.h
> new file mode 100644
> index 0000000..4b33d17
> --- /dev/null
> +++ b/parse-options.h
> @@ -0,0 +1,37 @@
> +#ifndef PARSE_OPTIONS_H
> +#define PARSE_OPTIONS_H
> +
> +enum option_type {
> +	OPTION_BOOLEAN,

I know that I proposed "BOOLEAN", but actually, you use it more like an 
"INCREMENTAL", right?

Other than that: I like it very much.

Ciao,
Dscho

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 4865 bytes --]

On Sat, Oct 13, 2007 at 02:39:10PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
> 
> > Aggregation of single switches is allowed:
> >   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> 
> I'd be more interested in "-rC 0" working...  Is that supported, too?

  yes it is.

> > +static inline const char *get_arg(struct optparse_t *p)
> > +{
> > +	if (p->opt) {
> > +		const char *res = p->opt;
> > +		p->opt = NULL;
> > +		return res;
> > +	}
> > +	p->argc--;
> > +	return *++p->argv;
> > +}
> 
> This is only used once; I wonder if it is really that more readable having 
> this as a function in its own right.

  it's used twice, and it makes the code more readable I believe.

> > +static inline const char *skippfx(const char *str, const char *prefix)
> 
> Personally, I do not like abbreviations like that.  They do not save that 
> much screen estate (skip_prefix is only 4 characters longer, but much more 
> readable).  Same goes for "cnt" later.

  Ack I'll fix that.

> > +static int parse_long_opt(struct optparse_t *p, const char *arg,
> > +                          struct option *options, int count)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		const char *rest;
> > +		int flags = 0;
> > +		
> > +		if (!options[i].long_name)
> > +			continue;
> > +
> > +		rest = skippfx(arg, options[i].long_name);
> > +		if (!rest && !strncmp(arg, "no-", 3)) {
> > +			rest = skippfx(arg + 3, options[i].long_name);
> > +			flags |= OPT_SHORT;
> > +		}
> 
> Would this not be more intuitive as
> 
> 		if (!prefixcmp(arg, "no-")) {
> 			arg += 3;
> 			flags |= OPT_UNSET;
> 		}
> 		rest = skip_prefix(arg, options[i].long_name);

  Yes, that can be done indeed, but the point is, we have sometimes
option whose long-name is "no-foo" (because it's what makes sense) but I
can rework that.

> Hm?  (Note that I say UNSET, not SHORT... ;-)

  fsck, good catch.

> > +		if (!rest)
> > +			continue;
> > +		if (*rest) {
> > +			if (*rest != '=')
> > +				continue;
> 
> Is this really no error?  For example, "git log 
> --decorate-walls-and-roofs" would not fail...

  it would be an error, it will yield a "option not found".

> > +int parse_options(int argc, const char **argv,
> > +                  struct option *options, int count,
> > +				  const char * const usagestr[], int flags)
> 
> Please indent by the same amount.

  oops, stupid space vs. tab thing.

> > +		if (arg[1] != '-') {
> > +			optp.opt = arg + 1;
> > +			do {
> > +				if (*optp.opt == 'h')
> > +					make_usage(usagestr, options, count);
> 
> How about calling this "usage_with_options()"?  With that name I expected 
> make_usage() to return a strbuf.

  will do.

> > +		if (!arg[2]) { /* "--" */
> > +			if (!(flags & OPT_COPY_DASHDASH))
> > +				optp.argc--, optp.argv++;
> 
> I would prefer this as 
> 
> 			if (!(flags & OPT_COPY_DASHDASH)) {
> 				optp.argc--;
> 				optp.argv++;
> 			}
> 
> While I'm at it: could you use "args" instead of "optp"?  It is misleading 
> both in that it not only contains options (but other arguments, too) as in 
> that it is not a pointer (the trailing "p" is used as an indicator of that 
> very often, including git's source code).

  okay.

> In the same vein, OPT_COPY_DASHDASH should be named 
> PARSE_OPT_KEEP_DASHDASH.

  okay.

> 
> > +		if (opts->short_name) {
> > +			strbuf_addf(&sb, "-%c", opts->short_name);
> > +		}
> > +		if (opts->long_name) {
> > +			strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
> > +						opts->long_name);
> > +		}
> 
> Please lose the curly brackets.
> 
> > +		if (sb.len - pos <= USAGE_OPTS_WIDTH) {
> > +			int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
> > +			strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
> > +		} else {
> > +			strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> > +						opts->help);
> > +		}
> 
> Same here.  (And I'd also make sure that the lines are not that long.)

  okay.

> 
> > diff --git a/parse-options.h b/parse-options.h
> > new file mode 100644
> > index 0000000..4b33d17
> > --- /dev/null
> > +++ b/parse-options.h
> > @@ -0,0 +1,37 @@
> > +#ifndef PARSE_OPTIONS_H
> > +#define PARSE_OPTIONS_H
> > +
> > +enum option_type {
> > +	OPTION_BOOLEAN,
> 
> I know that I proposed "BOOLEAN", but actually, you use it more like an 
> "INCREMENTAL", right?

  yes, I don't like _BOOLEAN either, I would have prefered _FLAG or sth
like that. INCREMENTAL is just too long.

> Other than that: I like it very much.

:P

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a simple option parser.

[Alex Riesen]

Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> +static int opterror(struct option *opt, const char *reason, int flags)

"const struct option *opt"? You never modify the struct option itself,
only the values under the pointers it contains. Using const here will
allow the compiler to reuse string constants (not that there will be
much of the opportunity, but anyway) in the option arrays.

> +static int get_value(struct optparse_t *p, struct option *opt, int flags)

"const struct option *opt"?

> +static int parse_short_opt(struct optparse_t *p, struct option *options, int count)

"const struct option *options"?

> +int parse_options(int argc, const char **argv,
> +                  struct option *options, int count,
> +				  const char * const usagestr[], int flags)

"const struct option *options"?

> +void make_usage(const char * const usagestr[], struct option *opts, int cnt)

"const struct option *opts"?

Why not "const char *const *usagestr"? Especially if you change
"usagestr" (the pointer itself) later. "[]" is sometimes a hint that
the pointer itself should not be changed, being an array.

And you want make opts const.

BTW, it does not "make" usage. It calls the usage() or prints a usage
description. "make" implies it creates the "usage", which according to
the prototype is later nowhere to be found.

> +{
> +	struct strbuf sb;
> +
> +	strbuf_init(&sb, 4096);
> +	do {
> +		strbuf_addstr(&sb, *usagestr++);
> +		strbuf_addch(&sb, '\n');
> +	} while (*usagestr);

This will crash for empty usagestr, like  "{ NULL }". Was it
deliberately? (I'd make it deliberately, if I were you. I'd even used
cnt of opts, to force people to document all options).

> +     strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> +		    opts->help);
...
> +	usage(sb.buf);

BTW, if you just printed the usage message out (it is about usage of a
program, isn't it?) and called exit() everyone would be just as happy.
And you wouldn't have to include strbuf (it is the only use of it),
less code, too. It'd make simplier to stea^Wcopy your implementation,
which I like :)

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

On Sat, Oct 13, 2007 at 07:16:55PM +0000, Alex Riesen wrote:
> Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> > [...]
> 
> "const struct option *opts"?
> 
> Why not "const char *const *usagestr"? Especially if you change
> "usagestr" (the pointer itself) later. "[]" is sometimes a hint that
> the pointer itself should not be changed, being an array.
> 
> And you want make opts const.

  Ok.

> BTW, it does not "make" usage. It calls the usage() or prints a usage
> description. "make" implies it creates the "usage", which according to
> the prototype is later nowhere to be found.

  Yes this has been spotted and fixed already.

> 
> > +{
> > +	struct strbuf sb;
> > +
> > +	strbuf_init(&sb, 4096);
> > +	do {
> > +		strbuf_addstr(&sb, *usagestr++);
> > +		strbuf_addch(&sb, '\n');
> > +	} while (*usagestr);
> 
> This will crash for empty usagestr, like  "{ NULL }". Was it
> deliberately? (I'd make it deliberately, if I were you. I'd even used
> cnt of opts, to force people to document all options).

  Yes this is intentional, there should be at least on string in the
usagestr array.


> > +     strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> > +		    opts->help);
> ....
> > +	usage(sb.buf);
> 
> BTW, if you just printed the usage message out (it is about usage of a
> program, isn't it?) and called exit() everyone would be just as happy.
> And you wouldn't have to include strbuf (it is the only use of it),
> less code, too. It'd make simplier to stea^Wcopy your implementation,
> which I like :)

  the reason is that usage() is a wrapper around a callback, and I
suppose it's used by some GUI's or anything like that.

  FWIW you can rework the .c like this:

  pos = 0; /* and not pos = sb.len */

  replace the strbuf_add* by the equivalents:
  pos += printf("....");

  and tada, you're done.


  Note that in the most recent version, I also deal with a
OPTION_CALLBACK that passes the value to a callback.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a simple option parser.

[Alex Riesen]

Pierre Habouzit, Sat, Oct 13, 2007 22:54:04 +0200:
> On Sat, Oct 13, 2007 at 07:16:55PM +0000, Alex Riesen wrote:
> > Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> > BTW, if you just printed the usage message out (it is about usage of a
> > program, isn't it?) and called exit() everyone would be just as happy.
> > And you wouldn't have to include strbuf (it is the only use of it),
> > less code, too. It'd make simplier to stea^Wcopy your implementation,
> > which I like :)
> 
>   the reason is that usage() is a wrapper around a callback, and I
> suppose it's used by some GUI's or anything like that.

It is not. Not yet. What could they use a usage text for?
Besides, you could just export the callback (call_usage_callback or
something) from usage.c and call it.

>   FWIW you can rework the .c like this:

on top of yours:

From: Alex Riesen <raa.lkml@gmail.com>
Date: Sun, 14 Oct 2007 00:10:51 +0200
Subject: [PATCH] Rework make_usage to print the usage message immediately

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 parse-options.c |   60 ++++++++++++++++++++++++------------------------------
 1 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 07abb50..1e3940f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1,6 +1,5 @@
 #include "git-compat-util.h"
 #include "parse-options.h"
-#include "strbuf.h"
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -171,57 +170,52 @@ int parse_options(int argc, const char **argv,
 
 void make_usage(const char * const usagestr[], struct option *opts, int cnt)
 {
-	struct strbuf sb;
-
-	strbuf_init(&sb, 4096);
-	do {
-		strbuf_addstr(&sb, *usagestr++);
-		strbuf_addch(&sb, '\n');
-	} while (*usagestr);
+	fprintf(stderr, "usage: ");
+	while (*usagestr)
+		fprintf(stderr, "%s\n", *usagestr++);
 
 	if (cnt && opts->type != OPTION_GROUP)
-		strbuf_addch(&sb, '\n');
+		fputc('\n', stderr);
 
 	for (; cnt-- > 0; opts++) {
 		size_t pos;
 
 		if (opts->type == OPTION_GROUP) {
-			strbuf_addch(&sb, '\n');
+			fputc('\n', stderr);
 			if (*opts->help)
-				strbuf_addf(&sb, "%s\n", opts->help);
+				fprintf(stderr, "%s\n", opts->help);
 			continue;
 		}
 
-		pos = sb.len;
-		strbuf_addstr(&sb, "    ");
-		if (opts->short_name) {
-			strbuf_addf(&sb, "-%c", opts->short_name);
-		}
-		if (opts->long_name) {
-			strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
-						opts->long_name);
-		}
+		pos = fprintf(stderr, "    ");
+		if (opts->short_name)
+			pos += fprintf(stderr, "-%c", opts->short_name);
+		if (opts->long_name)
+			pos += fprintf(stderr,
+				       opts->short_name ? ", --%s" : "--%s",
+				       opts->long_name);
 		switch (opts->type) {
 		case OPTION_INTEGER:
-			strbuf_addstr(&sb, " <n>");
+			fputs(" <n>", stderr);
+			pos += 4;
 			break;
 		case OPTION_STRING:
-			if (opts->argh) {
-				strbuf_addf(&sb, " <%s>", opts->argh);
-			} else {
-				strbuf_addstr(&sb, " ...");
+			if (opts->argh)
+				pos += fprintf(stderr, " <%s>", opts->argh);
+			else {
+				fputs(" ...", stderr);
+				pos += 4;
 			}
 			break;
 		default:
 			break;
 		}
-		if (sb.len - pos <= USAGE_OPTS_WIDTH) {
-			int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
-			strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
-		} else {
-			strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
-						opts->help);
-		}
+		if (pos <= USAGE_OPTS_WIDTH) {
+			int pad = USAGE_OPTS_WIDTH - pos + USAGE_GAP;
+			fprintf(stderr, "%*s%s\n", pad, "", opts->help);
+		} else
+			fprintf(stderr, "\n%*s%s\n",
+				USAGE_OPTS_WIDTH + USAGE_GAP, "", opts->help);
 	}
-	usage(sb.buf);
+	exit(129);
 }
-- 
1.5.3.4.232.ga843

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Sat, Oct 13, 2007 at 10:14:50PM +0000, Alex Riesen wrote:
> Pierre Habouzit, Sat, Oct 13, 2007 22:54:04 +0200:
> > On Sat, Oct 13, 2007 at 07:16:55PM +0000, Alex Riesen wrote:
> > > Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> > > BTW, if you just printed the usage message out (it is about usage of a
> > > program, isn't it?) and called exit() everyone would be just as happy.
> > > And you wouldn't have to include strbuf (it is the only use of it),
> > > less code, too. It'd make simplier to stea^Wcopy your implementation,
> > > which I like :)
> > 
> >   the reason is that usage() is a wrapper around a callback, and I
> > suppose it's used by some GUI's or anything like that.
> 
> It is not. Not yet. What could they use a usage text for?
> Besides, you could just export the callback (call_usage_callback or
> something) from usage.c and call it.

  Okay makes sense.

> >   FWIW you can rework the .c like this:
> 
> on top of yours:

  Added (reworked a bit for the current state of parse_options), and pushed.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]