Go-github: CompareCommits() doesn't encode given base/head refs?

[taisph]

Our GitHub integration does a number of automated steps, one being comparing a PR branch with the base, to determine if it can be cleanly merged. It uses Repositories.CompareCommits() passing PullRequest.GetBase().GetRef() and PullRequest.GetHead().GetRef() as base and head refs.

Now, someone on our team thought it would be a great idea to name a branch "feature/#nnnn-some-task", causing the CompareCommits() call to request the URL "https://api.github.com/repos/owner/repo/compare/develop...feature/#nnnn-some-task" which gave an unexpected 404. Replacing the "#" with "%23" fixed this.

As a work around, I'm now doing this replacement before calling CompareCommits() but I think this should probably be handled by the library instead, as it has a better chance of knowing which characters should be encoded when talking to this part of the GitHub API.

[gmlewis]

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.

Thank you!

[urohit011]

I would like to take it up.

[gmlewis]

Thank you, @urohit011 !
It is yours.

[gmlewis]

Unfortunately, I did not carefully check #1649, and it had to be reverted.
The solution to this issue is going to involve only escaping the head and base ref portions of the URL, not the whole thing.

Reopening this issue for a new PR solution to solve it.

[gmlewis]

@srinidhis05 - would you like to address this issue?

[urohit011]

It was my fault for not testing it properly.Very sorry for that.

[gmlewis]

No problem, @urohit011 - I really should have caught that in my code review. The unit test should explicitly show the before and after and I should have insisted when I saw the unit tests... but let it go through.

The moment the same call to url.QueryEscape showed up in both the production code and the unit test was my red flag that something was wrong. I mentioned it once, but didn't follow up on how I really wanted to see it written... so it was completely my responsibility.

Feel free to create a new PR if you would like, but we are going to be much more diligent with the unit testing this time.

[urohit011]

Thanks @gmlewis , I be more carefull next time. I think it will take me a couple of days to create a new PR. I'll step aside if the change is needed quicker than that :)

[chughpiyush]

@gmlewis I can take this one :)

[gmlewis]

Thank you, @chughpiyush - it is yours.