Skip to content

Commit

Permalink
Merge pull request #402 from pjbgf/git-repository
Browse files Browse the repository at this point in the history 
Create `git/repository` and Consolidate use of `ClientOption`
  • Loading branch information
Paulo Gomes authored Nov 15, 2022
2 parents aec27af + 1337974 commit 286ec55
Show file tree
Hide file tree
Showing 20 changed files with 328 additions and 285 deletions.
47 changes: 27 additions & 20 deletions git/gogit/repository_client.go → git/gogit/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,15 @@ import (

"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/git/gogit/fs"
"github.com/fluxcd/pkg/git/repository"
)

// ClientName is the string representation of Client.
const ClientName = "go-git"

// Client implements git.RepositoryClient.
// Client implements repository.Client.
type Client struct {
*git.DiscardRepositoryCloser
*repository.DiscardCloser
path string
repository *extgogit.Repository
authOpts *git.AuthOptions
Expand All @@ -55,7 +56,7 @@ type Client struct {
credentialsOverHTTP bool
}

var _ git.RepositoryClient = &Client{}
var _ repository.Client = &Client{}

type ClientOption func(*Client) error

Expand All @@ -72,7 +73,7 @@ func NewClient(path string, authOpts *git.AuthOptions, clientOpts ...ClientOptio
}

if len(clientOpts) == 0 {
clientOpts = append(clientOpts, WithDiskStorage)
clientOpts = append(clientOpts, WithDiskStorage())
}

for _, clientOpt := range clientOpts {
Expand Down Expand Up @@ -105,19 +106,23 @@ func WithWorkTreeFS(wt billy.Filesystem) ClientOption {
}
}

func WithDiskStorage(g *Client) error {
wt := fs.New(g.path)
dot := fs.New(filepath.Join(g.path, extgogit.GitDirName))
func WithDiskStorage() ClientOption {
return func(c *Client) error {
wt := fs.New(c.path)
dot := fs.New(filepath.Join(c.path, extgogit.GitDirName))

g.storer = filesystem.NewStorage(dot, cache.NewObjectLRUDefault())
g.worktreeFS = wt
return nil
c.storer = filesystem.NewStorage(dot, cache.NewObjectLRUDefault())
c.worktreeFS = wt
return nil
}
}

func WithMemoryStorage(g *Client) error {
g.storer = memory.NewStorage()
g.worktreeFS = memfs.New()
return nil
func WithMemoryStorage() ClientOption {
return func(c *Client) error {
c.storer = memory.NewStorage()
c.worktreeFS = memfs.New()
return nil
}
}

// WithForcePush enables the use of force push for all push operations
Expand All @@ -132,9 +137,11 @@ func WithForcePush() ClientOption {

// WithInsecureCredentialsOverHTTP enables credentials being used over
// HTTP. This is not recommended for production environments.
func WithInsecureCredentialsOverHTTP(g *Client) error {
g.credentialsOverHTTP = true
return nil
func WithInsecureCredentialsOverHTTP() ClientOption {
return func(c *Client) error {
c.credentialsOverHTTP = true
return nil
}
}

func (g *Client) Init(ctx context.Context, url, branch string) error {
Expand Down Expand Up @@ -178,7 +185,7 @@ func (g *Client) Init(ctx context.Context, url, branch string) error {
return nil
}

func (g *Client) Clone(ctx context.Context, url string, cloneOpts git.CloneOptions) (*git.Commit, error) {
func (g *Client) Clone(ctx context.Context, url string, cloneOpts repository.CloneOptions) (*git.Commit, error) {
if err := g.validateUrl(url); err != nil {
return nil, err
}
Expand Down Expand Up @@ -238,12 +245,12 @@ func (g *Client) writeFile(path string, reader io.Reader) error {
return err
}

func (g *Client) Commit(info git.Commit, commitOpts ...git.CommitOption) (string, error) {
func (g *Client) Commit(info git.Commit, commitOpts ...repository.CommitOption) (string, error) {
if g.repository == nil {
return "", git.ErrNoGitRepository
}

options := &git.CommitOptions{}
options := &repository.CommitOptions{}
for _, o := range commitOpts {
o(options)
}
Expand Down
5 changes: 3 additions & 2 deletions git/gogit/repository_client_test.go → git/gogit/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
. "github.com/onsi/gomega"

"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/git/repository"
"github.com/fluxcd/pkg/gittestserver"
)

Expand Down Expand Up @@ -163,7 +164,7 @@ func TestCommit(t *testing.T) {
},
Message: "testing",
},
git.WithFiles(map[string]io.Reader{
repository.WithFiles(map[string]io.Reader{
"test": strings.NewReader("testing gogit commit"),
}),
)
Expand Down Expand Up @@ -275,7 +276,7 @@ func TestForcePush(t *testing.T) {
cc2, err := commitFile(repo2, "test", "first push from second clone", time.Now())
g.Expect(err).ToNot(HaveOccurred())

ggc2, err := NewClient(tmp2, nil, WithDiskStorage, WithForcePush())
ggc2, err := NewClient(tmp2, nil, WithDiskStorage(), WithForcePush())
g.Expect(err).ToNot(HaveOccurred())
ggc2.repository = repo2

Expand Down
38 changes: 29 additions & 9 deletions git/gogit/clone.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ import (
"github.com/fluxcd/go-git/v5/storage/memory"

"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/gitutil"
"github.com/fluxcd/pkg/git/repository"
"github.com/fluxcd/pkg/version"
)

func (g *Client) cloneBranch(ctx context.Context, url, branch string, opts git.CloneOptions) (*git.Commit, error) {
func (g *Client) cloneBranch(ctx context.Context, url, branch string, opts repository.CloneOptions) (*git.Commit, error) {
if g.authOpts == nil {
return nil, fmt.Errorf("unable to checkout repo with an empty set of auth options")
}
Expand Down Expand Up @@ -111,7 +111,7 @@ func (g *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C
}
}
if err != nil {
return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.GoGitError(err))
return nil, fmt.Errorf("unable to clone '%s': %w", url, goGitError(err))
}
}

Expand All @@ -127,7 +127,7 @@ func (g *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C
return buildCommitWithRef(cc, ref)
}

func (g *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOptions) (*git.Commit, error) {
func (g *Client) cloneTag(ctx context.Context, url, tag string, opts repository.CloneOptions) (*git.Commit, error) {
if g.authOpts == nil {
return nil, fmt.Errorf("unable to checkout repo with an empty set of auth options")
}
Expand Down Expand Up @@ -190,7 +190,7 @@ func (g *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp
URL: url,
}
}
return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.GoGitError(err))
return nil, fmt.Errorf("unable to clone '%s': %w", url, goGitError(err))
}

head, err := repo.Head()
Expand All @@ -205,7 +205,7 @@ func (g *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp
return buildCommitWithRef(cc, ref)
}

func (g *Client) cloneCommit(ctx context.Context, url, commit string, opts git.CloneOptions) (*git.Commit, error) {
func (g *Client) cloneCommit(ctx context.Context, url, commit string, opts repository.CloneOptions) (*git.Commit, error) {
authMethod, err := transportAuth(g.authOpts)
if err != nil {
return nil, fmt.Errorf("unable to construct auth method with options: %w", err)
Expand Down Expand Up @@ -235,7 +235,7 @@ func (g *Client) cloneCommit(ctx context.Context, url, commit string, opts git.C
URL: url,
}
}
return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.GoGitError(err))
return nil, fmt.Errorf("unable to clone '%s': %w", url, goGitError(err))
}

w, err := repo.Worktree()
Expand All @@ -257,7 +257,7 @@ func (g *Client) cloneCommit(ctx context.Context, url, commit string, opts git.C
return buildCommitWithRef(cc, cloneOpts.ReferenceName)
}

func (g *Client) cloneSemVer(ctx context.Context, url, semverTag string, opts git.CloneOptions) (*git.Commit, error) {
func (g *Client) cloneSemVer(ctx context.Context, url, semverTag string, opts repository.CloneOptions) (*git.Commit, error) {
verConstraint, err := semver.NewConstraint(semverTag)
if err != nil {
return nil, fmt.Errorf("semver parse error: %w", err)
Expand Down Expand Up @@ -291,7 +291,7 @@ func (g *Client) cloneSemVer(ctx context.Context, url, semverTag string, opts gi
URL: url,
}
}
return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.GoGitError(err))
return nil, fmt.Errorf("unable to clone '%s': %w", url, goGitError(err))
}

repoTags, err := repo.Tags()
Expand Down Expand Up @@ -453,3 +453,23 @@ func buildCommitWithRef(c *object.Commit, ref plumbing.ReferenceName) (*git.Comm
func isRemoteBranchNotFoundErr(err error, ref string) bool {
return strings.Contains(err.Error(), fmt.Sprintf("couldn't find remote ref '%s'", ref))
}

// goGitError translates an error from the go-git library, or returns
// `nil` if the argument is `nil`.
func goGitError(err error) error {
if err == nil {
return nil
}
switch strings.TrimSpace(err.Error()) {
case "unknown error: remote:":
// this unhelpful error arises because go-git takes the first
// line of the output on stderr, and for some git providers
// (GitLab, at least) the output has a blank line at the
// start. The rest of stderr is thrown away, so we can't get
// the actual error; but at least we know what was being
// attempted, and the likely cause.
return fmt.Errorf("push rejected; check git secret has write access")
default:
return err
}
}
76 changes: 59 additions & 17 deletions git/gogit/clone_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
"github.com/fluxcd/gitkit"
"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/git/gogit/fs"
"github.com/fluxcd/pkg/git/repository"
"github.com/fluxcd/pkg/gittestserver"
"github.com/fluxcd/pkg/ssh"
)
Expand Down Expand Up @@ -132,8 +133,8 @@ func TestClone_cloneBranch(t *testing.T) {
upstreamPath = repoPath
}

cc, err := ggc.Clone(context.TODO(), upstreamPath, git.CloneOptions{
CheckoutStrategy: git.CheckoutStrategy{
cc, err := ggc.Clone(context.TODO(), upstreamPath, repository.CloneOptions{
CheckoutStrategy: repository.CheckoutStrategy{
Branch: tt.branch,
},
ShallowClone: true,
Expand Down Expand Up @@ -245,8 +246,8 @@ func TestClone_cloneTag(t *testing.T) {
ggc, err := NewClient(tmpDir, &git.AuthOptions{Transport: git.HTTP})
g.Expect(err).ToNot(HaveOccurred())

opts := git.CloneOptions{
CheckoutStrategy: git.CheckoutStrategy{
opts := repository.CloneOptions{
CheckoutStrategy: repository.CheckoutStrategy{
Tag: tt.checkoutTag,
},
ShallowClone: true,
Expand Down Expand Up @@ -338,8 +339,8 @@ func TestClone_cloneCommit(t *testing.T) {
g := NewWithT(t)

tmpDir := t.TempDir()
opts := git.CloneOptions{
CheckoutStrategy: git.CheckoutStrategy{
opts := repository.CloneOptions{
CheckoutStrategy: repository.CheckoutStrategy{
Branch: tt.branch,
Commit: tt.commit,
},
Expand Down Expand Up @@ -452,8 +453,8 @@ func TestClone_cloneSemVer(t *testing.T) {
ggc, err := NewClient(tmpDir, nil)
g.Expect(err).ToNot(HaveOccurred())

opts := git.CloneOptions{
CheckoutStrategy: git.CheckoutStrategy{
opts := repository.CloneOptions{
CheckoutStrategy: repository.CheckoutStrategy{
SemVer: tt.constraint,
},
ShallowClone: true,
Expand Down Expand Up @@ -559,8 +560,8 @@ func Test_ssh_KeyTypes(t *testing.T) {
ggc, err := NewClient(tmpDir, &authOpts)
g.Expect(err).ToNot(HaveOccurred())

cc, err := ggc.Clone(ctx, repoURL, git.CloneOptions{
CheckoutStrategy: git.CheckoutStrategy{
cc, err := ggc.Clone(ctx, repoURL, repository.CloneOptions{
CheckoutStrategy: repository.CheckoutStrategy{
Branch: git.DefaultBranch,
},
ShallowClone: true,
Expand Down Expand Up @@ -690,8 +691,8 @@ func Test_ssh_KeyExchangeAlgos(t *testing.T) {
ggc, err := NewClient(tmpDir, &authOpts)
g.Expect(err).ToNot(HaveOccurred())

_, err = ggc.Clone(ctx, repoURL, git.CloneOptions{
CheckoutStrategy: git.CheckoutStrategy{
_, err = ggc.Clone(ctx, repoURL, repository.CloneOptions{
CheckoutStrategy: repository.CheckoutStrategy{
Branch: git.DefaultBranch,
},
ShallowClone: true,
Expand Down Expand Up @@ -862,8 +863,8 @@ func Test_ssh_HostKeyAlgos(t *testing.T) {
ggc, err := NewClient(tmpDir, &authOpts)
g.Expect(err).ToNot(HaveOccurred())

_, err = ggc.Clone(ctx, repoURL, git.CloneOptions{
CheckoutStrategy: git.CheckoutStrategy{
_, err = ggc.Clone(ctx, repoURL, repository.CloneOptions{
CheckoutStrategy: repository.CheckoutStrategy{
Branch: git.DefaultBranch,
},
ShallowClone: true,
Expand Down Expand Up @@ -1038,9 +1039,9 @@ func TestClone_CredentialsOverHttp(t *testing.T) {
previousRequestCount = totalRequests

tmpDir := t.TempDir()
opts := []ClientOption{WithDiskStorage}
opts := []ClientOption{WithDiskStorage()}
if tt.allowCredentialsOverHttp {
opts = append(opts, WithInsecureCredentialsOverHTTP)
opts = append(opts, WithInsecureCredentialsOverHTTP())
}

ggc, err := NewClient(tmpDir, &git.AuthOptions{
Expand All @@ -1056,7 +1057,7 @@ func TestClone_CredentialsOverHttp(t *testing.T) {
repoURL = tt.transformURL(ts.URL)
}

_, err = ggc.Clone(context.TODO(), repoURL, git.CloneOptions{})
_, err = ggc.Clone(context.TODO(), repoURL, repository.CloneOptions{})

if tt.expectCloneErr != "" {
g.Expect(err).To(HaveOccurred())
Expand All @@ -1074,6 +1075,47 @@ func TestClone_CredentialsOverHttp(t *testing.T) {
}
}

func TestGoGitErrorReplace(t *testing.T) {
// this is what go-git uses as the error message is if the remote
// sends a blank first line
unknownMessage := `unknown error: remote: `
err := errors.New(unknownMessage)
err = goGitError(err)
reformattedMessage := err.Error()
if reformattedMessage == unknownMessage {
t.Errorf("expected rewritten error, got %q", reformattedMessage)
}
}

func TestGoGitErrorUnchanged(t *testing.T) {
// this is (roughly) what GitHub sends if the deploy key doesn't
// have write access; go-git passes this on verbatim
regularMessage := `remote: ERROR: deploy key does not have write access`
expectedReformat := regularMessage
err := errors.New(regularMessage)
err = goGitError(err)
reformattedMessage := err.Error()
// test that it's been rewritten, without checking the exact content
if len(reformattedMessage) > len(expectedReformat) {
t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage)
}
}

func Fuzz_GoGitError(f *testing.F) {
f.Add("")
f.Add("unknown error: remote: ")
f.Add("some other error")

f.Fuzz(func(t *testing.T, msg string) {
var err error
if msg != "" {
err = errors.New(msg)
}

_ = goGitError(err)
})
}

func initRepo(tmpDir string) (*extgogit.Repository, string, error) {
sto := filesystem.NewStorage(fs.New(tmpDir), cache.NewObjectLRUDefault())
repo, err := extgogit.Init(sto, memfs.New())
Expand Down
Loading

0 comments on commit 286ec55

Please sign in to comment.