Escape branch string before inserting it in URL

[k0ral]

When a branch name with special characters like % is passed to the RepositoryService.GetBranch() method as is, an error is currently returned:

invalid URL escape "%"

Passing an escaped branch name to RepositoriesService.GetBranch() works fine.

Here's a minimal reproducer

import (
"context""net/http""testing""github.com/google/go-github/v53/github""github.com/stretchr/testify/require"
)
funcTestGetBranch(t*testing.T) {
t.Parallel()
ctx:=context.Background()
githubClient:=github.NewClient(&http.Client{
Transport: http.DefaultTransport,
})
owner:="samstickkz"project:="curr"branchName:="Feature/authentication_90%"branch, response, err:=githubClient.Repositories.GetBranch(ctx, owner, project, branchName, false)
require.Nil(t, err)
t.Logf("Response: %+v", response)
t.Logf("Branch: %+v", branch)
}

This sounds like a bug to me, as I believe users of that method shouldn't be expected to escape the branch name before calling it, as this is a private implementation quirk that they shouldn't be aware of. This PR patches all branch-related methods to automatically escape the branch name in the endpoint URL.

There's actually a precedent for ref parameter, and I suspect that other variables are prone to the same issue. I am conservatively patching only the branch variable, as this is the only one I have tested thoroughly. Note that, contrary to ref use case, my tests suggest that it's okay to escape / characters for branch names.

⚠️ Breaking change: for users that currently bypass this issue by feeding escaped strings to RepositoryService.GetBranch() method (and other branch-related methods), this change will be breaking.

[codecov]

Codecov Report

Merging #2948 (f77ef83) into master (5224e34) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #2948 +/- ##
=======================================
Coverage 98.17% 98.17% =======================================
Files 145 145 Lines 12735 12735 =======================================
Hits 12502 12502 Misses 158 158 Partials 75 75 

Files	Coverage Δ
github/repos.go	98.89% <100.00%> (ø)

[gmlewis]

Honestly, I'm quite surprised that this issue has never come up before... as this repo is how many years old? 10?!?

However, I think you are right.

I'm wondering if each of the modified endpoints needs a comment in the documentation stating something like:

// Note that the branch name is URL path escaped for you. See: https://pkg.go.dev/net/url#PathEscape

[gmlewis]

Can you please add unit tests that demonstrate this new super power?

[k0ral]

@gmlewis PR updated with additional comments and unit tests.

[gmlewis]

Thank you, @k0ral !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

[WillAbides]

I have a bit of concern about this being a subtle breaking change that could take a long time to discover after upgrading. I'm looking at ways to mitigate that. I'll be back to review later today.

[WillAbides]

I'm still a little hesitant about the breaking change, but after a some pondering I can't come up with anything better that doesn't add unnecessary complexity.

[gmlewis]

Thank you, @WillAbides !
Merging.