A side note regarding the related existing comment:

//Ensure directories have a trailing separator

from here:

https://github.com/golang/tools/blob/ed69e84b1518b5857a9f4e01d1f9cefdcc45246e/refactor/rename/mvpkg.go#L83

        // Ensure directories have a trailing separator.
        dest := strings.Replace(pkg,
            filepath.Join(from, ""),
            filepath.Join(to, ""),
            1)
        destinations[pkg] = filepath.ToSlash(dest)

Either that comment is misleading, or I don't understand that comment (either option being relatively likely ;-)

The Ensure directories have a trailing separator comment seems to imply that filepath.Join(foo, "") would add a trailing separator but I'm not sure that's true. I think that would be true for strings.Join with final empty string being joined, but I'm not sure that filepath.Join(foo, "") would add a trailing separator.

I'm not 100% sure of those statements, but here's a quick example:

https://play.golang.org/p/QkQ0Y1Pvbn

    // Sample data. Note 2 empty strings (especially note that last element is empty string)
    s := []string{"foo", "", "bar", "baz", ""}

    // Compare strings.Join vs. filepath.Join vs. path.Jon
    fmt.Println("strings.Join:  ", strings.Join(s, ","))
    fmt.Println("filepath.Join: ", filepath.Join(s...))
    fmt.Println("path.Join:     ", path.Join(s...))

    // Output:
    // strings.Join:   foo,,bar,baz,
    // filepath.Join:  foo/bar/baz
    // path.Join:      foo/bar/baz

Note that that filepath.Join behavior is a little different from what seems to be the Python and Ruby behavior for a similar filepath join with an empty string, where Python and Ruby would seem to include a final trailing separator if passed a final empty string (live Python/Ruby examples at bottom of this comment).

So, perhaps the author of this Go code incorrectly expected filepath.Join to behave either similar to how either Go behaves for strings.Join, and/or how other programming languages handle filepath join?

So perhaps the intent of that comment was something closer to the following (untested, sorry):

        // Normalize any trailing separators (via Clean) to ensure we 
        // replace the substrings in pkg consistently.
        dest := strings.Replace(pkg,
            path.Clean(from),
            path.Clean(to),
            1)

Python filepath join example:

https://repl.it/CfZD

print 'python string join (input includes 2 empty strings):'
print ','.join(['foo', '', 'bar', 'baz', ''])

print '\npython os.path join (input includes 2 empty strings):'
print os.path.join('foo', '', 'bar', 'baz', '')

# Outputs
# python string join (input includes 2 empty strings):
# foo,,bar,baz,
# 
# python os.path join (input includes 2 empty strings):
# foo/bar/baz/

Ruby filepath join example

(and sorry, I don't really know Ruby):

https://repl.it/CfZA

puts "ruby string join (input includes 2 empty strings):"
puts ["foo", "", "bar", "baz", ''].join(",")

puts "\nruby file join (input includes 2 empty strings):"
puts File.join("foo", "", "bar", "baz", "")

# Output:
# ruby string join (input includes 2 empty strings):
# foo,,bar,baz,
#
# ruby file join (input includes 2 empty strings):
# foo/bar/baz/

I also now see that #16384 recently added that filepath.ToSlash(dest) after the strings.Replace call.

I haven't gone through #16384 in detail to track down exactly why filepath.ToSlash(dest) was added there, but as I mentioned in my first comment above, that filepath.ToSlash(dest) might very well be unnecessary after the suggested possible fix from my first comment above. That said, it would likely require a review of the problem from #16384 to determine that, though.

Finally, sorry for the long report, but to more explicitly tie together my first comment and second comment above: if those guesses about original intent there are correct, the net result for all my comments would be:

diff --git a/refactor/rename/mvpkg.go b/refactor/rename/mvpkg.go
index cd416c5..26905ab 100644
--- a/refactor/rename/mvpkg.go
+++ b/refactor/rename/mvpkg.go
@@ -79,12 +79,13 @@ func Move(ctxt *build.Context, from, to, moveTmpl string) error {
                for r := range rev[pkg] {
                        affectedPackages[r] = true
                }
-               // Ensure directories have a trailing separator.
+               // Normalize any trailing separators (via Clean) to ensure we
+               // replace the substrings in pkg consistently.
                dest := strings.Replace(pkg,
-                       filepath.Join(from, ""),
-                       filepath.Join(to, ""),
+                       path.Clean(from),
+                       path.Clean(to),
                        1)
-               destinations[pkg] = filepath.ToSlash(dest)
+               destinations[pkg] = dest
        }

Thank you for problem report. I agree there is a problem in that code.

We even have test for it - TestMoves. The test is broken, but no one noticed because TestMoves is skipped on windows (by mistake). @alandonovan sent https://go-review.googlesource.com/#/c/24969 to enable the test, but then he discovered the test is broken.

I came to similar conclusion to yours. You can see my comment at the end of https://go-review.googlesource.com/#/c/24969

You can wait for Alan to get around to complete CL 24969. Or you can send alternative solution, if you like - it sounds like you have researched this area quite a bit. This https://golang.org/doc/contribute.html is how to contribute.

Alex

Ahh, I did not see that comment, but that would have saved me some time. ;-)

(Also, I am not very familiar with Gerrit, but I had thought that particular CL was merged already in what I was running, but as you say, looks like it hasn't been merged yet).

I suspect the original intent of that (confusing) filepath.Join(foo, "") was to avoid something like from in that stanza being something like a/b/ (trailing slash) but to being something like z/b (no trailing slash) and then transforming pkg from something like a/b/c to something like z/bc (lost the slash between b and c). Such as:

https://play.golang.org/p/gKPkO0JuUI

or in the opposite direction, which might end up with a double slash, or some other variation of that.

I don't know if that would happen in practice (e.g., maybe when switching levels?), but I was able to trigger a problem with a simple replace with something like a trailing slash on the command line:

%GOPATH%\bin\gomvpkg -from github.com/user/ -to github.com/new_top_level/newhello

which triggered similar error of:

gomvpkg: invalid move destination: github.com/new_top_level/newhello; package or subpackage github.com/user already exists.

But that same example on the command line with the path.Clean() seemed to work...

In any event, thanks for the comment, and I guess I'll see if there are further comments about original intent or otherwise desired approach here.

It looks like #16384 is closed in github, which is why I thought that associated CL was merged, but looking more carefully now I see that associated CL is still open.

I was curious if that seemingly incorrectly used filepath.Join(foo, "") was used elsewhere, so took a quick look last night and noticed a similar issue here in master in the method that computes subpackages:

https://github.com/golang/tools/blob/ed69e84b1518b5857a9f4e01d1f9cefdcc45246e/refactor/rename/mvpkg.go#L148

        if !strings.HasPrefix(pkg, path.Join(dir, "")) {
            return
        }

I think the expectation of the author here might have been that appending the empty string would cause a slash to be appended (similar to what might have been the incorrect expectation about the filepath.Join(from, "") earlier that had the // Ensure directories have a trailing separator comment).

In any event, it seems the logic here in is wrong, because packages with shared prefixes can incorrectly match here in this subpackages method.

For example, if you set up the following (and note the shared prefix between the first two):

github.com/user/old_something
github.com/user/old_something_else
github.com/user/new_something_else

And then try to move old_something to new_something, in theory that should work (and old_something_else should be left alone).

However, because old_something_else shares a prefix with old_something (where old_something is the user's requested 'from'), the code here in subpackages incorrectly thinks old_something_else is a sub-package of old_something, and incorrectly sets up the destinations map (that is, intending to move old_something_else when it shouldn't), and hence the code gets ready to (incorrectly) move old_something_else to new_something_else, which then collides with the pre-existing new_something_else, which reports an error

gomvpkg: invalid move destination: github.com/user/new_something; package or subpackage github.com/user/new_something_else already exists.

I think (but did not confirm) that would happen on Linux as well (that is, not specific to backslash vs. forward slash confusion on Windows).

That's one example I could see would fail and then tested it (and it did give incorrect error above), but I think in theory a different example could also incorrectly update an "innocent bystander" package's contents based on a second "innocent bystander" package sharing a prefix with a package that is being moved (though some chance other defensive sanity checks would prevent it and/or the subsequent code might otherwise "get lucky"; I didn't try a different test).

Finally, I believe this is likely already 100% clear to anyone who worked on #16384, but to summarize the current state of #16384 (as of right now and when I looked at it a couple days ago):

• Issue x/tools/refactor/rename: TestMoves is failing on Windows #16384 is marked closed in github
• Associated CL that is merged: https://go-review.googlesource.com/#/c/24943/
• Associated CL that is not yet merged: https://go-review.googlesource.com/#/c/24969/

When I had looked at it before, I had incorrectly thought #16384 was 'done' and in master. Alex above pointed out above that CL 24969 is still pending, which is the CL that re-enables the tests, so only part of the changes for #16384 are currently in master.

Like I said before, feel free to send a change. :-) If you can add new broken test (that gets fixed by your changes), no-one will be able to object.

Alex