Hi,

On Sat, 13 Oct 2007, Pierre Habouzit wrote:

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

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

quoted text
> +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.

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

quoted text
> +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... ;-)

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

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

quoted text
> +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.

quoted text
> +		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.

quoted text
> +		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.

quoted text
> +		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.

quoted text
> +		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.)

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

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html