code review 5633045: net/http: Don't set Content-Type header for HEAD reques...

net/http: Don't set Content-Type header for HEAD requests by default
since the real type is not inferred.
Fixes issue 2885.

[pmylund, 2012-02-06 04:41:21]

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

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

[dsymonds, 2012-02-06 04:55:55]

add a test please

http://codereview.appspot.com/5633045/diff/3001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

http://codereview.appspot.com/5633045/diff/3001/src/pkg/net/http/server.go#ne...
src/pkg/net/http/server.go:331: if code == StatusNotModified {
add
  || w.req.Method == "HEAD"
to this branch instead, and ditch the Content-Type part below.

[dsymonds, 2012-02-06 04:59:43]

http://codereview.appspot.com/5633045/diff/3001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

http://codereview.appspot.com/5633045/diff/3001/src/pkg/net/http/server.go#ne...
src/pkg/net/http/server.go:344: if w.header.Get("Content-Type") == "" &&
w.req.Method != "HEAD" {
actually, leave it here. The previous block removes a bunch of headers that
should be present in HEAD responses.

[pmylund, 2012-02-06 05:11:18]

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

Please take another look.

[pmylund, 2012-02-06 05:12:47]

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

Please take another look.

[dsymonds, 2012-02-06 05:16:22]

LGTM

but leaving for Brad/Russ to sanity check and submit.

http://codereview.appspot.com/5633045/diff/4002/src/pkg/net/http/serve_test.go
File src/pkg/net/http/serve_test.go (right):

http://codereview.appspot.com/5633045/diff/4002/src/pkg/net/http/serve_test.g...
src/pkg/net/http/serve_test.go:509: // Content-Type by default since the real
type of the body data is
s/is not/cannot/

http://codereview.appspot.com/5633045/diff/4002/src/pkg/net/http/serve_test.g...
src/pkg/net/http/serve_test.go:532: contentType :=
res.Header.Get("Content-Type")
if ct := ...; ct != "" {
  ...
}

[dsymonds, 2012-02-06 05:17:32]

Also, please fill out the CLA.

http://golang.org/doc/contribute.html#copyright

[pmylund, 2012-02-06 05:18:06]

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

Please take another look.

[pmylund, 2012-02-06 05:19:11]

On 2012/02/06 05:18:06, pmylund wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:dsymonds@golang.org (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Pardon the spam. Just clarified the test description.

(Individual) CLA is filled out as Patrick Mylund Nielsen <go@patrickmylund.com>.

[pmylund, 2012-02-06 05:22:49]

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

Please take another look.

[bradfitz, 2012-02-06 05:48:27]

On Sun, Feb 5, 2012 at 9:16 PM, <dsymonds@golang.org> wrote:

> LGTM
>
> but leaving for Brad/Russ to sanity check and submit.


People could view a HEAD as a way to save bandwidth more than server-side
CPU resources, which is pretty fair.  (if it's just serving a file or
something already in memory, it's pretty fast to write it to a blackholed
ResponseWriter)

In that case, if a handler _does_ write in response to a HEAD request, and
they don't set a header, why ignore it?  I'd still sniff, to be consistent
with GET.

I only think we should omit the Content-Type header (the dummy "text/plain"
one) if there's no body written at all in a HEAD handler.

[dsymonds, 2012-02-06 05:49:35]

On Mon, Feb 6, 2012 at 4:48 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> In that case, if a handler _does_ write in response to a HEAD request, and
> they don't set a header, why ignore it?

If they do that, we currently flag that as an error, don't we?

[bradfitz, 2012-02-06 05:55:19]

On Sun, Feb 5, 2012 at 9:49 PM, David Symonds <dsymonds@golang.org> wrote:

> On Mon, Feb 6, 2012 at 4:48 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
>
> > In that case, if a handler _does_ write in response to a HEAD request,
> and
> > they don't set a header, why ignore it?
>
> If they do that, we currently flag that as an error, don't we?
>

Oh, right, we do now.  Okay, then this is fine.  Feel free to submit when
you're happy.

[pmylund, 2012-02-06 06:02:06]

On 2012/02/06 05:55:19, bradfitz wrote:
> On Sun, Feb 5, 2012 at 9:49 PM, David Symonds <mailto:dsymonds@golang.org>
wrote:
> 
> > On Mon, Feb 6, 2012 at 4:48 PM, Brad Fitzpatrick
<mailto:bradfitz@golang.org>
> > wrote:
> >
> > > In that case, if a handler _does_ write in response to a HEAD request,
> > and
> > > they don't set a header, why ignore it?
> >
> > If they do that, we currently flag that as an error, don't we?
> >
> 
> Oh, right, we do now.  Okay, then this is fine.  Feel free to submit when
> you're happy.

http://code.google.com/p/go/issues/detail?id=2886

[dsymonds, 2012-02-06 06:55:53]

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

net/http: Don't set Content-Type header for HEAD requests by default
since the real type is not inferred.
Fixes issue 2885.

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

Committer: David Symonds <dsymonds@golang.org>