diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go
index 45c5c1cd14..53f3b4648a 100644
--- a/routers/api/v1/repo/branch.go
+++ b/routers/api/v1/repo/branch.go
@@ -150,11 +150,6 @@ func DeleteBranch(ctx *context.APIContext) {
}
}
- if ctx.Repo.Repository.IsMirror {
- ctx.Error(http.StatusForbidden, "IsMirrored", fmt.Errorf("can not delete branch of an mirror repository"))
- return
- }
-
if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil {
switch {
case git.IsErrBranchNotExist(err):
diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index 28d7379f07..1116a4e9b1 100644
--- a/routers/api/v1/repo/pull.go
+++ b/routers/api/v1/repo/pull.go
@@ -1057,49 +1057,54 @@ func MergePullRequest(ctx *context.APIContext) {
}
log.Trace("Pull request merged: %d", pr.ID)
- if form.DeleteBranchAfterMerge {
- // 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.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)
+ // 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.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
+ ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
return
}
- defer headRepo.Close()
- }
- if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
- ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
- return
- }
- if err := repo_service.DeleteBranch(ctx, ctx.Doer, 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, git_model.ErrBranchIsProtected):
- ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
- default:
- ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
+ 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.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
+ return
+ }
+ defer headRepo.Close()
+ }
+ if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
+ ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
+ return
+ }
+ if err := repo_service.DeleteBranch(ctx, ctx.Doer, 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, git_model.ErrBranchIsProtected):
+ ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
+ default:
+ ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
+ }
+ return
+ }
+ if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
+ // Do not fail here as branch has already been deleted
+ log.Error("DeleteBranch: %v", err)
}
- return
- }
- if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
- // Do not fail here as branch has already been deleted
- log.Error("DeleteBranch: %v", err)
}
}
diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go
index cd20c0b18e..e3b329d01d 100644
--- a/routers/web/repo/pull.go
+++ b/routers/web/repo/pull.go
@@ -1185,32 +1185,34 @@ func MergePullRequest(ctx *context.Context) {
log.Trace("Pull request merged: %d", pr.ID)
- if form.DeleteBranchAfterMerge {
- // 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)
- return
- }
- defer headRepo.Close()
- }
- deleteBranch(ctx, pr, headRepo)
+ 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)
+ return
+ }
+ defer headRepo.Close()
+ }
+ deleteBranch(ctx, pr, headRepo)
ctx.JSONRedirect(issue.Link())
}
@@ -1403,8 +1405,8 @@ func CleanUpPullRequest(ctx *context.Context) {
pr := issue.PullRequest
- // Don't cleanup unmerged and unclosed PRs
- if !pr.HasMerged && !issue.IsClosed {
+ // Don't cleanup unmerged and unclosed PRs and agit PRs
+ if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub {
ctx.NotFound("CleanUpPullRequest", nil)
return
}
@@ -1435,13 +1437,12 @@ func CleanUpPullRequest(ctx *context.Context) {
return
}
- perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, ctx.Doer)
- if err != nil {
- ctx.ServerError("GetUserRepoPermission", err)
- return
- }
- if !perm.CanWrite(unit.TypeCode) {
- ctx.NotFound("CleanUpPullRequest", nil)
+ if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err != nil {
+ if errors.Is(err, util.ErrPermissionDenied) {
+ ctx.NotFound("CanDeleteBranch", nil)
+ } else {
+ ctx.ServerError("CanDeleteBranch", err)
+ }
return
}
diff --git a/services/repository/branch.go b/services/repository/branch.go
index 600ba96e92..508817c83e 100644
--- a/services/repository/branch.go
+++ b/services/repository/branch.go
@@ -14,7 +14,9 @@ import (
"code.gitea.io/gitea/models/db"
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"
repo_model "code.gitea.io/gitea/models/repo"
+ "code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/cache"
"code.gitea.io/gitea/modules/git"
@@ -463,15 +465,17 @@ var (
ErrBranchIsDefault = errors.New("branch is default")
)
-// DeleteBranch delete branch
-func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error {
- err := repo.MustNotBeArchived()
+func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchName string, doer *user_model.User) error {
+ if branchName == repo.DefaultBranch {
+ return ErrBranchIsDefault
+ }
+
+ perm, err := access_model.GetUserRepoPermission(ctx, repo, doer)
if err != nil {
return err
}
-
- if branchName == repo.DefaultBranch {
- return ErrBranchIsDefault
+ if !perm.CanWrite(unit.TypeCode) {
+ return util.NewPermissionDeniedErrorf("permission denied to access repo %d unit %s", repo.ID, unit.TypeCode.LogString())
}
isProtected, err := git_model.IsBranchProtected(ctx, repo.ID, branchName)
@@ -481,6 +485,19 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R
if isProtected {
return git_model.ErrBranchIsProtected
}
+ return nil
+}
+
+// DeleteBranch delete branch
+func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error {
+ err := repo.MustNotBeArchived()
+ if err != nil {
+ return err
+ }
+
+ if err := CanDeleteBranch(ctx, repo, branchName, doer); err != nil {
+ return err
+ }
rawBranch, err := git_model.GetBranch(ctx, repo.ID, branchName)
if err != nil && !git_model.IsErrBranchNotExist(err) {
diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go
index bbd99f7aab..eb3743bc17 100644
--- a/tests/integration/pull_merge_test.go
+++ b/tests/integration/pull_merge_test.go
@@ -554,6 +554,10 @@ func TestPullRetargetChildOnBranchDelete(t *testing.T) {
testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
+ repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
+ branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
+ assert.True(t, branchBasePR.IsDeleted)
+
// Check child PR
req := NewRequest(t, "GET", test.RedirectURL(respChildPR))
resp := session.MakeRequest(t, req, http.StatusOK)
@@ -584,6 +588,10 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
+ repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"})
+ branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
+ assert.True(t, branchBasePR.IsDeleted)
+
// Check child PR
req := NewRequest(t, "GET", test.RedirectURL(respChildPR))
resp := session.MakeRequest(t, req, http.StatusOK)
@@ -598,6 +606,27 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
})
}
+func TestPullRequestMergedWithNoPermissionDeleteBranch(t *testing.T) {
+ onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+ session := loginUser(t, "user4")
+ testRepoFork(t, session, "user2", "repo1", "user4", "repo1", "")
+ testEditFileToNewBranch(t, session, "user4", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n")
+
+ respBasePR := testPullCreate(t, session, "user4", "repo1", false, "master", "base-pr", "Base Pull Request")
+ elemBasePR := strings.Split(test.RedirectURL(respBasePR), "/")
+ assert.EqualValues(t, "pulls", elemBasePR[3])
+
+ // user2 has no permission to delete branch of repo user1/repo1
+ session2 := loginUser(t, "user2")
+ testPullMerge(t, session2, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
+
+ repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user4", Name: "repo1"})
+ branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
+ // branch has not been deleted
+ assert.False(t, branchBasePR.IsDeleted)
+ })
+}
+
func TestPullMergeIndexerNotifier(t *testing.T) {
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
// create a pull request