path/filepath: regression in EvalSymlinks wrt ending slash

[kolyshkin]

Summary

In Go versions prior to 1.12-beta1, a call to filepath.EvalSymlinks() with an argument like "/path/to/existing/file/" results in no error and the trailing slash removed. With 1.12-beta1, it gives "file does not exist". Complete example here: https://play.golang.org/p/YmfDC5IcWMF

Whether the old behavior was deliberate or accidental, this looks like a regression.

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

$ go1.12beta1 version
go version go1.12beta1 linux/amd64

Does this issue reproduce with the latest release?

yes (beta)

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

go env Output

$ go1.12beta1 env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kir/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/kir/go"
GOPROXY=""
GORACE=""
GOROOT="/home/kir/sdk/go1.12beta1"
GOTMPDIR=""
GOTOOLDIR="/home/kir/sdk/go1.12beta1/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build989138565=/tmp/go-build -gno-record-gcc-switches"

What did you do?

	path, err := filepath.EvalSymlinks("/path/to/existing/file/")

Complete example: https://play.golang.org/p/YmfDC5IcWMF

What did you expect to see?

No error, trailing slash removed:

kir@kd:~/go/src/github.com/kolyshkin/test/go112$ go test -v .
=== RUN   TestEvalSymlinksWithEndingSlash
--- PASS: TestEvalSymlinksWithEndingSlash (0.00s)
PASS
ok  	github.com/kolyshkin/test/go112	0.001s

What did you see instead?

"file does not exist" error with no context:

kir@kd:~/go/src/github.com/kolyshkin/test/go112$ go1.12beta1 test -v .
=== RUN   TestEvalSymlinksWithEndingSlash
--- FAIL: TestEvalSymlinksWithEndingSlash (0.00s)
    filepath_test.go:23: file does not exist
FAIL
FAIL	github.com/kolyshkin/test/go112	0.001s

[kolyshkin]

Forgot to add that this was probably introduced by 7d27e87 (aka https://go-review.googlesource.com/121676).

Checked that the case of existing directory with the ending slash works fine; it's just for files so maybe not a regression but an (accidental) fix?

This should be at least described in release notes, and I guess a test case needs to be added.

[cuonglm]

@kolyshkin I think it fixed the bug accidentally but introduce new bug.

Indeed, os.LStat("/path/to/existing_file/") will return not a directory error. But in that walkSymlinks, dest was stripped from /path/to/existing_file/ to /path/to/existing_file, so os.LStat call return no error.

The later check then returns confusing error in this case.

[agnivade]

/cc @ianlancetaylor @alexbrainman

[ianlancetaylor]

While this is definitely a change, I have to say that returning an error for /path/to/file/ seems to me like a bug fix. Is this breaking any programs?

I agree that it would be better to return not-a-directory.

[gopherbot]

Change https://golang.org/cl/155597 mentions this issue: path/filepath: make walkSymlinks returns properly error

[cuonglm]

On Windows, it isn't fixed yet. Which why my tests added in CL failed on Windows. evalSymlinksUsingGetFinalPathNameByHandle just return the path name but doesn't check if it's a symlink or a directory.

I updated CL to do extra check before evalSymlinksUsingGetFinalPathNameByHandle return.

cc @ianlancetaylor @davecheney

[kolyshkin]

Is this breaking any programs?

In our case it was just a unit test case (and I hope no real production code depends on such behavior of EvalSymlinks).

I agree that it would be better to return not-a-directory.

That would actually fix the above unit test!

[kolyshkin]

I strongly support returning os.PathError rather than just syscall.ENOTDIR here. Adding context can be a lifesaver in many situations.

[gopherbot]

Change https://golang.org/cl/155997 mentions this issue: path/filepath: fix EvalSymlinks fails when target is file on Windows

[gopherbot]

Change https://golang.org/cl/156398 mentions this issue: path/filepath: return special error from EvalSymlinks