Feat add Get pull requests associated to a commit's SHA#36617
Conversation
|
Revert |
There was a problem hiding this comment.
Pull request overview
This pull request adds a new API endpoint to retrieve all pull requests associated with a specific commit SHA, addressing issue #36613. The feature mirrors GitHub's /commits/{sha}/pulls endpoint, enabling users to discover which PRs contain a given commit.
Changes:
- Added
GET /repos/{owner}/{repo}/commits/{sha}/pullsAPI endpoint that returns an array of pull requests - Created
GetPullRequestsByMergedCommitmodel function to query PRs by merged commit ID - Added comprehensive integration tests covering merged commits, commits without PRs, and invalid/nonexistent commits
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| routers/api/v1/api.go | Registers the new /commits/{sha}/pulls route with appropriate permissions |
| routers/api/v1/repo/commits.go | Implements the GetCommitPullRequests API handler with conversion logic |
| models/issues/pull.go | Adds GetPullRequestsByMergedCommit database query function |
| templates/swagger/v1_json.tmpl | Documents the new endpoint in the Swagger specification |
| tests/integration/api_repo_get_commit_pull_request_test.go | Provides integration tests for various scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| } | ||
|
|
||
| baseRepo := ctx.Repo.Repository |
There was a problem hiding this comment.
It seems it missed the permission check. It needs a read permission of pull request.
There was a problem hiding this comment.
Thanks for the review, could you clarify more?
The route is inside /commits PathGroup which already does the check using reqRepoReader.
Should I add an inline check for read permission?
or did I misunderstand something.
and thank you again.
There was a problem hiding this comment.
Just add a permission check inside the function. The endpoint is special because it needs both code unit permission and pull request unit permission.
Signed-off-by: kevo-1 <kevin.bastawrous@gmail.com>
Signed-off-by: kevo-1 <kevin.bastawrous@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kevin <154337845+kevo-1@users.noreply.github.com>
Signed-off-by: Kevin <154337845+kevo-1@users.noreply.github.com>
Signed-off-by: kevo-1 <kevin.bastawrous@gmail.com>
1bbb427 to
a4f0c00
Compare
|
Before continuing, compare your |
|
I think compat with GitHub's path at |
Get every PR that contains a commit SHA in the queried repo and it's branches Signed-off-by: kevo-1 <kevin.bastawrous@gmail.com>
Signed-off-by: kevo-1 <kevin.bastawrous@gmail.com>
|
As I have misunderstood what the proposal was actually asking for, this commit adds the |
I guess different branches could have the same commitid. And some pull requests could have the same merged commitids because the base branch might be different. |
|
This comment was written by Claude Code on behalf of @silverwind. Review:
|
Existing /pull (singular) |
New /pulls (plural) |
|
|---|---|---|
| Returns | Single PullRequest object |
Array of PullRequest objects |
| Scope | Only finds the PR whose merge commit matches the SHA | Finds PRs by merge commit and by branch containment (any PR whose head branch contains the commit) |
| On miss | Returns 404 |
Returns empty array [] |
| GitHub compat | No GitHub equivalent | Matches GitHub's GET /repos/{owner}/{repo}/commits/{commit_sha}/pulls |
Does it make sense to have both?
Yes, having both endpoints is justified:
-
They have different semantics.
/pullanswers "which PR was this SHA the merge commit of?" (a narrow, specific question)./pullsanswers "which PRs contain this commit?" (a broader question). The GitHub API only has the broader one. -
GitHub API compatibility. GitHub's endpoint is at
/pulls(plural), returns an array, and finds PRs both by merge commit and by branch containment — exactly what this PR implements. Adding/pullsgives tooling that targets the GitHub API (CI tools, scripts) a compatible endpoint. -
Backward compatibility. The existing
/pullendpoint is already in use. Removing or changing its return type from a single object to an array would be a breaking change. Keeping it as-is is correct.
Issues worth addressing
-
GetPullRequestsByHeadBranchonly matches onhead_repo_id, which means it only finds PRs originating from the same repo. For forked PRs, the head repo ID differs from the base repo ID, so those would be missed. The query should likely also (or instead) considerbase_repo_id, or the function should accept both IDs. -
No pagination. GitHub's equivalent supports
per_pageandpage. The new endpoint returns everything in one response. For repos with many PRs touching the same branches, this could be large. Worth adding pagination viautils.GetListOptions(). -
git for-each-ref --containscan be slow on large repositories with many branches. There's no timeout or limit on the number of branches returned. This could be a performance concern. -
Error swallowing. When
GetBranchesContainingfails (e.g., invalid SHA), the error is silently ignored andbranchesis set tonil. This means an invalid SHA returns200 OKwith an empty array rather than a404. GitHub does the same, so this may be intentional, but worth being explicit about.
|
@kevo-1 check the 4 points above. I think they are all worth addressing. |
I don't think it is true, the author of #36613 asked for:
The issue reporter's requirement is already in Gitea #29243 , the author just didn't find it. I don't see a real world use case for your new change. |
The use case seems to be (from #36617 (comment)):
I think it might be a match to the original issue actually. |
-Changed the GetPullRequestsByHeadBranch to also match on head repo id and base repo id -Applied Pagination to PRs list -The error swallowing behaviour is intended to match Github's API behaviour Signed-off-by: kevo-1 <kevin.bastawrous@gmail.com>
Signed-off-by: kevo-1 <kevin.bastawrous@gmail.com>
For the 4th issue, it was intentional to match Github's API behaviour, but I added comments to clarify that the behavior was intentional. |
But why? Can you show a real world case that such behavior is useful? And there is another question: if a repo contains thousands of PRs, and you query the first "init" commit, then all these thousands of PRs will be in the response? Does it make sense? Will it just cause DoS and take down the server? It is a kind of laziness and ignorance that only guess and imagine something is right but not really investigate into it. |
|
I fully understand the difference now: A commit can be theoretically be contained in multiple PRs and only |
Does GitHub really work this way? Besides the problems mentioned above, the current logic is also not right, for example: the PR branches usually are deleted after merge, then |
Info on when GitHub's List pull requests associated with a commit API returns multiple pull requests in the array (written by Claude):
The most common real-world scenario for multiple results is case 1: querying a commit that only lives on feature branches, where it may be associated with both an open PR and a previously merged PR. |
Do you really understand how GitHub works, or have you really tested its API? Or it is just your guess and imagination? |
I created a reproduction on my test repo, check this: $ curl -s -H "Accept: application/vnd.github+json" "https://api.github.com/repos/silverwind/symlink-test/commits/3cd304bb35c220fe69879c3d1d468d9f5c1b472f/pulls" | jq length
2This clearly demonstrates our singular |
|
Do you think |
I don't have a opinion on that, but see #36617 (comment) about it. |
So you mean you haven't really understood this PR's behavior and GitHub's behavior? And you haven't really tested them either? But just copy-paste the crap from AI? For what I can see, GitHub's behavior is useful. But this PR's behavior isn't and it is wrong. |
I fully understand the API and I have demonstrated how it works. I don't care about this feature itself so I won't waste time diving into the code myself.
Then raise these concerns clearly, just saying "wrong" is not constructive feedback that can be acted upon. |
The same to me: I don't care about this feature itself so I won't waste time diving into the code myself. I can tell it is wrong, and I have explained again and again above (#36617 (comment) , #36617 (comment) , #36617 (comment)), you just don't understand it. |
|
Yeah then let's wait on the author to resolve :) |
You're right that GetBranchesContaining + GetPullRequestsByHeadBranch is not fully equivalent to GitHub’s implementation. Would you like me to:
|
|
It's up to you.
|
Would you care to emphasize more what you expect the |
GitHub is here, you can try to figure out its design by testing on it directly, or if you need this feature, you should be able to know what its behavior should be. I only occasionally review PRs to prevent from design problems. For the features I don't need, I don't have a full picture either. |
Add API endpoint to get pull requests associated with a commit
Fixes #36613
Changes
GET /repos/{owner}/{repo}/commits/{sha}/pullscommit_idAPI Usage
GET /api/v1/repos/{owner}/{repo}/commits/{sha}/pullsResponse: Array of pull requests or empty array
[]Testing