git-codereview: make git change handle GitHub PRs

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

4 files changed, 58 insertions, 19 deletions

diff --git a/git-codereview/change.go b/git-codereview/change.go
index 6f76df1..5320d9d 100644
--- a/git-codereview/change.go
+++ b/git-codereview/change.go
@@ -103,16 +103,24 @@ func commitChanges(amend bool) {
 }
 
 func checkoutOrCreate(target string) {
-	// If it's a valid Gerrit number, checkout the CL.
+	// If it's a valid Gerrit number CL or CL/PS or GitHub pull request number PR,
+	// checkout the CL or PR.
 	cl, ps, isCL := parseCL(target)
 	if isCL {
-		if !haveGerrit() {
+		what := "CL"
+		if !haveGerrit() && haveGitHub() {
+			what = "PR"
+			if ps != "" {
+				dief("change PR syntax is NNN not NNN.PP")
+			}
+		}
+		if what == "CL" && !haveGerrit() {
 			dief("cannot change to a CL without gerrit")
 		}
 		if HasStagedChanges() || HasUnstagedChanges() {
-			dief("cannot change to a CL with uncommitted work")
+			dief("cannot change to a %s with uncommitted work", what)
 		}
-		checkoutCL(cl, ps)
+		checkoutCL(what, cl, ps)
 		return
 	}
 
@@ -176,8 +184,8 @@ func checkoutOrCreate(target string) {
 }
 
 // Checkout the patch set of the given CL. When patch set is empty, use the latest.
-func checkoutCL(cl, ps string) {
-	if ps == "" {
+func checkoutCL(what, cl, ps string) {
+	if what == "CL" && ps == "" {
 		change, err := readGerritChange(cl + "?o=CURRENT_REVISION")
 		if err != nil {
 			dief("cannot change to CL %s: %v", cl, err)
@@ -189,28 +197,36 @@ func checkoutCL(cl, ps string) {
 		ps = strconv.Itoa(rev.Number)
 	}
 
-	var group string
-	if len(cl) > 1 {
-		group = cl[len(cl)-2:]
+	var ref string
+	if what == "CL" {
+		var group string
+		if len(cl) > 1 {
+			group = cl[len(cl)-2:]
+		} else {
+			group = "0" + cl
+		}
+		cl = fmt.Sprintf("%s/%s", cl, ps)
+		ref = fmt.Sprintf("refs/changes/%s/%s", group, cl)
 	} else {
-		group = "0" + cl
+		ref = fmt.Sprintf("pull/%s/head", cl)
 	}
-	ref := fmt.Sprintf("refs/changes/%s/%s/%s", group, cl, ps)
-
 	err := runErr("git", "fetch", "-q", "origin", ref)
 	if err != nil {
-		dief("cannot change to CL %s/%s: %v", cl, ps, err)
+		dief("cannot change to %v %s: %v", what, cl, err)
 	}
 	err = runErr("git", "checkout", "-q", "FETCH_HEAD")
 	if err != nil {
-		dief("cannot change to CL %s/%s: %v", cl, ps, err)
+		dief("cannot change to %s %s: %v", what, cl, err)
+	}
+	if *noRun {
+		return
 	}
 	subject, err := trimErr(cmdOutputErr("git", "log", "--format=%s", "-1"))
 	if err != nil {
-		printf("changed to CL %s/%s.", cl, ps)
+		printf("changed to %s %s.", what, cl)
 		dief("cannot read change subject from git: %v", err)
 	}
-	printf("changed to CL %s/%s.\n\t%s", cl, ps, subject)
+	printf("changed to %s %s.\n\t%s", what, cl, subject)
 }
 
 var parseCLRE = regexp.MustCompile(`^([0-9]+)(?:/([0-9]+))?$`)
diff --git a/git-codereview/change_test.go b/git-codereview/change_test.go
index d3b37b0..3e3ea30 100644
--- a/git-codereview/change_test.go
+++ b/git-codereview/change_test.go
@@ -160,6 +160,21 @@ func TestChangeCL(t *testing.T) {
 	checkChangeCL("100/1", "refs/changes/00/100/1", hash1)
 	checkChangeCL("100/2", "refs/changes/00/100/2", hash2)
 	checkChangeCL("100", "refs/changes/00/100/3", hash1)
+
+	// turn off gerrit, make it look like we are on GitHub
+	write(t, gt.server+"/codereview.cfg", "nothing: here", 0644)
+	trun(t, gt.server, "git", "add", "codereview.cfg")
+	trun(t, gt.server, "git", "commit", "-m", "new codereview.cfg on main")
+	testMain(t, "change", "main")
+	trun(t, gt.client, "git", "pull", "-r")
+	trun(t, gt.client, "git", "remote", "set-url", "origin", "https://github.com/google/not-a-project")
+
+	testMain(t, "change", "-n", "123")
+	testNoStdout(t)
+	testPrintedStderr(t,
+		"git fetch -q origin pull/123/head",
+		"git checkout -q FETCH_HEAD",
+	)
 }
 
 func TestChangeWithMessage(t *testing.T) {
diff --git a/git-codereview/config.go b/git-codereview/config.go
index d7a802e..4e0a565 100644
--- a/git-codereview/config.go
+++ b/git-codereview/config.go
@@ -75,6 +75,11 @@ func haveGerritInternal(gerrit, origin string) bool {
 	return strings.HasSuffix(host, ".googlesource.com")
 }
 
+func haveGitHub() bool {
+	origin := trim(cmdOutput("git", "config", "remote.origin.url"))
+	return strings.Contains(origin, "github.com")
+}
+
 func parseConfig(raw string) (map[string]string, error) {
 	cfg := make(map[string]string)
 	for _, line := range nonBlankLines(raw) {
diff --git a/git-codereview/doc.go b/git-codereview/doc.go
index 4232869..a72b6db 100644
--- a/git-codereview/doc.go
+++ b/git-codereview/doc.go
@@ -154,10 +154,13 @@ The -m option specifies a commit message and skips the editor prompt. This
 option is only useful when creating commits (e.g. if there are unstaged
 changes). If a commit already exists, it is overwritten. If -q is also
 present, -q will be ignored.
-997As a special case, if branchname is a decimal CL number, such as 987, the change
+
+As a special case, if branchname is a decimal CL number, such as 987, the change
 command downloads the latest patch set of that CL from the server and switches to it.
-A specific patch set can be requested by adding a decimal point: 987.2 for patch set 2
-of CL 987.
+A specific patch set P can be requested by adding /P: 987.2 for patch set 2 of CL 987.
+If the origin server is GitHub instead of Gerrit, then the number is
+treated a GitHub pull request number, and the change command downloads the latest
+version of that pull request. In this case, the /P suffix is disallowed.
 
 Gofmt