PATCH /api/v1/repos/OWNER/REPO fails

[laf0rge]

Description

I'm trying to use the API to programmatically update the default_merge_style to 'rebase'.

The HTTP PATCH looks rather simple, I'm doing something like this from python:

When enabling the HTTPConnection.debuglevel of the python client, I get:

send: b'PATCH /api/v1/repos/laforge/3gpp-etsi-pdf-links HTTP/1.1\r\nHost: gitea.osmocom.org\r\nUser-Agent: python-requests/2.27.1\r\nAccept-Encoding: gzip, deflate, br\r\nAccept: */*\r\nConnection: keep-alive\r\nCookie: _csrf=rSUK77i1f32Y-foobar; i_like_gitea=foobar\r\nContent-Length: 33\r\nContent-Type: application/json\r\nAuthorization: token foobar\r\n\r\n'
send: b'{"default_merge_style": "rebase"}'
reply: 'HTTP/1.1 200 OK\r\n'

So what's odd here is

• the PATCH body definitely contains the {"default_merge_style": "rebase"}
• the HTTP status code is 200
• the change is not visible afterwards, not via the web UI
• the JSON returned in the response again states {"default_merge_style": "merge"} - which was the setting before issuing the patch.

The same problem is also observed when trying to change the allow_squash_merge in a similar PATCH

Gitea Version

1.16.8

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

Debian GNU/Linux 11

How are you running Gitea?

gitea docker container

Database

SQLite

[noerw]

This is a rather unfriendly 'feature' of the API — the request needs to contain "has_pull_requests": true for changes to PR settings to be accepted: https://github.com/go-gitea/gitea/blob/c9c5bd88beb30b06cf52444e6515ef4c7d75993f/routers/api/v1/repo/repo.go#L834 ...as is also noted in the docs: https://github.com/go-gitea/gitea/blob/c9c5bd88beb30b06cf52444e6515ef4c7d75993f/modules/structs/repo.go#L186-L187

---

As this usage error comes up repeatedly (#13620 #15121 #18773 #19069), I take this opportunity to discuss if

1. this check actually enables any use case that omitting the requirement for has_pull_requests: true would not?
2. changing this behavior for better API UX is acceptable or a breaking change to avoid?

Opinions? @6543

Edit: sample for proposed new behaviour:

diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go
index cdd1f7d5c..1ba97c637 100644
--- a/routers/api/v1/repo/repo.go
+++ b/routers/api/v1/repo/repo.go
@@ -830,8 +830,9 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
 		}
 	}
 
-	if opts.HasPullRequests != nil {
-		if *opts.HasPullRequests && !unit_model.TypePullRequests.UnitGlobalDisabled() {
+	if opts.HasPullRequests != nil && !*opts.HasPullRequests {
+		deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests)
+	} else if !unit_model.TypePullRequests.UnitGlobalDisabled() {
 			// We do allow setting individual PR settings through the API, so
 			// we get the config settings and then set them
 			// if those settings were provided in the opts.
@@ -891,9 +892,6 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
 				Type:   unit_model.TypePullRequests,
 				Config: config,
 			})
-		} else if !*opts.HasPullRequests && !unit_model.TypePullRequests.UnitGlobalDisabled() {
-			deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests)
-		}
 	}
 
 	if opts.HasProjects != nil && !unit_model.TypeProjects.UnitGlobalDisabled() {

[6543]

I would accept a patch if submited

[laf0rge]

This is a rather unfriendly 'feature' of the API — the request needs to contain "has_pull_requests": true for changes to PR settings to be accepted:

This is really odd. I read the related documentation, but I understood it as "you can only set this setting if the existing repo already has "has_pull_requests": true. That of course makes sense, why would you be able to configure details of merging pull requests, if you have no pull requests.

But having to send that "has_pull_requests": true via the API for a repo that already has this flag true before issuing the API request is really unexpected. It would never have occurred to me that this needs to be done, even after reading the documentation.

Thanks for the clarification.

[jolheiser]

Parent issue #13622