Make label templates have consistent behavior and priority (#23749)

Fix https://github.com/go-gitea/gitea/issues/23715

Other related PRs:

* #23717
* #23716
* #23719

This PR is different from others, it tries to resolve the problem fundamentally (and brings more benefits)

Although it looks like some more lines are added, actually many new lines are for tests.

----

Before, the code was just "guessing" the file type and try to parse them.

<details>

![image](https://user-images.githubusercontent.com/2114189/228002245-57d58e27-1078-4da9-bf42-5bc0b264c6ce.png)

</details>

This PR:

* Always remember the original option file names, and always use correct parser for them.

* Another benefit is that we can sort the Label Templates now (before there was a map, its key order is undefined)

![image](https://user-images.githubusercontent.com/2114189/228002432-931b9f18-3908-484b-a36b-04760c9ad132.png)
This commit is contained in:
wxiaoguang 2023-04-10 16:44:02 +08:00 committed by GitHub
parent bb6c670cff
commit 4e33481357
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 135 additions and 89 deletions

View file

@ -30,31 +30,23 @@ func IsErrTemplateLoad(err error) bool {
} }
func (err ErrTemplateLoad) Error() string { func (err ErrTemplateLoad) Error() string {
return fmt.Sprintf("Failed to load label template file '%s': %v", err.TemplateFile, err.OriginalError) return fmt.Sprintf("failed to load label template file %q: %v", err.TemplateFile, err.OriginalError)
} }
// GetTemplateFile loads the label template file by given name, // LoadTemplateFile loads the label template file by given file name, returns a slice of Label structs.
// then parses and returns a list of name-color pairs and optionally description. func LoadTemplateFile(fileName string) ([]*Label, error) {
func GetTemplateFile(name string) ([]*Label, error) { data, err := options.Labels(fileName)
data, err := options.Labels(name + ".yaml")
if err == nil && len(data) > 0 {
return parseYamlFormat(name+".yaml", data)
}
data, err = options.Labels(name + ".yml")
if err == nil && len(data) > 0 {
return parseYamlFormat(name+".yml", data)
}
data, err = options.Labels(name)
if err != nil { if err != nil {
return nil, ErrTemplateLoad{name, fmt.Errorf("GetRepoInitFile: %w", err)} return nil, ErrTemplateLoad{fileName, fmt.Errorf("LoadTemplateFile: %w", err)}
} }
return parseLegacyFormat(name, data) if strings.HasSuffix(fileName, ".yaml") || strings.HasSuffix(fileName, ".yml") {
return parseYamlFormat(fileName, data)
}
return parseLegacyFormat(fileName, data)
} }
func parseYamlFormat(name string, data []byte) ([]*Label, error) { func parseYamlFormat(fileName string, data []byte) ([]*Label, error) {
lf := &labelFile{} lf := &labelFile{}
if err := yaml.Unmarshal(data, lf); err != nil { if err := yaml.Unmarshal(data, lf); err != nil {
@ -65,11 +57,11 @@ func parseYamlFormat(name string, data []byte) ([]*Label, error) {
for _, l := range lf.Labels { for _, l := range lf.Labels {
l.Color = strings.TrimSpace(l.Color) l.Color = strings.TrimSpace(l.Color)
if len(l.Name) == 0 || len(l.Color) == 0 { if len(l.Name) == 0 || len(l.Color) == 0 {
return nil, ErrTemplateLoad{name, errors.New("label name and color are required fields")} return nil, ErrTemplateLoad{fileName, errors.New("label name and color are required fields")}
} }
color, err := NormalizeColor(l.Color) color, err := NormalizeColor(l.Color)
if err != nil { if err != nil {
return nil, ErrTemplateLoad{name, fmt.Errorf("bad HTML color code '%s' in label: %s", l.Color, l.Name)} return nil, ErrTemplateLoad{fileName, fmt.Errorf("bad HTML color code '%s' in label: %s", l.Color, l.Name)}
} }
l.Color = color l.Color = color
} }
@ -77,7 +69,7 @@ func parseYamlFormat(name string, data []byte) ([]*Label, error) {
return lf.Labels, nil return lf.Labels, nil
} }
func parseLegacyFormat(name string, data []byte) ([]*Label, error) { func parseLegacyFormat(fileName string, data []byte) ([]*Label, error) {
lines := strings.Split(string(data), "\n") lines := strings.Split(string(data), "\n")
list := make([]*Label, 0, len(lines)) list := make([]*Label, 0, len(lines))
for i := 0; i < len(lines); i++ { for i := 0; i < len(lines); i++ {
@ -88,18 +80,18 @@ func parseLegacyFormat(name string, data []byte) ([]*Label, error) {
parts, description, _ := strings.Cut(line, ";") parts, description, _ := strings.Cut(line, ";")
color, name, ok := strings.Cut(parts, " ") color, labelName, ok := strings.Cut(parts, " ")
if !ok { if !ok {
return nil, ErrTemplateLoad{name, fmt.Errorf("line is malformed: %s", line)} return nil, ErrTemplateLoad{fileName, fmt.Errorf("line is malformed: %s", line)}
} }
color, err := NormalizeColor(color) color, err := NormalizeColor(color)
if err != nil { if err != nil {
return nil, ErrTemplateLoad{name, fmt.Errorf("bad HTML color code '%s' in line: %s", color, line)} return nil, ErrTemplateLoad{fileName, fmt.Errorf("bad HTML color code '%s' in line: %s", color, line)}
} }
list = append(list, &Label{ list = append(list, &Label{
Name: strings.TrimSpace(name), Name: strings.TrimSpace(labelName),
Color: color, Color: color,
Description: strings.TrimSpace(description), Description: strings.TrimSpace(description),
}) })
@ -108,10 +100,10 @@ func parseLegacyFormat(name string, data []byte) ([]*Label, error) {
return list, nil return list, nil
} }
// LoadFormatted loads the labels' list of a template file as a string separated by comma // LoadTemplateDescription loads the labels from a template file, returns a description string by joining each Label.Name with comma
func LoadFormatted(name string) (string, error) { func LoadTemplateDescription(fileName string) (string, error) {
var buf strings.Builder var buf strings.Builder
list, err := GetTemplateFile(name) list, err := LoadTemplateFile(fileName)
if err != nil { if err != nil {
return "", err return "", err
} }

View file

@ -23,7 +23,6 @@ import (
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/models/webhook" "code.gitea.io/gitea/models/webhook"
"code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/label"
"code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs" api "code.gitea.io/gitea/modules/structs"
@ -190,7 +189,7 @@ func CreateRepository(doer, u *user_model.User, opts CreateRepoOptions) (*repo_m
// Check if label template exist // Check if label template exist
if len(opts.IssueLabels) > 0 { if len(opts.IssueLabels) > 0 {
if _, err := label.GetTemplateFile(opts.IssueLabels); err != nil { if _, err := LoadTemplateLabelsByDisplayName(opts.IssueLabels); err != nil {
return nil, err return nil, err
} }
} }

View file

@ -8,7 +8,6 @@ import (
"context" "context"
"fmt" "fmt"
"os" "os"
"path"
"path/filepath" "path/filepath"
"sort" "sort"
"strings" "strings"
@ -27,6 +26,11 @@ import (
asymkey_service "code.gitea.io/gitea/services/asymkey" asymkey_service "code.gitea.io/gitea/services/asymkey"
) )
type OptionFile struct {
DisplayName string
Description string
}
var ( var (
// Gitignores contains the gitiginore files // Gitignores contains the gitiginore files
Gitignores []string Gitignores []string
@ -37,65 +41,73 @@ var (
// Readmes contains the readme files // Readmes contains the readme files
Readmes []string Readmes []string
// LabelTemplates contains the label template files and the list of labels for each file // LabelTemplateFiles contains the label template files, each item has its DisplayName and Description
LabelTemplates map[string]string LabelTemplateFiles []OptionFile
labelTemplateFileMap = map[string]string{} // DisplayName => FileName mapping
) )
// LoadRepoConfig loads the repository config type optionFileList struct {
func LoadRepoConfig() { all []string // all files provided by bindata & custom-path. Sorted.
// Load .gitignore and license files and readme templates. custom []string // custom files provided by custom-path. Non-sorted, internal use only.
types := []string{"gitignore", "license", "readme", "label"}
typeFiles := make([][]string, 4)
for i, t := range types {
files, err := options.Dir(t)
if err != nil {
log.Fatal("Failed to get %s files: %v", t, err)
}
if t == "label" {
for i, f := range files {
ext := strings.ToLower(filepath.Ext(f))
if ext == ".yaml" || ext == ".yml" {
files[i] = f[:len(f)-len(ext)]
}
}
}
customPath := path.Join(setting.CustomPath, "options", t)
isDir, err := util.IsDir(customPath)
if err != nil {
log.Fatal("Failed to get custom %s files: %v", t, err)
}
if isDir {
customFiles, err := util.StatDir(customPath)
if err != nil {
log.Fatal("Failed to get custom %s files: %v", t, err)
} }
for _, f := range customFiles { // mergeCustomLabelFiles merges the custom label files. Always use the file's main name (DisplayName) as the key to de-duplicate.
if !util.SliceContainsString(files, f, true) { func mergeCustomLabelFiles(fl optionFileList) []string {
exts := map[string]int{"": 0, ".yml": 1, ".yaml": 2} // "yaml" file has the highest priority to be used.
m := map[string]string{}
merge := func(list []string) {
sort.Slice(list, func(i, j int) bool { return exts[filepath.Ext(list[i])] < exts[filepath.Ext(list[j])] })
for _, f := range list {
m[strings.TrimSuffix(f, filepath.Ext(f))] = f
}
}
merge(fl.all)
merge(fl.custom)
files := make([]string, 0, len(m))
for _, f := range m {
files = append(files, f) files = append(files, f)
} }
} sort.Strings(files)
} return files
typeFiles[i] = files
} }
Gitignores = typeFiles[0] // LoadRepoConfig loads the repository config
Licenses = typeFiles[1] func LoadRepoConfig() error {
Readmes = typeFiles[2] types := []string{"gitignore", "license", "readme", "label"} // option file directories
LabelTemplatesFiles := typeFiles[3] typeFiles := make([]optionFileList, len(types))
sort.Strings(Gitignores) for i, t := range types {
sort.Strings(Licenses) var err error
sort.Strings(Readmes) if typeFiles[i].all, err = options.Dir(t); err != nil {
sort.Strings(LabelTemplatesFiles) return fmt.Errorf("failed to list %s files: %w", t, err)
}
sort.Strings(typeFiles[i].all)
customPath := filepath.Join(setting.CustomPath, "options", t)
if isDir, err := util.IsDir(customPath); err != nil {
return fmt.Errorf("failed to check custom %s dir: %w", t, err)
} else if isDir {
if typeFiles[i].custom, err = util.StatDir(customPath); err != nil {
return fmt.Errorf("failed to list custom %s files: %w", t, err)
}
}
}
Gitignores = typeFiles[0].all
Licenses = typeFiles[1].all
Readmes = typeFiles[2].all
// Load label templates // Load label templates
LabelTemplates = make(map[string]string) LabelTemplateFiles = nil
for _, templateFile := range LabelTemplatesFiles { labelTemplateFileMap = map[string]string{}
labels, err := label.LoadFormatted(templateFile) for _, file := range mergeCustomLabelFiles(typeFiles[3]) {
description, err := label.LoadTemplateDescription(file)
if err != nil { if err != nil {
log.Error("Failed to load labels: %v", err) return fmt.Errorf("failed to load labels: %w", err)
} }
LabelTemplates[templateFile] = labels displayName := strings.TrimSuffix(file, filepath.Ext(file))
labelTemplateFileMap[displayName] = file
LabelTemplateFiles = append(LabelTemplateFiles, OptionFile{DisplayName: displayName, Description: description})
} }
// Filter out invalid names and promote preferred licenses. // Filter out invalid names and promote preferred licenses.
@ -111,6 +123,7 @@ func LoadRepoConfig() {
} }
} }
Licenses = sortedLicenses Licenses = sortedLicenses
return nil
} }
func prepareRepoCommit(ctx context.Context, repo *repo_model.Repository, tmpDir, repoPath string, opts CreateRepoOptions) error { func prepareRepoCommit(ctx context.Context, repo *repo_model.Repository, tmpDir, repoPath string, opts CreateRepoOptions) error {
@ -344,7 +357,7 @@ func initRepository(ctx context.Context, repoPath string, u *user_model.User, re
// InitializeLabels adds a label set to a repository using a template // InitializeLabels adds a label set to a repository using a template
func InitializeLabels(ctx context.Context, id int64, labelTemplate string, isOrg bool) error { func InitializeLabels(ctx context.Context, id int64, labelTemplate string, isOrg bool) error {
list, err := label.GetTemplateFile(labelTemplate) list, err := LoadTemplateLabelsByDisplayName(labelTemplate)
if err != nil { if err != nil {
return err return err
} }
@ -370,3 +383,11 @@ func InitializeLabels(ctx context.Context, id int64, labelTemplate string, isOrg
} }
return nil return nil
} }
// LoadTemplateLabelsByDisplayName loads a label template by its display name
func LoadTemplateLabelsByDisplayName(displayName string) ([]*label.Label, error) {
if fileName, ok := labelTemplateFileMap[displayName]; ok {
return label.LoadTemplateFile(fileName)
}
return nil, label.ErrTemplateLoad{TemplateFile: displayName, OriginalError: fmt.Errorf("label template %q not found", displayName)}
}

View file

@ -0,0 +1,30 @@
// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package repository
import (
"testing"
"github.com/stretchr/testify/assert"
)
func TestMergeCustomLabels(t *testing.T) {
files := mergeCustomLabelFiles(optionFileList{
all: []string{"a", "a.yaml", "a.yml"},
custom: nil,
})
assert.EqualValues(t, []string{"a.yaml"}, files, "yaml file should win")
files = mergeCustomLabelFiles(optionFileList{
all: []string{"a", "a.yaml"},
custom: []string{"a"},
})
assert.EqualValues(t, []string{"a"}, files, "custom file should win")
files = mergeCustomLabelFiles(optionFileList{
all: []string{"a", "a.yml", "a.yaml"},
custom: []string{"a", "a.yml"},
})
assert.EqualValues(t, []string{"a.yml"}, files, "custom yml file should win if no yaml")
}

View file

@ -246,6 +246,6 @@ func Labels(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("repo.labels") ctx.Data["Title"] = ctx.Tr("repo.labels")
ctx.Data["PageIsOrgSettings"] = true ctx.Data["PageIsOrgSettings"] = true
ctx.Data["PageIsOrgSettingsLabels"] = true ctx.Data["PageIsOrgSettingsLabels"] = true
ctx.Data["LabelTemplates"] = repo_module.LabelTemplates ctx.Data["LabelTemplateFiles"] = repo_module.LabelTemplateFiles
ctx.HTML(http.StatusOK, tplSettingsLabels) ctx.HTML(http.StatusOK, tplSettingsLabels)
} }

View file

@ -28,7 +28,7 @@ func Labels(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("repo.labels") ctx.Data["Title"] = ctx.Tr("repo.labels")
ctx.Data["PageIsIssueList"] = true ctx.Data["PageIsIssueList"] = true
ctx.Data["PageIsLabels"] = true ctx.Data["PageIsLabels"] = true
ctx.Data["LabelTemplates"] = repo_module.LabelTemplates ctx.Data["LabelTemplateFiles"] = repo_module.LabelTemplateFiles
ctx.HTML(http.StatusOK, tplLabels) ctx.HTML(http.StatusOK, tplLabels)
} }

View file

@ -10,6 +10,7 @@ import (
issues_model "code.gitea.io/gitea/models/issues" issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
"code.gitea.io/gitea/modules/repository"
"code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/test"
"code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/forms"
@ -30,6 +31,7 @@ func int64SliceToCommaSeparated(a []int64) string {
func TestInitializeLabels(t *testing.T) { func TestInitializeLabels(t *testing.T) {
unittest.PrepareTestEnv(t) unittest.PrepareTestEnv(t)
assert.NoError(t, repository.LoadRepoConfig())
ctx := test.MockContext(t, "user2/repo1/labels/initialize") ctx := test.MockContext(t, "user2/repo1/labels/initialize")
test.LoadUser(t, ctx, 2) test.LoadUser(t, ctx, 2)
test.LoadRepo(t, ctx, 2) test.LoadRepo(t, ctx, 2)

View file

@ -132,7 +132,7 @@ func Create(ctx *context.Context) {
// Give default value for template to render. // Give default value for template to render.
ctx.Data["Gitignores"] = repo_module.Gitignores ctx.Data["Gitignores"] = repo_module.Gitignores
ctx.Data["LabelTemplates"] = repo_module.LabelTemplates ctx.Data["LabelTemplateFiles"] = repo_module.LabelTemplateFiles
ctx.Data["Licenses"] = repo_module.Licenses ctx.Data["Licenses"] = repo_module.Licenses
ctx.Data["Readmes"] = repo_module.Readmes ctx.Data["Readmes"] = repo_module.Readmes
ctx.Data["readme"] = "Default" ctx.Data["readme"] = "Default"
@ -200,7 +200,7 @@ func CreatePost(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("new_repo") ctx.Data["Title"] = ctx.Tr("new_repo")
ctx.Data["Gitignores"] = repo_module.Gitignores ctx.Data["Gitignores"] = repo_module.Gitignores
ctx.Data["LabelTemplates"] = repo_module.LabelTemplates ctx.Data["LabelTemplateFiles"] = repo_module.LabelTemplateFiles
ctx.Data["Licenses"] = repo_module.Licenses ctx.Data["Licenses"] = repo_module.Licenses
ctx.Data["Readmes"] = repo_module.Readmes ctx.Data["Readmes"] = repo_module.Readmes

View file

@ -81,7 +81,9 @@ func PushCreateRepo(ctx context.Context, authUser, owner *user_model.User, repoN
// Init start repository service // Init start repository service
func Init() error { func Init() error {
repo_module.LoadRepoConfig() if err := repo_module.LoadRepoConfig(); err != nil {
return err
}
system_model.RemoveAllWithNotice(db.DefaultContext, "Clean up temporary repository uploads", setting.Repository.Upload.TempPath) system_model.RemoveAllWithNotice(db.DefaultContext, "Clean up temporary repository uploads", setting.Repository.Upload.TempPath)
system_model.RemoveAllWithNotice(db.DefaultContext, "Clean up temporary repositories", repo_module.LocalCopyPath()) system_model.RemoveAllWithNotice(db.DefaultContext, "Clean up temporary repositories", repo_module.LocalCopyPath())
return initPushQueue() return initPushQueue()

View file

@ -117,8 +117,8 @@
<div class="default text">{{.locale.Tr "repo.issue_labels_helper"}}</div> <div class="default text">{{.locale.Tr "repo.issue_labels_helper"}}</div>
<div class="menu"> <div class="menu">
<div class="item" data-value="">{{.locale.Tr "repo.issue_labels_helper"}}</div> <div class="item" data-value="">{{.locale.Tr "repo.issue_labels_helper"}}</div>
{{range $template, $labels := .LabelTemplates}} {{range .LabelTemplateFiles}}
<div class="item" data-value="{{$template}}">{{$template}}<br><i>({{$labels}})</i></div> <div class="item" data-value="{{.DisplayName}}">{{.DisplayName}}<br><i>({{.Description}})</i></div>
{{end}} {{end}}
</div> </div>
</div> </div>

View file

@ -10,8 +10,8 @@
<input type="hidden" name="template_name" value="Default"> <input type="hidden" name="template_name" value="Default">
<div class="default text">{{.locale.Tr "repo.issues.label_templates.helper"}}</div> <div class="default text">{{.locale.Tr "repo.issues.label_templates.helper"}}</div>
<div class="menu"> <div class="menu">
{{range $template, $labels := .LabelTemplates}} {{range .LabelTemplateFiles}}
<div class="item" data-value="{{$template}}">{{$template}}<br><i>({{$labels}})</i></div> <div class="item" data-value="{{.DisplayName}}">{{.DisplayName}}<br><i>({{.Description}})</i></div>
{{end}} {{end}}
</div> </div>
{{svg "octicon-triangle-down" 18 "dropdown icon"}} {{svg "octicon-triangle-down" 18 "dropdown icon"}}