Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/http: perhaps merge IsDir checks in fs.go #33385

Closed
spacewander opened this issue Jul 31, 2019 · 9 comments
Closed

net/http: perhaps merge IsDir checks in fs.go #33385

spacewander opened this issue Jul 31, 2019 · 9 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@spacewander
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version go1.12.6 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
~ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vagrant/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/vagrant/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build301219300=/tmp/go-build -gno-record-gcc-switches"

What did you see instead?

In net/http/tranfers.go: https://github.com/golang/go/blob/master/src/net/http/transfer.go#L254

        if t.ContentLength > 0 {
		return true
	}
	if t.ContentLength < 0 {
		return false
	}
	// Many servers expect a Content-Length for these methods
	if t.Method == "POST" || t.Method == "PUT" {
		return true
	}
	if t.ContentLength == 0 && isIdentity(t.TransferEncoding) {

Since we have already checked t.ContentLength > 0 and t.ContentLength < 0, the t.ContentLength == 0 in the fourth if branch looks useless.

In net/http/fs.go: https://github.com/golang/go/blob/master/src/net/http/fs.go#L586

	if d.IsDir() {
		url := r.URL.Path
		if url[len(url)-1] != '/' {
			localRedirect(w, r, path.Base(url)+"/")
			return
		}
	}

	// use contents of index.html for directory, if present
	if d.IsDir() {
		index := strings.TrimSuffix(name, "/") + indexPage
		ff, err := fs.Open(index)
		if err == nil {
			defer ff.Close()
			dd, err := ff.Stat()
			if err == nil {
				name = index
				d = dd
				f = ff
			}
		}
	}

Maybe we could merge this two if d.IsDir() { into one.

Please correct me if I am wrong.
@andybons
Copy link
Member

In the first example, the check may seem useless but it’s explicit, which can be more clear to people when reading the code. I don’t think opening a change for that is necessary.

The second example seems more appropriate to fix since it’s checking d.IsDir() three times in a row.

@andybons andybons added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 31, 2019
@andybons andybons added this to the Unplanned milestone Jul 31, 2019
@andybons andybons changed the title net/http: two code style suggestions net/http: merge three IsDir checks into one Jul 31, 2019
@bharaththiruveedula-zz
Copy link

@andybons , can I work on this?

@spacewander
Copy link
Contributor Author

@andybons
I am not sure if the three IsDir checks can be merged into one, because the d maybe be changed in the second IsDir block. And /path/to/index.html might be a directory which can be opened successfully. Maybe we should merge them into two.

I will give some time this weekend to submit a patch if a PR is welcome.

@andybons andybons changed the title net/http: merge three IsDir checks into one net/http: perhaps merge IsDir checks in fs.go Aug 1, 2019
@andybons
Copy link
Member

andybons commented Aug 1, 2019

@bharaththiruveedula @spacewander I will leave it up to both of you to determine who will author a patch.

Keep in mind we're still in the 1.13 freeze right now so the changes won't actually be merged until the 1.14 tree opens.

spacewander added a commit to spacewander/go that referenced this issue Aug 2, 2019
@spacewander
net/http: merge IsDir checks in fs.go's serveFile function
ac54efb 
Fixes golang#33385
@spacewander spacewander mentioned this issue Aug 2, 2019
Closed
@spacewander
Copy link
Contributor Author

@andybons
Just submit a PR with a few lines.

@gopherbot
Copy link

Change https://golang.org/cl/188677 mentions this issue: net/http: merge IsDir checks in fs.go's serveFile function

@bharaththiruveedula-zz
Copy link

@spacewander please follow first come first serve and wait if no activity is there. Please don't override anyone here. Anyways it's fine. Hope we follow some rules in the community!

@andybons
Copy link
Member

andybons commented Aug 3, 2019

@bharaththiruveedula as @spacewander filed the issue, it’s fair for them to contribute the change to fix it. There are plenty of other issues to help with.

@bharaththiruveedula-zz
Copy link

bharaththiruveedula-zz commented Aug 3, 2019 •
edited

oh is it.. my mistake...apologies @spacewander

spacewander added a commit to spacewander/go that referenced this issue Sep 2, 2019
@spacewander
net/http: merge IsDir checks in fs.go's serveFile function
3bf4838 
Fixes golang#33385
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@spacewander @t4n6a1ka
net/http: merge IsDir checks in fs.go's serveFile function
736497c 
Fixes golang#33385

Change-Id: I497ccd868d408a9c5648c72aa5ce41221368daf4
GitHub-Last-Rev: 3bf4838
GitHub-Pull-Request: golang#33423
Reviewed-on: https://go-review.googlesource.com/c/go/+/188677
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Sep 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

Successfully merging a pull request may close this issue.

net/http: merge IsDir checks in fs.go's serveFile function spacewander/go
4 participants
@andybons @bharaththiruveedula-zz @spacewander @gopherbot