df/70ecf1bbf633b050f12c46cb966b1546762ac9

   1 Return-Path: <amdragon@mit.edu>\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 EC2B2431FB6\r
   6         for <notmuch@notmuchmail.org>; Fri, 18 May 2012 12:22:01 -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: -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 lUsFhapNj3dv for <notmuch@notmuchmail.org>;\r
  16         Fri, 18 May 2012 12:22:01 -0700 (PDT)\r
  17 Received: from dmz-mailsec-scanner-6.mit.edu (DMZ-MAILSEC-SCANNER-6.MIT.EDU\r
  18         [18.7.68.35])\r
  19         by olra.theworths.org (Postfix) with ESMTP id EA93B431FAE\r
  20         for <notmuch@notmuchmail.org>; Fri, 18 May 2012 12:22:00 -0700 (PDT)\r
  21 X-AuditID: 12074423-b7fcc6d0000008a8-3d-4fb6a158d41b\r
  22 Received: from mailhub-auth-2.mit.edu ( [18.7.62.36])\r
  23         by dmz-mailsec-scanner-6.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 93.48.02216.851A6BF4; Fri, 18 May 2012 15:22:00 -0400 (EDT)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id q4IJLxdg028457; \r
  27         Fri, 18 May 2012 15:22:00 -0400\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q4IJLwB3022516\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Fri, 18 May 2012 15:21:59 -0400 (EDT)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1SVSkQ-00073K-1p; Fri, 18 May 2012 15:21:58 -0400\r
  37 Date: Fri, 18 May 2012 15:21:57 -0400\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Jameson Graef Rollins <jrollins@finestructure.net>\r
  40 Subject: Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only\r
  41         when needed\r
  42 Message-ID: <20120518192157.GV11804@mit.edu>\r
  43 References: <1337362357-31281-1-git-send-email-jrollins@finestructure.net>\r
  44         <1337362357-31281-2-git-send-email-jrollins@finestructure.net>\r
  45         <1337362357-31281-3-git-send-email-jrollins@finestructure.net>\r
  46         <1337362357-31281-4-git-send-email-jrollins@finestructure.net>\r
  47         <1337362357-31281-5-git-send-email-jrollins@finestructure.net>\r
  48         <1337362357-31281-6-git-send-email-jrollins@finestructure.net>\r
  49 MIME-Version: 1.0\r
  50 Content-Type: text/plain; charset=us-ascii\r
  51 Content-Disposition: inline\r
  52 In-Reply-To: <1337362357-31281-6-git-send-email-jrollins@finestructure.net>\r
  53 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  54 X-Brightmail-Tracker:\r
  55  H4sIAAAAAAAAA+NgFmpileLIzCtJLcpLzFFi42IRYrdT0Y1YuM3f4O0DSYs9+7wsrt+cyezA\r
  56         5HH3NJfHs1W3mAOYorhsUlJzMstSi/TtErgyNqz5zVzwWq/i16zTbA2My1S7GDk5JARMJC5f\r
  57         /scMYYtJXLi3nq2LkYtDSGAfo8S+yT0sEM4GRon9KzqgnJNMErPO/meGcJYwSnyc/QOsn0VA\r
  58         VeLu1nVgNpuAhsS2/csZQWwRATOJni9/wGxmAS2JrRs/gNnCAhESbxsusYDYvAI6Elu73oDF\r
  59         hQT6mSVW/tKHiAtKnJz5hAWm98a/l0xdjBxAtrTE8n8cIGFOAW+J2/MegJWICqhITDm5jW0C\r
  60         o9AsJN2zkHTPQuhewMi8ilE2JbdKNzcxM6c4NVm3ODkxLy+1SNdMLzezRC81pXQTIzioXZR3\r
  61         MP45qHSIUYCDUYmH9+Kkbf5CrIllxZW5hxglOZiURHl95wGF+JLyUyozEosz4otKc1KLDzFK\r
  62         cDArifDOnA6U401JrKxKLcqHSUlzsCiJ82povfMTEkhPLEnNTk0tSC2CycpwcChJ8K5bANQo\r
  63         WJSanlqRlplTgpBm4uAEGc4DNLwbpIa3uCAxtzgzHSJ/ilFRSpx3GUhCACSRUZoH1wtLOq8Y\r
  64         xYFeEeadDVLFA0xYcN2vgAYzAQ2uZAMbXJKIkJJqYDT++vpUc4mCWRTbvIMLtL15n4fkP3Q1\r
  65         tdsscMLdal+LbvferGYebu6djw+fk+n6tcXs6Sax5/d8JB8de7LmJPOrbay/vHX3h1Zxm7ja\r
  66         JjuVBe8JXuvZL9Nme+4qQ0LS5LNfF5qGv7TXCAh++WFSQ21w3d/NhvsYmc8dFnLwNTuerCzf\r
  67         4WO6X4mlOCPRUIu5qDgRAMs+ntsVAwAA\r
  68 Cc: Notmuch Mail <notmuch@notmuchmail.org>\r
  69 X-BeenThere: notmuch@notmuchmail.org\r
  70 X-Mailman-Version: 2.1.13\r
  71 Precedence: list\r
  72 List-Id: "Use and development of the notmuch mail system."\r
  73         <notmuch.notmuchmail.org>\r
  74 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  75         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  76 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  77 List-Post: <mailto:notmuch@notmuchmail.org>\r
  78 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  79 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  80         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  81 X-List-Received-Date: Fri, 18 May 2012 19:22:02 -0000\r
  82 \r
  83 Quoth Jameson Graef Rollins on May 18 at 10:32 am:\r
  84 > Move the creation of the crypto ctx into mime-node.c and create it\r
  85 > only when needed.  This removes code duplication from notmuch-show and\r
  86 > notmuch-reply, and should speed up these functions considerably if the\r
  87 > crypto flags are provided but the messages don't have any\r
  88 > cryptographic parts.\r
  89 > ---\r
  90 >  mime-node.c      |   25 +++++++++++++++++++++++++\r
  91 >  notmuch-client.h |    3 ++-\r
  92 >  notmuch-reply.c  |   19 -------------------\r
  93 >  notmuch-show.c   |   23 -----------------------\r
  94 >  4 files changed, 27 insertions(+), 43 deletions(-)\r
  95 > \r
  96 > diff --git a/mime-node.c b/mime-node.c\r
  97 > index 3adbe5a..592e0b6 100644\r
  98 > --- a/mime-node.c\r
  99 > +++ b/mime-node.c\r
 100 > @@ -182,6 +182,31 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)\r
 101 >       return NULL;\r
 102 >      }\r
 103 >  \r
 104 > +    /* Lazily create the gpgctx if it's needed and hasn't been initialized yet */\r
 105 > +    if ((GMIME_IS_MULTIPART_ENCRYPTED (part) || GMIME_IS_MULTIPART_SIGNED (part))\r
 106 > +     && (node->ctx->crypto->verify || node->ctx->crypto->decrypt)) {\r
 107 > +     if (!node->ctx->crypto->gpgctx) {\r
 108 \r
 109 These if conditions could be combined, like\r
 110 \r
 111     if ((GMIME_IS_MULTIPART_ENCRYPTED (part) || GMIME_IS_MULTIPART_SIGNED (part))\r
 112         && (node->ctx->crypto->verify || node->ctx->crypto->decrypt)\r
 113         && !node->ctx->crypto->gpgctx) {\r
 114 \r
 115 When I see two nested 'if's like this, I expect there to be an else\r
 116 part to the inner if or something after the inner if (why else would\r
 117 it be separate?) and then I wind up matching braces when I don't see\r
 118 anything.  Also, one 'if' would save a level of indentation.\r
 119 \r
 120 > +#ifdef GMIME_ATLEAST_26\r
 121 > +         /* TODO: GMimePasswordRequestFunc */\r
 122 > +         node->ctx->crypto->gpgctx = g_mime_gpg_context_new (NULL, "gpg");\r
 123 > +#else\r
 124 > +         GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);\r
 125 > +         node->ctx->crypto->gpgctx = g_mime_gpg_context_new (session, "gpg");\r
 126 > +         g_object_unref (session);\r
 127 > +#endif\r
 128 > +         if (node->ctx->crypto->gpgctx) {\r
 129 > +             g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) node->ctx->crypto->gpgctx, FALSE);\r
 130 > +         } else {\r
 131 > +             /* If we fail to create the gpgctx set the verify and\r
 132 > +              * decrypt flags to FALSE so we don't try to do any\r
 133 > +              * further verification or decryption */\r
 134 > +             node->ctx->crypto->verify = FALSE;\r
 135 > +             node->ctx->crypto->decrypt = FALSE;\r
 136 > +             fprintf (stderr, "Failed to construct gpg context.\n");\r
 137 > +         }\r
 138 > +     }\r
 139 > +    }\r
 140 > +\r
 141 >      /* Handle PGP/MIME parts */\r
 142 >      if (GMIME_IS_MULTIPART_ENCRYPTED (part) && node->ctx->crypto->decrypt) {\r
 143 >       if (node->nchildren != 2) {\r
 144 > diff --git a/notmuch-client.h b/notmuch-client.h\r
 145 > index c671c13..c79eaa9 100644\r
 146 > --- a/notmuch-client.h\r
 147 > +++ b/notmuch-client.h\r
 148 > @@ -348,7 +348,8 @@ struct mime_node {\r
 149 >  /* Construct a new MIME node pointing to the root message part of\r
 150 >   * message. If crypto->verify is true, signed child parts will be\r
 151 >   * verified. If crypto->decrypt is true, encrypted child parts will be\r
 152 > - * decrypted.\r
 153 > + * decrypted.  The GPG context crypto->gpgctx does not need to be\r
 154 > + * pre-initialized as it will be initialized lazily as needed.\r
 155 \r
 156 Perhaps "If crypto->gpgctx is NULL, it will be lazily initialized."?\r
 157 The variable does have to be "initialized", in the sense that it can't\r
 158 be uninitialized data.\r
 159 \r
 160 It's slightly awkward that it's the caller's responsibility to free\r
 161 this lazily constructed object.  That should probably be documented.\r
 162 We could more carefully reference count it, but I think that would\r
 163 actually be worse because the reference count would probably bounce\r
 164 through zero frequently.\r
 165 \r
 166 >   *\r
 167 >   * Return value:\r
 168 >   *\r
 169 > diff --git a/notmuch-reply.c b/notmuch-reply.c\r
 170 > index 345be76..1a61aa7 100644\r
 171 > --- a/notmuch-reply.c\r
 172 > +++ b/notmuch-reply.c\r
 173 > @@ -707,25 +707,6 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])\r
 174 >      else\r
 175 >       reply_format_func = notmuch_reply_format_default;\r
 176 >  \r
 177 > -    if (crypto.decrypt) {\r
 178 > -#ifdef GMIME_ATLEAST_26\r
 179 > -     /* TODO: GMimePasswordRequestFunc */\r
 180 > -     crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");\r
 181 > -#else\r
 182 > -     GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);\r
 183 > -     crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");\r
 184 > -#endif\r
 185 > -     if (crypto.gpgctx) {\r
 186 > -         g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto.gpgctx, FALSE);\r
 187 > -     } else {\r
 188 > -         crypto.decrypt = FALSE;\r
 189 > -         fprintf (stderr, "Failed to construct gpg context.\n");\r
 190 > -     }\r
 191 > -#ifndef GMIME_ATLEAST_26\r
 192 > -     g_object_unref (session);\r
 193 > -#endif\r
 194 > -    }\r
 195 > -\r
 196 >      config = notmuch_config_open (ctx, NULL, NULL);\r
 197 >      if (config == NULL)\r
 198 >       return 1;\r
 199 > diff --git a/notmuch-show.c b/notmuch-show.c\r
 200 > index f4ee038..5f785d0 100644\r
 201 > --- a/notmuch-show.c\r
 202 > +++ b/notmuch-show.c\r
 203 > @@ -1056,29 +1056,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))\r
 204 >       break;\r
 205 >      }\r
 206 >  \r
 207 > -    if (params.crypto.decrypt || params.crypto.verify) {\r
 208 > -#ifdef GMIME_ATLEAST_26\r
 209 > -     /* TODO: GMimePasswordRequestFunc */\r
 210 > -     params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");\r
 211 > -#else\r
 212 > -     GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);\r
 213 > -     params.crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");\r
 214 > -#endif\r
 215 > -     if (params.crypto.gpgctx) {\r
 216 > -         g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE);\r
 217 > -     } else {\r
 218 > -         /* If we fail to create the gpgctx set the verify and\r
 219 > -          * decrypt flags to FALSE so we don't try to do any\r
 220 > -          * further verification or decryption */\r
 221 > -         params.crypto.verify = FALSE;\r
 222 > -         params.crypto.decrypt = FALSE;\r
 223 > -         fprintf (stderr, "Failed to construct gpg context.\n");\r
 224 > -     }\r
 225 > -#ifndef GMIME_ATLEAST_26\r
 226 > -     g_object_unref (session);\r
 227 > -#endif\r
 228 > -    }\r
 229 > -\r
 230 >      config = notmuch_config_open (ctx, NULL, NULL);\r
 231 >      if (config == NULL)\r
 232 >       return 1;\r