From ccb1087009286f33757c65b2060d65f8f228c7e4 Mon Sep 17 00:00:00 2001 From: Tom Neuber Date: Mon, 21 Oct 2024 21:21:50 +0200 Subject: [PATCH] Add branch auto deletion for scheduled PRs --- models/forgejo_migrations/migrate.go | 2 ++ models/forgejo_migrations/v23.go | 16 ++++++++++ models/pull/automerge.go | 26 +++++++++------- routers/api/v1/repo/pull.go | 2 +- routers/private/hook_post_receive_test.go | 2 +- routers/web/repo/pull.go | 2 +- services/automerge/automerge.go | 16 ++++++++-- services/repository/branch.go | 31 +++++++++++++++++++ .../integration/actions_commit_status_test.go | 2 +- tests/integration/pull_merge_test.go | 12 +++---- .../js/components/PullRequestMergeForm.vue | 2 +- 11 files changed, 88 insertions(+), 25 deletions(-) create mode 100644 models/forgejo_migrations/v23.go diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index cca83d6b4d..116579eeff 100644 --- a/models/forgejo_migrations/migrate.go +++ b/models/forgejo_migrations/migrate.go @@ -82,6 +82,8 @@ var migrations = []*Migration{ NewMigration("Add SSH keypair to `pull_mirror` table", AddSSHKeypairToPushMirror), // v22 -> v23 NewMigration("Add `legacy` to `web_authn_credential` table", AddLegacyToWebAuthnCredential), + // v23 -> v24 + NewMigration("Add `delete_branch_after_merge` to `auto_merge` table", AddDeleteBranchAfterMergeToAutoMerge), } // GetCurrentDBVersion returns the current Forgejo database version. diff --git a/models/forgejo_migrations/v23.go b/models/forgejo_migrations/v23.go new file mode 100644 index 0000000000..20a916a716 --- /dev/null +++ b/models/forgejo_migrations/v23.go @@ -0,0 +1,16 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package forgejo_migrations //nolint:revive + +import "xorm.io/xorm" + +// AddDeleteBranchAfterMergeToAutoMerge: add DeleteBranchAfterMerge column, setting existing rows to false +func AddDeleteBranchAfterMergeToAutoMerge(x *xorm.Engine) error { + type AutoMerge struct { + ID int64 `xorm:"pk autoincr"` + DeleteBranchAfterMerge bool `xorm:"NOT NULL DEFAULT false"` + } + + return x.Sync(&AutoMerge{}) +} diff --git a/models/pull/automerge.go b/models/pull/automerge.go index f31159a8d8..d739d5a145 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 `xorm:"NOT NULL DEFAULT false"` + 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, deleteBranch 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: deleteBranch, }) return err } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 748ef18802..263db7e1e8 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -988,7 +988,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/private/hook_post_receive_test.go b/routers/private/hook_post_receive_test.go index bfd647e365..df4d739ce7 100644 --- a/routers/private/hook_post_receive_test.go +++ b/routers/private/hook_post_receive_test.go @@ -28,7 +28,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", true) require.NoError(t, err) autoMerge := unittest.AssertExistsAndLoadBean(t, &pull_model.AutoMerge{PullID: pr.ID}) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index bc85012700..6404f887ee 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1302,7 +1302,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 fcc013348c..0870a21c0d 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -22,6 +22,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" shared_automerge "code.gitea.io/gitea/services/shared/automerge" ) @@ -52,9 +53,9 @@ func handler(items ...string) []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, deleteBranch 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, deleteBranch); err != nil { return err } scheduled = true @@ -210,4 +211,15 @@ 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 scheduledPRM.DeleteBranchAfterMerge { + deleted, err := repo_service.DeleteBranchIfUnused(ctx, pr, headGitRepo, doer) + if err != nil { + if deleted { + log.Info("%d branch deleted but got error during comment creation: %v", pr.ID, err) + } else { + log.Error("%d repo_service.DeleteBranchIfUnused: %v", pr.ID, err) + } + } + } } diff --git a/services/repository/branch.go b/services/repository/branch.go index f0e7120926..098221e5bf 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -26,6 +26,7 @@ import ( "code.gitea.io/gitea/modules/timeutil" 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" @@ -475,6 +476,36 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R return nil } +func DeleteBranchIfUnused(ctx context.Context, pr *issues_model.PullRequest, repo *git.Repository, doer *user_model.User) (bool, error) { + // 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 { + log.Error("HasUnmergedPullRequestsByHeadInfo", err) + return false, err + } + if exist { + log.Info("%d was scheduled to automerge with branch deletion but branch is still used", pr.ID) + return false, nil + } + + if err := pull_service.RetargetChildrenOnMerge(ctx, doer, pr); err != nil { + log.Error("RetargetChildrenOnMerge", err) + return false, err + } + + if err := DeleteBranch(ctx, doer, pr.HeadRepo, repo, pr.HeadBranch); err != nil { + log.Error("DeleteBranch: %v", err) + return false, err + } + + if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { + log.Error("AddDeletePRBranchComment: %v", err) + return true, err + } + + return true, nil +} + type BranchSyncOptions struct { RepoID int64 } diff --git a/tests/integration/actions_commit_status_test.go b/tests/integration/actions_commit_status_test.go index ace0fbd323..4d33989c5d 100644 --- a/tests/integration/actions_commit_status_test.go +++ b/tests/integration/actions_commit_status_test.go @@ -34,7 +34,7 @@ func TestActionsAutomerge(t *testing.T) { assert.False(t, pr.HasMerged, "PR should not be merged") assert.Equal(t, issues_model.PullRequestStatusMergeable, pr.Status, "PR should be mergeable") - scheduled, err := automerge.ScheduleAutoMerge(ctx, user, pr, repo_model.MergeStyleMerge, "Dummy") + scheduled, err := automerge.ScheduleAutoMerge(ctx, user, pr, repo_model.MergeStyleMerge, "Dummy", true) require.NoError(t, err, "PR should be scheduled for automerge") assert.True(t, scheduled, "PR should be scheduled for automerge") diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 0f25bdebac..28f84eea80 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -880,12 +880,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", true) require.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", true) require.Error(t, err) assert.False(t, scheduled) @@ -964,12 +964,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", true) require.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", true) require.Error(t, err) assert.False(t, scheduled) @@ -1093,12 +1093,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", true) require.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", true) require.Error(t, err) assert.False(t, scheduled) diff --git a/web_src/js/components/PullRequestMergeForm.vue b/web_src/js/components/PullRequestMergeForm.vue index bd0901a7b5..46bda3a025 100644 --- a/web_src/js/components/PullRequestMergeForm.vue +++ b/web_src/js/components/PullRequestMergeForm.vue @@ -130,7 +130,7 @@ export default { {{ mergeForm.textCancel }} -
+