[PATCH 0/2] Bug and potential fix: forwarded messages

[PATCH 0/2] Bug and potential fix: forwarded messages

[Adam Wolfe Gordon]

Hi everyone,

I encountered today a bug that I remember being mentioned on IRC where
some messages are shown in emacs as completely empty, even though their
content is present and simple. For me, the bug shows up with messages that
have been forwarded by an Outlook user who didn't add any content to the top
of the message. The first line of the message is "-----Original Message-----",
and the rest is the forwarded text. Notmuch tries to collapse the forwarded
content into a button, but fails mysteriously.

The first patch in this series adds a test (marked as broken) that demonstrates
the bug. I think this should be pushed regardless of whether my solution is
the right one. My solution is to check whether we're collapsing the entire
message before doing so, and avoid turning it into a button if that's the case.
I think this is a desirable behavior, since if someone has forwarded a message
without adding anything the user probably wants to read that message. But,
I'll admit that I didn't figure out the real cause of the problem, and I
would be happy to hear other suggestions.

It also occurs to me that this might indicate a bigger problem with how
notmuch-wash.el handles messages starting with "-----Original Message-----".
Notmuch seems to assume that this indicates the rest of the message is quoted
stuff that's been top-posted on. In my office this isn't necessarily the case,
since Outlook produces that line at the top of every reply, and it's up to
the user whether to top-post or not (and not everyone does).

I'll have to experiment a bit more to verify whether a problem (i.e. whether
Notmuch hides inappropriate things when someone replies inline with Outlook).
Someone please correct me if I'm missing something in how the code operates.

Adam Wolfe Gordon (2):
  test: Add broken test for showing forwarded messages
  emacs: Fix broken showing of forwarded messages.

 emacs/notmuch-wash.el |    5 +++--
 test/emacs            |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] test: Add broken test for showing forwarded messages

[Adam Wolfe Gordon]

Messages are sometimes forwarded with no new text in them, such
that the entire message looks like a top-posted reply with no reply
text. Showing such a message in emacs results in no message body
or button being inserted, so the buffer is empty except for headers.
---
 test/emacs |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 8ca4c8a..b8b13a7 100755
--- a/test/emacs
+++ b/test/emacs
@@ -349,6 +349,39 @@ Thanks for the advice! I will be sure to put it to good use.
 [ 9-line hidden original message. Click/Enter to show. ]" > EXPECTED
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "Non-hiding of forwarded message with no new text"
+test_subtest_known_broken
+add_message [id]=forwarded-top-post \
+	    '[subject]="FW: The problem with top-posting"' \
+	    '[body]="
+
+-----Original Message-----
+From: Top Poster <top@poster.com>
+To: Notmuch Test Suite <test_suite@notmuchmai.org>
+Sent: Fri, 05 Jan 2001 15:43:57 +0000
+Subject: FW: The problem with top-posting
+
+Q: Why is top-posting such a bad thing?
+A: Top-posting.
+Q: What is the most annoying thing in e-mail?"'
+test_emacs "(notmuch-show \"FW: The problem with top-posting\")
+	    (test-visible-output)"
+echo "Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-05) (inbox)
+Subject: FW: The problem with top-posting
+To: Notmuch Test Suite <test_suite@notmuchmail.org>
+Date: Fri, 05 Jan 2001 15:43:57 +0000
+
+-----Original Message-----
+From: Top Poster <top@poster.com>
+To: Notmuch Test Suite <test_suite@notmuchmai.org>
+Sent: Fri, 05 Jan 2001 15:43:57 +0000
+Subject: FW: The problem with top-posting
+
+Q: Why is top-posting such a bad thing?
+A: Top-posting.
+Q: What is the most annoying thing in e-mail?" > EXPECTED
+test_expect_equal_file OUTPUT EXPECTED
+
 test_begin_subtest "Hiding message in notmuch-show view"
 test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com")
 	    (notmuch-show-toggle-message)
-- 
1.7.5.4

[PATCH 2/2] emacs: Fix broken showing of forwarded messages.

[Adam Wolfe Gordon]

Fix the bug in showing forwarded messages with no new text by not
hiding "original" messages if they are the entire message.
---
 emacs/notmuch-wash.el |    5 +++--
 test/emacs            |    1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 67143e5..2fe9e5d 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -176,8 +176,9 @@ insert before the button, probably for indentation."
       (let* ((msg-start (match-beginning 0))
 	     (msg-end (point-max))
 	     (msg-lines (count-lines msg-start msg-end)))
-	(notmuch-wash-region-to-button
-	 msg msg-start msg-end "original" "\n")))
+	(when (< msg-lines (count-lines (point-min) (point-max)))
+	  (notmuch-wash-region-to-button
+	   msg msg-start msg-end "original" "\n"))))
   (while (and (< (point) (point-max))
 	      (re-search-forward notmuch-wash-citation-regexp nil t))
     (let* ((cite-start (match-beginning 0))
diff --git a/test/emacs b/test/emacs
index b8b13a7..52683dd 100755
--- a/test/emacs
+++ b/test/emacs
@@ -350,7 +350,6 @@ Thanks for the advice! I will be sure to put it to good use.
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Non-hiding of forwarded message with no new text"
-test_subtest_known_broken
 add_message [id]=forwarded-top-post \
 	    '[subject]="FW: The problem with top-posting"' \
 	    '[body]="
-- 
1.7.5.4

Re: [PATCH 0/2] Bug and potential fix: forwarded messages

[Adam Wolfe Gordon]

Oh, and I just noticed that Dmitry has already fixed this, probably in
a better way [1] (though I maintain that there still may be a problem
with the approach in general).  I clearly haven't been following the
list closely enough this week.

[1] id:"1327926286-16680-1-git-send-email-dmitry.kurochkin@gmail.com"

On Fri, Feb 3, 2012 at 17:32, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> Hi everyone,
>
> I encountered today a bug that I remember being mentioned on IRC where
> some messages are shown in emacs as completely empty, even though their
> content is present and simple. For me, the bug shows up with messages that
> have been forwarded by an Outlook user who didn't add any content to the top
> of the message. The first line of the message is "-----Original Message-----",
> and the rest is the forwarded text. Notmuch tries to collapse the forwarded
> content into a button, but fails mysteriously.
>
> The first patch in this series adds a test (marked as broken) that demonstrates
> the bug. I think this should be pushed regardless of whether my solution is
> the right one. My solution is to check whether we're collapsing the entire
> message before doing so, and avoid turning it into a button if that's the case.
> I think this is a desirable behavior, since if someone has forwarded a message
> without adding anything the user probably wants to read that message. But,
> I'll admit that I didn't figure out the real cause of the problem, and I
> would be happy to hear other suggestions.
>
> It also occurs to me that this might indicate a bigger problem with how
> notmuch-wash.el handles messages starting with "-----Original Message-----".
> Notmuch seems to assume that this indicates the rest of the message is quoted
> stuff that's been top-posted on. In my office this isn't necessarily the case,
> since Outlook produces that line at the top of every reply, and it's up to
> the user whether to top-post or not (and not everyone does).
>
> I'll have to experiment a bit more to verify whether a problem (i.e. whether
> Notmuch hides inappropriate things when someone replies inline with Outlook).
> Someone please correct me if I'm missing something in how the code operates.
>
> Adam Wolfe Gordon (2):
>  test: Add broken test for showing forwarded messages
>  emacs: Fix broken showing of forwarded messages.
>
>  emacs/notmuch-wash.el |    5 +++--
>  test/emacs            |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 2 deletions(-)
>
> --
> 1.7.5.4
>

Re: [PATCH 0/2] Bug and potential fix: forwarded messages

[Dmitry Kurochkin]

On Fri, 3 Feb 2012 23:01:23 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> Oh, and I just noticed that Dmitry has already fixed this, probably in
> a better way [1] (though I maintain that there still may be a problem
> with the approach in general).  I clearly haven't been following the
> list closely enough this week.
> 

Yep, creating buttons for regions at the beginning of message is fixed.
But I like the idea of not hiding the message if there is no text.
Though, I think we should create the button, just not hide the text
initially.

This change would conflict with my fix, so we should probably wait until
it is applied before working on this.

Regards,
  Dmitry

> [1] id:"1327926286-16680-1-git-send-email-dmitry.kurochkin@gmail.com"
> 
> On Fri, Feb 3, 2012 at 17:32, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> > Hi everyone,
> >
> > I encountered today a bug that I remember being mentioned on IRC where
> > some messages are shown in emacs as completely empty, even though their
> > content is present and simple. For me, the bug shows up with messages that
> > have been forwarded by an Outlook user who didn't add any content to the top
> > of the message. The first line of the message is "-----Original Message-----",
> > and the rest is the forwarded text. Notmuch tries to collapse the forwarded
> > content into a button, but fails mysteriously.
> >
> > The first patch in this series adds a test (marked as broken) that demonstrates
> > the bug. I think this should be pushed regardless of whether my solution is
> > the right one. My solution is to check whether we're collapsing the entire
> > message before doing so, and avoid turning it into a button if that's the case.
> > I think this is a desirable behavior, since if someone has forwarded a message
> > without adding anything the user probably wants to read that message. But,
> > I'll admit that I didn't figure out the real cause of the problem, and I
> > would be happy to hear other suggestions.
> >
> > It also occurs to me that this might indicate a bigger problem with how
> > notmuch-wash.el handles messages starting with "-----Original Message-----".
> > Notmuch seems to assume that this indicates the rest of the message is quoted
> > stuff that's been top-posted on. In my office this isn't necessarily the case,
> > since Outlook produces that line at the top of every reply, and it's up to
> > the user whether to top-post or not (and not everyone does).
> >
> > I'll have to experiment a bit more to verify whether a problem (i.e. whether
> > Notmuch hides inappropriate things when someone replies inline with Outlook).
> > Someone please correct me if I'm missing something in how the code operates.
> >
> > Adam Wolfe Gordon (2):
> >  test: Add broken test for showing forwarded messages
> >  emacs: Fix broken showing of forwarded messages.
> >
> >  emacs/notmuch-wash.el |    5 +++--
> >  test/emacs            |   32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+), 2 deletions(-)
> >
> > --
> > 1.7.5.4
> >
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 0/2] Bug and potential fix: forwarded messages

[Adam Wolfe Gordon]

On Fri, Feb 3, 2012 at 23:10, Dmitry Kurochkin
<dmitry.kurochkin@gmail.com> wrote:
> Yep, creating buttons for regions at the beginning of message is fixed.
> But I like the idea of not hiding the message if there is no text.
> Though, I think we should create the button, just not hide the text
> initially.
>
> This change would conflict with my fix, so we should probably wait until
> it is applied before working on this.

Yes, this sounds like a good idea.  I'll wait until your fix is in
before giving it more thought.