git-review: add support for non-master origin branches
The basic idea is that each work branch explicitly tracks
a particular origin branch, instead of assuming that everything
tracks "master". In addition to enabling development branches
like dev.cc or dev.garbage, this makes plain 'git pull' work
better in work branches.
In fact, after this commit, 'git sync' could be rewritten to be
nothing more than 'git pull -r'.
- disallow names with dots in 'git-review change':
we are reserving that set of names for our own use
(dev.garbage, release-branch.go1.4, work.uploaded and so on)
- make Branch methods invoke git on demand as called
- always use Branch, not branch name, for inspecting local branches
- start test framework
- add basic tests for change
- add 'git-review hooks' for people not using git-review otherwise
- delete revert (at least for now)
Change-Id: Ib9f51a78a1b23ce7514c938da246afc1377fde9e
Reviewed-on: https://go-review.googlesource.com/1221
Reviewed-by: Andrew Gerrand <adg@golang.org>
diff --git a/git-review/branch.go b/git-review/branch.go
index 628ee96..812d574 100644
--- a/git-review/branch.go
+++ b/git-review/branch.go
@@ -5,6 +5,10 @@
package main
import (
+ "bytes"
+ "fmt"
+ "os"
+ "os/exec"
"regexp"
"strings"
)
@@ -12,33 +16,54 @@
// Branch describes a Git branch.
type Branch struct {
Name string // branch name
- ChangeID string // Change-Id of pending commit ("" if nothing pending)
- Subject string // first line of pending commit ("" if nothing pending)
+ changeID string // Change-Id of pending commit ("" if nothing pending)
+ subject string // first line of pending commit ("" if nothing pending)
}
-// Submitted reports whether some form of b's pending commit
-// has been cherry picked to master.
-func (b *Branch) Submitted() bool {
- return b.ChangeID != "" && changeSubmitted(b.ChangeID)
-}
-
+// CurrentBranch returns the current branch.
func CurrentBranch() *Branch {
- b := &Branch{Name: currentBranchName()}
- if hasPendingCommit(b.Name) {
- b.ChangeID = headChangeID(b.Name)
- b.Subject = commitSubject(b.Name)
- }
- return b
+ name := getOutput("git", "rev-parse", "--abbrev-ref", "HEAD")
+ return &Branch{Name: name}
}
-func hasPendingCommit(branch string) bool {
- head := getOutput("git", "rev-parse", branch)
- base := getOutput("git", "merge-base", "origin/master", branch)
+func (b *Branch) OriginBranch() string {
+ argv := []string{"git", "rev-parse", "--abbrev-ref", "@{u}"}
+ out, err := exec.Command(argv[0], argv[1:]...).CombinedOutput()
+ if err == nil && len(out) > 0 {
+ return string(bytes.TrimSpace(out))
+ }
+ if strings.Contains(string(out), "No upstream configured") {
+ // Assume branch was created before we set upstream correctly.
+ return "origin/master"
+ }
+ fmt.Fprintf(os.Stderr, "%v\n%s\n", commandString(argv[0], argv[1:]), out)
+ dief("%v", err)
+ panic("not reached")
+}
+
+func (b *Branch) IsLocalOnly() bool {
+ return "origin/"+b.Name != b.OriginBranch()
+}
+
+func (b *Branch) HasPendingCommit() bool {
+ head := getOutput("git", "rev-parse", b.Name)
+ base := getOutput("git", "merge-base", b.OriginBranch(), b.Name)
return base != head
}
-func currentBranchName() string {
- return getOutput("git", "rev-parse", "--abbrev-ref", "HEAD")
+func (b *Branch) ChangeID() string {
+ if b.changeID == "" {
+ if b.HasPendingCommit() {
+ b.changeID = headChangeID(b.Name)
+ b.subject = commitSubject(b.Name)
+ }
+ }
+ return b.changeID
+}
+
+func (b *Branch) Subject() string {
+ b.ChangeID() // page in subject
+ return b.subject
}
func commitSubject(ref string) string {
@@ -46,11 +71,6 @@
return getOutput("git", "log", "-n", "1", f, ref, "--")
}
-func changeSubmitted(id string) bool {
- s := "Change-Id: " + id
- return len(getOutput("git", "log", "--grep", s, "origin/master")) > 0
-}
-
func headChangeID(branch string) string {
const (
p = "Change-Id: "
@@ -65,9 +85,15 @@
panic("unreachable")
}
+// Submitted reports whether some form of b's pending commit
+// has been cherry picked to origin.
+func (b *Branch) Submitted(id string) bool {
+ return len(getOutput("git", "log", "--grep", "Change-Id: "+id, b.OriginBranch())) > 0
+}
+
var stagedRe = regexp.MustCompile(`^[ACDMR] `)
-func hasStagedChanges() bool {
+func HasStagedChanges() bool {
for _, s := range getLines("git", "status", "-b", "--porcelain") {
if stagedRe.MatchString(s) {
return true
@@ -76,9 +102,24 @@
return false
}
-func localBranches() (branches []string) {
- for _, s := range getLines("git", "branch", "-l", "-q") {
- branches = append(branches, strings.TrimPrefix(s, "* "))
+func LocalBranches() []*Branch {
+ var branches []*Branch
+ for _, s := range getLines("git", "branch", "-q") {
+ branches = append(branches, &Branch{Name: strings.TrimPrefix(s, "* ")})
+ }
+ return branches
+}
+
+func OriginBranches() []string {
+ var branches []string
+ for _, line := range getLines("git", "branch", "-a", "-q") {
+ if i := strings.Index(line, " -> "); i >= 0 {
+ line = line[:i]
+ }
+ name := strings.TrimSpace(strings.TrimPrefix(line, "* "))
+ if strings.HasPrefix(name, "remotes/origin/") {
+ branches = append(branches, strings.TrimPrefix(name, "remotes/"))
+ }
}
return branches
}
diff --git a/git-review/branch_test.go b/git-review/branch_test.go
new file mode 100644
index 0000000..10949fe
--- /dev/null
+++ b/git-review/branch_test.go
@@ -0,0 +1,59 @@
+// Copyright 2014 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import "testing"
+
+func TestCurrentBranch(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ t.Logf("on master")
+ checkCurrentBranch(t, "master", "origin/master", false, false, "", "")
+
+ t.Logf("on newbranch")
+ trun(t, gt.client, "git", "checkout", "-b", "newbranch")
+ checkCurrentBranch(t, "newbranch", "origin/master", true, false, "", "")
+
+ t.Logf("making change")
+ write(t, gt.client+"/file", "i made a change")
+ trun(t, gt.client, "git", "commit", "-a", "-m", "My change line.\n\nChange-Id: I0123456789abcdef0123456789abcdef\n")
+ checkCurrentBranch(t, "newbranch", "origin/master", true, true, "I0123456789abcdef0123456789abcdef", "My change line.")
+
+ t.Logf("on dev.branch")
+ trun(t, gt.client, "git", "checkout", "-t", "-b", "dev.branch", "origin/dev.branch")
+ checkCurrentBranch(t, "dev.branch", "origin/dev.branch", false, false, "", "")
+
+ t.Logf("on newdev")
+ trun(t, gt.client, "git", "checkout", "-t", "-b", "newdev", "origin/dev.branch")
+ checkCurrentBranch(t, "newdev", "origin/dev.branch", true, false, "", "")
+
+ t.Logf("making change")
+ 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.")
+}
+
+func checkCurrentBranch(t *testing.T, name, origin string, isLocal, hasPending bool, changeID, subject string) {
+ b := CurrentBranch()
+ if b.Name != name {
+ t.Errorf("b.Name = %q, want %q", b.Name, name)
+ }
+ if x := b.OriginBranch(); x != origin {
+ t.Errorf("b.OriginBranch() = %q, want %q", x, origin)
+ }
+ if x := b.IsLocalOnly(); x != isLocal {
+ t.Errorf("b.IsLocalOnly() = %v, want %v", x, isLocal)
+ }
+ if x := b.HasPendingCommit(); x != hasPending {
+ t.Errorf("b.HasPendingCommit() = %v, want %v", x, isLocal)
+ }
+ if x := b.ChangeID(); x != changeID {
+ t.Errorf("b.ChangeID() = %q, want %q", x, changeID)
+ }
+ if x := b.Subject(); x != subject {
+ t.Errorf("b.Subject() = %q, want %q", x, subject)
+ }
+}
diff --git a/git-review/change.go b/git-review/change.go
index e8f5300..4414f37 100644
--- a/git-review/change.go
+++ b/git-review/change.go
@@ -7,6 +7,7 @@
import (
"fmt"
"os"
+ "strings"
)
func change(args []string) {
@@ -18,27 +19,27 @@
}
// Checkout or create branch, if specified.
- checkedOut := false
- if branch := flags.Arg(0); branch != "" {
- checkoutOrCreate(branch)
- checkedOut = true
+ target := flags.Arg(0)
+ if target != "" {
+ checkoutOrCreate(target)
}
// Create or amend change commit.
- branch := CurrentBranch()
- if branch.Name == "master" {
- if checkedOut {
+ b := CurrentBranch()
+ if !b.IsLocalOnly() {
+ if target != "" {
// Permit "review change master".
return
}
- dief("can't commit to master branch (use '%s change branchname').", os.Args[0])
+ dief("can't commit to %s branch (use '%s change branchname').", b.Name, os.Args[0])
}
- if branch.ChangeID == "" {
+
+ if b.ChangeID() == "" {
// No change commit on this branch, create one.
commitChanges(false)
return
}
- if checkedOut {
+ if target != "" {
// If we switched to an existing branch, don't amend the
// commit. (The user can run 'review change' to do that.)
return
@@ -47,32 +48,60 @@
commitChanges(true)
}
+var testCommitMsg string
+
func commitChanges(amend bool) {
- if !hasStagedChanges() {
+ if !HasStagedChanges() {
printf("no staged changes. Did you forget to 'git add'?")
}
args := []string{"commit", "-q", "--allow-empty"}
if amend {
args = append(args, "--amend")
}
+ if testCommitMsg != "" {
+ args = append(args, "-m", testCommitMsg)
+ }
run("git", args...)
printf("change updated.")
}
-func checkoutOrCreate(branch string) {
- // If branch exists, check it out.
- for _, b := range localBranches() {
- if b == branch {
- run("git", "checkout", "-q", branch)
- printf("changed to branch %v.", branch)
+func checkoutOrCreate(target string) {
+ // If local branch exists, check it out.
+ for _, b := range LocalBranches() {
+ if b.Name == target {
+ run("git", "checkout", "-q", target)
+ printf("changed to branch %v.", target)
return
}
}
- // If it doesn't exist, create a new branch.
- if currentBranchName() != "master" {
- dief("can't create a new branch from non-master branch.")
+ // If origin branch exists, create local branch tracking it.
+ for _, name := range OriginBranches() {
+ if name == "origin/"+target {
+ run("git", "checkout", "-q", "-t", "-b", target, name)
+ printf("created branch %v tracking %s.", target, name)
+ return
+ }
}
- run("git", "checkout", "-q", "-b", branch)
- printf("changed to new branch %v.", branch)
+
+ // Otherwise, this is a request to create a local work branch.
+ // Check for reserved names. We take everything with a dot.
+ if strings.Contains(target, ".") {
+ dief("invalid branch name %v: branch names with dots are reserved for git-review.", target)
+ }
+
+ // If the current branch has a pending commit, building
+ // on top of it will not help. Don't allow that.
+ // Otherwise, inherit HEAD and upstream from the current branch.
+ b := CurrentBranch()
+ if b.HasPendingCommit() {
+ dief("cannot branch from work branch; change back to %v first.", strings.TrimPrefix(b.OriginBranch(), "origin/"))
+ }
+
+ origin := b.OriginBranch()
+
+ // NOTE: This is different from git checkout -q -t -b branch. It does not move HEAD.
+ run("git", "checkout", "-q", "-b", target)
+ run("git", "branch", "--set-upstream-to", origin)
+ printf("created branch %v tracking %s.", target, origin)
}
diff --git a/git-review/change_test.go b/git-review/change_test.go
new file mode 100644
index 0000000..811a684
--- /dev/null
+++ b/git-review/change_test.go
@@ -0,0 +1,31 @@
+// Copyright 2014 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import "testing"
+
+func TestChange(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ t.Logf("master -> master")
+ testMain(t, "change", "master")
+ testRan(t, "git checkout -q master")
+
+ testCommitMsg = "my commit msg"
+ t.Logf("master -> work")
+ testMain(t, "change", "work")
+ testRan(t, "git checkout -q -b work",
+ "git branch --set-upstream-to origin/master",
+ "git commit -q --allow-empty -m my commit msg")
+
+ t.Logf("work -> master")
+ testMain(t, "change", "master")
+ testRan(t, "git checkout -q master")
+
+ t.Logf("master -> dev.branch")
+ testMain(t, "change", "dev.branch")
+ testRan(t, "git checkout -q -t -b dev.branch origin/dev.branch")
+}
diff --git a/git-review/mail.go b/git-review/mail.go
index 5cb6742..2dd03e2 100644
--- a/git-review/mail.go
+++ b/git-review/mail.go
@@ -27,20 +27,17 @@
os.Exit(2)
}
- branch := CurrentBranch()
- if branch.Name == "master" {
- dief("on master branch; can't mail.")
- }
- if branch.ChangeID == "" {
+ b := CurrentBranch()
+ if b.ChangeID() == "" {
dief("no pending change; can't mail.")
}
if *diff {
- run("git", "diff", "master..HEAD")
+ run("git", "diff", "HEAD^..HEAD")
return
}
- if !*force && hasStagedChanges() {
+ if !*force && HasStagedChanges() {
dief("there are staged changes; aborting.\n" +
"Use 'review change' to include them or 'review mail -f' to force it.")
}
diff --git a/git-review/pending.go b/git-review/pending.go
index 3e987e8..88205fa 100644
--- a/git-review/pending.go
+++ b/git-review/pending.go
@@ -10,19 +10,18 @@
expectZeroArgs(args, "pending")
// TODO(adg): implement -r
- current := currentBranchName()
- for _, branch := range localBranches() {
+ current := CurrentBranch().Name
+ for _, branch := range LocalBranches() {
p := " "
- if branch == current {
+ if branch.Name == current {
p = "* "
}
- pending := hasPendingCommit(branch)
- if branch == current || pending {
- sub := "(no pending change)"
- if pending {
- sub = commitSubject(branch)
- }
- fmt.Printf("%v%v: %v\n", p, branch, sub)
+ pending := branch.HasPendingCommit()
+ if pending {
+ fmt.Printf("%v%v: %v\n", p, branch.Name, branch.Subject())
+ } else if branch.Name == current {
+ // Nothing pending but print the line to show where we are.
+ fmt.Printf("%v%v: (no pending change)\n", p, branch.Name)
}
}
}
diff --git a/git-review/revert.go b/git-review/revert.go
deleted file mode 100644
index 1a8a334..0000000
--- a/git-review/revert.go
+++ /dev/null
@@ -1,29 +0,0 @@
-// Copyright 2014 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package main
-
-import (
- "fmt"
- "os"
-)
-
-func revert(args []string) {
- flags.Parse(args)
- files := flags.Args()
- if len(files) == 0 {
- fmt.Fprintf(os.Stderr, "Usage: %s %s revert files...\n", os.Args[0], globalFlags)
- os.Exit(2)
- }
- branch := CurrentBranch()
- if branch.Name == "master" {
- dief("on master branch; can't revert.")
- }
- if branch.ChangeID == "" {
- dief("no pending change; can't revert.")
- }
- // TODO(adg): make this work correctly before hooking it up
- run("git", append([]string{"checkout", "HEAD^"}, files...)...)
- run("git", append([]string{"add"}, files...)...)
-}
diff --git a/git-review/review.go b/git-review/review.go
index e2ddc4f..27cc65c 100644
--- a/git-review/review.go
+++ b/git-review/review.go
@@ -55,9 +55,16 @@
out that branch (creating it if it does not exist).
(Does not amend the existing commit when switching branches.)
- pending [-r]
- Show local branches and their head commits.
- If -r is specified, show additional information from Gerrit.
+ gofmt
+ TBD
+
+ help
+ Show this help text.
+
+ hooks
+ Install Git commit hooks for Gerrit and gofmt.
+ Every other operation except help also does this, if they are not
+ already installed.
mail [-f] [-r reviewer,...] [-cc mail,...]
Upload change commit to the code review server and send mail
@@ -67,22 +74,23 @@
mail -diff
Show the changes but do not send mail or upload.
+ pending [-r]
+ Show local branches and their head commits.
+ If -r is specified, show additional information from Gerrit.
+
sync
Fetch changes from the remote repository and merge them into
the current branch, rebasing the change commit on top of them.
- revert files...
- Revert the specified files to their state before the change
- commit. (Be careful! This will discard your changes!)
-
- gofmt
- TBD
`
func main() {
if len(os.Args) < 2 {
flags.Usage()
+ if dieTrap != nil {
+ dieTrap()
+ }
os.Exit(2)
}
command, args := os.Args[1], os.Args[2:]
@@ -97,16 +105,16 @@
switch command {
case "change", "c":
change(args)
- case "pending", "p":
- pending(args)
- case "mail", "m":
- mail(args)
- case "sync", "s":
- doSync(args)
- case "revert":
- dief("revert not implemented")
case "gofmt":
dief("gofmt not implemented")
+ case "hooks":
+ // done - installHook already ran
+ case "mail", "m":
+ mail(args)
+ case "pending", "p":
+ pending(args)
+ case "sync", "s":
+ doSync(args)
default:
flags.Usage()
}
@@ -131,6 +139,8 @@
}
}
+var runLog []string
+
func runErr(command string, args ...string) error {
if *verbose || *noRun {
fmt.Fprintln(os.Stderr, commandString(command, args))
@@ -138,6 +148,9 @@
if *noRun {
return nil
}
+ if runLog != nil {
+ runLog = append(runLog, strings.TrimSpace(command+" "+strings.Join(args, " ")))
+ }
cmd := exec.Command(command, args...)
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
@@ -176,8 +189,13 @@
return strings.Join(append([]string{command}, args...), " ")
}
+var dieTrap func()
+
func dief(format string, args ...interface{}) {
printf(format, args...)
+ if dieTrap != nil {
+ dieTrap()
+ }
os.Exit(1)
}
@@ -188,5 +206,5 @@
}
func printf(format string, args ...interface{}) {
- fmt.Fprintf(os.Stderr, os.Args[0]+": "+format+"\n", args...)
+ fmt.Fprintf(os.Stderr, "%s: %s\n", os.Args[0], fmt.Sprintf(format, args...))
}
diff --git a/git-review/sync.go b/git-review/sync.go
index 4914da3..f2a7b91 100644
--- a/git-review/sync.go
+++ b/git-review/sync.go
@@ -10,25 +10,26 @@
// Fetch remote changes.
run("git", "fetch", "-q")
- // If we're on master or there's no pending change, just fast-forward.
- branch := CurrentBranch()
- if branch.Name == "master" || branch.ChangeID == "" {
- run("git", "merge", "-q", "--ff-only", "origin/master")
+ // If there's no pending change, just fast-forward.
+ b := CurrentBranch()
+ if !b.HasPendingCommit() {
+ run("git", "merge", "-q", "--ff-only", b.OriginBranch())
return
}
// Don't sync with staged changes.
// TODO(adg): should we handle unstaged changes also?
- if hasStagedChanges() {
- dief("you have staged changes. Run 'review change' before sync.")
+ if HasStagedChanges() {
+ dief("run 'git-review change' to commit staged changes before sync.")
}
- // Sync current branch to master.
- run("git", "rebase", "-q", "origin/master")
+ // Sync current branch to origin.
+ id := b.ChangeID()
+ run("git", "rebase", "-q", b.OriginBranch())
// If the change commit has been submitted,
// roll back change leaving any changes unstaged.
- if branch.Submitted() && hasPendingCommit(branch.Name) {
+ if b.Submitted(id) && b.HasPendingCommit() {
run("git", "reset", "HEAD^")
}
}
diff --git a/git-review/util_test.go b/git-review/util_test.go
new file mode 100644
index 0000000..2834563
--- /dev/null
+++ b/git-review/util_test.go
@@ -0,0 +1,121 @@
+// Copyright 2014 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+ "io/ioutil"
+ "os"
+ "os/exec"
+ "path/filepath"
+ "reflect"
+ "strings"
+ "testing"
+)
+
+type gitTest struct {
+ pwd string // current directory before test
+ tmpdir string // temporary directory holding repos
+ server string // server repo root
+ client string // client repo root
+}
+
+func (gt *gitTest) done() {
+ os.RemoveAll(gt.tmpdir)
+ os.Chdir(gt.pwd)
+}
+
+func newGitTest(t *testing.T) *gitTest {
+ tmpdir, err := ioutil.TempDir("", "git-review-test")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ server := tmpdir + "/git-origin"
+
+ mkdir(t, server)
+ write(t, server+"/file", "this is master")
+ trun(t, server, "git", "init", ".")
+ trun(t, server, "git", "add", "file")
+ trun(t, server, "git", "commit", "-m", "on master")
+
+ 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)
+ }
+
+ client := tmpdir + "/git-client"
+ mkdir(t, client)
+ trun(t, client, "git", "clone", server, ".")
+ trun(t, client, "git", "config", "core.editor", "false")
+ pwd, err := os.Getwd()
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if err := os.Chdir(client); err != nil {
+ t.Fatal(err)
+ }
+
+ gt := &gitTest{
+ pwd: pwd,
+ tmpdir: tmpdir,
+ server: server,
+ client: client,
+ }
+
+ return gt
+}
+
+func mkdir(t *testing.T, dir string) {
+ if err := os.Mkdir(dir, 0777); err != nil {
+ t.Fatal(err)
+ }
+}
+
+func write(t *testing.T, file, data string) {
+ if err := ioutil.WriteFile(file, []byte(data), 0666); err != nil {
+ t.Fatal(err)
+ }
+}
+
+func trun(t *testing.T, dir string, cmdline ...string) {
+ cmd := exec.Command(cmdline[0], cmdline[1:]...)
+ cmd.Dir = dir
+ out, err := cmd.CombinedOutput()
+ if err != nil {
+ t.Fatalf("in %s/, ran %s: %v\n%s", filepath.Base(dir), cmdline, err, out)
+ }
+}
+
+func testMain(t *testing.T, args ...string) {
+ t.Logf("git-review %s", strings.Join(args, " "))
+ runLog = []string{} // non-nil, to trigger saving of commands
+
+ defer func() {
+ if err := recover(); err != nil {
+ runLog = nil
+ dieTrap = nil
+ t.Fatalf("panic: %v", err)
+ }
+ }()
+
+ dieTrap = func() {
+ panic("died")
+ }
+
+ os.Args = append([]string{"git-review"}, args...)
+ main()
+
+ dieTrap = nil
+}
+
+func testRan(t *testing.T, cmds ...string) {
+ if !reflect.DeepEqual(runLog, cmds) {
+ t.Errorf("ran:\n%s", strings.Join(runLog, "\n"))
+ t.Errorf("wanted:\n%s", strings.Join(cmds, "\n"))
+ }
+}
