code review 6300081: net/http: use index.html modtime (not directory) for If...

net/http: use index.html modtime (not directory) for If-Modified-Since

Thanks to Håvid Falch for finding the problem.

Fixes issue 3414

[bradfitz, 2012-06-11 15:45:36]

Hello golang-dev@googlegroups.com,

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

[r, 2012-06-11 15:52:01]

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

http://codereview.appspot.com/6300081/diff/5001/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:260: if checkLastModified(w, r, d.ModTime()) {
aren't you still checking the modtime if it's a dir?

[bradfitz, 2012-06-11 16:01:09]

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

http://codereview.appspot.com/6300081/diff/5001/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:260: if checkLastModified(w, r, d.ModTime()) {
On 2012/06/11 15:52:01, r wrote:
> aren't you still checking the modtime if it's a dir?

Yes, but only if it's a directory with no index.html file.  The index.html check
is in the if d.IsDir() block above, which then changes the d variable to be "dd"
(not the best name).

The new If-Modified-Since check is happening within serveContent, not here.

[r, 2012-06-11 16:07:11]

the test seems overbuilt

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

http://codereview.appspot.com/6300081/diff/5001/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:260: if checkLastModified(w, r, d.ModTime()) {
in that case, i don't understand this code at all. where is the check in the if
above? it looks deleted to me

[rsc, 2012-06-13 19:32:17]

Code looks fine, but a comment would be nice in the index.html case saying

// serveContent will check modification time

I agree that the test seems like overkill. Can you use the local file system
instead?

[bradfitz, 2012-06-13 19:38:06]

On Wed, Jun 13, 2012 at 12:32 PM, <rsc@golang.org> wrote:

> Code looks fine, but a comment would be nice in the index.html case
> saying
>
> // serveContent will check modification time
>
> I agree that the test seems like overkill. Can you use the local file
> system instead?


I thought about it but had fear of Windows and/or time.Sleep calls.  I
totally forgot we had a portable os.Chtimes.

I suppose I could make a temp directory instead and write in that. I like
to avoid touching the filesystem in general, though. *shrug*

[bradfitz, 2012-06-13 21:53:10]

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

net/http: use index.html modtime (not directory) for If-Modified-Since

Thanks to Håvid Falch for finding the problem.

Fixes issue 3414

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/6300081