From a9f2ea720be96435129c1d551806bf369260aa50 Mon Sep 17 00:00:00 2001 From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com> Date: Tue, 21 Oct 2025 22:06:56 -0700 Subject: [PATCH] Honor delete branch on merge repo setting when using merge API (#35488) Fix #35463. --------- Co-authored-by: wxiaoguang --- models/git/protected_branch.go | 3 +- modules/util/error.go | 53 +++--- modules/util/error_test.go | 29 +++ routers/api/v1/repo/pull.go | 56 ++---- routers/web/repo/actions/view.go | 4 +- routers/web/repo/editor_apply_patch.go | 2 +- routers/web/repo/editor_cherry_pick.go | 2 +- routers/web/repo/editor_error.go | 4 +- routers/web/repo/pull.go | 172 +++--------------- services/actions/workflow.go | 10 +- services/automerge/automerge.go | 21 +-- services/forms/repo_form.go | 2 +- services/pull/merge.go | 21 +++ services/repository/branch.go | 91 ++++++++- .../issue/view_content/pull_merge_box.tmpl | 4 +- tests/integration/api_issue_config_test.go | 2 +- tests/integration/api_pull_test.go | 74 ++++++++ .../integration/api_repo_file_create_test.go | 4 +- tests/integration/api_repo_file_helpers.go | 42 +++-- tests/integration/pull_merge_test.go | 2 +- 20 files changed, 334 insertions(+), 264 deletions(-) create mode 100644 modules/util/error_test.go diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 511f7563cf..13e1ced0e1 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -5,7 +5,6 @@ package git import ( "context" - "errors" "fmt" "slices" "strings" @@ -25,7 +24,7 @@ import ( "xorm.io/builder" ) -var ErrBranchIsProtected = errors.New("branch is protected") +var ErrBranchIsProtected = util.ErrorWrap(util.ErrPermissionDenied, "branch is protected") // ProtectedBranch struct type ProtectedBranch struct { diff --git a/modules/util/error.go b/modules/util/error.go index 24fa1ba151..4c2566a2a3 100644 --- a/modules/util/error.go +++ b/modules/util/error.go @@ -6,6 +6,7 @@ package util import ( "errors" "fmt" + "html/template" ) // Common Errors forming the base of our error system @@ -40,22 +41,6 @@ func (w errorWrapper) Unwrap() error { return w.Err } -type LocaleWrapper struct { - err error - TrKey string - TrArgs []any -} - -// Error returns the message -func (w LocaleWrapper) Error() string { - return w.err.Error() -} - -// Unwrap returns the underlying error -func (w LocaleWrapper) Unwrap() error { - return w.err -} - // ErrorWrap returns an error that formats as the given text but unwraps as the provided error func ErrorWrap(unwrap error, message string, args ...any) error { if len(args) == 0 { @@ -84,15 +69,39 @@ func NewNotExistErrorf(message string, args ...any) error { return ErrorWrap(ErrNotExist, message, args...) } -// ErrorWrapLocale wraps an err with a translation key and arguments -func ErrorWrapLocale(err error, trKey string, trArgs ...any) error { - return LocaleWrapper{err: err, TrKey: trKey, TrArgs: trArgs} +// ErrorTranslatable wraps an error with translation information +type ErrorTranslatable interface { + error + Unwrap() error + Translate(ErrorLocaleTranslator) template.HTML } -func ErrorAsLocale(err error) *LocaleWrapper { - var e LocaleWrapper +type errorTranslatableWrapper struct { + err error + trKey string + trArgs []any +} + +type ErrorLocaleTranslator interface { + Tr(key string, args ...any) template.HTML +} + +func (w *errorTranslatableWrapper) Error() string { return w.err.Error() } + +func (w *errorTranslatableWrapper) Unwrap() error { return w.err } + +func (w *errorTranslatableWrapper) Translate(t ErrorLocaleTranslator) template.HTML { + return t.Tr(w.trKey, w.trArgs...) +} + +func ErrorWrapTranslatable(err error, trKey string, trArgs ...any) ErrorTranslatable { + return &errorTranslatableWrapper{err: err, trKey: trKey, trArgs: trArgs} +} + +func ErrorAsTranslatable(err error) ErrorTranslatable { + var e *errorTranslatableWrapper if errors.As(err, &e) { - return &e + return e } return nil } diff --git a/modules/util/error_test.go b/modules/util/error_test.go new file mode 100644 index 0000000000..31d0c91260 --- /dev/null +++ b/modules/util/error_test.go @@ -0,0 +1,29 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package util + +import ( + "io" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestErrorTranslatable(t *testing.T) { + var err error + + err = ErrorWrapTranslatable(io.EOF, "key", 1) + assert.ErrorIs(t, err, io.EOF) + assert.Equal(t, "EOF", err.Error()) + assert.Equal(t, "key", err.(*errorTranslatableWrapper).trKey) + assert.Equal(t, []any{1}, err.(*errorTranslatableWrapper).trArgs) + + err = ErrorWrap(err, "new msg %d", 100) + assert.ErrorIs(t, err, io.EOF) + assert.Equal(t, "new msg 100", err.Error()) + + errTr := ErrorAsTranslatable(err) + assert.Equal(t, "EOF", errTr.Error()) + assert.Equal(t, "key", errTr.(*errorTranslatableWrapper).trKey) +} diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 22851ad2c5..2561ff3975 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -13,7 +13,6 @@ import ( "time" activities_model "code.gitea.io/gitea/models/activities" - git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" pull_model "code.gitea.io/gitea/models/pull" @@ -938,7 +937,7 @@ func MergePullRequest(ctx *context.APIContext) { } else if errors.Is(err, pull_service.ErrNoPermissionToMerge) { ctx.APIError(http.StatusMethodNotAllowed, "User not allowed to merge PR") } else if errors.Is(err, pull_service.ErrHasMerged) { - ctx.APIError(http.StatusMethodNotAllowed, "") + ctx.APIError(http.StatusMethodNotAllowed, "The PR is already merged") } else if errors.Is(err, pull_service.ErrIsWorkInProgress) { ctx.APIError(http.StatusMethodNotAllowed, "Work in progress PRs cannot be merged") } else if errors.Is(err, pull_service.ErrNotMergeableState) { @@ -989,8 +988,14 @@ func MergePullRequest(ctx *context.APIContext) { message += "\n\n" + form.MergeMessageField } + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, form.DeleteBranchAfterMerge, ctx.Repo.Repository, pr) + if err != nil { + ctx.APIErrorInternal(err) + return + } + if form.MergeWhenChecksSucceed { - scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { if pull_model.IsErrAlreadyScheduledToAutoMerge(err) { ctx.APIError(http.StatusConflict, err) @@ -1035,47 +1040,10 @@ func MergePullRequest(ctx *context.APIContext) { } log.Trace("Pull request merged: %d", pr.ID) - // for agit flow, we should not delete the agit reference after merge - if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub { - // check permission even it has been checked in repo_service.DeleteBranch so that we don't need to - // do RetargetChildrenOnMerge - if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil { - // 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 { - ctx.APIErrorInternal(err) - return - } - if exist { - ctx.Status(http.StatusOK) - return - } - - 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 = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.APIErrorInternal(err) - return - } - defer headRepo.Close() - } - - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch, pr); err != nil { - switch { - case git.IsErrBranchNotExist(err): - ctx.APIErrorNotFound(err) - case errors.Is(err, repo_service.ErrBranchIsDefault): - ctx.APIError(http.StatusForbidden, errors.New("can not delete default branch")) - case errors.Is(err, git_model.ErrBranchIsProtected): - ctx.APIError(http.StatusForbidden, errors.New("branch protected")) - default: - ctx.APIErrorInternal(err) - } - return - } + if deleteBranchAfterMerge { + if err = repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr.ID, nil); err != nil { + // no way to tell users that what error happens, and the PR has been merged, so ignore the error + log.Debug("DeleteBranchAfterMerge: pr %d, err: %v", pr.ID, err) } } diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 013dab4acf..f7e57c91a8 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -943,8 +943,8 @@ func Run(ctx *context_module.Context) { return nil }) if err != nil { - if errLocale := util.ErrorAsLocale(err); errLocale != nil { - ctx.Flash.Error(ctx.Tr(errLocale.TrKey, errLocale.TrArgs...)) + if errTr := util.ErrorAsTranslatable(err); errTr != nil { + ctx.Flash.Error(errTr.Translate(ctx.Locale)) ctx.Redirect(redirectURL) } else { ctx.ServerError("DispatchActionWorkflow", err) diff --git a/routers/web/repo/editor_apply_patch.go b/routers/web/repo/editor_apply_patch.go index bd2811cc5f..aad7b4129c 100644 --- a/routers/web/repo/editor_apply_patch.go +++ b/routers/web/repo/editor_apply_patch.go @@ -41,7 +41,7 @@ func NewDiffPatchPost(ctx *context.Context) { Committer: parsed.GitCommitter, }) if err != nil { - err = util.ErrorWrapLocale(err, "repo.editor.fail_to_apply_patch") + err = util.ErrorWrapTranslatable(err, "repo.editor.fail_to_apply_patch") } if err != nil { editorHandleFileOperationError(ctx, parsed.NewBranchName, err) diff --git a/routers/web/repo/editor_cherry_pick.go b/routers/web/repo/editor_cherry_pick.go index 10c2741b1c..099814a9fa 100644 --- a/routers/web/repo/editor_cherry_pick.go +++ b/routers/web/repo/editor_cherry_pick.go @@ -74,7 +74,7 @@ func CherryPickPost(ctx *context.Context) { opts.Content = buf.String() _, err = files.ApplyDiffPatch(ctx, ctx.Repo.Repository, ctx.Doer, opts) if err != nil { - err = util.ErrorWrapLocale(err, "repo.editor.fail_to_apply_patch") + err = util.ErrorWrapTranslatable(err, "repo.editor.fail_to_apply_patch") } } if err != nil { diff --git a/routers/web/repo/editor_error.go b/routers/web/repo/editor_error.go index 245226a039..e1473a34b3 100644 --- a/routers/web/repo/editor_error.go +++ b/routers/web/repo/editor_error.go @@ -38,8 +38,8 @@ func editorHandleFileOperationErrorRender(ctx *context_service.Context, message, } func editorHandleFileOperationError(ctx *context_service.Context, targetBranchName string, err error) { - if errAs := util.ErrorAsLocale(err); errAs != nil { - ctx.JSONError(ctx.Tr(errAs.TrKey, errAs.TrArgs...)) + if errAs := util.ErrorAsTranslatable(err); errAs != nil { + ctx.JSONError(errAs.Translate(ctx.Locale)) } else if errAs, ok := errorAs[git.ErrNotExist](err); ok { ctx.JSONError(ctx.Tr("repo.editor.file_modifying_no_longer_exists", errAs.RelPath)) } else if errAs, ok := errorAs[git_model.ErrLFSFileLocked](err); ok { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index c8da025b15..1d7e5be31f 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1130,11 +1130,17 @@ func MergePullRequest(ctx *context.Context) { message += "\n\n" + form.MergeMessageField } + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, form.DeleteBranchAfterMerge, ctx.Repo.Repository, pr) + if err != nil { + ctx.ServerError("ShouldDeleteBranchAfterMerge", err) + return + } + if form.MergeWhenChecksSucceed { // 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, form.DeleteBranchAfterMerge) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { ctx.ServerError("ScheduleAutoMerge", err) return @@ -1220,37 +1226,29 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) - if !form.DeleteBranchAfterMerge { - ctx.JSONRedirect(issue.Link()) - return - } - - // Don't cleanup when other pr use this branch as head branch - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) - if err != nil { - ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) - return - } - if exist { - ctx.JSONRedirect(issue.Link()) - return - } - - 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 = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) + if deleteBranchAfterMerge { + deleteBranchAfterMergeAndFlashMessage(ctx, pr.ID) + if ctx.Written() { return } - defer headRepo.Close() } - deleteBranch(ctx, pr, headRepo) ctx.JSONRedirect(issue.Link()) } +func deleteBranchAfterMergeAndFlashMessage(ctx *context.Context, prID int64) { + var fullBranchName string + err := repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, prID, &fullBranchName) + if errTr := util.ErrorAsTranslatable(err); errTr != nil { + ctx.Flash.Error(errTr.Translate(ctx.Locale)) + return + } else if err == nil { + ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName)) + return + } + // catch unknown errors + ctx.ServerError("DeleteBranchAfterMerge", err) +} + // CancelAutoMergePullRequest cancels a scheduled pr func CancelAutoMergePullRequest(ctx *context.Context) { issue, ok := getPullInfo(ctx) @@ -1437,131 +1435,17 @@ func CompareAndPullRequestPost(ctx *context.Context) { } // CleanUpPullRequest responses for delete merged branch when PR has been merged +// Used by "DeleteBranchLink" for "delete branch" button func CleanUpPullRequest(ctx *context.Context) { issue, ok := getPullInfo(ctx) if !ok { return } - - pr := issue.PullRequest - - // Don't cleanup unmerged and unclosed PRs and agit PRs - if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub { - ctx.NotFound(nil) + deleteBranchAfterMergeAndFlashMessage(ctx, issue.PullRequest.ID) + if ctx.Written() { return } - - // 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 { - ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) - return - } - if exist { - ctx.NotFound(nil) - return - } - - if err := pr.LoadHeadRepo(ctx); err != nil { - ctx.ServerError("LoadHeadRepo", err) - return - } else if pr.HeadRepo == nil { - // Forked repository has already been deleted - ctx.NotFound(nil) - return - } else if err = pr.LoadBaseRepo(ctx); err != nil { - ctx.ServerError("LoadBaseRepo", err) - return - } else if err = pr.HeadRepo.LoadOwner(ctx); err != nil { - ctx.ServerError("HeadRepo.LoadOwner", err) - return - } - - if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err != nil { - if errors.Is(err, util.ErrPermissionDenied) { - ctx.NotFound(nil) - } else { - ctx.ServerError("CanDeleteBranch", err) - } - return - } - - fullBranchName := pr.HeadRepo.Owner.Name + "/" + pr.HeadBranch - - 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 = gitrepo.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.FullName()), 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 = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) - return - } - defer gitRepo.Close() - } - - defer func() { - ctx.JSONRedirect(issue.Link()) - }() - - // Check if branch has no new commits - headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitHeadRefName()) - if err != nil { - log.Error("GetRefCommitID: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - branchCommitID, err := gitRepo.GetBranchCommitID(pr.HeadBranch) - if err != nil { - log.Error("GetBranchCommitID: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - if headCommitID != branchCommitID { - ctx.Flash.Error(ctx.Tr("repo.branch.delete_branch_has_new_commits", fullBranchName)) - return - } - - deleteBranch(ctx, pr, gitRepo) -} - -func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) { - fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch - - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, gitRepo, pr.HeadBranch, pr); err != nil { - switch { - case git.IsErrBranchNotExist(err): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - case errors.Is(err, repo_service.ErrBranchIsDefault): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - case errors.Is(err, git_model.ErrBranchIsProtected): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - default: - log.Error("DeleteBranch: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - } - return - } - - ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName)) + ctx.JSONRedirect(issue.Link()) } // DownloadPullDiff render a pull's raw diff diff --git a/services/actions/workflow.go b/services/actions/workflow.go index 25801d6fa1..4f72aad112 100644 --- a/services/actions/workflow.go +++ b/services/actions/workflow.go @@ -46,14 +46,14 @@ func EnableOrDisableWorkflow(ctx *context.APIContext, workflowID string, isEnabl func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, workflowID, ref string, processInputs func(model *model.WorkflowDispatch, inputs map[string]any) error) error { if workflowID == "" { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("workflowID is empty"), "actions.workflow.not_found", workflowID, ) } if ref == "" { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("ref is empty"), "form.target_ref_not_exist", ref, ) @@ -63,7 +63,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re cfgUnit := repo.MustGetUnit(ctx, unit.TypeActions) cfg := cfgUnit.ActionsConfig() if cfg.IsWorkflowDisabled(workflowID) { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewPermissionDeniedErrorf("workflow is disabled"), "actions.workflow.disabled", ) @@ -82,7 +82,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re runTargetCommit, err = gitRepo.GetBranchCommit(ref) } if err != nil { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("ref %q doesn't exist", ref), "form.target_ref_not_exist", ref, ) @@ -122,7 +122,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re } if entry == nil { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("workflow %q doesn't exist", workflowID), "actions.workflow.not_found", workflowID, ) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index a60883b4cc..eabfeff143 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -205,18 +205,6 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - var headGitRepo *git.Repository - if pr.BaseRepoID == pr.HeadRepoID { - headGitRepo = baseGitRepo - } else { - headGitRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - log.Error("OpenRepository %-v: %v", pr.HeadRepo, err) - return - } - defer headGitRepo.Close() - } - switch pr.Flow { case issues_model.PullRequestFlowGithub: headBranchExist := pr.HeadRepo != nil && gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) @@ -276,9 +264,12 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - if pr.Flow == issues_model.PullRequestFlowGithub && scheduledPRM.DeleteBranchAfterMerge { - if err := repo_service.DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch, pr); err != nil { - log.Error("DeletePullRequestHeadBranch: %v", err) + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, &scheduledPRM.DeleteBranchAfterMerge, pr.BaseRepo, pr) + if err != nil { + log.Error("ShouldDeleteBranchAfterMerge: %v", err) + } else if deleteBranchAfterMerge { + if err = repo_service.DeleteBranchAfterMerge(ctx, doer, pr.ID, nil); err != nil { + log.Error("DeleteBranchAfterMerge: %v", err) } } } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index cb267f891c..67f24c4cbe 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -540,7 +540,7 @@ type MergePullRequestForm struct { HeadCommitID string `json:"head_commit_id,omitempty"` ForceMerge bool `json:"force_merge,omitempty"` MergeWhenChecksSucceed bool `json:"merge_when_checks_succeed,omitempty"` - DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"` + DeleteBranchAfterMerge *bool `json:"delete_branch_after_merge,omitempty"` } // Validate validates the fields diff --git a/services/pull/merge.go b/services/pull/merge.go index 1e8e9d444b..43bb3dd235 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -730,3 +730,24 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return true, nil }) } + +func ShouldDeleteBranchAfterMerge(ctx context.Context, userOption *bool, repo *repo_model.Repository, pr *issues_model.PullRequest) (bool, error) { + if pr.Flow != issues_model.PullRequestFlowGithub { + // only support GitHub workflow (branch-based) + // for agit workflow, there is no branch, so nothing to delete + // TODO: maybe in the future, it should delete the "agit reference (refs/for/xxxx)"? + return false, nil + } + + // if user has set an option, respect it + if userOption != nil { + return *userOption, nil + } + + // otherwise, use repository default + prUnit, err := repo.GetUnit(ctx, unit.TypePullRequests) + if err != nil { + return false, err + } + return prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge, nil +} diff --git a/services/repository/branch.go b/services/repository/branch.go index 5d8178375e..b49478422c 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -484,10 +484,7 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m return "", nil } -// enmuerates all branch related errors -var ( - ErrBranchIsDefault = errors.New("branch is default") -) +var ErrBranchIsDefault = util.ErrorWrap(util.ErrPermissionDenied, "branch is default") func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchName string, doer *user_model.User) error { if branchName == repo.DefaultBranch { @@ -745,3 +742,89 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo info.BaseHasNewCommits = info.HeadCommitsBehind > 0 return info, nil } + +func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int64, outFullBranchName *string) error { + pr, err := issues_model.GetPullRequestByID(ctx, prID) + if err != nil { + return err + } + if err = pr.LoadIssue(ctx); err != nil { + return err + } + if err = pr.LoadBaseRepo(ctx); err != nil { + return err + } + if err := pr.LoadHeadRepo(ctx); err != nil { + return err + } + if pr.HeadRepo == nil { + // Forked repository has already been deleted + return util.ErrorWrapTranslatable(util.ErrNotExist, "repo.branch.deletion_failed", "(deleted-repo):"+pr.HeadBranch) + } + if err = pr.HeadRepo.LoadOwner(ctx); err != nil { + return err + } + + fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch + if outFullBranchName != nil { + *outFullBranchName = fullBranchName + } + + errFailedToDelete := func(err error) error { + return util.ErrorWrapTranslatable(err, "repo.branch.deletion_failed", fullBranchName) + } + + // Don't clean up unmerged and unclosed PRs and agit PRs + if !pr.HasMerged && !pr.Issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub { + return errFailedToDelete(util.ErrUnprocessableContent) + } + + // Don't clean up 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 { + return err + } + if exist { + return errFailedToDelete(util.ErrUnprocessableContent) + } + + if err := CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, doer); err != nil { + if errors.Is(err, util.ErrPermissionDenied) { + return errFailedToDelete(err) + } + return err + } + + gitBaseRepo, gitBaseCloser, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) + if err != nil { + return err + } + defer gitBaseCloser.Close() + + gitHeadRepo, gitHeadCloser, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.HeadRepo) + if err != nil { + return err + } + defer gitHeadCloser.Close() + + // Check if branch has no new commits + headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitHeadRefName()) + if err != nil { + log.Error("GetRefCommitID: %v", err) + return errFailedToDelete(err) + } + branchCommitID, err := gitHeadRepo.GetBranchCommitID(pr.HeadBranch) + if err != nil { + log.Error("GetBranchCommitID: %v", err) + return errFailedToDelete(err) + } + if headCommitID != branchCommitID { + return util.ErrorWrapTranslatable(util.ErrUnprocessableContent, "repo.branch.delete_branch_has_new_commits", fullBranchName) + } + + err = DeleteBranch(ctx, doer, pr.HeadRepo, gitHeadRepo, pr.HeadBranch, pr) + if errors.Is(err, util.ErrPermissionDenied) || errors.Is(err, util.ErrNotExist) { + return errFailedToDelete(err) + } + return err +} diff --git a/templates/repo/issue/view_content/pull_merge_box.tmpl b/templates/repo/issue/view_content/pull_merge_box.tmpl index c0d717e854..2b943d2069 100644 --- a/templates/repo/issue/view_content/pull_merge_box.tmpl +++ b/templates/repo/issue/view_content/pull_merge_box.tmpl @@ -50,7 +50,7 @@
- +
{{end}} @@ -68,7 +68,7 @@ {{if and .IsPullBranchDeletable (not .IsPullRequestBroken)}}
- +
{{end}} diff --git a/tests/integration/api_issue_config_test.go b/tests/integration/api_issue_config_test.go index ad39965443..f6045e1a80 100644 --- a/tests/integration/api_issue_config_test.go +++ b/tests/integration/api_issue_config_test.go @@ -129,7 +129,7 @@ func TestAPIRepoIssueConfigPaths(t *testing.T) { configData, err := yaml.Marshal(configMap) assert.NoError(t, err) - _, err = createFileInBranch(owner, repo, fullPath, repo.DefaultBranch, string(configData)) + _, err = createFile(owner, repo, fullPath, string(configData)) assert.NoError(t, err) issueConfig := getIssueConfig(t, owner.Name, repo.Name) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 8376fd2717..433dce3d5e 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -17,19 +17,23 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" + unit_model "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/convert" "code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/gitdiff" issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" + repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIViewPulls(t *testing.T) { @@ -186,6 +190,76 @@ func TestAPIMergePullWIP(t *testing.T) { MakeRequest(t, req, http.StatusMethodNotAllowed) } +func TestAPIMergePull(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + apiCtx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository) + + checkBranchExists := func(t *testing.T, branchName string, status int) { + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", owner.Name, repo.Name, branchName)).AddTokenAuth(apiCtx.Token) + MakeRequest(t, req, status) + } + + createTestBranchPR := func(t *testing.T, branchName string) *api.PullRequest { + testCreateFileInBranch(t, owner, repo, createFileInBranchOptions{NewBranch: branchName}, map[string]string{"a-new-file-" + branchName + ".txt": "dummy content"}) + prDTO, err := doAPICreatePullRequest(apiCtx, repo.OwnerName, repo.Name, repo.DefaultBranch, branchName)(t) + require.NoError(t, err) + return &prDTO + } + + performMerge := func(t *testing.T, prIndex int64, params map[string]any, optExpectedStatus ...int) { + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prIndex), params).AddTokenAuth(apiCtx.Token) + expectedStatus := util.OptionalArg(optExpectedStatus, http.StatusOK) + MakeRequest(t, req, expectedStatus) + } + + t.Run("Normal", func(t *testing.T) { + newBranch := "test-pull-1" + prDTO := createTestBranchPR(t, newBranch) + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}) + checkBranchExists(t, newBranch, http.StatusOK) + // try to merge again, make sure we cannot perform a merge on the same PR + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}, http.StatusMethodNotAllowed) + }) + + t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) { + newBranch := "test-pull-2" + prDTO := createTestBranchPR(t, newBranch) + performMerge(t, prDTO.Index, map[string]any{"do": "merge", "delete_branch_after_merge": true}) + checkBranchExists(t, newBranch, http.StatusNotFound) + }) + + updateRepoUnitDefaultDeleteBranchAfterMerge := func(t *testing.T, repo *repo_model.Repository, value bool) { + prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) + require.NoError(t, err) + + prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge = value + require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, []repo_model.RepoUnit{{ + RepoID: repo.ID, + Type: unit_model.TypePullRequests, + Config: prUnit.PullRequestsConfig(), + }}, nil)) + } + + t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) { + newBranch := "test-pull-3" + prDTO := createTestBranchPR(t, newBranch) + updateRepoUnitDefaultDeleteBranchAfterMerge(t, repo, true) + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}) + checkBranchExists(t, newBranch, http.StatusNotFound) + }) + + t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) { + newBranch := "test-pull-4" + prDTO := createTestBranchPR(t, newBranch) + updateRepoUnitDefaultDeleteBranchAfterMerge(t, repo, false) + performMerge(t, prDTO.Index, map[string]any{"do": "merge", "delete_branch_after_merge": true}) + checkBranchExists(t, newBranch, http.StatusNotFound) + }) + }) +} + func TestAPICreatePullSuccess(t *testing.T) { defer tests.PrepareTestEnv(t)() repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) diff --git a/tests/integration/api_repo_file_create_test.go b/tests/integration/api_repo_file_create_test.go index af3bc54680..7cf1083248 100644 --- a/tests/integration/api_repo_file_create_test.go +++ b/tests/integration/api_repo_file_create_test.go @@ -133,7 +133,7 @@ func BenchmarkAPICreateFileSmall(b *testing.B) { b.ResetTimer() for n := 0; b.Loop(); n++ { treePath := fmt.Sprintf("update/file%d.txt", n) - _, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath) + _, _ = createFile(user2, repo1, treePath) } }) } @@ -149,7 +149,7 @@ func BenchmarkAPICreateFileMedium(b *testing.B) { for n := 0; b.Loop(); n++ { treePath := fmt.Sprintf("update/file%d.txt", n) copy(data, treePath) - _, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath) + _, _ = createFile(user2, repo1, treePath) } }) } diff --git a/tests/integration/api_repo_file_helpers.go b/tests/integration/api_repo_file_helpers.go index 37962028bd..785416b0f5 100644 --- a/tests/integration/api_repo_file_helpers.go +++ b/tests/integration/api_repo_file_helpers.go @@ -6,26 +6,36 @@ package integration import ( "context" "strings" + "testing" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" files_service "code.gitea.io/gitea/services/repository/files" + + "github.com/stretchr/testify/require" ) -func createFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, branchName, content string) (*api.FilesResponse, error) { +type createFileInBranchOptions struct { + OldBranch, NewBranch string +} + +func testCreateFileInBranch(t *testing.T, user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) *api.FilesResponse { + resp, err := createFileInBranch(user, repo, createOpts, files) + require.NoError(t, err) + return resp +} + +func createFileInBranch(user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) (*api.FilesResponse, error) { ctx := context.TODO() - opts := &files_service.ChangeRepoFilesOptions{ - Files: []*files_service.ChangeRepoFile{ - { - Operation: "create", - TreePath: treePath, - ContentReader: strings.NewReader(content), - }, - }, - OldBranch: branchName, - Author: nil, - Committer: nil, + opts := &files_service.ChangeRepoFilesOptions{OldBranch: createOpts.OldBranch, NewBranch: createOpts.NewBranch} + for path, content := range files { + opts.Files = append(opts.Files, &files_service.ChangeRepoFile{ + Operation: "create", + TreePath: path, + ContentReader: strings.NewReader(content), + }) } return files_service.ChangeRepoFiles(ctx, repo, user, opts) } @@ -53,10 +63,12 @@ func createOrReplaceFileInBranch(user *user_model.User, repo *repo_model.Reposit return err } - _, err = createFileInBranch(user, repo, treePath, branchName, content) + _, err = createFileInBranch(user, repo, createFileInBranchOptions{OldBranch: branchName}, map[string]string{treePath: content}) return err } -func createFile(user *user_model.User, repo *repo_model.Repository, treePath string) (*api.FilesResponse, error) { - return createFileInBranch(user, repo, treePath, repo.DefaultBranch, "This is a NEW file") +// TODO: replace all usages of this function with testCreateFileInBranch or testCreateFile +func createFile(user *user_model.User, repo *repo_model.Repository, treePath string, optContent ...string) (*api.FilesResponse, error) { + content := util.OptionalArg(optContent, "This is a NEW file") // some tests need this default content because its SHA is hardcoded + return createFileInBranch(user, repo, createFileInBranchOptions{}, map[string]string{treePath: content}) } diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 3345216838..32ab741382 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -93,7 +93,7 @@ func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum str // Click the little button to create a pull htmlDoc := NewHTMLParser(t, resp.Body) - link, exists := htmlDoc.doc.Find(".timeline-item .delete-button").Attr("data-url") + link, exists := htmlDoc.doc.Find(".timeline-item .delete-branch-after-merge").Attr("data-url") assert.True(t, exists, "The template has changed, can not find delete button url") req = NewRequestWithValues(t, "POST", link, map[string]string{ "_csrf": htmlDoc.GetCSRF(),