From 9ea796b9ab0b3ef80c1fb05e4cc08d42e2be25fe Mon Sep 17 00:00:00 2001 From: Michael Jerger Date: Mon, 16 Jun 2025 20:27:47 +0200 Subject: [PATCH 1/8] [gitea] week 2025-21 cherry pick (gitea/main -> forgejo) (#8040) ## Checklist - [x] go to the last cherry-pick PR (forgejo/forgejo#7965) to figure out how far it went: [gitea@9d4ebc1f2c](https://github.com/go-gitea/gitea/commit/9d4ebc1f2caa2cad9f5815e2760c49185b0809e4) - [x] cherry-pick and open PR (forgejo/forgejo#8040) - [ ] have the PR pass the CI - end-to-end (specially important if there are actions related changes) - [ ] add `run-end-to-end` label - [ ] check the result - [ ] write release notes - [ ] assign reviewers - [ ] 48h later, last call - merge 1 hour after the last call ## Legend - :question: - No decision about the commit has been made. - :cherries: - The commit has been cherry picked. - :fast_forward: - The commit has been skipped. - :bulb: - The commit has been skipped, but should be ported to Forgejo. - :writing_hand: - The commit has been skipped, and a port to Forgejo already exists. ## Commits - :cherries: [`gitea`](https://github.com/go-gitea/gitea/commit/50d95650884a877936784fae01aaf3f7fea513c6) -> [`forgejo`](https://codeberg.org/forgejo/forgejo/commit/c3e6eab73235ac189ca3a36ce2b8ea8d8ad82c81) Add sort option recentclose for issues and pulls ([gitea#34525](https://github.com/go-gitea/gitea/pull/34525)) ## TODO - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/d5bbaee64e44327e78e39ad7a1977e21ddc59e1c) Retain issue sort type when a keyword search is introduced ([gitea#34559](https://github.com/go-gitea/gitea/pull/34559)) UI: Small bat might be nice. Test needed? Do we've frontend tests covering the search? ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/82ea2387e4c225617d607c0e25043920837dba7b) Always use an empty line to separate the commit message and trailer ([gitea#34512](https://github.com/go-gitea/gitea/pull/34512)) Needs merge ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/74858dc5aefeb177c8d377f3f9a408331887d92a) Fix line-button issue after file selection in file tree ([gitea#34574](https://github.com/go-gitea/gitea/pull/34574)) Frontend: Makes it sense to pick/port ui logic in *.ts files? ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/7149c9c55d7e960bd5fa59a66243fc8445e7bf37) Fix doctor deleting orphaned issues attachments ([gitea#34142](https://github.com/go-gitea/gitea/pull/34142)) Doctor: seems useful. ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/0cec4b84e2e2385d33cd19351f8a9e098a29ecc2) Fix actions skipped commit status indicator ([gitea#34507](https://github.com/go-gitea/gitea/pull/34507)) Actions: Might benefit from additional tests. ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/4cb0c641ce856224bb53245b6946d9da0f2c38dd) Add "View workflow file" to Actions list page ([gitea#34538](https://github.com/go-gitea/gitea/pull/34538)) Actions: Needs tests ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/b0936f4f4120169fab31f21610e9fcd915e36fb4) Do not mutate incoming options to RenderUserSearch and SearchUsers ([gitea#34544](https://github.com/go-gitea/gitea/pull/34544)) Nice refactoring but needs manual merge. ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/498088c0536f228eceb7c04ab53742f69a0024b4) Add webhook assigning test and fix possible bug ([gitea#34420](https://github.com/go-gitea/gitea/pull/34420)) Integrationtest has conflicts needs merge. ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/24a51059d7fffe75564f7dc2fffeba1f3789f55e) Fix possible nil description of pull request when migrating from CodeCommit ([gitea#34541](https://github.com/go-gitea/gitea/pull/34541)) Is this relevant to forgejo? Did not find the place to apply this small change. ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/688da55f543f82265cc7df2bd1cf2bce53188b7a) Split GetLatestCommitStatus as two functions ([gitea#34535](https://github.com/go-gitea/gitea/pull/34535)) Merge required. ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/ab9691291d313aedab7b459c42e1e1dd15f05461) Don't display error log when .git-blame-ignore-revs doesn't exist ([gitea#34457](https://github.com/go-gitea/gitea/pull/34457)) Unsure wheter this affects forgejo. Tests missing. ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/11ee7ff3bfb21fedf6a069d61149b0b3d51ce7f2) fix: return 201 Created for CreateVariable API responses ([gitea#34517](https://github.com/go-gitea/gitea/pull/34517)) Actions: This is marked as breaking the api. Pls think about whether this breaking change iss needed & how this impact api-version-increase. The corresponding clinet change can be found here: https://gitea.com/gitea/go-sdk/pulls/713/files ------ - :bulb: [`gitea`](https://github.com/go-gitea/gitea/commit/9b295e984a15f0b903675adfe4ee8bd2c5f7c0d4) Actions list ([gitea#34530](https://github.com/go-gitea/gitea/pull/34530)) Actions: Regression from https://github.com/go-gitea/gitea/pull/34337 Part of https://codeberg.org/forgejo/forgejo/pulls/7909 ------ ## Skipped - :fast_forward: [`gitea`](https://github.com/go-gitea/gitea/commit/bb6377d0807e56586a94a1f14a5ffa7da7155865) [skip ci] Updated translations via Crowdin ------ - :fast_forward: [`gitea`](https://github.com/go-gitea/gitea/commit/07d802a815aceec9474661102fc8d73e67afd64f) [skip ci] Updated translations via Crowdin ------ - :fast_forward: [`gitea`](https://github.com/go-gitea/gitea/commit/c6e2093f427b05624d8a21381710b50206cd17a4) Clean up "file-view" related styles ([gitea#34558](https://github.com/go-gitea/gitea/pull/34558)) - gitea ui specific specific ------ - :fast_forward: [`gitea`](https://github.com/go-gitea/gitea/commit/9f10885b2194b6ee19f9b165b06edda775b792b2) Refactor commit reader ([gitea#34542](https://github.com/go-gitea/gitea/pull/34542)) - gitea refactor specific ------

Stats
Between [`gitea@9d4ebc1f2c`](https://github.com/go-gitea/gitea/commit/9d4ebc1f2caa2cad9f5815e2760c49185b0809e4) and [`gitea@d5bbaee64e`](https://github.com/go-gitea/gitea/commit/d5bbaee64e44327e78e39ad7a1977e21ddc59e1c), **18** commits have been reviewed. We picked **1**, skipped **4**, and decided to port **13**.
Co-authored-by: Markus Amshove Co-authored-by: Lunny Xiao Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8040 Reviewed-by: Earl Warren Co-authored-by: Michael Jerger Co-committed-by: Michael Jerger --- models/issues/issue_search.go | 2 ++ models/issues/pull_list.go | 3 ++- models/issues/pull_test.go | 41 ++++++++++++++++++++++++++++++++++ routers/api/v1/repo/pull.go | 2 +- templates/swagger/v1_json.tmpl | 1 + 5 files changed, 47 insertions(+), 2 deletions(-) diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index bf4b89ee0b..91a69c26a7 100644 --- a/models/issues/issue_search.go +++ b/models/issues/issue_search.go @@ -66,6 +66,8 @@ func applySorts(sess *xorm.Session, sortType string, priorityRepoID int64) { sess.Asc("issue.created_unix").Asc("issue.id") case "recentupdate": sess.Desc("issue.updated_unix").Desc("issue.created_unix").Desc("issue.id") + case "recentclose": + sess.Desc("issue.closed_unix").Desc("issue.created_unix").Desc("issue.id") case "leastupdate": sess.Asc("issue.updated_unix").Asc("issue.created_unix").Asc("issue.id") case "mostcomment": diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index a448673454..8fc0491026 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -152,7 +152,8 @@ func PullRequests(ctx context.Context, baseRepoID int64, opts *PullRequestsOptio applySorts(findSession, opts.SortType, 0) findSession = db.SetSessionPagination(findSession, opts) prs := make([]*PullRequest, 0, opts.PageSize) - return prs, maxResults, findSession.Find(&prs) + found := findSession.Find(&prs) + return prs, maxResults, found } // PullRequestList defines a list of pull requests diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 5a8e8d8aaf..3682f6fd25 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -79,6 +79,47 @@ func TestPullRequestsNewest(t *testing.T) { } } +func TestPullRequests_Closed_RecentSortType(t *testing.T) { + // Issue ID | Closed At. | Updated At + // 2 | 1707270001 | 1707270001 + // 3 | 1707271000 | 1707279999 + // 11 | 1707279999 | 1707275555 + tests := []struct { + sortType string + expectedIssueIDOrder []int64 + }{ + {"recentupdate", []int64{3, 11, 2}}, + {"recentclose", []int64{11, 3, 2}}, + } + + require.NoError(t, unittest.PrepareTestDatabase()) + _, err := db.Exec(db.DefaultContext, "UPDATE issue SET closed_unix = 1707270001, updated_unix = 1707270001, is_closed = true WHERE id = 2") + require.NoError(t, err) + _, err = db.Exec(db.DefaultContext, "UPDATE issue SET closed_unix = 1707271000, updated_unix = 1707279999, is_closed = true WHERE id = 3") + require.NoError(t, err) + _, err = db.Exec(db.DefaultContext, "UPDATE issue SET closed_unix = 1707279999, updated_unix = 1707275555, is_closed = true WHERE id = 11") + require.NoError(t, err) + + for _, test := range tests { + t.Run(test.sortType, func(t *testing.T) { + prs, _, err := issues_model.PullRequests(db.DefaultContext, 1, &issues_model.PullRequestsOptions{ + ListOptions: db.ListOptions{ + Page: 1, + }, + State: "closed", + SortType: test.sortType, + }) + require.NoError(t, err) + + if assert.Len(t, prs, len(test.expectedIssueIDOrder)) { + for i := range test.expectedIssueIDOrder { + assert.Equal(t, test.expectedIssueIDOrder[i], prs[i].IssueID) + } + } + }) + } +} + func TestLoadRequestedReviewers(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 75b4870e51..c9dda124de 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -71,7 +71,7 @@ func ListPullRequests(ctx *context.APIContext) { // in: query // description: Type of sort // type: string - // enum: [oldest, recentupdate, leastupdate, mostcomment, leastcomment, priority] + // enum: [oldest, recentupdate, recentclose, leastupdate, mostcomment, leastcomment, priority] // - name: milestone // in: query // description: ID of the milestone diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 124d5bd1bd..bba4ac4949 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -12994,6 +12994,7 @@ "enum": [ "oldest", "recentupdate", + "recentclose", "leastupdate", "mostcomment", "leastcomment", From 0c55cdf6b6b3df4d2588ec44232ed2c5ff899898 Mon Sep 17 00:00:00 2001 From: Renovate Bot Date: Tue, 17 Jun 2025 03:48:53 +0200 Subject: [PATCH 2/8] Update dependency chart.js to v4.5.0 (forgejo) (#8190) Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8190 Reviewed-by: Gusted Co-authored-by: Renovate Bot Co-committed-by: Renovate Bot --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 4a91eeecc2..40601d8536 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ "@primer/octicons": "19.14.0", "ansi_up": "6.0.5", "asciinema-player": "3.8.2", - "chart.js": "4.4.9", + "chart.js": "4.5.0", "chartjs-adapter-dayjs-4": "1.0.4", "chartjs-plugin-zoom": "2.2.0", "clippie": "4.1.7", @@ -5373,9 +5373,9 @@ } }, "node_modules/chart.js": { - "version": "4.4.9", - "resolved": "https://registry.npmjs.org/chart.js/-/chart.js-4.4.9.tgz", - "integrity": "sha512-EyZ9wWKgpAU0fLJ43YAEIF8sr5F2W3LqbS40ZJyHIner2lY14ufqv2VMp69MAiZ2rpwxEUxEhIH/0U3xyRynxg==", + "version": "4.5.0", + "resolved": "https://registry.npmjs.org/chart.js/-/chart.js-4.5.0.tgz", + "integrity": "sha512-aYeC/jDgSEx8SHWZvANYMioYMZ2KX02W6f6uVfyteuCGcadDLcYVHdfdygsTQkQ4TKn5lghoojAsPj5pu0SnvQ==", "license": "MIT", "dependencies": { "@kurkle/color": "^0.3.0" diff --git a/package.json b/package.json index f7f04b1c0b..b4d170fdb9 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "@primer/octicons": "19.14.0", "ansi_up": "6.0.5", "asciinema-player": "3.8.2", - "chart.js": "4.4.9", + "chart.js": "4.5.0", "chartjs-adapter-dayjs-4": "1.0.4", "chartjs-plugin-zoom": "2.2.0", "clippie": "4.1.7", From b52264c953dcba20b8d23fc450da3f0aadb8d259 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 17 Jun 2025 07:47:00 +0200 Subject: [PATCH 3/8] fix: do not check for `object_format_name` field (#8202) - Only check for the `object_format_name` field if SHA256 is supported. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8202 Reviewed-by: Earl Warren Co-authored-by: Gusted Co-committed-by: Gusted --- tests/integration/repo_generate_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/integration/repo_generate_test.go b/tests/integration/repo_generate_test.go index d22e6ecc74..44987c14b0 100644 --- a/tests/integration/repo_generate_test.go +++ b/tests/integration/repo_generate_test.go @@ -15,6 +15,7 @@ import ( repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + "forgejo.org/modules/git" "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/test" @@ -43,9 +44,13 @@ func assertRepoCreateForm(t *testing.T, htmlDoc *HTMLDoc, owner *user_model.User // the template menu is loaded client-side, so don't assert the option exists assert.Equal(t, templateID, htmlDoc.GetInputValueByName("repo_template"), "Unexpected repo_template selection") - for _, name := range []string{"issue_labels", "gitignores", "license", "object_format_name"} { + for _, name := range []string{"issue_labels", "gitignores", "license"} { htmlDoc.AssertDropdownHasOptions(t, name) } + + if git.SupportHashSha256 { + htmlDoc.AssertDropdownHasOptions(t, "object_format_name") + } } func testRepoGenerate(t *testing.T, session *TestSession, templateID, templateOwnerName, templateRepoName string, user, generateOwner *user_model.User, generateRepoName string) { From 3a8cea52cd0bcbde839e6c4779265a94bf6c4700 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 17 Jun 2025 08:28:26 +0200 Subject: [PATCH 4/8] chore: remove gopls in Makefile (#8205) - `lint-go-gopls` runs `gopls check` over Forgejo's codebase. It report errors found by the diagnosis tool of gopls, most of it are errors that can be catched by existing linters. It is not used in the CI, remove it. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8205 Reviewed-by: Earl Warren Reviewed-by: Michael Kriese Co-authored-by: Gusted Co-committed-by: Gusted --- Makefile | 8 -------- tools/lint-go-gopls.sh | 23 ----------------------- 2 files changed, 31 deletions(-) delete mode 100755 tools/lint-go-gopls.sh diff --git a/Makefile b/Makefile index e6416d5a6f..852c85ccd6 100644 --- a/Makefile +++ b/Makefile @@ -47,7 +47,6 @@ GO_LICENSES_PACKAGE ?= github.com/google/go-licenses@v1.6.0 # renovate: datasour GOVULNCHECK_PACKAGE ?= golang.org/x/vuln/cmd/govulncheck@v1 # renovate: datasource=go DEADCODE_PACKAGE ?= golang.org/x/tools/cmd/deadcode@v0.34.0 # renovate: datasource=go GOMOCK_PACKAGE ?= go.uber.org/mock/mockgen@v0.5.2 # renovate: datasource=go -GOPLS_PACKAGE ?= golang.org/x/tools/gopls@v0.18.1 # renovate: datasource=go RENOVATE_NPM_PACKAGE ?= renovate@40.57.1 # renovate: datasource=docker packageName=data.forgejo.org/renovate/renovate # https://github.com/disposable-email-domains/disposable-email-domains/commits/main/ @@ -222,7 +221,6 @@ help: @echo " - lint-go lint go files" @echo " - lint-go-fix lint go files and fix issues" @echo " - lint-go-vet lint go files with vet" - @echo " - lint-go-gopls lint go files with gopls" @echo " - lint-js lint js files" @echo " - lint-js-fix lint js files and fix issues" @echo " - lint-css lint css files" @@ -487,11 +485,6 @@ lint-go-vet: @echo "Running go vet..." @$(GO) vet ./... -.PHONY: lint-go-gopls -lint-go-gopls: - @echo "Running gopls check..." - @GO=$(GO) GOPLS_PACKAGE=$(GOPLS_PACKAGE) tools/lint-go-gopls.sh $(GO_SOURCES_NO_BINDATA) - .PHONY: lint-editorconfig lint-editorconfig: $(GO) run $(EDITORCONFIG_CHECKER_PACKAGE) templates .forgejo/workflows @@ -932,7 +925,6 @@ deps-tools: $(GO) install $(GO_LICENSES_PACKAGE) $(GO) install $(GOVULNCHECK_PACKAGE) $(GO) install $(GOMOCK_PACKAGE) - $(GO) install $(GOPLS_PACKAGE) node_modules: package-lock.json npm install --no-save diff --git a/tools/lint-go-gopls.sh b/tools/lint-go-gopls.sh deleted file mode 100755 index a222ea14d7..0000000000 --- a/tools/lint-go-gopls.sh +++ /dev/null @@ -1,23 +0,0 @@ -#!/bin/bash -set -uo pipefail - -cd "$(dirname -- "${BASH_SOURCE[0]}")" && cd .. - -IGNORE_PATTERNS=( - "is deprecated" # TODO: fix these -) - -# lint all go files with 'gopls check' and look for lines starting with the -# current absolute path, indicating a error was found. This is necessary -# because the tool does not set non-zero exit code when errors are found. -# ref: https://github.com/golang/go/issues/67078 -ERROR_LINES=$("$GO" run "$GOPLS_PACKAGE" check "$@" 2>/dev/null | grep -E "^$PWD" | grep -vFf <(printf '%s\n' "${IGNORE_PATTERNS[@]}")); -NUM_ERRORS=$(echo -n "$ERROR_LINES" | wc -l) - -if [ "$NUM_ERRORS" -eq "0" ]; then - exit 0; -else - echo "$ERROR_LINES" - echo "Found $NUM_ERRORS 'gopls check' errors" - exit 1; -fi From 3a986d282fcb27a094a3e6e076e3bf9b0e6c09cd Mon Sep 17 00:00:00 2001 From: Lucas Schwiderski Date: Tue, 17 Jun 2025 09:31:50 +0200 Subject: [PATCH 5/8] Implement single-commit PR review flow (#7155) This implements the UI controls and information displays necessary to allow reviewing pull requests by stepping through commits individually. Notable changes: - Within the PR page, commit links now stay in the PR context by navigating to `{owner}/{repo}/pulls/{id}/commits/{sha}` - When showing a single commit in the "Files changed" tab, the commit header containing commit message and metadata is displayed - I dropped the existing buttons, since they make less sense to me in the PR context - The SHA links to the separate, dedicated commit view - "Previous"/"Next" buttons have been added to that header to allow stepping through commits - Reviews can be submitted in "single commit" view Talking points: - The "Showing only changes from" banner made sense when that view was limited (e.g. review submit was disabled). Now that it's on par with the "all commits" view, and visually distinct due to the commit header, this banner could potentially be dropped. Closes: #5670 #5126 #5671 #2281 #8084 ![image](/attachments/cff441dc-a080-42f8-86ae-9b80490761bf) ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [x] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7155 Reviewed-by: Earl Warren Reviewed-by: Beowulf Co-authored-by: Lucas Schwiderski Co-committed-by: Lucas Schwiderski --- models/fixtures/comment.yml | 40 +++++++ options/locale_next/locale_en-US.json | 2 + routers/web/repo/pull.go | 110 +++++++++++++++++- routers/web/web.go | 5 +- templates/repo/commit_header.tmpl | 32 +++-- .../repo/commit_load_branches_and_tags.tmpl | 2 +- templates/repo/commits_list.tmpl | 3 + templates/repo/commits_list_small.tmpl | 9 +- templates/repo/diff/box.tmpl | 9 +- templates/repo/diff/new_review.tmpl | 8 +- tests/e2e/pr-review.test.e2e.ts | 91 ++++++++++++++- .../commitsonpr.git/logs/refs/heads/branch1 | 1 + .../06/c5865eaeaaa2f92e8fce75a281f6272ee68e90 | Bin 0 -> 223 bytes .../2d/65d92dc800c6f448541240c18e82bf36b954bb | Bin 0 -> 221 bytes .../9b/93963cf6de4dc33f915bb67f192d099c301f43 | Bin 0 -> 224 bytes .../ab/a2b386a1eb0b0273ada0fed4f7d075d6e343c1 | Bin 0 -> 223 bytes .../b3/b178fe7c45986f6325aeac1b036c74825ae8f4 | Bin 0 -> 223 bytes .../c9/cf8a3095808af2425255056e01746fef420801 | Bin 0 -> 223 bytes .../user2/commitsonpr.git/refs/heads/branch1 | 2 +- .../user2/commitsonpr.git/refs/pull/1/head | 2 +- tests/integration/pull_commit_test.go | 17 +++ tests/integration/pull_diff_test.go | 13 +-- tests/integration/pull_files_test.go | 106 +++++++++++++++++ tests/integration/pull_test.go | 37 ++++++ web_src/css/repo.css | 15 ++- 25 files changed, 471 insertions(+), 33 deletions(-) create mode 100644 tests/gitea-repositories-meta/user2/commitsonpr.git/objects/06/c5865eaeaaa2f92e8fce75a281f6272ee68e90 create mode 100644 tests/gitea-repositories-meta/user2/commitsonpr.git/objects/2d/65d92dc800c6f448541240c18e82bf36b954bb create mode 100644 tests/gitea-repositories-meta/user2/commitsonpr.git/objects/9b/93963cf6de4dc33f915bb67f192d099c301f43 create mode 100644 tests/gitea-repositories-meta/user2/commitsonpr.git/objects/ab/a2b386a1eb0b0273ada0fed4f7d075d6e343c1 create mode 100644 tests/gitea-repositories-meta/user2/commitsonpr.git/objects/b3/b178fe7c45986f6325aeac1b036c74825ae8f4 create mode 100644 tests/gitea-repositories-meta/user2/commitsonpr.git/objects/c9/cf8a3095808af2425255056e01746fef420801 create mode 100644 tests/integration/pull_files_test.go diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index f4121284a6..2c47196c05 100644 --- a/models/fixtures/comment.yml +++ b/models/fixtures/comment.yml @@ -113,3 +113,43 @@ review_id: 22 assignee_id: 5 created_unix: 946684817 + +- + id: 13 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":false,"commit_ids":["4ca8bcaf27e28504df7bf996819665986b01c847","96cef4a7b72b3c208340ae6f0cf55a93e9077c93","c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2"]}' + created_unix: 1688672373 + +- + id: 14 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":false,"commit_ids":["23576dd018294e476c06e569b6b0f170d0558705"]}' + created_unix: 1688672374 + +- + id: 15 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":false,"commit_ids":["3e64625bd6eb5bcba69ac97de6c8f507402df861", "c704db5794097441aa2d9dd834d5b7e2f8f08108"]}' + created_unix: 1688672375 + +- + id: 16 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":false,"commit_ids":["811d46c7e518f4f180afb862c0db5cb8c80529ce", "747ddb3506a4fa04a7747808eb56ae16f9e933dc", "837d5c8125633d7d258f93b998e867eab0145520", "1978192d98bb1b65e11c2cf37da854fbf94bffd6"]}' + created_unix: 1688672376 + +- + id: 17 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":true,"commit_ids":["1978192d98bb1b65e11c2cf37da854fbf94bffd6", "9b93963cf6de4dc33f915bb67f192d099c301f43"]}' + created_unix: 1749734240 diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 356aaaead0..368152095e 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -89,6 +89,8 @@ "mail.actions.run_info_previous_status": "Previous Run's Status: %[1]s", "mail.actions.run_info_ref": "Branch: %[1]s (%[2]s)", "mail.actions.run_info_trigger": "Triggered because: %[1]s by: %[2]s", + "repo.diff.commit.next-short": "Next", + "repo.diff.commit.previous-short": "Prev", "discussion.locked": "This discussion has been locked. Commenting is limited to contributors.", "editor.textarea.tab_hint": "Line already indented. Press Tab again or Escape to leave the editor.", "editor.textarea.shift_tab_hint": "No indentation on this line. Press Shift + Tab again or Escape to leave the editor.", diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 2db982d3b6..8547bbcc03 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -10,13 +10,16 @@ import ( "errors" "fmt" "html" + "html/template" "net/http" "net/url" + "path" "strconv" "strings" "forgejo.org/models" activities_model "forgejo.org/models/activities" + asymkey_model "forgejo.org/models/asymkey" "forgejo.org/models/db" git_model "forgejo.org/models/git" issues_model "forgejo.org/models/issues" @@ -28,11 +31,13 @@ import ( "forgejo.org/models/unit" user_model "forgejo.org/models/user" "forgejo.org/modules/base" + "forgejo.org/modules/charset" "forgejo.org/modules/emoji" "forgejo.org/modules/git" "forgejo.org/modules/gitrepo" issue_template "forgejo.org/modules/issue/template" "forgejo.org/modules/log" + "forgejo.org/modules/markup" "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/structs" @@ -498,6 +503,7 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue) ctx.Data["IsPullRequestBroken"] = true ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 + ctx.Data["CommitIDs"] = map[string]bool{} ctx.Data["NumFiles"] = 0 return nil } @@ -508,6 +514,12 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue) ctx.Data["NumCommits"] = len(compareInfo.Commits) ctx.Data["NumFiles"] = compareInfo.NumFiles + commitIDs := map[string]bool{} + for _, commit := range compareInfo.Commits { + commitIDs[commit.ID.String()] = true + } + ctx.Data["CommitIDs"] = commitIDs + if len(compareInfo.Commits) != 0 { sha := compareInfo.Commits[0].ID.String() commitStatuses, _, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, sha, db.ListOptionsAll) @@ -591,6 +603,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["IsPullRequestBroken"] = true ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 + ctx.Data["CommitIDs"] = map[string]bool{} ctx.Data["NumFiles"] = 0 return nil } @@ -601,6 +614,13 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["NumCommits"] = len(compareInfo.Commits) ctx.Data["NumFiles"] = compareInfo.NumFiles + + commitIDs := map[string]bool{} + for _, commit := range compareInfo.Commits { + commitIDs[commit.ID.String()] = true + } + ctx.Data["CommitIDs"] = commitIDs + return compareInfo } @@ -659,6 +679,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C } ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 + ctx.Data["CommitIDs"] = map[string]bool{} ctx.Data["NumFiles"] = 0 return nil } @@ -736,6 +757,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["IsPullRequestBroken"] = true ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 + ctx.Data["CommitIDs"] = map[string]bool{} ctx.Data["NumFiles"] = 0 return nil } @@ -760,6 +782,13 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["NumCommits"] = len(compareInfo.Commits) ctx.Data["NumFiles"] = compareInfo.NumFiles + + commitIDs := map[string]bool{} + for _, commit := range compareInfo.Commits { + commitIDs[commit.ID.String()] = true + } + ctx.Data["CommitIDs"] = commitIDs + return compareInfo } @@ -919,7 +948,81 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit - if willShowSpecifiedCommit || willShowSpecifiedCommitRange { + if willShowSpecifiedCommit { + commitID := specifiedEndCommit + + ctx.Data["CommitID"] = commitID + + var prevCommit, curCommit, nextCommit *git.Commit + + // Iterate in reverse to properly map "previous" and "next" buttons + for i := len(prInfo.Commits) - 1; i >= 0; i-- { + commit := prInfo.Commits[i] + + if curCommit != nil { + nextCommit = commit + break + } + + if commit.ID.String() == commitID { + curCommit = commit + } else { + prevCommit = commit + } + } + + if curCommit == nil { + ctx.ServerError("Repo.GitRepo.viewPullFiles", git.ErrNotExist{ID: commitID}) + return + } + + ctx.Data["Commit"] = curCommit + if prevCommit != nil { + ctx.Data["PrevCommitLink"] = path.Join(ctx.Repo.RepoLink, "pulls", strconv.FormatInt(issue.Index, 10), "commits", prevCommit.ID.String()) + } + if nextCommit != nil { + ctx.Data["NextCommitLink"] = path.Join(ctx.Repo.RepoLink, "pulls", strconv.FormatInt(issue.Index, 10), "commits", nextCommit.ID.String()) + } + + statuses, _, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, commitID, db.ListOptionsAll) + if err != nil { + log.Error("GetLatestCommitStatus: %v", err) + } + if !ctx.Repo.CanRead(unit.TypeActions) { + git_model.CommitStatusesHideActionsURL(ctx, statuses) + } + + ctx.Data["CommitStatus"] = git_model.CalcCommitStatus(statuses) + ctx.Data["CommitStatuses"] = statuses + + verification := asymkey_model.ParseCommitWithSignature(ctx, curCommit) + ctx.Data["Verification"] = verification + ctx.Data["Author"] = user_model.ValidateCommitWithEmail(ctx, curCommit) + + note := &git.Note{} + err = git.GetNote(ctx, ctx.Repo.GitRepo, specifiedEndCommit, note) + if err == nil { + ctx.Data["NoteCommit"] = note.Commit + ctx.Data["NoteAuthor"] = user_model.ValidateCommitWithEmail(ctx, note.Commit) + ctx.Data["NoteRendered"], err = markup.RenderCommitMessage(&markup.RenderContext{ + Links: markup.Links{ + Base: ctx.Repo.RepoLink, + BranchPath: path.Join("commit", util.PathEscapeSegments(commitID)), + }, + Metas: ctx.Repo.Repository.ComposeMetas(ctx), + GitRepo: ctx.Repo.GitRepo, + Ctx: ctx, + }, template.HTMLEscapeString(string(charset.ToUTF8WithFallback(note.Message, charset.ConvertOpts{})))) + if err != nil { + ctx.ServerError("RenderCommitMessage", err) + return + } + } + + endCommitID = commitID + startCommitID = prInfo.MergeBase + ctx.Data["IsShowingAllCommits"] = false + } else if willShowSpecifiedCommitRange { if len(specifiedEndCommit) > 0 { endCommitID = specifiedEndCommit } else { @@ -930,6 +1033,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } else { startCommitID = prInfo.MergeBase } + ctx.Data["IsShowingAllCommits"] = false } else { endCommitID = headCommitID @@ -937,10 +1041,10 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi ctx.Data["IsShowingAllCommits"] = true } - ctx.Data["Username"] = ctx.Repo.Owner.Name - ctx.Data["Reponame"] = ctx.Repo.Repository.Name ctx.Data["AfterCommitID"] = endCommitID ctx.Data["BeforeCommitID"] = startCommitID + ctx.Data["Username"] = ctx.Repo.Owner.Name + ctx.Data["Reponame"] = ctx.Repo.Repository.Name fileOnly := ctx.FormBool("file-only") diff --git a/routers/web/web.go b/routers/web/web.go index f8a13dab7e..4b39f22f7d 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1510,7 +1510,10 @@ func registerRoutes(m *web.Route) { m.Group("/commits", func() { m.Get("", context.RepoRef(), repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits) m.Get("/list", context.RepoRef(), repo.GetPullCommits) - m.Get("/{sha:[a-f0-9]{4,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit) + m.Group("/{sha:[a-f0-9]{4,40}}", func() { + m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit) + m.Post("/reviews/submit", context.RepoMustNotBeArchived(), web.Bind(forms.SubmitReviewForm{}), repo.SubmitReview) + }) }) m.Post("/merge", context.RepoMustNotBeArchived(), web.Bind(forms.MergePullRequestForm{}), context.EnforceQuotaWeb(quota_model.LimitSubjectSizeGitAll, context.QuotaTargetRepo), repo.MergePullRequest) m.Post("/cancel_auto_merge", context.RepoMustNotBeArchived(), repo.CancelAutoMergePullRequest) diff --git a/templates/repo/commit_header.tmpl b/templates/repo/commit_header.tmpl index 8a074e9545..9604daf2b0 100644 --- a/templates/repo/commit_header.tmpl +++ b/templates/repo/commit_header.tmpl @@ -14,10 +14,19 @@ {{end}} {{end}}
-
+

{{RenderCommitMessage $.Context .Commit.Message ($.Repository.ComposeMetas ctx)}}{{template "repo/commit_statuses" dict "Status" .CommitStatus "Statuses" .CommitStatuses}}

- {{if not $.PageIsWiki}} -
+
+ {{if .PageIsPullFiles}} +
+ + {{svg "octicon-chevron-left"}} {{ctx.Locale.Tr "repo.diff.commit.previous-short"}} + + + {{ctx.Locale.Tr "repo.diff.commit.next-short"}} {{svg "octicon-chevron-right"}} + +
+ {{else if not $.PageIsWiki}} {{ctx.Locale.Tr "repo.diff.browse_source"}} @@ -132,8 +141,8 @@
{{end}} -
- {{end}} + {{end}} +
{{if IsMultilineCommitMessage .Commit.Message}} 
{{RenderCommitBody $.Context .Commit.Message ($.Repository.ComposeMetas ctx)}}
@@ -185,9 +194,16 @@ {{end}}
{{ctx.Locale.Tr "repo.diff.commit"}} - - {{ShortSha .CommitID}} - + {{if .PageIsPullFiles}} + {{$commitShaLink := (printf "%s/commit/%s" $.RepoLink (PathEscape .CommitID))}} + + {{ShortSha .CommitID}} + + {{else}} + + {{ShortSha .CommitID}} + + {{end}}
diff --git a/templates/repo/commit_load_branches_and_tags.tmpl b/templates/repo/commit_load_branches_and_tags.tmpl index ffa0e530e8..25402ca2f4 100644 --- a/templates/repo/commit_load_branches_and_tags.tmpl +++ b/templates/repo/commit_load_branches_and_tags.tmpl @@ -1,4 +1,4 @@ -{{if not .PageIsWiki}} +{{if not (or .PageIsWiki .PageIsPullFiles)}}
{{if and (not $.Repository.IsArchived) (not .DiffNotAvailable)}} diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl index 13d09babe1..ded7a6c5fc 100644 --- a/templates/repo/diff/new_review.tmpl +++ b/templates/repo/diff/new_review.tmpl @@ -1,17 +1,14 @@
-
- {{if $.IsShowingAllCommits}}
@@ -55,5 +52,4 @@
- {{end}}
diff --git a/tests/e2e/pr-review.test.e2e.ts b/tests/e2e/pr-review.test.e2e.ts index b619cdbcd1..577c8bf20a 100644 --- a/tests/e2e/pr-review.test.e2e.ts +++ b/tests/e2e/pr-review.test.e2e.ts @@ -11,7 +11,7 @@ import {save_visual, test} from './utils_e2e.ts'; test.use({user: 'user2'}); -test('PR: Finish review', async ({page}) => { +test('PR: Create review from files', async ({page}) => { const response = await page.goto('/user2/repo1/pulls/5/files'); expect(response?.status()).toBe(200); @@ -22,4 +22,93 @@ test('PR: Finish review', async ({page}) => { await page.locator('#review-box .js-btn-review').click(); await expect(page.locator('.tippy-box .review-box-panel')).toBeVisible(); await save_visual(page); + + await page.locator('.review-box-panel textarea#_combo_markdown_editor_0') + .fill('This is a review'); + await page.locator('.review-box-panel button.btn-submit[value="approve"]').click(); + await page.waitForURL(/.*\/user2\/repo1\/pulls\/5#issuecomment-\d+/); + await save_visual(page); +}); + +test('PR: Create review from commit', async ({page}) => { + const response = await page.goto('/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269'); + expect(response?.status()).toBe(200); + + await page.locator('button.add-code-comment').click(); + const code_comment = page.locator('.comment-code-cloud form textarea.markdown-text-editor'); + await expect(code_comment).toBeVisible(); + + await code_comment.fill('This is a code comment'); + await save_visual(page); + + const start_button = page.locator('.comment-code-cloud form button.btn-start-review'); + // Workaround for #7152, where there might already be a pending review state from previous + // test runs (most likely to happen when debugging tests). + if (await start_button.isVisible({timeout: 100})) { + await start_button.click(); + } else { + await page.locator('.comment-code-cloud form button.btn-add-comment').click(); + } + + await expect(page.locator('.comment-list .comment-container')).toBeVisible(); + + // We need to wait for the review to be processed. Checking the comment counter + // conveniently does that. + await expect(page.locator('#review-box .js-btn-review > span.review-comments-counter')).toHaveText('1'); + + await page.locator('#review-box .js-btn-review').click(); + await expect(page.locator('.tippy-box .review-box-panel')).toBeVisible(); + await save_visual(page); + + await page.locator('.review-box-panel textarea.markdown-text-editor') + .fill('This is a review'); + await page.locator('.review-box-panel button.btn-submit[value="approve"]').click(); + await page.waitForURL(/.*\/user2\/repo1\/pulls\/3#issuecomment-\d+/); + await save_visual(page); + + // In addition to testing the ability to delete comments, this also + // performs clean up. If tests are run for multiple platforms, the data isn't reset + // in-between, and subsequent runs of this test would fail, because when there already is + // a comment, the on-hover button to start a conversation doesn't appear anymore. + await page.goto('/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269'); + await page.locator('.comment-header-right.actions a.context-menu').click(); + + await expect(page.locator('.comment-header-right.actions div.menu').getByText(/Copy link.*/)).toBeVisible(); + // The button to delete a comment will prompt for confirmation using a browser alert. + page.on('dialog', (dialog) => dialog.accept()); + await page.locator('.comment-header-right.actions div.menu .delete-comment').click(); + + await expect(page.locator('.comment-list .comment-container')).toBeHidden(); + await save_visual(page); +}); + +test('PR: Navigate by single commit', async ({page}) => { + const response = await page.goto('/user2/repo1/pulls/3/commits'); + expect(response?.status()).toBe(200); + + await page.locator('tbody.commit-list td.message a').nth(1).click(); + await page.waitForURL(/.*\/user2\/repo1\/pulls\/3\/commits\/4a357436d925b5c974181ff12a994538ddc5a269/); + await save_visual(page); + + let prevButton = page.locator('.commit-header-buttons').getByText(/Prev/); + let nextButton = page.locator('.commit-header-buttons').getByText(/Next/); + await prevButton.waitFor(); + await nextButton.waitFor(); + + await expect(prevButton).toHaveClass(/disabled/); + await expect(nextButton).not.toHaveClass(/disabled/); + await expect(nextButton).toHaveAttribute('href', '/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd'); + await nextButton.click(); + + await page.waitForURL(/.*\/user2\/repo1\/pulls\/3\/commits\/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd/); + await save_visual(page); + + prevButton = page.locator('.commit-header-buttons').getByText(/Prev/); + nextButton = page.locator('.commit-header-buttons').getByText(/Next/); + await prevButton.waitFor(); + await nextButton.waitFor(); + + await expect(prevButton).not.toHaveClass(/disabled/); + await expect(nextButton).toHaveClass(/disabled/); + await expect(prevButton).toHaveAttribute('href', '/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269'); }); diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1 b/tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1 index cf96195665..34c9ccecdd 100644 --- a/tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1 +++ b/tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1 @@ -1 +1,2 @@ 0000000000000000000000000000000000000000 1978192d98bb1b65e11c2cf37da854fbf94bffd6 Gitea 1688672383 +0200 push +1978192d98bb1b65e11c2cf37da854fbf94bffd6 9b93963cf6de4dc33f915bb67f192d099c301f43 Forgejo 1749737639 +0200 push diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/06/c5865eaeaaa2f92e8fce75a281f6272ee68e90 b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/06/c5865eaeaaa2f92e8fce75a281f6272ee68e90 new file mode 100644 index 0000000000000000000000000000000000000000..009c9849d9cd98d1b00f31317ff405b14e76dbcd GIT binary patch literal 223 zcmV<503iQ(0ZY!$&CM)PFfuY@C@D%!RWLQOFiB1{Pct<)HMcM{OEoi3Hn6lXGBQcD zFg7z!Of)hzF)=bSO5rL6)QV32>N-QqPOw3aVPAp9=Qm`ooQF_HNVTbhG#LOJMB#Z380%11^YTOHzvz-13XkQ?v3FY|pDtGx literal 0 HcmV?d00001 diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/9b/93963cf6de4dc33f915bb67f192d099c301f43 b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/9b/93963cf6de4dc33f915bb67f192d099c301f43 new file mode 100644 index 0000000000000000000000000000000000000000..41814996f1fb04ca0555fd0e2118797036fd8d4d GIT binary patch literal 224 zcmV<603ZK&0ZY!$&CM)PFfuY{C@D%!RWMIYF-$QrN=-?$G&D3YGdD{yO*T$6H#ai0 zG&4;xOHDCMOiQ&ePUb2|EK1EQQ7}p|GflBHN=ddbFi18_GcmC+H8C_YF-SJFNVPCZ zN;5V~vNSbGO5#c^Ey>6)QV32>N-QqPOw3aVPAp9=Qm`ooQF_HNVTbhG#LOJMB#Z380%11^YTOHzvz-13XkQ?v3FY|6)QV32>N-QqPOw3aVPAp9=Qm`ooQF_HNVTbhG#LOJMB#Z380%11^YTOHzvz-13XkQ?v3FY|cHZV0$F-l2H zvam=qwMa5BNJ+9VHsmTuEK1EQQAkWmG)gkIFiSK{O)^L_FfunzOi46IOHDCJGfy!v zH%&21H8wF$HsneyEy>6)QV32>N-QqPOw3aVPAp9=Qm`ooQF_HNVTbhG#LOJMB#Z380%11^YTOHzvz-13XkQ?v3FY|P@Vt) literal 0 HcmV?d00001 diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/c9/cf8a3095808af2425255056e01746fef420801 b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/c9/cf8a3095808af2425255056e01746fef420801 new file mode 100644 index 0000000000000000000000000000000000000000..32a51a2837f6416b0eea0793fbeb53e47a4e74c2 GIT binary patch literal 223 zcmV<503iQ(0ZY!$&CM)PFfuY@C@D%!RY)~7Pc=wOG%`ptPBAhxGPN``OEXGKGD6)QV32>N-QqPOw3aVPAp9=Qm`ooQF_HNVTbhG#LOJMB#Z380%11^YTOHzvz-13XkQ?v3FY| Date: Tue, 17 Jun 2025 10:15:48 +0200 Subject: [PATCH 6/8] fix: git_model.CommitStatusesHideActionsURL is obsolete (#8209) Refs: https://codeberg.org/forgejo/forgejo/pulls/7155 Refs: https://codeberg.org/forgejo/forgejo/pulls/8177 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8209 Reviewed-by: Beowulf Reviewed-by: Michael Kriese Reviewed-by: Lucas Co-authored-by: Earl Warren Co-committed-by: Earl Warren --- routers/web/repo/pull.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 8547bbcc03..fd18646211 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -988,9 +988,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi if err != nil { log.Error("GetLatestCommitStatus: %v", err) } - if !ctx.Repo.CanRead(unit.TypeActions) { - git_model.CommitStatusesHideActionsURL(ctx, statuses) - } ctx.Data["CommitStatus"] = git_model.CalcCommitStatus(statuses) ctx.Data["CommitStatuses"] = statuses From adc273e3a893bf64b7d95416d3ae9c39a4a23df0 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Tue, 17 Jun 2025 10:58:07 +0200 Subject: [PATCH 7/8] fix: do not ignore automerge while a PR is checking for conflicts (#8189) Automerge can be ignored when the following race happens: * Conflict check is happening on a repository and `pr.Status = issues_model.PullRequestStatusChecking` for all open pull requests (this happens every time a pull request is merged). * While the conflict check is ongoing, an event (Forgejo Actions being successful for instance) happens and and `StartPRCheckAndAutoMerge*` is called. * Because `pr.CanAutoMerge()` is false, the pull request is not selected and not added to the automerge queue. * When the conflict check completes and `pr.CanAutoMerge()` becomes true, there no longer is a task in the auto merge queue and the auto merge does not happen. This is fixed by adding a task to the auto merge queue when the conflict check for a pull request completes. This is done when the mutx protecting the conflict check task is released to prevent a deadlock when a synchronous queues are used in the following situation: * the conflict check task finds the pull request is mergeable * it schedules the auto merge tasks that finds it must be merged * merging concludes with scheduling a conflict check task Avoid an extra loop where a conflict check task queues an auto merge task that will schedule a conflict check task if the pull request can be merged. The auto merge row is removed from the database before merging. It would otherwise be removed after the merge commit is received via the git hook which happens asynchronously and can lead to a race. StartPRCheckAndAutoMerge is modified to re-use HeadCommitID when available, such as when called after a pull request conflict check. --- A note on tests: they cover the new behavior, i.e. automerge being triggered by a successful conflict check. This is also on the critical paths for every test that involve creating, merging or updating a pull request. - `tests/integration/git_test.go` - `tests/integration/actions_commit_status_test.go` - `tests/integration/api_helper_for_declarative_test.go` - `tests/integration/patch_status_test.go` - `tests/integration/pull_merge_test.go` The [missing fixture file](https://codeberg.org/forgejo/forgejo/pulls/8189/files#diff-b86fdd79108b3ba3cb2e56ffcfd1be2a7b32f46c) for the auto merge table can be verified to be necessary simply by removing it an observing that the integration tests fail. The [scheduling of the auto merge task](https://codeberg.org/forgejo/forgejo/pulls/8189/files#diff-9489262e93967f6bb2db41837f37c06f4e70d978) in `testPR` can be verified to be required by moving it in the `testPRProtected` function and observing that the tests hang forever because of the deadlock. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. ## Release notes - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/8189): do not ignore automerge while a PR is checking for conflicts Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8189 Reviewed-by: Michael Kriese Reviewed-by: Lucas Co-authored-by: Earl Warren Co-committed-by: Earl Warren --- models/fixtures/pull_auto_merge.yml | 1 + models/pull/automerge.go | 9 ++++- services/automerge/automerge.go | 5 +++ services/pull/check.go | 30 +++++++++++----- services/shared/automerge/automerge.go | 48 ++++++++++++++++++-------- tests/integration/README.md | 4 +-- tests/integration/patch_status_test.go | 26 ++++++++++++-- 7 files changed, 94 insertions(+), 29 deletions(-) create mode 100644 models/fixtures/pull_auto_merge.yml diff --git a/models/fixtures/pull_auto_merge.yml b/models/fixtures/pull_auto_merge.yml new file mode 100644 index 0000000000..ca780a73aa --- /dev/null +++ b/models/fixtures/pull_auto_merge.yml @@ -0,0 +1 @@ +[] # empty diff --git a/models/pull/automerge.go b/models/pull/automerge.go index 63f572309b..dcc1f39271 100644 --- a/models/pull/automerge.go +++ b/models/pull/automerge.go @@ -10,6 +10,7 @@ import ( "forgejo.org/models/db" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/log" "forgejo.org/modules/timeutil" ) @@ -58,13 +59,15 @@ func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pullID int64, return ErrAlreadyScheduledToAutoMerge{PullID: pullID} } - _, err := db.GetEngine(ctx).Insert(&AutoMerge{ + scheduledPRM, err := db.GetEngine(ctx).Insert(&AutoMerge{ DoerID: doer.ID, PullID: pullID, MergeStyle: style, Message: message, DeleteBranchAfterMerge: deleteBranch, }) + log.Trace("ScheduleAutoMerge %+v for PR %d", scheduledPRM, pullID) + return err } @@ -81,6 +84,8 @@ func GetScheduledMergeByPullID(ctx context.Context, pullID int64) (bool, *AutoMe return false, nil, err } + log.Trace("GetScheduledMergeByPullID found %+v for PR %d", scheduledPRM, pullID) + scheduledPRM.Doer = doer return true, scheduledPRM, nil } @@ -94,6 +99,8 @@ func DeleteScheduledAutoMerge(ctx context.Context, pullID int64) error { return db.ErrNotExist{Resource: "auto_merge", ID: pullID} } + log.Trace("DeleteScheduledAutoMerge %+v for PR %d", scheduledPRM, pullID) + _, err = db.GetEngine(ctx).ID(scheduledPRM.ID).Delete(&AutoMerge{}) return err } diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index f183136907..cbfe3bd54e 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -107,6 +107,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } if !exists { + log.Trace("GetScheduledMergeByPullID found nothing for PR %d", pullID) return } @@ -204,6 +205,10 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } + if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { + log.Error("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err) + } + if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil { log.Error("pull_service.Merge: %v", err) // FIXME: if merge failed, we should display some error message to the pull request page. diff --git a/services/pull/check.go b/services/pull/check.go index afeb7e675e..6002e2ae26 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -28,6 +28,7 @@ import ( "forgejo.org/modules/timeutil" asymkey_service "forgejo.org/services/asymkey" notify_service "forgejo.org/services/notify" + shared_automerge "forgejo.org/services/shared/automerge" ) // prPatchCheckerQueue represents a queue to handle update pull request tests @@ -170,7 +171,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer // checkAndUpdateStatus checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. -func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { +func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) bool { // If status has not been changed to conflict by testPatch then we are mergeable if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable @@ -184,12 +185,15 @@ func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { if has { log.Trace("Not updating status for %-v as it is due to be rechecked", pr) - return + return false } if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { log.Error("Update[%-v]: %v", pr, err) + return false } + + return true } // getMergeCommit checks if a pull request has been merged @@ -339,15 +343,22 @@ func handler(items ...string) []string { } func testPR(id int64) { - pullWorkingPool.CheckIn(fmt.Sprint(id)) - defer pullWorkingPool.CheckOut(fmt.Sprint(id)) ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("Test PR[%d] from patch checking queue", id)) defer finished() + if pr, updated := testPRProtected(ctx, id); pr != nil && updated { + shared_automerge.AddToQueueIfMergeable(ctx, pr) + } +} + +func testPRProtected(ctx context.Context, id int64) (*issues_model.PullRequest, bool) { + pullWorkingPool.CheckIn(fmt.Sprint(id)) + defer pullWorkingPool.CheckOut(fmt.Sprint(id)) + pr, err := issues_model.GetPullRequestByID(ctx, id) if err != nil { log.Error("Unable to GetPullRequestByID[%d] for testPR: %v", id, err) - return + return nil, false } log.Trace("Testing %-v", pr) @@ -357,12 +368,12 @@ func testPR(id int64) { if pr.HasMerged { log.Trace("%-v is already merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) - return + return nil, false } if manuallyMerged(ctx, pr) { log.Trace("%-v is manually merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) - return + return nil, false } if err := TestPatch(pr); err != nil { @@ -371,9 +382,10 @@ func testPR(id int64) { if err := pr.UpdateCols(ctx, "status"); err != nil { log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err) } - return + return nil, false } - checkAndUpdateStatus(ctx, pr) + + return pr, checkAndUpdateStatus(ctx, pr) } // CheckPRsForBaseBranch check all pulls with baseBrannch diff --git a/services/shared/automerge/automerge.go b/services/shared/automerge/automerge.go index 1dc309f4b3..be7b2f6eb4 100644 --- a/services/shared/automerge/automerge.go +++ b/services/shared/automerge/automerge.go @@ -21,9 +21,9 @@ import ( var PRAutoMergeQueue *queue.WorkerPoolQueue[string] func addToQueue(pr *issues_model.PullRequest, sha string) { - log.Trace("Adding pullID: %d to the pull requests patch checking queue with sha %s", pr.ID, sha) + log.Trace("Adding pullID: %d to the automerge queue with sha %s", pr.ID, sha) if err := PRAutoMergeQueue.Push(fmt.Sprintf("%d_%s", pr.ID, sha)); err != nil { - log.Error("Error adding pullID: %d to the pull requests patch checking queue %v", pr.ID, err) + log.Error("Error adding pullID: %d to the automerge queue %v", pr.ID, err) } } @@ -43,32 +43,29 @@ func StartPRCheckAndAutoMergeBySHA(ctx context.Context, sha string, repo *repo_m return nil } -// StartPRCheckAndAutoMerge start an automerge check and auto merge task for a pull request func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullRequest) { if pull == nil || pull.HasMerged || !pull.CanAutoMerge() { return } - if err := pull.LoadBaseRepo(ctx); err != nil { - log.Error("LoadBaseRepo: %v", err) - return + commitID := pull.HeadCommitID + if commitID == "" { + commitID = getCommitIDFromRefName(ctx, pull) } - gitRepo, err := gitrepo.OpenRepository(ctx, pull.BaseRepo) - if err != nil { - log.Error("OpenRepository: %v", err) - return - } - defer gitRepo.Close() - commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) - if err != nil { - log.Error("GetRefCommitID: %v", err) + if commitID == "" { return } addToQueue(pull, commitID) } +var AddToQueueIfMergeable = func(ctx context.Context, pull *issues_model.PullRequest) { + if pull.Status == issues_model.PullRequestStatusMergeable { + StartPRCheckAndAutoMerge(ctx, pull) + } +} + func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model.Repository, filter func(*issues_model.PullRequest) bool) (map[int64]*issues_model.PullRequest, error) { gitRepo, err := gitrepo.OpenRepository(ctx, repo) if err != nil { @@ -118,3 +115,24 @@ func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model. return pulls, nil } + +func getCommitIDFromRefName(ctx context.Context, pull *issues_model.PullRequest) string { + if err := pull.LoadBaseRepo(ctx); err != nil { + log.Error("LoadBaseRepo: %v", err) + return "" + } + + gitRepo, err := gitrepo.OpenRepository(ctx, pull.BaseRepo) + if err != nil { + log.Error("OpenRepository: %v", err) + return "" + } + defer gitRepo.Close() + commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) + if err != nil { + log.Error("GetRefCommitID: %v", err) + return "" + } + + return commitID +} diff --git a/tests/integration/README.md b/tests/integration/README.md index d83685388e..a6be7fe72c 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -71,11 +71,11 @@ TEST_MYSQL_HOST=localhost:3306 TEST_MYSQL_DBNAME=test?multiStatements=true TEST_ ### Run pgsql integration tests Setup a pgsql database inside docker ``` -docker run -e "POSTGRES_DB=test" -e POSTGRES_PASSWORD=postgres -p 5432:5432 --rm --name pgsql postgres:latest #(Ctrl-c to stop the database) +docker run -e "POSTGRES_DB=test" -e POSTGRES_PASSWORD=postgres POSTGRESQL_FSYNC=off POSTGRESQL_EXTRA_FLAGS="-c full_page_writes=off" -p 5432:5432 --rm --name pgsql data.forgejo.org/oci/bitnami/postgresql:16 #(Ctrl-c to stop the database) ``` Start tests based on the database container ``` -TEST_STORAGE_TYPE=local TEST_PGSQL_HOST=localhost:5432 TEST_PGSQL_DBNAME=test TEST_PGSQL_USERNAME=postgres TEST_PGSQL_PASSWORD=postgres make test-pgsql +TEST_STORAGE_TYPE=local TEST_PGSQL_HOST=localhost:5432 TEST_PGSQL_DBNAME=test TEST_PGSQL_USERNAME=postgres TEST_PGSQL_PASSWORD=postgres make 'test-pgsql#Test' ``` ### Running individual tests diff --git a/tests/integration/patch_status_test.go b/tests/integration/patch_status_test.go index 078051fe63..3ce1dc4cb9 100644 --- a/tests/integration/patch_status_test.go +++ b/tests/integration/patch_status_test.go @@ -4,6 +4,7 @@ package integration import ( + "context" "fmt" "net/http" "net/url" @@ -20,7 +21,10 @@ import ( user_model "forgejo.org/models/user" "forgejo.org/modules/git" "forgejo.org/modules/optional" + "forgejo.org/modules/test" + pull_service "forgejo.org/services/pull" files_service "forgejo.org/services/repository/files" + shared_automerge "forgejo.org/services/shared/automerge" "forgejo.org/tests" "github.com/stretchr/testify/assert" @@ -51,6 +55,20 @@ func TestPatchStatus(t *testing.T) { }) defer f() + testAutomergeQueued := func(t *testing.T, pr *issues_model.PullRequest, expected issues_model.PullRequestStatus) { + t.Helper() + + var actual issues_model.PullRequestStatus = -1 + defer test.MockVariableValue(&shared_automerge.AddToQueueIfMergeable, func(ctx context.Context, pull *issues_model.PullRequest) { + actual = pull.Status + })() + + pull_service.AddToTaskQueue(t.Context(), pr) + assert.Eventually(t, func() bool { + return expected == actual + }, time.Second*5, time.Millisecond*200) + } + testRepoFork(t, session, "user2", repo.Name, "org3", "forked-repo") forkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "org3", Name: "forked-repo"}) @@ -91,7 +109,9 @@ func TestPatchStatus(t *testing.T) { require.NoError(t, git.NewCommand(t.Context(), "push", "fork", "HEAD:normal").Run(&git.RunOpts{Dir: dstPath})) testPullCreateDirectly(t, session, repo.OwnerName, repo.Name, repo.DefaultBranch, forkRepo.OwnerName, forkRepo.Name, "normal", "across repo normal") - test(t, unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0")) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0") + test(t, pr) + testAutomergeQueued(t, pr, issues_model.PullRequestStatusMergeable) }) t.Run("Same repository", func(t *testing.T) { @@ -144,7 +164,9 @@ func TestPatchStatus(t *testing.T) { t.Run("Existing", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - test(t, unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0")) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0") + test(t, pr) + testAutomergeQueued(t, pr, issues_model.PullRequestStatusConflict) }) t.Run("New", func(t *testing.T) { From 86d65ba0bebd48470a7fa1240f8566dfdd4d6242 Mon Sep 17 00:00:00 2001 From: David Rotermund Date: Tue, 17 Jun 2025 13:10:03 +0200 Subject: [PATCH 8/8] tests/e2e/declare_repos_test.go: Removed unneccessary if condition --- tests/e2e/declare_repos_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/declare_repos_test.go b/tests/e2e/declare_repos_test.go index 0ccb776ff7..4c68566a48 100644 --- a/tests/e2e/declare_repos_test.go +++ b/tests/e2e/declare_repos_test.go @@ -82,7 +82,7 @@ func newRepo(t *testing.T, userID int64, repoName string, fileChanges []FileChan for _, file := range fileChanges { for i, version := range file.Versions { operation := "update" - if i == 0 && file.Filename != "README.md" { + if i == 0 { operation = "create" }