From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 01 Oct 2007 18:13:27 GMT
Subject: Re: [PATCH 1/4] Add a simple option parser for use by builtin-commit.c.
Message-ID: <Pine.LNX.4.64.0710011909291.28395@racer.site>
In-Reply-To: <1191255975.25093.26.camel@hinata.boston.redhat.com>

Hi,

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

> On Sun, 2007-09-30 at 15:11 +0200, Jonas Fonseca wrote:
>
> > 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.

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

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