1 Return-Path: <bremner@unb.ca>
\r
2 X-Original-To: notmuch@notmuchmail.org
\r
3 Delivered-To: notmuch@notmuchmail.org
\r
4 Received: from localhost (localhost [127.0.0.1])
\r
5 by olra.theworths.org (Postfix) with ESMTP id 7B47D429E26
\r
6 for <notmuch@notmuchmail.org>; Wed, 7 Dec 2011 04:27:44 -0800 (PST)
\r
7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org
\r
11 X-Spam-Status: No, score=-2.3 tagged_above=-999 required=5
\r
12 tests=[RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled
\r
13 Received: from olra.theworths.org ([127.0.0.1])
\r
14 by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)
\r
15 with ESMTP id XrF9h6ikirlj for <notmuch@notmuchmail.org>;
\r
16 Wed, 7 Dec 2011 04:27:43 -0800 (PST)
\r
17 Received: from tempo.its.unb.ca (tempo.its.unb.ca [131.202.1.21])
\r
18 (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
\r
19 (No client certificate requested)
\r
20 by olra.theworths.org (Postfix) with ESMTPS id 99F6B431FD0
\r
21 for <notmuch@notmuchmail.org>; Wed, 7 Dec 2011 04:27:43 -0800 (PST)
\r
22 Received: from zancas.localnet
\r
23 (fctnnbsc36w-156034079193.pppoe-dynamic.High-Speed.nb.bellaliant.net
\r
24 [156.34.79.193]) (authenticated bits=0)
\r
25 by tempo.its.unb.ca (8.13.8/8.13.8) with ESMTP id pB7CRZaf005680
\r
26 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NO);
\r
27 Wed, 7 Dec 2011 08:27:35 -0400
\r
28 Received: from bremner by zancas.localnet with local (Exim 4.77)
\r
29 (envelope-from <bremner@unb.ca>)
\r
30 id 1RYGb0-0006Bk-Ln; Wed, 07 Dec 2011 08:27:34 -0400
\r
31 From: David Bremner <david@tethera.net>
\r
32 To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
\r
33 Subject: Re: [PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for
\r
35 In-Reply-To: <87zkf5xwjw.fsf@nikula.org>
\r
36 References: <1323013675-6929-1-git-send-email-david@tethera.net>
\r
37 <1323013675-6929-2-git-send-email-david@tethera.net>
\r
38 <87zkf5xwjw.fsf@nikula.org>
\r
39 User-Agent: Notmuch/0.10.2 (http://notmuchmail.org) Emacs/23.3.1
\r
40 (x86_64-pc-linux-gnu)
\r
41 Date: Wed, 07 Dec 2011 08:27:34 -0400
\r
42 Message-ID: <87iplso9c9.fsf@zancas.localnet>
\r
44 Content-Type: text/plain; charset=us-ascii
\r
45 X-BeenThere: notmuch@notmuchmail.org
\r
46 X-Mailman-Version: 2.1.13
\r
48 List-Id: "Use and development of the notmuch mail system."
\r
49 <notmuch.notmuchmail.org>
\r
50 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
\r
51 <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
\r
52 List-Archive: <http://notmuchmail.org/pipermail/notmuch>
\r
53 List-Post: <mailto:notmuch@notmuchmail.org>
\r
54 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
\r
55 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
\r
56 <mailto:notmuch-request@notmuchmail.org?subject=subscribe>
\r
57 X-List-Received-Date: Wed, 07 Dec 2011 12:27:44 -0000
\r
59 On Tue, 06 Dec 2011 22:41:23 +0200, Jani Nikula <jani@nikula.org> wrote:
\r
61 > We chatted about reserving notmuch-*.c to notmuch commands, but I guess
\r
62 > it was only after you sent these. I think it would make sense though.
\r
64 renamed to "command-line-arguments.[ch]". Hard luck for those of you on
\r
68 > > + Search the list of keywords for a given argument, assigning the
\r
69 > > + output variable to the corresponding value. Return FALSE if nothing
\r
73 > Array of keywords to be really pedantic.
\r
78 > > + /* skip delimiter */
\r
81 > I think the caller should check and skip the delimiters. See my comments
\r
82 > below where this gets called.
\r
84 done, and error checking tightened up.
\r
87 > So if ->output_var is NULL, the parameter is accepted but silently
\r
88 > ignored? I'm not sure if I should consider this a feature or a bug. :)
\r
90 >From one extreme to another, it is now an INTERNAL_ERROR to have
\r
91 output_var NULL. I couldn't see a use case for silently ignoring command
\r
94 > > +parse_option (const char *arg,
\r
95 > > + const notmuch_opt_desc_t *options){
\r
97 > Missing space between ) and {.
\r
99 done. but you missed some other missing spaces ;).
\r
101 > I think here you should check that arg[strlen(try->name)] is '=' or ':'
\r
102 > for NOTMUCH_OPT_KEYWORD and NOTMUCH_OPT_INT, and '\0' otherwise. After
\r
103 > the check, you could pass just the value part to _process_keyword_arg().
\r
107 > I can't figure out if and how you handle arguments with arbitrary string
\r
108 > values (for example --file=/path/to/file). You do specify --in-file and
\r
109 > --out-file in later patches, but those are with NOTMUCH_OPT_POSITION,
\r
111 there is now NOTMUCH_OPT_STRING which does this, but it is untested as
\r
112 notmuch doesn't take these kind of arguments at the moment (restore
\r
113 --match is one example, but those patches are unmerged so far).
\r
115 > I'm not sure if much weight should be put to getopt_long()
\r
116 > compatibility, but it accepts "--parameter value" as well as
\r
117 > "--parameter=value".
\r
119 Yeah, maybe I shouldn't let the implementation drive things this much,
\r
120 but having one argmument per element of argv simplfies things
\r
121 nicely. For now, I will skip this.
\r
124 > I think notmuch_parse_args() is a complicated function enough to warrant
\r
125 > a proper description here.
\r
131 > > +notmuch_parse_args (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index){
\r
133 > Missing space between ) and {.
\r
137 > > +typedef struct notmuch_opt {
\r
139 > > + int keyword_id;
\r
140 > > + const char *string;
\r
141 > > +} notmuch_opt_t;
\r
143 > You're not using this for anything, are you?
\r
148 > > +notmuch_bool_t
\r
149 > > +parse_option (const char *arg, const notmuch_opt_desc_t* options);
\r
151 > > +notmuch_bool_t
\r
152 > > +parse_position_arg (const char *arg,
\r
153 > > + int position_arg_index,
\r
154 > > + const notmuch_opt_desc_t* options);
\r
156 > Is there a good reason the above are not static?
\r
158 I had in mind to provide the user with the option of a getopt style loop
\r
159 if the loop in parse_arguments was not flexible enough. I could leave
\r
160 them as static until such a need arises, I suppose.
\r
162 Thanks for the review! Updated series to follow.
\r