Feature/implement windows parser and on drive letter #206
Conversation
stagnation
force-pushed
the
feature/implement-windows-parser-and-on-drive-letter
branch
4 times, most recently
from
4df2fb7 to
21eea46
Compare
stagnation
force-pushed
the
feature/implement-windows-parser-and-on-drive-letter
branch
from
21eea46 to
5ab271a
Compare
stagnation
force-pushed
the
feature/implement-windows-parser-and-on-drive-letter
branch
from
5ab271a to
af92c74
Compare
To give better error messages for which file or directory operation fails.
stagnation
force-pushed
the
feature/implement-windows-parser-and-on-drive-letter
branch
from
af92c74 to
fedee3e
Compare
stagnation
force-pushed
the
feature/implement-windows-parser-and-on-drive-letter
branch
3 times, most recently
from
829d800 to
2806e8b
Compare
stagnation
force-pushed
the
feature/implement-windows-parser-and-on-drive-letter
branch
from
2806e8b to
b60a04d
Compare
stagnation
force-pushed
the
feature/implement-windows-parser-and-on-drive-letter
branch
2 times, most recently
from
daac6d8 to
144774b
Compare
stagnation
force-pushed
the
feature/implement-windows-parser-and-on-drive-letter
branch
from
144774b to
e7cd08c
Compare
| "github.com/stretchr/testify/require" | ||
| "google.golang.org/grpc/codes" |
There was a problem hiding this comment.
Nit: Add empty line between github.com and golang.org imports, for consistency with the rest of the code base.
|
|
||
| func validateWindowsPathComponent(component string) error { | ||
| if strings.ContainsFunc(component, unicode.IsControl) || strings.ContainsAny(component, "<>:\"/\\|?*") { | ||
| return status.Errorf(codes.InvalidArgument, "Path component contains a control character or invalid character %#v.", component) |
There was a problem hiding this comment.
Please remove the trailing dot. Also move the %#v to the right place in the sentence.
return status.Errorf(codes.InvalidArgument, "Path component %#v contains a control character or invalid character %#v", component)That said, the general pattern in the Buildbarn codebase is to never format errors with values that also happen to be arguments of the function. Probably better to do this?
if strings.ContainsFunc(component, unicode.IsControl) {
return status.Error(codes.InvalidArgument, "Path component contains control characters")
}
if strings.ContainsAny(component, "<>:\"/\\|?*") {
return status.Error(codes.InvalidArgument, "Path component contains reserved characters")
}And then let the caller do a util.StatusWrapf(err, "Invalid path component %#v", component.String()).
There was a problem hiding this comment.
Note that there are also some additional restrictions on Windows, such as filenames not being allowed to end with dots or whitespace. And there are obviously reserved names such as LPT1. But whatever. I think we should only add checks for that if it ever becomes problematic. This should do.
There was a problem hiding this comment.
Good point on the former! I did not make that connection, just thought about the error myopically. That is a great rule.
Argh windows... "But wait, there is more!"
| } | ||
| return GotDirectory{Child: parent}, remainder, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we add a call to validateWindowsPathComponent() here? This ensures that we also don't parse malformed paths.
There was a problem hiding this comment.
Adding such a check would also allow you to get rid of
if strings.ContainsRune(path, '\x00') {
return nil, status.Error(codes.InvalidArgument, "Path contains a null byte")
}Because validateWindowsPathComponent() also does that for you.
There was a problem hiding this comment.
Thinking slightly ahead: if the strings.ContainsRune(path, '\x00') check is removed from NewWindowsParser(), then it can no longer return any errors. That also removes the need for having MustNewWindowsParser().
| if err != nil { | ||
| return err | ||
| return util.StatusWrapf(err, "Failed to stat component %#v", childPath) |
There was a problem hiding this comment.
s/component//, because it's not actually a path component but a full path? Also add .GetUNIXString(). Same with the other calls below.
|
Hey @stagnation , I just processed the remaining comments myself and merged this change. Thanks a lot for working on this! |
No description provided.