From 78118a3b029ee4eb140d47be22e86df17253a786 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Tue, 13 Jul 2021 01:26:25 +0200 Subject: [PATCH] Add checkbox to delete pull branch after successful merge (#16049) * Add checkbox to delete pull branch after successful merge * Omit DeleteBranchAfterMerge field in json * Log a warning instead of error when PR head branch deleted * Add DefaultDeleteBranchAfterMerge to PullRequestConfig * Add support for delete_branch_after_merge via API * Fix for API: the branch should be deleted from the HEAD repo If head and base repo are the same, reuse the already opened ctx.Repo.GitRepo * Don't delegate to CleanupBranch, only reuse branch deletion code CleanupBranch contains too much logic that has already been performed by the Merge * Reuse gitrepo in MergePullRequest Co-authored-by: Andrew Thornton --- models/repo_unit.go | 17 +++--- modules/structs/repo.go | 2 + options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/pull.go | 34 ++++++++++++ routers/api/v1/repo/repo.go | 20 ++++--- routers/web/repo/pull.go | 61 +++++++++++++++++---- routers/web/repo/setting.go | 17 +++--- services/forms/repo_form.go | 12 ++-- services/pull/pull.go | 6 +- services/pull/temp_repo.go | 7 ++- services/pull/update.go | 4 +- templates/repo/issue/view_content/pull.tmpl | 30 ++++++++++ templates/repo/settings/options.tmpl | 6 ++ templates/swagger/v1_json.tmpl | 9 +++ 14 files changed, 182 insertions(+), 44 deletions(-) diff --git a/models/repo_unit.go b/models/repo_unit.go index d8060d16a..a12e056a7 100644 --- a/models/repo_unit.go +++ b/models/repo_unit.go @@ -91,14 +91,15 @@ func (cfg *IssuesConfig) ToDB() ([]byte, error) { // PullRequestsConfig describes pull requests config type PullRequestsConfig struct { - IgnoreWhitespaceConflicts bool - AllowMerge bool - AllowRebase bool - AllowRebaseMerge bool - AllowSquash bool - AllowManualMerge bool - AutodetectManualMerge bool - DefaultMergeStyle MergeStyle + IgnoreWhitespaceConflicts bool + AllowMerge bool + AllowRebase bool + AllowRebaseMerge bool + AllowSquash bool + AllowManualMerge bool + AutodetectManualMerge bool + DefaultDeleteBranchAfterMerge bool + DefaultMergeStyle MergeStyle } // FromDB fills up a PullRequestsConfig from serialized format. diff --git a/modules/structs/repo.go b/modules/structs/repo.go index cef864c02..2089f4d69 100644 --- a/modules/structs/repo.go +++ b/modules/structs/repo.go @@ -172,6 +172,8 @@ type EditRepoOption struct { AllowManualMerge *bool `json:"allow_manual_merge,omitempty"` // either `true` to enable AutodetectManualMerge, or `false` to prevent it. `has_pull_requests` must be `true`, Note: In some special cases, misjudgments can occur. AutodetectManualMerge *bool `json:"autodetect_manual_merge,omitempty"` + // set to `true` to delete pr branch after merge by default + DefaultDeleteBranchAfterMerge *bool `json:"default_delete_branch_after_merge,omitempty"` // set to a merge style to be used by this repository: "merge", "rebase", "rebase-merge", or "squash". `has_pull_requests` must be `true`. DefaultMergeStyle *string `json:"default_merge_style,omitempty"` // set to `true` to archive this repository. diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 191cb5de6..c0ea28172 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1664,6 +1664,7 @@ settings.pulls.allow_rebase_merge_commit = Enable Rebasing with explicit merge c settings.pulls.allow_squash_commits = Enable Squashing to Merge Commits settings.pulls.allow_manual_merge = Enable Mark PR as manually merged settings.pulls.enable_autodetect_manual_merge = Enable autodetect manual merge (Note: In some special cases, misjudgments can occur) +settings.pulls.default_delete_branch_after_merge = Delete pull request branch after merge by default settings.projects_desc = Enable Repository Projects settings.admin_settings = Administrator Settings settings.admin_enable_health_check = Enable Repository Health Checks (git fsck) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 0c09a9a86..66bcabfd3 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -5,6 +5,7 @@ package repo import ( + "errors" "fmt" "math" "net/http" @@ -25,6 +26,7 @@ import ( "code.gitea.io/gitea/services/forms" issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" + repo_service "code.gitea.io/gitea/services/repository" ) // ListPullRequests returns a list of all PRs @@ -878,6 +880,38 @@ func MergePullRequest(ctx *context.APIContext) { } log.Trace("Pull request merged: %d", pr.ID) + + if form.DeleteBranchAfterMerge { + 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 = git.OpenRepository(pr.HeadRepo.RepoPath()) + if err != nil { + ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.RepoPath()), err) + return + } + defer headRepo.Close() + } + if err := repo_service.DeleteBranch(ctx.User, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil { + switch { + case git.IsErrBranchNotExist(err): + ctx.NotFound(err) + case errors.Is(err, repo_service.ErrBranchIsDefault): + ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch")) + case errors.Is(err, repo_service.ErrBranchIsProtected): + ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected")) + default: + ctx.Error(http.StatusInternalServerError, "DeleteBranch", err) + } + return + } + if err := models.AddDeletePRBranchComment(ctx.User, 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/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 5d397191a..77691b4d1 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -833,14 +833,15 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { if err != nil { // Unit type doesn't exist so we make a new config file with default values config = &models.PullRequestsConfig{ - IgnoreWhitespaceConflicts: false, - AllowMerge: true, - AllowRebase: true, - AllowRebaseMerge: true, - AllowSquash: true, - AllowManualMerge: true, - AutodetectManualMerge: false, - DefaultMergeStyle: models.MergeStyleMerge, + IgnoreWhitespaceConflicts: false, + AllowMerge: true, + AllowRebase: true, + AllowRebaseMerge: true, + AllowSquash: true, + AllowManualMerge: true, + AutodetectManualMerge: false, + DefaultDeleteBranchAfterMerge: false, + DefaultMergeStyle: models.MergeStyleMerge, } } else { config = unit.PullRequestsConfig() @@ -867,6 +868,9 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { if opts.AutodetectManualMerge != nil { config.AutodetectManualMerge = *opts.AutodetectManualMerge } + if opts.DefaultDeleteBranchAfterMerge != nil { + config.DefaultDeleteBranchAfterMerge = *opts.DefaultDeleteBranchAfterMerge + } if opts.DefaultMergeStyle != nil { config.DefaultMergeStyle = models.MergeStyle(*opts.DefaultMergeStyle) } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index e5554e966..a29979964 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -965,6 +965,22 @@ func MergePullRequest(ctx *context.Context) { } log.Trace("Pull request merged: %d", pr.ID) + + if form.DeleteBranchAfterMerge { + 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 = git.OpenRepository(pr.HeadRepo.RepoPath()) + if err != nil { + ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.RepoPath()), err) + return + } + defer headRepo.Close() + } + deleteBranch(ctx, pr, headRepo) + } + ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + fmt.Sprint(pr.Index)) } @@ -1170,19 +1186,35 @@ func CleanUpPullRequest(ctx *context.Context) { fullBranchName := pr.HeadRepo.Owner.Name + "/" + pr.HeadBranch - gitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.RepoPath()), err) - return - } - defer gitRepo.Close() + var gitBaseRepo *git.Repository - gitBaseRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.RepoPath()), err) - return + // 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 = git.OpenRepository(pr.BaseRepo.RepoPath()) + if err != nil { + ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.RepoPath()), 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 = git.OpenRepository(pr.HeadRepo.RepoPath()) + if err != nil { + ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.RepoPath()), err) + return + } + defer gitRepo.Close() } - defer gitBaseRepo.Close() defer func() { ctx.JSON(http.StatusOK, map[string]interface{}{ @@ -1208,6 +1240,11 @@ func CleanUpPullRequest(ctx *context.Context) { return } + deleteBranch(ctx, pr, gitRepo) +} + +func deleteBranch(ctx *context.Context, pr *models.PullRequest, gitRepo *git.Repository) { + fullBranchName := pr.HeadRepo.Owner.Name + "/" + pr.HeadBranch if err := repo_service.DeleteBranch(ctx.User, pr.HeadRepo, gitRepo, pr.HeadBranch); err != nil { switch { case git.IsErrBranchNotExist(err): @@ -1223,7 +1260,7 @@ func CleanUpPullRequest(ctx *context.Context) { return } - if err := models.AddDeletePRBranchComment(ctx.User, pr.BaseRepo, issue.ID, pr.HeadBranch); err != nil { + if err := models.AddDeletePRBranchComment(ctx.User, pr.BaseRepo, pr.IssueID, pr.HeadBranch); err != nil { // Do not fail here as branch has already been deleted log.Error("DeleteBranch: %v", err) } diff --git a/routers/web/repo/setting.go b/routers/web/repo/setting.go index 5e8c2c527..0a84f15bf 100644 --- a/routers/web/repo/setting.go +++ b/routers/web/repo/setting.go @@ -416,14 +416,15 @@ func SettingsPost(ctx *context.Context) { RepoID: repo.ID, Type: models.UnitTypePullRequests, Config: &models.PullRequestsConfig{ - IgnoreWhitespaceConflicts: form.PullsIgnoreWhitespace, - AllowMerge: form.PullsAllowMerge, - AllowRebase: form.PullsAllowRebase, - AllowRebaseMerge: form.PullsAllowRebaseMerge, - AllowSquash: form.PullsAllowSquash, - AllowManualMerge: form.PullsAllowManualMerge, - AutodetectManualMerge: form.EnableAutodetectManualMerge, - DefaultMergeStyle: models.MergeStyle(form.PullsDefaultMergeStyle), + IgnoreWhitespaceConflicts: form.PullsIgnoreWhitespace, + AllowMerge: form.PullsAllowMerge, + AllowRebase: form.PullsAllowRebase, + AllowRebaseMerge: form.PullsAllowRebaseMerge, + AllowSquash: form.PullsAllowSquash, + AllowManualMerge: form.PullsAllowManualMerge, + AutodetectManualMerge: form.EnableAutodetectManualMerge, + DefaultDeleteBranchAfterMerge: form.DefaultDeleteBranchAfterMerge, + DefaultMergeStyle: models.MergeStyle(form.PullsDefaultMergeStyle), }, }) } else if !models.UnitTypePullRequests.UnitGlobalDisabled() { diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 71a83a8be..7c79c4dc2 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -151,6 +151,7 @@ type RepoSettingForm struct { PullsAllowManualMerge bool PullsDefaultMergeStyle string EnableAutodetectManualMerge bool + DefaultDeleteBranchAfterMerge bool EnableTimetracker bool AllowOnlyContributorsToTrackTime bool EnableIssueDependencies bool @@ -551,11 +552,12 @@ func (f *InitializeLabelsForm) Validate(req *http.Request, errs binding.Errors) type MergePullRequestForm struct { // required: true // enum: merge,rebase,rebase-merge,squash,manually-merged - Do string `binding:"Required;In(merge,rebase,rebase-merge,squash,manually-merged)"` - MergeTitleField string - MergeMessageField string - MergeCommitID string // only used for manually-merged - ForceMerge *bool `json:"force_merge,omitempty"` + Do string `binding:"Required;In(merge,rebase,rebase-merge,squash,manually-merged)"` + MergeTitleField string + MergeMessageField string + MergeCommitID string // only used for manually-merged + ForceMerge *bool `json:"force_merge,omitempty"` + DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"` } // Validate validates the fields diff --git a/services/pull/pull.go b/services/pull/pull.go index db216ddbf..6b3acd200 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -303,7 +303,11 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy for _, pr := range prs { divergence, err := GetDiverging(pr) if err != nil { - log.Error("GetDiverging: %v", err) + if models.IsErrBranchDoesNotExist(err) && !git.IsBranchExist(pr.HeadRepo.RepoPath(), pr.HeadBranch) { + log.Warn("Cannot test PR %s/%d: head_branch %s no longer exists", pr.BaseRepo.Name, pr.IssueID, pr.HeadBranch) + } else { + log.Error("GetDiverging: %v", err) + } } else { err = pr.UpdateCommitDivergence(divergence.Ahead, divergence.Behind) if err != nil { diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 45cd10b65..19b488790 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -141,10 +141,15 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) { trackingBranch := "tracking" // Fetch head branch if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, git.BranchPrefix+pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { - log.Error("Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) } + if !git.IsBranchExist(pr.HeadRepo.RepoPath(), pr.HeadBranch) { + return "", models.ErrBranchDoesNotExist{ + BranchName: pr.HeadBranch, + } + } + log.Error("Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("Unable to fetch head_repo head branch [%s:%s -> tracking in tmpBasePath]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, err, outbuf.String(), errbuf.String()) } outbuf.Reset() diff --git a/services/pull/update.go b/services/pull/update.go index f4f7859a4..f35e47cbf 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -88,7 +88,9 @@ func GetDiverging(pr *models.PullRequest) (*git.DivergeObject, error) { tmpRepo, err := createTemporaryRepo(pr) if err != nil { - log.Error("CreateTemporaryPath: %v", err) + if !models.IsErrBranchDoesNotExist(err) { + log.Error("CreateTemporaryRepo: %v", err) + } return nil, err } defer func() { diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 3bdec4bec..fcb3597ae 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -315,6 +315,12 @@ + {{if .IsPullBranchDeletable}} +
+ + +
+ {{end}} {{end}} @@ -328,6 +334,12 @@ + {{if .IsPullBranchDeletable}} +
+ + +
+ {{end}} {{end}} @@ -347,6 +359,12 @@ + {{if .IsPullBranchDeletable}} +
+ + +
+ {{end}} {{end}} @@ -366,6 +384,12 @@ + {{if .IsPullBranchDeletable}} +
+ + +
+ {{end}} {{end}} @@ -382,6 +406,12 @@ + {{if .IsPullBranchDeletable}} +
+ + +
+ {{end}} {{end}} diff --git a/templates/repo/settings/options.tmpl b/templates/repo/settings/options.tmpl index eb76a3b72..054df7c36 100644 --- a/templates/repo/settings/options.tmpl +++ b/templates/repo/settings/options.tmpl @@ -445,6 +445,12 @@ +
+
+ + +
+

{{.i18n.Tr "repo.settings.default_merge_style_desc"}} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 669e3552c..de61b9dd2 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14058,6 +14058,11 @@ "type": "string", "x-go-name": "DefaultBranch" }, + "default_delete_branch_after_merge": { + "description": "set to `true` to delete pr branch after merge by default", + "type": "boolean", + "x-go-name": "DefaultDeleteBranchAfterMerge" + }, "default_merge_style": { "description": "set to a merge style to be used by this repository: \"merge\", \"rebase\", \"rebase-merge\", or \"squash\". `has_pull_requests` must be `true`.", "type": "string", @@ -15137,6 +15142,10 @@ "MergeTitleField": { "type": "string" }, + "delete_branch_after_merge": { + "type": "boolean", + "x-go-name": "DeleteBranchAfterMerge" + }, "force_merge": { "type": "boolean", "x-go-name": "ForceMerge"