code review 1681049: mime/multipart and HTTP multipart/form-data support

mime/multipart and HTTP multipart/form-data support

	Somewhat of a work-in-progress (in that MIME is a large spec), but this is
	functional and enough for discussion and/or code review.

	In addition to the unit tests, I've tested with curl and Chrome with
	a variety of test files, making sure the digests of files are unaltered
        when read via a multipart Part.

[brad_danga_com, 2010-07-01 04:45:39]

Maybe my brain is too warped into only understanding git, but I can't figure out
hg or rietveld enough to add this file to this change...

diff -r f050c0ef2f1a src/pkg/http/request.go
--- a/src/pkg/http/request.go   Wed Jun 30 20:45:50 2010 -0700
+++ b/src/pkg/http/request.go   Wed Jun 30 21:44:52 2010 -0700
@@ -16,6 +16,8 @@
        "fmt"
        "io"
        "io/ioutil"
+       "mime/multipart"
+       "mime/contentdisposition"
        "os"
        "strconv"
        "strings"
@@ -139,6 +141,24 @@
                r.ProtoMajor == major && r.ProtoMinor >= minor
 }
 
+// Returns a MIME multipart reader if this is a multipart/form-data POST
request,
+// else returns nil.
+func (r *Request) MultipartReader() multipart.Reader {
+       v, ok := r.Header["Content-Disposition"]
+       if !ok {
+               return nil
+       }
+       d, params := contentdisposition.Parse(v)
+       if d != "form-data" {
+               return nil
+       }
+       boundary, ok := params["boundary"]
+       if !ok {
+               return nil
+       }
+       return multipart.NewReader(r.Body, boundary)
+}
+
 // Return value if nonempty, def otherwise.
 func valueOrDefault(value, def string) string {
        if value != "" {

[brad_danga_com, 2010-07-01 04:48:13]

Please take a (first) look....

Sorry, every time I use either Rietveld or Mercurial I get confused.  Too used
to Git & Gerrit.  :/

[dsymonds, 2010-07-01 05:11:10]

Just one quick thought. Let's see what Russ/Andrew have to say.

http://codereview.appspot.com/1681049/diff/1/4
File src/pkg/mime/contentdisposition/contentdisposition.go (right):

http://codereview.appspot.com/1681049/diff/1/4#newcode5
src/pkg/mime/contentdisposition/contentdisposition.go:5: package
contentdisposition
This doesn't feel like it warrants its own package. I think it would be fine as
part of the MIME package (though still in its own file).

[brad_danga_com, 2010-07-02 04:25:55]

On 2010/07/01 05:11:10, dsymonds1 wrote:
> src/pkg/mime/contentdisposition/contentdisposition.go:5: package
> contentdisposition
> This doesn't feel like it warrants its own package. I think it would be fine
as
> part of the MIME package (though still in its own file).

That's how I had it for awhile, but it made package mime look a little funny in
godoc, having two random unrelated methods.  (mime type lookups and one random
parsing method...)

I'm happy to move it wherever, though.

I'm going to do more browser and MUA testing now, adding more unit tests and
bug/compatibility fixes as necessary, and start on MIME charset decoding next
probably.

[rsc, 2010-07-03 00:12:16]

I'd probably put contentdisposition stuff in mime
under the assumption that it will have other things too.

About to head out the door for a week but I'll have
another look when I get back.

Have fun.
Russ


On Thu, Jul 1, 2010 at 21:25,  <bradfitz@gmail.com> wrote:
> On 2010/07/01 05:11:10, dsymonds1 wrote:
>>
>> src/pkg/mime/contentdisposition/contentdisposition.go:5: package
>> contentdisposition
>> This doesn't feel like it warrants its own package. I think it would
>
> be fine as
>>
>> part of the MIME package (though still in its own file).
>
> That's how I had it for awhile, but it made package mime look a little
> funny in godoc, having two random unrelated methods.  (mime type lookups
> and one random parsing method...)
>
> I'm happy to move it wherever, though.
>
> I'm going to do more browser and MUA testing now, adding more unit tests
> and bug/compatibility fixes as necessary, and start on MIME charset
> decoding next probably.
>
> http://codereview.appspot.com/1681049/show
>

[brad_danga_com, 2010-07-07 02:46:13]

Hello rsc, adg, dsymonds1 (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[brad_danga_com, 2010-07-07 02:46:30]

Hello rsc, adg, dsymonds1 (cc: golang-dev@googlegroups.com),

Please take another look.

[brad_danga_com, 2010-07-07 04:29:27]

Hello rsc, adg, dsymonds1 (cc: golang-dev@googlegroups.com),

Please take another look.

[dangabrad, 2010-07-07 04:33:47]

Sorry for spam.  Fixed a bug found in testing and added more unit tests.

On Tue, Jul 6, 2010 at 9:29 PM, <bradfitz@gmail.com> wrote:

> Hello rsc, adg, dsymonds1 (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/1681049/show
>

[brad_danga_com, 2010-07-07 14:13:00]

Hello rsc, adg, dsymonds1 (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[brad_danga_com, 2010-07-07 14:14:46]

Hello rsc, adg, dsymonds1 (cc: golang-dev@googlegroups.com),

Please take another look.

[dangabrad, 2010-07-07 14:15:18]

Now with missing file grammar.go added to the CL.

On Wed, Jul 7, 2010 at 7:14 AM, <bradfitz@gmail.com> wrote:

> Hello rsc, adg, dsymonds1 (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
> http://codereview.appspot.com/1681049/show
>

[agl1, 2010-07-07 14:51:38]

http://codereview.appspot.com/1681049/diff/30001/20002
File src/pkg/http/request.go (right):

http://codereview.appspot.com/1681049/diff/30001/20002#newcode19
src/pkg/http/request.go:19: "mime/multipart"
Imports are typically in alphabetical order.

http://codereview.appspot.com/1681049/diff/30001/20002#newcode149
src/pkg/http/request.go:149: return nil, os.NewError("No Content-Type header.")
I believe that error strings typically start with a lower case letter so that
they can be printed with "Blah failed: %s"

http://codereview.appspot.com/1681049/diff/30001/20002#newcode153
src/pkg/http/request.go:153: return nil, os.NewError("Content-Type isn't
multipat/form-data")
lower case and typo in "multipart"

http://codereview.appspot.com/1681049/diff/30001/20002#newcode157
src/pkg/http/request.go:157: return nil, os.NewError("No Content-Type 'boundary'
parameter")
lower case

http://codereview.appspot.com/1681049/diff/30001/20006
File src/pkg/mime/contentdisposition/contentdisposition.go (right):

http://codereview.appspot.com/1681049/diff/30001/20006#newcode5
src/pkg/mime/contentdisposition/contentdisposition.go:5: package
contentdisposition
Quick package comment?

http://codereview.appspot.com/1681049/diff/30001/20006#newcode17
src/pkg/mime/contentdisposition/contentdisposition.go:17: // parameters is
non-nil on success and nil on failure.
This comment doesn't quite match up with the code: the returned params is never
nil.

http://codereview.appspot.com/1681049/diff/30001/20006#newcode50
src/pkg/mime/contentdisposition/contentdisposition.go:50: notPos :=
strings.IndexFunc(v, isNotTokenChar)
(aside: this code rather suggests that a SplitFunc func is missing from strings)

http://codereview.appspot.com/1681049/diff/30001/20006#newcode72
src/pkg/mime/contentdisposition/contentdisposition.go:72: var idx int = 0
(nit: could just be "var idx, rune int")

http://codereview.appspot.com/1681049/diff/30001/20006#newcode74
src/pkg/mime/contentdisposition/contentdisposition.go:74: nextIsLiteral := false
these three could also be one line

http://codereview.appspot.com/1681049/diff/30001/20006#newcode78
src/pkg/mime/contentdisposition/contentdisposition.go:78: if nextIsLiteral {
nit: have you considered a "switch {" for all these cases? Would save the
"continues"

switch {
case nextIsLiteral:
  if run >= 0x80 {
     return "", v
  }
  buffer.WriteRune(rune)
  nextIsLiteral = false
case rune == '"':
  return buffer.String, rest[idx+1:]
case mime.IsQText(rune):
  buffer.WriteRune(rune)
case rune == '\\':
  nextIsLiteral = true
default:
  return "", v
}

http://codereview.appspot.com/1681049/diff/30001/20010
File src/pkg/mime/multipart/multipart.go (right):

http://codereview.appspot.com/1681049/diff/30001/20010#newcode41
src/pkg/mime/multipart/multipart.go:41: Header map[string]string
(nit: I would naively expect this to be called 'Headers')

http://codereview.appspot.com/1681049/diff/30001/20010#newcode63
src/pkg/mime/multipart/multipart.go:63: // Creates a new multi-part Reader given
a io.Reader and a MIME
s/a/an/

http://codereview.appspot.com/1681049/diff/30001/20010#newcode113
src/pkg/mime/multipart/multipart.go:113: return os.NewError("Unterminated
headers found parsing multipart body")
Is this code ever reached?

http://codereview.appspot.com/1681049/diff/30001/20010#newcode125
src/pkg/mime/multipart/multipart.go:125: return 0, os.NewError("Unexpected EOF
parsing multipart")
nit: have you considered using io.ErrUnexpectedEOF?

http://codereview.appspot.com/1681049/diff/30001/20010#newcode137
src/pkg/mime/multipart/multipart.go:137: return 0, os.NewError("Unexpected EOF
parsing multipart")
ditto

http://codereview.appspot.com/1681049/diff/30001/20010#newcode212
src/pkg/mime/multipart/multipart.go:212: return nil, os.NewError("Unexpected EOF
during multipart parse.")
(nit: io.ErrUnexpectedEOF again)

http://codereview.appspot.com/1681049/diff/30001/20010#newcode240
src/pkg/mime/multipart/multipart.go:240: panic("")
s/""/"unreachable"/

http://codereview.appspot.com/1681049/diff/30001/20010#newcode254
src/pkg/mime/multipart/multipart.go:254: return
onlyVerticalWhitespace(line[len(dashPrefix):(len(line) - 2)])
no need for the ( ) around len(line) - 2

http://codereview.appspot.com/1681049/diff/30001/20010#newcode259
src/pkg/mime/multipart/multipart.go:259: return
onlyVerticalWhitespace(line[len(dashPrefix):(len(line) - 1)])
ditto

http://codereview.appspot.com/1681049/diff/30001/20010#newcode264
src/pkg/mime/multipart/multipart.go:264: func onlyVerticalWhitespace(s string)
bool {
Isn't this only *horizontal* whitespace?

[brad_danga_com, 2010-07-07 15:37:13]

Hello rsc, adg, dsymonds1, agl1 (cc: golang-dev@googlegroups.com),

Please take another look.

[dangabrad, 2010-07-07 15:40:01]

Will address agl's comments in next CL.  Most recent update was only
cleaning up Makefiles so all.bash and deps.bash would play together nicely.

On Wed, Jul 7, 2010 at 8:37 AM, <bradfitz@gmail.com> wrote:

> Hello rsc, adg, dsymonds1, agl1 (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/1681049/show
>

[brad_danga_com, 2010-07-08 02:59:19]

Hello rsc, adg, dsymonds1, agl1 (cc: golang-dev@googlegroups.com),

Please take another look.

[brad_danga_com, 2010-07-08 02:59:35]

All of agl's comments addressed.  PTAL...

http://codereview.appspot.com/1681049/diff/30001/20002
File src/pkg/http/request.go (right):

http://codereview.appspot.com/1681049/diff/30001/20002#newcode19
src/pkg/http/request.go:19: "mime/multipart"
On 2010/07/07 14:51:38, agl1 wrote:
> Imports are typically in alphabetical order.

Done.

http://codereview.appspot.com/1681049/diff/30001/20006
File src/pkg/mime/contentdisposition/contentdisposition.go (right):

http://codereview.appspot.com/1681049/diff/30001/20006#newcode5
src/pkg/mime/contentdisposition/contentdisposition.go:5: package
contentdisposition
On 2010/07/07 14:51:38, agl1 wrote:
> Quick package comment?

I actually moved this into package mime, per an earlier request, and documented
it, renaming it to its proper name in the process "ParseMediaType", as it's used
by both Content-Type and Content-Disposition, among other things, and "Internet
Media Type" is its official name

http://codereview.appspot.com/1681049/diff/30001/20006#newcode17
src/pkg/mime/contentdisposition/contentdisposition.go:17: // parameters is
non-nil on success and nil on failure.
On 2010/07/07 14:51:38, agl1 wrote:
> This comment doesn't quite match up with the code: the returned params is
never
> nil.

Whoops, fixed code, and added test.

http://codereview.appspot.com/1681049/diff/30001/20006#newcode72
src/pkg/mime/contentdisposition/contentdisposition.go:72: var idx int = 0
On 2010/07/07 14:51:38, agl1 wrote:
> (nit: could just be "var idx, rune int")

Done.

http://codereview.appspot.com/1681049/diff/30001/20006#newcode74
src/pkg/mime/contentdisposition/contentdisposition.go:74: nextIsLiteral := false
On 2010/07/07 14:51:38, agl1 wrote:
> these three could also be one line

Done.

http://codereview.appspot.com/1681049/diff/30001/20006#newcode78
src/pkg/mime/contentdisposition/contentdisposition.go:78: if nextIsLiteral {
On 2010/07/07 14:51:38, agl1 wrote:
> nit: have you considered a "switch {" for all these cases? Would save the
> "continues"

oh, much nicer!

http://codereview.appspot.com/1681049/diff/30001/20010
File src/pkg/mime/multipart/multipart.go (right):

http://codereview.appspot.com/1681049/diff/30001/20010#newcode41
src/pkg/mime/multipart/multipart.go:41: Header map[string]string
On 2010/07/07 14:51:38, agl1 wrote:
> (nit: I would naively expect this to be called 'Headers')

Me too.  That's what I originally named it, but wanted to be consistent with the
existing naming of http.Request.Header

http://codereview.appspot.com/1681049/diff/30001/20010#newcode63
src/pkg/mime/multipart/multipart.go:63: // Creates a new multi-part Reader given
a io.Reader and a MIME
On 2010/07/07 14:51:38, agl1 wrote:
> s/a/an/

Done.

http://codereview.appspot.com/1681049/diff/30001/20010#newcode113
src/pkg/mime/multipart/multipart.go:113: return os.NewError("Unterminated
headers found parsing multipart body")
On 2010/07/07 14:51:38, agl1 wrote:
> Is this code ever reached?

Whoops, fixed.

http://codereview.appspot.com/1681049/diff/30001/20010#newcode125
src/pkg/mime/multipart/multipart.go:125: return 0, os.NewError("Unexpected EOF
parsing multipart")
On 2010/07/07 14:51:38, agl1 wrote:
> nit: have you considered using io.ErrUnexpectedEOF?

Ah, no, I hadn't seen that before.  Changed!

http://codereview.appspot.com/1681049/diff/30001/20010#newcode137
src/pkg/mime/multipart/multipart.go:137: return 0, os.NewError("Unexpected EOF
parsing multipart")
On 2010/07/07 14:51:38, agl1 wrote:
> ditto

Done.

http://codereview.appspot.com/1681049/diff/30001/20010#newcode212
src/pkg/mime/multipart/multipart.go:212: return nil, os.NewError("Unexpected EOF
during multipart parse.")
On 2010/07/07 14:51:38, agl1 wrote:
> (nit: io.ErrUnexpectedEOF again)

Done.

http://codereview.appspot.com/1681049/diff/30001/20010#newcode240
src/pkg/mime/multipart/multipart.go:240: panic("")
On 2010/07/07 14:51:38, agl1 wrote:
> s/""/"unreachable"/

Done.  And in other place.

http://codereview.appspot.com/1681049/diff/30001/20010#newcode254
src/pkg/mime/multipart/multipart.go:254: return
onlyVerticalWhitespace(line[len(dashPrefix):(len(line) - 2)])
On 2010/07/07 14:51:38, agl1 wrote:
> no need for the ( ) around len(line) - 2

Done.

http://codereview.appspot.com/1681049/diff/30001/20010#newcode259
src/pkg/mime/multipart/multipart.go:259: return
onlyVerticalWhitespace(line[len(dashPrefix):(len(line) - 1)])
On 2010/07/07 14:51:38, agl1 wrote:
> ditto

Done.

http://codereview.appspot.com/1681049/diff/30001/20010#newcode264
src/pkg/mime/multipart/multipart.go:264: func onlyVerticalWhitespace(s string)
bool {
On 2010/07/07 14:51:38, agl1 wrote:
> Isn't this only *horizontal* whitespace?

Doh.  Right.  Think I confused myself with reading too many specs and double
negatives.  Or something.  :)

[agl1, 2010-07-08 14:01:47]

LGTM

[brad_danga_com, 2010-07-08 16:20:23]

Hello rsc, adg, dsymonds1, agl1 (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[brad_danga_com, 2010-07-08 16:22:47]

Patch set 10 only updates the package docs for godoc:

http://codereview.appspot.com/1681049/diff2/42001:48001/49009

No code changes.

[rsc1, 2010-07-12 19:50:10]

looks pretty good; thanks for tackling this.
a bunch of comment style nits below.
the recurring one is that unlike typical c++ or java
conventions, the doc comments should be full
sentences beginning with the function being
described, to admit a variety of presentation formats.
(for example, godoc (doesn't but) could format the 
docs as a man page, with all the prototypes in one
section and the descriptions in another.)

http://codereview.appspot.com/1681049/diff/48001/49002
File src/pkg/http/request.go (right):

http://codereview.appspot.com/1681049/diff/48001/49002#newcode144
src/pkg/http/request.go:144: // Returns a MIME multipart reader if this is a
multipart/form-data POST request,
// MultipartReader returns a MIME multipart reader if r is a multipart/form-data
POST request.

(Full sentences; see http://golang.org/doc/effective_go.html#commentary)

http://codereview.appspot.com/1681049/diff/48001/49002#newcode149
src/pkg/http/request.go:149: return nil, os.NewError("no Content-Type header.")
Perhaps these should be ProtocolError?

http://codereview.appspot.com/1681049/diff/48001/49004
File src/pkg/mime/Makefile (right):

http://codereview.appspot.com/1681049/diff/48001/49004#newcode9
src/pkg/mime/Makefile:9: grammar.go\
sort list and add \ on mediatype.go line

http://codereview.appspot.com/1681049/diff/48001/49005
File src/pkg/mime/grammar.go (right):

http://codereview.appspot.com/1681049/diff/48001/49005#newcode16
src/pkg/mime/grammar.go:16: // Valid runes in 'token' as defined by RFC 1531 and
RFC 2045.
// IsTokenChar returns true if rune is in 'token' as defined by ...

http://codereview.appspot.com/1681049/diff/48001/49005#newcode23
src/pkg/mime/grammar.go:23: // Valid runes in 'qtext' as defined by RFC 822.
// IsQText returns true if ...

http://codereview.appspot.com/1681049/diff/48001/49006
File src/pkg/mime/mediatype.go (right):

http://codereview.appspot.com/1681049/diff/48001/49006#newcode13
src/pkg/mime/mediatype.go:13: // Parses a media type value and any optional
parameters, per RFC
// ParseMediaType parses ...

http://codereview.appspot.com/1681049/diff/48001/49006#newcode14
src/pkg/mime/mediatype.go:14: // 1531.  Media types are used as the values in
Content-Type and
s/are used as the/are the/

http://codereview.appspot.com/1681049/diff/48001/49006#newcode16
src/pkg/mime/mediatype.go:16: // in lowercase and trimmed of whitespace.  On
parse error, an empy
On success, ParseMediaType returns the media type converted to lowercase
and trimmed of white space and a non-nil params.  On error, it returns 
an empty string and a nil params.

http://codereview.appspot.com/1681049/diff/48001/49006#newcode48
src/pkg/mime/mediatype.go:48: // Consume a token from the beginning of provided
string, per RFC 2045
// consumeToken ...

http://codereview.appspot.com/1681049/diff/48001/49006#newcode63
src/pkg/mime/mediatype.go:63: // Consumes a "value" per RFC 2045.  (value :=
token / quoted-string)
// consumeValue ...

http://codereview.appspot.com/1681049/diff/48001/49008
File src/pkg/mime/multipart/Makefile (right):

http://codereview.appspot.com/1681049/diff/48001/49008#newcode9
src/pkg/mime/multipart/Makefile:9: multipart.go
trailing \ please
just makes it easier to add more line later

http://codereview.appspot.com/1681049/diff/48001/49009
File src/pkg/mime/multipart/multipart.go (right):

http://codereview.appspot.com/1681049/diff/48001/49009#newcode6
src/pkg/mime/multipart/multipart.go:6: MIME multipart parser, as defined in RFC
2046.
can tighten up the text

/*
Package multipart implements MIME multipart parsing, as defined in RFC 2046.
The implementation is sufficient for HTTP (RFC 2388) and the multipart 
bodies generated by popular browsers.
*/

http://codereview.appspot.com/1681049/diff/48001/49009#newcode13
src/pkg/mime/multipart/multipart.go:13: This package implements a read-only
interface without seeking.  Given
can move to doc comment for Reader

http://codereview.appspot.com/1681049/diff/48001/49009#newcode31
src/pkg/mime/multipart/multipart.go:31: // An iterator over parts in a MIME
multipart body.
good place for the stuff about seeking, in case someone runs

godoc mime/multipart Reader

// A Reader iterates over parts in a MIME multipart body.
// The interface is read only with no seeking.  A Reader
// returns sequential Parts that have Headers and can be Read.

Why is this an interface?
Are there going to be multiple implementations?

http://codereview.appspot.com/1681049/diff/48001/49009#newcode33
src/pkg/mime/multipart/multipart.go:33: // Returns the next Body in the
multipart, or (nil, nil) on EOF.
// NextPart returns

http://codereview.appspot.com/1681049/diff/48001/49009#newcode39
src/pkg/mime/multipart/multipart.go:39: // A part of a multipart body.
// A Part represents a single part in a multipart body.

http://codereview.appspot.com/1681049/diff/48001/49009#newcode50
src/pkg/mime/multipart/multipart.go:50: // If this part has a
Content-Disposition of type "form-data" and has
// FormName returns the name parameter if p has a Content-Disposition
// of type "form-data".  Otherwise it returns the empty string.

http://codereview.appspot.com/1681049/diff/48001/49009#newcode66
src/pkg/mime/multipart/multipart.go:66: // Creates a new multi-part Reader given
an io.Reader and a MIME
// NewReader creates a new multipart Reader reading from r using the given 
// MIME boundary.

probably don't need to mention package http here

http://codereview.appspot.com/1681049/diff/48001/49009#newcode71
src/pkg/mime/multipart/multipart.go:71: mreader := new(multiReader)
can avoid some of the repetition with a composite literal:

return &multiReader{
    boundary: boundary,
    dashBoundary: "--" + boundary,
    endLine: "--" + boundary + "--",
    bufReader: bufio.NewReader(r),
}

http://codereview.appspot.com/1681049/diff/48001/49009#newcode87
src/pkg/mime/multipart/multipart.go:87: var devNull devNullWriter =
devNullWriter(false)
s/devNullWriter =/=/

http://codereview.appspot.com/1681049/diff/48001/49011
File src/pkg/mime/type.go (right):

http://codereview.appspot.com/1681049/diff/48001/49011#newcode5
src/pkg/mime/type.go:5: // A partial MIME package.
// The mime package implements parts of the MIME spec.

[brad_danga_com, 2010-07-13 01:04:30]

On 2010/07/12 19:50:10, rsc1 wrote:
> looks pretty good; thanks for tackling this.
> a bunch of comment style nits below.
> the recurring one is that unlike typical c++ or java
> conventions, the doc comments should be full
> sentences beginning with the function being
> described, to admit a variety of presentation formats.

Gotcha.  Okay, all those comments addressed (locally).  Let me do another read
over it all to make sure they're all in that style and then I'll upload again.

[brad_danga_com, 2010-07-13 15:54:23]

Hello rsc, adg, dsymonds1, agl1 (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-07-14 16:14:25]

LGTM

thanks

[rsc, 2010-07-14 16:36:33]

not sure how this slipped past the codereview
extension on your side, but grammar.go is not
gofmt'ed.  please run

hg gofmt
hg mail 1681049

thanks.  i could do it locally before submitting
but you'll get a merge conflict if i do.

russ

[brad_danga_com, 2010-07-14 19:22:33]

Hello rsc, adg, dsymonds1, agl1 (cc: golang-dev@googlegroups.com),

Please take another look.

[dangabrad, 2010-07-14 19:28:17]

Oh, hg mail did show the warning about gofmt on that file it turns out, but
I'd apparently not seen it at the time.

Fixed and remailed.

On Wed, Jul 14, 2010 at 9:36 AM, Russ Cox <rsc@golang.org> wrote:

> not sure how this slipped past the codereview
> extension on your side, but grammar.go is not
> gofmt'ed.  please run
>
> hg gofmt
> hg mail 1681049
>
> thanks.  i could do it locally before submitting
> but you'll get a merge conflict if i do.
>
> russ
>

[rsc, 2010-07-15 00:26:17]

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

mime/multipart and HTTP multipart/form-data support

Somewhat of a work-in-progress (in that MIME is a large spec), but this is
functional and enough for discussion and/or code review.

In addition to the unit tests, I've tested with curl and Chrome with
a variety of test files, making sure the digests of files are unaltered
when read via a multipart Part.

R=rsc, adg, dsymonds1, agl1
CC=golang-dev
http://codereview.appspot.com/1681049

Committer: Russ Cox <rsc@golang.org>