code review 8536045: mime/multipart: don't strip leading space/tab in quoted...

mime/multipart: don't strip leading space/tab in quoted-printable decoding

Late bug fix, but this is arguably a regression from Go 1.0,
since we added this transparent decoding since then. Without
this fix, Go 1.0 users could decode this correctly, but Go 1.1
users would not be able to.

The newly added test is from the RFC itself.

The updated tests had the wrong "want" values before. They
were there to test \r\n vs \n equivalence (which is
unchanged), not leading whitespace.

The skipWhite decoder struct field was added in the battles of
Issue 4771 in revision b3bb265bfecf. It was just a wrong
strategy, from an earlier round of attempts in
https://codereview.appspot.com/7300092/

Update issue 4771
Fixes issue 5295

[bradfitz, 2013-04-17 20:53:16]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2013-04-17 20:58:27]

R=minux, r



On Wed, Apr 17, 2013 at 1:53 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> mime/multipart: don't strip leading space/tab in quoted-printable
> decoding
>
> Late bug fix, but this is arguably a regression from Go 1.0,
> since we added this transparent decoding since then. Without
> this fix, Go 1.0 users could decode this correctly, but Go 1.1
> users would not be able to.
>
> The newly added test is from the RFC itself.
>
> The updated tests had the wrong "want" values before. They
> were there to test \r\n vs \n equivalence (which is
> unchanged), not leading whitespace.
>
> The skipWhite decoder struct field was added in the battles of
> Issue 4771 in revision b3bb265bfecf. It was just a wrong
> strategy, from an earlier round of attempts in
>
https://codereview.appspot.**com/7300092/<https://codereview.appspot.com/7300...
>
> Update issue 4771
> Fixes issue 5295
>
> Please review this at
https://codereview.appspot.**com/8536045/<https://codereview.appspot.com/8536...
>
> Affected files:
>   M src/pkg/mime/multipart/**quotedprintable.go
>   M src/pkg/mime/multipart/**quotedprintable_test.go
>
>
> Index: src/pkg/mime/multipart/**quotedprintable.go
> ==============================**==============================**=======
> --- a/src/pkg/mime/multipart/**quotedprintable.go
> +++ b/src/pkg/mime/multipart/**quotedprintable.go
> @@ -55,10 +55,6 @@
>         return hb<<4 | lb, nil
>  }
>
> -func isQPSkipWhiteByte(b byte) bool {
> -       return b == ' ' || b == '\t'
> -}
> -
>  func isQPDiscardWhitespace(r rune) bool {
>         switch r {
>         case '\n', '\r', ' ', '\t':
> @@ -79,7 +75,6 @@
>                         if q.rerr != nil {
>                                 return n, q.rerr
>                         }
> -                       q.skipWhite = true
>                         q.line, q.rerr = q.br.ReadSlice('\n')
>
>                         // Does the line end in CRLF instead of just LF?
> @@ -103,11 +98,6 @@
>                         continue
>                 }
>                 b := q.line[0]
> -               if q.skipWhite && isQPSkipWhiteByte(b) {
> -                       q.line = q.line[1:]
> -                       continue
> -               }
> -               q.skipWhite = false
>
>                 switch {
>                 case b == '=':
> Index: src/pkg/mime/multipart/**quotedprintable_test.go
> ==============================**==============================**=======
> --- a/src/pkg/mime/multipart/**quotedprintable_test.go
> +++ b/src/pkg/mime/multipart/**quotedprintable_test.go
> @@ -32,8 +32,8 @@
>                 {in: "foo bar=0", want: "foo bar", err:
> io.ErrUnexpectedEOF},
>                 {in: "foo bar=ab", want: "foo bar", err: "multipart:
> invalid quoted-printable hex byte 0x61"},
>                 {in: "foo bar=0D=0A", want: "foo bar\r\n"},
> -               {in: " A B =\r\n C ", want: "A B C"},
> -               {in: " A B =\n C ", want: "A B C"}, // lax. treating LF as
> CRLF
> +               {in: " A B =\r\n C ", want: " A B  C"},
> +               {in: " A B =\n C ", want: " A B  C"}, // lax. treating LF
> as CRLF
>                 {in: "foo=\nbar", want: "foobar"},
>                 {in: "foo\x00bar", want: "foo", err: "multipart: invalid
> unescaped byte 0x00 in quoted-printable body"},
>                 {in: "foo bar\xff", want: "foo bar", err: "multipart:
> invalid unescaped byte 0xff in quoted-printable body"},
> @@ -57,6 +57,10 @@
>                 {in: "foo=\nbar", want: "foobar"},
>                 {in: "foo=\rbar", want: "foo", err: "multipart: invalid
> quoted-printable hex byte 0x0d"},
>                 {in: "foo=\r\r\r \nbar", want: "foo", err: `multipart:
> invalid bytes after =: "\r\r\r \n"`},
> +
> +               // Exsample from RFC 2045:
> +               {in: "Now's the time =\n" + "for all folk to come=\n" + "
> to the aid of their country.",
> +                       want: "Now's the time for all folk to come to the
> aid of their country."},
>         }
>         for _, tt := range tests {
>                 var buf bytes.Buffer
>
>
>

[r, 2013-04-17 22:54:23]

LGTM

[bradfitz, 2013-04-18 03:05:12]

*** Submitted as https://code.google.com/p/go/source/detail?r=fa6a760fcc63 ***

mime/multipart: don't strip leading space/tab in quoted-printable decoding

Late bug fix, but this is arguably a regression from Go 1.0,
since we added this transparent decoding since then. Without
this fix, Go 1.0 users could decode this correctly, but Go 1.1
users would not be able to.

The newly added test is from the RFC itself.

The updated tests had the wrong "want" values before. They
were there to test \r\n vs \n equivalence (which is
unchanged), not leading whitespace.

The skipWhite decoder struct field was added in the battles of
Issue 4771 in revision b3bb265bfecf. It was just a wrong
strategy, from an earlier round of attempts in
https://codereview.appspot.com/7300092/

Update issue 4771
Fixes issue 5295

R=golang-dev, r
CC=golang-dev
https://codereview.appspot.com/8536045

[minux1, 2013-04-19 01:27:51]

LGTM.