From: Jonas Fonseca <fonseca@diku.dk>
Date: Sun, 30 Sep 2007 13:11:33 GMT
Subject: Re: [PATCH 1/4] Add a simple option parser for use by builtin-commit.c.
Message-ID: <20070930131133.GA11209@diku.dk>
In-Reply-To: <1190868632-29287-1-git-send-email-krh@redhat.com>

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.

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.

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

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.

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

> +	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);
		}

> +		*(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.

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

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

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

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

-- 
Jonas Fonseca