git-codereview: disable Gerrit-specific hook behavior if Gerrit is not in use

Every time I accidentally type 'git pending' in a non-Gerrit repository,
that command installs hooks, and then I have to zero the hook files
to keep working.

First, don't install the hooks if this looks like a non-Gerrit git repo.
That seems like it will play well with others.

But, people might also want to use the non-Gerrit parts of the tool,
so if people do say 'git codereview hooks', let that install them.

And then if the hooks are invoked in a non-Gerrit repo, selectively
disable the Gerrit-specific parts. For example, if it's not a Gerrit repo
we might still want to check and fix 'Fixes #N' lines if directed by the
config, but we don't want to add Change-Id lines.

Now that things are a bit better behaved, also leave the hooks on
in detached head mode. I've found that it's quite frustrating not
to have the hooks on when working midway through git rebase -i.
If we find reasons that the hooks were off we can try to figure out
how to split the difference.

Fixes golang/go#12170.

Change-Id: I69fa156ce4fd11c0c84416088cd972957ce1ce6d
Reviewed-on: https://go-review.googlesource.com/19560
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Austin Clements <austin@google.com>

7 files changed, 151 insertions, 60 deletions

diff --git a/git-codereview/config.go b/git-codereview/config.go
index 1adf71f..739c4f5 100644
--- a/git-codereview/config.go
+++ b/git-codereview/config.go
@@ -36,6 +36,28 @@ func config() map[string]string {
 	return cachedConfig
 }
 
+func haveGerrit() bool {
+	gerrit := config()["gerrit"]
+	if gerrit == "off" {
+		return false
+	}
+	if gerrit != "" {
+		return true
+	}
+	origin := trim(cmdOutput("git", "config", "remote.origin.url"))
+	if strings.Contains(origin, "github.com") {
+		return false
+	}
+	if !strings.Contains(origin, "https://") {
+		return false
+	}
+	if strings.Count(origin, "/") != 3 {
+		return false
+	}
+	host := origin[:strings.LastIndex(origin, "/")]
+	return strings.HasSuffix(host, ".googlesource.com")
+}
+
 func parseConfig(raw string) (map[string]string, error) {
 	cfg := make(map[string]string)
 	for _, line := range nonBlankLines(raw) {
diff --git a/git-codereview/hook.go b/git-codereview/hook.go
index 570001e..264c85b 100644
--- a/git-codereview/hook.go
+++ b/git-codereview/hook.go
@@ -124,12 +124,18 @@ func hookCommitMsg(args []string) {
 		dief("usage: git-codereview hook-invoke commit-msg message.txt\n")
 	}
 
-	b := CurrentBranch()
-	if b.DetachedHead() {
-		// Likely executing rebase or some other internal operation.
-		// Probably a mistake to make commit message changes.
-		return
-	}
+	// We used to bail in detached head mode, but it's very common
+	// to be modifying things during git rebase -i and it's annoying
+	// that those new commits made don't get Commit-Msg lines.
+	// Let's try keeping the hook on and see what breaks.
+	/*
+		b := CurrentBranch()
+		if b.DetachedHead() {
+			// Likely executing rebase or some other internal operation.
+			// Probably a mistake to make commit message changes.
+			return
+		}
+	*/
 
 	file := args[0]
 	oldData, err := ioutil.ReadFile(file)
@@ -161,34 +167,37 @@ func hookCommitMsg(args []string) {
 	oldFixesRE := regexp.MustCompile(fmt.Sprintf(oldFixesRETemplate, regexp.QuoteMeta(issueRepo)))
 	data = oldFixesRE.ReplaceAll(data, []byte("Fixes "+issueRepo+"#${issueNum}"))
 
-	// Complain if two Change-Ids are present.
-	// This can happen during an interactive rebase;
-	// it is easy to forget to remove one of them.
-	nChangeId := bytes.Count(data, []byte("\nChange-Id: "))
-	if nChangeId > 1 {
-		dief("multiple Change-Id lines")
-	}
-
-	// Add Change-Id to commit message if not present.
-	if nChangeId == 0 {
-		n := len(data)
-		for n > 0 && data[n-1] == '\n' {
-			n--
+	if haveGerrit() {
+		// Complain if two Change-Ids are present.
+		// This can happen during an interactive rebase;
+		// it is easy to forget to remove one of them.
+		nChangeId := bytes.Count(data, []byte("\nChange-Id: "))
+		if nChangeId > 1 {
+			dief("multiple Change-Id lines")
 		}
-		var id [20]byte
-		if _, err := io.ReadFull(rand.Reader, id[:]); err != nil {
-			dief("generating Change-Id: %v", err)
+
+		// Add Change-Id to commit message if not present.
+		if nChangeId == 0 {
+			n := len(data)
+			for n > 0 && data[n-1] == '\n' {
+				n--
+			}
+			var id [20]byte
+			if _, err := io.ReadFull(rand.Reader, id[:]); err != nil {
+				dief("generating Change-Id: %v", err)
+			}
+			data = append(data[:n], fmt.Sprintf("\n\nChange-Id: I%x\n", id[:])...)
 		}
-		data = append(data[:n], fmt.Sprintf("\n\nChange-Id: I%x\n", id[:])...)
-	}
 
-	// Add branch prefix to commit message if not present and not on master
-	// and not a special Git fixup! or squash! commit message.
-	branch := strings.TrimPrefix(b.OriginBranch(), "origin/")
-	if branch != "master" {
-		prefix := "[" + branch + "] "
-		if !bytes.HasPrefix(data, []byte(prefix)) && !isFixup(data) {
-			data = []byte(prefix + string(data))
+		// Add branch prefix to commit message if not present and not on master
+		// and not a special Git fixup! or squash! commit message.
+		b := CurrentBranch()
+		branch := strings.TrimPrefix(b.OriginBranch(), "origin/")
+		if strings.HasPrefix(branch, "dev.") {
+			prefix := "[" + branch + "] "
+			if !bytes.HasPrefix(data, []byte(prefix)) && !isFixup(data) {
+				data = []byte(prefix + string(data))
+			}
 		}
 	}
 
@@ -222,15 +231,25 @@ func stripComments(in []byte) []byte {
 // the change are gofmt'd, and if not it prints gofmt instructions
 // and exits with nonzero status.
 func hookPreCommit(args []string) {
-	// Prevent commits to master branches.
-	b := CurrentBranch()
-	if b.DetachedHead() {
-		// This is an internal commit such as during git rebase.
-		// Don't die, and don't force gofmt.
-		return
-	}
-	if !b.IsLocalOnly() {
-		dief("cannot commit on %s branch", b.Name)
+	// We used to bail in detached head mode, but it's very common
+	// to be modifying things during git rebase -i and it's annoying
+	// that those new commits made don't get the gofmt check.
+	// Let's try keeping the hook on and see what breaks.
+	/*
+		b := CurrentBranch()
+		if b.DetachedHead() {
+			// This is an internal commit such as during git rebase.
+			// Don't die, and don't force gofmt.
+			return
+		}
+	*/
+
+	// Prevent commits to master branches, but only if we're here for code review.
+	if haveGerrit() {
+		b := CurrentBranch()
+		if !b.IsLocalOnly() && b.Name != "HEAD" {
+			dief("cannot commit on %s branch", b.Name)
+		}
 	}
 
 	hookGofmt()
@@ -238,7 +257,7 @@ func hookPreCommit(args []string) {
 
 func hookGofmt() {
 	if os.Getenv("GIT_GOFMT_HOOK") == "off" {
-		fmt.Fprintf(stderr(), "git-gofmt-hook disabled by $GIT_GOFMT_HOOK=off\n")
+		fmt.Fprintf(stderr(), "git-codereview pre-commit gofmt hook disabled by $GIT_GOFMT_HOOK=off\n")
 		return
 	}
 
diff --git a/git-codereview/hook_test.go b/git-codereview/hook_test.go
index fc17e1e..247666c 100644
--- a/git-codereview/hook_test.go
+++ b/git-codereview/hook_test.go
@@ -14,10 +14,9 @@ import (
 	"testing"
 )
 
-const lenChangeId = len("\n\nChange-Id: I") + 2*20
-
-func TestHookCommitMsg(t *testing.T) {
+func TestHookCommitMsgGerrit(t *testing.T) {
 	gt := newGitTest(t)
+	gt.enableGerrit(t)
 	defer gt.done()
 
 	// Check that hook adds Change-Id.
@@ -42,6 +41,11 @@ func TestHookCommitMsg(t *testing.T) {
 	if got := testStderr.String(); got != multiple {
 		t.Fatalf("unexpected output:\ngot: %q\nwant: %q", got, multiple)
 	}
+}
+
+func TestHookCommitMsg(t *testing.T) {
+	gt := newGitTest(t)
+	defer gt.done()
 
 	// Check that hook fails when message is empty.
 	write(t, gt.client+"/empty.txt", "\n\n# just a file with\n# comments\n")
@@ -76,9 +80,6 @@ func TestHookCommitMsg(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		// pull off the Change-Id that got appended
-		got = got[:len(got)-lenChangeId]
-		want = want[:len(want)-lenChangeId]
 		if !bytes.Equal(got, want) {
 			t.Fatalf("failed to rewrite:\n%s\n\ngot:\n\n%s\n\nwant:\n\n%s\n", tt.in, got, want)
 		}
@@ -96,13 +97,11 @@ func TestHookCommitMsgIssueRepoRewrite(t *testing.T) {
 		"math/big: catch all the rats\n\nFixes issue #99999, at least for now\n",
 		"math/big: catch all the rats\n\nFixes issue 99999, at least for now\n",
 		// Don't forget to write back if Change-Id already exists
-		"math/big: catch all the rats\n\nFixes issue #99999, at least for now\n\nChange-Id: Ie77358867e38cf976a0688b6e2f80525dae3891e\n",
 	}
 	for _, msg := range msgs {
 		write(t, gt.client+"/msg.txt", msg)
 		testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
 		got := read(t, gt.client+"/msg.txt")
-		got = got[:len(got)-lenChangeId]
 		const want = "math/big: catch all the rats\n\nFixes #99999, at least for now\n"
 		if string(got) != want {
 			t.Errorf("issue rewrite failed: got\n\n%s\nwant\n\n%s\nlen %d and %d", got, want, len(got), len(want))
@@ -125,13 +124,11 @@ func TestHookCommitMsgIssueRepoRewrite(t *testing.T) {
 		"math/big: catch all the rats\n\nFixes issue #99999, at least for now\n",
 		"math/big: catch all the rats\n\nFixes issue 99999, at least for now\n",
 		"math/big: catch all the rats\n\nFixes issue golang/go#99999, at least for now\n",
-		"math/big: catch all the rats\n\nFixes issue #99999, at least for now\n\nChange-Id: Ie77358867e38cf976a0688b6e2f80525dae3891e\n",
 	}
 	for _, msg := range msgs {
 		write(t, gt.client+"/msg.txt", msg)
 		testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
 		got := read(t, gt.client+"/msg.txt")
-		got = got[:len(got)-lenChangeId]
 		const want = "math/big: catch all the rats\n\nFixes golang/go#99999, at least for now\n"
 		if string(got) != want {
 			t.Errorf("issue rewrite failed: got\n\n%s\nwant\n\n%s", got, want)
@@ -144,7 +141,17 @@ func TestHookCommitMsgIssueRepoRewrite(t *testing.T) {
 }
 
 func TestHookCommitMsgBranchPrefix(t *testing.T) {
+	testHookCommitMsgBranchPrefix(t, false)
+	testHookCommitMsgBranchPrefix(t, true)
+}
+
+func testHookCommitMsgBranchPrefix(t *testing.T, gerrit bool) {
+	t.Logf("gerrit=%v", gerrit)
+
 	gt := newGitTest(t)
+	if gerrit {
+		gt.enableGerrit(t)
+	}
 	defer gt.done()
 
 	checkPrefix := func(prefix string) {
@@ -179,11 +186,19 @@ func TestHookCommitMsgBranchPrefix(t *testing.T) {
 	trun(t, gt.server, "git", "checkout", "-b", "dev.cc")
 	trun(t, gt.client, "git", "fetch", "-q")
 	testMain(t, "change", "dev.cc")
-	checkPrefix("[dev.cc] Test message.\n")
+	if gerrit {
+		checkPrefix("[dev.cc] Test message.\n")
+	} else {
+		checkPrefix("Test message.\n")
+	}
 
 	// Work branch with server branch as upstream.
 	testMain(t, "change", "ccwork")
-	checkPrefix("[dev.cc] Test message.\n")
+	if gerrit {
+		checkPrefix("[dev.cc] Test message.\n")
+	} else {
+		checkPrefix("Test message.\n")
+	}
 
 	// Master has no prefix.
 	testMain(t, "change", "master")
@@ -271,9 +286,30 @@ func TestHookPreCommitDetachedHead(t *testing.T) {
 	trun(t, gt.client, "git", "add", ".")
 	trun(t, gt.client, "git", "checkout", "HEAD^0")
 
-	testMain(t, "hook-invoke", "pre-commit")
-	testNoStdout(t)
-	testNoStderr(t)
+	testMainDied(t, "hook-invoke", "pre-commit")
+	testPrintedStderr(t, "gofmt needs to format these files (run 'git gofmt'):", "bad.go")
+
+	/*
+		OLD TEST, back when we disabled gofmt in detached head,
+		in case we go back to that:
+
+		// If we're in detached head mode, something special is going on,
+		// like git rebase. We disable the gofmt-checking precommit hook,
+		// since we expect it would just get in the way at that point.
+		// (It also used to crash.)
+
+		gt := newGitTest(t)
+		defer gt.done()
+		gt.work(t)
+
+		write(t, gt.client+"/bad.go", badGo)
+		trun(t, gt.client, "git", "add", ".")
+		trun(t, gt.client, "git", "checkout", "HEAD^0")
+
+		testMain(t, "hook-invoke", "pre-commit")
+		testNoStdout(t)
+		testNoStderr(t)
+	*/
 }
 
 func TestHookPreCommitEnv(t *testing.T) {
@@ -290,7 +326,7 @@ func TestHookPreCommitEnv(t *testing.T) {
 
 	testMain(t, "hook-invoke", "pre-commit")
 	testNoStdout(t)
-	testPrintedStderr(t, "git-gofmt-hook disabled by $GIT_GOFMT_HOOK=off")
+	testPrintedStderr(t, "git-codereview pre-commit gofmt hook disabled by $GIT_GOFMT_HOOK=off")
 }
 
 func TestHookPreCommitUnstaged(t *testing.T) {
@@ -407,6 +443,7 @@ func testInstallHook(t *testing.T, gt *gitTest) (restore func()) {
 
 func TestHookCommitMsgFromGit(t *testing.T) {
 	gt := newGitTest(t)
+	gt.enableGerrit(t)
 	defer gt.done()
 
 	restore := testInstallHook(t, gt)
diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index 7d77618..a3b4db8 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -387,7 +387,7 @@ func allSubmitted(work []*Commit) bool {
 func (b *Branch) errors() string {
 	b.loadPending()
 	var buf bytes.Buffer
-	if !b.IsLocalOnly() && b.commitsAhead > 0 {
+	if haveGerrit() && !b.IsLocalOnly() && b.commitsAhead > 0 {
 		fmt.Fprintf(&buf, "Branch contains %d commit%s not on origin/%s.\n", b.commitsAhead, suffix(b.commitsAhead, "s"), b.Name)
 		fmt.Fprintf(&buf, "\tDo not commit directly to %s branch.\n", b.Name)
 	}
diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go
index 7f2d354..f4168ac 100644
--- a/git-codereview/pending_test.go
+++ b/git-codereview/pending_test.go
@@ -152,6 +152,7 @@ func TestPendingComplex(t *testing.T) {
 
 func TestPendingErrors(t *testing.T) {
 	gt := newGitTest(t)
+	gt.enableGerrit(t)
 	defer gt.done()
 
 	trun(t, gt.client, "git", "checkout", "master")
diff --git a/git-codereview/review.go b/git-codereview/review.go
index 15766ef..779b77c 100644
--- a/git-codereview/review.go
+++ b/git-codereview/review.go
@@ -125,7 +125,10 @@ func main() {
 		return
 	}
 
-	installHook()
+	// Install hooks automatically, but only if this is a Gerrit repo.
+	if haveGerrit() {
+		installHook()
+	}
 
 	switch command {
 	case "branchpoint":
@@ -137,7 +140,7 @@ func main() {
 	case "hook-invoke":
 		cmdHookInvoke(args)
 	case "hooks":
-		// done - installHook already ran
+		installHook() // in case above was bypassed
 	case "mail", "m":
 		cmdMail(args)
 	case "pending":
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index b010f08..a0ff469 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -61,6 +61,7 @@ func (gt *gitTest) done() {
 	os.Chdir(gt.pwd) // change out of gt.tmpdir first, otherwise following os.RemoveAll fails on windows
 	resetReadOnlyFlagAll(gt.tmpdir)
 	os.RemoveAll(gt.tmpdir)
+	cachedConfig = nil
 }
 
 // doWork simulates commit 'n' touching 'file' in 'dir'
@@ -176,6 +177,13 @@ func newGitTest(t *testing.T) (gt *gitTest) {
 	}
 }
 
+func (gt *gitTest) enableGerrit(t *testing.T) {
+	write(t, gt.server+"/codereview.cfg", "gerrit: myserver\n")
+	trun(t, gt.server, "git", "add", "codereview.cfg")
+	trun(t, gt.server, "git", "commit", "-m", "add gerrit")
+	trun(t, gt.client, "git", "pull", "-r")
+}
+
 func (gt *gitTest) removeStubHooks() {
 	for _, h := range hookFiles {
 		os.RemoveAll(gt.client + "/.git/hooks/" + h)
@@ -261,6 +269,7 @@ func testMainCanDie(t *testing.T, args ...string) {
 func testMain(t *testing.T, args ...string) {
 	*noRun = false
 	*verbose = 0
+	cachedConfig = nil
 
 	t.Logf("git-codereview %s", strings.Join(args, " "))