From ae2b260688b183a6320fc03709b3792e7dbb4874 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Wed, 1 Dec 2021 04:46:50 -0800 Subject: [PATCH] Provide arguments to NewGitServer and NewGitProtocol as Opts structs There are now way too many arguments for these functions, so let's do a small API-breaking change to make it easier to extend. #major --- README.md | 17 +++-- browser_test.go | 112 +++++++++-------------------- go.mod | 2 +- protocol.go | 55 ++++++++------- protocol_test.go | 178 +++++++++++++---------------------------------- server.go | 42 ++++++----- server_test.go | 72 ++++++++++--------- 7 files changed, 189 insertions(+), 289 deletions(-) diff --git a/README.md b/README.md index 29fb911..9d2033a 100644 --- a/README.md +++ b/README.md @@ -12,15 +12,18 @@ the `git_repositories/` directory: package main import ( - "github.com/inconshreveable/log15" - "github.com/omegaup/githttp" -        "net/http" + "net/http" + + "github.com/omegaup/githttp" ) func main() { - panic(http.Server{ - Addr: ":80", -                Handler: githttp.GitServer("git_repositories", true, nil, nil, nil, log15.New()), - }.ListenAndServe()) + panic(http.Server{ + Addr: ":80", + Handler: githttp.NewGitServer(githttp.GitServerOpts{ + RootPath: "git_repositories", + EnableBrowse: true, + }), + }.ListenAndServe()) } ``` diff --git a/browser_test.go b/browser_test.go index 450e2a2..5ade5db 100644 --- a/browser_test.go +++ b/browser_test.go @@ -15,14 +15,9 @@ import ( func TestHandleRefs(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ) + protocol := NewGitProtocol(GitProtocolOpts{ + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -60,20 +55,16 @@ func TestHandleRefs(t *testing.T) { func TestHandleRefsWithReferenceDiscoveryCallback(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - func( + protocol := NewGitProtocol(GitProtocolOpts{ + ReferenceDiscoveryCallback: func( ctx context.Context, repository *git.Repository, referenceName string, ) bool { return referenceName == "refs/heads/public" }, - nil, - nil, - false, - log, - ) + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -100,14 +91,9 @@ func TestHandleRefsWithReferenceDiscoveryCallback(t *testing.T) { func TestHandleRestrictedRefs(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ) + protocol := NewGitProtocol(GitProtocolOpts{ + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -142,14 +128,9 @@ func TestHandleRestrictedRefs(t *testing.T) { func TestHandleArchiveCommit(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ) + protocol := NewGitProtocol(GitProtocolOpts{ + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -184,14 +165,9 @@ func TestHandleArchiveCommit(t *testing.T) { func TestHandleLog(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ) + protocol := NewGitProtocol(GitProtocolOpts{ + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -254,14 +230,9 @@ func TestHandleLog(t *testing.T) { func TestHandleLogCommit(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ) + protocol := NewGitProtocol(GitProtocolOpts{ + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -308,14 +279,9 @@ func TestHandleLogCommit(t *testing.T) { func TestHandleShowCommit(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ) + protocol := NewGitProtocol(GitProtocolOpts{ + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -359,14 +325,9 @@ func TestHandleShowCommit(t *testing.T) { func TestHandleShowTree(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ) + protocol := NewGitProtocol(GitProtocolOpts{ + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -414,14 +375,9 @@ func TestHandleShowTree(t *testing.T) { func TestHandleShowBlob(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ) + protocol := NewGitProtocol(GitProtocolOpts{ + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { @@ -461,20 +417,16 @@ func TestHandleShowBlob(t *testing.T) { func TestHandleNotFound(t *testing.T) { log := base.StderrLog(false) - protocol := NewGitProtocol( - nil, - func( + protocol := NewGitProtocol(GitProtocolOpts{ + ReferenceDiscoveryCallback: func( ctx context.Context, repository *git.Repository, referenceName string, ) bool { return referenceName == "refs/heads/public" }, - nil, - nil, - false, - log, - ) + Log: log, + }) repository, err := git.OpenRepository("testdata/repo.git") if err != nil { diff --git a/go.mod b/go.mod index 53a0013..bd5355a 100644 --- a/go.mod +++ b/go.mod @@ -1,4 +1,4 @@ -module github.com/omegaup/githttp +module github.com/omegaup/githttp/v2 go 1.17 diff --git a/protocol.go b/protocol.go index bb09e36..11a99b8 100644 --- a/protocol.go +++ b/protocol.go @@ -125,38 +125,43 @@ type GitProtocol struct { log log15.Logger } +// GitProtocolOpts contains all the possible options to initialize the git Server. +type GitProtocolOpts struct { + doNotCompare + + AuthCallback AuthorizationCallback + ReferenceDiscoveryCallback ReferenceDiscoveryCallback + UpdateCallback UpdateCallback + PreprocessCallback PreprocessCallback + AllowNonFastForward bool + Log log15.Logger +} + // NewGitProtocol returns a new instance of GitProtocol. -func NewGitProtocol( - authCallback AuthorizationCallback, - referenceDiscoveryCallback ReferenceDiscoveryCallback, - updateCallback UpdateCallback, - preprocessCallback PreprocessCallback, - allowNonFastForward bool, - log log15.Logger, -) *GitProtocol { - if authCallback == nil { - authCallback = noopAuthorizationCallback +func NewGitProtocol(opts GitProtocolOpts) *GitProtocol { + if opts.AuthCallback == nil { + opts.AuthCallback = noopAuthorizationCallback } - - if referenceDiscoveryCallback == nil { - referenceDiscoveryCallback = noopReferenceDiscoveryCallback + if opts.ReferenceDiscoveryCallback == nil { + opts.ReferenceDiscoveryCallback = noopReferenceDiscoveryCallback } - - if updateCallback == nil { - updateCallback = noopUpdateCallback + if opts.UpdateCallback == nil { + opts.UpdateCallback = noopUpdateCallback } - - if preprocessCallback == nil { - preprocessCallback = noopPreprocessCallback + if opts.PreprocessCallback == nil { + opts.PreprocessCallback = noopPreprocessCallback + } + if opts.Log == nil { + opts.Log = log15.New() } return &GitProtocol{ - AuthCallback: authCallback, - ReferenceDiscoveryCallback: referenceDiscoveryCallback, - UpdateCallback: updateCallback, - PreprocessCallback: preprocessCallback, - AllowNonFastForward: allowNonFastForward, - log: log, + AuthCallback: opts.AuthCallback, + ReferenceDiscoveryCallback: opts.ReferenceDiscoveryCallback, + UpdateCallback: opts.UpdateCallback, + PreprocessCallback: opts.PreprocessCallback, + AllowNonFastForward: opts.AllowNonFastForward, + log: opts.Log, } } diff --git a/protocol_test.go b/protocol_test.go index e152abc..52a4914 100644 --- a/protocol_test.go +++ b/protocol_test.go @@ -64,14 +64,9 @@ func TestHandlePrePullRestricted(t *testing.T) { context.Background(), "testdata/repo.git", AuthorizationAllowedRestricted, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &buf, ) @@ -102,14 +97,9 @@ func TestHandlePrePull(t *testing.T) { context.Background(), "testdata/repo.git", AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &buf, ) @@ -141,14 +131,9 @@ func TestHandlePrePush(t *testing.T) { context.Background(), "testdata/repo.git", AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &buf, ) @@ -179,14 +164,9 @@ func TestHandleEmptyPrePull(t *testing.T) { context.Background(), "testdata/empty.git", AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &buf, ) @@ -214,14 +194,9 @@ func TestHandleEmptyPrePush(t *testing.T) { context.Background(), "testdata/empty.git", AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &buf, ) @@ -621,14 +596,9 @@ func TestHandlePushUnborn(t *testing.T) { context.Background(), dir, AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &inBuf, &outBuf, @@ -654,14 +624,9 @@ func TestHandlePushUnborn(t *testing.T) { context.Background(), dir, AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &buf, ) @@ -724,11 +689,8 @@ func TestHandlePushPreprocess(t *testing.T) { context.Background(), dir, AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - func( + NewGitProtocol(GitProtocolOpts{ + PreprocessCallback: func( ctx context.Context, originalRepository *git.Repository, tmpDir string, @@ -791,9 +753,8 @@ func TestHandlePushPreprocess(t *testing.T) { return newPackPath, newCommands, nil }, - false, - log, - ), + Log: log, + }), log, &inBuf, &outBuf, @@ -819,14 +780,9 @@ func TestHandlePushPreprocess(t *testing.T) { context.Background(), dir, AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &buf, ) @@ -889,10 +845,8 @@ func TestHandlePushCallback(t *testing.T) { context.Background(), dir, AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - func( + NewGitProtocol(GitProtocolOpts{ + UpdateCallback: func( ctx context.Context, repository *git.Repository, level AuthorizationLevel, @@ -901,10 +855,8 @@ func TestHandlePushCallback(t *testing.T) { ) error { return errors.New("go away") }, - nil, - false, - log, - ), + Log: log, + }), log, &inBuf, &outBuf, @@ -963,14 +915,9 @@ func TestHandlePushUnknownCommit(t *testing.T) { context.Background(), dir, AuthorizationAllowed, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &inBuf, &outBuf, @@ -1029,14 +976,9 @@ func TestHandlePushRestrictedRef(t *testing.T) { context.Background(), dir, AuthorizationAllowedRestricted, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &inBuf, &outBuf, @@ -1095,14 +1037,9 @@ func TestHandlePushMerge(t *testing.T) { context.Background(), dir, AuthorizationAllowedRestricted, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &inBuf, &outBuf, @@ -1161,14 +1098,9 @@ func TestHandlePushMultipleCommits(t *testing.T) { context.Background(), dir, AuthorizationAllowedRestricted, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &inBuf, &outBuf, @@ -1227,14 +1159,9 @@ func TestHandleNonFastForward(t *testing.T) { context.Background(), dir, AuthorizationAllowedRestricted, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &inBuf, &outBuf, @@ -1276,14 +1203,9 @@ func TestHandleNonFastForward(t *testing.T) { context.Background(), dir, AuthorizationAllowedRestricted, - NewGitProtocol( - nil, - nil, - nil, - nil, - false, - log, - ), + NewGitProtocol(GitProtocolOpts{ + Log: log, + }), log, &inBuf, &outBuf, diff --git a/server.go b/server.go index f10256b..ca0818c 100644 --- a/server.go +++ b/server.go @@ -16,6 +16,8 @@ import ( git "github.com/libgit2/git2go/v33" ) +type doNotCompare [0]func() + // A GitOperation describes the current operation type GitOperation int @@ -493,29 +495,37 @@ func (h *gitHTTPHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { ) } +// GitServerOpts contains all the possible options to initialize the git Server. +type GitServerOpts struct { + doNotCompare + + RootPath string + RepositorySuffix string + EnableBrowse bool + Protocol *GitProtocol + ContextCallback ContextCallback + Log log15.Logger +} + // GitServer returns an http.Handler that implements git's smart protocol, // as documented on // https://git-scm.com/book/en/v2/Git-Internals-Transfer-Protocols#_the_smart_protocol . // The callbacks will be invoked as a way to allow callers to perform // additional authorization and pre-upload checks. -func GitServer( - rootPath string, - repositorySuffix string, - enableBrowse bool, - protocol *GitProtocol, - contextCallback ContextCallback, - log log15.Logger, -) http.Handler { - if contextCallback == nil { - contextCallback = noopContextCallback +func NewGitServer(opts GitServerOpts) http.Handler { + if opts.ContextCallback == nil { + opts.ContextCallback = noopContextCallback + } + if opts.Log == nil { + opts.Log = log15.New() } return &gitHTTPHandler{ - rootPath: rootPath, - repositorySuffix: repositorySuffix, - enableBrowse: enableBrowse, - contextCallback: contextCallback, - protocol: protocol, - log: log, + rootPath: opts.RootPath, + repositorySuffix: opts.RepositorySuffix, + enableBrowse: opts.EnableBrowse, + contextCallback: opts.ContextCallback, + protocol: opts.Protocol, + log: opts.Log, } } diff --git a/server_test.go b/server_test.go index df90310..788ac21 100644 --- a/server_test.go +++ b/server_test.go @@ -49,14 +49,16 @@ func TestServerClone(t *testing.T) { defer os.RemoveAll(dir) log := base.StderrLog(false) - handler := GitServer( - "testdata", - ".git", - true, - NewGitProtocol(allowAuthorizationCallback, nil, nil, nil, false, log), - nil, - log, - ) + handler := NewGitServer(GitServerOpts{ + RootPath: "testdata", + RepositorySuffix: ".git", + EnableBrowse: true, + Protocol: NewGitProtocol(GitProtocolOpts{ + AuthCallback: allowAuthorizationCallback, + Log: log, + }), + Log: log, + }) ts := httptest.NewServer(handler) defer ts.Close() @@ -91,14 +93,16 @@ func TestServerCloneShallow(t *testing.T) { defer os.RemoveAll(dir) log := base.StderrLog(false) - handler := GitServer( - "testdata", - ".git", - true, - NewGitProtocol(allowAuthorizationCallback, nil, nil, nil, false, log), - nil, - log, - ) + handler := NewGitServer(GitServerOpts{ + RootPath: "testdata", + RepositorySuffix: ".git", + EnableBrowse: true, + Protocol: NewGitProtocol(GitProtocolOpts{ + AuthCallback: allowAuthorizationCallback, + Log: log, + }), + Log: log, + }) ts := httptest.NewServer(handler) defer ts.Close() @@ -158,14 +162,16 @@ func TestServerPush(t *testing.T) { repo.Free() } - handler := GitServer( - dir, - ".git", - true, - NewGitProtocol(allowAuthorizationCallback, nil, nil, nil, false, log), - nil, - log, - ) + handler := NewGitServer(GitServerOpts{ + RootPath: dir, + RepositorySuffix: ".git", + EnableBrowse: true, + Protocol: NewGitProtocol(GitProtocolOpts{ + AuthCallback: allowAuthorizationCallback, + Log: log, + }), + Log: log, + }) ts := httptest.NewServer(handler) defer ts.Close() @@ -251,14 +257,16 @@ func TestGitbomb(t *testing.T) { repo.Free() } - handler := GitServer( - dir, - ".git", - true, - NewGitProtocol(allowAuthorizationCallback, nil, nil, nil, false, log), - nil, - log, - ) + handler := NewGitServer(GitServerOpts{ + RootPath: dir, + RepositorySuffix: ".git", + EnableBrowse: true, + Protocol: NewGitProtocol(GitProtocolOpts{ + AuthCallback: allowAuthorizationCallback, + Log: log, + }), + Log: log, + }) ts := httptest.NewServer(handler) defer ts.Close()