diff --git a/cmd/gorename/main.go b/cmd/gorename/main.go index 922dc0cae8..b1b895cf1c 100644 --- a/cmd/gorename/main.go +++ b/cmd/gorename/main.go @@ -11,8 +11,8 @@ import ( "flag" "fmt" "go/build" + "log" "os" - "runtime" "golang.org/x/tools/go/buildutil" "golang.org/x/tools/refactor/rename" @@ -28,25 +28,17 @@ var ( func init() { flag.Var((*buildutil.TagsFlag)(&build.Default.BuildTags), "tags", buildutil.TagsFlagDoc) flag.BoolVar(&rename.Force, "force", false, "proceed, even if conflicts were reported") - flag.BoolVar(&rename.DryRun, "dryrun", false, "show the change, but do not apply it") flag.BoolVar(&rename.Verbose, "v", false, "print verbose information") - - // If $GOMAXPROCS isn't set, use the full capacity of the machine. - // For small machines, use at least 4 threads. - if os.Getenv("GOMAXPROCS") == "" { - n := runtime.NumCPU() - if n < 4 { - n = 4 - } - runtime.GOMAXPROCS(n) - } + flag.BoolVar(&rename.Diff, "d", false, "display diffs instead of rewriting files") + flag.StringVar(&rename.DiffCmd, "diffcmd", "diff", "diff command invoked when using -d") } func main() { + log.SetPrefix("gorename: ") + log.SetFlags(0) flag.Parse() if len(flag.Args()) > 0 { - fmt.Fprintln(os.Stderr, "gorename: surplus arguments.") - os.Exit(1) + log.Fatal("surplus arguments") } if *helpFlag || (*offsetFlag == "" && *fromFlag == "" && *toFlag == "") { @@ -56,7 +48,7 @@ func main() { if err := rename.Main(&build.Default, *offsetFlag, *fromFlag, *toFlag); err != nil { if err != rename.ConflictError { - fmt.Fprintf(os.Stderr, "gorename: %s\n", err) + log.Fatal(err) } os.Exit(1) } diff --git a/refactor/rename/mvpkg.go b/refactor/rename/mvpkg.go index 4eb8da6b4b..927195c11c 100644 --- a/refactor/rename/mvpkg.go +++ b/refactor/rename/mvpkg.go @@ -17,6 +17,7 @@ import ( "fmt" "go/ast" "go/build" + "go/format" "go/token" "log" "os" @@ -321,8 +322,13 @@ func (m *mover) move() error { } for f := range filesToUpdate { + var buf bytes.Buffer + if err := format.Node(&buf, m.iprog.Fset, f); err != nil { + log.Printf("failed to pretty-print syntax tree: %v", err) + continue + } tokenFile := m.iprog.Fset.File(f.Pos()) - rewriteFile(m.iprog.Fset, f, tokenFile.Name()) + writeFile(tokenFile.Name(), buf.Bytes()) } // Move the directories. diff --git a/refactor/rename/mvpkg_test.go b/refactor/rename/mvpkg_test.go index 77037197f8..e07b6b400a 100644 --- a/refactor/rename/mvpkg_test.go +++ b/refactor/rename/mvpkg_test.go @@ -5,12 +5,8 @@ package rename import ( - "bytes" "fmt" - "go/ast" "go/build" - "go/format" - "go/token" "io/ioutil" "path/filepath" "regexp" @@ -79,12 +75,8 @@ var _ foo.T ctxt := test.ctxt got := make(map[string]string) - rewriteFile = func(fset *token.FileSet, f *ast.File, orig string) error { - var out bytes.Buffer - if err := format.Node(&out, fset, f); err != nil { - return err - } - got[orig] = out.String() + writeFile = func(filename string, content []byte) error { + got[filename] = string(content) return nil } moveDirectory = func(from, to string) error { @@ -304,12 +296,8 @@ var _ bar.T } got[path] = string(bytes) }) - rewriteFile = func(fset *token.FileSet, f *ast.File, orig string) error { - var out bytes.Buffer - if err := format.Node(&out, fset, f); err != nil { - return err - } - got[orig] = out.String() + writeFile = func(filename string, content []byte) error { + got[filename] = string(content) return nil } moveDirectory = func(from, to string) error { diff --git a/refactor/rename/rename.go b/refactor/rename/rename.go index 8eef4f42b4..e8eaa4127e 100644 --- a/refactor/rename/rename.go +++ b/refactor/rename/rename.go @@ -16,8 +16,11 @@ import ( "go/format" "go/parser" "go/token" + "io" "io/ioutil" + "log" "os" + "os/exec" "path" "sort" "strconv" @@ -76,7 +79,7 @@ Flags: (In due course this bug will be fixed by moving certain analyses into the type-checker.) --dryrun causes the tool to report conflicts but not update any files. +-d display diffs instead of rewriting files -v enables verbose logging. @@ -137,8 +140,12 @@ var ( // It may even cause gorename to crash. TODO(adonovan): fix that. Force bool - // DryRun causes the tool to report conflicts but not update any files. - DryRun bool + // Diff causes the tool to display diffs instead of rewriting files. + Diff bool + + // DiffCmd specifies the diff command used by the -d feature. + // (The command must accept a -u flag and two filename arguments.) + DiffCmd = "diff" // ConflictError is returned by Main when it aborts the renaming due to conflicts. // (It is distinguished because the interesting errors are the conflicts themselves.) @@ -148,6 +155,8 @@ var ( Verbose bool ) +var stdout io.Writer + type renamer struct { iprog *loader.Program objsToUpdate map[types.Object]bool @@ -212,6 +221,11 @@ func Main(ctxt *build.Context, offsetFlag, fromFlag, to string) error { return fmt.Errorf("-to %q: not a valid identifier", to) } + if Diff { + defer func(saved func(string, []byte) error) { writeFile = saved }(writeFile) + writeFile = diff + } + var spec *spec var err error if fromFlag != "" { @@ -248,7 +262,7 @@ func Main(ctxt *build.Context, offsetFlag, fromFlag, to string) error { // package defining the object, plus their tests. if Verbose { - fmt.Fprintln(os.Stderr, "Potentially global renaming; scanning workspace...") + log.Print("Potentially global renaming; scanning workspace...") } // Scan the workspace and build the import graph. @@ -329,10 +343,6 @@ func Main(ctxt *build.Context, offsetFlag, fromFlag, to string) error { if r.hadConflicts && !Force { return ConflictError } - if DryRun { - // TODO(adonovan): print the delta? - return nil - } return r.update() } @@ -361,7 +371,7 @@ func loadProgram(ctxt *build.Context, pkgs map[string]bool) (*loader.Program, er } sort.Strings(list) for _, pkg := range list { - fmt.Fprintf(os.Stderr, "Loading package: %s\n", pkg) + log.Printf("Loading package: %s", pkg) } } @@ -433,21 +443,30 @@ func (r *renamer) update() error { npkgs++ first = false if Verbose { - fmt.Fprintf(os.Stderr, "Updating package %s\n", - info.Pkg.Path()) + log.Printf("Updating package %s", info.Pkg.Path()) } } - if err := rewriteFile(r.iprog.Fset, f, tokenFile.Name()); err != nil { - fmt.Fprintf(os.Stderr, "gorename: %s\n", err) + + filename := tokenFile.Name() + var buf bytes.Buffer + if err := format.Node(&buf, r.iprog.Fset, f); err != nil { + log.Printf("failed to pretty-print syntax tree: %v", err) + nerrs++ + continue + } + if err := writeFile(filename, buf.Bytes()); err != nil { + log.Print(err) nerrs++ } } } } - fmt.Fprintf(os.Stderr, "Renamed %d occurrence%s in %d file%s in %d package%s.\n", - nidents, plural(nidents), - len(filesToUpdate), plural(len(filesToUpdate)), - npkgs, plural(npkgs)) + if !Diff { + fmt.Printf("Renamed %d occurrence%s in %d file%s in %d package%s.\n", + nidents, plural(nidents), + len(filesToUpdate), plural(len(filesToUpdate)), + npkgs, plural(npkgs)) + } if nerrs > 0 { return fmt.Errorf("failed to rewrite %d file%s", nerrs, plural(nerrs)) } @@ -461,15 +480,29 @@ func plural(n int) string { return "" } -var rewriteFile = func(fset *token.FileSet, f *ast.File, filename string) (err error) { - // TODO(adonovan): print packages and filenames in a form useful - // to editors (so they can reload files). - if Verbose { - fmt.Fprintf(os.Stderr, "\t%s\n", filename) - } - var buf bytes.Buffer - if err := format.Node(&buf, fset, f); err != nil { - return fmt.Errorf("failed to pretty-print syntax tree: %v", err) - } - return ioutil.WriteFile(filename, buf.Bytes(), 0644) +// writeFile is a seam for testing and for the -d flag. +var writeFile = reallyWriteFile + +func reallyWriteFile(filename string, content []byte) error { + return ioutil.WriteFile(filename, content, 0644) +} + +func diff(filename string, content []byte) error { + renamed := filename + ".renamed" + if err := ioutil.WriteFile(renamed, content, 0644); err != nil { + return err + } + defer os.Remove(renamed) + + diff, err := exec.Command(DiffCmd, "-u", filename, renamed).CombinedOutput() + if len(diff) > 0 { + // diff exits with a non-zero status when the files don't match. + // Ignore that failure as long as we get output. + stdout.Write(diff) + return nil + } + if err != nil { + return fmt.Errorf("computing diff: %v", err) + } + return nil } diff --git a/refactor/rename/rename_test.go b/refactor/rename/rename_test.go index 1d282a1aec..778fa78b64 100644 --- a/refactor/rename/rename_test.go +++ b/refactor/rename/rename_test.go @@ -7,10 +7,9 @@ package rename import ( "bytes" "fmt" - "go/ast" "go/build" - "go/format" "go/token" + "os" "path/filepath" "regexp" "strings" @@ -22,11 +21,11 @@ import ( // TODO(adonovan): test reported source positions, somehow. func TestConflicts(t *testing.T) { - defer func(savedDryRun bool, savedReportError func(token.Position, string)) { - DryRun = savedDryRun + defer func(savedWriteFile func(string, []byte) error, savedReportError func(token.Position, string)) { + writeFile = savedWriteFile reportError = savedReportError - }(DryRun, reportError) - DryRun = true + }(writeFile, reportError) + writeFile = func(string, []byte) error { return nil } var ctxt *build.Context for _, test := range []struct { @@ -417,9 +416,9 @@ var _ I = E{} } func TestRewrites(t *testing.T) { - defer func(savedRewriteFile func(*token.FileSet, *ast.File, string) error) { - rewriteFile = savedRewriteFile - }(rewriteFile) + defer func(savedWriteFile func(string, []byte) error) { + writeFile = savedWriteFile + }(writeFile) var ctxt *build.Context for _, test := range []struct { @@ -977,12 +976,8 @@ var _ = I(C(0)).(J) } got := make(map[string]string) - rewriteFile = func(fset *token.FileSet, f *ast.File, orig string) error { - var out bytes.Buffer - if err := format.Node(&out, fset, f); err != nil { - return err - } - got[filepath.ToSlash(orig)] = out.String() + writeFile = func(filename string, content []byte) error { + got[filepath.ToSlash(filename)] = string(content) return nil } @@ -1017,6 +1012,34 @@ var _ = I(C(0)).(J) } } +func TestDiff(t *testing.T) { + defer func() { + Diff = false + stdout = os.Stdout + }() + Diff = true + stdout = new(bytes.Buffer) + + if err := Main(&build.Default, "", `"golang.org/x/tools/refactor/rename".justHereForTestingDiff`, "Foo"); err != nil { + t.Fatal(err) + } + + // NB: there are tabs in the string literal! + if !strings.Contains(stdout.(fmt.Stringer).String(), ` +-func justHereForTestingDiff() { +- justHereForTestingDiff() ++func Foo() { ++ Foo() + } +`) { + t.Errorf("unexpected diff:\n<<%s>>", stdout) + } +} + +func justHereForTestingDiff() { + justHereForTestingDiff() +} + // --------------------------------------------------------------------- // Simplifying wrapper around buildutil.FakeContext for packages whose diff --git a/refactor/rename/spec.go b/refactor/rename/spec.go index 3c73653574..f37c32806c 100644 --- a/refactor/rename/spec.go +++ b/refactor/rename/spec.go @@ -15,6 +15,7 @@ import ( "go/build" "go/parser" "go/token" + "log" "os" "path/filepath" "strconv" @@ -126,7 +127,7 @@ func parseFromFlag(ctxt *build.Context, fromFlag string) (*spec, error) { } if Verbose { - fmt.Fprintf(os.Stderr, "-from spec: %+v\n", spec) + log.Printf("-from spec: %+v", spec) } return &spec, nil