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

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

Johannes Schindelin [off-list ref] wrote Mon, Oct 01, 2007:

quoted

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

quoted

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

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.

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