cb/cd2a9d109c03f970c4227d9068c51ca891dc36

   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 6789A431FBD\r
   6         for <notmuch@notmuchmail.org>; Thu, 24 Apr 2014 23:42:10 -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.502\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=0.502 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_LOW=-0.7] 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 3rljN1cILgEi for <notmuch@notmuchmail.org>;\r
  17         Thu, 24 Apr 2014 23:42:04 -0700 (PDT)\r
  18 Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\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 093AB431FAE\r
  22         for <notmuch@notmuchmail.org>; Thu, 24 Apr 2014 23:42:04 -0700 (PDT)\r
  23 Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
  24         by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
  25         (envelope-from <m.walters@qmul.ac.uk>)\r
  26         id 1WdZph-0003ze-Il; Fri, 25 Apr 2014 07:42:01 +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 1WdZph-0007Wc-1X; Fri, 25 Apr 2014 07:42:01 +0100\r
  31 From: Mark Walters <markwalters1009@gmail.com>\r
  32 To: Austin Clements <amdragon@MIT.EDU>\r
  33 Subject: Re: [PATCH 08/11] emacs: Support caching in\r
  34         notmuch-get-bodypart-{binary, text}\r
  35 In-Reply-To: <20140424181216.GO25817@mit.edu>\r
  36 References: <1398105468-14317-1-git-send-email-amdragon@mit.edu>\r
  37         <1398105468-14317-9-git-send-email-amdragon@mit.edu>\r
  38         <87sip3t37f.fsf@qmul.ac.uk> <20140424181216.GO25817@mit.edu>\r
  39 User-Agent: Notmuch/0.15.2+615~g78e3a93 (http://notmuchmail.org) Emacs/23.4.1\r
  40         (x86_64-pc-linux-gnu)\r
  41 Date: Fri, 25 Apr 2014 07:42:00 +0100\r
  42 Message-ID: <87d2g5uczr.fsf@qmul.ac.uk>\r
  43 MIME-Version: 1.0\r
  44 Content-Type: text/plain; charset=us-ascii\r
  45 X-Sender-Host-Address: 87.81.223.162\r
  46 X-QM-Geographic: According to ripencc,\r
  47         this message was delivered by a machine in Britain (UK) (GB).\r
  48 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  49 X-QM-Body-MD5: 14239cbd11f4e8c3a5792e1fe7b93f1a (of first 20000 bytes)\r
  50 X-SpamAssassin-Score: -0.1\r
  51 X-SpamAssassin-SpamBar: /\r
  52 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  53         determine if it is\r
  54         spam. We require at least 5.0 points to mark a message as spam.\r
  55         This message scored -0.1 points.\r
  56         Summary of the scoring: \r
  57         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  58         provider *      (markwalters1009[at]gmail.com)\r
  59         * -0.1 AWL AWL: From: address is in the auto white-list\r
  60 X-QM-Scan-Virus: ClamAV says the message is clean\r
  61 Cc: notmuch@notmuchmail.org\r
  62 X-BeenThere: notmuch@notmuchmail.org\r
  63 X-Mailman-Version: 2.1.13\r
  64 Precedence: list\r
  65 List-Id: "Use and development of the notmuch mail system."\r
  66         <notmuch.notmuchmail.org>\r
  67 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  68         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  69 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  70 List-Post: <mailto:notmuch@notmuchmail.org>\r
  71 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  72 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  73         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  74 X-List-Received-Date: Fri, 25 Apr 2014 06:42:10 -0000\r
  75 \r
  76 \r
  77 On Thu, 24 Apr 2014, Austin Clements <amdragon@MIT.EDU> wrote:\r
  78 > Quoth Mark Walters on Apr 24 at 11:46 am:\r
  79 >> \r
  80 >> On Mon, 21 Apr 2014, Austin Clements <amdragon@MIT.EDU> wrote:\r
  81 >> > (The actual code change here is small, but requires re-indenting\r
  82 >> > existing code.)\r
  83 >> > ---\r
  84 >> >  emacs/notmuch-lib.el | 52 ++++++++++++++++++++++++++++++----------------------\r
  85 >> >  1 file changed, 30 insertions(+), 22 deletions(-)\r
  86 >> >\r
  87 >> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el\r
  88 >> > index fc67b14..fee8512 100644\r
  89 >> > --- a/emacs/notmuch-lib.el\r
  90 >> > +++ b/emacs/notmuch-lib.el\r
  91 >> > @@ -503,33 +503,39 @@ (defun notmuch-parts-filter-by-type (parts type)\r
  92 >> >     (lambda (part) (notmuch-match-content-type (plist-get part :content-type) type))\r
  93 >> >     parts))\r
  94 >> >  \r
  95 >> > -(defun notmuch-get-bodypart-binary (msg part process-crypto)\r
  96 >> > +(defun notmuch-get-bodypart-binary (msg part process-crypto &optional cache)\r
  97 >> >    "Return the unprocessed content of PART in MSG as a unibyte string.\r
  98 >> >  \r
  99 >> >  This returns the \"raw\" content of the given part after content\r
 100 >> >  transfer decoding, but with no further processing (see the\r
 101 >> >  discussion of --format=raw in man notmuch-show).  In particular,\r
 102 >> >  this does no charset conversion."\r
 103 >> > -  (let ((args `("show" "--format=raw"\r
 104 >> > -          ,(format "--part=%d" (plist-get part :id))\r
 105 >> > -          ,@(when process-crypto '("--decrypt"))\r
 106 >> > -          ,(notmuch-id-to-query (plist-get msg :id)))))\r
 107 >> > -    (with-temp-buffer\r
 108 >> > -      ;; Emacs internally uses a UTF-8-like multibyte string\r
 109 >> > -      ;; representation by default (regardless of the coding system,\r
 110 >> > -      ;; which only affects how it goes from outside data to this\r
 111 >> > -      ;; internal representation).  This *almost* never matters.\r
 112 >> > -      ;; Annoyingly, it does matter if we use this data in an image\r
 113 >> > -      ;; descriptor, since Emacs will use its internal data buffer\r
 114 >> > -      ;; directly and this multibyte representation corrupts binary\r
 115 >> > -      ;; image formats.  Since the caller is asking for binary data, a\r
 116 >> > -      ;; unibyte string is a more appropriate representation anyway.\r
 117 >> > -      (set-buffer-multibyte nil)\r
 118 >> > -      (let ((coding-system-for-read 'no-conversion))\r
 119 >> > -  (apply #'call-process notmuch-command nil '(t nil) nil args)\r
 120 >> > -  (buffer-string)))))\r
 121 >> > -\r
 122 >> > -(defun notmuch-get-bodypart-text (msg part process-crypto)\r
 123 >> > +  (let ((data (plist-get part :binary-content)))\r
 124 >> > +    (when (not data)\r
 125 >> > +      (let ((args `("show" "--format=raw"\r
 126 >> > +              ,(format "--part=%d" (plist-get part :id))\r
 127 >> > +              ,@(when process-crypto '("--decrypt"))\r
 128 >> > +              ,(notmuch-id-to-query (plist-get msg :id)))))\r
 129 >> > +  (with-temp-buffer\r
 130 >> > +    ;; Emacs internally uses a UTF-8-like multibyte string\r
 131 >> > +    ;; representation by default (regardless of the coding\r
 132 >> > +    ;; system, which only affects how it goes from outside data\r
 133 >> > +    ;; to this internal representation).  This *almost* never\r
 134 >> > +    ;; matters.  Annoyingly, it does matter if we use this data\r
 135 >> > +    ;; in an image descriptor, since Emacs will use its internal\r
 136 >> > +    ;; data buffer directly and this multibyte representation\r
 137 >> > +    ;; corrupts binary image formats.  Since the caller is\r
 138 >> > +    ;; asking for binary data, a unibyte string is a more\r
 139 >> > +    ;; appropriate representation anyway.\r
 140 >> > +    (set-buffer-multibyte nil)\r
 141 >> > +    (let ((coding-system-for-read 'no-conversion))\r
 142 >> > +      (apply #'call-process notmuch-command nil '(t nil) nil args)\r
 143 >> > +      (setq data (buffer-string)))))\r
 144 >> > +      (when cache\r
 145 >> > +  (plist-put part :binary-content data)))\r
 146 >> > +    data))\r
 147 >> \r
 148 >> I am a little puzzled by this but that could be lack of familiarity with\r
 149 >> elisp. As far as I can see plist-put will sometimes modify the original\r
 150 >> plist and sometimes return a new plist. If the latter happens then I\r
 151 >> think it works out as if we hadn't cached anything as the part passed to\r
 152 >> the function is unmodified. That might not matter in this case (though I\r
 153 >> find the lack of determinism disturbing).\r
 154 >> \r
 155 >> Or is something else going on?\r
 156 >\r
 157 > No, your familiarity with Elisp serves you well.  I'm completely\r
 158 > cheating here.  According to the specification of plist-put, it's\r
 159 > allowed to return a new list but in reality this only happens when the\r
 160 > original plist is nil.  We lean on this already all over the\r
 161 > notmuch-emacs code, but maybe that doesn't excuse me adding one more\r
 162 > cheat.\r
 163 >\r
 164 > I could add a comment here explaining what's going on, I could\r
 165 > manually do the list insertion in a way that's guaranteed to mutate it\r
 166 > in place, or I could add a nil :binary-content property when parts are\r
 167 > created (since plist-put is guaranteed to mutate existing keys in\r
 168 > place).\r
 169 \r
 170 I think a comment is fine. \r
 171 \r
 172 (Incidentally what is the best way of telling if emacs has changed an\r
 173 object or returned a new one for other commands? Something like (setq\r
 174 oldobject object) (setq object (operation-on object)) (if (eq object\r
 175 oldobject) ... ))\r
 176 \r
 177 Also, I think the function should have a comment about the lifetime of\r
 178 the caching. I think in some cases the addition of :binary-content could\r
 179 occur on load and thus the plist with binary content added would get\r
 180 saved in the buffer when the msg plist was saved as a\r
 181 text-property. However, maybe in other cases this gets called after the\r
 182 initial insertion and thus the cached value is just used during this\r
 183 operation on msg? Sorry that is a little incoherent as I haven't checked\r
 184 all callers.\r
 185 \r
 186 Best wishes\r
 187 \r
 188 Mark\r
 189 \r
 190 \r
 191 \r
 192 >> Best wishes\r
 193 >> \r
 194 >> Mark\r
 195 >> \r
 196 >> \r
 197 >> \r
 198 >> > +\r
 199 >> > +(defun notmuch-get-bodypart-text (msg part process-crypto &optional cache)\r
 200 >> >    "Return the text content of PART in MSG.\r
 201 >> >  \r
 202 >> >  This returns the content of the given part as a multibyte Lisp\r
 203 >> > @@ -546,7 +552,9 @@ (defun notmuch-get-bodypart-text (msg part process-crypto)\r
 204 >> >         (npart (apply #'notmuch-call-notmuch-sexp args)))\r
 205 >> >    (setq content (plist-get npart :content))\r
 206 >> >    (when (not content)\r
 207 >> > -    (error "Internal error: No :content from %S" args))))\r
 208 >> > +    (error "Internal error: No :content from %S" args)))\r
 209 >> > +      (when cache\r
 210 >> > +  (plist-put part :content content)))\r
 211 >> >      content))\r
 212 >> >  \r
 213 >> >  ;; Workaround: The call to `mm-display-part' below triggers a bug in\r