code review 4635063: mime/multipart: remove newline at top of the multipart.

mime/multipart: remove newline at top of the multipart.

[mattn, 2011-06-23 09:41:22]

Hello golang-dev@googlecode.com (cc: golang-dev@googlegroups.com),

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

[bradfitz, 2011-06-23 15:24:43]

Why?  I can't imagine this breaks anything.  Lots of multipart
implementations start with a banner like,

        This email is a multi-part mime message!  Your mail client can't
read it.
        --BOUNDARY

What's the harm in a leading newline?

On Thu, Jun 23, 2011 at 2:41 AM, <mattn.jp@gmail.com> wrote:

> Reviewers: golang-dev_googlecode.com,
>
> Message:
> Hello golang-dev@googlecode.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> http://go.googlecode.com/hg/
>
>
> Description:
> mime/multipart: remove newline at top of the multipart.
>
> Please review this at
http://codereview.appspot.com/**4635063/<http://codereview.appspot.com/4635063/>
>
> Affected files:
>  M src/pkg/mime/multipart/writer.**go
>
>
> Index: src/pkg/mime/multipart/writer.**go
> ==============================**==============================**=======
> --- a/src/pkg/mime/multipart/**writer.go
> +++ b/src/pkg/mime/multipart/**writer.go
> @@ -61,7 +61,11 @@
>                }
>        }
>        var b bytes.Buffer
> -       fmt.Fprintf(&b, "\r\n--%s\r\n", w.boundary)
> +       if w.lastpart != nil {
> +               fmt.Fprintf(&b, "\r\n--%s\r\n", w.boundary)
> +       } else {
> +               fmt.Fprintf(&b, "--%s\r\n", w.boundary)
> +       }
>        // TODO(bradfitz): move this to textproto.MimeHeader.Write(w), have
> it sort
>        // and clean, like http.Header.Write(w) does.
>        for k, vv := range header {
>
>
>

[cainetighe, 2011-06-23 15:45:27]

I would tend to agree with Brad on this one.  The cleaner code is not
incorrect and easier to understand.

caine

On Thu, Jun 23, 2011 at 11:24 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Why?  I can't imagine this breaks anything.  Lots of multipart
> implementations start with a banner like,
>         This email is a multi-part mime message!  Your mail client can't
> read it.
>         --BOUNDARY
> What's the harm in a leading newline?
> On Thu, Jun 23, 2011 at 2:41 AM, <mattn.jp@gmail.com> wrote:
>>
>> Reviewers: golang-dev_googlecode.com,
>>
>> Message:
>> Hello golang-dev@googlecode.com (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> http://go.googlecode.com/hg/
>>
>>
>> Description:
>> mime/multipart: remove newline at top of the multipart.
>>
>> Please review this at http://codereview.appspot.com/4635063/
>>
>> Affected files:
>>  M src/pkg/mime/multipart/writer.go
>>
>>
>> Index: src/pkg/mime/multipart/writer.go
>> ===================================================================
>> --- a/src/pkg/mime/multipart/writer.go
>> +++ b/src/pkg/mime/multipart/writer.go
>> @@ -61,7 +61,11 @@
>>                }
>>        }
>>        var b bytes.Buffer
>> -       fmt.Fprintf(&b, "\r\n--%s\r\n", w.boundary)
>> +       if w.lastpart != nil {
>> +               fmt.Fprintf(&b, "\r\n--%s\r\n", w.boundary)
>> +       } else {
>> +               fmt.Fprintf(&b, "--%s\r\n", w.boundary)
>> +       }
>>        // TODO(bradfitz): move this to textproto.MimeHeader.Write(w), have
>> it sort
>>        // and clean, like http.Header.Write(w) does.
>>        for k, vv := range header {
>>
>>
>
>

[bradfitz, 2011-06-23 16:25:25]

Okay.  Well, if it's worth fixing to work-around a bug in Rack, it's worth
adding a tiny test for.

Add a test and I'm happy.

On Thu, Jun 23, 2011 at 9:12 AM, mattn <mattn.jp@gmail.com> wrote:

> Current code always add newline at the start of multipart.
> But some webserver expect the begining line should be started with
> boundary.
> For example rack-1.2 is that.
>
> https://github.com/rack/rack/blob/rack-1.2/lib/rack/utils.rb#L495
>
> Probably, rack-1.3 or later is fixed.
>
> I tried sinatra app and go. But it occur "bad content body". :-(
>
>

[mattn, 2011-06-23 16:33:13]

Hello golang-dev@googlecode.com, bradfitz@golang.org, arctanofyourface@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-06-23 17:11:51]

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

mime/multipart: remove newline at top of the multipart.

R=golang-dev, bradfitz, arctanofyourface
CC=golang-dev
http://codereview.appspot.com/4635063

Committer: Brad Fitzpatrick <bradfitz@golang.org>