Refactor

commands/pull_request.go

@@ -89,40 +89,21 @@ func pullRequest(cmd *Command, args *Args) {
 		base, head           string
 		force, explicitOwner bool
 	)
+
 	force = flagPullRequestForce
+
 	if flagPullRequestBase != "" {
-		if strings.Contains(flagPullRequestBase, ":") {
-			split := strings.SplitN(flagPullRequestBase, ":", 2)
-			base = split[1]
-			var name string
-			if !strings.Contains(split[0], "/") {
-				name = baseProject.Name
-			}
-			baseProject = github.NewProject(split[0], name, baseProject.Host)
-		} else {
-			base = flagPullRequestBase
-		}
+		baseProject, base = parsePullRequestProject(baseProject, flagPullRequestBase)
 	}
 
 	if flagPullRequestHead != "" {
-		if strings.Contains(flagPullRequestHead, ":") {
-			split := strings.SplitN(flagPullRequestHead, ":", 2)
-			head = split[1]
-			headProject = github.NewProject(split[0], headProject.Name, headProject.Host)
-			explicitOwner = true
-		} else {
-			head = flagPullRequestHead
-		}
+		headProject, head = parsePullRequestProject(headProject, flagPullRequestHead)
+		explicitOwner = strings.Contains(flagPullRequestHead, ":")
 	}
 
 	if args.ParamsSize() == 1 {
 		arg := args.RemoveParam(0)
-		u, e := github.ParseURL(arg)
-		r := regexp.MustCompile(`^issues\/(\d+)`)
-		p := u.ProjectPath()
-		if e == nil && r.MatchString(p) {
-			flagPullRequestIssue = r.FindStringSubmatch(p)[1]
-		}
+		flagPullRequestIssue = parsePullRequestIssueNumber(arg)
 	}
 
 	if base == "" {
@@ -131,37 +112,36 @@ func pullRequest(cmd *Command, args *Args) {
 		base = masterBranch.ShortName()
 	}
 
-	trackedBranch, tberr := currentBranch.Upstream()
+	trackedBranch, _ := currentBranch.Upstream()
 	if head == "" {
-		if err == nil {
-			if trackedBranch.IsRemote() {
-				if reflect.DeepEqual(baseProject, headProject) && base == trackedBranch.ShortName() {
-					e := fmt.Errorf(`Aborted: head branch is the same as base ("%s")`, base)
-					e = fmt.Errorf("%s\n(use `-h <branch>` to specify an explicit pull request head)", e)
-					utils.Check(e)
-				}
-			} else {
-				// the current branch tracking another branch
-				// pretend there's no upstream at all
-				tberr = fmt.Errorf("No upstream found for current branch")
+		if trackedBranch != nil && trackedBranch.IsRemote() {
+			if reflect.DeepEqual(baseProject, headProject) && base == trackedBranch.ShortName() {
+				e := fmt.Errorf(`Aborted: head branch is the same as base ("%s")`, base)
+				e = fmt.Errorf("%s\n(use `-h <branch>` to specify an explicit pull request head)", e)
+				utils.Check(e)
 			}
+		} else {
+			// the current branch tracking another branch
+			// pretend there's no upstream at all
+			trackedBranch = nil
 		}
 
-		if tberr == nil {
-			head = trackedBranch.ShortName()
-		} else {
+		if trackedBranch == nil {
 			head = currentBranch.ShortName()
+		} else {
+			head = trackedBranch.ShortName()
 		}
 	}
 
 	client := github.NewClient(baseProject)
 
 	// when no tracking, assume remote branch is published under active user's fork
-	if tberr != nil && !explicitOwner && client.Credentials.User != headProject.Owner {
+	if trackedBranch == nil && !explicitOwner && client.Credentials.User != headProject.Owner {
 		headProject = github.NewProject("", headProject.Name, headProject.Host)
 	}
 
 	var title, body string
+
 	if flagPullRequestMessage != "" {
 		title, body = readMsg(flagPullRequestMessage)
 	}
@@ -184,7 +164,7 @@ func pullRequest(cmd *Command, args *Args) {
 	fullHead := fmt.Sprintf("%s:%s", headProject.Owner, head)
 
 	commits, _ := git.RefList(base, head)
-	if !force && tberr == nil && len(commits) > 0 {
+	if !force && trackedBranch != nil && len(commits) > 0 {
 		err = fmt.Errorf("Aborted: %d commits are not yet pushed to %s", len(commits), trackedBranch.LongName())
 		err = fmt.Errorf("%s\n(use `-f` to force submit a pull request anyway)", err)
 		utils.Check(err)
@@ -374,3 +354,35 @@ func readMsg(msg string) (title, body string) {
 
 	return
 }
+
+func parsePullRequestProject(context *github.Project, s string) (p *github.Project, ref string) {
+	p = context
+	ref = s
+
+	if strings.Contains(s, ":") {
+		split := strings.SplitN(s, ":", 2)
+		ref = split[1]
+		var name string
+		if !strings.Contains(split[0], "/") {
+			name = context.Name
+		}
+		p = github.NewProject(split[0], name, context.Host)
+	}
+
+	return
+}
+
+func parsePullRequestIssueNumber(url string) string {
+	u, e := github.ParseURL(url)
+	if e != nil {
+		return ""
+	}
+
+	r := regexp.MustCompile(`^issues\/(\d+)`)
+	p := u.ProjectPath()
+	if r.MatchString(p) {
+		return r.FindStringSubmatch(p)[1]
+	}
+
+	return ""
+}

commands/pull_request_test.go

@@ -3,6 +3,7 @@ package commands
 import (
 	"bufio"
 	"github.com/bmizerany/assert"
+	"github.com/jingweno/gh/github"
 	"strings"
 	"testing"
 )
@@ -22,3 +23,28 @@ A body continues
 	assert.Equal(t, "A title A title continues", title)
 	assert.Equal(t, "A body\nA body continues", body)
 }
+
+func TestParsePullRequestProject(t *testing.T) {
+	c := &github.Project{Host: "github.com", Owner: "jingweno", Name: "gh"}
+
+	s := "develop"
+	p, ref := parsePullRequestProject(c, s)
+	assert.Equal(t, "develop", ref)
+	assert.Equal(t, "github.com", p.Host)
+	assert.Equal(t, "jingweno", p.Owner)
+	assert.Equal(t, "gh", p.Name)
+
+	s = "mojombo:develop"
+	p, ref = parsePullRequestProject(c, s)
+	assert.Equal(t, "develop", ref)
+	assert.Equal(t, "github.com", p.Host)
+	assert.Equal(t, "mojombo", p.Owner)
+	assert.Equal(t, "gh", p.Name)
+
+	s = "mojombo/jekyll:develop"
+	p, ref = parsePullRequestProject(c, s)
+	assert.Equal(t, "develop", ref)
+	assert.Equal(t, "github.com", p.Host)
+	assert.Equal(t, "mojombo", p.Owner)
+	assert.Equal(t, "jekyll", p.Name)
+}

github/branch.go

@@ -7,38 +7,40 @@ import (
 	"strings"
 )
 
-type Branch string
+type Branch struct {
+	Name string
+}
 
-func (b Branch) ShortName() string {
+func (b *Branch) ShortName() string {
 	reg := regexp.MustCompile("^refs/(remotes/)?.+?/")
-	return reg.ReplaceAllString(string(b), "")
+	return reg.ReplaceAllString(b.Name, "")
 }
 
-func (b Branch) LongName() string {
+func (b *Branch) LongName() string {
 	reg := regexp.MustCompile("refs/(remotes/)?")
-	return reg.ReplaceAllString(string(b), "")
+	return reg.ReplaceAllString(b.Name, "")
 }
 
-func (b Branch) Upstream() (u Branch, err error) {
+func (b *Branch) Upstream() (u *Branch, err error) {
 	name, err := git.SymbolicFullName(fmt.Sprintf("%s@{upstream}", b.ShortName()))
 	if err != nil {
 		return
 	}
 
-	u = Branch(name)
+	u = &Branch{name}
 
 	return
 }
 
-func (b Branch) RemoteName() string {
+func (b *Branch) RemoteName() (n string) {
 	reg := regexp.MustCompile("^refs/remotes/([^/]+)")
-	if reg.MatchString(string(b)) {
-		return reg.FindStringSubmatch(string(b))[1]
+	if reg.MatchString(b.Name) {
+		n = reg.FindStringSubmatch(b.Name)[1]
 	}
 
-	return ""
+	return
 }
 
-func (b Branch) IsRemote() bool {
-	return strings.HasPrefix(string(b), "refs/remotes")
+func (b *Branch) IsRemote() bool {
+	return strings.HasPrefix(b.Name, "refs/remotes")
 }

github/branch_test.go

@@ -6,24 +6,24 @@ import (
 )
 
 func TestBranch_ShortName(t *testing.T) {
-	b := Branch("refs/heads/master")
+	b := Branch{"refs/heads/master"}
 	assert.Equal(t, "master", b.ShortName())
 }
 
 func TestBranch_LongName(t *testing.T) {
-	b := Branch("refs/heads/master")
+	b := Branch{"refs/heads/master"}
 	assert.Equal(t, "heads/master", b.LongName())
 
-	b = Branch("refs/remotes/origin/master")
+	b = Branch{"refs/remotes/origin/master"}
 	assert.Equal(t, "origin/master", b.LongName())
 }
 
 func TestBranch_RemoveName(t *testing.T) {
-	b := Branch("refs/remotes/origin/master")
+	b := Branch{"refs/remotes/origin/master"}
 	assert.Equal(t, "origin", b.RemoteName())
 }
 
 func TestBranch_IsRemote(t *testing.T) {
-	b := Branch("refs/remotes/origin/master")
+	b := Branch{"refs/remotes/origin/master"}
 	assert.T(t, b.IsRemote())
 }

github/github.go

@@ -8,9 +8,9 @@ import (
 )
 
 const (
-	GitHubHost    string = "github.com"
-	GitHubApiURL  string = "https://api.github.com"
-	OAuthAppURL   string = "http://owenou.com/gh"
+	GitHubHost   string = "github.com"
+	GitHubApiURL string = "https://api.github.com"
+	OAuthAppURL  string = "http://owenou.com/gh"
 )
 
 type GitHub struct {

github/project.go

@@ -62,7 +62,7 @@ func (p *Project) LocalRepoWith(base, head string) *Repo {
 		if err != nil {
 			utils.Check(fmt.Errorf("Aborted: not currently on any branch."))
 		}
-		head = Branch(headBranch).ShortName()
+		head = (&Branch{headBranch}).ShortName()
 	}
 
 	return &Repo{base, head, p}

github/repo.go

@@ -32,18 +32,18 @@ func (r *GitHubRepo) remotesByName(name string) (*git.Remote, error) {
 	return nil, fmt.Errorf("No git remote with name %s", name)
 }
 
-func (r *GitHubRepo) CurrentBranch() (branch Branch, err error) {
+func (r *GitHubRepo) CurrentBranch() (branch *Branch, err error) {
 	head, err := git.Head()
 	if err != nil {
 		err = fmt.Errorf("Aborted: not currently on any branch.")
 		return
 	}
 
-	branch = Branch(head)
+	branch = &Branch{head}
 	return
 }
 
-func (r *GitHubRepo) MasterBranch() (branch Branch, err error) {
+func (r *GitHubRepo) MasterBranch() (branch *Branch, err error) {
 	origin, err := r.remotesByName("origin")
 	if err != nil {
 		return
@@ -55,7 +55,7 @@ func (r *GitHubRepo) MasterBranch() (branch Branch, err error) {
 		err = nil
 	}
 
-	branch = Branch(name)
+	branch = &Branch{name}
 
 	return
 }