[PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch.
David Bremner
david at tethera.net
Wed Dec 7 04:27:34 PST 2011
On Tue, 06 Dec 2011 22:41:23 +0200, Jani Nikula <jani at 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
More information about the notmuch
mailing list