bug in attachment handling

bug in attachment handling

[Mark Walters]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]


Hi

I have been seeing a bug in attachment handling in recent notmuch. If a
pdf part is sent as application/octet-stream then notmuch says
[[file.pdf: application/octet-stream (as application/pdf) ] but when
viewing it with . v or the default button action (when customised to
view) it treats it as application/octet-stream.

I think this is a regression but haven't checked yet: it probably has a
simple fix so we may want to fix it before 0.16 (users may be grumpy
about the key binding changes etc as it is!)

I attach a pdf with this message (just a mozilla print-to-file of the
notmuch webpage) which should demonstrate the problem.

Best wishes

Mark


[-- Attachment #2: mozilla.pdf --]
[-- Type: application/octet-stream, Size: 48535 bytes --]

Re: bug in attachment handling

[Mark Walters]

Hi
On Tue, 30 Jul 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Hi
>
> I have been seeing a bug in attachment handling in recent notmuch. If a
> pdf part is sent as application/octet-stream then notmuch says
> [[file.pdf: application/octet-stream (as application/pdf) ] but when
> viewing it with . v or the default button action (when customised to
> view) it treats it as application/octet-stream.
>
> I think this is a regression but haven't checked yet: it probably has a
> simple fix so we may want to fix it before 0.16 (users may be grumpy
> about the key binding changes etc as it is!)
>
> I attach a pdf with this message (just a mozilla print-to-file of the
> notmuch webpage) which should demonstrate the problem.

I have now checked that it is a regression: 0.15 is fine and git bisect
gives the first bad commit as 1546387d723ec47cd281f3c2bf6da2fddf18c045.

The problem comes because we used to store the content-type with the
button: it was called content-type but in the function
notmuch-show-insert-part-header this was the "calculated content-type"
(with declared-type being the originally declared type).

Elsewhere though content-type is used for the original declared-type
including where the version post 1546 above picks it up (in
notmuch-show-current-part-handle).

So somehow we need to get the calculated content-type into that
function. I am not sure what the cleanest way  to do that is so will
just post this for now.

Best wishes

Mark

[PATCH] emacs: bugfix attachment content-type as mime-type handling

[Mark Walters]

Notmuch puts attachments in as declared content-type except when the
content-type is application/octet-stream it tries to guess the type
from the filename/extension. This means that viewing a pdf (for
example) which is sent as application/octet-strem invokes the pdf
viewer rather than just offering to save the part.

Recent changes to the attachment handling (commit 1546387d) changed
(broke) this. This patch stores the calculated mime-type with the part
and changes the attachment part handlers can use it instead.
---
This seems to fix the bug for me. I am not sure I like the naming
(mime-type/content-type). Maybe it would be better to be explicit with
something like calculated-content-type but probably that should be a
more wholesale renaming.

Best wishes

Mark


 emacs/notmuch-show.el |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index c4e0a99..9f47f5e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -892,6 +892,9 @@ If HIDE is non-nil then initially hide this part."
 		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
 	 (content-beg (point)))
 
+    ;; Store the calculated mime-type for later use (e.g. by attachment handlers).
+    (plist-put part :mime-type mime-type)
+
     (if (not hide)
         (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
       (button-put button :notmuch-lazy-part
@@ -2055,10 +2058,10 @@ caller is responsible for killing this buffer as appropriate."
 	 (message-id (notmuch-show-get-message-id))
 	 (nth (plist-get part :id))
 	 (buf (notmuch-show-generate-part-buffer message-id nth))
-	 (content-type (plist-get part :content-type))
+	 (mime-type (plist-get part :mime-type))
 	 (filename (plist-get part :filename))
 	 (disposition (if filename `(attachment (filename . ,filename)))))
-    (mm-make-handle buf (list content-type) nil nil disposition)))
+    (mm-make-handle buf (list mime-type) nil nil disposition)))
 
 (defun notmuch-show-apply-to-current-part-handle (fn)
   "Apply FN to an mm-handle for the part containing point.
-- 
1.7.9.1

Re: [PATCH] emacs: bugfix attachment content-type as mime-type handling

[Tomi Ollila]

On Tue, Jul 30 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> Notmuch puts attachments in as declared content-type except when the
> content-type is application/octet-stream it tries to guess the type
> from the filename/extension. This means that viewing a pdf (for
> example) which is sent as application/octet-strem invokes the pdf
> viewer rather than just offering to save the part.
>
> Recent changes to the attachment handling (commit 1546387d) changed
> (broke) this. This patch stores the calculated mime-type with the part
> and changes the attachment part handlers can use it instead.
> ---
> This seems to fix the bug for me. I am not sure I like the naming
> (mime-type/content-type). Maybe it would be better to be explicit with
> something like calculated-content-type but probably that should be a
> more wholesale renaming.

I tested this (before and after applying the patch), using notmuch
remotely and viewing id:87ppu0t7yf.fsf@qmul.ac.uk .
Before applying emacs promted me an output file, after applying xdg-open
was executed - which executed evince to show the pdf document in that email.

If this restores the behaviour that was experienced with notmuch 0.15(.2)
then this imho is a good fix to be applied quickly before 0.16 is released...

>
> Best wishes
>
> Mark

Tomi

Re: [PATCH] emacs: bugfix attachment content-type as mime-type handling

[Austin Clements]

Quoth Mark Walters on Jul 30 at  5:15 pm:
> Notmuch puts attachments in as declared content-type except when the
> content-type is application/octet-stream it tries to guess the type
> from the filename/extension. This means that viewing a pdf (for
> example) which is sent as application/octet-strem invokes the pdf
> viewer rather than just offering to save the part.
> 
> Recent changes to the attachment handling (commit 1546387d) changed
> (broke) this. This patch stores the calculated mime-type with the part
> and changes the attachment part handlers can use it instead.
> ---
> This seems to fix the bug for me. I am not sure I like the naming
> (mime-type/content-type). Maybe it would be better to be explicit with
> something like calculated-content-type but probably that should be a
> more wholesale renaming.

Code LGTM, though I agree the naming could be better.  It would be
nice to have a test for this so we don't regress again in the future.

[PATCH v2] emacs: bugfix attachment content-type as mime-type handling

[Mark Walters]

Notmuch puts attachments in as declared content-type except when the
content-type is application/octet-stream it tries to guess the type
from the filename/extension. This means that viewing a pdf (for
example) which is sent as application/octet-strem invokes the pdf
viewer rather than just offering to save the part.

Recent changes to the attachment handling (commit 1546387d) changed
(broke) this. This patch stores the calculated mime-type with the part
and changes the attachment part handlers can use it instead.
---

This is the same as v1 except that it uses the name computed-type
rather than mime-type for the computed mime type.

We may want to change some of the other uses of the mime-type variable
name too but the view on irc was that that could wait.

Best wishes

Mark


 emacs/notmuch-show.el |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index c4e0a99..82b70ba 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -892,6 +892,9 @@ If HIDE is non-nil then initially hide this part."
 		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
 	 (content-beg (point)))
 
+    ;; Store the computed mime-type for later use (e.g. by attachment handlers).
+    (plist-put part :computed-type mime-type)
+
     (if (not hide)
         (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
       (button-put button :notmuch-lazy-part
@@ -2055,10 +2058,10 @@ caller is responsible for killing this buffer as appropriate."
 	 (message-id (notmuch-show-get-message-id))
 	 (nth (plist-get part :id))
 	 (buf (notmuch-show-generate-part-buffer message-id nth))
-	 (content-type (plist-get part :content-type))
+	 (computed-type (plist-get part :computed-type))
 	 (filename (plist-get part :filename))
 	 (disposition (if filename `(attachment (filename . ,filename)))))
-    (mm-make-handle buf (list content-type) nil nil disposition)))
+    (mm-make-handle buf (list computed-type) nil nil disposition)))
 
 (defun notmuch-show-apply-to-current-part-handle (fn)
   "Apply FN to an mm-handle for the part containing point.
-- 
1.7.9.1

Re: [PATCH v2] emacs: bugfix attachment content-type as mime-type handling

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>
> This is the same as v1 except that it uses the name computed-type
> rather than mime-type for the computed mime type.
>

pushed, and tagged as 0.16_rc2

d