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

Backport #23749

Fix #23715

Backported manually and tested manually.

Author: wxiaoguang

modules/label/parser.go

@@ -30,31 +30,23 @@ func IsErrTemplateLoad(err error) bool {
 }
 
 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,
-// then parses and returns a list of name-color pairs and optionally description.
-func GetTemplateFile(name string) ([]*Label, error) {
-	data, err := options.GetRepoInitFile("label", name+".yaml")
-	if err == nil && len(data) > 0 {
-		return parseYamlFormat(name+".yaml", data)
-	}
-
-	data, err = options.GetRepoInitFile("label", name+".yml")
-	if err == nil && len(data) > 0 {
-		return parseYamlFormat(name+".yml", data)
-	}
-
-	data, err = options.GetRepoInitFile("label", name)
+// LoadTemplateFile loads the label template file by given file name, returns a slice of Label structs.
+func LoadTemplateFile(fileName string) ([]*Label, error) {
+	data, err := options.Labels(fileName)
 	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{}
 
 	if err := yaml.Unmarshal(data, lf); err != nil {
@@ -65,19 +57,19 @@ func parseYamlFormat(name string, data []byte) ([]*Label, error) {
 	for _, l := range lf.Labels {
 		l.Color = strings.TrimSpace(l.Color)
 		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)
 		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
 	}
 
 	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")
 	list := make([]*Label, 0, len(lines))
 	for i := 0; i < len(lines); i++ {
@@ -88,18 +80,18 @@ func parseLegacyFormat(name string, data []byte) ([]*Label, error) {
 
 		parts, description, _ := strings.Cut(line, ";")
 
-		color, name, ok := strings.Cut(parts, " ")
+		color, labelName, ok := strings.Cut(parts, " ")
 		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)
 		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{
-			Name:        strings.TrimSpace(name),
+			Name:        strings.TrimSpace(labelName),
 			Color:       color,
 			Description: strings.TrimSpace(description),
 		})
@@ -108,10 +100,10 @@ func parseLegacyFormat(name string, data []byte) ([]*Label, error) {
 	return list, nil
 }
 
-// LoadFormatted loads the labels' list of a template file as a string separated by comma
-func LoadFormatted(name string) (string, error) {
+// LoadTemplateDescription loads the labels from a template file, returns a description string by joining each Label.Name with comma
+func LoadTemplateDescription(fileName string) (string, error) {
 	var buf strings.Builder
-	list, err := GetTemplateFile(name)
+	list, err := LoadTemplateFile(fileName)
 	if err != nil {
 		return "", err
 	}

modules/repository/create.go

@@ -23,7 +23,6 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/models/webhook"
 	"code.gitea.io/gitea/modules/git"
-	"code.gitea.io/gitea/modules/label"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	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
 	if len(opts.IssueLabels) > 0 {
-		if _, err := label.GetTemplateFile(opts.IssueLabels); err != nil {
+		if _, err := LoadTemplateLabelsByDisplayName(opts.IssueLabels); err != nil {
 			return nil, err
 		}
 	}

modules/repository/init.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"path"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -27,6 +26,11 @@ import (
 	asymkey_service "code.gitea.io/gitea/services/asymkey"
 )
 
+type OptionFile struct {
+	DisplayName string
+	Description string
+}
+
 var (
 	// Gitignores contains the gitiginore files
 	Gitignores []string
@@ -37,65 +41,73 @@ var (
 	// Readmes contains the readme files
 	Readmes []string
 
-	// LabelTemplates contains the label template files and the list of labels for each file
-	LabelTemplates map[string]string
+	// LabelTemplateFiles contains the label template files, each item has its DisplayName and Description
+	LabelTemplateFiles   []OptionFile
+	labelTemplateFileMap = map[string]string{} // DisplayName => FileName mapping
 )
 
+type optionFileList struct {
+	all    []string // all files provided by bindata & custom-path. Sorted.
+	custom []string // custom files provided by custom-path. Non-sorted, internal use only.
+}
+
+// mergeCustomLabelFiles merges the custom label files. Always use the file's main name (DisplayName) as the key to de-duplicate.
+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)
+	}
+	sort.Strings(files)
+	return files
+}
+
 // LoadRepoConfig loads the repository config
-func LoadRepoConfig() {
-	// Load .gitignore and license files and readme templates.
-	types := []string{"gitignore", "license", "readme", "label"}
-	typeFiles := make([][]string, 4)
+func LoadRepoConfig() error {
+	types := []string{"gitignore", "license", "readme", "label"} // option file directories
+	typeFiles := make([]optionFileList, len(types))
 	for i, t := range types {
-		files, err := options.Dir(t)
-		if err != nil {
-			log.Fatal("Failed to get %s files: %v", t, err)
+		var err error
+		if typeFiles[i].all, err = options.Dir(t); err != nil {
+			return fmt.Errorf("failed to list %s files: %w", 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)]
-				}
+		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)
 			}
 		}
-		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 {
-				if !util.SliceContainsString(files, f, true) {
-					files = append(files, f)
-				}
-			}
-		}
-		typeFiles[i] = files
 	}
 
-	Gitignores = typeFiles[0]
-	Licenses = typeFiles[1]
-	Readmes = typeFiles[2]
-	LabelTemplatesFiles := typeFiles[3]
-	sort.Strings(Gitignores)
-	sort.Strings(Licenses)
-	sort.Strings(Readmes)
-	sort.Strings(LabelTemplatesFiles)
+	Gitignores = typeFiles[0].all
+	Licenses = typeFiles[1].all
+	Readmes = typeFiles[2].all
 
 	// Load label templates
-	LabelTemplates = make(map[string]string)
-	for _, templateFile := range LabelTemplatesFiles {
-		labels, err := label.LoadFormatted(templateFile)
+	LabelTemplateFiles = nil
+	labelTemplateFileMap = map[string]string{}
+	for _, file := range mergeCustomLabelFiles(typeFiles[3]) {
+		description, err := label.LoadTemplateDescription(file)
 		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.
@@ -111,6 +123,7 @@ func LoadRepoConfig() {
 		}
 	}
 	Licenses = sortedLicenses
+	return nil
 }
 
 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
 func InitializeLabels(ctx context.Context, id int64, labelTemplate string, isOrg bool) error {
-	list, err := label.GetTemplateFile(labelTemplate)
+	list, err := LoadTemplateLabelsByDisplayName(labelTemplate)
 	if err != nil {
 		return err
 	}
@@ -370,3 +383,11 @@ func InitializeLabels(ctx context.Context, id int64, labelTemplate string, isOrg
 	}
 	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)}
+}

modules/repository/init_test.go

@@ -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")
+}

routers/web/org/setting.go

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

routers/web/repo/issue_label.go

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

routers/web/repo/issue_label_test.go

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

routers/web/repo/repo.go

@@ -132,7 +132,7 @@ func Create(ctx *context.Context) {
 
 	// Give default value for template to render.
 	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["Readmes"] = repo_module.Readmes
 	ctx.Data["readme"] = "Default"
@@ -200,7 +200,7 @@ func CreatePost(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("new_repo")
 
 	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["Readmes"] = repo_module.Readmes
 

services/repository/repository.go

@@ -81,7 +81,9 @@ func PushCreateRepo(authUser, owner *user_model.User, repoName string) (*repo_mo
 
 // Init start repository service
 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 repositories", repo_module.LocalCopyPath())
 	return initPushQueue()

templates/repo/create.tmpl

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