[PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch.
Jani Nikula
jani at nikula.org
Tue Dec 6 12:41:23 PST 2011
On Sun, 4 Dec 2011 11:47:52 -0400, David Bremner <david at tethera.net> wrote:
> From: David Bremner <bremner at debian.org>
>
> As we noticed when Jani kindly converted things to getopt_long, much
> of the work in argument parsing in notmuch is due to the the key-value
> style arguments like --format=(raw|json|text).
Hi David -
Apologies for not finding time to do proper review earlier. All in all I
think this is good work, and I especially like the simplicity of
defining arguments in the commands. Even if it makes me slightly sad to
have to reinvent the argument parsing wheel... but this is much better
than just using getopt_long() like I did.
So the design is good, but there are a few issues and nitpicks; please
find my comments below.
BR,
Jani.
> In this version I implement Austin Clements' suggestion of basing the
> main API on taking pointers to output variables.
> ---
> Makefile.local | 1 +
> notmuch-opts.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> notmuch-opts.h | 44 ++++++++++++++++++
> 3 files changed, 181 insertions(+), 0 deletions(-)
> create mode 100644 notmuch-opts.c
> create mode 100644 notmuch-opts.h
>
> diff --git a/Makefile.local b/Makefile.local
> index c94402b..6606be8 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -303,6 +303,7 @@ notmuch_client_srcs = \
> notmuch-count.c \
> notmuch-dump.c \
> notmuch-new.c \
> + notmuch-opts.c \
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.
> notmuch-reply.c \
> notmuch-restore.c \
> notmuch-search.c \
> diff --git a/notmuch-opts.c b/notmuch-opts.c
> new file mode 100644
> index 0000000..62d94bf
> --- /dev/null
> +++ b/notmuch-opts.c
> @@ -0,0 +1,136 @@
> +#include <assert.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include "error_util.h"
> +#include "notmuch-opts.h"
> +
> +/*
> + 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.
> +
> +static notmuch_bool_t
> +_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
> +
> + if (arg_str[0] != ':' && arg_str[0] != '=') {
> + return FALSE;
> + }
> +
> + /* skip delimiter */
> + arg_str++;
I think the caller should check and skip the delimiters. See my comments
below where this gets called.
> +
> + notmuch_keyword_t *keywords = arg_desc->keywords;
> +
> + while (keywords->name) {
> + if (strcmp (arg_str, keywords->name) == 0) {
> + if (arg_desc->output_var) {
> + *((int *)arg_desc->output_var) = keywords->value;
> + }
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. :)
This applies below in similar places; I'll not repeat myself.
> + return TRUE;
> + }
> + keywords++;
> + }
> + fprintf (stderr, "unknown keyword: %s\n", arg_str);
> + return FALSE;
> +}
> +
> +/*
> + Search for the {pos_arg_index}th position argument, return FALSE if
> + that does not exist.
> +*/
> +
> +notmuch_bool_t
> +parse_position_arg (const char *arg_str, int pos_arg_index, const notmuch_opt_desc_t *arg_desc) {
> +
> + int pos_arg_counter = 0;
> + while (arg_desc->name){
> + if (arg_desc->opt_type == NOTMUCH_OPT_POSITION) {
> + if (pos_arg_counter == pos_arg_index) {
> + if (arg_desc->output_var) {
> + *((const char **)arg_desc->output_var) = arg_str;
> + }
> + return TRUE;
> + }
> + pos_arg_counter++;
> + }
> + arg_desc++;
> + }
> + return FALSE;
> +}
> +
> +notmuch_bool_t
> +parse_option (const char *arg,
> + const notmuch_opt_desc_t *options){
Missing space between ) and {.
> +
> + assert(arg);
> + assert(options);
> +
> + arg += 2;
> +
> + const notmuch_opt_desc_t *try = options;
> + while (try->name) {
> + if (strncmp (arg, try->name, strlen(try->name)) == 0) {
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().
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,
which, AFAICT, would fall in the internal error path below. They'll
*only* be handled as positional arguments, not option args.
I'm not sure if much weight should be put to getopt_long()
compatibility, but it accepts "--parameter value" as well as
"--parameter=value".
> +
> + switch (try->opt_type) {
> + case NOTMUCH_OPT_KEYWORD:
> + return _process_keyword_arg (try, arg+strlen(try->name));
Two spaces after return.
> + break;
> + case NOTMUCH_OPT_BOOLEAN:
> + if (try->output_var)
> + *((notmuch_bool_t *)try->output_var) = TRUE;
> + return TRUE;
> + break;
> + case NOTMUCH_OPT_INT:
> + if (try->output_var)
> + *((int *)try->output_var) =
> + atol (arg + strlen (try->name) + 1);
As implied above, the "+1" here might skip any character. Also, two
spaces after +.
> + return TRUE;
> + break;
> +
> + case NOTMUCH_OPT_POSITION:
> + case NOTMUCH_OPT_NULL:
> + default:
> + INTERNAL_ERROR ("unknown or unhandled option type %d", try->opt_type);
> + /*UNREACHED*/
> + }
> + }
> + try++;
> + }
> + fprintf (stderr, "Unrecognized option: --%s\n", arg);
> + return FALSE;
> +}
> +
I think notmuch_parse_args() is a complicated function enough to warrant
a proper description here.
> +int
> +notmuch_parse_args (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index){
Missing space between ) and {.
> +
> + int pos_arg_index = 0;
> + notmuch_bool_t more_args = TRUE;
> +
> + while (more_args && opt_index < argc) {
> + if (strncmp (argv[opt_index],"--",2) != 0) {
> +
> + more_args = parse_position_arg (argv[opt_index], pos_arg_index, options);
> +
> + if (more_args) {
> + pos_arg_index++;
> + opt_index++;
> + }
> +
> + } else {
> +
> + if (strlen (argv[opt_index]) == 2)
> + return opt_index+1;
> +
> + more_args = parse_option (argv[opt_index], options);
> + if (more_args) {
> + opt_index++;
> + } else {
> + opt_index = -1;
> + }
> +
> + }
> + }
> +
> + return opt_index;
> +}
> diff --git a/notmuch-opts.h b/notmuch-opts.h
> new file mode 100644
> index 0000000..2280fea
> --- /dev/null
> +++ b/notmuch-opts.h
> @@ -0,0 +1,44 @@
> +#ifndef NOTMUCH_OPTS_H
> +#define NOTMUCH_OPTS_H
> +
> +#include "notmuch.h"
> +
> +enum notmuch_opt_type {
> + NOTMUCH_OPT_NULL = 0,
> + NOTMUCH_OPT_BOOLEAN,
> + NOTMUCH_OPT_INT,
> + NOTMUCH_OPT_KEYWORD,
> + NOTMUCH_OPT_POSITION
> +};
> +
> +typedef struct notmuch_keyword {
> + const char *name;
> + int value;
> +} notmuch_keyword_t;
> +
> +typedef struct notmuch_opt_desc {
> + const char *name;
> + int arg_id;
> + enum notmuch_opt_type opt_type;
> + struct notmuch_keyword *keywords;
> + void *output_var;
> +} notmuch_opt_desc_t;
> +
> +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?
> +
> +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?
> +
> +int
> +notmuch_parse_args(int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index);
> +
> +#endif
> --
> 1.7.7.3
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
More information about the notmuch
mailing list