From 3b468a72c102ccc5feee8d8416af1c791cfad16c Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Sun, 14 Sep 2025 12:40:28 -0700 Subject: [PATCH 01/14] Honor delete branch on merge repo setting when using merge API --- routers/api/v1/repo/pull.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index ff6ddbce2d4bc..05a062db3c199 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -19,6 +19,7 @@ import ( pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" + unit_model "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/git" @@ -1035,8 +1036,17 @@ func MergePullRequest(ctx *context.APIContext) { } log.Trace("Pull request merged: %d", pr.ID) + prUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit_model.TypePullRequests) + if err != nil { + ctx.APIErrorInternal(err) + return + } + // for agit flow, we should not delete the agit reference after merge - if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub { + shouldDeleteBranch := (prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge || form.DeleteBranchAfterMerge) && + pr.Flow == issues_model.PullRequestFlowGithub + + if shouldDeleteBranch { // check permission even it has been checked in repo_service.DeleteBranch so that we don't need to // do RetargetChildrenOnMerge if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil { From 61c4e69e5f248eda2880151576ce35cd3199a9ac Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Sun, 14 Sep 2025 12:49:30 -0700 Subject: [PATCH 02/14] PR linter feedback --- routers/api/v1/repo/pull.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 05a062db3c199..084171789f9b7 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -19,7 +19,6 @@ import ( pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" - unit_model "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/git" @@ -1036,7 +1035,7 @@ func MergePullRequest(ctx *context.APIContext) { } log.Trace("Pull request merged: %d", pr.ID) - prUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit_model.TypePullRequests) + prUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit.TypePullRequests) if err != nil { ctx.APIErrorInternal(err) return From 01dce976f0055c3eda398193decec4eddf630cd7 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Sun, 19 Oct 2025 11:51:02 -0700 Subject: [PATCH 03/14] Add tests --- tests/integration/api_pull_test.go | 117 +++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 8376fd2717d3b..f4e8e78f80869 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -17,6 +17,8 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + 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/setting" @@ -26,10 +28,12 @@ import ( "code.gitea.io/gitea/services/gitdiff" issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" + 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" + "github.com/stretchr/testify/require" ) func TestAPIViewPulls(t *testing.T) { @@ -186,6 +190,76 @@ func TestAPIMergePullWIP(t *testing.T) { MakeRequest(t, req, http.StatusMethodNotAllowed) } +func TestAPIMergePull(t *testing.T) { + doCheckBranchExists := func(user *user_model.User, token, branchName string, repo *repo_model.Repository, status int) { + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", user.Name, repo.Name, branchName)).AddTokenAuth(token) + MakeRequest(t, req, status) + } + + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + session := loginUser(t, owner.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + t.Run("Normal", func(t *testing.T) { + newBranch := "test-pull-1" + prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ + Do: "merge", + }).AddTokenAuth(token) + + MakeRequest(t, req, http.StatusOK) + doCheckBranchExists(owner, token, newBranch, repo, http.StatusOK) + // make sure we cannot perform a merge on the same PR + MakeRequest(t, req, http.StatusUnprocessableEntity) + doCheckBranchExists(owner, token, newBranch, repo, http.StatusOK) + }) + + t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) { + newBranch := "test-pull-2" + prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ + Do: "merge", + DeleteBranchAfterMerge: true, + }).AddTokenAuth(token) + + MakeRequest(t, req, http.StatusOK) + doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound) + }) + + t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) { + newBranch := "test-pull-3" + prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + + // set the default branch after merge setting at the repo level + prUnit, err := repo.GetUnit(t.Context(), unit.TypePullRequests) + require.NoError(t, err) + + prConfig := prUnit.PullRequestsConfig() + prConfig.DefaultDeleteBranchAfterMerge = true + + var units []repo_model.RepoUnit + units = append(units, repo_model.RepoUnit{ + RepoID: repo.ID, + Type: unit_model.TypePullRequests, + Config: prConfig, + }) + require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil)) + + // perform merge + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ + Do: "merge", + }).AddTokenAuth(token) + + MakeRequest(t, req, http.StatusOK) + doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound) + }) + }) +} + func TestAPICreatePullSuccess(t *testing.T) { defer tests.PrepareTestEnv(t)() repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) @@ -520,3 +594,46 @@ func TestAPIViewPullFilesWithHeadRepoDeleted(t *testing.T) { })(t) }) } + +func creatPullRequestWithCommit(t *testing.T, user *user_model.User, token, newBranch string, repo *repo_model.Repository) *api.PullRequest { + // create a new branch with a commit + filesResp, err := files_service.ChangeRepoFiles(t.Context(), repo, user, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: strings.ReplaceAll(newBranch, " ", "_"), + ContentReader: strings.NewReader("This is a test."), + }, + }, + Message: "Add test file using branch name as its name", + OldBranch: repo.DefaultBranch, + NewBranch: newBranch, + Author: &files_service.IdentityOptions{ + GitUserName: user.Name, + GitUserEmail: user.Email, + }, + Committer: &files_service.IdentityOptions{ + GitUserName: user.Name, + GitUserEmail: user.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + + require.NoError(t, err) + require.NotEmpty(t, filesResp) + + // create a corresponding PR + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", user.Name, repo.Name), &api.CreatePullRequestOption{ + Head: newBranch, + Base: repo.DefaultBranch, + Title: "Add a test file", + }).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusCreated) + pull := new(api.PullRequest) + DecodeJSON(t, resp, pull) + + return pull +} From 6d5d154b4982ce07a2a949381d542dbc501eb5b7 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Sun, 19 Oct 2025 12:05:26 -0700 Subject: [PATCH 04/14] CI feedback --- tests/integration/api_pull_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index f4e8e78f80869..ae22325a3df6d 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -17,7 +17,6 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" unit_model "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -235,7 +234,7 @@ func TestAPIMergePull(t *testing.T) { prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) // set the default branch after merge setting at the repo level - prUnit, err := repo.GetUnit(t.Context(), unit.TypePullRequests) + prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) require.NoError(t, err) prConfig := prUnit.PullRequestsConfig() From 20b5d9d4509243b6c54b63de2179acfff439c70a Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Sun, 19 Oct 2025 13:09:51 -0700 Subject: [PATCH 05/14] Give form field higher precedence then repo setting equivalent --- routers/api/v1/repo/pull.go | 13 +++++++++--- routers/web/repo/pull.go | 5 +++-- services/forms/repo_form.go | 2 +- tests/integration/api_pull_test.go | 33 +++++++++++++++++++++++++++++- 4 files changed, 46 insertions(+), 7 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 084171789f9b7..3aa0a444605a8 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -25,6 +25,7 @@ import ( "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" @@ -990,7 +991,7 @@ func MergePullRequest(ctx *context.APIContext) { } if form.MergeWhenChecksSucceed { - scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false)) if err != nil { if pull_model.IsErrAlreadyScheduledToAutoMerge(err) { ctx.APIError(http.StatusConflict, err) @@ -1042,8 +1043,14 @@ func MergePullRequest(ctx *context.APIContext) { } // for agit flow, we should not delete the agit reference after merge - shouldDeleteBranch := (prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge || form.DeleteBranchAfterMerge) && - pr.Flow == issues_model.PullRequestFlowGithub + shouldDeleteBranch := pr.Flow == issues_model.PullRequestFlowGithub + if form.DeleteBranchAfterMerge != nil { + // if the form field is defined, it takes precedence over the repo setting equivalent + shouldDeleteBranch = shouldDeleteBranch && *form.DeleteBranchAfterMerge + } else { + // otherwise, we look at the repo setting to make the determination + shouldDeleteBranch = shouldDeleteBranch && prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge + } if shouldDeleteBranch { // check permission even it has been checked in repo_service.DeleteBranch so that we don't need to diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 83aaf75363fbd..782010430396e 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -30,6 +30,7 @@ import ( "code.gitea.io/gitea/modules/graceful" issue_template "code.gitea.io/gitea/modules/issue/template" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/templates" "code.gitea.io/gitea/modules/util" @@ -1099,7 +1100,7 @@ func MergePullRequest(ctx *context.Context) { // delete all scheduled auto merges _ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) // schedule auto merge - scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false)) if err != nil { ctx.ServerError("ScheduleAutoMerge", err) return @@ -1185,7 +1186,7 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) - if !form.DeleteBranchAfterMerge { + if !optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) { ctx.JSONRedirect(issue.Link()) return } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index cb267f891ccb7..67f24c4cbe0f7 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -540,7 +540,7 @@ type MergePullRequestForm struct { HeadCommitID string `json:"head_commit_id,omitempty"` ForceMerge bool `json:"force_merge,omitempty"` MergeWhenChecksSucceed bool `json:"merge_when_checks_succeed,omitempty"` - DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"` + DeleteBranchAfterMerge *bool `json:"delete_branch_after_merge,omitempty"` } // Validate validates the fields diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index ae22325a3df6d..67c32c99d751f 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -219,10 +219,11 @@ func TestAPIMergePull(t *testing.T) { t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) { newBranch := "test-pull-2" prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + deleteBranch := true req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ Do: "merge", - DeleteBranchAfterMerge: true, + DeleteBranchAfterMerge: &deleteBranch, }).AddTokenAuth(token) MakeRequest(t, req, http.StatusOK) @@ -256,6 +257,36 @@ func TestAPIMergePull(t *testing.T) { MakeRequest(t, req, http.StatusOK) doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound) }) + + t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) { + newBranch := "test-pull-4" + prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + + // make sure the default branch after merge setting is unset at the repo level + prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) + require.NoError(t, err) + + prConfig := prUnit.PullRequestsConfig() + prConfig.DefaultDeleteBranchAfterMerge = false + + var units []repo_model.RepoUnit + units = append(units, repo_model.RepoUnit{ + RepoID: repo.ID, + Type: unit_model.TypePullRequests, + Config: prConfig, + }) + require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil)) + + // perform merge + deleteBranch := true + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ + Do: "merge", + DeleteBranchAfterMerge: &deleteBranch, + }).AddTokenAuth(token) + + MakeRequest(t, req, http.StatusOK) + doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound) + }) }) } From da4bb406a873b6da859a52de0dda831708e63d89 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Mon, 20 Oct 2025 19:53:30 -0700 Subject: [PATCH 06/14] PR feedback + some touchups --- tests/integration/api_pull_test.go | 43 +++++++++++++----------------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 67c32c99d751f..c70f924dbfe83 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -190,7 +190,7 @@ func TestAPIMergePullWIP(t *testing.T) { } func TestAPIMergePull(t *testing.T) { - doCheckBranchExists := func(user *user_model.User, token, branchName string, repo *repo_model.Repository, status int) { + doCheckBranchExists := func(t *testing.T, user *user_model.User, token, branchName string, repo *repo_model.Repository, status int) { req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", user.Name, repo.Name, branchName)).AddTokenAuth(token) MakeRequest(t, req, status) } @@ -203,36 +203,36 @@ func TestAPIMergePull(t *testing.T) { t.Run("Normal", func(t *testing.T) { newBranch := "test-pull-1" - prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + prDTO := createPullRequestWithCommit(t, owner, newBranch, repo) - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{ Do: "merge", }).AddTokenAuth(token) MakeRequest(t, req, http.StatusOK) - doCheckBranchExists(owner, token, newBranch, repo, http.StatusOK) + doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusOK) // make sure we cannot perform a merge on the same PR MakeRequest(t, req, http.StatusUnprocessableEntity) - doCheckBranchExists(owner, token, newBranch, repo, http.StatusOK) + doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusOK) }) t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) { newBranch := "test-pull-2" - prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + prDTO := createPullRequestWithCommit(t, owner, newBranch, repo) deleteBranch := true - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{ Do: "merge", DeleteBranchAfterMerge: &deleteBranch, }).AddTokenAuth(token) MakeRequest(t, req, http.StatusOK) - doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound) + doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound) }) t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) { newBranch := "test-pull-3" - prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + prDTO := createPullRequestWithCommit(t, owner, newBranch, repo) // set the default branch after merge setting at the repo level prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) @@ -250,17 +250,17 @@ func TestAPIMergePull(t *testing.T) { require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil)) // perform merge - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{ Do: "merge", }).AddTokenAuth(token) MakeRequest(t, req, http.StatusOK) - doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound) + doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound) }) t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) { newBranch := "test-pull-4" - prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo) + prDTO := createPullRequestWithCommit(t, owner, newBranch, repo) // make sure the default branch after merge setting is unset at the repo level prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) @@ -279,13 +279,13 @@ func TestAPIMergePull(t *testing.T) { // perform merge deleteBranch := true - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{ + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{ Do: "merge", DeleteBranchAfterMerge: &deleteBranch, }).AddTokenAuth(token) MakeRequest(t, req, http.StatusOK) - doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound) + doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound) }) }) } @@ -625,7 +625,7 @@ func TestAPIViewPullFilesWithHeadRepoDeleted(t *testing.T) { }) } -func creatPullRequestWithCommit(t *testing.T, user *user_model.User, token, newBranch string, repo *repo_model.Repository) *api.PullRequest { +func createPullRequestWithCommit(t *testing.T, user *user_model.User, newBranch string, repo *repo_model.Repository) *api.PullRequest { // create a new branch with a commit filesResp, err := files_service.ChangeRepoFiles(t.Context(), repo, user, &files_service.ChangeRepoFilesOptions{ Files: []*files_service.ChangeRepoFile{ @@ -656,14 +656,9 @@ func creatPullRequestWithCommit(t *testing.T, user *user_model.User, token, newB require.NotEmpty(t, filesResp) // create a corresponding PR - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", user.Name, repo.Name), &api.CreatePullRequestOption{ - Head: newBranch, - Base: repo.DefaultBranch, - Title: "Add a test file", - }).AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusCreated) - pull := new(api.PullRequest) - DecodeJSON(t, resp, pull) + apiCtx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository) + prDTO, err := doAPICreatePullRequest(apiCtx, repo.OwnerName, repo.Name, repo.DefaultBranch, newBranch)(t) + require.NoError(t, err) - return pull + return &prDTO } From 64db3687831318dab7cbb036b3f8e7ae8f350e55 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 14:12:36 +0800 Subject: [PATCH 07/14] fix --- routers/api/v1/repo/pull.go | 8 +- routers/web/repo/pull.go | 3 +- tests/integration/api_issue_config_test.go | 2 +- tests/integration/api_pull_test.go | 119 ++++++------------ .../integration/api_repo_file_create_test.go | 4 +- tests/integration/api_repo_file_helpers.go | 42 ++++--- 6 files changed, 74 insertions(+), 104 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 0de28dac8c03c..8ddd32f81c775 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -939,7 +939,7 @@ func MergePullRequest(ctx *context.APIContext) { } else if errors.Is(err, pull_service.ErrNoPermissionToMerge) { ctx.APIError(http.StatusMethodNotAllowed, "User not allowed to merge PR") } else if errors.Is(err, pull_service.ErrHasMerged) { - ctx.APIError(http.StatusMethodNotAllowed, "") + ctx.APIError(http.StatusMethodNotAllowed, "The PR is already merged") } else if errors.Is(err, pull_service.ErrIsWorkInProgress) { ctx.APIError(http.StatusMethodNotAllowed, "Work in progress PRs cannot be merged") } else if errors.Is(err, pull_service.ErrNotMergeableState) { @@ -991,7 +991,8 @@ func MergePullRequest(ctx *context.APIContext) { } if form.MergeWhenChecksSucceed { - scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false)) + deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { if pull_model.IsErrAlreadyScheduledToAutoMerge(err) { ctx.APIError(http.StatusConflict, err) @@ -1043,6 +1044,9 @@ func MergePullRequest(ctx *context.APIContext) { } // for agit flow, we should not delete the agit reference after merge + // FIXME: old code has that comment above. Is that comment valid? What would go wrong if a agit branch is deleted after merge? + // * If a agit branch can be deleted after merge, then fix the comment and maybe other related code + // * If a agit branch should not be deleted, then we need to fix the logic and add more tests shouldDeleteBranch := pr.Flow == issues_model.PullRequestFlowGithub if form.DeleteBranchAfterMerge != nil { // if the form field is defined, it takes precedence over the repo setting equivalent diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 7392d5814fb3d..7df67e4beec4c 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1107,7 +1107,8 @@ func MergePullRequest(ctx *context.Context) { // delete all scheduled auto merges _ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) // schedule auto merge - scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false)) + deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { ctx.ServerError("ScheduleAutoMerge", err) return diff --git a/tests/integration/api_issue_config_test.go b/tests/integration/api_issue_config_test.go index ad399654437f8..f6045e1a80518 100644 --- a/tests/integration/api_issue_config_test.go +++ b/tests/integration/api_issue_config_test.go @@ -129,7 +129,7 @@ func TestAPIRepoIssueConfigPaths(t *testing.T) { configData, err := yaml.Marshal(configMap) assert.NoError(t, err) - _, err = createFileInBranch(owner, repo, fullPath, repo.DefaultBranch, string(configData)) + _, err = createFile(owner, repo, fullPath, string(configData)) assert.NoError(t, err) issueConfig := getIssueConfig(t, owner.Name, repo.Name) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index c70f924dbfe83..d030ae19d825e 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -22,6 +22,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/convert" "code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/gitdiff" @@ -190,49 +191,50 @@ func TestAPIMergePullWIP(t *testing.T) { } func TestAPIMergePull(t *testing.T) { - doCheckBranchExists := func(t *testing.T, user *user_model.User, token, branchName string, repo *repo_model.Repository, status int) { - req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", user.Name, repo.Name, branchName)).AddTokenAuth(token) - MakeRequest(t, req, status) - } - onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) - session := loginUser(t, owner.Name) - token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + apiCtx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository) - t.Run("Normal", func(t *testing.T) { - newBranch := "test-pull-1" - prDTO := createPullRequestWithCommit(t, owner, newBranch, repo) + checkBranchExists := func(t *testing.T, branchName string, status int) { + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", owner.Name, repo.Name, branchName)).AddTokenAuth(apiCtx.Token) + MakeRequest(t, req, status) + } - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{ - Do: "merge", - }).AddTokenAuth(token) + createTestBranchPR := func(t *testing.T, branchName string) *api.PullRequest { + testCreateFileInBranch(t, owner, repo, createFileInBranchOptions{NewBranch: branchName}, map[string]string{"a-new-file-" + branchName + ".txt": "dummy content"}) + prDTO, err := doAPICreatePullRequest(apiCtx, repo.OwnerName, repo.Name, repo.DefaultBranch, branchName)(t) + require.NoError(t, err) + return &prDTO + } - MakeRequest(t, req, http.StatusOK) - doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusOK) - // make sure we cannot perform a merge on the same PR - MakeRequest(t, req, http.StatusUnprocessableEntity) - doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusOK) + performMerge := func(t *testing.T, prIndex int64, deleteBranchAfterMerge *bool, optExpectedStatus ...int) { + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prIndex), &forms.MergePullRequestForm{ + Do: "merge", + DeleteBranchAfterMerge: deleteBranchAfterMerge, + }).AddTokenAuth(apiCtx.Token) + expectedStatus := util.OptionalArg(optExpectedStatus, http.StatusOK) + MakeRequest(t, req, expectedStatus) + } + + t.Run("Normal", func(t *testing.T) { + newBranch := "test-pull-1" + prDTO := createTestBranchPR(t, newBranch) + performMerge(t, prDTO.Index, nil) + checkBranchExists(t, newBranch, http.StatusOK) + performMerge(t, prDTO.Index, nil, http.StatusMethodNotAllowed) // make sure we cannot perform a merge on the same PR }) t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) { newBranch := "test-pull-2" - prDTO := createPullRequestWithCommit(t, owner, newBranch, repo) - deleteBranch := true - - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{ - Do: "merge", - DeleteBranchAfterMerge: &deleteBranch, - }).AddTokenAuth(token) - - MakeRequest(t, req, http.StatusOK) - doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound) + prDTO := createTestBranchPR(t, newBranch) + performMerge(t, prDTO.Index, util.ToPointer(true)) + checkBranchExists(t, newBranch, http.StatusNotFound) }) t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) { newBranch := "test-pull-3" - prDTO := createPullRequestWithCommit(t, owner, newBranch, repo) + prDTO := createTestBranchPR(t, newBranch) // set the default branch after merge setting at the repo level prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) @@ -249,18 +251,13 @@ func TestAPIMergePull(t *testing.T) { }) require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil)) - // perform merge - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{ - Do: "merge", - }).AddTokenAuth(token) - - MakeRequest(t, req, http.StatusOK) - doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound) + performMerge(t, prDTO.Index, nil) + checkBranchExists(t, newBranch, http.StatusNotFound) }) t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) { newBranch := "test-pull-4" - prDTO := createPullRequestWithCommit(t, owner, newBranch, repo) + prDTO := createTestBranchPR(t, newBranch) // make sure the default branch after merge setting is unset at the repo level prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) @@ -278,14 +275,8 @@ func TestAPIMergePull(t *testing.T) { require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil)) // perform merge - deleteBranch := true - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{ - Do: "merge", - DeleteBranchAfterMerge: &deleteBranch, - }).AddTokenAuth(token) - - MakeRequest(t, req, http.StatusOK) - doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound) + performMerge(t, prDTO.Index, util.ToPointer(true)) + checkBranchExists(t, newBranch, http.StatusNotFound) }) }) } @@ -624,41 +615,3 @@ func TestAPIViewPullFilesWithHeadRepoDeleted(t *testing.T) { })(t) }) } - -func createPullRequestWithCommit(t *testing.T, user *user_model.User, newBranch string, repo *repo_model.Repository) *api.PullRequest { - // create a new branch with a commit - filesResp, err := files_service.ChangeRepoFiles(t.Context(), repo, user, &files_service.ChangeRepoFilesOptions{ - Files: []*files_service.ChangeRepoFile{ - { - Operation: "create", - TreePath: strings.ReplaceAll(newBranch, " ", "_"), - ContentReader: strings.NewReader("This is a test."), - }, - }, - Message: "Add test file using branch name as its name", - OldBranch: repo.DefaultBranch, - NewBranch: newBranch, - Author: &files_service.IdentityOptions{ - GitUserName: user.Name, - GitUserEmail: user.Email, - }, - Committer: &files_service.IdentityOptions{ - GitUserName: user.Name, - GitUserEmail: user.Email, - }, - Dates: &files_service.CommitDateOptions{ - Author: time.Now(), - Committer: time.Now(), - }, - }) - - require.NoError(t, err) - require.NotEmpty(t, filesResp) - - // create a corresponding PR - apiCtx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository) - prDTO, err := doAPICreatePullRequest(apiCtx, repo.OwnerName, repo.Name, repo.DefaultBranch, newBranch)(t) - require.NoError(t, err) - - return &prDTO -} diff --git a/tests/integration/api_repo_file_create_test.go b/tests/integration/api_repo_file_create_test.go index af3bc546803a1..7cf1083248aaf 100644 --- a/tests/integration/api_repo_file_create_test.go +++ b/tests/integration/api_repo_file_create_test.go @@ -133,7 +133,7 @@ func BenchmarkAPICreateFileSmall(b *testing.B) { b.ResetTimer() for n := 0; b.Loop(); n++ { treePath := fmt.Sprintf("update/file%d.txt", n) - _, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath) + _, _ = createFile(user2, repo1, treePath) } }) } @@ -149,7 +149,7 @@ func BenchmarkAPICreateFileMedium(b *testing.B) { for n := 0; b.Loop(); n++ { treePath := fmt.Sprintf("update/file%d.txt", n) copy(data, treePath) - _, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath) + _, _ = createFile(user2, repo1, treePath) } }) } diff --git a/tests/integration/api_repo_file_helpers.go b/tests/integration/api_repo_file_helpers.go index 37962028bd6e1..fc68bed7b3e13 100644 --- a/tests/integration/api_repo_file_helpers.go +++ b/tests/integration/api_repo_file_helpers.go @@ -6,26 +6,36 @@ package integration import ( "context" "strings" + "testing" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" files_service "code.gitea.io/gitea/services/repository/files" + + "github.com/stretchr/testify/require" ) -func createFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, branchName, content string) (*api.FilesResponse, error) { +type createFileInBranchOptions struct { + OldBranch, NewBranch string +} + +func testCreateFileInBranch(t *testing.T, user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) *api.FilesResponse { + resp, err := createFileInBranch(user, repo, createOpts, files) + require.NoError(t, err) + return resp +} + +func createFileInBranch(user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) (*api.FilesResponse, error) { ctx := context.TODO() - opts := &files_service.ChangeRepoFilesOptions{ - Files: []*files_service.ChangeRepoFile{ - { - Operation: "create", - TreePath: treePath, - ContentReader: strings.NewReader(content), - }, - }, - OldBranch: branchName, - Author: nil, - Committer: nil, + opts := &files_service.ChangeRepoFilesOptions{OldBranch: createOpts.OldBranch, NewBranch: createOpts.NewBranch} + for path, content := range files { + opts.Files = append(opts.Files, &files_service.ChangeRepoFile{ + Operation: "create", + TreePath: path, + ContentReader: strings.NewReader(content), + }) } return files_service.ChangeRepoFiles(ctx, repo, user, opts) } @@ -53,10 +63,12 @@ func createOrReplaceFileInBranch(user *user_model.User, repo *repo_model.Reposit return err } - _, err = createFileInBranch(user, repo, treePath, branchName, content) + _, err = createFileInBranch(user, repo, createFileInBranchOptions{OldBranch: branchName}, map[string]string{treePath: content}) return err } -func createFile(user *user_model.User, repo *repo_model.Repository, treePath string) (*api.FilesResponse, error) { - return createFileInBranch(user, repo, treePath, repo.DefaultBranch, "This is a NEW file") +// TODO: replace all usages of this function with testCreateFileInBranch or testCreateFile +func createFile(user *user_model.User, repo *repo_model.Repository, treePath string, optContent ...string) (*api.FilesResponse, error) { + content := util.OptionalArg(optContent, "file content "+treePath) + return createFileInBranch(user, repo, createFileInBranchOptions{}, map[string]string{treePath: content}) } From 4df4d459e5f659e1370efa7ac58bcde7b4a628c9 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 15:04:55 +0800 Subject: [PATCH 08/14] fix test --- tests/integration/api_repo_file_helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/api_repo_file_helpers.go b/tests/integration/api_repo_file_helpers.go index fc68bed7b3e13..785416b0f580d 100644 --- a/tests/integration/api_repo_file_helpers.go +++ b/tests/integration/api_repo_file_helpers.go @@ -69,6 +69,6 @@ func createOrReplaceFileInBranch(user *user_model.User, repo *repo_model.Reposit // TODO: replace all usages of this function with testCreateFileInBranch or testCreateFile func createFile(user *user_model.User, repo *repo_model.Repository, treePath string, optContent ...string) (*api.FilesResponse, error) { - content := util.OptionalArg(optContent, "file content "+treePath) + content := util.OptionalArg(optContent, "This is a NEW file") // some tests need this default content because its SHA is hardcoded return createFileInBranch(user, repo, createFileInBranchOptions{}, map[string]string{treePath: content}) } From 6998025a9aeee633a7124816c54fa4a8790ea006 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 16:04:53 +0800 Subject: [PATCH 09/14] fine tune --- routers/web/repo/pull.go | 4 +- tests/integration/api_pull_test.go | 59 ++++++++++-------------------- 2 files changed, 21 insertions(+), 42 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 7df67e4beec4c..ee579ecea5dd5 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1103,11 +1103,11 @@ func MergePullRequest(ctx *context.Context) { message += "\n\n" + form.MergeMessageField } + deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) if form.MergeWhenChecksSucceed { // delete all scheduled auto merges _ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) // schedule auto merge - deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { ctx.ServerError("ScheduleAutoMerge", err) @@ -1194,7 +1194,7 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) - if !optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) { + if !deleteBranchAfterMerge { ctx.JSONRedirect(issue.Link()) return } diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index d030ae19d825e..433dce3d5eec5 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -208,11 +208,8 @@ func TestAPIMergePull(t *testing.T) { return &prDTO } - performMerge := func(t *testing.T, prIndex int64, deleteBranchAfterMerge *bool, optExpectedStatus ...int) { - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prIndex), &forms.MergePullRequestForm{ - Do: "merge", - DeleteBranchAfterMerge: deleteBranchAfterMerge, - }).AddTokenAuth(apiCtx.Token) + performMerge := func(t *testing.T, prIndex int64, params map[string]any, optExpectedStatus ...int) { + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prIndex), params).AddTokenAuth(apiCtx.Token) expectedStatus := util.OptionalArg(optExpectedStatus, http.StatusOK) MakeRequest(t, req, expectedStatus) } @@ -220,62 +217,44 @@ func TestAPIMergePull(t *testing.T) { t.Run("Normal", func(t *testing.T) { newBranch := "test-pull-1" prDTO := createTestBranchPR(t, newBranch) - performMerge(t, prDTO.Index, nil) + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}) checkBranchExists(t, newBranch, http.StatusOK) - performMerge(t, prDTO.Index, nil, http.StatusMethodNotAllowed) // make sure we cannot perform a merge on the same PR + // try to merge again, make sure we cannot perform a merge on the same PR + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}, http.StatusMethodNotAllowed) }) t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) { newBranch := "test-pull-2" prDTO := createTestBranchPR(t, newBranch) - performMerge(t, prDTO.Index, util.ToPointer(true)) + performMerge(t, prDTO.Index, map[string]any{"do": "merge", "delete_branch_after_merge": true}) checkBranchExists(t, newBranch, http.StatusNotFound) }) - t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) { - newBranch := "test-pull-3" - prDTO := createTestBranchPR(t, newBranch) - - // set the default branch after merge setting at the repo level + updateRepoUnitDefaultDeleteBranchAfterMerge := func(t *testing.T, repo *repo_model.Repository, value bool) { prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) require.NoError(t, err) - prConfig := prUnit.PullRequestsConfig() - prConfig.DefaultDeleteBranchAfterMerge = true - - var units []repo_model.RepoUnit - units = append(units, repo_model.RepoUnit{ + prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge = value + require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, []repo_model.RepoUnit{{ RepoID: repo.ID, Type: unit_model.TypePullRequests, - Config: prConfig, - }) - require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil)) + Config: prUnit.PullRequestsConfig(), + }}, nil)) + } - performMerge(t, prDTO.Index, nil) + t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) { + newBranch := "test-pull-3" + prDTO := createTestBranchPR(t, newBranch) + updateRepoUnitDefaultDeleteBranchAfterMerge(t, repo, true) + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}) checkBranchExists(t, newBranch, http.StatusNotFound) }) t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) { newBranch := "test-pull-4" prDTO := createTestBranchPR(t, newBranch) - - // make sure the default branch after merge setting is unset at the repo level - prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) - require.NoError(t, err) - - prConfig := prUnit.PullRequestsConfig() - prConfig.DefaultDeleteBranchAfterMerge = false - - var units []repo_model.RepoUnit - units = append(units, repo_model.RepoUnit{ - RepoID: repo.ID, - Type: unit_model.TypePullRequests, - Config: prConfig, - }) - require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil)) - - // perform merge - performMerge(t, prDTO.Index, util.ToPointer(true)) + updateRepoUnitDefaultDeleteBranchAfterMerge(t, repo, false) + performMerge(t, prDTO.Index, map[string]any{"do": "merge", "delete_branch_after_merge": true}) checkBranchExists(t, newBranch, http.StatusNotFound) }) }) From 2a57ea3bed0205fd64dc3aeed34f3809c75b951d Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 16:08:21 +0800 Subject: [PATCH 10/14] make ScheduleAutoMerge share the same "delete branch after merge" logic --- routers/api/v1/repo/pull.go | 42 ++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 8ddd32f81c775..22df300e7100b 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -25,7 +25,6 @@ import ( "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" @@ -990,8 +989,26 @@ func MergePullRequest(ctx *context.APIContext) { message += "\n\n" + form.MergeMessageField } + prUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit.TypePullRequests) + if err != nil { + ctx.APIErrorInternal(err) + return + } + + // for agit flow, we should not delete the agit reference after merge + // FIXME: old code has that comment above. Is that comment valid? What would go wrong if a agit branch is deleted after merge? + // * If a agit branch can be deleted after merge, then fix the comment and maybe other related code + // * If a agit branch should not be deleted, then we need to fix the logic and add more tests + deleteBranchAfterMerge := pr.Flow == issues_model.PullRequestFlowGithub + if form.DeleteBranchAfterMerge != nil { + // if the form field is defined, it takes precedence over the repo setting equivalent + deleteBranchAfterMerge = deleteBranchAfterMerge && *form.DeleteBranchAfterMerge + } else { + // otherwise, we look at the repo setting to make the determination + deleteBranchAfterMerge = deleteBranchAfterMerge && prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge + } + if form.MergeWhenChecksSucceed { - deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { if pull_model.IsErrAlreadyScheduledToAutoMerge(err) { @@ -1037,26 +1054,7 @@ func MergePullRequest(ctx *context.APIContext) { } log.Trace("Pull request merged: %d", pr.ID) - prUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit.TypePullRequests) - if err != nil { - ctx.APIErrorInternal(err) - return - } - - // for agit flow, we should not delete the agit reference after merge - // FIXME: old code has that comment above. Is that comment valid? What would go wrong if a agit branch is deleted after merge? - // * If a agit branch can be deleted after merge, then fix the comment and maybe other related code - // * If a agit branch should not be deleted, then we need to fix the logic and add more tests - shouldDeleteBranch := pr.Flow == issues_model.PullRequestFlowGithub - if form.DeleteBranchAfterMerge != nil { - // if the form field is defined, it takes precedence over the repo setting equivalent - shouldDeleteBranch = shouldDeleteBranch && *form.DeleteBranchAfterMerge - } else { - // otherwise, we look at the repo setting to make the determination - shouldDeleteBranch = shouldDeleteBranch && prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge - } - - if shouldDeleteBranch { + if deleteBranchAfterMerge { // check permission even it has been checked in repo_service.DeleteBranch so that we don't need to // do RetargetChildrenOnMerge if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil { From a68474a144863ac7dd6175cee02cf101a519f134 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 19:09:50 +0800 Subject: [PATCH 11/14] rewrite --- models/git/protected_branch.go | 3 +- modules/util/error.go | 53 +++--- routers/api/v1/repo/pull.go | 58 +----- routers/web/repo/actions/view.go | 4 +- routers/web/repo/editor_apply_patch.go | 2 +- routers/web/repo/editor_cherry_pick.go | 2 +- routers/web/repo/editor_error.go | 4 +- routers/web/repo/pull.go | 170 +++--------------- services/actions/workflow.go | 10 +- services/automerge/automerge.go | 21 +-- services/pull/merge.go | 21 +++ services/repository/branch.go | 93 +++++++++- .../issue/view_content/pull_merge_box.tmpl | 4 +- 13 files changed, 191 insertions(+), 254 deletions(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 511f7563cf52d..13e1ced0e1849 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -5,7 +5,6 @@ package git import ( "context" - "errors" "fmt" "slices" "strings" @@ -25,7 +24,7 @@ import ( "xorm.io/builder" ) -var ErrBranchIsProtected = errors.New("branch is protected") +var ErrBranchIsProtected = util.ErrorWrap(util.ErrPermissionDenied, "branch is protected") // ProtectedBranch struct type ProtectedBranch struct { diff --git a/modules/util/error.go b/modules/util/error.go index 6b2721618ec60..e8e8639632b3e 100644 --- a/modules/util/error.go +++ b/modules/util/error.go @@ -6,6 +6,7 @@ package util import ( "errors" "fmt" + "html/template" ) // Common Errors forming the base of our error system @@ -39,22 +40,6 @@ func (w errorWrapper) Unwrap() error { return w.Err } -type LocaleWrapper struct { - err error - TrKey string - TrArgs []any -} - -// Error returns the message -func (w LocaleWrapper) Error() string { - return w.err.Error() -} - -// Unwrap returns the underlying error -func (w LocaleWrapper) Unwrap() error { - return w.err -} - // ErrorWrap returns an error that formats as the given text but unwraps as the provided error func ErrorWrap(unwrap error, message string, args ...any) error { if len(args) == 0 { @@ -83,15 +68,39 @@ func NewNotExistErrorf(message string, args ...any) error { return ErrorWrap(ErrNotExist, message, args...) } -// ErrorWrapLocale wraps an err with a translation key and arguments -func ErrorWrapLocale(err error, trKey string, trArgs ...any) error { - return LocaleWrapper{err: err, TrKey: trKey, TrArgs: trArgs} +// ErrorTranslatable wraps an error with translation information +type ErrorTranslatable interface { + error + Unwrap() error + Translate(ErrorLocaleTranslator) template.HTML +} + +type errorTranslatableWrapper struct { + err error + trKey string + trArgs []any +} + +type ErrorLocaleTranslator interface { + Tr(key string, args ...any) template.HTML +} + +func (w *errorTranslatableWrapper) Error() string { return w.err.Error() } + +func (w *errorTranslatableWrapper) Unwrap() error { return w.err } + +func (w *errorTranslatableWrapper) Translate(t ErrorLocaleTranslator) template.HTML { + return t.Tr(w.trKey, w.trArgs...) +} + +func ErrorWrapTranslatable(err error, trKey string, trArgs ...any) ErrorTranslatable { + return &errorTranslatableWrapper{err: err, trKey: trKey, trArgs: trArgs} } -func ErrorAsLocale(err error) *LocaleWrapper { - var e LocaleWrapper +func ErrorAsTranslatable(err error) ErrorTranslatable { + var e *errorTranslatableWrapper if errors.As(err, &e) { - return &e + return e } return nil } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 22df300e7100b..81bcec571b320 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -13,7 +13,6 @@ import ( "time" activities_model "code.gitea.io/gitea/models/activities" - git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" pull_model "code.gitea.io/gitea/models/pull" @@ -989,25 +988,12 @@ func MergePullRequest(ctx *context.APIContext) { message += "\n\n" + form.MergeMessageField } - prUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit.TypePullRequests) + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, form.DeleteBranchAfterMerge, ctx.Repo.Repository, pr) if err != nil { ctx.APIErrorInternal(err) return } - // for agit flow, we should not delete the agit reference after merge - // FIXME: old code has that comment above. Is that comment valid? What would go wrong if a agit branch is deleted after merge? - // * If a agit branch can be deleted after merge, then fix the comment and maybe other related code - // * If a agit branch should not be deleted, then we need to fix the logic and add more tests - deleteBranchAfterMerge := pr.Flow == issues_model.PullRequestFlowGithub - if form.DeleteBranchAfterMerge != nil { - // if the form field is defined, it takes precedence over the repo setting equivalent - deleteBranchAfterMerge = deleteBranchAfterMerge && *form.DeleteBranchAfterMerge - } else { - // otherwise, we look at the repo setting to make the determination - deleteBranchAfterMerge = deleteBranchAfterMerge && prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge - } - if form.MergeWhenChecksSucceed { scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { @@ -1055,45 +1041,9 @@ func MergePullRequest(ctx *context.APIContext) { log.Trace("Pull request merged: %d", pr.ID) if deleteBranchAfterMerge { - // check permission even it has been checked in repo_service.DeleteBranch so that we don't need to - // do RetargetChildrenOnMerge - if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil { - // Don't cleanup when there are other PR's that use this branch as head branch. - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) - if err != nil { - ctx.APIErrorInternal(err) - return - } - if exist { - ctx.Status(http.StatusOK) - return - } - - var headRepo *git.Repository - if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil { - headRepo = ctx.Repo.GitRepo - } else { - headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.APIErrorInternal(err) - return - } - defer headRepo.Close() - } - - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch, pr); err != nil { - switch { - case git.IsErrBranchNotExist(err): - ctx.APIErrorNotFound(err) - case errors.Is(err, repo_service.ErrBranchIsDefault): - ctx.APIError(http.StatusForbidden, errors.New("can not delete default branch")) - case errors.Is(err, git_model.ErrBranchIsProtected): - ctx.APIError(http.StatusForbidden, errors.New("branch protected")) - default: - ctx.APIErrorInternal(err) - } - return - } + // no way to tell users that what error happens, and the PR has been merged, so ignore the error + if err = repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr.ID, nil); err != nil { + log.Debug("DeleteBranchAfterMerge: pr %d, err: %v", pr.ID, err) } } diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 4250e6ff77d73..a05379582161e 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -887,8 +887,8 @@ func Run(ctx *context_module.Context) { return nil }) if err != nil { - if errLocale := util.ErrorAsLocale(err); errLocale != nil { - ctx.Flash.Error(ctx.Tr(errLocale.TrKey, errLocale.TrArgs...)) + if errTr := util.ErrorAsTranslatable(err); errTr != nil { + ctx.Flash.Error(errTr.Translate(ctx.Locale)) ctx.Redirect(redirectURL) } else { ctx.ServerError("DispatchActionWorkflow", err) diff --git a/routers/web/repo/editor_apply_patch.go b/routers/web/repo/editor_apply_patch.go index bd2811cc5f01d..aad7b4129c795 100644 --- a/routers/web/repo/editor_apply_patch.go +++ b/routers/web/repo/editor_apply_patch.go @@ -41,7 +41,7 @@ func NewDiffPatchPost(ctx *context.Context) { Committer: parsed.GitCommitter, }) if err != nil { - err = util.ErrorWrapLocale(err, "repo.editor.fail_to_apply_patch") + err = util.ErrorWrapTranslatable(err, "repo.editor.fail_to_apply_patch") } if err != nil { editorHandleFileOperationError(ctx, parsed.NewBranchName, err) diff --git a/routers/web/repo/editor_cherry_pick.go b/routers/web/repo/editor_cherry_pick.go index 10c2741b1cb22..099814a9fa9bb 100644 --- a/routers/web/repo/editor_cherry_pick.go +++ b/routers/web/repo/editor_cherry_pick.go @@ -74,7 +74,7 @@ func CherryPickPost(ctx *context.Context) { opts.Content = buf.String() _, err = files.ApplyDiffPatch(ctx, ctx.Repo.Repository, ctx.Doer, opts) if err != nil { - err = util.ErrorWrapLocale(err, "repo.editor.fail_to_apply_patch") + err = util.ErrorWrapTranslatable(err, "repo.editor.fail_to_apply_patch") } } if err != nil { diff --git a/routers/web/repo/editor_error.go b/routers/web/repo/editor_error.go index 245226a039e4d..e1473a34b397c 100644 --- a/routers/web/repo/editor_error.go +++ b/routers/web/repo/editor_error.go @@ -38,8 +38,8 @@ func editorHandleFileOperationErrorRender(ctx *context_service.Context, message, } func editorHandleFileOperationError(ctx *context_service.Context, targetBranchName string, err error) { - if errAs := util.ErrorAsLocale(err); errAs != nil { - ctx.JSONError(ctx.Tr(errAs.TrKey, errAs.TrArgs...)) + if errAs := util.ErrorAsTranslatable(err); errAs != nil { + ctx.JSONError(errAs.Translate(ctx.Locale)) } else if errAs, ok := errorAs[git.ErrNotExist](err); ok { ctx.JSONError(ctx.Tr("repo.editor.file_modifying_no_longer_exists", errAs.RelPath)) } else if errAs, ok := errorAs[git_model.ErrLFSFileLocked](err); ok { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index ee579ecea5dd5..cbecb24352727 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -32,7 +32,6 @@ import ( "code.gitea.io/gitea/modules/graceful" issue_template "code.gitea.io/gitea/modules/issue/template" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/templates" "code.gitea.io/gitea/modules/util" @@ -1103,7 +1102,12 @@ func MergePullRequest(ctx *context.Context) { message += "\n\n" + form.MergeMessageField } - deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, form.DeleteBranchAfterMerge, ctx.Repo.Repository, pr) + if err != nil { + ctx.ServerError("ShouldDeleteBranchAfterMerge", err) + return + } + if form.MergeWhenChecksSucceed { // delete all scheduled auto merges _ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) @@ -1194,35 +1198,27 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) - if !deleteBranchAfterMerge { - ctx.JSONRedirect(issue.Link()) - return + if deleteBranchAfterMerge { + deleteBranchAfterMergeAndFlashMessage(ctx, pr.ID) + if ctx.Written() { + return + } } + ctx.JSONRedirect(issue.Link()) +} - // Don't cleanup when other pr use this branch as head branch - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) - if err != nil { - ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) +func deleteBranchAfterMergeAndFlashMessage(ctx *context.Context, prID int64) { + var fullBranchName string + err := repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, prID, &fullBranchName) + if errTr := util.ErrorAsTranslatable(err); errTr != nil { + ctx.Flash.Error(errTr.Translate(ctx.Locale)) return - } - if exist { - ctx.JSONRedirect(issue.Link()) + } else if err == nil { + ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName)) return } - - var headRepo *git.Repository - if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil { - headRepo = ctx.Repo.GitRepo - } else { - headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) - return - } - defer headRepo.Close() - } - deleteBranch(ctx, pr, headRepo) - ctx.JSONRedirect(issue.Link()) + // catch unknown errors + ctx.ServerError("DeleteBranchAfterMerge", err) } // CancelAutoMergePullRequest cancels a scheduled pr @@ -1411,131 +1407,17 @@ func CompareAndPullRequestPost(ctx *context.Context) { } // CleanUpPullRequest responses for delete merged branch when PR has been merged +// Used by "DeleteBranchLink" for "delete branch" button func CleanUpPullRequest(ctx *context.Context) { issue, ok := getPullInfo(ctx) if !ok { return } - - pr := issue.PullRequest - - // Don't cleanup unmerged and unclosed PRs and agit PRs - if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub { - ctx.NotFound(nil) - return - } - - // Don't cleanup when there are other PR's that use this branch as head branch. - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) - if err != nil { - ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) - return - } - if exist { - ctx.NotFound(nil) - return - } - - if err := pr.LoadHeadRepo(ctx); err != nil { - ctx.ServerError("LoadHeadRepo", err) - return - } else if pr.HeadRepo == nil { - // Forked repository has already been deleted - ctx.NotFound(nil) - return - } else if err = pr.LoadBaseRepo(ctx); err != nil { - ctx.ServerError("LoadBaseRepo", err) - return - } else if err = pr.HeadRepo.LoadOwner(ctx); err != nil { - ctx.ServerError("HeadRepo.LoadOwner", err) - return - } - - if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err != nil { - if errors.Is(err, util.ErrPermissionDenied) { - ctx.NotFound(nil) - } else { - ctx.ServerError("CanDeleteBranch", err) - } - return - } - - fullBranchName := pr.HeadRepo.Owner.Name + "/" + pr.HeadBranch - - var gitBaseRepo *git.Repository - - // Assume that the base repo is the current context (almost certainly) - if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.BaseRepoID && ctx.Repo.GitRepo != nil { - gitBaseRepo = ctx.Repo.GitRepo - } else { - // If not just open it - gitBaseRepo, err = gitrepo.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.FullName()), err) - return - } - defer gitBaseRepo.Close() - } - - // Now assume that the head repo is the same as the base repo (reasonable chance) - gitRepo := gitBaseRepo - // But if not: is it the same as the context? - if pr.BaseRepoID != pr.HeadRepoID && ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil { - gitRepo = ctx.Repo.GitRepo - } else if pr.BaseRepoID != pr.HeadRepoID { - // Otherwise just load it up - gitRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) - return - } - defer gitRepo.Close() - } - - defer func() { - ctx.JSONRedirect(issue.Link()) - }() - - // Check if branch has no new commits - headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitHeadRefName()) - if err != nil { - log.Error("GetRefCommitID: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - branchCommitID, err := gitRepo.GetBranchCommitID(pr.HeadBranch) - if err != nil { - log.Error("GetBranchCommitID: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - if headCommitID != branchCommitID { - ctx.Flash.Error(ctx.Tr("repo.branch.delete_branch_has_new_commits", fullBranchName)) - return - } - - deleteBranch(ctx, pr, gitRepo) -} - -func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) { - fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch - - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, gitRepo, pr.HeadBranch, pr); err != nil { - switch { - case git.IsErrBranchNotExist(err): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - case errors.Is(err, repo_service.ErrBranchIsDefault): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - case errors.Is(err, git_model.ErrBranchIsProtected): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - default: - log.Error("DeleteBranch: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - } + deleteBranchAfterMergeAndFlashMessage(ctx, issue.PullRequest.ID) + if ctx.Written() { return } - - ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName)) + ctx.JSONRedirect(issue.Link()) } // DownloadPullDiff render a pull's raw diff diff --git a/services/actions/workflow.go b/services/actions/workflow.go index 25801d6fa1da1..4f72aad11244a 100644 --- a/services/actions/workflow.go +++ b/services/actions/workflow.go @@ -46,14 +46,14 @@ func EnableOrDisableWorkflow(ctx *context.APIContext, workflowID string, isEnabl func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, workflowID, ref string, processInputs func(model *model.WorkflowDispatch, inputs map[string]any) error) error { if workflowID == "" { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("workflowID is empty"), "actions.workflow.not_found", workflowID, ) } if ref == "" { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("ref is empty"), "form.target_ref_not_exist", ref, ) @@ -63,7 +63,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re cfgUnit := repo.MustGetUnit(ctx, unit.TypeActions) cfg := cfgUnit.ActionsConfig() if cfg.IsWorkflowDisabled(workflowID) { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewPermissionDeniedErrorf("workflow is disabled"), "actions.workflow.disabled", ) @@ -82,7 +82,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re runTargetCommit, err = gitRepo.GetBranchCommit(ref) } if err != nil { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("ref %q doesn't exist", ref), "form.target_ref_not_exist", ref, ) @@ -122,7 +122,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re } if entry == nil { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("workflow %q doesn't exist", workflowID), "actions.workflow.not_found", workflowID, ) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index a60883b4cc501..eabfeff1430b2 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -205,18 +205,6 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - var headGitRepo *git.Repository - if pr.BaseRepoID == pr.HeadRepoID { - headGitRepo = baseGitRepo - } else { - headGitRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - log.Error("OpenRepository %-v: %v", pr.HeadRepo, err) - return - } - defer headGitRepo.Close() - } - switch pr.Flow { case issues_model.PullRequestFlowGithub: headBranchExist := pr.HeadRepo != nil && gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) @@ -276,9 +264,12 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - if pr.Flow == issues_model.PullRequestFlowGithub && scheduledPRM.DeleteBranchAfterMerge { - if err := repo_service.DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch, pr); err != nil { - log.Error("DeletePullRequestHeadBranch: %v", err) + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, &scheduledPRM.DeleteBranchAfterMerge, pr.BaseRepo, pr) + if err != nil { + log.Error("ShouldDeleteBranchAfterMerge: %v", err) + } else if deleteBranchAfterMerge { + if err = repo_service.DeleteBranchAfterMerge(ctx, doer, pr.ID, nil); err != nil { + log.Error("DeleteBranchAfterMerge: %v", err) } } } diff --git a/services/pull/merge.go b/services/pull/merge.go index 1e8e9d444b593..548a13c5dd7d3 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -730,3 +730,24 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return true, nil }) } + +func ShouldDeleteBranchAfterMerge(ctx context.Context, userOption *bool, repo *repo_model.Repository, pr *issues_model.PullRequest) (bool, error) { + if pr.Flow != issues_model.PullRequestFlowGithub { + // only support GitHub workflow (branch-based) + // for agit workflow, there is no branch, so nothing to delete + // TODO: maybe in the future, it should delete the "agit reference (/efs/for/xxxx)"? + return false, nil + } + + // if user has set an option, respect it + if userOption != nil { + return *userOption, nil + } + + // otherwise, use repository default + prUnit, err := repo.GetUnit(ctx, unit.TypePullRequests) + if err != nil { + return false, err + } + return prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge, nil +} diff --git a/services/repository/branch.go b/services/repository/branch.go index 5d8178375e033..dadec7d6859d4 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -484,10 +484,7 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m return "", nil } -// enmuerates all branch related errors -var ( - ErrBranchIsDefault = errors.New("branch is default") -) +var ErrBranchIsDefault = util.ErrorWrap(util.ErrPermissionDenied, "branch is default") func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchName string, doer *user_model.User) error { if branchName == repo.DefaultBranch { @@ -745,3 +742,91 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo info.BaseHasNewCommits = info.HeadCommitsBehind > 0 return info, nil } + +func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int64, outFullBranchName *string) error { + pr, err := issues_model.GetPullRequestByID(ctx, prID) + if err != nil { + return err + } + if err = pr.LoadIssue(ctx); err != nil { + return err + } + issue := pr.Issue + + if err = pr.LoadBaseRepo(ctx); err != nil { + return err + } + if err := pr.LoadHeadRepo(ctx); err != nil { + return err + } + if pr.HeadRepo == nil { + // Forked repository has already been deleted + return util.ErrorWrapTranslatable(util.ErrNotExist, "repo.branch.deletion_failed", "(deleted-repo):"+pr.HeadBranch) + } + if err = pr.HeadRepo.LoadOwner(ctx); err != nil { + return err + } + + fullBranchName := pr.HeadRepo.Owner.Name + ":" + pr.HeadBranch + if outFullBranchName != nil { + *outFullBranchName = fullBranchName + } + + errFailedToDelete := func() error { + return util.ErrorWrapTranslatable(util.ErrUnprocessableContent, "repo.branch.deletion_failed", fullBranchName) + } + + // Don't clean up unmerged and unclosed PRs and agit PRs + if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub { + return errFailedToDelete() + } + + // Don't clean up when there are other PR's that use this branch as head branch. + exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) + if err != nil { + return err + } + if exist { + return errFailedToDelete() + } + + if err := CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, doer); err != nil { + if errors.Is(err, util.ErrPermissionDenied) { + return errFailedToDelete() + } + return err + } + + gitBaseRepo, gitBaseCloser, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) + if err != nil { + return err + } + defer gitBaseCloser.Close() + + gitHeadRepo, gitHeadCloser, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) + if err != nil { + return err + } + defer gitHeadCloser.Close() + + // Check if branch has no new commits + headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitHeadRefName()) + if err != nil { + log.Error("GetRefCommitID: %v", err) + return errFailedToDelete() + } + branchCommitID, err := gitHeadRepo.GetBranchCommitID(pr.HeadBranch) + if err != nil { + log.Error("GetBranchCommitID: %v", err) + return errFailedToDelete() + } + if headCommitID != branchCommitID { + return util.ErrorWrapTranslatable(util.ErrUnprocessableContent, "repo.branch.delete_branch_has_new_commits", fullBranchName) + } + + err = DeleteBranch(ctx, doer, pr.HeadRepo, gitHeadRepo, pr.HeadBranch, pr) + if errors.Is(err, util.ErrPermissionDenied) || errors.Is(err, util.ErrNotExist) { + return errFailedToDelete() + } + return err +} diff --git a/templates/repo/issue/view_content/pull_merge_box.tmpl b/templates/repo/issue/view_content/pull_merge_box.tmpl index b0ac24c9f6c3f..b1831dc3f39c0 100644 --- a/templates/repo/issue/view_content/pull_merge_box.tmpl +++ b/templates/repo/issue/view_content/pull_merge_box.tmpl @@ -51,7 +51,7 @@
- +
{{end}} @@ -69,7 +69,7 @@ {{if and .IsPullBranchDeletable (not .IsPullRequestBroken)}}
- +
{{end}} From 8882022badac81511ec1137f2e2a3d5321053358 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 19:50:07 +0800 Subject: [PATCH 12/14] fix --- modules/util/error_test.go | 29 +++++++++++++++++++++++++++++ routers/api/v1/repo/pull.go | 2 +- services/repository/branch.go | 2 +- 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 modules/util/error_test.go diff --git a/modules/util/error_test.go b/modules/util/error_test.go new file mode 100644 index 0000000000000..31d0c9126087a --- /dev/null +++ b/modules/util/error_test.go @@ -0,0 +1,29 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package util + +import ( + "io" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestErrorTranslatable(t *testing.T) { + var err error + + err = ErrorWrapTranslatable(io.EOF, "key", 1) + assert.ErrorIs(t, err, io.EOF) + assert.Equal(t, "EOF", err.Error()) + assert.Equal(t, "key", err.(*errorTranslatableWrapper).trKey) + assert.Equal(t, []any{1}, err.(*errorTranslatableWrapper).trArgs) + + err = ErrorWrap(err, "new msg %d", 100) + assert.ErrorIs(t, err, io.EOF) + assert.Equal(t, "new msg 100", err.Error()) + + errTr := ErrorAsTranslatable(err) + assert.Equal(t, "EOF", errTr.Error()) + assert.Equal(t, "key", errTr.(*errorTranslatableWrapper).trKey) +} diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 81bcec571b320..2561ff3975860 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1041,8 +1041,8 @@ func MergePullRequest(ctx *context.APIContext) { log.Trace("Pull request merged: %d", pr.ID) if deleteBranchAfterMerge { - // no way to tell users that what error happens, and the PR has been merged, so ignore the error if err = repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr.ID, nil); err != nil { + // no way to tell users that what error happens, and the PR has been merged, so ignore the error log.Debug("DeleteBranchAfterMerge: pr %d, err: %v", pr.ID, err) } } diff --git a/services/repository/branch.go b/services/repository/branch.go index dadec7d6859d4..ff7dfd5384991 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -803,7 +803,7 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int } defer gitBaseCloser.Close() - gitHeadRepo, gitHeadCloser, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) + gitHeadRepo, gitHeadCloser, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.HeadRepo) if err != nil { return err } From 4fddb542c834f175e34752179c110078c828032e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 21:13:43 +0800 Subject: [PATCH 13/14] fix test --- services/repository/branch.go | 2 +- templates/repo/issue/view_content/pull_merge_box.tmpl | 4 ++-- tests/integration/pull_merge_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index ff7dfd5384991..c1f26070f2111 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -767,7 +767,7 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int return err } - fullBranchName := pr.HeadRepo.Owner.Name + ":" + pr.HeadBranch + fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch if outFullBranchName != nil { *outFullBranchName = fullBranchName } diff --git a/templates/repo/issue/view_content/pull_merge_box.tmpl b/templates/repo/issue/view_content/pull_merge_box.tmpl index b1831dc3f39c0..4de67be698502 100644 --- a/templates/repo/issue/view_content/pull_merge_box.tmpl +++ b/templates/repo/issue/view_content/pull_merge_box.tmpl @@ -51,7 +51,7 @@
- +
{{end}} @@ -69,7 +69,7 @@ {{if and .IsPullBranchDeletable (not .IsPullRequestBroken)}}
- +
{{end}} diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 3345216838987..32ab7413829c7 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -93,7 +93,7 @@ func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum str // Click the little button to create a pull htmlDoc := NewHTMLParser(t, resp.Body) - link, exists := htmlDoc.doc.Find(".timeline-item .delete-button").Attr("data-url") + link, exists := htmlDoc.doc.Find(".timeline-item .delete-branch-after-merge").Attr("data-url") assert.True(t, exists, "The template has changed, can not find delete button url") req = NewRequestWithValues(t, "POST", link, map[string]string{ "_csrf": htmlDoc.GetCSRF(), From 70592752f92cc04d71d91212408decad8d4a9548 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 21:54:01 +0800 Subject: [PATCH 14/14] fine tune --- services/pull/merge.go | 2 +- services/repository/branch.go | 20 +++++++++----------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/services/pull/merge.go b/services/pull/merge.go index 548a13c5dd7d3..43bb3dd235c52 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -735,7 +735,7 @@ func ShouldDeleteBranchAfterMerge(ctx context.Context, userOption *bool, repo *r if pr.Flow != issues_model.PullRequestFlowGithub { // only support GitHub workflow (branch-based) // for agit workflow, there is no branch, so nothing to delete - // TODO: maybe in the future, it should delete the "agit reference (/efs/for/xxxx)"? + // TODO: maybe in the future, it should delete the "agit reference (refs/for/xxxx)"? return false, nil } diff --git a/services/repository/branch.go b/services/repository/branch.go index c1f26070f2111..b49478422c51e 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -751,8 +751,6 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int if err = pr.LoadIssue(ctx); err != nil { return err } - issue := pr.Issue - if err = pr.LoadBaseRepo(ctx); err != nil { return err } @@ -772,13 +770,13 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int *outFullBranchName = fullBranchName } - errFailedToDelete := func() error { - return util.ErrorWrapTranslatable(util.ErrUnprocessableContent, "repo.branch.deletion_failed", fullBranchName) + errFailedToDelete := func(err error) error { + return util.ErrorWrapTranslatable(err, "repo.branch.deletion_failed", fullBranchName) } // Don't clean up unmerged and unclosed PRs and agit PRs - if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub { - return errFailedToDelete() + if !pr.HasMerged && !pr.Issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub { + return errFailedToDelete(util.ErrUnprocessableContent) } // Don't clean up when there are other PR's that use this branch as head branch. @@ -787,12 +785,12 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int return err } if exist { - return errFailedToDelete() + return errFailedToDelete(util.ErrUnprocessableContent) } if err := CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, doer); err != nil { if errors.Is(err, util.ErrPermissionDenied) { - return errFailedToDelete() + return errFailedToDelete(err) } return err } @@ -813,12 +811,12 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { log.Error("GetRefCommitID: %v", err) - return errFailedToDelete() + return errFailedToDelete(err) } branchCommitID, err := gitHeadRepo.GetBranchCommitID(pr.HeadBranch) if err != nil { log.Error("GetBranchCommitID: %v", err) - return errFailedToDelete() + return errFailedToDelete(err) } if headCommitID != branchCommitID { return util.ErrorWrapTranslatable(util.ErrUnprocessableContent, "repo.branch.delete_branch_has_new_commits", fullBranchName) @@ -826,7 +824,7 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int err = DeleteBranch(ctx, doer, pr.HeadRepo, gitHeadRepo, pr.HeadBranch, pr) if errors.Is(err, util.ErrPermissionDenied) || errors.Is(err, util.ErrNotExist) { - return errFailedToDelete() + return errFailedToDelete(err) } return err }