Add API endpoint to get changed files of a PR

[qwerty287]

This adds an api endpoint /files to PRs that allows to get a list of changed files.

built upon #18228, reviews there are included closes https://github.com/go-gitea/gitea/issues/654

[6543]

// GitHub API docs: https://docs.github.com/en/rest/pulls/pulls#list-pull-requests-files

[6543]

one or two api calls to this endpoint as test in integration tests would be nice ... ... api regressions do happen if we dont do this

[6543]

beside missing test's (and the undocumented option) i would say it's ok ...

[lunny]

Per https://docs.github.com/en/rest/pulls/pulls#list-pull-requests-files ,

[
  {
    "sha": "bbcd538c8e72b8c175046e27cc8f907076331401",
    "filename": "file1.txt",
    "status": "added",
    "additions": 103,
    "deletions": 21,
    "changes": 124,
    "blob_url": "https://github.com/octocat/Hello-World/blob/6dcb09b5b57875f334f61aebed695e2e4193db5e/file1.txt",
    "raw_url": "https://github.com/octocat/Hello-World/raw/6dcb09b5b57875f334f61aebed695e2e4193db5e/file1.txt",
    "contents_url": "https://api.github.com/repos/octocat/Hello-World/contents/file1.txt?ref=6dcb09b5b57875f334f61aebed695e2e4193db5e",
    "patch": "@@ -132,7 +132,7 @@ module Test @@ -1000,7 +1000,7 @@ module Test"
  }
]

Keep compatible with Github API maybe a good idea.

[qwerty287]

I don't really think so. the difference is sha, blob_url and patch. We don't have a blob_url, and adding a patch is also not really good if there are huge canges. Just use the raw diff endpoint to get the actual file difference. Only sha might be useful, but I'm not sure how to add it.

[6543]

Adding the specific sha can be done later...

[qwerty287]

Regarding the test, it's failing: how can I modify a repo or edit a file? It fails when trying this: https://drone.gitea.io/go-gitea/gitea/60577/2/14

[6543]

I would just create & initialize a new repo for the test via api and delete it afterwards

[qwerty287]

I tried that, but it didn't work as well (locally). Any ideas?

[delvh]

@6543 I disagree with not sending the name hash as well. As we have seen in #21230, there are definitely uses for it.

---

Furthermore, I've noticed one thing: From the current implementation, it is straightforward to extend this API call with the IsViewed and HasChangedSinceLastReview attributes for each file. There are only five things needed to implement it:

1. Enhance the ChangedFile struct
2. Set the values in struct instances (provided by DiffFile)
3. Get user ID from the request
4. if user is anonymous: Use current behavior
5. otherwise, change the call gitdiff.GetDiff to gitdiff.SyncAndGetUserSpecificDiff

Then, we would already have a read-only API for viewed states of files, and another PR could add a POST route. Or is that too out of scope for this PR?

[wxiaoguang]

I do not see any useful purpose of sending the name hash. This API is for CI/CD purpose, I do not think it's necessary to make it too complex.

My only concern is that he name HTMLURL string `json:"html_url,omitempty"` is quite misleading.

IMO HTMLURL is a pretty bad name (unclear, although it was used in history). I am not sure whether people really like this name ..... As a new API, could there be a better name? Just a question, not a blocker.

No answer to this question, does it mean everyone like the name html_url?

[qwerty287]

I don't like it, but I don't have another idea.

[delvh]

same

[delvh]

@wxiaoguang the benefit is the following: We have already calculated the hash, meaning it is inexpensive for us to send that. And if users want to use it for any reason they can then. I'd also be more opposed to it if we had to calculate the hash only to send it, but as it is, I don't see a reason not to send it.

[qwerty287]

I think I agree here. There's no disadvantage of sending, to why not. I need to see when I get some time to implement it, don't have access to a computer right now.

[wxiaoguang]

For html_url, I think it's acceptable now since there is no better name, and it's only suitable to be used in HTML. So let's continue.

But for the name hash, I have strong objection. The reason is:

1. If nobody knows how the name hash would be used, it's just redundancy and would be wrong in the end. As an API response, it should be designed carefully with real cases.
2. If it's useful, it could be added at any time in the future with a clear design and document.
3. Take a look at GitHub's API, they have a sha in response, which seems to be the git object id, it's not the hash of the name. They do not send the name hash either.

(Update: I believe in https://en.wikipedia.org/wiki/Occam's_razor "entities should not be multiplied beyond necessity")

---

From my side, the only necessary thing to do for this PR is to pass the CI tests (maybe other nits could be improved later)

[qwerty287]

Any ideas how I could fix the test? Nothing I tried worked locally.

[delvh]

Well, according to the message, pull.Base is nil in line 206… But I don't know why it shouldn't be deserialized from the JSON.

[qwerty287]

The issue is that it can't modify the test repo. The error is

UpdateFile: PushRejected Error: exit status 1: remote: 

remote: Gitea: Internal Server Error        

To /tmp/tmp.yDfs8wh5DG/tests/integration/gitea-integration-mysql/gitea-repositories/user12/repo10.git

! [remote rejected] 57bafb39742effb4e08a865723696cbde77e0d9d -> get-pull-files-test-branch (pre-receive hook declined)

error: failed to push some refs to '/tmp/tmp.yDfs8wh5DG/tests/integration/gitea-integration-mysql/gitea-repositories/user12/repo10.git

How can I fix this?

[anbraten]

Maybe something like ui_url instead of html_url. If you all are not happy with the name I would suggest to leave it out for now as extending api results is much easier as breaking them later again.

Same for the hashed name. I guess as you already get the filename and the previous filename hashing that name could also easily be done in the client / SSR part where needed.

[6543]

☝️ that s my view too

[6543]

The issue is that it can't modify the test repo. The error is

UpdateFile: PushRejected Error: exit status 1: remote: 

remote: Gitea: Internal Server Error        

To /tmp/tmp.yDfs8wh5DG/tests/integration/gitea-integration-mysql/gitea-repositories/user12/repo10.git

! [remote rejected] 57bafb39742effb4e08a865723696cbde77e0d9d -> get-pull-files-test-branch (pre-receive hook declined)

error: failed to push some refs to '/tmp/tmp.yDfs8wh5DG/tests/integration/gitea-integration-mysql/gitea-repositories/user12/repo10.git

How can I fix this?

I'll have a look

[lafriks]

Maybe something like ui_url instead of html_url. If you all are not happy with the name I would suggest to leave it out for now as extending api results is much easier as breaking them later again.

GitHub and in other our API methods use html_url for such things (even tho this exact github api have no such field), so I would not mind if we had it.

Example: https://github.com/go-gitea/gitea/blob/692707f14519b677de4601e5f40469989f82eed6/modules/structs/repo_file.go#L104

[6543]

I'll have a look

fixture and actual git test data missmatched too much

I just used another repo with an already existing pull where i just had to correct two lines to make it work (models/fixtures/pull_request.yml)

and I switched to declarative testing schema ... we long term should do so per topic completly