git-codereview: fix a few corner case failures
- make it clearer what the random git command at the end of failures means
- avoid some problems with detached HEAD mode in hooks run during git rebase
- redirect all stdout/stderr into test buffers
Change-Id: I102f22fc870f69c884728eaa46ecc95792d5b795
Reviewed-on: https://go-review.googlesource.com/2011
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
diff --git a/git-codereview/branch.go b/git-codereview/branch.go
index 9c9d8e7..40288c7 100644
--- a/git-codereview/branch.go
+++ b/git-codereview/branch.go
@@ -7,7 +7,6 @@
import (
"bytes"
"fmt"
- "os"
"os/exec"
"regexp"
"strings"
@@ -34,10 +33,24 @@
return &Branch{Name: name}
}
+// DetachedHead reports whether branch b corresponds to a detached HEAD
+// (does not have a real branch name).
+func (b *Branch) DetachedHead() bool {
+ return b.Name == "HEAD"
+}
+
// OriginBranch returns the name of the origin branch that branch b tracks.
// The returned name is like "origin/master" or "origin/dev.garbage" or
// "origin/release-branch.go1.4".
func (b *Branch) OriginBranch() string {
+ if b.DetachedHead() {
+ // Detached head mode.
+ // "origin/HEAD" is clearly false, but it should be easy to find when it
+ // appears in other commands. Really any caller of OriginBranch
+ // should check for detached head mode.
+ return "origin/HEAD"
+ }
+
if b.originBranch != "" {
return b.originBranch
}
@@ -52,7 +65,7 @@
b.originBranch = "origin/master"
return b.originBranch
}
- fmt.Fprintf(os.Stderr, "%v\n%s\n", commandString(argv[0], argv[1:]), out)
+ fmt.Fprintf(stderr(), "%v\n%s\n", commandString(argv[0], argv[1:]), out)
dief("%v", err)
panic("not reached")
}
@@ -99,6 +112,10 @@
}
b.loadedPending = true
+ if b.DetachedHead() {
+ return
+ }
+
const numField = 5
all := getOutput("git", "log", "--format=format:%H%x00%h%x00%P%x00%s%x00%B%x00", b.OriginBranch()+".."+b.Name)
fields := strings.Split(all, "\x00")
@@ -192,8 +209,20 @@
func LocalBranches() []*Branch {
var branches []*Branch
+ current := CurrentBranch()
for _, s := range getLines("git", "branch", "-q") {
- s = strings.TrimPrefix(strings.TrimSpace(s), "* ")
+ s = strings.TrimSpace(s)
+ if strings.HasPrefix(s, "* ") {
+ // * marks current branch in output.
+ // Normally the current branch has a name like any other,
+ // but in detached HEAD mode the branch listing shows
+ // a localized (translated) textual description instead of
+ // a branch name. Avoid language-specific differences
+ // by using CurrentBranch().Name for the current branch.
+ // It detects detached HEAD mode in a more portable way.
+ // (git rev-parse --abbrev-ref HEAD returns 'HEAD').
+ s = current.Name
+ }
branches = append(branches, &Branch{Name: s})
}
return branches
diff --git a/git-codereview/branch_test.go b/git-codereview/branch_test.go
index 10949fe..1c18cfa 100644
--- a/git-codereview/branch_test.go
+++ b/git-codereview/branch_test.go
@@ -4,7 +4,10 @@
package main
-import "testing"
+import (
+ "reflect"
+ "testing"
+)
func TestCurrentBranch(t *testing.T) {
gt := newGitTest(t)
@@ -34,6 +37,10 @@
write(t, gt.client+"/file", "i made another change")
trun(t, gt.client, "git", "commit", "-a", "-m", "My other change line.\n\nChange-Id: I1123456789abcdef0123456789abcdef\n")
checkCurrentBranch(t, "newdev", "origin/dev.branch", true, true, "I1123456789abcdef0123456789abcdef", "My other change line.")
+
+ t.Logf("detached head mode")
+ trun(t, gt.client, "git", "checkout", "HEAD^0")
+ checkCurrentBranch(t, "HEAD", "origin/HEAD", false, false, "", "")
}
func checkCurrentBranch(t *testing.T, name, origin string, isLocal, hasPending bool, changeID, subject string) {
@@ -57,3 +64,30 @@
t.Errorf("b.Subject() = %q, want %q", x, subject)
}
}
+
+func TestLocalBranches(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ t.Logf("on master")
+ checkLocalBranches(t, "master")
+
+ t.Logf("on dev branch")
+ trun(t, gt.client, "git", "checkout", "-b", "newbranch")
+ checkLocalBranches(t, "master", "newbranch")
+
+ t.Logf("detached head mode")
+ trun(t, gt.client, "git", "checkout", "HEAD^0")
+ checkLocalBranches(t, "HEAD", "master", "newbranch")
+}
+
+func checkLocalBranches(t *testing.T, want ...string) {
+ var names []string
+ branches := LocalBranches()
+ for _, b := range branches {
+ names = append(names, b.Name)
+ }
+ if !reflect.DeepEqual(names, want) {
+ t.Errorf("LocalBranches() = %v, want %v", names, want)
+ }
+}
diff --git a/git-codereview/change.go b/git-codereview/change.go
index f7b8ec1..4281b6a 100644
--- a/git-codereview/change.go
+++ b/git-codereview/change.go
@@ -19,7 +19,7 @@
flags.BoolVar(&changeQuick, "q", false, "do not edit pending commit msg")
flags.Parse(args)
if len(flags.Args()) > 1 {
- fmt.Fprintf(os.Stderr, "Usage: %s change %s [branch]\n", os.Args[0], globalFlags)
+ fmt.Fprintf(stderr(), "Usage: %s change %s [branch]\n", os.Args[0], globalFlags)
os.Exit(2)
}
@@ -71,6 +71,10 @@
var testCommitMsg string
func commitChanges(amend bool) {
+ // git commit will run the gofmt hook.
+ // Run it now to give a better error (won't show a git commit command failing).
+ hookGofmt()
+
if HasUnstagedChanges() && !HasStagedChanges() && !changeAuto {
printf("warning: unstaged changes and no staged changes; use 'git add' or 'git change -a'")
}
@@ -102,6 +106,13 @@
}
func checkoutOrCreate(target string) {
+ if strings.ToUpper(target) == "HEAD" {
+ // Git gets very upset and confused if you 'git change head'
+ // on systems with case-insensitive file names: the branch
+ // head conflicts with the usual HEAD.
+ dief("invalid branch name %q: ref name HEAD is reserved for git.", target)
+ }
+
// If local branch exists, check it out.
for _, b := range LocalBranches() {
if b.Name == target {
diff --git a/git-codereview/change_test.go b/git-codereview/change_test.go
index 4a24c6a..daf06e3 100644
--- a/git-codereview/change_test.go
+++ b/git-codereview/change_test.go
@@ -36,6 +36,14 @@
testRan(t, "git checkout -q -t -b dev.branch origin/dev.branch")
}
+func TestChangeHEAD(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ testMainDied(t, "change", "HeAd")
+ testPrintedStderr(t, "invalid branch name \"HeAd\": ref name HEAD is reserved for git")
+}
+
func TestMessageRE(t *testing.T) {
for _, c := range []struct {
in string
diff --git a/git-codereview/gofmt.go b/git-codereview/gofmt.go
index 4cde005..648b795 100644
--- a/git-codereview/gofmt.go
+++ b/git-codereview/gofmt.go
@@ -8,7 +8,6 @@
"bytes"
"flag"
"fmt"
- "io"
"os"
"os/exec"
"path/filepath"
@@ -22,7 +21,7 @@
flags.BoolVar(&gofmtList, "l", false, "list files that need to be formatted")
flags.Parse(args)
if len(flag.Args()) > 0 {
- fmt.Fprintf(os.Stderr, "Usage: %s gofmt %s [-l]\n", globalFlags, os.Args[0])
+ fmt.Fprintf(stderr(), "Usage: %s gofmt %s [-l]\n", globalFlags, os.Args[0])
os.Exit(2)
}
@@ -33,12 +32,9 @@
files, stderr := runGofmt(f)
if gofmtList {
- var out io.Writer = os.Stdout
- if stdoutTrap != nil {
- out = stdoutTrap
- }
+ w := stdout()
for _, file := range files {
- fmt.Fprintf(out, "%s\n", file)
+ fmt.Fprintf(w, "%s\n", file)
}
}
if stderr != "" {
diff --git a/git-codereview/gofmt_test.go b/git-codereview/gofmt_test.go
index a4b884c..8cd3d81 100644
--- a/git-codereview/gofmt_test.go
+++ b/git-codereview/gofmt_test.go
@@ -46,10 +46,10 @@
testPrintedStdout(t, "bad.go\n", "!good.go", "!test/bad", "test/bench/bad.go")
testMain(t, "gofmt")
- testPrintedStdout(t, "!.go") // no output
+ testNoStdout(t)
testMain(t, "gofmt", "-l")
- testPrintedStdout(t, "!.go") // no output
+ testNoStdout(t)
write(t, gt.client+"/bad.go", badGo)
write(t, gt.client+"/broken.go", brokenGo)
@@ -146,7 +146,7 @@
// Reformat in place.
testMainDied(t, "gofmt")
- testPrintedStdout(t, "!.go")
+ testNoStdout(t)
testPrintedStderr(t, wantErr...)
// Read files to make sure unstaged did not bleed into staged.
@@ -166,6 +166,6 @@
// Check that gofmt -l still shows the errors.
testMainDied(t, "gofmt", "-l")
- testPrintedStdout(t, "!.go")
+ testNoStdout(t)
testPrintedStderr(t, wantErr...)
}
diff --git a/git-codereview/hook.go b/git-codereview/hook.go
index 165e6e8..921fd91 100644
--- a/git-codereview/hook.go
+++ b/git-codereview/hook.go
@@ -156,6 +156,11 @@
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)
}
diff --git a/git-codereview/hook_test.go b/git-codereview/hook_test.go
index 6cd822f..15b04a0 100644
--- a/git-codereview/hook_test.go
+++ b/git-codereview/hook_test.go
@@ -43,7 +43,7 @@
write(t, gt.client+"/empty.txt", "\n\n# just a file with\n# comments\n")
testMainDied(t, "hook-invoke", "commit-msg", gt.client+"/empty.txt")
const want = "git-codereview: empty commit message\n"
- if got := stderr.String(); got != want {
+ if got := testStderr.String(); got != want {
t.Fatalf("unexpected output:\ngot: %q\nwant: %q", got, want)
}
}
@@ -81,6 +81,55 @@
"gofmt reported errors:", "broken.go")
}
+func TestHookChangeGofmt(t *testing.T) {
+ // During git change, we run the gofmt check before invoking commit,
+ // so we should not see the line about 'git commit' failing.
+ // That is, the failure should come from git change, not from
+ // the commit hook.
+ gt := newGitTest(t)
+ defer gt.done()
+ gt.work(t)
+
+ // Write out a non-Go file.
+ write(t, gt.client+"/bad.go", badGo)
+ trun(t, gt.client, "git", "add", ".")
+
+ t.Logf("invoking commit hook explicitly")
+ testMainDied(t, "hook-invoke", "pre-commit")
+ testPrintedStderr(t, "gofmt needs to format these files (run 'git gofmt'):", "bad.go")
+
+ t.Logf("change without hook installed")
+ testCommitMsg = "foo: msg"
+ testMainDied(t, "change")
+ testPrintedStderr(t, "gofmt needs to format these files (run 'git gofmt'):", "bad.go", "!running: git")
+
+ t.Logf("change with hook installed")
+ restore := testInstallHook(t, gt)
+ defer restore()
+ testCommitMsg = "foo: msg"
+ testMainDied(t, "change")
+ testPrintedStderr(t, "gofmt needs to format these files (run 'git gofmt'):", "bad.go", "!running: git")
+}
+
+func TestHookPreCommitDetachedHead(t *testing.T) {
+ // 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 TestHookPreCommitUnstaged(t *testing.T) {
gt := newGitTest(t)
defer gt.done()
@@ -181,17 +230,25 @@
}
}
+func testInstallHook(t *testing.T, gt *gitTest) (restore func()) {
+ trun(t, gt.pwd, "go", "build", "-o", gt.client+"/git-codereview")
+ path := os.Getenv("PATH")
+ os.Setenv("PATH", gt.client+string(filepath.ListSeparator)+path)
+ gt.removeStubHooks()
+ testMain(t, "hooks") // install hooks
+
+ return func() {
+ os.Setenv("PATH", path)
+ }
+}
+
func TestHookCommitMsgFromGit(t *testing.T) {
gt := newGitTest(t)
defer gt.done()
- trun(t, gt.pwd, "go", "build", "-o", gt.client+"/git-codereview")
- path := os.Getenv("PATH")
- defer os.Setenv("PATH", path)
- os.Setenv("PATH", gt.client+string(filepath.ListSeparator)+path)
+ restore := testInstallHook(t, gt)
+ defer restore()
- gt.removeStubHooks()
- testMain(t, "hooks") // install hooks
testMain(t, "change", "mybranch")
write(t, gt.client+"/file", "more data")
trun(t, gt.client, "git", "add", "file")
diff --git a/git-codereview/mail.go b/git-codereview/mail.go
index 01dd4b8..c06a784 100644
--- a/git-codereview/mail.go
+++ b/git-codereview/mail.go
@@ -22,7 +22,7 @@
flags.Var(ccList, "cc", "comma-separated list of people to CC:")
flags.Usage = func() {
- fmt.Fprintf(os.Stderr, "Usage: %s mail %s [-r reviewer,...] [-cc mail,...]\n", os.Args[0], globalFlags)
+ fmt.Fprintf(stderr(), "Usage: %s mail %s [-r reviewer,...] [-cc mail,...]\n", os.Args[0], globalFlags)
}
flags.Parse(args)
if len(flags.Args()) != 0 {
diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index e4e2a96..a90f6ce 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -52,7 +52,7 @@
flags.BoolVar(&pendingLocal, "l", false, "use only local information - no network operations")
flags.Parse(args)
if len(flags.Args()) > 0 {
- fmt.Fprintf(os.Stderr, "Usage: %s pending %s [-l]\n", os.Args[0], globalFlags)
+ fmt.Fprintf(stderr(), "Usage: %s pending %s [-l]\n", os.Args[0], globalFlags)
os.Exit(2)
}
@@ -219,11 +219,7 @@
fmt.Fprintf(&buf, "\n")
}
- if stdoutTrap != nil {
- stdoutTrap.Write(buf.Bytes())
- } else {
- os.Stdout.Write(buf.Bytes())
- }
+ stdout().Write(buf.Bytes())
}
// errors returns any errors that should be displayed
diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go
index b1030e2..68c0f00 100644
--- a/git-codereview/pending_test.go
+++ b/git-codereview/pending_test.go
@@ -241,7 +241,7 @@
want = strings.TrimPrefix(want, "\n")
testMain(t, "pending")
- out := stdout.Bytes()
+ out := testStdout.Bytes()
out = regexp.MustCompile(`\b[0-9a-f]{7}\b`).ReplaceAllLiteral(out, []byte("REVHASH"))
out = regexp.MustCompile(`\b127\.0\.0\.1:\d+\b`).ReplaceAllLiteral(out, []byte("127.0.0.1:PORT"))
diff --git a/git-codereview/review.go b/git-codereview/review.go
index a2a60fb..fe8867e 100644
--- a/git-codereview/review.go
+++ b/git-codereview/review.go
@@ -29,7 +29,7 @@
func initFlags() {
flags = flag.NewFlagSet("", flag.ExitOnError)
flags.Usage = func() {
- fmt.Fprintf(os.Stderr, usage, os.Args[0], os.Args[0])
+ fmt.Fprintf(stderr(), usage, os.Args[0], os.Args[0])
}
flags.Var(verbose, "v", "report git commands")
flags.BoolVar(noRun, "n", false, "print but do not run commands")
@@ -117,7 +117,7 @@
command, args := os.Args[1], os.Args[2:]
if command == "help" {
- fmt.Fprintf(os.Stdout, help, os.Args[0])
+ fmt.Fprintf(stdout(), help, os.Args[0])
return
}
@@ -148,7 +148,7 @@
func expectZeroArgs(args []string, command string) {
flags.Parse(args)
if len(flags.Args()) > 0 {
- fmt.Fprintf(os.Stderr, "Usage: %s %s %s\n", os.Args[0], command, globalFlags)
+ fmt.Fprintf(stderr(), "Usage: %s %s %s\n", os.Args[0], command, globalFlags)
os.Exit(2)
}
}
@@ -158,7 +158,7 @@
if *verbose == 0 {
// If we're not in verbose mode, print the command
// before dying to give context to the failure.
- fmt.Fprintln(os.Stderr, commandString(command, args))
+ fmt.Fprintf(stderr(), "(running: %s)\n", commandString(command, args))
}
dief("%v", err)
}
@@ -172,7 +172,7 @@
func runDirErr(dir, command string, args ...string) error {
if *verbose > 0 || *noRun {
- fmt.Fprintln(os.Stderr, commandString(command, args))
+ fmt.Fprintln(stderr(), commandString(command, args))
}
if *noRun {
return nil
@@ -182,14 +182,8 @@
}
cmd := exec.Command(command, args...)
cmd.Stdin = os.Stdin
- cmd.Stdout = os.Stdout
- if stdoutTrap != nil {
- cmd.Stdout = stdoutTrap
- }
- cmd.Stderr = os.Stderr
- if stderrTrap != nil {
- cmd.Stderr = stderrTrap
- }
+ cmd.Stdout = stdout()
+ cmd.Stderr = stderr()
return cmd.Run()
}
@@ -201,7 +195,7 @@
func getOutput(command string, args ...string) string {
s, err := getOutputErr(command, args...)
if err != nil {
- fmt.Fprintf(os.Stderr, "%v\n%s\n", commandString(command, args), s)
+ fmt.Fprintf(stderr(), "%v\n%s\n", commandString(command, args), s)
dief("%v", err)
}
return s
@@ -215,7 +209,7 @@
// the git repo" commands, which is confusing if you are just trying to find
// out what git sync means.
if *verbose > 1 {
- fmt.Fprintln(os.Stderr, commandString(command, args))
+ fmt.Fprintln(stderr(), commandString(command, args))
}
b, err := exec.Command(command, args...).CombinedOutput()
return string(bytes.TrimSpace(b)), err
@@ -257,12 +251,22 @@
var stdoutTrap, stderrTrap *bytes.Buffer
-func printf(format string, args ...interface{}) {
- w := io.Writer(os.Stderr)
- if stderrTrap != nil {
- w = stderrTrap
+func stdout() io.Writer {
+ if stdoutTrap != nil {
+ return stdoutTrap
}
- fmt.Fprintf(w, "%s: %s\n", os.Args[0], fmt.Sprintf(format, args...))
+ return os.Stdout
+}
+
+func stderr() io.Writer {
+ if stderrTrap != nil {
+ return stderrTrap
+ }
+ return os.Stderr
+}
+
+func printf(format string, args ...interface{}) {
+ fmt.Fprintf(stderr(), "%s: %s\n", os.Args[0], fmt.Sprintf(format, args...))
}
// count is a flag.Value that is like a flag.Bool and a flag.Int.
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index 27ed6c9..0be544a 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -125,10 +125,10 @@
}
var (
- runLog []string
- stderr *bytes.Buffer
- stdout *bytes.Buffer
- died bool
+ runLog []string
+ testStderr *bytes.Buffer
+ testStdout *bytes.Buffer
+ died bool
)
var mainCanDie bool
@@ -137,7 +137,7 @@
mainCanDie = true
testMain(t, args...)
if !died {
- t.Fatalf("expected to die, did not\nstdout:\n%sstderr:\n%s", stdout, stderr)
+ t.Fatalf("expected to die, did not\nstdout:\n%sstderr:\n%s", testStdout, testStderr)
}
}
@@ -157,8 +157,8 @@
defer func() {
runLog = runLogTrap
- stdout = stdoutTrap
- stderr = stderrTrap
+ testStdout = stdoutTrap
+ testStderr = stderrTrap
dieTrap = nil
runLogTrap = nil
@@ -174,7 +174,7 @@
} else {
msg = fmt.Sprintf("panic: %v", err)
}
- t.Fatalf("%s\nstdout:\n%sstderr:\n%s", msg, stdout, stderr)
+ t.Fatalf("%s\nstdout:\n%sstderr:\n%s", msg, testStdout, testStderr)
}
}()
@@ -221,22 +221,22 @@
}
func testPrintedStdout(t *testing.T, messages ...string) {
- testPrinted(t, stdout, "stdout", messages...)
+ testPrinted(t, testStdout, "stdout", messages...)
}
func testPrintedStderr(t *testing.T, messages ...string) {
- testPrinted(t, stderr, "stderr", messages...)
+ testPrinted(t, testStderr, "stderr", messages...)
}
func testNoStdout(t *testing.T) {
- if stdout.Len() != 0 {
- t.Fatalf("unexpected stdout:\n%s", stdout)
+ if testStdout.Len() != 0 {
+ t.Fatalf("unexpected stdout:\n%s", testStdout)
}
}
func testNoStderr(t *testing.T) {
- if stderr.Len() != 0 {
- t.Fatalf("unexpected stderr:\n%s", stderr)
+ if testStderr.Len() != 0 {
+ t.Fatalf("unexpected stderr:\n%s", testStderr)
}
}
