cmd/gorename: -d flag prints diffs instead of rewriting files

The user may specify the diff tool using the -diffcmd flag.

+ test.

Also:
- eliminate redundant DryRun flag
- simplify Verbose messages using log.SetPrefix

Fixes issue #13355

Change-Id: I917edc73e31ddf0f5d5b9b30c43f826465529da1
Reviewed-on: https://go-review.googlesource.com/18208
Reviewed-by: Michael Matloob <matloob@golang.org>

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")
-
+	flag.BoolVar(&rename.Diff, "d", false, "display diffs instead of rewriting files")
-	// If $GOMAXPROCS isn't set, use the full capacity of the machine.
+	flag.StringVar(&rename.DiffCmd, "diffcmd", "diff", "diff command invoked when using -d")
-	// For small machines, use at least 4 threads.
-	if os.Getenv("GOMAXPROCS") == "" {
-		n := runtime.NumCPU()
-		if n < 4 {
-			n = 4
-		}
-		runtime.GOMAXPROCS(n)
-	}
 }
 
 func main() {
+	log.SetPrefix("gorename: ")
+	log.SetFlags(0)
 	flag.Parse()
 	if len(flag.Args()) > 0 {
-		fmt.Fprintln(os.Stderr, "gorename: surplus arguments.")
+		log.Fatal("surplus arguments")
-		os.Exit(1)
 	}
 
 	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)
 	}

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.

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 {
+		writeFile = func(filename string, content []byte) error {
-			var out bytes.Buffer
+			got[filename] = string(content)
-			if err := format.Node(&out, fset, f); err != nil {
-				return err
-			}
-			got[orig] = out.String()
 			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 {
+		writeFile = func(filename string, content []byte) error {
-			var out bytes.Buffer
+			got[filename] = string(content)
-			if err := format.Node(&out, fset, f); err != nil {
-				return err
-			}
-			got[orig] = out.String()
 			return nil
 		}
 		moveDirectory = func(from, to string) error {

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.
+	// Diff causes the tool to display diffs instead of rewriting files.
-	DryRun bool
+	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",
+						log.Printf("Updating package %s", info.Pkg.Path())
-							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",
+	if !Diff {
-		nidents, plural(nidents),
+		fmt.Printf("Renamed %d occurrence%s in %d file%s in %d package%s.\n",
-		len(filesToUpdate), plural(len(filesToUpdate)),
+			nidents, plural(nidents),
-		npkgs, plural(npkgs))
+			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) {
+// writeFile is a seam for testing and for the -d flag.
-	// TODO(adonovan): print packages and filenames in a form useful
+var writeFile = reallyWriteFile
-	// to editors (so they can reload files).
+
-	if Verbose {
+func reallyWriteFile(filename string, content []byte) error {
-		fmt.Fprintf(os.Stderr, "\t%s\n", filename)
+	return ioutil.WriteFile(filename, content, 0644)
-	}
+}
-	var buf bytes.Buffer
+
-	if err := format.Node(&buf, fset, f); err != nil {
+func diff(filename string, content []byte) error {
-		return fmt.Errorf("failed to pretty-print syntax tree: %v", err)
+	renamed := filename + ".renamed"
-	}
+	if err := ioutil.WriteFile(renamed, content, 0644); err != nil {
-	return ioutil.WriteFile(filename, buf.Bytes(), 0644)
+		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
 }

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)) {
+	defer func(savedWriteFile func(string, []byte) error, savedReportError func(token.Position, string)) {
-		DryRun = savedDryRun
+		writeFile = savedWriteFile
 		reportError = savedReportError
-	}(DryRun, reportError)
+	}(writeFile, reportError)
-	DryRun = true
+	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) {
+	defer func(savedWriteFile func(string, []byte) error) {
-		rewriteFile = savedRewriteFile
+		writeFile = savedWriteFile
-	}(rewriteFile)
+	}(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 {
+		writeFile = func(filename string, content []byte) error {
-			var out bytes.Buffer
+			got[filepath.ToSlash(filename)] = string(content)
-			if err := format.Node(&out, fset, f); err != nil {
-				return err
-			}
-			got[filepath.ToSlash(orig)] = out.String()
 			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

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