Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"code.gitea.io/gitea/modules/gitrepo"
"code.gitea.io/gitea/modules/graceful"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/timeutil"
Expand Down Expand Up @@ -990,7 +991,7 @@ func MergePullRequest(ctx *context.APIContext) {
}

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, optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false))
if err != nil {
if pull_model.IsErrAlreadyScheduledToAutoMerge(err) {
ctx.APIError(http.StatusConflict, err)
Expand Down Expand Up @@ -1035,8 +1036,23 @@ func MergePullRequest(ctx *context.APIContext) {
}
log.Trace("Pull request merged: %d", pr.ID)

prUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit.TypePullRequests)
if err != nil {
ctx.APIErrorInternal(err)
return
}

// for agit flow, we should not delete the agit reference after merge
if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub {
shouldDeleteBranch := pr.Flow == issues_model.PullRequestFlowGithub
if form.DeleteBranchAfterMerge != nil {
// if the form field is defined, it takes precedence over the repo setting equivalent
shouldDeleteBranch = shouldDeleteBranch && *form.DeleteBranchAfterMerge
} else {
// otherwise, we look at the repo setting to make the determination
shouldDeleteBranch = shouldDeleteBranch && prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge
}

if shouldDeleteBranch {
// 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 {
Expand Down
5 changes: 3 additions & 2 deletions routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"code.gitea.io/gitea/modules/graceful"
issue_template "code.gitea.io/gitea/modules/issue/template"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/util"
Expand Down Expand Up @@ -1106,7 +1107,7 @@ func MergePullRequest(ctx *context.Context) {
// 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, optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false))
if err != nil {
ctx.ServerError("ScheduleAutoMerge", err)
return
Expand Down Expand Up @@ -1192,7 +1193,7 @@ func MergePullRequest(ctx *context.Context) {

log.Trace("Pull request merged: %d", pr.ID)

if !form.DeleteBranchAfterMerge {
if !optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) {
ctx.JSONRedirect(issue.Link())
return
}
Expand Down
2 changes: 1 addition & 1 deletion services/forms/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
147 changes: 147 additions & 0 deletions tests/integration/api_pull_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ 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"
Expand All @@ -26,10 +27,12 @@ import (
"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) {
Expand Down Expand Up @@ -186,6 +189,107 @@ func TestAPIMergePullWIP(t *testing.T) {
MakeRequest(t, req, http.StatusMethodNotAllowed)
}

func TestAPIMergePull(t *testing.T) {
doCheckBranchExists := func(user *user_model.User, token, branchName string, repo *repo_model.Repository, status int) {
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", user.Name, repo.Name, branchName)).AddTokenAuth(token)
MakeRequest(t, req, status)
}

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})
session := loginUser(t, owner.Name)
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)

t.Run("Normal", func(t *testing.T) {
newBranch := "test-pull-1"
prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo)

req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{
Do: "merge",
}).AddTokenAuth(token)

MakeRequest(t, req, http.StatusOK)
doCheckBranchExists(owner, token, newBranch, repo, http.StatusOK)
// make sure we cannot perform a merge on the same PR
MakeRequest(t, req, http.StatusUnprocessableEntity)
Copy link
Contributor

@wxiaoguang wxiaoguang Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not right. StatusUnprocessableEntity is caused by reused req, it is an error caused by client, but not the "already merged" error.

The "already merged" error code is 405 (well, it is abused ... but old code does so)

doCheckBranchExists(owner, token, newBranch, repo, http.StatusOK)
})

t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) {
newBranch := "test-pull-2"
prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo)
deleteBranch := true

req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{
Do: "merge",
DeleteBranchAfterMerge: &deleteBranch,
}).AddTokenAuth(token)

MakeRequest(t, req, http.StatusOK)
doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound)
})

t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) {
newBranch := "test-pull-3"
prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo)

// set the default branch after merge setting at the repo level
prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests)
require.NoError(t, err)

prConfig := prUnit.PullRequestsConfig()
prConfig.DefaultDeleteBranchAfterMerge = true

var units []repo_model.RepoUnit
units = append(units, repo_model.RepoUnit{
RepoID: repo.ID,
Type: unit_model.TypePullRequests,
Config: prConfig,
})
require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil))

// perform merge
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{
Do: "merge",
}).AddTokenAuth(token)

MakeRequest(t, req, http.StatusOK)
doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound)
})

t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) {
newBranch := "test-pull-4"
prResp := creatPullRequestWithCommit(t, owner, token, newBranch, repo)

// make sure the default branch after merge setting is unset at the repo level
prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests)
require.NoError(t, err)

prConfig := prUnit.PullRequestsConfig()
prConfig.DefaultDeleteBranchAfterMerge = false

var units []repo_model.RepoUnit
units = append(units, repo_model.RepoUnit{
RepoID: repo.ID,
Type: unit_model.TypePullRequests,
Config: prConfig,
})
require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil))

// perform merge
deleteBranch := true
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prResp.Index), &forms.MergePullRequestForm{
Do: "merge",
DeleteBranchAfterMerge: &deleteBranch,
}).AddTokenAuth(token)

MakeRequest(t, req, http.StatusOK)
doCheckBranchExists(owner, token, newBranch, repo, http.StatusNotFound)
})
})
}

func TestAPICreatePullSuccess(t *testing.T) {
defer tests.PrepareTestEnv(t)()
repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
Expand Down Expand Up @@ -520,3 +624,46 @@ func TestAPIViewPullFilesWithHeadRepoDeleted(t *testing.T) {
})(t)
})
}

func creatPullRequestWithCommit(t *testing.T, user *user_model.User, token, newBranch string, repo *repo_model.Repository) *api.PullRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicates:

  • createFileInBranch
  • doAPICreatePullRequest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

da4bb40 uses doAPICreatePullRequest(), but I don't think I can use createFileInBranch() as it won't allow me to create a new branch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be done with some small changes 64db368

// create a new branch with a commit
filesResp, err := files_service.ChangeRepoFiles(t.Context(), repo, user, &files_service.ChangeRepoFilesOptions{
Files: []*files_service.ChangeRepoFile{
{
Operation: "create",
TreePath: strings.ReplaceAll(newBranch, " ", "_"),
ContentReader: strings.NewReader("This is a test."),
},
},
Message: "Add test file using branch name as its name",
OldBranch: repo.DefaultBranch,
NewBranch: newBranch,
Author: &files_service.IdentityOptions{
GitUserName: user.Name,
GitUserEmail: user.Email,
},
Committer: &files_service.IdentityOptions{
GitUserName: user.Name,
GitUserEmail: user.Email,
},
Dates: &files_service.CommitDateOptions{
Author: time.Now(),
Committer: time.Now(),
},
})

require.NoError(t, err)
require.NotEmpty(t, filesResp)

// create a corresponding PR
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", user.Name, repo.Name), &api.CreatePullRequestOption{
Head: newBranch,
Base: repo.DefaultBranch,
Title: "Add a test file",
}).AddTokenAuth(token)
resp := MakeRequest(t, req, http.StatusCreated)
pull := new(api.PullRequest)
DecodeJSON(t, resp, pull)

return pull
}