Merge pull request 'Fix misleading comparisons when comparing branches' (#2194) from algernon/forgejo:b/diff-compare/avoid-404s into forgejo-dependency

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2194
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

routers/api/v1/shared/runners.go

@@ -12,7 +12,7 @@ import (
 	"code.gitea.io/gitea/modules/util"
 )
 
-// RegistrationToken is response related to registeration token
+// RegistrationToken is a string used to register a runner with a server
 // swagger:response RegistrationToken
 type RegistrationToken struct {
 	Token string `json:"token"`

routers/web/repo/view.go

@@ -1035,7 +1035,43 @@ func renderCode(ctx *context.Context) {
 			}
 		}
 
-		ctx.Data["RecentlyPushedNewBranches"] = branches
+		// Filter out branches that have no relation to the default branch of
+		// the repository.
+		var filteredBranches []*git_model.Branch
+		for _, branch := range branches {
+			repo, err := branch.GetRepo(ctx)
+			if err != nil {
+				continue
+			}
+			gitRepo, err := git.OpenRepository(ctx, repo.RepoPath())
+			if err != nil {
+				continue
+			}
+			defer gitRepo.Close()
+			head, err := gitRepo.GetCommit(branch.CommitID)
+			if err != nil {
+				continue
+			}
+			defaultBranch, err := gitRepo.GetDefaultBranch()
+			if err != nil {
+				continue
+			}
+			defaultBranchHead, err := gitRepo.GetCommit(defaultBranch)
+			if err != nil {
+				continue
+			}
+
+			hasMergeBase, err := head.HasPreviousCommit(defaultBranchHead.ID)
+			if err != nil {
+				continue
+			}
+
+			if hasMergeBase {
+				filteredBranches = append(filteredBranches, branch)
+			}
+		}
+
+		ctx.Data["RecentlyPushedNewBranches"] = filteredBranches
 	}
 
 PostRecentBranchCheck:

templates/repo/diff/compare.tmpl

@@ -67,12 +67,12 @@
 					{{range .Branches}}
 						<div class="item {{if eq $.BaseBranch .}}selected{{end}}" data-url="{{$.RepoLink}}/compare/{{PathEscapeSegments .}}{{$.CompareSeparator}}{{if not $.PullRequestCtx.SameRepo}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{end}}{{PathEscapeSegments $.HeadBranch}}">{{$BaseCompareName}}:{{.}}</div>
 					{{end}}
-					{{if not .PullRequestCtx.SameRepo}}
+					{{if and (not .PullRequestCtx.SameRepo) ($.HeadRepo.AllowsPulls ctx)}}
 						{{range .HeadBranches}}
 							<div class="item" data-url="{{$.HeadRepo.Link}}/compare/{{PathEscapeSegments .}}{{$.CompareSeparator}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{PathEscapeSegments $.HeadBranch}}">{{$HeadCompareName}}:{{.}}</div>
 						{{end}}
 					{{end}}
-					{{if .OwnForkRepo}}
+					{{if and .OwnForkRepo (.OwnForkRepo.AllowsPulls ctx)}}
 						{{range .OwnForkRepoBranches}}
 							<div class="item" data-url="{{$.OwnForkRepo.Link}}/compare/{{PathEscapeSegments .}}{{$.CompareSeparator}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{PathEscapeSegments $.HeadBranch}}">{{$OwnForkCompareName}}:{{.}}</div>
 						{{end}}
@@ -87,17 +87,17 @@
 					{{range .Tags}}
 						<div class="item {{if eq $.BaseBranch .}}selected{{end}}" data-url="{{$.RepoLink}}/compare/{{PathEscapeSegments .}}{{$.CompareSeparator}}{{if not $.PullRequestCtx.SameRepo}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{end}}{{PathEscapeSegments $.HeadBranch}}">{{$BaseCompareName}}:{{.}}</div>
 					{{end}}
-					{{if not .PullRequestCtx.SameRepo}}
+					{{if and (not .PullRequestCtx.SameRepo) ($.HeadRepo.AllowsPulls ctx)}}
 						{{range .HeadTags}}
 							<div class="item" data-url="{{$.HeadRepo.Link}}/compare/{{PathEscapeSegments .}}{{$.CompareSeparator}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{PathEscapeSegments $.HeadBranch}}">{{$HeadCompareName}}:{{.}}</div>
 						{{end}}
 					{{end}}
-					{{if .OwnForkRepo}}
+					{{if and .OwnForkRepo (.OwnForkRepo.AllowsPulls ctx)}}
 						{{range .OwnForkRepoTags}}
 							<div class="item" data-url="{{$.OwnForkRepo.Link}}/compare/{{PathEscapeSegments .}}{{$.CompareSeparator}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{PathEscapeSegments $.HeadBranch}}">{{$OwnForkCompareName}}:{{.}}</div>
 						{{end}}
 					{{end}}
-					{{if .RootRepo}}
+					{{if and .RootRepo (.RootRepo.AllowsPulls ctx)}}
 						{{range .RootRepoTags}}
 							<div class="item" data-url="{{$.RootRepo.Link}}/compare/{{PathEscapeSegments .}}{{$.CompareSeparator}}{{PathEscape $.HeadUser.Name}}/{{PathEscape $.HeadRepo.Name}}:{{PathEscapeSegments $.HeadBranch}}">{{$RootRepoCompareName}}:{{.}}</div>
 						{{end}}

templates/swagger/v1_json.tmpl

@@ -24354,7 +24354,7 @@
       }
     },
     "RegistrationToken": {
-      "description": "RegistrationToken is response related to registeration token",
+      "description": "RegistrationToken is a string used to register a runner with a server",
       "headers": {
         "token": {
           "type": "string"

tests/integration/compare_test.go

@@ -1,4 +1,5 @@
 // Copyright 2021 The Gitea Authors. All rights reserved.
+// Copyright 2024 The Forgejo Authors c/o Codeberg e.V.. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package integration
@@ -6,9 +7,14 @@ package integration
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 	"strings"
 	"testing"
 
+	"code.gitea.io/gitea/models/db"
+	repo_model "code.gitea.io/gitea/models/repo"
+	unit_model "code.gitea.io/gitea/models/unit"
+	repo_service "code.gitea.io/gitea/services/repository"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
@@ -118,3 +124,61 @@ func TestCompareBranches(t *testing.T) {
 
 	inspectCompare(t, htmlDoc, diffCount, diffChanges)
 }
+
+func TestCompareWithPRsDisabled(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		session := loginUser(t, "user1")
+		testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
+		testCreateBranch(t, session, "user1", "repo1", "branch/master", "recent-push", http.StatusSeeOther)
+		testEditFile(t, session, "user1", "repo1", "recent-push", "README.md", "Hello recently!\n")
+
+		repo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, "user1", "repo1")
+		assert.NoError(t, err)
+
+		defer func() {
+			// Reenable PRs on the repo
+			err := repo_service.UpdateRepositoryUnits(db.DefaultContext, repo,
+				[]repo_model.RepoUnit{{
+					RepoID: repo.ID,
+					Type:   unit_model.TypePullRequests,
+				}},
+				nil)
+			assert.NoError(t, err)
+		}()
+
+		// Disable PRs on the repo
+		err = repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, nil,
+			[]unit_model.Type{unit_model.TypePullRequests})
+		assert.NoError(t, err)
+
+		t.Run("branch view doesn't offer creating PRs", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			req := NewRequest(t, "GET", "/user1/repo1/branches")
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			htmlDoc.AssertElement(t, "a[href='/user1/repo1/compare/master...recent-push']", false)
+		})
+
+		t.Run("compare doesn't offer local branches", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			req := NewRequest(t, "GET", "/user2/repo1/compare/master...user1/repo1:recent-push")
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			branches := htmlDoc.Find(".choose.branch .menu .reference-list-menu.base-branch-list .item, .choose.branch .menu .reference-list-menu.base-tag-list .item")
+
+			expectedPrefix := "user2:"
+			for i := 0; i < len(branches.Nodes); i++ {
+				assert.True(t, strings.HasPrefix(branches.Eq(i).Text(), expectedPrefix))
+			}
+		})
+
+		t.Run("comparing against a disabled-PR repo is 404", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			req := NewRequest(t, "GET", "/user1/repo1/compare/master...recent-push")
+			session.MakeRequest(t, req, http.StatusNotFound)
+		})
+	})
+}

tests/integration/pull_create_test.go

@@ -16,8 +16,13 @@ import (
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	unit_model "code.gitea.io/gitea/models/unit"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/graceful"
 	"code.gitea.io/gitea/modules/test"
 	repo_service "code.gitea.io/gitea/services/repository"
+	files_service "code.gitea.io/gitea/services/repository/files"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
@@ -331,5 +336,58 @@ func TestRecentlyPushed(t *testing.T) {
 			// PR link compares against the correct rep, and qualified branch name
 			assert.Equal(t, "/user2/repo1/compare/master...user1/repo1:recent-push", forkPRLink)
 		})
+
+		t.Run("unrelated branches are not shown", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			adminUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{IsAdmin: true})
+
+			// Create a new branch with no relation to the default branch.
+			// 1. Create a new Tree object
+			cmd := git.NewCommand(db.DefaultContext, "write-tree")
+			treeID, _, gitErr := cmd.RunStdString(&git.RunOpts{Dir: repo.RepoPath()})
+			assert.NoError(t, gitErr)
+			treeID = strings.TrimSpace(treeID)
+			// 2. Create a new (empty) commit
+			cmd = git.NewCommand(db.DefaultContext, "commit-tree", "-m", "Initial orphan commit").AddDynamicArguments(treeID)
+			commitID, _, gitErr := cmd.RunStdString(&git.RunOpts{Dir: repo.RepoPath()})
+			assert.NoError(t, gitErr)
+			commitID = strings.TrimSpace(commitID)
+			// 3. Create a new ref pointing to the orphaned commit
+			cmd = git.NewCommand(db.DefaultContext, "update-ref", "refs/heads/orphan1").AddDynamicArguments(commitID)
+			_, _, gitErr = cmd.RunStdString(&git.RunOpts{Dir: repo.RepoPath()})
+			assert.NoError(t, gitErr)
+			// 4. Sync the git repo to the database
+			syncErr := repo_service.AddAllRepoBranchesToSyncQueue(graceful.GetManager().ShutdownContext(), adminUser.ID)
+			assert.NoError(t, syncErr)
+			// 5. Add a fresh commit, so that FindRecentlyPushedBranches has
+			// something to find.
+			owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user1"})
+			changeResp, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, owner,
+				&files_service.ChangeRepoFilesOptions{
+					Files: []*files_service.ChangeRepoFile{
+						{
+							Operation:     "create",
+							TreePath:      "README.md",
+							ContentReader: strings.NewReader("a readme file"),
+						},
+					},
+					Message:   "Add README.md",
+					OldBranch: "orphan1",
+					NewBranch: "orphan1",
+				})
+			assert.NoError(t, err)
+			assert.NotEmpty(t, changeResp)
+
+			// Check that we only have 1 message on the main repo, the orphaned
+			// one is not shown.
+			req := NewRequest(t, "GET", "/user1/repo1")
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			htmlDoc := NewHTMLParser(t, resp.Body)
+
+			htmlDoc.AssertElement(t, ".ui.message", true)
+			link, _ := htmlDoc.Find(".ui.message a[href*='/src/branch/']").Attr("href")
+			assert.Equal(t, "/user1/repo1/src/branch/recent-push", link)
+		})
 	})
 }