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

embed: document that paths must satisfy fs.ValidPath(name) #44012

Closed
gazerro opened this issue Jan 30, 2021 · 6 comments
Closed

embed: document that paths must satisfy fs.ValidPath(name) #44012

gazerro opened this issue Jan 30, 2021 · 6 comments
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@gazerro
Copy link
Contributor

gazerro commented Jan 30, 2021

The embed documentation does not says that paths must also satisfy fs.ValidPath(name). It only says

The path separator is a forward slash, even on Windows systems.

and

Patterns must not contain ‘.’ or ‘..’ path elements nor begin with a leading slash.

For example in this case:

//go:embed data/

the compiler fails with the error:

pattern data/: invalid pattern syntax

but "data/" is valid as pattern, it is not valid for the fs.ValidPath function.

I think that the sentence

A //go:embed directive above a variable declaration specifies which files to embed, using one or more path.Match patterns.

should mention fs.ValidPath and consequently "The path separator is a forward slash, even on Windows systems" and "Patterns must not contain ‘.’ or ‘..’ path elements nor begin with a leading slash" can be removed.

@ghost
Copy link

ghost commented Jan 30, 2021

I think that fs.ValidPath should accept data/. It's surprising it doesn't.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 2, 2021
@bcmills bcmills added this to the Go1.16 milestone Feb 2, 2021
@bcmills
Copy link
Contributor

bcmills commented Feb 2, 2021

CC @rsc @jayconrod @matloob

@bcmills
Copy link
Contributor

bcmills commented Feb 2, 2021

Note that the documentation for fs.ValidPath doesn't currently mention the need for the path to be clean either:

ValidPath reports whether the given path name is valid for use in a call to Open. Path names passed to open are unrooted, slash-separated sequences of path elements, like “x/y/z”. Path names must not contain a “.” or “..” or empty element, except for the special case that the root directory is named “.”.

Paths are slash-separated on all systems, even Windows. Backslashes must not appear in path names.

@gazerro
Copy link
Contributor Author

gazerro commented Feb 2, 2021

The documentation for fs.ValidPath says that a path name is a "slash-separated sequences of path elements". So if it is assumed that a path element cannot be an empty string, the fs.ValidPath documentation does not allow "x/y/z/".

I think there is no need to mention that the path needs to be clean, because the documentation specifies exactly how a path should be.

@jayconrod jayconrod self-assigned this Feb 2, 2021
@gopherbot
Copy link

Change https://golang.org/cl/290071 mentions this issue: embed, io/fs: clarify that leading and trailing slashes are disallowed

@jayconrod
Copy link
Contributor

I've updated the docs to explain the restrictions on valid patterns. They're the same as io/fs.ValidPath, but the documentation repeats those restrictions rather than linking to ValidPath.

Also clarified that leading and trailing slashes are disallowed in ValidPath, though as @gazerro pointed out, that was implied by the rule against empty path elements. This simplifies logic for FS implementations: there's only one valid way to spell each path.

@golang golang locked and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants