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

Hi,

On Mon, 1 Oct 2007, Kristian H?gsberg wrote:

On Sun, 2007-09-30 at 15:11 +0200, Jonas Fonseca wrote:

quoted

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.

That's a good idea; I would put the usage string there, too.

quoted

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.

It might be convenient, but I think that it is really more confusing than 
helpful, especially with options that share a prefix.  Besides, we have 
good completion for bash now (and I hear that this "zsh" thing also has 
quite good completion), I recommend <TAB> over prefix DWIMery.

quoted

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

FWIW I like the ARRAY_SIZE() approach better, too, since it is less error 
prone.

Ciao,
Dscho