From 4d71f21e2e7cea5d5eae4af28e4ac457c1bfb2e9 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Fri, 11 Nov 2022 12:35:47 +0000 Subject: [PATCH 1/4] git: Rename git.RepositoryClient to repository.Client Left over changes from the initial PR, which aims at making the pkg/git packages more Go idiomatic. xref: https://github.com/fluxcd/pkg/pull/300#discussion_r932037900 Signed-off-by: Paulo Gomes --- git/gogit/{repository_client.go => client.go} | 7 ++-- ...pository_client_test.go => client_test.go} | 0 git/internal/e2e/utils.go | 5 ++- .../{repository_client.go => client.go} | 3 +- ...pository_client_test.go => client_test.go} | 0 git/libgit2/go.mod | 2 +- git/{ => repository}/client.go | 42 ++++++++++--------- 7 files changed, 32 insertions(+), 27 deletions(-) rename git/gogit/{repository_client.go => client.go} (98%) rename git/gogit/{repository_client_test.go => client_test.go} (100%) rename git/libgit2/{repository_client.go => client.go} (99%) rename git/libgit2/{repository_client_test.go => client_test.go} (100%) rename git/{ => repository}/client.go (67%) diff --git a/git/gogit/repository_client.go b/git/gogit/client.go similarity index 98% rename from git/gogit/repository_client.go rename to git/gogit/client.go index be2493b7..d976e117 100644 --- a/git/gogit/repository_client.go +++ b/git/gogit/client.go @@ -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 @@ -55,7 +56,7 @@ type Client struct { credentialsOverHTTP bool } -var _ git.RepositoryClient = &Client{} +var _ repository.Client = &Client{} type ClientOption func(*Client) error diff --git a/git/gogit/repository_client_test.go b/git/gogit/client_test.go similarity index 100% rename from git/gogit/repository_client_test.go rename to git/gogit/client_test.go diff --git a/git/internal/e2e/utils.go b/git/internal/e2e/utils.go index 53857a7c..59092e8c 100644 --- a/git/internal/e2e/utils.go +++ b/git/internal/e2e/utils.go @@ -37,12 +37,13 @@ import ( . "github.com/onsi/gomega" "github.com/fluxcd/pkg/git" + "github.com/fluxcd/pkg/git/repository" "github.com/fluxcd/pkg/ssh" ) var letterRunes = []rune("abcdefghijklmnopqrstuvwxyz1234567890") -func testUsingClone(g *WithT, client git.RepositoryClient, repoURL *url.URL, upstreamRepo upstreamRepoInfo) { +func testUsingClone(g *WithT, client repository.Client, repoURL *url.URL, upstreamRepo upstreamRepoInfo) { // clone repo _, err := client.Clone(context.TODO(), repoURL.String(), git.CloneOptions{ CheckoutStrategy: git.CheckoutStrategy{ @@ -105,7 +106,7 @@ func testUsingClone(g *WithT, client git.RepositoryClient, repoURL *url.URL, ups g.Expect(headCommit).To(Equal(cc)) } -func testUsingInit(g *WithT, client git.RepositoryClient, repoURL *url.URL, upstreamRepo upstreamRepoInfo) { +func testUsingInit(g *WithT, client repository.Client, repoURL *url.URL, upstreamRepo upstreamRepoInfo) { // Create a new repository err := client.Init(context.TODO(), repoURL.String(), "main") g.Expect(err).ToNot(HaveOccurred()) diff --git a/git/libgit2/repository_client.go b/git/libgit2/client.go similarity index 99% rename from git/libgit2/repository_client.go rename to git/libgit2/client.go index 899b99f1..46fb0dfa 100644 --- a/git/libgit2/repository_client.go +++ b/git/libgit2/client.go @@ -37,6 +37,7 @@ import ( "github.com/fluxcd/pkg/git" "github.com/fluxcd/pkg/git/libgit2/transport" + "github.com/fluxcd/pkg/git/repository" "github.com/fluxcd/pkg/gitutil" ) @@ -62,7 +63,7 @@ type Client struct { credentialsOverHTTP bool } -var _ git.RepositoryClient = &Client{} +var _ repository.Client = &Client{} type ClientOption func(*Client) error diff --git a/git/libgit2/repository_client_test.go b/git/libgit2/client_test.go similarity index 100% rename from git/libgit2/repository_client_test.go rename to git/libgit2/client_test.go diff --git a/git/libgit2/go.mod b/git/libgit2/go.mod index a0dc00c3..baa5b32f 100644 --- a/git/libgit2/go.mod +++ b/git/libgit2/go.mod @@ -5,7 +5,7 @@ go 1.18 replace ( github.com/fluxcd/pkg/git => ../../git // Enables the use of pkg/git/gogit/fs. - github.com/fluxcd/pkg/git/gogit => ../../git/gogit + github.com/fluxcd/pkg/git/gogit => ../gogit github.com/fluxcd/pkg/gittestserver => ../../gittestserver github.com/fluxcd/pkg/gitutil => ../../gitutil github.com/fluxcd/pkg/http/transport => ../../http/transport diff --git a/git/client.go b/git/repository/client.go similarity index 67% rename from git/client.go rename to git/repository/client.go index e9d96abb..4f33380f 100644 --- a/git/client.go +++ b/git/repository/client.go @@ -14,31 +14,33 @@ See the License for the specific language governing permissions and limitations under the License. */ -package git +package repository import ( "context" + + "github.com/fluxcd/pkg/git" ) -// RepositoryReader knows how to perform local and remote read operations +// Reader knows how to perform local and remote read operations // on a Git repository. -type RepositoryReader interface { +type Reader interface { // Clone clones a repository from the provided url using the options provided. // It returns a Commit object describing the Git commit that the repository // HEAD points to. If the repository is empty, it returns a nil Commit. - Clone(ctx context.Context, url string, cloneOpts CloneOptions) (*Commit, error) + Clone(ctx context.Context, url string, cloneOpts git.CloneOptions) (*git.Commit, error) // IsClean returns whether the working tree is clean. IsClean() (bool, error) // Head returns the hash of the current HEAD of the repo. Head() (string, error) // Path returns the path of the repository. Path() string - RepositoryCloser + Closer } -// RepositoryWriter knows how to perform local and remote write operations +// Writer knows how to perform local and remote write operations // on a Git repository. -type RepositoryWriter interface { +type Writer interface { // Init initializes a repository at the configured path with the remote // origin set to url on the provided branch. Init(ctx context.Context, url, branch string) error @@ -49,28 +51,28 @@ type RepositoryWriter interface { SwitchBranch(ctx context.Context, branch string) error // Commit commits any changes made to the repository. commitOpts is an // optional argument which can be provided to configure the commit. - Commit(info Commit, commitOpts ...CommitOption) (string, error) - RepositoryCloser + Commit(info git.Commit, commitOpts ...git.CommitOption) (string, error) + Closer } -// RepositoryCloser knows how to perform any operations that need to happen -// at the end of the lifecycle of a RepositoryWriter/RepositoryReader. +// Closer knows how to perform any operations that need to happen +// at the end of the lifecycle of a Writer/Reader. // When this is not required by the implementation, it can simply embed an -// anonymous pointer to DiscardRepositoryCloser. -type RepositoryCloser interface { +// anonymous pointer to DiscardCloser. +type Closer interface { // Close closes any resources that need to be closed at the end of // a Git repository client's lifecycle. Close() } -// RepositoryClient knows how to perform local and remote operations on +// Client knows how to perform local and remote operations on // a Git repository. -type RepositoryClient interface { - RepositoryReader - RepositoryWriter +type Client interface { + Reader + Writer } -// DiscardRepositoryCloser is a RepositoryCloser which discards calls to Close(). -type DiscardRepositoryCloser struct{} +// DiscardCloser is a Closer which discards calls to Close(). +type DiscardCloser struct{} -func (c *DiscardRepositoryCloser) Close() {} +func (c *DiscardCloser) Close() {} From dccd499d625618d22c06a004f375a50770063f2e Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Fri, 11 Nov 2022 12:42:27 +0000 Subject: [PATCH 2/4] git: Consolidate the use of ClientOption The use of options were a mix of value and funcs, which in some cases was confusing: NewClient(os.TempDir(), nil, WithDiskStorage, WithForcePush(), WithInsecureCredentialsOverHTTP) With the changes, all options are exposed as funcs instead: NewClient(os.TempDir(), nil, WithDiskStorage(), WithForcePush(), WithInsecureCredentialsOverHTTP()) The above changes aligns with the standards used in source controller internal/reconcile/summarize. Signed-off-by: Paulo Gomes --- git/gogit/client.go | 34 ++++++++++++++++++++-------------- git/gogit/client_test.go | 2 +- git/gogit/clone_test.go | 4 ++-- git/libgit2/client.go | 26 ++++++++++++++++---------- git/libgit2/clone_test.go | 4 ++-- 5 files changed, 41 insertions(+), 29 deletions(-) diff --git a/git/gogit/client.go b/git/gogit/client.go index d976e117..bfae32b9 100644 --- a/git/gogit/client.go +++ b/git/gogit/client.go @@ -73,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 { @@ -106,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 @@ -133,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 { diff --git a/git/gogit/client_test.go b/git/gogit/client_test.go index a36ecaa5..3bdd8e24 100644 --- a/git/gogit/client_test.go +++ b/git/gogit/client_test.go @@ -275,7 +275,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 diff --git a/git/gogit/clone_test.go b/git/gogit/clone_test.go index 27a1253c..65cf4c4c 100644 --- a/git/gogit/clone_test.go +++ b/git/gogit/clone_test.go @@ -1038,9 +1038,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{ diff --git a/git/libgit2/client.go b/git/libgit2/client.go index 46fb0dfa..776a5e87 100644 --- a/git/libgit2/client.go +++ b/git/libgit2/client.go @@ -79,7 +79,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 { @@ -95,21 +95,27 @@ func NewClient(path string, authOpts *git.AuthOptions, clientOpts ...ClientOptio return l, nil } -func WithDiskStorage(l *Client) error { - l.repoFS = osfs.New(l.path) - return nil +func WithDiskStorage() ClientOption { + return func(c *Client) error { + c.repoFS = osfs.New(c.path) + return nil + } } -func WithMemoryStorage(l *Client) error { - l.repoFS = memfs.New() - return nil +func WithMemoryStorage() ClientOption { + return func(c *Client) error { + c.repoFS = memfs.New() + return nil + } } // WithInsecureCredentialsOverHTTP enables credentials being used over // HTTP. This is not recommended for production environments. -func WithInsecureCredentialsOverHTTP(l *Client) error { - l.credentialsOverHTTP = true - return nil +func WithInsecureCredentialsOverHTTP() ClientOption { + return func(c *Client) error { + c.credentialsOverHTTP = true + return nil + } } func (l *Client) Init(ctx context.Context, url, branch string) error { diff --git a/git/libgit2/clone_test.go b/git/libgit2/clone_test.go index dd8c707f..b4965bb7 100644 --- a/git/libgit2/clone_test.go +++ b/git/libgit2/clone_test.go @@ -647,9 +647,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()) } lgc, err := NewClient(tmpDir, &git.AuthOptions{ From 6db7f537983e9fc394267e00f57b8ccb7980dc65 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 15 Nov 2022 13:06:51 +0000 Subject: [PATCH 3/4] git: Move pkg/gitutil into pkg/git The gitutil package sole purpose was to provide better error handling for both go-git and libgit2 implementations. By moving this package into the respective git implementations, they become self-contained. Signed-off-by: Paulo Gomes --- git/gogit/clone.go | 29 ++++++++++++++--- git/gogit/clone_test.go | 41 +++++++++++++++++++++++ git/gogit/go.mod | 2 -- git/internal/e2e/go.mod | 2 -- git/libgit2/client.go | 5 ++- git/libgit2/clone.go | 21 ++++++------ {gitutil => git/libgit2}/errors.go | 23 +------------ {gitutil => git/libgit2}/errors_test.go | 43 +------------------------ git/libgit2/go.mod | 2 -- gitutil/go.mod | 3 -- 10 files changed, 79 insertions(+), 92 deletions(-) rename {gitutil => git/libgit2}/errors.go (68%) rename {gitutil => git/libgit2}/errors_test.go (69%) delete mode 100644 gitutil/go.mod diff --git a/git/gogit/clone.go b/git/gogit/clone.go index b1608cc6..63038e49 100644 --- a/git/gogit/clone.go +++ b/git/gogit/clone.go @@ -34,7 +34,6 @@ import ( "github.com/fluxcd/go-git/v5/storage/memory" "github.com/fluxcd/pkg/git" - "github.com/fluxcd/pkg/gitutil" "github.com/fluxcd/pkg/version" ) @@ -111,7 +110,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)) } } @@ -190,7 +189,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() @@ -235,7 +234,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() @@ -291,7 +290,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() @@ -453,3 +452,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 + } +} diff --git a/git/gogit/clone_test.go b/git/gogit/clone_test.go index 65cf4c4c..36bea3aa 100644 --- a/git/gogit/clone_test.go +++ b/git/gogit/clone_test.go @@ -1074,6 +1074,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()) diff --git a/git/gogit/go.mod b/git/gogit/go.mod index b530e97b..2a2f402b 100644 --- a/git/gogit/go.mod +++ b/git/gogit/go.mod @@ -5,7 +5,6 @@ go 1.18 replace ( github.com/fluxcd/pkg/git => ../../git github.com/fluxcd/pkg/gittestserver => ../../gittestserver - github.com/fluxcd/pkg/gitutil => ../../gitutil github.com/fluxcd/pkg/ssh => ../../ssh github.com/fluxcd/pkg/version => ../../version ) @@ -16,7 +15,6 @@ require ( github.com/fluxcd/go-git/v5 v5.0.0-20221104190732-329fd6659b10 github.com/fluxcd/pkg/git v0.6.1 github.com/fluxcd/pkg/gittestserver v0.8.0 - github.com/fluxcd/pkg/gitutil v0.2.0 github.com/fluxcd/pkg/ssh v0.7.0 github.com/fluxcd/pkg/version v0.2.0 github.com/go-git/go-billy/v5 v5.3.1 diff --git a/git/internal/e2e/go.mod b/git/internal/e2e/go.mod index 3247afd1..7206fbca 100644 --- a/git/internal/e2e/go.mod +++ b/git/internal/e2e/go.mod @@ -7,7 +7,6 @@ replace ( github.com/fluxcd/pkg/git/gogit => ../../gogit github.com/fluxcd/pkg/git/libgit2 => ../../libgit2 github.com/fluxcd/pkg/gittestserver => ../../../gittestserver - github.com/fluxcd/pkg/gitutil => ../../../gitutil github.com/fluxcd/pkg/http/transport => ../../../http/transport github.com/fluxcd/pkg/ssh => ../../../ssh github.com/fluxcd/pkg/version => ../../../version @@ -51,7 +50,6 @@ require ( github.com/emirpasic/gods v1.18.1 // indirect github.com/evanphx/json-patch/v5 v5.6.0 // indirect github.com/fluxcd/gitkit v0.6.0 // indirect - github.com/fluxcd/pkg/gitutil v0.2.0 // indirect github.com/fluxcd/pkg/http/transport v0.1.0 // indirect github.com/fluxcd/pkg/version v0.2.0 // indirect github.com/fsnotify/fsnotify v1.5.4 // indirect diff --git a/git/libgit2/client.go b/git/libgit2/client.go index 776a5e87..6e64ba3f 100644 --- a/git/libgit2/client.go +++ b/git/libgit2/client.go @@ -38,7 +38,6 @@ import ( "github.com/fluxcd/pkg/git" "github.com/fluxcd/pkg/git/libgit2/transport" "github.com/fluxcd/pkg/git/repository" - "github.com/fluxcd/pkg/gitutil" ) // ClientName is the string representation of Client. @@ -128,7 +127,7 @@ func (l *Client) Init(ctx context.Context, url, branch string) error { } repo, err := git2go.InitRepository(l.path, false) if err != nil { - return fmt.Errorf("unable to init repository for '%s': %w", url, gitutil.LibGit2Error(err)) + return fmt.Errorf("unable to init repository for '%s': %w", url, LibGit2Error(err)) } l.registerTransportOptions(ctx, url) @@ -167,7 +166,7 @@ func (l *Client) Init(ctx context.Context, url, branch string) error { } } else { repo.Free() - return fmt.Errorf("unable to create remote for '%s': %w", url, gitutil.LibGit2Error(err)) + return fmt.Errorf("unable to create remote for '%s': %w", url, LibGit2Error(err)) } } diff --git a/git/libgit2/clone.go b/git/libgit2/clone.go index 00be13d3..4362b486 100644 --- a/git/libgit2/clone.go +++ b/git/libgit2/clone.go @@ -27,7 +27,6 @@ import ( git2go "github.com/libgit2/git2go/v34" "github.com/fluxcd/pkg/git" - "github.com/fluxcd/pkg/gitutil" "github.com/fluxcd/pkg/version" ) @@ -43,7 +42,7 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C // Open remote connection. err = l.remote.ConnectFetch(&remoteCallBacks, nil, nil) if err != nil { - return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, LibGit2Error(err)) } defer l.remote.Disconnect() @@ -52,7 +51,7 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C if opts.LastObservedCommit != "" { heads, err := l.remote.Ls(branch) if err != nil { - return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, LibGit2Error(err)) } if len(heads) > 0 { hash := heads[0].Id.String() @@ -76,7 +75,7 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C }, "") if err != nil { - return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, LibGit2Error(err)) } isEmpty, err := l.repository.IsEmpty() @@ -89,13 +88,13 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C branchRef, err := l.repository.References.Lookup(fmt.Sprintf("refs/remotes/origin/%s", branch)) if err != nil { - return nil, fmt.Errorf("unable to lookup branch '%s' for '%s': %w", branch, url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to lookup branch '%s' for '%s': %w", branch, url, LibGit2Error(err)) } defer branchRef.Free() upstreamCommit, err := l.repository.LookupCommit(branchRef.Target()) if err != nil { - return nil, fmt.Errorf("unable to lookup commit '%s' for '%s': %w", branch, url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to lookup commit '%s' for '%s': %w", branch, url, LibGit2Error(err)) } defer upstreamCommit.Free() @@ -160,7 +159,7 @@ func (l *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp // Open remote connection. err = l.remote.ConnectFetch(&remoteCallBacks, nil, nil) if err != nil { - return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, LibGit2Error(err)) } defer l.remote.Disconnect() @@ -169,7 +168,7 @@ func (l *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp if opts.LastObservedCommit != "" { heads, err := l.remote.Ls(tag) if err != nil { - return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, LibGit2Error(err)) } if len(heads) > 0 { hash := heads[0].Id.String() @@ -203,7 +202,7 @@ func (l *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp "") if err != nil { - return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, LibGit2Error(err)) } cc, err := checkoutDetachedDwim(l.repository, tag) @@ -226,7 +225,7 @@ func (l *Client) cloneCommit(ctx context.Context, url, commit string, opts git.C }, }) if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to clone '%s': %w", url, LibGit2Error(err)) } l.repository = repo @@ -264,7 +263,7 @@ func (l *Client) cloneSemVer(ctx context.Context, url, semverTag string, opts gi }, }) if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to clone '%s': %w", url, LibGit2Error(err)) } l.repository = repo remote, err := repo.Remotes.Lookup(git.DefaultRemote) diff --git a/gitutil/errors.go b/git/libgit2/errors.go similarity index 68% rename from gitutil/errors.go rename to git/libgit2/errors.go index e7864972..9b1cf598 100644 --- a/gitutil/errors.go +++ b/git/libgit2/errors.go @@ -14,34 +14,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -package gitutil +package libgit2 import ( "errors" - "fmt" "strings" ) -// 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 - } -} - // LibGit2Error translates an error from the libgit2 library, or // returns `nil` if the argument is `nil`. func LibGit2Error(err error) error { diff --git a/gitutil/errors_test.go b/git/libgit2/errors_test.go similarity index 69% rename from gitutil/errors_test.go rename to git/libgit2/errors_test.go index 91a5468e..d36f22dc 100644 --- a/gitutil/errors_test.go +++ b/git/libgit2/errors_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package gitutil +package libgit2 import ( "errors" @@ -75,32 +75,6 @@ func TestLibgit2ErrorUnchanged(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_LibGit2Error(f *testing.F) { f.Add("") f.Add("single line error") @@ -115,18 +89,3 @@ func Fuzz_LibGit2Error(f *testing.F) { _ = LibGit2Error(err) }) } - -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) - }) -} diff --git a/git/libgit2/go.mod b/git/libgit2/go.mod index baa5b32f..ecc1d8f1 100644 --- a/git/libgit2/go.mod +++ b/git/libgit2/go.mod @@ -7,7 +7,6 @@ replace ( // Enables the use of pkg/git/gogit/fs. github.com/fluxcd/pkg/git/gogit => ../gogit github.com/fluxcd/pkg/gittestserver => ../../gittestserver - github.com/fluxcd/pkg/gitutil => ../../gitutil github.com/fluxcd/pkg/http/transport => ../../http/transport github.com/fluxcd/pkg/ssh => ../../ssh github.com/fluxcd/pkg/version => ../../version @@ -35,7 +34,6 @@ require ( github.com/fluxcd/gitkit v0.6.0 github.com/fluxcd/pkg/git v0.6.1 github.com/fluxcd/pkg/gittestserver v0.8.0 - github.com/fluxcd/pkg/gitutil v0.2.0 github.com/fluxcd/pkg/http/transport v0.1.0 github.com/fluxcd/pkg/ssh v0.7.0 github.com/fluxcd/pkg/version v0.2.0 diff --git a/gitutil/go.mod b/gitutil/go.mod deleted file mode 100644 index fcd5fdd0..00000000 --- a/gitutil/go.mod +++ /dev/null @@ -1,3 +0,0 @@ -module github.com/fluxcd/pkg/gitutil - -go 1.18 From 13379749d7a9e9ed797c95a3dcd4702d987b6d3c Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 15 Nov 2022 14:14:04 +0000 Subject: [PATCH 4/4] git: Move repository options to git/repository Signed-off-by: Paulo Gomes --- git/gogit/client.go | 6 +- git/gogit/client_test.go | 3 +- git/gogit/clone.go | 9 +-- git/gogit/clone_test.go | 31 ++++++----- git/internal/e2e/utils.go | 16 +++--- git/libgit2/client.go | 10 ++-- git/libgit2/client_test.go | 3 +- git/libgit2/clone.go | 29 +++++----- git/libgit2/clone_ssh_test.go | 13 +++-- git/libgit2/clone_test.go | 23 ++++---- git/libgit2/errors.go | 4 +- git/libgit2/errors_test.go | 8 +-- git/options.go | 75 ------------------------- git/repository/client.go | 4 +- git/repository/options.go | 101 ++++++++++++++++++++++++++++++++++ tests/fuzz/oss_fuzz_build.sh | 6 ++ 16 files changed, 190 insertions(+), 151 deletions(-) create mode 100644 git/repository/options.go diff --git a/git/gogit/client.go b/git/gogit/client.go index bfae32b9..0567a9d5 100644 --- a/git/gogit/client.go +++ b/git/gogit/client.go @@ -185,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 } @@ -245,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) } diff --git a/git/gogit/client_test.go b/git/gogit/client_test.go index 3bdd8e24..6ad54c39 100644 --- a/git/gogit/client_test.go +++ b/git/gogit/client_test.go @@ -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" ) @@ -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"), }), ) diff --git a/git/gogit/clone.go b/git/gogit/clone.go index 63038e49..dad33455 100644 --- a/git/gogit/clone.go +++ b/git/gogit/clone.go @@ -34,10 +34,11 @@ import ( "github.com/fluxcd/go-git/v5/storage/memory" "github.com/fluxcd/pkg/git" + "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") } @@ -126,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") } @@ -204,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) @@ -256,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) diff --git a/git/gogit/clone_test.go b/git/gogit/clone_test.go index 36bea3aa..0c698d2d 100644 --- a/git/gogit/clone_test.go +++ b/git/gogit/clone_test.go @@ -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" ) @@ -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, @@ -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, @@ -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, }, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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()) diff --git a/git/internal/e2e/utils.go b/git/internal/e2e/utils.go index 59092e8c..df49f2ea 100644 --- a/git/internal/e2e/utils.go +++ b/git/internal/e2e/utils.go @@ -45,8 +45,8 @@ var letterRunes = []rune("abcdefghijklmnopqrstuvwxyz1234567890") func testUsingClone(g *WithT, client repository.Client, repoURL *url.URL, upstreamRepo upstreamRepoInfo) { // clone repo - _, err := client.Clone(context.TODO(), repoURL.String(), git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + _, err := client.Clone(context.TODO(), repoURL.String(), repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ Branch: "main", }, }) @@ -55,7 +55,7 @@ func testUsingClone(g *WithT, client repository.Client, repoURL *url.URL, upstre // commit and push to origin cc, err := client.Commit( mockCommitInfo(), - git.WithFiles(map[string]io.Reader{ + repository.WithFiles(map[string]io.Reader{ "test": strings.NewReader(randStringRunes(10)), }), ) @@ -75,7 +75,7 @@ func testUsingClone(g *WithT, client repository.Client, repoURL *url.URL, upstre // commit to and push new branch cc, err = client.Commit( mockCommitInfo(), - git.WithFiles(map[string]io.Reader{ + repository.WithFiles(map[string]io.Reader{ "test": strings.NewReader(randStringRunes(10)), }), ) @@ -94,7 +94,7 @@ func testUsingClone(g *WithT, client repository.Client, repoURL *url.URL, upstre _, err = client.Commit( mockCommitInfo(), - git.WithFiles(map[string]io.Reader{ + repository.WithFiles(map[string]io.Reader{ "test": strings.NewReader(randStringRunes(10)), }), ) @@ -113,7 +113,7 @@ func testUsingInit(g *WithT, client repository.Client, repoURL *url.URL, upstrea cc, err := client.Commit( mockCommitInfo(), - git.WithFiles(map[string]io.Reader{ + repository.WithFiles(map[string]io.Reader{ "test": strings.NewReader(randStringRunes(10)), }), ) @@ -131,7 +131,7 @@ func testUsingInit(g *WithT, client repository.Client, repoURL *url.URL, upstrea cc, err = client.Commit( mockCommitInfo(), - git.WithFiles(map[string]io.Reader{ + repository.WithFiles(map[string]io.Reader{ "test": strings.NewReader(randStringRunes(10)), }), ) @@ -149,7 +149,7 @@ func testUsingInit(g *WithT, client repository.Client, repoURL *url.URL, upstrea _, err = client.Commit( mockCommitInfo(), - git.WithFiles(map[string]io.Reader{ + repository.WithFiles(map[string]io.Reader{ "test": strings.NewReader(randStringRunes(10)), }), ) diff --git a/git/libgit2/client.go b/git/libgit2/client.go index 6e64ba3f..52cf7bbf 100644 --- a/git/libgit2/client.go +++ b/git/libgit2/client.go @@ -127,7 +127,7 @@ func (l *Client) Init(ctx context.Context, url, branch string) error { } repo, err := git2go.InitRepository(l.path, false) if err != nil { - return fmt.Errorf("unable to init repository for '%s': %w", url, LibGit2Error(err)) + return fmt.Errorf("unable to init repository for '%s': %w", url, libGit2Error(err)) } l.registerTransportOptions(ctx, url) @@ -166,7 +166,7 @@ func (l *Client) Init(ctx context.Context, url, branch string) error { } } else { repo.Free() - return fmt.Errorf("unable to create remote for '%s': %w", url, LibGit2Error(err)) + return fmt.Errorf("unable to create remote for '%s': %w", url, libGit2Error(err)) } } @@ -175,7 +175,7 @@ func (l *Client) Init(ctx context.Context, url, branch string) error { return nil } -func (l *Client) Clone(ctx context.Context, url string, cloneOpts git.CloneOptions) (*git.Commit, error) { +func (l *Client) Clone(ctx context.Context, url string, cloneOpts repository.CloneOptions) (*git.Commit, error) { if err := l.validateUrl(url); err != nil { return nil, err } @@ -238,12 +238,12 @@ func (l *Client) writeFile(path string, reader io.Reader) error { return nil } -func (l *Client) Commit(info git.Commit, commitOpts ...git.CommitOption) (string, error) { +func (l *Client) Commit(info git.Commit, commitOpts ...repository.CommitOption) (string, error) { if l.repository == nil { return "", git.ErrNoGitRepository } - options := &git.CommitOptions{} + options := &repository.CommitOptions{} for _, o := range commitOpts { o(options) } diff --git a/git/libgit2/client_test.go b/git/libgit2/client_test.go index bc4aa665..a0705205 100644 --- a/git/libgit2/client_test.go +++ b/git/libgit2/client_test.go @@ -32,6 +32,7 @@ import ( "github.com/fluxcd/pkg/git" "github.com/fluxcd/pkg/git/libgit2/internal/test" "github.com/fluxcd/pkg/git/libgit2/transport" + "github.com/fluxcd/pkg/git/repository" "github.com/fluxcd/pkg/gittestserver" ) @@ -170,7 +171,7 @@ func TestCommit(t *testing.T) { }, Message: "testing", }, - git.WithFiles(map[string]io.Reader{ + repository.WithFiles(map[string]io.Reader{ "test": strings.NewReader("testing libgit2 commit"), }), ) diff --git a/git/libgit2/clone.go b/git/libgit2/clone.go index 4362b486..30eb62a7 100644 --- a/git/libgit2/clone.go +++ b/git/libgit2/clone.go @@ -27,10 +27,11 @@ import ( git2go "github.com/libgit2/git2go/v34" "github.com/fluxcd/pkg/git" + "github.com/fluxcd/pkg/git/repository" "github.com/fluxcd/pkg/version" ) -func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.CloneOptions) (_ *git.Commit, err error) { +func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts repository.CloneOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) err = l.Init(ctx, url, branch) @@ -42,7 +43,7 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C // Open remote connection. err = l.remote.ConnectFetch(&remoteCallBacks, nil, nil) if err != nil { - return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, libGit2Error(err)) } defer l.remote.Disconnect() @@ -51,7 +52,7 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C if opts.LastObservedCommit != "" { heads, err := l.remote.Ls(branch) if err != nil { - return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, libGit2Error(err)) } if len(heads) > 0 { hash := heads[0].Id.String() @@ -75,7 +76,7 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C }, "") if err != nil { - return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, libGit2Error(err)) } isEmpty, err := l.repository.IsEmpty() @@ -88,13 +89,13 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C branchRef, err := l.repository.References.Lookup(fmt.Sprintf("refs/remotes/origin/%s", branch)) if err != nil { - return nil, fmt.Errorf("unable to lookup branch '%s' for '%s': %w", branch, url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to lookup branch '%s' for '%s': %w", branch, url, libGit2Error(err)) } defer branchRef.Free() upstreamCommit, err := l.repository.LookupCommit(branchRef.Target()) if err != nil { - return nil, fmt.Errorf("unable to lookup commit '%s' for '%s': %w", branch, url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to lookup commit '%s' for '%s': %w", branch, url, libGit2Error(err)) } defer upstreamCommit.Free() @@ -148,7 +149,7 @@ func (l *Client) cloneBranch(ctx context.Context, url, branch string, opts git.C return buildCommit(cc, "refs/heads/"+branch), nil } -func (l *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOptions) (_ *git.Commit, err error) { +func (l *Client) cloneTag(ctx context.Context, url, tag string, opts repository.CloneOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) remoteCallBacks := RemoteCallbacks() @@ -159,7 +160,7 @@ func (l *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp // Open remote connection. err = l.remote.ConnectFetch(&remoteCallBacks, nil, nil) if err != nil { - return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, libGit2Error(err)) } defer l.remote.Disconnect() @@ -168,7 +169,7 @@ func (l *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp if opts.LastObservedCommit != "" { heads, err := l.remote.Ls(tag) if err != nil { - return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, libGit2Error(err)) } if len(heads) > 0 { hash := heads[0].Id.String() @@ -202,7 +203,7 @@ func (l *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp "") if err != nil { - return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, libGit2Error(err)) } cc, err := checkoutDetachedDwim(l.repository, tag) @@ -213,7 +214,7 @@ func (l *Client) cloneTag(ctx context.Context, url, tag string, opts git.CloneOp return buildCommit(cc, "refs/tags/"+tag), nil } -func (l *Client) cloneCommit(ctx context.Context, url, commit string, opts git.CloneOptions) (_ *git.Commit, err error) { +func (l *Client) cloneCommit(ctx context.Context, url, commit string, opts repository.CloneOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) l.registerTransportOptions(ctx, url) @@ -225,7 +226,7 @@ func (l *Client) cloneCommit(ctx context.Context, url, commit string, opts git.C }, }) if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to clone '%s': %w", url, libGit2Error(err)) } l.repository = repo @@ -246,7 +247,7 @@ func (l *Client) cloneCommit(ctx context.Context, url, commit string, opts git.C return buildCommit(cc, ""), nil } -func (l *Client) cloneSemVer(ctx context.Context, url, semverTag string, opts git.CloneOptions) (_ *git.Commit, err error) { +func (l *Client) cloneSemVer(ctx context.Context, url, semverTag string, opts repository.CloneOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) verConstraint, err := semver.NewConstraint(semverTag) @@ -263,7 +264,7 @@ func (l *Client) cloneSemVer(ctx context.Context, url, semverTag string, opts gi }, }) if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", url, LibGit2Error(err)) + return nil, fmt.Errorf("unable to clone '%s': %w", url, libGit2Error(err)) } l.repository = repo remote, err := repo.Remotes.Lookup(git.DefaultRemote) diff --git a/git/libgit2/clone_ssh_test.go b/git/libgit2/clone_ssh_test.go index ed306fd1..db017fc0 100644 --- a/git/libgit2/clone_ssh_test.go +++ b/git/libgit2/clone_ssh_test.go @@ -27,6 +27,7 @@ import ( "time" "github.com/fluxcd/gitkit" + "github.com/fluxcd/pkg/git/repository" "github.com/fluxcd/pkg/gittestserver" "github.com/fluxcd/pkg/ssh" @@ -148,8 +149,8 @@ func Test_ssh_keyTypes(t *testing.T) { defer cancel() // Checkout the repo. - commit, err := lgc.Clone(ctx, repoURL, git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + commit, err := lgc.Clone(ctx, repoURL, repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ Branch: git.DefaultBranch, }, }) @@ -281,8 +282,8 @@ func Test_ssh_keyExchangeAlgos(t *testing.T) { defer cancel() // Checkout the repo. - _, err = lgc.Clone(ctx, repoURL, git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + _, err = lgc.Clone(ctx, repoURL, repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ Branch: git.DefaultBranch, }, }) @@ -454,8 +455,8 @@ func Test_ssh_hostKeyAlgos(t *testing.T) { defer cancel() // Checkout the repo. - _, err = lgc.Clone(ctx, repoURL, git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + _, err = lgc.Clone(ctx, repoURL, repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ Branch: git.DefaultBranch, }, }) diff --git a/git/libgit2/clone_test.go b/git/libgit2/clone_test.go index b4965bb7..4d1d263a 100644 --- a/git/libgit2/clone_test.go +++ b/git/libgit2/clone_test.go @@ -33,6 +33,7 @@ import ( "github.com/fluxcd/pkg/git" "github.com/fluxcd/pkg/git/libgit2/internal/test" + "github.com/fluxcd/pkg/git/repository" "github.com/fluxcd/pkg/gittestserver" ) @@ -138,8 +139,8 @@ func TestClone_cloneBranch(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) defer lgc.Close() - cc, err := lgc.Clone(context.TODO(), repoURL, git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + cc, err := lgc.Clone(context.TODO(), repoURL, repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ Branch: tt.branch, }, LastObservedCommit: tt.lastRevision, @@ -263,8 +264,8 @@ func TestClone_cloneTag(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) defer lgc.Close() - cloneOpts := git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + cloneOpts := repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ Tag: tt.checkoutTag, }, } @@ -340,8 +341,8 @@ func TestClone_cloneCommit(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) defer lgc.Close() - cc, err := lgc.Clone(context.TODO(), repoURL, git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + cc, err := lgc.Clone(context.TODO(), repoURL, repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ Commit: c.String(), }, }) @@ -357,8 +358,8 @@ func TestClone_cloneCommit(t *testing.T) { }) g.Expect(err).ToNot(HaveOccurred()) - cc, err = lgc.Clone(context.TODO(), repoURL, git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + cc, err = lgc.Clone(context.TODO(), repoURL, repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ Commit: "4dc3185c5fc94eb75048376edeb44571cece25f4", }, }) @@ -487,8 +488,8 @@ func TestClone_cloneSemVer(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) defer lgc.Close() - cc, err := lgc.Clone(context.TODO(), repoURL, git.CloneOptions{ - CheckoutStrategy: git.CheckoutStrategy{ + cc, err := lgc.Clone(context.TODO(), repoURL, repository.CloneOptions{ + CheckoutStrategy: repository.CheckoutStrategy{ SemVer: tt.constraint, }, }) @@ -665,7 +666,7 @@ func TestClone_CredentialsOverHttp(t *testing.T) { repoURL = tt.transformURL(ts.URL) } - _, err = lgc.Clone(context.TODO(), repoURL, git.CloneOptions{}) + _, err = lgc.Clone(context.TODO(), repoURL, repository.CloneOptions{}) if tt.expectCloneErr != "" { g.Expect(err).To(HaveOccurred()) diff --git a/git/libgit2/errors.go b/git/libgit2/errors.go index 9b1cf598..5aba47f9 100644 --- a/git/libgit2/errors.go +++ b/git/libgit2/errors.go @@ -21,9 +21,9 @@ import ( "strings" ) -// LibGit2Error translates an error from the libgit2 library, or +// libGit2Error translates an error from the libgit2 library, or // returns `nil` if the argument is `nil`. -func LibGit2Error(err error) error { +func libGit2Error(err error) error { if err == nil { return err } diff --git a/git/libgit2/errors_test.go b/git/libgit2/errors_test.go index d36f22dc..ddb05396 100644 --- a/git/libgit2/errors_test.go +++ b/git/libgit2/errors_test.go @@ -34,7 +34,7 @@ remote: expectedReformat := "remote: This deploy key does not have write access to this project." err := errors.New(gitlabMessage) - err = LibGit2Error(err) + err = libGit2Error(err) reformattedMessage := err.Error() if reformattedMessage != expectedReformat { t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage) @@ -56,7 +56,7 @@ remote: expectedReformat := "remote: This deploy key does not have write access to this project. You will need to create a new deploy key." err := errors.New(multilineMessage) - err = LibGit2Error(err) + err = libGit2Error(err) reformattedMessage := err.Error() if reformattedMessage != expectedReformat { t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage) @@ -68,7 +68,7 @@ func TestLibgit2ErrorUnchanged(t *testing.T) { regularMessage := `remote: ERROR: deploy key does not have permissions` expectedReformat := regularMessage err := errors.New(regularMessage) - err = LibGit2Error(err) + err = libGit2Error(err) reformattedMessage := err.Error() if reformattedMessage != expectedReformat { t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage) @@ -86,6 +86,6 @@ func Fuzz_LibGit2Error(f *testing.F) { err = errors.New(msg) } - _ = LibGit2Error(err) + _ = libGit2Error(err) }) } diff --git a/git/options.go b/git/options.go index c7035dea..85b9ca39 100644 --- a/git/options.go +++ b/git/options.go @@ -18,10 +18,7 @@ package git import ( "fmt" - "io" "net/url" - - "github.com/ProtonMail/go-crypto/openpgp" ) const ( @@ -30,78 +27,6 @@ const ( DefaultPublicKeyAuthUser = "git" ) -// CloneOptions are the options used for a Git clone. -type CloneOptions struct { - // CheckoutStrategy defines a strategy to use while checking out - // the cloned repository to a specific target. - CheckoutStrategy - - // RecurseSubmodules defines if submodules should be checked out, - // not supported by all Implementations. - RecurseSubmodules bool - - // LastObservedCommit holds the last observed commit hash of a - // Git repository. - // If provided, the clone operation will compare it with the HEAD commit - // of the branch or tag (as configured via CheckoutStrategy) in the remote - // repository. If they match, cloning will be skipped and a "non-concrete" - // commit will be returned, which can be verified using `IsConcreteCommit()`. - // This functionality is not supported when using a semver range or a commit - // to checkout. - LastObservedCommit string - - // ShallowClone defines if the repository should be shallow cloned, - // not supported by all implementations - ShallowClone bool -} - -// CheckoutStrategy provides options to checkout a repository to a target. -type CheckoutStrategy struct { - // Branch to checkout. If supported by the client, it can be combined - // with Commit. - Branch string - - // Tag to checkout, takes precedence over Branch. - Tag string - - // SemVer tag expression to checkout, takes precedence over Tag. - SemVer string `json:"semver,omitempty"` - - // Commit SHA1 to checkout, takes precedence over Tag and SemVer. - // If supported by the client, it can be combined with Branch. - Commit string -} - -// CommitOptions provides options to configure a Git commit operation. -type CommitOptions struct { - // Signer can be used to sign a commit using OpenPGP. - Signer *openpgp.Entity - // Files contains file names mapped to the file's content. - // Its used to write files which are then included in the commit. - Files map[string]io.Reader -} - -// CommitOption defines an option for a commit operation. -type CommitOption func(*CommitOptions) - -// WithSigner allows for the commit to be signed using the provided -// OpenPGP signer. -func WithSigner(signer *openpgp.Entity) CommitOption { - return func(co *CommitOptions) { - co.Signer = signer - } -} - -// WithFiles instructs the Git client to write the provided files and include -// them in the commit. -// files contains file names as its key and the content of the file as the -// value. If the file already exists, its overwritten. -func WithFiles(files map[string]io.Reader) CommitOption { - return func(co *CommitOptions) { - co.Files = files - } -} - type TransportType string const ( diff --git a/git/repository/client.go b/git/repository/client.go index 4f33380f..1485fe2a 100644 --- a/git/repository/client.go +++ b/git/repository/client.go @@ -28,7 +28,7 @@ type Reader interface { // Clone clones a repository from the provided url using the options provided. // It returns a Commit object describing the Git commit that the repository // HEAD points to. If the repository is empty, it returns a nil Commit. - Clone(ctx context.Context, url string, cloneOpts git.CloneOptions) (*git.Commit, error) + Clone(ctx context.Context, url string, cloneOpts CloneOptions) (*git.Commit, error) // IsClean returns whether the working tree is clean. IsClean() (bool, error) // Head returns the hash of the current HEAD of the repo. @@ -51,7 +51,7 @@ type Writer interface { SwitchBranch(ctx context.Context, branch string) error // Commit commits any changes made to the repository. commitOpts is an // optional argument which can be provided to configure the commit. - Commit(info git.Commit, commitOpts ...git.CommitOption) (string, error) + Commit(info git.Commit, commitOpts ...CommitOption) (string, error) Closer } diff --git a/git/repository/options.go b/git/repository/options.go new file mode 100644 index 00000000..740f43c9 --- /dev/null +++ b/git/repository/options.go @@ -0,0 +1,101 @@ +/* +Copyright 2022 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package repository + +import ( + "io" + + "github.com/ProtonMail/go-crypto/openpgp" +) + +const ( + DefaultRemote = "origin" + DefaultBranch = "master" + DefaultPublicKeyAuthUser = "git" +) + +// CloneOptions are the options used for a Git clone. +type CloneOptions struct { + // CheckoutStrategy defines a strategy to use while checking out + // the cloned repository to a specific target. + CheckoutStrategy + + // RecurseSubmodules defines if submodules should be checked out, + // not supported by all Implementations. + RecurseSubmodules bool + + // LastObservedCommit holds the last observed commit hash of a + // Git repository. + // If provided, the clone operation will compare it with the HEAD commit + // of the branch or tag (as configured via CheckoutStrategy) in the remote + // repository. If they match, cloning will be skipped and a "non-concrete" + // commit will be returned, which can be verified using `IsConcreteCommit()`. + // This functionality is not supported when using a semver range or a commit + // to checkout. + LastObservedCommit string + + // ShallowClone defines if the repository should be shallow cloned, + // not supported by all implementations + ShallowClone bool +} + +// CheckoutStrategy provides options to checkout a repository to a target. +type CheckoutStrategy struct { + // Branch to checkout. If supported by the client, it can be combined + // with Commit. + Branch string + + // Tag to checkout, takes precedence over Branch. + Tag string + + // SemVer tag expression to checkout, takes precedence over Tag. + SemVer string `json:"semver,omitempty"` + + // Commit SHA1 to checkout, takes precedence over Tag and SemVer. + // If supported by the client, it can be combined with Branch. + Commit string +} + +// CommitOptions provides options to configure a Git commit operation. +type CommitOptions struct { + // Signer can be used to sign a commit using OpenPGP. + Signer *openpgp.Entity + // Files contains file names mapped to the file's content. + // Its used to write files which are then included in the commit. + Files map[string]io.Reader +} + +// CommitOption defines an option for a commit operation. +type CommitOption func(*CommitOptions) + +// WithSigner allows for the commit to be signed using the provided +// OpenPGP signer. +func WithSigner(signer *openpgp.Entity) CommitOption { + return func(co *CommitOptions) { + co.Signer = signer + } +} + +// WithFiles instructs the Git client to write the provided files and include +// them in the commit. +// files contains file names as its key and the content of the file as the +// value. If the file already exists, its overwritten. +func WithFiles(files map[string]io.Reader) CommitOption { + return func(co *CommitOptions) { + co.Files = files + } +} diff --git a/tests/fuzz/oss_fuzz_build.sh b/tests/fuzz/oss_fuzz_build.sh index 4e44867c..dad4be33 100755 --- a/tests/fuzz/oss_fuzz_build.sh +++ b/tests/fuzz/oss_fuzz_build.sh @@ -76,6 +76,12 @@ for module in ${modules}; do # Iterate through all Go Fuzz targets, compiling each into a fuzzer. for file in ${test_files}; do + # If the subdir is a module, skip this file, as it will be handled + # at the next iteration of the outer loop. + if [ -f "$(dirname "${file}")/go.mod" ]; then + continue + fi + remove_test_funcs "${file}" targets=$(grep -oP 'func \K(Fuzz\w*)' "${file}")