Repository: google/go-github (go)

Issue:
Refs should not encode /
The commit #1099 changed the ref name to be URL Query encoded. This breaks ref names which contain a /:

A call to `curl -vH "Authorization: Bearer $TOKEN" https://$GHE_URL/repos/o/r/git/ref/heads/master` works, but `https://$GHE_URL/repos/o/r/git/ref/heads%2Fmaster` (like `GetRef()`, `GetRefs()` use it) fail with 404.



Pull request description:
do not encode / in ref names
fixes #1432

Signed-off-by: Hanno Hecker <hanno@zalando.de>

Patch:
diff --git a/github/actions_workflows.go b/github/actions_workflows.go
index fc4cf4a5..9743f507 100644
--- a/github/actions_workflows.go
+++ b/github/actions_workflows.go
@@ -12,22 +12,22 @@ import (
 
 // Workflow represents a repository action workflow.
 type Workflow struct {
-	ID        int64     `json:"id"`
-	NodeID    string    `json:"node_id"`
-	Name      string    `json:"name"`
-	Path      string    `json:"path"`
-	State     string    `json:"state"`
-	CreatedAt Timestamp `json:"created_at"`
-	UpdatedAt Timestamp `json:"updated_at"`
-	URL       string    `json:"url"`
-	HTMLURL   string    `json:"html_url"`
-	BadgeURL  string    `json:"badge_url"`
+	ID        *int64     `json:"id,omitempty"`
+	NodeID    *string    `json:"node_id,omitempty"`
+	Name      *string    `json:"name,omitempty"`
+	Path      *string    `json:"path,omitempty"`
+	State     *string    `json:"state,omitempty"`
+	CreatedAt *Timestamp `json:"created_at,omitempty"`
+	UpdatedAt *Timestamp `json:"updated_at,omitempty"`
+	URL       *string    `json:"url,omitempty"`
+	HTMLURL   *string    `json:"html_url,omitempty"`
+	BadgeURL  *string    `json:"badge_url,omitempty"`
 }
 
 // Workflows represents a slice of repository action workflows.
 type Workflows struct {
-	TotalCount int         `json:"total_count"`
-	Workflows  []*Workflow `json:"workflows"`
+	TotalCount *int        `json:"total_count,omitempty"`
+	Workflows  []*Workflow `json:"workflows,omitempty"`
 }
 
 // ListWorkflows lists all workflows in a repository.
diff --git a/github/checks.go b/github/checks.go
index 55adf14e..67b7e770 100644
--- a/github/checks.go
+++ b/github/checks.go
@@ -8,7 +8,6 @@ package github
 import (
 	"context"
 	"fmt"
-	"net/url"
 )
 
 // ChecksService provides access to the Checks API in the
@@ -258,7 +257,7 @@ type ListCheckRunsResults struct {
 //
 // GitHub API docs: https://developer.github.com/v3/checks/runs/#list-check-runs-for-a-specific-ref
 func (s *ChecksService) ListCheckRunsForRef(ctx context.Context, owner, repo, ref string, opts *ListCheckRunsOptions) (*ListCheckRunsResults, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/commits/%v/check-runs", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/commits/%v/check-runs", owner, repo, refURLEscape(ref))
 	u, err := addOptions(u, opts)
 	if err != nil {
 		return nil, nil, err
@@ -324,7 +323,7 @@ type ListCheckSuiteResults struct {
 //
 // GitHub API docs: https://developer.github.com/v3/checks/suites/#list-check-suites-for-a-specific-ref
 func (s *ChecksService) ListCheckSuitesForRef(ctx context.Context, owner, repo, ref string, opts *ListCheckSuiteOptions) (*ListCheckSuiteResults, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/commits/%v/check-suites", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/commits/%v/check-suites", owner, repo, refURLEscape(ref))
 	u, err := addOptions(u, opts)
 	if err != nil {
 		return nil, nil, err
diff --git a/github/git_refs.go b/github/git_refs.go
index 18494dae..a6269e5a 100644
--- a/github/git_refs.go
+++ b/github/git_refs.go
@@ -58,7 +58,7 @@ type updateRefRequest struct {
 // GitHub API docs: https://developer.github.com/v3/git/refs/#get-a-reference
 func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref string) (*Reference, *Response, error) {
 	ref = strings.TrimPrefix(ref, "refs/")
-	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, refURLEscape(ref))
 	req, err := s.client.NewRequest("GET", u, nil)
 	if err != nil {
 		return nil, nil, err
@@ -79,6 +79,16 @@ func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref
 	return r, resp, nil
 }
 
+// refURLEscape escapes every path segment of the given ref. Those must
+// not contain escaped "/" - as "%2F" - or github will not recognize it.
+func refURLEscape(ref string) string {
+	parts := strings.Split(ref, "/")
+	for i, s := range parts {
+		parts[i] = url.PathEscape(s)
+	}
+	return strings.Join(parts, "/")
+}
+
 // GetRefs fetches a slice of Reference objects for a given Git ref.
 // If there is an exact match, only that ref is returned.
 // If there is no exact match, GitHub returns all refs that start with ref.
@@ -92,7 +102,7 @@ func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref
 // GitHub API docs: https://developer.github.com/v3/git/refs/#get-a-reference
 func (s *GitService) GetRefs(ctx context.Context, owner string, repo string, ref string) ([]*Reference, *Response, error) {
 	ref = strings.TrimPrefix(ref, "refs/")
-	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, refURLEscape(ref))
 	req, err := s.client.NewRequest("GET", u, nil)
 	if err != nil {
 		return nil, nil, err
@@ -212,7 +222,7 @@ func (s *GitService) UpdateRef(ctx context.Context, owner string, repo string, r
 // GitHub API docs: https://developer.github.com/v3/git/refs/#delete-a-reference
 func (s *GitService) DeleteRef(ctx context.Context, owner string, repo string, ref string) (*Response, error) {
 	ref = strings.TrimPrefix(ref, "refs/")
-	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, refURLEscape(ref))
 	req, err := s.client.NewRequest("DELETE", u, nil)
 	if err != nil {
 		return nil, err
diff --git a/github/github-accessors.go b/github/github-accessors.go
index 212e334b..a1f3e0c1 100644
--- a/github/github-accessors.go
+++ b/github/github-accessors.go
@@ -13851,3 +13851,91 @@ func (w *WeeklyStats) GetWeek() Timestamp {
 	}
 	return *w.Week
 }
+
+// GetBadgeURL returns the BadgeURL field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetBadgeURL() string {
+	if w == nil || w.BadgeURL == nil {
+		return ""
+	}
+	return *w.BadgeURL
+}
+
+// GetCreatedAt returns the CreatedAt field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetCreatedAt() Timestamp {
+	if w == nil || w.CreatedAt == nil {
+		return Timestamp{}
+	}
+	return *w.CreatedAt
+}
+
+// GetHTMLURL returns the HTMLURL field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetHTMLURL() string {
+	if w == nil || w.HTMLURL == nil {
+		return ""
+	}
+	return *w.HTMLURL
+}
+
+// GetID returns the ID field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetID() int64 {
+	if w == nil || w.ID == nil {
+		return 0
+	}
+	return *w.ID
+}
+
+// GetName returns the Name field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetName() string {
+	if w == nil || w.Name == nil {
+		return ""
+	}
+	return *w.Name
+}
+
+// GetNodeID returns the NodeID field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetNodeID() string {
+	if w == nil || w.NodeID == nil {
+		return ""
+	}
+	return *w.NodeID
+}
+
+// GetPath returns the Path field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetPath() string {
+	if w == nil || w.Path == nil {
+		return ""
+	}
+	return *w.Path
+}
+
+// GetState returns the State field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetState() string {
+	if w == nil || w.State == nil {
+		return ""
+	}
+	return *w.State
+}
+
+// GetUpdatedAt returns the UpdatedAt field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetUpdatedAt() Timestamp {
+	if w == nil || w.UpdatedAt == nil {
+		return Timestamp{}
+	}
+	return *w.UpdatedAt
+}
+
+// GetURL returns the URL field if it's non-nil, zero value otherwise.
+func (w *Workflow) GetURL() string {
+	if w == nil || w.URL == nil {
+		return ""
+	}
+	return *w.URL
+}
+
+// GetTotalCount returns the TotalCount field if it's non-nil, zero value otherwise.
+func (w *Workflows) GetTotalCount() int {
+	if w == nil || w.TotalCount == nil {
+		return 0
+	}
+	return *w.TotalCount
+}
diff --git a/github/repos_commits.go b/github/repos_commits.go
index a4ff2be4..d6960124 100644
--- a/github/repos_commits.go
+++ b/github/repos_commits.go
@@ -9,7 +9,6 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"net/url"
 	"time"
 )
 
@@ -198,7 +197,7 @@ func (s *RepositoriesService) GetCommitRaw(ctx context.Context, owner string, re
 //
 // GitHub API docs: https://developer.github.com/v3/repos/commits/#get-the-sha-1-of-a-commit-reference
 func (s *RepositoriesService) GetCommitSHA1(ctx context.Context, owner, repo, ref, lastSHA string) (string, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/commits/%v", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/commits/%v", owner, repo, refURLEscape(ref))
 
 	req, err := s.client.NewRequest("GET", u, nil)
 	if err != nil {
diff --git a/github/repos_statuses.go b/github/repos_statuses.go
index 55432658..5576d311 100644
--- a/github/repos_statuses.go
+++ b/github/repos_statuses.go
@@ -8,7 +8,6 @@ package github
 import (
 	"context"
 	"fmt"
-	"net/url"
 	"time"
 )
 
@@ -46,7 +45,7 @@ func (r RepoStatus) String() string {
 //
 // GitHub API docs: https://developer.github.com/v3/repos/statuses/#list-statuses-for-a-specific-ref
 func (s *RepositoriesService) ListStatuses(ctx context.Context, owner, repo, ref string, opts *ListOptions) ([]*RepoStatus, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/commits/%v/statuses", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/commits/%v/statuses", owner, repo, refURLEscape(ref))
 	u, err := addOptions(u, opts)
 	if err != nil {
 		return nil, nil, err
@@ -71,7 +70,7 @@ func (s *RepositoriesService) ListStatuses(ctx context.Context, owner, repo, ref
 //
 // GitHub API docs: https://developer.github.com/v3/repos/statuses/#create-a-status
 func (s *RepositoriesService) CreateStatus(ctx context.Context, owner, repo, ref string, status *RepoStatus) (*RepoStatus, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/statuses/%v", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/statuses/%v", owner, repo, refURLEscape(ref))
 	req, err := s.client.NewRequest("POST", u, status)
 	if err != nil {
 		return nil, nil, err
@@ -110,7 +109,7 @@ func (s CombinedStatus) String() string {
 //
 // GitHub API docs: https://developer.github.com/v3/repos/statuses/#get-the-combined-status-for-a-specific-ref
 func (s *RepositoriesService) GetCombinedStatus(ctx context.Context, owner, repo, ref string, opts *ListOptions) (*CombinedStatus, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/commits/%v/status", owner, repo, url.QueryEscape(ref))
+	u := fmt.Sprintf("repos/%v/%v/commits/%v/status", owner, repo, refURLEscape(ref))
 	u, err := addOptions(u, opts)
 	if err != nil {
 		return nil, nil, err


Test patch:
diff --git a/github/actions_workflows_test.go b/github/actions_workflows_test.go
index 8d9f9232..e9448645 100644
--- a/github/actions_workflows_test.go
+++ b/github/actions_workflows_test.go
@@ -31,10 +31,10 @@ func TestActionsService_ListWorkflows(t *testing.T) {
 	}
 
 	want := &Workflows{
-		TotalCount: 4,
+		TotalCount: Int(4),
 		Workflows: []*Workflow{
-			{ID: 72844, CreatedAt: Timestamp{time.Date(2019, time.January, 02, 15, 04, 05, 0, time.UTC)}, UpdatedAt: Timestamp{time.Date(2020, time.January, 02, 15, 04, 05, 0, time.UTC)}},
-			{ID: 72845, CreatedAt: Timestamp{time.Date(2019, time.January, 02, 15, 04, 05, 0, time.UTC)}, UpdatedAt: Timestamp{time.Date(2020, time.January, 02, 15, 04, 05, 0, time.UTC)}},
+			{ID: Int64(72844), CreatedAt: &Timestamp{time.Date(2019, time.January, 02, 15, 04, 05, 0, time.UTC)}, UpdatedAt: &Timestamp{time.Date(2020, time.January, 02, 15, 04, 05, 0, time.UTC)}},
+			{ID: Int64(72845), CreatedAt: &Timestamp{time.Date(2019, time.January, 02, 15, 04, 05, 0, time.UTC)}, UpdatedAt: &Timestamp{time.Date(2020, time.January, 02, 15, 04, 05, 0, time.UTC)}},
 		},
 	}
 	if !reflect.DeepEqual(workflows, want) {
@@ -57,9 +57,9 @@ func TestActionsService_GetWorkflowByID(t *testing.T) {
 	}
 
 	want := &Workflow{
-		ID:        72844,
-		CreatedAt: Timestamp{time.Date(2019, time.January, 02, 15, 04, 05, 0, time.UTC)},
-		UpdatedAt: Timestamp{time.Date(2020, time.January, 02, 15, 04, 05, 0, time.UTC)},
+		ID:        Int64(72844),
+		CreatedAt: &Timestamp{time.Date(2019, time.January, 02, 15, 04, 05, 0, time.UTC)},
+		UpdatedAt: &Timestamp{time.Date(2020, time.January, 02, 15, 04, 05, 0, time.UTC)},
 	}
 	if !reflect.DeepEqual(workflow, want) {
 		t.Errorf("Actions.GetWorkflowByID returned %+v, want %+v", workflow, want)
@@ -81,9 +81,9 @@ func TestActionsService_GetWorkflowByFileName(t *testing.T) {
 	}
 
 	want := &Workflow{
-		ID:        72844,
-		CreatedAt: Timestamp{time.Date(2019, time.January, 02, 15, 04, 05, 0, time.UTC)},
-		UpdatedAt: Timestamp{time.Date(2020, time.January, 02, 15, 04, 05, 0, time.UTC)},
+		ID:        Int64(72844),
+		CreatedAt: &Timestamp{time.Date(2019, time.January, 02, 15, 04, 05, 0, time.UTC)},
+		UpdatedAt: &Timestamp{time.Date(2020, time.January, 02, 15, 04, 05, 0, time.UTC)},
 	}
 	if !reflect.DeepEqual(workflow, want) {
 		t.Errorf("Actions.GetWorkflowByFileName returned %+v, want %+v", workflow, want)
diff --git a/github/git_refs_test.go b/github/git_refs_test.go
index fb91c043..22dc8eb6 100644
--- a/github/git_refs_test.go
+++ b/github/git_refs_test.go
@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -444,3 +445,30 @@ func TestGitService_DeleteRef(t *testing.T) {
 		t.Errorf("Git.DeleteRef returned error: %v", err)
 	}
 }
+
+func TestGitService_GetRef_pathEscape(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")
+		if strings.Contains(r.URL.RawPath, "%2F") {
+			t.Errorf("RawPath still contains escaped / as %%2F: %v", r.URL.RawPath)
+		}
+		fmt.Fprint(w, `
+		  {
+		    "ref": "refs/heads/b",
+		    "url": "https://api.github.com/repos/o/r/git/refs/heads/b",
+		    "object": {
+		      "type": "commit",
+		      "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd",
+		      "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd"
+		    }
+		  }`)
+	})
+
+	_, _, err := client.Git.GetRef(context.Background(), "o", "r", "refs/heads/b")
+	if err != nil {
+		t.Fatalf("Git.GetRef returned error: %v", err)
+	}
+}
diff --git a/github/repos_commits_test.go b/github/repos_commits_test.go
index 54a60147..1c4fd774 100644
--- a/github/repos_commits_test.go
+++ b/github/repos_commits_test.go
@@ -262,6 +262,45 @@ func TestRepositoriesService_NonAlphabetCharacter_GetCommitSHA1(t *testing.T) {
 	}
 }
 
+func TestRepositoriesService_TrailingPercent_GetCommitSHA1(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+	const sha1 = "01234abcde"
+
+	mux.HandleFunc("/repos/o/r/commits/comm%", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")
+		testHeader(t, r, "Accept", mediaTypeV3SHA)
+
+		fmt.Fprintf(w, sha1)
+	})
+
+	got, _, err := client.Repositories.GetCommitSHA1(context.Background(), "o", "r", "comm%", "")
+	if err != nil {
+		t.Errorf("Repositories.GetCommitSHA1 returned error: %v", err)
+	}
+
+	if want := sha1; got != want {
+		t.Errorf("Repositories.GetCommitSHA1 = %v, want %v", got, want)
+	}
+
+	mux.HandleFunc("/repos/o/r/commits/tag", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")
+		testHeader(t, r, "Accept", mediaTypeV3SHA)
+		testHeader(t, r, "If-None-Match", `"`+sha1+`"`)
+
+		w.WriteHeader(http.StatusNotModified)
+	})
+
+	got, _, err = client.Repositories.GetCommitSHA1(context.Background(), "o", "r", "tag", sha1)
+	if err == nil {
+		t.Errorf("Expected HTTP 304 response")
+	}
+
+	if want := ""; got != want {
+		t.Errorf("Repositories.GetCommitSHA1 = %v, want %v", got, want)
+	}
+}
+
 func TestRepositoriesService_CompareCommits(t *testing.T) {
 	client, mux, _, teardown := setup()
 	defer teardown()