Add rebase push display wrong comments bug (#35560) (#35580)

Backport #35560 by @lunny

Fix #35518

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/fixtures/branch.yml

@@ -213,3 +213,15 @@
   is_deleted: false
   deleted_by_id: 0
   deleted_unix: 0
+
+-
+  id: 26
+  repo_id: 10
+  name: 'feature/1'
+  commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d'
+  commit_message: 'Initial commit'
+  commit_time: 1489950479
+  pusher_id: 2
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0

options/locale/locale_en-US.ini

@@ -2433,7 +2433,9 @@ settings.event_workflow_job_desc = Gitea Actions Workflow job queued, waiting, i
 settings.event_package = Package
 settings.event_package_desc = Package created or deleted in a repository.
 settings.branch_filter = Branch filter
-settings.branch_filter_desc = Branch whitelist for push, branch creation and branch deletion events, specified as glob pattern. If empty or <code>*</code>, events for all branches are reported. See <a href="%[1]s">%[2]s</a> documentation for syntax. Examples: <code>master</code>, <code>{master,release*}</code>.
+settings.branch_filter_desc_1 = Branch (and ref name) allowlist for push, branch creation and branch deletion events, specified as glob pattern. If empty or <code>*</code>, events for all branches and tags are reported.
+settings.branch_filter_desc_2 = Use <code>refs/heads/</code> or <code>refs/tags/</code> prefix to match full ref names.
+settings.branch_filter_desc_doc = See <a href="%[1]s">%[2]s</a> documentation for syntax.
 settings.authorization_header = Authorization Header
 settings.authorization_header_desc = Will be included as authorization header for requests when present. Examples: %s.
 settings.active = Active

routers/web/repo/pull.go

@@ -1583,7 +1583,16 @@ func UpdatePullRequestTarget(ctx *context.Context) {
 	}
 
 	if err := pull_service.ChangeTargetBranch(ctx, pr, ctx.Doer, targetBranch); err != nil {
-		if issues_model.IsErrPullRequestAlreadyExists(err) {
+		switch {
+		case git_model.IsErrBranchNotExist(err):
+			errorMessage := ctx.Tr("form.target_branch_not_exist")
+
+			ctx.Flash.Error(errorMessage)
+			ctx.JSON(http.StatusBadRequest, map[string]any{
+				"error":      err.Error(),
+				"user_error": errorMessage,
+			})
+		case issues_model.IsErrPullRequestAlreadyExists(err):
 			err := err.(issues_model.ErrPullRequestAlreadyExists)
 
 			RepoRelPath := ctx.Repo.Owner.Name + "/" + ctx.Repo.Repository.Name
@@ -1594,7 +1603,7 @@ func UpdatePullRequestTarget(ctx *context.Context) {
 				"error":      err.Error(),
 				"user_error": errorMessage,
 			})
-		} else if issues_model.IsErrIssueIsClosed(err) {
+		case issues_model.IsErrIssueIsClosed(err):
 			errorMessage := ctx.Tr("repo.pulls.is_closed")
 
 			ctx.Flash.Error(errorMessage)
@@ -1602,7 +1611,7 @@ func UpdatePullRequestTarget(ctx *context.Context) {
 				"error":      err.Error(),
 				"user_error": errorMessage,
 			})
-		} else if pull_service.IsErrPullRequestHasMerged(err) {
+		case pull_service.IsErrPullRequestHasMerged(err):
 			errorMessage := ctx.Tr("repo.pulls.has_merged")
 
 			ctx.Flash.Error(errorMessage)
@@ -1610,7 +1619,7 @@ func UpdatePullRequestTarget(ctx *context.Context) {
 				"error":      err.Error(),
 				"user_error": errorMessage,
 			})
-		} else if git_model.IsErrBranchesEqual(err) {
+		case git_model.IsErrBranchesEqual(err):
 			errorMessage := ctx.Tr("repo.pulls.nothing_to_compare")
 
 			ctx.Flash.Error(errorMessage)
@@ -1618,7 +1627,7 @@ func UpdatePullRequestTarget(ctx *context.Context) {
 				"error":      err.Error(),
 				"user_error": errorMessage,
 			})
-		} else {
+		default:
 			ctx.ServerError("UpdatePullRequestTarget", err)
 		}
 		return

services/pull/comment.go

@@ -63,6 +63,7 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss
 	var data issues_model.PushActionContent
 	if opts.IsForcePush {
 		data.CommitIDs = []string{oldCommitID, newCommitID}
+		data.IsForcePush = true
 	} else {
 		data.CommitIDs, err = getCommitIDsFromRepo(ctx, pr.BaseRepo, oldCommitID, newCommitID, pr.BaseBranch)
 		if err != nil {

services/pull/pull.go

@@ -244,6 +244,17 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer
 		}
 	}
 
+	exist, err := git_model.IsBranchExist(ctx, pr.BaseRepoID, targetBranch)
+	if err != nil {
+		return err
+	}
+	if !exist {
+		return git_model.ErrBranchNotExist{
+			RepoID:     pr.BaseRepoID,
+			BranchName: targetBranch,
+		}
+	}
+
 	// Check if branches are equal
 	branchesEqual, err := IsHeadEqualWithBranch(ctx, pr, targetBranch)
 	if err != nil {

services/webhook/webhook.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"strings"
 
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
@@ -46,21 +45,25 @@ func IsValidHookTaskType(name string) bool {
 // hookQueue is a global queue of web hooks
 var hookQueue *queue.WorkerPoolQueue[int64]
 
-// getPayloadBranch returns branch for hook event, if applicable.
-func getPayloadBranch(p api.Payloader) string {
+// getPayloadRef returns the full ref name for hook event, if applicable.
+func getPayloadRef(p api.Payloader) git.RefName {
 	switch pp := p.(type) {
 	case *api.CreatePayload:
-		if pp.RefType == "branch" {
-			return pp.Ref
+		switch pp.RefType {
+		case "branch":
+			return git.RefNameFromBranch(pp.Ref)
+		case "tag":
+			return git.RefNameFromTag(pp.Ref)
 		}
 	case *api.DeletePayload:
-		if pp.RefType == "branch" {
-			return pp.Ref
+		switch pp.RefType {
+		case "branch":
+			return git.RefNameFromBranch(pp.Ref)
+		case "tag":
+			return git.RefNameFromTag(pp.Ref)
 		}
 	case *api.PushPayload:
-		if strings.HasPrefix(pp.Ref, git.BranchPrefix) {
-			return pp.Ref[len(git.BranchPrefix):]
-		}
+		return git.RefName(pp.Ref)
 	}
 	return ""
 }
@@ -108,19 +111,22 @@ func enqueueHookTask(taskID int64) error {
 	return nil
 }
 
-func checkBranch(w *webhook_model.Webhook, branch string) bool {
-	if w.BranchFilter == "" || w.BranchFilter == "*" {
+func checkBranchFilter(branchFilter string, ref git.RefName) bool {
+	if branchFilter == "" || branchFilter == "*" || branchFilter == "**" {
 		return true
 	}
 
-	g, err := glob.Compile(w.BranchFilter)
+	g, err := glob.Compile(branchFilter)
 	if err != nil {
 		// should not really happen as BranchFilter is validated
-		log.Error("CheckBranch failed: %s", err)
+		log.Debug("checkBranchFilter failed to compile filer %q, err: %s", branchFilter, err)
 		return false
 	}
 
-	return g.Match(branch)
+	if ref.IsBranch() && g.Match(ref.BranchName()) {
+		return true
+	}
+	return g.Match(ref.String())
 }
 
 // PrepareWebhook creates a hook task and enqueues it for processing.
@@ -144,11 +150,10 @@ func PrepareWebhook(ctx context.Context, w *webhook_model.Webhook, event webhook
 		return nil
 	}
 
-	// If payload has no associated branch (e.g. it's a new tag, issue, etc.),
-	// branch filter has no effect.
-	if branch := getPayloadBranch(p); branch != "" {
-		if !checkBranch(w, branch) {
-			log.Info("Branch %q doesn't match branch filter %q, skipping", branch, w.BranchFilter)
+	// If payload has no associated branch (e.g. it's a new tag, issue, etc.), branch filter has no effect.
+	if ref := getPayloadRef(p); ref != "" {
+		// Check the payload's git ref against the webhook's branch filter.
+		if !checkBranchFilter(w.BranchFilter, ref) {
 			return nil
 		}
 	}

services/webhook/webhook_test.go

@@ -10,6 +10,7 @@ import (
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	webhook_model "code.gitea.io/gitea/models/webhook"
+	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/test"
@@ -90,3 +91,30 @@ func TestWebhookUserMail(t *testing.T) {
 	assert.Equal(t, user.GetPlaceholderEmail(), convert.ToUser(t.Context(), user, nil).Email)
 	assert.Equal(t, user.Email, convert.ToUser(t.Context(), user, user).Email)
 }
+
+func TestCheckBranchFilter(t *testing.T) {
+	cases := []struct {
+		filter string
+		ref    git.RefName
+		match  bool
+	}{
+		{"", "any-ref", true},
+		{"*", "any-ref", true},
+		{"**", "any-ref", true},
+
+		{"main", git.RefNameFromBranch("main"), true},
+		{"main", git.RefNameFromTag("main"), false},
+
+		{"feature/*", git.RefNameFromBranch("feature"), false},
+		{"feature/*", git.RefNameFromBranch("feature/foo"), true},
+		{"feature/*", git.RefNameFromTag("feature/foo"), false},
+
+		{"{refs/heads/feature/*,refs/tags/release/*}", git.RefNameFromBranch("feature/foo"), true},
+		{"{refs/heads/feature/*,refs/tags/release/*}", git.RefNameFromBranch("main"), false},
+		{"{refs/heads/feature/*,refs/tags/release/*}", git.RefNameFromTag("release/bar"), true},
+		{"{refs/heads/feature/*,refs/tags/release/*}", git.RefNameFromTag("dev"), false},
+	}
+	for _, v := range cases {
+		assert.Equal(t, v.match, checkBranchFilter(v.filter, v.ref), "filter: %q ref: %q", v.filter, v.ref)
+	}
+}

templates/repo/settings/webhook/settings.tmpl

@@ -44,7 +44,16 @@
 <div class="field">
 	<label>{{ctx.Locale.Tr "repo.settings.branch_filter"}}</label>
 	<input name="branch_filter" type="text" value="{{or .Webhook.BranchFilter "*"}}">
-	<span class="help">{{ctx.Locale.Tr "repo.settings.branch_filter_desc" "https://pkg.go.dev/github.com/gobwas/glob#Compile" "github.com/gobwas/glob"}}</span>
+	<span class="help">
+		{{ctx.Locale.Tr "repo.settings.branch_filter_desc_1"}}
+		{{ctx.Locale.Tr "repo.settings.branch_filter_desc_2"}}
+		{{ctx.Locale.Tr "repo.settings.branch_filter_desc_doc" "https://pkg.go.dev/github.com/gobwas/glob#Compile" "github.com/gobwas/glob"}}
+		<ul>
+			<li><code>main</code></li>
+			<li><code>{main,feature/*}</code></li>
+			<li><code>{refs/heads/feature/*,refs/tags/release/*}</code></li>
+		</ul>
+	</span>
 </div>
 
 <div class="field">

tests/integration/git_helper_for_declarative_test.go

@@ -23,6 +23,7 @@ import (
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func withKeyFile(t *testing.T, keyname string, callback func(string)) {
@@ -160,20 +161,27 @@ func doGitPushTestRepositoryFail(dstPath string, args ...string) func(*testing.T
 	}
 }
 
-func doGitAddSomeCommits(dstPath, branch string) func(*testing.T) {
-	return func(t *testing.T) {
-		doGitCheckoutBranch(dstPath, branch)(t)
+type localGitAddCommitOptions struct {
+	LocalRepoPath   string
+	CheckoutBranch  string
+	TreeFilePath    string
+	TreeFileContent string
+}
 
-		assert.NoError(t, os.WriteFile(filepath.Join(dstPath, fmt.Sprintf("file-%s.txt", branch)), []byte("file "+branch), 0o644))
-		assert.NoError(t, git.AddChanges(t.Context(), dstPath, true))
+func doGitCheckoutWriteFileCommit(opts localGitAddCommitOptions) func(*testing.T) {
+	return func(t *testing.T) {
+		doGitCheckoutBranch(opts.LocalRepoPath, opts.CheckoutBranch)(t)
+		localFilePath := filepath.Join(opts.LocalRepoPath, opts.TreeFilePath)
+		require.NoError(t, os.WriteFile(localFilePath, []byte(opts.TreeFileContent), 0o644))
+		require.NoError(t, git.AddChanges(t.Context(), opts.LocalRepoPath, true))
 		signature := git.Signature{
 			Email: "test@test.test",
 			Name:  "test",
 		}
-		assert.NoError(t, git.CommitChanges(t.Context(), dstPath, git.CommitChangesOptions{
+		require.NoError(t, git.CommitChanges(t.Context(), opts.LocalRepoPath, git.CommitChangesOptions{
 			Committer: &signature,
 			Author:    &signature,
-			Message:   "update " + branch,
+			Message:   fmt.Sprintf("update %s @ %s", opts.TreeFilePath, opts.CheckoutBranch),
 		}))
 	}
 }

tests/integration/git_push_test.go

@@ -58,8 +58,12 @@ func testGitPush(t *testing.T, u *url.URL) {
 			for i := range 5 {
 				branchName := fmt.Sprintf("branch-%d", i)
 				pushed = append(pushed, branchName)
-
-				doGitAddSomeCommits(gitPath, branchName)(t)
+				doGitCheckoutWriteFileCommit(localGitAddCommitOptions{
+					LocalRepoPath:   gitPath,
+					CheckoutBranch:  branchName,
+					TreeFilePath:    fmt.Sprintf("file-%s.txt", branchName),
+					TreeFileContent: "file " + branchName,
+				})(t)
 			}
 
 			for i := 5; i < 10; i++ {

tests/integration/pull_comment_test.go

@@ -0,0 +1,109 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"fmt"
+	"net/http"
+	"net/url"
+	"os"
+	"testing"
+	"time"
+
+	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/unittest"
+	issues_service "code.gitea.io/gitea/services/issue"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func testWaitForPullRequestStatus(t *testing.T, prIssue *issues_model.Issue, expectedStatus issues_model.PullRequestStatus) (retIssue *issues_model.Issue) {
+	require.Eventually(t, func() bool {
+		prIssueCond := *prIssue
+		retIssue = unittest.AssertExistsAndLoadBean(t, &prIssueCond)
+		require.NoError(t, retIssue.LoadPullRequest(t.Context()))
+		return retIssue.PullRequest.Status == expectedStatus
+	}, 5*time.Second, 20*time.Millisecond)
+	return retIssue
+}
+
+func testPullCommentRebase(t *testing.T, u *url.URL, session *TestSession) {
+	testPRTitle := "Test PR for rebase comment"
+	// make a change on forked branch
+	testEditFile(t, session, "user1", "repo1", "test-branch/rebase", "README.md", "Hello, World (Edited)\n")
+	testPullCreate(t, session, "user1", "repo1", false, "test-branch/rebase", "test-branch/rebase", testPRTitle)
+	// create a conflict on base repo branch
+	testEditFile(t, session, "user2", "repo1", "test-branch/rebase", "README.md", "Hello, World (Edited Conflicted)\n")
+
+	// Now the pull request status should be conflicted
+	testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusConflict)
+
+	dstPath := t.TempDir()
+	u.Path = "/user2/repo1.git"
+	doGitClone(dstPath, u)(t)
+	doGitCheckoutBranch(dstPath, "test-branch/rebase")(t)
+	doGitCreateBranch(dstPath, "local-branch/rebase")(t)
+	content, _ := os.ReadFile(dstPath + "/README.md")
+	require.Equal(t, "Hello, World (Edited Conflicted)\n", string(content))
+
+	doGitCheckoutWriteFileCommit(localGitAddCommitOptions{
+		LocalRepoPath:   dstPath,
+		CheckoutBranch:  "local-branch/rebase",
+		TreeFilePath:    "README.md",
+		TreeFileContent: "Hello, World (Edited Conflict Resolved)\n",
+	})(t)
+
+	// do force push
+	u.Path = "/user1/repo1.git"
+	u.User = url.UserPassword("user1", userPassword)
+	doGitAddRemote(dstPath, "base-repo", u)(t)
+	doGitPushTestRepositoryFail(dstPath, "base-repo", "local-branch/rebase:test-branch/rebase")(t)
+	doGitPushTestRepository(dstPath, "--force", "base-repo", "local-branch/rebase:test-branch/rebase")(t)
+
+	// reload the pr
+	prIssue := testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusMergeable)
+	comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{
+		IssueID: prIssue.ID,
+		Type:    issues_model.CommentTypeUndefined, // get all comments type
+	})
+	require.NoError(t, err)
+	lastComment := comments[len(comments)-1]
+	assert.NoError(t, issues_service.LoadCommentPushCommits(t.Context(), lastComment))
+	assert.True(t, lastComment.IsForcePush)
+}
+
+func testPullCommentRetarget(t *testing.T, u *url.URL, session *TestSession) {
+	testPRTitle := "Test PR for retarget comment"
+	// keep a non-conflict branch
+	testCreateBranch(t, session, "user2", "repo1", "branch/test-branch/retarget", "test-branch/retarget-no-conflict", http.StatusSeeOther)
+	// make a change on forked branch
+	testEditFile(t, session, "user1", "repo1", "test-branch/retarget", "README.md", "Hello, World (Edited)\n")
+	testPullCreate(t, session, "user1", "repo1", false, "test-branch/retarget", "test-branch/retarget", testPRTitle)
+	// create a conflict line on user2/repo1 README.md
+	testEditFile(t, session, "user2", "repo1", "test-branch/retarget", "README.md", "Hello, World (Edited Conflicted)\n")
+
+	// Now the pull request status should be conflicted
+	prIssue := testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusConflict)
+
+	// do retarget
+	req := NewRequestWithValues(t, "POST", fmt.Sprintf("/user2/repo1/pull/%d/target_branch", prIssue.PullRequest.Index), map[string]string{
+		"_csrf":         GetUserCSRFToken(t, session),
+		"target_branch": "test-branch/retarget-no-conflict",
+	})
+	session.MakeRequest(t, req, http.StatusOK)
+	testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusMergeable)
+}
+
+func TestPullComment(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		session := loginUser(t, "user1")
+		testCreateBranch(t, session, "user2", "repo1", "branch/master", "test-branch/rebase", http.StatusSeeOther)
+		testCreateBranch(t, session, "user2", "repo1", "branch/master", "test-branch/retarget", http.StatusSeeOther)
+		testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
+
+		t.Run("RebaseComment", func(t *testing.T) { testPullCommentRebase(t, u, session) })
+		t.Run("RetargetComment", func(t *testing.T) { testPullCommentRetarget(t, u, session) })
+	})
+}

tests/integration/pull_create_test.go

@@ -260,14 +260,16 @@ func TestCreateAgitPullWithReadPermission(t *testing.T) {
 		u.Path = "user2/repo1.git"
 		u.User = url.UserPassword("user4", userPassword)
 
-		t.Run("Clone", doGitClone(dstPath, u))
+		doGitClone(dstPath, u)(t)
+		doGitCheckoutWriteFileCommit(localGitAddCommitOptions{
+			LocalRepoPath:   dstPath,
+			CheckoutBranch:  "master",
+			TreeFilePath:    "new-file-for-agit.txt",
+			TreeFileContent: "temp content",
+		})(t)
 
-		t.Run("add commit", doGitAddSomeCommits(dstPath, "master"))
-
-		t.Run("do agit pull create", func(t *testing.T) {
-			err := gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o").AddDynamicArguments("topic="+"test-topic").Run(t.Context(), &gitcmd.RunOpts{Dir: dstPath})
-			assert.NoError(t, err)
-		})
+		err := gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o").AddDynamicArguments("topic="+"test-topic").Run(t.Context(), &gitcmd.RunOpts{Dir: dstPath})
+		assert.NoError(t, err)
 	})
 }
 

web_src/css/base.css

@@ -101,6 +101,13 @@ samp,
   font-size: 0.95em; /* compensate for monospace fonts being usually slightly larger */
 }
 
+code {
+  padding: 1px 4px;
+  border-radius: var(--border-radius);
+  background-color: var(--color-label-bg);
+  color: var(--color-label-text);
+}
+
 b,
 strong,
 h1,
@@ -177,6 +184,11 @@ table {
   border-collapse: collapse;
 }
 
+ul {
+  margin: 0.5em 0;
+  padding: 0 0 0 1.5em;
+}
+
 button {
   cursor: pointer;
 }
@@ -603,11 +615,6 @@ img.ui.avatar,
   text-align: center;
 }
 
-.ui .message > ul {
-  margin: 0;
-  padding: 0 1em;
-}
-
 .ui .header > i + .content {
   padding-left: 0.75rem;
   vertical-align: middle;

web_src/css/form.css

@@ -218,11 +218,16 @@ textarea:focus,
 
 .form .help {
   color: var(--color-secondary-dark-5);
+  margin-top: 0.25em;
   padding-bottom: 0.6em;
   display: inline-block;
   text-wrap: balance;
 }
 
+.form .help code {
+  color: var(--color-text-light-1);
+}
+
 .m-captcha-style {
   width: 100%;
   height: 5em;

web_src/css/repo/issue-list.css

@@ -84,10 +84,3 @@
   margin-right: 8px;
   text-align: left;
 }
-
-.label-filter-exclude-info code {
-  border: 1px solid var(--color-secondary);
-  border-radius: var(--border-radius);
-  padding: 1px 2px;
-  font-size: 11px;
-}