From e22862e0bb5238ac29f92bc0829b9ec9823603aa Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 25 Oct 2024 19:47:59 -0700 Subject: [PATCH 01/18] Automerge supports deleting branch automatically after merging --- models/migrations/migrations.go | 2 ++ models/migrations/v1_23/v307.go | 29 +++++++++++++++++++ models/pull/automerge.go | 26 +++++++++-------- routers/api/v1/repo/pull.go | 2 +- routers/web/repo/pull.go | 2 +- services/automerge/automerge.go | 4 +-- .../js/components/PullRequestMergeForm.vue | 2 +- 7 files changed, 50 insertions(+), 17 deletions(-) create mode 100644 models/migrations/v1_23/v307.go diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index f0651ddbfafd3..cb749dcb78bbb 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -605,6 +605,8 @@ var migrations = []Migration{ NewMigration("Add Repository Licenses", v1_23.AddRepositoryLicenses), // v306 -> v307 NewMigration("Add BlockAdminMergeOverride to ProtectedBranch", v1_23.AddBlockAdminMergeOverrideBranchProtection), + // v307 -> v308 + NewMigration("Add DeleteBranchAfterMerge to AutoMerge", v1_23.AddDeleteBranchAfterMergeForAutoMerge), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_23/v307.go b/models/migrations/v1_23/v307.go new file mode 100644 index 0000000000000..6a2868ea4c82f --- /dev/null +++ b/models/migrations/v1_23/v307.go @@ -0,0 +1,29 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_23 //nolint + +import ( + "code.gitea.io/gitea/modules/timeutil" + + "xorm.io/xorm" +) + +type pullAutoMerge struct { + ID int64 `xorm:"pk autoincr"` + PullID int64 `xorm:"UNIQUE"` + DoerID int64 `xorm:"INDEX NOT NULL"` + MergeStyle string `xorm:"varchar(30)"` + Message string `xorm:"LONGTEXT"` + DeleteBranchAfterMerge bool + CreatedUnix timeutil.TimeStamp `xorm:"created"` +} + +// TableName return database table name for xorm +func (pullAutoMerge) TableName() string { + return "pull_auto_merge" +} + +func AddDeleteBranchAfterMergeForAutoMerge(x *xorm.Engine) error { + return x.Sync(new(pullAutoMerge)) +} diff --git a/models/pull/automerge.go b/models/pull/automerge.go index f69fcb60d16ca..3cafacc3a4108 100644 --- a/models/pull/automerge.go +++ b/models/pull/automerge.go @@ -15,13 +15,14 @@ import ( // AutoMerge represents a pull request scheduled for merging when checks succeed type AutoMerge struct { - ID int64 `xorm:"pk autoincr"` - PullID int64 `xorm:"UNIQUE"` - DoerID int64 `xorm:"INDEX NOT NULL"` - Doer *user_model.User `xorm:"-"` - MergeStyle repo_model.MergeStyle `xorm:"varchar(30)"` - Message string `xorm:"LONGTEXT"` - CreatedUnix timeutil.TimeStamp `xorm:"created"` + ID int64 `xorm:"pk autoincr"` + PullID int64 `xorm:"UNIQUE"` + DoerID int64 `xorm:"INDEX NOT NULL"` + Doer *user_model.User `xorm:"-"` + MergeStyle repo_model.MergeStyle `xorm:"varchar(30)"` + Message string `xorm:"LONGTEXT"` + DeleteBranchAfterMerge bool + CreatedUnix timeutil.TimeStamp `xorm:"created"` } // TableName return database table name for xorm @@ -49,7 +50,7 @@ func IsErrAlreadyScheduledToAutoMerge(err error) bool { } // ScheduleAutoMerge schedules a pull request to be merged when all checks succeed -func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pullID int64, style repo_model.MergeStyle, message string) error { +func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pullID int64, style repo_model.MergeStyle, message string, deleteBranchAfterMerge bool) error { // Check if we already have a merge scheduled for that pull request if exists, _, err := GetScheduledMergeByPullID(ctx, pullID); err != nil { return err @@ -58,10 +59,11 @@ func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pullID int64, } _, err := db.GetEngine(ctx).Insert(&AutoMerge{ - DoerID: doer.ID, - PullID: pullID, - MergeStyle: style, - Message: message, + DoerID: doer.ID, + PullID: pullID, + MergeStyle: style, + Message: message, + DeleteBranchAfterMerge: deleteBranchAfterMerge, }) return err } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 34ebcb42d5aef..9c0dd378f995e 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1000,7 +1000,7 @@ func MergePullRequest(ctx *context.APIContext) { } if form.MergeWhenChecksSucceed { - scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge) if err != nil { if pull_model.IsErrAlreadyScheduledToAutoMerge(err) { ctx.Error(http.StatusConflict, "ScheduleAutoMerge", err) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 0efe1be76a310..bc8467b9f6439 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1097,7 +1097,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) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge) if err != nil { ctx.ServerError("ScheduleAutoMerge", err) return diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index a1ee204882f51..b280444ca72a7 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -63,9 +63,9 @@ func addToQueue(pr *issues_model.PullRequest, sha string) { } // ScheduleAutoMerge if schedule is false and no error, pull can be merged directly -func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pull *issues_model.PullRequest, style repo_model.MergeStyle, message string) (scheduled bool, err error) { +func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pull *issues_model.PullRequest, style repo_model.MergeStyle, message string, deleteBranchAfterMerge bool) (scheduled bool, err error) { err = db.WithTx(ctx, func(ctx context.Context) error { - if err := pull_model.ScheduleAutoMerge(ctx, doer, pull.ID, style, message); err != nil { + if err := pull_model.ScheduleAutoMerge(ctx, doer, pull.ID, style, message, deleteBranchAfterMerge); err != nil { return err } scheduled = true diff --git a/web_src/js/components/PullRequestMergeForm.vue b/web_src/js/components/PullRequestMergeForm.vue index fc9541b6a699f..0ea606150fc68 100644 --- a/web_src/js/components/PullRequestMergeForm.vue +++ b/web_src/js/components/PullRequestMergeForm.vue @@ -130,7 +130,7 @@ export default { {{ mergeForm.textCancel }} -
+
From 499eb515fcc40539a47d852efacc27887c2ff4cb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 25 Oct 2024 22:57:14 -0700 Subject: [PATCH 02/18] Fix lint --- routers/private/hook_post_receive_test.go | 2 +- tests/integration/pull_merge_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/routers/private/hook_post_receive_test.go b/routers/private/hook_post_receive_test.go index 658557d3cfc03..32c78bd009da8 100644 --- a/routers/private/hook_post_receive_test.go +++ b/routers/private/hook_post_receive_test.go @@ -27,7 +27,7 @@ func TestHandlePullRequestMerging(t *testing.T) { user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - err = pull_model.ScheduleAutoMerge(db.DefaultContext, user1, pr.ID, repo_model.MergeStyleSquash, "squash merge a pr") + err = pull_model.ScheduleAutoMerge(db.DefaultContext, user1, pr.ID, repo_model.MergeStyleSquash, "squash merge a pr", false) assert.NoError(t, err) autoMerge := unittest.AssertExistsAndLoadBean(t, &pull_model.AutoMerge{PullID: pr.ID}) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 43210e852e53b..127d9ec11f486 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -706,12 +706,12 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { session.MakeRequest(t, req, http.StatusSeeOther) // first time insert automerge record, return true - scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") + scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test", false) assert.NoError(t, err) assert.True(t, scheduled) // second time insert automerge record, return false because it does exist - scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") + scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test", false) assert.Error(t, err) assert.False(t, scheduled) @@ -790,12 +790,12 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { session.MakeRequest(t, req, http.StatusSeeOther) // first time insert automerge record, return true - scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") + scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test", false) assert.NoError(t, err) assert.True(t, scheduled) // second time insert automerge record, return false because it does exist - scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") + scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test", false) assert.Error(t, err) assert.False(t, scheduled) @@ -919,12 +919,12 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing. user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) // first time insert automerge record, return true - scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") + scheduled, err := automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test", false) assert.NoError(t, err) assert.True(t, scheduled) // second time insert automerge record, return false because it does exist - scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test") + scheduled, err = automerge.ScheduleAutoMerge(db.DefaultContext, user1, pr, repo_model.MergeStyleMerge, "auto merge test", false) assert.Error(t, err) assert.False(t, scheduled) From 47c9581a8017ddaa03c2e67ed8ccf80c3ba1c673 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 4 Nov 2024 20:42:20 -0800 Subject: [PATCH 03/18] Fix bug --- services/automerge/automerge.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index b280444ca72a7..8cb3af0b5058e 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -24,6 +24,7 @@ import ( "code.gitea.io/gitea/modules/queue" notify_service "code.gitea.io/gitea/services/notify" pull_service "code.gitea.io/gitea/services/pull" + repo_service "code.gitea.io/gitea/services/repository" ) // prAutoMergeQueue represents a queue to handle update pull request tests @@ -303,4 +304,10 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { // on the pull request page. But this should not be finished in a bug fix PR which will be backport to release branch. return } + + if pr.Flow == issues_model.PullRequestFlowGithub && scheduledPRM.DeleteBranchAfterMerge { + if err := repo_service.DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch); err != nil { + log.Error("deleteBranch after automerge for pull[%d] failed: %v", pr.ID, err) + } + } } From 6d79d8ac4cc8e73df2cbc6d6907f7803c4db2949 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 8 Nov 2024 10:52:16 -0800 Subject: [PATCH 04/18] Add permission check when deleting branch after automerge succeed --- services/automerge/automerge.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 8cb3af0b5058e..d0310fc52be30 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -15,6 +15,7 @@ import ( access_model "code.gitea.io/gitea/models/perm/access" pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" @@ -306,8 +307,16 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { } if pr.Flow == issues_model.PullRequestFlowGithub && scheduledPRM.DeleteBranchAfterMerge { - if err := repo_service.DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch); err != nil { - log.Error("deleteBranch after automerge for pull[%d] failed: %v", pr.ID, err) + perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, doer) + if err != nil { + log.Error("GetUserRepoPermission %-v: %v", pr.HeadRepo, err) + return + } + + if perm.CanWrite(unit.TypeCode) { // default branch and branch protection will be checked in DeleteBranch + if err := repo_service.DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch); err != nil { + log.Error("deleteBranch after automerge for pull[%d] failed: %v", pr.ID, err) + } } } } From 75623936e9586f6e731743353fd3f72deeea239b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 8 Nov 2024 18:30:42 -0800 Subject: [PATCH 05/18] refactor delete head branch of pull request --- services/automerge/automerge.go | 13 ++----------- services/repository/branch.go | 34 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index d0310fc52be30..94ceac3f26cb7 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -15,7 +15,6 @@ import ( access_model "code.gitea.io/gitea/models/perm/access" pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" @@ -307,16 +306,8 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { } if pr.Flow == issues_model.PullRequestFlowGithub && scheduledPRM.DeleteBranchAfterMerge { - perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, doer) - if err != nil { - log.Error("GetUserRepoPermission %-v: %v", pr.HeadRepo, err) - return - } - - if perm.CanWrite(unit.TypeCode) { // default branch and branch protection will be checked in DeleteBranch - if err := repo_service.DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch); err != nil { - log.Error("deleteBranch after automerge for pull[%d] failed: %v", pr.ID, err) - } + if err := repo_service.DeletePullRequestHeadBranch(ctx, pr, doer, headGitRepo); err != nil { + log.Error("DeletePullRequestHeadBranch: %v", err) } } } diff --git a/services/repository/branch.go b/services/repository/branch.go index 600ba96e92e17..f3d388357bac0 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -14,7 +14,9 @@ import ( "code.gitea.io/gitea/models/db" 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" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/git" @@ -29,6 +31,7 @@ import ( "code.gitea.io/gitea/modules/util" webhook_module "code.gitea.io/gitea/modules/webhook" notify_service "code.gitea.io/gitea/services/notify" + pull_service "code.gitea.io/gitea/services/pull" files_service "code.gitea.io/gitea/services/repository/files" "xorm.io/builder" @@ -460,12 +463,22 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m // enmuerates all branch related errors var ( - ErrBranchIsDefault = errors.New("branch is default") + ErrBranchIsDefault = errors.New("branch is default") + ErrInsufficientAccess = errors.New("insufficient access") ) // DeleteBranch delete branch func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error { - err := repo.MustNotBeArchived() + perm, err := access_model.GetUserRepoPermission(ctx, repo, doer) + if err != nil { + return err + } + + if !perm.CanWrite(unit.TypeCode) { + return ErrInsufficientAccess + } + + err = repo.MustNotBeArchived() if err != nil { return err } @@ -528,6 +541,23 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R return nil } +func DeletePullRequestHeadBranch(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, headGitRepo *git.Repository) error { + if err := pull_service.RetargetChildrenOnMerge(ctx, doer, pr); err != nil { + return err + } + + if err := DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch); err != nil { + return err + } + + if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.IssueID, pr.HeadBranch); err != nil { + // Do not fail here as branch has already been deleted + log.Error("AddDeletePRBranchComment: %v", err) + } + + return nil +} + type BranchSyncOptions struct { RepoID int64 } From e70e375996a75f445f5dc00a9ca05d6b315b21b8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 9 Nov 2024 18:07:58 -0800 Subject: [PATCH 06/18] Move all checks into DeleteBranch and PushUpdate --- routers/api/v1/repo/branch.go | 12 +----------- routers/api/v1/repo/pull.go | 21 +-------------------- routers/web/repo/branch.go | 2 +- routers/web/repo/pull.go | 12 +----------- services/automerge/automerge.go | 2 +- services/pull/pull.go | 20 +++++++------------- services/repository/branch.go | 26 +++++++------------------- services/repository/push.go | 7 +++++++ 8 files changed, 26 insertions(+), 76 deletions(-) diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index bb16858c81160..0f5995889fafe 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -133,11 +133,6 @@ func DeleteBranch(ctx *context.APIContext) { branchName := ctx.PathParam("*") - if ctx.Repo.Repository.IsEmpty { - ctx.Error(http.StatusForbidden, "", "Git Repository is empty.") - return - } - // check whether branches of this repository has been synced totalNumOfBranches, err := db.Count[git_model.Branch](ctx, git_model.FindBranchOptions{ RepoID: ctx.Repo.Repository.ID, @@ -155,12 +150,7 @@ func DeleteBranch(ctx *context.APIContext) { } } - if ctx.Repo.Repository.IsMirror { - ctx.Error(http.StatusForbidden, "IsMirrored", fmt.Errorf("can not delete branch of an mirror repository")) - return - } - - if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil { + if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName, nil); err != nil { switch { case git.IsErrBranchNotExist(err): ctx.NotFound(err) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 9c0dd378f995e..c60e7a9d77329 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1046,17 +1046,6 @@ func MergePullRequest(ctx *context.APIContext) { log.Trace("Pull request merged: %d", pr.ID) if form.DeleteBranchAfterMerge { - // 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.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 @@ -1068,11 +1057,7 @@ func MergePullRequest(ctx *context.APIContext) { } defer headRepo.Close() } - if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { - ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err) - return - } - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil { + if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch, pr); err != nil { switch { case git.IsErrBranchNotExist(err): ctx.NotFound(err) @@ -1085,10 +1070,6 @@ func MergePullRequest(ctx *context.APIContext) { } return } - if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { - // Do not fail here as branch has already been deleted - log.Error("DeleteBranch: %v", err) - } } ctx.Status(http.StatusOK) diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index 4a62237838a4a..c56b55bf15738 100644 --- a/routers/web/repo/branch.go +++ b/routers/web/repo/branch.go @@ -98,7 +98,7 @@ func DeleteBranchPost(ctx *context.Context) { defer redirect(ctx) branchName := ctx.FormString("name") - if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil { + if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName, nil); err != nil { switch { case git.IsErrBranchNotExist(err): log.Debug("DeleteBranch: Can't delete non existing branch '%s'", branchName) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 4b3b14268b51f..9bde0d0f23ad0 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1493,12 +1493,7 @@ func CleanUpPullRequest(ctx *context.Context) { func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) { fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch - if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, gitRepo, pr.HeadBranch); err != nil { + 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)) @@ -1513,11 +1508,6 @@ func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *g return } - if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.IssueID, pr.HeadBranch); err != nil { - // Do not fail here as branch has already been deleted - log.Error("DeleteBranch: %v", err) - } - ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName)) } diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 94ceac3f26cb7..bdb0493ae8c04 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -306,7 +306,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { } if pr.Flow == issues_model.PullRequestFlowGithub && scheduledPRM.DeleteBranchAfterMerge { - if err := repo_service.DeletePullRequestHeadBranch(ctx, pr, doer, headGitRepo); err != nil { + if err := repo_service.DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch, pr); err != nil { log.Error("DeletePullRequestHeadBranch: %v", err) } } diff --git a/services/pull/pull.go b/services/pull/pull.go index bab4e49998e15..5d9adf28da75d 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -606,17 +606,9 @@ func (errs errlist) Error() string { return "" } -// RetargetChildrenOnMerge retarget children pull requests on merge if possible -func RetargetChildrenOnMerge(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest) error { - if setting.Repository.PullRequest.RetargetChildrenOnMerge && pr.BaseRepoID == pr.HeadRepoID { - return RetargetBranchPulls(ctx, doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch) - } - return nil -} - // RetargetBranchPulls change target branch for all pull requests whose base branch is the branch // Both branch and targetBranch must be in the same repo (for security reasons) -func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { +func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch string, targetBranch string) error { prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) if err != nil { return err @@ -650,12 +642,14 @@ func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, return err } - prs2, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) - if err != nil { - return err + if !setting.Repository.PullRequest.RetargetChildrenOnMerge { + prs2, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) + if err != nil { + return err + } + prs = append(prs, prs2...) } - prs = append(prs, prs2...) if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { return err } diff --git a/services/repository/branch.go b/services/repository/branch.go index f3d388357bac0..6d10f08558add 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -31,7 +31,6 @@ import ( "code.gitea.io/gitea/modules/util" webhook_module "code.gitea.io/gitea/modules/webhook" notify_service "code.gitea.io/gitea/services/notify" - pull_service "code.gitea.io/gitea/services/pull" files_service "code.gitea.io/gitea/services/repository/files" "xorm.io/builder" @@ -468,7 +467,7 @@ var ( ) // DeleteBranch delete branch -func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error { +func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string, pr *issues_model.PullRequest) error { perm, err := access_model.GetUserRepoPermission(ctx, repo, doer) if err != nil { return err @@ -515,6 +514,12 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R } } + if pr != nil { + if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { + return fmt.Errorf("DeleteBranch: %v", err) + } + } + return gitRepo.DeleteBranch(branchName, git.DeleteBranchOptions{ Force: true, }) @@ -541,23 +546,6 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R return nil } -func DeletePullRequestHeadBranch(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, headGitRepo *git.Repository) error { - if err := pull_service.RetargetChildrenOnMerge(ctx, doer, pr); err != nil { - return err - } - - if err := DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch); err != nil { - return err - } - - if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.IssueID, pr.HeadBranch); err != nil { - // Do not fail here as branch has already been deleted - log.Error("AddDeletePRBranchComment: %v", err) - } - - return nil -} - type BranchSyncOptions struct { RepoID int64 } diff --git a/services/repository/push.go b/services/repository/push.go index 06ad65e48fb6b..07252cb11b386 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -275,6 +275,13 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { } } else { notify_service.DeleteRef(ctx, pusher, repo, opts.RefFullName) + + if setting.Repository.PullRequest.RetargetChildrenOnMerge { + if err := pull_service.RetargetBranchPulls(ctx, pusher, repo.ID, branch, repo.DefaultBranch); err != nil { + return err + } + } + if err = pull_service.CloseBranchPulls(ctx, pusher, repo.ID, branch); err != nil { // close all related pulls log.Error("close related pull request failed: %v", err) From 70b06b8fcc5b80b216f168193987306ca72abc07 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 9 Nov 2024 20:38:37 -0800 Subject: [PATCH 07/18] Fix lint --- services/pull/pull.go | 2 +- tests/integration/actions_trigger_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 43b5f99293143..55d7cbe48343f 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -631,7 +631,7 @@ func (errs errlist) Error() string { // RetargetBranchPulls change target branch for all pull requests whose base branch is the branch // Both branch and targetBranch must be in the same repo (for security reasons) -func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch string, targetBranch string) error { +func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) if err != nil { return err diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 4718aa7e7352c..aa61cc9462c84 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -423,7 +423,7 @@ func TestCreateDeleteRefEvent(t *testing.T) { assert.NotNil(t, run) // delete the branch - err = repo_service.DeleteBranch(db.DefaultContext, user2, repo, gitRepo, "test-create-branch") + err = repo_service.DeleteBranch(db.DefaultContext, user2, repo, gitRepo, "test-create-branch", nil) assert.NoError(t, err) run = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ Title: "add workflow", From ac0a4eae428398f3a28d9b1e9939ff9108ae078d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 9 Nov 2024 22:04:09 -0800 Subject: [PATCH 08/18] Add deleting branch comment --- services/pull/pull.go | 54 ++++++++++++++++++++++++++++--------- services/repository/push.go | 2 +- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 55d7cbe48343f..bba86ab00f5a7 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -658,35 +658,65 @@ func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int6 return nil } -// CloseBranchPulls close all the pull requests who's head branch is the branch -func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch string) error { +// ClosePullsCausedByBranchDeleted close all the pull requests who's head branch is the branch +// Or who's base branch is the branch if setting.Repository.PullRequest.RetargetChildrenOnMerge is true +func ClosePullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User, repoID int64, branch string) error { + // branch as head branch prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repoID, branch) if err != nil { return err } - if !setting.Repository.PullRequest.RetargetChildrenOnMerge { - prs2, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) - if err != nil { - return err - } - prs = append(prs, prs2...) - } - if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { return err } + if err := issues_model.PullRequestList(prs).LoadRepositories(ctx); err != nil { + return err + } var errs errlist for _, pr := range prs { if err = issue_service.ChangeStatus(ctx, pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) } + if err == nil { + if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { + log.Error("AddDeletePRBranchComment: %v", err) + errs = append(errs, err) + } + } } - if len(errs) > 0 { + + if setting.Repository.PullRequest.RetargetChildrenOnMerge { return errs } - return nil + + // branch as base branch + prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) + if err != nil { + return err + } + + if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { + return err + } + if err := issues_model.PullRequestList(prs).LoadRepositories(ctx); err != nil { + return err + } + + errs = nil + for _, pr := range prs { + if err = issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.BaseBranch); err != nil { + log.Error("AddDeletePRBranchComment: %v", err) + errs = append(errs, err) + } + if err == nil { + if err = issue_service.ChangeStatus(ctx, pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { + errs = append(errs, err) + } + } + } + return errs } // CloseRepoBranchesPulls close all pull requests which head branches are in the given repository, but only whose base repo is not in the given repository diff --git a/services/repository/push.go b/services/repository/push.go index 07252cb11b386..ef33200d3e5ed 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -282,7 +282,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { } } - if err = pull_service.CloseBranchPulls(ctx, pusher, repo.ID, branch); err != nil { + if err = pull_service.ClosePullsCausedByBranchDeleted(ctx, pusher, repo.ID, branch); err != nil { // close all related pulls log.Error("close related pull request failed: %v", err) } From a1546971913adc4ba48f4f964d89688c5cd40447 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 9 Nov 2024 22:25:57 -0800 Subject: [PATCH 09/18] Fix lint --- services/pull/pull.go | 7 ++++++- services/repository/push.go | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index bba86ab00f5a7..fb631af08a546 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -629,6 +629,8 @@ func (errs errlist) Error() string { return "" } +var _ error = &errlist{} + // RetargetBranchPulls change target branch for all pull requests whose base branch is the branch // Both branch and targetBranch must be in the same repo (for security reasons) func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { @@ -716,7 +718,10 @@ func ClosePullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User, } } } - return errs + if len(errs) > 0 { + return errs + } + return nil } // CloseRepoBranchesPulls close all pull requests which head branches are in the given repository, but only whose base repo is not in the given repository diff --git a/services/repository/push.go b/services/repository/push.go index ef33200d3e5ed..e18c6685e3356 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -282,7 +282,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { } } - if err = pull_service.ClosePullsCausedByBranchDeleted(ctx, pusher, repo.ID, branch); err != nil { + if err := pull_service.ClosePullsCausedByBranchDeleted(ctx, pusher, repo.ID, branch); err != nil { // close all related pulls log.Error("close related pull request failed: %v", err) } From 6c08cc5bcb45803848ecaca678a086f962e0ccb2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 10 Nov 2024 12:03:16 -0800 Subject: [PATCH 10/18] don't return error even retargetbranch of pull request failed --- services/repository/push.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/repository/push.go b/services/repository/push.go index e18c6685e3356..57cba042deb10 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -278,7 +278,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { if setting.Repository.PullRequest.RetargetChildrenOnMerge { if err := pull_service.RetargetBranchPulls(ctx, pusher, repo.ID, branch, repo.DefaultBranch); err != nil { - return err + log.Error("retargetBranchPulls failed: %v", err) } } From 2dbf22255db681c137c92dca7362f6adf5462e2e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 5 Dec 2024 21:23:51 -0800 Subject: [PATCH 11/18] Fix bug --- models/migrations/migrations.go | 2 +- routers/api/v1/repo/pull.go | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 7eea8e8e7640c..ca5a9df128064 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -369,7 +369,7 @@ func prepareMigrationTasks() []*migration { newMigration(309, "Improve Notification table indices", v1_23.ImproveNotificationTableIndices), newMigration(310, "Add Priority to ProtectedBranch", v1_23.AddPriorityToProtectedBranch), newMigration(311, "Add TimeEstimate to Issue table", v1_23.AddTimeEstimateColumnToIssueTable), - newMigration(310, "Add DeleteBranchAfterMerge to AutoMerge", v1_23.AddDeleteBranchAfterMergeForAutoMerge), + newMigration(312, "Add DeleteBranchAfterMerge to AutoMerge", v1_23.AddDeleteBranchAfterMergeForAutoMerge), } return preparedMigrations } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 1301f954d443d..6fe4c439b61fe 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1084,9 +1084,13 @@ func MergePullRequest(ctx *context.APIContext) { } defer headRepo.Close() } - if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { - ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err) - return + + // TODO: why only retarget same repository pull requests? + if setting.Repository.PullRequest.RetargetChildrenOnMerge && pr.BaseRepoID == pr.HeadRepoID { + if err := pull_service.RetargetBranchPulls(ctx, ctx.Doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch); err != nil { + ctx.Error(http.StatusInternalServerError, "RetargetBranchPulls", err) + return + } } if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch, pr); err != nil { switch { From 16bd6510acdf3f549f781977e9b7c1fd162ad5d0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 5 Dec 2024 21:33:43 -0800 Subject: [PATCH 12/18] Remove unnecessary variable --- services/repository/branch.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index b4a4f6426b4e7..7443c3859c93e 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -462,8 +462,7 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m // enmuerates all branch related errors var ( - ErrBranchIsDefault = errors.New("branch is default") - ErrInsufficientAccess = errors.New("insufficient access") + ErrBranchIsDefault = errors.New("branch is default") ) func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchName string, doer *user_model.User) error { From fd7c953677c0b552a7670cedee07d31e855212fb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 5 Dec 2024 21:58:52 -0800 Subject: [PATCH 13/18] We don't need to duplicated operation when delete branch --- models/issues/pull_list.go | 17 +++++++++++++++++ routers/api/v1/repo/pull.go | 11 ----------- services/pull/pull.go | 21 ++++++++++++++------- services/repository/push.go | 8 +------- 4 files changed, 32 insertions(+), 25 deletions(-) diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index 9155ea0834685..70b6e370164d2 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -165,6 +165,23 @@ func (prs PullRequestList) getRepositoryIDs() []int64 { return repoIDs.Values() } +func (prs PullRequestList) SetBaseRepo(baseRepo *repo_model.Repository) { + for _, pr := range prs { + if pr.BaseRepo == nil { + pr.BaseRepo = baseRepo + } + } +} + +func (prs PullRequestList) SetHeadRepo(headRepo *repo_model.Repository) { + for _, pr := range prs { + if pr.HeadRepo == nil { + pr.HeadRepo = headRepo + pr.isHeadRepoLoaded = true + } + } +} + func (prs PullRequestList) LoadRepositories(ctx context.Context) error { repoIDs := prs.getRepositoryIDs() reposMap := make(map[int64]*repo_model.Repository, len(repoIDs)) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 6fe4c439b61fe..2e7e9a16c3e64 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1085,13 +1085,6 @@ func MergePullRequest(ctx *context.APIContext) { defer headRepo.Close() } - // TODO: why only retarget same repository pull requests? - if setting.Repository.PullRequest.RetargetChildrenOnMerge && pr.BaseRepoID == pr.HeadRepoID { - if err := pull_service.RetargetBranchPulls(ctx, ctx.Doer, pr.HeadRepoID, pr.HeadBranch, pr.BaseBranch); err != nil { - ctx.Error(http.StatusInternalServerError, "RetargetBranchPulls", err) - return - } - } if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch, pr); err != nil { switch { case git.IsErrBranchNotExist(err): @@ -1105,10 +1098,6 @@ func MergePullRequest(ctx *context.APIContext) { } return } - if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { - // Do not fail here as branch has already been deleted - log.Error("DeleteBranch: %v", err) - } } } diff --git a/services/pull/pull.go b/services/pull/pull.go index fb631af08a546..a0987cc70b1db 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -631,9 +631,9 @@ func (errs errlist) Error() string { var _ error = &errlist{} -// RetargetBranchPulls change target branch for all pull requests whose base branch is the branch +// retargetBranchPulls change target branch for all pull requests whose base branch is the branch // Both branch and targetBranch must be in the same repo (for security reasons) -func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { +func retargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) if err != nil { return err @@ -660,11 +660,12 @@ func RetargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int6 return nil } -// ClosePullsCausedByBranchDeleted close all the pull requests who's head branch is the branch -// Or who's base branch is the branch if setting.Repository.PullRequest.RetargetChildrenOnMerge is true -func ClosePullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User, repoID int64, branch string) error { +// AdjustPullsCausedByBranchDeleted close all the pull requests who's head branch is the branch +// Or Close all the plls who's base branch is the branch if setting.Repository.PullRequest.RetargetChildrenOnMerge is false. +// If it's true, Retarget all these pulls to the default branch. +func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, branch string) error { // branch as head branch - prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repoID, branch) + prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repo.ID, branch) if err != nil { return err } @@ -672,6 +673,7 @@ func ClosePullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User, if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { return err } + issues_model.PullRequestList(prs).SetHeadRepo(repo) if err := issues_model.PullRequestList(prs).LoadRepositories(ctx); err != nil { return err } @@ -690,11 +692,15 @@ func ClosePullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User, } if setting.Repository.PullRequest.RetargetChildrenOnMerge { + if err := retargetBranchPulls(ctx, doer, repo.ID, branch, repo.DefaultBranch); err != nil { + log.Error("retargetBranchPulls failed: %v", err) + errs = append(errs, err) + } return errs } // branch as base branch - prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) + prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repo.ID, branch) if err != nil { return err } @@ -702,6 +708,7 @@ func ClosePullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User, if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { return err } + issues_model.PullRequestList(prs).SetBaseRepo(repo) if err := issues_model.PullRequestList(prs).LoadRepositories(ctx); err != nil { return err } diff --git a/services/repository/push.go b/services/repository/push.go index 57cba042deb10..0ea51f9c072d2 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -276,13 +276,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { } else { notify_service.DeleteRef(ctx, pusher, repo, opts.RefFullName) - if setting.Repository.PullRequest.RetargetChildrenOnMerge { - if err := pull_service.RetargetBranchPulls(ctx, pusher, repo.ID, branch, repo.DefaultBranch); err != nil { - log.Error("retargetBranchPulls failed: %v", err) - } - } - - if err := pull_service.ClosePullsCausedByBranchDeleted(ctx, pusher, repo.ID, branch); err != nil { + if err := pull_service.AdjustPullsCausedByBranchDeleted(ctx, pusher, repo, branch); err != nil { // close all related pulls log.Error("close related pull request failed: %v", err) } From 93e8dfdacdb9ad829c5d044860fa352eb38b42a2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 5 Dec 2024 22:55:21 -0800 Subject: [PATCH 14/18] Fix test --- tests/integration/pull_merge_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 480ac4c23330a..4c6b80bd71d64 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -586,6 +586,8 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) { elemChildPR := strings.Split(test.RedirectURL(respChildPR), "/") assert.EqualValues(t, "pulls", elemChildPR[3]) + defer test.MockVariableValue(&setting.Repository.PullRequest.RetargetChildrenOnMerge, false)() + testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true) repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"}) From be5a5ea627b2eb340018dcc780759bca49f31545 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 30 Dec 2024 12:15:39 -0800 Subject: [PATCH 15/18] Fix merge bug --- services/pull/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 277a81dc25a54..ff4062089fbbf 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -742,7 +742,7 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User errs = append(errs, err) } if err == nil { - if err = issue_service.ChangeStatus(ctx, pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { + if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) } } From fa12a2415512b86a01b09b89d64bfe1eb5ce2736 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 30 Dec 2024 16:41:05 -0800 Subject: [PATCH 16/18] Remove unnecessary errlist --- services/pull/pull.go | 43 ++++++++----------------------------------- 1 file changed, 8 insertions(+), 35 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index ff4062089fbbf..fd255e8c9d1bb 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -6,6 +6,7 @@ package pull import ( "bytes" "context" + "errors" "fmt" "io" "os" @@ -635,24 +636,6 @@ func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) { return err } -type errlist []error - -func (errs errlist) Error() string { - if len(errs) > 0 { - var buf strings.Builder - for i, err := range errs { - if i > 0 { - buf.WriteString(", ") - } - buf.WriteString(err.Error()) - } - return buf.String() - } - return "" -} - -var _ error = &errlist{} - // retargetBranchPulls change target branch for all pull requests whose base branch is the branch // Both branch and targetBranch must be in the same repo (for security reasons) func retargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, branch, targetBranch string) error { @@ -665,7 +648,7 @@ func retargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int6 return err } - var errs errlist + var errs []error for _, pr := range prs { if err = pr.Issue.LoadRepo(ctx); err != nil { errs = append(errs, err) @@ -675,11 +658,7 @@ func retargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int6 errs = append(errs, err) } } - - if len(errs) > 0 { - return errs - } - return nil + return errors.Join(errs...) } // AdjustPullsCausedByBranchDeleted close all the pull requests who's head branch is the branch @@ -700,7 +679,7 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User return err } - var errs errlist + var errs []error for _, pr := range prs { if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) @@ -718,7 +697,7 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User log.Error("retargetBranchPulls failed: %v", err) errs = append(errs, err) } - return errs + return errors.Join(errs...) } // branch as base branch @@ -747,10 +726,7 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User } } } - if len(errs) > 0 { - return errs - } - return nil + return errors.Join(errs...) } // CloseRepoBranchesPulls close all pull requests which head branches are in the given repository, but only whose base repo is not in the given repository @@ -760,7 +736,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re return err } - var errs errlist + var errs []error for _, branch := range branches { prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repo.ID, branch.Name) if err != nil { @@ -783,10 +759,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re } } - if len(errs) > 0 { - return errs - } - return nil + return errors.Join(errs...) } var commitMessageTrailersPattern = regexp.MustCompile(`(?:^|\n\n)(?:[\w-]+[ \t]*:[^\n]+\n*(?:[ \t]+[^\n]+\n*)*)+$`) From f549185d77fa0fc853494e8b06c86f0891fbd94e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 30 Dec 2024 20:07:54 -0800 Subject: [PATCH 17/18] Move migration to v1.24 --- models/migrations/migrations.go | 5 ++++- models/migrations/{v1_23 => v1_24}/v312.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) rename models/migrations/{v1_23 => v1_24}/v312.go (94%) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index ca5a9df128064..95364ab705751 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/models/migrations/v1_21" "code.gitea.io/gitea/models/migrations/v1_22" "code.gitea.io/gitea/models/migrations/v1_23" + "code.gitea.io/gitea/models/migrations/v1_24" "code.gitea.io/gitea/models/migrations/v1_6" "code.gitea.io/gitea/models/migrations/v1_7" "code.gitea.io/gitea/models/migrations/v1_8" @@ -369,7 +370,9 @@ func prepareMigrationTasks() []*migration { newMigration(309, "Improve Notification table indices", v1_23.ImproveNotificationTableIndices), newMigration(310, "Add Priority to ProtectedBranch", v1_23.AddPriorityToProtectedBranch), newMigration(311, "Add TimeEstimate to Issue table", v1_23.AddTimeEstimateColumnToIssueTable), - newMigration(312, "Add DeleteBranchAfterMerge to AutoMerge", v1_23.AddDeleteBranchAfterMergeForAutoMerge), + + // Gitea 1.23.0-rc0 ends at migration ID number 311 (database version 312) + newMigration(312, "Add DeleteBranchAfterMerge to AutoMerge", v1_24.AddDeleteBranchAfterMergeForAutoMerge), } return preparedMigrations } diff --git a/models/migrations/v1_23/v312.go b/models/migrations/v1_24/v312.go similarity index 94% rename from models/migrations/v1_23/v312.go rename to models/migrations/v1_24/v312.go index dcb5b98127b6c..9766dc1ccfb63 100644 --- a/models/migrations/v1_23/v312.go +++ b/models/migrations/v1_24/v312.go @@ -1,7 +1,7 @@ // Copyright 2024 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT -package v1_23 //nolint +package v1_24 //nolint import ( "xorm.io/xorm" From 50c2316fecc7fa64c7f107be50969cf0a0fd69d9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 9 Jan 2025 10:43:18 -0800 Subject: [PATCH 18/18] Fix merge bug --- services/pull/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 0bea1b96d8e87..5d3758eca6d14 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -722,7 +722,7 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User errs = append(errs, err) } if err == nil { - if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { + if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) } }