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>
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 @@
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 @@
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 @@
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)
- }
- 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 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[:])...)
+ }
+
+ // 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 @@
// 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 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 @@
"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 @@
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 @@
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 @@
"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 @@
"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 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 @@
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 @@
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 @@
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 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 (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 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 @@
return
}
- installHook()
+ // Install hooks automatically, but only if this is a Gerrit repo.
+ if haveGerrit() {
+ installHook()
+ }
switch command {
case "branchpoint":
@@ -137,7 +140,7 @@
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 @@
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 (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 testMain(t *testing.T, args ...string) {
*noRun = false
*verbose = 0
+ cachedConfig = nil
t.Logf("git-codereview %s", strings.Join(args, " "))
