Fix GetBranch response #490

isqua · 2016-12-10T20:34:27Z

I want to get a time of the last commit in branch. I read Github API: Get Branch docs and found the RepositoriesService.GetBranch method. I called it like so:

client.Repositories.GetBranch(orgName, repoName, branchName)

But get the following data (e.g. for this repo):

{
	github.Commit {
		SHA:"5c45bca952b96dedace6c307caff915483013003",
		Author:github.CommitAuthor{},
		Committer:github.CommitAuthor{},
		Parents:[
			github.Commit{
				SHA:"5c5ff1dadae3bb0b306004d480a88e7ddc1714ca",
				URL:"https://api.github.com/repos/google/go-github/commits/5c5ff1dadae3bb0b306004d480a88e7ddc1714ca"
			}
		],
		URL:"https://api.github.com/repos/google/go-github/commits/5c45bca952b96dedace6c307caff915483013003"
	}
}

As you see, the Author and Committer fields has no info. I tried to call the endpoint with curl:

curl -H 'Accept: application/vnd.github.v3+json' \
     -H "Authorization: token ${MY_GITHUB_TOKEN}" \
     "https://api.github.com/repos/google/go-github/branches/master"

And get data I expected:

{
  "name": "master",
  "commit": {
    "sha": "5c45bca952b96dedace6c307caff915483013003",
    "commit": {
      "author": {
        "name": "Ronak Jain",
        "email": "ronakjain@outlook.in",
        "date": "2016-12-09T18:12:45Z"
      },
      "committer": {
        "name": "Glenn Lewis",
        "email": "gmlewis@google.com",
        "date": "2016-12-09T18:58:51Z"
      },
      "message": "improve docs ...",
      "tree": {
        "sha": "de558bd3716b5304a7d40e90c4871a5173ac40a2",
        "url": "https://api.github.com/repos/google/go-github/git/trees/de558bd3716b5304a7d40e90c4871a5173ac40a2"
      },
      "url": "https://api.github.com/repos/google/go-github/git/commits/5c45bca952b96dedace6c307caff915483013003",
      "comment_count": 0
    },
    ...
  }
}

Looks like commit at the root of response is a RepositoryCommit.

googlebot · 2016-12-10T20:34:34Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

isqua · 2016-12-10T20:40:22Z

I signed it!

googlebot · 2016-12-10T20:40:23Z

CLAs look good, thanks!

dmitshur · 2016-12-10T23:55:49Z

You're absolutely right. It was surprising to me that we'd have such a glaring error, so I dug through git blame to see how it occurred (in order to make sure I'm not overlooking something).

It seems we've had the issue since its very initial implementation in 2014. From that commit, I have a pretty good of how it happened. ListBranches was implemented first, and Branch type was created for it.

If you look at https://developer.github.com/v3/repos/branches/#list-branches, you'll see the example that the list endpoint provides has only "sha" and "url" fields inside the "commit" object. A coincidence is that both Commit and RepositoryCommit have those fields, so it's easy to use one instead of the other.

GetBranch was added later and seemingly no one else ran into this (or reported it) until now.

Thanks for the patch!

dmitshur

LGTM.

dmitshur · 2016-12-11T00:05:01Z

github/repos_test.go

@@ -457,7 +457,7 @@ func TestRepositoriesService_GetBranch(t *testing.T) {

 	want := &Branch{
 		Name:      String("n"),
-		Commit:    &Commit{SHA: String("s")},
+		Commit:    &RepositoryCommit{SHA: String("s")},


This is good, but I think it's a good opportunity to expand the test data to include a Commit inside. That way it'd be a little more representative of the real data you get from GitHub, ala https://developer.github.com/v3/repos/branches/#get-branch.

Something like this:

want := &Branch{ Name: String("n"), Commit: &RepositoryCommit{ SHA: String("s"), Commit: &Commit{ Message: String("m"), }, }, Protected: Bool(true), }

You'd have to update the mux.HandleFunc("/repos/o/r/branches/b" above, of course.

Doing this is optional, if you agree it'd be an improvement for this Go package.

Well, I’ve done it: 7920789

Looks great, thanks!

gmlewis · 2016-12-12T19:22:40Z

Thank you, @isqua and @shurcooL!
LGTM. Merging.

gmlewis · 2016-12-12T19:25:34Z

Included in a77ccc9.

Closes google#490. Change-Id: Ie12aca585fe07cb572b347ef3b8f42c26b229808

Fix GetBranch response

c93db92

dmitshur approved these changes Dec 11, 2016

View reviewed changes

Fix GetBranch response; Tests

dd945dc

gmlewis closed this in a77ccc9 Dec 12, 2016

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

Fix GetBranch response

6cd5360

Closes google#490. Change-Id: Ie12aca585fe07cb572b347ef3b8f42c26b229808

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix GetBranch response #490

Fix GetBranch response #490

Uh oh!

isqua commented Dec 10, 2016

Uh oh!

googlebot commented Dec 10, 2016

Uh oh!

isqua commented Dec 10, 2016

Uh oh!

googlebot commented Dec 10, 2016

Uh oh!

dmitshur commented Dec 10, 2016 •

edited

Loading

Uh oh!

dmitshur left a comment

Uh oh!

dmitshur Dec 11, 2016

Uh oh!

isqua Dec 11, 2016 •

edited

Loading

Uh oh!

dmitshur Dec 11, 2016

Uh oh!

gmlewis commented Dec 12, 2016

Uh oh!

gmlewis commented Dec 12, 2016

Uh oh!

Uh oh!

Fix GetBranch response #490

Fix GetBranch response #490

Uh oh!

Conversation

isqua commented Dec 10, 2016

Uh oh!

googlebot commented Dec 10, 2016

Uh oh!

isqua commented Dec 10, 2016

Uh oh!

googlebot commented Dec 10, 2016

Uh oh!

dmitshur commented Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

isqua Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Dec 12, 2016

Uh oh!

gmlewis commented Dec 12, 2016

Uh oh!

Uh oh!

dmitshur commented Dec 10, 2016 •

edited

Loading

isqua Dec 11, 2016 •

edited

Loading