From 554992523574684ecce36d38bf5310bff52c8c03 Mon Sep 17 00:00:00 2001 From: Michael Muré Date: Sun, 4 Apr 2021 13:28:21 +0200 Subject: cache: many fixes following the dag entity migration --- bug/operation.go | 2 ++ cache/repo_cache.go | 2 +- cache/repo_cache_test.go | 5 +++-- cache/resolvers.go | 29 +++++++++++++++++++++++++++++ entity/dag/clock.go | 7 ++++--- identity/resolver.go | 35 +++++++++++++++++++++++++++++++++++ repository/gogit.go | 2 +- 7 files changed, 75 insertions(+), 7 deletions(-) diff --git a/bug/operation.go b/bug/operation.go index 8daa2cde..b3610381 100644 --- a/bug/operation.go +++ b/bug/operation.go @@ -113,6 +113,8 @@ func operationUnmarshaller(author identity.Interface, raw json.RawMessage) (dag. op.Author_ = author case *CreateOperation: op.Author_ = author + case *EditCommentOperation: + op.Author_ = author case *LabelChangeOperation: op.Author_ = author case *NoOpOperation: diff --git a/cache/repo_cache.go b/cache/repo_cache.go index 58022bda..14d5f3db 100644 --- a/cache/repo_cache.go +++ b/cache/repo_cache.go @@ -195,7 +195,7 @@ func (c *RepoCache) buildCache() error { c.bugExcerpts = make(map[entity.Id]*BugExcerpt) - allBugs := bug.ReadAll(c.repo) + allBugs := bug.ReadAllWithResolver(c.repo, newIdentityCacheResolverNoLock(c)) // wipe the index just to be sure err := c.repo.ClearBleveIndex("bug") diff --git a/cache/repo_cache_test.go b/cache/repo_cache_test.go index fab8fff0..d13fa026 100644 --- a/cache/repo_cache_test.go +++ b/cache/repo_cache_test.go @@ -86,11 +86,12 @@ func TestCache(t *testing.T) { require.Empty(t, cache.identities) require.Empty(t, cache.identitiesExcerpts) - // Reload, only excerpt are loaded + // Reload, only excerpt are loaded, but as we need to load the identities used in the bugs + // to check the signatures, we also load the identity used above cache, err = NewRepoCache(repo) require.NoError(t, err) require.Empty(t, cache.bugs) - require.Empty(t, cache.identities) + require.Len(t, cache.identities, 1) require.Len(t, cache.bugExcerpts, 2) require.Len(t, cache.identitiesExcerpts, 2) diff --git a/cache/resolvers.go b/cache/resolvers.go index 36b70d3b..e53c3660 100644 --- a/cache/resolvers.go +++ b/cache/resolvers.go @@ -20,3 +20,32 @@ func newIdentityCacheResolver(cache *RepoCache) *identityCacheResolver { func (i *identityCacheResolver) ResolveIdentity(id entity.Id) (identity.Interface, error) { return i.cache.ResolveIdentity(id) } + +var _ identity.Resolver = &identityCacheResolverNoLock{} + +// identityCacheResolverNoLock is an identity Resolver that retrieve identities from +// the cache, without locking it. +type identityCacheResolverNoLock struct { + cache *RepoCache +} + +func newIdentityCacheResolverNoLock(cache *RepoCache) *identityCacheResolverNoLock { + return &identityCacheResolverNoLock{cache: cache} +} + +func (ir *identityCacheResolverNoLock) ResolveIdentity(id entity.Id) (identity.Interface, error) { + cached, ok := ir.cache.identities[id] + if ok { + return cached, nil + } + + i, err := identity.ReadLocal(ir.cache.repo, id) + if err != nil { + return nil, err + } + + cached = NewIdentityCache(ir.cache, i) + ir.cache.identities[id] = cached + + return cached, nil +} diff --git a/entity/dag/clock.go b/entity/dag/clock.go index dc9bb72d..793fa1bf 100644 --- a/entity/dag/clock.go +++ b/entity/dag/clock.go @@ -9,7 +9,7 @@ import ( // ClockLoader is the repository.ClockLoader for Entity func ClockLoader(defs ...Definition) repository.ClockLoader { - clocks := make([]string, len(defs)*2) + clocks := make([]string, 0, len(defs)*2) for _, def := range defs { clocks = append(clocks, fmt.Sprintf(creationClockPattern, def.Namespace)) clocks = append(clocks, fmt.Sprintf(editClockPattern, def.Namespace)) @@ -18,8 +18,9 @@ func ClockLoader(defs ...Definition) repository.ClockLoader { return repository.ClockLoader{ Clocks: clocks, Witnesser: func(repo repository.ClockedRepo) error { - // We don't care about the actual identity so an IdentityStub will do - resolver := identity.NewStubResolver() + // we need to actually load the identities because of the commit signature check when reading, + // which require the full identities with crypto keys + resolver := identity.NewCachedResolver(identity.NewSimpleResolver(repo)) for _, def := range defs { // we actually just need to read all entities, diff --git a/identity/resolver.go b/identity/resolver.go index ab380a12..8e066e9d 100644 --- a/identity/resolver.go +++ b/identity/resolver.go @@ -1,6 +1,8 @@ package identity import ( + "sync" + "github.com/MichaelMure/git-bug/entity" "github.com/MichaelMure/git-bug/repository" ) @@ -34,3 +36,36 @@ func NewStubResolver() *StubResolver { func (s *StubResolver) ResolveIdentity(id entity.Id) (Interface, error) { return &IdentityStub{id: id}, nil } + +// CachedResolver is a resolver ensuring that loading is done only once through another Resolver. +type CachedResolver struct { + mu sync.RWMutex + resolver Resolver + identities map[entity.Id]Interface +} + +func NewCachedResolver(resolver Resolver) *CachedResolver { + return &CachedResolver{ + resolver: resolver, + identities: make(map[entity.Id]Interface), + } +} + +func (c *CachedResolver) ResolveIdentity(id entity.Id) (Interface, error) { + c.mu.RLock() + if i, ok := c.identities[id]; ok { + c.mu.RUnlock() + return i, nil + } + c.mu.RUnlock() + + c.mu.Lock() + defer c.mu.Unlock() + + i, err := c.resolver.ResolveIdentity(id) + if err != nil { + return nil, err + } + c.identities[id] = i + return i, nil +} diff --git a/repository/gogit.go b/repository/gogit.go index 248c34d5..20454bd7 100644 --- a/repository/gogit.go +++ b/repository/gogit.go @@ -335,7 +335,7 @@ func (repo *GoGitRepo) ClearBleveIndex(name string) error { repo.indexesMutex.Lock() defer repo.indexesMutex.Unlock() - path := filepath.Join(repo.path, "indexes", name) + path := filepath.Join(repo.path, "git-bug", "indexes", name) err := os.RemoveAll(path) if err != nil { -- cgit