-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
lib/osutil: IsDir returns true if path does not exist (fixes #3839) #3883
Conversation
Note that this is a bugfix for the known cases when we are synchronizing a file in a directory that does not exist:
|
Should rename it if we do this check as the name is otherwise misleading. |
Yeah I had like 3 minutes before I had to leave the house. Probably should be TraversesSymlink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise.
// base. | ||
func IsDir(base, name string) bool { | ||
path := base | ||
info, err := Lstat(path) | ||
if err != nil { | ||
return false | ||
return os.IsNotExist(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should change this return. If the base directory doesn't exist we're in deep water, probably "folder path missing" etc. This should not be the one causing issues today.
@@ -14,13 +14,14 @@ import ( | |||
|
|||
// IsDir returns true if base and every path component of name up to and | |||
// including filepath.Join(base, name) is a directory (and not a symlink or | |||
// similar). Base and name must both be clean and name must be relative to | |||
// similar), but returns true if the path is inaccessible, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is broken. :) Maybe just ... is a directory or non-existent (and not a symlink
?
@st-review lgtm |
@calmh: Noted! Need another LGTM or explicit merge command. |
8d44756
to
fec211e
Compare
Merge this or the other implementation, whatever you prefer. |
@st-review merge it! |
👌 Merged as 1a1e35d. Thanks, @AudriusButkevicius! |
The intent of the function is to verify that we are not traversing symlinks.
If the path along any component does not exist, we are not traversing anything, and it should be fine to let the updates through (as they are most likely only metadata updates)
/cc @Unrud