git-codereview: do not gofmt committed files during merge

Change-Id: Ie772b8f46587ad0d56350b697e75601f45b4c393
Reviewed-on: https://go-review.googlesource.com/2785
Reviewed-by: Andrew Gerrand <adg@golang.org>

3 files changed, 53 insertions, 4 deletions

diff --git a/git-codereview/gofmt.go b/git-codereview/gofmt.go
index f9644a0..28edf5d 100644
--- a/git-codereview/gofmt.go
+++ b/git-codereview/gofmt.go
@@ -119,7 +119,15 @@ func runGofmt(flags int) (files []string, stderrText string) {
 	}
 
 	// Find files modified in the index compared to the branchpoint.
-	indexFiles := addRoot(repo, filter(gofmtRequired, getLines("git", "diff", "--name-only", "--diff-filter=ACM", "--cached", b.Branchpoint(), "--")))
+	branchpt := b.Branchpoint()
+	if strings.Contains(getOutput("git", "branch", "-r", "--contains", b.Name), "origin/") {
+		// This is a branch tag move, not an actual change.
+		// Use HEAD as branch point, so nothing will appear changed.
+		// We don't want to think about gofmt on already published
+		// commits.
+		branchpt = "HEAD"
+	}
+	indexFiles := addRoot(repo, filter(gofmtRequired, getLines("git", "diff", "--name-only", "--diff-filter=ACM", "--cached", branchpt, "--")))
 	localFiles := addRoot(repo, filter(gofmtRequired, getLines("git", "diff", "--name-only", "--diff-filter=ACM")))
 	localFilesMap := stringMap(localFiles)
 	isUnstaged := func(file string) bool {
diff --git a/git-codereview/gofmt_test.go b/git-codereview/gofmt_test.go
index e96c717..62184ab 100644
--- a/git-codereview/gofmt_test.go
+++ b/git-codereview/gofmt_test.go
@@ -179,3 +179,42 @@ func TestGofmtAmbiguousRevision(t *testing.T) {
 
 	testMain(t, "gofmt")
 }
+
+func TestGofmtFastForwardMerge(t *testing.T) {
+	gt := newGitTest(t)
+	defer gt.done()
+
+	// merge dev.branch into master
+	write(t, gt.server+"/file", "more work")
+	trun(t, gt.server, "git", "commit", "-m", "work", "file")
+	trun(t, gt.server, "git", "merge", "-m", "merge", "dev.branch")
+
+	// add bad go file on master
+	write(t, gt.server+"/bad.go", "package {\n")
+	trun(t, gt.server, "git", "add", "bad.go")
+	trun(t, gt.server, "git", "commit", "-m", "bad go")
+
+	// update client
+	trun(t, gt.client, "git", "checkout", "master")
+	trun(t, gt.client, "git", "pull")
+	trun(t, gt.client, "git", "change", "dev.branch")
+	trun(t, gt.client, "git", "pull")
+
+	// merge master into dev.branch, fast forward merge
+	trun(t, gt.client, "git", "merge", "--ff-only", "master")
+
+	// verify that now client is in a state where just the tag is changing; there's no new commit.
+	masterHash := strings.TrimSpace(trun(t, gt.server, "git", "rev-parse", "master"))
+	devHash := strings.TrimSpace(trun(t, gt.client, "git", "rev-parse", "HEAD"))
+
+	if masterHash != devHash {
+		t.Logf("branches:\n%s", trun(t, gt.client, "git", "branch", "-a", "-v"))
+		t.Logf("log:\n%s", trun(t, gt.client, "git", "log", "--graph", "--decorate"))
+		t.Fatalf("setup wrong - got different commit hashes on master and dev branch")
+	}
+
+	// check that gofmt finds nothing to do, ignoring the bad (but committed) file1.go.
+	testMain(t, "gofmt")
+	testNoStdout(t)
+	testNoStderr(t)
+}
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index a5f1418..76a835d 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -92,10 +92,12 @@ func newGitTest(t *testing.T) *gitTest {
 
 	for _, name := range []string{"dev.branch", "release.branch"} {
 		trun(t, server, "git", "checkout", "master")
-		trun(t, server, "git", "branch", name)
-		write(t, server+"/file", "this is "+name)
-		trun(t, server, "git", "commit", "-a", "-m", "on "+name)
+		trun(t, server, "git", "checkout", "-b", name)
+		write(t, server+"/file."+name, "this is "+name)
+		trun(t, server, "git", "add", "file."+name)
+		trun(t, server, "git", "commit", "-m", "on "+name)
 	}
+	trun(t, server, "git", "checkout", "master")
 
 	client := tmpdir + "/git-client"
 	mkdir(t, client)