git-codereview: add a few more tests

Change-Id: I17ad065e8b4c1eb00cdd2f17af82be76bae4fc09
Reviewed-on: https://go-review.googlesource.com/c/review/+/279717
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

12 files changed, 99 insertions, 35 deletions

diff --git a/git-codereview/change.go b/git-codereview/change.go
index ccc7836..991707d 100644
--- a/git-codereview/change.go
+++ b/git-codereview/change.go
@@ -6,7 +6,6 @@ package main
 
 import (
 	"fmt"
-	"os"
 	"regexp"
 	"strconv"
 	"strings"
@@ -24,7 +23,7 @@ func cmdChange(args []string) {
 	flags.Parse(args)
 	if len(flags.Args()) > 1 {
 		fmt.Fprintf(stderr(), "Usage: %s change %s [-a] [-m msg] [-q] [branch]\n", progName, globalFlags)
-		os.Exit(2)
+		exit(2)
 	}
 
 	// Checkout or create branch, if specified.
@@ -56,13 +55,11 @@ func cmdChange(args []string) {
 }
 
 func (b *Branch) check() {
-	// TODO(rsc): Test
 	staged, unstaged, _ := LocalChanges()
 	if len(staged) == 0 && len(unstaged) == 0 {
 		// No staged changes, no unstaged changes.
 		// If the branch is behind upstream, now is a good time to point that out.
 		// This applies to both local work branches and tracking branches.
-		// TODO(rsc): Test.
 		b.loadPending()
 		if n := b.CommitsBehind(); n > 0 {
 			printf("warning: %d commit%s behind %s; run 'git codereview sync' to update.", n, suffix(n, "s"), b.OriginBranch())
diff --git a/git-codereview/change_test.go b/git-codereview/change_test.go
index 206ed82..8a3960d 100644
--- a/git-codereview/change_test.go
+++ b/git-codereview/change_test.go
@@ -38,6 +38,20 @@ func TestChange(t *testing.T) {
 	t.Logf("main -> dev.branch")
 	testMain(t, "change", "dev.branch")
 	testRan(t, "git checkout -q -t -b dev.branch origin/dev.branch")
+
+	testMain(t, "pending", "-c")
+	testPrintedStdout(t, "tracking dev.branch")
+	testMain(t, "change", "main")
+	testMain(t, "change", "work2")
+
+	t.Logf("server work × 2")
+	gt.serverWork(t)
+	gt.serverWork(t)
+	testMain(t, "sync")
+
+	t.Logf("-> work with server ahead")
+	testMain(t, "change", "work")
+	testPrintedStderr(t, "warning: 2 commits behind origin/main; run 'git codereview sync' to update")
 }
 
 func TestChangeHEAD(t *testing.T) {
diff --git a/git-codereview/gofmt.go b/git-codereview/gofmt.go
index bfba123..7986d07 100644
--- a/git-codereview/gofmt.go
+++ b/git-codereview/gofmt.go
@@ -23,7 +23,7 @@ func cmdGofmt(args []string) {
 	flags.Parse(args)
 	if len(flag.Args()) > 0 {
 		fmt.Fprintf(stderr(), "Usage: %s gofmt %s [-l]\n", progName, globalFlags)
-		os.Exit(2)
+		exit(2)
 	}
 
 	f := gofmtCommand
diff --git a/git-codereview/mail.go b/git-codereview/mail.go
index e6cdb90..55ab8ed 100644
--- a/git-codereview/mail.go
+++ b/git-codereview/mail.go
@@ -6,7 +6,6 @@ package main
 
 import (
 	"fmt"
-	"os"
 	"regexp"
 	"sort"
 	"strings"
@@ -38,11 +37,12 @@ func cmdMail(args []string) {
 			"Usage: %s mail %s [-r reviewer,...] [-cc mail,...]\n"+
 				"\t[-f] [-diff] [-hashtag tag,...] [-nokeycheck] [-topic topic]\n"+
 				"\t[-trust] [-trybot] [-wip] [commit]\n", progName, globalFlags)
+		exit(2)
 	}
 	flags.Parse(args)
 	if len(flags.Args()) > 1 {
 		flags.Usage()
-		os.Exit(2)
+		exit(2)
 	}
 
 	b := CurrentBranch()
@@ -221,7 +221,7 @@ func mailList(start, tag string, flagList string) string {
 		verbosef("expanded %s to %s", short[1:], long[1:])
 	}
 	if errors {
-		die()
+		exit(1)
 	}
 	return spec
 }
diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index e17a905..5808764 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"os"
 	"sort"
 	"strings"
 	"time"
@@ -87,7 +86,7 @@ func cmdPending(args []string) {
 	flags.Parse(args)
 	if len(flags.Args()) > 0 {
 		fmt.Fprintf(stderr(), "Usage: %s pending %s [-c] [-l] [-s]\n", progName, globalFlags)
-		os.Exit(2)
+		exit(2)
 	}
 
 	// Fetch info about remote changes, so that we can say which branches need sync.
diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go
index ccb2f76..1124e4d 100644
--- a/git-codereview/pending_test.go
+++ b/git-codereview/pending_test.go
@@ -323,6 +323,7 @@ func TestPendingGerrit(t *testing.T) {
 				file
 
 	`)
+
 	testPendingArgs(t, []string{"-l", "-s"}, `
 		work REVHASH..REVHASH (current branch, 1 behind)
 		+ REVHASH msg
diff --git a/git-codereview/review.go b/git-codereview/review.go
index 0242ec7..0deb039 100644
--- a/git-codereview/review.go
+++ b/git-codereview/review.go
@@ -34,6 +34,7 @@ func initFlags() {
 	flags.Usage = func() {
 		fmt.Fprintf(stderr(), usage, progName, progName)
 	}
+	flags.SetOutput(stderr())
 	flags.BoolVar(noRun, "n", false, "print but do not run commands")
 	flags.Var(verbose, "v", "report commands")
 }
@@ -76,10 +77,7 @@ func main() {
 
 	if len(os.Args) < 2 {
 		flags.Usage()
-		if dieTrap != nil {
-			dieTrap()
-		}
-		os.Exit(2)
+		exit(2)
 	}
 	command, args := os.Args[1], os.Args[2:]
 
@@ -88,7 +86,7 @@ func main() {
 	switch command {
 	default:
 		flags.Usage()
-		return // avoid installing hooks.
+		exit(2) // avoid installing hooks.
 	case "help":
 		fmt.Fprintf(stdout(), help, progName)
 		return // avoid installing hooks.
@@ -140,7 +138,7 @@ func expectZeroArgs(args []string, command string) {
 	flags.Parse(args)
 	if len(flags.Args()) > 0 {
 		fmt.Fprintf(stderr(), "Usage: %s %s %s\n", progName, command, globalFlags)
-		os.Exit(2)
+		exit(2)
 	}
 }
 
@@ -292,18 +290,18 @@ func commandString(command string, args []string) string {
 	return strings.Join(append([]string{command}, args...), " ")
 }
 
-var dieTrap func()
-
 func dief(format string, args ...interface{}) {
 	printf(format, args...)
-	die()
+	exit(1)
 }
 
-func die() {
-	if dieTrap != nil {
-		dieTrap()
+var exitTrap func()
+
+func exit(code int) {
+	if exitTrap != nil {
+		exitTrap()
 	}
-	os.Exit(1)
+	os.Exit(code)
 }
 
 func verbosef(format string, args ...interface{}) {
diff --git a/git-codereview/submit.go b/git-codereview/submit.go
index 9045ea4..5f684ca 100644
--- a/git-codereview/submit.go
+++ b/git-codereview/submit.go
@@ -7,7 +7,6 @@ package main
 import (
 	"bytes"
 	"fmt"
-	"os"
 	"strings"
 	"time"
 )
@@ -22,7 +21,7 @@ func cmdSubmit(args []string) {
 	flags.Parse(args)
 	if interactive && flags.NArg() > 0 {
 		flags.Usage()
-		os.Exit(2)
+		exit(2)
 	}
 
 	b := CurrentBranch()
diff --git a/git-codereview/submit_test.go b/git-codereview/submit_test.go
index fbe2cb3..29f9c02 100644
--- a/git-codereview/submit_test.go
+++ b/git-codereview/submit_test.go
@@ -80,7 +80,7 @@ func TestSubmitErrors(t *testing.T) {
 	testPrintedStderr(t, "cannot submit: change abandoned")
 
 	t.Logf("> missing approval")
-	srv.setJSON(id, `{"status": "NEW", "labels": {"Code-Review": {}}}`)
+	srv.setJSON(id, `{"status": "NEW", "labels": {"Code-Review": {}, "Color": {"Optional": true}}}`)
 	testMainDied(t, "submit")
 	testRan(t) // nothing
 	testPrintedStderr(t, "cannot submit: change missing Code-Review approval")
@@ -160,6 +160,15 @@ func TestSubmit(t *testing.T) {
 		submitted = true
 		return gerritReply{body: ")]}'\n" + submittedJSON}
 	}})
+
+	testMain(t, "submit", "-n")
+	testPrintedStderr(t, "submitting")
+	testPrintedStderr(t, "stopped before submit")
+
+	testMain(t, "pending", "-c", "-l")
+	testPrintedStdout(t, "(current branch)")
+	testPrintedStdout(t, "Files in this change:")
+
 	testMain(t, "submit")
 	testRan(t,
 		"git fetch -q",
@@ -196,17 +205,31 @@ func TestSubmitInteractive(t *testing.T) {
 	gt := newGitTest(t)
 	defer gt.done()
 
+	os.Setenv("GIT_EDITOR", ":")
+	testMain(t, "submit", "-i")
+	testPrintedStderr(t, "nothing to submit")
+
 	srv := newGerritServer(t)
 	defer srv.done()
 
 	cl1, cl2 := testSubmitMultiple(t, gt, srv)
-	os.Setenv("GIT_EDITOR", "echo "+cl1.CurrentRevision+" > ")
+
+	os.Setenv("GIT_EDITOR", "echo > ")
+	testMain(t, "submit", "-i")
+	if cl1.Status != "NEW" {
+		t.Fatalf("want cl1.Status == NEW; got %v", cl1.Status)
+	}
+	if cl2.Status != "NEW" {
+		t.Fatalf("want cl2.Status == NEW; got %v", cl2.Status)
+	}
+
+	os.Setenv("GIT_EDITOR", "( echo "+cl1.CurrentRevision+"; echo; echo '# comment' ) > ")
 	testMain(t, "submit", "-i")
 	if cl1.Status != "MERGED" {
 		t.Fatalf("want cl1.Status == MERGED; got %v", cl1.Status)
 	}
 	if cl2.Status != "NEW" {
-		t.Fatalf("want cl2.Status == NEW; got %v", cl1.Status)
+		t.Fatalf("want cl2.Status == NEW; got %v", cl2.Status)
 	}
 }
 
diff --git a/git-codereview/sync.go b/git-codereview/sync.go
index dc03c61..f518a3f 100644
--- a/git-codereview/sync.go
+++ b/git-codereview/sync.go
@@ -33,11 +33,11 @@ func cmdSync(args []string) {
 		run("git", "pull", "-q", "-r", "origin", strings.TrimPrefix(b.OriginBranch(), "origin/"))
 	}
 
-	// If the change commit has been submitted,
-	// roll back change leaving any changes unstaged.
-	// Pull should have done this for us, but check just in case.
 	b = CurrentBranch() // discard any cached information
 	if len(b.Pending()) == 1 && b.Submitted(id) {
+		// If the change commit has been submitted,
+		// roll back change leaving any changes unstaged.
+		// Pull should have done this for us, but check just in case.
 		run("git", "reset", b.Branchpoint())
 	}
 }
diff --git a/git-codereview/sync_test.go b/git-codereview/sync_test.go
index a4210ab..72cbdf8 100644
--- a/git-codereview/sync_test.go
+++ b/git-codereview/sync_test.go
@@ -104,4 +104,22 @@ func TestSyncRebase(t *testing.T) {
 	if len(b.Pending()) != 0 {
 		t.Fatalf("have %d pending CLs after final sync, want 0", len(b.Pending()))
 	}
+
+	// sync -v prints git output.
+	// also exercising -v parsing.
+	testMain(t, "sync", "-v=true")
+	testNoStdout(t)
+	testPrintedStderr(t, "git pull -q -r origin main")
+
+	testMain(t, "sync", "-v=1")
+	testNoStdout(t)
+	testPrintedStderr(t, "git pull -q -r origin main")
+
+	testMain(t, "sync", "-v")
+	testNoStdout(t)
+	testPrintedStderr(t, "git pull -q -r origin main")
+
+	testMain(t, "sync", "-v=false")
+	testNoStdout(t)
+	testNoStderr(t)
 }
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index 90a5b93..b1b5534 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -290,10 +290,9 @@ var (
 	testStderr *bytes.Buffer
 	testStdout *bytes.Buffer
 	died       bool
+	mainCanDie bool
 )
 
-var mainCanDie bool
-
 func testMainDied(t *testing.T, args ...string) {
 	t.Helper()
 	mainCanDie = true
@@ -319,7 +318,7 @@ func testMain(t *testing.T, args ...string) {
 		testStdout = stdoutTrap
 		testStderr = stderrTrap
 
-		dieTrap = nil
+		exitTrap = nil
 		runLogTrap = nil
 		stdoutTrap = nil
 		stderrTrap = nil
@@ -337,7 +336,7 @@ func testMain(t *testing.T, args ...string) {
 		}
 	}()
 
-	dieTrap = func() {
+	exitTrap = func() {
 		died = true
 		panic("died")
 	}
@@ -526,3 +525,19 @@ func (s *gerritServer) serveChangesQuery(w http.ResponseWriter, req *http.Reques
 	fmt.Fprintf(&buf, "%s", end)
 	w.Write(buf.Bytes())
 }
+
+func TestUsage(t *testing.T) {
+	gt := newGitTest(t)
+	defer gt.done()
+
+	testMainDied(t)
+	testPrintedStderr(t, "Usage: git-codereview <command>")
+
+	testMainDied(t, "not-a-command")
+	testPrintedStderr(t, "Usage: git-codereview <command>")
+
+	// During tests we configure the flag package to panic on error
+	// instead of
+	testMainDied(t, "mail", "-not-a-flag")
+	testPrintedStderr(t, "flag provided but not defined")
+}