From 38b40c0e2b9e3a62d9a99d62fa70595a5f68e6c9 Mon Sep 17 00:00:00 2001 From: Joshua Sing Date: Sun, 19 Nov 2023 02:19:34 +1100 Subject: 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 LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov TryBot-Result: Gopher Robot Reviewed-by: Dmitri Shuralyov Auto-Submit: Dmitri Shuralyov Run-TryBot: Joel Sing --- git-codereview/config.go | 15 +++++++-------- git-codereview/config_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) 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 @@ package main import ( "fmt" + "net/url" "os" "path/filepath" "strings" @@ -59,20 +60,18 @@ func haveGerritInternal(gerrit, origin string) bool { 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://") { - return false - } - if strings.Count(origin, "/") != 3 { + if u.Scheme != "https" { return false } - host := origin[:strings.LastIndex(origin, "/")] - return strings.HasSuffix(host, ".googlesource.com") + return strings.HasSuffix(u.Host, ".googlesource.com") } func haveGitHub() bool { 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 TestParseConfig(t *testing.T) { } } } + +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) + } + } +} -- cgit v1.3