From ca720f165cb286d4372ad48595e532a2423f2f07 Mon Sep 17 00:00:00 2001 From: Michael Muré Date: Wed, 16 Sep 2020 16:22:02 +0200 Subject: cache,bug,identity: structural change - bug doesn't commit identities anymore, only make sure they are commit - cache use an IdentityResolver to load bugs with identities from the cache (deps injection) - IdentityCache now are identity.Interface --- bug/bug.go | 82 ++++++++++++++++++++---------------------- bug/bug_actions.go | 10 ++++-- bug/bug_actions_test.go | 32 ++++++++++------- bug/bug_test.go | 50 +++++++++++++++----------- bug/clocks.go | 5 ++- bug/operation_iterator_test.go | 13 ++++--- bug/operation_pack.go | 7 ++-- bug/operation_test.go | 2 +- 8 files changed, 112 insertions(+), 89 deletions(-) (limited to 'bug') diff --git a/bug/bug.go b/bug/bug.go index 2ee89031..6f5d0a7a 100644 --- a/bug/bug.go +++ b/bug/bug.go @@ -74,48 +74,32 @@ func NewBug() *Bug { return &Bug{} } -// FindLocalBug find an existing Bug matching a prefix -func FindLocalBug(repo repository.ClockedRepo, prefix string) (*Bug, error) { - ids, err := ListLocalIds(repo) - - if err != nil { - return nil, err - } - - // preallocate but empty - matching := make([]entity.Id, 0, 5) - - for _, id := range ids { - if id.HasPrefix(prefix) { - matching = append(matching, id) - } - } - - if len(matching) == 0 { - return nil, errors.New("no matching bug found.") - } - - if len(matching) > 1 { - return nil, NewErrMultipleMatchBug(matching) - } - - return ReadLocalBug(repo, matching[0]) +// ReadLocal will read a local bug from its hash +func ReadLocal(repo repository.ClockedRepo, id entity.Id) (*Bug, error) { + ref := bugsRefPattern + id.String() + return read(repo, identity.NewSimpleResolver(repo), ref) } -// ReadLocalBug will read a local bug from its hash -func ReadLocalBug(repo repository.ClockedRepo, id entity.Id) (*Bug, error) { +// ReadLocalWithResolver will read a local bug from its hash +func ReadLocalWithResolver(repo repository.ClockedRepo, identityResolver identity.Resolver, id entity.Id) (*Bug, error) { ref := bugsRefPattern + id.String() - return readBug(repo, ref) + return read(repo, identityResolver, ref) +} + +// ReadRemote will read a remote bug from its hash +func ReadRemote(repo repository.ClockedRepo, remote string, id entity.Id) (*Bug, error) { + ref := fmt.Sprintf(bugsRemoteRefPattern, remote) + id.String() + return read(repo, identity.NewSimpleResolver(repo), ref) } -// ReadRemoteBug will read a remote bug from its hash -func ReadRemoteBug(repo repository.ClockedRepo, remote string, id entity.Id) (*Bug, error) { +// ReadRemoteWithResolver will read a remote bug from its hash +func ReadRemoteWithResolver(repo repository.ClockedRepo, identityResolver identity.Resolver, remote string, id entity.Id) (*Bug, error) { ref := fmt.Sprintf(bugsRemoteRefPattern, remote) + id.String() - return readBug(repo, ref) + return read(repo, identityResolver, ref) } -// readBug will read and parse a Bug from git -func readBug(repo repository.ClockedRepo, ref string) (*Bug, error) { +// read will read and parse a Bug from git +func read(repo repository.ClockedRepo, identityResolver identity.Resolver, ref string) (*Bug, error) { refSplit := strings.Split(ref, "/") id := entity.Id(refSplit[len(refSplit)-1]) @@ -233,8 +217,7 @@ func readBug(repo repository.ClockedRepo, ref string) (*Bug, error) { } // Make sure that the identities are properly loaded - resolver := identity.NewSimpleResolver(repo) - err = bug.EnsureIdentities(resolver) + err = bug.EnsureIdentities(identityResolver) if err != nil { return nil, err } @@ -297,19 +280,30 @@ type StreamedBug struct { Err error } -// ReadAllLocalBugs read and parse all local bugs -func ReadAllLocalBugs(repo repository.ClockedRepo) <-chan StreamedBug { - return readAllBugs(repo, bugsRefPattern) +// ReadAllLocal read and parse all local bugs +func ReadAllLocal(repo repository.ClockedRepo) <-chan StreamedBug { + return readAll(repo, identity.NewSimpleResolver(repo), bugsRefPattern) +} + +// ReadAllLocalWithResolver read and parse all local bugs +func ReadAllLocalWithResolver(repo repository.ClockedRepo, identityResolver identity.Resolver) <-chan StreamedBug { + return readAll(repo, identityResolver, bugsRefPattern) +} + +// ReadAllRemote read and parse all remote bugs for a given remote +func ReadAllRemote(repo repository.ClockedRepo, remote string) <-chan StreamedBug { + refPrefix := fmt.Sprintf(bugsRemoteRefPattern, remote) + return readAll(repo, identity.NewSimpleResolver(repo), refPrefix) } -// ReadAllRemoteBugs read and parse all remote bugs for a given remote -func ReadAllRemoteBugs(repo repository.ClockedRepo, remote string) <-chan StreamedBug { +// ReadAllRemoteWithResolver read and parse all remote bugs for a given remote +func ReadAllRemoteWithResolver(repo repository.ClockedRepo, identityResolver identity.Resolver, remote string) <-chan StreamedBug { refPrefix := fmt.Sprintf(bugsRemoteRefPattern, remote) - return readAllBugs(repo, refPrefix) + return readAll(repo, identityResolver, refPrefix) } // Read and parse all available bug with a given ref prefix -func readAllBugs(repo repository.ClockedRepo, refPrefix string) <-chan StreamedBug { +func readAll(repo repository.ClockedRepo, identityResolver identity.Resolver, refPrefix string) <-chan StreamedBug { out := make(chan StreamedBug) go func() { @@ -322,7 +316,7 @@ func readAllBugs(repo repository.ClockedRepo, refPrefix string) <-chan StreamedB } for _, ref := range refs { - b, err := readBug(repo, ref) + b, err := read(repo, identityResolver, ref) if err != nil { out <- StreamedBug{Err: err} diff --git a/bug/bug_actions.go b/bug/bug_actions.go index f99f83ad..21ce3733 100644 --- a/bug/bug_actions.go +++ b/bug/bug_actions.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/MichaelMure/git-bug/entity" + "github.com/MichaelMure/git-bug/identity" "github.com/MichaelMure/git-bug/repository" "github.com/pkg/errors" ) @@ -57,6 +58,11 @@ func Pull(repo repository.ClockedRepo, remote string) error { func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeResult { out := make(chan entity.MergeResult) + // no caching for the merge, we load everything from git even if that means multiple + // copy of the same entity in memory. The cache layer will intercept the results to + // invalidate entities if necessary. + identityResolver := identity.NewSimpleResolver(repo) + go func() { defer close(out) @@ -77,7 +83,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeRes continue } - remoteBug, err := readBug(repo, remoteRef) + remoteBug, err := read(repo, identityResolver, remoteRef) if err != nil { out <- entity.NewMergeInvalidStatus(id, errors.Wrap(err, "remote bug is not readable").Error()) @@ -111,7 +117,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeRes continue } - localBug, err := readBug(repo, localRef) + localBug, err := read(repo, identityResolver, localRef) if err != nil { out <- entity.NewMergeError(errors.Wrap(err, "local bug is not readable"), id) diff --git a/bug/bug_actions_test.go b/bug/bug_actions_test.go index c867ac80..df35a5e5 100644 --- a/bug/bug_actions_test.go +++ b/bug/bug_actions_test.go @@ -16,6 +16,8 @@ func TestPushPull(t *testing.T) { defer repository.CleanupTestRepos(repoA, repoB, remote) reneA := identity.NewIdentity("René Descartes", "rene@descartes.fr") + err := reneA.Commit(repoA) + require.NoError(t, err) bug1, _, err := Create(reneA, time.Now().Unix(), "bug1", "message") require.NoError(t, err) @@ -37,7 +39,7 @@ func TestPushPull(t *testing.T) { err = Pull(repoB, "origin") require.NoError(t, err) - bugs := allBugs(t, ReadAllLocalBugs(repoB)) + bugs := allBugs(t, ReadAllLocal(repoB)) if len(bugs) != 1 { t.Fatal("Unexpected number of bugs") @@ -58,7 +60,7 @@ func TestPushPull(t *testing.T) { err = Pull(repoA, "origin") require.NoError(t, err) - bugs = allBugs(t, ReadAllLocalBugs(repoA)) + bugs = allBugs(t, ReadAllLocal(repoA)) if len(bugs) != 2 { t.Fatal("Unexpected number of bugs") @@ -91,6 +93,8 @@ func _RebaseTheirs(t testing.TB) { defer repository.CleanupTestRepos(repoA, repoB, remote) reneA := identity.NewIdentity("René Descartes", "rene@descartes.fr") + err := reneA.Commit(repoA) + require.NoError(t, err) bug1, _, err := Create(reneA, time.Now().Unix(), "bug1", "message") require.NoError(t, err) @@ -114,7 +118,7 @@ func _RebaseTheirs(t testing.TB) { err = Pull(repoB, "origin") require.NoError(t, err) - bug2, err := ReadLocalBug(repoB, bug1.Id()) + bug2, err := ReadLocal(repoB, bug1.Id()) require.NoError(t, err) assert.False(t, bug2.NeedCommit()) @@ -140,13 +144,13 @@ func _RebaseTheirs(t testing.TB) { err = Pull(repoA, "origin") require.NoError(t, err) - bugs := allBugs(t, ReadAllLocalBugs(repoB)) + bugs := allBugs(t, ReadAllLocal(repoB)) if len(bugs) != 1 { t.Fatal("Unexpected number of bugs") } - bug3, err := ReadLocalBug(repoA, bug1.Id()) + bug3, err := ReadLocal(repoA, bug1.Id()) require.NoError(t, err) if nbOps(bug3) != 4 { @@ -169,6 +173,8 @@ func _RebaseOurs(t testing.TB) { defer repository.CleanupTestRepos(repoA, repoB, remote) reneA := identity.NewIdentity("René Descartes", "rene@descartes.fr") + err := reneA.Commit(repoA) + require.NoError(t, err) bug1, _, err := Create(reneA, time.Now().Unix(), "bug1", "message") require.NoError(t, err) @@ -220,13 +226,13 @@ func _RebaseOurs(t testing.TB) { err = Pull(repoA, "origin") require.NoError(t, err) - bugs := allBugs(t, ReadAllLocalBugs(repoA)) + bugs := allBugs(t, ReadAllLocal(repoA)) if len(bugs) != 1 { t.Fatal("Unexpected number of bugs") } - bug2, err := ReadLocalBug(repoA, bug1.Id()) + bug2, err := ReadLocal(repoA, bug1.Id()) require.NoError(t, err) if nbOps(bug2) != 10 { @@ -258,6 +264,8 @@ func _RebaseConflict(t testing.TB) { defer repository.CleanupTestRepos(repoA, repoB, remote) reneA := identity.NewIdentity("René Descartes", "rene@descartes.fr") + err := reneA.Commit(repoA) + require.NoError(t, err) bug1, _, err := Create(reneA, time.Now().Unix(), "bug1", "message") require.NoError(t, err) @@ -305,7 +313,7 @@ func _RebaseConflict(t testing.TB) { err = bug1.Commit(repoA) require.NoError(t, err) - bug2, err := ReadLocalBug(repoB, bug1.Id()) + bug2, err := ReadLocal(repoB, bug1.Id()) require.NoError(t, err) reneB, err := identity.ReadLocal(repoA, reneA.Id()) @@ -346,13 +354,13 @@ func _RebaseConflict(t testing.TB) { err = Pull(repoB, "origin") require.NoError(t, err) - bugs := allBugs(t, ReadAllLocalBugs(repoB)) + bugs := allBugs(t, ReadAllLocal(repoB)) if len(bugs) != 1 { t.Fatal("Unexpected number of bugs") } - bug3, err := ReadLocalBug(repoB, bug1.Id()) + bug3, err := ReadLocal(repoB, bug1.Id()) require.NoError(t, err) if nbOps(bug3) != 19 { @@ -367,13 +375,13 @@ func _RebaseConflict(t testing.TB) { err = Pull(repoA, "origin") require.NoError(t, err) - bugs = allBugs(t, ReadAllLocalBugs(repoA)) + bugs = allBugs(t, ReadAllLocal(repoA)) if len(bugs) != 1 { t.Fatal("Unexpected number of bugs") } - bug4, err := ReadLocalBug(repoA, bug1.Id()) + bug4, err := ReadLocal(repoA, bug1.Id()) require.NoError(t, err) if nbOps(bug4) != 19 { diff --git a/bug/bug_test.go b/bug/bug_test.go index ac7da693..05f6e617 100644 --- a/bug/bug_test.go +++ b/bug/bug_test.go @@ -5,7 +5,6 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/MichaelMure/git-bug/identity" @@ -18,11 +17,14 @@ func TestBugId(t *testing.T) { bug1 := NewBug() rene := identity.NewIdentity("René Descartes", "rene@descartes.fr") + err := rene.Commit(mockRepo) + require.NoError(t, err) + createOp := NewCreateOp(rene, time.Now().Unix(), "title", "message", nil) bug1.Append(createOp) - err := bug1.Commit(mockRepo) + err = bug1.Commit(mockRepo) if err != nil { t.Fatal(err) @@ -37,6 +39,9 @@ func TestBugValidity(t *testing.T) { bug1 := NewBug() rene := identity.NewIdentity("René Descartes", "rene@descartes.fr") + err := rene.Commit(mockRepo) + require.NoError(t, err) + createOp := NewCreateOp(rene, time.Now().Unix(), "title", "message", nil) if bug1.Validate() == nil { @@ -49,7 +54,7 @@ func TestBugValidity(t *testing.T) { t.Fatal("Bug with just a CreateOp should be valid") } - err := bug1.Commit(mockRepo) + err = bug1.Commit(mockRepo) if err != nil { t.Fatal(err) } @@ -67,9 +72,14 @@ func TestBugValidity(t *testing.T) { } func TestBugCommitLoad(t *testing.T) { + repo := repository.NewMockRepoForTest() + bug1 := NewBug() rene := identity.NewIdentity("René Descartes", "rene@descartes.fr") + err := rene.Commit(repo) + require.NoError(t, err) + createOp := NewCreateOp(rene, time.Now().Unix(), "title", "message", nil) setTitleOp := NewSetTitleOp(rene, time.Now().Unix(), "title2", "title1") addCommentOp := NewAddCommentOp(rene, time.Now().Unix(), "message2", nil) @@ -77,35 +87,33 @@ func TestBugCommitLoad(t *testing.T) { bug1.Append(createOp) bug1.Append(setTitleOp) - repo := repository.NewMockRepoForTest() - - assert.True(t, bug1.NeedCommit()) + require.True(t, bug1.NeedCommit()) - err := bug1.Commit(repo) - assert.Nil(t, err) - assert.False(t, bug1.NeedCommit()) + err = bug1.Commit(repo) + require.Nil(t, err) + require.False(t, bug1.NeedCommit()) - bug2, err := ReadLocalBug(repo, bug1.Id()) - assert.NoError(t, err) + bug2, err := ReadLocal(repo, bug1.Id()) + require.NoError(t, err) equivalentBug(t, bug1, bug2) // add more op bug1.Append(addCommentOp) - assert.True(t, bug1.NeedCommit()) + require.True(t, bug1.NeedCommit()) err = bug1.Commit(repo) - assert.Nil(t, err) - assert.False(t, bug1.NeedCommit()) + require.Nil(t, err) + require.False(t, bug1.NeedCommit()) - bug3, err := ReadLocalBug(repo, bug1.Id()) - assert.NoError(t, err) + bug3, err := ReadLocal(repo, bug1.Id()) + require.NoError(t, err) equivalentBug(t, bug1, bug3) } func equivalentBug(t *testing.T, expected, actual *Bug) { - assert.Equal(t, len(expected.packs), len(actual.packs)) + require.Equal(t, len(expected.packs), len(actual.packs)) for i := range expected.packs { for j := range expected.packs[i].Operations { @@ -113,7 +121,7 @@ func equivalentBug(t *testing.T, expected, actual *Bug) { } } - assert.Equal(t, expected, actual) + require.Equal(t, expected, actual) } func TestBugRemove(t *testing.T) { @@ -163,13 +171,13 @@ func TestBugRemove(t *testing.T) { err = RemoveBug(repo, b.Id()) require.NoError(t, err) - _, err = ReadLocalBug(repo, b.Id()) + _, err = ReadLocal(repo, b.Id()) require.Error(t, ErrBugNotExist, err) - _, err = ReadRemoteBug(repo, "remoteA", b.Id()) + _, err = ReadRemote(repo, "remoteA", b.Id()) require.Error(t, ErrBugNotExist, err) - _, err = ReadRemoteBug(repo, "remoteB", b.Id()) + _, err = ReadRemote(repo, "remoteB", b.Id()) require.Error(t, ErrBugNotExist, err) ids, err := ListLocalIds(repo) diff --git a/bug/clocks.go b/bug/clocks.go index ba93e0dc..58fce923 100644 --- a/bug/clocks.go +++ b/bug/clocks.go @@ -1,6 +1,7 @@ package bug import ( + "github.com/MichaelMure/git-bug/identity" "github.com/MichaelMure/git-bug/repository" ) @@ -8,7 +9,9 @@ import ( var ClockLoader = repository.ClockLoader{ Clocks: []string{creationClockName, editClockName}, Witnesser: func(repo repository.ClockedRepo) error { - for b := range ReadAllLocalBugs(repo) { + // We don't care about the actual identity so an IdentityStub will do + resolver := identity.NewStubResolver() + for b := range ReadAllLocalWithResolver(repo, resolver) { if b.Err != nil { return b.Err } diff --git a/bug/operation_iterator_test.go b/bug/operation_iterator_test.go index fcc61d0f..5d245185 100644 --- a/bug/operation_iterator_test.go +++ b/bug/operation_iterator_test.go @@ -5,7 +5,7 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/MichaelMure/git-bug/identity" "github.com/MichaelMure/git-bug/repository" @@ -28,6 +28,9 @@ func TestOpIterator(t *testing.T) { mockRepo := repository.NewMockRepoForTest() rene := identity.NewIdentity("René Descartes", "rene@descartes.fr") + err := rene.Commit(mockRepo) + require.NoError(t, err) + unix := time.Now().Unix() createOp := NewCreateOp(rene, unix, "title", "message", nil) @@ -48,15 +51,15 @@ func TestOpIterator(t *testing.T) { bug1.Append(addCommentOp) bug1.Append(setStatusOp) bug1.Append(labelChangeOp) - err := bug1.Commit(mockRepo) - assert.NoError(t, err) + err = bug1.Commit(mockRepo) + require.NoError(t, err) // second pack bug1.Append(genTitleOp()) bug1.Append(genTitleOp()) bug1.Append(genTitleOp()) err = bug1.Commit(mockRepo) - assert.NoError(t, err) + require.NoError(t, err) // staging bug1.Append(genTitleOp()) @@ -71,5 +74,5 @@ func TestOpIterator(t *testing.T) { counter++ } - assert.Equal(t, 10, counter) + require.Equal(t, 10, counter) } diff --git a/bug/operation_pack.go b/bug/operation_pack.go index 58c8381c..6a134b94 100644 --- a/bug/operation_pack.go +++ b/bug/operation_pack.go @@ -143,10 +143,11 @@ func (opp *OperationPack) Write(repo repository.ClockedRepo) (repository.Hash, e } // First, make sure that all the identities are properly Commit as well + // TODO: this might be downgraded to "make sure it exist in git" but then, what make + // sure no data is lost on identities ? for _, op := range opp.Operations { - err := op.base().Author.CommitAsNeeded(repo) - if err != nil { - return "", err + if op.base().Author.NeedCommit() { + return "", fmt.Errorf("identity need commmit") } } diff --git a/bug/operation_test.go b/bug/operation_test.go index 21ae5eff..64bff41f 100644 --- a/bug/operation_test.go +++ b/bug/operation_test.go @@ -106,7 +106,7 @@ func TestID(t *testing.T) { require.Equal(t, id1, id2) - b2, err := ReadLocalBug(repo, b.Id()) + b2, err := ReadLocal(repo, b.Id()) require.Nil(t, err) op3 := b2.FirstOp() -- cgit