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 7B47D429E26 for ; Wed, 7 Dec 2011 04:27:44 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -2.3 X-Spam-Level: X-Spam-Status: No, score=-2.3 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_MED=-2.3] 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 XrF9h6ikirlj for ; Wed, 7 Dec 2011 04:27:43 -0800 (PST) Received: from tempo.its.unb.ca (tempo.its.unb.ca [131.202.1.21]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 99F6B431FD0 for ; Wed, 7 Dec 2011 04:27:43 -0800 (PST) Received: from zancas.localnet (fctnnbsc36w-156034079193.pppoe-dynamic.High-Speed.nb.bellaliant.net [156.34.79.193]) (authenticated bits=0) by tempo.its.unb.ca (8.13.8/8.13.8) with ESMTP id pB7CRZaf005680 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NO); Wed, 7 Dec 2011 08:27:35 -0400 Received: from bremner by zancas.localnet with local (Exim 4.77) (envelope-from ) id 1RYGb0-0006Bk-Ln; Wed, 07 Dec 2011 08:27:34 -0400 From: David Bremner To: Jani Nikula , notmuch@notmuchmail.org Subject: Re: [PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch. In-Reply-To: <87zkf5xwjw.fsf@nikula.org> References: <1323013675-6929-1-git-send-email-david@tethera.net> <1323013675-6929-2-git-send-email-david@tethera.net> <87zkf5xwjw.fsf@nikula.org> User-Agent: Notmuch/0.10.2 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) Date: Wed, 07 Dec 2011 08:27:34 -0400 Message-ID: <87iplso9c9.fsf@zancas.localnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: Wed, 07 Dec 2011 12:27:44 -0000 On Tue, 06 Dec 2011 22:41:23 +0200, Jani Nikula 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