Skip to content

Fix: GitHub branch pagination#98

Merged
HarshMN2345 merged 8 commits into
mainfrom
fix/github-branches-pagination
May 14, 2026
Merged

Fix: GitHub branch pagination#98
HarshMN2345 merged 8 commits into
mainfrom
fix/github-branches-pagination

Conversation

@HarshMN2345
Copy link
Copy Markdown
Member

No description provided.

@HarshMN2345 HarshMN2345 requested a review from Meldiron May 11, 2026 11:06
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 11, 2026

Greptile Summary

This PR adds configurable pagination to GitHub::listBranches and implements the previously-stubbed createBranch method, accompanied by integration tests covering paginated fetches, empty repositories, and non-existent repos.

  • listBranches now accepts $perPage and $page parameters (defaulting to 100 and 1) and correctly guards against non-2xx responses before mapping branch names.
  • createBranch is newly implemented: it fetches the tip commit of $oldBranchName via getLatestCommit and POSTs a new ref to the GitHub API, but unlike listBranches it does not check the HTTP status code, so API errors (duplicate branch, missing permissions) are returned silently as plain arrays.
  • The new tests use assertEqualsCanonicalizing for the full-list assertion, which is correct, but the per-page assertSame assertions depend implicitly on GitHub's alphabetical branch ordering.

Confidence Score: 4/5

Safe to merge after adding error handling to createBranch; the pagination logic in listBranches itself is correct.

The createBranch implementation omits the HTTP status-code check that was explicitly added to listBranches in this same PR, meaning the method silently returns GitHub error payloads on failure (duplicate branch, 403, etc.).

src/VCS/Adapter/Git/GitHub.php — specifically the createBranch method

Important Files Changed

Filename Overview
src/VCS/Adapter/Git/GitHub.php Adds createBranch implementation and rewrites listBranches with pagination support and status-code validation; createBranch lacks the same status-code guard added to listBranches, silently returning error bodies on failure.
tests/VCS/Adapter/GitHubTest.php Adds pagination, empty-repo, and non-existent-repo tests for listBranches; uses assertEqualsCanonicalizing for the full-list assertion (good), but page-specific assertSame assertions rely implicitly on GitHub's alphabetical branch ordering.

Reviews (8): Last reviewed commit: "fix: use assertEqualsCanonicalizing for ..." | Re-trigger Greptile

@HarshMN2345 HarshMN2345 requested a review from ChiragAgg5k May 11, 2026 11:07
Comment thread src/VCS/Adapter/Git/GitHub.php Outdated
Comment thread tests/VCS/Adapter/GitHubTest.php Outdated
Comment thread tests/VCS/Adapter/GitHubTest.php Outdated
Comment thread src/VCS/Adapter/Git/GitHub.php Outdated
…ation test

- Implement createBranch using GitHub refs API
- Add page parameter to listBranches for consistency with other methods
- Rewrite testListBranchesFetchesMultiplePages to use a real created repository
  with actual branches instead of a hardcoded external repo
Comment thread src/VCS/Adapter/Git/GitHub.php Outdated
Comment thread tests/VCS/Adapter/GitHubTest.php Outdated
$adapter = $this->vcsAdapter;

$page1 = $adapter->listBranches(static::$owner, $repositoryName, 1, 1);
$this->assertCount(1, $page1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exact value

Comment thread tests/VCS/Adapter/GitHubTest.php
Comment thread tests/VCS/Adapter/GitHubTest.php Outdated
$this->vcsAdapter->createRepository(static::$owner, $repositoryName, false);

try {
$branches = $this->vcsAdapter->listBranches(static::$owner, $repositoryName);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If failing because of default branch, find a way to deleteBranch()

Comment thread tests/VCS/Adapter/GitHubTest.php Outdated
Comment on lines 183 to +192
{
throw new Exception("Not implemented");
$latestCommit = $this->getLatestCommit($owner, $repositoryName, $oldBranchName);
$sha = $latestCommit['commitHash'];

$response = $this->call(self::METHOD_POST, "/repos/$owner/$repositoryName/git/refs", ['Authorization' => "Bearer $this->accessToken"], [
'ref' => "refs/heads/$newBranchName",
'sha' => $sha,
]);

return $response['body'] ?? [];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 createBranch silently swallows API errors

The method doesn't inspect the HTTP status code before returning. If GitHub rejects the request (e.g., 422 "Reference already exists", 404 branch not found, 403 permission denied), $response['body'] contains an error object like {"message":"Reference already exists",...} and createBranch returns it as if it succeeded. By contrast, listBranches in this same PR correctly gates its output on $statusCode < 200 || $statusCode >= 300. Callers of createBranch have no way to distinguish a successful ref payload from an error payload without parsing the message key themselves.

@HarshMN2345 HarshMN2345 merged commit 2850dbe into main May 14, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants