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

vfsutil: ReadDir compatibility issue with statik/fs #8

Closed
shakahl opened this issue May 26, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@shakahl
Copy link

commented May 26, 2019

Hi Everybody!

I noticed that the vfsutil.ReadDir used by vfstemplate.ParseGlob is incompatible with https://github.com/rakyll/statik binary file embedding tool.

The problem

// ReadDir reads the contents of the directory associated with file and
// returns a slice of FileInfo values in directory order.
func ReadDir(fs http.FileSystem, name string) ([]os.FileInfo, error) {
	f, err := fs.Open(name)
	if err != nil {
		return nil, err
	}
	defer f.Close()
	return f.Readdir(0)
}

Notice the f.Readdir(0). Some other libraries are ignoring the 0 count like rakyll/statik.

Although the file interface allows 0 for specifying the count but it would be better to specify -1 here to be more compatible with other tools.

Relevant documentation can be found here: https://golang.org/pkg/os/#File.Readdir

Summary

f.Readdir(0) should f.Readdir(-1).

The problem occured in the project called statik. Check out their implementation: https://github.com/rakyll/statik/blob/3bac566d30cdbeddef402a80f3d6305860e59f12/fs/fs.go#L190

I will make a pullrequest for them too.

@dmitshur

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Hi @shakahl,

Thanks for the issue report. As far as I can tell, calling the Readdir method with 0 or any negative value should have the same effect. As you pointed out, that's what its documentation suggests:

If n <= 0, Readdir returns all the FileInfo from the directory in a single slice.

Given that, if the current rakyll/statik implementation doesn't treat 0 and -1 the same, I think it would make more sense to change it so that it does. What do you think?

/cc @rakyll

@dmitshur dmitshur changed the title vfsutil.ReadDir compatibility issue with statik/fs vfsutil: ReadDir compatibility issue with statik/fs May 27, 2019

@shakahl

This comment has been minimized.

Copy link
Author

commented May 27, 2019

I agree. My PR won't break any existing stuff just makes more compatible with another good library. Anyway I will make a pull request for rakyll/statik as well. I think the 0 can be potentially mistrteated in another libs too. My assumption based on that rakyll/statik is used by many devs and the issue appeared for me now, maybe first.

rakyll added a commit to rakyll/statik that referenced this issue Jul 9, 2019

fs: handle Readdir(0) same as Readdir(< 0) (#69)
Implementations of the File.Readdir method are expected to treat
all n <= 0 values the same, returning all the os.FileInfo structures
from the directory in a single slice. This is documented at
https://godoc.org/net/http#File.Readdir and https://godoc.org/os#File.Readdir.

Previously, the n == 0 value was not being handled correctly.
This change fixes it by making it work the same as all other
negative values of n.

Add a test case for it.

Also simplify string(out.Bytes()) to just out.String() in a test.

Fixes #68.
Fixes shurcooL/httpfs#8.
Closes shurcooL/httpfs#9.
@dmitshur

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Fixed via rakyll/statik#69.

@dmitshur dmitshur closed this Jul 9, 2019

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