some saftey precautions for windows #142
Conversation
jessfraz
commented
Jan 14, 2017
jessfraz
commented
- change all "path" to "filepath"
- fix rename since syscall.Rename on windows cannot rename a directory
| @@ -19,6 +19,14 @@ func renameWithFallback(src, dest string) error { | |||
| return err | |||
| } | |||
|
|
|||
| // Windows cannot use syscall.Rename to rename a directory | |||
There was a problem hiding this comment.
OK forgive me for being obtuse about this, but I'm trying to understand the background on this limitation, and I've yet to turn up anything super clear. From what I can gather, Go relies on the MoveFile API in general; for syscall.Rename(), it calls MoveFile(), which I guess should be capable of handling dir renames.
os.Rename, otoh, also ultimately uses the MoveFile API to do its work - which means it should also be capable of dir renames? The only difference is that it uses MoveFileEx(), with the MOVEFILE_REPLACE_EXISTING flag (I believe that's maybe new as of Go 1.6?), which makes behavior generally more similar to POSIX systems than a pure MoveFile() call.
So yeah, I'm confused on this, and pointers to some explanation of the limitation here would be super appreciated.
There was a problem hiding this comment.
this is from the go 1.8 release notes:
On Unix systems, os.Rename will now return an error when used to rename a directory to an
existing empty directory. Previously it would fail when renaming to a non-empty directory but
succeed when renaming to an empty directory. This makes the behavior on Unix correspond to
that of other systems.
https://tip.golang.org/doc/go1.8 so it will fail on unix in go 1.8 as well
There was a problem hiding this comment.
Right, but that doesn't say it fails when attempting to rename a directory, period - just that it fails if attempting to rename a directory to an existing empty directory. At this point, we haven't established yet whether the target dir exists, or (if it does) whether it's empty.
|
man, how did i miss so many of these... 🤦♂️ thanks! |
|
Oh right, so...we have some failures because (HAPPY DAY!) at least some of those actually should be |
deduce.go
Outdated
| @@ -264,9 +264,9 @@ func (m gopkginDeducer) deduceSource(p string, u *url.URL) (maybeSource, error) | |||
| u.Host = "github.com" | |||
| if v[2] == "" { | |||
| elem := v[3][1:] | |||
| u.Path = path.Join("/go-"+elem, elem) | |||
| u.Path = filepath.Join("/go-"+elem, elem) | |||
There was a problem hiding this comment.
import path or URL here, not fs, so path.Join() is right
deduce.go
Outdated
| } else { | ||
| u.Path = path.Join(v[2], v[3]) | ||
| u.Path = filepath.Join(v[2], v[3]) |
deduce.go
Outdated
| @@ -716,7 +716,7 @@ func normalizeURI(p string) (u *url.URL, newpath string, err error) { | |||
| if u.Host == "" { | |||
| newpath = p | |||
| } else { | |||
| newpath = path.Join(u.Host, u.Path) | |||
| newpath = filepath.Join(u.Host, u.Path) | |||
manager_test.go
Outdated
| @@ -446,7 +445,7 @@ func TestDeduceProjectRoot(t *testing.T) { | |||
| } | |||
|
|
|||
| // Now do a subpath | |||
| sub := path.Join(in, "foo") | |||
| sub := filepath.Join(in, "foo") | |||
manager_test.go
Outdated
| @@ -459,7 +458,7 @@ func TestDeduceProjectRoot(t *testing.T) { | |||
|
|
|||
| // Now do a fully different root, but still on github | |||
| in2 := "github.com/bagel/lox" | |||
| sub2 := path.Join(in2, "cheese") | |||
| sub2 := filepath.Join(in2, "cheese") | |||
|
i was gonna say that, when |
- change all "path" to "filepath" - fix rename since syscall.Rename on windows cannot rename a directory Signed-off-by: Jess Frazelle <[email protected]>
Codecov Report@@ Coverage Diff @@
## master #142 +/- ##
=========================================
- Coverage 78.88% 77.9% -0.98%
=========================================
Files 24 24
Lines 3709 3703 -6
=========================================
- Hits 2926 2885 -41
- Misses 582 619 +37
+ Partials 201 199 -2
Continue to review full report at Codecov.
|
sdboyer
force-pushed
the
master
branch
2 times, most recently
from
6fd33b5 to
4284958
Compare
|
Don't think this applies anymore - closing. |
