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 <art27@cantab.net>
This commit is contained in:
Jimmy Praet 2021-07-13 01:26:25 +02:00 committed by GitHub
parent 46a4c6835d
commit 78118a3b02
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 182 additions and 44 deletions

View File

@ -98,6 +98,7 @@ type PullRequestsConfig struct {
AllowSquash bool AllowSquash bool
AllowManualMerge bool AllowManualMerge bool
AutodetectManualMerge bool AutodetectManualMerge bool
DefaultDeleteBranchAfterMerge bool
DefaultMergeStyle MergeStyle DefaultMergeStyle MergeStyle
} }

View File

@ -172,6 +172,8 @@ type EditRepoOption struct {
AllowManualMerge *bool `json:"allow_manual_merge,omitempty"` 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. // 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"` 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`. // 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"` DefaultMergeStyle *string `json:"default_merge_style,omitempty"`
// set to `true` to archive this repository. // set to `true` to archive this repository.

View File

@ -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_squash_commits = Enable Squashing to Merge Commits
settings.pulls.allow_manual_merge = Enable Mark PR as manually merged 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.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.projects_desc = Enable Repository Projects
settings.admin_settings = Administrator Settings settings.admin_settings = Administrator Settings
settings.admin_enable_health_check = Enable Repository Health Checks (git fsck) settings.admin_enable_health_check = Enable Repository Health Checks (git fsck)

View File

@ -5,6 +5,7 @@
package repo package repo
import ( import (
"errors"
"fmt" "fmt"
"math" "math"
"net/http" "net/http"
@ -25,6 +26,7 @@ import (
"code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/forms"
issue_service "code.gitea.io/gitea/services/issue" issue_service "code.gitea.io/gitea/services/issue"
pull_service "code.gitea.io/gitea/services/pull" pull_service "code.gitea.io/gitea/services/pull"
repo_service "code.gitea.io/gitea/services/repository"
) )
// ListPullRequests returns a list of all PRs // ListPullRequests returns a list of all PRs
@ -878,6 +880,38 @@ func MergePullRequest(ctx *context.APIContext) {
} }
log.Trace("Pull request merged: %d", pr.ID) 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) ctx.Status(http.StatusOK)
} }

View File

@ -840,6 +840,7 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
AllowSquash: true, AllowSquash: true,
AllowManualMerge: true, AllowManualMerge: true,
AutodetectManualMerge: false, AutodetectManualMerge: false,
DefaultDeleteBranchAfterMerge: false,
DefaultMergeStyle: models.MergeStyleMerge, DefaultMergeStyle: models.MergeStyleMerge,
} }
} else { } else {
@ -867,6 +868,9 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
if opts.AutodetectManualMerge != nil { if opts.AutodetectManualMerge != nil {
config.AutodetectManualMerge = *opts.AutodetectManualMerge config.AutodetectManualMerge = *opts.AutodetectManualMerge
} }
if opts.DefaultDeleteBranchAfterMerge != nil {
config.DefaultDeleteBranchAfterMerge = *opts.DefaultDeleteBranchAfterMerge
}
if opts.DefaultMergeStyle != nil { if opts.DefaultMergeStyle != nil {
config.DefaultMergeStyle = models.MergeStyle(*opts.DefaultMergeStyle) config.DefaultMergeStyle = models.MergeStyle(*opts.DefaultMergeStyle)
} }

View File

@ -965,6 +965,22 @@ func MergePullRequest(ctx *context.Context) {
} }
log.Trace("Pull request merged: %d", pr.ID) 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)) 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 fullBranchName := pr.HeadRepo.Owner.Name + "/" + pr.HeadBranch
gitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) var gitBaseRepo *git.Repository
if err != nil {
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.RepoPath()), err)
return
}
defer gitRepo.Close()
gitBaseRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) // 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 { if err != nil {
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.RepoPath()), err) ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.RepoPath()), err)
return return
} }
defer gitBaseRepo.Close() 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 func() { defer func() {
ctx.JSON(http.StatusOK, map[string]interface{}{ ctx.JSON(http.StatusOK, map[string]interface{}{
@ -1208,6 +1240,11 @@ func CleanUpPullRequest(ctx *context.Context) {
return 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 { if err := repo_service.DeleteBranch(ctx.User, pr.HeadRepo, gitRepo, pr.HeadBranch); err != nil {
switch { switch {
case git.IsErrBranchNotExist(err): case git.IsErrBranchNotExist(err):
@ -1223,7 +1260,7 @@ func CleanUpPullRequest(ctx *context.Context) {
return 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 // Do not fail here as branch has already been deleted
log.Error("DeleteBranch: %v", err) log.Error("DeleteBranch: %v", err)
} }

View File

@ -423,6 +423,7 @@ func SettingsPost(ctx *context.Context) {
AllowSquash: form.PullsAllowSquash, AllowSquash: form.PullsAllowSquash,
AllowManualMerge: form.PullsAllowManualMerge, AllowManualMerge: form.PullsAllowManualMerge,
AutodetectManualMerge: form.EnableAutodetectManualMerge, AutodetectManualMerge: form.EnableAutodetectManualMerge,
DefaultDeleteBranchAfterMerge: form.DefaultDeleteBranchAfterMerge,
DefaultMergeStyle: models.MergeStyle(form.PullsDefaultMergeStyle), DefaultMergeStyle: models.MergeStyle(form.PullsDefaultMergeStyle),
}, },
}) })

View File

@ -151,6 +151,7 @@ type RepoSettingForm struct {
PullsAllowManualMerge bool PullsAllowManualMerge bool
PullsDefaultMergeStyle string PullsDefaultMergeStyle string
EnableAutodetectManualMerge bool EnableAutodetectManualMerge bool
DefaultDeleteBranchAfterMerge bool
EnableTimetracker bool EnableTimetracker bool
AllowOnlyContributorsToTrackTime bool AllowOnlyContributorsToTrackTime bool
EnableIssueDependencies bool EnableIssueDependencies bool
@ -556,6 +557,7 @@ type MergePullRequestForm struct {
MergeMessageField string MergeMessageField string
MergeCommitID string // only used for manually-merged MergeCommitID string // only used for manually-merged
ForceMerge *bool `json:"force_merge,omitempty"` ForceMerge *bool `json:"force_merge,omitempty"`
DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"`
} }
// Validate validates the fields // Validate validates the fields

View File

@ -303,7 +303,11 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy
for _, pr := range prs { for _, pr := range prs {
divergence, err := GetDiverging(pr) divergence, err := GetDiverging(pr)
if err != nil { if err != nil {
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) log.Error("GetDiverging: %v", err)
}
} else { } else {
err = pr.UpdateCommitDivergence(divergence.Ahead, divergence.Behind) err = pr.UpdateCommitDivergence(divergence.Ahead, divergence.Behind)
if err != nil { if err != nil {

View File

@ -141,10 +141,15 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) {
trackingBranch := "tracking" trackingBranch := "tracking"
// Fetch head branch // Fetch head branch
if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, git.BranchPrefix+pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { 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 { if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) 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()) 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() outbuf.Reset()

View File

@ -88,7 +88,9 @@ func GetDiverging(pr *models.PullRequest) (*git.DivergeObject, error) {
tmpRepo, err := createTemporaryRepo(pr) tmpRepo, err := createTemporaryRepo(pr)
if err != nil { if err != nil {
log.Error("CreateTemporaryPath: %v", err) if !models.IsErrBranchDoesNotExist(err) {
log.Error("CreateTemporaryRepo: %v", err)
}
return nil, err return nil, err
} }
defer func() { defer func() {

View File

@ -315,6 +315,12 @@
<button class="ui button merge-cancel"> <button class="ui button merge-cancel">
{{$.i18n.Tr "cancel"}} {{$.i18n.Tr "cancel"}}
</button> </button>
{{if .IsPullBranchDeletable}}
<div class="ui checkbox ml-2">
<input name="delete_branch_after_merge" type="checkbox" {{if $prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}}checked{{end}}>
<label>{{$.i18n.Tr "repo.branch.delete" .HeadTarget}}</label>
</div>
{{end}}
</form> </form>
</div> </div>
{{end}} {{end}}
@ -328,6 +334,12 @@
<button class="ui button merge-cancel"> <button class="ui button merge-cancel">
{{$.i18n.Tr "cancel"}} {{$.i18n.Tr "cancel"}}
</button> </button>
{{if .IsPullBranchDeletable}}
<div class="ui checkbox ml-2">
<input name="delete_branch_after_merge" type="checkbox" {{if $prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}}checked{{end}}>
<label>{{$.i18n.Tr "repo.branch.delete" .HeadTarget}}</label>
</div>
{{end}}
</form> </form>
</div> </div>
{{end}} {{end}}
@ -347,6 +359,12 @@
<button class="ui button merge-cancel"> <button class="ui button merge-cancel">
{{$.i18n.Tr "cancel"}} {{$.i18n.Tr "cancel"}}
</button> </button>
{{if .IsPullBranchDeletable}}
<div class="ui checkbox ml-2">
<input name="delete_branch_after_merge" type="checkbox" {{if $prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}}checked{{end}}>
<label>{{$.i18n.Tr "repo.branch.delete" .HeadTarget}}</label>
</div>
{{end}}
</form> </form>
</div> </div>
{{end}} {{end}}
@ -366,6 +384,12 @@
<button class="ui button merge-cancel"> <button class="ui button merge-cancel">
{{$.i18n.Tr "cancel"}} {{$.i18n.Tr "cancel"}}
</button> </button>
{{if .IsPullBranchDeletable}}
<div class="ui checkbox ml-2">
<input name="delete_branch_after_merge" type="checkbox" {{if $prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}}checked{{end}}>
<label>{{$.i18n.Tr "repo.branch.delete" .HeadTarget}}</label>
</div>
{{end}}
</form> </form>
</div> </div>
{{end}} {{end}}
@ -382,6 +406,12 @@
<button class="ui button merge-cancel"> <button class="ui button merge-cancel">
{{$.i18n.Tr "cancel"}} {{$.i18n.Tr "cancel"}}
</button> </button>
{{if .IsPullBranchDeletable}}
<div class="ui checkbox ml-2">
<input name="delete_branch_after_merge" type="checkbox" {{if $prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}}checked{{end}}>
<label>{{$.i18n.Tr "repo.branch.delete" .HeadTarget}}</label>
</div>
{{end}}
</form> </form>
</div> </div>
{{end}} {{end}}

View File

@ -445,6 +445,12 @@
<label>{{.i18n.Tr "repo.settings.pulls.enable_autodetect_manual_merge"}}</label> <label>{{.i18n.Tr "repo.settings.pulls.enable_autodetect_manual_merge"}}</label>
</div> </div>
</div> </div>
<div class="field">
<div class="ui checkbox">
<input name="default_delete_branch_after_merge" type="checkbox" {{if or (not $pullRequestEnabled) ($prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge)}}checked{{end}}>
<label>{{.i18n.Tr "repo.settings.pulls.default_delete_branch_after_merge"}}</label>
</div>
</div>
<div class="field"> <div class="field">
<p> <p>
{{.i18n.Tr "repo.settings.default_merge_style_desc"}} {{.i18n.Tr "repo.settings.default_merge_style_desc"}}

View File

@ -14058,6 +14058,11 @@
"type": "string", "type": "string",
"x-go-name": "DefaultBranch" "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": { "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`.", "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", "type": "string",
@ -15137,6 +15142,10 @@
"MergeTitleField": { "MergeTitleField": {
"type": "string" "type": "string"
}, },
"delete_branch_after_merge": {
"type": "boolean",
"x-go-name": "DeleteBranchAfterMerge"
},
"force_merge": { "force_merge": {
"type": "boolean", "type": "boolean",
"x-go-name": "ForceMerge" "x-go-name": "ForceMerge"