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

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

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

Comments

@spacewander
Copy link

commented Jul 31, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

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 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

This comment has been minimized.

Copy link

commented Jul 31, 2019

@andybons , can I work on this?

@spacewander

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

commented Aug 2, 2019

@andybons
Just submit a PR with a few lines.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 2, 2019

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

@bharaththiruveedula

This comment has been minimized.

Copy link

commented Aug 3, 2019

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Aug 3, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.