From 4d70c5f1d69fc3ef6c17a503fd232fb76ff8096a Mon Sep 17 00:00:00 2001 From: Rohit Upadhyay Date: Sun, 15 Nov 2020 11:50:52 +0530 Subject: [PATCH 1/3] Replace # with %23 in CompareCommits --- github/repos_commits.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/github/repos_commits.go b/github/repos_commits.go index 5d3784a3d71..f4be37014db 100644 --- a/github/repos_commits.go +++ b/github/repos_commits.go @@ -9,6 +9,7 @@ import ( "bytes" "context" "fmt" + "strings" "time" ) @@ -223,7 +224,10 @@ func (s *RepositoriesService) GetCommitSHA1(ctx context.Context, owner, repo, re // CompareCommits compares a range of commits with each other. // // GitHub API docs: https://docs.github.com/en/rest/reference/repos/#compare-two-commits -func (s *RepositoriesService) CompareCommits(ctx context.Context, owner, repo string, base, head string) (*CommitsComparison, *Response, error) { +func (s *RepositoriesService) CompareCommits(ctx context.Context, owner, repo, base, head string) (*CommitsComparison, *Response, error) { + base = strings.ReplaceAll(base, "#", "%23") + head = strings.ReplaceAll(head, "#", "%23") + u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, base, head) req, err := s.client.NewRequest("GET", u, nil) From eb6377cdf0d9822a22aaa8359a43cdb36fc586b0 Mon Sep 17 00:00:00 2001 From: Rohit Upadhyay Date: Sat, 21 Nov 2020 02:00:36 +0530 Subject: [PATCH 2/3] Add test for CompareCommits --- github/repos_commits.go | 9 +- github/repos_commits_test.go | 186 +++++++++++++++++++---------------- 2 files changed, 107 insertions(+), 88 deletions(-) diff --git a/github/repos_commits.go b/github/repos_commits.go index f4be37014db..3f32609c1c7 100644 --- a/github/repos_commits.go +++ b/github/repos_commits.go @@ -9,7 +9,7 @@ import ( "bytes" "context" "fmt" - "strings" + "net/url" "time" ) @@ -223,12 +223,11 @@ func (s *RepositoriesService) GetCommitSHA1(ctx context.Context, owner, repo, re // CompareCommits compares a range of commits with each other. // +// The url string u is escaped before making the request to api +// // GitHub API docs: https://docs.github.com/en/rest/reference/repos/#compare-two-commits func (s *RepositoriesService) CompareCommits(ctx context.Context, owner, repo, base, head string) (*CommitsComparison, *Response, error) { - base = strings.ReplaceAll(base, "#", "%23") - head = strings.ReplaceAll(head, "#", "%23") - - u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, base, head) + u := url.QueryEscape(fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, base, head)) req, err := s.client.NewRequest("GET", u, nil) if err != nil { diff --git a/github/repos_commits_test.go b/github/repos_commits_test.go index 6da964fec56..489092ae7d7 100644 --- a/github/repos_commits_test.go +++ b/github/repos_commits_test.go @@ -9,6 +9,7 @@ import ( "context" "fmt" "net/http" + "net/url" "reflect" "strings" "testing" @@ -305,99 +306,118 @@ func TestRepositoriesService_CompareCommits(t *testing.T) { client, mux, _, teardown := setup() defer teardown() - mux.HandleFunc("/repos/o/r/compare/b...h", func(w http.ResponseWriter, r *http.Request) { - testMethod(t, r, "GET") - fmt.Fprintf(w, `{ - "base_commit": { - "sha": "s", - "commit": { - "author": { "name": "n" }, - "committer": { "name": "n" }, - "message": "m", - "tree": { "sha": "t" } - }, - "author": { "login": "l" }, - "committer": { "login": "l" }, - "parents": [ { "sha": "s" } ] - }, - "status": "s", - "ahead_by": 1, - "behind_by": 2, - "total_commits": 1, - "commits": [ - { - "sha": "s", - "commit": { "author": { "name": "n" } }, - "author": { "login": "l" }, - "committer": { "login": "l" }, - "parents": [ { "sha": "s" } ] - } - ], - "files": [ { "filename": "f" } ], - "html_url": "https://github.com/o/r/compare/b...h", - "permalink_url": "https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17", - "diff_url": "https://github.com/o/r/compare/b...h.diff", - "patch_url": "https://github.com/o/r/compare/b...h.patch", - "url": "https://api.github.com/repos/o/r/compare/b...h" - }`) - }) + cases := []struct { + name string + b string + h string + }{ + {name: "case 1", b: "b", h: "h"}, + {name: "case 2", b: "#b#", h: "#h#"}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + b := url.QueryEscape(c.b) + h := url.QueryEscape(c.h) + + u := fmt.Sprintf("/repos/o/r/compare/%v...%v", c.b, c.h) + + mux.HandleFunc(u, func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprintf(w, `{ + "base_commit": { + "sha": "s", + "commit": { + "author": { "name": "n" }, + "committer": { "name": "n" }, + "message": "m", + "tree": { "sha": "t" } + }, + "author": { "login": "l" }, + "committer": { "login": "l" }, + "parents": [ { "sha": "s" } ] + }, + "status": "s", + "ahead_by": 1, + "behind_by": 2, + "total_commits": 1, + "commits": [ + { + "sha": "s", + "commit": { "author": { "name": "n" } }, + "author": { "login": "l" }, + "committer": { "login": "l" }, + "parents": [ { "sha": "s" } ] + } + ], + "files": [ { "filename": "f" } ], + "html_url": "https://github.com/o/r/compare/%[1]v...%[2]v", + "permalink_url": "https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17", + "diff_url": "https://github.com/o/r/compare/%[1]v...%[2]v.diff", + "patch_url": "https://github.com/o/r/compare/%[1]v...%[2]v.patch", + "url": "https://api.github.com/repos/o/r/compare/%[1]v...%[2]v" + }`, b, h) + }) - got, _, err := client.Repositories.CompareCommits(context.Background(), "o", "r", "b", "h") - if err != nil { - t.Errorf("Repositories.CompareCommits returned error: %v", err) - } + got, _, err := client.Repositories.CompareCommits(context.Background(), "o", "r", c.b, c.h) + if err != nil { + t.Errorf("Repositories.CompareCommits returned error: %v", err) + } - want := &CommitsComparison{ - BaseCommit: &RepositoryCommit{ - SHA: String("s"), - Commit: &Commit{ - Author: &CommitAuthor{Name: String("n")}, - Committer: &CommitAuthor{Name: String("n")}, - Message: String("m"), - Tree: &Tree{SHA: String("t")}, - }, - Author: &User{Login: String("l")}, - Committer: &User{Login: String("l")}, - Parents: []*Commit{ - { + want := &CommitsComparison{ + BaseCommit: &RepositoryCommit{ SHA: String("s"), + Commit: &Commit{ + Author: &CommitAuthor{Name: String("n")}, + Committer: &CommitAuthor{Name: String("n")}, + Message: String("m"), + Tree: &Tree{SHA: String("t")}, + }, + Author: &User{Login: String("l")}, + Committer: &User{Login: String("l")}, + Parents: []*Commit{ + { + SHA: String("s"), + }, + }, }, - }, - }, - Status: String("s"), - AheadBy: Int(1), - BehindBy: Int(2), - TotalCommits: Int(1), - Commits: []*RepositoryCommit{ - { - SHA: String("s"), - Commit: &Commit{ - Author: &CommitAuthor{Name: String("n")}, - }, - Author: &User{Login: String("l")}, - Committer: &User{Login: String("l")}, - Parents: []*Commit{ + Status: String("s"), + AheadBy: Int(1), + BehindBy: Int(2), + TotalCommits: Int(1), + Commits: []*RepositoryCommit{ { SHA: String("s"), + Commit: &Commit{ + Author: &CommitAuthor{Name: String("n")}, + }, + Author: &User{Login: String("l")}, + Committer: &User{Login: String("l")}, + Parents: []*Commit{ + { + SHA: String("s"), + }, + }, }, }, - }, - }, - Files: []*CommitFile{ - { - Filename: String("f"), - }, - }, - HTMLURL: String("https://github.com/o/r/compare/b...h"), - PermalinkURL: String("https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17"), - DiffURL: String("https://github.com/o/r/compare/b...h.diff"), - PatchURL: String("https://github.com/o/r/compare/b...h.patch"), - URL: String("https://api.github.com/repos/o/r/compare/b...h"), - } + Files: []*CommitFile{ + { + Filename: String("f"), + }, + }, + HTMLURL: String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v", b, h)), + PermalinkURL: String("https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17"), + DiffURL: String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v.diff", b, h)), + PatchURL: String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v.patch", b, h)), + URL: String(fmt.Sprintf("https://api.github.com/repos/o/r/compare/%v...%v", b, h)), + } - if !reflect.DeepEqual(got, want) { - t.Errorf("Repositories.CompareCommits returned \n%+v, want \n%+v", got, want) + if !reflect.DeepEqual(got, want) { + t.Errorf("Repositories.CompareCommits returned \n%+v, want \n%+v", got, want) + } + }) } + } func TestRepositoriesService_CompareCommitsRaw_diff(t *testing.T) { From ee2258fd56fefbe506fcb0b96d32d7e13c3f9fde Mon Sep 17 00:00:00 2001 From: Rohit Upadhyay Date: Tue, 24 Nov 2020 10:53:29 +0530 Subject: [PATCH 3/3] Update github/repos_commits.go Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com> --- github/repos_commits.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/repos_commits.go b/github/repos_commits.go index 3f32609c1c7..06210c579c7 100644 --- a/github/repos_commits.go +++ b/github/repos_commits.go @@ -223,7 +223,7 @@ func (s *RepositoriesService) GetCommitSHA1(ctx context.Context, owner, repo, re // CompareCommits compares a range of commits with each other. // -// The url string u is escaped before making the request to api +// The url query string is escaped before making the request. // // GitHub API docs: https://docs.github.com/en/rest/reference/repos/#compare-two-commits func (s *RepositoriesService) CompareCommits(ctx context.Context, owner, repo, base, head string) (*CommitsComparison, *Response, error) {