code review 6405072: textproto: new field serialization routine

textproto: new field serialization routine

[pascal, 2012-07-21 12:55:06]

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

I'd like you to review this change to
https://code.google.com/p/go/

[adg, 2012-07-22 22:31:41]

Does this fix any issues? What's the purpose of this change?

http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer.go
File src/pkg/net/textproto/writer.go (right):

http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer....
src/pkg/net/textproto/writer.go:15: const LongLineLength = 78
need these be exported constants? I don't think so

[pascal, 2012-07-23 17:21:32]

Hi Andrew,

> Does this fix any issues? What's the purpose of this change?

WriteMIMEField adds folding functionality which can be used for writing HTTP 
and SMTP headers. I shared my patch because the resolved comment in 
transfer.go requested it explicitly:

	// TODO: At some point, there should be a generic mechanism for
	// writing long headers, using HTTP line splitting

> http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer
> .go#newcode15 src/pkg/net/textproto/writer.go:15: const LongLineLength = 78
> need these be exported constants? I don't think so

Unlikely indeed.

[bradfitz, 2012-07-30 01:11:46]

http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer.go
File src/pkg/net/textproto/writer.go (right):

http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer....
src/pkg/net/textproto/writer.go:131: field := make([]byte,
len(name)+len(body)+2)
in the common case, this function shouldn't allocate any memory.

[rsc, 2012-07-30 01:15:16]

I'm a little skeptical that this is necessary, but it's definitely quite a bit
more complex than it should be. If you can simplify it then we can look at
whether it pays for itself. Right now it doesn't.

[pascal, 2012-08-03 13:49:21]

Your concerns are well understood. After a forced Google account merge I can't 
submit changes to this issue anymore. The attachment contains an update.

[rsc, 2012-08-03 19:21:47]

Please open a new CL if you want to continue working on this patch.
You can rm $GOROOT/.hg/codereview/cl.6405072 and then run hg change to
make a new one as if this one had never existed.

I looked at the file, though, and it seems like it still allocates in
the common case. The current code uses WriteString explicitly to avoid
that problem.
I'd like to understand what this does to benchmarks (I think Brad has
one in the http package for this function) and I'd also like to
understand how important this really is. Are there commonly used
servers or clients that reject lines > 80 characters?

Russ

[bradfitz, 2012-08-07 04:08:17]

http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/http/header.go
File src/pkg/net/http/header.go (left):

http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/http/header.go#ol...
src/pkg/net/http/header.go:96: ws, ok := w.(writeStringer)
all this writeStringer stuff was done for performance reasons.  I'd hate to
throw that away for a corner case.  If anything, only call
textproto.WriteMIMEField if len(v) > textproto.MaxFieldValueLength (const) or
something.