code review 4654089: http: fix Content-Type of file extension.

http: fix Content-Type of file extension.
ServeFile() pass empty string to serveFile(). serveFile() should get
file extension via joining root and filename.

[mattn, 2011-07-05 12:46:30]

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

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

[rsc, 2011-07-12 00:06:06]

The right fix probably in ServeFile:

dir, file := filepath.Split(name)
serveFile(w, r, Dir(dir), file, false)

[bradfitz, 2011-07-12 00:08:33]

what is testdata/style.css for?  Is there a missing test file in this CL?


On Tue, Jul 5, 2011 at 5:46 AM, <mattn.jp@gmail.com> wrote:

> Reviewers: golang-dev_googlecode.com, bradfitz,
>
> Message:
> Hello golang-dev@googlecode.com, bradfitz@golang.org (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> http://go.googlecode.com/hg/
>
>
> Description:
> http: fix Content-Type of file extension.
> ServeFile() pass empty string to serveFile(). serveFile() should get
> file extension via joining root and filename.
>
> Please review this at
http://codereview.appspot.com/**4654089/<http://codereview.appspot.com/4654089/>
>
> Affected files:
>  M src/pkg/http/fs.go
>  A src/pkg/http/testdata/style.**css
>
>
> Index: src/pkg/http/fs.go
> ==============================**==============================**=======
> --- a/src/pkg/http/fs.go
> +++ b/src/pkg/http/fs.go
> @@ -170,7 +170,8 @@
>
>        // If Content-Type isn't set, use the file's extension to find it.
>        if w.Header().Get("Content-Type") == "" {
> -               ctype := mime.TypeByExtension(filepath.**Ext(name))
> +               fn := filepath.Join(string(fs.(Dir))**,
> filepath.FromSlash(path.Clean(**"/"+name)))
> +               ctype := mime.TypeByExtension(filepath.**Ext(fn))
>                if ctype == "" {
>                        // read a chunk to decide between utf-8 text and
> binary
>                        var buf [1024]byte
> Index: src/pkg/http/testdata/style.**css
> ==============================**==============================**=======
> new file mode 100644
> --- /dev/null
> +++ b/src/pkg/http/testdata/style.**css
> @@ -0,0 +1,1 @@
> +body {}
>
>
>

[mattn, 2011-07-12 00:23:45]

@rsc done

@bradfiz

Please see: http://codereview.appspot.com/4654089/patch/7005/10002
I guess that this is better to be this test case that serve content-type
normaly.

On 2011/07/12 00:08:33, bradfitz wrote:
> what is testdata/style.css for?  Is there a missing test file in this CL?
> 
> 
> On Tue, Jul 5, 2011 at 5:46 AM, <mailto:mattn.jp@gmail.com> wrote:
> 
> > Reviewers: http://golang-dev_googlecode.com, bradfitz,
> >
> > Message:
> > Hello mailto:golang-dev@googlecode.com, mailto:bradfitz@golang.org (cc:
> > mailto:golang-dev@googlegroups.com),
> >
> > I'd like you to review this change to
> > http://go.googlecode.com/hg/
> >
> >
> > Description:
> > http: fix Content-Type of file extension.
> > ServeFile() pass empty string to serveFile(). serveFile() should get
> > file extension via joining root and filename.
> >
> > Please review this at
>
http://codereview.appspot.com/**4654089/%3Chttp://codereview.appspot.com/4654...>
> >
> > Affected files:
> >  M src/pkg/http/fs.go
> >  A src/pkg/http/testdata/style.**css
> >
> >
> > Index: src/pkg/http/fs.go
> > ==============================**==============================**=======
> > --- a/src/pkg/http/fs.go
> > +++ b/src/pkg/http/fs.go
> > @@ -170,7 +170,8 @@
> >
> >        // If Content-Type isn't set, use the file's extension to find it.
> >        if w.Header().Get("Content-Type") == "" {
> > -               ctype := mime.TypeByExtension(filepath.**Ext(name))
> > +               fn := filepath.Join(string(fs.(Dir))**,
> > filepath.FromSlash(path.Clean(**"/"+name)))
> > +               ctype := mime.TypeByExtension(filepath.**Ext(fn))
> >                if ctype == "" {
> >                        // read a chunk to decide between utf-8 text and
> > binary
> >                        var buf [1024]byte
> > Index: src/pkg/http/testdata/style.**css
> > ==============================**==============================**=======
> > new file mode 100644
> > --- /dev/null
> > +++ b/src/pkg/http/testdata/style.**css
> > @@ -0,0 +1,1 @@
> > +body {}
> >
> >
> >

[bradfitz, 2011-07-12 00:26:20]

http://codereview.appspot.com/4654089/diff/7005/src/pkg/http/fs_test.go
File src/pkg/http/fs_test.go (right):

http://codereview.appspot.com/4654089/diff/7005/src/pkg/http/fs_test.go#newco...
src/pkg/http/fs_test.go:189: t.Errorf("Content-Type mismatch: got %d, want %d",
h, want)
%q, not %d, in both places

[mattn, 2011-07-12 00:28:27]

http://codereview.appspot.com/4654089/diff/7005/src/pkg/http/fs_test.go
File src/pkg/http/fs_test.go (right):

http://codereview.appspot.com/4654089/diff/7005/src/pkg/http/fs_test.go#newco...
src/pkg/http/fs_test.go:189: t.Errorf("Content-Type mismatch: got %d, want %d",
h, want)
On 2011/07/12 00:26:20, bradfitz wrote:
> %q, not %d, in both places

Done.

Good catch.

[rsc, 2011-07-13 21:39:25]

LGTM

[rsc, 2011-07-13 21:39:37]

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

http: fix Content-Type of file extension.
ServeFile() pass empty string to serveFile(). serveFile() should get
file extension via joining root and filename.

R=bradfitz, rsc
CC=golang-dev
http://codereview.appspot.com/4654089

Committer: Russ Cox <rsc@golang.org>