From 5d43801b72790ce5862aefdc4520edb06bb4cbba Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 29 Oct 2024 15:43:47 +0100 Subject: 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* --- models/git/protected_banch_list_test.go | 76 ---------------------- models/git/protected_branch.go | 22 ++++--- models/git/protected_branch_list_test.go | 105 +++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 84 deletions(-) delete mode 100644 models/git/protected_banch_list_test.go create mode 100644 models/git/protected_branch_list_test.go diff --git a/models/git/protected_banch_list_test.go b/models/git/protected_banch_list_test.go deleted file mode 100644 index 4bb3136d58..0000000000 --- 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 0033a42d4e..37d933a982 100644 --- 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 index 0000000000..94a48f37e6 --- /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) +} -- cgit v1.2.3