From 753a50bb59700e0ea1f788af553b1992f2131337 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Wed, 2 Jun 2021 09:02:57 +0200 Subject: [PATCH 01/10] Add checkbox to delete pull branch after successful merge --- routers/repo/pull.go | 21 +++++++++++---- services/forms/repo_form.go | 11 ++++---- templates/repo/issue/view_content/pull.tmpl | 30 +++++++++++++++++++++ 3 files changed, 52 insertions(+), 10 deletions(-) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index bb166c68a60d..5aef13cbad1e 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -961,6 +961,11 @@ func MergePullRequest(ctx *context.Context) { } log.Trace("Pull request merged: %d", pr.ID) + + if form.DeleteBranchAfterMerge { + cleanUpPullRequest(ctx, false) + } + ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + fmt.Sprint(pr.Index)) } @@ -1126,6 +1131,10 @@ func TriggerTask(ctx *context.Context) { // CleanUpPullRequest responses for delete merged branch when PR has been merged func CleanUpPullRequest(ctx *context.Context) { + cleanUpPullRequest(ctx, true) +} + +func cleanUpPullRequest(ctx *context.Context, json bool) { issue := checkPullInfo(ctx) if ctx.Written() { return @@ -1180,11 +1189,13 @@ func CleanUpPullRequest(ctx *context.Context) { } defer gitBaseRepo.Close() - defer func() { - ctx.JSON(http.StatusOK, map[string]interface{}{ - "redirect": pr.BaseRepo.Link() + "/pulls/" + fmt.Sprint(issue.Index), - }) - }() + if json { + defer func() { + ctx.JSON(http.StatusOK, map[string]interface{}{ + "redirect": pr.BaseRepo.Link() + "/pulls/" + fmt.Sprint(issue.Index), + }) + }() + } if pr.HeadBranch == pr.HeadRepo.DefaultBranch || !gitRepo.IsBranchExist(pr.HeadBranch) { ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 55d1f6e3bc38..3981e1ac28e7 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -546,11 +546,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 } // Validate validates the fields diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 3bdec4becb02..c24770c1389a 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}} From a9cab401edeab04a2705147731db671793adcee3 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Wed, 2 Jun 2021 09:34:07 +0200 Subject: [PATCH 02/10] Omit DeleteBranchAfterMerge field in json --- services/forms/repo_form.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 3981e1ac28e7..db631bc4d21e 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -551,7 +551,7 @@ type MergePullRequestForm struct { MergeMessageField string MergeCommitID string // only used for manually-merged ForceMerge *bool `json:"force_merge,omitempty"` - DeleteBranchAfterMerge bool + DeleteBranchAfterMerge bool `json:"-"` } // Validate validates the fields From 2a9062b6b9a2742e76e34b49498c121c2ec34621 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Thu, 3 Jun 2021 22:40:35 +0200 Subject: [PATCH 03/10] Log a warning instead of error when PR head branch deleted --- services/pull/pull.go | 6 +++++- services/pull/temp_repo.go | 7 ++++++- services/pull/update.go | 4 +++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index cc560fb199d2..4e05fb6b118b 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -302,7 +302,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 45cd10b65bbe..19b488790a0c 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 f4f7859a49ec..f35e47cbf820 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() { From 996ee469be7ac320dcc5845bbd4b7be245a195c5 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Fri, 4 Jun 2021 20:38:52 +0200 Subject: [PATCH 04/10] Add DefaultDeleteBranchAfterMerge to PullRequestConfig --- models/repo_unit.go | 17 +++++++++-------- modules/structs/repo.go | 2 ++ options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/repo.go | 20 ++++++++++++-------- routers/repo/setting.go | 17 +++++++++-------- services/forms/repo_form.go | 1 + templates/repo/issue/view_content/pull.tmpl | 10 +++++----- templates/repo/settings/options.tmpl | 6 ++++++ templates/swagger/v1_json.tmpl | 5 +++++ 9 files changed, 50 insertions(+), 29 deletions(-) diff --git a/models/repo_unit.go b/models/repo_unit.go index 1d54579a6e72..696131182253 100644 --- a/models/repo_unit.go +++ b/models/repo_unit.go @@ -95,14 +95,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 4fdc1e54cb28..10320a4877e1 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 4df9965bcb07..0b77ff6431e4 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1586,6 +1586,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/repo.go b/routers/api/v1/repo/repo.go index 7a3160fa9937..f365329df1a7 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -724,14 +724,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() @@ -758,6 +759,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/repo/setting.go b/routers/repo/setting.go index 51a0e0116475..a5ecf3cde4af 100644 --- a/routers/repo/setting.go +++ b/routers/repo/setting.go @@ -325,14 +325,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 db631bc4d21e..4e1c57028aea 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -146,6 +146,7 @@ type RepoSettingForm struct { PullsAllowManualMerge bool PullsDefaultMergeStyle string EnableAutodetectManualMerge bool + DefaultDeleteBranchAfterMerge bool EnableTimetracker bool AllowOnlyContributorsToTrackTime bool EnableIssueDependencies bool diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index c24770c1389a..fcb3597ae866 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -317,7 +317,7 @@ {{if .IsPullBranchDeletable}}
- +
{{end}} @@ -336,7 +336,7 @@ {{if .IsPullBranchDeletable}}
- +
{{end}} @@ -361,7 +361,7 @@ {{if .IsPullBranchDeletable}}
- +
{{end}} @@ -386,7 +386,7 @@ {{if .IsPullBranchDeletable}}
- +
{{end}} @@ -408,7 +408,7 @@ {{if .IsPullBranchDeletable}}
- +
{{end}} diff --git a/templates/repo/settings/options.tmpl b/templates/repo/settings/options.tmpl index ece439f3d9b5..274bd3f7a256 100644 --- a/templates/repo/settings/options.tmpl +++ b/templates/repo/settings/options.tmpl @@ -361,6 +361,12 @@ +
+
+ + +
+

{{.i18n.Tr "repo.settings.default_merge_style_desc"}} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index e3ac4a4c8a68..376e48fc2a8b 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -13647,6 +13647,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", From 8a2b5c88d31459dc31ee405a7aac781419fc1688 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Mon, 7 Jun 2021 19:36:50 +0200 Subject: [PATCH 05/10] Add support for delete_branch_after_merge via API --- routers/api/v1/repo/pull.go | 23 +++++++++++++++++++++++ services/forms/repo_form.go | 2 +- templates/swagger/v1_json.tmpl | 4 ++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index eff998ee996a..4fa512080ae3 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -5,6 +5,7 @@ package repo import ( + "errors" "fmt" "net/http" "strings" @@ -23,6 +24,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 @@ -876,6 +878,27 @@ func MergePullRequest(ctx *context.APIContext) { } log.Trace("Pull request merged: %d", pr.ID) + + if form.DeleteBranchAfterMerge { + if err := repo_service.DeleteBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.GitRepo, 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/services/forms/repo_form.go b/services/forms/repo_form.go index 4e1c57028aea..d87fbe0ad0c2 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -552,7 +552,7 @@ type MergePullRequestForm struct { MergeMessageField string MergeCommitID string // only used for manually-merged ForceMerge *bool `json:"force_merge,omitempty"` - DeleteBranchAfterMerge bool `json:"-"` + DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"` } // Validate validates the fields diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 376e48fc2a8b..0523357e138a 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14658,6 +14658,10 @@ "MergeTitleField": { "type": "string" }, + "delete_branch_after_merge": { + "type": "boolean", + "x-go-name": "DeleteBranchAfterMerge" + }, "force_merge": { "type": "boolean", "x-go-name": "ForceMerge" From f9e238bedd687e03c7cc8859c0c2aed490a09f03 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Wed, 30 Jun 2021 21:06:24 +0200 Subject: [PATCH 06/10] 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 --- routers/api/v1/repo/pull.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 4fa512080ae3..38d1ba548be8 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -880,7 +880,18 @@ func MergePullRequest(ctx *context.APIContext) { log.Trace("Pull request merged: %d", pr.ID) if form.DeleteBranchAfterMerge { - if err := repo_service.DeleteBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.GitRepo, pr.HeadBranch); err != nil { + var headRepo *git.Repository + if pr.HeadRepoID == pr.BaseRepoID { + 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) From f55f607040d49b0455ff02f82d8c7413f487cf26 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Wed, 30 Jun 2021 21:08:23 +0200 Subject: [PATCH 07/10] Don't delegate to CleanupBranch, only reuse branch deletion code CleanupBranch contains too much logic that has already been performed by the Merge --- routers/web/repo/pull.go | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 05e61985dcda..ab91554c0986 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -967,7 +967,13 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) if form.DeleteBranchAfterMerge { - cleanUpPullRequest(ctx, false) + 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)) @@ -1135,10 +1141,6 @@ func TriggerTask(ctx *context.Context) { // CleanUpPullRequest responses for delete merged branch when PR has been merged func CleanUpPullRequest(ctx *context.Context) { - cleanUpPullRequest(ctx, true) -} - -func cleanUpPullRequest(ctx *context.Context, json bool) { issue := checkPullInfo(ctx) if ctx.Written() { return @@ -1193,13 +1195,11 @@ func cleanUpPullRequest(ctx *context.Context, json bool) { } defer gitBaseRepo.Close() - if json { - defer func() { - ctx.JSON(http.StatusOK, map[string]interface{}{ - "redirect": pr.BaseRepo.Link() + "/pulls/" + fmt.Sprint(issue.Index), - }) - }() - } + defer func() { + ctx.JSON(http.StatusOK, map[string]interface{}{ + "redirect": pr.BaseRepo.Link() + "/pulls/" + fmt.Sprint(issue.Index), + }) + }() // Check if branch has no new commits headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitRefName()) @@ -1219,6 +1219,11 @@ func cleanUpPullRequest(ctx *context.Context, json bool) { 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): @@ -1234,7 +1239,7 @@ func cleanUpPullRequest(ctx *context.Context, json bool) { return } - if err := models.AddDeletePRBranchComment(ctx.User, pr.BaseRepo, issue.ID, pr.HeadBranch); err != nil { + 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) } From a2a37801198a8a1f6924ed076f8e011a821538ad Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 5 Jul 2021 05:59:42 +0100 Subject: [PATCH 08/10] reuse gitrepo in MergePullRequest Signed-off-by: Andrew Thornton --- routers/web/repo/pull.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index ab91554c0986..c410c8c32d5c 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -967,12 +967,17 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) if form.DeleteBranchAfterMerge { - headRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.RepoPath()), err) - return + var headRepo *git.Repository + if pr.HeadRepoID == ctx.Repo.Repository.ID { + 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() } - defer headRepo.Close() deleteBranch(ctx, pr, headRepo) } @@ -1239,7 +1244,7 @@ func deleteBranch(ctx *context.Context, pr *models.PullRequest, gitRepo *git.Rep return } - if err := models.AddDeletePRBranchComment(ctx.User, pr.BaseRepo, pr.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) } From 86a2a295906b41ee83abb6059cb9043a5af6ca7a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 5 Jul 2021 06:14:33 +0100 Subject: [PATCH 09/10] more reuse Signed-off-by: Andrew Thornton --- routers/web/repo/pull.go | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index c410c8c32d5c..125fb39f0b41 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1186,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.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{}{ From 96e160da002a59db12ca606214e38be964e50565 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 5 Jul 2021 06:22:49 +0100 Subject: [PATCH 10/10] really check Signed-off-by: Andrew Thornton --- routers/api/v1/repo/pull.go | 2 +- routers/web/repo/pull.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 38d1ba548be8..c3c383a1e541 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -881,7 +881,7 @@ func MergePullRequest(ctx *context.APIContext) { if form.DeleteBranchAfterMerge { var headRepo *git.Repository - if pr.HeadRepoID == pr.BaseRepoID { + 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()) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 125fb39f0b41..a29979964777 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -968,7 +968,7 @@ func MergePullRequest(ctx *context.Context) { if form.DeleteBranchAfterMerge { var headRepo *git.Repository - if pr.HeadRepoID == ctx.Repo.Repository.ID { + 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()) @@ -1189,7 +1189,7 @@ func CleanUpPullRequest(ctx *context.Context) { var gitBaseRepo *git.Repository // Assume that the base repo is the current context (almost certainly) - if ctx.Repo != nil && ctx.Repo.Repository.ID == pr.BaseRepoID && ctx.Repo.GitRepo != nil { + 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