commit 38b40c0e2b9e3a62d9a99d62fa70595a5f68e6c9
author Joshua Sing <joshua@hypera.dev> Sun Nov 19 02:19:34 2023 +1100
committer Gopher Robot <gobot@golang.org> Thu Feb 29 18:41:27 2024 +0000
tree bb7a19b85a177d261ff47a4fc2be127fac53389c
parent 0e3f2836ef3e230ced34f5550aa937610e076531

git-codereview: improve haveGerritInternal

Use url.Parse and check URL components rather than performing imprecise
string matching. This addresses a bug where git-codereview does not work
when the Git origin ends with a forward slash.

Note that the check for 'github.com' has been removed since the test for
'.googlesource.com' already excludes it.

Change-Id: I083bccdbacf2152cbfddd2407fb20afa47c8e91e
Reviewed-on: https://go-review.googlesource.com/c/review/+/543495
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Joel Sing <joel@sing.id.au>

git-codereview/config.go

diff --git a/git-codereview/config.go b/git-codereview/config.go
index debeb5e..cc9286c 100644
--- a/git-codereview/config.go
+++ b/git-codereview/config.go
@@ -6,6 +6,7 @@
 
 import (
 	"fmt"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
@@ -59,20 +60,18 @@
 	if gerrit != "" {
 		return true
 	}
-	if strings.Contains(origin, "github.com") {
+
+	u, err := url.Parse(origin)
+	if err != nil {
 		return false
 	}
-	if strings.HasPrefix(origin, "sso://") || strings.HasPrefix(origin, "rpc://") {
+	if u.Scheme == "sso" || u.Scheme == "rpc" {
 		return true
 	}
-	if !strings.Contains(origin, "https://") {
+	if u.Scheme != "https" {
 		return false
 	}
-	if strings.Count(origin, "/") != 3 {
-		return false
-	}
-	host := origin[:strings.LastIndex(origin, "/")]
-	return strings.HasSuffix(host, ".googlesource.com")
+	return strings.HasSuffix(u.Host, ".googlesource.com")
 }
 
 func haveGitHub() bool {

git-codereview/config_test.go

diff --git a/git-codereview/config_test.go b/git-codereview/config_test.go
index 249b7df..a862788 100644
--- a/git-codereview/config_test.go
+++ b/git-codereview/config_test.go
@@ -32,3 +32,38 @@
 		}
 	}
 }
+
+func TestHaveGerritInternal(t *testing.T) {
+	tests := []struct {
+		gerrit string
+		origin string
+		want   bool
+	}{
+		{gerrit: "off", want: false},
+		{gerrit: "on", want: true},
+		{origin: "invalid url", want: false},
+		{origin: "https://github.com/golang/go", want: false},
+		{origin: "http://github.com/golang/go", want: false},
+		{origin: "git@github.com:golang/go", want: false},
+		{origin: "git@github.com:golang/go.git", want: false},
+		{origin: "git@github.com:/golang/go", want: false},
+		{origin: "git@github.com:/golang/go.git", want: false},
+		{origin: "ssh://git@github.com/golang/go", want: false},
+		{origin: "ssh://git@github.com/golang/go.git", want: false},
+		{origin: "git+ssh://git@github.com/golang/go", want: false},
+		{origin: "git+ssh://git@github.com/golang/go.git", want: false},
+		{origin: "git://github.com/golang/go", want: false},
+		{origin: "git://github.com/golang/go.git", want: false},
+		{origin: "sso://go/tools", want: true}, // Google-internal
+		{origin: "rpc://go/tools", want: true}, // Google-internal
+		{origin: "http://go.googlesource.com/sys", want: false},
+		{origin: "https://go.googlesource.com/review", want: true},
+		{origin: "https://go.googlesource.com/review/", want: true},
+	}
+
+	for _, test := range tests {
+		if got := haveGerritInternal(test.gerrit, test.origin); got != test.want {
+			t.Errorf("haveGerritInternal(%q, %q) = %t, want %t", test.gerrit, test.origin, got, test.want)
+		}
+	}
+}