cmd/gomovepkg: a package moving tool.

cmd/gomovepkg: a package moving tool.

Currently very rough. It has basic functionality for moving a directory and renaming imports to the directory's subpackages.

[adonovan, 2014-10-21 21:26:45]

Looking good.  

When you say it "doesn't do anything yet", what do you mean?  It looks like it
does most of the work already.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go
File cmd/gomovepkg/main.go (right):

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:6: // to the package to point to the new location.
TODO: think about windows, which uses \ as its directory separator.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:52: // want to check whether dir exists
In this case, ctxt.Import should suffice.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:54: _, err := os.Stat(path)
Ideally you shouldn't use os.Stat on a file path from the build context, which
is (potentially) a virtualized file system.  (Of course, this tool is really
meant to run on a physical file system, but if you're careful, this discipline
will make writing tests easier---like gorename.)

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:62: func srcDir(ctxt *build.Context, pkg string) (string,
error) {
This code here and above could perhaps be made common.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:77: //	does not already exist.
"file or directory represented..."
(Admittedly it would be weird if it was a file, but not impossible.

Your actual moveValid logic looks good, it's just the comment that's
inconsistent.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:79: return fmt.Errorf("invalid move destination: %s", to)
The error message should include the name of the conflicting file or directory
(which could be beneath GOROOT, GOPATH[0], ...).

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:82: // Should also do a check that 'from' doesn't have any
subdirectories
Always use TODO(matloob) for such comments so they're easy to find later.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:110: }
return?

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:115: affectedPackages = map[string]bool{}
affectedPackages = map[string]bool{from: true}

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:117: importingPackages = map[string]bool{}
I prefer make(T) over T{} when T is a map.

(Especially when it's a named typed, as you can't immediately tell that it's not
a struct type.)

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:117: importingPackages = map[string]bool{}
You could combine these two maps and just remember later to ignore 'from'.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:125: // Re-load the larger program.
Here it's not "Re-"load.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:162: // TODO(matloob): the import path is quoted. do
unquoting properly
Delete "TODO" since you've already done it. :)

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:172: newPath := strings.Replace(impPath, from, to, 1)
If I rename "foo" to "bar", will this spuriously rename "foof" to "barf"?

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:185: }
// TODO: think about dot imports.

No rush---dot imports are evil---but do document that they're not yet supported.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:202: // (TODO) - add 'to' to version control
You should invoke some external script to do the move (as one operation, not
two).

The Go-ish way to allow clients to specify the the version control command is to
use the text/template mechanism, like "go list -f" does:
 e.g. go list -f '{{.ImportName}}{{.Imports}}' fmt
Type go help list for details.

Ideally we would even recognize common cases (e.g. github and have a template
already made for that case.)  TODO?

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:235: pkgs := map[string]bool{}
pkgs := map[string]bool{dir: true}

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:239: filepath.Walk(dirPath, func(path string, fi
os.FileInfo, err error) error {
This should ideally not use the local file system, but should go through the
ctxt virtualization.  The call to importgraph.Build already did the tricky and
costly I/O, so you should be able to express this as a simple textual filter
over the keys (package names) of the importgraph.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:263: pkgs[rel] = true
You probably want to make a mapping from oldname to newname.

[matloob, 2014-10-23 19:13:43]

The "doesn'

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go
File cmd/gomovepkg/main.go (right):

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:6: // to the package to point to the new location.
On 2014/10/21 21:26:44, adonovan wrote:
> TODO: think about windows, which uses \ as its directory separator.

Added TODO

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:52: // want to check whether dir exists
On 2014/10/21 21:26:43, adonovan wrote:
> In this case, ctxt.Import should suffice.

What does import return if the directory exists but doesn't contain a package?

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:54: _, err := os.Stat(path)
On 2014/10/21 21:26:44, adonovan wrote:
> Ideally you shouldn't use os.Stat on a file path from the build context, which
> is (potentially) a virtualized file system.  (Of course, this tool is really
> meant to run on a physical file system, but if you're careful, this discipline
> will make writing tests easier---like gorename.)

Ok. Replacing this with buildutil.IsDir.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:62: func srcDir(ctxt *build.Context, pkg string) (string,
error) {
On 2014/10/21 21:26:44, adonovan wrote:
> This code here and above could perhaps be made common.

added TODO

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:77: //	does not already exist.
On 2014/10/21 21:26:44, adonovan wrote:
> "file or directory represented..."
> (Admittedly it would be weird if it was a file, but not impossible.
> 
> Your actual moveValid logic looks good, it's just the comment that's
> inconsistent.

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:79: return fmt.Errorf("invalid move destination: %s", to)
On 2014/10/21 21:26:44, adonovan wrote:
> The error message should include the name of the conflicting file or directory
> (which could be beneath GOROOT, GOPATH[0], ...).

Ok. returning error from moveValid

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:82: // Should also do a check that 'from' doesn't have any
subdirectories
On 2014/10/21 21:26:43, adonovan wrote:
> Always use TODO(matloob) for such comments so they're easy to find later.

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:110: }
On 2014/10/21 21:26:43, adonovan wrote:
> return?

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:115: affectedPackages = map[string]bool{}
On 2014/10/21 21:26:44, adonovan wrote:
> affectedPackages = map[string]bool{from: true}

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:117: importingPackages = map[string]bool{}
On 2014/10/21 21:26:44, adonovan wrote:
> You could combine these two maps and just remember later to ignore 'from'.

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:117: importingPackages = map[string]bool{}
On 2014/10/21 21:26:44, adonovan wrote:
> I prefer make(T) over T{} when T is a map.
> 
> (Especially when it's a named typed, as you can't immediately tell that it's
not
> a struct type.)

Makes sense. I've wondered when to choose one over the other.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:125: // Re-load the larger program.
On 2014/10/21 21:26:43, adonovan wrote:
> Here it's not "Re-"load.

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:162: // TODO(matloob): the import path is quoted. do
unquoting properly
On 2014/10/21 21:26:44, adonovan wrote:
> Delete "TODO" since you've already done it. :)

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:172: newPath := strings.Replace(impPath, from, to, 1)
On 2014/10/21 21:26:43, adonovan wrote:
> If I rename "foo" to "bar", will this spuriously rename "foof" to "barf"?

Yes

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:185: }
On 2014/10/21 21:26:43, adonovan wrote:
> // TODO: think about dot imports.
> 
> No rush---dot imports are evil---but do document that they're not yet
supported.

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:202: // (TODO) - add 'to' to version control
On 2014/10/21 21:26:43, adonovan wrote:
> You should invoke some external script to do the move (as one operation, not
> two).
> 
> The Go-ish way to allow clients to specify the the version control command is
to
> use the text/template mechanism, like "go list -f" does:
>  e.g. go list -f '{{.ImportName}}{{.Imports}}' fmt
> Type go help list for details.
> 
> Ideally we would even recognize common cases (e.g. github and have a template
> already made for that case.)  TODO? 

Added a TODO with these notes.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:235: pkgs := map[string]bool{}
On 2014/10/21 21:26:44, adonovan wrote:
> pkgs := map[string]bool{dir: true}

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:239: filepath.Walk(dirPath, func(path string, fi
os.FileInfo, err error) error {
On 2014/10/21 21:26:44, adonovan wrote:
> This should ideally not use the local file system, but should go through the
> ctxt virtualization.  The call to importgraph.Build already did the tricky and
> costly I/O, so you should be able to express this as a simple textual filter
> over the keys (package names) of the importgraph.

Done.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:263: pkgs[rel] = true
On 2014/10/21 21:26:44, adonovan wrote:
> You probably want to make a mapping from oldname to newname.

Where would the mapping be used?

[matloob, 2014-10-23 19:14:33]

I had just forgotten to update the change description.

Responded to some comments, added TODOs for the others.

[adonovan, 2014-10-23 19:22:20]

Let me know if/when you want me to take another look over it.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go
File cmd/gomovepkg/main.go (right):

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:52: // want to check whether dir exists
On 2014/10/23 19:13:42, matloob wrote:
> On 2014/10/21 21:26:43, adonovan wrote:
> > In this case, ctxt.Import should suffice.
> 
> What does import return if the directory exists but doesn't contain a package?

It returns a specific kind of error, *build.NoGoError.

https://codereview.appspot.com/153300043/diff/40001/cmd/gomovepkg/main.go#new...
cmd/gomovepkg/main.go:263: pkgs[rel] = true
On 2014/10/23 19:13:43, matloob wrote:
> On 2014/10/21 21:26:44, adonovan wrote:
> > You probably want to make a mapping from oldname to newname.
> 
> Where would the mapping be used?

if fromSubpackages[impPath] {
  newPath := strings.Replace(impPath, from, to, 1)
  ...

would become:

if newPath, ok := subpackages[impPath]; ok {

[adonovan, 2014-12-03 20:52:52]

https://codereview.appspot.com/153300043/diff/100001/cmd/gomovepkg/main.go
File cmd/gomovepkg/main.go (right):

https://codereview.appspot.com/153300043/diff/100001/cmd/gomovepkg/main.go#ne...
cmd/gomovepkg/main.go:13: fromFlag = flag.String("from", "", "directory to be
moved")
"Import path of package to be moved"

https://codereview.appspot.com/153300043/diff/100001/cmd/gomovepkg/main.go#ne...
cmd/gomovepkg/main.go:14: toFlag   = flag.String("to", "", "new location for
directory")
"Destination import path for package"

https://codereview.appspot.com/153300043/diff/100001/cmd/gomovepkg/main.go#ne...
cmd/gomovepkg/main.go:25: fmt.Fprintf(os.Stderr, "Error: %s.\n", err)
"gomovepkg: %s.\n"

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg.go
File refactor/movepkg/movepkg.go (right):

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:1: // TODO(matloob): what happens if the package is
moving across version
// Package movepkg moves ...
package movepkg

// TODO(matloob):
// - item1
// - item2
// - item3

This section would be a good place for a general description of the package, its
concepts and algorithm.

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:29: // TODO(matloob): Is this affected by the github
migration?
All of these can now be:

"golang.org/x/tools/go/buildutil"

etc.

You should also do:
 % mv code.google.com/p/go.tools golang.org/x/tools
in your workspace.  You don't need to run any hg commands.

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:36: // anything other than directory already
existing that needs to be checked?
Just a piece of general advice: don't share code for review (no matter how
unfinished you feel the logic is) without first cleaning up the syntax (writing
comments, eliminating cruft, etc).

Remember, the point of review is to get someone else's brain to help analyze the
underlying problem, so make sure the appearance of the code doesn't get in the
way.  It rarely takes long to clean up a piece of code without changing its
behaviour, so long as you understand what it does at this moment.  ("TODO"
comments are fine for things you plan to do later.)

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:48: for _, srcDir := range ctxt.SrcDirs() {
You can avoid calling SrcDirs by using the logic in refactor/importgraph: you
need to know the set of direct dependencies anyway, and you should always design
applications around the minimal number of I/O calls.

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:149: // TODO(matloob): Would it be better to have
the code in move in this
No, this is fine.  You don't have an iprog until now.

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:170: //			pkg/
FYI: "src/pkg" is now just "src", everywhere.

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:178: //  Could pkg and pkg/sub exist in the same
importgraph? If they could, pkg/sub
I would expect that the go/build API operates on the logical, combined
hierarchy, with all contributions from GOROOT, GOPATH[0], ... GOPATH[n-1] taken
into account.  If the implementations don't match that, report a bug.

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:209: // TODO(matloob): figure out how to reuse the
methods from package rename
Copy/paste (with a comment) is always fine as a starting point, but I'm starting
to think that moving at least part of this package into rename is a good idea. 
See below.

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:263: var filesToUpdate = make(map[*ast.File]bool)
filesToUpdate :=

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:264: var importRenames = []importRename{}
var importRenames []importRename

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:266: // rename package name of "from" package
(subpackages are not affected)
// Change the moved package's "package" declaration to its new base name.

https://codereview.appspot.com/153300043/diff/100001/refactor/movepkg/movepkg...
refactor/movepkg/movepkg.go:367: err := rename.Main(m.ctxt, "",
fmt.Sprintf("%s::%s", r.file, r.from), r.to)
The "file::from" notation won't always uniquely match the import spec, so this
could fail spuriously if there's (say) a local var also called 'from'.

Also, it's very inefficient to call rename.Main sequentially for each file since
it reloads all the dependencies.

I recommend that you make this function similar to rename.Main (called it
rename.Imports):

   package rename 

   func Imports(iprog *loader.Program, fromPkg *types.Package, to string) error
{
        for each info in iprog {       
          for each file f in info.Files {
            from := the import spec of 'fromPkg' in file f
            if from == nil, continue
    	    r := renamer{
		iprog:        iprog,
		objsToUpdate: make(map[types.Object]bool),
		to:           to,
		packages:     map[*types.Package]*loader.PackageInfo{info.Pkg: info},
	    }
	    r.check(from)
            if errors, print as warnings and continue
            r.update()
       }
       if any r.update() call failed, return an error
   }

Individual failures should be tolerated: e.g. if a renaming would shadow a
built-in such as len, then we should just skip that file.   Currently your loop
breaks out.