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

  yes it is.

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

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

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

  Ack I'll fix that.

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

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

  fsck, good catch.

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

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

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

  oops, stupid space vs. tab thing.

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

  will do.

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

  okay.

quoted text
> In the same vein, OPT_COPY_DASHDASH should be named=20
> PARSE_OPT_KEEP_DASHDASH.

  okay.

quoted text
>=20
> > +		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);
> > +		}
>=20
> Please lose the curly brackets.
>=20
> > +		if (sb.len - pos <=3D USAGE_OPTS_WIDTH) {
> > +			int pad =3D 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);
> > +		}
>=20
> Same here.  (And I'd also make sure that the lines are not that long.)

  okay.

quoted text
>=20
> > 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,
>=20
> I know that I proposed "BOOLEAN", but actually, you use it more like an=
=20
quoted text
> "INCREMENTAL", right?

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

quoted text
> Other than that: I like it very much.

:P

--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org