This is strange, but I'm getting 404 even for a seemingly protected branch. Are you able to get a 200 OK response still?

I tried it on two repos I know have protected master branches, and I get 404 for everything other than "get branch" endpoint.

For example:

$ curl -H "Accept: application/vnd.github.loki-preview+json" \
'https://api.github.com/repos/gregjones/httpcache/branches/master'
{
  "name": "master",
...
  "protected": true,
  "protection": {
    "enabled": true,
    "required_status_checks": {
      "enforcement_level": "non_admins",
      "contexts": [
        "continuous-integration/travis-ci"
      ]
    }
  },
  "protection_url": "https://api.github.com/repos/gregjones/httpcache/branches/master/protection"
}

$ curl -H "Accept: application/vnd.github.loki-preview+json" \
'https://api.github.com/repos/gregjones/httpcache/branches/master/protection/required_status_checks'
{
  "message": "Not Found",
  "documentation_url": "https://developer.github.com/v3"
}

$ curl -H "Accept: application/vnd.github.loki-preview+json" \
'https://api.github.com/repos/gregjones/httpcache/branches/master/protection/required_status_checks/contexts'
{
  "message": "Not Found",
  "documentation_url": "https://developer.github.com/v3"
}

I also tried authenticating with repo scope, no difference.

Any ideas what I'm doing wrong? /cc @jimmidyson

I'm not sure of the exact scope required to see branch protection but it works fine for me when using a token with all scopes on one of my repos :) Perhaps you don't have enough rights to that particular repo?

Ok, let me try again with all scopes. Thanks.

Thanks, that was helpful. I was able to reproduce 200 OK responses now.

It turns out the issue was that I needed admin access to the repository, push access isn't enough. For scopes, just public_repo scope is sufficient.

It's strange, given one can access the same information via "Get Branch" endpoint, but I'll write it off as overly-strict-permission-requirement quirks of preview API.

Now, I get responses such as:

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks'
HTTP/1.1 200 OK
{
  "url": "https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks",
  "strict": false,
  "include_admins": false,
  "contexts": [
    "ci/gopherci/push",
    "continuous-integration/travis-ci"
  ],
  "contexts_url": "https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks/contexts"
}

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/gorilla-schema-to-parse-query-form/protection/required_status_checks'
HTTP/1.1 404 Not Found
{
  "message": "Branch not protected",
  "documentation_url": "https://developer.github.com/v3"
}

I noticed there's a subtle difference between no contexts, and zero contexts. When a branch is protected, you get:

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks/contexts'
HTTP/1.1 200 OK
[
  "ci/gopherci/push",
  "continuous-integration/travis-ci"
]

Or, if there are no contexts, you get empty array:

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks/contexts'
HTTP/1.1 200 OK
[

]

But if it's not protected, you get 404:

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks/contexts'
HTTP/1.1 404 Not Found
{
  "message": "Branch not protected",
  "documentation_url": "https://developer.github.com/v3"
}

With the current implementation of PR #626, one cannot tell the difference between the 2nd and 3rd situations.

Maybe that's okay, and for the best. But an alternative solution we can consider is returning an error such as ErrBranchUnprotected, when the GitHub response is 404 with "message": "Branch not protected".

I'm not sure if that's better and worth it, though. What do you think? /cc @jimmidyson @gmlewis

My thoughts on the above... I think such a ErrBranchUnprotected may have some positive benefit, but it's too early to say.

I propose we go ahead and merge #626 as is, which returns nil error and nil pointer/empty slices for the getter methods when a branch is unprotected.

If there's demand or evidence in the future that it'd be better for those methods to report unprotected branch as an error, we can make a breaking API change and introduce it then. It's hard to introduce such breaking changes, but I think we reserve the right as long as this API is still in preview on GitHub side.

Actually, after thinking more about it, I'm becoming more convinced that returning an error for unprotected branches is a better thing to do.

It might also be very easy to implement by adding detection of the 404 response in CheckResponse.

Here's my rationale. If you look at names of relevant endpoints at https://developer.github.com/v3/repos/branches/, they all have "of protected branch" as a common suffix. That tells me their behavior is defined for protected branches only, and it's very reasonable to return a "this is an unprotected branch" error if the branch is not protected.

To give an analogy, suppose you make an API call to list all branches of a repository. If the repository doesn't exist, it makes more sense to get a "repo doesn't exist" error, rather than an empty list of branches and nil error, right?

Thoughts?

I agree. I like exposing the extra detail as it helps developers and users of the API distinguish between the two whereas if we hide the distinction, we cause a great deal more frustration and people would have to instrument the code to figure out what is going on underneath.

So I am all in-favor of ErrBranchUnprotected.

Ok, so we're in agreement that querying protection details on an unprotected branch should return a non-nil error.

I think this issue is about making that error detectable and different from other types of unexpected errors (such as, some network error or GitHub API being down).

Whatever the case, we'll need to document what the error is when a branch is unprotected.

Returning a specific ErrBranchUnprotected is one possible solution.

Another solution I'm thinking about is documenting that a "not found" error is returned when the branch is unprotected.

Since GitHub API already returns 404 Not Found responses, I feel that's both a good mapping, and also one that's easier to implement generally.

We can create a helper (inspired by os.IsNotExist) that reports whether an error is of "not found" category:

// IsNotFound reports whether the error is known to report that a resource was not found.
// It is satisfied by a response from GitHub API with 404 Not Found status code.
func IsNotFound(err error) bool {
	err, ok := err.(*ErrorResponse)
	return ok && err.Response.StatusCode == http.StatusNotFound
}

Then we can document methods such as GetRequiredStatusChecks like this:

// GetRequiredStatusChecks gets the required status checks for a given protected branch.
// A not found error, such that github.IsNotFound(err) reports true, is returned if the branch
// is not protected.
//
// GitHub API docs: https://developer.github.com/v3/repos/branches/#get-required-status-checks-of-protected-branch
func (s *RepositoriesService) GetRequiredStatusChecks(ctx context.Context, owner, repo, branch string) (*RequiredStatusChecks, *Response, error) {

If we really think it's worth it to return more specific ErrBranchUnprotected errors for these methods, then I would suggest we implement that in the relevant methods themselves, something like:

func (s *RepositoriesService) GetRequiredStatusChecks(ctx context.Context, owner, repo, branch string) (*RequiredStatusChecks, *Response, error) {
	...

	resp, err := s.client.Do(ctx, req, p)
	if err != nil {
		if er, ok := err.(*ErrorResponse); ok &&
			er.Response.StatusCode == http.StatusNotFound &&
			er.Message == "Branch not protected" {
				return ErrBranchUnprotected
		}
		return nil, resp, err
	}

	return p, resp, nil
}

(That logic can be moved into a private helper.)

I like IsNotFound... that keeps things simple and still understandable.

Bikeshed question, is it better to call it IsNotFound or IsNotExist, to mirror language used by os.IsNotExist?

I'm in two minds about this. While it sounds fine, GitHub returns 404s for unauthorized so a 404 doesn't necessarily mean the branch is unprotected.

True. To be really sure it's unprotected, we need this check to be done:

er.Message == "Branch not protected"

Either by the user, or by us. This doesn't sound like something that's reasonable for us to ask the user to do, so I think we should.

So that points more to the ErrBranchUnprotected or github.IsBranchNotProtected (or whatever) rather than github.IsNotFound which is more generic.

Checking the message is so potentially fragile but I agree I can't see another way if we want to be stricter with the return.

We should probably consider the GetBranchProtection method more closely first.

I would expect, at the very least, it should be possible to call that method and use its response to determine whether a branch is protected or not protected.

The endpoint is called "Get branch protection", not "Get branch protection of protected branch", after all.

We may or may not want to also make it possible to determine whether a branch is unprotected in the more specific endpoints that have a "of protected branch" suffix.

So how about adding another return value, a bool to show if branch is protected? So something like:

GetBranchProtection(ctx context.Context, owner, repo, branch string) (protection *Protection, isProtected bool, resp *Response, err error)

And similar for all related methods?

GitHub returns 404s for unauthorized so a 404 doesn't necessarily mean the branch is unprotected.

Another observation related to this is that we don't really do anything for other endpoints.

Consider the List Branches endpoint, implemented as RepositoriesService.ListBranches:

$ goexec -quiet 'c := github.NewClient(nil); _, _, err := c.Repositories.ListBranches(context.Background(), "shurcooL", "secret", nil); fmt.Println(err)'
GET https://api.github.com/repos/shurcooL/secret/branches: 404 Not Found []

You get 404 not found response regardless whether it's a private repository and you're not authorized to access its branches, or if it's a repository that doesn't exist. GitHub doesn't give 401 Unauthorized 403 Forbidden responses to avoid leaking information about private repos (even though it's against HTTP spec apparently not).

So why should we do it for these endpoints?

It's a 404 instead of a 403 I think (see https://developer.github.com/v3/#authentication) and it is compliant afaik - see https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4.

However, if you are authenticated then you can figure out whether the branch is protected or not as you've mentioned above by checking message.

It would be good if the library was consistent in this behaviour, but that's going to require a fair amount of revisiting previous implementations. I think what you're proposing here makes sense but will potentially affect maintainability and make consistency harder to retain across the breadth of API this library supports.

Whoops, yeah, I meant 403. Edited my post. Thanks.

Interesting that returning 404 for 403 situations is a supported way of not leaking information.

It would be good if the library was consistent in this behaviour, but that's going to require a fair amount of revisiting previous implementations. I think what you're proposing here makes sense

Which one are you referring to? I proposed many conflicting things here so far, while thinking this through. Can you please specify?

My latest thoughts are that we should think of a good solution for GetBranchProtection method, and once we have that, we can think how and whether to also apply it to the other endpoints. I suspect it might be fine to leave the other endpoints as is.

Another factor to consider is that GitHub might tighten up and document the 404 responses by the time this API graduates from a preview, which means it might be easier to solve this if we wait (or it might not).

So how about adding another return value, a bool

Can you give some examples of how the API would be used, I think that'd be helpful to evaluate the idea. I'm imagining:

p, isProtected, _, err := client.GetBranchProtection(ctx, owner, repo, branch)
if err != nil || !isProtected {
	// handle error or not protected branch
}
// use p

For the more specific methods, such as ListRequiredStatusChecksContexts:

contexts, isProtected, _, err := client.ListRequiredStatusChecksContexts(ctx, owner, repo, branch)
if err != nil {
	// handle error
}
// use contexts, isProtected

I suppose if someone doesn't care about isProtected, they can simply do:

contexts, _, _, err := client.ListRequiredStatusChecksContexts(ctx, owner, repo, branch)
if err != nil {
	// handle error
}
// use contexts

I want to think it over more, but it seems surprisingly reasonable so far.

My only concern so far is that people might still be tempted to check that p != nil too, making the usage less nice:

p, isProtected, _, err := client.GetBranchProtection(ctx, owner, repo, branch)
if err != nil || !isProtected || p == nil {
	// handle error or not protected branch
}
// use p

That is exactly what I was thinking. As long as we document the returns, i.e. will be nil if not protected or unauthorised, then we should be good. Alternatively, return a pointer to a zeroed BranchProtection rather than nil. Kinda prefer returning nil though.

My comments on consistency through the library were really around how other functions return/differentiate between real not exists and unauthorised. Personally I'm not too worried about it, but is something to think about if we start introducing that distinction in this functional area.

This would be a great PR for any new contributor to this repo or a new Go developer.
All contributions are greatly appreciated!

Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.

Please check out our CONTRIBUTING.md guide to get started. (In particular, please remember to go generate ./... and don't use force-push to your PRs.)

Thank you!