Skip to content

Commit 64db368

Browse files
wxiaoguangwxiaoguang
committed
fix
1 parent da4bb40 commit 64db368

File tree

6 files changed

+74
-104
lines changed

6 files changed

+74
-104
lines changed

‎routers/api/v1/repo/pull.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -939,7 +939,7 @@ func MergePullRequest(ctx *context.APIContext) {
939939
} else if errors.Is(err, pull_service.ErrNoPermissionToMerge) {
940940
ctx.APIError(http.StatusMethodNotAllowed, "User not allowed to merge PR")
941941
} else if errors.Is(err, pull_service.ErrHasMerged) {
942-
ctx.APIError(http.StatusMethodNotAllowed, "")
942+
ctx.APIError(http.StatusMethodNotAllowed, "The PR is already merged")
943943
} else if errors.Is(err, pull_service.ErrIsWorkInProgress) {
944944
ctx.APIError(http.StatusMethodNotAllowed, "Work in progress PRs cannot be merged")
945945
} else if errors.Is(err, pull_service.ErrNotMergeableState) {
@@ -991,7 +991,8 @@ func MergePullRequest(ctx *context.APIContext) {
991991
}
992992

993993
if form.MergeWhenChecksSucceed {
994-
scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false))
994+
deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false)
995+
scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge)
995996
if err != nil {
996997
if pull_model.IsErrAlreadyScheduledToAutoMerge(err) {
997998
ctx.APIError(http.StatusConflict, err)
@@ -1043,6 +1044,9 @@ func MergePullRequest(ctx *context.APIContext) {
10431044
}
10441045

10451046
// for agit flow, we should not delete the agit reference after merge
1047+
// FIXME: old code has that comment above. Is that comment valid? What would go wrong if a agit branch is deleted after merge?
1048+
// * If a agit branch can be deleted after merge, then fix the comment and maybe other related code
1049+
// * If a agit branch should not be deleted, then we need to fix the logic and add more tests
10461050
shouldDeleteBranch := pr.Flow == issues_model.PullRequestFlowGithub
10471051
if form.DeleteBranchAfterMerge != nil {
10481052
// if the form field is defined, it takes precedence over the repo setting equivalent

‎routers/web/repo/pull.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1107,7 +1107,8 @@ func MergePullRequest(ctx *context.Context) {
11071107
// delete all scheduled auto merges
11081108
_ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID)
11091109
// schedule auto merge
1110-
scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false))
1110+
deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false)
1111+
scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge)
11111112
if err != nil {
11121113
ctx.ServerError("ScheduleAutoMerge", err)
11131114
return

‎tests/integration/api_issue_config_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ func TestAPIRepoIssueConfigPaths(t *testing.T) {
129129
configData, err := yaml.Marshal(configMap)
130130
assert.NoError(t, err)
131131

132-
_, err = createFileInBranch(owner, repo, fullPath, repo.DefaultBranch, string(configData))
132+
_, err = createFile(owner, repo, fullPath, string(configData))
133133
assert.NoError(t, err)
134134

135135
issueConfig := getIssueConfig(t, owner.Name, repo.Name)

‎tests/integration/api_pull_test.go‎

Lines changed: 36 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
user_model "code.gitea.io/gitea/models/user"
2323
"code.gitea.io/gitea/modules/setting"
2424
api "code.gitea.io/gitea/modules/structs"
25+
"code.gitea.io/gitea/modules/util"
2526
"code.gitea.io/gitea/services/convert"
2627
"code.gitea.io/gitea/services/forms"
2728
"code.gitea.io/gitea/services/gitdiff"
@@ -190,49 +191,50 @@ func TestAPIMergePullWIP(t *testing.T) {
190191
}
191192

192193
func TestAPIMergePull(t *testing.T) {
193-
doCheckBranchExists := func(t *testing.T, user *user_model.User, token, branchName string, repo *repo_model.Repository, status int) {
194-
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", user.Name, repo.Name, branchName)).AddTokenAuth(token)
195-
MakeRequest(t, req, status)
196-
}
197-
198194
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
199195
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
200196
owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
201-
session := loginUser(t, owner.Name)
202-
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
197+
apiCtx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository)
203198

204-
t.Run("Normal", func(t *testing.T) {
205-
newBranch := "test-pull-1"
206-
prDTO := createPullRequestWithCommit(t, owner, newBranch, repo)
199+
checkBranchExists := func(t *testing.T, branchName string, status int) {
200+
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", owner.Name, repo.Name, branchName)).AddTokenAuth(apiCtx.Token)
201+
MakeRequest(t, req, status)
202+
}
207203

208-
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{
209-
Do: "merge",
210-
}).AddTokenAuth(token)
204+
createTestBranchPR := func(t *testing.T, branchName string) *api.PullRequest {
205+
testCreateFileInBranch(t, owner, repo, createFileInBranchOptions{NewBranch: branchName}, map[string]string{"a-new-file-" + branchName + ".txt": "dummy content"})
206+
prDTO, err := doAPICreatePullRequest(apiCtx, repo.OwnerName, repo.Name, repo.DefaultBranch, branchName)(t)
207+
require.NoError(t, err)
208+
return &prDTO
209+
}
211210

212-
MakeRequest(t, req, http.StatusOK)
213-
doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusOK)
214-
// make sure we cannot perform a merge on the same PR
215-
MakeRequest(t, req, http.StatusUnprocessableEntity)
216-
doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusOK)
211+
performMerge := func(t *testing.T, prIndex int64, deleteBranchAfterMerge *bool, optExpectedStatus ...int) {
212+
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prIndex), &forms.MergePullRequestForm{
213+
Do: "merge",
214+
DeleteBranchAfterMerge: deleteBranchAfterMerge,
215+
}).AddTokenAuth(apiCtx.Token)
216+
expectedStatus := util.OptionalArg(optExpectedStatus, http.StatusOK)
217+
MakeRequest(t, req, expectedStatus)
218+
}
219+
220+
t.Run("Normal", func(t *testing.T) {
221+
newBranch := "test-pull-1"
222+
prDTO := createTestBranchPR(t, newBranch)
223+
performMerge(t, prDTO.Index, nil)
224+
checkBranchExists(t, newBranch, http.StatusOK)
225+
performMerge(t, prDTO.Index, nil, http.StatusMethodNotAllowed) // make sure we cannot perform a merge on the same PR
217226
})
218227

219228
t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) {
220229
newBranch := "test-pull-2"
221-
prDTO := createPullRequestWithCommit(t, owner, newBranch, repo)
222-
deleteBranch := true
223-
224-
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{
225-
Do: "merge",
226-
DeleteBranchAfterMerge: &deleteBranch,
227-
}).AddTokenAuth(token)
228-
229-
MakeRequest(t, req, http.StatusOK)
230-
doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound)
230+
prDTO := createTestBranchPR(t, newBranch)
231+
performMerge(t, prDTO.Index, util.ToPointer(true))
232+
checkBranchExists(t, newBranch, http.StatusNotFound)
231233
})
232234

233235
t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) {
234236
newBranch := "test-pull-3"
235-
prDTO := createPullRequestWithCommit(t, owner, newBranch, repo)
237+
prDTO := createTestBranchPR(t, newBranch)
236238

237239
// set the default branch after merge setting at the repo level
238240
prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests)
@@ -249,18 +251,13 @@ func TestAPIMergePull(t *testing.T) {
249251
})
250252
require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil))
251253

252-
// perform merge
253-
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{
254-
Do: "merge",
255-
}).AddTokenAuth(token)
256-
257-
MakeRequest(t, req, http.StatusOK)
258-
doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound)
254+
performMerge(t, prDTO.Index, nil)
255+
checkBranchExists(t, newBranch, http.StatusNotFound)
259256
})
260257

261258
t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) {
262259
newBranch := "test-pull-4"
263-
prDTO := createPullRequestWithCommit(t, owner, newBranch, repo)
260+
prDTO := createTestBranchPR(t, newBranch)
264261

265262
// make sure the default branch after merge setting is unset at the repo level
266263
prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests)
@@ -278,14 +275,8 @@ func TestAPIMergePull(t *testing.T) {
278275
require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil))
279276

280277
// perform merge
281-
deleteBranch := true
282-
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prDTO.Index), &forms.MergePullRequestForm{
283-
Do: "merge",
284-
DeleteBranchAfterMerge: &deleteBranch,
285-
}).AddTokenAuth(token)
286-
287-
MakeRequest(t, req, http.StatusOK)
288-
doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusNotFound)
278+
performMerge(t, prDTO.Index, util.ToPointer(true))
279+
checkBranchExists(t, newBranch, http.StatusNotFound)
289280
})
290281
})
291282
}
@@ -624,41 +615,3 @@ func TestAPIViewPullFilesWithHeadRepoDeleted(t *testing.T) {
624615
})(t)
625616
})
626617
}
627-
628-
func createPullRequestWithCommit(t *testing.T, user *user_model.User, newBranch string, repo *repo_model.Repository) *api.PullRequest {
629-
// create a new branch with a commit
630-
filesResp, err := files_service.ChangeRepoFiles(t.Context(), repo, user, &files_service.ChangeRepoFilesOptions{
631-
Files: []*files_service.ChangeRepoFile{
632-
{
633-
Operation: "create",
634-
TreePath: strings.ReplaceAll(newBranch, " ", "_"),
635-
ContentReader: strings.NewReader("This is a test."),
636-
},
637-
},
638-
Message: "Add test file using branch name as its name",
639-
OldBranch: repo.DefaultBranch,
640-
NewBranch: newBranch,
641-
Author: &files_service.IdentityOptions{
642-
GitUserName: user.Name,
643-
GitUserEmail: user.Email,
644-
},
645-
Committer: &files_service.IdentityOptions{
646-
GitUserName: user.Name,
647-
GitUserEmail: user.Email,
648-
},
649-
Dates: &files_service.CommitDateOptions{
650-
Author: time.Now(),
651-
Committer: time.Now(),
652-
},
653-
})
654-
655-
require.NoError(t, err)
656-
require.NotEmpty(t, filesResp)
657-
658-
// create a corresponding PR
659-
apiCtx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository)
660-
prDTO, err := doAPICreatePullRequest(apiCtx, repo.OwnerName, repo.Name, repo.DefaultBranch, newBranch)(t)
661-
require.NoError(t, err)
662-
663-
return &prDTO
664-
}

‎tests/integration/api_repo_file_create_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ func BenchmarkAPICreateFileSmall(b *testing.B) {
133133
b.ResetTimer()
134134
for n := 0; b.Loop(); n++ {
135135
treePath := fmt.Sprintf("update/file%d.txt", n)
136-
_, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath)
136+
_, _ = createFile(user2, repo1, treePath)
137137
}
138138
})
139139
}
@@ -149,7 +149,7 @@ func BenchmarkAPICreateFileMedium(b *testing.B) {
149149
for n := 0; b.Loop(); n++ {
150150
treePath := fmt.Sprintf("update/file%d.txt", n)
151151
copy(data, treePath)
152-
_, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath)
152+
_, _ = createFile(user2, repo1, treePath)
153153
}
154154
})
155155
}

‎tests/integration/api_repo_file_helpers.go‎

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,26 +6,36 @@ package integration
66
import (
77
"context"
88
"strings"
9+
"testing"
910

1011
repo_model "code.gitea.io/gitea/models/repo"
1112
user_model "code.gitea.io/gitea/models/user"
1213
api "code.gitea.io/gitea/modules/structs"
14+
"code.gitea.io/gitea/modules/util"
1315
files_service "code.gitea.io/gitea/services/repository/files"
16+
17+
"github.com/stretchr/testify/require"
1418
)
1519

16-
func createFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, branchName, content string) (*api.FilesResponse, error) {
20+
type createFileInBranchOptions struct {
21+
OldBranch, NewBranch string
22+
}
23+
24+
func testCreateFileInBranch(t *testing.T, user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) *api.FilesResponse {
25+
resp, err := createFileInBranch(user, repo, createOpts, files)
26+
require.NoError(t, err)
27+
return resp
28+
}
29+
30+
func createFileInBranch(user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) (*api.FilesResponse, error) {
1731
ctx := context.TODO()
18-
opts := &files_service.ChangeRepoFilesOptions{
19-
Files: []*files_service.ChangeRepoFile{
20-
{
21-
Operation: "create",
22-
TreePath: treePath,
23-
ContentReader: strings.NewReader(content),
24-
},
25-
},
26-
OldBranch: branchName,
27-
Author: nil,
28-
Committer: nil,
32+
opts := &files_service.ChangeRepoFilesOptions{OldBranch: createOpts.OldBranch, NewBranch: createOpts.NewBranch}
33+
for path, content := range files {
34+
opts.Files = append(opts.Files, &files_service.ChangeRepoFile{
35+
Operation: "create",
36+
TreePath: path,
37+
ContentReader: strings.NewReader(content),
38+
})
2939
}
3040
return files_service.ChangeRepoFiles(ctx, repo, user, opts)
3141
}
@@ -53,10 +63,12 @@ func createOrReplaceFileInBranch(user *user_model.User, repo *repo_model.Reposit
5363
return err
5464
}
5565

56-
_, err = createFileInBranch(user, repo, treePath, branchName, content)
66+
_, err = createFileInBranch(user, repo, createFileInBranchOptions{OldBranch: branchName}, map[string]string{treePath: content})
5767
return err
5868
}
5969

60-
func createFile(user *user_model.User, repo *repo_model.Repository, treePath string) (*api.FilesResponse, error) {
61-
return createFileInBranch(user, repo, treePath, repo.DefaultBranch, "This is a NEW file")
70+
// TODO: replace all usages of this function with testCreateFileInBranch or testCreateFile
71+
func createFile(user *user_model.User, repo *repo_model.Repository, treePath string, optContent ...string) (*api.FilesResponse, error) {
72+
content := util.OptionalArg(optContent, "file content "+treePath)
73+
return createFileInBranch(user, repo, createFileInBranchOptions{}, map[string]string{treePath: content})
6274
}

0 commit comments

Comments
 (0)