Re: [PATCH 08/11] emacs: Support caching in notmuch-get-bodypart-{binary, text}

diff --git a/c1/e60daf79134afdbce66971976cd3731c953634 b/c1/e60daf79134afdbce66971976cd3731c953634
new file mode 100644 (file)
index 0000000..4a05893
--- /dev/null
+++ b/c1/e60daf79134afdbce66971976cd3731c953634
@@ -0,0 +1,195 @@
+Return-Path: <amdragon@mit.edu>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id 5A437431FAF\r
+       for <notmuch@notmuchmail.org>; Thu, 24 Apr 2014 11:12:30 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.7\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
+       tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+       by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+       with ESMTP id JmVCETnxE4pR for <notmuch@notmuchmail.org>;\r
+       Thu, 24 Apr 2014 11:12:24 -0700 (PDT)\r
+Received: from dmz-mailsec-scanner-5.mit.edu (dmz-mailsec-scanner-5.mit.edu\r
+       [18.7.68.34])\r
+       by olra.theworths.org (Postfix) with ESMTP id 94479431FAE\r
+       for <notmuch@notmuchmail.org>; Thu, 24 Apr 2014 11:12:24 -0700 (PDT)\r
+X-AuditID: 12074422-f79186d00000135a-cd-53595407e39e\r
+Received: from mailhub-auth-1.mit.edu ( [18.9.21.35])\r
+       (using TLS with cipher AES256-SHA (256/256 bits))\r
+       (Client did not present a certificate)\r
+       by dmz-mailsec-scanner-5.mit.edu (Symantec Messaging Gateway) with SMTP\r
+       id 4B.79.04954.70459535; Thu, 24 Apr 2014 14:12:23 -0400 (EDT)\r
+Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11])\r
+       by mailhub-auth-1.mit.edu (8.13.8/8.9.2) with ESMTP id s3OICM0Z023927; \r
+       Thu, 24 Apr 2014 14:12:23 -0400\r
+Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
+       (authenticated bits=0)\r
+       (User authenticated as amdragon@ATHENA.MIT.EDU)\r
+       by outgoing.mit.edu (8.13.8/8.12.4) with ESMTP id s3OICKge007507\r
+       (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT);\r
+       Thu, 24 Apr 2014 14:12:21 -0400\r
+Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80)\r
+       (envelope-from <amdragon@MIT.EDU>)\r
+       id 1WdO8B-0005Qt-Kd; Thu, 24 Apr 2014 14:12:19 -0400\r
+Date: Thu, 24 Apr 2014 14:12:17 -0400\r
+From: Austin Clements <amdragon@MIT.EDU>\r
+To: Mark Walters <markwalters1009@gmail.com>\r
+Subject: Re: [PATCH 08/11] emacs: Support caching in\r
+       notmuch-get-bodypart-{binary, text}\r
+Message-ID: <20140424181216.GO25817@mit.edu>\r
+References: <1398105468-14317-1-git-send-email-amdragon@mit.edu>\r
+       <1398105468-14317-9-git-send-email-amdragon@mit.edu>\r
+       <87sip3t37f.fsf@qmul.ac.uk>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+Content-Disposition: inline\r
+In-Reply-To: <87sip3t37f.fsf@qmul.ac.uk>\r
+User-Agent: Mutt/1.5.21 (2010-09-15)\r
+X-Brightmail-Tracker:\r
+ H4sIAAAAAAAAA+NgFmpkleLIzCtJLcpLzFFi42IR4hRV1mUPiQw2eL3JymL1XB6L6zdnMjsw\r
+       eeycdZfd49mqW8wBTFFcNimpOZllqUX6dglcGbMn72UpuKxVsWcbbwPjQsUuRg4OCQETiXX7\r
+       8rsYOYFMMYkL99azdTFycQgJzGaS6Nx7mh3C2cgo8eVZLwuEc5pJYkHLFyYIZwmjxM69s1lB\r
+       RrEIqEqsvWQNMopNQENi2/7ljCC2iICOxO1DC9hBbGYBaYlvv5uZQGxhgSiJrp3P2UBsXqCa\r
+       bWfnQ22byijxdeYMJoiEoMTJmU9YIJq1JG78e8kEsgtk0PJ/HCBhTqBdn9p3s4LYogIqElNO\r
+       bmObwCg0C0n3LCTdsxC6FzAyr2KUTcmt0s1NzMwpTk3WLU5OzMtLLdI11cvNLNFLTSndxAgK\r
+       aXYXpR2MPw8qHWIU4GBU4uF9IR8ZLMSaWFZcmXuIUZKDSUmUN84PKMSXlJ9SmZFYnBFfVJqT\r
+       WnyIUYKDWUmEd50tUI43JbGyKrUoHyYlzcGiJM771toqWEggPbEkNTs1tSC1CCYrw8GhJMFr\r
+       FwzUKFiUmp5akZaZU4KQZuLgBBnOAzQ8JAhkeHFBYm5xZjpE/hSjopQ4bwZIswBIIqM0D64X\r
+       lnJeMYoDvSLMWw5SxQNMV3Ddr4AGMwENLpgQDjK4JBEhJdXAqL//h+LHbdxJ0Y9WvDJ86eOT\r
+       ZLj4gn8SY5Vq6QxhdVmvFqvggPv8ut/5uPeZfzb8I+XErci71D9/1pxzi5gC75dJhJ/JkpsY\r
+       KhBQ9qH33faNwTsk+SKeCxsf3XRrhwDD0SvVR/9zxC3bHWBbqWzGe3fD9OJLaoZZthNnf8tv\r
+       NjHh3776mpWpEktxRqKhFnNRcSIAo6iODBQDAAA=\r
+Cc: notmuch@notmuchmail.org\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+       <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Thu, 24 Apr 2014 18:12:30 -0000\r
+\r
+Quoth Mark Walters on Apr 24 at 11:46 am:\r
+> \r
+> On Mon, 21 Apr 2014, Austin Clements <amdragon@MIT.EDU> wrote:\r
+> > (The actual code change here is small, but requires re-indenting\r
+> > existing code.)\r
+> > ---\r
+> >  emacs/notmuch-lib.el | 52 ++++++++++++++++++++++++++++++----------------------\r
+> >  1 file changed, 30 insertions(+), 22 deletions(-)\r
+> >\r
+> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el\r
+> > index fc67b14..fee8512 100644\r
+> > --- a/emacs/notmuch-lib.el\r
+> > +++ b/emacs/notmuch-lib.el\r
+> > @@ -503,33 +503,39 @@ (defun notmuch-parts-filter-by-type (parts type)\r
+> >     (lambda (part) (notmuch-match-content-type (plist-get part :content-type) type))\r
+> >     parts))\r
+> >  \r
+> > -(defun notmuch-get-bodypart-binary (msg part process-crypto)\r
+> > +(defun notmuch-get-bodypart-binary (msg part process-crypto &optional cache)\r
+> >    "Return the unprocessed content of PART in MSG as a unibyte string.\r
+> >  \r
+> >  This returns the \"raw\" content of the given part after content\r
+> >  transfer decoding, but with no further processing (see the\r
+> >  discussion of --format=raw in man notmuch-show).  In particular,\r
+> >  this does no charset conversion."\r
+> > -  (let ((args `("show" "--format=raw"\r
+> > -          ,(format "--part=%d" (plist-get part :id))\r
+> > -          ,@(when process-crypto '("--decrypt"))\r
+> > -          ,(notmuch-id-to-query (plist-get msg :id)))))\r
+> > -    (with-temp-buffer\r
+> > -      ;; Emacs internally uses a UTF-8-like multibyte string\r
+> > -      ;; representation by default (regardless of the coding system,\r
+> > -      ;; which only affects how it goes from outside data to this\r
+> > -      ;; internal representation).  This *almost* never matters.\r
+> > -      ;; Annoyingly, it does matter if we use this data in an image\r
+> > -      ;; descriptor, since Emacs will use its internal data buffer\r
+> > -      ;; directly and this multibyte representation corrupts binary\r
+> > -      ;; image formats.  Since the caller is asking for binary data, a\r
+> > -      ;; unibyte string is a more appropriate representation anyway.\r
+> > -      (set-buffer-multibyte nil)\r
+> > -      (let ((coding-system-for-read 'no-conversion))\r
+> > -  (apply #'call-process notmuch-command nil '(t nil) nil args)\r
+> > -  (buffer-string)))))\r
+> > -\r
+> > -(defun notmuch-get-bodypart-text (msg part process-crypto)\r
+> > +  (let ((data (plist-get part :binary-content)))\r
+> > +    (when (not data)\r
+> > +      (let ((args `("show" "--format=raw"\r
+> > +              ,(format "--part=%d" (plist-get part :id))\r
+> > +              ,@(when process-crypto '("--decrypt"))\r
+> > +              ,(notmuch-id-to-query (plist-get msg :id)))))\r
+> > +  (with-temp-buffer\r
+> > +    ;; Emacs internally uses a UTF-8-like multibyte string\r
+> > +    ;; representation by default (regardless of the coding\r
+> > +    ;; system, which only affects how it goes from outside data\r
+> > +    ;; to this internal representation).  This *almost* never\r
+> > +    ;; matters.  Annoyingly, it does matter if we use this data\r
+> > +    ;; in an image descriptor, since Emacs will use its internal\r
+> > +    ;; data buffer directly and this multibyte representation\r
+> > +    ;; corrupts binary image formats.  Since the caller is\r
+> > +    ;; asking for binary data, a unibyte string is a more\r
+> > +    ;; appropriate representation anyway.\r
+> > +    (set-buffer-multibyte nil)\r
+> > +    (let ((coding-system-for-read 'no-conversion))\r
+> > +      (apply #'call-process notmuch-command nil '(t nil) nil args)\r
+> > +      (setq data (buffer-string)))))\r
+> > +      (when cache\r
+> > +  (plist-put part :binary-content data)))\r
+> > +    data))\r
+> \r
+> I am a little puzzled by this but that could be lack of familiarity with\r
+> elisp. As far as I can see plist-put will sometimes modify the original\r
+> plist and sometimes return a new plist. If the latter happens then I\r
+> think it works out as if we hadn't cached anything as the part passed to\r
+> the function is unmodified. That might not matter in this case (though I\r
+> find the lack of determinism disturbing).\r
+> \r
+> Or is something else going on?\r
+\r
+No, your familiarity with Elisp serves you well.  I'm completely\r
+cheating here.  According to the specification of plist-put, it's\r
+allowed to return a new list but in reality this only happens when the\r
+original plist is nil.  We lean on this already all over the\r
+notmuch-emacs code, but maybe that doesn't excuse me adding one more\r
+cheat.\r
+\r
+I could add a comment here explaining what's going on, I could\r
+manually do the list insertion in a way that's guaranteed to mutate it\r
+in place, or I could add a nil :binary-content property when parts are\r
+created (since plist-put is guaranteed to mutate existing keys in\r
+place).\r
+\r
+> Best wishes\r
+> \r
+> Mark\r
+> \r
+> \r
+> \r
+> > +\r
+> > +(defun notmuch-get-bodypart-text (msg part process-crypto &optional cache)\r
+> >    "Return the text content of PART in MSG.\r
+> >  \r
+> >  This returns the content of the given part as a multibyte Lisp\r
+> > @@ -546,7 +552,9 @@ (defun notmuch-get-bodypart-text (msg part process-crypto)\r
+> >         (npart (apply #'notmuch-call-notmuch-sexp args)))\r
+> >    (setq content (plist-get npart :content))\r
+> >    (when (not content)\r
+> > -    (error "Internal error: No :content from %S" args))))\r
+> > +    (error "Internal error: No :content from %S" args)))\r
+> > +      (when cache\r
+> > +  (plist-put part :content content)))\r
+> >      content))\r
+> >  \r
+> >  ;; Workaround: The call to `mm-display-part' below triggers a bug in\r