Skip to content

Commit 761b9d4

Browse files
silverwindclaudewxiaoguangGiteaBot
authored
Fix API not persisting pull request unit config when has_pull_requests is not set (#36718)
The `PATCH /api/v1/repos/{owner}/{repo}` endpoint silently ignores pull request config fields (like `default_delete_branch_after_merge`, `allow_squash_merge`, etc.) unless `has_pull_requests: true` is also included in the request body. This is because the entire PR unit config block was gated behind `if opts.HasPullRequests != nil`. This PR restructures the logic so that PR config options are applied whenever the pull request unit already exists on the repo, without requiring `has_pull_requests` to be explicitly set. A new unit is only created when `has_pull_requests: true` is explicitly sent. Fixes #36466 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: Giteabot <teabot@gitea.io>
1 parent 054eb6d commit 761b9d4

8 files changed

Lines changed: 91 additions & 98 deletions

File tree

‎models/repo/repo_unit.go‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,25 @@ type PullRequestsConfig struct {
134134
DefaultTargetBranch string
135135
}
136136

137+
func DefaultPullRequestsConfig() *PullRequestsConfig {
138+
cfg := &PullRequestsConfig{
139+
AllowMerge: true,
140+
AllowRebase: true,
141+
AllowRebaseMerge: true,
142+
AllowSquash: true,
143+
AllowFastForwardOnly: true,
144+
AllowRebaseUpdate: true,
145+
DefaultAllowMaintainerEdit: true,
146+
}
147+
cfg.DefaultMergeStyle = MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle)
148+
cfg.DefaultMergeStyle = util.IfZero(cfg.DefaultMergeStyle, MergeStyleMerge)
149+
return cfg
150+
}
151+
137152
// FromDB fills up a PullRequestsConfig from serialized format.
138153
func (cfg *PullRequestsConfig) FromDB(bs []byte) error {
139-
// AllowRebaseUpdate = true as default for existing PullRequestConfig in DB
140-
cfg.AllowRebaseUpdate = true
154+
// set default values for existing PullRequestConfig in DB
155+
*cfg = *DefaultPullRequestsConfig()
141156
return json.UnmarshalHandleDoubleEncode(bs, &cfg)
142157
}
143158

@@ -156,17 +171,8 @@ func (cfg *PullRequestsConfig) IsMergeStyleAllowed(mergeStyle MergeStyle) bool {
156171
mergeStyle == MergeStyleManuallyMerged && cfg.AllowManualMerge
157172
}
158173

159-
// GetDefaultMergeStyle returns the default merge style for this pull request
160-
func (cfg *PullRequestsConfig) GetDefaultMergeStyle() MergeStyle {
161-
if len(cfg.DefaultMergeStyle) != 0 {
162-
return cfg.DefaultMergeStyle
163-
}
164-
165-
if setting.Repository.PullRequest.DefaultMergeStyle != "" {
166-
return MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle)
167-
}
168-
169-
return MergeStyleMerge
174+
func DefaultPullRequestsUnit(repoID int64) RepoUnit {
175+
return RepoUnit{RepoID: repoID, Type: unit.TypePullRequests, Config: DefaultPullRequestsConfig()}
170176
}
171177

172178
type ActionsConfig struct {

‎modules/optional/option.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,17 @@ func ParseBool(s string) Option[bool] {
6767
}
6868
return Some(v)
6969
}
70+
71+
func AssignPtrValue[T comparable](changed *bool, target, src *T) {
72+
if src != nil && *src != *target {
73+
*target = *src
74+
*changed = true
75+
}
76+
}
77+
78+
func AssignPtrString[TO, FROM ~string](changed *bool, target *TO, src *FROM) {
79+
if src != nil && string(*src) != string(*target) {
80+
*target = TO(*src)
81+
*changed = true
82+
}
83+
}

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

Lines changed: 34 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -884,77 +884,44 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
884884
}
885885
}
886886

887-
if opts.HasPullRequests != nil && !unit_model.TypePullRequests.UnitGlobalDisabled() {
888-
if *opts.HasPullRequests {
889-
// We do allow setting individual PR settings through the API, so
890-
// we get the config settings and then set them
891-
// if those settings were provided in the opts.
887+
if !unit_model.TypePullRequests.UnitGlobalDisabled() {
888+
mustDeletePullRequestUnit := opts.HasPullRequests != nil && !*opts.HasPullRequests
889+
mustInsertPullRequestUnit := opts.HasPullRequests != nil && *opts.HasPullRequests
890+
if mustDeletePullRequestUnit {
891+
deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests)
892+
} else {
893+
// We do allow setting individual PR settings through the API,
894+
// so we get the config settings and then set them if those settings were provided in the opts.
892895
unit, err := repo.GetUnit(ctx, unit_model.TypePullRequests)
893-
var config *repo_model.PullRequestsConfig
894-
if err != nil {
895-
// Unit type doesn't exist so we make a new config file with default values
896-
config = &repo_model.PullRequestsConfig{
897-
IgnoreWhitespaceConflicts: false,
898-
AllowMerge: true,
899-
AllowRebase: true,
900-
AllowRebaseMerge: true,
901-
AllowSquash: true,
902-
AllowFastForwardOnly: true,
903-
AllowManualMerge: true,
904-
AutodetectManualMerge: false,
905-
AllowRebaseUpdate: true,
906-
DefaultDeleteBranchAfterMerge: false,
907-
DefaultMergeStyle: repo_model.MergeStyleMerge,
908-
DefaultAllowMaintainerEdit: false,
909-
}
910-
} else {
911-
config = unit.PullRequestsConfig()
912-
}
913-
914-
if opts.IgnoreWhitespaceConflicts != nil {
915-
config.IgnoreWhitespaceConflicts = *opts.IgnoreWhitespaceConflicts
916-
}
917-
if opts.AllowMerge != nil {
918-
config.AllowMerge = *opts.AllowMerge
919-
}
920-
if opts.AllowRebase != nil {
921-
config.AllowRebase = *opts.AllowRebase
922-
}
923-
if opts.AllowRebaseMerge != nil {
924-
config.AllowRebaseMerge = *opts.AllowRebaseMerge
925-
}
926-
if opts.AllowSquash != nil {
927-
config.AllowSquash = *opts.AllowSquash
928-
}
929-
if opts.AllowFastForwardOnly != nil {
930-
config.AllowFastForwardOnly = *opts.AllowFastForwardOnly
931-
}
932-
if opts.AllowManualMerge != nil {
933-
config.AllowManualMerge = *opts.AllowManualMerge
934-
}
935-
if opts.AutodetectManualMerge != nil {
936-
config.AutodetectManualMerge = *opts.AutodetectManualMerge
937-
}
938-
if opts.AllowRebaseUpdate != nil {
939-
config.AllowRebaseUpdate = *opts.AllowRebaseUpdate
940-
}
941-
if opts.DefaultDeleteBranchAfterMerge != nil {
942-
config.DefaultDeleteBranchAfterMerge = *opts.DefaultDeleteBranchAfterMerge
943-
}
944-
if opts.DefaultMergeStyle != nil {
945-
config.DefaultMergeStyle = repo_model.MergeStyle(*opts.DefaultMergeStyle)
896+
if err != nil && !errors.Is(err, util.ErrNotExist) {
897+
return err
946898
}
947-
if opts.DefaultAllowMaintainerEdit != nil {
948-
config.DefaultAllowMaintainerEdit = *opts.DefaultAllowMaintainerEdit
899+
if unit == nil {
900+
// Unit doesn't exist yet but is being enabled, create with defaults
901+
unit = new(repo_model.DefaultPullRequestsUnit(repo.ID))
949902
}
950903

951-
units = append(units, repo_model.RepoUnit{
952-
RepoID: repo.ID,
953-
Type: unit_model.TypePullRequests,
954-
Config: config,
955-
})
956-
} else {
957-
deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests)
904+
changed := new(false)
905+
config := unit.PullRequestsConfig()
906+
optional.AssignPtrValue(changed, &config.IgnoreWhitespaceConflicts, opts.IgnoreWhitespaceConflicts)
907+
optional.AssignPtrValue(changed, &config.AllowMerge, opts.AllowMerge)
908+
optional.AssignPtrValue(changed, &config.AllowRebase, opts.AllowRebase)
909+
optional.AssignPtrValue(changed, &config.AllowRebaseMerge, opts.AllowRebaseMerge)
910+
optional.AssignPtrValue(changed, &config.AllowSquash, opts.AllowSquash)
911+
optional.AssignPtrValue(changed, &config.AllowFastForwardOnly, opts.AllowFastForwardOnly)
912+
optional.AssignPtrValue(changed, &config.AllowManualMerge, opts.AllowManualMerge)
913+
optional.AssignPtrValue(changed, &config.AutodetectManualMerge, opts.AutodetectManualMerge)
914+
optional.AssignPtrValue(changed, &config.AllowRebaseUpdate, opts.AllowRebaseUpdate)
915+
optional.AssignPtrValue(changed, &config.DefaultDeleteBranchAfterMerge, opts.DefaultDeleteBranchAfterMerge)
916+
optional.AssignPtrValue(changed, &config.DefaultAllowMaintainerEdit, opts.DefaultAllowMaintainerEdit)
917+
optional.AssignPtrString(changed, &config.DefaultMergeStyle, opts.DefaultMergeStyle)
918+
if *changed || mustInsertPullRequestUnit {
919+
units = append(units, repo_model.RepoUnit{
920+
RepoID: repo.ID,
921+
Type: unit_model.TypePullRequests,
922+
Config: config,
923+
})
924+
}
958925
}
959926
}
960927

‎routers/web/repo/issue_view.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -905,9 +905,8 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss
905905
// Check correct values and select default
906906
if ms, ok := ctx.Data["MergeStyle"].(repo_model.MergeStyle); !ok ||
907907
!prConfig.IsMergeStyleAllowed(ms) {
908-
defaultMergeStyle := prConfig.GetDefaultMergeStyle()
909-
if prConfig.IsMergeStyleAllowed(defaultMergeStyle) && !ok {
910-
mergeStyle = defaultMergeStyle
908+
if prConfig.IsMergeStyleAllowed(prConfig.DefaultMergeStyle) && !ok {
909+
mergeStyle = prConfig.DefaultMergeStyle
911910
} else if prConfig.AllowMerge {
912911
mergeStyle = repo_model.MergeStyleMerge
913912
} else if prConfig.AllowRebase {

‎services/convert/repository.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ func innerToRepo(ctx context.Context, repo *repo_model.Repository, permissionInR
118118
allowManualMerge = config.AllowManualMerge
119119
autodetectManualMerge = config.AutodetectManualMerge
120120
defaultDeleteBranchAfterMerge = config.DefaultDeleteBranchAfterMerge
121-
defaultMergeStyle = config.GetDefaultMergeStyle()
121+
defaultMergeStyle = config.DefaultMergeStyle
122122
defaultAllowMaintainerEdit = config.DefaultAllowMaintainerEdit
123123
defaultTargetBranch = config.DefaultTargetBranch
124124
}

‎services/repository/create.go‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -386,15 +386,7 @@ func createRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *r
386386
},
387387
})
388388
case unit.TypePullRequests:
389-
units = append(units, repo_model.RepoUnit{
390-
RepoID: repo.ID,
391-
Type: tp,
392-
Config: &repo_model.PullRequestsConfig{
393-
AllowMerge: true, AllowRebase: true, AllowRebaseMerge: true, AllowSquash: true, AllowFastForwardOnly: true,
394-
DefaultMergeStyle: repo_model.MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle),
395-
AllowRebaseUpdate: true,
396-
},
397-
})
389+
units = append(units, repo_model.DefaultPullRequestsUnit(repo.ID))
398390
case unit.TypeProjects:
399391
units = append(units, repo_model.RepoUnit{
400392
RepoID: repo.ID,

‎tests/integration/api_pull_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -279,10 +279,10 @@ func TestAPICreatePullSuccess(t *testing.T) {
279279
}).AddTokenAuth(token)
280280
MakeRequest(t, req, http.StatusCreated)
281281

282-
// Also test that AllowMaintainerEdit is false by default
282+
// Also test that AllowMaintainerEdit is true by default, the "false" case is covered by TestAPICreatePullBasePermission
283283
prIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: prTitle})
284284
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: prIssue.ID})
285-
assert.False(t, pr.AllowMaintainerEdit)
285+
assert.True(t, pr.AllowMaintainerEdit)
286286

287287
MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail
288288
}
@@ -304,7 +304,7 @@ func TestAPICreatePullBasePermission(t *testing.T) {
304304
Base: "master",
305305
Title: prTitle,
306306

307-
AllowMaintainerEdit: new(true),
307+
AllowMaintainerEdit: new(false),
308308
}
309309
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
310310
MakeRequest(t, req, http.StatusForbidden)
@@ -317,10 +317,10 @@ func TestAPICreatePullBasePermission(t *testing.T) {
317317
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
318318
MakeRequest(t, req, http.StatusCreated)
319319

320-
// Also test that AllowMaintainerEdit is set to true
320+
// Also test that AllowMaintainerEdit is set to false, the default "true" case is covered by TestAPICreatePullSuccess
321321
prIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: prTitle})
322322
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: prIssue.ID})
323-
assert.True(t, pr.AllowMaintainerEdit)
323+
assert.False(t, pr.AllowMaintainerEdit)
324324
}
325325

326326
func TestAPICreatePullHeadPermission(t *testing.T) {

‎tests/integration/api_repo_edit_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,5 +418,20 @@ func TestAPIRepoEdit(t *testing.T) {
418418
req = NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/repos/%s/%s", user2.Name, repo1.Name), &repoEditOption).
419419
AddTokenAuth(token4)
420420
MakeRequest(t, req, http.StatusForbidden)
421+
422+
// Test updating pull request settings without setting has_pull_requests
423+
repo1 = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
424+
url = fmt.Sprintf("/api/v1/repos/%s/%s", user2.Name, repo1.Name)
425+
req = NewRequestWithJSON(t, "PATCH", url, &api.EditRepoOption{
426+
DefaultDeleteBranchAfterMerge: &bTrue,
427+
}).AddTokenAuth(token2)
428+
resp = MakeRequest(t, req, http.StatusOK)
429+
DecodeJSON(t, resp, &repo)
430+
assert.True(t, repo.DefaultDeleteBranchAfterMerge)
431+
// reset
432+
req = NewRequestWithJSON(t, "PATCH", url, &api.EditRepoOption{
433+
DefaultDeleteBranchAfterMerge: &bFalse,
434+
}).AddTokenAuth(token2)
435+
_ = MakeRequest(t, req, http.StatusOK)
421436
})
422437
}

0 commit comments

Comments
 (0)