[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] Port builtin-add.c to use the new option parser.

[Johannes Schindelin]

Hi,

On Sat, 13 Oct 2007, Pierre Habouzit wrote:

> +static struct option builtin_add_options[] = {
> +	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
> +	OPT_BOOLEAN('n', NULL, &show_only, "dry-run"),
> +	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
> +	OPT_BOOLEAN('v', NULL, &verbose, "be verbose"),
> +	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files that git already knows about"),
> +	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh stat() informations in the index"),
> +};

I see you terminate the list by a ",".  How does this play with the option 
parser?

Thinking about this more, I am reverting my stance on the ARRAY_SIZE() 
issue.  I think if you introduce a "OPTION_NONE = 0" in the enum, then 
this single last comma should be enough.

In the same vein, you would not need the NULL in builtin_add_usage[], 
right?

Ciao,
Dscho

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

[Pierre Habouzit]

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

On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
> 
> > +static struct option builtin_add_options[] = {
> > +	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
> > +	OPT_BOOLEAN('n', NULL, &show_only, "dry-run"),
> > +	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
> > +	OPT_BOOLEAN('v', NULL, &verbose, "be verbose"),
> > +	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files that git already knows about"),
> > +	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh stat() informations in the index"),
> > +};
> 
> I see you terminate the list by a ",".  How does this play with the option 
> parser?
> 
> Thinking about this more, I am reverting my stance on the ARRAY_SIZE() 
> issue.  I think if you introduce a "OPTION_NONE = 0" in the enum, then 
> this single last comma should be enough.

  adding a trailing comma does not add a NULL after that, it's ignored,
you're confused.

    ┌─(17:00)────
    └[artemis] cat a.c
    #include <stdio.h>

      int main(void) {
	const char * const arr[] = { "1", "2", };
	printf("%d\n", sizeof(arr) / sizeof(arr[0]));
	return 0;
    };
    ┌─(17:00)────
    └[artemis] ./a
    2

  Very few compilers do not grok trailing commas, I always put them
because it avoids spurious diffs for nothing, and that you can reorder
lines easily too.

  Note that I don't really like using ARRAY_SIZE either, I kept it that
way, but my taste would rather be to have an "empty" option, and
explicitely mark the end of the array.

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

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

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

[Alex Riesen]

Pierre Habouzit, Sat, Oct 13, 2007 17:03:06 +0200:
> On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> > Thinking about this more, I am reverting my stance on the ARRAY_SIZE() 
> > issue.  I think if you introduce a "OPTION_NONE = 0" in the enum, then 
> > this single last comma should be enough.
> 
>   adding a trailing comma does not add a NULL after that, it's ignored,
> you're confused.

Yep

>   Note that I don't really like using ARRAY_SIZE either, I kept it that
> way, but my taste would rather be to have an "empty" option, and
> explicitely mark the end of the array.

You can have both. Just stop at NULL-entry or when the 'size' elements
passed, whatever happens first.

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

[Pierre Habouzit]

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

On Sat, Oct 13, 2007 at 07:22:13PM +0000, Alex Riesen wrote:
> Pierre Habouzit, Sat, Oct 13, 2007 17:03:06 +0200:
> > On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> > > Thinking about this more, I am reverting my stance on the ARRAY_SIZE() 
> > > issue.  I think if you introduce a "OPTION_NONE = 0" in the enum, then 
> > > this single last comma should be enough.
> > 
> >   adding a trailing comma does not add a NULL after that, it's ignored,
> > you're confused.
> 
> Yep
> 
> >   Note that I don't really like using ARRAY_SIZE either, I kept it that
> > way, but my taste would rather be to have an "empty" option, and
> > explicitely mark the end of the array.
> 
> You can have both. Just stop at NULL-entry or when the 'size' elements
> passed, whatever happens first.

  Well I dislike the "count" thing, and Dscho agreed that it somehow
sucked too. If you go see the current state of the ph/parseopt series
you'll see it's not here anymore.

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

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