devapp: display subrepo that CL belongs to
This change largely reuses the logic to parse CL subjects used
on gochanges.org. There is no support for expanding curly brackets
like "image/{png,jpeg}", but this wasn't implemented here previously
either.
There is room for performance improvement by performing less
allocations, but performance is not a bottleneck for current
needs, so I am currently prioritizing adding support for edge
cases (unless performance proves to be a blocker).
For brevity, use x/subrepo instead of golang.org/x/subrepo form.
Omit displaying the path prefix in the title when displaying CLs
grouped under their full import path, to reduce stutter.
Fixes golang/go#30096
Change-Id: I957661724d1a26af4dfebea1eee20e6a7595c7b9
Reviewed-on: https://go-review.googlesource.com/c/161219
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/devapp/release.go b/devapp/release.go
index e8e7117..1b7f7ee 100644
--- a/devapp/release.go
+++ b/devapp/release.go
@@ -151,8 +151,9 @@
type gerritCL struct {
*maintner.GerritCL
- Closed bool
- Milestone string
+ NoPrefixTitle string // CL title without the directory prefix (e.g., "improve ListenAndServe" without leading "net/http: ").
+ Closed bool
+ Milestone string
}
// ReviewURL returns the code review address of cl.
@@ -195,6 +196,7 @@
}
var (
+ pkgs, title = ParsePrefixedChangeTitle(projectRoot(p), cl.Subject())
closed bool
closedVersion int32
milestone string
@@ -216,20 +218,20 @@
}
}
gcl := &gerritCL{
- GerritCL: cl,
- Closed: closed,
- Milestone: milestone,
+ GerritCL: cl,
+ NoPrefixTitle: title,
+ Closed: closed,
+ Milestone: milestone,
}
for _, r := range cl.GitHubIssueRefs {
issueToCLs[r.Number] = append(issueToCLs[r.Number], gcl)
}
- dirs := titleDirs(cl.Subject())
- if len(dirs) == 0 {
+ if len(pkgs) == 0 {
dirToCLs[""] = append(dirToCLs[""], gcl)
} else {
- for _, d := range dirs {
- dirToCLs[d] = append(dirToCLs[d], gcl)
+ for _, p := range pkgs {
+ dirToCLs[p] = append(dirToCLs[p], gcl)
}
}
return nil
@@ -295,6 +297,38 @@
s.data.release.dirty = false
}
+// projectRoot returns the import path corresponding to the repo root
+// of the Gerrit project p. For golang.org/x subrepos, the golang.org
+// part is omitted for previty.
+func projectRoot(p *maintner.GerritProject) string {
+ switch p.Server() {
+ case "go.googlesource.com":
+ switch subrepo := p.Project(); subrepo {
+ case "go":
+ // Main Go repo.
+ return ""
+ case "dl":
+ // dl is a special subrepo, there's no /x/ in its import path.
+ return "golang.org/dl"
+ case "gddo":
+ // There is no golang.org/x/gddo vanity import path, and
+ // the canonical import path for gddo is on GitHub.
+ return "github.com/golang/gddo"
+ default:
+ // For brevity, use x/subrepo rather than golang.org/x/subrepo.
+ return "x/" + subrepo
+ }
+ case "code.googlesource.com":
+ switch p.Project() {
+ case "gocloud":
+ return "cloud.google.com/go"
+ case "google-api-go-client":
+ return "google.golang.org/api"
+ }
+ }
+ return p.ServerSlashProject()
+}
+
// requires s.cMu be locked.
func (s *server) appendOpenIssues(dirToIssues map[string][]*maintner.GitHubIssue, issueToCLs map[int32][]*gerritCL) {
var issueDirs []string
diff --git a/devapp/templates/release.tmpl b/devapp/templates/release.tmpl
index 30c8fcd..3be5920 100644
--- a/devapp/templates/release.tmpl
+++ b/devapp/templates/release.tmpl
@@ -91,7 +91,7 @@
<span class="Item-num">
{{if $i}}⤷{{end}} <a href="{{.ReviewURL}}" target="_blank">CL {{.Number}}</a>
</span>
- <span class="Item-title">{{if $i}}⤷{{end}} {{.Subject}}</span>
+ <span class="Item-title">{{if $i}}⤷{{end}} {{.NoPrefixTitle}}</span>
</div>
{{end}}
{{end}}
diff --git a/devapp/title.go b/devapp/title.go
new file mode 100644
index 0000000..139a28d
--- /dev/null
+++ b/devapp/title.go
@@ -0,0 +1,56 @@
+// Copyright 2019 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 (
+ "path"
+ "strings"
+)
+
+// ParsePrefixedChangeTitle parses a prefixed change title.
+// It returns a list of paths from the prefix joined with root, and the remaining change title.
+// It does not try to verify whether each path is an existing Go package.
+//
+// Supported forms include:
+//
+// "root", "import/path: change title" -> ["root/import/path"], "change title"
+// "root", "path1, path2: change title" -> ["root/path1", "root/path2"], "change title" # Multiple comma-separated paths.
+//
+// If there's no path prefix (preceded by ": "), title is returned unmodified
+// with a paths list containing root:
+//
+// "root", "change title" -> ["root"], "change title"
+//
+// If there's a branch prefix in square brackets, title is returned with said prefix:
+//
+// "root", "[branch] path: change title" -> ["root/path"], "[branch] change title"
+//
+func ParsePrefixedChangeTitle(root, prefixedTitle string) (paths []string, title string) {
+ // Parse branch prefix in square brackets, if any.
+ // E.g., "[branch] path: change title" -> "[branch] ", "path: change title".
+ var branch string // "[branch] " or empty string.
+ if strings.HasPrefix(prefixedTitle, "[") {
+ if idx := strings.Index(prefixedTitle, "] "); idx != -1 {
+ branch, prefixedTitle = prefixedTitle[:idx+len("] ")], prefixedTitle[idx+len("] "):]
+ }
+ }
+
+ // Parse the rest of the prefixed change title.
+ // E.g., "path1, path2: change title" -> ["path1", "path2"], "change title".
+ idx := strings.Index(prefixedTitle, ": ")
+ if idx == -1 {
+ return []string{root}, branch + prefixedTitle
+ }
+ prefix, title := prefixedTitle[:idx], prefixedTitle[idx+len(": "):]
+ if strings.ContainsAny(prefix, "{}") {
+ // TODO: Parse "image/{png,jpeg}" as ["image/png", "image/jpeg"], maybe?
+ return []string{path.Join(root, strings.TrimSpace(prefix))}, branch + title
+ }
+ paths = strings.Split(prefix, ",")
+ for i := range paths {
+ paths[i] = path.Join(root, strings.TrimSpace(paths[i]))
+ }
+ return paths, branch + title
+}
diff --git a/devapp/title_test.go b/devapp/title_test.go
new file mode 100644
index 0000000..cd44c13
--- /dev/null
+++ b/devapp/title_test.go
@@ -0,0 +1,77 @@
+// Copyright 2019 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_test
+
+import (
+ "reflect"
+ "testing"
+
+ devapp "golang.org/x/build/devapp"
+)
+
+func TestParsePrefixedChangeTitle(t *testing.T) {
+ tests := []struct {
+ inRoot string
+ in string
+ wantPaths []string
+ wantTitle string
+ }{
+ {
+ in: "import/path: Change title.",
+ wantPaths: []string{"import/path"}, wantTitle: "Change title.",
+ },
+ {
+ inRoot: "root",
+ in: "import/path: Change title.",
+ wantPaths: []string{"root/import/path"}, wantTitle: "Change title.",
+ },
+ {
+ inRoot: "root",
+ in: "[release-branch.go1.11] import/path: Change title.",
+ wantPaths: []string{"root/import/path"}, wantTitle: "[release-branch.go1.11] Change title.",
+ },
+
+ // Multiple comma-separated paths.
+ {
+ in: "path1, path2: Change title.",
+ wantPaths: []string{"path1", "path2"}, wantTitle: "Change title.",
+ },
+ {
+ inRoot: "root",
+ in: "path1, path2: Change title.",
+ wantPaths: []string{"root/path1", "root/path2"}, wantTitle: "Change title.",
+ },
+ {
+ inRoot: "root",
+ in: "[release-branch.go1.11] path1, path2: Change title.",
+ wantPaths: []string{"root/path1", "root/path2"}, wantTitle: "[release-branch.go1.11] Change title.",
+ },
+
+ // No path prefix.
+ {
+ in: "Change title.",
+ wantPaths: []string{""}, wantTitle: "Change title.",
+ },
+ {
+ inRoot: "root",
+ in: "Change title.",
+ wantPaths: []string{"root"}, wantTitle: "Change title.",
+ },
+ {
+ inRoot: "root",
+ in: "[release-branch.go1.11] Change title.",
+ wantPaths: []string{"root"}, wantTitle: "[release-branch.go1.11] Change title.",
+ },
+ }
+ for i, tc := range tests {
+ gotPaths, gotTitle := devapp.ParsePrefixedChangeTitle(tc.inRoot, tc.in)
+ if !reflect.DeepEqual(gotPaths, tc.wantPaths) {
+ t.Errorf("%d: got paths: %q, want: %q", i, gotPaths, tc.wantPaths)
+ }
+ if gotTitle != tc.wantTitle {
+ t.Errorf("%d: got title: %q, want: %q", i, gotTitle, tc.wantTitle)
+ }
+ }
+}
