New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot start line with ‘On’ in reply #23

Open
sorbits opened this Issue Mar 26, 2014 · 10 comments

Comments

Projects
None yet
4 participants
@sorbits

sorbits commented Mar 26, 2014

A bottom-quoted email that has a line in the reply start with “On” will discard that line and everything below it.

The code responsible can be found on line #87-90 of email_reply_parser.rb:

if text =~ /^(On\s(.+)wrote:)$/nm
  # Remove all new lines from the reply header.
  text.gsub! $1, $1.gsub("\n", " ")
end

An example email that shows the issue follows:

On your remote host you can run:

     telnet 127.0.0.1 52698

This should connect to TextMate (on your Mac, via the tunnel). If that 
fails, the tunnel is not working.

On 9 Jan 2014, at 2:47, George Plymale wrote:

> I am having an odd issue wherein suddenly port forwarding stopped 
> working in a particular scenario for me.  By default I have ssh set to 
> use the following config (my ~/.ssh/config file):
> […]
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/textmate/rmate/issues/29
@danielchatfield

This comment has been minimized.

Contributor

danielchatfield commented Aug 13, 2014

As far as I can see the code you have quoted won't do that - using http://rubular.com I can't get it to match that entire paragraph.

@sorbits

This comment has been minimized.

sorbits commented Aug 13, 2014

You probably forgot to add the options.

Here is the regexp and example text from my issue:
http://rubular.com/r/6VimI2jkFX

On 13 Aug 2014, at 16:37, Daniel Chatfield wrote:

As far as I can see the code you have quoted won't do that - using
http://rubular.com I can't get it to match that entire paragraph.


Reply to this email directly or view it on GitHub:
#23 (comment)

@danielchatfield

This comment has been minimized.

Contributor

danielchatfield commented Aug 13, 2014

👍

@danielchatfield

This comment has been minimized.

Contributor

danielchatfield commented Aug 13, 2014

Could this be fixed by first trying to match a single line and then only if that doesn't match look for a multiline header. Seems like it should work.

danielchatfield added a commit to danielchatfield/email_reply_parser that referenced this issue Aug 13, 2014

Make reply header regexp even less greedy
This is the fix to github#23. It uses the negative lookahead assertion to prevent matches where On appears twice.

The whitespace matcher after the second on is required to make sure that people with names beginning with 'On' don't trigger it.
@danielchatfield

This comment has been minimized.

Contributor

danielchatfield commented Aug 13, 2014

Pull request opened

technoweenie added a commit that referenced this issue Aug 25, 2014

@technoweenie

This comment has been minimized.

Contributor

technoweenie commented Aug 25, 2014

Should be fixed in v0.5.8. If not, please give us another test case.

@sorbits

This comment has been minimized.

sorbits commented Aug 26, 2014

Thanks!

Maybe worth adding the test case from the first PR:
danielchatfield@c7678f0#commitcomment-7374421

On 26 Aug 2014, at 1:53, risk danger olson wrote:

Should be fixed in
v0.5.8.
If not, please give us another test case.


Reply to this email directly or view it on GitHub:
#23 (comment)

@sorbits

This comment has been minimized.

sorbits commented Dec 11, 2014

Is GitHub using this version? Cause I am still occasionally seeing issues, most latest one is here textmate/markdown.tmbundle#25 (comment)

But as far as I can tell, this version should not have a problem with the linked email reply.

@danielchatfield

This comment has been minimized.

Contributor

danielchatfield commented Dec 11, 2014

@sorbits Doesn't look like they have updated.

@gsdean

This comment has been minimized.

gsdean commented Feb 11, 2015

FWIW zapier ported this, and just fixed the same issue
zapier/email-reply-parser#12

msaffitz added a commit to apptentive-engineering/email_reply_parser that referenced this issue May 3, 2017

Merge branch 'master' of https://github.com/github/email_reply_parser
…into github-master

* 'master' of https://github.com/github/email_reply_parser: (30 commits)
  fix warnings
  Remove binary encoding
  Release 0.5.9
  bumping version
  don't force encode emails to binary
  Add license to gemspec; is MIT
  Release 0.5.8
  ignore some stuff
  add test case from github#23
  Making regexp less greedy
  update support url
  update readme
  add script/release and script/test
  Release 0.5.7
  don't use such a greedy matcher
  Release 0.5.6
  fix the regex to ensure the dashes are in front
  Fix link to docs
  Add failing test for parsing inline replies
  Improve signature delimiter regexp so that dashes/underscores/etc in the middle of text don't get interpreted as beginning of signature.
  ...

# Conflicts:
#	lib/email_reply_parser.rb

msaffitz added a commit to apptentive-engineering/email_reply_parser that referenced this issue May 3, 2017

Merge branch 'github-master'
* github-master: (30 commits)
  fix warnings
  Remove binary encoding
  Release 0.5.9
  bumping version
  don't force encode emails to binary
  Add license to gemspec; is MIT
  Release 0.5.8
  ignore some stuff
  add test case from github#23
  Making regexp less greedy
  update support url
  update readme
  add script/release and script/test
  Release 0.5.7
  don't use such a greedy matcher
  Release 0.5.6
  fix the regex to ensure the dashes are in front
  Fix link to docs
  Add failing test for parsing inline replies
  Improve signature delimiter regexp so that dashes/underscores/etc in the middle of text don't get interpreted as beginning of signature.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment