c7/8e0d22ceb7663bd1788a28ab74f660c693a9a1

   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 E4DBF431FAF\r
   6         for <notmuch@notmuchmail.org>; Sun,  5 Feb 2012 23:54:07 -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 YBjgbDAsYSkw for <notmuch@notmuchmail.org>;\r
  16         Sun,  5 Feb 2012 23:54:06 -0800 (PST)\r
  17 Received: from mail-qw0-f46.google.com (mail-qw0-f46.google.com\r
  18         [209.85.216.46]) (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 C6A5C431FAE\r
  21         for <notmuch@notmuchmail.org>; Sun,  5 Feb 2012 23:54:06 -0800 (PST)\r
  22 Received: by qadc10 with SMTP id c10so2257013qad.5\r
  23         for <notmuch@notmuchmail.org>; Sun, 05 Feb 2012 23:54:06 -0800 (PST)\r
  24 Received: by 10.224.197.5 with SMTP id ei5mr19060677qab.73.1328514846090;\r
  25         Sun, 05 Feb 2012 23:54:06 -0800 (PST)\r
  26 Received: from localhost (nikula.org. [92.243.24.172])\r
  27         by mx.google.com with ESMTPS id dk2sm32206242qab.12.2012.02.05.23.54.04\r
  28         (version=SSLv3 cipher=OTHER); Sun, 05 Feb 2012 23:54:05 -0800 (PST)\r
  29 From: Jani Nikula <jani@nikula.org>\r
  30 To: Austin Clements <amdragon@MIT.EDU>\r
  31 Subject: Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument\r
  32         parser\r
  33 In-Reply-To: <20120206041205.GP10898@mit.edu>\r
  34 References:\r
  35  <df2e9bd7682e9c81e1b205cc1d92a1526cdd1a9b.1328308635.git.jani@nikula.org>\r
  36         <20120206041205.GP10898@mit.edu>\r
  37 User-Agent: Notmuch/0.10.2+187~g43d4f26 (http://notmuchmail.org) Emacs/23.1.1\r
  38         (i686-pc-linux-gnu)\r
  39 Date: Mon, 06 Feb 2012 07:54:02 +0000\r
  40 Message-ID: <87ty34bdnp.fsf@nikula.org>\r
  41 MIME-Version: 1.0\r
  42 Content-Type: text/plain; charset=us-ascii\r
  43 Cc: notmuch@notmuchmail.org\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: Mon, 06 Feb 2012 07:54:08 -0000\r
  57 \r
  58 On Sun, 5 Feb 2012 23:12:05 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  59 > Yikes.  I don't envy you this patch.  Two minor nits, otherwise this\r
  60 > and the next patch LGTM.\r
  61 \r
  62 Thanks for the review. I'll roll another version, I think it will be\r
  63 better with your suggestions incorporated.\r
  64 \r
  65 > Quoth Jani Nikula on Feb 04 at 12:41 am:\r
  66 > > Use the new notmuch argument parser to handle arguments in "notmuch\r
  67 > > show". There are two corner case functional changes:\r
  68 > > \r
  69 > > 1) Also set params.raw = 1 when defaulting to raw format when part is\r
  70 > >    requested but format is not specified.\r
  71 > \r
  72 > Huh.  So "notmuch show --part=0 <query>" was broken before.\r
  73 \r
  74 Hmm, yes, it seems so. Do you think I should make this a separate fix?\r
  75 \r
  76 BTW an alternative would be to require setting --format if --part is\r
  77 specified, and not adapt the default format depending on --part.\r
  78 \r
  79 > > 2) Do not set params.decrypt if crypto context creation fails.\r
  80 > \r
  81 > Technically this also behaves differently if given multiple --format\r
  82 > arguments, but I'll let that slide.\r
  83 \r
  84 Ugh, right, the old parsing was broken also that way. Luckily that falls\r
  85 in the corner case department too.\r
  86 \r
  87 > > \r
  88 > > Signed-off-by: Jani Nikula <jani@nikula.org>\r
  89 > > ---\r
  90 > >  notmuch-show.c |  153 +++++++++++++++++++++++++++++---------------------------\r
  91 > >  1 files changed, 79 insertions(+), 74 deletions(-)\r
  92 > > \r
  93 > > diff --git a/notmuch-show.c b/notmuch-show.c\r
  94 > > index dec799c..f93e121 100644\r
  95 > > --- a/notmuch-show.c\r
  96 > > +++ b/notmuch-show.c\r
  97 > > @@ -1049,6 +1049,14 @@ do_show (void *ctx,\r
  98 > >      return 0;\r
  99 > >  }\r
 100 > >  \r
 101 > > +enum {\r
 102 > > +    NOTMUCH_FORMAT_NOT_SPECIFIED,\r
 103 > > +    NOTMUCH_FORMAT_JSON,\r
 104 > > +    NOTMUCH_FORMAT_TEXT,\r
 105 > > +    NOTMUCH_FORMAT_MBOX,\r
 106 > > +    NOTMUCH_FORMAT_RAW\r
 107 > > +};\r
 108 > > +\r
 109 > >  int\r
 110 > >  notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))\r
 111 > >  {\r
 112 > > @@ -1056,92 +1064,98 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))\r
 113 > >      notmuch_database_t *notmuch;\r
 114 > >      notmuch_query_t *query;\r
 115 > >      char *query_string;\r
 116 > > -    char *opt;\r
 117 > > +    int opt_index;\r
 118 > >      const notmuch_show_format_t *format = &format_text;\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 > > +    int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;\r
 125 > > +    notmuch_bool_t decrypt = FALSE;\r
 126 > > +    notmuch_bool_t verify = FALSE;\r
 127 > > +    notmuch_bool_t entire_thread = FALSE;\r
 128 > > +\r
 129 > > +    notmuch_opt_desc_t options[] = {\r
 130 > > +   { NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',\r
 131 > > +     (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },\r
 132 > > +                             { "text", NOTMUCH_FORMAT_TEXT },\r
 133 > > +                             { "mbox", NOTMUCH_FORMAT_MBOX },\r
 134 > > +                             { "raw", NOTMUCH_FORMAT_RAW },\r
 135 > > +                             { 0, 0 } } },\r
 136 > > +   { NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },\r
 137 > > +   { NOTMUCH_OPT_BOOLEAN, &entire_thread, "entire-thread", 't', 0 },\r
 138 > > +   { NOTMUCH_OPT_BOOLEAN, &decrypt, "decrypt", 'd', 0 },\r
 139 > > +   { NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },\r
 140 > > +   { 0, 0, 0, 0, 0 }\r
 141 > > +    };\r
 142 > > +\r
 143 > > +    opt_index = parse_arguments (argc, argv, options, 1);\r
 144 > > +    if (opt_index < 0) {\r
 145 > > +   /* diagnostics already printed */\r
 146 > > +   return 1;\r
 147 > > +    }\r
 148 > >  \r
 149 > > -    params.entire_thread = 0;\r
 150 > > -    params.raw = 0;\r
 151 > > -    params.part = -1;\r
 152 > > -    params.cryptoctx = NULL;\r
 153 > > -    params.decrypt = 0;\r
 154 > > +    params.entire_thread = entire_thread;\r
 155 > \r
 156 > If you make params.entire_thread a notmuch_bool_t (instead of an int),\r
 157 > you could pass &params.entire_thread in the notmuch_opt_desc_t and get\r
 158 > rid of the local variable.\r
 159 \r
 160 You're right; I was a bit lazy and didn't consider changing\r
 161 notmuch_show_params_t. I'll change both this and params.decrypt to\r
 162 notmuch_bool_t.\r
 163 \r
 164 > \r
 165 > >  \r
 166 > > -    argc--; argv++; /* skip subcommand argument */\r
 167 > > +    if (format_sel == NOTMUCH_FORMAT_NOT_SPECIFIED) {\r
 168 > > +   /* if part was requested and format was not specified, use format=raw */\r
 169 > > +   if (params.part >= 0)\r
 170 > > +       format_sel = NOTMUCH_FORMAT_RAW;\r
 171 > > +   else\r
 172 > > +       format_sel = NOTMUCH_FORMAT_TEXT;\r
 173 > > +    }\r
 174 > >  \r
 175 > > -    for (i = 0; i < argc && argv[i][0] == '-'; i++) {\r
 176 > > -   if (strcmp (argv[i], "--") == 0) {\r
 177 > > -       i++;\r
 178 > > -       break;\r
 179 > > +    switch (format_sel) {\r
 180 > > +    case NOTMUCH_FORMAT_JSON:\r
 181 > > +   format = &format_json;\r
 182 > > +   params.entire_thread = 1;\r
 183 > > +   break;\r
 184 > > +    case NOTMUCH_FORMAT_TEXT:\r
 185 > > +   format = &format_text;\r
 186 > > +   break;\r
 187 > > +    case NOTMUCH_FORMAT_MBOX:\r
 188 > > +   if (params.part > 0) {\r
 189 > > +       fprintf (stderr, "Error: specifying parts is incompatible with mbox output format.\n");\r
 190 > > +       return 1;\r
 191 > >     }\r
 192 > > -   if (STRNCMP_LITERAL (argv[i], "--format=") == 0) {\r
 193 > > -       opt = argv[i] + sizeof ("--format=") - 1;\r
 194 > > -       if (strcmp (opt, "text") == 0) {\r
 195 > > -           format = &format_text;\r
 196 > > -       } else if (strcmp (opt, "json") == 0) {\r
 197 > > -           format = &format_json;\r
 198 > > -           params.entire_thread = 1;\r
 199 > > -       } else if (strcmp (opt, "mbox") == 0) {\r
 200 > > -           format = &format_mbox;\r
 201 > > -           mbox = 1;\r
 202 > > -       } else if (strcmp (opt, "raw") == 0) {\r
 203 > > -           format = &format_raw;\r
 204 > > -           params.raw = 1;\r
 205 > > -       } else {\r
 206 > > -           fprintf (stderr, "Invalid value for --format: %s\n", opt);\r
 207 > > -           return 1;\r
 208 > > -       }\r
 209 > > -       format_specified = 1;\r
 210 > > -   } else if (STRNCMP_LITERAL (argv[i], "--part=") == 0) {\r
 211 > > -       params.part = atoi(argv[i] + sizeof ("--part=") - 1);\r
 212 > > -   } else if (STRNCMP_LITERAL (argv[i], "--entire-thread") == 0) {\r
 213 > > -       params.entire_thread = 1;\r
 214 > > -   } else if ((STRNCMP_LITERAL (argv[i], "--verify") == 0) ||\r
 215 > > -              (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)) {\r
 216 > > -       if (params.cryptoctx == NULL) {\r
 217 > > +   format = &format_mbox;\r
 218 > > +   break;\r
 219 > > +    case NOTMUCH_FORMAT_RAW:\r
 220 > > +   format = &format_raw;\r
 221 > > +   /* If --format=raw specified without specifying part, we can only\r
 222 > > +    * output single message, so set part=0 */\r
 223 > > +   if (params.part < 0)\r
 224 > > +       params.part = 0;\r
 225 > > +   params.raw = 1;\r
 226 > > +   break;\r
 227 > > +    }\r
 228 > > +\r
 229 > > +    if (decrypt || verify) {\r
 230 > >  #ifdef GMIME_ATLEAST_26\r
 231 > > -           /* TODO: GMimePasswordRequestFunc */\r
 232 > > -           if (NULL == (params.cryptoctx = g_mime_gpg_context_new(NULL, "gpg")))\r
 233 > > +   /* TODO: GMimePasswordRequestFunc */\r
 234 > > +   params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");\r
 235 > >  #else\r
 236 > > -           GMimeSession* session = g_object_new(g_mime_session_get_type(), NULL);\r
 237 > > -           if (NULL == (params.cryptoctx = g_mime_gpg_context_new(session, "gpg")))\r
 238 > > -#endif\r
 239 > > -               fprintf (stderr, "Failed to construct gpg context.\n");\r
 240 > > -           else\r
 241 > > -               g_mime_gpg_context_set_always_trust((GMimeGpgContext*)params.cryptoctx, FALSE);\r
 242 > > -#ifndef GMIME_ATLEAST_26\r
 243 > > -           g_object_unref (session);\r
 244 > > -           session = NULL;\r
 245 > > +   GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);\r
 246 > > +   params.cryptoctx = g_mime_gpg_context_new (session, "gpg");\r
 247 > >  #endif\r
 248 > > -       }\r
 249 > > -       if (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)\r
 250 > > -           params.decrypt = 1;\r
 251 > > +   if (params.cryptoctx) {\r
 252 > > +       g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);\r
 253 > > +       params.decrypt = decrypt;\r
 254 > \r
 255 > Would it be easier to pass &params.decrypt in the notmuch_opt_desc_t\r
 256 > and set it to FALSE in the 'else' branch of this?\r
 257 \r
 258 Agreed. That can be done if decrypt is changed to notmuch_bool_t.\r
 259 \r
 260 > \r
 261 > >     } else {\r
 262 > > -       fprintf (stderr, "Unrecognized option: %s\n", argv[i]);\r
 263 > > -       return 1;\r
 264 > > +       fprintf (stderr, "Failed to construct gpg context.\n");\r
 265 > >     }\r
 266 > > +#ifndef GMIME_ATLEAST_26\r
 267 > > +   g_object_unref (session);\r
 268 > > +#endif\r
 269 > >      }\r
 270 > >  \r
 271 > > -    argc -= i;\r
 272 > > -    argv += i;\r
 273 > > -\r
 274 > >      config = notmuch_config_open (ctx, NULL, NULL);\r
 275 > >      if (config == NULL)\r
 276 > >     return 1;\r
 277 > >  \r
 278 > > -    query_string = query_string_from_args (ctx, argc, argv);\r
 279 > > +    query_string = query_string_from_args (ctx, argc-opt_index, argv+opt_index);\r
 280 > >      if (query_string == NULL) {\r
 281 > >     fprintf (stderr, "Out of memory\n");\r
 282 > >     return 1;\r
 283 > >      }\r
 284 > >  \r
 285 > > -    if (mbox && params.part > 0) {\r
 286 > > -   fprintf (stderr, "Error: specifying parts is incompatible with mbox output format.\n");\r
 287 > > -   return 1;\r
 288 > > -    }\r
 289 > > -\r
 290 > >      if (*query_string == '\0') {\r
 291 > >     fprintf (stderr, "Error: notmuch show requires at least one search term.\n");\r
 292 > >     return 1;\r
 293 > > @@ -1158,15 +1172,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))\r
 294 > >     return 1;\r
 295 > >      }\r
 296 > >  \r
 297 > > -    /* if part was requested and format was not specified, use format=raw */\r
 298 > > -    if (params.part >= 0 && !format_specified)\r
 299 > > -   format = &format_raw;\r
 300 > > -\r
 301 > > -    /* If --format=raw specified without specifying part, we can only\r
 302 > > -     * output single message, so set part=0 */\r
 303 > > -    if (params.raw && params.part < 0)\r
 304 > > -   params.part = 0;\r
 305 > > -\r
 306 > >      if (params.part >= 0)\r
 307 > >     return do_show_single (ctx, query, format, &params);\r
 308 > >      else\r