Optimize branch protection rule loading (#32280)

before if it was nonglob each load would try to glob it and the check
that is not glob ... now we only do that once and no future loading will
trigger it

---
*Sponsored by Kithara Software GmbH*

diff --git a/models/git/protected_banch_list_test.go b/models/git/protected_banch_list_test.go
deleted file mode 100644 (file)
index 4bb3136..0000000
--- a/models/git/protected_banch_list_test.go
+++ /dev/null
@@ -1,76 +0,0 @@
-// Copyright 2023 The Gitea Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
-
-package git
-
-import (
-       "fmt"
-       "testing"
-
-       "github.com/stretchr/testify/assert"
-)
-
-func TestBranchRuleMatchPriority(t *testing.T) {
-       kases := []struct {
-               Rules            []string
-               BranchName       string
-               ExpectedMatchIdx int
-       }{
-               {
-                       Rules:            []string{"release/*", "release/v1.17"},
-                       BranchName:       "release/v1.17",
-                       ExpectedMatchIdx: 1,
-               },
-               {
-                       Rules:            []string{"release/v1.17", "release/*"},
-                       BranchName:       "release/v1.17",
-                       ExpectedMatchIdx: 0,
-               },
-               {
-                       Rules:            []string{"release/**/v1.17", "release/test/v1.17"},
-                       BranchName:       "release/test/v1.17",
-                       ExpectedMatchIdx: 1,
-               },
-               {
-                       Rules:            []string{"release/test/v1.17", "release/**/v1.17"},
-                       BranchName:       "release/test/v1.17",
-                       ExpectedMatchIdx: 0,
-               },
-               {
-                       Rules:            []string{"release/**", "release/v1.0.0"},
-                       BranchName:       "release/v1.0.0",
-                       ExpectedMatchIdx: 1,
-               },
-               {
-                       Rules:            []string{"release/v1.0.0", "release/**"},
-                       BranchName:       "release/v1.0.0",
-                       ExpectedMatchIdx: 0,
-               },
-               {
-                       Rules:            []string{"release/**", "release/v1.0.0"},
-                       BranchName:       "release/v2.0.0",
-                       ExpectedMatchIdx: 0,
-               },
-               {
-                       Rules:            []string{"release/*", "release/v1.0.0"},
-                       BranchName:       "release/1/v2.0.0",
-                       ExpectedMatchIdx: -1,
-               },
-       }
-
-       for _, kase := range kases {
-               var pbs ProtectedBranchRules
-               for _, rule := range kase.Rules {
-                       pbs = append(pbs, &ProtectedBranch{RuleName: rule})
-               }
-               pbs.sort()
-               matchedPB := pbs.GetFirstMatched(kase.BranchName)
-               if matchedPB == nil {
-                       if kase.ExpectedMatchIdx >= 0 {
-                               assert.Error(t, fmt.Errorf("no matched rules but expected %s[%d]", kase.Rules[kase.ExpectedMatchIdx], kase.ExpectedMatchIdx))
-                       }
-               } else {
-                       assert.EqualValues(t, kase.Rules[kase.ExpectedMatchIdx], matchedPB.RuleName)
-               }
-       }
-}

diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go
index 0033a42d4e39f51df46125ed6ae43d0694a4f449..37d933a9826103ce12e4f52aef5faf96ff7aed91 100644 (file)
--- a/models/git/protected_branch.go
+++ b/models/git/protected_branch.go
@@ -84,14 +84,20 @@ func IsRuleNameSpecial(ruleName string) bool {
 }
 
 func (protectBranch *ProtectedBranch) loadGlob() {
-       if protectBranch.globRule == nil {
-               var err error
-               protectBranch.globRule, err = glob.Compile(protectBranch.RuleName, '/')
-               if err != nil {
-                       log.Warn("Invalid glob rule for ProtectedBranch[%d]: %s %v", protectBranch.ID, protectBranch.RuleName, err)
-                       protectBranch.globRule = glob.MustCompile(glob.QuoteMeta(protectBranch.RuleName), '/')
-               }
-               protectBranch.isPlainName = !IsRuleNameSpecial(protectBranch.RuleName)
+       if protectBranch.isPlainName || protectBranch.globRule != nil {
+               return
+       }
+       // detect if it is not glob
+       if !IsRuleNameSpecial(protectBranch.RuleName) {
+               protectBranch.isPlainName = true
+               return
+       }
+       // now we load the glob
+       var err error
+       protectBranch.globRule, err = glob.Compile(protectBranch.RuleName, '/')
+       if err != nil {
+               log.Warn("Invalid glob rule for ProtectedBranch[%d]: %s %v", protectBranch.ID, protectBranch.RuleName, err)
+               protectBranch.globRule = glob.MustCompile(glob.QuoteMeta(protectBranch.RuleName), '/')
        }
 }
 

diff --git a/models/git/protected_branch_list_test.go b/models/git/protected_branch_list_test.go
new file mode 100644 (file)
index 0000000..94a48f3
--- /dev/null
+++ b/models/git/protected_branch_list_test.go
@@ -0,0 +1,105 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package git
+
+import (
+       "fmt"
+       "testing"
+
+       "github.com/stretchr/testify/assert"
+)
+
+func TestBranchRuleMatchPriority(t *testing.T) {
+       kases := []struct {
+               Rules            []string
+               BranchName       string
+               ExpectedMatchIdx int
+       }{
+               {
+                       Rules:            []string{"release/*", "release/v1.17"},
+                       BranchName:       "release/v1.17",
+                       ExpectedMatchIdx: 1,
+               },
+               {
+                       Rules:            []string{"release/v1.17", "release/*"},
+                       BranchName:       "release/v1.17",
+                       ExpectedMatchIdx: 0,
+               },
+               {
+                       Rules:            []string{"release/**/v1.17", "release/test/v1.17"},
+                       BranchName:       "release/test/v1.17",
+                       ExpectedMatchIdx: 1,
+               },
+               {
+                       Rules:            []string{"release/test/v1.17", "release/**/v1.17"},
+                       BranchName:       "release/test/v1.17",
+                       ExpectedMatchIdx: 0,
+               },
+               {
+                       Rules:            []string{"release/**", "release/v1.0.0"},
+                       BranchName:       "release/v1.0.0",
+                       ExpectedMatchIdx: 1,
+               },
+               {
+                       Rules:            []string{"release/v1.0.0", "release/**"},
+                       BranchName:       "release/v1.0.0",
+                       ExpectedMatchIdx: 0,
+               },
+               {
+                       Rules:            []string{"release/**", "release/v1.0.0"},
+                       BranchName:       "release/v2.0.0",
+                       ExpectedMatchIdx: 0,
+               },
+               {
+                       Rules:            []string{"release/*", "release/v1.0.0"},
+                       BranchName:       "release/1/v2.0.0",
+                       ExpectedMatchIdx: -1,
+               },
+       }
+
+       for _, kase := range kases {
+               var pbs ProtectedBranchRules
+               for _, rule := range kase.Rules {
+                       pbs = append(pbs, &ProtectedBranch{RuleName: rule})
+               }
+               pbs.sort()
+               matchedPB := pbs.GetFirstMatched(kase.BranchName)
+               if matchedPB == nil {
+                       if kase.ExpectedMatchIdx >= 0 {
+                               assert.Error(t, fmt.Errorf("no matched rules but expected %s[%d]", kase.Rules[kase.ExpectedMatchIdx], kase.ExpectedMatchIdx))
+                       }
+               } else {
+                       assert.EqualValues(t, kase.Rules[kase.ExpectedMatchIdx], matchedPB.RuleName)
+               }
+       }
+}
+
+func TestBranchRuleSort(t *testing.T) {
+       in := []*ProtectedBranch{{
+               RuleName:    "b",
+               CreatedUnix: 1,
+       }, {
+               RuleName:    "b/*",
+               CreatedUnix: 3,
+       }, {
+               RuleName:    "a/*",
+               CreatedUnix: 2,
+       }, {
+               RuleName:    "c",
+               CreatedUnix: 0,
+       }, {
+               RuleName:    "a",
+               CreatedUnix: 4,
+       }}
+       expect := []string{"c", "b", "a", "a/*", "b/*"}
+
+       pbr := ProtectedBranchRules(in)
+       pbr.sort()
+
+       var got []string
+       for i := range pbr {
+               got = append(got, pbr[i].RuleName)
+       }
+       assert.Equal(t, expect, got)
+}