Last Comment Bug 411544 - unnecessary line breaks and excessive indentation in email subject lines
: unnecessary line breaks and excessive indentation in email subject lines
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Email Notifications (show other bugs)
: 3.0
: All All
: -- normal with 1 vote (vote)
: Bugzilla 3.0
Assigned To: Jeff Lawson
: default-qa
:
Mentors:
: 410796 (view as bug list)
Depends on: 374424
Blocks: 410796
  Show dependency treegraph
 
Reported: 2008-01-09 12:01 PST by Jeff Lawson
Modified: 2008-10-01 14:35 PDT (History)
23 users (show)
mkanat: approval+
mkanat: blocking3.1.3+
mkanat: approval3.0+
mkanat: blocking3.0.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
test case demonstating broken fix, plus two alternative fixes (689 bytes, text/plain)
2008-01-09 12:01 PST, Jeff Lawson
no flags Details
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x) (961 bytes, patch)
2008-01-09 14:05 PST, Jeff Lawson
mkanat: review+
Details | Diff | Splinter Review
Eliminate extra line breaks using method 2 (833 bytes, patch)
2008-01-14 09:52 PST, Jeff Lawson
mkanat: review+
jlawson-mozbug: review-
Details | Diff | Splinter Review
Eliminate extra line breaks using method 2 and handle UTF-8 encoded passages (884 bytes, patch)
2008-01-14 10:30 PST, Jeff Lawson
no flags Details | Diff | Splinter Review
avoid excessive line wrapping done by Encode using method 3 (patch for 3.2 Tip) (871 bytes, patch)
2008-01-26 11:02 PST, Jeff Lawson
mkanat: review+
Details | Diff | Splinter Review

Description Jeff Lawson 2008-01-09 12:01:48 PST
Created attachment 296191 [details]
test case demonstating broken fix, plus two alternative fixes

+++ This bug was initially created as a clone of Bug #374424 +++

if a bug's summary has an apostrophe in it (like this one) emails generated by it will have the Subject: line split by a newline.

This is a new bug because the fix performed in Bug 374424 was defective and incomplete.
Comment 1 Jeff Lawson 2008-01-09 14:05:28 PST
Created attachment 296215 [details] [diff] [review]
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)

This patch changes the wrapping boundary used by Encode::MIME, rather than attempting to undo any wrapping that might have been done by it with a regex.
Comment 2 Max Kanat-Alexander 2008-01-09 22:52:50 PST
Comment on attachment 296215 [details] [diff] [review]
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)

No, editing undocumented internal variables is not safe in a distributed product like Bugzilla.
Comment 3 Max Kanat-Alexander 2008-01-14 08:53:28 PST
Comment on attachment 296191 [details]
test case demonstating broken fix, plus two alternative fixes

I'll take your "method 2" here, if you want to attach it.

I like the technical aspects of method 3 better, but I haven't seen that variable documented anywhere, so I'm worried that it could change from under us and we'd have to fix things again later.
Comment 4 Jeff Lawson 2008-01-14 09:52:40 PST
Created attachment 296984 [details] [diff] [review]
Eliminate extra line breaks using method 2

I too agree method 3 would be better, since it avoids having to "undo" the work done by Encode.  But this is not a hill I wanna die on.
Comment 5 Max Kanat-Alexander 2008-01-14 09:53:17 PST
Comment on attachment 296984 [details] [diff] [review]
Eliminate extra line breaks using method 2

Excellent! Thank you very much. :-)
Comment 6 Marc Schumann [:Wurblzap] 2008-01-14 09:59:31 PST
Are the findings of this bug in any way applicable to bug 328628?
Comment 7 Max Kanat-Alexander 2008-01-14 10:02:35 PST
(In reply to comment #6)
> Are the findings of this bug in any way applicable to bug 328628?

  No, they're unrelated.
Comment 8 Marc Schumann [:Wurblzap] 2008-01-14 10:16:11 PST
Hrm, I'm coming late, so maybe we want a new bug for this. It seems to me there's a s/\?=\n =\?UTF-8\?Q\?//g; missing in here. If encoded parts are being split, then each new line starts with =?UTF-8?Q?, which breaks the re-combined string.
Comment 9 Jeff Lawson 2008-01-14 10:30:40 PST
Created attachment 296993 [details] [diff] [review]
Eliminate extra line breaks using method 2 and handle UTF-8 encoded passages

I can repro a problem when the merged lines contain UTF-8 encoded passages.  Here is an updated patch based on the attachment 280276 [details] [diff] [review] from bug 328628.
Comment 10 Frédéric Buclin 2008-01-14 10:33:08 PST
Please don't r- your own patches. If there is something wrong with them, simply mark them as obsolete.
Comment 11 Jeff Lawson 2008-01-14 10:51:02 PST
There is nothing on http://wiki.mozilla.org/Bugzilla:Review nor on http://wiki.mozilla.org/Bugzilla:Submitting_Patches that says I cannot r- my own.
Comment 12 Frédéric Buclin 2008-01-14 10:56:34 PST
Only reviewers can review patches. And now you know. ;)
Comment 13 Max Kanat-Alexander 2008-01-17 10:17:28 PST
Comment on attachment 296993 [details] [diff] [review]
Eliminate extra line breaks using method 2 and handle UTF-8 encoded passages

Ugh. Um, could you maybe contact the Encode maintainer and ask him if your "method 3" is safe and future-proof? It's looking more like we'll want to use that instead. This is a very hacky-looking regex.
Comment 14 Max Kanat-Alexander 2008-01-17 10:18:43 PST
Also, there's nothing technically wrong with r-'ing your own patch. I can't see who that hurts. It's not what we standardly do (we just obsolete things usually), but we've done it ourselves before.
Comment 15 Jeff Lawson 2008-01-17 15:06:55 PST
(In reply to comment #13)
> (From update of attachment 296993 [details] [diff] [review])
> Ugh. Um, could you maybe contact the Encode maintainer and ask him if your
> "method 3" is safe and future-proof? It's looking more like we'll want to use
> that instead. This is a very hacky-looking regex.
> 

Ok, I've emailed 'dankogai@dan.co.jp', who is listed as the maintainer according to http://search.cpan.org/dist/Encode/

I agree that it would be better to use the internal variable and just eliminate the wrapping, rather than attempting to "undo" all variations of the wrapping/escapement possibilities.  Even if it breaks or changes in the future, I argue it would be reasonable to just re-address it again if/when it does.
Comment 16 Frédéric Buclin 2008-01-17 15:19:18 PST
Except that you don't know the version of Encode being used, and you would have to list all possible versions and what to do for each of them.
Comment 17 Max Kanat-Alexander 2008-01-17 15:34:38 PST
You know the minimum version is whatever is shipped with perl 5.8.1.
Comment 18 Jeff Lawson 2008-01-17 16:01:45 PST
If there is ever a change in the future that eliminates the availability of "bpl", then we can add a comparison against the version of $Encode::MIME::Header::VERSION and do different logic as needed.

However, I'm not yet aware of any incompatibilities yet.  Encode-2.12 (released 08 Sep 2005) through the current Encode-2.23 (released 29 May 2007) all seem to include use of the "bpl" variable.  It's probable that earlier versions than 2.12 also made use of that variable but that is the earliest that seems to be available in CPAN:  http://cpan.belfry.net/modules/by-module/Encode/
Comment 19 Jeff Lawson 2008-01-17 16:11:47 PST
For reference, Perl 5.8.1 appears to ship with Encode 1.9801 (released 2003/08/20) and it also includes use of the "bpl" variable.
Comment 20 Frédéric Buclin 2008-01-25 09:54:07 PST
*** Bug 410796 has been marked as a duplicate of this bug. ***
Comment 21 Jeff Lawson 2008-01-25 10:52:48 PST
I have received a reply from the Encode maintainer endorsing the technique used in method 3 (attachment 296215 [details] [diff] [review]).

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff,

Thank you for your report.  As fo the use of bpl, that is okay (you are even careful enough to localize it).  Maybe I should document this on Encode::MIME::Header.

Dan the Encode Maintainer

On Jan 26, 2008, at 02:59 , Jeff Lawson wrote:

> Hello Dan,
>
> I am currently investigating a problem in Mozilla's Bugzilla project 
> that deals with Encode::MIME::Header and its line wrapping of headers 
> that are too long.  This problem is most apparent in the "Subject:" 
> headers since the wrapping causes a visible space to occur in mail 
> readers, sometimes in the middle of a word or after certain 
> punctuation.
>
> I have developed a workaround, but it involves accessing the 
> undocumented "bpl" element, however there have been some concerns over 
> using such an undocumented variable.  Here is an example of the code I 
> am using:
>
>   use Encode::MIME::Header;
>   local $Encode::Encoding{'MIME-Q'}->{'bpl'} = 998;
>   my $encoded = encode('MIME-Q', $value);
>
> I would like to get your opinion about the legitimacy of modifying 
> this internal variable and whether you foresee any immediate concerns 
> about future compatibility.
>
> I have a self-contained Perl test-case that demonstrates the wrapping 
> here:
> https://bugzilla.mozilla.org/attachment.cgi?id=296186
>
>
> Also, if you could comment on why the line wrapping is causing 
> problems with email readers at all, that would be helpful too.
>
>
> The bugs that relate to this issue are available here:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=411544
> https://bugzilla.mozilla.org/show_bug.cgi?id=374424
> http://rt.cpan.org/Public/Bug/Display.html?id=27395
>
>
> Thanks,
> Jeff
>
>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iD8DBQFHmi6vErJia/WXtBsRAurVAJ9TXHxWhm0yfkdz0U4dIPMo2GmLrACePzwL
l0UQ8CUztCvncH8Ix0SNHn8=
=hCvu
-----END PGP SIGNATURE-----

Comment 22 Jeff Lawson 2008-01-25 10:54:20 PST
Comment on attachment 296215 [details] [diff] [review]
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)

seek re-approval after endorsement of technique.
Comment 23 Max Kanat-Alexander 2008-01-25 15:30:50 PST
Comment on attachment 296215 [details] [diff] [review]
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)

Okay, looks good! :-)
Comment 24 Max Kanat-Alexander 2008-01-25 23:22:05 PST
No, process is that it stays assigned to you. :-) I keep track of everything that needs to be checked in by a saved search, no worries.
Comment 25 Frédéric Buclin 2008-01-26 02:54:35 PST
Canceling approval+ for 3.2. This patch doesn't apply cleanly on tip as this part of the code changed quite a bit:

http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/Bugzilla/Mailer.pm#75

It requires a separate patch for 3.2. Jeff, can you write the patch?
Comment 26 Jeff Lawson 2008-01-26 11:02:01 PST
Created attachment 299434 [details] [diff] [review]
 avoid excessive line wrapping done by Encode using method 3 (patch for 3.2 Tip)  

patch against Bugzilla-3.2 (tip)
Comment 27 Max Kanat-Alexander 2008-01-26 16:04:47 PST
Comment on attachment 299434 [details] [diff] [review]
 avoid excessive line wrapping done by Encode using method 3 (patch for 3.2 Tip)  

Actually, I'm just realizing that there's a "use" statement in the middle of the code here. We should be "use"ing that at the top, or this should be a "require".

That needs to be fixed on checkin for both patches.
Comment 28 Max Kanat-Alexander 2008-01-27 10:13:31 PST
Thank you very much for this patch, Jeff!

tip:

Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.17; previous revision: 1.16
done

3.0 branch:

Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.7.2.7; previous revision: 1.7.2.6
done

Note You need to log in before you can comment on or make changes to this bug.