tests/e2e/declare_repos_test.go: Removed unneccessary if condition

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/<pull request number>.md` to be be used for the release notes instead of the title.

<!--start release-notes-assistant-->

## Release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Bug fixes
  - [PR](https://codeberg.org/forgejo/forgejo/pulls/8189): <!--number 8189 --><!--line 0 --><!--description ZG8gbm90IGlnbm9yZSBhdXRvbWVyZ2Ugd2hpbGUgYSBQUiBpcyBjaGVja2luZyBmb3IgY29uZmxpY3Rz-->do not ignore automerge while a PR is checking for conflicts<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8189
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: Lucas <sclu1034@noreply.codeberg.org>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>

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

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

models/fixtures/pull_auto_merge.yml

@@ -0,0 +1 @@
+[] # empty

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":

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

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())
 

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
 }

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 <kbd>Tab</kbd> again or <kbd>Escape</kbd> to leave the editor.",
     "editor.textarea.shift_tab_hint": "No indentation on this line. Press <kbd>Shift</kbd> + <kbd>Tab</kbd> again or <kbd>Escape</kbd> to leave the editor.",

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"

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",

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

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,78 @@ 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)
+		}
+
+		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 +1030,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 		} else {
 			startCommitID = prInfo.MergeBase
 		}
+
 		ctx.Data["IsShowingAllCommits"] = false
 	} else {
 		endCommitID = headCommitID
@@ -937,10 +1038,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")
 

routers/web/web.go

@@ -1511,7 +1511,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)

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.

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

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
+}

templates/repo/commit_header.tmpl

@@ -14,10 +14,19 @@
 	{{end}}
 {{end}}
 <div class="ui top attached header clearing segment tw-relative commit-header {{$class}}">
-	<div class="tw-flex tw-mb-4 tw-gap-1">
+	<div class="tw-flex tw-mb-4 tw-gap-1 max-sm:tw-flex-col">
 		<h3 class="tw-mb-0 tw-flex-1"><span class="commit-summary" title="{{.Commit.Summary}}">{{RenderCommitMessage $.Context .Commit.Message ($.Repository.ComposeMetas ctx)}}</span>{{template "repo/commit_statuses" dict "Status" .CommitStatus "Statuses" .CommitStatuses}}</h3>
-		{{if not $.PageIsWiki}}
-			<div class="commit-header-buttons">
+		<div class="commit-header-buttons">
+			{{if .PageIsPullFiles}}
+				<div class="ui buttons">
+					<a class="ui small button {{if .PrevCommitLink}}" href="{{.PrevCommitLink}}"{{else}}disabled"{{end}}>
+						{{svg "octicon-chevron-left"}} {{ctx.Locale.Tr "repo.diff.commit.previous-short"}}
+					</a>
+					<a class="ui small button {{if .NextCommitLink}}" href="{{.NextCommitLink}}"{{else}}disabled"{{end}}>
+						{{ctx.Locale.Tr "repo.diff.commit.next-short"}} {{svg "octicon-chevron-right"}}
+					</a>
+				</div>
+			{{else if not $.PageIsWiki}}
 				<a class="ui primary tiny button" href="{{.SourcePath}}">
 					{{ctx.Locale.Tr "repo.diff.browse_source"}}
 				</a>
@@ -132,8 +141,8 @@
 						</div>
 					</div>
 				{{end}}
-			</div>
-		{{end}}
+			{{end}}
+		</div>
 	</div>
 	{{if IsMultilineCommitMessage .Commit.Message}}
 		<pre class="commit-body">{{RenderCommitBody $.Context .Commit.Message ($.Repository.ComposeMetas ctx)}}</pre>
@@ -185,9 +194,16 @@
 			{{end}}
 			<div class="item">
 				<span>{{ctx.Locale.Tr "repo.diff.commit"}}</span>
-				<span class="ui primary sha label">
-					<span class="shortsha">{{ShortSha .CommitID}}</span>
-				</span>
+				{{if .PageIsPullFiles}}
+					{{$commitShaLink := (printf "%s/commit/%s" $.RepoLink (PathEscape .CommitID))}}
+					<a href="{{$commitShaLink}}" class="ui primary sha label">
+						<span class="shortsha">{{ShortSha .CommitID}}</span>
+					</a>
+				{{else}}
+					<span class="ui primary sha label">
+						<span class="shortsha">{{ShortSha .CommitID}}</span>
+					</span>
+				{{end}}
 			</div>
 		</div>
 </div>

templates/repo/commit_load_branches_and_tags.tmpl

@@ -1,4 +1,4 @@
-{{if not .PageIsWiki}}
+{{if not (or .PageIsWiki .PageIsPullFiles)}}
 <div class="branch-and-tag-area" data-text-default-branch-tooltip="{{ctx.Locale.Tr "repo.commit.contained_in_default_branch"}}">
 	<button class="ui button ellipsis-button load-branches-and-tags tw-mt-2" aria-expanded="false"
 		data-fetch-url="{{.RepoLink}}/commit/{{.CommitID}}/load-branches-and-tags"

templates/repo/commits_list.tmpl

@@ -49,6 +49,9 @@
 								<span class="commit-summary {{if gt .ParentCount 1}} grey text{{end}}" title="{{.Summary}}">{{.Summary | RenderEmoji $.Context}}</span>
 							{{else}}
 								{{$commitLink:= printf "%s/commit/%s" $commitRepoLink (PathEscape .ID.String)}}
+								{{if $.PageIsPullCommits}}
+									{{$commitLink = (printf "%s/pulls/%d/commits/%s" $commitRepoLink $.Issue.Index (PathEscape .ID.String))}}
+								{{end}}
 								<span class="commit-summary {{if gt .ParentCount 1}} grey text{{end}}" title="{{.Summary}}">{{RenderCommitMessageLinkSubject $.Context .Message $commitLink ($.Repository.ComposeMetas ctx)}}</span>
 							{{end}}
 							</span>

templates/repo/commits_list_small.tmpl

@@ -11,7 +11,14 @@
 			{{ctx.AvatarUtils.AvatarByEmail .Author.Email .Author.Name 20}}
 		{{end}}
 
-		{{$commitLink:= printf "%s/commit/%s" $.comment.Issue.PullRequest.BaseRepo.Link (PathEscape .ID.String)}}
+		{{$commitLink := printf "%s/commit/%s" $.comment.Issue.PullRequest.BaseRepo.Link (PathEscape .ID.String)}}
+
+		{{/* Only link those commits in the file diff view that can actually be reviewed there.
+					A force push or other rewriting of history will cause a commit to not be
+					part of the pull request anymore. */}}
+		{{if index $.root.CommitIDs .ID.String}}
+			{{$commitLink = printf "%s/pulls/%d/commits/%s" $.comment.Issue.PullRequest.BaseRepo.Link $.comment.Issue.Index (PathEscape .ID.String)}}
+		{{end}}
 
 		<span class="tw-flex-1 tw-font-mono gt-ellipsis" title="{{.Summary}}">
 			{{- RenderCommitMessageLinkSubject $.root.Context .Message $commitLink ($.comment.Issue.PullRequest.BaseRepo.ComposeMetas ctx) -}}

templates/repo/diff/box.tmpl

@@ -53,7 +53,7 @@
 	{{if not .DiffNotAvailable}}
 		{{if and .IsShowingOnlySingleCommit .PageIsPullFiles}}
 			<div class="ui info message">
-				<div>{{ctx.Locale.Tr "repo.pulls.showing_only_single_commit" (ShortSha .AfterCommitID)}} - <a href="{{$.Issue.Link}}/files?style={{if $.IsSplitStyle}}split{{else}}unified{{end}}&whitespace={{$.WhitespaceBehavior}}&show-outdated={{$.ShowOutdatedComments}}">{{ctx.Locale.Tr "repo.pulls.show_all_commits"}}</a></div>
+				<div>{{ctx.Locale.Tr "repo.pulls.showing_only_single_commit" (ShortSha .CommitID)}} - <a href="{{$.Issue.Link}}/files?style={{if $.IsSplitStyle}}split{{else}}unified{{end}}&whitespace={{$.WhitespaceBehavior}}&show-outdated={{$.ShowOutdatedComments}}">{{ctx.Locale.Tr "repo.pulls.show_all_commits"}}</a></div>
 			</div>
 		{{else if and (not .IsShowingAllCommits) .PageIsPullFiles}}
 			<div class="ui info message">
@@ -93,6 +93,12 @@
 				if (diffTreeVisible) document.getElementById('diff-file-tree').classList.remove('tw-hidden');
 			</script>
 		{{end}}
+		<div id="diff-content-container">
+		{{if .IsShowingOnlySingleCommit}}
+			<div id="diff-commit-header">
+			{{template "repo/commit_header" .}}
+			</div>
+		{{end}}
 		{{if .DiffNotAvailable}}
 			<h4>{{ctx.Locale.Tr "repo.diff.data_not_available"}}</h4>
 		{{else}}
@@ -230,6 +236,7 @@
 				{{end}}
 			</div>
 		{{end}}
+		</div>
 	</div>
 
 	{{if and (not $.Repository.IsArchived) (not .DiffNotAvailable)}}

templates/repo/diff/new_review.tmpl

@@ -1,17 +1,14 @@
 <div id="review-box">
 	<div
-		{{if not $.IsShowingAllCommits}}
-			data-tooltip-content="{{ctx.Locale.Tr "repo.pulls.review_only_possible_for_full_diff"}}"
-		{{else if .Repository.IsArchived}}
+		{{if .Repository.IsArchived}}
 			data-tooltip-content="{{ctx.Locale.Tr "repo.archive.pull.noreview"}}"
 		{{end}}>
-		<button class="ui tiny primary button tw-pr-1 tw-flex js-btn-review {{if or (not $.IsShowingAllCommits) .Repository.IsArchived}}disabled{{end}}">
+		<button class="ui tiny primary button tw-pr-1 tw-flex js-btn-review {{if .Repository.IsArchived}}disabled{{end}}">
 			{{ctx.Locale.Tr "repo.diff.review"}}
 			<span class="ui small label review-comments-counter" data-pending-comment-number="{{.PendingCodeCommentNumber}}">{{.PendingCodeCommentNumber}}</span>
 			{{svg "octicon-triangle-down" 14 "dropdown icon"}}
 		</button>
 	</div>
-	{{if $.IsShowingAllCommits}}
 	<div class="review-box-panel tippy-target">
 		<div class="ui segment">
 			<form class="ui form form-fetch-action" action="{{.Link}}/reviews/submit" method="post">
@@ -55,5 +52,4 @@
 			</form>
 		</div>
 	</div>
-	{{end}}
 </div>

templates/swagger/v1_json.tmpl

@@ -12994,6 +12994,7 @@
             "enum": [
               "oldest",
               "recentupdate",
+              "recentclose",
               "leastupdate",
               "mostcomment",
               "leastcomment",

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"
 			}
 

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');
 });

tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1

@@ -1 +1,2 @@
 0000000000000000000000000000000000000000 1978192d98bb1b65e11c2cf37da854fbf94bffd6 Gitea <gitea@fake.local> 1688672383 +0200	push
+1978192d98bb1b65e11c2cf37da854fbf94bffd6 9b93963cf6de4dc33f915bb67f192d099c301f43 Forgejo <forgejo@fake.local> 1749737639 +0200	push

tests/gitea-repositories-meta/user2/commitsonpr.git/refs/heads/branch1

@@ -1 +1 @@
-1978192d98bb1b65e11c2cf37da854fbf94bffd6
+9b93963cf6de4dc33f915bb67f192d099c301f43

tests/gitea-repositories-meta/user2/commitsonpr.git/refs/pull/1/head

@@ -1 +1 @@
-1978192d98bb1b65e11c2cf37da854fbf94bffd6
+9b93963cf6de4dc33f915bb67f192d099c301f43

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

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) {

tests/integration/pull_commit_test.go

@@ -31,3 +31,20 @@ func TestListPullCommits(t *testing.T) {
 	}
 	assert.Equal(t, "4a357436d925b5c974181ff12a994538ddc5a269", pullCommitList.LastReviewCommitSha)
 }
+
+func TestPullCommitLinks(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	req := NewRequest(t, "GET", "/user2/repo1/pulls/3/commits")
+	resp := MakeRequest(t, req, http.StatusOK)
+
+	htmlDoc := NewHTMLParser(t, resp.Body)
+
+	commitSha := htmlDoc.Find(".commit-list td.sha a.sha.label").First()
+	commitShaHref, _ := commitSha.Attr("href")
+	assert.Equal(t, "/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", commitShaHref)
+
+	commitLink := htmlDoc.Find(".commit-list td.message a").First()
+	commitLinkHref, _ := commitLink.Attr("href")
+	assert.Equal(t, "/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", commitLinkHref)
+}

tests/integration/pull_diff_test.go

@@ -14,22 +14,22 @@ import (
 )
 
 func TestPullDiff_CompletePRDiff(t *testing.T) {
-	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files", false, []string{"test1.txt", "test10.txt", "test2.txt", "test3.txt", "test4.txt", "test5.txt", "test6.txt", "test7.txt", "test8.txt", "test9.txt"})
+	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files", []string{"test1.txt", "test10.txt", "test2.txt", "test3.txt", "test4.txt", "test5.txt", "test6.txt", "test7.txt", "test8.txt", "test9.txt"})
 }
 
 func TestPullDiff_SingleCommitPRDiff(t *testing.T) {
-	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test3.txt"})
+	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", []string{"test3.txt"})
 }
 
 func TestPullDiff_CommitRangePRDiff(t *testing.T) {
-	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", true, []string{"test2.txt", "test3.txt", "test4.txt"})
+	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", []string{"test2.txt", "test3.txt", "test4.txt"})
 }
 
 func TestPullDiff_StartingFromBaseToCommitPRDiff(t *testing.T) {
-	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test1.txt", "test2.txt", "test3.txt"})
+	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", []string{"test1.txt", "test2.txt", "test3.txt"})
 }
 
-func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expectedFilenames []string) {
+func doTestPRDiff(t *testing.T, prDiffURL string, expectedFilenames []string) {
 	defer tests.PrepareTestEnv(t)()
 
 	session := loginUser(t, "user2")
@@ -52,7 +52,4 @@ func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expect
 		filename, _ := s.Attr("data-old-filename")
 		assert.Equal(t, expectedFilenames[i], filename)
 	})
-
-	// Ensure the review button is enabled for full PR reviews
-	assert.Equal(t, reviewBtnDisabled, doc.doc.Find(".js-btn-review").HasClass("disabled"))
 }

tests/integration/pull_files_test.go

@@ -0,0 +1,106 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+
+	repo_model "forgejo.org/models/repo"
+	"forgejo.org/models/unittest"
+	"forgejo.org/modules/git"
+	"forgejo.org/tests"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPullFilesCommitHeader(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	t.Run("Verify commit info", func(t *testing.T) {
+		repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+		gitRepo, err := git.OpenRepository(git.DefaultContext, repo.RepoPath())
+		require.NoError(t, err)
+		defer gitRepo.Close()
+
+		commit, err := gitRepo.GetCommit("62fb502a7172d4453f0322a2cc85bddffa57f07a")
+		require.NoError(t, err)
+
+		req := NewRequest(t, "GET", "/user2/repo1/pulls/5/commits/62fb502a7172d4453f0322a2cc85bddffa57f07a")
+		resp := MakeRequest(t, req, http.StatusOK)
+
+		htmlDoc := NewHTMLParser(t, resp.Body)
+		header := htmlDoc.doc.Find("#diff-commit-header")
+
+		summary := header.Find(".commit-header h3")
+		assert.Equal(t, commit.Summary(), strings.TrimSpace(summary.Text()))
+
+		author := header.Find(".author strong")
+		assert.Equal(t, commit.Author.Name, author.Text())
+
+		date, _ := header.Find("#authored-time relative-time").Attr("datetime")
+		assert.Equal(t, commit.Author.When.Format(time.RFC3339), date)
+
+		sha := header.Find(".commit-header-row .sha.label")
+		shaHref, _ := sha.Attr("href")
+		assert.Equal(t, commit.ID.String()[:10], sha.Find(".shortsha").Text())
+		assert.Equal(t, "/user2/repo1/commit/62fb502a7172d4453f0322a2cc85bddffa57f07a", shaHref)
+	})
+
+	t.Run("Navigation", func(t *testing.T) {
+		t.Run("No previous on first commit", func(t *testing.T) {
+			req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847")
+			resp := MakeRequest(t, req, http.StatusOK)
+
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button")
+
+			assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header")
+
+			assert.True(t, buttons.First().HasClass("disabled"), "'prev' button should be disabled")
+			assert.False(t, buttons.Last().HasClass("disabled"), "'next' button should not be disabled")
+
+			href, _ := buttons.Last().Attr("href")
+			assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/96cef4a7b72b3c208340ae6f0cf55a93e9077c93", href)
+		})
+
+		t.Run("No next on last commit", func(t *testing.T) {
+			req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/9b93963cf6de4dc33f915bb67f192d099c301f43")
+			resp := MakeRequest(t, req, http.StatusOK)
+
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button")
+
+			assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header")
+
+			assert.False(t, buttons.First().HasClass("disabled"), "'prev' button should not be disabled")
+			assert.True(t, buttons.Last().HasClass("disabled"), "'next' button should be disabled")
+
+			href, _ := buttons.First().Attr("href")
+			assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/2d65d92dc800c6f448541240c18e82bf36b954bb", href)
+		})
+
+		t.Run("Both directions on middle commit", func(t *testing.T) {
+			req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2")
+			resp := MakeRequest(t, req, http.StatusOK)
+
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button")
+
+			assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header")
+
+			assert.False(t, buttons.First().HasClass("disabled"), "'prev' button should not be disabled")
+			assert.False(t, buttons.Last().HasClass("disabled"), "'next' button should not be disabled")
+
+			href, _ := buttons.First().Attr("href")
+			assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/96cef4a7b72b3c208340ae6f0cf55a93e9077c93", href)
+
+			href, _ = buttons.Last().Attr("href")
+			assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/23576dd018294e476c06e569b6b0f170d0558705", href)
+		})
+	})
+}

tests/integration/pull_test.go

@@ -30,6 +30,43 @@ func TestViewPulls(t *testing.T) {
 	assert.Equal(t, "Search pulls…", placeholder)
 }
 
+func TestPullViewConversation(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1")
+	resp := MakeRequest(t, req, http.StatusOK)
+
+	htmlDoc := NewHTMLParser(t, resp.Body)
+
+	t.Run("Commits", func(t *testing.T) {
+		commitLists := htmlDoc.Find(".timeline-item.commits-list")
+		assert.Equal(t, 4, commitLists.Length())
+
+		commits := commitLists.Find(".singular-commit")
+		assert.Equal(t, 10, commits.Length())
+
+		// First one has not been affected by a force push, therefore it's still part of the
+		// PR and should link to the PR-scoped review tab
+		firstCommit := commits.Eq(0)
+		firstCommitMessageHref, _ := firstCommit.Find("a.default-link").Attr("href")
+		firstCommitShaHref, _ := firstCommit.Find("a.sha.label").Attr("href")
+		assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847", firstCommitMessageHref)
+		assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847", firstCommitShaHref)
+
+		// The fifth commit has been overwritten by a force push.
+		// Attempting to view the old one in the review tab won't work:
+		req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/3e64625bd6eb5bcba69ac97de6c8f507402df861")
+		MakeRequest(t, req, http.StatusNotFound)
+
+		// Therefore, this commit should link to the non-PR commit view instead
+		fifthCommit := commits.Eq(4)
+		fifthCommitMessageHref, _ := fifthCommit.Find("a.default-link").Attr("href")
+		fifthCommitShaHref, _ := fifthCommit.Find("a.sha.label").Attr("href")
+		assert.Equal(t, "/user2/commitsonpr/commit/3e64625bd6eb5bcba69ac97de6c8f507402df861", fifthCommitMessageHref)
+		assert.Equal(t, "/user2/commitsonpr/commit/3e64625bd6eb5bcba69ac97de6c8f507402df861", fifthCommitShaHref)
+	})
+}
+
 func TestPullManuallyMergeWarning(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 

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) {

tools/lint-go-gopls.sh

@@ -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

web_src/css/repo.css

@@ -2468,8 +2468,21 @@ tbody.commit-list {
   display: flex;
 }
 
-#diff-file-boxes {
+#diff-content-container {
   flex: 1;
+}
+
+#diff-commit-header {
+  /* Counteract the `+2px` for width in `.segment` */
+  padding: 0 2px;
+}
+
+#diff-commit-header + h4,
+#diff-commit-header + #diff-file-boxes {
+  margin-top: 8px;
+}
+
+#diff-file-boxes {
   max-width: 100%;
   display: flex;
   flex-direction: column;