code review 6854067: mime/multipart: transparently decode quoted-printable t...

mime/multipart: transparently decode quoted-printable transfer encoding

Fixes issue 4411

[bradfitz, 2012-11-20 01:01:04]

Hello golang-dev@googlegroups.com,

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

[gobot, 2012-11-20 01:15:46]

R=dsymonds (assigned by bradfitz)

[dsymonds, 2012-11-20 01:35:26]

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/multi...
File src/pkg/mime/multipart/multipart.go (right):

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/multi...
src/pkg/mime/multipart/multipart.go:104:
bp.Header.Del("Content-Transfer-Encoding")
I'd be fractionally more comfortable with a little
  const cte = "Content-Transfer-Encoding"

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
File src/pkg/mime/multipart/quotedprintable.go (right):

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
src/pkg/mime/multipart/quotedprintable.go:18: rerr error  // last read error
the "r" prefix here seems unnecessary

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
src/pkg/mime/multipart/quotedprintable.go:32: case b >= 'A' && b <= 'F':
can't get lowercase hex? I know the RFC doesn't permit them, but perhaps they
appear in the wild?

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
src/pkg/mime/multipart/quotedprintable.go:67: q.line =
bytes.TrimRightFunc(q.line, isQPDiscardWhitespace)
bytes.TrimRight(q.line, " \t\r\n")?

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
src/pkg/mime/multipart/quotedprintable.go:82: q.line = q.line[2:]
shouldn't this be [3:]? One for the '=', and two for the hex?

[bradfitz, 2012-11-20 02:10:57]

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/multi...
File src/pkg/mime/multipart/multipart.go (right):

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/multi...
src/pkg/mime/multipart/multipart.go:104:
bp.Header.Del("Content-Transfer-Encoding")
On 2012/11/20 01:35:26, dsymonds wrote:
> I'd be fractionally more comfortable with a little
>   const cte = "Content-Transfer-Encoding"

Done.

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
File src/pkg/mime/multipart/quotedprintable.go (right):

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
src/pkg/mime/multipart/quotedprintable.go:18: rerr error  // last read error
On 2012/11/20 01:35:26, dsymonds wrote:
> the "r" prefix here seems unnecessary

I prefer it, so it doesn't seem like a general sticky error.

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
src/pkg/mime/multipart/quotedprintable.go:32: case b >= 'A' && b <= 'F':
On 2012/11/20 01:35:26, dsymonds wrote:
> can't get lowercase hex? I know the RFC doesn't permit them, but perhaps they
> appear in the wild?

I'd like to start by following the spec and solve our immediate problems and
only resort to disobeying it later, if problems arise.

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
src/pkg/mime/multipart/quotedprintable.go:67: q.line =
bytes.TrimRightFunc(q.line, isQPDiscardWhitespace)
On 2012/11/20 01:35:26, dsymonds wrote:
> bytes.TrimRight(q.line, " \t\r\n")?

generates garbage.

https://codereview.appspot.com/6854067/diff/5001/src/pkg/mime/multipart/quote...
src/pkg/mime/multipart/quotedprintable.go:82: q.line = q.line[2:]
On 2012/11/20 01:35:26, dsymonds wrote:
> shouldn't this be [3:]? One for the '=', and two for the hex?

another 1 is done below.  adding a comment.

[bradfitz, 2012-11-20 02:12:06]

Hello dsymonds@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2012-11-20 03:17:14]

LGTM

[bradfitz, 2012-11-20 03:50:27]

*** Submitted as http://code.google.com/p/go/source/detail?r=73c62d1bf08c ***

mime/multipart: transparently decode quoted-printable transfer encoding

Fixes issue 4411

R=dsymonds
CC=gobot, golang-dev
http://codereview.appspot.com/6854067