code review 4426069: http: add Header.Write method

http: add Header.Write method

[eds, 2011-04-28 05:10:15]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2011-04-28 05:18:19]

You should probably add a couple tests for this method too.

http://codereview.appspot.com/4426069/diff/5001/src/pkg/http/cgi/child.go
File src/pkg/http/cgi/child.go (right):

http://codereview.appspot.com/4426069/diff/5001/src/pkg/http/cgi/child.go#new...
src/pkg/http/cgi/child.go:164: r.header.Set("Status", fmt.Sprintf("%d %s", code,
http.StatusText(code)))
Status is special and needs to come on the first line in CGI, iirc.

We can't let it be sorted by Header.Write.  You should keep this handled
separately.

http://codereview.appspot.com/4426069/diff/5001/src/pkg/http/fcgi/child.go
File src/pkg/http/fcgi/child.go (right):

http://codereview.appspot.com/4426069/diff/5001/src/pkg/http/fcgi/child.go#ne...
src/pkg/http/fcgi/child.go:171: r.header.Set("Status", fmt.Sprintf("%d %s",
code, http.StatusText(code)))
order doesn't matter in FastCGI?

http://codereview.appspot.com/4426069/diff/5001/src/pkg/http/header.go
File src/pkg/http/header.go (right):

http://codereview.appspot.com/4426069/diff/5001/src/pkg/http/header.go#newcode47
src/pkg/http/header.go:47: func (h Header) Write(w io.Writer, exclude
map[string]bool) os.Error {
This feels like a weird signature. I expect most users will have a nil exclude.

Perhaps:
  Write(w io.Writer) os.Error
+
  WriteSubset(w io.Writer, exclude map[string]bool) os.Error

I'd like to keep Write simple and matching the other Write signatures.

[eds, 2011-04-28 05:26:52]

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

Please take another look.

[bradfitz, 2011-04-28 05:30:27]

Amuse me with a test or two?

On Wed, Apr 27, 2011 at 10:26 PM, <chickencha@gmail.com> wrote:

> Hello golang-dev@googlegroups.com, bradfitz (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
> http://codereview.appspot.com/4426069/
>

[eds, 2011-04-28 05:33:07]

On Thu, Apr 28, 2011 at 5:30 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Amuse me with a test or two?

Sorry, I was checking off the inline comments and forgot that one.
Tests coming up.

- Evan

[eds, 2011-04-28 05:48:46]

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

Please take another look.

[bradfitz, 2011-04-28 06:02:08]

http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header.go
File src/pkg/http/header.go (right):

http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header.go#newcode73
src/pkg/http/header.go:73: return nil
I think we can/should make this include the final \r\n.

The only caller that would need changing is response.go, and just to reorder the
set-cookie lines earlier.

The other callers all do their own \r\n afterwards.

Update the docs on both methods too, adding ", including the terminating CRLF."

http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header_test.go
File src/pkg/http/header_test.go (right):

http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header_test.go#n...
src/pkg/http/header_test.go:12: var headerWriteTests = []struct {
add a test with duplicate headers?  two values for one header key.

http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header_test.go#n...
src/pkg/http/header_test.go:51: t.Errorf("#%d Header =  %q want %q", i,
buf.String(), test.expected)
when the differing strings are long, we typically try to make them line up:

t.Errorf("test #d:\n got: %q\nwant: %q", i, buf.String(), test.expected)

that makes it easier to diagnose when it fails

[eds, 2011-04-28 06:17:04]

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

Please take another look.

[dsymonds, 2011-04-28 06:23:17]

http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header.go
File src/pkg/http/header.go (right):

http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header.go#newcode73
src/pkg/http/header.go:73: return nil
On 2011/04/28 06:02:08, bradfitz wrote:
> I think we can/should make this include the final \r\n.

Sorry, can I disagree here? The separating "\r\n" is not semantically part of
the header (I think the RFC talks about it as a separator between the header and
body). Adding it here also messes with re-using this code for HTTP Trailers (if
we ever get to implementing them), and it makes printing out this object (e.g.
for debugging) have an unnecessary blank line at the end.

[bradfitz, 2011-04-28 06:40:21]

On Wed, Apr 27, 2011 at 11:23 PM, <dsymonds@golang.org> wrote:

>
> http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header.go
> File src/pkg/http/header.go (right):
>
>
>
http://codereview.appspot.com/4426069/diff/8002/src/pkg/http/header.go#newcode73
> src/pkg/http/header.go:73: return nil
> On 2011/04/28 06:02:08, bradfitz wrote:
>
>> I think we can/should make this include the final \r\n.
>>
>
> Sorry, can I disagree here?


Always. It's encouraged.


> The separating "\r\n" is not semantically
> part of the header (I think the RFC talks about it as a separator
> between the header and body).


Right you are: http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.1


> Adding it here also messes with re-using
> this code for HTTP Trailers (if we ever get to implementing them),


Good arguments. I'm sold.


> and
> it makes printing out this object (e.g. for debugging) have an
> unnecessary blank line at the end.
>
>
> http://codereview.appspot.com/4426069/
>

[eds, 2011-04-28 07:04:01]

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

Please take another look.

[bradfitz, 2011-04-28 07:13:57]

LGTM

On Thu, Apr 28, 2011 at 12:04 AM, <chickencha@gmail.com> wrote:

> Hello golang-dev@googlegroups.com, bradfitz, dsymonds (cc:
>
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4426069/
>

[bradfitz, 2011-04-28 07:16:18]

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

http: add Header.Write method

R=golang-dev, bradfitz, dsymonds
CC=golang-dev
http://codereview.appspot.com/4426069

Committer: Brad Fitzpatrick <bradfitz@golang.org>