From 643f476e35aa3e5ea00a567fd189b710306a83b0 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 29 Oct 2024 15:43:47 +0100 Subject: [PATCH 01/13] 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* (cherry picked from commit 5d43801b72790ce5862aefdc4520edb06bb4cbba) --- models/git/protected_branch.go | 22 +++++++++----- ..._test.go => protected_branch_list_test.go} | 29 +++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) rename models/git/{protected_banch_list_test.go => protected_branch_list_test.go} (79%) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index a8b8c81bb..4fc08020e 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -79,14 +79,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_banch_list_test.go b/models/git/protected_branch_list_test.go similarity index 79% rename from models/git/protected_banch_list_test.go rename to models/git/protected_branch_list_test.go index 09319d21a..db7e54f68 100644 --- a/models/git/protected_banch_list_test.go +++ b/models/git/protected_branch_list_test.go @@ -75,3 +75,32 @@ func TestBranchRuleMatchPriority(t *testing.T) { } } } + +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) +} From 42724b09c11b3062b77712dae63530deb4c6a505 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 29 Oct 2024 09:27:03 -0700 Subject: [PATCH 02/13] Fix clean tmp dir (#32360) Try to fix #31792 Credit to @jeroenlaylo Copied from https://github.com/go-gitea/gitea/issues/31792#issuecomment-2311920520 --------- Co-authored-by: wxiaoguang (cherry picked from commit feca8802b85dd75090c533ebdb92835d3d529f17) --- modules/git/repo_index.go | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/modules/git/repo_index.go b/modules/git/repo_index.go index 839057009..f45b6e619 100644 --- a/modules/git/repo_index.go +++ b/modules/git/repo_index.go @@ -50,25 +50,35 @@ func (repo *Repository) readTreeToIndex(id ObjectID, indexFilename ...string) er } // ReadTreeToTemporaryIndex reads a treeish to a temporary index file -func (repo *Repository) ReadTreeToTemporaryIndex(treeish string) (filename, tmpDir string, cancel context.CancelFunc, err error) { - tmpDir, err = os.MkdirTemp("", "index") - if err != nil { - return filename, tmpDir, cancel, err - } +func (repo *Repository) ReadTreeToTemporaryIndex(treeish string) (tmpIndexFilename, tmpDir string, cancel context.CancelFunc, err error) { + defer func() { + // if error happens and there is a cancel function, do clean up + if err != nil && cancel != nil { + cancel() + cancel = nil + } + }() - filename = filepath.Join(tmpDir, ".tmp-index") - cancel = func() { - err := util.RemoveAll(tmpDir) - if err != nil { - log.Error("failed to remove tmp index file: %v", err) + removeDirFn := func(dir string) func() { // it can't use the return value "tmpDir" directly because it is empty when error occurs + return func() { + if err := util.RemoveAll(dir); err != nil { + log.Error("failed to remove tmp index dir: %v", err) + } } } - err = repo.ReadTreeToIndex(treeish, filename) + + tmpDir, err = os.MkdirTemp("", "index") if err != nil { - defer cancel() - return "", "", func() {}, err + return "", "", nil, err } - return filename, tmpDir, cancel, err + + tmpIndexFilename = filepath.Join(tmpDir, ".tmp-index") + cancel = removeDirFn(tmpDir) + err = repo.ReadTreeToIndex(treeish, tmpIndexFilename) + if err != nil { + return "", "", cancel, err + } + return tmpIndexFilename, tmpDir, cancel, err } // EmptyIndex empties the index From e426ce257c2285e3c62233a0577b4acb48c5df0b Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Wed, 30 Oct 2024 00:12:48 -0500 Subject: [PATCH 03/13] remove unused call to $.HeadRepo in view_title template (#32317) This is only populated in [`ParseCompareInfo`](https://github.com/search?q=repo%3Ago-gitea%2Fgitea%20%20.Data%5B%22HeadRepo%22%5D&type=code) which is called in two handlers: * [`CompareAndPullRequestPost`](https://github.com/go-gitea/gitea/blob/9206fbb55fd28f21720072fce6a36cc22277934c/routers/web/repo/pull.go#L1246) - a JSON post handler that doesn't render templates * [`CompareDiff`](https://github.com/go-gitea/gitea/blob/9206fbb55fd28f21720072fce6a36cc22277934c/routers/web/repo/compare.go#L706) - which can render `diff/box.tmpl` and `diff/compare.tmpl` (cherry picked from commit 1cd3f698591edf4fba7880a150b05855cdf40d47) --- templates/repo/issue/view_title.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/repo/issue/view_title.tmpl b/templates/repo/issue/view_title.tmpl index f63cea185..13580ebbf 100644 --- a/templates/repo/issue/view_title.tmpl +++ b/templates/repo/issue/view_title.tmpl @@ -117,7 +117,7 @@ {{$sameBase := ne $.BaseName $.HeadUserName}} {{$differentBranch := ne . $.HeadBranch}} {{if or $sameBase $differentBranch}} -
{{$.BaseName}}{{if $.HeadRepo}}/{{$.HeadRepo}}{{end}}:{{.}}
+
{{$.BaseName}}:{{.}}
{{end}} {{end}} From fe5adbbbdc0862736de37026808d4cd72adf4dab Mon Sep 17 00:00:00 2001 From: Royce Remer Date: Tue, 29 Oct 2024 22:41:55 -0700 Subject: [PATCH 04/13] Add new [lfs_client].BATCH_SIZE and [server].LFS_MAX_BATCH_SIZE config settings. (#32307) This contains two backwards-compatible changes: * in the lfs http_client, the number of lfs oids requested per batch is loaded from lfs_client#BATCH_SIZE and defaulted to the previous value of 20 * in the lfs server/service, the max number of lfs oids allowed in a batch api request is loaded from server#LFS_MAX_BATCH_SIZE and defaults to 'nil' which equates to the previous behavior of 'infinite' This fixes #32306 --------- Signed-off-by: Royce Remer Co-authored-by: wxiaoguang (cherry picked from commit c60e4dc1095ef90a790582cacfad27c972637bb2) Conflicts: - services/lfs/server.go Conflict due to our Quota implementation. Resolved by manually adding the change after the quota check. --- custom/conf/app.example.ini | 8 ++++++++ modules/lfs/http_client.go | 5 ++--- modules/setting/lfs.go | 21 +++++++++++++++++---- modules/setting/lfs_test.go | 16 ++++++++++++++++ services/lfs/server.go | 5 +++++ 5 files changed, 48 insertions(+), 7 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 3f0e9c447..f0fd40da5 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -328,6 +328,10 @@ RUN_USER = ; git ;; Maximum number of locks returned per page ;LFS_LOCKS_PAGING_NUM = 50 ;; +;; When clients make lfs batch requests, reject them if there are more pointers than this number +;; zero means 'unlimited' +;LFS_MAX_BATCH_SIZE = 0 +;; ;; Allow graceful restarts using SIGHUP to fork ;ALLOW_GRACEFUL_RESTARTS = true ;; @@ -2672,6 +2676,10 @@ LEVEL = Info ;; override the minio base path if storage type is minio ;MINIO_BASE_PATH = lfs/ +;[lfs_client] +;; When mirroring an upstream lfs endpoint, limit the number of pointers in each batch request to this number +;BATCH_SIZE = 20 + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; settings for packages, will override storage setting diff --git a/modules/lfs/http_client.go b/modules/lfs/http_client.go index 4859fe61e..aa9e744d7 100644 --- a/modules/lfs/http_client.go +++ b/modules/lfs/http_client.go @@ -16,10 +16,9 @@ import ( "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/proxy" + "code.gitea.io/gitea/modules/setting" ) -const httpBatchSize = 20 - // HTTPClient is used to communicate with the LFS server // https://github.com/git-lfs/git-lfs/blob/main/docs/api/batch.md type HTTPClient struct { @@ -30,7 +29,7 @@ type HTTPClient struct { // BatchSize returns the preferred size of batchs to process func (c *HTTPClient) BatchSize() int { - return httpBatchSize + return setting.LFSClient.BatchSize } func newHTTPClient(endpoint *url.URL, httpTransport *http.Transport) *HTTPClient { diff --git a/modules/setting/lfs.go b/modules/setting/lfs.go index 750101747..ebb234a3e 100644 --- a/modules/setting/lfs.go +++ b/modules/setting/lfs.go @@ -10,22 +10,31 @@ import ( "code.gitea.io/gitea/modules/generate" ) -// LFS represents the configuration for Git LFS +// LFS represents the server-side configuration for Git LFS. +// Ideally these options should be in a section like "[lfs_server]", +// but they are in "[server]" section due to historical reasons. +// Could be refactored in the future while keeping backwards compatibility. var LFS = struct { StartServer bool `ini:"LFS_START_SERVER"` JWTSecretBytes []byte `ini:"-"` HTTPAuthExpiry time.Duration `ini:"LFS_HTTP_AUTH_EXPIRY"` MaxFileSize int64 `ini:"LFS_MAX_FILE_SIZE"` LocksPagingNum int `ini:"LFS_LOCKS_PAGING_NUM"` + MaxBatchSize int `ini:"LFS_MAX_BATCH_SIZE"` Storage *Storage }{} +// LFSClient represents configuration for Gitea's LFS clients, for example: mirroring upstream Git LFS +var LFSClient = struct { + BatchSize int `ini:"BATCH_SIZE"` +}{} + func loadLFSFrom(rootCfg ConfigProvider) error { + mustMapSetting(rootCfg, "lfs_client", &LFSClient) + + mustMapSetting(rootCfg, "server", &LFS) sec := rootCfg.Section("server") - if err := sec.MapTo(&LFS); err != nil { - return fmt.Errorf("failed to map LFS settings: %v", err) - } lfsSec, _ := rootCfg.GetSection("lfs") @@ -52,6 +61,10 @@ func loadLFSFrom(rootCfg ConfigProvider) error { LFS.LocksPagingNum = 50 } + if LFSClient.BatchSize < 1 { + LFSClient.BatchSize = 20 + } + LFS.HTTPAuthExpiry = sec.Key("LFS_HTTP_AUTH_EXPIRY").MustDuration(24 * time.Hour) if !LFS.StartServer || !InstallLock { diff --git a/modules/setting/lfs_test.go b/modules/setting/lfs_test.go index c7f16379b..324965781 100644 --- a/modules/setting/lfs_test.go +++ b/modules/setting/lfs_test.go @@ -100,3 +100,19 @@ STORAGE_TYPE = minio assert.EqualValues(t, "gitea", LFS.Storage.MinioConfig.Bucket) assert.EqualValues(t, "lfs/", LFS.Storage.MinioConfig.BasePath) } + +func Test_LFSClientServerConfigs(t *testing.T) { + iniStr := ` +[server] +LFS_MAX_BATCH_SIZE = 100 +[lfs_client] +# will default to 20 +BATCH_SIZE = 0 +` + cfg, err := NewConfigProviderFromData(iniStr) + assert.NoError(t, err) + + assert.NoError(t, loadLFSFrom(cfg)) + assert.EqualValues(t, 100, LFS.MaxBatchSize) + assert.EqualValues(t, 20, LFSClient.BatchSize) +} diff --git a/services/lfs/server.go b/services/lfs/server.go index a300de19c..225dfdb02 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -192,6 +192,11 @@ func BatchHandler(ctx *context.Context) { } } + if setting.LFS.MaxBatchSize != 0 && len(br.Objects) > setting.LFS.MaxBatchSize { + writeStatus(ctx, http.StatusRequestEntityTooLarge) + return + } + contentStore := lfs_module.NewContentStore() var responseObjects []*lfs_module.ObjectResponse From 2a38208004b124827eeea5322d3edbd9e8ed73a3 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Thu, 31 Oct 2024 04:05:40 +0900 Subject: [PATCH 05/13] Fix the missing menu in organization project view page (#32313) #29248 didn't modify the view page. The class name is not good enough, so this is a quick fix. Before: org: ![image](https://github.com/user-attachments/assets/3e26502d-66b4-4043-ab03-003ba7391487) user: ![image](https://github.com/user-attachments/assets/9b22b90c-d63c-4228-acad-4d9fb20590ac) After: org: ![image](https://github.com/user-attachments/assets/21bf98a7-8a5b-4dc6-950a-88f529e36450) user: (no change) ![image](https://github.com/user-attachments/assets/fea0dcae-3625-44e8-bb9e-4c3733da8764) Co-authored-by: Giteabot (cherry picked from commit dd1f67491f5e2f798a537a61c082b1bf12e47635) --- templates/org/projects/view.tmpl | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/templates/org/projects/view.tmpl b/templates/org/projects/view.tmpl index e1ab81c4c..bd74114fe 100644 --- a/templates/org/projects/view.tmpl +++ b/templates/org/projects/view.tmpl @@ -1,9 +1,13 @@ {{template "base/head" .}} -
- {{template "shared/user/org_profile_avatar" .}} -
- {{template "user/overview/header" .}} -
+
+ {{if .ContextUser.IsOrganization}} + {{template "org/header" .}} + {{else}} + {{template "shared/user/org_profile_avatar" .}} +
+ {{template "user/overview/header" .}} +
+ {{end}}
{{template "projects/view" .}}
From 4aa61601c3db92fcd6f0eac55c774d6ab72156b8 Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Wed, 30 Oct 2024 21:36:24 +0200 Subject: [PATCH 06/13] refactor: remove redundant err declarations (#32381) (cherry picked from commit f4d3aaeeb9e1b11c5495e4608a3f52f316c35758) Conflicts: - modules/charset/charset_test.go Resolved by manually changing a `=` to `:=`, as per the original patch. Conflict was due to `require.NoError`. --- build/generate-emoji.go | 4 ---- models/git/lfs.go | 2 -- models/issues/label_test.go | 3 +-- modules/charset/charset_test.go | 4 +--- modules/markup/markdown/goldmark.go | 3 +-- routers/api/v1/repo/repo.go | 1 - routers/api/v1/user/repo.go | 1 - routers/web/repo/actions/view.go | 1 - routers/web/repo/activity.go | 1 - routers/web/repo/view.go | 1 - services/issue/template.go | 2 -- 11 files changed, 3 insertions(+), 20 deletions(-) diff --git a/build/generate-emoji.go b/build/generate-emoji.go index 5a88e456e..98c2f15d7 100644 --- a/build/generate-emoji.go +++ b/build/generate-emoji.go @@ -53,8 +53,6 @@ func (e Emoji) MarshalJSON() ([]byte, error) { } func main() { - var err error - flag.Parse() // generate data @@ -83,8 +81,6 @@ var replacer = strings.NewReplacer( var emojiRE = regexp.MustCompile(`\{Emoji:"([^"]*)"`) func generate() ([]byte, error) { - var err error - // load gemoji data res, err := http.Get(gemojiURL) if err != nil { diff --git a/models/git/lfs.go b/models/git/lfs.go index 44b741c4c..635d70d9b 100644 --- a/models/git/lfs.go +++ b/models/git/lfs.go @@ -136,8 +136,6 @@ var ErrLFSObjectNotExist = db.ErrNotExist{Resource: "LFS Meta object"} // NewLFSMetaObject stores a given populated LFSMetaObject structure in the database // if it is not already present. func NewLFSMetaObject(ctx context.Context, repoID int64, p lfs.Pointer) (*LFSMetaObject, error) { - var err error - ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err diff --git a/models/issues/label_test.go b/models/issues/label_test.go index b03fc1cd2..3e3d09745 100644 --- a/models/issues/label_test.go +++ b/models/issues/label_test.go @@ -231,8 +231,7 @@ func TestGetLabelsByOrgID(t *testing.T) { testSuccess(3, "reversealphabetically", []int64{4, 3}) testSuccess(3, "default", []int64{3, 4}) - var err error - _, err = issues_model.GetLabelsByOrgID(db.DefaultContext, 0, "leastissues", db.ListOptions{}) + _, err := issues_model.GetLabelsByOrgID(db.DefaultContext, 0, "leastissues", db.ListOptions{}) assert.True(t, issues_model.IsErrOrgLabelNotExist(err)) _, err = issues_model.GetLabelsByOrgID(db.DefaultContext, -1, "leastissues", db.ListOptions{}) diff --git a/modules/charset/charset_test.go b/modules/charset/charset_test.go index 42c841537..f7ea2becc 100644 --- a/modules/charset/charset_test.go +++ b/modules/charset/charset_test.go @@ -41,14 +41,12 @@ func TestMaybeRemoveBOM(t *testing.T) { func TestToUTF8(t *testing.T) { resetDefaultCharsetsOrder() - var res string - var err error // Note: golang compiler seems so behave differently depending on the current // locale, so some conversions might behave differently. For that reason, we don't // depend on particular conversions but in expected behaviors. - res, err = ToUTF8([]byte{0x41, 0x42, 0x43}, ConvertOpts{}) + res, err := ToUTF8([]byte{0x41, 0x42, 0x43}, ConvertOpts{}) require.NoError(t, err) assert.Equal(t, "ABC", res) diff --git a/modules/markup/markdown/goldmark.go b/modules/markup/markdown/goldmark.go index 0290e1312..1d3e04224 100644 --- a/modules/markup/markdown/goldmark.go +++ b/modules/markup/markdown/goldmark.go @@ -203,8 +203,7 @@ func (r *HTMLRenderer) renderIcon(w util.BufWriter, source []byte, node ast.Node return ast.WalkContinue, nil } - var err error - _, err = w.WriteString(fmt.Sprintf(``, name)) + _, err := w.WriteString(fmt.Sprintf(``, name)) if err != nil { return ast.WalkStop, err } diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index e2e784a67..2c4a65934 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -202,7 +202,6 @@ func Search(ctx *context.APIContext) { } } - var err error repos, count, err := repo_model.SearchRepository(ctx, opts) if err != nil { ctx.JSON(http.StatusInternalServerError, api.SearchError{ diff --git a/routers/api/v1/user/repo.go b/routers/api/v1/user/repo.go index 86716ff44..3b304c30b 100644 --- a/routers/api/v1/user/repo.go +++ b/routers/api/v1/user/repo.go @@ -130,7 +130,6 @@ func ListMyRepos(ctx *context.APIContext) { return } - var err error repos, count, err := repo_model.SearchRepository(ctx, opts) if err != nil { ctx.Error(http.StatusInternalServerError, "SearchRepository", err) diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index bc1ecbfc1..a343f60a9 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -307,7 +307,6 @@ func ViewPost(ctx *context_module.Context) { if validCursor { length := step.LogLength - cursor.Cursor offset := task.LogIndexes[index] - var err error logRows, err := actions.ReadLogs(ctx, task.LogInStorage, task.LogFilename, offset, length) if err != nil { ctx.Error(http.StatusInternalServerError, err.Error()) diff --git a/routers/web/repo/activity.go b/routers/web/repo/activity.go index ba776c84d..351578e1d 100644 --- a/routers/web/repo/activity.go +++ b/routers/web/repo/activity.go @@ -94,7 +94,6 @@ func ActivityAuthors(ctx *context.Context) { timeFrom = timeUntil.Add(-time.Hour * 168) } - var err error authors, err := activities_model.GetActivityStatsTopAuthors(ctx, ctx.Repo.Repository, timeFrom, 10) if err != nil { ctx.ServerError("GetActivityStatsTopAuthors", err) diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index f1445c580..41ff5f97f 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -147,7 +147,6 @@ func FindReadmeFileInEntries(ctx *context.Context, entries []*git.TreeEntry, try // this should be impossible; if subTreeEntry exists so should this. continue } - var err error childEntries, err := subTree.ListEntries() if err != nil { return "", nil, err diff --git a/services/issue/template.go b/services/issue/template.go index 47633e5d8..9a2b04840 100644 --- a/services/issue/template.go +++ b/services/issue/template.go @@ -56,8 +56,6 @@ func GetTemplateConfig(gitRepo *git.Repository, path string, commit *git.Commit) return GetDefaultTemplateConfig(), nil } - var err error - treeEntry, err := commit.GetTreeEntryByPath(path) if err != nil { return GetDefaultTemplateConfig(), err From 6b74043b85ee03215705e789cebf79bc6f604bda Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Thu, 31 Oct 2024 23:28:25 +0800 Subject: [PATCH 07/13] Fix `missing signature key` error when pulling Docker images with `SERVE_DIRECT` enabled (#32365) Fix #28121 I did some tests and found that the `missing signature key` error is caused by an incorrect `Content-Type` header. Gitea correctly sets the `Content-Type` header when serving files. https://github.com/go-gitea/gitea/blob/348d1d0f322ca57c459acd902f54821d687ca804/routers/api/packages/container/container.go#L712-L717 However, when `SERVE_DIRECT` is enabled, the `Content-Type` header may be set to an incorrect value by the storage service. To fix this issue, we can use query parameters to override response header values. https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html In this PR, I introduced a new parameter to the `URL` method to support additional parameters. ``` URL(path, name string, reqParams url.Values) (*url.URL, error) ``` --- Most S3-like services support specifying the content type when storing objects. However, Gitea always use `application/octet-stream`. Therefore, I believe we also need to improve the `Save` method to support storing objects with the correct content type. https://github.com/go-gitea/gitea/blob/b7fb20e73e63b8edc9b90c52073e248bef428fcc/modules/storage/minio.go#L214-L221 (cherry picked from commit 0690cb076bf63f71988a709f62a9c04660b51a4f) Conflicts: - modules/storage/azureblob.go Dropped the change, as we do not support Azure blob storage. - modules/storage/helper.go Resolved by adjusting their `discardStorage` to our `DiscardStorage` - routers/api/actions/artifacts.go routers/api/actions/artifactsv4.go routers/web/repo/actions/view.go routers/web/repo/download.go Resolved the conflicts by manually adding the new `nil` parameter to the `storage.Attachments.URL()` calls. Originally conflicted due to differences in the if expression above these calls. --- modules/packages/content_store.go | 4 ++-- modules/storage/helper.go | 2 +- modules/storage/helper_test.go | 2 +- modules/storage/local.go | 2 +- modules/storage/minio.go | 8 ++++++-- modules/storage/storage.go | 2 +- routers/api/actions/artifacts.go | 2 +- routers/api/actions/artifactsv4.go | 2 +- routers/api/packages/container/container.go | 4 +++- routers/api/packages/maven/maven.go | 2 +- routers/api/v1/repo/file.go | 4 ++-- routers/web/base.go | 2 +- routers/web/repo/actions/view.go | 3 ++- routers/web/repo/attachment.go | 2 +- routers/web/repo/download.go | 4 ++-- routers/web/repo/repo.go | 2 +- services/lfs/server.go | 2 +- services/packages/packages.go | 6 +++--- 18 files changed, 31 insertions(+), 24 deletions(-) diff --git a/modules/packages/content_store.go b/modules/packages/content_store.go index da93e6cf6..6438fb174 100644 --- a/modules/packages/content_store.go +++ b/modules/packages/content_store.go @@ -37,8 +37,8 @@ func (s *ContentStore) ShouldServeDirect() bool { return setting.Packages.Storage.MinioConfig.ServeDirect } -func (s *ContentStore) GetServeDirectURL(key BlobHash256Key, filename string) (*url.URL, error) { - return s.store.URL(KeyToRelativePath(key), filename) +func (s *ContentStore) GetServeDirectURL(key BlobHash256Key, filename string, reqParams url.Values) (*url.URL, error) { + return s.store.URL(KeyToRelativePath(key), filename, reqParams) } // FIXME: Workaround to be removed in v1.20 diff --git a/modules/storage/helper.go b/modules/storage/helper.go index 95f1c7b9a..8bec3a004 100644 --- a/modules/storage/helper.go +++ b/modules/storage/helper.go @@ -30,7 +30,7 @@ func (s DiscardStorage) Delete(_ string) error { return fmt.Errorf("%s", s) } -func (s DiscardStorage) URL(_, _ string) (*url.URL, error) { +func (s DiscardStorage) URL(_, _ string, _ url.Values) (*url.URL, error) { return nil, fmt.Errorf("%s", s) } diff --git a/modules/storage/helper_test.go b/modules/storage/helper_test.go index 60a7c6128..dd30c9b8a 100644 --- a/modules/storage/helper_test.go +++ b/modules/storage/helper_test.go @@ -38,7 +38,7 @@ func Test_discardStorage(t *testing.T) { require.Error(t, err, string(tt)) } { - got, err := tt.URL("path", "name") + got, err := tt.URL("path", "name", nil) assert.Nil(t, got) require.Errorf(t, err, string(tt)) } diff --git a/modules/storage/local.go b/modules/storage/local.go index 9bb532f1d..00c7f668a 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -114,7 +114,7 @@ func (l *LocalStorage) Delete(path string) error { } // URL gets the redirect URL to a file -func (l *LocalStorage) URL(path, name string) (*url.URL, error) { +func (l *LocalStorage) URL(path, name string, reqParams url.Values) (*url.URL, error) { return nil, ErrURLNotSupported } diff --git a/modules/storage/minio.go b/modules/storage/minio.go index d0c2dec65..b02eec7aa 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -276,8 +276,12 @@ func (m *MinioStorage) Delete(path string) error { } // URL gets the redirect URL to a file. The presigned link is valid for 5 minutes. -func (m *MinioStorage) URL(path, name string) (*url.URL, error) { - reqParams := make(url.Values) +func (m *MinioStorage) URL(path, name string, serveDirectReqParams url.Values) (*url.URL, error) { + // copy serveDirectReqParams + reqParams, err := url.ParseQuery(serveDirectReqParams.Encode()) + if err != nil { + return nil, err + } // TODO it may be good to embed images with 'inline' like ServeData does, but we don't want to have to read the file, do we? reqParams.Set("response-content-disposition", "attachment; filename=\""+quoteEscaper.Replace(name)+"\"") u, err := m.client.PresignedGetObject(m.ctx, m.bucket, m.buildMinioPath(path), 5*time.Minute, reqParams) diff --git a/modules/storage/storage.go b/modules/storage/storage.go index b83b1c792..9cc694925 100644 --- a/modules/storage/storage.go +++ b/modules/storage/storage.go @@ -63,7 +63,7 @@ type ObjectStorage interface { Save(path string, r io.Reader, size int64) (int64, error) Stat(path string) (os.FileInfo, error) Delete(path string) error - URL(path, name string) (*url.URL, error) + URL(path, name string, reqParams url.Values) (*url.URL, error) IterateObjects(path string, iterator func(path string, obj Object) error) error } diff --git a/routers/api/actions/artifacts.go b/routers/api/actions/artifacts.go index bc29e4481..405686a05 100644 --- a/routers/api/actions/artifacts.go +++ b/routers/api/actions/artifacts.go @@ -437,7 +437,7 @@ func (ar artifactRoutes) getDownloadArtifactURL(ctx *ArtifactContext) { for _, artifact := range artifacts { var downloadURL string if setting.Actions.ArtifactStorage.MinioConfig.ServeDirect { - u, err := ar.fs.URL(artifact.StoragePath, artifact.ArtifactName) + u, err := ar.fs.URL(artifact.StoragePath, artifact.ArtifactName, nil) if err != nil && !errors.Is(err, storage.ErrURLNotSupported) { log.Error("Error getting serve direct url: %v", err) } diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 677e89da2..0417f9824 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -530,7 +530,7 @@ func (r *artifactV4Routes) getSignedArtifactURL(ctx *ArtifactContext) { respData := GetSignedArtifactURLResponse{} if setting.Actions.ArtifactStorage.MinioConfig.ServeDirect { - u, err := storage.ActionsArtifacts.URL(artifact.StoragePath, artifact.ArtifactPath) + u, err := storage.ActionsArtifacts.URL(artifact.StoragePath, artifact.ArtifactPath, nil) if u != nil && err == nil { respData.SignedUrl = u.String() } diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index f376e7bc5..9c9da3842 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -689,7 +689,9 @@ func DeleteManifest(ctx *context.Context) { } func serveBlob(ctx *context.Context, pfd *packages_model.PackageFileDescriptor) { - s, u, _, err := packages_service.GetPackageBlobStream(ctx, pfd.File, pfd.Blob) + serveDirectReqParams := make(url.Values) + serveDirectReqParams.Set("response-content-type", pfd.Properties.GetByName(container_module.PropertyMediaType)) + s, u, _, err := packages_service.GetPackageBlobStream(ctx, pfd.File, pfd.Blob, serveDirectReqParams) if err != nil { apiError(ctx, http.StatusInternalServerError, err) return diff --git a/routers/api/packages/maven/maven.go b/routers/api/packages/maven/maven.go index 418157745..521ef2209 100644 --- a/routers/api/packages/maven/maven.go +++ b/routers/api/packages/maven/maven.go @@ -217,7 +217,7 @@ func servePackageFile(ctx *context.Context, params parameters, serveContent bool return } - s, u, _, err := packages_service.GetPackageBlobStream(ctx, pf, pb) + s, u, _, err := packages_service.GetPackageBlobStream(ctx, pf, pb, nil) if err != nil { apiError(ctx, http.StatusInternalServerError, err) return diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 1fa44d50c..50d2786ec 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -214,7 +214,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) { if setting.LFS.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.LFS.URL(pointer.RelativePath(), blob.Name()) + u, err := storage.LFS.URL(pointer.RelativePath(), blob.Name(), nil) if u != nil && err == nil { ctx.Redirect(u.String()) return @@ -341,7 +341,7 @@ func download(ctx *context.APIContext, archiveName string, archiver *repo_model. rPath := archiver.RelativePath() if setting.RepoArchive.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.RepoArchives.URL(rPath, downloadName) + u, err := storage.RepoArchives.URL(rPath, downloadName, nil) if u != nil && err == nil { ctx.Redirect(u.String()) return diff --git a/routers/web/base.go b/routers/web/base.go index 78dde57fa..285d1ecdd 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -39,7 +39,7 @@ func storageHandler(storageSetting *setting.Storage, prefix string, objStore sto rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") rPath = util.PathJoinRelX(rPath) - u, err := objStore.URL(rPath, path.Base(rPath)) + u, err := objStore.URL(rPath, path.Base(rPath), nil) if err != nil { if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { log.Warn("Unable to find %s %s", prefix, rPath) diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index a343f60a9..e7dbb6d97 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -688,7 +688,8 @@ func ArtifactsDownloadView(ctx *context_module.Context) { if len(artifacts) == 1 && artifacts[0].ArtifactName+".zip" == artifacts[0].ArtifactPath && artifacts[0].ContentEncoding == "application/zip" { art := artifacts[0] if setting.Actions.ArtifactStorage.MinioConfig.ServeDirect { - u, err := storage.ActionsArtifacts.URL(art.StoragePath, art.ArtifactPath) + u, err := storage.ActionsArtifacts.URL(art.StoragePath, art.ArtifactPath, nil) + if u != nil && err == nil { ctx.Redirect(u.String()) return diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index b42effd8c..b5078e1f6 100644 --- a/routers/web/repo/attachment.go +++ b/routers/web/repo/attachment.go @@ -134,7 +134,7 @@ func ServeAttachment(ctx *context.Context, uuid string) { if setting.Attachment.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.Attachments.URL(attach.RelativePath(), attach.Name) + u, err := storage.Attachments.URL(attach.RelativePath(), attach.Name, nil) if u != nil && err == nil { ctx.Redirect(u.String()) diff --git a/routers/web/repo/download.go b/routers/web/repo/download.go index c4a8baecc..1e87bbf01 100644 --- a/routers/web/repo/download.go +++ b/routers/web/repo/download.go @@ -54,8 +54,8 @@ func ServeBlobOrLFS(ctx *context.Context, blob *git.Blob, lastModified *time.Tim } if setting.LFS.Storage.MinioConfig.ServeDirect { - // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.LFS.URL(pointer.RelativePath(), blob.Name()) + // If we have a signed url (S3, object storage, blob storage), redirect to this directly. + u, err := storage.LFS.URL(pointer.RelativePath(), blob.Name(), nil) if u != nil && err == nil { ctx.Redirect(u.String()) return nil diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 956249144..8036bcae6 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -505,7 +505,7 @@ func download(ctx *context.Context, archiveName string, archiver *repo_model.Rep rPath := archiver.RelativePath() if setting.RepoArchive.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.RepoArchives.URL(rPath, downloadName) + u, err := storage.RepoArchives.URL(rPath, downloadName, nil) if u != nil && err == nil { if archiver.ReleaseID != 0 { err = repo_model.CountArchiveDownload(ctx, ctx.Repo.Repository.ID, archiver.ReleaseID, archiver.Type) diff --git a/services/lfs/server.go b/services/lfs/server.go index 225dfdb02..51d6f4277 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -485,7 +485,7 @@ func buildObjectResponse(rc *requestContext, pointer lfs_module.Pointer, downloa var link *lfs_module.Link if setting.LFS.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.LFS.URL(pointer.RelativePath(), pointer.Oid) + u, err := storage.LFS.URL(pointer.RelativePath(), pointer.Oid, nil) if u != nil && err == nil { // Presigned url does not need the Authorization header // https://github.com/go-gitea/gitea/issues/21525 diff --git a/services/packages/packages.go b/services/packages/packages.go index a5b84506d..72ab19ee2 100644 --- a/services/packages/packages.go +++ b/services/packages/packages.go @@ -602,12 +602,12 @@ func GetPackageFileStream(ctx context.Context, pf *packages_model.PackageFile) ( return nil, nil, nil, err } - return GetPackageBlobStream(ctx, pf, pb) + return GetPackageBlobStream(ctx, pf, pb, nil) } // GetPackageBlobStream returns the content of the specific package blob // If the storage supports direct serving and it's enabled, only the direct serving url is returned. -func GetPackageBlobStream(ctx context.Context, pf *packages_model.PackageFile, pb *packages_model.PackageBlob) (io.ReadSeekCloser, *url.URL, *packages_model.PackageFile, error) { +func GetPackageBlobStream(ctx context.Context, pf *packages_model.PackageFile, pb *packages_model.PackageBlob, serveDirectReqParams url.Values) (io.ReadSeekCloser, *url.URL, *packages_model.PackageFile, error) { key := packages_module.BlobHash256Key(pb.HashSHA256) cs := packages_module.NewContentStore() @@ -617,7 +617,7 @@ func GetPackageBlobStream(ctx context.Context, pf *packages_model.PackageFile, p var err error if cs.ShouldServeDirect() { - u, err = cs.GetServeDirectURL(key, pf.Name) + u, err = cs.GetServeDirectURL(key, pf.Name, serveDirectReqParams) if err != nil && !errors.Is(err, storage.ErrURLNotSupported) { log.Error("Error getting serve direct url: %v", err) } From 748ae10e7c1805c7ec4e16f5ca0b144cf5479f5e Mon Sep 17 00:00:00 2001 From: "Kyle D." Date: Thu, 31 Oct 2024 22:29:54 -0400 Subject: [PATCH 08/13] Add artifacts test fixture (#30300) Closes https://github.com/go-gitea/gitea/issues/30296 - Adds a DB fixture for actions artifacts - Adds artifacts test files - Clears artifacts test files between each run - Note: I initially initialized the artifacts only for artifacts tests, but because the files are small it only takes ~8ms, so I changed it to always run in test setup for simplicity - Fix some otherwise flaky tests by making them not depend on previous tests (cherry picked from commit 66971e591e5dddd5b6dc1572ac48f4e4ab29b8e0) Conflicts: - tests/integration/api_actions_artifact_test.go Conflict resolved by manually changing the tested artifact name from "artifact" to "artifact-download" - tests/integration/api_actions_artifact_v4_test.go Conflict resolved by manually updating the tested artifact names, and adjusting the test case only present in our tree. - tests/test_utils.go Resolved by manually copying the added function. --- models/fixtures/action_artifact.yml | 71 ++++++++++++++++++ modules/storage/storage.go | 2 +- .../integration/api_actions_artifact_test.go | 74 +++++++++++-------- .../api_actions_artifact_v4_test.go | 24 +++--- tests/test_utils.go | 17 +++++ .../artifacts/26/1/1712166500347189545.chunk | 1 + .../artifacts/26/19/1712348022422036662.chunk | 1 + .../artifacts/26/20/1712348022423431524.chunk | 1 + .../artifacts/27/5/1730330775594233150.chunk | 1 + 9 files changed, 147 insertions(+), 45 deletions(-) create mode 100644 models/fixtures/action_artifact.yml create mode 100644 tests/testdata/data/artifacts/26/1/1712166500347189545.chunk create mode 100644 tests/testdata/data/artifacts/26/19/1712348022422036662.chunk create mode 100644 tests/testdata/data/artifacts/26/20/1712348022423431524.chunk create mode 100644 tests/testdata/data/artifacts/27/5/1730330775594233150.chunk diff --git a/models/fixtures/action_artifact.yml b/models/fixtures/action_artifact.yml new file mode 100644 index 000000000..2c51c11eb --- /dev/null +++ b/models/fixtures/action_artifact.yml @@ -0,0 +1,71 @@ +- + id: 1 + run_id: 791 + runner_id: 1 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + storage_path: "26/1/1712166500347189545.chunk" + file_size: 1024 + file_compressed_size: 1024 + content_encoding: "" + artifact_path: "abc.txt" + artifact_name: "artifact-download" + status: 1 + created_unix: 1712338649 + updated_unix: 1712338649 + expired_unix: 1720114649 + +- + id: 19 + run_id: 791 + runner_id: 1 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + storage_path: "26/19/1712348022422036662.chunk" + file_size: 1024 + file_compressed_size: 1024 + content_encoding: "" + artifact_path: "abc.txt" + artifact_name: "multi-file-download" + status: 2 + created_unix: 1712348022 + updated_unix: 1712348022 + expired_unix: 1720124022 + +- + id: 20 + run_id: 791 + runner_id: 1 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + storage_path: "26/20/1712348022423431524.chunk" + file_size: 1024 + file_compressed_size: 1024 + content_encoding: "" + artifact_path: "xyz/def.txt" + artifact_name: "multi-file-download" + status: 2 + created_unix: 1712348022 + updated_unix: 1712348022 + expired_unix: 1720124022 + +- + id: 22 + run_id: 792 + runner_id: 1 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + storage_path: "27/5/1730330775594233150.chunk" + file_size: 1024 + file_compressed_size: 1024 + content_encoding: "application/zip" + artifact_path: "artifact-v4-download.zip" + artifact_name: "artifact-v4-download" + status: 2 + created_unix: 1730330775 + updated_unix: 1730330775 + expired_unix: 1738106775 diff --git a/modules/storage/storage.go b/modules/storage/storage.go index 9cc694925..d944b8618 100644 --- a/modules/storage/storage.go +++ b/modules/storage/storage.go @@ -131,7 +131,7 @@ var ( ActionsArtifacts ObjectStorage = UninitializedStorage ) -// Init init the stoarge +// Init init the storage func Init() error { for _, f := range []func() error{ initAttachments, diff --git a/tests/integration/api_actions_artifact_test.go b/tests/integration/api_actions_artifact_test.go index 2798024c1..7063d9934 100644 --- a/tests/integration/api_actions_artifact_test.go +++ b/tests/integration/api_actions_artifact_test.go @@ -38,21 +38,21 @@ func TestActionsArtifactUploadSingleFile(t *testing.T) { // get upload url idx := strings.Index(uploadResp.FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") - url := uploadResp.FileContainerResourceURL[idx:] + "?itemPath=artifact/abc.txt" + url := uploadResp.FileContainerResourceURL[idx:] + "?itemPath=artifact/abc-2.txt" // upload artifact chunk - body := strings.Repeat("A", 1024) + body := strings.Repeat("C", 1024) req = NewRequestWithBody(t, "PUT", url, strings.NewReader(body)). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a"). SetHeader("Content-Range", "bytes 0-1023/1024"). SetHeader("x-tfs-filelength", "1024"). - SetHeader("x-actions-results-md5", "1HsSe8LeLWh93ILaw1TEFQ==") // base64(md5(body)) + SetHeader("x-actions-results-md5", "XVlf820rMInUi64wmMi6EA==") // base64(md5(body)) MakeRequest(t, req, http.StatusOK) t.Logf("Create artifact confirm") // confirm artifact upload - req = NewRequest(t, "PATCH", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts?artifactName=artifact"). + req = NewRequest(t, "PATCH", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts?artifactName=artifact-single"). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") MakeRequest(t, req, http.StatusOK) } @@ -115,29 +115,40 @@ func TestActionsArtifactDownload(t *testing.T) { resp := MakeRequest(t, req, http.StatusOK) var listResp listArtifactsResponse DecodeJSON(t, resp, &listResp) - assert.Equal(t, int64(1), listResp.Count) - assert.Equal(t, "artifact", listResp.Value[0].Name) - assert.Contains(t, listResp.Value[0].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") + assert.Equal(t, int64(2), listResp.Count) - idx := strings.Index(listResp.Value[0].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") - url := listResp.Value[0].FileContainerResourceURL[idx+1:] + "?itemPath=artifact" + // Return list might be in any order. Get one file. + var artifactIdx int + for i, artifact := range listResp.Value { + if artifact.Name == "artifact-download" { + artifactIdx = i + break + } + } + assert.NotNil(t, artifactIdx) + assert.Equal(t, listResp.Value[artifactIdx].Name, "artifact-download") + assert.Contains(t, listResp.Value[artifactIdx].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") + + idx := strings.Index(listResp.Value[artifactIdx].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") + url := listResp.Value[artifactIdx].FileContainerResourceURL[idx+1:] + "?itemPath=artifact-download" req = NewRequest(t, "GET", url). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) var downloadResp downloadArtifactResponse DecodeJSON(t, resp, &downloadResp) assert.Len(t, downloadResp.Value, 1) - assert.Equal(t, "artifact/abc.txt", downloadResp.Value[0].Path) - assert.Equal(t, "file", downloadResp.Value[0].ItemType) - assert.Contains(t, downloadResp.Value[0].ContentLocation, "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") + assert.Equal(t, "artifact-download/abc.txt", downloadResp.Value[artifactIdx].Path) + assert.Equal(t, "file", downloadResp.Value[artifactIdx].ItemType) + assert.Contains(t, downloadResp.Value[artifactIdx].ContentLocation, "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") - idx = strings.Index(downloadResp.Value[0].ContentLocation, "/api/actions_pipeline/_apis/pipelines/") - url = downloadResp.Value[0].ContentLocation[idx:] + idx = strings.Index(downloadResp.Value[artifactIdx].ContentLocation, "/api/actions_pipeline/_apis/pipelines/") + url = downloadResp.Value[artifactIdx].ContentLocation[idx:] req = NewRequest(t, "GET", url). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) + body := strings.Repeat("A", 1024) - assert.Equal(t, resp.Body.String(), body) + assert.Equal(t, body, resp.Body.String()) } func TestActionsArtifactUploadMultipleFile(t *testing.T) { @@ -163,14 +174,14 @@ func TestActionsArtifactUploadMultipleFile(t *testing.T) { files := []uploadingFile{ { - Path: "abc.txt", - Content: strings.Repeat("A", 1024), - MD5: "1HsSe8LeLWh93ILaw1TEFQ==", + Path: "abc-3.txt", + Content: strings.Repeat("D", 1024), + MD5: "9nqj7E8HZmfQtPifCJ5Zww==", }, { - Path: "xyz/def.txt", - Content: strings.Repeat("B", 1024), - MD5: "6fgADK/7zjadf+6cB9Q1CQ==", + Path: "xyz/def-2.txt", + Content: strings.Repeat("E", 1024), + MD5: "/s1kKvxeHlUX85vaTaVxuA==", }, } @@ -199,7 +210,7 @@ func TestActionsArtifactUploadMultipleFile(t *testing.T) { func TestActionsArtifactDownloadMultiFiles(t *testing.T) { defer tests.PrepareTestEnv(t)() - const testArtifactName = "multi-files" + const testArtifactName = "multi-file-download" req := NewRequest(t, "GET", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts"). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") @@ -226,7 +237,7 @@ func TestActionsArtifactDownloadMultiFiles(t *testing.T) { DecodeJSON(t, resp, &downloadResp) assert.Len(t, downloadResp.Value, 2) - downloads := [][]string{{"multi-files/abc.txt", "A"}, {"multi-files/xyz/def.txt", "B"}} + downloads := [][]string{{"multi-file-download/abc.txt", "B"}, {"multi-file-download/xyz/def.txt", "C"}} for _, v := range downloadResp.Value { var bodyChar string var path string @@ -247,8 +258,7 @@ func TestActionsArtifactDownloadMultiFiles(t *testing.T) { req = NewRequest(t, "GET", url). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) - body := strings.Repeat(bodyChar, 1024) - assert.Equal(t, resp.Body.String(), body) + assert.Equal(t, strings.Repeat(bodyChar, 1024), resp.Body.String()) } } @@ -300,7 +310,7 @@ func TestActionsArtifactOverwrite(t *testing.T) { DecodeJSON(t, resp, &listResp) idx := strings.Index(listResp.Value[0].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") - url := listResp.Value[0].FileContainerResourceURL[idx+1:] + "?itemPath=artifact" + url := listResp.Value[0].FileContainerResourceURL[idx+1:] + "?itemPath=artifact-download" req = NewRequest(t, "GET", url). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) @@ -320,14 +330,14 @@ func TestActionsArtifactOverwrite(t *testing.T) { // upload same artifact, it uses 4096 B req := NewRequestWithJSON(t, "POST", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts", getUploadArtifactRequest{ Type: "actions_storage", - Name: "artifact", + Name: "artifact-download", }).AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp := MakeRequest(t, req, http.StatusOK) var uploadResp uploadArtifactResponse DecodeJSON(t, resp, &uploadResp) idx := strings.Index(uploadResp.FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") - url := uploadResp.FileContainerResourceURL[idx:] + "?itemPath=artifact/abc.txt" + url := uploadResp.FileContainerResourceURL[idx:] + "?itemPath=artifact-download/abc.txt" body := strings.Repeat("B", 4096) req = NewRequestWithBody(t, "PUT", url, strings.NewReader(body)). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a"). @@ -337,7 +347,7 @@ func TestActionsArtifactOverwrite(t *testing.T) { MakeRequest(t, req, http.StatusOK) // confirm artifact upload - req = NewRequest(t, "PATCH", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts?artifactName=artifact"). + req = NewRequest(t, "PATCH", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts?artifactName=artifact-download"). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") MakeRequest(t, req, http.StatusOK) } @@ -352,15 +362,15 @@ func TestActionsArtifactOverwrite(t *testing.T) { var uploadedItem listArtifactsResponseItem for _, item := range listResp.Value { - if item.Name == "artifact" { + if item.Name == "artifact-download" { uploadedItem = item break } } - assert.Equal(t, "artifact", uploadedItem.Name) + assert.Equal(t, "artifact-download", uploadedItem.Name) idx := strings.Index(uploadedItem.FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") - url := uploadedItem.FileContainerResourceURL[idx+1:] + "?itemPath=artifact" + url := uploadedItem.FileContainerResourceURL[idx+1:] + "?itemPath=artifact-download" req = NewRequest(t, "GET", url). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index f55250f6c..bf28f2353 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -313,7 +313,7 @@ func TestActionsArtifactV4DownloadSingle(t *testing.T) { // acquire artifact upload url req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/ListArtifacts", toProtoJSON(&actions.ListArtifactsRequest{ - NameFilter: wrapperspb.String("artifact"), + NameFilter: wrapperspb.String("artifact-v4-download"), WorkflowRunBackendId: "792", WorkflowJobRunBackendId: "193", })).AddTokenAuth(token) @@ -324,7 +324,7 @@ func TestActionsArtifactV4DownloadSingle(t *testing.T) { // confirm artifact upload req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/GetSignedArtifactURL", toProtoJSON(&actions.GetSignedArtifactURLRequest{ - Name: "artifact", + Name: "artifact-v4-download", WorkflowRunBackendId: "792", WorkflowJobRunBackendId: "193", })). @@ -336,20 +336,20 @@ func TestActionsArtifactV4DownloadSingle(t *testing.T) { req = NewRequest(t, "GET", finalizeResp.SignedUrl) resp = MakeRequest(t, req, http.StatusOK) - body := strings.Repeat("A", 1024) + body := strings.Repeat("D", 1024) assert.Equal(t, "bytes", resp.Header().Get("accept-ranges")) assert.Equal(t, body, resp.Body.String()) // Download artifact via user-facing URL - req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact") + req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact-v4-download") resp = MakeRequest(t, req, http.StatusOK) assert.Equal(t, "bytes", resp.Header().Get("accept-ranges")) assert.Equal(t, body, resp.Body.String()) // Partial artifact download - req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact").SetHeader("range", "bytes=0-99") + req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact-v4-download").SetHeader("range", "bytes=0-99") resp = MakeRequest(t, req, http.StatusPartialContent) - body = strings.Repeat("A", 100) + body = strings.Repeat("D", 100) assert.Equal(t, "bytes 0-99/1024", resp.Header().Get("content-range")) assert.Equal(t, body, resp.Body.String()) } @@ -357,13 +357,13 @@ func TestActionsArtifactV4DownloadSingle(t *testing.T) { func TestActionsArtifactV4DownloadRange(t *testing.T) { defer tests.PrepareTestEnv(t)() - bstr := strings.Repeat("B", 100) + bstr := strings.Repeat("D", 100) body := strings.Repeat("A", 100) + bstr token := uploadArtifact(t, body) // Download (Actions API) req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/GetSignedArtifactURL", toProtoJSON(&actions.GetSignedArtifactURLRequest{ - Name: "artifact", + Name: "artifact-v4-download", WorkflowRunBackendId: "792", WorkflowJobRunBackendId: "193", })). @@ -375,13 +375,13 @@ func TestActionsArtifactV4DownloadRange(t *testing.T) { req = NewRequest(t, "GET", finalizeResp.SignedUrl).SetHeader("range", "bytes=100-199") resp = MakeRequest(t, req, http.StatusPartialContent) - assert.Equal(t, "bytes 100-199/200", resp.Header().Get("content-range")) + assert.Equal(t, "bytes 100-199/1024", resp.Header().Get("content-range")) assert.Equal(t, bstr, resp.Body.String()) // Download (user-facing API) - req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact").SetHeader("range", "bytes=100-199") + req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact-v4-download").SetHeader("range", "bytes=100-199") resp = MakeRequest(t, req, http.StatusPartialContent) - assert.Equal(t, "bytes 100-199/200", resp.Header().Get("content-range")) + assert.Equal(t, "bytes 100-199/1024", resp.Header().Get("content-range")) assert.Equal(t, bstr, resp.Body.String()) } @@ -393,7 +393,7 @@ func TestActionsArtifactV4Delete(t *testing.T) { // delete artifact by name req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/DeleteArtifact", toProtoJSON(&actions.DeleteArtifactRequest{ - Name: "artifact", + Name: "artifact-v4-download", WorkflowRunBackendId: "792", WorkflowJobRunBackendId: "193", })).AddTokenAuth(token) diff --git a/tests/test_utils.go b/tests/test_utils.go index ee1068961..052fdb322 100644 --- a/tests/test_utils.go +++ b/tests/test_utils.go @@ -224,6 +224,20 @@ func cancelProcesses(t testing.TB, delay time.Duration) { t.Logf("PrepareTestEnv: all processes cancelled within %s", time.Since(start)) } +func PrepareArtifactsStorage(t testing.TB) { + // prepare actions artifacts directory and files + assert.NoError(t, storage.Clean(storage.ActionsArtifacts)) + + s, err := storage.NewStorage(setting.LocalStorageType, &setting.Storage{ + Path: filepath.Join(filepath.Dir(setting.AppPath), "tests", "testdata", "data", "artifacts"), + }) + assert.NoError(t, err) + assert.NoError(t, s.IterateObjects("", func(p string, obj storage.Object) error { + _, err = storage.Copy(storage.ActionsArtifacts, p, s, p) + return err + })) +} + func PrepareTestEnv(t testing.TB, skip ...int) func() { t.Helper() ourSkip := 1 @@ -263,6 +277,9 @@ func PrepareTestEnv(t testing.TB, skip ...int) func() { } } + // Initialize actions artifact data + PrepareArtifactsStorage(t) + // load LFS object fixtures // (LFS storage can be on any of several backends, including remote servers, so we init it with the storage API) lfsFixtures, err := storage.NewStorage(setting.LocalStorageType, &setting.Storage{ diff --git a/tests/testdata/data/artifacts/26/1/1712166500347189545.chunk b/tests/testdata/data/artifacts/26/1/1712166500347189545.chunk new file mode 100644 index 000000000..bc7c569e9 --- /dev/null +++ b/tests/testdata/data/artifacts/26/1/1712166500347189545.chunk @@ -0,0 +1 @@ +AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA \ No newline at end of file diff --git a/tests/testdata/data/artifacts/26/19/1712348022422036662.chunk b/tests/testdata/data/artifacts/26/19/1712348022422036662.chunk new file mode 100644 index 000000000..b4fb0b078 --- /dev/null +++ b/tests/testdata/data/artifacts/26/19/1712348022422036662.chunk @@ -0,0 +1 @@ +BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB \ No newline at end of file diff --git a/tests/testdata/data/artifacts/26/20/1712348022423431524.chunk b/tests/testdata/data/artifacts/26/20/1712348022423431524.chunk new file mode 100644 index 000000000..45b2320ec --- /dev/null +++ b/tests/testdata/data/artifacts/26/20/1712348022423431524.chunk @@ -0,0 +1 @@ +CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC \ No newline at end of file diff --git a/tests/testdata/data/artifacts/27/5/1730330775594233150.chunk b/tests/testdata/data/artifacts/27/5/1730330775594233150.chunk new file mode 100644 index 000000000..b1d6b8e04 --- /dev/null +++ b/tests/testdata/data/artifacts/27/5/1730330775594233150.chunk @@ -0,0 +1 @@ +DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD \ No newline at end of file From befafe9a05f118b34990db16a57a7ebc188aaa9a Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Fri, 1 Nov 2024 22:29:37 -0500 Subject: [PATCH 09/13] improve performance of diffs (#32393) This has two major changes that significantly reduce the amount of work done for large diffs: * Kill a running git process when reaching the maximum number of files in a diff, preventing it from processing the entire diff. * When loading a diff with the URL param `file-only=true`, skip loading stats. This speeds up loading both hidden files of a diff and sections of a diff when clicking the "Show More" button. A couple of minor things from profiling are also included: * Reuse existing repo in `PrepareViewPullInfo` if head and base are the same. The performance impact is going to depend heavily on the individual diff and the hardware it runs on, but when testing locally on a diff changing 100k+ lines over hundreds of files, I'm seeing a roughly 75% reduction in time to load the result of "Show More" --------- Co-authored-by: wxiaoguang (cherry picked from commit 7dcccc3bb19655a6f83dd495ffc332708d0c8678) --- routers/web/repo/commit.go | 1 + routers/web/repo/compare.go | 3 ++ routers/web/repo/pull.go | 7 +++-- services/gitdiff/gitdiff.go | 60 ++++++++++++++++--------------------- 4 files changed, 34 insertions(+), 37 deletions(-) diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 0e5d1f0a1..142823807 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -338,6 +338,7 @@ func Diff(ctx *context.Context) { MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, MaxFiles: maxFiles, WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)), + FileOnly: fileOnly, }, files...) if err != nil { ctx.NotFound("GetDiff", err) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 38d6004ec..e5eab2bff 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -611,6 +611,8 @@ func PrepareCompareDiff( maxLines, maxFiles = -1, -1 } + fileOnly := ctx.FormBool("file-only") + diff, err := gitdiff.GetDiff(ctx, ci.HeadGitRepo, &gitdiff.DiffOptions{ BeforeCommitID: beforeCommitID, @@ -621,6 +623,7 @@ func PrepareCompareDiff( MaxFiles: maxFiles, WhitespaceBehavior: whitespaceBehavior, DirectComparison: ci.DirectComparison, + FileOnly: fileOnly, }, ctx.FormStrings("files")...) if err != nil { ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 9d0dcad61..98dacc1a0 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -614,12 +614,12 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C var headBranchSha string // HeadRepo may be missing if pull.HeadRepo != nil { - headGitRepo, err := gitrepo.OpenRepository(ctx, pull.HeadRepo) + headGitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pull.HeadRepo) if err != nil { - ctx.ServerError("OpenRepository", err) + ctx.ServerError("RepositoryFromContextOrOpen", err) return nil } - defer headGitRepo.Close() + defer closer.Close() if pull.Flow == issues_model.PullRequestFlowGithub { headBranchExist = headGitRepo.IsBranchExist(pull.HeadBranch) @@ -966,6 +966,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, MaxFiles: maxFiles, WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)), + FileOnly: fileOnly, } if !willShowSpecifiedCommit { diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 91b1f135c..7d137fb21 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -379,18 +379,11 @@ func (diffFile *DiffFile) GetType() int { } // GetTailSection creates a fake DiffLineSection if the last section is not the end of the file -func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommitID, rightCommitID string) *DiffSection { +func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommit, rightCommit *git.Commit) *DiffSection { if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile { return nil } - leftCommit, err := gitRepo.GetCommit(leftCommitID) - if err != nil { - return nil - } - rightCommit, err := gitRepo.GetCommit(rightCommitID) - if err != nil { - return nil - } + lastSection := diffFile.Sections[len(diffFile.Sections)-1] lastLine := lastSection.Lines[len(lastSection.Lines)-1] leftLineCount := getCommitFileLineCount(leftCommit, diffFile.Name) @@ -532,11 +525,6 @@ parsingLoop: lastFile := createDiffFile(diff, line) diff.End = lastFile.Name diff.IsIncomplete = true - _, err := io.Copy(io.Discard, reader) - if err != nil { - // By the definition of io.Copy this never returns io.EOF - return diff, fmt.Errorf("error during io.Copy: %w", err) - } break parsingLoop } @@ -1097,6 +1085,7 @@ type DiffOptions struct { MaxFiles int WhitespaceBehavior git.TrustedCmdArgs DirectComparison bool + FileOnly bool } // GetDiff builds a Diff between two commits of a repository. @@ -1105,12 +1094,16 @@ type DiffOptions struct { func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { repoPath := gitRepo.Path + var beforeCommit *git.Commit commit, err := gitRepo.GetCommit(opts.AfterCommitID) if err != nil { return nil, err } - cmdDiff := git.NewCommand(gitRepo.Ctx) + cmdCtx, cmdCancel := context.WithCancel(ctx) + defer cmdCancel() + + cmdDiff := git.NewCommand(cmdCtx) objectFormat, err := gitRepo.GetObjectFormat() if err != nil { return nil, err @@ -1132,6 +1125,12 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi AddArguments(opts.WhitespaceBehavior...). AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID) opts.BeforeCommitID = actualBeforeCommitID + + var err error + beforeCommit, err = gitRepo.GetCommit(opts.BeforeCommitID) + if err != nil { + return nil, err + } } // In git 2.31, git diff learned --skip-to which we can use to shortcut skip to file @@ -1166,7 +1165,9 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi _ = writer.Close() }() - diff, err := ParsePatch(ctx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) + diff, err := ParsePatch(cmdCtx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) + // Ensure the git process is killed if it didn't exit already + cmdCancel() if err != nil { return nil, fmt.Errorf("unable to ParsePatch: %w", err) } @@ -1207,37 +1208,28 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi diffFile.IsGenerated = analyze.IsGenerated(diffFile.Name) } - tailSection := diffFile.GetTailSection(gitRepo, opts.BeforeCommitID, opts.AfterCommitID) + tailSection := diffFile.GetTailSection(gitRepo, beforeCommit, commit) if tailSection != nil { diffFile.Sections = append(diffFile.Sections, tailSection) } } - separator := "..." - if opts.DirectComparison { - separator = ".." + if opts.FileOnly { + return diff, nil } - diffPaths := []string{opts.BeforeCommitID + separator + opts.AfterCommitID} - if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String() { - diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID} - } - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) - if err != nil && strings.Contains(err.Error(), "no merge base") { - // git >= 2.28 now returns an error if base and head have become unrelated. - // previously it would return the results of git diff --shortstat base head so let's try that... - diffPaths = []string{opts.BeforeCommitID, opts.AfterCommitID} - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) - } + stats, err := GetPullDiffStats(gitRepo, opts) if err != nil { return nil, err } + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion = stats.NumFiles, stats.TotalAddition, stats.TotalDeletion + return diff, nil } type PullDiffStats struct { - TotalAddition, TotalDeletion int + NumFiles, TotalAddition, TotalDeletion int } // GetPullDiffStats @@ -1261,12 +1253,12 @@ func GetPullDiffStats(gitRepo *git.Repository, opts *DiffOptions) (*PullDiffStat diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID} } - _, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) if err != nil && strings.Contains(err.Error(), "no merge base") { // git >= 2.28 now returns an error if base and head have become unrelated. // previously it would return the results of git diff --shortstat base head so let's try that... diffPaths = []string{opts.BeforeCommitID, opts.AfterCommitID} - _, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) } if err != nil { return nil, err From 268276d4a74af35d46816d54fdb9bb0c0781bca6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Nov 2024 23:11:38 -0700 Subject: [PATCH 10/13] Fix created_unix for mirroring (#32342) Fix #32233 (cherry picked from commit 13a203828c40f9ad1005b16b4ae26256a7df8263) --- modules/repository/repo.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/repository/repo.go b/modules/repository/repo.go index e15526d78..9ce330b21 100644 --- a/modules/repository/repo.go +++ b/modules/repository/repo.go @@ -341,9 +341,10 @@ func pullMirrorReleaseSync(ctx context.Context, repo *repo_model.Repository, git for _, tag := range updates { if _, err := db.GetEngine(ctx).Where("repo_id = ? AND lower_tag_name = ?", repo.ID, strings.ToLower(tag.Name)). - Cols("sha1"). + Cols("sha1", "created_unix"). Update(&repo_model.Release{ - Sha1: tag.Object.String(), + Sha1: tag.Object.String(), + CreatedUnix: timeutil.TimeStamp(tag.Tagger.When.Unix()), }); err != nil { return fmt.Errorf("unable to update tag %s for pull-mirror Repo[%d:%s/%s]: %w", tag.Name, repo.ID, repo.OwnerName, repo.Name, err) } From ae6292ba380e6af49c6d24522a89313638570dee Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 3 Nov 2024 11:33:32 +0100 Subject: [PATCH 11/13] chore: Fix a few lint errors - Adjust `PrepareArtifactsStorage` to use `require.NoError` instead of `assert.NoError` - Adjust `TestActionsArtifactDownload` to have the proper order of `assert.Equal` arguments. Signed-off-by: Gergely Nagy --- tests/integration/api_actions_artifact_test.go | 2 +- tests/test_utils.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration/api_actions_artifact_test.go b/tests/integration/api_actions_artifact_test.go index 7063d9934..2e35d4390 100644 --- a/tests/integration/api_actions_artifact_test.go +++ b/tests/integration/api_actions_artifact_test.go @@ -126,7 +126,7 @@ func TestActionsArtifactDownload(t *testing.T) { } } assert.NotNil(t, artifactIdx) - assert.Equal(t, listResp.Value[artifactIdx].Name, "artifact-download") + assert.Equal(t, "artifact-download", listResp.Value[artifactIdx].Name) assert.Contains(t, listResp.Value[artifactIdx].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") idx := strings.Index(listResp.Value[artifactIdx].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") diff --git a/tests/test_utils.go b/tests/test_utils.go index 052fdb322..89016f9f6 100644 --- a/tests/test_utils.go +++ b/tests/test_utils.go @@ -226,13 +226,13 @@ func cancelProcesses(t testing.TB, delay time.Duration) { func PrepareArtifactsStorage(t testing.TB) { // prepare actions artifacts directory and files - assert.NoError(t, storage.Clean(storage.ActionsArtifacts)) + require.NoError(t, storage.Clean(storage.ActionsArtifacts)) s, err := storage.NewStorage(setting.LocalStorageType, &setting.Storage{ Path: filepath.Join(filepath.Dir(setting.AppPath), "tests", "testdata", "data", "artifacts"), }) - assert.NoError(t, err) - assert.NoError(t, s.IterateObjects("", func(p string, obj storage.Object) error { + require.NoError(t, err) + require.NoError(t, s.IterateObjects("", func(p string, obj storage.Object) error { _, err = storage.Copy(storage.ActionsArtifacts, p, s, p) return err })) From 597f83c73527c1a171bd16f52efccc6eae848bb4 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 5 Nov 2024 09:23:56 +0100 Subject: [PATCH 12/13] fix(tests): Fix TestMigrateActionsArtifacts() Since we have artifact fixtures now, some ids are in use. To avoid reusing IDs, start them at 42, rather than 0. That's past the ids used by the fixtures. Signed-off-by: Gergely Nagy --- cmd/migrate_storage_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/migrate_storage_test.go b/cmd/migrate_storage_test.go index 800a15e21..56745e9a3 100644 --- a/cmd/migrate_storage_test.go +++ b/cmd/migrate_storage_test.go @@ -91,7 +91,7 @@ func TestMigrateActionsArtifacts(t *testing.T) { srcStorage, _ := createLocalStorage(t) defer test.MockVariableValue(&storage.ActionsArtifacts, srcStorage)() - id := int64(0) + id := int64(42) addArtifact := func(storagePath string, status actions.ArtifactStatus) { id++ From cc03ac9e8fc0b4967864ba8409565ef42f1d1bbf Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 5 Nov 2024 09:30:22 +0100 Subject: [PATCH 13/13] chore(release-notes): notes for the week 2024-45 weekly cherry pick --- release-notes/5789.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 release-notes/5789.md diff --git a/release-notes/5789.md b/release-notes/5789.md new file mode 100644 index 000000000..0c0763a46 --- /dev/null +++ b/release-notes/5789.md @@ -0,0 +1,6 @@ +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/362ad0ba39bdbc87202e349678e21fc2a75ff7cb) Update force-pushed tags too when syncing mirrors +chore: [commit](https://codeberg.org/forgejo/forgejo/commit/b308bcca7c950b7f0d127ee4282019c2a9923299) Improved diff view performance +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/4c5bdddf7751a35985c08ba6506f1f30103749d6) Fix `missing signature key` error when pulling Docker images with `SERVE_DIRECT` enabled +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/2c5fdb108ff9e23e8f907fb6afe59177c6bb202e) Fix the missing menu in organization project view page +feat: [commit](https://codeberg.org/forgejo/forgejo/commit/1e595979625e54d375a0eaa440b84ef5e17af160) Add new [lfs_client].BATCH_SIZE and [server].LFS_MAX_BATCH_SIZE config settings. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/2358c0d899faec8311e46dcb0550041496bcd532) Properly clean temporary index files