Re: [PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch.

Subject: Re: [PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch.

Date: Wed, 07 Dec 2011 08:27:34 -0400

To: Jani Nikula, notmuch@notmuchmail.org

Cc:

From: David Bremner

On Tue, 06 Dec 2011 22:41:23 +0200, Jani Nikula <jani@nikula.org> wrote:
> 
> We chatted about reserving notmuch-*.c to notmuch commands, but I guess
> it was only after you sent these. I think it would make sense though.

renamed to "command-line-arguments.[ch]". Hard luck for those of you on
8.3 file systems.  

> > +/*
> > +  Search the list of keywords for a given argument, assigning the
> > +  output variable to the corresponding value.  Return FALSE if nothing
> > +  matches.
> > +*/
> 
> Array of keywords to be really pedantic.

Done.

> > +
> > +    /* skip delimiter */
> > +    arg_str++;
> 
> I think the caller should check and skip the delimiters. See my comments
> below where this gets called.

done, and error checking tightened up.

> 
> So if ->output_var is NULL, the parameter is accepted but silently
> ignored? I'm not sure if I should consider this a feature or a bug. :)

>From one extreme to another, it is now an INTERNAL_ERROR to have
output_var NULL. I couldn't see a use case for silently ignoring command
line arguments.

> > +parse_option (const char *arg,
> > +	      const notmuch_opt_desc_t *options){
> 
> Missing space between ) and {.

done. but you missed some other missing spaces ;).

> I think here you should check that arg[strlen(try->name)] is '=' or ':'
> for NOTMUCH_OPT_KEYWORD and NOTMUCH_OPT_INT, and '\0' otherwise. After
> the check, you could pass just the value part to _process_keyword_arg().

done.

> I can't figure out if and how you handle arguments with arbitrary string
> values (for example --file=/path/to/file). You do specify --in-file and
> --out-file in later patches, but those are with NOTMUCH_OPT_POSITION,

there is now NOTMUCH_OPT_STRING which does this, but it is untested as
notmuch doesn't take these kind of arguments at the moment (restore
--match is one example, but those patches are unmerged so far).

> I'm not sure if much weight should be put to getopt_long()
> compatibility, but it accepts "--parameter value" as well as
> "--parameter=value".

Yeah, maybe I shouldn't let the implementation drive things this much,
but having one argmument per element of argv simplfies things
nicely. For now, I will skip this.

> 
> I think notmuch_parse_args() is a complicated function enough to warrant
> a proper description here.

Done.

> 
> > +int
> > +notmuch_parse_args (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index){
> 
> Missing space between ) and {.

Done.

> > +typedef struct notmuch_opt {
> > +    int arg_id;
> > +    int keyword_id;
> > +    const char *string;
> > +} notmuch_opt_t;
> 
> You're not using this for anything, are you?

Oops, deleted.
> 
> > +
> > +notmuch_bool_t
> > +parse_option (const char *arg, const notmuch_opt_desc_t* options);
> > +
> > +notmuch_bool_t
> > +parse_position_arg (const char *arg,
> > +		    int position_arg_index,
> > +		    const notmuch_opt_desc_t* options);
> 
> Is there a good reason the above are not static?

I had in mind to provide the user with the option of a getopt style loop
if the loop in parse_arguments was not flexible enough.  I could leave
them as static until such a need arises, I suppose.

Thanks for the review! Updated series to follow.

d

Thread: