be/6e8b64bbe8ad9677c3cdb7ad9b2e676f812bc7

   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
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -2.3\r
  10 X-Spam-Level: \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
  34         notmuch.\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
  43 MIME-Version: 1.0\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
  47 Precedence: list\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
  58 \r
  59 On Tue, 06 Dec 2011 22:41:23 +0200, Jani Nikula <jani@nikula.org> wrote:\r
  60 > \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
  63 \r
  64 renamed to "command-line-arguments.[ch]". Hard luck for those of you on\r
  65 8.3 file systems.  \r
  66 \r
  67 > > +/*\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
  70 > > +  matches.\r
  71 > > +*/\r
  72 > \r
  73 > Array of keywords to be really pedantic.\r
  74 \r
  75 Done.\r
  76 \r
  77 > > +\r
  78 > > +    /* skip delimiter */\r
  79 > > +    arg_str++;\r
  80 > \r
  81 > I think the caller should check and skip the delimiters. See my comments\r
  82 > below where this gets called.\r
  83 \r
  84 done, and error checking tightened up.\r
  85 \r
  86 > \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
  89 \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
  92 line arguments.\r
  93 \r
  94 > > +parse_option (const char *arg,\r
  95 > > +         const notmuch_opt_desc_t *options){\r
  96 > \r
  97 > Missing space between ) and {.\r
  98 \r
  99 done. but you missed some other missing spaces ;).\r
 100 \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
 104 \r
 105 done.\r
 106 \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
 110 \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
 114 \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
 118 \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
 122 \r
 123 > \r
 124 > I think notmuch_parse_args() is a complicated function enough to warrant\r
 125 > a proper description here.\r
 126 \r
 127 Done.\r
 128 \r
 129 > \r
 130 > > +int\r
 131 > > +notmuch_parse_args (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index){\r
 132 > \r
 133 > Missing space between ) and {.\r
 134 \r
 135 Done.\r
 136 \r
 137 > > +typedef struct notmuch_opt {\r
 138 > > +    int arg_id;\r
 139 > > +    int keyword_id;\r
 140 > > +    const char *string;\r
 141 > > +} notmuch_opt_t;\r
 142 > \r
 143 > You're not using this for anything, are you?\r
 144 \r
 145 Oops, deleted.\r
 146 > \r
 147 > > +\r
 148 > > +notmuch_bool_t\r
 149 > > +parse_option (const char *arg, const notmuch_opt_desc_t* options);\r
 150 > > +\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
 155 > \r
 156 > Is there a good reason the above are not static?\r
 157 \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
 161 \r
 162 Thanks for the review! Updated series to follow.\r
 163 \r
 164 d\r
 165 \r
 166 \r
 167 \r