code review 4634048: http: permit handlers to explicitly remove the Date header

http: permit handlers to explicitly remove the Date header

We'll do the right thing by default, but people wanting minimal
response sizes can explicitly remove the Date header.
(empty fields aren't written out)

[bradfitz, 2011-06-16 21:37:10]

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

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

[rsc, 2011-06-16 21:42:46]

where is it documented that empty fields are not written out?

[bradfitz, 2011-06-16 21:45:12]

Good question.  Updated docs.  PTAL.

On Thu, Jun 16, 2011 at 2:42 PM, Russ Cox <rsc@golang.org> wrote:

> where is it documented that empty fields are not written out?
>

[bradfitz, 2011-06-16 21:45:12]

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

Please take another look.

[rsc, 2011-06-20 19:13:58]

I don't think this is right.  Conflating the
empty list with a list containing the empty string
usually leads to problems.

Now that the header holds []string instead of string,
I think that the date check in server.go should say

if w.header["Date"] != nil {

and then users can do w.Header()["Date"] = []string{}
to make sure that no Date: lines are sent.

Russ

[bradfitz, 2011-06-20 19:33:09]

Okay, I changed the serve_test from this:

    w.Header().Set("Date", "")
to:
    w.Header()["Date"] = []string{}

The existing logic works unchanged:

     if _, present := w.header["Date"]; !present {

The not writing of empty headers was the existing behavior.  I just
documented it.  Unless you want to be able to send headers with empty
values, in which case we could fix it and document it otherwise.


On Mon, Jun 20, 2011 at 12:13 PM, Russ Cox <rsc@golang.org> wrote:

> I don't think this is right.  Conflating the
> empty list with a list containing the empty string
> usually leads to problems.
>
> Now that the header holds []string instead of string,
> I think that the date check in server.go should say
>
> if w.header["Date"] != nil {
>
> and then users can do w.Header()["Date"] = []string{}
> to make sure that no Date: lines are sent.
>
> Russ
>

[bradfitz, 2011-06-20 19:33:34]

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

Please take another look.

[rsc, 2011-06-20 19:36:29]

I like your new semantics much better.

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

http://codereview.appspot.com/4634048/diff/12001/src/pkg/http/header.go#newco...
src/pkg/http/header.go:46: // Empty values are not written.
Delete.

http://codereview.appspot.com/4634048/diff/12001/src/pkg/http/header.go#newco...
src/pkg/http/header.go:52: // Empty values are not written.
Delete.

http://codereview.appspot.com/4634048/diff/12001/src/pkg/http/serve_test.go
File src/pkg/http/serve_test.go (right):

http://codereview.appspot.com/4634048/diff/12001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:786: w.Header()["Date"] = []string{}
This can even be nil now.  (s/[]string{}/nil/)

http://codereview.appspot.com/4634048/diff/12001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4634048/diff/12001/src/pkg/http/server.go#newco...
src/pkg/http/server.go:257: if _, present := w.header["Date"]; !present {
I think s/present/ok/ is more common but either way.

[bradfitz, 2011-06-20 19:40:54]

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

Please take another look.

[rsc, 2011-06-20 19:43:04]

LGTM

[rsc, 2011-06-20 19:45:53]

I can't figure out where the code is that writes the header,
but if there is a check for "", it should go away.
In general, all the checks for "" should be reexamined
to make sure they shouldn't be checks for present in map.

Russ

[bradfitz, 2011-06-20 19:50:28]

You just commented on the code a minute ago.  It's in header.go.

On Mon, Jun 20, 2011 at 12:45 PM, Russ Cox <rsc@golang.org> wrote:

> I can't figure out where the code is that writes the header,
> but if there is a check for "", it should go away.
> In general, all the checks for "" should be reexamined
> to make sure they shouldn't be checks for present in map.
>
> Russ
>

[rsc, 2011-06-20 19:52:07]

On Mon, Jun 20, 2011 at 15:50, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> You just commented on the code a minute ago.  It's in header.go.

Okay.  Somehow I ended up in transfer.go and lost the trail.
I suspect that this:

			if v == "" {
				continue
			}

should go away.  If someone does h["Foo"] = []string{""}
then I think that should write a Foo: header.

Russ

[bradfitz, 2011-06-20 20:05:09]

Will do.  I see nothing in RFC 2616 prohibiting it.  I don't know why they
were stripped.

On Mon, Jun 20, 2011 at 12:52 PM, Russ Cox <rsc@golang.org> wrote:

> On Mon, Jun 20, 2011 at 15:50, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > You just commented on the code a minute ago.  It's in header.go.
>
> Okay.  Somehow I ended up in transfer.go and lost the trail.
> I suspect that this:
>
>                        if v == "" {
>                                continue
>                        }
>
> should go away.  If someone does h["Foo"] = []string{""}
> then I think that should write a Foo: header.
>
> Russ
>

[rsc, 2011-06-20 20:09:58]

On Mon, Jun 20, 2011 at 16:05, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Will do.  I see nothing in RFC 2616 prohibiting it.  I don't know why they
> were stripped.

Almost certainly because the header used to be map[string]string
so there was no other way to say "no output here".

[bradfitz, 2011-06-20 20:39:14]

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

http: permit handlers to explicitly remove the Date header

We'll do the right thing by default, but people wanting minimal
response sizes can explicitly remove the Date header.
(empty fields aren't written out)

R=rsc
CC=golang-dev
http://codereview.appspot.com/4634048

[bradfitz, 2011-06-21 18:20:52]

*** Abandoned ***

[bradfitz, 2011-06-21 18:28:55]

er, no.  wrong -d vs -D again.  *sigh*

On Tue, Jun 21, 2011 at 11:20 AM, <bradfitz@golang.org> wrote:

> *** Abandoned ***
>
>
http://codereview.appspot.com/**4634048/<http://codereview.appspot.com/4634048/>
>

[rsc, 2011-06-21 18:36:09]

On Tue, Jun 21, 2011 at 14:28, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> er, no.  wrong -d vs -D again.  *sigh*

instead of manual deletion just run hg sync