code review 5541059: net/http: export ServeFilesystemFile for serving files ...

net/http: export ServeFilesystemFile for serving files from custom FileSystems.
Fixes issue 2039.

[mcroydon, 2012-01-14 21:55:41]

Hello golang-dev@googlegroups.com,

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

[adg, 2012-01-15 08:51:18]

http://codereview.appspot.com/5541059/diff/4001/src/pkg/net/http/fs.go
File src/pkg/net/http/fs.go (right):

http://codereview.appspot.com/5541059/diff/4001/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:113: // ServeFilesystemFile replies to a request with the
contents of the named file or directory using the
how can it response with a directory? Trying to use ServeFile with a directory
should be an error, right?

http://codereview.appspot.com/5541059/diff/4001/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:115: // redirect behavior.  Like ServeFile, name is
separated with '/' and not filepath.Separator.
The comment doesn't explain what the "redirect behavior" is that's being
overridden.

[adg, 2012-01-15 22:22:30]

I was thinking about this today and I think this might be a better approach:

http://codereview.appspot.com/5541063

Interested to hear what Brad has to say.

[mcroydon, 2012-01-16 03:12:41]

On 2012/01/15 22:22:30, adg wrote:
> I was thinking about this today and I think this might be a better approach:
> 
> http://codereview.appspot.com/5541063
> 
> Interested to hear what Brad has to say.

I can't quantify it, but I definitely like passing a File to ServeFileSystemFile
instead of a FileSystem as long as it fits Brad's use case.  I'm happy to clean
up this CL if it's the right direction but I'll await further feedback.

[rsc, 2012-01-16 20:18:05]

Better name please.

[bradfitz, 2012-01-16 21:02:20]

Last I recall, we couldn't think of a better name, which is why this has
been an open bug for months.

On Mon, Jan 16, 2012 at 12:18 PM, Russ Cox <rsc@golang.org> wrote:

> Better name please.
>

[mcroydon, 2012-01-16 21:27:29]

ServeLocalFile perhaps?

On 2012/01/16 21:02:20, bradfitz wrote:
> Last I recall, we couldn't think of a better name, which is why this has
> been an open bug for months.
> 
> On Mon, Jan 16, 2012 at 12:18 PM, Russ Cox <mailto:rsc@golang.org> wrote:
> 
> > Better name please.
> >

[adg, 2012-01-16 22:58:49]

On 17 January 2012 08:02, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Last I recall, we couldn't think of a better name, which is why this has
> been an open bug for months.

I would prefer to make ServeFile the one that serves an http.File, and
have a separate helper ServeOSFile (or whatever) to serve files from
disk.

I also think the fancy redirect behavior should be confined to the
FileServer implementation. It annoys me that ServeFile is so complex
and opaque. My CL goes some way to simplifying it.

Is that too much churn?

Andrew

[bradfitz, 2012-01-16 23:00:22]

I like that plan.

Gofix without type info might be hard, though.
 On Jan 16, 2012 2:58 PM, "Andrew Gerrand" <adg@golang.org> wrote:

> On 17 January 2012 08:02, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> > Last I recall, we couldn't think of a better name, which is why this has
> > been an open bug for months.
>
> I would prefer to make ServeFile the one that serves an http.File, and
> have a separate helper ServeOSFile (or whatever) to serve files from
> disk.
>
> I also think the fancy redirect behavior should be confined to the
> FileServer implementation. It annoys me that ServeFile is so complex
> and opaque. My CL goes some way to simplifying it.
>
> Is that too much churn?
>
> Andrew
>

[adg, 2012-01-16 23:02:43]

On 17 January 2012 10:00, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I like that plan.
>
> Gofix without type info might be hard, though.

We might have to forgo gofix in this case, because the behavior of
ServeFile will change. Some people will want to switch to using a
FileServer instead, while others will want to switch to ServeOSFile.

> On Jan 16, 2012 2:58 PM, "Andrew Gerrand" <adg@golang.org> wrote:
>>
>> On 17 January 2012 08:02, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>> > Last I recall, we couldn't think of a better name, which is why this has
>> > been an open bug for months.
>>
>> I would prefer to make ServeFile the one that serves an http.File, and
>> have a separate helper ServeOSFile (or whatever) to serve files from
>> disk.
>>
>> I also think the fancy redirect behavior should be confined to the
>> FileServer implementation. It annoys me that ServeFile is so complex
>> and opaque. My CL goes some way to simplifying it.
>>
>> Is that too much churn?
>>
>> Andrew

[rsc, 2012-01-16 23:10:27]

So the function no one will use is called ServeFile
and the function everyone already uses gets renamed
to ServeOSFile?  That doesn't seem like a win either.

As Brad points out, the reason this is still open is that
we don't have a good API.  Without a good API, I am
happy to close this WorkingAsIntended.

Russ

[adg, 2012-01-16 23:17:49]

On 17 January 2012 10:10, Russ Cox <rsc@golang.org> wrote:
> So the function no one will use is called ServeFile
> and the function everyone already uses gets renamed
> to ServeOSFile?  That doesn't seem like a win either.
>
> As Brad points out, the reason this is still open is that
> we don't have a good API.  Without a good API, I am
> happy to close this WorkingAsIntended.

This is a good point, but we're just hung up on naming, now, right?

I feel strongly that we should simplify ServeFile regardless of
whether it is renamed, and push those who need the redirect stuff
toward FileServer.

Possible names for the new function that serves an http.File:
  ServeFSFile
  ServeOpenFile (the provided File is already open)
  ServeHTTPFile (it's an http.File?)
  ServeAnyFile

[adg, 2012-01-16 23:19:19]

Alternately, if we're going to rename ServeFile and you hate
ServeOSFile, we could go with ServeLocalFile ?

[rsc, 2012-01-17 01:44:32]

Right now, you can already serve index.html by
creating a FileSystem implementation that serves
it under some other name (say index_html) and
rewriting the request to use that name.

As I have said from the beginning, this is helper
code.  It doesn't have to do everything.  Especially
if trying to do everything makes the API worse.

Russ