don't make Attributes expose internal map

wxiaoguang · wxiaoguang · commit 1d333296eb7c · 2025-04-11T15:10:05.000+08:00
diff --git a/modules/git/attribute/attribute.go b/modules/git/attribute/attribute.go
@@ -19,6 +19,7 @@ const (
 	LinguistLanguage      = "linguist-language"
 	GitlabLanguage        = "gitlab-language"
 	Lockable              = "lockable"
+	Filter                = "filter"
 )
 
 var LinguistAttributes = []string{
@@ -41,7 +42,7 @@ func (a Attribute) ToString() optional.Option[string] {
 	return optional.None[string]()
 }
 
-// true if "set"/"true", false if "unset"/"false", none otherwise
+// ToBool converts the attribute value to optional boolean: true if "set"/"true", false if "unset"/"false", none otherwise
 func (a Attribute) ToBool() optional.Option[bool] {
 	switch a {
 	case "set", "true":
@@ -52,36 +53,42 @@ func (a Attribute) ToBool() optional.Option[bool] {
 	return optional.None[bool]()
 }
 
-type Attributes map[string]Attribute
+type Attributes struct {
+	m map[string]Attribute
+}
+
+func NewAttributes() *Attributes {
+	return &Attributes{m: make(map[string]Attribute)}
+}
 
-func (attrs Attributes) Get(name string) Attribute {
-	if value, has := attrs[name]; has {
+func (attrs *Attributes) Get(name string) Attribute {
+	if value, has := attrs.m[name]; has {
 		return value
 	}
 	return ""
 }
 
-func (attrs Attributes) GetVendored() optional.Option[bool] {
+func (attrs *Attributes) GetVendored() optional.Option[bool] {
 	return attrs.Get(LinguistVendored).ToBool()
 }
 
-func (attrs Attributes) GetGenerated() optional.Option[bool] {
+func (attrs *Attributes) GetGenerated() optional.Option[bool] {
 	return attrs.Get(LinguistGenerated).ToBool()
 }
 
-func (attrs Attributes) GetDocumentation() optional.Option[bool] {
+func (attrs *Attributes) GetDocumentation() optional.Option[bool] {
 	return attrs.Get(LinguistDocumentation).ToBool()
 }
 
-func (attrs Attributes) GetDetectable() optional.Option[bool] {
+func (attrs *Attributes) GetDetectable() optional.Option[bool] {
 	return attrs.Get(LinguistDetectable).ToBool()
 }
 
-func (attrs Attributes) GetLinguistLanguage() optional.Option[string] {
+func (attrs *Attributes) GetLinguistLanguage() optional.Option[string] {
 	return attrs.Get(LinguistLanguage).ToString()
 }
 
-func (attrs Attributes) GetGitlabLanguage() optional.Option[string] {
+func (attrs *Attributes) GetGitlabLanguage() optional.Option[string] {
 	attrStr := attrs.Get(GitlabLanguage).ToString()
 	if attrStr.Has() {
 		raw := attrStr.Value()
@@ -95,7 +102,7 @@ func (attrs Attributes) GetGitlabLanguage() optional.Option[string] {
 	return attrStr
 }
 
-func (attrs Attributes) GetLanguage() optional.Option[string] {
+func (attrs *Attributes) GetLanguage() optional.Option[string] {
 	// prefer linguist-language over gitlab-language
 	// if linguist-language is not set, use gitlab-language
 	// if both are not set, return none
diff --git a/modules/git/attribute/attribute_test.go b/modules/git/attribute/attribute_test.go
@@ -16,13 +16,15 @@ func Test_Attribute(t *testing.T) {
 	assert.Equal(t, "Java", Attribute("Java").ToString().Value())
 
 	attributes := Attributes{
-		LinguistGenerated:     "true",
-		LinguistDocumentation: "false",
-		LinguistDetectable:    "set",
-		LinguistLanguage:      "Python",
-		GitlabLanguage:        "Java",
-		"filter":              "unspecified",
-		"test":                "",
+		m: map[string]Attribute{
+			LinguistGenerated:     "true",
+			LinguistDocumentation: "false",
+			LinguistDetectable:    "set",
+			LinguistLanguage:      "Python",
+			GitlabLanguage:        "Java",
+			"filter":              "unspecified",
+			"test":                "",
+		},
 	}
 
 	assert.Empty(t, attributes.Get("test").ToString().Value())
diff --git a/modules/git/attribute/batch.go b/modules/git/attribute/batch.go
@@ -94,7 +94,7 @@ func NewBatchChecker(repo *git.Repository, treeish string, attributes []string)
 }
 
 // CheckPath check attr for given path
-func (c *BatchChecker) CheckPath(path string) (rs Attributes, err error) {
+func (c *BatchChecker) CheckPath(path string) (rs *Attributes, err error) {
 	defer func() {
 		if err != nil && err != c.ctx.Err() {
 			log.Error("Unexpected error when checking path %s in %s, error: %v", path, filepath.Base(c.repo.Path), err)
@@ -128,7 +128,7 @@ func (c *BatchChecker) CheckPath(path string) (rs Attributes, err error) {
 		return fmt.Errorf("CheckPath timeout: %s", debugMsg)
 	}
 
-	rs = make(map[string]Attribute)
+	rs = NewAttributes()
 	for i := 0; i < c.attributesNum; i++ {
 		select {
 		case <-time.After(5 * time.Second):
@@ -138,7 +138,7 @@ func (c *BatchChecker) CheckPath(path string) (rs Attributes, err error) {
 			if !ok {
 				return nil, c.ctx.Err()
 			}
-			rs[attr.Attribute] = Attribute(attr.Value)
+			rs.m[attr.Attribute] = Attribute(attr.Value)
 		case <-c.ctx.Done():
 			return nil, c.ctx.Err()
 		}
diff --git a/modules/git/attribute/checker.go b/modules/git/attribute/checker.go
@@ -55,7 +55,7 @@ type CheckAttributeOpts struct {
 
 // CheckAttributes return the attributes of the given filenames and attributes in the given treeish.
 // If treeish is empty, then it will use current working directory, otherwise it will use the provided treeish on the bare repo
-func CheckAttributes(ctx context.Context, gitRepo *git.Repository, treeish string, opts CheckAttributeOpts) (map[string]Attributes, error) {
+func CheckAttributes(ctx context.Context, gitRepo *git.Repository, treeish string, opts CheckAttributeOpts) (map[string]*Attributes, error) {
 	cmd, envs, cancel, err := checkAttrCommand(gitRepo, treeish, opts.Filenames, opts.Attributes)
 	if err != nil {
 		return nil, err
@@ -79,17 +79,17 @@ func CheckAttributes(ctx context.Context, gitRepo *git.Repository, treeish strin
 		return nil, errors.New("wrong number of fields in return from check-attr")
 	}
 
-	attributesMap := make(map[string]Attributes)
+	attributesMap := make(map[string]*Attributes)
 	for i := 0; i < (len(fields) / 3); i++ {
 		filename := string(fields[3*i])
 		attribute := string(fields[3*i+1])
 		info := string(fields[3*i+2])
-		attribute2info := attributesMap[filename]
-		if attribute2info == nil {
-			attribute2info = make(Attributes)
+		attribute2info, ok := attributesMap[filename]
+		if !ok {
+			attribute2info = NewAttributes()
+			attributesMap[filename] = attribute2info
 		}
-		attribute2info[attribute] = Attribute(info)
-		attributesMap[filename] = attribute2info
+		attribute2info.m[attribute] = Attribute(info)
 	}
 
 	return attributesMap, nil
diff --git a/routers/web/repo/setting/lfs.go b/routers/web/repo/setting/lfs.go
@@ -152,7 +152,7 @@ func LFSLocks(ctx *context.Context) {
 			log.Error("Unable to check attributes in %s: %s (%v)", tmpBasePath, lock.Path, err)
 			continue
 		}
-		lockables[i] = attrs["lockable"].ToBool().Value()
+		lockables[i] = attrs.Get(attribute.Lockable).ToBool().Value()
 	}
 	ctx.Data["Lockables"] = lockables
 
diff --git a/routers/web/repo/view_file.go b/routers/web/repo/view_file.go
@@ -147,8 +147,22 @@ func prepareToRenderFile(ctx *context.Context, entry *git.TreeEntry) {
 		ctx.Data["EditFileTooltip"] = ctx.Tr("repo.editor.cannot_edit_non_text_files")
 	}
 
-	var attrs attribute.Attributes
-	attributes := []string{attribute.LinguistGenerated, attribute.LinguistVendored}
+	// read all needed attributes which will be used later
+	// there should be no performance different between reading 2 or 4 here
+	attrsMap, err := attribute.CheckAttributes(ctx, ctx.Repo.GitRepo, ctx.Repo.CommitID, attribute.CheckAttributeOpts{
+		Filenames:  []string{ctx.Repo.TreePath},
+		Attributes: []string{attribute.LinguistGenerated, attribute.LinguistVendored, attribute.LinguistLanguage, attribute.GitlabLanguage},
+	})
+	if err != nil {
+		ctx.ServerError("attribute.CheckAttributes", err)
+		return
+	}
+	attrs := attrsMap[ctx.Repo.TreePath]
+	if attrs == nil {
+		// this case shouldn't happe, just in case.
+		setting.PanicInDevOrTesting("no attributes found for %s", ctx.Repo.TreePath)
+		attrs = attribute.NewAttributes()
+	}
 
 	switch {
 	case isRepresentableAsText:
@@ -212,19 +226,7 @@ func prepareToRenderFile(ctx *context.Context, entry *git.TreeEntry) {
 				ctx.Data["NumLines"] = bytes.Count(buf, []byte{'\n'}) + 1
 			}
 
-			var language string
-			attributes = append(attributes, attribute.LinguistLanguage, attribute.GitlabLanguage)
-			attrsMap, err := attribute.CheckAttributes(ctx, ctx.Repo.GitRepo, ctx.Repo.CommitID, attribute.CheckAttributeOpts{
-				Filenames:  []string{ctx.Repo.TreePath},
-				Attributes: attributes,
-			})
-			if err != nil {
-				log.Error("Unable to get file language for %-v:%s. Error: %v", ctx.Repo.Repository, ctx.Repo.TreePath, err)
-			} else {
-				attrs = attrsMap[ctx.Repo.TreePath] // then it will be reused out of the switch block
-				language = attrs.GetLanguage().Value()
-			}
-
+			language := attrs.GetLanguage().Value()
 			fileContent, lexerName, err := highlight.File(blob.Name(), language, buf)
 			ctx.Data["LexerName"] = lexerName
 			if err != nil {
@@ -294,18 +296,6 @@ func prepareToRenderFile(ctx *context.Context, entry *git.TreeEntry) {
 		}
 	}
 
-	if attrs == nil {
-		attrsMap, err := attribute.CheckAttributes(ctx, ctx.Repo.GitRepo, ctx.Repo.CommitID, attribute.CheckAttributeOpts{
-			Filenames:  []string{ctx.Repo.TreePath},
-			Attributes: attributes,
-		})
-		if err != nil {
-			ctx.ServerError("attribute.CheckAttributes", err)
-			return
-		}
-		attrs = attrsMap[ctx.Repo.TreePath]
-	}
-
 	ctx.Data["IsVendored"], ctx.Data["IsGenerated"] = attrs.GetVendored().Value(), attrs.GetGenerated().Value()
 
 	if fInfo.st.IsImage() && !fInfo.st.IsSvgImage() {
diff --git a/services/repository/files/update.go b/services/repository/files/update.go
@@ -490,14 +490,14 @@ func CreateOrUpdateFile(ctx context.Context, t *TemporaryUploadRepository, file
 	if setting.LFS.StartServer && hasOldBranch {
 		// Check there is no way this can return multiple infos
 		attributesMap, err := attribute.CheckAttributes(ctx, t.gitRepo, "" /* use temp repo's working dir */, attribute.CheckAttributeOpts{
-			Attributes: []string{"filter"},
+			Attributes: []string{attribute.Filter},
 			Filenames:  []string{file.Options.treePath},
 		})
 		if err != nil {
 			return err
 		}
 
-		if attributesMap[file.Options.treePath] != nil && attributesMap[file.Options.treePath]["filter"] == "lfs" {
+		if attributesMap[file.Options.treePath] != nil && attributesMap[file.Options.treePath].Get(attribute.Filter).ToString().Value() == "lfs" {
 			// OK so we are supposed to LFS this data!
 			pointer, err := lfs.GeneratePointer(treeObjectContentReader)
 			if err != nil {
diff --git a/services/repository/files/upload.go b/services/repository/files/upload.go
@@ -106,10 +106,10 @@ func UploadRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 		}
 	}
 
-	var attributesMap map[string]attribute.Attributes
+	var attributesMap map[string]*attribute.Attributes
 	if setting.LFS.StartServer {
 		attributesMap, err = attribute.CheckAttributes(ctx, t.gitRepo, "" /* use temp repo's working dir */, attribute.CheckAttributeOpts{
-			Attributes: []string{"filter"},
+			Attributes: []string{attribute.Filter},
 			Filenames:  names,
 		})
 		if err != nil {
@@ -176,15 +176,15 @@ func UploadRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 	return repo_model.DeleteUploads(ctx, uploads...)
 }
 
-func copyUploadedLFSFileIntoRepository(ctx context.Context, info *uploadInfo, attributesMap map[string]attribute.Attributes, t *TemporaryUploadRepository, treePath string) error {
+func copyUploadedLFSFileIntoRepository(ctx context.Context, info *uploadInfo, attributesMap map[string]*attribute.Attributes, t *TemporaryUploadRepository, treePath string) error {
 	file, err := os.Open(info.upload.LocalPath())
 	if err != nil {
 		return err
 	}
 	defer file.Close()
 
 	var objectHash string
-	if setting.LFS.StartServer && attributesMap[info.upload.Name] != nil && attributesMap[info.upload.Name]["filter"] == "lfs" {
+	if setting.LFS.StartServer && attributesMap[info.upload.Name] != nil && attributesMap[info.upload.Name].Get(attribute.Filter).ToString().Value() == "lfs" {
 		// Handle LFS
 		// FIXME: Inefficient! this should probably happen in models.Upload
 		pointer, err := lfs.GeneratePointer(file)

Original file line number	Diff line number	Diff line change
`@@ -94,7 +94,7 @@ func NewBatchChecker(repo *git.Repository, treeish string, attributes []string)`
`94`	`94`	`}`
`95`	`95`
`96`	`96`	`// CheckPath check attr for given path`
`97`		`-func (c *BatchChecker) CheckPath(path string) (rs Attributes, err error) {`
	`97`	`+func (c BatchChecker) CheckPath(path string) (rs Attributes, err error) {`
`98`	`98`	`defer func() {`
`99`	`99`	`if err != nil && err != c.ctx.Err() {`
`100`	`100`	`log.Error("Unexpected error when checking path %s in %s, error: %v", path, filepath.Base(c.repo.Path), err)`
`@@ -128,7 +128,7 @@ func (c *BatchChecker) CheckPath(path string) (rs Attributes, err error) {`
`128`	`128`	`return fmt.Errorf("CheckPath timeout: %s", debugMsg)`
`129`	`129`	`}`
`130`	`130`
`131`		`- rs = make(map[string]Attribute)`
	`131`	`+ rs = NewAttributes()`
`132`	`132`	`for i := 0; i < c.attributesNum; i++ {`
`133`	`133`	`select {`
`134`	`134`	`case <-time.After(5 * time.Second):`
`@@ -138,7 +138,7 @@ func (c *BatchChecker) CheckPath(path string) (rs Attributes, err error) {`
`138`	`138`	`if !ok {`
`139`	`139`	`return nil, c.ctx.Err()`
`140`	`140`	`}`
`141`		`- rs[attr.Attribute] = Attribute(attr.Value)`
	`141`	`+ rs.m[attr.Attribute] = Attribute(attr.Value)`
`142`	`142`	`case <-c.ctx.Done():`
`143`	`143`	`return nil, c.ctx.Err()`
`144`	`144`	`}`
Original file line number	Diff line number	Diff line change
`@@ -152,7 +152,7 @@ func LFSLocks(ctx *context.Context) {`
`152`	`152`	`log.Error("Unable to check attributes in %s: %s (%v)", tmpBasePath, lock.Path, err)`
`153`	`153`	`continue`
`154`	`154`	`}`
`155`		`- lockables[i] = attrs["lockable"].ToBool().Value()`
	`155`	`+ lockables[i] = attrs.Get(attribute.Lockable).ToBool().Value()`
`156`	`156`	`}`
`157`	`157`	`ctx.Data["Lockables"] = lockables`
`158`	`158`