From 49fb246514ccd468da904b9488c1c7dd542d9615 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Fri, 8 Feb 2019 18:53:25 -0500 Subject: [PATCH] refactor/rename: re-enable TestMoves on Windows This CL is based on the work and discussion in CL 24969 and CL 32392. It simplifies the code further and makes trybots pass on Windows. Also update the documentation and implementation of the subpackages function slightly to make it more clear what it's doing. Background CL 24943 has made progress on fixing rename.Move behavior on Windows, but hadn't re-enabled TestMoves at the time. CL 24969 attempted to do so, but the tests weren't passing and it didn't get merged at the time. Since that time, CL 65672 has fixed a bug in the subpackages function. As a result, it's possible to simplify code in Move now. The subpackages function returns a set of packages in the given srcDir whose import path equals to root or has "root/" as the prefix. After CL 65672, it correctly filters out ones that are not true sub-packages of root. That's why it's not necessary to try to append a trailing separator when doing the string replacement in Move now. Given subpackages reports their import paths (always '/'-separated), not filesystem paths, it's not necessary to use filepath.ToSlash either. Updates golang/go#16384 Change-Id: Ia808fbaaeffe44ea1e51d7cc3ed9499bc2b438cf Reviewed-on: https://go-review.googlesource.com/c/161777 Run-TryBot: Dmitri Shuralyov TryBot-Result: Gobot Gobot Reviewed-by: Michael Matloob --- refactor/rename/mvpkg.go | 24 +++++++----------------- refactor/rename/mvpkg_test.go | 4 ---- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/refactor/rename/mvpkg.go b/refactor/rename/mvpkg.go index 638d0cfc25..91c00ff14c 100644 --- a/refactor/rename/mvpkg.go +++ b/refactor/rename/mvpkg.go @@ -9,7 +9,6 @@ package rename // TODO(matloob): // - think about what happens if the package is moving across version control systems. -// - think about windows, which uses "\" as its directory separator. // - dot imports are not supported. Make sure it's clearly documented. import ( @@ -52,7 +51,6 @@ func Move(ctxt *build.Context, from, to, moveTmpl string) error { // This should be the only place in the program that constructs // file paths. - // TODO(matloob): test on Microsoft Windows. fromDir := buildutil.JoinPath(ctxt, srcDir, filepath.FromSlash(from)) toDir := buildutil.JoinPath(ctxt, srcDir, filepath.FromSlash(to)) toParent := filepath.Dir(toDir) @@ -79,12 +77,7 @@ func Move(ctxt *build.Context, from, to, moveTmpl string) error { for r := range rev[pkg] { affectedPackages[r] = true } - // Ensure directories have a trailing separator. - dest := strings.Replace(pkg, - filepath.Join(from, ""), - filepath.Join(to, ""), - 1) - destinations[pkg] = filepath.ToSlash(dest) + destinations[pkg] = strings.Replace(pkg, from, to, 1) } // Load all the affected packages. @@ -135,19 +128,17 @@ func srcDir(ctxt *build.Context, pkg string) (string, error) { } // subpackages returns the set of packages in the given srcDir whose -// import paths start with dir. -func subpackages(ctxt *build.Context, srcDir string, dir string) map[string]bool { - subs := map[string]bool{dir: true} - - // Find all packages under srcDir whose import paths start with dir. +// import path equals to root, or has "root/" as the prefix. +func subpackages(ctxt *build.Context, srcDir string, root string) map[string]bool { + var subs = make(map[string]bool) buildutil.ForEachPackage(ctxt, func(pkg string, err error) { if err != nil { log.Fatalf("unexpected error in ForEachPackage: %v", err) } - // Only process the package or a sub-package - if !(strings.HasPrefix(pkg, dir) && - (len(pkg) == len(dir) || pkg[len(dir)] == '/')) { + // Only process the package root, or a sub-package of it. + if !(strings.HasPrefix(pkg, root) && + (len(pkg) == len(root) || pkg[len(root)] == '/')) { return } @@ -164,7 +155,6 @@ func subpackages(ctxt *build.Context, srcDir string, dir string) map[string]bool subs[pkg] = true }) - return subs } diff --git a/refactor/rename/mvpkg_test.go b/refactor/rename/mvpkg_test.go index 38e8a90ee0..aca3472b78 100644 --- a/refactor/rename/mvpkg_test.go +++ b/refactor/rename/mvpkg_test.go @@ -12,7 +12,6 @@ import ( "path/filepath" "reflect" "regexp" - "runtime" "strings" "testing" @@ -122,9 +121,6 @@ var _ foo.T } func TestMoves(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("broken on Windows; see golang.org/issue/16384") - } tests := []struct { ctxt *build.Context from, to string