From 96d7fb9a16114fa6ffddcdeadd5b77f849ad5a91 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Sun, 18 Sep 2022 17:13:15 -0700 Subject: [PATCH] Revert "Introduce RepositoryHandle (#32)" (#34) It turns out that this wasn't quite ready to land because there are some spots in gitserver where raw repositories are obtained. This reverts commit 275ce63b4248729bfb4f3edf3ab39adfc7a170c0. --- browser.go | 156 ++++++++++++++++++++++--------- browser_test.go | 33 +++---- commits.go | 39 ++++---- commits_test.go | 3 - protocol.go | 119 ++++++++++++++++++------ protocol_test.go | 1 - repository_handle.go | 212 ------------------------------------------- 7 files changed, 237 insertions(+), 326 deletions(-) delete mode 100644 repository_handle.go diff --git a/browser.go b/browser.go index 43b42a5..edb6dcf 100644 --- a/browser.go +++ b/browser.go @@ -244,28 +244,50 @@ func formatBlob( // of the refs that are viewable by the requestor. func isCommitIDReachable( ctx context.Context, - handle *RepositoryHandle, + repository *git.Repository, level AuthorizationLevel, protocol *GitProtocol, commitID *git.Oid, ) error { - var oids []*git.Oid - references, err := handle.References() + it, err := repository.NewReferenceIterator() if err != nil { - return err + return errors.Wrap( + err, + "failed to create a reference iterator", + ) } - for _, ref := range references { - if level == AuthorizationAllowedRestricted && isRestrictedRef(ref.Name) { + defer it.Free() + + references := make(map[string]*git.Oid) + for { + ref, err := it.Next() + if err != nil { + if git.IsErrorCode(err, git.ErrorCodeIterOver) { + break + } + return errors.Wrap( + err, + "failed to get an entry from the reference iterator", + ) + } + defer ref.Free() + + references[ref.Name()] = ref.Target() + } + + var oids []*git.Oid + for name, target := range references { + if level == AuthorizationAllowedRestricted && isRestrictedRef(name) { continue } - if !protocol.ReferenceDiscoveryCallback(ctx, handle.Repository, ref.Name) { + if !protocol.ReferenceDiscoveryCallback(ctx, repository, name) { continue } - oids = append(oids, ref.Target) + oids = append(oids, target) } - reachable, err := handle.Repository.ReachableFromAny(commitID, oids) + reachable, err := repository.ReachableFromAny(commitID, oids) if err != nil { return base.ErrorWithCategory( ErrNotFound, @@ -295,40 +317,70 @@ func isCommitIDReachable( func handleRefs( ctx context.Context, - handle *RepositoryHandle, + repository *git.Repository, level AuthorizationLevel, protocol *GitProtocol, method string, ) (RefsResult, error) { - references, err := handle.References() + it, err := repository.NewReferenceIterator() if err != nil { - return nil, err + return nil, errors.Wrap( + err, + "failed to create a reference iterator", + ) } + defer it.Free() result := make(RefsResult) - head, err := handle.Repository.Head() + head, err := repository.Head() if err == nil { defer head.Free() } - for _, ref := range references { - if level == AuthorizationAllowedRestricted && isRestrictedRef(ref.Name) { + for { + ref, err := it.Next() + if err != nil { + if git.IsErrorCode(err, git.ErrorCodeIterOver) { + break + } + return nil, errors.Wrap( + err, + "failed to get an entry from the reference iterator", + ) + } + defer ref.Free() + + if level == AuthorizationAllowedRestricted && isRestrictedRef(ref.Name()) { continue } - if !protocol.ReferenceDiscoveryCallback(ctx, handle.Repository, ref.Name) { + if !protocol.ReferenceDiscoveryCallback(ctx, repository, ref.Name()) { continue } - if head != nil && head.Name() == ref.Name { + if head != nil && head.Name() == ref.Name() { result["HEAD"] = &RefResult{ Target: head.Name(), Value: head.Target().String(), } } - result[ref.Name] = &RefResult{ - Target: ref.SymbolicTarget, - Value: ref.Target.String(), + refResult := &RefResult{} + if ref.Type() == git.ReferenceSymbolic { + refResult.Target = ref.SymbolicTarget() + target, err := ref.Resolve() + if err != nil { + return nil, errors.Wrapf( + err, + "failed to resolve the symbolic target for %s(%s)", + ref.Name(), + ref.Target(), + ) + } + defer target.Free() + refResult.Value = target.Target().String() + } else if ref.Type() == git.ReferenceOid { + refResult.Value = ref.Target().String() } + result[ref.Name()] = refResult } return result, nil @@ -336,7 +388,7 @@ func handleRefs( func handleLog( ctx context.Context, - handle *RepositoryHandle, + repository *git.Repository, level AuthorizationLevel, protocol *GitProtocol, requestPath string, @@ -353,7 +405,7 @@ func handleLog( if len(splitPath) == 3 && len(splitPath[2]) != 0 { rev = splitPath[2] } - obj, err := handle.Repository.RevparseSingle(rev) + obj, err := repository.RevparseSingle(rev) if err != nil { return nil, base.ErrorWithCategory( ErrNotFound, @@ -374,7 +426,7 @@ func handleLog( if err := isCommitIDReachable( ctx, - handle, + repository, level, protocol, obj.Id(), @@ -386,7 +438,7 @@ func handleLog( return nil, nil } - walk, err := handle.Repository.Walk() + walk, err := repository.Walk() if err != nil { return nil, errors.Wrap( err, @@ -466,7 +518,7 @@ func (a *tarArchive) Create(path string, size int64) (io.Writer, error) { func handleArchive( ctx context.Context, - handle *RepositoryHandle, + repository *git.Repository, level AuthorizationLevel, protocol *GitProtocol, requestPath string, @@ -500,12 +552,12 @@ func handleArchive( errors.New("empty revision"), ) } - odb, err := handle.Repository.Odb() + odb, err := repository.Odb() if err != nil { return errors.Wrapf(err, "failed to get repository odb") } defer odb.Free() - obj, err := handle.Repository.RevparseSingle(rev) + obj, err := repository.RevparseSingle(rev) if err != nil { return base.ErrorWithCategory( ErrNotFound, @@ -521,7 +573,7 @@ func handleArchive( if obj.Type() == git.ObjectCommit { if err := isCommitIDReachable( ctx, - handle, + repository, level, protocol, obj.Id(), @@ -617,7 +669,7 @@ func handleArchive( return nil } - blob, err := handle.Repository.LookupBlob(entry.Id) + blob, err := repository.LookupBlob(entry.Id) if err != nil { return errors.Wrapf( err, @@ -670,7 +722,7 @@ func handleArchive( func handleShow( ctx context.Context, - handle *RepositoryHandle, + repository *git.Repository, level AuthorizationLevel, protocol *GitProtocol, requestPath string, @@ -686,7 +738,7 @@ func handleShow( } rev := splitPath[2] - obj, err := handle.Repository.RevparseSingle(rev) + obj, err := repository.RevparseSingle(rev) if err != nil { return nil, base.ErrorWithCategory( ErrNotFound, @@ -702,7 +754,7 @@ func handleShow( if obj.Type() == git.ObjectCommit { if err := isCommitIDReachable( ctx, - handle, + repository, level, protocol, obj.Id(), @@ -713,7 +765,7 @@ func handleShow( if len(splitPath) > 3 { // URLs of the form /+/rev/path. This shows either a tree or a blob. rev = fmt.Sprintf("%s:%s", rev, splitPath[3]) - obj, err = handle.Repository.RevparseSingle(rev) + obj, err = repository.RevparseSingle(rev) if err != nil { return nil, base.ErrorWithCategory( ErrNotFound, @@ -752,7 +804,7 @@ func handleShow( ), ) } - obj, err = handle.Repository.Lookup(entry.Id) + obj, err = repository.Lookup(entry.Id) if err != nil { return nil, base.ErrorWithCategory( ErrNotFound, @@ -802,7 +854,7 @@ func handleShow( return formatCommit(commit), nil } else if obj.Type() == git.ObjectTree { - return formatTree(handle.Repository, obj.Id()) + return formatTree(repository, obj.Id()) } else if obj.Type() == git.ObjectBlob { blob, err := obj.AsBlob() if err != nil { @@ -844,37 +896,57 @@ func handleBrowse( method := r.Method acceptMIMEType := r.Header.Get("Accept") txn := tracing.FromContext(ctx) - handle, err := OpenRepositoryHandle(ctx, m, repositoryPath, protocol.log) + repository, err := openRepository(ctx, repositoryPath) if err != nil { return errors.Wrap( err, - "failed to read references", + "failed to open git repository", + ) + } + defer repository.Free() + + lockfile := m.NewLockfile(repository.Path()) + if ok, err := lockfile.TryRLock(); !ok { + protocol.log.Info( + "Waiting for the lockfile", + map[string]interface{}{ + "err": err, + }, ) + if err := lockfile.RLock(); err != nil { + protocol.log.Error( + "Failed to acquire the lockfile", + map[string]interface{}{ + "err": err, + }, + ) + return err + } } - defer handle.Release() + defer lockfile.Unlock() var result any if requestPath == "/+refs" || requestPath == "/+refs/" { txn.SetName(method + " /:repo/+refs/") - result, err = handleRefs(ctx, handle, level, protocol, method) + result, err = handleRefs(ctx, repository, level, protocol, method) if err != nil { return err } } else if strings.HasPrefix(requestPath, "/+log/") { txn.SetName(method + " /:repo/+log/") - result, err = handleLog(ctx, handle, level, protocol, requestPath, method) + result, err = handleLog(ctx, repository, level, protocol, requestPath, method) if err != nil { return err } } else if strings.HasPrefix(requestPath, "/+archive/") { txn.SetName(method + " /:repo/+archive/") - err = handleArchive(ctx, handle, level, protocol, requestPath, r, w) + err = handleArchive(ctx, repository, level, protocol, requestPath, r, w) if err != nil { return err } } else if strings.HasPrefix(requestPath, "/+/") { txn.SetName(method + " /:repo/+/") - result, err = handleShow(ctx, handle, level, protocol, requestPath, method, acceptMIMEType) + result, err = handleShow(ctx, repository, level, protocol, requestPath, method, acceptMIMEType) if err != nil { return err } diff --git a/browser_test.go b/browser_test.go index 440d5ef..89acfef 100644 --- a/browser_test.go +++ b/browser_test.go @@ -29,11 +29,10 @@ func TestHandleRefs(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} result, err := handleRefs( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, "GET", @@ -77,11 +76,10 @@ func TestHandleRefsWithReferenceDiscoveryCallback(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} result, err := handleRefs( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, "GET", @@ -107,11 +105,10 @@ func TestHandleRestrictedRefs(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} result, err := handleRefs( context.Background(), - handle, + repository, AuthorizationAllowedRestricted, protocol, "GET", @@ -145,7 +142,6 @@ func TestHandleArchiveCommitZip(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} requestPath := "/+archive/88aa3454adb27c3c343ab57564d962a0a7f6a3c1.zip" req, err := http.NewRequest("GET", "http://test"+requestPath, nil) @@ -157,7 +153,7 @@ func TestHandleArchiveCommitZip(t *testing.T) { response := httptest.NewRecorder() if err := handleArchive( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, requestPath, @@ -190,7 +186,6 @@ func TestHandleArchiveCommitTarball(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} requestPath := "/+archive/88aa3454adb27c3c343ab57564d962a0a7f6a3c1.tar.gz" req, err := http.NewRequest("GET", "http://test"+requestPath, nil) @@ -203,7 +198,7 @@ func TestHandleArchiveCommitTarball(t *testing.T) { response := httptest.NewRecorder() if err := handleArchive( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, requestPath, @@ -260,7 +255,6 @@ func TestHandleArchiveCommitTarballFromTree(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} requestPath := "/+archive/417c01c8795a35b8e835113a85a5c0c1c77f67fb.tar.gz" req, err := http.NewRequest("GET", "http://test"+requestPath, nil) @@ -272,7 +266,7 @@ func TestHandleArchiveCommitTarballFromTree(t *testing.T) { response := httptest.NewRecorder() if err := handleArchive( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, requestPath, @@ -329,11 +323,10 @@ func TestHandleLog(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} result, err := handleLog( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, "/+log/", @@ -395,11 +388,10 @@ func TestHandleLogCommit(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} result, err := handleLog( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, "/+log/88aa3454adb27c3c343ab57564d962a0a7f6a3c1", @@ -445,11 +437,10 @@ func TestHandleShowCommit(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} result, err := handleShow( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, "/+/88aa3454adb27c3c343ab57564d962a0a7f6a3c1", @@ -492,7 +483,6 @@ func TestHandleShowTree(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} expected := &TreeResult{ ID: "417c01c8795a35b8e835113a85a5c0c1c77f67fb", @@ -515,7 +505,7 @@ func TestHandleShowTree(t *testing.T) { } { result, err := handleShow( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, requestURL, @@ -543,7 +533,6 @@ func TestHandleShowBlob(t *testing.T) { t.Fatalf("Error opening git repository: %v", err) } defer repository.Free() - handle := &RepositoryHandle{Repository: repository} expected := &BlobResult{ ID: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", @@ -559,7 +548,7 @@ func TestHandleShowBlob(t *testing.T) { } { result, err := handleShow( context.Background(), - handle, + repository, AuthorizationAllowed, protocol, requestURL, diff --git a/commits.go b/commits.go index 84d4d89..21f6504 100644 --- a/commits.go +++ b/commits.go @@ -13,6 +13,7 @@ import ( "strings" "github.com/omegaup/go-base/v3/logging" + "github.com/omegaup/go-base/v3/tracing" git "github.com/libgit2/git2go/v33" "github.com/pkg/errors" @@ -455,7 +456,6 @@ func SplitCommit( // the caller already has acquired one. func SpliceCommit( repository *git.Repository, - lockfileManager *LockfileManager, commit, parentCommit *git.Commit, overrides map[string]io.Reader, descriptions []SplitCommitDescription, @@ -466,13 +466,13 @@ func SpliceCommit( newPackPath string, log logging.Logger, ) ([]*GitCommand, error) { - newHandle, err := OpenRepositoryHandle(context.TODO(), lockfileManager, repository.Path(), log) + newRepository, err := openRepository(context.TODO(), repository.Path()) if err != nil { return nil, errors.Wrapf(err, "failed to open git repository at %s", repository.Path()) } - defer newHandle.Release() + defer newRepository.Free() - odb, err := newHandle.Repository.Odb() + odb, err := newRepository.Odb() if err != nil { return nil, errors.Wrap(err, "failed to open git odb") } @@ -500,7 +500,7 @@ func SpliceCommit( defer originalTree.Free() if len(overrides) != 0 { - overrideTree, err := BuildTree(newHandle.Repository, overrides, log) + overrideTree, err := BuildTree(newRepository, overrides, log) if err != nil { return nil, errors.Wrapf(err, "failed to create the override tree for commit %s", commit.Id()) } @@ -510,11 +510,11 @@ func SpliceCommit( return nil, errors.Wrapf(err, "failed to obtain the override tree for commit %s", commit.Id()) } defer originalTree.Free() - if err = copyTree(repository, originalTree.Id(), newHandle.Repository); err != nil { + if err = copyTree(repository, originalTree.Id(), newRepository); err != nil { return nil, errors.Wrap(err, "failed to copy the tree to the new repository") } mergedOverrideTree, err := MergeTrees( - newHandle.Repository, + newRepository, overrideTree, originalTree, ) @@ -527,7 +527,7 @@ func SpliceCommit( if parentCommit != nil { overrideCommitParents = append(overrideCommitParents, parentCommit.Id()) } - overrideCommitID, err := newHandle.Repository.CreateCommitFromIds( + overrideCommitID, err := newRepository.CreateCommitFromIds( "", commit.Author(), commit.Committer(), @@ -538,21 +538,21 @@ func SpliceCommit( if err != nil { return nil, errors.Wrap(err, "failed to create merged override commit") } - if commit, err = newHandle.Repository.LookupCommit(overrideCommitID); err != nil { + if commit, err = newRepository.LookupCommit(overrideCommitID); err != nil { return nil, errors.Wrap(err, "failed to look up merged override commit") } defer commit.Free() // Now that all the objects needed are in the new repository, we can just // do everything in that one. - repository = newHandle.Repository + repository = newRepository } splitCommits, err := SplitCommit( commit, repository, descriptions, - newHandle.Repository, + newRepository, author, committer, commitMessageTag, @@ -570,7 +570,7 @@ func SpliceCommit( } for i, splitCommit := range splitCommits { - newCommit, err := newHandle.Repository.LookupCommit(splitCommit.CommitID) + newCommit, err := newRepository.LookupCommit(splitCommit.CommitID) if err != nil { return nil, errors.Wrapf(err, "failed to look up new private commit %s", splitCommit.CommitID) } @@ -584,7 +584,7 @@ func SpliceCommit( oldTreeID = descriptions[i].ParentCommit.TreeId() } - newTree, err := newHandle.Repository.LookupTree(splitCommit.TreeID) + newTree, err := newRepository.LookupTree(splitCommit.TreeID) if err != nil { return nil, errors.Wrapf(err, "failed to look up new private tree %s", splitCommit.TreeID) } @@ -612,7 +612,7 @@ func SpliceCommit( } mergedTree, err := MergeTrees( - newHandle.Repository, + newRepository, newTrees..., ) if err != nil { @@ -627,7 +627,7 @@ func SpliceCommit( // This cannot use CreateCommit, since the parent commits are not yet in the // repository. We are yet to create a packfile with them. - mergedID, err := newHandle.Repository.CreateCommitFromIds( + mergedID, err := newRepository.CreateCommitFromIds( "", author, committer, @@ -658,7 +658,7 @@ func SpliceCommit( }, ) - walk, err := newHandle.Repository.Walk() + walk, err := newRepository.Walk() if err != nil { return nil, errors.Wrap(err, "failed to create revwalk") } @@ -680,7 +680,7 @@ func SpliceCommit( } defer f.Close() - pb, err := newHandle.Repository.NewPackbuilder() + pb, err := newRepository.NewPackbuilder() if err != nil { return nil, errors.Wrapf(err, "failed to create packbuilder") } @@ -776,3 +776,8 @@ func BuildTree( ) return repository.LookupTree(mergedTreeID) } + +func openRepository(ctx context.Context, repositoryPath string) (*git.Repository, error) { + defer tracing.FromContext(ctx).StartSegment("openRepository").End() + return git.OpenRepository(repositoryPath) +} diff --git a/commits_test.go b/commits_test.go index 5865440..aaf357c 100644 --- a/commits_test.go +++ b/commits_test.go @@ -243,8 +243,6 @@ func TestSpliceCommit(t *testing.T) { if os.Getenv("PRESERVE") == "" { defer os.RemoveAll(dir) } - m := NewLockfileManager() - defer m.Clear() repository, err := git.InitRepository(dir, true) if err != nil { @@ -306,7 +304,6 @@ func TestSpliceCommit(t *testing.T) { newPackPath := path.Join(dir, "new.pack") newCommands, err := SpliceCommit( repository, - m, originalCommit, nil, map[string]io.Reader{ diff --git a/protocol.go b/protocol.go index 2a6f62e..8303955 100644 --- a/protocol.go +++ b/protocol.go @@ -487,21 +487,42 @@ func handleInfoRefs( log logging.Logger, w io.Writer, ) error { - handle, err := OpenRepositoryHandle(ctx, m, repositoryPath, log) + repository, err := openRepository(ctx, repositoryPath) if err != nil { - return err + return errors.Wrap( + err, + "failed to open git repository", + ) + } + defer repository.Free() + + lockfile := m.NewLockfile(repository.Path()) + if ok, err := lockfile.TryRLock(); !ok { + log.Info( + "Waiting for the lockfile", + map[string]interface{}{ + "err": err, + }, + ) + if err := lockfile.RLock(); err != nil { + return errors.Wrap( + err, + "failed to acquire the lockfile", + ) + } } - defer handle.Release() + defer lockfile.Unlock() - references, err := handle.References() + it, err := repository.NewReferenceIterator() if err != nil { return errors.Wrap( err, "failed to read references", ) } + defer it.Free() - head, err := handle.Repository.Head() + head, err := repository.Head() if err != nil && !git.IsErrorCode(err, git.ErrorCodeUnbornBranch) { return errors.Wrap( err, @@ -531,24 +552,37 @@ func handleInfoRefs( ))) sentCapabilities = true } - for _, ref := range references { - if level == AuthorizationAllowedRestricted && isRestrictedRef(ref.Name) { + for { + ref, err := it.Next() + if err != nil { + if !git.IsErrorCode(err, git.ErrorCodeIterOver) { + log.Error( + "Error getting reference", + map[string]interface{}{ + "err": err, + }, + ) + } + break + } + defer ref.Free() + if level == AuthorizationAllowedRestricted && isRestrictedRef(ref.Name()) { continue } - if !protocol.ReferenceDiscoveryCallback(ctx, handle.Repository, ref.Name) { + if !protocol.ReferenceDiscoveryCallback(ctx, repository, ref.Name()) { continue } if sentCapabilities { p.WritePktLine([]byte(fmt.Sprintf( "%s %s\n", - ref.Target.String(), - ref.Name, + ref.Target().String(), + ref.Name(), ))) } else { p.WritePktLine([]byte(fmt.Sprintf( "%s %s\x00%s\n", - ref.Target.String(), - ref.Name, + ref.Target().String(), + ref.Name(), strings.Join(capabilities, " "), ))) sentCapabilities = true @@ -604,16 +638,33 @@ func handlePull( r io.Reader, w io.Writer, ) error { - handle, err := OpenRepositoryHandle(ctx, m, repositoryPath, log) + repository, err := openRepository(ctx, repositoryPath) if err != nil { return errors.Wrap( err, "failed to open git repository", ) } - defer handle.Release() + defer repository.Free() - pb, err := handle.Repository.NewPackbuilder() + lockfile := m.NewLockfile(repository.Path()) + if ok, err := lockfile.TryRLock(); !ok { + log.Info( + "Waiting for the lockfile", + map[string]interface{}{ + "err": err, + }, + ) + if err := lockfile.RLock(); err != nil { + return errors.Wrap( + err, + "failed to acquire the lockfile", + ) + } + } + defer lockfile.Unlock() + + pb, err := repository.NewPackbuilder() if err != nil { return errors.Wrap( err, @@ -691,7 +742,7 @@ func handlePull( errors.Errorf("invalid OID: %s", tokens[1]), ) } - commit, err := handle.Repository.LookupCommit(oid) + commit, err := repository.LookupCommit(oid) if err != nil { log.Debug( "Unknown commit requested", @@ -803,7 +854,7 @@ func handlePull( errors.Errorf("invalid OID: %s", tokens[1]), ) } - commit, err := handle.Repository.LookupCommit(oid) + commit, err := repository.LookupCommit(oid) if err == nil { commit.Free() if !acked { @@ -921,14 +972,31 @@ func handlePush( r io.Reader, w io.Writer, ) error { - handle, err := OpenRepositoryHandle(ctx, m, repositoryPath, log) + repository, err := openRepository(ctx, repositoryPath) if err != nil { return errors.Wrap( err, "failed to open git repository", ) } - defer handle.Release() + defer repository.Free() + + lockfile := m.NewLockfile(repository.Path()) + if ok, err := lockfile.TryRLock(); !ok { + log.Info( + "Waiting for the lockfile", + map[string]interface{}{ + "err": err, + }, + ) + if err := lockfile.RLock(); err != nil { + return errors.Wrap( + err, + "failed to acquire the lockfile", + ) + } + } + defer lockfile.Unlock() pr := NewPktLineReader(r) reportStatus := false @@ -977,7 +1045,7 @@ func handlePush( ReferenceName: tokens[2], } if _, ok := references[command.ReferenceName]; !ok { - ref, err := handle.Repository.References.Lookup(command.ReferenceName) + ref, err := repository.References.Lookup(command.ReferenceName) if err == nil { defer ref.Free() } @@ -1005,19 +1073,12 @@ func handlePush( _, err, unpackErr := protocol.PushPackfile( ctx, - handle.Repository, - handle.Lockfile, + repository, + lockfile, level, commands, r, ) - if handle.Lockfile.State() == LockfileStateLocked { - // We successfully acquired the write lock. Regardless of whether we were - // able to modify the repository, we invalidate all other pre-existing - // handles. - handle.DoNotReturnToPool = true - defer EvictRepositoryHandles(repositoryPath) - } if !reportStatus { return err } diff --git a/protocol_test.go b/protocol_test.go index cd4447b..3763615 100644 --- a/protocol_test.go +++ b/protocol_test.go @@ -803,7 +803,6 @@ func TestHandlePushPreprocess(t *testing.T) { newPackPath := path.Join(tmpDir, "new.pack") newCommands, err := SpliceCommit( originalRepository, - m, originalCommit, nil, map[string]io.Reader{}, diff --git a/repository_handle.go b/repository_handle.go deleted file mode 100644 index fba71e0..0000000 --- a/repository_handle.go +++ /dev/null @@ -1,212 +0,0 @@ -package githttp - -import ( - "context" - "sync" - - "github.com/omegaup/go-base/v3" - "github.com/omegaup/go-base/v3/logging" - "github.com/omegaup/go-base/v3/tracing" - - git "github.com/libgit2/git2go/v33" - "github.com/pkg/errors" -) - -var ( - repositoryHandlePool *base.KeyedPool[*RepositoryHandle] - repositoryHandlePoolOnce sync.Once -) - -// RepositoryHandle contains a reference to an open git repository and its -// lockfile. It also contains lazily-obtained references. -type RepositoryHandle struct { - Repository *git.Repository - Lockfile *Lockfile - DoNotReturnToPool bool - - path string - references []*RepositoryReference - log logging.Logger -} - -func newRepositoryHandle(m *LockfileManager, path string, log logging.Logger) (*RepositoryHandle, error) { - log.Info( - "Acquiring a repository handle", - map[string]any{ - "path": path, - }, - ) - repository, err := git.OpenRepository(path) - if err != nil { - return nil, errors.Wrapf( - err, - "failed to open git repository at %q", - path, - ) - } - - return &RepositoryHandle{ - Repository: repository, - Lockfile: m.NewLockfile(repository.Path()), - - path: path, - log: log, - }, nil -} - -// OpenRepositoryHandle opens a git repository and returns a handle to it. -// Opening git repositories from scratch is moderately expensive, so once the -// caller is done with the handle, they can Release() it into an object pool so -// that it can be reused again in the future (unless the repository has -// changed). -func OpenRepositoryHandle(ctx context.Context, m *LockfileManager, path string, log logging.Logger) (*RepositoryHandle, error) { - txn := tracing.FromContext(ctx) - defer txn.StartSegment("OpenRepositoryHandle").End() - repositoryHandlePoolOnce.Do(func() { - repositoryHandlePool = base.NewKeyedPool(base.KeyedPoolOptions[*RepositoryHandle]{ - New: func(path string) (*RepositoryHandle, error) { - return newRepositoryHandle(m, path, log) - }, - OnEvicted: func(key string, value *RepositoryHandle) { - value.free() - }, - }) - }) - - handle, err := repositoryHandlePool.Get(path) - if err != nil { - return nil, err - } - - defer txn.StartSegment("acquire lock").End() - if ok, err := handle.Lockfile.TryRLock(); !ok { - log.Info( - "Waiting for the lockfile", - map[string]any{ - "err": err, - }, - ) - // If we failed to acquire the read lock immediately, it means that there's - // another handle that's currently writing to the repository. When that - // happens, we cannot rely on the cached information from the previously - // opened handle, so we need to create a brand new one. - handle.DoNotReturnToPool = true - handle.Release() - - handle, err = newRepositoryHandle(m, path, log) - if err != nil { - return nil, err - } - if err := handle.Lockfile.RLock(); err != nil { - handle.Release() - return nil, errors.Wrapf( - err, - "failed to acquire the lockfile at %q", - path, - ) - } - } - - return handle, nil -} - -// EvictRepositoryHandles removes any repository handles that exist in the -// handle pool. This should be called if a particular repository is modified to -// invalidate all cached objects. -func EvictRepositoryHandles(path string) { - repositoryHandlePool.Remove(path) -} - -// Release releases the repositoy and puts it back into the pool unless -// DoNotReturnToPool was set, in which case the resources are immediately freed -// and forgotten. -// -// Once in the pool's ownership, the underlying repository can be freed at some -// point in the future. -func (h *RepositoryHandle) Release() { - h.Lockfile.Unlock() - if h.DoNotReturnToPool { - h.free() - return - } - repositoryHandlePool.Put(h.path, h) -} - -func (h *RepositoryHandle) free() { - h.log.Info( - "Releasing a repository handle", - map[string]any{ - "path": h.path, - }, - ) - h.Repository.Free() -} - -// RepositoryReference contains the fully resolved information of a -// git.Reference. -type RepositoryReference struct { - Name string - Target *git.Oid - SymbolicTarget string -} - -// References returns the list of all references (after fully resolving them) -// in the repository. This information is lazily computed the first time it's -// used and then cached for future usages. -func (h *RepositoryHandle) References() ([]*RepositoryReference, error) { - if h.references == nil { - it, err := h.Repository.NewReferenceIterator() - if err != nil { - return nil, errors.Wrap( - err, - "failed to create a reference iterator", - ) - } - defer it.Free() - - // Make sure this is non-nil even if it's empty. - references := []*RepositoryReference{} - for { - ref, err := it.Next() - if err != nil { - if git.IsErrorCode(err, git.ErrorCodeIterOver) { - break - } - - return nil, errors.Wrap( - err, - "failed to read next reference", - ) - } - - if ref.Type() == git.ReferenceSymbolic { - target, err := ref.Resolve() - if err != nil { - ref.Free() - return nil, errors.Wrapf( - err, - "failed to resolve the symbolic target for %s(%s)", - ref.Name(), - ref.Target(), - ) - } - references = append(references, &RepositoryReference{ - Name: ref.Name(), - Target: target.Target(), - SymbolicTarget: ref.SymbolicTarget(), - }) - target.Free() - } else if ref.Type() == git.ReferenceOid { - references = append(references, &RepositoryReference{ - Name: ref.Name(), - Target: ref.Target(), - }) - } - ref.Free() - } - - h.references = references - } - - return h.references, nil -}