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

From: Kristian Høgsberg <krh@redhat.com>
Date: Oct 3, 2007, 21:53 UTC
Message-ID: <1191448438.7134.8.camel@hinata.boston.redhat.com>
In-Reply-To: <20071003201129.GB25856@diku.dk>

On Wed, 2007-10-03 at 22:11 +0200, Jonas Fonseca wrote:

Show 27 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote Mon, Oct 01, 2007:
> > On Mon, 1 Oct 2007, Kristian H?gsberg wrote:
> > > On Sun, 2007-09-30 at 15:11 +0200, Jonas Fonseca wrote:
> > > > > +
> > > > > +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.
> 
> OK, I must have missed that comment. Good point.
> 
> Thanks for the comments both of you. It's great to have something to
> work from. However, I also fear it will also require that some extra
> flags or information is added to the option information to make it more
> generally usable. But I guess that is easier to discuss in the context
> of a patch.

I just sent an updated option parser patch that incorporates your suggestions along with a patch that ports builtin-add.c to use it. I looked briefly into porting over a few other builtins, but you're right, we need a couple of extra features for this to be really worthwile:

      * OPTION_SET_FLAG: sets the bit (we need to add a bit value that
        the option parser can or in)
      * OPTION_CLEAR_FLAG: clear the bit
      * OPTION_ADD: adds the value to the destination integer
      * OPTION_CALLBACK: calls the given function when the option is
        matched.  We'll need this for builtin-grep that has positional
        args such as --and etc.

Also, the option parser should probably verify that a string option isn't passed more than once. Bundling of single letter options would be nice to add. But the patch I just sent out should be a good start, and it lets us move forward with builtin-commit.c.

cheers, Kristian