feat: Refactor repositories download contents

[stevehipwell]

This PR refactors the behaviour of DownloadContents & DownloadContentsWithMeta with the former now being a direct passthrough to the latter as the only difference was the signature. The code has been refactored to use the API directly instead of via an unnecessary layer of indirection.

I've added an OpenAPI update to this PR as it proves that the updated code works against GitHub.

This change is required for #4151.

[codecov]

Codecov Report

❌ Patch coverage is 28.84615% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.68%. Comparing base (1d6a852) to head (45817d9).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
example/contents/main.go	0.00%	34 Missing ⚠️
github/repos_contents.go	83.33%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4153      +/-   ##
==========================================
- Coverage   93.83%   93.68%   -0.15%     
==========================================
  Files         209      210       +1     
  Lines       19685    19695      +10     
==========================================
- Hits        18472    18452      -20     
- Misses       1015     1047      +32     
+ Partials      198      196       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[stevehipwell]

@gmlewis can we get this merged?

[gmlewis]

I'm quite concerned about this PR because it appears to me that the behavior of following redirects has been deleted and there are many unit tests that have also simply been deleted without comment or explanation. One of the great things about unit tests is that when major refactors are performed like this one, if the unit tests are left alone we can easily detect regressions. As it is in this PR, however, where a major refactor happens and unit tests are also heavily refactored and/or deleted, it is hard to tell what is actually happening.

Can this be broken down into 3 PRs?

1. Update the openapi_operations.yaml file - I'll do that myself momentarily.
2. Refactor the download methods without modifying unit tests
3. Refactor and/or delete unit tests

[stevehipwell]

@gmlewis let me take a look, but the main problem here is that the tests appear to be tightly coupled to the implementation with mocks designed to make the test pass rather than to mirror the actual API. I'll add the deleted tests back, but the mocks will need to be refactored to add the schema required download_url to the content payload.

On a slight tangent, shouldn't the mock payloads be validated against the schema?

[gmlewis]

On a slight tangent, shouldn't the mock payloads be validated against the schema?

Yes, they probably should. I don't remember when GitHub v3 API docs started sharing schemas for endpoints, but it is possible that these were written prior to that.

I think my biggest concern is following redirects because I remember a bunch of issues devoted solely to this topic, and to my shock and disappointment, I don't see any of the unit tests actually testing out following redirects and I could have sworn that it took a good deal of effort to get those unit tests to pass at one point. :-(

[stevehipwell]

I think my biggest concern is following redirects because I remember a bunch of issues devoted solely to this topic, and to my shock and disappointment, I don't see any of the unit tests actually testing out following redirects and I could have sworn that it took a good deal of effort to get those unit tests to pass at one point. :-(

@gmlewis there are no redirects in the removed tests. The old code pattern was just ignoring the presence of download_url on the file content response and making an unnecessary call to the same get content endpoint for the parent directory (if the content wasn't returned inline). I've just removed this unnecessary step, if the content isn't returned inline we still use exactly the same pattern for fetching it from the download link.

FYI the following example snippet will error using the current code but pass with the updated code as the last file requested is at an index greater than 1000 and has a size of greater than 1mb so won't have returned content.

package main

import (
	"context"
	"fmt"
	"io"
	"os"

	"github.com/google/go-github/v84/github"
)

// downloadContents downloads the contents of a file in a repository and returns it as a byte slice.
func downloadContents(ctx context.Context, client *github.Client, owner, repo, path, ref string) ([]byte, error) {
	rc, _, err := client.Repositories.DownloadContents(ctx, owner, repo, path, &github.RepositoryContentGetOptions{Ref: ref})
	if err != nil {
		return nil, err
	}
	defer rc.Close()

	by, err := io.ReadAll(rc)
	if err != nil {
		return nil, err
	}

	fmt.Printf("Downloaded %v/%v/%v as %d bytes\n", owner, repo, path, len(by))
	return by, nil
}

func main() {
	client := github.NewClient(nil)

	t := []struct {
		owner string
		repo  string
		path  string
		ref   string
	}{
		{"google", "go-github", "README.md", "master"},
		{"github", "rest-api-description", "descriptions/api.github.com/api.github.com.2026-03-10.yaml", "main"},
		{"ScoopInstaller", "Main", "bucket/yq.json", "master"},
		{"stevehipwell", "scoop-main-bucket", "bucket/zzztest.bin", "test-content"},
	}

	for _, v := range t {
		if _, err := downloadContents(context.Background(), client, v.owner, v.repo, v.path, v.ref); err != nil {
			fmt.Printf("Error: %v\n", err)
			os.Exit(1)
		}
	}
}

[stevehipwell]

@gmlewis I've added back the removed tests and undone some of the cosmetic changes to make the diff clearer that none of the actual tests have changed (it's only the mocks). I haven't rebased to fix the conflict yet in case you want to look at anything first?

[gmlewis]

@gmlewis I've added back the removed tests and undone some of the cosmetic changes to make the diff clearer that none of the actual tests have changed (it's only the mocks). I haven't rebased to fix the conflict yet in case you want to look at anything first?

Thank you, @stevehipwell!
This looks great. Yes, let's proceed with this PR and get this in so you can continue to make progress on the context overhaul.

[stevehipwell]

@gmlewis I've rebased this and it should be good to go.

@alexandear I've updated the example to be closer to the other patterns and to have a valid comment.

[gmlewis]

Thank you, @stevehipwell and @alexandear.
Merging.