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

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 [off-list ref] wrote Thu, Sep 27, 2007:

quoted

Signed-off-by: Kristian Høgsberg <redacted>
---
 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.

quoted

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

quoted

+	switch (options[i].type) {

	case OPTION_LAST
		usage(usage_string);
		break;

quoted

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

quoted

+	case OPTION_STRING:
+		if (value == NULL)
+			die("option %s requires a value.", (*argv)[-1]);

Maybe change this ...

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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