Merge pull request #42 from chhsia0/cleanup

Cleaned up content service and added missing tests.

Author: bradrydzewski

scm/driver/gitea/git.go

@@ -25,8 +25,7 @@ func (s *gitService) FindBranch(ctx context.Context, repo, name string) (*scm.Re
 }
 
 func (s *gitService) FindCommit(ctx context.Context, repo, ref string) (*scm.Commit, *scm.Response, error) {
-	ref = url.PathEscape(ref)
-	path := fmt.Sprintf("api/v1/repos/%s/git/commits/%s", repo, ref)
+	path := fmt.Sprintf("api/v1/repos/%s/git/commits/%s", repo, url.PathEscape(ref))
 	out := new(commitInfo)
 	res, err := s.client.do(ctx, "GET", path, nil, out)
 	return convertCommitInfo(out), res, err

scm/driver/gitea/git_test.go

@@ -19,7 +19,7 @@ import (
 // commit sub-tests
 //
 
-func TestCommitFind(t *testing.T) {
+func TestGitFindCommit(t *testing.T) {
 	gock.New("https://try.gitea.io").
 		Get("/api/v1/repos/gitea/gitea/git/commits/c43399cad8766ee521b873a32c1652407c5a4630").
 		Reply(200).
@@ -46,27 +46,41 @@ func TestCommitFind(t *testing.T) {
 	}
 }
 
-func TestCommitList(t *testing.T) {
+func TestGitListCommits(t *testing.T) {
 	client, _ := New("https://try.gitea.io")
 	_, _, err := client.Git.ListCommits(context.Background(), "go-gitea/gitea", scm.CommitListOptions{})
 	if err != scm.ErrNotSupported {
 		t.Errorf("Expect Not Supported error")
 	}
 }
 
-func TestChangeList(t *testing.T) {
+func TestGitListChanges(t *testing.T) {
 	client, _ := New("https://try.gitea.io")
 	_, _, err := client.Git.ListChanges(context.Background(), "go-gitea/gitea", "f05f642b892d59a0a9ef6a31f6c905a24b5db13a", scm.ListOptions{})
 	if err != scm.ErrNotSupported {
 		t.Errorf("Expect Not Supported error")
 	}
 }
 
+func TestGitCompareChanges(t *testing.T) {
+	client, _ := New("https://try.gitea.io")
+	_, _, err := client.Git.CompareChanges(
+		context.Background(),
+		"go-gitea/gitea",
+		"d293a2b9d6722dffde7998c953c3087e47a38a83",
+		"f05f642b892d59a0a9ef6a31f6c905a24b5db13a",
+		scm.ListOptions{},
+	)
+	if err != scm.ErrNotSupported {
+		t.Errorf("Expect Not Supported error")
+	}
+}
+
 //
 // branch sub-tests
 //
 
-func TestBranchFind(t *testing.T) {
+func TestGitFindBranch(t *testing.T) {
 	defer gock.Off()
 
 	gock.New("https://try.gitea.io").
@@ -91,7 +105,7 @@ func TestBranchFind(t *testing.T) {
 	}
 }
 
-func TestBranchList(t *testing.T) {
+func TestGitListBranches(t *testing.T) {
 	defer gock.Off()
 
 	gock.New("https://try.gitea.io").
@@ -120,15 +134,15 @@ func TestBranchList(t *testing.T) {
 // tag sub-tests
 //
 
-func TestTagFind(t *testing.T) {
+func TestGitFindTag(t *testing.T) {
 	client, _ := New("https://try.gitea.io")
 	_, _, err := client.Git.FindTag(context.Background(), "go-gitea/gitea", "v1.0.0")
 	if err != scm.ErrNotSupported {
 		t.Errorf("Expect Not Supported error")
 	}
 }
 
-func TestTagList(t *testing.T) {
+func TestGitListTags(t *testing.T) {
 	client, _ := New("https://try.gitea.io")
 	_, _, err := client.Git.ListTags(context.Background(), "go-gitea/gitea", scm.ListOptions{})
 	if err != scm.ErrNotSupported {

scm/driver/gitlab/content.go

@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"net/url"
 	"strconv"
-	"strings"
 
 	"github.com/drone/go-scm/scm"
 )
@@ -20,9 +19,7 @@ type contentService struct {
 }
 
 func (s *contentService) Find(ctx context.Context, repo, path, ref string) (*scm.Content, *scm.Response, error) {
-	path = url.QueryEscape(path)
-	path = strings.Replace(path, ".", "%2E", -1)
-	endpoint := fmt.Sprintf("api/v4/projects/%s/repository/files/%s?ref=%s", encode(repo), path, ref)
+	endpoint := fmt.Sprintf("api/v4/projects/%s/repository/files/%s?ref=%s", encode(repo), encodePath(path), ref)
 	out := new(content)
 	res, err := s.client.do(ctx, "GET", endpoint, nil, out)
 	raw, berr := base64.StdEncoding.DecodeString(out.Content)
@@ -41,9 +38,7 @@ func (s *contentService) Find(ctx context.Context, repo, path, ref string) (*scm
 }
 
 func (s *contentService) Create(ctx context.Context, repo, path string, params *scm.ContentParams) (*scm.Response, error) {
-	path = url.QueryEscape(path)
-	path = strings.Replace(path, ".", "%2E", -1)
-	endpoint := fmt.Sprintf("api/v4/projects/%s/repository/files/%s", encode(repo), path)
+	endpoint := fmt.Sprintf("api/v4/projects/%s/repository/files/%s", encode(repo), encodePath(path))
 	in := &createUpdateContent{
 		Branch:        params.Branch,
 		Content:       params.Data,
@@ -58,9 +53,7 @@ func (s *contentService) Create(ctx context.Context, repo, path string, params *
 }
 
 func (s *contentService) Update(ctx context.Context, repo, path string, params *scm.ContentParams) (*scm.Response, error) {
-	path = url.QueryEscape(path)
-	path = strings.Replace(path, ".", "%2E", -1)
-	endpoint := fmt.Sprintf("api/v4/projects/%s/repository/files/%s", encode(repo), path)
+	endpoint := fmt.Sprintf("api/v4/projects/%s/repository/files/%s", encode(repo), encodePath(path))
 	in := &createUpdateContent{
 		Branch:        params.Branch,
 		Content:       params.Data,

scm/driver/gitlab/content_test.go

@@ -158,41 +158,3 @@ func TestContentList(t *testing.T) {
 	t.Run("Request", testRequest(res))
 	t.Run("Rate", testRate(res))
 }
-
-var fileContent = []byte(`require 'digest/md5'
-
-class Key < ActiveRecord::Base
-  include Gitlab::CurrentSettings
-  include Sortable
-
-  belongs_to :user
-
-  before_validation :generate_fingerprint
-
-  validates :title,
-    presence: true,
-    length: { maximum: 255 }
-
-  validates :key,
-    presence: true,
-    length: { maximum: 5000 },
-    format: { with: /\A(ssh|ecdsa)-.*\Z/ }
-
-  validates :fingerprint,
-    uniqueness: true,
-    presence: { message: 'cannot be generated' }
-
-  validate :key_meets_restrictions
-
-  delegate :name, :email, to: :user, prefix: true
-
-  after_commit :add_to_shell, on: :create
-  after_create :post_create_hook
-  after_create :refresh_user_cache
-  after_commit :remove_from_shell, on: :destroy
-  after_destroy :post_destroy_hook
-  after_destroy :refresh_user_cache
-
-  def key=(value)
-    value&.delete!("\n\r")
-    value.strip! unless value.blank`)

scm/driver/gitlab/integration/issue_test.go

@@ -103,7 +103,7 @@ func testIssue(issue *scm.Issue) func(t *testing.T) {
 		if got, want := issue.Title, "New issue"; got != want {
 			t.Errorf("Want issue Title %q, got %q", want, got)
 		}
-		if got, want := issue.Link, "https://gitlab.com/gitlab-org/testme/issues/1"; got != want {
+		if got, want := issue.Link, "https://gitlab.com/gitlab-org/testme/-/issues/1"; got != want {
 			t.Errorf("Want issue Title %q, got %q", want, got)
 		}
 		if got, want := issue.Author.Login, "marin"; got != want {

scm/driver/gitlab/integration/pr_test.go

@@ -147,7 +147,7 @@ func testPullRequest(pr *scm.PullRequest) func(t *testing.T) {
 		if got, want := pr.Sha, "12d65c8dd2b2676fa3ac47d955accc085a37a9c1"; got != want {
 			t.Errorf("Want pr Sha %q, got %q", want, got)
 		}
-		if got, want := pr.Link, "https://gitlab.com/gitlab-org/testme/merge_requests/1"; got != want {
+		if got, want := pr.Link, "https://gitlab.com/gitlab-org/testme/-/merge_requests/1"; got != want {
 			t.Errorf("Want pr Link %q, got %q", want, got)
 		}
 		if got, want := pr.Author.Login, "dblessing"; got != want {

scm/driver/gitlab/testdata/content.rb

@@ -0,0 +1,37 @@
+require 'digest/md5'
+
+class Key < ActiveRecord::Base
+  include Gitlab::CurrentSettings
+  include Sortable
+
+  belongs_to :user
+
+  before_validation :generate_fingerprint
+
+  validates :title,
+    presence: true,
+    length: { maximum: 255 }
+
+  validates :key,
+    presence: true,
+    length: { maximum: 5000 },
+    format: { with: /\A(ssh|ecdsa)-.*\Z/ }
+
+  validates :fingerprint,
+    uniqueness: true,
+    presence: { message: 'cannot be generated' }
+
+  validate :key_meets_restrictions
+
+  delegate :name, :email, to: :user, prefix: true
+
+  after_commit :add_to_shell, on: :create
+  after_create :post_create_hook
+  after_create :refresh_user_cache
+  after_commit :remove_from_shell, on: :destroy
+  after_destroy :post_destroy_hook
+  after_destroy :refresh_user_cache
+
+  def key=(value)
+    value&.delete!("\n\r")
+    value.strip! unless value.blank

scm/driver/gitlab/util.go

@@ -16,6 +16,14 @@ func encode(s string) string {
 	return strings.Replace(s, "/", "%2F", -1)
 }
 
+func encodePath(s string) string {
+	// Gitlab documentation provides inconsistent example for whether '.' should be escaped:
+	// https://docs.gitlab.com/ee/api/README.html#file-path-branches-and-tags-name-encoding
+	// https://docs.gitlab.com/ee/api/repository_files.html#get-file-from-repository
+	// Although not escaping '.' seems to work, we still escape it here to be safe.
+	return strings.Replace(url.PathEscape(s), ".", "%2E", -1)
+}
+
 func encodeListOptions(opts scm.ListOptions) string {
 	params := url.Values{}
 	if opts.Page != 0 {

scm/driver/stash/git.go

@@ -64,10 +64,7 @@ func (s *gitService) ListBranches(ctx context.Context, repo string, opts scm.Lis
 	path := fmt.Sprintf("rest/api/1.0/projects/%s/repos/%s/branches?%s", namespace, name, encodeListOptions(opts))
 	out := new(branches)
 	res, err := s.client.do(ctx, "GET", path, nil, out)
-	if !out.pagination.LastPage.Bool {
-		res.Page.First = 1
-		res.Page.Next = opts.Page + 1
-	}
+	copyPagination(out.pagination, res)
 	return convertBranchList(out), res, err
 }
 
@@ -79,35 +76,26 @@ func (s *gitService) ListTags(ctx context.Context, repo string, opts scm.ListOpt
 	namespace, name := scm.Split(repo)
 	path := fmt.Sprintf("rest/api/1.0/projects/%s/repos/%s/tags?%s", namespace, name, encodeListOptions(opts))
 	out := new(branches)
-	res, err := s.client.do(ctx, "GET", path, nil, &out)
-	if !out.pagination.LastPage.Bool {
-		res.Page.First = 1
-		res.Page.Next = opts.Page + 1
-	}
+	res, err := s.client.do(ctx, "GET", path, nil, out)
+	copyPagination(out.pagination, res)
 	return convertTagList(out), res, err
 }
 
 func (s *gitService) ListChanges(ctx context.Context, repo, ref string, opts scm.ListOptions) ([]*scm.Change, *scm.Response, error) {
 	namespace, name := scm.Split(repo)
 	path := fmt.Sprintf("rest/api/1.0/projects/%s/repos/%s/commits/%s/changes?%s", namespace, name, ref, encodeListOptions(opts))
 	out := new(diffstats)
-	res, err := s.client.do(ctx, "GET", path, nil, &out)
-	if !out.pagination.LastPage.Bool {
-		res.Page.First = 1
-		res.Page.Next = opts.Page + 1
-	}
+	res, err := s.client.do(ctx, "GET", path, nil, out)
+	copyPagination(out.pagination, res)
 	return convertDiffstats(out), res, err
 }
 
 func (s *gitService) CompareChanges(ctx context.Context, repo, source, target string, opts scm.ListOptions) ([]*scm.Change, *scm.Response, error) {
 	namespace, name := scm.Split(repo)
 	path := fmt.Sprintf("rest/api/1.0/projects/%s/repos/%s/compare/changes?from=%s&to=%s&%s", namespace, name, source, target, encodeListOptions(opts))
 	out := new(diffstats)
-	res, err := s.client.do(ctx, "GET", path, nil, &out)
-	if !out.pagination.LastPage.Bool {
-		res.Page.First = 1
-		res.Page.Next = opts.Page + 1
-	}
+	res, err := s.client.do(ctx, "GET", path, nil, out)
+	copyPagination(out.pagination, res)
 	return convertDiffstats(out), res, err
 }