2b/edf8fa01440cecdb55820f0c324af3b56ac365

   1 Return-Path: <m.walters@qmul.ac.uk>\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 1664B431FB6\r
   6         for <notmuch@notmuchmail.org>; Wed, 22 Oct 2014 03:44:28 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -1.098\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
  12         tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
  13         NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id GORAoli-E1LV for <notmuch@notmuchmail.org>;\r
  17         Wed, 22 Oct 2014 03:44:20 -0700 (PDT)\r
  18 Received: from mail1.qmul.ac.uk (mail1.qmul.ac.uk [138.37.6.7])\r
  19         (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id CC156431FAE\r
  22         for <notmuch@notmuchmail.org>; Wed, 22 Oct 2014 03:44:19 -0700 (PDT)\r
  23 Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
  24         by mail1.qmul.ac.uk with esmtp (Exim 4.71)\r
  25         (envelope-from <m.walters@qmul.ac.uk>)\r
  26         id 1XgtOm-0003ZZ-Bs; Wed, 22 Oct 2014 11:44:14 +0100\r
  27 Received: from 5751dfa2.skybroadband.com ([87.81.223.162] helo=localhost)\r
  28         by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.71)\r
  29         (envelope-from <m.walters@qmul.ac.uk>)\r
  30         id 1XgtOm-0007vI-0u; Wed, 22 Oct 2014 11:44:12 +0100\r
  31 From: Mark Walters <markwalters1009@gmail.com>\r
  32 To: Michal Sojka <sojkam1@fel.cvut.cz>, notmuch@notmuchmail.org\r
  33 Subject: Re: [PATCH v3 1/4] cli: Refactor option passing in the search command\r
  34 In-Reply-To: <1413150093-8383-2-git-send-email-sojkam1@fel.cvut.cz>\r
  35 References: <87zjd51phx.fsf@steelpick.2x.cz>\r
  36         <1413150093-8383-1-git-send-email-sojkam1@fel.cvut.cz>\r
  37         <1413150093-8383-2-git-send-email-sojkam1@fel.cvut.cz>\r
  38 User-Agent: Notmuch/0.18.1+86~gef5e66a (http://notmuchmail.org) Emacs/23.4.1\r
  39         (x86_64-pc-linux-gnu)\r
  40 Date: Wed, 22 Oct 2014 11:44:11 +0100\r
  41 Message-ID: <87ppdkbczo.fsf@qmul.ac.uk>\r
  42 MIME-Version: 1.0\r
  43 Content-Type: text/plain; charset=us-ascii\r
  44 X-Sender-Host-Address: 87.81.223.162\r
  45 X-QM-Geographic: According to ripencc,\r
  46         this message was delivered by a machine in Britain (UK) (GB).\r
  47 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  48 X-QM-Body-MD5: 59cf02e9d54476dd4ee74ecb45112bca (of first 20000 bytes)\r
  49 X-SpamAssassin-Score: -0.1\r
  50 X-SpamAssassin-SpamBar: /\r
  51 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  52         determine if it is\r
  53         spam. We require at least 5.0 points to mark a message as spam.\r
  54         This message scored -0.1 points.\r
  55         Summary of the scoring: \r
  56         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  57         provider *      (markwalters1009[at]gmail.com)\r
  58         * -0.1 AWL AWL: From: address is in the auto white-list\r
  59 X-QM-Scan-Virus: ClamAV says the message is clean\r
  60 X-BeenThere: notmuch@notmuchmail.org\r
  61 X-Mailman-Version: 2.1.13\r
  62 Precedence: list\r
  63 List-Id: "Use and development of the notmuch mail system."\r
  64         <notmuch.notmuchmail.org>\r
  65 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  66         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  67 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  68 List-Post: <mailto:notmuch@notmuchmail.org>\r
  69 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  70 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  71         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  72 X-List-Received-Date: Wed, 22 Oct 2014 10:44:28 -0000\r
  73 \r
  74 On Sun, 12 Oct 2014, Michal Sojka <sojkam1@fel.cvut.cz> wrote:\r
  75 > Many functions that implement the search command need to access command\r
  76 > line options. Instead of passing each option in a separate variable, put\r
  77 > them in a structure and pass only this structure.\r
  78 >\r
  79 > This will become handy in the following patches.\r
  80 \r
  81 Hi\r
  82 \r
  83 This basically looks good to me. I have some style queries/preferences\r
  84 mentioned below but am happy to be overruled by you or others.\r
  85 \r
  86 > ---\r
  87 >  notmuch-search.c | 122 ++++++++++++++++++++++++++++---------------------------\r
  88 >  1 file changed, 62 insertions(+), 60 deletions(-)\r
  89 >\r
  90 > diff --git a/notmuch-search.c b/notmuch-search.c\r
  91 > index bc9be45..5ac2a26 100644\r
  92 > --- a/notmuch-search.c\r
  93 > +++ b/notmuch-search.c\r
  94 > @@ -30,6 +30,16 @@ typedef enum {\r
  95 >      OUTPUT_TAGS\r
  96 >  } output_t;\r
  97 >  \r
  98 > +typedef struct {\r
  99 > +    sprinter_t *format;\r
 100 > +    notmuch_query_t *query;\r
 101 > +    notmuch_sort_t sort;\r
 102 > +    output_t output;\r
 103 > +    int offset;\r
 104 > +    int limit;\r
 105 > +    int dupe;\r
 106 > +} search_options_t;\r
 107 > +\r
 108 \r
 109 I don't think of format as being a "search option", whereas I do think\r
 110 of all the others as search options. So I would prefer to omit format\r
 111 from search_options_t and pass it explicitly.\r
 112 \r
 113 >  /* Return two stable query strings that identify exactly the matched\r
 114 >   * and unmatched messages currently in thread.  If there are no\r
 115 >   * matched or unmatched messages, the returned buffers will be\r
 116 > @@ -70,46 +80,42 @@ get_thread_query (notmuch_thread_t *thread,\r
 117 >  }\r
 118 >  \r
 119 >  static int\r
 120 > -do_search_threads (sprinter_t *format,\r
 121 > -                notmuch_query_t *query,\r
 122 > -                notmuch_sort_t sort,\r
 123 > -                output_t output,\r
 124 > -                int offset,\r
 125 > -                int limit)\r
 126 > +do_search_threads (search_options_t *o)\r
 127 \r
 128 Personally, I prefer longer variable names (and names which aren't 'o'):\r
 129 even just opt would be a definite improvement in my opinion.\r
 130 \r
 131 >  {\r
 132 >      notmuch_thread_t *thread;\r
 133 >      notmuch_threads_t *threads;\r
 134 >      notmuch_tags_t *tags;\r
 135 > +    sprinter_t *format = o->format;\r
 136 >      time_t date;\r
 137 >      int i;\r
 138 >  \r
 139 > -    if (offset < 0) {\r
 140 > -     offset += notmuch_query_count_threads (query);\r
 141 > -     if (offset < 0)\r
 142 > -         offset = 0;\r
 143 > +    if (o->offset < 0) {\r
 144 > +     o->offset += notmuch_query_count_threads (o->query);\r
 145 > +     if (o->offset < 0)\r
 146 > +         o->offset = 0;\r
 147 >      }\r
 148 >  \r
 149 > -    threads = notmuch_query_search_threads (query);\r
 150 > +    threads = notmuch_query_search_threads (o->query);\r
 151 >      if (threads == NULL)\r
 152 >       return 1;\r
 153 >  \r
 154 >      format->begin_list (format);\r
 155 >  \r
 156 >      for (i = 0;\r
 157 > -      notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit);\r
 158 > +      notmuch_threads_valid (threads) && (o->limit < 0 || i < o->offset + o->limit);\r
 159 >        notmuch_threads_move_to_next (threads), i++)\r
 160 >      {\r
 161 >       thread = notmuch_threads_get (threads);\r
 162 >  \r
 163 > -     if (i < offset) {\r
 164 > +     if (i < o->offset) {\r
 165 >           notmuch_thread_destroy (thread);\r
 166 >           continue;\r
 167 >       }\r
 168 >  \r
 169 > -     if (output == OUTPUT_THREADS) {\r
 170 > +     if (o->output == OUTPUT_THREADS) {\r
 171 >           format->set_prefix (format, "thread");\r
 172 >           format->string (format,\r
 173 > -                         notmuch_thread_get_thread_id (thread));\r
 174 > +                            notmuch_thread_get_thread_id (thread));\r
 175 >           format->separator (format);\r
 176 >       } else { /* output == OUTPUT_SUMMARY */\r
 177 >           void *ctx_quote = talloc_new (thread);\r
 178 > @@ -123,7 +129,7 @@ do_search_threads (sprinter_t *format,\r
 179 >  \r
 180 >           format->begin_map (format);\r
 181 >  \r
 182 > -         if (sort == NOTMUCH_SORT_OLDEST_FIRST)\r
 183 > +         if (o->sort == NOTMUCH_SORT_OLDEST_FIRST)\r
 184 >               date = notmuch_thread_get_oldest_date (thread);\r
 185 >           else\r
 186 >               date = notmuch_thread_get_newest_date (thread);\r
 187 > @@ -215,40 +221,36 @@ do_search_threads (sprinter_t *format,\r
 188 >  }\r
 189 >  \r
 190 >  static int\r
 191 > -do_search_messages (sprinter_t *format,\r
 192 > -                 notmuch_query_t *query,\r
 193 > -                 output_t output,\r
 194 > -                 int offset,\r
 195 > -                 int limit,\r
 196 > -                 int dupe)\r
 197 > +do_search_messages (search_options_t *o)\r
 198 >  {\r
 199 >      notmuch_message_t *message;\r
 200 >      notmuch_messages_t *messages;\r
 201 >      notmuch_filenames_t *filenames;\r
 202 > +    sprinter_t *format = o->format;\r
 203 >      int i;\r
 204 >  \r
 205 > -    if (offset < 0) {\r
 206 > -     offset += notmuch_query_count_messages (query);\r
 207 > -     if (offset < 0)\r
 208 > -         offset = 0;\r
 209 > +    if (o->offset < 0) {\r
 210 > +     o->offset += notmuch_query_count_messages (o->query);\r
 211 > +     if (o->offset < 0)\r
 212 > +         o->offset = 0;\r
 213 >      }\r
 214 >  \r
 215 > -    messages = notmuch_query_search_messages (query);\r
 216 > +    messages = notmuch_query_search_messages (o->query);\r
 217 >      if (messages == NULL)\r
 218 >       return 1;\r
 219 >  \r
 220 >      format->begin_list (format);\r
 221 >  \r
 222 >      for (i = 0;\r
 223 > -      notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit);\r
 224 > +      notmuch_messages_valid (messages) && (o->limit < 0 || i < o->offset + o->limit);\r
 225 >        notmuch_messages_move_to_next (messages), i++)\r
 226 >      {\r
 227 > -     if (i < offset)\r
 228 > +     if (i < o->offset)\r
 229 >           continue;\r
 230 >  \r
 231 >       message = notmuch_messages_get (messages);\r
 232 >  \r
 233 > -     if (output == OUTPUT_FILES) {\r
 234 > +     if (o->output == OUTPUT_FILES) {\r
 235 >           int j;\r
 236 >           filenames = notmuch_message_get_filenames (message);\r
 237 >  \r
 238 > @@ -256,7 +258,7 @@ do_search_messages (sprinter_t *format,\r
 239 >                notmuch_filenames_valid (filenames);\r
 240 >                notmuch_filenames_move_to_next (filenames), j++)\r
 241 >           {\r
 242 > -             if (dupe < 0 || dupe == j) {\r
 243 > +             if (o->dupe < 0 || o->dupe == j) {\r
 244 >                   format->string (format, notmuch_filenames_get (filenames));\r
 245 >                   format->separator (format);\r
 246 >               }\r
 247 > @@ -333,16 +335,16 @@ int\r
 248 >  notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])\r
 249 >  {\r
 250 >      notmuch_database_t *notmuch;\r
 251 > -    notmuch_query_t *query;\r
 252 > +    search_options_t o = {\r
 253 > +     .sort = NOTMUCH_SORT_NEWEST_FIRST,\r
 254 > +     .output = OUTPUT_SUMMARY,\r
 255 > +     .offset = 0,\r
 256 > +     .limit = -1, /* unlimited */\r
 257 > +     .dupe = -1,\r
 258 > +    };\r
 259 >      char *query_str;\r
 260 > -    notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST;\r
 261 > -    sprinter_t *format = NULL;\r
 262 >      int opt_index, ret;\r
 263 > -    output_t output = OUTPUT_SUMMARY;\r
 264 > -    int offset = 0;\r
 265 > -    int limit = -1; /* unlimited */\r
 266 >      notmuch_exclude_t exclude = NOTMUCH_EXCLUDE_TRUE;\r
 267 > -    int dupe = -1;\r
 268 >      unsigned int i;\r
 269 >  \r
 270 >      enum {\r
 271 > @@ -353,7 +355,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])\r
 272 >      } format_sel = NOTMUCH_FORMAT_TEXT;\r
 273 >  \r
 274 >      notmuch_opt_desc_t options[] = {\r
 275 > -     { NOTMUCH_OPT_KEYWORD, &sort, "sort", 's',\r
 276 > +     { NOTMUCH_OPT_KEYWORD, &o.sort, "sort", 's',\r
 277 >         (notmuch_keyword_t []){ { "oldest-first", NOTMUCH_SORT_OLDEST_FIRST },\r
 278 >                                 { "newest-first", NOTMUCH_SORT_NEWEST_FIRST },\r
 279 >                                 { 0, 0 } } },\r
 280 > @@ -364,7 +366,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])\r
 281 >                                 { "text0", NOTMUCH_FORMAT_TEXT0 },\r
 282 >                                 { 0, 0 } } },\r
 283 >       { NOTMUCH_OPT_INT, &notmuch_format_version, "format-version", 0, 0 },\r
 284 > -     { NOTMUCH_OPT_KEYWORD, &output, "output", 'o',\r
 285 > +     { NOTMUCH_OPT_KEYWORD, &o.output, "output", 'o',\r
 286 >         (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },\r
 287 >                                 { "threads", OUTPUT_THREADS },\r
 288 >                                 { "messages", OUTPUT_MESSAGES },\r
 289 > @@ -377,9 +379,9 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])\r
 290 >                                    { "flag", NOTMUCH_EXCLUDE_FLAG },\r
 291 >                                    { "all", NOTMUCH_EXCLUDE_ALL },\r
 292 >                                    { 0, 0 } } },\r
 293 > -     { NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },\r
 294 > -     { NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },\r
 295 > -     { NOTMUCH_OPT_INT, &dupe, "duplicate", 'D', 0  },\r
 296 > +     { NOTMUCH_OPT_INT, &o.offset, "offset", 'O', 0 },\r
 297 > +     { NOTMUCH_OPT_INT, &o.limit, "limit", 'L', 0  },\r
 298 > +     { NOTMUCH_OPT_INT, &o.dupe, "duplicate", 'D', 0  },\r
 299 >       { 0, 0, 0, 0, 0 }\r
 300 >      };\r
 301 >  \r
 302 > @@ -389,20 +391,20 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])\r
 303 >  \r
 304 >      switch (format_sel) {\r
 305 >      case NOTMUCH_FORMAT_TEXT:\r
 306 > -     format = sprinter_text_create (config, stdout);\r
 307 > +     o.format = sprinter_text_create (config, stdout);\r
 308 >       break;\r
 309 >      case NOTMUCH_FORMAT_TEXT0:\r
 310 > -     if (output == OUTPUT_SUMMARY) {\r
 311 > +     if (o.output == OUTPUT_SUMMARY) {\r
 312 >           fprintf (stderr, "Error: --format=text0 is not compatible with --output=summary.\n");\r
 313 >           return EXIT_FAILURE;\r
 314 >       }\r
 315 > -     format = sprinter_text0_create (config, stdout);\r
 316 > +     o.format = sprinter_text0_create (config, stdout);\r
 317 >       break;\r
 318 >      case NOTMUCH_FORMAT_JSON:\r
 319 > -     format = sprinter_json_create (config, stdout);\r
 320 > +     o.format = sprinter_json_create (config, stdout);\r
 321 >       break;\r
 322 >      case NOTMUCH_FORMAT_SEXP:\r
 323 > -     format = sprinter_sexp_create (config, stdout);\r
 324 > +     o.format = sprinter_sexp_create (config, stdout);\r
 325 >       break;\r
 326 >      default:\r
 327 >       /* this should never happen */\r
 328 > @@ -425,15 +427,15 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])\r
 329 >       return EXIT_FAILURE;\r
 330 >      }\r
 331 >  \r
 332 > -    query = notmuch_query_create (notmuch, query_str);\r
 333 > -    if (query == NULL) {\r
 334 > +    o.query = notmuch_query_create (notmuch, query_str);\r
 335 > +    if (o.query == NULL) {\r
 336 >       fprintf (stderr, "Out of memory\n");\r
 337 >       return EXIT_FAILURE;\r
 338 >      }\r
 339 >  \r
 340 > -    notmuch_query_set_sort (query, sort);\r
 341 > +    notmuch_query_set_sort (o.query, o.sort);\r
 342 >  \r
 343 > -    if (exclude == NOTMUCH_EXCLUDE_FLAG && output != OUTPUT_SUMMARY) {\r
 344 > +    if (exclude == NOTMUCH_EXCLUDE_FLAG && o.output != OUTPUT_SUMMARY) {\r
 345 >       /* If we are not doing summary output there is nowhere to\r
 346 >        * print the excluded flag so fall back on including the\r
 347 >        * excluded messages. */\r
 348 > @@ -448,29 +450,29 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])\r
 349 >       search_exclude_tags = notmuch_config_get_search_exclude_tags\r
 350 >           (config, &search_exclude_tags_length);\r
 351 >       for (i = 0; i < search_exclude_tags_length; i++)\r
 352 > -         notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);\r
 353 > -     notmuch_query_set_omit_excluded (query, exclude);\r
 354 > +         notmuch_query_add_tag_exclude (o.query, search_exclude_tags[i]);\r
 355 > +     notmuch_query_set_omit_excluded (o.query, exclude);\r
 356 >      }\r
 357 >  \r
 358 > -    switch (output) {\r
 359 > +    switch (o.output) {\r
 360 >      default:\r
 361 >      case OUTPUT_SUMMARY:\r
 362 >      case OUTPUT_THREADS:\r
 363 > -     ret = do_search_threads (format, query, sort, output, offset, limit);\r
 364 > +     ret = do_search_threads (&o);\r
 365 >       break;\r
 366 >      case OUTPUT_MESSAGES:\r
 367 >      case OUTPUT_FILES:\r
 368 > -     ret = do_search_messages (format, query, output, offset, limit, dupe);\r
 369 > +     ret = do_search_messages (&o);\r
 370 >       break;\r
 371 >      case OUTPUT_TAGS:\r
 372 > -     ret = do_search_tags (notmuch, format, query);\r
 373 > +     ret = do_search_tags (notmuch, o.format, o.query);\r
 374 \r
 375 It feels slightly strange to me to have do_search_tags taking a\r
 376 different argument style than the other two do_search_* functions so I\r
 377 would slightly prefer making it do_search_tags (&o).\r
 378 \r
 379 Obviously all of these are trivialities, so as I said above, I am happy\r
 380 to be overruled.\r
 381 \r
 382 Best wishes\r
 383 \r
 384 Mark\r
 385 \r
 386 \r
 387 \r
 388 >       break;\r
 389 >      }\r
 390 >  \r
 391 > -    notmuch_query_destroy (query);\r
 392 > +    notmuch_query_destroy (o.query);\r
 393 >      notmuch_database_destroy (notmuch);\r
 394 >  \r
 395 > -    talloc_free (format);\r
 396 > +    talloc_free (o.format);\r
 397 >  \r
 398 >      return ret ? EXIT_FAILURE : EXIT_SUCCESS;\r
 399 >  }\r
 400 > -- \r
 401 > 2.1.1\r
 402 >\r
 403 > _______________________________________________\r
 404 > notmuch mailing list\r
 405 > notmuch@notmuchmail.org\r
 406 > http://notmuchmail.org/mailman/listinfo/notmuch\r