ff/1a645d55227573220e825898495f03af69082c

   1 Return-Path: <jani@nikula.org>\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 6EDA0431FAF\r
   6         for <notmuch@notmuchmail.org>; Sat,  4 Feb 2012 00:59:51 -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: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 X9bDvp9qyh64 for <notmuch@notmuchmail.org>;\r
  16         Sat,  4 Feb 2012 00:59:50 -0800 (PST)\r
  17 Received: from mail-lpp01m010-f53.google.com (mail-lpp01m010-f53.google.com\r
  18         [209.85.215.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id C472B431FAE\r
  21         for <notmuch@notmuchmail.org>; Sat,  4 Feb 2012 00:59:49 -0800 (PST)\r
  22 Received: by lahd3 with SMTP id d3so2445024lah.26\r
  23         for <notmuch@notmuchmail.org>; Sat, 04 Feb 2012 00:59:46 -0800 (PST)\r
  24 Received: by 10.152.147.38 with SMTP id th6mr5256509lab.47.1328345986655;\r
  25         Sat, 04 Feb 2012 00:59:46 -0800 (PST)\r
  26 Received: from localhost (dsl-hkibrasgw4-fe50f800-253.dhcp.inet.fi.\r
  27         [84.248.80.253])\r
  28         by mx.google.com with ESMTPS id ny9sm4469597lab.6.2012.02.04.00.59.43\r
  29         (version=SSLv3 cipher=OTHER); Sat, 04 Feb 2012 00:59:45 -0800 (PST)\r
  30 From: Jani Nikula <jani@nikula.org>\r
  31 To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org\r
  32 Subject: Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument\r
  33         parser\r
  34 In-Reply-To: <87aa4zo4cf.fsf@qmul.ac.uk>\r
  35 References:\r
  36  <df2e9bd7682e9c81e1b205cc1d92a1526cdd1a9b.1328308635.git.jani@nikula.org>\r
  37         <87aa4zo4cf.fsf@qmul.ac.uk>\r
  38 User-Agent: Notmuch/0.11+139~g4340989 (http://notmuchmail.org) Emacs/23.3.1\r
  39         (i686-pc-linux-gnu)\r
  40 Date: Sat, 04 Feb 2012 10:59:42 +0200\r
  41 Message-ID: <87haz755z5.fsf@nikula.org>\r
  42 MIME-Version: 1.0\r
  43 Content-Type: text/plain; charset=us-ascii\r
  44 X-BeenThere: notmuch@notmuchmail.org\r
  45 X-Mailman-Version: 2.1.13\r
  46 Precedence: list\r
  47 List-Id: "Use and development of the notmuch mail system."\r
  48         <notmuch.notmuchmail.org>\r
  49 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  50         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  51 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  52 List-Post: <mailto:notmuch@notmuchmail.org>\r
  53 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  54 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  55         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  56 X-List-Received-Date: Sat, 04 Feb 2012 08:59:51 -0000\r
  57 \r
  58 On Sat, 04 Feb 2012 00:00:00 +0000, Mark Walters <markwalters1009@gmail.com> wrote:\r
  59 > \r
  60 > On Sat,  4 Feb 2012 00:41:08 +0200, Jani Nikula <jani@nikula.org> wrote:\r
  61 > > Use the new notmuch argument parser to handle arguments in "notmuch\r
  62 > > show". There are two corner case functional changes:\r
  63 > > \r
  64 > > 1) Also set params.raw = 1 when defaulting to raw format when part is\r
  65 > >    requested but format is not specified.\r
  66 > > \r
  67 > > 2) Do not set params.decrypt if crypto context creation fails.\r
  68 > \r
  69 > This looks great. As far as I can see it is fine (I haven't run or even\r
  70 > compiled it yet). I only have two query/nits below.\r
  71 > \r
  72 > Best wishes\r
  73 > \r
  74 > Mark\r
  75 > \r
  76 > > Signed-off-by: Jani Nikula <jani@nikula.org>\r
  77 > > ---\r
  78 > >  notmuch-show.c |  153 +++++++++++++++++++++++++++++---------------------------\r
  79 > >  1 files changed, 79 insertions(+), 74 deletions(-)\r
  80 > > \r
  81 > > diff --git a/notmuch-show.c b/notmuch-show.c\r
  82 > > index dec799c..f93e121 100644\r
  83 > > --- a/notmuch-show.c\r
  84 > > +++ b/notmuch-show.c\r
  85 > > @@ -1049,6 +1049,14 @@ do_show (void *ctx,\r
  86 > >      return 0;\r
  87 > >  }\r
  88 > >  \r
  89 > > +enum {\r
  90 > > +    NOTMUCH_FORMAT_NOT_SPECIFIED,\r
  91 > > +    NOTMUCH_FORMAT_JSON,\r
  92 > > +    NOTMUCH_FORMAT_TEXT,\r
  93 > > +    NOTMUCH_FORMAT_MBOX,\r
  94 > > +    NOTMUCH_FORMAT_RAW\r
  95 > > +};\r
  96 > > +\r
  97 > >  int\r
  98 > >  notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))\r
  99 > >  {\r
 100 > > @@ -1056,92 +1064,98 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))\r
 101 > >      notmuch_database_t *notmuch;\r
 102 > >      notmuch_query_t *query;\r
 103 > >      char *query_string;\r
 104 > > -    char *opt;\r
 105 > > +    int opt_index;\r
 106 > >      const notmuch_show_format_t *format = &format_text;\r
 107 > \r
 108 > I think you don't need the default value here. If you think it is\r
 109 > clearer with the default then that is fine. I think I slightly prefer\r
 110 > without since in some cases the default is raw but entirely up to you.\r
 111 \r
 112 I actually dropped this at first, but the compiler has no way of knowing\r
 113 that all the cases are covered in the switch, and thinks format may be\r
 114 uninitialized. I was wondering whether to have a default case in the\r
 115 switch (which would be just to make the compiler happy), but settled on\r
 116 this instead.\r
 117 \r
 118 > \r
 119 > > -    notmuch_show_params_t params;\r
 120 > > -    int mbox = 0;\r
 121 > > -    int format_specified = 0;\r
 122 > > -    int i;\r
 123 > > +    notmuch_show_params_t params = { .part = -1 };\r
 124 > \r
 125 > Does this initialize all the other params bits to zero/NULL?  \r
 126 \r
 127 Yes. It's called a "designated initializer for aggregate type" if you\r
 128 want to look it up.\r
 129 \r
 130 Thanks for the review.\r
 131 \r
 132 \r
 133 BR,\r
 134 Jani.\r
 135 \r
 136 > \r
 137 > \r
 138 > \r
 139 > \r
 140 > > +    int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;\r
 141 > > +    notmuch_bool_t decrypt = FALSE;\r
 142 > > +    notmuch_bool_t verify = FALSE;\r
 143 > > +    notmuch_bool_t entire_thread = FALSE;\r
 144 > > +\r
 145 > > +    notmuch_opt_desc_t options[] = {\r
 146 > > +   { NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',\r
 147 > > +     (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },\r
 148 > > +                             { "text", NOTMUCH_FORMAT_TEXT },\r
 149 > > +                             { "mbox", NOTMUCH_FORMAT_MBOX },\r
 150 > > +                             { "raw", NOTMUCH_FORMAT_RAW },\r
 151 > > +                             { 0, 0 } } },\r
 152 > > +   { NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },\r
 153 > > +   { NOTMUCH_OPT_BOOLEAN, &entire_thread, "entire-thread", 't', 0 },\r
 154 > > +   { NOTMUCH_OPT_BOOLEAN, &decrypt, "decrypt", 'd', 0 },\r
 155 > > +   { NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },\r
 156 > > +   { 0, 0, 0, 0, 0 }\r
 157 > > +    };\r
 158 > > +\r
 159 > > +    opt_index = parse_arguments (argc, argv, options, 1);\r
 160 > > +    if (opt_index < 0) {\r
 161 > > +   /* diagnostics already printed */\r
 162 > > +   return 1;\r
 163 > > +    }\r
 164 > >  \r
 165 > > -    params.entire_thread = 0;\r
 166 > > -    params.raw = 0;\r
 167 > > -    params.part = -1;\r
 168 > > -    params.cryptoctx = NULL;\r
 169 > > -    params.decrypt = 0;\r
 170 > > +    params.entire_thread = entire_thread;\r
 171 > >  \r
 172 > > -    argc--; argv++; /* skip subcommand argument */\r
 173 > > +    if (format_sel == NOTMUCH_FORMAT_NOT_SPECIFIED) {\r
 174 > > +   /* if part was requested and format was not specified, use format=raw */\r
 175 > > +   if (params.part >= 0)\r
 176 > > +       format_sel = NOTMUCH_FORMAT_RAW;\r
 177 > > +   else\r
 178 > > +       format_sel = NOTMUCH_FORMAT_TEXT;\r
 179 > > +    }\r
 180 > >  \r
 181 > > -    for (i = 0; i < argc && argv[i][0] == '-'; i++) {\r
 182 > > -   if (strcmp (argv[i], "--") == 0) {\r
 183 > > -       i++;\r
 184 > > -       break;\r
 185 > > +    switch (format_sel) {\r
 186 > > +    case NOTMUCH_FORMAT_JSON:\r
 187 > > +   format = &format_json;\r
 188 > > +   params.entire_thread = 1;\r
 189 > > +   break;\r
 190 > > +    case NOTMUCH_FORMAT_TEXT:\r
 191 > > +   format = &format_text;\r
 192 > > +   break;\r
 193 > > +    case NOTMUCH_FORMAT_MBOX:\r
 194 > > +   if (params.part > 0) {\r
 195 > > +       fprintf (stderr, "Error: specifying parts is incompatible with mbox output format.\n");\r
 196 > > +       return 1;\r
 197 > >     }\r
 198 > > -   if (STRNCMP_LITERAL (argv[i], "--format=") == 0) {\r
 199 > > -       opt = argv[i] + sizeof ("--format=") - 1;\r
 200 > > -       if (strcmp (opt, "text") == 0) {\r
 201 > > -           format = &format_text;\r
 202 > > -       } else if (strcmp (opt, "json") == 0) {\r
 203 > > -           format = &format_json;\r
 204 > > -           params.entire_thread = 1;\r
 205 > > -       } else if (strcmp (opt, "mbox") == 0) {\r
 206 > > -           format = &format_mbox;\r
 207 > > -           mbox = 1;\r
 208 > > -       } else if (strcmp (opt, "raw") == 0) {\r
 209 > > -           format = &format_raw;\r
 210 > > -           params.raw = 1;\r
 211 > > -       } else {\r
 212 > > -           fprintf (stderr, "Invalid value for --format: %s\n", opt);\r
 213 > > -           return 1;\r
 214 > > -       }\r
 215 > > -       format_specified = 1;\r
 216 > > -   } else if (STRNCMP_LITERAL (argv[i], "--part=") == 0) {\r
 217 > > -       params.part = atoi(argv[i] + sizeof ("--part=") - 1);\r
 218 > > -   } else if (STRNCMP_LITERAL (argv[i], "--entire-thread") == 0) {\r
 219 > > -       params.entire_thread = 1;\r
 220 > > -   } else if ((STRNCMP_LITERAL (argv[i], "--verify") == 0) ||\r
 221 > > -              (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)) {\r
 222 > > -       if (params.cryptoctx == NULL) {\r
 223 > > +   format = &format_mbox;\r
 224 > > +   break;\r
 225 > > +    case NOTMUCH_FORMAT_RAW:\r
 226 > > +   format = &format_raw;\r
 227 > > +   /* If --format=raw specified without specifying part, we can only\r
 228 > > +    * output single message, so set part=0 */\r
 229 > > +   if (params.part < 0)\r
 230 > > +       params.part = 0;\r
 231 > > +   params.raw = 1;\r
 232 > > +   break;\r
 233 > > +    }\r
 234 > > +\r
 235 > > +    if (decrypt || verify) {\r
 236 > >  #ifdef GMIME_ATLEAST_26\r
 237 > > -           /* TODO: GMimePasswordRequestFunc */\r
 238 > > -           if (NULL == (params.cryptoctx = g_mime_gpg_context_new(NULL, "gpg")))\r
 239 > > +   /* TODO: GMimePasswordRequestFunc */\r
 240 > > +   params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");\r
 241 > >  #else\r
 242 > > -           GMimeSession* session = g_object_new(g_mime_session_get_type(), NULL);\r
 243 > > -           if (NULL == (params.cryptoctx = g_mime_gpg_context_new(session, "gpg")))\r
 244 > > -#endif\r
 245 > > -               fprintf (stderr, "Failed to construct gpg context.\n");\r
 246 > > -           else\r
 247 > > -               g_mime_gpg_context_set_always_trust((GMimeGpgContext*)params.cryptoctx, FALSE);\r
 248 > > -#ifndef GMIME_ATLEAST_26\r
 249 > > -           g_object_unref (session);\r
 250 > > -           session = NULL;\r
 251 > > +   GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);\r
 252 > > +   params.cryptoctx = g_mime_gpg_context_new (session, "gpg");\r
 253 > >  #endif\r
 254 > > -       }\r
 255 > > -       if (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)\r
 256 > > -           params.decrypt = 1;\r
 257 > > +   if (params.cryptoctx) {\r
 258 > > +       g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);\r
 259 > > +       params.decrypt = decrypt;\r
 260 > >     } else {\r
 261 > > -       fprintf (stderr, "Unrecognized option: %s\n", argv[i]);\r
 262 > > -       return 1;\r
 263 > > +       fprintf (stderr, "Failed to construct gpg context.\n");\r
 264 > >     }\r
 265 > > +#ifndef GMIME_ATLEAST_26\r
 266 > > +   g_object_unref (session);\r
 267 > > +#endif\r
 268 > >      }\r
 269 > >  \r
 270 > > -    argc -= i;\r
 271 > > -    argv += i;\r
 272 > > -\r
 273 > >      config = notmuch_config_open (ctx, NULL, NULL);\r
 274 > >      if (config == NULL)\r
 275 > >     return 1;\r
 276 > >  \r
 277 > > -    query_string = query_string_from_args (ctx, argc, argv);\r
 278 > > +    query_string = query_string_from_args (ctx, argc-opt_index, argv+opt_index);\r
 279 > >      if (query_string == NULL) {\r
 280 > >     fprintf (stderr, "Out of memory\n");\r
 281 > >     return 1;\r
 282 > >      }\r
 283 > >  \r
 284 > > -    if (mbox && params.part > 0) {\r
 285 > > -   fprintf (stderr, "Error: specifying parts is incompatible with mbox output format.\n");\r
 286 > > -   return 1;\r
 287 > > -    }\r
 288 > > -\r
 289 > >      if (*query_string == '\0') {\r
 290 > >     fprintf (stderr, "Error: notmuch show requires at least one search term.\n");\r
 291 > >     return 1;\r
 292 > > @@ -1158,15 +1172,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))\r
 293 > >     return 1;\r
 294 > >      }\r
 295 > >  \r
 296 > > -    /* if part was requested and format was not specified, use format=raw */\r
 297 > > -    if (params.part >= 0 && !format_specified)\r
 298 > > -   format = &format_raw;\r
 299 > > -\r
 300 > > -    /* If --format=raw specified without specifying part, we can only\r
 301 > > -     * output single message, so set part=0 */\r
 302 > > -    if (params.raw && params.part < 0)\r
 303 > > -   params.part = 0;\r
 304 > > -\r
 305 > >      if (params.part >= 0)\r
 306 > >     return do_show_single (ctx, query, format, &params);\r
 307 > >      else\r
 308 > > -- \r
 309 > > 1.7.5.4\r
 310 > > \r
 311 > > _______________________________________________\r
 312 > > notmuch mailing list\r
 313 > > notmuch@notmuchmail.org\r
 314 > > http://notmuchmail.org/mailman/listinfo/notmuch\r