git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/4] Add a simple option parser for use by builtin-commit.c.

From
Jonas Fonseca <fonseca@diku.dk>
Date
Sep 30, 2007, 13:11 UTC
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:
Show 44 quoted lines
> 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.

Show 36 quoted lines
> +
> +	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 ...
Show 5 quoted lines
> +		*(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);
		}
Show 32 quoted lines
> +		*(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.

Show 7 quoted lines
> +
> +/* 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
Previous: Junio C HamanoNext: Johannes Schindelin
Message 9 of 17 in “Add a simple option parser for use by builtin-commit.c.”
  1. 1/4 Add a simple option parser for use by builtin-commit.c.Kristian Høgsberg, Sep 27, 2007
  2. 2/4 This exports the update() function from builtin-add.c asKristian Høgsberg, Sep 27, 2007
  3. 3/4 Implement git commit as a builtin command.Kristian Høgsberg, Sep 27, 2007
  4. 4/4 Move launch_editor() and stripspace() to new file editor.c.Kristian Høgsberg, Sep 27, 2007
  5. Junio C HamanoSep 27, 2007
  6. Kristian HøgsbergOct 3, 2007
  7. Johannes SchindelinSep 27, 2007
  8. Junio C HamanoSep 27, 2007
  9. Jonas FonsecaSep 30, 2007
  10. Johannes SchindelinOct 1, 2007
  11. Jonas FonsecaOct 1, 2007
  12. Johannes SchindelinOct 1, 2007
  13. Jeff KingOct 1, 2007
  14. Kristian HøgsbergOct 1, 2007
  15. Johannes SchindelinOct 1, 2007
  16. Jonas FonsecaOct 3, 2007
  17. Kristian HøgsbergOct 3, 2007

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.