From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 067D5429E26 for ; Tue, 6 Dec 2011 12:41:32 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id mLVSUxvIFcGP for ; Tue, 6 Dec 2011 12:41:31 -0800 (PST) Received: from mail-bw0-f53.google.com (mail-bw0-f53.google.com [209.85.214.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id BFFD1429E21 for ; Tue, 6 Dec 2011 12:41:30 -0800 (PST) Received: by bkbzu5 with SMTP id zu5so7885467bkb.26 for ; Tue, 06 Dec 2011 12:41:29 -0800 (PST) Received: by 10.180.103.170 with SMTP id fx10mr19246650wib.56.1323204089148; Tue, 06 Dec 2011 12:41:29 -0800 (PST) Received: from localhost (dsl-hkibrasgw4-fe5cdc00-23.dhcp.inet.fi. [80.220.92.23]) by mx.google.com with ESMTPS id n9sm30086671wbo.16.2011.12.06.12.41.25 (version=SSLv3 cipher=OTHER); Tue, 06 Dec 2011 12:41:27 -0800 (PST) From: Jani Nikula To: David Bremner , notmuch@notmuchmail.org Subject: Re: [PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch. In-Reply-To: <1323013675-6929-2-git-send-email-david@tethera.net> References: <1323013675-6929-1-git-send-email-david@tethera.net> <1323013675-6929-2-git-send-email-david@tethera.net> User-Agent: Notmuch/0.10.1+59~ga1814f2 (http://notmuchmail.org) Emacs/23.3.1 (i686-pc-linux-gnu) Date: Tue, 06 Dec 2011 22:41:23 +0200 Message-ID: <87zkf5xwjw.fsf@nikula.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: David Bremner X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Dec 2011 20:41:32 -0000 On Sun, 4 Dec 2011 11:47:52 -0400, David Bremner wrote: > From: David Bremner > > 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 > +#include > +#include > +#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@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch