Add checkbox to delete pull branch after successful merge (go-gitea#1…

…6049)

* 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>

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.

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.

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)

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)
 }
 

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)
 			}

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
+	var gitBaseRepo *git.Repository
+
+	// Assume that the base repo is the current context (almost certainly)
+	if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.BaseRepoID && ctx.Repo.GitRepo != nil {
+		gitBaseRepo = ctx.Repo.GitRepo
+	} else {
+		// If not just open it
+		gitBaseRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath())
+		if err != nil {
+			ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.RepoPath()), err)
+			return
+		}
+		defer gitBaseRepo.Close()
 	}
-	defer gitRepo.Close()
 
-	gitBaseRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
-	if err != nil {
-		ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.RepoPath()), err)
-		return
+	// 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)
 	}

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() {

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

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 {

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()

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() {

templates/repo/issue/view_content/pull.tmpl

@@ -315,6 +315,12 @@
 									<button class="ui button merge-cancel">
 										{{$.i18n.Tr "cancel"}}
 									</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>
 							</div>
 							{{end}}
@@ -328,6 +334,12 @@
 									<button class="ui button merge-cancel">
 										{{$.i18n.Tr "cancel"}}
 									</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>
 							</div>
 							{{end}}
@@ -347,6 +359,12 @@
 									<button class="ui button merge-cancel">
 										{{$.i18n.Tr "cancel"}}
 									</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>
 							</div>
 							{{end}}
@@ -366,6 +384,12 @@
 									<button class="ui button merge-cancel">
 										{{$.i18n.Tr "cancel"}}
 									</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>
 							</div>
 							{{end}}
@@ -382,6 +406,12 @@
 										<button class="ui button merge-cancel">
 											{{$.i18n.Tr "cancel"}}
 										</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>
 								</div>
 							{{end}}

templates/repo/settings/options.tmpl

@@ -445,6 +445,12 @@
 								<label>{{.i18n.Tr "repo.settings.pulls.enable_autodetect_manual_merge"}}</label>
 							</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">
 							<p>
 								{{.i18n.Tr "repo.settings.default_merge_style_desc"}}