From: Kristian Høgsberg <krh@redhat.com>
Date: Mon, 01 Oct 2007 16:26:15 GMT
Subject: Re: [PATCH 1/4] Add a simple option parser for use by builtin-commit.c.
Message-ID: <1191255975.25093.26.camel@hinata.boston.redhat.com>
In-Reply-To: <20070930131133.GA11209@diku.dk>

On Sun, 2007-09-30 at 15:11 +0200, Jonas Fonseca wrote:
> Hello Kristian,
> 
> I have some comments on your patch. Some of the "improvement" might have
> to wait until after your builtin-commit changes hits git.git. However,
> if we could agree on some of the general changes, I could start porting
> other of the main porcelain commands to use the option parser without
> depending on the state of the remaining builtin-commit series.

Hi Jonas,

That's sounds like a good plan.  In fact, in you want to update the
patch with your changes (they all sound good) and start porting over
some of the other builtins feel free.  I don't have much time follow up
on these comments right now, but I will get to it eventually - unless
you beat me to it of course ;)  I will update builtin-commit.c to work
with whatever changes you introduce once I get around to updating that
patch.

> Kristian Høgsberg <krh@redhat.com> wrote Thu, Sep 27, 2007:
> > Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> > ---
> >  Makefile        |    2 +-
> >  parse-options.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  parse-options.h |   29 +++++++++++++++++++++
> >  3 files changed, 104 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..2fb30cd
> > --- /dev/null
> > +++ b/parse-options.c
> > @@ -0,0 +1,74 @@
> > +#include "git-compat-util.h"
> > +#include "parse-options.h"
> > +
> > +int parse_options(const char ***argv,
> > +		  struct option *options, int count,
> > +		  const char *usage_string)
> > +{
> > +	const char *value, *eq;
> > +	int i;
> > +
> > +	if (**argv == NULL)
> > +		return 0;
> > +	if ((**argv)[0] != '-')
> > +		return 0;
> > +	if (!strcmp(**argv, "--"))
> > +		return 0;
> 
> I don't know if this is a bug, but you do not remove "--" from argv,
> which is later (in the patch that adds builtin-commit.c) passed to
> add_files_to_cache and then get_pathspec where it is not removed or
> detected either.

That's an oversight, good catch.

> > +
> > +	value = NULL;
> > +	for (i = 0; i < count; i++) {
> > +		if ((**argv)[1] == '-') {
> > +			if (!prefixcmp(options[i].long_name, **argv + 2)) {
> > +				if (options[i].type != OPTION_BOOLEAN)
> > +					value = *++(*argv);
> > +				goto match;
> > +			}
> > +
> > +			eq = strchr(**argv + 2, '=');
> > +			if (eq && options[i].type != OPTION_BOOLEAN &&
> > +			    !strncmp(**argv + 2,
> > +				     options[i].long_name, eq - **argv - 2)) {
> > +				value = eq + 1;
> > +				goto match;
> > +			}
> > +		}
> > +
> > +		if ((**argv)[1] == options[i].short_name) {
> > +			if ((**argv)[2] == '\0') {
> > +				if (options[i].type != OPTION_BOOLEAN)
> > +					value = *++(*argv);
> > +				goto match;
> > +			}
> > +
> > +			if (options[i].type != OPTION_BOOLEAN) {
> > +				value = **argv + 2;
> > +				goto match;
> > +			}
> > +		}
> > +	}
> > +
> > +	usage(usage_string);
> > +
> > + match:
> 
> I think the goto can be avoided by simply breaking out of the above loop
> when an option has been found and add ...
> 
> > +	switch (options[i].type) {
> 	case OPTION_LAST
> 		usage(usage_string);
> 		break;
> 
> > +	case OPTION_BOOLEAN:
> > +		*(int *)options[i].value = 1;
> > +		break;

Yeah, that looks nicer.  I think the goto structure is a leftover from
when there was more logic between the loop and the switch.  It's good to
get some fresh eyes on this code.  Junio didn't like the OPTION_LAST
terminator, so I changed the interface to take a count.  We can do

	if (i == count)
		usage();
	else switch (options[i].type) {
		...
	}

of course.

> I've been looking at builtin-blame.c which IMO has some of the most
> obscure option parsing and maybe this can be changed to increment in
> order to support stuff like changing the meaning by passing the same arg
> multiple times (e.g. "-C -C -C") better.

That would be fine, yes.

> Blame option parsing also sports (enum) flags being masked together,
> this can of course be rewritten to a boolean option followed by
> masking when parse_options is done (to keep it sane).

Yup.

> > +	case OPTION_STRING:
> > +		if (value == NULL)
> > +			die("option %s requires a value.", (*argv)[-1]);
> 
> Maybe change this ...
> 
> > +		*(const char **)options[i].value = value;
> > +		break;
> > +	case OPTION_INTEGER:
> > +		if (value == NULL)
> > +			die("option %s requires a value.", (*argv)[-1]);
> 
> ... and this to:
> 
> 		if (!value) {
> 			error("option %s requires a value.", (*argv)[-1]);
> 			usage(usage_string);
> 		}

Sure, that's friendlier.

> > +		*(int *)options[i].value = atoi(value);
> > +		break;
> > +	default:
> > +		assert(0);
> > +	}
> > +
> > +	(*argv)++;
> > +
> > +	return 1;
> > +}
> > diff --git a/parse-options.h b/parse-options.h
> > new file mode 100644
> > index 0000000..39399c3
> > --- /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,
> > +    OPTION_LAST,
> > +};
> > +
> > +struct option {
> > +    enum option_type type;
> > +    const char *long_name;
> > +    char short_name;
> > +    void *value;
> > +};
> 
> Space vs tab indentation.
> 
> One of the last things I miss from Cogito is the nice abbreviated help
> messages that was available via '-h'. I don't know if it would be
> acceptable (at least for the main porcelain commands) to put this
> functionality into the option parser by adding a "description" member to
> struct option and have parse_options print a nice:
> 
> 	<error message if any>
> 	<usage string>
> 	<option summary>
> 
> on failure, or, if that is regarded as too verbose, simply when -h is
> detected.

Yeah, that might be nice.  We can add it in a follow-on patch, if the
list agrees that it's a good thing, I guess.

> > +
> > +/* 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.
> > + */
> 
> This prefix aware option parsing has not been ported over to the other
> builtins when they were lifted from shell code. It might be nice to have
> of course. Is it really needed?

I don't ever use it myself and I think it's more confusing than helpful.
I only added it to avoid introducing behavior changes in the port.  I
don't have strong feelings either way.

> > +
> > +extern int parse_options(const char ***argv,
> > +			 struct option *options, int count,
> > +			 const char *usage_string);
> 
> I think the interface could be improved a bit. For example, it doesn't
> need to count argument since the last entry in the options array is
> OPTION_LAST and thus the size can be detected that way.

Hehe, yeah, that's how I did it first.  I don't have a strong preference
for terminator elements vs. ARRAY_SIZE(), but Junio prefers the
ARRAY_SIZE() approach, I guess.  At this point I'm just trying the get
the patches upstream...

> Also, I think for this to be more usable for other built-in programs it
> shouldn't modify argv, but instead take both argc and argv (so we don't
> need to have code like "*++(*argv)" ;), parse _all_ options in one go,
> and return the index (of argv) for any remaining options.
> 
> Then the following:
> 
> 	while (parse_options(argv, commit_options, ARRAY_SIZE(commit_options),
> 		builtin_commit_usage))
> 		;
> 
> becomes:
> 
> 	int i;
> 	...
> 	i = parse_options(argc, argv, commit_options, builtin_commit_usage);
> 
> This fits better with how option parsing is currently done. Take
> builtin-add for example:
> 
> 	for (i = 1 ; i < argc ; i++) {
> 		const char *arg = argv[i];
> 		/* ... */
> 	}
> 	if (argc <= i)
> 		usage(builtin_rm_usage);

No objections, I think that looks better too.

> [ BTW, blame option parsing actually wants to know if "--" has been seen,
>   but I think that can be worked around by simply checking argv[i - 1]
>   after calling the option parser. ]
> 
> > +
> > +#endif
> 
> OK, I will stop these ramblings here. I hope the fact that I read your
> patch both back and forth and added comments in the process didn't make
> it too confusing.

Heh, that's what I do myself :)

thanks for the comments,
Kristian